Skip to content

fix(nearby): filter stop sheet routes to the tapped mode (shared stop_id) #176

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ai-tiro wants to merge 1 commit into from
Closed

fix(nearby): filter stop sheet routes to the tapped mode (shared stop_id) #176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .../core/network/src/main/kotlin/ac/jfx/openptv/core/network/RetrofitStopDetailDataSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,6 @@ internal class RetrofitStopDetailDataSource
): StopDetail? {
val typeCode = routeType.toPtvCode()
val path = "stops/${stopId.value}/route_type/$typeCode?stop_location=true&stop_disruptions=true"
return api.getStop(urlResolver.resolve(path)).toDomain()
return api.getStop(urlResolver.resolve(path)).toDomain(requestedRouteType = routeType)
}
}
19 changes: 17 additions & 2 deletions mobile/core/network/src/main/kotlin/ac/jfx/openptv/core/network/model/StopResponseDto.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,20 @@ internal data class RouteDto(
* Map the PTV wire envelope into the domain [StopDetail]. Returns `null` when the response has
* no stop block — the repository surfaces that as `Result.Error` so the UI can show "stop not
* found" rather than rendering a half-empty header.
*
* [requestedRouteType] filters the serving-routes list to the mode the caller asked for. PTV's
* `/stops/{id}/route_type/{type}` endpoint IGNORES the `route_type` path param for the `routes`
* array and returns *every* route serving the physical `stop_id`. At co-located stops that share
* a `stop_id` (Richmond is the canonical case: the metro platforms and the V/Line platform are
* one `stop_id` 1162) this mixes modes, so a V/Line stop comes back carrying metro train routes
* (issue #175 — the Nearby sheet rendered Alamein/Belgrave/… chips on the V/Line pin).
* Departures are already fetched per `(stopId, routeType)` and are correct, so filtering the
* routes here keeps `servingRoutes` consistent with the mode every caller requested.
* [RouteType.Unknown] disables the filter (we never put Unknown on the wire — this only guards
* against an unexpected upstream value, where dropping every route would be worse than passing
* the raw list through).
*/
internal fun StopResponseDto.toDomain(): StopDetail? {
internal fun StopResponseDto.toDomain(requestedRouteType: RouteType): StopDetail? {
val s = stop ?: return null
// Pick the nested `stop_location.gps` pair when the top-level fields are absent (= default
// 0.0) — the single-stop endpoint only populates the nested shape. The nearby endpoint
Expand All @@ -94,7 +106,10 @@ internal fun StopResponseDto.toDomain(): StopDetail? {
latitude = lat,
longitude = lng,
),
servingRoutes = s.routes.map { it.toDomain() },
servingRoutes =
s.routes
.map { it.toDomain() }
.filter { requestedRouteType == RouteType.Unknown || it.routeType == requestedRouteType },
)
}

Expand Down
86 changes: 81 additions & 5 deletions mobile/core/network/src/test/kotlin/ac/jfx/openptv/core/network/StopDetailDtoMapperTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class StopDetailDtoMapperTest {
.build(),
)

val detail = response.toDomain()
val detail = response.toDomain(requestedRouteType = RouteType.Train)

assertThat(detail).isNotNull()
assertThat(detail!!.stop.name).isEqualTo("Flinders Street Railway Station")
Expand All @@ -50,14 +50,14 @@ class StopDetailDtoMapperTest {
@Test
fun `response with no stop block returns null`() {
val response = StopResponseDto(stop = null)
assertThat(response.toDomain()).isNull()
assertThat(response.toDomain(requestedRouteType = RouteType.Train)).isNull()
}

@Test
fun `response with stop but no routes maps to empty servingRoutes`() {
val response = StopResponseDto(stop = StopDetailsDtoMother.aStopDetailsDto().build())

val detail = response.toDomain()
val detail = response.toDomain(requestedRouteType = RouteType.Train)

assertThat(detail).isNotNull()
assertThat(detail!!.servingRoutes).isEmpty()
Expand All @@ -78,7 +78,7 @@ class StopDetailDtoMapperTest {
.build(),
)

val detail = response.toDomain()!!
val detail = response.toDomain(requestedRouteType = RouteType.Tram)!!

assertThat(detail.stop.routeType).isEqualTo(RouteType.Tram)
assertThat(detail.servingRoutes[0].routeType).isEqualTo(RouteType.Tram)
Expand All @@ -103,8 +103,84 @@ class StopDetailDtoMapperTest {
.build(),
)

val detail = response.toDomain()!!
val detail = response.toDomain(requestedRouteType = RouteType.Tram)!!

assertThat(detail.servingRoutes.map { it.number }).containsExactly("75", "70").inOrder()
}

@Test
fun `serving routes are filtered to the requested route type at a shared stop_id`() {
// Regression coverage for issue #175. Richmond's metro platforms and the V/Line platform
// share `stop_id` 1162, and PTV's `/stops/{id}/route_type/{type}` endpoint ignores the
// path `route_type` for the `routes` array — it returns every route serving the physical
// stop. Captured shape from `/api/v3/stops/1162/route_type/3`: metro (route_type 0) lines
// mixed in with the V/Line (route_type 3) services. When the caller asked for V/Line, the
// mapper must drop the metro routes so the Nearby sheet's Routes chips match the tapped
// mode (departures are already fetched per `(stopId, routeType)` and were correct).
val response =
StopResponseDto(
stop =
StopDetailsDtoMother.aStopDetailsDto()
.withStopName("Richmond Railway Station")
.withRouteType(3)
.withRoutes(
listOf(
RouteDto(routeId = 1, routeName = "Alamein", routeNumber = "", routeType = 0),
RouteDto(routeId = 2, routeName = "Belgrave", routeNumber = "", routeType = 0),
RouteDto(
routeId = 3,
routeName = "Bairnsdale - Melbourne via Sale & Traralgon",
routeNumber = "",
routeType = 3,
),
RouteDto(
routeId = 4,
routeName = "Traralgon - Melbourne via Morwell & Moe & Pakenham",
routeNumber = "",
routeType = 3,
),
),
)
.build(),
)

val detail = response.toDomain(requestedRouteType = RouteType.VLine)!!

assertThat(detail.servingRoutes.map { it.routeType })
.containsExactly(RouteType.VLine, RouteType.VLine)
assertThat(detail.servingRoutes.map { it.name })
.containsExactly(
"Bairnsdale - Melbourne via Sale & Traralgon",
"Traralgon - Melbourne via Morwell & Moe & Pakenham",
)
.inOrder()
}

@Test
fun `requesting metro at a shared stop_id keeps only metro routes`() {
// Mirror of the #175 case from the other side: tapping the metro Richmond pin must show
// only the metro lines, not the co-located V/Line services.
val response =
StopResponseDto(
stop =
StopDetailsDtoMother.aStopDetailsDto()
.withRouteType(0)
.withRoutes(
listOf(
RouteDto(routeId = 1, routeName = "Alamein", routeNumber = "", routeType = 0),
RouteDto(
routeId = 3,
routeName = "Bairnsdale - Melbourne via Sale & Traralgon",
routeNumber = "",
routeType = 3,
),
),
)
.build(),
)

val detail = response.toDomain(requestedRouteType = RouteType.Train)!!

assertThat(detail.servingRoutes.map { it.name }).containsExactly("Alamein")
}
}