feat(StopPageRedesign): "no service" via route patterns

[thecristen]

Summary of changes

Asana Ticket: No ticket

This gave me flashbacks to earlier in this page's development when we were struggling to distinguish "doesn't serve this stop" vs "no schedules or predictions for this stop." But I think I figured out an okay approach.

Background

The changes from #1762 went too far, and ends up leaving us unable to display any route card when no service is running. It turns out that produces a really jarring display on certain bus stop pages.

For example, there are several bus routes which only operate on weekdays -- visiting the stop pages serving these routes on the weekends will greet you with a cryptic blank display:

(screens from a technically unrelated bug report, but that's also the blank display you get in the scenario described above – I failed to take a screenshot of a relevant stop over the weekend 😅)

So I wanted to make another attempt at showing route cards in the case where transit service isn't available today.

It was somewhat straightforward – we needed to keep the list of route patterns intact, instead of prematurely removing them before grouping them by route and headsign. Click for the pseudocode description:

# Before - a headsign for a route pattern associated with 
# a service not running today will be removed entirely
all_route_patterns
|> remove_not_todays
|> group_by_route_id_and_headsign

# After - a headsign for a route pattern associated with a service not
# running today will be associated with a route pattern list that's empty
all_route_patterns
|> group_by_route_id_and_headsign
|> Enum.map(fn group ->
    group 
    |> remove_not_todays
end)

Highlights

I tried to split this into smaller, incremental commits that each describe an incremental change. But this goal inspired modest changes on both the backend and frontend.

Backend

• Moved the Enum(&not_serving_today?/1) operation in the grouped route patterns endpoint to operate over a list of route patterns for each route, instead of removing route patterns before we group the list by route+headsign. That way we could keep all routes+headsigns as part of the response, even if they're not associated with today's service.
• Modified the grouped route patterns response to include, for each route+headsign, the relevant direction ID, instead of parsing the direction ID from the list of route patterns in the frontend. That way the direction ID won't be missing if that list of route patterns is empty.

Less relevant changes:

• Using Stream in favor of Enum when chaining operations to group the route patterns, hoping for a minor performance boost. Not a big deal, but the grouped route patterns endpoint is relatively slow.
• Removed services with the ID "canonical" for being considered as today's valid service. Those are meant for reference and are not associated with any day.

Frontend

Most changes are adapting to the changed backend.

• Adds a "No service today" text if the route+headsign's list of route patterns is an empty array
• Elaborates this to "No service today, Oct 30" (for example) when clicking the headsign to view the <DepartureList />.
• Adds a helper function to generate that "Oct 30" part.

Reviewing the result

• It'll be easier to spot on weekends when Weekday-only buses go out of service. But we can also preview this on stops along the Foxboro Event Service route, and at stops served by the CapeFLYER Hyannis route -- both of which are not in service currently. These routes can both be seen at South Station.

• Otherwise, we can manually emulate the no-service conditions by using React Dev Tools to set the hasService prop to false. For example:

---

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.

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1205864682150635

[KaylaBrady]

Excited for this change! 🎉 Added a few questions & comments. Looks like some checks are failing too.
Seeing 282 elixir unit test failures, which is 1 more failing than the last merge to main, but didn't dig into which one is the difference.

[KaylaBrady]

question: Do these backend changes need to be deployed separately from the frontend changes in a backwards compatible way? What would a user on the stop page see during a deploy if they have the new frontend and the old backend (or the new frontend and the old backend)?

[thecristen]

we don't do separately versioned backends and frontends here, they'll be deployed together 😉

[KaylaBrady]

I may be missing a nuance here, but my previous understanding on Skate was that this would be considered a breaking change. Even though the frontend and backends are deployed together, there will be a period of time where some ecs instances have the old backend and some instances have the new backend, during which someone using the stop page has no control over whether the new or old instances are hit, so there could be a mismatch between the data that is returned and what the frontend expects. Is that situation avoided with how dotcom is set up?

[thecristen]

we've never thought to mitigate this before! but when you explain it here it makes sense...aah 🫠 🫠 🫠 🫠

[thecristen]

I do think that problem might be less frequently encountered on Dotcom, because folks don't have it open very long per session (just a few minutes -- I presume folks working in Skate maintain a much longer working session during typical use). I'll have to figure out how to handle that more systematically but for the sake of this PR will consider it out of scope. But thanks so much for raising this point!!

[KaylaBrady]

I wonder if because sessions are shorter on Dotcom this could be more of an issue, because users might be hopping between things more in a short duration. Here is a search in splunk that shows how many ips hit this endpoint more than once in the last X minutes. I'm seeing 24 / 85 ips that hit this endpoint in the lat 5 minutes hit it more than once. If that is representative of traffic during a deploy, then ~25% of users on the stop page could receive errors on the page during a deploy.

A way we typically handled this issue on Skate is to split this work over multiple PRs, potentially by adding a new backend endpoint in one PR, then removing the old endpoint once the frontend has been updated to stop calling it. Would that kind of approach be feasible here?

[KaylaBrady]

question: Did switching to stream improving performance? I find the use of Enum.group_by more readable if it performs roughly the same. Potentially Enum.group_by/3 would help, where the 3rd param is a function to transform the value could cut down on some of the passes through the list.

related non-blocking thought: Would it help simplify this logic if by_route_and_headsign was something like %{Route.id_t() => [headsign_info], where headsign_info also contains a headsign field? I'm not sure if that would have negative affects on the frontend side of things though

[thecristen]

ooh I forgot about Enum.group_by/3, I'll give this a try

[KaylaBrady]

suggestion: Consider renaming this - with_headsign_info reads to me like it is only adding headsign information to the route patterns, but this is also filtering & adding polylines

[thecristen]

Sorry about the failing tests -- those particular fails have been popping up more here, I think it's from a race condition with how the workflow steps are building the application before running the tests 😵‍💫 Anyway, thanks @KaylaBrady for the many great points, I've added some revisions here.

[KaylaBrady]

suggestion(non-blocking): this could save a couple of passes through the list of routes

Suggested change

	|> Enum.group_by(& &1.route_id)
	|> Enum.map(&with_headsign_groups/1)
	|> Map.new()
	|> Enum.group_by(& &1.route_id, &with_headsign_groups/1)

[KaylaBrady]

suggestion (non-blocking): Enum.reject(&not_serving_today?/1) still seems a bit like it doesn't match the name of this function. Maybe this filtering could be done as a separate step in with_headsign_groups?

  defp with_headsign_groups(route_patterns) do
    route_patterns
    |> Enum.group_by(& &1.headsign)
    |> Enum.reject(&not_serving_today?/1)
    |> Enum.map(&with_annotation/1)
    |> Map.new()
  end

[KaylaBrady]

I wonder if because sessions are shorter on Dotcom this could be more of an issue, because users might be hopping between things more in a short duration. Here is a search in splunk that shows how many ips hit this endpoint more than once in the last X minutes. I'm seeing 24 / 85 ips that hit this endpoint in the lat 5 minutes hit it more than once. If that is representative of traffic during a deploy, then ~25% of users on the stop page could receive errors on the page during a deploy.

A way we typically handled this issue on Skate is to split this work over multiple PRs, potentially by adding a new backend endpoint in one PR, then removing the old endpoint once the frontend has been updated to stop calling it. Would that kind of approach be feasible here?

[KaylaBrady]

Approved with offline discussion about preventing breaking changes in the future!