fix: Combine contiguous active periods (#2402)

lib/dotcom/alerts/disruptions/subway.ex

@@ -9,6 +9,7 @@ defmodule Dotcom.Alerts.Disruptions.Subway do
 
   alias Alerts.Alert
   alias Dotcom.Utils
+  alias Dotcom.Utils.ServiceDateTime
 
   @alerts_repo Application.compile_env!(:dotcom, :repo_modules)[:alerts]
   @date_time_module Application.compile_env!(:dotcom, :date_time_module)
@@ -38,6 +39,7 @@ defmodule Dotcom.Alerts.Disruptions.Subway do
   defp disruption_groups() do
     subway_route_ids()
     |> @alerts_repo.by_route_ids(@date_time_module.now())
+    |> Enum.flat_map(&split_by_discontiguous_active_periods/1)
     |> Enum.filter(&service_impacting_alert?/1)
     |> Enum.reduce(%{}, &group_alerts/2)
     |> Enum.map(fn {group, alerts} ->
@@ -46,6 +48,52 @@ defmodule Dotcom.Alerts.Disruptions.Subway do
     |> Enum.into(%{})
   end
 
+  # Given an alert with multiple active periods, some of which may be
+  # contiguous with one another (next one starts the same day or the
+  # day after the previous one), combines the contiguous active
+  # periods, and then returns one alert per combined-contiguous active
+  # period
+  #
+  # For instance, if an alert has three active periods, {Sun, Mon},
+  # {Mon, Tue}, and {Thu, Fri}, then {Sun, Mon} and {Mon, Tue} are
+  # combined into {Sun, Tue}, but {Thu, Fri} is kept separate. The
+  # result is then two alerts, both equivalent to the alert passed in,
+  # except that one has a single active period of {Sun, Tue}, and the
+  # other has a single active period of {Thu, Fri}.
+  defp split_by_discontiguous_active_periods(alert) do
+    alert.active_period
+    |> combine_contiguous_active_periods()
+    |> Enum.map(fn active_period ->
+      %Alerts.Alert{alert | active_period: [active_period]}
+    end)
+  end
+
+  # Given a list of active periods, combines contiguous ones. See
+  # split_by_discontiguous_active_periods/1 for more detail.  Active
+  # periods are considered contiguous if the start of the next one is
+  # the same day as the end of the previous one, or if it's the day
+  # after, so active periods of {Mon, Wed} and {Thu, Fri} would be
+  # combined into {Mon, Fri}, as would {Mon, Wed} and {Wed, Fri}, but
+  # {Mon, Tue} and {Thu, Fri} would be kept separate.
+  defp combine_contiguous_active_periods([active_period1, active_period2 | rest]) do
+    {start1, end1} = active_period1
+    {start2, end2} = active_period2
+
+    if Date.before?(
+         ServiceDateTime.service_date(end1) |> Date.shift(day: 1),
+         ServiceDateTime.service_date(start2)
+       ) do
+      [active_period1 | combine_contiguous_active_periods([active_period2 | rest])]
+    else
+      combined_active_period = {start1, end2}
+      combine_contiguous_active_periods([combined_active_period | rest])
+    end
+  end
+
+  defp combine_contiguous_active_periods(active_periods) do
+    active_periods
+  end
+
   # Looks at every active period for an alert and groups that alert by service range.
   # Alerts can overlap service ranges, in which case we want them to appear in both.
   defp group_alerts(alert, groups) do

test/dotcom/alerts/disruptions/subway_test.exs

@@ -14,7 +14,9 @@ defmodule Dotcom.Alerts.Disruptions.SubwayTest do
 
   import Mox
 
+  alias Dotcom.Utils.ServiceDateTime
   alias Test.Support.Factories
+  alias Test.Support.Generators
 
   setup :verify_on_exit!
 
@@ -43,7 +45,7 @@ defmodule Dotcom.Alerts.Disruptions.SubwayTest do
       {alert_after_next_week_start, _} = service_range_after_next_week()
 
       alert_after_next_week =
-        {alert_after_next_week_start, Timex.shift(alert_after_next_week_start, days: 1)}
+        {alert_after_next_week_start, DateTime.shift(alert_after_next_week_start, day: 1)}
         |> disruption_alert()
 
       expect(Alerts.Repo.Mock, :by_route_ids, fn _route_ids, _now ->
@@ -64,7 +66,7 @@ defmodule Dotcom.Alerts.Disruptions.SubwayTest do
       {alert_after_next_week_start, _} = service_range_after_next_week()
 
       long_alert =
-        [{alert_today_start, Timex.shift(alert_after_next_week_start, days: 1)}]
+        [{alert_today_start, DateTime.shift(alert_after_next_week_start, day: 1)}]
         |> disruption_alert()
 
       expect(Alerts.Repo.Mock, :by_route_ids, fn _route_ids, _now ->
@@ -81,18 +83,191 @@ defmodule Dotcom.Alerts.Disruptions.SubwayTest do
 
     test "handles alert with more than one active_period" do
       # Setup
+      long_alert = [service_range_this_week(), service_range_next_week()] |> disruption_alert()
+
+      expect(Alerts.Repo.Mock, :by_route_ids, fn _route_ids, _now ->
+        [long_alert]
+      end)
+
+      # Exercise
+      disruptions = future_disruptions()
+
+      # Verify
+      [alert_this_week] = disruptions.this_week
+      [alert_next_week] = disruptions.next_week
+
+      assert alert_this_week.id == long_alert.id
+      assert alert_next_week.id == long_alert.id
+    end
+
+    test "splits the active_periods of alerts with more than one into different buckets" do
+      # Setup
+      {beginning_of_next_week, end_of_next_week} = service_range_next_week()
+
+      active_period_1_start =
+        Generators.DateTime.random_time_range_date_time(
+          {beginning_of_next_week, DateTime.shift(end_of_next_week, day: -2)}
+        )
+
+      active_period_1_end =
+        Generators.DateTime.random_time_range_date_time(
+          {active_period_1_start, DateTime.shift(end_of_next_week, day: -1)}
+        )
+
+      {beginning_of_week_after_next, _} = service_range_after_next_week()
+
+      active_period_2_start =
+        Generators.DateTime.random_time_range_date_time({beginning_of_week_after_next, nil})
+
+      active_period_2_end =
+        Generators.DateTime.random_time_range_date_time({active_period_2_start, nil})
+
       long_alert =
-        [service_range_this_week(), service_range_next_week()] |> disruption_alert()
+        [
+          {active_period_1_start, active_period_1_end},
+          {active_period_2_start, active_period_2_end}
+        ]
+        |> disruption_alert()
 
       expect(Alerts.Repo.Mock, :by_route_ids, fn _route_ids, _now ->
         [long_alert]
       end)
 
-      # Exercise/Verify
-      assert %{
-               this_week: [^long_alert],
-               next_week: [^long_alert]
-             } = future_disruptions()
+      # Exercise
+      disruptions = future_disruptions()
+
+      # Verify
+      [alert_next_week] = disruptions.next_week
+      assert alert_next_week.id == long_alert.id
+      assert alert_next_week.active_period == [{active_period_1_start, active_period_1_end}]
+
+      [alert_after_next_week] = disruptions.after_next_week
+      assert alert_after_next_week.id == long_alert.id
+      assert alert_after_next_week.active_period == [{active_period_2_start, active_period_2_end}]
+    end
+
+    test "combines consecutive active periods if the boundary is on the same day" do
+      # Setup
+      {beginning_of_week, end_of_week} = service_range_next_week()
+
+      active_period_1_end =
+        Generators.DateTime.random_time_range_date_time(
+          {beginning_of_week, DateTime.shift(end_of_week, day: -2)}
+        )
+
+      active_period_2_start =
+        Generators.DateTime.random_time_range_date_time(
+          {active_period_1_end, ServiceDateTime.end_of_service_day(active_period_1_end)}
+        )
+
+      multi_active_period_alert =
+        [{beginning_of_week, active_period_1_end}, {active_period_2_start, end_of_week}]
+        |> disruption_alert()
+
+      expect(Alerts.Repo.Mock, :by_route_ids, fn _route_ids, _now ->
+        [multi_active_period_alert]
+      end)
+
+      # Exercise
+      disruptions = future_disruptions()
+
+      # Verify
+      assert Enum.count(disruptions.next_week) == 1
+      [combined_active_period_alert] = disruptions.next_week
+      assert combined_active_period_alert.active_period == [{beginning_of_week, end_of_week}]
+    end
+
+    test "combines consecutive active periods if the boundary is on consecutive days" do
+      # Setup
+      {beginning_of_week, end_of_week} = service_range_next_week()
+
+      active_period_1_end =
+        Generators.DateTime.random_time_range_date_time(
+          {beginning_of_week, DateTime.shift(end_of_week, day: -2)}
+        )
+
+      active_period_2_start =
+        Generators.DateTime.random_time_range_date_time(
+          {active_period_1_end |> ServiceDateTime.beginning_of_next_service_day(),
+           active_period_1_end
+           |> ServiceDateTime.beginning_of_next_service_day()
+           |> ServiceDateTime.end_of_service_day()}
+        )
+
+      multi_active_period_alert =
+        [{beginning_of_week, active_period_1_end}, {active_period_2_start, end_of_week}]
+        |> disruption_alert()
+
+      expect(Alerts.Repo.Mock, :by_route_ids, fn _route_ids, _now ->
+        [multi_active_period_alert]
+      end)
+
+      # Exercise
+      disruptions = future_disruptions()
+
+      # Verify
+      assert Enum.count(disruptions.next_week) == 1
+      [combined_active_period_alert] = disruptions.next_week
+      assert combined_active_period_alert.active_period == [{beginning_of_week, end_of_week}]
+    end
+
+    test "combines consecutive active periods in the middle of the active period list" do
+      # Setup
+      {beginning_of_week, end_of_week} = service_range_next_week()
+
+      active_period_0_start =
+        Generators.DateTime.random_time_range_date_time(
+          {beginning_of_week, DateTime.shift(beginning_of_week, day: 1)}
+        )
+
+      active_period_0_end =
+        Generators.DateTime.random_time_range_date_time(
+          {active_period_0_start, DateTime.shift(active_period_0_start, day: 1)}
+        )
+
+      active_period_1_start =
+        Generators.DateTime.random_time_range_date_time(
+          {DateTime.shift(active_period_0_end, day: 2), DateTime.shift(end_of_week, day: -2)}
+        )
+
+      active_period_1_end =
+        Generators.DateTime.random_time_range_date_time(
+          {active_period_1_start, DateTime.shift(end_of_week, day: -2)}
+        )
+
+      active_period_2_start =
+        Generators.DateTime.random_time_range_date_time(
+          {active_period_1_end, ServiceDateTime.end_of_service_day(active_period_1_end)}
+        )
+
+      active_period_2_end = end_of_week
+
+      multi_active_period_alert =
+        [
+          {active_period_0_start, active_period_0_end},
+          {active_period_1_start, active_period_1_end},
+          {active_period_2_start, active_period_2_end}
+        ]
+        |> disruption_alert()
+
+      expect(Alerts.Repo.Mock, :by_route_ids, fn _route_ids, _now ->
+        [multi_active_period_alert]
+      end)
+
+      # Exercise
+      disruptions = future_disruptions()
+
+      # Verify
+      assert Enum.count(disruptions.next_week) == 2
+      [first_active_period_alert, combined_later_active_period_alert] = disruptions.next_week
+
+      assert first_active_period_alert.active_period == [
+               {active_period_0_start, active_period_0_end}
+             ]
+
+      assert combined_later_active_period_alert.active_period == [
+               {active_period_1_start, active_period_2_end}
+             ]
     end
   end
 
@@ -137,17 +312,21 @@ defmodule Dotcom.Alerts.Disruptions.SubwayTest do
         [alert_today_and_beyond, alert_today_and_other_dates]
       end)
 
-      # Exercise/Verify
-      assert %{
-               today: [^alert_today_and_beyond, ^alert_today_and_other_dates]
-             } = todays_disruptions()
+      # Exercise
+      disruptions = todays_disruptions()
+
+      # Verify
+      assert disruptions.today |> Enum.map(& &1.id) == [
+               alert_today_and_beyond.id,
+               alert_today_and_other_dates.id
+             ]
     end
 
     test "sorts alerts by start time" do
       # Setup
       {start, stop} = service_range_day()
       alert_today = disruption_alert({start, stop})
-      alert_later = disruption_alert({Timex.shift(start, seconds: 1), stop})
+      alert_later = disruption_alert({DateTime.shift(start, second: 1), stop})
 
       expect(Alerts.Repo.Mock, :by_route_ids, fn _route_ids, _now ->
         [alert_later, alert_today]

test/support/generators/date_time.ex

@@ -30,7 +30,7 @@ defmodule Test.Support.Generators.DateTime do
 
   @doc "Generate a random date_time between the beginning and end of the time range."
   def time_range_date_time_generator({start, nil}) do
-    stop = Timex.shift(start, years: 10)
+    stop = Timex.shift(start, years: 10) |> @date_time_module.coerce_ambiguous_date_time()
     time_range_date_time_generator({start, stop})
   end