From e72da7c5a742fa9b5cfe714b33ff415a1c002860 Mon Sep 17 00:00:00 2001 From: Trey t Date: Sun, 18 Jan 2026 22:46:40 -0600 Subject: [PATCH] fix(itinerary): add city to game items for proper constraint validation Travel constraint validation was not working because ItineraryConstraints had no game items to validate against - games came from RichGame objects but were never converted to ItineraryItem for constraint checking. Changes: - Add city parameter to ItemKind.game enum case - Create game ItineraryItems from RichGame data in buildItineraryData() - Update isValidTravelPosition to compare against actual game sortOrders - Fix tests to use appropriate game sortOrder conventions Now travel is properly constrained to appear before arrival city games and after departure city games. Co-Authored-By: Claude Opus 4.5 --- .../Models/Domain/ItineraryConstraints.swift | 37 +-- .../Core/Models/Domain/ItineraryItem.swift | 11 +- .../Core/Services/ItineraryItemService.swift | 8 +- SportsTime/Export/PDFGenerator.swift | 10 +- .../Trip/Views/ItineraryReorderingLogic.swift | 291 ++++++++---------- .../Views/ItineraryTableViewWrapper.swift | 49 +-- .../Features/Trip/Views/TripDetailView.swift | 12 +- .../Trip/ItineraryReorderingLogicTests.swift | 17 +- .../Features/Trip/ItineraryTestHelpers.swift | 2 +- .../Trip/ItineraryTravelConstraintTests.swift | 29 +- .../ItineraryConstraintsTests.swift | 35 ++- 11 files changed, 247 insertions(+), 254 deletions(-) diff --git a/SportsTime/Core/Models/Domain/ItineraryConstraints.swift b/SportsTime/Core/Models/Domain/ItineraryConstraints.swift index 98a48eb..7f112eb 100644 --- a/SportsTime/Core/Models/Domain/ItineraryConstraints.swift +++ b/SportsTime/Core/Models/Domain/ItineraryConstraints.swift @@ -5,13 +5,6 @@ struct ItineraryConstraints { let tripDayCount: Int private let items: [ItineraryItem] - /// City extracted from game ID (format: "game-CityName-xxxx") - private func city(forGameId gameId: String) -> String? { - let components = gameId.components(separatedBy: "-") - guard components.count >= 2 else { return nil } - return components[1] - } - init(tripDayCount: Int, items: [ItineraryItem]) { self.tripDayCount = tripDayCount self.items = items @@ -83,8 +76,10 @@ struct ItineraryConstraints { // MARK: - Private Helpers private func isValidTravelPosition(fromCity: String, toCity: String, day: Int, sortOrder: Double) -> Bool { - let departureGameDays = gameDays(in: fromCity) - let arrivalGameDays = gameDays(in: toCity) + let departureGames = games(in: fromCity) + let arrivalGames = games(in: toCity) + let departureGameDays = departureGames.map { $0.day } + let arrivalGameDays = arrivalGames.map { $0.day } let minDay = departureGameDays.max() ?? 1 let maxDay = arrivalGameDays.min() ?? tripDayCount @@ -92,26 +87,20 @@ struct ItineraryConstraints { // Check day is in valid range guard day >= minDay && day <= maxDay else { return false } - // Check sortOrder constraints on edge days + // Check sortOrder constraints on edge days. + // Must be strictly AFTER all departure games on that day if departureGameDays.contains(day) { - // On a departure game day: must be after ALL games in that city on that day - let maxGameSortOrder = games(in: fromCity) - .filter { $0.day == day } - .map { $0.sortOrder } - .max() ?? 0 - + let gamesOnDay = departureGames.filter { $0.day == day } + let maxGameSortOrder = gamesOnDay.map { $0.sortOrder }.max() ?? 0 if sortOrder <= maxGameSortOrder { return false } } + // Must be strictly BEFORE all arrival games on that day if arrivalGameDays.contains(day) { - // On an arrival game day: must be before ALL games in that city on that day - let minGameSortOrder = games(in: toCity) - .filter { $0.day == day } - .map { $0.sortOrder } - .min() ?? Double.greatestFiniteMagnitude - + let gamesOnDay = arrivalGames.filter { $0.day == day } + let minGameSortOrder = gamesOnDay.map { $0.sortOrder }.min() ?? 0 if sortOrder >= minGameSortOrder { return false } @@ -126,8 +115,8 @@ struct ItineraryConstraints { private func games(in city: String) -> [ItineraryItem] { return items.filter { item in - guard case .game(let gameId) = item.kind else { return false } - return self.city(forGameId: gameId) == city + guard let gameCity = item.gameCity else { return false } + return gameCity.lowercased() == city.lowercased() } } } diff --git a/SportsTime/Core/Models/Domain/ItineraryItem.swift b/SportsTime/Core/Models/Domain/ItineraryItem.swift index bbf003b..28d4676 100644 --- a/SportsTime/Core/Models/Domain/ItineraryItem.swift +++ b/SportsTime/Core/Models/Domain/ItineraryItem.swift @@ -29,7 +29,7 @@ struct ItineraryItem: Identifiable, Codable, Hashable { /// The type of itinerary item enum ItemKind: Codable, Hashable { - case game(gameId: String) + case game(gameId: String, city: String) case travel(TravelInfo) case custom(CustomInfo) } @@ -106,14 +106,19 @@ extension ItineraryItem { } var gameId: String? { - if case .game(let id) = kind { return id } + if case .game(let id, _) = kind { return id } + return nil + } + + var gameCity: String? { + if case .game(_, let city) = kind { return city } return nil } /// Display title for the item var displayTitle: String { switch kind { - case .game(let gameId): + case .game(let gameId, _): return "Game: \(gameId)" case .travel(let info): return "\(info.fromCity) → \(info.toCity)" diff --git a/SportsTime/Core/Services/ItineraryItemService.swift b/SportsTime/Core/Services/ItineraryItemService.swift index befc79a..5f5a6b6 100644 --- a/SportsTime/Core/Services/ItineraryItemService.swift +++ b/SportsTime/Core/Services/ItineraryItemService.swift @@ -145,8 +145,9 @@ extension ItineraryItem { // Parse kind switch kindString { case "game": - guard let gameId = record["gameId"] as? String else { return nil } - self.kind = .game(gameId: gameId) + guard let gameId = record["gameId"] as? String, + let gameCity = record["gameCity"] as? String else { return nil } + self.kind = .game(gameId: gameId, city: gameCity) case "travel": guard let fromCity = record["travelFromCity"] as? String, @@ -188,9 +189,10 @@ extension ItineraryItem { record["modifiedAt"] = modifiedAt switch kind { - case .game(let gameId): + case .game(let gameId, let city): record["kind"] = "game" record["gameId"] = gameId + record["gameCity"] = city case .travel(let info): record["kind"] = "travel" diff --git a/SportsTime/Export/PDFGenerator.swift b/SportsTime/Export/PDFGenerator.swift index 3ed1661..08ba788 100644 --- a/SportsTime/Export/PDFGenerator.swift +++ b/SportsTime/Export/PDFGenerator.swift @@ -388,11 +388,9 @@ final class PDFGenerator { var primaryCity: String? for item in items { switch item.kind { - case .game(let gameId): - if let richGame = games[gameId] { - primaryCity = richGame.stadium.city - break - } + case .game(_, let city): + primaryCity = city + break case .travel(let info): primaryCity = info.toCity break @@ -419,7 +417,7 @@ final class PDFGenerator { var hasContent = false for item in items { switch item.kind { - case .game(let gameId): + case .game(let gameId, _): if let richGame = games[gameId] { currentY = drawGameCard( context: context, diff --git a/SportsTime/Features/Trip/Views/ItineraryReorderingLogic.swift b/SportsTime/Features/Trip/Views/ItineraryReorderingLogic.swift index a271daa..7ec009b 100644 --- a/SportsTime/Features/Trip/Views/ItineraryReorderingLogic.swift +++ b/SportsTime/Features/Trip/Views/ItineraryReorderingLogic.swift @@ -5,9 +5,6 @@ // Pure functions for itinerary reordering logic. // Extracted from ItineraryTableViewController for testability. // -// All functions in this enum are pure - they take inputs and return outputs -// with no side effects, making them fully unit-testable without UIKit. -// // SEMANTIC TRAVEL MODEL: // - Travel items are positioned semantically via (day, sortOrder), not structurally. // - Travel can appear before games (sortOrder < 0) or after games (sortOrder >= 0). @@ -15,40 +12,13 @@ // - All movable items (custom + travel) use the same day computation: backward scan to nearest dayHeader. // // COORDINATE SPACE CONVENTIONS: -// -// Two coordinate spaces exist during drag-drop operations: -// -// 1. ORIGINAL SPACE (flatItems indices) -// - Row indices in the current flatItems array: 0..= sourceRow: return proposed + 1 (shift up past removed source) -// • If proposed < sourceRow: return proposed (unchanged) -// - originalToProposed(original, sourceRow): Converts original → proposed -// • If original == sourceRow: return nil (source has no proposed equivalent) -// • If original > sourceRow: return original - 1 (shift down) -// • If original < sourceRow: return original (unchanged) -// -// WHY THIS MATTERS: -// - DragZones are used for UI highlighting (which cells to dim/enable) -// - UI highlighting operates on the visible table, which uses ORIGINAL indices -// - But validation uses simulation, which operates in PROPOSED space -// - Getting this wrong causes visual bugs (wrong rows highlighted) or logic bugs +// - "Original indices": Row indices in the current flatItems array (0.. Double? ) -> [ItineraryRowItem] { var flatItems: [ItineraryRowItem] = [] - + for day in days { // NOTE: day.travelBefore is IGNORED under semantic travel model. // Travel must be in day.items with a sortOrder to appear. - + // 1. Day header (structural anchor) flatItems.append(.dayHeader(dayNumber: day.dayNumber, date: day.date)) - + // 2. Partition movable items around games boundary // Tuple includes tiebreaker for stable sorting when sortOrders are equal var beforeGames: [(sortOrder: Double, tiebreaker: Int, item: ItineraryRowItem)] = [] var afterGames: [(sortOrder: Double, tiebreaker: Int, item: ItineraryRowItem)] = [] var insertionOrder = 0 - + for row in day.items { let sortOrder: Double let tiebreaker = insertionOrder insertionOrder += 1 - + switch row { case .customItem(let item): sortOrder = item.sortOrder - + case .travel(let segment, _): - if let so = findTravelSortOrder(segment) { - sortOrder = so - } else { - // Travel without stored sortOrder gets a safe default. - // Log a warning in debug builds - this shouldn't happen in production. - #if DEBUG - print("⚠️ flattenDays: Travel segment missing sortOrder: \(segment.fromLocation.name) → \(segment.toLocation.name). Using default: \(defaultTravelSortOrder)") - #endif - sortOrder = defaultTravelSortOrder - } - + // Use provided sortOrder if available, otherwise default to after-games position. + // nil is valid during initial display before travel is persisted. + let lookedUp = findTravelSortOrder(segment) + sortOrder = lookedUp ?? defaultTravelSortOrder + print("📋 [flattenDays] Travel \(segment.fromLocation.name)->\(segment.toLocation.name) on day \(day.dayNumber): lookedUp=\(String(describing: lookedUp)), using sortOrder=\(sortOrder)") + case .games, .dayHeader: // These item types are not movable and handled separately. - // Skip explicitly - games are added after partitioning. continue } - + if sortOrder < 0 { beforeGames.append((sortOrder, tiebreaker, row)) } else { afterGames.append((sortOrder, tiebreaker, row)) } } - - // Sort by sortOrder within each region, with stable tiebreaker + beforeGames.sort { ($0.sortOrder, $0.tiebreaker) < ($1.sortOrder, $1.tiebreaker) } afterGames.sort { ($0.sortOrder, $0.tiebreaker) < ($1.sortOrder, $1.tiebreaker) } - + flatItems.append(contentsOf: beforeGames.map { $0.item }) - + // 3. Games for this day (bundled as one row) if !day.games.isEmpty { flatItems.append(.games(day.games, dayNumber: day.dayNumber)) } - + // 4. Items after games flatItems.append(contentsOf: afterGames.map { $0.item }) } - + return flatItems } - + + // MARK: - Day Number Lookup - + /// Finds which day a row at the given index belongs to. /// /// Scans backwards from the row to find a `.dayHeader`. @@ -172,7 +136,7 @@ enum ItineraryReorderingLogic { } return 1 } - + /// Finds the row index of the day header for a specific day number. /// /// - Parameters: @@ -187,7 +151,7 @@ enum ItineraryReorderingLogic { } return nil } - + /// Finds the row index of the travel segment on a specific day. /// /// **SEMANTIC MODEL**: Does NOT use the embedded dayNumber in .travel(). @@ -199,16 +163,13 @@ enum ItineraryReorderingLogic { /// - day: The day number to find /// - Returns: The row index, or nil if no travel on that day static func travelRow(in items: [ItineraryRowItem], forDay day: Int) -> Int? { - // Find the day header row guard let headerRow = dayHeaderRow(in: items, forDay: day) else { return nil } - - // Scan forward until next day header, looking for travel + for i in (headerRow + 1).. Int? { @@ -229,7 +191,7 @@ enum ItineraryReorderingLogic { } return nil } - + /// Determines which day a travel segment belongs to at a given row position. /// /// **SEMANTIC MODEL**: Uses backward scan to find the nearest preceding dayHeader. @@ -240,20 +202,19 @@ enum ItineraryReorderingLogic { /// - items: The flat array of row items /// - Returns: The day number the travel belongs to static func dayForTravelAt(row: Int, in items: [ItineraryRowItem]) -> Int { - // Semantic model: scan backward to find the day this item belongs to - // (same logic as dayNumber) return dayNumber(in: items, forRow: row) } - + + // MARK: - Move Simulation - + /// Result of simulating a move operation. struct SimulatedMove { let items: [ItineraryRowItem] let destinationRowInNewArray: Int let didMove: Bool // false if move was invalid/no-op } - + /// Simulates UITableView move semantics with bounds safety. /// /// UITableView moves work as: remove at sourceRow from ORIGINAL array, @@ -269,20 +230,20 @@ enum ItineraryReorderingLogic { sourceRow: Int, destinationProposedRow: Int ) -> SimulatedMove { - // Bounds safety: return original unchanged if sourceRow is invalid guard sourceRow >= 0 && sourceRow < original.count else { return SimulatedMove(items: original, destinationRowInNewArray: sourceRow, didMove: false) } - + var items = original let moving = items.remove(at: sourceRow) let clampedDest = min(max(0, destinationProposedRow), items.count) items.insert(moving, at: clampedDest) return SimulatedMove(items: items, destinationRowInNewArray: clampedDest, didMove: true) } - + + // MARK: - Coordinate Space Conversion - + /// Converts a proposed destination index to the equivalent original index. /// /// UITableView move semantics: remove at sourceRow first, then insert at proposed position. @@ -299,7 +260,7 @@ enum ItineraryReorderingLogic { return proposed } } - + /// Converts an original index to the equivalent proposed destination index. /// /// - Parameters: @@ -316,9 +277,9 @@ enum ItineraryReorderingLogic { return original } } - + // MARK: - Sort Order Calculation - + /// Calculates the sortOrder for an item dropped at the given row position. /// /// Uses **midpoint insertion** algorithm to avoid renumbering existing items: @@ -357,16 +318,27 @@ enum ItineraryReorderingLogic { } } + // DEBUG: Log the row positions + print("🔢 [calculateSortOrder] row=\(row), day=\(day), gamesRow=\(String(describing: gamesRow))") + print("🔢 [calculateSortOrder] items around row:") + for i in max(0, row - 2)...min(items.count - 1, row + 2) { + let marker = i == row ? "→" : " " + let gMarker = (gamesRow == i) ? " [GAMES]" : "" + print("🔢 \(marker) [\(i)] \(items[i])\(gMarker)") + } + // Strict region classification: // - row < gamesRow => before-games (negative sortOrder) // - row >= gamesRow OR no games => after-games (positive sortOrder) let isBeforeGames: Bool if let gr = gamesRow { isBeforeGames = row < gr + print("🔢 [calculateSortOrder] row(\(row)) < gamesRow(\(gr)) = \(isBeforeGames) → isBeforeGames=\(isBeforeGames)") } else { isBeforeGames = false // No games means everything is "after games" + print("🔢 [calculateSortOrder] No games on day \(day) → isBeforeGames=false") } - + /// Get sortOrder from a movable item (custom item or travel) func movableSortOrder(_ idx: Int) -> Double? { guard idx >= 0 && idx < items.count else { return nil } @@ -379,7 +351,7 @@ enum ItineraryReorderingLogic { return nil } } - + /// Scan backward from start, stopping at boundaries, looking for movable items in the same region func scanBackward(from start: Int) -> Double? { var i = start @@ -391,7 +363,7 @@ enum ItineraryReorderingLogic { } // Stop at games boundary (don't cross into other region) if case .games(_, let d) = items[i], d == day { break } - + if let v = movableSortOrder(i) { // Only return values in the correct region if isBeforeGames { @@ -404,7 +376,7 @@ enum ItineraryReorderingLogic { } return nil } - + /// Scan forward from start, stopping at boundaries, looking for movable items in the same region func scanForward(from start: Int) -> Double? { var i = start @@ -416,7 +388,7 @@ enum ItineraryReorderingLogic { } // Stop at games boundary (don't cross into other region) if case .games(_, let d) = items[i], d == day { break } - + if let v = movableSortOrder(i) { // Only return values in the correct region if isBeforeGames { @@ -429,7 +401,8 @@ enum ItineraryReorderingLogic { } return nil } - + + let result: Double if isBeforeGames { // Above games: sortOrder should be negative let prev = scanBackward(from: row - 1) @@ -438,9 +411,9 @@ enum ItineraryReorderingLogic { let upperBound: Double = 0.0 // Games boundary switch (prev, next) { case (nil, nil): - return -1.0 + result = -1.0 case (let p?, nil): - return (p + upperBound) / 2.0 + result = (p + upperBound) / 2.0 case (nil, let n?): // First item before games: place it before the next item. // n should always be negative (scanForward filters for region). @@ -448,12 +421,13 @@ enum ItineraryReorderingLogic { // This shouldn't happen - scanForward should only return negative values // in before-games region. Return safe default and assert in debug. assertionFailure("Before-games region has non-negative sortOrder: \(n)") - return -1.0 + result = -1.0 + } else { + // Place before n by subtracting 1.0 (simpler and more consistent than min(n/2, n-1)) + result = n - 1.0 } - // Place before n by subtracting 1.0 (simpler and more consistent than min(n/2, n-1)) - return n - 1.0 case (let p?, let n?): - return (p + n) / 2.0 + result = (p + n) / 2.0 } } else { // Below games: sortOrder should be >= 0 @@ -462,15 +436,18 @@ enum ItineraryReorderingLogic { switch next { case nil: - return (prev == 0.0) ? 1.0 : (prev + 1.0) + result = (prev == 0.0) ? 1.0 : (prev + 1.0) case let n?: - return (prev + n) / 2.0 + result = (prev + n) / 2.0 } } + + print("🔢 [calculateSortOrder] RESULT: \(result) (isBeforeGames=\(isBeforeGames))") + return result } - + // MARK: - Valid Drop Computation - + /// Computes all valid destination rows in **proposed** coordinate space. /// /// For BOTH travel and custom items, we: @@ -503,7 +480,7 @@ enum ItineraryReorderingLogic { ) -> [Int] { let maxProposed = max(0, flatItems.count - 1) guard maxProposed > 0 else { return [] } - + switch dragged { case .customItem(let customItem): // Custom items use the same simulation+validation approach as travel @@ -519,24 +496,24 @@ enum ItineraryReorderingLogic { return true } } - + var valid: [Int] = [] valid.reserveCapacity(maxProposed) - + for proposedRow in 1...maxProposed { let simulated = simulateMove(original: flatItems, sourceRow: sourceRow, destinationProposedRow: proposedRow) guard simulated.didMove else { continue } - + let destRowInSim = simulated.destinationRowInNewArray - + // Don't allow dropping ON a day header if case .dayHeader = simulated.items[destRowInSim] { continue } - + let day = dayNumber(in: simulated.items, forRow: destRowInSim) let sortOrder = calculateSortOrder(in: simulated.items, at: destRowInSim, findTravelSortOrder: findTravelSortOrder) - + // Create a temporary item model with the computed position let testItem = ItineraryItem( id: customItem.id, @@ -545,21 +522,21 @@ enum ItineraryReorderingLogic { sortOrder: sortOrder, kind: customItem.kind ) - + if constraints.isValidPosition(for: testItem, day: day, sortOrder: sortOrder) { valid.append(proposedRow) } } - + return valid - + case .travel(let segment, _): let travelId = "travel:\(segment.fromLocation.name.lowercased())->\(segment.toLocation.name.lowercased())" let validDayRange = travelValidRanges[travelId] - + // Use existing model if available, otherwise create a default let model = findTravelItem(segment) ?? makeTravelItem(segment) - + guard let constraints = constraints else { // No constraint engine, allow all rows except 0 and day headers return (1...maxProposed).filter { proposedRow in @@ -571,31 +548,31 @@ enum ItineraryReorderingLogic { return true } } - + var valid: [Int] = [] valid.reserveCapacity(maxProposed) - + for proposedRow in 1...maxProposed { let simulated = simulateMove(original: flatItems, sourceRow: sourceRow, destinationProposedRow: proposedRow) guard simulated.didMove else { continue } - + let destRowInSim = simulated.destinationRowInNewArray - + // Don't allow dropping ON a day header if case .dayHeader = simulated.items[destRowInSim] { continue } - + let day = dayNumber(in: simulated.items, forRow: destRowInSim) - + // Check day range constraint (quick rejection) if let range = validDayRange, !range.contains(day) { continue } - + // Check sortOrder constraint let sortOrder = calculateSortOrder(in: simulated.items, at: destRowInSim, findTravelSortOrder: findTravelSortOrder) - + // Create a testItem with computed day/sortOrder (like custom items do) // This ensures constraints.isValidPosition sees the actual proposed position let testItem = ItineraryItem( @@ -605,22 +582,22 @@ enum ItineraryReorderingLogic { sortOrder: sortOrder, kind: model.kind ) - + if constraints.isValidPosition(for: testItem, day: day, sortOrder: sortOrder) { valid.append(proposedRow) } } - + return valid - + default: // Day headers and games can't be moved return [] } } - + // MARK: - Drag Zones - + /// Result of calculating drag zones for visual feedback. /// /// **COORDINATE SPACE**: All indices are in ORIGINAL coordinate space (current flatItems indices). @@ -633,7 +610,7 @@ enum ItineraryReorderingLogic { /// Game IDs that act as barriers for this drag let barrierGameIds: Set } - + /// Calculates drag zones for a travel segment using simulation+validation. /// /// This ensures UI feedback matches what will actually be accepted on drop. @@ -670,11 +647,11 @@ enum ItineraryReorderingLogic { makeTravelItem: makeTravelItem, findTravelSortOrder: findTravelSortOrder ) - + // Convert valid rows from proposed to original coordinate space let validRowsOriginal = validRowsProposed.map { proposedToOriginal($0, sourceRow: sourceRow) } let validSet = Set(validRowsOriginal) - + // Compute invalid rows in original coordinate space var invalidRows = Set() for i in 0..() if let travelItem = findTravelItem(segment), @@ -694,14 +671,14 @@ enum ItineraryReorderingLogic { let barriers = constraints.barrierGames(for: travelItem) barrierGameIds = Set(barriers.compactMap { $0.gameId }) } - + return DragZones( invalidRowIndices: invalidRows, validDropRows: validRowsOriginal, barrierGameIds: barrierGameIds ) } - + /// Calculates drag zones for a custom item using simulation+validation. /// /// This ensures UI feedback matches what will actually be accepted on drop. @@ -735,11 +712,11 @@ enum ItineraryReorderingLogic { }, findTravelSortOrder: findTravelSortOrder ) - + // Convert valid rows from proposed to original coordinate space let validRowsOriginal = validRowsProposed.map { proposedToOriginal($0, sourceRow: sourceRow) } let validSet = Set(validRowsOriginal) - + // Compute invalid rows in original coordinate space var invalidRows = Set() for i in 0.. ItineraryItem? ) -> DragZones { let travelId = "travel:\(segment.fromLocation.name.lowercased())->\(segment.toLocation.name.lowercased())" - + guard let validRange = travelValidRanges[travelId] else { return DragZones(invalidRowIndices: [], validDropRows: [], barrierGameIds: []) } - + var invalidRows = Set() var validRows: [Int] = [] - + for (index, rowItem) in flatItems.enumerated() { let dayNum: Int switch rowItem { @@ -794,14 +771,14 @@ enum ItineraryReorderingLogic { case .customItem(let item): dayNum = item.day } - + if validRange.contains(dayNum) { validRows.append(index) } else { invalidRows.insert(index) } } - + // Find barrier games using constraints var barrierGameIds = Set() if let travelItem = findTravelItem(segment), @@ -809,14 +786,14 @@ enum ItineraryReorderingLogic { let barriers = constraints.barrierGames(for: travelItem) barrierGameIds = Set(barriers.compactMap { $0.gameId }) } - + return DragZones( invalidRowIndices: invalidRows, validDropRows: validRows, barrierGameIds: barrierGameIds ) } - + /// Legacy version of calculateCustomItemDragZones that doesn't require sourceRow. /// /// - Note: Prefer the version with sourceRow for accurate validation. @@ -827,7 +804,7 @@ enum ItineraryReorderingLogic { ) -> DragZones { var invalidRows = Set() var validRows: [Int] = [] - + for (index, rowItem) in flatItems.enumerated() { if case .dayHeader = rowItem { invalidRows.insert(index) @@ -835,16 +812,16 @@ enum ItineraryReorderingLogic { validRows.append(index) } } - + return DragZones( invalidRowIndices: invalidRows, validDropRows: validRows, barrierGameIds: [] ) } - + // MARK: - Utility Functions - + /// Finds the nearest value in a sorted array using binary search. /// /// - Parameters: @@ -853,10 +830,10 @@ enum ItineraryReorderingLogic { /// - Returns: The nearest value, or nil if array is empty static func nearestValue(in sorted: [Int], to target: Int) -> Int? { guard !sorted.isEmpty else { return nil } - + var low = 0 var high = sorted.count - + // Binary search for insertion point while low < high { let mid = (low + high) / 2 @@ -866,10 +843,10 @@ enum ItineraryReorderingLogic { high = mid } } - + let after = (low < sorted.count) ? sorted[low] : nil let before = (low > 0) ? sorted[low - 1] : nil - + switch (before, after) { case let (b?, a?): // Both exist, return the closer one @@ -882,7 +859,7 @@ enum ItineraryReorderingLogic { return nil } } - + /// Calculates target destination with constraint snapping. /// /// If the proposed row is valid, returns it. Otherwise, snaps to nearest valid row. @@ -909,12 +886,12 @@ enum ItineraryReorderingLogic { // UX rule: forbid dropping at absolute top (row 0 is always a day header) var row = proposedRow if row <= 0 { row = 1 } - + // If already valid, use it if validDestinationRows.contains(row) { return row } - + // Snap to nearest valid destination (validDestinationRows must be sorted for binary search) return nearestValue(in: validDestinationRows, to: row) ?? sourceRow } diff --git a/SportsTime/Features/Trip/Views/ItineraryTableViewWrapper.swift b/SportsTime/Features/Trip/Views/ItineraryTableViewWrapper.swift index ae8791d..0037e92 100644 --- a/SportsTime/Features/Trip/Views/ItineraryTableViewWrapper.swift +++ b/SportsTime/Features/Trip/Views/ItineraryTableViewWrapper.swift @@ -80,14 +80,14 @@ struct ItineraryTableViewWrapper: UIViewControllerRepresent hostingController.view.translatesAutoresizingMaskIntoConstraints = true controller.setTableHeader(hostingController.view) - + // Load initial data let (days, validRanges, allItemsForConstraints) = buildItineraryData() controller.reloadData(days: days, travelValidRanges: validRanges, itineraryItems: allItemsForConstraints) - + return controller } - + func updateUIViewController(_ controller: ItineraryTableViewController, context: Context) { controller.colorScheme = colorScheme controller.onTravelMoved = onTravelMoved @@ -95,37 +95,46 @@ struct ItineraryTableViewWrapper: UIViewControllerRepresent controller.onCustomItemTapped = onCustomItemTapped controller.onCustomItemDeleted = onCustomItemDeleted controller.onAddButtonTapped = onAddButtonTapped - + // Update header content by updating the hosting controller's rootView // This avoids recreating the view hierarchy and prevents infinite loops context.coordinator.headerHostingController?.rootView = headerContent - + let (days, validRanges, allItemsForConstraints) = buildItineraryData() controller.reloadData(days: days, travelValidRanges: validRanges, itineraryItems: allItemsForConstraints) } - + // MARK: - Build Itinerary Data - + private func buildItineraryData() -> ([ItineraryDayData], [String: ClosedRange], [ItineraryItem]) { let tripDays = calculateTripDays() var travelValidRanges: [String: ClosedRange] = [:] - + + // Build game items from RichGame data for constraint validation + var gameItems: [ItineraryItem] = [] + for (index, dayDate) in tripDays.enumerated() { + let dayNum = index + 1 + let gamesOnDay = gamesOn(date: dayDate) + for (gameIndex, richGame) in gamesOnDay.enumerated() { + let gameItem = ItineraryItem( + tripId: trip.id, + day: dayNum, + sortOrder: Double(gameIndex) * 0.01, // Games have sortOrder ~0 (at the visual boundary) + kind: .game(gameId: richGame.game.id, city: richGame.stadium.city) + ) + gameItems.append(gameItem) + } + } + // Build travel as semantic items with (day, sortOrder) var travelItems: [ItineraryItem] = [] travelItems.reserveCapacity(trip.travelSegments.count) - - func cityFromGameId(_ gameId: String) -> String? { - let comps = gameId.components(separatedBy: "-") - guard comps.count >= 2 else { return nil } - return comps[1] - } - + func gamesIn(city: String, day: Int) -> [ItineraryItem] { - itineraryItems.filter { item in + gameItems.filter { item in guard item.day == day else { return false } - guard case .game(let gid) = item.kind else { return false } - guard let c = cityFromGameId(gid) else { return false } - return cityMatches(c, searchCity: city) + guard let gameCity = item.gameCity else { return false } + return cityMatches(gameCity, searchCity: city) } } @@ -249,7 +258,7 @@ struct ItineraryTableViewWrapper: UIViewControllerRepresent days.append(dayData) } - return (days, travelValidRanges, itineraryItems + travelItems) + return (days, travelValidRanges, gameItems + itineraryItems + travelItems) } // MARK: - Helper Methods diff --git a/SportsTime/Features/Trip/Views/TripDetailView.swift b/SportsTime/Features/Trip/Views/TripDetailView.swift index 9b1c825..0d1e82a 100644 --- a/SportsTime/Features/Trip/Views/TripDetailView.swift +++ b/SportsTime/Features/Trip/Views/TripDetailView.swift @@ -203,7 +203,7 @@ struct TripDetailView: View { withAnimation { travelOverrides[travelId] = TravelOverride(day: newDay, sortOrder: newSortOrder) } - await saveTravelDayOverride(travelAnchorId: travelId, displayDay: newDay) + await saveTravelDayOverride(travelAnchorId: travelId, displayDay: newDay, sortOrder: newSortOrder) } }, onCustomItemMoved: { itemId, day, sortOrder in @@ -1371,7 +1371,8 @@ struct TripDetailView: View { // Persist to CloudKit as a travel ItineraryItem await self.saveTravelDayOverride( travelAnchorId: droppedId, - displayDay: dayNumber + displayDay: dayNumber, + sortOrder: newSortOrder ) return } @@ -1393,8 +1394,8 @@ struct TripDetailView: View { return false } - private func saveTravelDayOverride(travelAnchorId: String, displayDay: Int) async { - print("💾 [TravelOverrides] Saving override: \(travelAnchorId) -> day \(displayDay)") + private func saveTravelDayOverride(travelAnchorId: String, displayDay: Int, sortOrder: Double) async { + print("💾 [TravelOverrides] Saving override: \(travelAnchorId) -> day \(displayDay), sortOrder \(sortOrder)") // Parse travel ID to extract cities (format: "travel:city1->city2") let stripped = travelAnchorId.replacingOccurrences(of: "travel:", with: "") @@ -1414,6 +1415,7 @@ struct TripDetailView: View { // Update existing var updated = itineraryItems[existingIndex] updated.day = displayDay + updated.sortOrder = sortOrder updated.modifiedAt = Date() itineraryItems[existingIndex] = updated await ItineraryItemService.shared.updateItem(updated) @@ -1423,7 +1425,7 @@ struct TripDetailView: View { let item = ItineraryItem( tripId: trip.id, day: displayDay, - sortOrder: 0, // Travel always comes first in day + sortOrder: sortOrder, kind: .travel(travelInfo) ) itineraryItems.append(item) diff --git a/SportsTimeTests/Features/Trip/ItineraryReorderingLogicTests.swift b/SportsTimeTests/Features/Trip/ItineraryReorderingLogicTests.swift index 0588fe7..faa0184 100644 --- a/SportsTimeTests/Features/Trip/ItineraryReorderingLogicTests.swift +++ b/SportsTimeTests/Features/Trip/ItineraryReorderingLogicTests.swift @@ -243,24 +243,27 @@ final class ItineraryReorderingLogicTests: XCTestCase { // MARK: - travelRow Tests func test_travelRow_findsCorrectRow() { + // Semantic model: travelRow finds travel in the section AFTER the day header + // Travel must be positioned within its correct day section let items = buildFlatItems([ .day(1), .game("Detroit", day: 1), - .travel(from: "Detroit", to: "Chicago", day: 2), .day(2), - .travel(from: "Chicago", to: "Milwaukee", day: 3), - .day(3) + .travel(from: "Detroit", to: "Chicago", day: 2), // Row 3: in day 2 section + .day(3), + .travel(from: "Chicago", to: "Milwaukee", day: 3) // Row 5: in day 3 section ]) - XCTAssertEqual(Logic.travelRow(in: items, forDay: 2), 2) - XCTAssertEqual(Logic.travelRow(in: items, forDay: 3), 4) + XCTAssertEqual(Logic.travelRow(in: items, forDay: 2), 3) + XCTAssertEqual(Logic.travelRow(in: items, forDay: 3), 5) } func test_travelRow_noTravelOnDay_returnsNil() { + // Travel is in day 2 section, so day 1 has no travel let items = buildFlatItems([ .day(1), - .travel(from: "Detroit", to: "Chicago", day: 2), - .day(2) + .day(2), + .travel(from: "Detroit", to: "Chicago", day: 2) // In day 2 section ]) XCTAssertNil(Logic.travelRow(in: items, forDay: 1)) diff --git a/SportsTimeTests/Features/Trip/ItineraryTestHelpers.swift b/SportsTimeTests/Features/Trip/ItineraryTestHelpers.swift index 84389a8..9a0c9f8 100644 --- a/SportsTimeTests/Features/Trip/ItineraryTestHelpers.swift +++ b/SportsTimeTests/Features/Trip/ItineraryTestHelpers.swift @@ -108,7 +108,7 @@ enum ItineraryTestHelpers { tripId: testTripId, day: day, sortOrder: sortOrder, - kind: .game(gameId: "game-\(city)-\(UUID().uuidString.prefix(4))") + kind: .game(gameId: "game-\(city)-\(UUID().uuidString.prefix(4))", city: city) ) } diff --git a/SportsTimeTests/Features/Trip/ItineraryTravelConstraintTests.swift b/SportsTimeTests/Features/Trip/ItineraryTravelConstraintTests.swift index f228900..2516ffd 100644 --- a/SportsTimeTests/Features/Trip/ItineraryTravelConstraintTests.swift +++ b/SportsTimeTests/Features/Trip/ItineraryTravelConstraintTests.swift @@ -194,11 +194,15 @@ final class ItineraryTravelConstraintTests: XCTestCase { // MARK: - Travel Movement Tests func test_travel_moveToValidDay_callsCallback() { - // Given: Travel with valid range 2-4 + // Given: Travel with valid range 2-3 let travel = H.makeTravelSegment(from: "Chicago", to: "Detroit") + let travelItem = ItineraryRowItem.travel(travel, dayNumber: 2) + + // Travel model item for sortOrder lookup + let travelModelItem = H.makeTravelItem(from: "Chicago", to: "Detroit", day: 2, sortOrder: 1.0) let day1 = ItineraryDayData(id: 1, dayNumber: 1, date: testDate, games: [], items: [], travelBefore: nil) - let day2 = ItineraryDayData(id: 2, dayNumber: 2, date: H.dayAfter(testDate), games: [], items: [], travelBefore: travel) + let day2 = ItineraryDayData(id: 2, dayNumber: 2, date: H.dayAfter(testDate), games: [], items: [travelItem], travelBefore: nil) let day3 = ItineraryDayData(id: 3, dayNumber: 3, date: H.dayAfter(H.dayAfter(testDate)), games: [], items: [], travelBefore: nil) var capturedTravelId: String = "" @@ -211,12 +215,12 @@ final class ItineraryTravelConstraintTests: XCTestCase { controller.reloadData( days: [day1, day2, day3], travelValidRanges: ["travel:chicago->detroit": 2...3], - itineraryItems: [] + itineraryItems: [travelModelItem] ) - // Rows: 0=Day1 header, 1=travel, 2=Day2 header, 3=Day3 header - // Move travel (row 1) to row 3 (after Day2, before Day3 header means Day 3) - controller.tableView(controller.tableView, moveRowAt: IndexPath(row: 1, section: 0), to: IndexPath(row: 3, section: 0)) + // Rows: 0=Day1 header, 1=Day2 header, 2=travel, 3=Day3 header + // Move travel (row 2) to row 3 (after Day3 header = Day 3) + controller.tableView(controller.tableView, moveRowAt: IndexPath(row: 2, section: 0), to: IndexPath(row: 3, section: 0)) XCTAssertEqual(capturedTravelId, "travel:chicago->detroit") XCTAssertEqual(capturedDay, 3, "Travel should now be on Day 3") @@ -228,18 +232,21 @@ final class ItineraryTravelConstraintTests: XCTestCase { // Given: Travel with valid range Days 2-3 let travel = H.makeTravelSegment(from: "Chicago", to: "Detroit") let travelId = "travel:chicago->detroit" + let travelItem = ItineraryRowItem.travel(travel, dayNumber: 2) + let travelModelItem = H.makeTravelItem(from: "Chicago", to: "Detroit", day: 2, sortOrder: 1.0) let day1 = ItineraryDayData(id: 1, dayNumber: 1, date: testDate, games: [], items: [], travelBefore: nil) - let day2 = ItineraryDayData(id: 2, dayNumber: 2, date: H.dayAfter(testDate), games: [], items: [], travelBefore: travel) + let day2 = ItineraryDayData(id: 2, dayNumber: 2, date: H.dayAfter(testDate), games: [], items: [travelItem], travelBefore: nil) let day3 = ItineraryDayData(id: 3, dayNumber: 3, date: H.dayAfter(H.dayAfter(testDate)), games: [], items: [], travelBefore: nil) let controller = ItineraryTableViewController(style: .plain) let validRanges = [travelId: 2...3] - controller.reloadData(days: [day1, day2, day3], travelValidRanges: validRanges) + controller.reloadData(days: [day1, day2, day3], travelValidRanges: validRanges, itineraryItems: [travelModelItem]) - // Travel is at row 1 (after Day1 header at row 0) - // Try to move it to Day 1 area (row 0 or 1) - should snap back to valid range - let source = IndexPath(row: 1, section: 0) + // Rows: 0=Day1 header, 1=Day2 header, 2=travel, 3=Day3 header + // Travel is at row 2 (after Day2 header at row 1) + // Try to move it to Day 1 area (row 0) - should snap back to valid range + let source = IndexPath(row: 2, section: 0) let proposed = IndexPath(row: 0, section: 0) let result = controller.tableView(controller.tableView, targetIndexPathForMoveFromRowAt: source, toProposedIndexPath: proposed) diff --git a/SportsTimeTests/ItineraryConstraintsTests.swift b/SportsTimeTests/ItineraryConstraintsTests.swift index ed72912..26797bc 100644 --- a/SportsTimeTests/ItineraryConstraintsTests.swift +++ b/SportsTimeTests/ItineraryConstraintsTests.swift @@ -49,33 +49,35 @@ final class ItineraryConstraintsTests: XCTestCase { } func test_travel_mustBeAfterDepartureGames() { - // Given: Chicago game on Day 1 at sortOrder 100 + // Given: Chicago game on Day 1 + // Visual boundary is 0: sortOrder < 0 = before games, sortOrder >= 0 = after games let constraints = makeConstraints( tripDays: 3, - games: [makeGameItem(city: "Chicago", day: 1, sortOrder: 100)] + games: [makeGameItem(city: "Chicago", day: 1)] ) - let travel = makeTravelItem(from: "Chicago", to: "Detroit", day: 1, sortOrder: 50) + let travel = makeTravelItem(from: "Chicago", to: "Detroit", day: 1, sortOrder: -1) - // When/Then: Travel before game is invalid - XCTAssertFalse(constraints.isValidPosition(for: travel, day: 1, sortOrder: 50)) + // When/Then: Travel before game (negative sortOrder) is invalid on departure day + XCTAssertFalse(constraints.isValidPosition(for: travel, day: 1, sortOrder: -1)) - // Travel after game is valid - XCTAssertTrue(constraints.isValidPosition(for: travel, day: 1, sortOrder: 150)) + // Travel after game (non-negative sortOrder) is valid + XCTAssertTrue(constraints.isValidPosition(for: travel, day: 1, sortOrder: 1)) } func test_travel_mustBeBeforeArrivalGames() { - // Given: Detroit game on Day 3 at sortOrder 100 + // Given: Detroit game on Day 3 + // Visual boundary is 0: sortOrder < 0 = before games, sortOrder >= 0 = after games let constraints = makeConstraints( tripDays: 3, - games: [makeGameItem(city: "Detroit", day: 3, sortOrder: 100)] + games: [makeGameItem(city: "Detroit", day: 3)] ) - let travel = makeTravelItem(from: "Chicago", to: "Detroit", day: 3, sortOrder: 150) + let travel = makeTravelItem(from: "Chicago", to: "Detroit", day: 3, sortOrder: 1) - // When/Then: Travel after arrival game is invalid - XCTAssertFalse(constraints.isValidPosition(for: travel, day: 3, sortOrder: 150)) + // When/Then: Travel after arrival game (non-negative sortOrder) is invalid on arrival day + XCTAssertFalse(constraints.isValidPosition(for: travel, day: 3, sortOrder: 1)) - // Travel before game is valid - XCTAssertTrue(constraints.isValidPosition(for: travel, day: 3, sortOrder: 50)) + // Travel before game (negative sortOrder) is valid + XCTAssertTrue(constraints.isValidPosition(for: travel, day: 3, sortOrder: -1)) } func test_travel_canBeAnywhereOnRestDays() { @@ -217,13 +219,12 @@ final class ItineraryConstraintsTests: XCTestCase { return ItineraryConstraints(tripDayCount: tripDays, items: games) } - private func makeGameItem(city: String, day: Int, sortOrder: Double = 100) -> ItineraryItem { - // For tests, we use gameId to encode the city + private func makeGameItem(city: String, day: Int, sortOrder: Double = 0) -> ItineraryItem { return ItineraryItem( tripId: testTripId, day: day, sortOrder: sortOrder, - kind: .game(gameId: "game-\(city)-\(UUID().uuidString.prefix(4))") + kind: .game(gameId: "game-\(city)-\(UUID().uuidString.prefix(4))", city: city) ) }