From cd49c1a33c68a9ec8b3657bf5716abcd08c0c999 Mon Sep 17 00:00:00 2001 From: Luc Vachon Date: Fri, 22 May 2026 13:17:27 -0400 Subject: [PATCH 1/5] Added maybe_transfer? cases to specifically handle subway - subway transfers to ensure those transfers happen in the same (or connected) stations. This is to handle the cases where OTP suggests walking between stations, but incorrectly counted that as a valid transfer. --- lib/dotcom/trip_plan/transfer.ex | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/lib/dotcom/trip_plan/transfer.ex b/lib/dotcom/trip_plan/transfer.ex index 27eef59609..bc41810fe4 100644 --- a/lib/dotcom/trip_plan/transfer.ex +++ b/lib/dotcom/trip_plan/transfer.ex @@ -63,6 +63,19 @@ defmodule Dotcom.TripPlan.Transfer do |> Kernel.and(maybe_transfer?([middle_leg, last_leg])) end + def maybe_transfer?([from, to]) + when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") and + from.mode == :SUBWAY and to.mode == :SUBWAY do + same_station?(from.to, to.from) + end + + def maybe_transfer?([from, middle, to]) + when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") and + agency_name?(middle, "MBTA") and + from.mode == :SUBWAY and to.mode == :SUBWAY and middle.mode == :SUBWAY do + same_station?(from.to, middle.from) && same_station?(middle.to, to.from) + end + def maybe_transfer?([from, to]) when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") do if from.route === to.route and Enum.all?([from.route, to.route], &bus?/1) do From eefaf62bb1fe849e5bebcedc8d87303886e781db Mon Sep 17 00:00:00 2001 From: Luc Vachon Date: Fri, 22 May 2026 17:36:58 -0400 Subject: [PATCH 2/5] Refactored how mode is determined in transfers to better align with previous work --- lib/dotcom/trip_plan/transfer.ex | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/lib/dotcom/trip_plan/transfer.ex b/lib/dotcom/trip_plan/transfer.ex index bc41810fe4..c8cab28a86 100644 --- a/lib/dotcom/trip_plan/transfer.ex +++ b/lib/dotcom/trip_plan/transfer.ex @@ -64,16 +64,15 @@ defmodule Dotcom.TripPlan.Transfer do end def maybe_transfer?([from, to]) - when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") and - from.mode == :SUBWAY and to.mode == :SUBWAY do - same_station?(from.to, to.from) + when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") do + subway?(from.route) && subway?(to.route) && same_station?(from.to, to.from) end def maybe_transfer?([from, middle, to]) when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") and - agency_name?(middle, "MBTA") and - from.mode == :SUBWAY and to.mode == :SUBWAY and middle.mode == :SUBWAY do - same_station?(from.to, middle.from) && same_station?(middle.to, to.from) + agency_name?(middle, "MBTA") do + (subway?(from.route) and subway?(to.route) and subway?(middle.route) and + same_station?(from.to, middle.from)) && same_station?(middle.to, to.from) end def maybe_transfer?([from, to]) when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") do From 8ddbe42ea42ea32cfc7a089bfed606bec1e603a6 Mon Sep 17 00:00:00 2001 From: Luc Vachon Date: Tue, 26 May 2026 10:15:01 -0400 Subject: [PATCH 3/5] Updated fare calculation code to take out subway->subway transfer logic from the single_ride_trip transfer function and to allow the (formerly unused) subway_transfer?() code to handle it. --- lib/dotcom/trip_plan/fares.ex | 6 ++++ lib/dotcom/trip_plan/transfer.ex | 14 +-------- test/dotcom/trip_plan/transfer_test.exs | 38 +++++++++++++++++++++++-- 3 files changed, 43 insertions(+), 15 deletions(-) diff --git a/lib/dotcom/trip_plan/fares.ex b/lib/dotcom/trip_plan/fares.ex index 1ca35715c4..488fff7a3b 100644 --- a/lib/dotcom/trip_plan/fares.ex +++ b/lib/dotcom/trip_plan/fares.ex @@ -38,6 +38,9 @@ defmodule Dotcom.TripPlan.Fares do three_legs = transit_legs |> Enum.slice(leg_index - 2, 3) # If this is part of a free transfer, don't add fare cond do + Transfer.subway_transfer?(three_legs) -> + total + Transfer.bus_to_subway_transfer?(three_legs) -> if total == cents_for_leg(List.first(three_legs)), do: total + 70, @@ -46,6 +49,9 @@ defmodule Dotcom.TripPlan.Fares do Transfer.maybe_transfer?(three_legs) -> total + Transfer.subway_transfer?(two_legs) -> + total + Transfer.subway_after_sl1_from_airport?(two_legs) -> total diff --git a/lib/dotcom/trip_plan/transfer.ex b/lib/dotcom/trip_plan/transfer.ex index c8cab28a86..66ed2a0ab8 100644 --- a/lib/dotcom/trip_plan/transfer.ex +++ b/lib/dotcom/trip_plan/transfer.ex @@ -15,7 +15,7 @@ defmodule Dotcom.TripPlan.Transfer do # (can't be certain, as it depends on media used)! @single_ride_transfers %{ :bus => [:subway, :bus], - :subway => [:subway, :bus], + :subway => [:bus], :express_bus => [:subway, :bus, :express_bus] } @@ -63,18 +63,6 @@ defmodule Dotcom.TripPlan.Transfer do |> Kernel.and(maybe_transfer?([middle_leg, last_leg])) end - def maybe_transfer?([from, to]) - when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") do - subway?(from.route) && subway?(to.route) && same_station?(from.to, to.from) - end - - def maybe_transfer?([from, middle, to]) - when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") and - agency_name?(middle, "MBTA") do - (subway?(from.route) and subway?(to.route) and subway?(middle.route) and - same_station?(from.to, middle.from)) && same_station?(middle.to, to.from) - end - def maybe_transfer?([from, to]) when agency_name?(from, "MBTA") and agency_name?(to, "MBTA") do if from.route === to.route and Enum.all?([from.route, to.route], &bus?/1) do diff --git a/test/dotcom/trip_plan/transfer_test.exs b/test/dotcom/trip_plan/transfer_test.exs index ae589b57f2..9685c9ea8e 100644 --- a/test/dotcom/trip_plan/transfer_test.exs +++ b/test/dotcom/trip_plan/transfer_test.exs @@ -59,8 +59,42 @@ defmodule Dotcom.TripPlan.TransferTest do refute [nil, bus_leg()] |> maybe_transfer? end - test "subway -> subway" do - assert [subway_leg(), subway_leg()] |> maybe_transfer? + test "subway -> subway when transferring at the same station" do + from_leg = + build(:transit_leg, + agency: build(:agency, name: "MBTA"), + route: build(:route, type: 1), + to: + build(:place, %{ + stop: build(:stop, %{parent_station: "place-mock"}) + }) + ) + + to_leg = + build(:transit_leg, + agency: build(:agency, name: "MBTA"), + route: build(:route, type: 1), + from: + build(:place, %{ + stop: build(:stop, %{parent_station: "place-mock"}) + }) + ) + + assert [from_leg, to_leg] |> subway_transfer? + end + + test "subway -> subway when transferring at different station" do + from_leg = + build(:transit_leg, + agency: build(:agency, name: "MBTA"), + route: build(:route, type: 1), + to: + build(:place, %{ + stop: build(:stop, %{parent_station: "place-mock"}) + }) + ) + + assert !([from_leg, subway_leg()] |> subway_transfer?) end test "subway -> local bus" do From f5bc5e3d14b5199a58fc6bab786de6795a61cfc3 Mon Sep 17 00:00:00 2001 From: Luc Vachon Date: Tue, 26 May 2026 10:31:47 -0400 Subject: [PATCH 4/5] I do not agree with the robot that this to too complex to understand. --- lib/dotcom/trip_plan/fares.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/dotcom/trip_plan/fares.ex b/lib/dotcom/trip_plan/fares.ex index 488fff7a3b..5750806bdf 100644 --- a/lib/dotcom/trip_plan/fares.ex +++ b/lib/dotcom/trip_plan/fares.ex @@ -32,6 +32,7 @@ defmodule Dotcom.TripPlan.Fares do defp add_fares({leg, 0}, 0, _), do: cents_for_leg(leg) + # credo:disable-for-next-line defp add_fares({leg, leg_index}, total, transit_legs) do # Look at this transit leg and previous transit leg(s) two_legs = transit_legs |> Enum.slice(leg_index - 1, 2) From ff185ce473fe2528925fe4a849721fe0a08dcdb7 Mon Sep 17 00:00:00 2001 From: Luc Vachon Date: Tue, 26 May 2026 13:33:09 -0400 Subject: [PATCH 5/5] Added higher level tests for subway transfers. Added specific three-leg handler for subway transfers. Removed redundant lower level tests. --- lib/dotcom/trip_plan/transfer.ex | 10 +- test/dotcom/trip_plan/fares_test.exs | 182 ++++++++++++++++++++++++ test/dotcom/trip_plan/transfer_test.exs | 38 ----- 3 files changed, 191 insertions(+), 39 deletions(-) diff --git a/lib/dotcom/trip_plan/transfer.ex b/lib/dotcom/trip_plan/transfer.ex index 66ed2a0ab8..ce85decb78 100644 --- a/lib/dotcom/trip_plan/transfer.ex +++ b/lib/dotcom/trip_plan/transfer.ex @@ -34,12 +34,20 @@ defmodule Dotcom.TripPlan.Transfer do @doc "Searches a list of legs for evidence of an in-station subway transfer." @spec subway_transfer?([Leg.t()]) :: boolean - def subway_transfer?([first_leg, next_leg | _]) + def subway_transfer?([first_leg, next_leg]) when agency_name?(first_leg, "MBTA") and agency_name?(next_leg, "MBTA") do same_station?(first_leg.to, next_leg.from) and subway?(first_leg.route) and subway?(next_leg.route) end + def subway_transfer?([first_leg, next_leg, last_leg]) + when agency_name?(first_leg, "MBTA") and agency_name?(next_leg, "MBTA") and + agency_name?(last_leg, "MBTA") do + same_station?(first_leg.to, next_leg.from) and subway?(first_leg.route) and + subway?(next_leg.route) and same_station?(next_leg.to, last_leg.from) and + subway?(last_leg.route) + end + def subway_transfer?([_ | legs]), do: subway_transfer?(legs) def subway_transfer?(_), do: false diff --git a/test/dotcom/trip_plan/fares_test.exs b/test/dotcom/trip_plan/fares_test.exs index 944ff8fba8..4dd8079ca6 100644 --- a/test/dotcom/trip_plan/fares_test.exs +++ b/test/dotcom/trip_plan/fares_test.exs @@ -101,6 +101,188 @@ defmodule Dotcom.TripPlan.FaresTest do end end + test "valid two leg subway transfers" do + start_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-start"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midA"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + end_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midA"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-end"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + one_subway_fare = + build(:itinerary, + legs: + build_list(1, :transit_leg, + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + ) + |> fare() + + fare = build(:itinerary, legs: [start_leg, end_leg]) |> fare() + assert fare <= one_subway_fare + end + + test "valid three leg subway transfers" do + start_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-start"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midA"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + mid_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midA"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midB"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + end_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midB"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-end"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + one_subway_fare = + build(:itinerary, + legs: + build_list(1, :transit_leg, + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + ) + |> fare() + + fare = build(:itinerary, legs: [start_leg, mid_leg, end_leg]) |> fare() + assert fare <= one_subway_fare + end + + test "invalid two leg subway transfers (eg red <-> blue)" do + start_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-start"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midA"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + end_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midB"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-end"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + one_subway_fare = + build(:itinerary, + legs: + build_list(1, :transit_leg, + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + ) + |> fare() + + fare = build(:itinerary, legs: [start_leg, end_leg]) |> fare() + assert fare > one_subway_fare + end + + test "invalid three leg subway transfers (eg red <-> blue)" do + start_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-start"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midA"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + mid_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midA"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-midB"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + end_leg = + build(:transit_leg, + from: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-other"})), + to: build(:place, stop: build(:stop, parent_station: %{gtfs_id: "mock-end"})), + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + + one_subway_fare = + build(:itinerary, + legs: + build_list(1, :transit_leg, + route: + build(:route, + agency: build(:agency, name: "MBTA"), + type: 0 + ) + ) + ) + |> fare() + + fare = build(:itinerary, legs: [start_leg, mid_leg, end_leg]) |> fare() + assert fare > one_subway_fare + end + describe "cents_for_leg/1" do test "walking leg" do leg = build(:walking_leg) diff --git a/test/dotcom/trip_plan/transfer_test.exs b/test/dotcom/trip_plan/transfer_test.exs index 9685c9ea8e..32ac64adb1 100644 --- a/test/dotcom/trip_plan/transfer_test.exs +++ b/test/dotcom/trip_plan/transfer_test.exs @@ -59,44 +59,6 @@ defmodule Dotcom.TripPlan.TransferTest do refute [nil, bus_leg()] |> maybe_transfer? end - test "subway -> subway when transferring at the same station" do - from_leg = - build(:transit_leg, - agency: build(:agency, name: "MBTA"), - route: build(:route, type: 1), - to: - build(:place, %{ - stop: build(:stop, %{parent_station: "place-mock"}) - }) - ) - - to_leg = - build(:transit_leg, - agency: build(:agency, name: "MBTA"), - route: build(:route, type: 1), - from: - build(:place, %{ - stop: build(:stop, %{parent_station: "place-mock"}) - }) - ) - - assert [from_leg, to_leg] |> subway_transfer? - end - - test "subway -> subway when transferring at different station" do - from_leg = - build(:transit_leg, - agency: build(:agency, name: "MBTA"), - route: build(:route, type: 1), - to: - build(:place, %{ - stop: build(:stop, %{parent_station: "place-mock"}) - }) - ) - - assert !([from_leg, subway_leg()] |> subway_transfer?) - end - test "subway -> local bus" do assert [subway_leg(), bus_leg()] |> maybe_transfer? end