feat: Create a data structure to populate the system status widget

[joshlarson]

Summary of changes

Asana Ticket: System Status Flowchart | Implement the bulk of the flowchart
Parent Ticket: System Status | Add module that implements the system-status flowchart

---

Depends on:

• feat: Add module that retrieves system-status relevant alerts #2327

[anthonyshull]

Could probably be cleaned up a bit. One suggestion would be to write comments for all of the private functions. I find that it helps add clarity when I have to write narrative descriptions.

[thecristen]

suggestion(non-blocking): This could read real nice broken out into a function, like Enum.all?(lines, &shows_normal_service?/1) which validates the nil time and "Normal Service" text (though I suppose you'd want a separate check to verify the correct route IDs involved, maybe)

[joshlarson]

Question: I've given the tests a pretty thorough once-over (actually more like a twice- or thrice- over). Do you think this is still necessary?

[thecristen]

This comment should be in a @doc above the def line

[joshlarson]

I should restructure this comment to make it clearer that it's actually about @lines, not groups/2

[thecristen]

That function should be alerts_for_route if it operates per-route, no?

I also wonder if something in the Alerts.Match module might meet your needs, what if Alerts.Match.match(alerts, %InformedEntity{route: &1})?

[joshlarson]

That function should be alerts_for_route if it operates per-route, no?

Yep! Done.

I also wonder if something in the Alerts.Match module might meet your needs, what if Alerts.Match.match(alerts, %InformedEntity{route: &1})?

Ugh - I agree, but I've been struggling to actually get Alerts.Match to work, and I'm not sure exactly why. I still think it's worth using the machinery that already exists, but would you mind if I did that as a follow-up instead?

[joshlarson]

Pulled back into draft because I realized that the data structure isn't quite the right shape to accommodate the desired frontend

[anthonyshull]

Overall, I don't like the implicit use of nil as a time in order to try to simplify logic. It makes for using functions that purportedly take a time, but don't really. You could utilize module attributes a lot more to hardcode a lot less.

[anthonyshull]

Any code that goes toward building an alert should go in the alerts factory.

[joshlarson]

I sort of see that, although exactly what belongs in a factory versus just here eludes me at the moment.

The problem is that I don't want to have to copy-pasta the informed_entity / informed_entity_set nesting over and over again, but I'm not sure that it makes sense to pull this into a factory as-is either.

There probably is a way to do it gracefully, but unless we can get that taken care of real fast, I think I'll ask that we tackle it as a follow-up, since this work is blocking the rest of the project.

[anthonyshull]

You can see in this factory: https://github.com/mbta/dotcom/pull/2336/files#diff-8fbc124c966696c04346223e24b3cc686bb63d07f0f3a35316e93f8dd30c6384 how I separated _factory functions from other functions.

[anthonyshull]

We can export these out of the module.

defmodule Dotcom.SystemStatus.Groups do
  @heavy_rail_lines ~w[Blue Orange Red]
  @all_rail_lines @heavy_rails_lines ++ ~w[Green]
  
  def heavy_rail_lines, do: @heavy_rail_lines
  
  def all_rail_lines, do: @all_rail_lines
end

defmodule Dotcom.SystemStatus.GroupsTest do
  describe "groups/2" do
    test "sorts groups by heavy rail first" do
      # Exercise
      groups = Group.groups([], time_today())
      
      # Verify
      route_ids = groups |> Enum.map(&1.route_id)
      heavy_rail_lines = heavy_rail_lines()
      assert Enum.take(route_ids, Kernel.length(heavy_rail_lines)) == heavy_rail_lines
    end
  end
end

[joshlarson]

I see that that's possible, but what is the benefit?

My problem is that it makes the tests a bit more tautological - instead of the assertion coming from the tests, the assertion comes from a mix of the source code and the tests, which means that the tests don't validate a user-facing behavior so much as validating that the code is interacting with its own internal constants.

I especially feel that way about this suggestion because the constants you mentioned aren't present in the source file, because I didn't need them. I would be exporting an internal constant that the source file doesn't even need for the sake of the tests, which is the kind of refactor that I've usually considered an antipattern.

I'll think on it for a little longer, but that's my first impression.

[anthonyshull]

You don't necessarily have to have the heavy rail lines in a module attribute, though I think you did before. If you're getting them from somewhere else then you can get them there in the test.

[anthonyshull]

Looks like you're just using @lines now. So, you can export that.

[joshlarson]

...though I think you did before.

This is exactly why I don't want to export the module attributes. I did a pretty hefty refactor to Groups.groups/2 that changed which module attributes existed, and that refactor would have been harder if I had been exporting them. They're implementation details, and they change during refactors.

I see that that's possible, but what is the benefit?

Bump on this question, which kind of feels like it makes all the difference.

[anthonyshull]

The benefit is that the test is more communicative.

[anthonyshull]

I'm not clear on why setting the time to nil means that it is active.

[joshlarson]

The way this will be rendered in the frontend will be something like:

case time do
  nil -> description
  _ -> "#{time}: #{description}"
end

Thinking about other ways of representing this.... maybe prefix instead of time, since in a certain sense, that's actually what it is /slash/ how it will be used?

[anthonyshull]

Is there ever a case where the description is "Normal Service" and the time is not nil? Also, I see "Normal Service" in multiple places in this module and in the tests. Why not make it a module attribute and export it?

[joshlarson]

Is there ever a case where the description is "Normal Service" and the time is not nil?

I don't think so.

Also, I see "Normal Service" in multiple places in this module and in the tests. Why not make it a module attribute and export it?

Touché, although I'm not keen on exporting it for the same reason that I mentioned here.

[anthonyshull]

This could be a module attribute of a map or a function using pattern matching. What happens if the alert.effect isn't in this list?

[joshlarson]

This could be a module attribute of a map or a function using pattern matching.

True. This felt pretty much equivalent to both of those 🤷‍♂️.

What happens if the alert.effect isn't in this list?

If the alert effect isn't in this list, that's an error that I'd probably want to see in Sentry. How do we typically handle this kind of thing?

[anthonyshull]

Usually pattern matching and the fallback logs the error with the argument.

[anthonyshull]

I'm not sure we need these since they just wrap Timex functions.

[joshlarson]

ends_before? is needed, but maybe starts_before? isn't. It looked a bit odd to me to extract one and not the other, but I guess I can inline starts_before?.

[anthonyshull]

I don't like the fact that stringify_time takes something that isn't a time and returns something that isn't a string.

[joshlarson]

Do you have a better name in mind? maybe_time_to_prefix (assuming this suggestion)?

[anthonyshull]

We don't have these spaces in the moduledoc of any other modules.

[anthonyshull]

We don't use this empty line in docs anywhere else. We shouldn't describe this module as being for use in front-end component. It should be useful on its own. Something like takes alerts groups them in order to communicate system status of each route.

[thecristen]

question: Does Alerts.Match.any_time_match?/2 do the same thing?