-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add module that retrieves system-status relevant alerts #2327
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a lot of suggestions around naming and clarity. And, I think the tests could be improved in terms of using Faker and being more clear.
subway_alerts_for_today(Timex.now()) | ||
end | ||
|
||
defp subway_alerts_for_today(now) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe rename this function to subway_alerts_for_day
. That would make more sense with the function above. It's also confusing to name this argument now
as it can be any time.
""" | ||
@relevant_effects [:delay, :shuttle, :suspension, :station_closure] | ||
|
||
defp has_started?(active_period_start, now) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing with all of these. now
should be a more generic date_time
or something.
...> ) | ||
false | ||
""" | ||
def active_today?(alert, now) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, I would name this active_on_day?
since it can take any time.
Given a list of alerts, filters only the ones that are active today, as defined in `&active_today?/2`. | ||
See that function for details | ||
""" | ||
def for_today(alerts, now) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for_day
now |> Timex.end_of_day() |> Timex.after?(active_period_start) | ||
end | ||
|
||
defp has_not_ended?(nil, _now) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We put all public functions higher in the module than all private functions. I generally try to alphabetize them in order as well.
|
||
alias Dotcom.SystemStatus.Alerts | ||
|
||
defp local_datetime(naive) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually put private functions at the bottom of the file, though, tbh, not sure we need this. See below.
local_datetime(~N[2025-01-09 12:00:00]), | ||
local_datetime(~N[2025-01-09 20:00:00]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than hardcoding these, you can use Faker: https://hexdocs.pm/faker/Faker.DateTime.html. It's a good way to catch subtle bugs that otherwise wouldn't occur because of the values chosen.
To me, this is much easier to understand as well.
# Setup
start = Timex.now()
end = Timex.shift(start, hours: 24)
alert = build(:alert, active_period: [{start, end}]
time = Faker.DateTime.between(start, end)
# Exercise/Verify
assert Alerts.active_for_day?(alert, time)
@doc """ | ||
Returns a list of alerts that satisfy the following criteria: | ||
- They are for one of the subway or trolley lines (including Mattapan), and | ||
- They are either currently active, or will be later today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could make this more straightforward by saying that the alert is active today... if it is currently active, it will necessarily be active today.
Summary of changes
This adds a
Dotcom.SystemStatus
module, as well asDotcom.SystemStatus.Alerts
, which combined will retrieve a list of all alerts that are either active now, or will become active later today (or whatever day is passed in asnow
).This also adds a live view that I was using to test this out. I see that live view morphing into one that previews the system status widget as we get closer to done on this project.
Some questions that I'd be interested in noodling on:
Dotcom.SystemStatus.Alerts
the right place for that filtering logic, or would it make more sense for some or all of that to live in theAlerts
namespace?Asana Ticket: System Status | Add ability to get all future alerts for the day