refactor(TransitDetail): use trip struct (#2260)

* refactor(TransitDetail): use trip struct * cleanup: move route guard functions * refactor: better transit leg route/headsign text * format * update test to use trip headsigns * fix: improve flaky headsign tests * deps: bump open_trip_planner_client * feat: OTP gets trip direction_id * Update test/dotcom_web/live/trip_planner_test.exs Co-authored-by: Josh Larson <[email protected]> * order the test itinerary start times --------- Co-authored-by: Josh Larson <[email protected]>
mbta · Dec 13, 2024 · 0a8128e · 0a8128e
1 parent d497969
commit 0a8128e
Show file tree

Hide file tree

Showing 19 changed files with 194 additions and 133 deletions.
diff --git a/lib/dotcom/trip_plan/alerts.ex b/lib/dotcom/trip_plan/alerts.ex
@@ -42,7 +42,7 @@ defmodule Dotcom.TripPlan.Alerts do
     end
   end
 
-  defp mode_entities(%TransitDetail{route: route, trip_id: trip_id}) do
+  defp mode_entities(%TransitDetail{route: route, trip: %{id: trip_id}}) do
     trip =
       if is_nil(route.external_agency_name) do
         Schedules.Repo.trip(trip_id)

diff --git a/lib/dotcom/trip_plan/leg.ex b/lib/dotcom/trip_plan/leg.ex
@@ -41,12 +41,12 @@ defmodule Dotcom.TripPlan.Leg do
 
   @doc "Returns the trip ID for the leg, if present"
   @spec trip_id(t) :: {:ok, Schedules.Trip.id_t()} | :error
-  def trip_id(%__MODULE__{mode: %TransitDetail{trip_id: trip_id}}), do: {:ok, trip_id}
+  def trip_id(%__MODULE__{mode: %TransitDetail{trip: %{id: trip_id}}}), do: {:ok, trip_id}
   def trip_id(%__MODULE__{}), do: :error
 
   @spec route_trip_ids(t) :: {:ok, {Routes.Route.id_t(), Schedules.Trip.id_t()}} | :error
   def route_trip_ids(%__MODULE__{mode: %TransitDetail{} = mode}) do
-    {:ok, {mode.route.id, mode.trip_id}}
+    {:ok, {mode.route.id, mode.trip.id}}
   end
 
   def route_trip_ids(%__MODULE__{}) do

diff --git a/lib/dotcom/trip_plan/parser.ex b/lib/dotcom/trip_plan/parser.ex
@@ -103,7 +103,7 @@ defmodule Dotcom.TripPlan.Parser do
       mode: mode,
       intermediate_stops: Enum.map(stops, &build_stop/1),
       route: build_route(route, agency_name),
-      trip_id: id_from_gtfs(trip.gtfs_id)
+      trip: build_trip(trip)
     }
   end
 
@@ -132,6 +132,22 @@ defmodule Dotcom.TripPlan.Parser do
     }
   end
 
+  defp build_trip(%Schema.Trip{
+         direction_id: direction_id,
+         gtfs_id: gtfs_id,
+         trip_headsign: headsign,
+         trip_short_name: name
+       }) do
+    id = id_from_gtfs(gtfs_id)
+
+    %Schedules.Trip{
+      direction_id: if(direction_id in ["0", "1"], do: String.to_integer(direction_id)),
+      id: id,
+      name: name,
+      headsign: headsign
+    }
+  end
+
   defp route_name("Logan Express", _short_name, long_name), do: "#{long_name} Logan Express"
 
   defp route_name("Massport", short_name, long_name) do

diff --git a/lib/dotcom/trip_plan/related_link.ex b/lib/dotcom/trip_plan/related_link.ex
@@ -77,9 +77,9 @@ defmodule Dotcom.TripPlan.RelatedLink do
   defp route_links(itinerary) do
     not_shuttle? = fn route -> route.description !== :rail_replacement_bus end
 
-    for %Leg{mode: %TransitDetail{route: route, trip_id: trip_id}} <- itinerary.legs,
+    for %Leg{mode: %TransitDetail{route: route, trip: trip}} <- itinerary.legs,
         not_shuttle?.(route) do
-      route_link(route, trip_id, itinerary)
+      route_link(route, if(trip, do: trip.id), itinerary)
     end
   end
 
@@ -104,10 +104,10 @@ defmodule Dotcom.TripPlan.RelatedLink do
     new("Massport schedules", url, :massport_shuttle)
   end
 
