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.

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

Conversation

EmmaSimon
Copy link
Contributor

Summary

Ticket: Nearby transit shows non-CR routes between 0.5 and 1 mile away

The nearby transit backend is separately fetching CR stops with a 1 mile radius, and any other stops with a 0.5 mile radius, but they're all passed to the frontend without any associated distance, so nearby transit is seeing subway or bus routes that are served by those more distant CR stops, and is displaying them even though they fall outside of 0.5 miles. This adds a filter to remove any routes over that threshold unless they're CR.

Testing

Added a test to make sure distant subway/bus routes are filtered out.

The backend uses a different distance for nearby commuter rail vs every
other mode, but when non-CR routes were also being served by those CR
stations, they were also showing up in nearby transit. This was made
worse by hoisting the subway routes up to the top, you could get weird
results where it was showing stops that were actually quite far as one
of the closest routes.
@@ -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.

@EmmaSimon EmmaSimon marked this pull request as ready for review April 10, 2024 23:04
@EmmaSimon EmmaSimon requested a review from a team as a code owner April 10, 2024 23:04
@EmmaSimon EmmaSimon requested a review from KaylaBrady April 10, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants