diff --git a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt index a05f3f001..44bd3e972 100644 --- a/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt +++ b/onebusaway-android/src/main/java/org/onebusaway/android/ui/tripplan/Otp2Planner.kt @@ -95,16 +95,42 @@ class Otp2Planner @Inject constructor( throw TripPlanException(TripPlanError.Unknown, e) } - data.planConnection?.routingErrors?.firstOrNull()?.let { - throw TripPlanException(otp2ErrorFor(it.code, it.inputField)) - } + return resolveOtp2Plan(data) + } +} - val itineraries = data.toTripItineraries() - if (itineraries.isEmpty()) { - throw TripPlanException(TripPlanError.NoRoute) - } +/** + * Resolves an OTP2 `planConnection` [PlanQuery.Data] into itineraries, or throws a classified + * [TripPlanException] when there is genuinely nothing to show. + * + * **Itineraries win over routing errors.** A `routingErrors` entry can accompany a perfectly good + * result, so we must return the result rather than surface the error. The motivating case (#1947) is + * [RoutingErrorCode.WALKING_BETTER_THAN_TRANSIT]: OTP2 always computes a direct WALK itinerary + * alongside transit — the `planConnection.modes` default is documented as "all transit modes are + * usable and WALK is used for direct street suggestions" — and when that walk's *generalized* cost + * (which folds in wait/transfer/boarding penalties, not just distance) beats every transit option, + * the filter chain deletes the *transit* itineraries and attaches this error while **keeping the + * walk-only itinerary in `edges`**. Surfacing the error there would throw away a valid walk route and + * show a "Try walking instead" advisory with no result — even for trips too long to actually walk. + * Returning whatever itineraries came back shows that walk route as a normal option instead. + * + * This is safe for every *fatal* code — `LOCATION_NOT_FOUND`, `OUTSIDE_BOUNDS`, + * `NO_TRANSIT_CONNECTION`, the same-location `WALKING_BETTER_THAN_TRANSIT` raised by OTP's + * `SameEdgeAdjuster`, … — because those always come back with empty `edges`, so consulting + * `routingErrors` only when there are no itineraries still classifies them exactly as before. + * + * Top-level and `internal` (no [Context] dependency) so it's JVM-unit-testable from a + * [PlanQuery.Data] fixture without Apollo, like [otp2ErrorFor]. + */ +internal fun resolveOtp2Plan(data: PlanQuery.Data): List { + val itineraries = data.toTripItineraries() + if (itineraries.isNotEmpty()) { return itineraries } + data.planConnection?.routingErrors?.firstOrNull()?.let { + throw TripPlanException(otp2ErrorFor(it.code, it.inputField)) + } + throw TripPlanException(TripPlanError.NoRoute) } /** diff --git a/onebusaway-android/src/test/java/org/onebusaway/android/ui/tripplan/Otp2PlanResolveTest.kt b/onebusaway-android/src/test/java/org/onebusaway/android/ui/tripplan/Otp2PlanResolveTest.kt new file mode 100644 index 000000000..0f9041e18 --- /dev/null +++ b/onebusaway-android/src/test/java/org/onebusaway/android/ui/tripplan/Otp2PlanResolveTest.kt @@ -0,0 +1,144 @@ +/* + * Copyright (C) 2026 Open Transit Software Foundation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.onebusaway.android.ui.tripplan + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertThrows +import org.junit.Test +import org.onebusaway.android.R +import org.onebusaway.android.api.graphql.PlanQuery +import org.onebusaway.android.api.graphql.fragment.PlaceFields +import org.onebusaway.android.api.graphql.type.InputField +import org.onebusaway.android.api.graphql.type.Mode +import org.onebusaway.android.api.graphql.type.RoutingErrorCode +import org.onebusaway.android.directions.model.TripMode + +/** + * Covers [resolveOtp2Plan]: the rule that OTP2 itineraries win over a coexisting `routingErrors` + * entry, and that fatal errors (which always arrive with empty `edges`) still classify as before. + * + * The regression this guards is #1947: OTP2 emits [RoutingErrorCode.WALKING_BETTER_THAN_TRANSIT] + * *while keeping* the walk-only itinerary in the response, and the old code threw that advisory + * before ever reading the itinerary — hiding a valid walk route behind a "Try walking instead" + * message. Pure JVM: builds Apollo-generated [PlanQuery.Data] fixtures directly, no Apollo/HTTP. + */ +class Otp2PlanResolveTest { + + /** + * The core fix: a `WALKING_BETTER_THAN_TRANSIT` error alongside a surviving walk itinerary must + * yield the walk itinerary, not throw the advisory. + */ + @Test + fun walkingBetterThanTransit_withWalkItinerary_returnsTheWalk() { + val data = planData( + routingErrors = listOf(routingError(RoutingErrorCode.WALKING_BETTER_THAN_TRANSIT)), + edges = listOf(walkEdge()), + ) + + val itineraries = resolveOtp2Plan(data) + + assertEquals(1, itineraries.size) + assertEquals(TripMode.WALK, itineraries[0].legs[0].mode) + } + + /** + * The same-location degenerate case (OTP's `SameEdgeAdjuster`) emits the same code but with no + * itineraries — with nothing to show, the advisory still surfaces. + */ + @Test + fun walkingBetterThanTransit_withoutItineraries_throwsAdvisory() { + val data = planData( + routingErrors = listOf(routingError(RoutingErrorCode.WALKING_BETTER_THAN_TRANSIT)), + edges = emptyList(), + ) + + val error = assertThrows(TripPlanException::class.java) { resolveOtp2Plan(data) }.error + assertEquals(TripPlanError.Category.ADVISORY, error.category) + assertEquals(R.string.tripplanner_error_walking_better_than_transit, error.detailRes) + } + + /** A fatal error (always empty `edges`) still classifies exactly as before the fix. */ + @Test + fun fatalError_withoutItineraries_throwsClassifiedError() { + val data = planData( + routingErrors = listOf(routingError(RoutingErrorCode.LOCATION_NOT_FOUND, InputField.FROM)), + edges = emptyList(), + ) + + val error = assertThrows(TripPlanException::class.java) { resolveOtp2Plan(data) }.error + assertEquals(TripPlanError.Category.LOCATION, error.category) + assertEquals(R.string.tripplanner_error_geocode_from_not_found, error.detailRes) + } + + /** No itineraries and no error is a plain no-route result. */ + @Test + fun noItinerariesNoError_throwsNoRoute() { + val data = planData(routingErrors = emptyList(), edges = emptyList()) + + val error = assertThrows(TripPlanException::class.java) { resolveOtp2Plan(data) }.error + assertEquals(TripPlanError.NoRoute, error) + } + + // ---- fixtures ---- + + private fun planData( + routingErrors: List, + edges: List, + ) = PlanQuery.Data( + planConnection = PlanQuery.PlanConnection( + searchDateTime = "2026-07-11T10:00:00-07:00", + routingErrors = routingErrors, + edges = edges, + ), + ) + + private fun routingError(code: RoutingErrorCode, inputField: InputField? = null) = + PlanQuery.RoutingError(code = code, description = code.name, inputField = inputField) + + private fun walkEdge(): PlanQuery.Edge { + val leg = PlanQuery.Leg( + mode = Mode.WALK, + duration = 2400.0, + distance = 3200.0, + realTime = null, + start = PlanQuery.Start(scheduledTime = "2026-07-11T10:00:00-07:00", estimated = null), + end = PlanQuery.End(scheduledTime = "2026-07-11T10:40:00-07:00", estimated = null), + from = from("Origin", 47.60, -122.30), + to = to("Destination", 47.62, -122.34), + route = null, + trip = null, + legGeometry = null, + steps = null, + ) + val node = PlanQuery.Node( + start = "2026-07-11T10:00:00-07:00", + end = "2026-07-11T10:40:00-07:00", + duration = 2400L, + numberOfTransfers = 0, + legs = listOf(leg), + ) + return PlanQuery.Edge(node = node) + } + + private fun placeFields(name: String, lat: Double, lon: Double) = + PlaceFields(name, lat, lon, null, null, null, null) + + private fun from(name: String, lat: Double, lon: Double) = + PlanQuery.From(__typename = "Place", placeFields = placeFields(name, lat, lon)) + + private fun to(name: String, lat: Double, lon: Double) = + PlanQuery.To(__typename = "Place", placeFields = placeFields(name, lat, lon)) +}