fix(TripDetails): Filter out already-visited stops before computing vehicle status#3293
Open
joshlarson wants to merge 6 commits into
Open
fix(TripDetails): Filter out already-visited stops before computing vehicle status#3293joshlarson wants to merge 6 commits into
joshlarson wants to merge 6 commits into
Conversation
…ops with past schedules
…ic stop_sequence setup
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
The bug is as follows:
trip_details/1into showing:scheduled_to_depart.Asana Ticket: 📆🔎🐞 Hide stops already visited from trip details when vehicle info is available
Implementation
(Apologies for the kind of messy code diff! I had to make a bunch of changes kind of all at once, and I couldn't think of a way of splitting it out into separate commits without having that splitting take forever.)
At a high level, the fix is to filter out upstream
predicted_schedulesbefore computingvehicle_info. In practice, this turned out to be surprisingly involved, because the way we were checking vehicle location for other parts of trip-details computation usedvehicle_info, which is the thing that needed to change! So I needed to move the "drop upstream prediction/schedule" call earlier in the pipeline, usevehiclefor that, and make sure thatvehicleis actually set correctly (turns out... it wasn't!)A few random other notes:
drop_first_trip_stop_if_vehicle_is_stoppedout ofdrop_predicted_schedules_before_current_station... they're already different things,drop_first_trip_stop_if_vehicle_is_stoppedis a lot easier withvehicle_inforather thanvehicle, andvehicle_infoonly relies ondrop_predicted_schedules_before_current_station.predicted_schedulewas just a schedule, thenUpcomingDepartures.Processor.trip_details/1was getting anilvehicle instead of the vehicle from the predictions. I updated it to look up the vehicle frompredictions, rather thanpredicted_schedules.TripDetailsTestdidn't have realistic stop sequence setup. I@tag :skip'ed those tests for now, because I think the Right™ call for them is to give them the same treatment asUpcomingDeparturesTest- i.e. usePredictedScheduleHelper, and I didn't want to delay the bugfix for long enough to actually do that (I did create a follow-up ticket for that).Screenshots
How to test
If you run ☝️ snippet in iEX or LiveBook (attached to Dotcom), then, it'll tell you which trips are underway with origin departures scheduled in the future. Those are the trips that show this bug. For instance, when I took the screenshot above, this snippet output something like
So in order to see the bug/solution in action, you look at a
Green-Edepartures page at Fenwood or any stop downstream of Fenwood (but not too far downstream or else this trip won't be in the first five).(Annoyingly, as I type this out, there are no trips running that early, so I'm seeing
[])