-  defp route_link(route, trip_id, itinerary) do
+  defp route_link(route, _, itinerary) do
     icon_name = Route.icon_atom(route)
     date = Date.to_iso8601(itinerary.start)
-    url = schedule_path(DotcomWeb.Endpoint, :show, route, date: date, trip: trip_id)
+    url = schedule_path(DotcomWeb.Endpoint, :show, route, date: date)
 
     route
     |> link_text()

diff --git a/lib/dotcom/trip_plan/transit_detail.ex b/lib/dotcom/trip_plan/transit_detail.ex
@@ -14,14 +14,14 @@ defmodule Dotcom.TripPlan.TransitDetail do
             },
             mode: :TRANSIT,
             route: nil,
-            trip_id: "",
+            trip: nil,
             intermediate_stops: []
 
   @type t :: %__MODULE__{
           fares: fares,
           mode: Leg.mode(),
           route: Routes.Route.t(),
-          trip_id: Schedules.Trip.id_t(),
+          trip: Schedules.Trip.t(),
           intermediate_stops: [Stops.Stop.t()]
         }
 

diff --git a/lib/dotcom_web/components/route_symbols.ex b/lib/dotcom_web/components/route_symbols.ex
@@ -6,19 +6,13 @@ defmodule DotcomWeb.Components.RouteSymbols do
   use Phoenix.Component
 
   import MbtaMetro.Components.Icon
+  import Routes.Route, only: [is_external?: 1, is_shuttle?: 1]
 
   alias Routes.Route
 
   @logan_express_icon_names Route.logan_express_icon_names()
   @massport_icon_names Route.massport_icon_names()
 
