fix(TimetableController): better reflect new service changes (#2070)

* hotfix(TimetableController): add stop to CR-Providence
* fix(TimetableController): omit trips with single stop
* hotfix(TimetableController): add and fix trip message
* fix(TimetableController): adjust route pattern sorting

lib/dotcom_web/controllers/schedule/timetable_controller.ex

@@ -1,8 +1,10 @@
 defmodule DotcomWeb.ScheduleController.TimetableController do
   @moduledoc "Handles the Timetable tab for commuter rail routes."
   use DotcomWeb, :controller
+
   alias Plug.Conn
   alias Routes.Route
+  alias RoutePatterns.RoutePattern
   alias Stops.Stop
   alias DotcomWeb.ScheduleView
 
@@ -54,7 +56,6 @@ defmodule DotcomWeb.ScheduleController.TimetableController do
         %{assigns: %{route: route, direction_id: direction_id, date_in_rating?: true}} = conn
       ) do
     timetable_schedules = timetable_schedules(conn)
-    header_schedules = header_schedules(timetable_schedules)
     vehicle_schedules = vehicle_schedules(conn, timetable_schedules)
     prior_stops = prior_stops(vehicle_schedules)
 
@@ -63,6 +64,11 @@ defmodule DotcomWeb.ScheduleController.TimetableController do
       trip_stops: trip_stops
     } = build_timetable(conn, timetable_schedules)
 
+    header_schedules =
+      trip_schedules
+      |> Map.values()
+      |> header_schedules()
+
     track_changes = track_changes(trip_schedules, Enum.map(trip_stops, & &1.id))
 
     header_stops =
@@ -159,7 +165,7 @@ defmodule DotcomWeb.ScheduleController.TimetableController do
   end
 
   def trip_messages(%Routes.Route{id: "CR-Franklin"}, 1) do
-    ["740", "728", "758", "732", "760"]
+    ["740", "752", "728", "758", "732", "760"]
     |> Enum.flat_map(&franklin_via_fairmount(&1, 1))
     |> Enum.into(%{})
   end
@@ -185,11 +191,11 @@ defmodule DotcomWeb.ScheduleController.TimetableController do
   end
 
   defp stops_for_fairmount(1) do
-    ["place-DB-0095", "place-forhl", "place-rugg", "place-bbsta"]
+    ["place-NEC-2203", "place-forhl", "place-rugg", "place-bbsta"]
   end
 
   defp stops_for_fairmount(0) do
-    ["place-bbsta", "place-rugg", "place-forhl", "place-NEC-2203", "place-DB-0095"]
+    ["place-bbsta", "place-rugg", "place-forhl", "place-NEC-2203"]
   end
 
   def make_via_list(list) do
@@ -246,17 +252,58 @@ defmodule DotcomWeb.ScheduleController.TimetableController do
         direction_id: conn.assigns.direction_id,
         canonical: Routes.Route.type_atom(conn.assigns.route) in [:commuter_rail, :subway]
       )
+      |> with_prioritized_pattern()
       |> Enum.map(&@stops_repo.by_trip(&1.representative_trip_id))
       |> Enum.reduce(&merge_stop_lists(&1, &2, inbound?))
       |> remove_unused_stops(schedules)
       |> add_new_stops(schedules, inbound?)
 
     %{
-      trip_schedules: trip_schedules,
+      trip_schedules:
+        trip_schedules
+        |> Map.reject(&schedule_from_other_stop?(&1, trip_stops))
+        |> remove_single_stop_trips(),
       trip_stops: trip_stops
     }
   end
 
+  # Timetable stops will be ordered based on the route patterns from which they
+  # correspond to, but some timetable PDFs are laid out differently than that
+  # from the default route pattern sort_order. For these cases, we need to
+  # manipulate this by adjusting which route pattern is processed first.
+  defp with_prioritized_pattern([%RoutePattern{route_id: "CR-Franklin"} | _] = route_patterns) do
+    route_patterns
+    |> with_prioritized_pattern("Foxboro")
+  end
+
+  defp with_prioritized_pattern([%RoutePattern{route_id: "CR-Providence"} | _] = route_patterns) do
+    route_patterns
+    |> with_prioritized_pattern("Stoughton")
+  end
+
+  defp with_prioritized_pattern(route_patterns), do: route_patterns
+
+  defp with_prioritized_pattern(route_patterns, pattern_name) do
+    Enum.sort_by(route_patterns, &String.contains?(&1.name, pattern_name), :desc)
+  end
+
+  defp schedule_from_other_stop?({{_, stop_id}, _}, stops) do
+    !contains_stop?(stops, %Stop{id: stop_id})
+  end
+
+  defp remove_single_stop_trips(trip_schedules) do
+    single_stop_trip_ids =
+      trip_schedules
+      |> Enum.frequencies_by(fn {{trip_id, _}, _} -> trip_id end)
+      |> Enum.filter(fn {_, count} -> count == 1 end)
+      |> Enum.map(fn {trip_id, _} -> trip_id end)
+
+    trip_schedules
+    |> Map.reject(fn {{trip_id, _}, _} ->
+      trip_id in single_stop_trip_ids
+    end)
+  end
+
   @spec merge_stop_lists([Stop.t()], [Stop.t()], boolean()) :: [Stop.t()]
   defp merge_stop_lists(incoming_stops, base_stops, inbound?) do
     if Enum.all?(incoming_stops, &contains_stop?(base_stops, &1)) do
