Planned disruptions logic #2373

anthonyshull · 2025-02-10T19:14:44Z

https://app.asana.com/0/555089885850811/1209323323089311/f

This is the backend logic for planned disruptions. It also includes today's disruptions for the system status component.

lib/alerts.ex

thecristen

There's a lot here! Thanks for adding the tests and supporting behaviours and mocks.

thecristen · 2025-02-11T16:33:49Z

test/dotcom/alerts/disruptions/subway_test.exs

+      service_range_after_next_week: 0
+    ]
+
+  import Mox


Don't forget the setup :verify_on_exit! cleanup! ✨

thecristen · 2025-02-11T16:38:25Z

lib/dotcom/routes.ex

+  A collection of functions that help to work with routes in a unified way.
+  """
+
+  @subway_route_ids ["Blue", "Green", "Orange", "Red"]


I can see a future where we work branches/lines/routes and relationships into here in some way.

Couple of thoughts on this line of code in particular:

Idea: Would it be worth having a type for line ID that only allows these four values? Then the type of subway_route_ids would be [Dotcom.Routes.line_id()].

Also, the name subway_route_ids() is misleading, because "Green" is not a Route ID - the subway Route ID's are "Red", "Mattapan", "Orange", "Green-B", "Green-C", "Green-D", "Green-E", and "Blue". I'd call this subway_line_ids or subway_lines.

And finally - love extracting this and putting it somewhere clean and universal!

Eventually, we won't need this because we'll just be able to query routes by type.

And then query alerts by whatever attributes.

thecristen · 2025-02-11T16:50:51Z

lib/dotcom/alerts/disruptions/subway.ex

+  @doc """
+  Disruptions that occur during today's service range.
+  """
+  @spec todays_disruptions() :: %{Utils.ServiceDateTime.named_service_range() => [Alert.t()]}


technically just %{today: [Alert.t()]}

joshlarson

A lot of this looks great! Nice to see a bunch of kind of universal concerns moving into better homes.

I have two comments that are blocking, related to the actual functionality:

It looks like todays_disruptions includes alerts that shouldn't be included, and the tests don't make it clear what happens in that circumstance.
The way subway_route_ids() looks like it will miss all green line and Mattapan alerts.

joshlarson · 2025-02-11T16:58:58Z

lib/dotcom/alerts.ex

+  @doc """
+  Returns a list of the alert effects that are considered service-impacting.
+  """
+  @spec service_impacting_effects() :: [atom()]


Suggestion: I think you could move the type from SystemStatus.Alerts here.

(When you started on this PR, I don't think the PR introducing that type had been merged yet)

joshlarson · 2025-02-11T17:10:10Z

lib/dotcom/alerts/disruptions/subway.ex

+  Disruptions that occur during today's service range.
+  """
+  @spec todays_disruptions() :: %{Utils.ServiceDateTime.named_service_range() => [Alert.t()]}
+  def todays_disruptions() do


I may be reading this wrong, but it looks like this will include alerts from earlier today, even if their effect periods have ended. If so, we should double-check the requirements, because I had thought that we only want to include alerts if they're either currently active or will be active later in the day.

Either way, I think it would be good to have a test explicitly highlighting the "earlier-today" alerts behavior.

This is grabbing today's disruptions. If the view doesn't want ones that are earlier than now they can remove them. The intention here isn't to do all of the work for the only current consumer. They will still have to do grouping and sorting by line if they want it.

And, it will only get those alerts if the effect period ended after the start of the service day. So, the disruption effected 'today'.

joshlarson · 2025-02-11T17:11:33Z

lib/dotcom/routes.ex

+  @doc """
+  Returns a list of route ids for all subway routes.
+  """
+  @spec subway_route_ids() :: [String.t()]


Praise This is a better place for this than where it was - nice to see this attr gradually migrating to its optimal home 🏠

joshlarson · 2025-02-11T17:13:36Z

lib/dotcom/routes.ex

+  A collection of functions that help to work with routes in a unified way.
+  """
+
+  @subway_route_ids ["Blue", "Green", "Orange", "Red"]


Couple of thoughts on this line of code in particular:

Idea: Would it be worth having a type for line ID that only allows these four values? Then the type of subway_route_ids would be [Dotcom.Routes.line_id()].

Also, the name subway_route_ids() is misleading, because "Green" is not a Route ID - the subway Route ID's are "Red", "Mattapan", "Orange", "Green-B", "Green-C", "Green-D", "Green-E", and "Blue". I'd call this subway_line_ids or subway_lines.

And finally - love extracting this and putting it somewhere clean and universal!

joshlarson · 2025-02-11T17:14:55Z

lib/dotcom/system_status/alerts.ex

@@ -92,7 +88,7 @@ defmodule Dotcom.SystemStatus.Alerts do
  }
  """
  def filter_relevant(alerts) do
-    alerts |> Enum.filter(fn %{effect: effect} -> effect in @service_effects end)
+    alerts |> Enum.filter(&service_impacting_alert?/1)


Nice!

I could eventually see this whole module getting lumped into Dotcom.Alerts, but that's a project for a future day.

joshlarson · 2025-02-11T17:17:06Z

