-
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
cleanup: Add more alert factories for use in SystemStatus.SubwayTests #2347
base: main
Are you sure you want to change the base?
Conversation
5d53205
to
f852999
Compare
def for_route(alert, route_id) do | ||
%{ | ||
alert | ||
| informed_entity: | ||
InformedEntitySet.build(:informed_entity_set, | ||
route: route_id, | ||
entities: [ | ||
InformedEntity.build(:informed_entity, route: route_id) | ||
] | ||
) | ||
} | ||
end | ||
|
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.
suggestion: If you adjust this function to be its own factory, you wouldn't need to create an alert and pass it to for_route/2
. Say you want to call build(:alert_with_route, %{route_id: "Blue"})
, that could work with something like this....
def alert_with_route_factory(%{route_id: route_id} = attrs) do
informed_entity = InformedEntitySet.build(:informed_entity_set,
route: route_id,
entities: InformedEntity.build_list(1, :informed_entity, route: route_id)
)
build(:alert, informed_entity: informed_entity)
end
alias Test.Support.Factories.Alerts.InformedEntitySet | ||
|
||
@high_priority_effects [:delay, :shuttle, :station_closure, :suspension] |
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.
It'd be nice to not have to duplicate this from its source of truth.
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.
Yeah.... What IS its source of truth?
Also I just realized this point might be moot, since SystemStatus.Subway.subway_status/2
doesn't actually do anything with the effect
now. 🤔
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.
Wait, what is giving the subway status feature the "high priority" alerts then?? 😅
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.
In alerts.ex
, the filter_relevant/1
function ensures that only certain alerts are returned, and which effects are "relevant" is defined here.
In this PR, the results from alerts.ex
are passed into subway_status/2
.
That's how it works now - I'm not overly attached to that thought - there's probably a better way.
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.
Ah okay! In that case, the link behind "here" is the source of truth I'm referring to.
If there were a function there that exposes that list, say Dotcom.SystemStatus.Alerts.relevant_effects()
, you could call that in this module where needed.
f852999
to
5176d5e
Compare
5176d5e
to
f587772
Compare
Follow-up to: