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

feat(TripPlanner): render Massport and Logan Exp. icons and more #2123

Merged
merged 6 commits into from
Jul 11, 2024

Conversation

thecristen
Copy link
Collaborator

Warning

Because of the high number of files, I recommended reviewing this one commit at a time. Many of the commit messages include a more detailed description of what's going on in each.

Previewing changes in the trip planner

  • Uncheck the accessible checkbox
  • Select bus mode only for best results
  • Try airport itineraries to/from any of the five Logan Express termini - I recommend pasting in exact addresses.
  • Going between the airport and various points in Revere or Chelsea often result in Massport routes being used

Summary of changes

Asana Ticket: Trip Planner display of itineraries using Logan Express & Massport Shuttle uses incorrect stop names

Important

We import the Massport GTFS into our OTP build, so that it can give us results which use Massport and Logan Express routes. My primary goal was to handle ingesting itineraries using those routes and stops in a more elegant way than we do now.

So I ended up majorly refactoring how Dotcom was parsing responses from OpenTripPlanner. Honestly I don't think it's perfect yet, but I ended up making all these other PRs to get me here... 😆

Anyway. Our prior logic responded to those Massport routes by putting together a %Routes.Route{} struct with custom_route?: true. This PR makes that ever so slightly more useful by populating our new external_agency_field with the relevant agency name ("Massport" or "Logan Express" in this case). Now downstream logic can use that field's value to render specific icons, colors, fare information, and so on!

Note

This leverages an open_trip_planner_client change I made, which returns itineraries as typed structs rather than plain Elixir maps. Sadly I wasn't able to leverage OTP's own GraphQL schema definition (Absinthe bug report), and I ended up creating my own model of the itinerary data. Might it become obsolete one day!

This made the result more straightforward to parse, and more fun to create an OTP-specific test factory, which was also released as part of those changes.

Many trip planner and trip planner-adjacent unit tests are refactored to use the rewritten test factory for generating trip plan itineraries, legs, and so on. Some of the updated tests could likely be cleaned up further. But I was mostly focused on "re-enable the test (if skipped) / un-break the test (if broken) and get it working".

Important

My secondary goal was to reduce the amount of supplemental data needed to render itineraries.

I wanted to see if I could piece together everything needed for the trip planner's view without invoking extra Stops.Repo.get/1 or Routes.Repo.get/1 calls. The open_trip_planner_client update also adds the name field to place stops and leg intermediate stops, and the type, color, textColor, desc fields to leg routes. It mostly worked! This also made it much easier to rewrite/revise the tests.


General checks

  • Are the changes organized into self-contained commits with descriptive and well-formatted commit messages? This is a good practice that can facilitate easier reviews.
  • Testing. Do the changes include relevant passing updates to tests? This includes updating screenshots. Preferably tests are run locally to verify that there are no test failures created by these changes, before opening a PR.
  • Tech debt. Have you checked for tech debt you can address in the area you're working in? This can be a good time to address small issues, or create Asana tickets for larger issues.

- cleanup the config values
- rename TripPlan.Api.OpenTripPlanner to TripPlanner.OpenTripPlanner and
-   make it more responsible for fetching and parsing OTP results by moving relevant functionality from Dotcom.TripPlan.Query
- add some testing
- the upgraded OTP client returns more data, which can be parsed here in lieu of making our own Routes.Repo or Stops.Repo calls to supplement the data.
- use Agency info to populate route.external_agency_name
- special parsing for route names for Logan Express and Massport
- overwrite route colors for Logan Express routes because their GTFS doesn't match their website branding (!)
- convert miles and minutes units here instead of in the views

refactor(TripPlanner.FarePasses): extract the fare/passes computing into its own module
- add Massport/Logan Express fares
fix everything else to work with the following changes:
- chore(TripPlan.IntermediateStop): use stop struct instead of stop_id
- chore(TripPlan.Location): use stop struct instead of stop_id
- chore(TripPlan.NamedPosition): use stop struct instead of stop_id
- chore(TripPlan.TransitDetail): use route struct instead of route_id
…tions

- fix(Fares): correct fare atom for rail replacement buses
@thecristen thecristen requested a review from a team as a code owner July 9, 2024 21:06
@thecristen thecristen requested a review from anthonyshull July 9, 2024 21:06
@thecristen thecristen merged commit 1f0e83a into main Jul 11, 2024
26 checks passed
@thecristen thecristen deleted the cbj/trip-planner-parsing branch July 11, 2024 14:41
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