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

fix: Combine contiguous active periods #2402

Merged
merged 12 commits into from
Feb 27, 2025

Conversation

joshlarson
Copy link
Contributor

@joshlarson joshlarson commented Feb 26, 2025

Changes

Screenshot 2025-02-26 at 2 42 27 PM

Before is on the left, After is on the right. Note that:

  • The OL disruptions have narrower date ranges now, based on not combining the whole active period range anymore. (For instance, the "This week" OL disruption only goes until Feb 28)
  • The BL disruptions got split up into each weekend, instead of spanning the whole week.
  • Disruptions are still not sorted correctly within their buckets - that's out of scope for this PR.

Asana Ticket: [PW] Fix contiguous active period logic


TODO

  • Tests are a bit flaky - I goofed something in my Faker setup
  • Docs have some sentence fragments
  • Doctest or regular test for DateTime.shift/2

@joshlarson joshlarson requested a review from a team as a code owner February 26, 2025 19:48
@joshlarson joshlarson added the dev-green Deploy to dev-green label Feb 26, 2025
@joshlarson joshlarson removed the dev-green Deploy to dev-green label Feb 26, 2025
Copy link
Collaborator

@thecristen thecristen left a comment

Choose a reason for hiding this comment

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

Thanks!!

@@ -7,6 +7,7 @@ defmodule Dotcom.Alerts.Disruptions.Subway do
import Dotcom.Routes, only: [subway_route_ids: 0]
import Dotcom.Utils.ServiceDateTime, only: [all_service_ranges: 0, service_range: 1]

alias Dotcom.Utils.ServiceDateTime
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be alphabetical with the ones below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops - good catch thanks!

@joshlarson joshlarson enabled auto-merge (squash) February 27, 2025 16:28
@joshlarson joshlarson merged commit f61e14f into main Feb 27, 2025
17 checks passed
@joshlarson joshlarson deleted the jdl/planned-work/combine-contiguous-active-periods branch February 27, 2025 16:37
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.

3 participants