Skip to content
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

fix: Fix issue with non-CR routes showing up in nearby transit for stops that also serve distant CR #126

Closed
wants to merge 4 commits into from
Closed

fix: Fix issue with non-CR routes showing up in nearby transit for stops that also serve distant CR #126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f28f0a3
fix: Don't display non-CR routes further than .5mi
EmmaSimon Apr 10, 2024
65dd143
test: Add test for filtering out distant routes
EmmaSimon Apr 10, 2024
12c58b5
Merge branch 'main' into es-fix-nearby-mode-distances
EmmaSimon Apr 10, 2024
9ca73ea
test: Fix iOS tests to be within stop range
EmmaSimon Apr 11, 2024
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
34 changes: 17 additions & 17 deletions iosApp/iosAppTests/Views/NearbyTransitViewTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ final class NearbyTransitViewTests: XCTestCase {

var sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down Expand Up @@ -148,7 +148,7 @@ final class NearbyTransitViewTests: XCTestCase {
}
}
}
loadedLocation = CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78)
loadedLocation = CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4)
}

override func getNearby(global _: GlobalResponse, location _: CLLocationCoordinate2D) async {}
Expand All @@ -157,7 +157,7 @@ final class NearbyTransitViewTests: XCTestCase {
func testRoutePatternsGroupedByRouteAndStop() throws {
let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down Expand Up @@ -243,7 +243,7 @@ final class NearbyTransitViewTests: XCTestCase {

let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down Expand Up @@ -322,7 +322,7 @@ final class NearbyTransitViewTests: XCTestCase {
testFormatter.timeStyle = .short
let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down Expand Up @@ -382,7 +382,7 @@ final class NearbyTransitViewTests: XCTestCase {
let predictionsFetcher = FakePredictionsFetcher(sawmillAtWalshExpectation: sawmillAtWalshExpectation, lechmereExpectation: lechmereExpectation)
let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand All @@ -398,7 +398,7 @@ final class NearbyTransitViewTests: XCTestCase {

nearbyFetcher.nearbyByRouteAndStop = NearbyStaticData.companion.build { builder in
builder.route(route: nearbyFetcher.nearbyByRouteAndStop!.data[0].route) { builder in
let lechmere = Stop(id: "place-lech", latitude: 90.12, longitude: 34.56, name: "Lechmere", locationType: .station, parentStationId: nil, childStopIds: [])
let lechmere = Stop(id: "place-lech", latitude: 1.2, longitude: 3.4, name: "Lechmere", locationType: .station, parentStationId: nil, childStopIds: [])
builder.stop(stop: lechmere) { _ in
}
}
Expand All @@ -413,7 +413,7 @@ final class NearbyTransitViewTests: XCTestCase {
let predictionsFetcher = PredictionsFetcher(socket: MockSocket())
let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down Expand Up @@ -474,7 +474,7 @@ final class NearbyTransitViewTests: XCTestCase {
let predictionsFetcher = FakePredictionsFetcher(joinExpectation: joinExpectation, leaveExpectation: leaveExpectation)
let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down Expand Up @@ -519,7 +519,7 @@ final class NearbyTransitViewTests: XCTestCase {
let predictionsFetcher = FakePredictionsFetcher(joinExpectation: joinExpectation, leaveExpectation: leaveExpectation)
let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down Expand Up @@ -567,7 +567,7 @@ final class NearbyTransitViewTests: XCTestCase {
let predictionsFetcher = FakePredictionsFetcher(joinExpectation: joinExpectation, leaveExpectation: leaveExpectation)
let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down Expand Up @@ -598,7 +598,7 @@ final class NearbyTransitViewTests: XCTestCase {

let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand All @@ -615,7 +615,7 @@ final class NearbyTransitViewTests: XCTestCase {
class FakeNearbyFetcher: NearbyFetcher {
init() {
super.init(backend: IdleBackend())
loadedLocation = CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78)
loadedLocation = CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4)
nearbyByRouteAndStop = NearbyStaticData(data: [])
}
}
Expand All @@ -628,7 +628,7 @@ final class NearbyTransitViewTests: XCTestCase {

let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down Expand Up @@ -660,7 +660,7 @@ final class NearbyTransitViewTests: XCTestCase {
getNearbyExpectation.expectedFulfillmentCount = 2

let fakeFetcher = FakeNearbyFetcher(getNearbyExpectation: getNearbyExpectation)
let currentLocation = CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78)
let currentLocation = CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4)
let locationProvider: NearbyTransitLocationProvider = .init(
currentLocation: currentLocation,
cameraLocation: ViewportProvider.defaultCenter,
Expand All @@ -679,7 +679,7 @@ final class NearbyTransitViewTests: XCTestCase {
alertsFetcher: .init(socket: MockSocket())
)

let newLocation = CLLocationCoordinate2D(latitude: 0.0, longitude: 0.0)
let newLocation = CLLocationCoordinate2D(latitude: 1.21, longitude: 3.41)

let hasAppeared = sut.inspection.inspect(after: 0.2) { view in
XCTAssertEqual(try view.actualView().nearbyFetcher.loadedLocation, currentLocation)
Expand Down Expand Up @@ -727,7 +727,7 @@ final class NearbyTransitViewTests: XCTestCase {

let sut = NearbyTransitView(
locationProvider: .init(
currentLocation: CLLocationCoordinate2D(latitude: 12.34, longitude: -56.78),
currentLocation: CLLocationCoordinate2D(latitude: 1.2, longitude: 3.4),
cameraLocation: ViewportProvider.defaultCenter,
isFollowing: true
),
Expand Down
14 changes: 11 additions & 3 deletions shared/src/commonMain/kotlin/com/mbta/tid/mbta_app/model/StopAssociatedRoute.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import com.mbta.tid.mbta_app.model.response.PredictionsStreamDataResponse
import com.mbta.tid.mbta_app.model.response.ScheduleResponse
import io.github.dellisd.spatialk.geojson.Position
import io.github.dellisd.spatialk.turf.ExperimentalTurfApi
import io.github.dellisd.spatialk.turf.Units
import io.github.dellisd.spatialk.turf.distance
import kotlin.time.Duration.Companion.minutes
import kotlinx.datetime.Instant
Expand All @@ -13,6 +14,8 @@ data class UpcomingTripKey(val routeId: String, val headsign: String, val stopId

typealias UpcomingTripsMap = Map<UpcomingTripKey, List<UpcomingTrip>>

const val COMMUTER_NEARBY_THRESHOLD = 0.5
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates a value between the frontend and the backend, which seems inappropriate. How would you feel about a follow up where we remove 0.5 mile-ish threshold from the backend, and only make a single stops request with a 1 mile radius, then only have this distance threshold here. It's still weird to have the longer radius there and the shorter one here, but I think it's still better than this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it seems a little weird to have a longer radius filter on the backend and a shorter one on the frontend.

What do you think of keeping the filtering entirely on the backend? We could filter cr_included_stops to be only CR stops based on Stop.vehicle_type, which is a field we aren't currently parsing.

That was one approach we discussed at refinement but didn't capture in detail in the ticket, sorry for the missing context there!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! mbta/mobile_app_backend#116 I was initially going to implement it on the backend but couldn't see a clear path to doing so. I didn't realize that vehicle_type was a thing, that can maybe simplify some stuff on the frontend now that we have access to it.


/**
* @property patterns [RoutePattern] listed in ascending order based on [RoutePattern.sortOrder]
* @property upcomingTrips Every [UpcomingTrip] for the [Stop] in the containing [PatternsByStop]
Expand Down Expand Up @@ -174,7 +177,7 @@ data class PatternsByStop(
)

@OptIn(ExperimentalTurfApi::class)
fun distanceFrom(position: Position) = distance(position, this.position)
fun distanceFrom(position: Position) = distance(position, this.position, units = Units.Miles)

fun allUpcomingTrips(): List<UpcomingTrip> =
this.patternsByHeadsign.flatMap { it.upcomingTrips ?: emptyList() }.sorted()
Expand Down Expand Up @@ -207,7 +210,8 @@ data class StopAssociatedRoute(
)

@OptIn(ExperimentalTurfApi::class)
fun distanceFrom(position: Position) = distance(position, patternsByStop.first().position)
fun distanceFrom(position: Position) =
distance(position, patternsByStop.first().position, units = Units.Miles)
}

/**
Expand Down Expand Up @@ -254,7 +258,11 @@ fun NearbyStaticData.withRealtimeInfo(
activeRelevantAlerts
)
}
.filterNot { it.patternsByStop.isEmpty() }
.filter {
it.patternsByStop.isNotEmpty() &&
(it.route.type == RouteType.COMMUTER_RAIL ||
it.distanceFrom(sortByDistanceFrom) < COMMUTER_NEARBY_THRESHOLD)
}
.sortedWith(compareBy({ it.distanceFrom(sortByDistanceFrom) }, { it.route }))
.sortedWith(compareBy(Route.subwayFirstComparator) { it.route })
}
99 changes: 93 additions & 6 deletions shared/src/commonTest/kotlin/com/mbta/tid/mbta_app/model/NearbyResponseTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,18 +807,18 @@ class NearbyResponseTest {
val closeBusStop = objects.stop()
val farBusStop =
objects.stop {
latitude = closeBusStop.latitude + 0.1
longitude = closeBusStop.longitude + 0.1
latitude = closeBusStop.latitude + 0.001
longitude = closeBusStop.longitude + 0.001
}
val closeSubwayStop =
objects.stop {
latitude = closeBusStop.latitude + 0.2
longitude = closeBusStop.longitude + 0.2
latitude = closeBusStop.latitude + 0.002
longitude = closeBusStop.longitude + 0.002
}
val farSubwayStop =
objects.stop {
latitude = closeBusStop.latitude + 0.3
longitude = closeBusStop.longitude + 0.3
latitude = closeBusStop.latitude + 0.003
longitude = closeBusStop.longitude + 0.003
}

val closeBusRoute = objects.route { type = RouteType.BUS }
Expand Down Expand Up @@ -915,6 +915,93 @@ class NearbyResponseTest {
)
}

@Test
fun `withRealtimeInfo filters out non-CR routes served by distant CR stations`() {
val objects = ObjectCollectionBuilder()

val closeSubwayStop = objects.stop()
val farCRSubwayStop =
objects.stop {
latitude = closeSubwayStop.latitude + 0.04
longitude = closeSubwayStop.longitude + 0.04
}

val closeSubwayRoute = objects.route { type = RouteType.LIGHT_RAIL }
val farSubwayRoute = objects.route { type = RouteType.HEAVY_RAIL }
val farCommuterRoute = objects.route { type = RouteType.COMMUTER_RAIL }

val closeSubwayPattern =
objects.routePattern(closeSubwayRoute) {
sortOrder = 1
representativeTrip { headsign = "Magoun Square" }
}
val farSubwayPattern =
objects.routePattern(farSubwayRoute) {
sortOrder = 1
representativeTrip { headsign = "Alewife" }
}
val farCommuterPattern =
objects.routePattern(farCommuterRoute) {
sortOrder = 1
representativeTrip { headsign = "Alewife" }
}

val staticData =
NearbyStaticData.build {
route(farSubwayRoute) {
stop(farCRSubwayStop) { headsign("Alewife", listOf(farSubwayPattern)) }
}
route(farCommuterRoute) {
stop(farCRSubwayStop) { headsign("Alewife", listOf(farCommuterPattern)) }
}
route(closeSubwayRoute) {
stop(closeSubwayStop) { headsign("Magoun Square", listOf(closeSubwayPattern)) }
}
}

val time = Instant.parse("2024-02-21T09:30:08-05:00")

// close subway prediction
objects.prediction {
arrivalTime = time
departureTime = time
routeId = closeSubwayRoute.id
stopId = closeSubwayStop.id
tripId = closeSubwayPattern.representativeTripId
}

// far subway prediction
objects.prediction {
arrivalTime = time
departureTime = time
routeId = farSubwayRoute.id
stopId = farCRSubwayStop.id
tripId = farSubwayPattern.representativeTripId
}

// far commuter prediction
objects.prediction {
arrivalTime = time
departureTime = time
routeId = farCommuterRoute.id
stopId = farCRSubwayStop.id
tripId = farCommuterPattern.representativeTripId
}

val realtimeRoutesSorted =
staticData.withRealtimeInfo(
sortByDistanceFrom = closeSubwayStop.position,
predictions = PredictionsStreamDataResponse(objects),
filterAtTime = time,
schedules = ScheduleResponse(objects),
alerts = null,
)
assertEquals(
listOf(closeSubwayRoute, farCommuterRoute),
realtimeRoutesSorted.map { it.route }
)
}

@Test
fun `withRealtimeInfo handles parent stops`() {
val objects = ObjectCollectionBuilder()
Expand Down