@@ -286,7 +333,9 @@ defmodule DotcomWeb.ScheduleController.TimetableController do
     "38671" => "Weymouth Landing/East Braintree",
     "NHRML-0127-B" => "Reading",
     "place-ER-0115" => "Swampscott",
-    "place-wondl" => "Lynn"
+    "place-wondl" => "Lynn",
+    # Add Readville (canonically on other routes) back into the Providence timetable
+    "place-DB-0095" => "Route 128"
   }
   @shuttle_ids Map.keys(@shuttle_overrides)
 

test/dotcom_web/controllers/schedule/timetable_controller_test.exs

@@ -20,13 +20,27 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do
       trip: %Trip{id: "trip-1", name: "123"},
       platform_stop_id: "stop-1-platform-1"
     },
+    %Schedule{
+      stop_sequence: 2,
+      time: DateTime.from_unix!(600),
+      stop: %Stop{id: "2", name: "name2"},
+      trip: %Trip{id: "trip-1", name: "123"},
+      platform_stop_id: "stop-2-platform-1"
+    },
     %Schedule{
       stop_sequence: 2,
       time: DateTime.from_unix!(5000),
       stop: %Stop{id: "2", name: "name2"},
       trip: %Trip{id: "trip-2", name: "456"},
       platform_stop_id: "stop-2-platform-1"
     },
+    %Schedule{
+      stop_sequence: 3,
+      time: DateTime.from_unix!(6000),
+      stop: %Stop{id: "5", name: "name5"},
+      trip: %Trip{id: "trip-2", name: "456"},
+      platform_stop_id: "stop-5-platform-1"
+    },
     %Schedule{
       stop_sequence: 3,
       time: DateTime.from_unix!(50_000),
@@ -68,9 +82,15 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do
       trip_id: "trip-3"
     },
     "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"}
+    "shuttle-trip-4" => %{stop_name: "shuttle", stop_sequence: 4, trip_id: "trip-4"},
+    "name2-trip-1" => %{stop_name: "name2", stop_sequence: 2, trip_id: "trip-1"},
+    "name5-trip-2" => %{stop_name: "name5", stop_sequence: 3, trip_id: "trip-2"}
   }
   @route %Route{id: "route1", type: 1}
+  @route_patterns [
+    %RoutePatterns.RoutePattern{representative_trip_id: "trip-1"},
+    %RoutePatterns.RoutePattern{representative_trip_id: "trip-2"}
+  ]
 
   setup :verify_on_exit!
 
@@ -81,37 +101,33 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do
       |> 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
+      @route_patterns
     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
+      expect(Stops.Repo.Mock, :by_trip, length(@route_patterns), fn _ ->
+        @stops
+      end)
+
       %{trip_schedules: trip_schedules} = build_timetable(conn, @schedules)
 
-      for schedule <- @schedules do
-        assert trip_schedules[{schedule.trip.id, schedule.stop.id}] == schedule
+      for {key, value} <- trip_schedules do
+        assert %Schedules.Schedule{} = value
+        assert {<<_::binary>>, <<_::binary>>} = key
       end
 
-      assert map_size(trip_schedules) == length(@schedules)
-    end
-
-    test "trip_stops: list of the stops in the same order", %{conn: conn} do
-      %{trip_stops: trip_stops} = build_timetable(conn, @schedules)
-
-      assert trip_stops == @stops
+      assert map_size(trip_schedules) <= length(@schedules)
     end
 
     test "trip_stops: if a stop isn't used, it's removed from the list", %{conn: conn} do
+      expect(Stops.Repo.Mock, :by_trip, length(@route_patterns), fn _ ->
+        @stops
+      end)
+
       schedules = Enum.take(@schedules, 1)
 
       %{trip_stops: trip_stops} = build_timetable(conn, schedules)
@@ -240,7 +256,9 @@ defmodule DotcomWeb.ScheduleController.TimetableControllerTest do
                "trip-2-2" => "name2-trip-2",
                "trip-3-3" => "name3-trip-3",
                "trip-4-4" => "shuttle-trip-4",
-               "trip-5-5" => "name3-trip-5"
+               "trip-5-5" => "name3-trip-5",
+               "trip-1-2" => "name2-trip-1",
+               "trip-2-3" => "name5-trip-2"
              } ==
                stops
     end