refactor: Use platform_name from Alert.InformedEntity for partial closures#2970
refactor: Use platform_name from Alert.InformedEntity for partial closures#2970robbie-sundstrom wants to merge 4 commits intomainfrom
Conversation
| @@ -169,13 +169,12 @@ defmodule Screens.V2.CandidateGenerator.Widgets.ReconstructedAlert do | |||
| with [informed_parent_station] <- Alert.informed_parent_stations(alert), | |||
| platforms <- fetch_subway_platforms_for_stop_fn.(informed_parent_station.stop.id), | |||
There was a problem hiding this comment.
I think we can go further here — on this line we are using the API to separately fetch the child stops of the informed station, but we should already have that data fetched into the parent station struct itself. We should be able to do away with this fetch_subway_platforms_for_stop_fn entirely.
There was a problem hiding this comment.
We don't get the child_stops of the parent station within the Alert.InformedEntity fetch, which we need to determine if it's a partial closure. I could modify the API call to include stops.child_stops, which would allow us to get rid of this call. I think that shouldn't be too tricky and am looking at that now
There was a problem hiding this comment.
Technically isn't the point of this function just to get the platform names that are affected by the alert? If that's the case, we shouldn't even need to fetch the child-stops relationship. The platforms affected by the alert should be represented as child stop informed entities, which now are fully-hydrated Stop structs, so we could get their platform names directly.
There was a problem hiding this comment.
Not exactly -- both this and the Subway Status all_platforms_at_informed_stations are a list of child_stop Stop Structs, which include platform names. We fetch the full list of platforms baed on the informed_parent_station in both cases so that we can compare that list of child_stops with the stops listed in the InformedEntity.
There was a problem hiding this comment.
I see, I think I was missing the part where Alert.station_closure_type does require the full set of platforms.
| informed_stop_ids = Enum.map(informed_entities, & &1.stop.id) | ||
|
|
||
| platform_names = | ||
| all_platforms_at_informed_stations |
There was a problem hiding this comment.
I'm sure we can do this as part of a future change, but just noting that it would be nice to entirely get rid of all_platforms_at_informed_stations (and thus the context field as a whole since that's the only thing in it), given this information is now knowable from the alert data itself; this should also eliminate a separate data fetch.
|
|
||
| platform_names = | ||
| t.alert.informed_entities | ||
| |> Enum.filter(&(&1.stop.id in child_stop_ids and &1.stop.platform_name)) |
There was a problem hiding this comment.
This has to iterate child_stop_ids for every entry in t.alert.informed_entities, making this an N² operation. Admittedly both "Ns" are small, but we could address this easily enough by introducing a MapSet (or two MapSets, and then intersecting them, instead of doing a filter).
| @type fetch :: (options() -> result()) | ||
|
|
||
| @base_includes ~w[facilities stops] | ||
| @base_includes ~w[facilities stops stops.child_stops] |
There was a problem hiding this comment.
stops.child_stops should potentially not be included with the @base_includes
2 participants
Asana task: Use platform_name from Alert IE for single-side closure alerts
With the change to Populate Alerts IE Stop data, we can now simplify our logic in some places when processing alerts.