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]

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?

[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.

[thecristen]

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

[joshlarson]

I don't think so. This function (probably worth renaming it, tbh) gives you either :current or {:future, effect_start_time}. I think any_time_match?/2 (and its friend any_period_match?/2) return booleans, so I think at best, we would use any_time_match?/2 to figure out whether to call, effectively, this function.

Open to pairing on whether I've missed something about this, but I'd prefer to punt that to a follow-up if it's not trivially easy to do :)

[thecristen]

suggestion: call GreenLine.branch_ids() for the same output! ✨

[thecristen]

Awesome. The change I am requesting is just for adding @specs for the public functions. I'd love to see the type definition of what a SystemStatus is!

[thecristen]

suggestion(blocking): let's add a @spec here! this also gives us a chance to explicitly describe the shape of the output (like, what are all the possible values of time or status in status_entries?).

[joshlarson]

Ohhh that reminds me that perhaps the output of this should be a struct instead of a map, for more explicit type-checking, clarity, and obvious errors if it gets populated wrong?

[thecristen]

https://hexdocs.pm/elixir/1.17.3/typespecs.html#literals you can get pretty descriptive with describing maps, and having this alone would help clarity a lot! I think a map could be sufficient but a struct would be great too!

[joshlarson]

Good call! Will do!

The exact shape of the data is going to change again (which is probably part of why a @spec would be a good idea - to communicate these changes! 😅)

[thecristen]

suggestion: let's add a @spec here too.

[thecristen]

suggestion(non-blocking): expanding on my earlier suggestion (maybe?), this function body could potentially be replaced with Alerts.Match.match(alerts, %InformedEntity{route: route_id})

[joshlarson]

I'll give this a try again! I'm pretty sure that this is exactly what I tried and couldn't get it to work, but I will give it another go!

[joshlarson]

@thecristen follow-up PR to add @spec's:

• cleanup: Add @spec annotations to Dotcom.SystemStatus.Subway #2360

I could pull that into this PR if you'd like, or keep it separate - I'm good either way.

Verbal confirmation that the issues were addressed