Skip to content
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

refactor: parse all elevator redundancy categories #2340

Merged
merged 1 commit into from
Dec 12, 2024

Conversation

digitalcora
Copy link
Contributor

@digitalcora digitalcora commented Dec 10, 2024

Rather than only storing whether an elevator's redundancy category is "nearby", we now store all categories, including any associated summary text. This is a prerequisite for displaying redundancy summaries on the system-wide outages view.

Also extracts this logic into a dedicated module which can be mocked for testing, removing the need for a separate fixture file.

@digitalcora digitalcora marked this pull request as ready for review December 10, 2024 16:31
@digitalcora digitalcora requested a review from a team as a code owner December 10, 2024 16:31
%{"redundancy" => 1} ->
%__MODULE__{id: id, redundancy: :nearby}

%{"redundancy" => 2} ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Category 2 should also be returning summary text as well shouldn't it? At least according to the task that this is a prerequisite for mentioned in the PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm conceiving of this module as purely an interface to the data in the spreadsheet — "rules" apart from the raw data, like "when the redundancy category is 2, always use this specific text" would be implemented in the widget generator. I decided to draw the line here because of the rule where the summary text for all categories changes if any of the "backup elevators" for a given elevator are down, which depends on data from outside the spreadsheet (active alerts).

Copy link
Contributor

@PaulJKim PaulJKim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Rather than only storing whether an elevator's redundancy category
is "nearby", we now store all categories, including any associated
summary text. This is a prerequisite for displaying redundancy
summaries on the system-wide outages view.

Also extracts this logic into a dedicated module which can be mocked for
testing, removing the need for a separate fixture file.
@digitalcora digitalcora force-pushed the cfg-backup-route-summary branch from cf53232 to db5a585 Compare December 12, 2024 22:25
@digitalcora digitalcora merged commit ef87088 into main Dec 12, 2024
11 checks passed
@digitalcora digitalcora deleted the cfg-backup-route-summary branch December 12, 2024 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants