From b5b918179450018389ac9883d332582463fd06f0 Mon Sep 17 00:00:00 2001 From: Jon Zimbel Date: Mon, 27 Jul 2026 15:48:03 -0400 Subject: [PATCH 1/3] refactor: Adhere to changeset validation pattern --- lib/arrow/shuttles/shuttle.ex | 53 ++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/lib/arrow/shuttles/shuttle.ex b/lib/arrow/shuttles/shuttle.ex index 649e178c8..f22415bcf 100644 --- a/lib/arrow/shuttles/shuttle.ex +++ b/lib/arrow/shuttles/shuttle.ex @@ -47,32 +47,47 @@ defmodule Arrow.Shuttles.Shuttle do end defp validate_for_active_status(changeset) do - routes = get_assoc(changeset, :routes) + changeset + |> validate_min_stops_on_routes() + |> validate_stop_times() + |> validate_route_shapes() + end - cond do - routes |> Enum.map(&get_assoc(&1, :route_stops)) |> Enum.any?(&(length(&1) < 2)) -> - add_error(changeset, :status, "must have at least two stops in each direction") + defp validate_min_stops_on_routes(changeset) do + changeset + |> get_assoc(:routes) + |> Enum.map(&get_assoc(&1, :route_stops)) + |> Enum.any?(&(length(&1) < 2)) + |> if( + do: add_error(changeset, :status, "must have at least two stops in each direction"), + else: changeset + ) + end - routes - |> Enum.map(&get_assoc(&1, :route_stops)) - |> Enum.any?(&route_stops_missing_time_to_next_stop?/1) -> + defp validate_stop_times(changeset) do + changeset + |> get_assoc(:routes) + |> Enum.map(&get_assoc(&1, :route_stops)) + |> Enum.any?(&route_stops_missing_time_to_next_stop?/1) + |> if( + do: add_error( changeset, :status, "all stops except the last in each direction must have a time to next stop" - ) - - routes - |> Enum.any?(fn route -> is_nil(route.data.shape) end) -> - add_error( - changeset, - :status, - "all routes must have an associated shape" - ) + ), + else: changeset + ) + end - true -> - changeset - end + defp validate_route_shapes(changeset) do + changeset + |> get_assoc(:routes) + |> Enum.any?(fn route -> is_nil(route.data.shape) end) + |> if( + do: add_error(changeset, :status, "all routes must have an associated shape"), + else: changeset + ) end defp validate_for_inactive_status(changeset, today) do From 5c875baa2467e3a648ede2c47dc5b3fbd31e4f0b Mon Sep 17 00:00:00 2001 From: Jon Zimbel Date: Mon, 27 Jul 2026 15:49:13 -0400 Subject: [PATCH 2/3] feat: Validate that non-blank shuttle route waypoints match --- lib/arrow/shuttles/shuttle.ex | 23 ++++++++++ test/arrow/shuttles/shuttle_test.exs | 63 ++++++++++++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/lib/arrow/shuttles/shuttle.ex b/lib/arrow/shuttles/shuttle.ex index f22415bcf..478ecab9b 100644 --- a/lib/arrow/shuttles/shuttle.ex +++ b/lib/arrow/shuttles/shuttle.ex @@ -35,6 +35,7 @@ defmodule Arrow.Shuttles.Shuttle do ) end) |> validate_required([:shuttle_name, :status]) + |> validate_zero_or_one_waypoint() |> validate_required_for(:status, today) |> foreign_key_constraint(:disrupted_route_id) |> unique_constraint(:shuttle_name) @@ -90,6 +91,28 @@ defmodule Arrow.Shuttles.Shuttle do ) end + defp validate_zero_or_one_waypoint(changeset) do + changeset + |> get_assoc(:routes) + |> Enum.map(&get_field(&1, :waypoint, "")) + |> Enum.uniq() + |> case do + [waypoint1, waypoint2] when byte_size(waypoint1) > 0 and byte_size(waypoint2) > 0 -> + update_change(changeset, :routes, fn routes -> + Enum.map(routes, fn route_changeset -> + add_error( + route_changeset, + :waypoint, + "if both waypoints are not blank, they must match" + ) + end) + end) + + _ -> + changeset + end + end + defp validate_for_inactive_status(changeset, today) do shuttle_id = get_field(changeset, :id) diff --git a/test/arrow/shuttles/shuttle_test.exs b/test/arrow/shuttles/shuttle_test.exs index cf7436716..2d87c8ebf 100644 --- a/test/arrow/shuttles/shuttle_test.exs +++ b/test/arrow/shuttles/shuttle_test.exs @@ -933,5 +933,68 @@ defmodule Arrow.Shuttles.ShuttleTest do assert %Ecto.Changeset{valid?: true} = inactive_changeset end + + test "non-blank waypoints must match" do + shuttle = shuttle_fixture() + route_id0 = Enum.at(shuttle.routes, 0).id + route_id1 = Enum.at(shuttle.routes, 1).id + + changeset = + Shuttle.changeset(shuttle, %{ + routes: [ + %{id: route_id0, waypoint: "Waypoint A"}, + %{id: route_id1, waypoint: "Waypoint B"} + ] + }) + + assert [ + %Ecto.Changeset{ + valid?: false, + errors: [waypoint: {"if both waypoints are not blank, they must match", []}] + }, + %Ecto.Changeset{ + valid?: false, + errors: [waypoint: {"if both waypoints are not blank, they must match", []}] + } + ] = + Ecto.Changeset.get_assoc(changeset, :routes) + end + + test "one non-blank waypoint is ok" do + shuttle1 = shuttle_fixture() + route_id0 = Enum.at(shuttle1.routes, 0).id + route_id1 = Enum.at(shuttle1.routes, 1).id + + changeset1 = + Shuttle.changeset(shuttle1, %{ + routes: [%{id: route_id0, waypoint: ""}, %{id: route_id1, waypoint: "A waypoint"}] + }) + + assert %Ecto.Changeset{valid?: true} = changeset1 + + shuttle2 = shuttle_fixture() + route_id0 = Enum.at(shuttle2.routes, 0).id + route_id1 = Enum.at(shuttle2.routes, 1).id + + changeset2 = + Shuttle.changeset(shuttle2, %{ + routes: [%{id: route_id0, waypoint: "A waypoint"}, %{id: route_id1, waypoint: ""}] + }) + + assert %Ecto.Changeset{valid?: true} = changeset2 + end + + test "two non-blank waypoints are ok" do + shuttle = shuttle_fixture() + route_id0 = Enum.at(shuttle.routes, 0).id + route_id1 = Enum.at(shuttle.routes, 1).id + + changeset = + Shuttle.changeset(shuttle, %{ + routes: [%{id: route_id0, waypoint: ""}, %{id: route_id1, waypoint: ""}] + }) + + assert %Ecto.Changeset{valid?: true} = changeset + end end end From dcf37a8b417f66f831989c0541b538c4016292ac Mon Sep 17 00:00:00 2001 From: Jon Zimbel Date: Mon, 27 Jul 2026 16:29:50 -0400 Subject: [PATCH 3/3] fix: Function body nesting, error message conciseness --- lib/arrow/shuttles/shuttle.ex | 37 +++++++++++++++------------- test/arrow/shuttles/shuttle_test.exs | 4 +-- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/lib/arrow/shuttles/shuttle.ex b/lib/arrow/shuttles/shuttle.ex index 478ecab9b..45d4fc2bd 100644 --- a/lib/arrow/shuttles/shuttle.ex +++ b/lib/arrow/shuttles/shuttle.ex @@ -92,24 +92,27 @@ defmodule Arrow.Shuttles.Shuttle do end defp validate_zero_or_one_waypoint(changeset) do - changeset - |> get_assoc(:routes) - |> Enum.map(&get_field(&1, :waypoint, "")) - |> Enum.uniq() - |> case do - [waypoint1, waypoint2] when byte_size(waypoint1) > 0 and byte_size(waypoint2) > 0 -> - update_change(changeset, :routes, fn routes -> - Enum.map(routes, fn route_changeset -> - add_error( - route_changeset, - :waypoint, - "if both waypoints are not blank, they must match" - ) - end) - end) + waypoints_mismatched? = + changeset + |> get_assoc(:routes) + |> Enum.map(&get_field(&1, :waypoint, "")) + |> Enum.uniq() + |> then( + &match?( + [waypoint1, waypoint2] when byte_size(waypoint1) > 0 and byte_size(waypoint2) > 0, + &1 + ) + ) - _ -> - changeset + if waypoints_mismatched? do + update_change(changeset, :routes, fn route_changesets -> + Enum.map( + route_changesets, + &add_error(&1, :waypoint, "non-blank waypoints must match") + ) + end) + else + changeset end end diff --git a/test/arrow/shuttles/shuttle_test.exs b/test/arrow/shuttles/shuttle_test.exs index 2d87c8ebf..89a3333f0 100644 --- a/test/arrow/shuttles/shuttle_test.exs +++ b/test/arrow/shuttles/shuttle_test.exs @@ -950,11 +950,11 @@ defmodule Arrow.Shuttles.ShuttleTest do assert [ %Ecto.Changeset{ valid?: false, - errors: [waypoint: {"if both waypoints are not blank, they must match", []}] + errors: [waypoint: {"non-blank waypoints must match", []}] }, %Ecto.Changeset{ valid?: false, - errors: [waypoint: {"if both waypoints are not blank, they must match", []}] + errors: [waypoint: {"non-blank waypoints must match", []}] } ] = Ecto.Changeset.get_assoc(changeset, :routes)