Skip to content

refactor(Alerts.Sort): more performant comparison #3311

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.

Sign up for GitHub

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

Open
thecristen wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactor(Alerts.Sort): more performant comparison #3311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1ff726c
refactor(StopController): assign alerts for individual stops
thecristen Jun 30, 2026
d895395
refactor(Alerts.Sort): more performant comparison
thecristen Jun 30, 2026
e50e005
!fixup(Alerts.StartTime): test, make it make sense
thecristen Jul 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 5 additions & 21 deletions lib/alerts/sort.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ defmodule Alerts.Sort do
lifecycle_index(alert.lifecycle),
-alert.severity,
-updated_at_date(alert.updated_at),
first_future_active_period_start(alert.active_period, now),
first_future_active_period_start(alert, now),
alert.id
}
end
Expand Down Expand Up @@ -91,26 +91,10 @@ defmodule Alerts.Sort do
defp priority(%{priority: :high}), do: 0
defp priority(%{priority: :system}), do: 1

# atoms are greater than any integer
defp first_future_active_period_start([], _now), do: :infinity

defp first_future_active_period_start(periods, now) do
# first active period that's in the future
now_unix = DateTime.to_unix(now, :second)

future_periods =
for {start, _} <- periods,
start,
# wrap in a list to avoid an Erlang 19.3 issue
unix <- [DateTime.to_unix(start)],
unix > now_unix do
unix
end

if future_periods == [] do
:infinity
else
Enum.min(future_periods)
defp first_future_active_period_start(alert, now) do
case Dotcom.Alerts.StartTime.next_active_time(alert, now) do
{_, datetime} -> DateTime.to_unix(datetime)
:past -> :infinity
end
end
end
5 changes: 5 additions & 0 deletions lib/dotcom/alerts/start_time.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,14 @@ defmodule Dotcom.Alerts.StartTime do
time
) do
cond do
# period already ended -- keep iterating
ends_before?(end_time, time) ->
next_active_period_active_time(rest_of_active_periods, time)

# period hasn't ended yet, but no defined start time -- use the current time
is_nil(start_time) ->

@thecristen thecristen Jul 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Some of our tests have alerts involving active periods with a nil start time, so I added this clause to handle those. I'm not sure if this'll ever be encountered in real alerts from Alerts Manager, though.

@lvachon1 lvachon1 Jul 1, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm glad I saw this PR, I have one open with yet a 3rd way of computing the active time period. I'm going to put mine into draft for now. I saw this exact same behavior regarding only our tests having nil start times, thanks for following up with the transit data team about if that's even possible.

{:current, time}

DateTime.before?(start_time, time) ->
{:current, start_time}

Expand Down
8 changes: 4 additions & 4 deletions lib/dotcom_web/controllers/stop_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ defmodule DotcomWeb.StopController do
routes: [route_with_directions]
}

plug(:alerts)
plug(DotcomWeb.Plugs.DateTime)
plug(DotcomWeb.Plugs.AlertsByTimeframe)

Expand Down Expand Up @@ -81,6 +80,7 @@ defmodule DotcomWeb.StopController do
conn
|> assign(:breadcrumbs, breadcrumbs(stop, routes_by_stop))
|> meta_description(stop, routes_by_stop)
|> assign_alerts()
|> render("show.html", %{
stop: stop,
amenity_param: Map.get(params, "amenity", "") |> String.to_atom(),
Expand Down Expand Up @@ -347,11 +347,11 @@ defmodule DotcomWeb.StopController do
@spec includes_predictions?(TransitNearMe.headsign_data()) :: boolean
defp includes_predictions?(%{times: times}), do: Enum.any?(times, &(&1.prediction != nil))

defp alerts(%{assigns: %{alerts: alerts}} = conn, _opts) do
defp assign_alerts(%{assigns: %{alerts: alerts}} = conn) do
assign(conn, :all_alerts_count, length(alerts))
end

defp alerts(%{path_params: %{"id" => id}} = conn, _opts) do
defp assign_alerts(%{path_params: %{"id" => id}} = conn) do
stop_id = URI.decode_www_form(id)

alerts =
Expand All @@ -364,7 +364,7 @@ defmodule DotcomWeb.StopController do
|> assign(:all_alerts_count, length(alerts))
end

defp alerts(conn, _opts) do
defp assign_alerts(conn) do
assign(conn, :alerts, AlertsRepo.all(conn.assigns.date_time))
end

Expand Down
9 changes: 9 additions & 0 deletions test/dotcom/alerts/start_time_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,15 @@ defmodule Dotcom.Alerts.StartTimeTest do

assert next_active_time(alert, time) == :past
end

test "returns :current if active period has nil start time but ends in the future" do
time = Test.Support.Generators.DateTime.random_date_time()
end_time = Test.Support.Generators.DateTime.random_date_time_after(time)

alert = Alert.build(:alert, active_period: [{nil, end_time}])

assert next_active_time(alert, time) == {:current, time}
end
end

describe "active?/2" do
Expand Down
Loading