From 49c744d033520f497ee7550d34ee6037fd7ea039 Mon Sep 17 00:00:00 2001 From: Emma Simon Date: Tue, 12 Nov 2024 16:24:40 -0500 Subject: [PATCH] fix: Display error state when trip doesn't exist (#527) * fix: Display error state when trip doesn't exist We can get added trip IDs that don't exist in the API in trip details, which before these changes was resulting in shimmer loading forever. Worse, it would continue to shimmer load even after switching to a different vehicle on the map. * fix: Fix tests and PR feedback --- .../Pages/TripDetails/TripDetailsPage.swift | 19 ++++++--- .../Pages/TripDetails/VehicleCardView.swift | 14 +++---- .../TripDetails/TripDetailsPageTests.swift | 37 ++++++++-------- .../TripDetails/VehicleCardViewTests.swift | 12 +++--- .../tid/mbta_app/model/TripDetailsStopList.kt | 40 ++++++++++-------- .../mbta_app/model/TripDetailsStopListTest.kt | 42 +++++++++++++++---- 6 files changed, 103 insertions(+), 61 deletions(-) diff --git a/iosApp/iosApp/Pages/TripDetails/TripDetailsPage.swift b/iosApp/iosApp/Pages/TripDetails/TripDetailsPage.swift index a0d00b7c..386e1554 100644 --- a/iosApp/iosApp/Pages/TripDetails/TripDetailsPage.swift +++ b/iosApp/iosApp/Pages/TripDetails/TripDetailsPage.swift @@ -24,6 +24,7 @@ struct TripDetailsPage: View { @State var globalResponse: GlobalResponse? @State var tripPredictionsRepository: ITripPredictionsRepository @State var tripPredictions: PredictionsStreamDataResponse? + @State var tripPredictionsLoaded: Bool = false @State var tripRepository: ITripRepository @State var trip: Trip? @State var tripSchedulesResponse: TripSchedulesResponse? @@ -71,9 +72,10 @@ struct TripDetailsPage: View { var body: some View { VStack(spacing: 16) { header - if let trip, let globalResponse, let vehicle = vehicleResponse?.vehicle, + if tripPredictionsLoaded, let globalResponse, let vehicle = vehicleResponse?.vehicle, let stops = TripDetailsStopList.companion.fromPieces( - trip: trip, + tripId: tripId, + directionId: trip?.directionId ?? vehicle.directionId, tripSchedules: tripSchedulesResponse, tripPredictions: tripPredictions, vehicle: vehicle, @@ -120,6 +122,10 @@ struct TripDetailsPage: View { .onAppear { joinRealtime() } .onDisappear { leaveRealtime() } .onChange(of: tripId) { + errorBannerVM.errorRepository.clearDataError(key: "TripDetailsPage.loadTripSchedules") + errorBannerVM.errorRepository.clearDataError(key: "TripDetailsPage.loadTrip") + loadTripSchedules() + loadTrip() leavePredictions() joinPredictions(tripId: $0) } @@ -221,12 +227,14 @@ struct TripDetailsPage: View { case let .ok(result): tripPredictions = result.data case .error: break } + tripPredictionsLoaded = true checkPredictionsStale() } } } private func leavePredictions() { + tripPredictionsLoaded = false tripPredictionsRepository.disconnect() } @@ -276,7 +284,7 @@ struct TripDetailsPage: View { } else { nil } - let route: Route? = if let routeId = trip?.routeId { + let route: Route? = if let routeId = trip?.routeId ?? vehicle?.routeId { globalResponse?.routes[routeId] } else { nil @@ -285,14 +293,15 @@ struct TripDetailsPage: View { vehicle: vehicle, route: route, stop: vehicleStop, - trip: trip + tripId: tripId ) } @ViewBuilder var header: some View { let trip: Trip? = tripPredictions?.trips[tripId] - let route: Route? = if let routeId = trip?.routeId { + let vehicle: Vehicle? = vehicleResponse?.vehicle + let route: Route? = if let routeId = trip?.routeId ?? vehicle?.routeId { globalResponse?.routes[routeId] } else { nil diff --git a/iosApp/iosApp/Pages/TripDetails/VehicleCardView.swift b/iosApp/iosApp/Pages/TripDetails/VehicleCardView.swift index f62099d0..a6f7f1f6 100644 --- a/iosApp/iosApp/Pages/TripDetails/VehicleCardView.swift +++ b/iosApp/iosApp/Pages/TripDetails/VehicleCardView.swift @@ -14,18 +14,18 @@ struct VehicleCardView: View { let vehicle: Vehicle? let route: Route? let stop: Stop? - let trip: Trip? + let tripId: String var body: some View { - if let vehicle, let route, let stop, let trip { + if let vehicle, let route, let stop { VehicleOnTripView( vehicle: vehicle, route: route, stop: stop, - trip: trip + tripId: tripId ) } else { - Text("Loading...") + EmptyView() } } } @@ -34,7 +34,7 @@ struct VehicleOnTripView: View { let vehicle: Vehicle let route: Route let stop: Stop - let trip: Trip + let tripId: String var backgroundColor: Color { Color(hex: route.color) @@ -58,7 +58,7 @@ struct VehicleOnTripView: View { @ViewBuilder private var description: some View { - if vehicle.tripId == trip.id { + if vehicle.tripId == tripId { VStack(alignment: .leading, spacing: 2) { vehicleStatusDescription(vehicle.currentStatus) .font(Typography.caption) @@ -162,7 +162,7 @@ struct VehicleCardView_Previews: PreviewProvider { } List { - VehicleCardView(vehicle: vehicle, route: red, stop: stop, trip: trip) + VehicleCardView(vehicle: vehicle, route: red, stop: stop, tripId: trip.id) } .previewDisplayName("VehicleCard") } diff --git a/iosApp/iosAppTests/Pages/TripDetails/TripDetailsPageTests.swift b/iosApp/iosAppTests/Pages/TripDetails/TripDetailsPageTests.swift index c60c58e0..c9e5add5 100644 --- a/iosApp/iosAppTests/Pages/TripDetails/TripDetailsPageTests.swift +++ b/iosApp/iosAppTests/Pages/TripDetails/TripDetailsPageTests.swift @@ -172,7 +172,7 @@ final class TripDetailsPageTests: XCTestCase { let vehicleId = "999" let nearbyVM = NearbyViewModel() nearbyVM.alerts = .init(alerts: [:]) - var sut = TripDetailsPage( + let sut = TripDetailsPage( tripId: tripId, vehicleId: vehicleId, routeId: trip.routeId, @@ -180,13 +180,13 @@ final class TripDetailsPageTests: XCTestCase { errorBannerVM: .init(), nearbyVM: nearbyVM, mapVM: .init(), - globalRepository: MockGlobalRepository(response: .init(objects: objects, patternIdsByStop: [:])), + globalRepository: MockGlobalRepository(response: .init(objects: objects)), tripPredictionsRepository: FakeTripPredictionsRepository(response: .init(objects: objects)), tripRepository: tripRepository, vehicleRepository: FakeVehicleRepository(response: .init(vehicle: vehicle)) ) - let splitViewExp = sut.on(\.didLoadData) { view in + let splitViewExp = sut.inspection.inspect(onReceive: tripSchedulesLoaded, after: 0.5) { view in XCTAssertNotNil(try view.find(TripDetailsStopListSplitView.self)) } @@ -307,15 +307,13 @@ final class TripDetailsPageTests: XCTestCase { wait(for: [routeExp], timeout: 5) } - @MainActor func testTripRequestError() throws { + @MainActor func testMissingTrip() throws { let objects = ObjectCollectionBuilder() - let trip = objects.trip { trip in - trip.routeId = "Red" - } + let tripId = "ADDED-trip" let vehicle = objects.vehicle { vehicle in - vehicle.tripId = trip.id + vehicle.tripId = tripId vehicle.currentStatus = .inTransitTo } @@ -336,15 +334,16 @@ final class TripDetailsPageTests: XCTestCase { onConnect: { _ in tripPredictionsLoaded.send() } ) - let tripId = trip.id let vehicleId = vehicle.id + let nearbyVM = NearbyViewModel(alertsRepository: MockAlertsRepository()) + nearbyVM.alerts = .init(alerts: [:]) let sut = TripDetailsPage( tripId: tripId, vehicleId: vehicleId, - routeId: trip.routeId, + routeId: "Red", target: nil, errorBannerVM: .init(), - nearbyVM: .init(), + nearbyVM: nearbyVM, mapVM: .init(), globalRepository: MockGlobalRepository(response: globalData), tripPredictionsRepository: tripPredictionsRepository, @@ -355,9 +354,8 @@ final class TripDetailsPageTests: XCTestCase { let everythingLoaded = tripSchedulesLoaded.zip(tripPredictionsLoaded) let routeExp = sut.inspection.inspect(onReceive: everythingLoaded, after: 0.1) { view in - XCTAssertNotNil(try view.find(where: { view in - (try? view.modifier(LoadingPlaceholderModifier.self)) != nil - })) + XCTAssertNotNil(try view.find(VehicleCardView.self)) + XCTAssertThrowsError(try view.find(TripDetailsStopView.self)) } ViewHosting.host(view: sut) @@ -647,11 +645,11 @@ final class TripDetailsPageTests: XCTestCase { } class FakeTripPredictionsRepository: ITripPredictionsRepository { - let response: PredictionsStreamDataResponse + let response: PredictionsStreamDataResponse? let onConnect: ((_ tripId: String) -> Void)? let onDisconnect: (() -> Void)? - init(response: PredictionsStreamDataResponse, + init(response: PredictionsStreamDataResponse?, onConnect: ((_ tripId: String) -> Void)? = nil, onDisconnect: (() -> Void)? = nil) { self.response = response @@ -664,7 +662,12 @@ final class TripDetailsPageTests: XCTestCase { onReceive: @escaping (ApiResult) -> Void ) { onConnect?(tripId) - onReceive(ApiResultOk(data: response)) + let result = if let response { + ApiResultOk(data: response) + } else { + ApiResultError(code: 404, message: "Failed to load predictions") + } + onReceive(result) } var lastUpdated: Instant? diff --git a/iosApp/iosAppTests/Pages/TripDetails/VehicleCardViewTests.swift b/iosApp/iosAppTests/Pages/TripDetails/VehicleCardViewTests.swift index 435e4855..f153a310 100644 --- a/iosApp/iosAppTests/Pages/TripDetails/VehicleCardViewTests.swift +++ b/iosApp/iosAppTests/Pages/TripDetails/VehicleCardViewTests.swift @@ -30,7 +30,7 @@ final class VehicleCardViewTests: XCTestCase { } let stop = objects.stop { _ in } - let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, trip: trip) + let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, tripId: trip.id) XCTAssertNotNil(try sut.inspect().find(text: "This vehicle is completing another trip")) } @@ -49,7 +49,7 @@ final class VehicleCardViewTests: XCTestCase { stop.name = "Davis" } - let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, trip: trip) + let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, tripId: trip.id) XCTAssertNotNil(try sut.inspect().find(text: "Approaching")) XCTAssertNotNil(try sut.inspect().find(text: "Davis")) } @@ -67,7 +67,7 @@ final class VehicleCardViewTests: XCTestCase { let route = objects.route { _ in } let stop = objects.stop { _ in } - let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, trip: trip) + let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, tripId: trip.id) XCTAssertNotNil(try sut.inspect().find(text: "Next stop")) } @@ -84,7 +84,7 @@ final class VehicleCardViewTests: XCTestCase { let route = objects.route { _ in } let stop = objects.stop { _ in } - let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, trip: trip) + let sut = VehicleCardView(vehicle: vehicle, route: route, stop: stop, tripId: trip.id) XCTAssertNotNil(try sut.inspect().find(text: "Now at")) } @@ -95,7 +95,7 @@ final class VehicleCardViewTests: XCTestCase { trip.headsign = "Alewife" } - let sut = VehicleCardView(vehicle: nil, route: nil, stop: nil, trip: trip) - XCTAssertNotNil(try sut.inspect().find(text: "Loading...")) + let sut = VehicleCardView(vehicle: nil, route: nil, stop: nil, tripId: trip.id) + XCTAssertThrowsError(try sut.inspect().find(VehicleOnTripView.self)) } } diff --git a/shared/src/commonMain/kotlin/com/mbta/tid/mbta_app/model/TripDetailsStopList.kt b/shared/src/commonMain/kotlin/com/mbta/tid/mbta_app/model/TripDetailsStopList.kt index df7980fa..7a056d46 100644 --- a/shared/src/commonMain/kotlin/com/mbta/tid/mbta_app/model/TripDetailsStopList.kt +++ b/shared/src/commonMain/kotlin/com/mbta/tid/mbta_app/model/TripDetailsStopList.kt @@ -126,24 +126,28 @@ data class TripDetailsStopList(val stops: List) { } fun fromPieces( - trip: Trip, + tripId: String, + directionId: Int, tripSchedules: TripSchedulesResponse?, tripPredictions: PredictionsStreamDataResponse?, vehicle: Vehicle?, alertsData: AlertsStreamDataResponse?, globalData: GlobalResponse, ): TripDetailsStopList? { - if (tripPredictions == null || alertsData == null) return null + if (alertsData == null) return null val entries = mutableMapOf() - val predictions = - deduplicatePredictionsByStopSequence( - tripPredictions.predictions.values, - tripSchedules, - globalData - ) + var predictions = emptyList() + if (tripPredictions != null) { + predictions = + deduplicatePredictionsByStopSequence( + tripPredictions.predictions.values, + tripSchedules, + globalData + ) - predictions.forEach { prediction -> entries.putPrediction(prediction, vehicle) } + predictions.forEach { prediction -> entries.putPrediction(prediction, vehicle) } + } if (tripSchedules is TripSchedulesResponse.Schedules) { tripSchedules.schedules.forEach { entries.putSchedule(it) } @@ -159,7 +163,7 @@ data class TripDetailsStopList(val stops: List) { } if (entries.isEmpty()) { - return null + return TripDetailsStopList(emptyList()) } return TripDetailsStopList( entries.entries @@ -167,7 +171,7 @@ data class TripDetailsStopList(val stops: List) { .dropWhile { if ( vehicle == null || - vehicle.tripId != trip.id || + vehicle.tripId != tripId || vehicle.currentStopSequence == null ) { false @@ -179,7 +183,7 @@ data class TripDetailsStopList(val stops: List) { Entry( globalData.stops.getValue(it.value.stopId), it.value.stopSequence, - getAlert(it.value, alertsData, globalData, trip), + getAlert(it.value, alertsData, globalData, tripId, directionId), it.value.schedule, it.value.prediction, it.value.vehicle, @@ -316,8 +320,9 @@ data class TripDetailsStopList(val stops: List) { while (scheduleIndex in stopIds.indices && predictionIndex in predictions.indices) { val stopId = stopIds[scheduleIndex] val prediction = predictions[predictionIndex] - check(Stop.equalOrFamily(stopId, prediction.stopId, globalData.stops)) { - "predictions=$predictions stopIds=$stopIds predictionIndex=$predictionIndex scheduleIndex=$scheduleIndex" + if (!Stop.equalOrFamily(stopId, prediction.stopId, globalData.stops)) { + scheduleIndex-- + continue } lastDelta = lastStopSequence?.minus(prediction.stopSequence) lastStopSequence = prediction.stopSequence @@ -359,7 +364,8 @@ data class TripDetailsStopList(val stops: List) { entry: WorkingEntry, alertsData: AlertsStreamDataResponse?, globalData: GlobalResponse, - trip: Trip + tripId: String, + directionId: Int ): Alert? { val entryTime = entry.prediction?.predictionTime ?: entry.schedule?.scheduleTime val entryRoute = entry.prediction?.routeId ?: entry.schedule?.routeId @@ -371,11 +377,11 @@ data class TripDetailsStopList(val stops: List) { alert.isActive(entryTime) && alert.anyInformedEntity { it.appliesTo( - directionId = trip.directionId, + directionId = directionId, routeId = entryRoute, routeType = entryRouteType, stopId = entry.stopId, - tripId = trip.id + tripId = tripId ) } && // there's no UI yet for secondary alerts in trip details diff --git a/shared/src/commonTest/kotlin/com/mbta/tid/mbta_app/model/TripDetailsStopListTest.kt b/shared/src/commonTest/kotlin/com/mbta/tid/mbta_app/model/TripDetailsStopListTest.kt index bef50246..5098c51f 100644 --- a/shared/src/commonTest/kotlin/com/mbta/tid/mbta_app/model/TripDetailsStopListTest.kt +++ b/shared/src/commonTest/kotlin/com/mbta/tid/mbta_app/model/TripDetailsStopListTest.kt @@ -111,7 +111,8 @@ class TripDetailsStopListTest { trip: Trip = Trip("trip", 0, "", "") ) = TripDetailsStopList.fromPieces( - trip, + trip.id, + trip.directionId, tripSchedules, tripPredictions, vehicle, @@ -138,24 +139,43 @@ class TripDetailsStopListTest { assertEquals("A", stop("A1").parentStationId) } - @Test fun `fromPieces returns null with no data`() = test { assertNull(fromPieces(null, null)) } + @Test + fun `fromPieces returns empty list with no data`() = test { + assertEquals(TripDetailsStopList(stops = emptyList()), fromPieces(null, null)) + } @Test - fun `fromPieces returns null with real schedules and no predictions`() = test { + fun `fromPieces returns schedules when there are no predictions`() = test { val sched1 = schedule("A", 10) val sched2 = schedule("B", 20) val sched3 = schedule("C", 30) - assertEquals(null, fromPieces(schedulesResponseOf(sched1, sched2, sched3), null)) + assertEquals( + stopListOf( + entry("A", 10, schedule = sched1), + entry("B", 20, schedule = sched2), + entry("C", 30, schedule = sched3) + ), + fromPieces(schedulesResponseOf(sched1, sched2, sched3), null) + ) } @Test fun `fromPieces returns null with scheduled IDs and no predictions`() = test { - assertEquals(null, fromPieces(schedulesResponseOf("A", "B", "C"), null)) + val sched1 = schedule("A", 10) + val sched2 = schedule("B", 20) + val sched3 = schedule("C", 30) + assertEquals( + stopListOf(entry("A", 997), entry("B", 998), entry("C", 999)), + fromPieces(schedulesResponseOf(sched1.stopId, sched2.stopId, sched3.stopId), null) + ) } @Test fun `fromPieces returns null with unavailable schedules and no predictions`() = test { - assertNull(fromPieces(TripSchedulesResponse.Unknown, null)) + assertEquals( + TripDetailsStopList(stops = emptyList()), + fromPieces(TripSchedulesResponse.Unknown, null) + ) } @Test @@ -357,7 +377,8 @@ class TripDetailsStopListTest { val list = TripDetailsStopList.fromPieces( - trip, + trip.id, + trip.directionId, schedules, predictions, null, @@ -424,7 +445,8 @@ class TripDetailsStopListTest { val list = TripDetailsStopList.fromPieces( - trip, + trip.id, + trip.directionId, schedules, predictions, vehicle, @@ -589,7 +611,9 @@ class TripDetailsStopListTest { val pred = prediction(stopB.id, 20, routeCurrent.id) assertEquals( - null, + stopListOf( + entry(stopA.id, stopSequence = 10, schedule = sched, routes = listOf(routeOther)) + ), fromPieces( schedulesResponseOf(sched), null,