From b61a5d0b57ce0c735321abe5cb3a3dab169b9125 Mon Sep 17 00:00:00 2001 From: Cristen Jones Date: Mon, 29 Apr 2024 23:40:02 -0400 Subject: [PATCH] feat(TimetableController): get stops from canonical route patterns --- .../schedule/timetable_controller.ex | 200 +++++++++--------- .../templates/schedule/_timetable.html.eex | 2 +- .../schedule/timetable_controller_test.exs | 126 +++++++++-- .../views/schedule/timetable_view_test.exs | 8 +- 4 files changed, 217 insertions(+), 119 deletions(-) diff --git a/lib/dotcom_web/controllers/schedule/timetable_controller.ex b/lib/dotcom_web/controllers/schedule/timetable_controller.ex index c1ca541bd8..d9d86d1b15 100644 --- a/lib/dotcom_web/controllers/schedule/timetable_controller.ex +++ b/lib/dotcom_web/controllers/schedule/timetable_controller.ex @@ -3,6 +3,7 @@ defmodule DotcomWeb.ScheduleController.TimetableController do use DotcomWeb, :controller alias Plug.Conn alias Routes.Route + alias Stops.Stop alias DotcomWeb.ScheduleView require Logger @@ -14,7 +15,6 @@ defmodule DotcomWeb.ScheduleController.TimetableController do plug(DotcomWeb.Plugs.DateInRating) plug(:tab_name) plug(:direction_id) - plug(:all_stops) plug(DotcomWeb.ScheduleController.RoutePdfs) plug(DotcomWeb.ScheduleController.Core) plug(:do_assign_trip_schedules) @@ -60,54 +60,25 @@ defmodule DotcomWeb.ScheduleController.TimetableController do %{ trip_schedules: trip_schedules, - all_stops: all_stops - } = build_timetable(conn.assigns.all_stops, timetable_schedules, direction_id) - - canonical_rps = - @route_patterns_repo.by_route_id(route.id, - direction_id: direction_id, - canonical: true, - include: "representative_trip.stops" - ) - - canonical_stop_ids = - canonical_rps - |> Enum.flat_map(& &1.stop_ids) - |> MapSet.new() - - track_changes = track_changes(trip_schedules, canonical_stop_ids) - - ## Logging to capture misordered stops on CR-Newburyport timetable ## - # First, is this code even present? - Logger.info("dotcom_web.schedule_controller.timetable_controller.assign_trip_schedules") + trip_stops: trip_stops + } = build_timetable(conn, timetable_schedules) - if route.id == "CR-Newburyport" && direction_id == 1 do - [last, next | _rest] = Enum.map(all_stops, & &1.name) |> Enum.reverse() + track_changes = track_changes(trip_schedules, Enum.map(trip_stops, & &1.id)) - case {last, next} do - {"North Station", "Chelsea"} -> - # Log both good and bad results so we can quantify - Logger.info( - "dotcom_web.schedule_controller.timetable_controller.assign_trip_schedules stop_order=expected" - ) - - _ -> - # Show the offending last two stops - Logger.warning( - "dotcom_web.schedule_controller.timetable_controller.assign_trip_schedules stop_order=unexpected last=#{last} next=#{next}" - ) - end - end + header_stops = + trip_stops + |> Enum.map(&@stops_repo.get_parent/1) + |> Enum.with_index() conn |> assign(:timetable_schedules, timetable_schedules) |> assign(:header_schedules, header_schedules) + |> assign(:header_stops, header_stops) |> assign(:trip_schedules, trip_schedules) |> assign(:track_changes, track_changes) |> assign(:vehicle_schedules, vehicle_schedules) |> assign(:prior_stops, prior_stops) |> assign(:trip_messages, trip_messages(route, direction_id)) - |> assign(:all_stops, all_stops) end def assign_trip_schedules(conn) do @@ -119,10 +90,10 @@ defmodule DotcomWeb.ScheduleController.TimetableController do end @spec track_changes( - %{required({Schedules.Trip.id_t(), Stops.Stop.id_t()}) => Schedules.Schedule.t()}, - MapSet.t(Stops.Stop.id_t()) + %{required({Schedules.Trip.id_t(), Stop.id_t()}) => Schedules.Schedule.t()}, + [Stop.id_t()] ) :: %{ - required({Schedules.Trip.id_t(), Stops.Stop.id_t()}) => Stops.Stop.t() | nil + required({Schedules.Trip.id_t(), Stop.id_t()}) => Stop.t() | nil } defp track_changes(trip_schedules, canonical_stop_ids) do Map.new(trip_schedules, fn {{trip_id, stop_id}, sch} -> @@ -137,8 +108,8 @@ defmodule DotcomWeb.ScheduleController.TimetableController do @spec track_change_for_schedule( Schedules.Schedule.t(), - MapSet.t(Stops.Stop.id_t()) - ) :: Stops.Stop.t() | nil + [Stop.id_t()] + ) :: Stop.t() | nil @doc """ If the scheduled platform stop is not canonical, then return the stop of that track change. """ @@ -157,8 +128,8 @@ defmodule DotcomWeb.ScheduleController.TimetableController do defp has_scheduled_track_change(schedule, canonical_stop_ids) do # if the scheduled stop doesn't match a canonical stop, there has been a track change - MapSet.size(canonical_stop_ids) > 0 && - !MapSet.member?(canonical_stop_ids, schedule.platform_stop_id) + length(canonical_stop_ids) > 0 && + schedule.platform_stop_id not in canonical_stop_ids end # Helper function for obtaining schedule data @@ -229,78 +200,107 @@ defmodule DotcomWeb.ScheduleController.TimetableController do |> Enum.map(fn {train, stop, value} -> {{train, stop}, value} end) end - defp all_stops(%Conn{assigns: %{date_in_rating?: false}} = conn, _) do - conn - end - - defp all_stops(conn, _) do - all_stops = - @stops_repo.by_route(conn.assigns.route.id, conn.assigns.direction_id, - date: conn.assigns.date - ) - - case all_stops do - {:error, error} -> - :ok = - Logger.warning( - "module=#{__MODULE__} fun=all_stops error=#{inspect(error)} route=#{conn.assigns.route.id} direction_id=#{conn.assigns.direction_id} date=#{conn.assigns.date}" - ) - - conn - - _ -> - assign(conn, :all_stops, all_stops) - end - end - defp tab_name(conn, _), do: assign(conn, :tab, "timetable") - @spec build_timetable([Stops.Stop.t()], [Schedules.Schedule.t()], 0 | 1) :: %{ + @doc """ + Organize the route's schedules for timetable format, where schedules are laid + out horizontally by stop and vertically by trip. + + Trips are derived from the input schedules, but stops are fetched for the + given route and direction via the route pattern relation. Where possible, + canonical route patterns are used to determine the ordered list of stops + visited. This list is augmented further by the input schedules, which may + introduce additional stops (in the case of a new shuttle route) or removed + stops (if such stop is skipped entirely). + + Stops from trips from multiple route patterns are consolidated into a single + list of unique, ordered stops, taking into consideration directional branching + (e.g. Newburyport/Rockport overlapping for half its route), multi-platform + stations (i.e. trips using different stop IDs that are actually in the same + station), station busways (which, like platforms, have distinct stop IDs), and + shuttle stops (which may or may not be associated with a station). + """ + @spec build_timetable(Conn.t(), [Schedules.Schedule.t()]) :: %{ required(:trip_schedules) => %{ required({Schedules.Trip.id_t(), Stops.Stop.id_t()}) => Schedules.Schedule.t() }, - required(:all_stops) => [Stops.Stop.t()] + required(:trip_stops) => [Stops.Stop.t()] } - def build_timetable(all_stops, schedules, direction_id) do + def build_timetable(conn, schedules) do trip_schedules = Map.new(schedules, &trip_schedule(&1)) - all_stops = - remove_unused_stops(all_stops, schedules) - |> Enum.sort_by(fn stop -> - {zone_to_sortable(stop, direction_id), trip_schedule_sequence_for_stop(stop, schedules)} - end) + trip_stops = + conn.assigns.route.id + |> @route_patterns_repo.by_route_id( + direction_id: conn.assigns.direction_id, + canonical: Routes.Route.type_atom(conn.assigns.route) in [:commuter_rail, :subway] + ) + |> Enum.map(&@stops_repo.by_trip(&1.representative_trip_id)) + |> Enum.reduce(&merge_stop_lists(&1, &2, conn.assigns.direction_id)) + |> remove_unused_stops(schedules) %{ trip_schedules: trip_schedules, - all_stops: all_stops + trip_stops: trip_stops } end - # Override North and South Station zones from 1A to 0 - defp zone_to_sortable(%Stops.Stop{id: stop_id}, _direction_id) - when stop_id in ["place-sstat", "place-north"], - do: 0.0 + @spec merge_stop_lists([Stop.t()], [Stop.t()], 0 | 1) :: [Stop.t()] + defp merge_stop_lists(incoming_stops, base_stops, direction_id) do + if Enum.all?(incoming_stops, &contains_stop?(base_stops, &1)) do + base_stops + else + incoming_stops + |> Enum.reject(&contains_stop?(base_stops, &1)) + |> do_merge_stop_lists(base_stops, direction_id == 1) + end + end - defp zone_to_sortable(%Stops.Stop{zone: nil}, _direction_id), do: 0.0 + @spec contains_stop?([Stop.t()], Stop.t()) :: boolean() + defp contains_stop?(stops, %Stops.Stop{id: id, parent_id: parent_id}) do + stop_ids = + stops + |> Enum.flat_map(&[&1.id, &1.parent_id]) + |> Enum.reject(&is_nil/1) - defp zone_to_sortable(%Stops.Stop{zone: "1A"}, direction_id), - do: zone_to_sortable("0.5", direction_id) + id in stop_ids or parent_id in stop_ids + end - defp zone_to_sortable(%Stops.Stop{zone: zone}, direction_id), - do: zone_to_sortable("#{zone}.0", direction_id) + # Some shuttle stops must be placed manually within the existing stops. + @shuttle_overrides %{ + "38671" => "Weymouth Landing/East Braintree", + "NHRML-0127-B" => "Reading", + "14748" => "Lynn Interim" + } + @shuttle_ids Map.keys(@shuttle_overrides) - defp zone_to_sortable(zone, 0) when is_binary(zone), do: String.to_float(zone) - defp zone_to_sortable(zone, 1) when is_binary(zone), do: -String.to_float(zone) - defp zone_to_sortable(_, _), do: 0.0 + @spec do_merge_stop_lists([Stop.t()], [Stop.t()], boolean()) :: [Stop.t()] + defp do_merge_stop_lists(stops, [], _), do: stops - # translate each stop into a general stop_sequence value. a given stop will - # have a different value for stop_sequence depending on the other stops in the - # trip, so we summarize here by taking the maximum value - defp trip_schedule_sequence_for_stop(stop, schedules) do - schedules - |> Enum.filter(&(&1.stop == stop)) - |> Enum.map(& &1.stop_sequence) - |> Enum.max(fn -> 0 end) + defp do_merge_stop_lists( + [%Stop{id: id} = stop], + base_stops, + is_inbound? + ) + when id in @shuttle_ids do + next_stop_name = @shuttle_overrides[id] + + case Enum.find_index(base_stops, &match?(%Stop{name: ^next_stop_name}, &1)) do + nil -> + base_stops + + index -> + base_stops + |> List.insert_at(if(is_inbound?, do: index + 1, else: index), stop) + end + end + + defp do_merge_stop_lists(stops, base_stops, is_inbound?) do + if is_inbound? do + stops ++ base_stops + else + base_stops ++ stops + end end @spec trip_schedule(Schedules.Schedule.t()) :: @@ -376,8 +376,8 @@ defmodule DotcomWeb.ScheduleController.TimetableController do end defp remove_unused_stops(all_stops, schedules) do - timetable_stop = MapSet.new(schedules, & &1.stop.id) - Enum.filter(all_stops, &MapSet.member?(timetable_stop, &1.id)) + timetable_stops = Enum.map(schedules, & &1.stop) |> Enum.uniq() + Enum.filter(all_stops, &contains_stop?(timetable_stops, &1)) end defp channel_id(conn, _) do diff --git a/lib/dotcom_web/templates/schedule/_timetable.html.eex b/lib/dotcom_web/templates/schedule/_timetable.html.eex index 788a3753da..ab6052aac5 100644 --- a/lib/dotcom_web/templates/schedule/_timetable.html.eex +++ b/lib/dotcom_web/templates/schedule/_timetable.html.eex @@ -74,7 +74,7 @@ <% end %> <% end %> - <%= for {stop, idx} <- Enum.with_index(@all_stops) do %> + <%= for {stop, idx} <- @header_stops do %> <% cell_background = if rem(idx, 2) == 0 do "white" else "gray" end %> <%= content_tag :tr, [class: stop_row_class(idx)] do %> <%= content_tag :th, [ diff --git a/test/dotcom_web/controllers/schedule/timetable_controller_test.exs b/test/dotcom_web/controllers/schedule/timetable_controller_test.exs index 285af6d536..7801071a22 100644 --- a/test/dotcom_web/controllers/schedule/timetable_controller_test.exs +++ b/test/dotcom_web/controllers/schedule/timetable_controller_test.exs @@ -1,7 +1,8 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do @moduledoc false - use ExUnit.Case, async: true + use DotcomWeb.ConnCase, async: true import DotcomWeb.ScheduleController.TimetableController + import Mox alias Routes.Route alias Stops.Stop alias Schedules.{Schedule, Trip} @@ -69,10 +70,33 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do "name3-trip-5" => %{stop_name: "name3", stop_sequence: 5, trip_id: "trip-5"}, "shuttle-trip-4" => %{stop_name: "shuttle", stop_sequence: 4, trip_id: "trip-4"} } + @route %Route{id: "route1", type: 1} - describe "build_timetable/3" do - test "trip_schedules: a map from trip_id/stop_id to a schedule" do - %{trip_schedules: trip_schedules} = build_timetable(@stops, @schedules, 0) + setup :verify_on_exit! + + setup do + conn = + default_conn() + |> assign(:route, @route) + |> assign(:direction_id, 1) + + stub(RoutePatterns.Repo.Mock, :by_route_id, fn _, _ -> + [ + %RoutePatterns.RoutePattern{representative_trip_id: "trip-1"}, + %RoutePatterns.RoutePattern{representative_trip_id: "trip-2"} + ] + end) + + stub(Stops.Repo.Mock, :by_trip, fn _ -> + @stops + end) + + {:ok, %{conn: conn}} + end + + describe "build_timetable/2" do + test "trip_schedules: a map from trip_id/stop_id to a schedule", %{conn: conn} do + %{trip_schedules: trip_schedules} = build_timetable(conn, @schedules) for schedule <- @schedules do assert trip_schedules[{schedule.trip.id, schedule.stop.id}] == schedule @@ -81,18 +105,92 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do assert map_size(trip_schedules) == length(@schedules) end - test "all_stops: list of the stops in the same order" do - %{all_stops: all_stops} = build_timetable(@stops, @schedules, 0) + test "trip_stops: list of the stops in the same order", %{conn: conn} do + %{trip_stops: trip_stops} = build_timetable(conn, @schedules) - assert all_stops == @stops + assert trip_stops == @stops end - test "all_stops: if a stop isn't used, it's removed from the list" do + test "trip_stops: if a stop isn't used, it's removed from the list", %{conn: conn} do schedules = Enum.take(@schedules, 1) - %{all_stops: all_stops} = build_timetable(@stops, schedules, 1) + %{trip_stops: trip_stops} = build_timetable(conn, schedules) # other two stops were removed - assert [%{id: "1"}] = all_stops + assert [%{id: "1"}] = trip_stops + end + + test "trip_stops: merges stops from multiple route patterns", %{conn: conn} do + Stops.Repo.Mock + |> expect(:by_trip, fn "trip-1" -> + @stops + end) + |> expect(:by_trip, fn "trip-2" -> + [ + %Stop{id: "4"}, + %Stop{id: "5"}, + %Stop{id: "6"} + ] + end) + + schedules = + @schedules ++ + [ + %Schedule{stop: %Stop{id: "4"}, trip: %Trip{}}, + %Schedule{stop: %Stop{id: "5"}, trip: %Trip{}}, + %Schedule{stop: %Stop{id: "6"}, trip: %Trip{}} + ] + + %{trip_stops: trip_stops} = build_timetable(conn, schedules) + + assert Enum.map(trip_stops, & &1.id) == ["4", "5", "6", "1", "2", "3"] + + opposite_direction_conn = Plug.Conn.assign(conn, :direction_id, 0) + + Stops.Repo.Mock + |> expect(:by_trip, fn "trip-1" -> + @stops + end) + |> expect(:by_trip, fn "trip-2" -> + [ + %Stop{id: "4"}, + %Stop{id: "5"}, + %Stop{id: "6"} + ] + end) + + %{trip_stops: trip_stops} = build_timetable(opposite_direction_conn, schedules) + + assert Enum.map(trip_stops, & &1.id) == ["1", "2", "3", "4", "5", "6"] + end + + test "trip_stops: overrides position of certain shuttle stops", %{conn: conn} do + Stops.Repo.Mock + |> expect(:by_trip, fn "trip-1" -> + @stops ++ + [ + %Stop{id: "4", name: "Reading"}, + %Stop{id: "5"}, + %Stop{id: "6"} + ] + end) + |> expect(:by_trip, fn "trip-2" -> + [ + %Stop{id: "NHRML-0127-B"} + ] + end) + + schedules = + @schedules ++ + [ + %Schedule{stop: %Stop{id: "4"}, trip: %Trip{}}, + %Schedule{stop: %Stop{id: "5"}, trip: %Trip{}}, + %Schedule{stop: %Stop{id: "6"}, trip: %Trip{}}, + %Schedule{stop: %Stop{id: "NHRML-0127-B"}, trip: %Trip{}} + ] + + %{trip_stops: trip_stops} = build_timetable(conn, schedules) + + assert Enum.map(trip_stops, & &1.id) == ["1", "2", "3", "4", "NHRML-0127-B", "5", "6"] end end @@ -153,7 +251,7 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do [schedule_1 | _others] = @schedules assert nil == - track_change_for_schedule(schedule_1, MapSet.new(), fn _platform_stop_id -> + track_change_for_schedule(schedule_1, [], fn _platform_stop_id -> %Stop{id: 101, platform_code: "new-platform", platform_name: "New Platform"} end) end @@ -171,7 +269,7 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do assert nil == track_change_for_schedule( schedule_1, - MapSet.new([schedule_1.platform_stop_id]), + [schedule_1.platform_stop_id], fn _platform_stop_id -> platform_stop end ) end @@ -189,7 +287,7 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do assert platform_stop == track_change_for_schedule( schedule_1, - MapSet.new(["stop-1-other-patform"]), + ["stop-1-other-patform"], fn _platform_stop_id -> platform_stop end ) end @@ -207,7 +305,7 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do assert nil == track_change_for_schedule( schedule_1, - MapSet.new(["stop-1-other-patform"]), + ["stop-1-other-patform"], fn _platform_stop_id -> platform_stop end ) end diff --git a/test/dotcom_web/views/schedule/timetable_view_test.exs b/test/dotcom_web/views/schedule/timetable_view_test.exs index b6df9be657..d1900d7bfa 100644 --- a/test/dotcom_web/views/schedule/timetable_view_test.exs +++ b/test/dotcom_web/views/schedule/timetable_view_test.exs @@ -64,8 +64,8 @@ defmodule DotcomWeb.Schedule.TimetableViewTest do origin = destination = nil alerts = [] - all_stops = [ - %Stops.Stop{id: "stop", name: "Stop"} + header_stops = [ + {%Stops.Stop{id: "stop", name: "Stop"}, 0} ] vehicle_tooltips = vehicle_locations = trip_messages = trip_schedules = %{} @@ -85,7 +85,7 @@ defmodule DotcomWeb.Schedule.TimetableViewTest do alerts: alerts, offset: offset, show_date_select?: show_date_select?, - all_stops: all_stops, + header_stops: header_stops, vehicle_tooltips: vehicle_tooltips, vehicle_locations: vehicle_locations, trip_messages: trip_messages, @@ -152,7 +152,7 @@ defmodule DotcomWeb.Schedule.TimetableViewTest do Keyword.merge(assigns, header_schedules: header_schedules, track_changes: track_changes, - all_stops: [original_stop], + header_stops: [{original_stop, 0}], trip_schedules: trip_schedules )