-  defguardp is_external_route?(assigns) when not is_nil(assigns.route.external_agency_name)
-
-  defguardp is_shuttle_route?(assigns)
-            when assigns.route.type == 3 and
-                   assigns.route.description == :rail_replacement_bus and
-                   not is_external_route?(assigns)
-
   variant(
     :size,
     [
@@ -55,8 +49,8 @@ defmodule DotcomWeb.Components.RouteSymbols do
   <.route_symbol route={%Routes.Route{type: 3}} data-toggle="tooltip" />
   ```
   """
-  def route_symbol(%{route: %Route{description: description, type: 3}} = assigns)
-      when not is_external_route?(assigns) and not is_shuttle_route?(assigns) and
+  def route_symbol(%{route: %Route{description: description, type: 3} = route} = assigns)
+      when not is_external?(route) and not is_shuttle?(route) and
              description != :rapid_transit do
     route_class =
       if(Routes.Route.silver_line?(assigns.route),
@@ -88,8 +82,8 @@ defmodule DotcomWeb.Components.RouteSymbols do
   attr(:rest, :global)
   attr(:route, Routes.Route, required: true)
 
-  def route_icon(%{route: %Route{name: shuttle_name}} = assigns)
-      when is_shuttle_route?(assigns) do
+  def route_icon(%{route: %Route{name: shuttle_name} = route} = assigns)
+      when is_shuttle?(route) do
     route_class =
       shuttle_name
       |> String.replace(" Shuttle", "")
@@ -110,7 +104,7 @@ defmodule DotcomWeb.Components.RouteSymbols do
     """
   end
 
-  def route_icon(assigns) when not is_external_route?(assigns) do
+  def route_icon(%{route: route} = assigns) when not is_external?(route) do
     assigns =
       assigns
       |> assign_new(:icon_name, fn %{route: route, size: size} ->

diff --git a/lib/dotcom_web/components/trip_planner/itinerary_detail.ex b/lib/dotcom_web/components/trip_planner/itinerary_detail.ex
@@ -39,7 +39,7 @@ defmodule DotcomWeb.Components.TripPlanner.ItineraryDetail do
     ~H"""
     <div :if={Enum.count(@itineraries) > 1}>
       <p class="text-sm mb-2 mt-3">Depart at</p>
-      <div class="flex flex-wrap gap-2">
+      <div id="itinerary-detail-departure-times" class="flex flex-wrap gap-2">
         <.depart_at_button
           :for={{itinerary, index} <- Enum.with_index(@itineraries)}
           active={@selected_itinerary_detail_index == index}

diff --git a/lib/dotcom_web/components/trip_planner/transit_leg.ex b/lib/dotcom_web/components/trip_planner/transit_leg.ex
@@ -9,6 +9,7 @@ defmodule DotcomWeb.Components.TripPlanner.TransitLeg do
   import DotcomWeb.Components.RouteSymbols, only: [route_symbol: 1]
   import DotcomWeb.Components.TripPlanner.Place
   import MbtaMetro.Components.Icon, only: [icon: 1]
+  import Routes.Route, only: [is_external?: 1, is_shuttle?: 1]
 
   alias Dotcom.TripPlan.TransitDetail
   alias Routes.Route
@@ -74,31 +75,67 @@ defmodule DotcomWeb.Components.TripPlanner.TransitLeg do
   defp leg_line_class(_), do: ""
 
   defp leg_summary(assigns) do
-    assigns = assign(assigns, :stops_count, Enum.count(assigns.leg.mode.intermediate_stops) + 1)
+    assigns =
+      assigns
+      |> assign(:stops_count, Enum.count(assigns.leg.mode.intermediate_stops) + 1)
+      |> assign(:headsign, headsign(assigns.leg.mode))
 
     ~H"""
     <div class="gap-x-1 py-2 grid grid-rows-2 grid-cols-[min-content_max-content] pl-4">
       <.route_symbol route={@leg.mode.route} />
-      <span class="font-semibold">{@leg.mode.trip_id}</span>
+      <span class="font-semibold">{@headsign}</span>
       <div class="text-sm col-start-2 row-start-2">
-        Ride the {route_name(@leg.mode.route)}
+        <.ride_message mode={@leg.mode} />
         <span class="font-semibold">{@stops_count} {Inflex.inflect("stop", @stops_count)}</span>
       </div>
     </div>
     """
   end
 
+  # Massport trips might not have headsigns, so use the route names instead
+  defp headsign(%{route: %Route{} = route}) when is_external?(route) do
+    route_name(route)
+  end
+
+  defp headsign(%{trip: %{headsign: headsign}}) when not is_nil(headsign) do
+    headsign
+  end
+
+  defp headsign(_), do: nil
+
+  defp ride_message(%{mode: %{route: %Route{} = route}} = assigns)
+       when is_external?(route) do
+    assigns =
+      assigns
+      |> assign(:vehicle_name, vehicle_name(route))
+
+    ~H"""
+    Ride the {@vehicle_name}
+    """
+  end
+
+  defp ride_message(%{mode: %{route: route, trip: trip}} = assigns) do
+    assigns =
+      assigns
+      |> assign(:route_name, route_name(route))
+      |> assign(:train_number, train_number(trip))
+      |> assign(:vehicle_name, vehicle_name(route))
+
+    ~H"""
+    Ride the {@route_name} {if @train_number, do: "Train #{@train_number}", else: @vehicle_name}
+    """
+  end
+
   # Returns the name of the route for the given row.
   # If there is an external agency name, we use the long name.
   # If it is a bus, we use the short name.
   # For all others, we use the long name.
-  defp route_name(%Route{external_agency_name: agency, long_name: long_name})
-       when is_binary(agency) and is_binary(long_name),
-       do: long_name
+  defp route_name(%Route{} = route) when is_external?(route),
+    do: route.long_name
 
   defp route_name(%Route{name: name, type: 3})
        when is_binary(name),
-       do: "#{name} bus"
+       do: name
 
   defp route_name(%Route{long_name: long_name})
        when is_binary(long_name),
@@ -107,6 +144,21 @@ defmodule DotcomWeb.Components.TripPlanner.TransitLeg do
   defp route_name(%Route{name: name}), do: name
   defp route_name(_), do: nil
 
+  defp train_number(%Schedules.Trip{name: name}) when not is_nil(name), do: name
+  defp train_number(_), do: nil
+
+  defp vehicle_name(%Route{} = route) when is_shuttle?(route) do
+    "shuttle bus"
+  end
+
+  defp vehicle_name(%Route{} = route) do
+    route
+    |> Route.vehicle_name()
+    |> String.downcase()
+  end
+
+  defp vehicle_name(nil), do: nil
+
   defp leg_details(assigns) do
     ~H"""
     <ul class="w-full m-0 pl-4 flex flex-col divide-y divide-gray-lighter">

diff --git a/lib/dotcom_web/components/trip_planner/trip_planner_results_section.ex b/lib/dotcom_web/components/trip_planner/trip_planner_results_section.ex
@@ -97,26 +97,27 @@ defmodule DotcomWeb.Components.TripPlanner.TripPlannerResultsSection do
               }}
          } = assigns
        ) do
-    %{itineraries: itineraries, summary: summary} = results |> Enum.at(itinerary_group_index)
-
-    assigns =
-      assigns
-      |> assign(:itineraries, itineraries)
-      |> assign(:summary, summary)
-      |> assign(:itinerary_index, itinerary_index)
+    with %{itineraries: itineraries, summary: summary} <-
+           results |> Enum.at(itinerary_group_index) do
+      assigns =
+        assigns
+        |> assign(:itineraries, itineraries)
+        |> assign(:summary, summary)
+        |> assign(:itinerary_index, itinerary_index)
+
+      ~H"""
+      <div class="mt-30">
+        <div class="border-b-[1px] border-gray-lighter">
+          <.itinerary_summary summary={@summary} />
+        </div>
 
-    ~H"""
-    <div class="mt-30">
-      <div class="border-b-[1px] border-gray-lighter">
-        <.itinerary_summary summary={@summary} />
+        <.itinerary_detail
+          itineraries={@itineraries}
+          selected_itinerary_detail_index={@itinerary_index}
+        />
       </div>
-
-      <.itinerary_detail
-        itineraries={@itineraries}
-        selected_itinerary_detail_index={@itinerary_index}
-      />
-    </div>
-    """
+      """
+    end
   end
 
   defp itinerary_panel(assigns) do

diff --git a/lib/routes/route.ex b/lib/routes/route.ex
@@ -92,6 +92,12 @@ defmodule Routes.Route do
   @silver_line ~w(741 742 743 746 749 751)
   @silver_line_set MapSet.new(@silver_line)
 
+  defguard is_external?(route) when not is_nil(route.external_agency_name)
+
+  defguard is_shuttle?(route)
+           when route.type == 3 and route.description == :rail_replacement_bus and
+                  not is_external?(route)
+
   def logan_express_icon_names, do: @logan_express_icon_names
   def massport_icon_names, do: @massport_icon_names
 

diff --git a/mix.exs b/mix.exs
@@ -113,7 +113,7 @@ defmodule DotCom.Mixfile do
       {:nebulex_redis_adapter, "2.4.1"},
       {
         :open_trip_planner_client,
-        [github: "mbta/open_trip_planner_client", tag: "v0.11.2"]
+        [github: "mbta/open_trip_planner_client", tag: "v0.11.3"]
       },
       {:parallel_stream, "1.1.0"},
       # latest version 1.7.14

diff --git a/mix.lock b/mix.lock
@@ -72,7 +72,7 @@
   "nimble_ownership": {:hex, :nimble_ownership, "1.0.0", "3f87744d42c21b2042a0aa1d48c83c77e6dd9dd357e425a038dd4b49ba8b79a1", [:mix], [], "hexpm", "7c16cc74f4e952464220a73055b557a273e8b1b7ace8489ec9d86e9ad56cb2cc"},
   "nimble_parsec": {:hex, :nimble_parsec, "1.4.0", "51f9b613ea62cfa97b25ccc2c1b4216e81df970acd8e16e8d1bdc58fef21370d", [:mix], [], "hexpm", "9c565862810fb383e9838c1dd2d7d2c437b3d13b267414ba6af33e50d2d1cf28"},
   "nimble_pool": {:hex, :nimble_pool, "1.1.0", "bf9c29fbdcba3564a8b800d1eeb5a3c58f36e1e11d7b7fb2e084a643f645f06b", [:mix], [], "hexpm", "af2e4e6b34197db81f7aad230c1118eac993acc0dae6bc83bac0126d4ae0813a"},
-  "open_trip_planner_client": {:git, "https://github.com/mbta/open_trip_planner_client.git", "3bf74242bda2c4174bbbc48363decd864562d0e9", [tag: "v0.11.2"]},
+  "open_trip_planner_client": {:git, "https://github.com/mbta/open_trip_planner_client.git", "b127ccfcc7cdfc40bc91e30c55d41754426a7cc9", [tag: "v0.11.3"]},
   "parallel_stream": {:hex, :parallel_stream, "1.1.0", "f52f73eb344bc22de335992377413138405796e0d0ad99d995d9977ac29f1ca9", [:mix], [], "hexpm", "684fd19191aedfaf387bbabbeb8ff3c752f0220c8112eb907d797f4592d6e871"},
   "parse_trans": {:hex, :parse_trans, "3.4.1", "6e6aa8167cb44cc8f39441d05193be6e6f4e7c2946cb2759f015f8c56b76e5ff", [:rebar3], [], "hexpm", "620a406ce75dada827b82e453c19cf06776be266f5a67cff34e1ef2cbb60e49a"},
   "phoenix": {:hex, :phoenix, "1.7.14", "a7d0b3f1bc95987044ddada111e77bd7f75646a08518942c72a8440278ae7825", [:mix], [{:castore, ">= 0.0.0", [hex: :castore, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:phoenix_pubsub, "~> 2.1", [hex: :phoenix_pubsub, repo: "hexpm", optional: false]}, {:phoenix_template, "~> 1.0", [hex: :phoenix_template, repo: "hexpm", optional: false]}, {:phoenix_view, "~> 2.0", [hex: :phoenix_view, repo: "hexpm", optional: true]}, {:plug, "~> 1.14", [hex: :plug, repo: "hexpm", optional: false]}, {:plug_cowboy, "~> 2.7", [hex: :plug_cowboy, repo: "hexpm", optional: true]}, {:plug_crypto, "~> 1.2 or ~> 2.0", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}, {:websock_adapter, "~> 0.5.3", [hex: :websock_adapter, repo: "hexpm", optional: false]}], "hexpm", "c7859bc56cc5dfef19ecfc240775dae358cbaa530231118a9e014df392ace61a"},

diff --git a/test/dotcom/trip_plan/itinerary_row_list_test.exs b/test/dotcom/trip_plan/itinerary_row_list_test.exs
@@ -179,7 +179,7 @@ defmodule Dotcom.TripPlan.ItineraryRowListTest do
         to: %NamedPosition{stop: %Stops.Stop{id: "place-pktrm"}, name: "Park Street"},
         mode: %TransitDetail{
           route: %Routes.Route{id: "Green-C"},
-          trip_id: "Green-1",
+          trip: %Schedules.Trip{id: "Green-1"},
           intermediate_stops: []
         }
       }
@@ -248,7 +248,7 @@ defmodule Dotcom.TripPlan.ItineraryRowListTest do
         to: %NamedPosition{stop: %Stops.Stop{id: "place-kencl"}, name: "Kenmore"},
         mode: %TransitDetail{
           route: %Routes.Route{id: "Green-C"},
-          trip_id: "Green-1",
+          trip: %Schedules.Trip{id: "Green-1"},
           intermediate_stops: []
         }
       }

diff --git a/test/dotcom/trip_plan/itinerary_test.exs b/test/dotcom/trip_plan/itinerary_test.exs
@@ -54,7 +54,7 @@ defmodule Dotcom.TripPlan.ItineraryTest do
       test_calculated_ids =
         Enum.flat_map(itinerary, fn leg ->
           case leg.mode do
-            %TransitDetail{trip_id: trip_id} -> [trip_id]
+            %TransitDetail{trip: %{id: trip_id}} -> [trip_id]
             _ -> []
           end
         end)
@@ -68,7 +68,7 @@ defmodule Dotcom.TripPlan.ItineraryTest do
       test_calculated_ids =
         Enum.flat_map(itinerary.legs, fn leg ->
           case leg.mode do
-            %TransitDetail{} = td -> [{td.route.id, td.trip_id}]
+            %TransitDetail{} = td -> [{td.route.id, td.trip.id}]
             _ -> []
           end
         end)

diff --git a/test/dotcom/trip_plan/leg_test.exs b/test/dotcom/trip_plan/leg_test.exs
@@ -48,7 +48,7 @@ defmodule Dotcom.TripPlan.LegTest do
 
   describe "trip_id/1" do
     test "returns {:ok, id} for a transit leg", context do
-      trip_id = context.transit_leg.mode.trip_id
+      trip_id = context.transit_leg.mode.trip.id
       assert {:ok, ^trip_id} = trip_id(context.transit_leg)
     end
 

diff --git a/test/dotcom/trip_plan/related_link_test.exs b/test/dotcom/trip_plan/related_link_test.exs
@@ -35,10 +35,8 @@ defmodule Dotcom.TripPlan.RelatedLinkTest do
         fn id -> %Stops.Stop{id: id} end
       )
 
-      [trip_id] = Itinerary.trip_ids(itinerary)
       assert [route_link, fare_link] = links_for_itinerary(itinerary)
       assert url(route_link) =~ Timex.format!(itinerary.start, "date={ISOdate}")
-      assert url(route_link) =~ ~s(trip=#{trip_id}) |> URI.encode()
       assert fare_link.text == "View fare information"
 
       if route.type == 3 do