fix: Handle platform closures #2107
Conversation
lib/screens/alerts/alert.ex
Outdated
| Enum.reject( | ||
| informed_entities, | ||
| &(String.starts_with?(&1.stop, "place-") or &1.route_type != 1) | ||
| ) |
There was a problem hiding this comment.
question: is there a way to rewrite this as a filter and explicitly include things instead of excluding things we don't want?
There was a problem hiding this comment.
Yeah sure. Platform IDs will always be integers so I can check that stop is an integer and route_type == 1.
There was a problem hiding this comment.
We're very much not guaranteed that platform stop IDs will always look like integers — it would be better to look at the location_type of the stop, which tells you explicitly whether a stop is a specific "stopping location" (type 0, a.k.a. a platform) or not.
There was a problem hiding this comment.
(That said, all current subway platform stop IDs do look like integers, and at this point it's possible enough other teams have made this assumption — contra the documented best practice of treating all IDs as opaque — that introducing a new platform stop ID that didn't look like an integer would be considered a breaking change.)
There was a problem hiding this comment.
Hmm yeah I see your concern. The issue is location_type is not added to informed_entities. I could add a bit more stability by passing the list of subway platforms to the function and find a union of the lists. The only problem with that is it makes the function dependent on a separate API call. That's not really a problem in this PR considering we are guaranteed to have that list when making the call. It might be better if I turn this into a Stop module function. Then I can just map informed_entities into their IDs and do the union that way.
There was a problem hiding this comment.
The only problem with that is it makes the function dependent on a separate API call.
Yep, that's pretty much the only fully-correct way to do this, as there's no way to fetch related resources on alert informed entities as part of the same call. Fetch alert -> fetch stops matching the IDs in the alert -> get attributes of stops. Of course, we are already doing a stop fetch here to get the full set of child stops for the parent station, so this doesn't strictly need to introduce a new API call, just change the parameters of an existing one.
That said, the shortcut of assuming place- is a parent station (and that parent stations and child stops are the only kinds of stop that will appear in a station closure alert) is probably going to be durable enough, kinda like how we assume a route is Commuter Rail iff the ID starts with CR-. It pains me a tiny bit, though.
Co-authored-by: sloane <[email protected]>
Coverage of commit
|
lib/screens/v2/candidate_generator/widgets/reconstructed_alert.ex
Outdated
Show resolved
Hide resolved
lib/screens/alerts/alert.ex
Outdated
| def informed_parent_stations(%__MODULE__{ | ||
| informed_entities: informed_entities | ||
| }) do | ||
| Enum.filter(informed_entities, &String.starts_with?(&1.stop, "place-")) |
There was a problem hiding this comment.
It appears we already have InformedEntity.parent_station? for this.
Coverage of commit
|
Coverage of commit
|
| @@ -360,6 +360,9 @@ defmodule Screens.V2.WidgetInstance.SubwayStatus do | |||
| Map.merge(%{route_pill: serialize_route_pill(route_id)}, serialize_alert(alert, route_id)) | |||
| end | |||
|
|
|||
| @spec serialize_alert(Alert.t() | nil, String.t()) :: alert() | |||
There was a problem hiding this comment.
We could use the type Route.id() here to be clear about the meaning (and same below).
Coverage of commit
|
Coverage of commit
|
Asana task: Handle single-child-stop alerts on pre-fares
This PR adds support for platform closures on Pre-Fare alerts and SubwayStatus. A few key changes:
Note: I definitely want to get code review started to catch an obvious mistakes, but I think a lot of this testing needs to be in dev-green using a variety of real alerts. I think I covered all the bases in tests but will let Dave work out any others. Feel free to do the same.