lib/dotcom/utils/date_time.ex

+
+  If we are given something tha tis not a DateTime, AmbiguousDateTime, or an error tuple, we log the input and return `now`.


Suggested change

If we are given something tha tis not a DateTime, AmbiguousDateTime, or an error tuple, we log the input and return `now`.

Begone!

livebooks/planned-disruptions.livemd

joshlarson · 2025-02-11T17:51:59Z

lib/dotcom/alerts/disruptions/subway.ex

+  # 2. Filters out non-service-impacting alerts
+  # 3. Groups them according to service range.
+  defp disruption_groups() do
+    subway_route_ids()


I don't think this will work correctly (unless I missed a change in by_route_ids/2. Green line alerts aren't associated with the route ID "Green" - they're associated with the branches - "Green-B"/etc. This will also miss Mattapan alerts, I think.

I was pulling Green Line alerts with this code. I can check to see if we want Mattapan alerts to show up in this component. Right now it only shows the main lines. Easy to add if we need/want them.

anthonyshull changed the base branch from main to ags/datetime-utils-lib February 10, 2025 19:15

anthonyshull force-pushed the ags/datetime-utils-lib branch from 2d94e04 to 61d5d0f Compare February 10, 2025 19:16

anthonyshull force-pushed the ags/planned-disruptions-logic branch 2 times, most recently from 6c49ce6 to 50a0bcd Compare February 10, 2025 20:18

joshlarson reviewed Feb 10, 2025

View reviewed changes

lib/alerts.ex Outdated Show resolved Hide resolved

anthonyshull force-pushed the ags/planned-disruptions-logic branch from 50a0bcd to 4f64616 Compare February 10, 2025 22:35

anthonyshull requested a review from joshlarson February 10, 2025 22:36

anthonyshull marked this pull request as ready for review February 10, 2025 22:36

anthonyshull requested a review from a team as a code owner February 10, 2025 22:36

anthonyshull force-pushed the ags/planned-disruptions-logic branch 3 times, most recently from e08f304 to 4ceff54 Compare February 10, 2025 23:51

anthonyshull added the do not merge ⏳ it's waiting on something label Feb 10, 2025

anthonyshull force-pushed the ags/planned-disruptions-logic branch from 5d24251 to 7020026 Compare February 11, 2025 11:26

Base automatically changed from ags/datetime-utils-lib to main February 11, 2025 16:46

thecristen approved these changes Feb 11, 2025

View reviewed changes

anthonyshull added 14 commits February 11, 2025 11:05

date_time module and tests

946ba02

docs

ce6f9c1

linting

59eeb0e

some cleanup

8de7cfd

some cleanup

1fcecd4

remove broken tests and add new tests

9d0d291

this week test

5be4845

make service rollover time configurable

2b9de8d

split into two modules

19f1523

more tests

dc8ada9

100% coverage

5e707d7

some docs

b454b57

some docs

c17011b

format

b1ff2e8

anthonyshull added 23 commits February 11, 2025 11:05

remove log and allow error

5b83bb2

add a mock for date_time

f8dfc84

mocks, tests, format

5aa779a

docs

da82601

one hundred percent coverage

4847945

make test more clear

c75c844

move test

65f4d26

docs

d2f86fe

start work

9c42b43

logic looks solid

24ec694

use helper func

13e38c7

tests

f8afecb

tests

6490bac

tests

075ad59

one hundred

2a6d31b

dialyzer

e35c0ad

format

b69d409

specs

a4196ca

tests

837a383

spec error

0626b87

change tests

e29f2f5

type check

342e158

type check

946552f

anthonyshull force-pushed the ags/planned-disruptions-logic branch from 29e341c to 946552f Compare February 11, 2025 17:06

anthonyshull added 3 commits February 11, 2025 11:09

pr feedback

84b0098

make behaviour load consistent

e500528

remove livebook

69c0a03

anthonyshull merged commit 9c7a295 into main Feb 11, 2025
17 checks passed

anthonyshull deleted the ags/planned-disruptions-logic branch February 11, 2025 17:58

joshlarson requested changes Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Planned disruptions logic #2373

Planned disruptions logic #2373

anthonyshull commented Feb 10, 2025

thecristen left a comment

thecristen Feb 11, 2025

thecristen Feb 11, 2025

joshlarson Feb 11, 2025

anthonyshull Feb 11, 2025

anthonyshull Feb 11, 2025

thecristen Feb 11, 2025

joshlarson left a comment

joshlarson Feb 11, 2025

joshlarson Feb 11, 2025

anthonyshull Feb 11, 2025 •

edited

Loading

anthonyshull Feb 11, 2025

joshlarson Feb 11, 2025

joshlarson Feb 11, 2025

joshlarson Feb 11, 2025

joshlarson Feb 11, 2025

joshlarson Feb 11, 2025

anthonyshull Feb 11, 2025


		If we are given something tha tis not a DateTime, AmbiguousDateTime, or an error tuple, we log the input and return `now`.

Planned disruptions logic #2373

Planned disruptions logic #2373

Conversation

anthonyshull commented Feb 10, 2025

thecristen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshlarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonyshull Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthonyshull Feb 11, 2025 •

edited

Loading