From b4880981f6a3ea12976cc5d1c5348a9257aa598f Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Wed, 21 Jan 2026 14:24:02 -0500 Subject: [PATCH 01/10] test: assert that invalid csv errors are surfaced to the frontend --- .../trainsformer_export_section_test.exs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/integration/disruptionsv2/trainsformer_export_section_test.exs b/test/integration/disruptionsv2/trainsformer_export_section_test.exs index 48c9e4ac1..8ff1f0a1a 100644 --- a/test/integration/disruptionsv2/trainsformer_export_section_test.exs +++ b/test/integration/disruptionsv2/trainsformer_export_section_test.exs @@ -88,6 +88,19 @@ defmodule Arrow.Integration.Disruptionsv2.TrainsformerExportSectionTest do |> assert_text("Invalid zip file") end + feature "reports invalid CSV errors", %{session: session} do + disruption = disruption_v2_fixture(%{mode: :commuter_rail}) + + session + |> visit("/disruptions/#{disruption.id}") + |> click(text("Upload Trainsformer export")) + |> assert_text("Upload Trainsformer .zip") + |> attach_file(file_field("trainsformer_export", visible: false), + path: "test/support/fixtures/trainsformer/invalid_csv.zip" + ) + |> assert_text("Row 2 has length 9 instead of expected length 11") + end + feature "shows error for invalid gtfs stops in trainsformer export", %{session: session} do disruption = disruption_v2_fixture(%{mode: :commuter_rail}) From 418581be12493e9aaead3aa9dbee2ec64fb7371c Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Wed, 21 Jan 2026 15:52:38 -0500 Subject: [PATCH 02/10] refactor: wrap `Unzip.LocalFile.open` in own function for error containment --- lib/arrow/trainsformer/export_upload.ex | 47 +++++++++++++++++++++---- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/lib/arrow/trainsformer/export_upload.ex b/lib/arrow/trainsformer/export_upload.ex index f2cebd5c6..7c57f380b 100644 --- a/lib/arrow/trainsformer/export_upload.ex +++ b/lib/arrow/trainsformer/export_upload.ex @@ -64,9 +64,8 @@ defmodule Arrow.Trainsformer.ExportUpload do unzip_module \\ Unzip, import_helper \\ Arrow.Gtfs.ImportHelper ) do - zip_bin = Unzip.LocalFile.open(zip_path) - - with {:ok, unzip} <- Unzip.new(zip_bin), + with {:ok, unzip_handle} <- open_zip(zip_path), + {:ok, unzip} <- Unzip.new(unzip_handle), {:ok, %{trips: trips, stop_times: stop_times, transfers: transfers}} <- validate_csvs(unzip, unzip_module, import_helper), {:ok, stop_ids} <- @@ -124,12 +123,48 @@ defmodule Arrow.Trainsformer.ExportUpload do {:ok, {:ok, export_data}} else errors -> + # Must be wrapped in an ok tuple for caller, consume_uploaded_entry/3 {:ok, errors} end + end + + # NOTE: Consider moving to `Arrow.Gtfs.ImportHelper`?... + defp open_zip(zip_path) do + # `Unzip.LocalFile.open` is a very simple implementation and, (as of writing) + # simply expects that `:file.open` succeeds and pattern matches on that success. + # But due to the pattern match, the errors aren't very clear, helpful, or useful + # for users not familiar with Elixir/Erlang `:file` or this application. + # + # We could consider making our own `LocalFile` implementation, but really the + # only issue with it is it's error reporting, and we're unlikely to hit any + # of those issues. + # + # Per https://www.erlang.org/docs/28/apps/kernel/file.html#open/2 + # These are the "typical" errors that `:file.open` could return, and why they + # may or may not apply: + # + # > `enoent` - The file does not exist. + # It'd be very strange if Phoenix gave us a file that didn't exist, not user actionable. + # + # > `eacces` - Missing permission for reading the file or searching one of the parent directories. + # It'd also be strange if Phoenix gave us a file that it wrote, but didn't have permissions for, not user actionable + # + # > `eisdir` - The named file is a directory. + # This would also be strange for Phoenix to give us a directory, + # _could_ be a security issue[^security-issue] if somehow we did receive a directory. + # + # [^security-issue]: (because that would mean that the "attacker" has gained control of Phoenix + # in a way we don't _currently_ expect) + # + # > `enospc` - There is no space left on the device (if write access was specified). + # This seems like the most likely error we'd get, but it's also not user actionable. + # + # Running out of space on our instance would likely mean we have bigger problems + # and not something we could "error handle" our way out of. + {:ok, Unzip.LocalFile.open(zip_path)} rescue - e -> - # Must be wrapped in an ok tuple for caller, consume_uploaded_entry/3 - {:ok, {:error, "Could not parse zip, message=#{Exception.format(:error, e)}"}} + error -> + {:error, "Failed to open zip file, message=#{Exception.format(:error, error)}"} end defp validate_unique_service_ids(trips) do From d027ecb30e74266b6b290495d844d7fe2258693b Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Tue, 27 Jan 2026 00:39:08 -0500 Subject: [PATCH 03/10] refactor: `validate_stop_times_in_gtfs` => `validate_stop_ids_in_gtfs` --- lib/arrow/trainsformer/export_upload.ex | 25 +++--- .../arrow/trainsformer/export_upload_test.exs | 78 +++++-------------- 2 files changed, 32 insertions(+), 71 deletions(-) diff --git a/lib/arrow/trainsformer/export_upload.ex b/lib/arrow/trainsformer/export_upload.ex index 7c57f380b..fe9d7d8e3 100644 --- a/lib/arrow/trainsformer/export_upload.ex +++ b/lib/arrow/trainsformer/export_upload.ex @@ -68,8 +68,8 @@ defmodule Arrow.Trainsformer.ExportUpload do {:ok, unzip} <- Unzip.new(unzip_handle), {:ok, %{trips: trips, stop_times: stop_times, transfers: transfers}} <- validate_csvs(unzip, unzip_module, import_helper), - {:ok, stop_ids} <- - validate_stop_times_in_gtfs(stop_times), + stop_ids <- get_stop_ids(stop_times), + :ok <- validate_stop_ids_in_gtfs(stop_ids), :ok <- validate_stop_order(stop_times), :ok <- validate_unique_service_ids(trips), {:ok, zip_bin} <- File.read(zip_path) do @@ -187,31 +187,26 @@ defmodule Arrow.Trainsformer.ExportUpload do end end - def validate_stop_times_in_gtfs( - stop_times, + def validate_stop_ids_in_gtfs( + stop_ids, repo \\ Arrow.Repo ) do - trainsformer_stop_ids = - stop_times - |> Enum.uniq_by(& &1.stop_id) - |> Enum.map(& &1.stop_id) - gtfs_stop_ids = MapSet.new( repo.all( from s in Arrow.Gtfs.Stop, - where: s.id in ^trainsformer_stop_ids, + where: s.id in ^stop_ids, select: s.id ) ) stops_missing_from_gtfs = - Enum.filter(trainsformer_stop_ids, fn stop -> !MapSet.member?(gtfs_stop_ids, stop) end) + Enum.filter(stop_ids, fn stop -> !MapSet.member?(gtfs_stop_ids, stop) end) if Enum.any?(stops_missing_from_gtfs) do {:error, {:invalid_export_stops, stops_missing_from_gtfs}} else - {:ok, trainsformer_stop_ids} + :ok end end @@ -654,4 +649,10 @@ defmodule Arrow.Trainsformer.ExportUpload do _ -> nil end end + + defp get_stop_ids(stop_times) do + stop_times + |> Enum.uniq_by(& &1.stop_id) + |> Enum.map(& &1.stop_id) + end end diff --git a/test/arrow/trainsformer/export_upload_test.exs b/test/arrow/trainsformer/export_upload_test.exs index a0179cf36..83de2380f 100644 --- a/test/arrow/trainsformer/export_upload_test.exs +++ b/test/arrow/trainsformer/export_upload_test.exs @@ -108,7 +108,7 @@ defmodule Arrow.Trainsformer.ExportUploadTest do end end - describe "validate_stop_times_in_gtfs/4" do + describe "validate_stop_ids_in_gtfs/4" do defmodule FakeRepo do def all(_) do ["WR-0329-02", "WR-0325-02", "WR-0264-02"] @@ -116,73 +116,33 @@ defmodule Arrow.Trainsformer.ExportUploadTest do end test "returns ok if all stops in gtfs" do - stop_times_with_stops_in_repo = [ - %{ - arrival_time: "10:26:00", - departure_time: "10:26:00", - stop_id: "WR-0329-02", - stop_sequence: "140", - timepoint: "1", - trip_id: "Base-772221-5208" - }, - %{ - arrival_time: "10:31:00", - departure_time: "10:31:00", - stop_id: "WR-0325-02", - stop_sequence: "150", - trip_id: "Base-772221-5208" - }, - %{ - arrival_time: "10:31:00", - departure_time: "10:31:00", - stop_id: "WR-0264-02", - stop_sequence: "150", - trip_id: "Base-772221-5208" - } + stop_ids = [ + "WR-0329-02", + "WR-0325-02", + "WR-0264-02" ] - assert {:ok, ["WR-0329-02", "WR-0325-02", "WR-0264-02"]} = - ExportUpload.validate_stop_times_in_gtfs( - stop_times_with_stops_in_repo, + assert :ok = + ExportUpload.validate_stop_ids_in_gtfs( + stop_ids, FakeRepo ) end test "returns missing stop if some stops missing from GTFS" do - stop_times_without_stops_in_repo = [ - %{ - arrival_time: "10:26:00", - departure_time: "10:26:00", - stop_id: "morket-borket", - stop_sequence: "140", - trip_id: "ReadWKNDHeadsigns-754204-5530" - }, - %{ - arrival_time: "10:31:00", - departure_time: "10:31:00", - stop_id: "mcdongals", - stop_sequence: "150", - trip_id: "ReadWKNDHeadsigns-754204-5530" - }, - %{ - arrival_time: "10:31:00", - departure_time: "10:31:00", - stop_id: "sbubby", - stop_sequence: "150", - trip_id: "ReadWKNDHeadsigns-754204-5530" - }, - %{ - arrival_time: "10:31:00", - departure_time: "10:31:00", - stop_id: "WR-0329-02", - stop_sequence: "150", - trip_id: "ReadWKNDHeadsigns-754204-5530" - } + stop_ids = [ + "morket-borket", + "mcdongals", + "sbubby", + "WR-0329-02" ] - assert {:error, {:invalid_export_stops, ["morket-borket", "mcdongals", "sbubby"]}} = - ExportUpload.validate_stop_times_in_gtfs( - stop_times_without_stops_in_repo, + assert {:error, + {:stop_id_not_in_gtfs, + {"Export has stops not present in GTFS", + [stop_id_not_in_gtfs: ["morket-borket", "mcdongals", "sbubby"]]}}} = + ExportUpload.validate_stop_ids_in_gtfs( + stop_ids, FakeRepo ) end From bd32dda7418545b9e1703b4b901fba3ff15a872c Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Tue, 27 Jan 2026 00:39:08 -0500 Subject: [PATCH 04/10] refactor: refactor error handling in `export_upload` --- lib/arrow/trainsformer/export_upload.ex | 197 +++++++++++++----- .../arrow/trainsformer/export_upload_test.exs | 93 ++++++--- 2 files changed, 207 insertions(+), 83 deletions(-) diff --git a/lib/arrow/trainsformer/export_upload.ex b/lib/arrow/trainsformer/export_upload.ex index fe9d7d8e3..675732fd0 100644 --- a/lib/arrow/trainsformer/export_upload.ex +++ b/lib/arrow/trainsformer/export_upload.ex @@ -15,19 +15,23 @@ defmodule Arrow.Trainsformer.ExportUpload do services: [String.t()], missing_routes: [String.t()], invalid_routes: [String.t()], - trips_missing_transfers: MapSet.t() + trips_missing_transfers: MapSet.t(), + warnings: list({:warning, {any(), {binary(), keyword()}}}) } @enforce_keys [ :zip_binary, - :one_of_north_south_stations, :routes, :services, + :warnings + ] + defstruct [ + :one_of_north_south_stations, :missing_routes, :invalid_routes, :trips_missing_transfers + | @enforce_keys ] - defstruct @enforce_keys @type transfer :: %{ @@ -56,32 +60,45 @@ defmodule Arrow.Trainsformer.ExportUpload do @spec extract_data_from_upload(%{path: binary()}) :: {:ok, {:ok, t()} - | {:error, {:invalid_stop_times, any()}}} - | {:error, {:invalid_export_stops, [String.t()]}} - | {:error, {:existing_service_id, [String.t()]}} + | {:error, list({:error | :warning, {any(), {binary(), keyword()}}})}} def extract_data_from_upload( %{path: zip_path}, unzip_module \\ Unzip, import_helper \\ Arrow.Gtfs.ImportHelper ) do with {:ok, unzip_handle} <- open_zip(zip_path), - {:ok, unzip} <- Unzip.new(unzip_handle), + {:ok, unzip} <- new_unzip(unzip_handle), {:ok, %{trips: trips, stop_times: stop_times, transfers: transfers}} <- validate_csvs(unzip, unzip_module, import_helper), stop_ids <- get_stop_ids(stop_times), - :ok <- validate_stop_ids_in_gtfs(stop_ids), - :ok <- validate_stop_order(stop_times), - :ok <- validate_unique_service_ids(trips), - {:ok, zip_bin} <- File.read(zip_path) do - one_of_north_south_stations = validate_one_of_north_south_stations(stop_ids) - {missing_routes, invalid_routes} = validate_one_or_all_routes_from_one_side(trips) - - trips_missing_transfers = - case validate_transfers(transfers, stop_times) do - :ok -> MapSet.new() - {:error, {:trips_missing_transfers, invalid_trips}} -> invalid_trips - end - + {:ok, warnings} <- + ( + errors_and_warnings = + [ + validate_stop_ids_in_gtfs(stop_ids), + validate_stop_order(stop_times), + validate_unique_service_ids(trips), + validate_one_of_north_south_stations(stop_ids), + validate_one_or_all_routes_from_one_side(trips), + validate_transfers(transfers, stop_times) + ] + |> Enum.reject(fn + :ok -> true + _ -> false + end) + |> List.flatten() + + errors_and_warnings + |> Enum.any?(fn + tuple when is_tuple(tuple) -> elem(tuple, 0) == :error + _ -> false + end) + |> case do + true -> errors_and_warnings + false -> {:ok, errors_and_warnings} + end + ), + {:read_zip, {:ok, zip_bin}} <- {:read_zip, File.read(zip_path)} do {routes, services} = trips |> Enum.reduce( @@ -112,19 +129,26 @@ defmodule Arrow.Trainsformer.ExportUpload do export_data = %__MODULE__{ zip_binary: zip_bin, - one_of_north_south_stations: one_of_north_south_stations, - missing_routes: missing_routes, - invalid_routes: invalid_routes, routes: route_maps, services: service_maps, - trips_missing_transfers: trips_missing_transfers + warnings: warnings } {:ok, {:ok, export_data}} else - errors -> - # Must be wrapped in an ok tuple for caller, consume_uploaded_entry/3 - {:ok, errors} + # Must be wrapped in an ok tuple for caller, consume_uploaded_entry/3 + errors when is_list(errors) -> + {:ok, {:error, errors}} + + {:error, _} = error -> + {:ok, {:error, List.wrap(error)}} + end + end + + defp new_unzip(unzip_handle) do + case Unzip.new(unzip_handle) do + {:ok, _} = ok_result -> ok_result + {:error, error_message} -> new_error(:zip_file, error_message) end end @@ -164,7 +188,10 @@ defmodule Arrow.Trainsformer.ExportUpload do {:ok, Unzip.LocalFile.open(zip_path)} rescue error -> - {:error, "Failed to open zip file, message=#{Exception.format(:error, error)}"} + new_error( + :zip_file, + "Failed to open zip file, message=#{Exception.format(:error, error)}" + ) end defp validate_unique_service_ids(trips) do @@ -180,10 +207,12 @@ defmodule Arrow.Trainsformer.ExportUpload do where: s.name in ^export_service_ids ) - if Enum.empty?(existing_service_ids_intersection) do - :ok + if Enum.any?(existing_service_ids_intersection) do + new_error(:existing_service_ids, "Export contains previously used service_id's", + existing_service_ids: existing_service_ids_intersection + ) else - {:error, {:existing_service_id, existing_service_ids_intersection}} + :ok end end @@ -204,14 +233,16 @@ defmodule Arrow.Trainsformer.ExportUpload do Enum.filter(stop_ids, fn stop -> !MapSet.member?(gtfs_stop_ids, stop) end) if Enum.any?(stops_missing_from_gtfs) do - {:error, {:invalid_export_stops, stops_missing_from_gtfs}} + new_error(:stop_id_not_in_gtfs, "Export has stops not present in GTFS", + stop_id_not_in_gtfs: stops_missing_from_gtfs + ) else :ok end end @spec validate_stop_order(any()) :: - :ok | {:error, {:invalid_stop_times, any()}} | {:ok, {:error, <<_::160>>}} + :ok | {:error, {:invalid_stop_times, {binary(), keyword()}}} def validate_stop_order(stop_times) do trainsformer_trips = Enum.group_by(stop_times, & &1.trip_id) @@ -220,7 +251,9 @@ defmodule Arrow.Trainsformer.ExportUpload do Enum.flat_map(trainsformer_trips, &validate_trip(&1)) if Enum.any?(invalid_stop_times) do - {:error, {:invalid_stop_times, invalid_stop_times}} + new_error(:invalid_stop_times, "Export contains trips with out-of-order stop times", + invalid_stop_times: invalid_stop_times + ) else :ok end @@ -318,10 +351,14 @@ defmodule Arrow.Trainsformer.ExportUpload do trips_needing_transfers_without_transfers = MapSet.difference(trips_needing_transfers, trips_with_transfers) - if Enum.empty?(trips_needing_transfers_without_transfers) do - :ok + if Enum.any?(trips_needing_transfers_without_transfers) do + new_warning( + :trips_missing_transfers, + "Some train trips that do not serve North Station, South Station, or Foxboro lack transfers", + trips_missing_transfers: trips_needing_transfers_without_transfers + ) else - {:error, {:trips_missing_transfers, trips_needing_transfers_without_transfers}} + :ok end end @@ -339,19 +376,23 @@ defmodule Arrow.Trainsformer.ExportUpload do end @spec validate_one_of_north_south_stations(any()) :: - :ok - | :both - | :neither + :ok | {:warning, {:invalid_sides, {binary(), keyword()}}} def validate_one_of_north_south_stations(trainsformer_stop_ids) do north_station_served = Enum.member?(trainsformer_stop_ids, "BNT-0000") south_station_served = Enum.member?(trainsformer_stop_ids, "NEC-2287") cond do north_station_served and south_station_served -> - :both + new_warning( + :invalid_sides, + "Export contains trips serving North and South Station" + ) not north_station_served and not south_station_served -> - :neither + new_warning( + :invalid_sides, + "Export does not contain trips serving North or South Station" + ) true -> :ok @@ -373,9 +414,8 @@ defmodule Arrow.Trainsformer.ExportUpload do # We require all of the routes from one side to be present, # or a single route. - # Returns {missing_routes, invalid_routes} @spec validate_one_or_all_routes_from_one_side(any()) :: - {[String.t()], [String.t()]} + :ok | {:warning, {:missing_routes | :invalid_routes, {binary(), keyword()}}} def validate_one_or_all_routes_from_one_side(trips) do trainsformer_route_ids = trips @@ -398,23 +438,35 @@ defmodule Arrow.Trainsformer.ExportUpload do cond do length(trainsformer_route_ids) == 1 -> - {[], []} + :ok num_southside_routes_missing == 0 -> - {[], []} + :ok num_northside_routes_missing == 0 -> - {[], []} + :ok num_southside_routes_missing < length(@southside_route_ids) -> - {southside_routes_missing, []} + new_warning( + :missing_routes, + "Not all southside routes are present", + missing_routes: southside_routes_missing + ) num_northside_routes_missing < length(@northside_route_ids) -> - {northside_routes_missing, []} + new_warning( + :missing_routes, + "Not all northside routes are present", + missing_routes: northside_routes_missing + ) # More than one route, and they all aren't in @northside_route_ids or @southside_route_ids true -> - {[], trainsformer_route_ids} + new_warning( + :invalid_routes, + "Multiple routes not north or southside", + invalid_routes: trainsformer_route_ids + ) end end @@ -582,10 +634,10 @@ defmodule Arrow.Trainsformer.ExportUpload do {errors, Map.put(result, data_type, rows)} rescue e -> - {[ - {:error, entry.file_name, Exception.format(:error, e)} - | errors - ], result} + { + [new_error(entry.file_name, Exception.format(:error, e)) | errors], + result + } end @files_to_parse ["trips.txt", "transfers.txt", "stop_times.txt"] @@ -655,4 +707,41 @@ defmodule Arrow.Trainsformer.ExportUpload do |> Enum.uniq_by(& &1.stop_id) |> Enum.map(& &1.stop_id) end + + defp new_error(key, message, metadata \\ []), + do: {:error, new_validation_message(key, message, metadata)} + + defp new_warning(key, message, metadata \\ []), + do: {:warning, new_validation_message(key, message, metadata)} + + # This function returns a data structure that follows similarly to how Ecto + # changesets encode errors + # + # Ecto changesets have a structure of + # ``` + # {key, {message, keys}} + # ``` + # where + # - `key` is the field the error is related to + # - `message` is the templated message that describes the error + # - `keys` is a keyword list of metadata that can be used to "hydrate" the `message` + # + # This structure is valuable for a few reasons: + # 1. `ArrowWeb.CoreComponents.translate_error/1` takes a `{msg, opts}` + # structure by default, which was a decent place to build off of + # due to how it mirrors/consumes Ecto `{message, keys}` data + # + # 2. Having the `message` and `keys` wrapped together means there's less + # modifications that need to be done when grouping by `key` + # + # 3. Having this all be it's own tuple, rather than a 4 element tuple when + # combined with an error, such as: `{:error, new_validation_message(...)}` + # has similar value to #2, i.e., you don't need to write as + # many match cases to extract values from the tuple if you're only dealing + # with checking the "type" or "key" values + # + # a downside is that it does complicate extracting a single element with the + # `Kernel.elem/2` function. + defp new_validation_message(key, message, metadata), + do: {key, {message, metadata}} end diff --git a/test/arrow/trainsformer/export_upload_test.exs b/test/arrow/trainsformer/export_upload_test.exs index 83de2380f..fbc9d2493 100644 --- a/test/arrow/trainsformer/export_upload_test.exs +++ b/test/arrow/trainsformer/export_upload_test.exs @@ -60,7 +60,14 @@ defmodule Arrow.Trainsformer.ExportUploadTest do data = ExportUpload.extract_data_from_upload(%{path: "#{@export_dir}/#{export}"}) - assert {:ok, {:error, {:existing_service_id, ["SPRING2025-SOUTHSS-Weekend-66"]}}} = data + assert {:ok, + {:error, + [ + {:error, + {:existing_service_ids, + {"Export contains previously used service_id's", + [existing_service_ids: ["SPRING2025-SOUTHSS-Weekend-66"]]}}} + ]}} = data end @tag export: "invalid_csv.zip" @@ -69,10 +76,15 @@ defmodule Arrow.Trainsformer.ExportUploadTest do ExportUpload.extract_data_from_upload(%{path: "#{@export_dir}/#{export}"}) assert {:ok, - [ - {:error, "SPRING2025-SOUTHSS-Weekend-66/stop_times.txt", - "** (CSV.RowLengthError) Row 2 has length 9 instead of expected length 11\n\nYou are seeing this error because :validate_row_length has been set to true\n"} - ]} = data + {:error, + [ + {:error, + {"SPRING2025-SOUTHSS-Weekend-66/stop_times.txt", + { + "** (CSV.RowLengthError) Row 2 has length 9 instead of expected length 11\n\nYou are seeing this error because :validate_row_length has been set to true\n", + [] + }}} + ]}} = data end end @@ -204,22 +216,25 @@ defmodule Arrow.Trainsformer.ExportUploadTest do assert {:error, {:invalid_stop_times, - [ - %{ - stop_id: "NEC-2287", - stop_sequence: "10", - arrival_time: "10:30:00", - departure_time: "10:30:00", - trip_id: "October26November2-781218-9733" - }, - %{ - stop_id: "NEC-2276-01", - stop_sequence: "20", - arrival_time: "11:01:00", - departure_time: "11:00:00", - trip_id: "October26November2-781218-9733" - } - ]}} = + {"Export contains trips with out-of-order stop times", + [ + invalid_stop_times: [ + %{ + stop_id: "NEC-2287", + stop_sequence: "10", + trip_id: "October26November2-781218-9733", + arrival_time: "10:30:00", + departure_time: "10:30:00" + }, + %{ + stop_id: "NEC-2276-01", + stop_sequence: "20", + trip_id: "October26November2-781218-9733", + arrival_time: "11:01:00", + departure_time: "11:00:00" + } + ] + ]}}} = ExportUpload.validate_stop_order(invalid_stop_times) end end @@ -242,7 +257,8 @@ defmodule Arrow.Trainsformer.ExportUploadTest do end test "returns error if North and South Stations are both served" do - assert :both = + assert {:warning, + {:invalid_sides, {"Export contains trips serving North and South Station", []}}} = ExportUpload.validate_one_of_north_south_stations([ "BNT-0000", "WR-0045-S", @@ -252,7 +268,9 @@ defmodule Arrow.Trainsformer.ExportUploadTest do end test "returns error if neither North nor South Station is served" do - assert :neither = + assert {:warning, + {:invalid_sides, + {"Export does not contain trips serving North or South Station", _}}} = ExportUpload.validate_one_of_north_south_stations([ "WR-0045-S", "WR-0053-S" @@ -262,7 +280,7 @@ defmodule Arrow.Trainsformer.ExportUploadTest do describe "validate_one_or_all_routes_from_one_side/3" do test "returns empty error route lists if all Northside routes have a trip" do - assert {[], []} = + assert :ok = ExportUpload.validate_one_or_all_routes_from_one_side([ %{ route_id: "CR-Newburyport", @@ -288,7 +306,7 @@ defmodule Arrow.Trainsformer.ExportUploadTest do end test "returns empty error route lists if exactly one Northside route has a trip" do - assert {[], []} = + assert :ok = ExportUpload.validate_one_or_all_routes_from_one_side([ %{ route_id: "CR-Newburyport", @@ -314,7 +332,16 @@ defmodule Arrow.Trainsformer.ExportUploadTest do end test "returns missing routes if more than one but not all Northside routes have a trip" do - assert {["CR-Fitchburg", "CR-Lowell"], []} = + assert { + :warning, + { + :missing_routes, + { + "Not all northside routes are present", + [missing_routes: ["CR-Fitchburg", "CR-Lowell"]] + } + } + } = ExportUpload.validate_one_or_all_routes_from_one_side([ %{ route_id: "CR-Newburyport", @@ -330,7 +357,7 @@ defmodule Arrow.Trainsformer.ExportUploadTest do end test "returns empty error route lists for a single route not in either side's required list" do - assert {[], []} = + assert :ok = ExportUpload.validate_one_or_all_routes_from_one_side([ %{ route_id: "CR-Foxboro", @@ -346,7 +373,10 @@ defmodule Arrow.Trainsformer.ExportUploadTest do end test "returns invalid routes for multiple routes not in either side's required list" do - assert {[], ["CR-Foxboro", "CR-Nowhere"]} = + assert {:warning, + {:invalid_routes, + {"Multiple routes not north or southside", + [invalid_routes: ["CR-Foxboro", "CR-Nowhere"]]}}} = ExportUpload.validate_one_or_all_routes_from_one_side([ %{ route_id: "CR-Foxboro", @@ -426,7 +456,12 @@ defmodule Arrow.Trainsformer.ExportUploadTest do test "returns an error if any transfers are missing" do expected_trips_with_missing_transfers = MapSet.new(["5678"]) - assert {:error, {:trips_missing_transfers, ^expected_trips_with_missing_transfers}} = + assert {:warning, + {:trips_missing_transfers, + { + "Some train trips that do not serve North Station, South Station, or Foxboro lack transfers", + [trips_missing_transfers: ^expected_trips_with_missing_transfers] + }}} = ExportUpload.validate_transfers( [], [ From 7635ab6842be9130ae03661777683a74f2ce0af7 Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Tue, 3 Feb 2026 07:46:41 -0500 Subject: [PATCH 05/10] refactor(ex/export_upload): convert `File.read/1` to own function with special error --- lib/arrow/trainsformer/export_upload.ex | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/arrow/trainsformer/export_upload.ex b/lib/arrow/trainsformer/export_upload.ex index 675732fd0..5134d8c85 100644 --- a/lib/arrow/trainsformer/export_upload.ex +++ b/lib/arrow/trainsformer/export_upload.ex @@ -98,7 +98,7 @@ defmodule Arrow.Trainsformer.ExportUpload do false -> {:ok, errors_and_warnings} end ), - {:read_zip, {:ok, zip_bin}} <- {:read_zip, File.read(zip_path)} do + {:ok, zip_bin} <- read_zip_binary(zip_path) do {routes, services} = trips |> Enum.reduce( @@ -194,6 +194,19 @@ defmodule Arrow.Trainsformer.ExportUpload do ) end + defp read_zip_binary(zip_path) do + case File.read(zip_path) do + {:ok, _} = result -> + result + + {:error, posix} -> + new_error(:zip_file, "Failed to read zip file contents: %{error_message}", + error_message: :file.format_error(posix), + posix_error: posix + ) + end + end + defp validate_unique_service_ids(trips) do export_service_ids = trips From cfc19ac34ef73c14cb39ab78bf6e4a7af465bef3 Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Tue, 3 Feb 2026 07:37:59 -0500 Subject: [PATCH 06/10] refactor(ex/export_upload): pull out errors and warnings validations into func --- lib/arrow/trainsformer/export_upload.ex | 49 +++++++++++-------------- 1 file changed, 22 insertions(+), 27 deletions(-) diff --git a/lib/arrow/trainsformer/export_upload.ex b/lib/arrow/trainsformer/export_upload.ex index 5134d8c85..327b94046 100644 --- a/lib/arrow/trainsformer/export_upload.ex +++ b/lib/arrow/trainsformer/export_upload.ex @@ -70,34 +70,8 @@ defmodule Arrow.Trainsformer.ExportUpload do {:ok, unzip} <- new_unzip(unzip_handle), {:ok, %{trips: trips, stop_times: stop_times, transfers: transfers}} <- validate_csvs(unzip, unzip_module, import_helper), - stop_ids <- get_stop_ids(stop_times), {:ok, warnings} <- - ( - errors_and_warnings = - [ - validate_stop_ids_in_gtfs(stop_ids), - validate_stop_order(stop_times), - validate_unique_service_ids(trips), - validate_one_of_north_south_stations(stop_ids), - validate_one_or_all_routes_from_one_side(trips), - validate_transfers(transfers, stop_times) - ] - |> Enum.reject(fn - :ok -> true - _ -> false - end) - |> List.flatten() - - errors_and_warnings - |> Enum.any?(fn - tuple when is_tuple(tuple) -> elem(tuple, 0) == :error - _ -> false - end) - |> case do - true -> errors_and_warnings - false -> {:ok, errors_and_warnings} - end - ), + validate_export_data(trips, stop_times, transfers), {:ok, zip_bin} <- read_zip_binary(zip_path) do {routes, services} = trips @@ -207,6 +181,27 @@ defmodule Arrow.Trainsformer.ExportUpload do end end + defp validate_export_data(trips, stop_times, transfers) do + stop_ids = get_stop_ids(stop_times) + + errors_and_warnings = + [ + validate_stop_ids_in_gtfs(stop_ids), + validate_stop_order(stop_times), + validate_unique_service_ids(trips), + validate_one_of_north_south_stations(stop_ids), + validate_one_or_all_routes_from_one_side(trips), + validate_transfers(transfers, stop_times) + ] + |> Enum.reject(&(&1 == :ok)) + + if Enum.any?(errors_and_warnings, &match?({:error, _}, &1)) do + errors_and_warnings + else + {:ok, errors_and_warnings} + end + end + defp validate_unique_service_ids(trips) do export_service_ids = trips From 21b543d3a0117546aec288d68368375bdb979001 Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Tue, 27 Jan 2026 00:39:08 -0500 Subject: [PATCH 07/10] feat(test/export_upload): add test for southside routes --- .../arrow/trainsformer/export_upload_test.exs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/test/arrow/trainsformer/export_upload_test.exs b/test/arrow/trainsformer/export_upload_test.exs index fbc9d2493..0f0403a42 100644 --- a/test/arrow/trainsformer/export_upload_test.exs +++ b/test/arrow/trainsformer/export_upload_test.exs @@ -356,6 +356,40 @@ defmodule Arrow.Trainsformer.ExportUploadTest do ]) end + test "returns missing routes if more than one but not all Southside routes have a trip" do + assert { + :warning, + { + :missing_routes, + { + "Not all southside routes are present", + [ + missing_routes: [ + "CR-Greenbush", + "CR-Kingston", + "CR-Needham", + "CR-NewBedford", + "CR-Providence", + "CR-Worcester" + ] + ] + } + } + } = + ExportUpload.validate_one_or_all_routes_from_one_side([ + %{ + route_id: "CR-Fairmount", + service_id: "FALL 2025-SOUTHWKD-Weekday-11A", + trip_id: "Weekday-789267-102" + }, + %{ + route_id: "CR-Franklin", + service_id: "FALL 2025-SOUTHWKD-Weekday-11A", + trip_id: "Weekday-789267-202" + } + ]) + end + test "returns empty error route lists for a single route not in either side's required list" do assert :ok = ExportUpload.validate_one_or_all_routes_from_one_side([ From 9d8735d41bdd1ab8a48360cedf9283298e7dd015 Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Tue, 27 Jan 2026 00:39:08 -0500 Subject: [PATCH 08/10] refactor(ex/edit_trainsformer_export_form): consume new error format on frontend --- lib/arrow/trainsformer/export_upload.ex | 12 +- .../components/disruption_components.ex | 20 ++ .../edit_trainsformer_export_form.ex | 241 +++++++++++------- .../trainsformer_export_section_test.exs | 16 +- 4 files changed, 178 insertions(+), 111 deletions(-) diff --git a/lib/arrow/trainsformer/export_upload.ex b/lib/arrow/trainsformer/export_upload.ex index 327b94046..0427750e1 100644 --- a/lib/arrow/trainsformer/export_upload.ex +++ b/lib/arrow/trainsformer/export_upload.ex @@ -10,12 +10,8 @@ defmodule Arrow.Trainsformer.ExportUpload do @type t :: %__MODULE__{ zip_binary: binary(), - one_of_north_south_stations: :ok | :both | :neither, routes: [String.t()], services: [String.t()], - missing_routes: [String.t()], - invalid_routes: [String.t()], - trips_missing_transfers: MapSet.t(), warnings: list({:warning, {any(), {binary(), keyword()}}}) } @@ -25,13 +21,7 @@ defmodule Arrow.Trainsformer.ExportUpload do :services, :warnings ] - defstruct [ - :one_of_north_south_stations, - :missing_routes, - :invalid_routes, - :trips_missing_transfers - | @enforce_keys - ] + defstruct @enforce_keys @type transfer :: %{ diff --git a/lib/arrow_web/components/disruption_components.ex b/lib/arrow_web/components/disruption_components.ex index 11217d718..a0b0621a6 100644 --- a/lib/arrow_web/components/disruption_components.ex +++ b/lib/arrow_web/components/disruption_components.ex @@ -725,4 +725,24 @@ defmodule ArrowWeb.DisruptionComponents do """ end + + attr :type, :atom, required: true, values: [:error, :warning] + slot :inner_block, required: true + + def upload_alert(assigns) do + assigns = + assign(assigns, + alert_type: + case assigns[:type] do + :error -> "alert-danger" + :warning -> "alert-warning" + end + ) + + ~H""" + + {render_slot(@inner_block)} + + """ + end end diff --git a/lib/arrow_web/components/edit_trainsformer_export_form.ex b/lib/arrow_web/components/edit_trainsformer_export_form.ex index fde05df77..b692ecec1 100644 --- a/lib/arrow_web/components/edit_trainsformer_export_form.ex +++ b/lib/arrow_web/components/edit_trainsformer_export_form.ex @@ -42,37 +42,9 @@ defmodule ArrowWeb.EditTrainsformerExportForm do add a new service schedule
Upload a new Trainsformer export
-
- {@error} -
-
- Some stops are not present in GTFS! - <.button - type="button" - class="alert-info d-block" - phx-click="download_invalid_export_stops" - phx-target={@myself} - > - Download list of invalid stops - -
-
- Some stop times are out of order! - <.button - type="button" - class="alert-info d-block" - phx-click="download_invalid_stop_times" - phx-target={@myself} - > - Download list of invalid stop times - -
-
- Export contains previously used service ids -
    -
  • {service_id}
    • -
-
+ + <.export_alert :for={error <- @errors} error={error} myself={@myself} /> +
{entry.progress}% @@ -253,34 +225,7 @@ defmodule ArrowWeb.EditTrainsformerExportForm do
-
- Warning: export contains trips serving North and South Station. -
- -
- Warning: export does not contain trips serving North or South Station. -
- -
- Warning: Not all northside or southside routes are present. Missing routes: -
    -
  • {route_id}
    • -
-
- -
- Warning: multiple routes not north or southside: -
    -
  • {route_id}
    • -
-
- -
- Warning: some train trips that do not serve North Station, South Station, or Foxboro lack transfers. -
    -
  • {trip_id}
    • -
-
+ <.export_alert :for={error <- @errors} error={error} myself={@myself} />
@@ -311,22 +256,131 @@ defmodule ArrowWeb.EditTrainsformerExportForm do """ end + defp export_alert(%{error: {type, {key, {_, metadata} = error_contents}}} = assigns) do + assigns = + assigns + |> assign( + type: type, + key: key, + message: translate_error(error_contents), + metadata: metadata + ) + |> Map.drop([:error]) + + ~H""" + + <.alert_message key={@key} message={@message} metadata={@metadata} myself={@myself} /> + + """ + end + + defp alert_message(%{key: :stop_id_not_in_gtfs} = assigns) do + ~H""" + Some stops are not present in GTFS! + <.button + type="button" + class="alert-info d-block" + phx-click="download_invalid_export_stops" + phx-target={@myself} + > + Download list of invalid stops + + """ + end + + defp alert_message(%{key: :invalid_stop_times} = assigns) do + ~H""" + Some stop times are out of order! + <.button + type="button" + class="alert-info d-block" + phx-click="download_invalid_stop_times" + phx-target={@myself} + > + Download list of invalid stop times + + """ + end + + defp alert_message(%{key: :invalid_service_ids} = assigns) do + ~H""" + Export contains previously used service ids +
    +
  • {service_id}
    • +
+ """ + end + + defp alert_message(%{key: {:invalid_side, :both}} = assigns) do + ~H""" + Warning: export contains trips serving North and South Station. + """ + end + + defp alert_message(%{key: {:invalid_side, :neither}} = assigns) do + ~H""" + Warning: export does not contain trips serving North or South Station. + """ + end + + defp alert_message(%{key: :invalid_sides} = assigns) do + ~H""" + Warning: {@message}. + """ + end + + defp alert_message(%{key: :missing_routes} = assigns) do + ~H""" + Warning: Not all northside or southside routes are present. Missing routes: +
    +
  • {route_id}
    • +
+ """ + end + + defp alert_message(%{key: :invalid_routes} = assigns) do + ~H""" + Warning: multiple routes not north or southside: +
    +
  • {route_id}
    • +
+ """ + end + + defp alert_message(%{key: :trips_missing_transfers} = assigns) do + ~H""" + Warning: some train trips that do not serve North Station, South Station, or Foxboro lack transfers. +
    +
  • {trip_id}
    • +
+ """ + end + + defp alert_message(assigns) do + assigns = + assign_new(assigns, :title, fn -> + if(is_binary(assigns[:key]), do: assigns[:key], else: nil) + end) + + ~H""" + <%= if not is_nil(@title) do %> + {@title} +
+ <% end %> + + {@message} + """ + end + @impl true def update(%{export: %{id: nil}} = assigns, socket) do socket = socket |> assign(assigns) - |> assign(:error, nil) + |> assign(:errors, []) |> assign(:show_upload_form, true) |> assign(:show_service_import_form, false) |> assign(:form, nil) - |> assign(:invalid_export_stops, nil) - |> assign(:invalid_stop_times, nil) - |> assign(:one_of_north_south_stations, :ok) - |> assign(:missing_routes, nil) - |> assign(:invalid_routes, nil) - |> assign(:trips_missing_transfers, nil) - |> assign(:existing_service_ids, nil) |> allow_upload(:trainsformer_export, accept: ~w(.zip), progress: &handle_progress/3, @@ -345,16 +399,10 @@ defmodule ArrowWeb.EditTrainsformerExportForm do socket = socket |> assign(assigns) + |> assign(:errors, []) |> assign(:show_upload_form, false) |> assign(:show_service_import_form, true) |> assign(:form, form) - |> assign(:invalid_export_stops, nil) - |> assign(:invalid_stop_times, nil) - |> assign(:one_of_north_south_stations, :ok) - |> assign(:missing_routes, []) - |> assign(:invalid_routes, []) - |> assign(:trips_missing_transfers, []) - |> assign(:error, nil) |> assign(:uploaded_file_name, nil) |> assign(:uploaded_file_routes, nil) |> allow_upload(:trainsformer_export, @@ -392,8 +440,14 @@ defmodule ArrowWeb.EditTrainsformerExportForm do def handle_event( "download_invalid_export_stops", _params, - %{assigns: %{invalid_export_stops: stops}} = socket + %{assigns: %{errors: errors}} = socket ) do + stops = + Enum.find_value(errors, fn + {_, {:stop_id_not_in_gtfs, {_, opts}}} -> opts[:stop_id_not_in_gtfs] + _ -> false + end) + socket = send_download( socket, @@ -408,8 +462,14 @@ defmodule ArrowWeb.EditTrainsformerExportForm do def handle_event( "download_invalid_stop_times", _params, - %{assigns: %{invalid_stop_times: stop_times}} = socket + %{assigns: %{errors: errors}} = socket ) do + stop_times = + Enum.find_value(errors, fn + {_, {:invalid_stop_times, {_, opts}}} -> opts[:invalid_stop_times] + _ -> false + end) + stop_times_lines = Enum.map(stop_times, fn stop_time -> "trip_id: #{stop_time[:trip_id]}, stop_id: #{stop_time[:stop_id]}, stop_sequence: #{stop_time[:stop_sequence]}, arrival_time: #{stop_time[:arrival_time]}, departure_time: #{stop_time[:departure_time]}" @@ -489,7 +549,7 @@ defmodule ArrowWeb.EditTrainsformerExportForm do %UploadEntry{client_name: client_name} = entry, socket ) do - socket = socket |> clear_flash() |> assign(error: nil) + socket = socket |> clear_flash() |> assign(errors: []) case consume_uploaded_entry( socket, @@ -514,25 +574,13 @@ defmodule ArrowWeb.EditTrainsformerExportForm do show_service_import_form: true, uploaded_file_name: client_name, uploaded_file_data: export_data.zip_binary, - one_of_north_south_stations: export_data.one_of_north_south_stations, - missing_routes: export_data.missing_routes, - invalid_routes: export_data.invalid_routes, - trips_missing_transfers: export_data.trips_missing_transfers, uploaded_file_routes: export_data.routes, - uploaded_file_services: export_data.services + uploaded_file_services: export_data.services, + errors: export_data.warnings )} - {:error, {:invalid_export_stops, stops}} -> - {:noreply, assign(socket, invalid_export_stops: stops)} - - {:error, {:invalid_stop_times, stop_times}} -> - {:noreply, assign(socket, invalid_stop_times: stop_times)} - - {:error, {:existing_service_id, existing_ids}} -> - {:noreply, assign(socket, existing_service_ids: existing_ids)} - - {:error, error} -> - {:noreply, assign(socket, error: error)} + {:error, errors_and_warnings} -> + {:noreply, assign(socket, errors: errors_and_warnings)} end end @@ -590,7 +638,12 @@ defmodule ArrowWeb.EditTrainsformerExportForm do {:noreply, assign(socket, form: to_form(changeset))} {:error, _} -> - {:noreply, assign(socket, error: "Failed to upload export to S3")} + {:noreply, + socket + |> update( + :errors, + &[{:error, {:s3_upload_failed, {"Failed to upload export to S3", []}}} | &1] + )} end end end diff --git a/test/integration/disruptionsv2/trainsformer_export_section_test.exs b/test/integration/disruptionsv2/trainsformer_export_section_test.exs index 8ff1f0a1a..ba1f8922a 100644 --- a/test/integration/disruptionsv2/trainsformer_export_section_test.exs +++ b/test/integration/disruptionsv2/trainsformer_export_section_test.exs @@ -136,7 +136,7 @@ defmodule Arrow.Integration.Disruptionsv2.TrainsformerExportSectionTest do |> attach_file(file_field("trainsformer_export", visible: false), path: "test/support/fixtures/trainsformer/valid_export.zip" ) - |> assert_text("Export contains previously used service ids") + |> assert_text("Export contains previously used service_id's") end feature "shows error for invalid stop order in trainsformer export", %{session: session} do @@ -164,7 +164,7 @@ defmodule Arrow.Integration.Disruptionsv2.TrainsformerExportSectionTest do |> attach_file(file_field("trainsformer_export", visible: false), path: "test/support/fixtures/trainsformer/invalid_export_north_and_south_station.zip" ) - |> assert_text("Warning: export contains trips serving North and South Station.") + |> assert_text("Export contains trips serving North and South Station") end feature "shows warning for trainsformer export containing neither North nor South Station", %{ @@ -180,7 +180,7 @@ defmodule Arrow.Integration.Disruptionsv2.TrainsformerExportSectionTest do path: "test/support/fixtures/trainsformer/invalid_export_neither_north_nor_south_station.zip" ) - |> assert_text("Warning: export does not contain trips serving North or South Station.") + |> assert_text("Export does not contain trips serving North or South Station") end feature "shows warning for trainsformer export containing some but not all routes for a side", @@ -196,7 +196,8 @@ defmodule Arrow.Integration.Disruptionsv2.TrainsformerExportSectionTest do |> attach_file(file_field("trainsformer_export", visible: false), path: "test/support/fixtures/trainsformer/invalid_export_missing_south_side_routes.zip" ) - |> assert_text("Warning: Not all northside or southside routes are present. Missing routes:") + |> assert_text("Not all southside routes are present") + |> assert_text("CR-Greenbush") end feature "shows warning for trainsformer export containing multiple routes that are neither north nor southside", @@ -212,7 +213,9 @@ defmodule Arrow.Integration.Disruptionsv2.TrainsformerExportSectionTest do |> attach_file(file_field("trainsformer_export", visible: false), path: "test/support/fixtures/trainsformer/invalid_export_multiple_no_side_routes.zip" ) - |> assert_text("Warning: multiple routes not north or southside:") + |> assert_text("Multiple routes not north or southside") + |> assert_text("CR-Nowhere") + |> assert_text("CR-Foxboro") end feature "shows warning for missing transfers in trainsformer export", %{session: session} do @@ -226,8 +229,9 @@ defmodule Arrow.Integration.Disruptionsv2.TrainsformerExportSectionTest do path: "test/support/fixtures/trainsformer/invalid_export_missing_transfers.zip" ) |> assert_text( - "Warning: some train trips that do not serve North Station, South Station, or Foxboro lack transfers." + "Some train trips that do not serve North Station, South Station, or Foxboro lack transfers" ) + |> assert_text("Dec14PatsGame-781225-9731") end feature "can cancel uploading a Trainsformer export", %{session: session} do From bc0c81131fa3ac6425fa13b53b2b45a54fe7596d Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Thu, 29 Jan 2026 15:46:51 -0500 Subject: [PATCH 09/10] refactor(ex/edit_trainsformer_export_form): convert export messages with list data to generic function --- .../edit_trainsformer_export_form.ex | 60 ++++--------------- 1 file changed, 10 insertions(+), 50 deletions(-) diff --git a/lib/arrow_web/components/edit_trainsformer_export_form.ex b/lib/arrow_web/components/edit_trainsformer_export_form.ex index b692ecec1..89776a703 100644 --- a/lib/arrow_web/components/edit_trainsformer_export_form.ex +++ b/lib/arrow_web/components/edit_trainsformer_export_form.ex @@ -263,13 +263,18 @@ defmodule ArrowWeb.EditTrainsformerExportForm do type: type, key: key, message: translate_error(error_contents), - metadata: metadata + data: if(is_atom(key), do: metadata[key], else: nil) ) |> Map.drop([:error]) ~H""" - <.alert_message key={@key} message={@message} metadata={@metadata} myself={@myself} /> + <.alert_message + key={@key} + message={@message} + data={@data} + myself={@myself} + /> """ end @@ -302,56 +307,11 @@ defmodule ArrowWeb.EditTrainsformerExportForm do """ end - defp alert_message(%{key: :invalid_service_ids} = assigns) do + defp alert_message(%{data: data} = assigns) when not is_nil(data) do ~H""" - Export contains previously used service ids -
    -
  • {service_id}
    • -
- """ - end - - defp alert_message(%{key: {:invalid_side, :both}} = assigns) do - ~H""" - Warning: export contains trips serving North and South Station. - """ - end - - defp alert_message(%{key: {:invalid_side, :neither}} = assigns) do - ~H""" - Warning: export does not contain trips serving North or South Station. - """ - end - - defp alert_message(%{key: :invalid_sides} = assigns) do - ~H""" - Warning: {@message}. - """ - end - - defp alert_message(%{key: :missing_routes} = assigns) do - ~H""" - Warning: Not all northside or southside routes are present. Missing routes: -
    -
  • {route_id}
    • -
- """ - end - - defp alert_message(%{key: :invalid_routes} = assigns) do - ~H""" - Warning: multiple routes not north or southside: -
    -
  • {route_id}
    • -
- """ - end - - defp alert_message(%{key: :trips_missing_transfers} = assigns) do - ~H""" - Warning: some train trips that do not serve North Station, South Station, or Foxboro lack transfers. + {@message}
    -
  • {trip_id}
    • +
  • {element}
""" end From 621fbc63cd8d340a84854df209d24d605c5548ed Mon Sep 17 00:00:00 2001 From: Kayla Firestack Date: Tue, 3 Feb 2026 17:46:48 -0500 Subject: [PATCH 10/10] refactor(ex/edit_trainsformer_export_form): try to make alert list element case more clear --- .../components/edit_trainsformer_export_form.ex | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/lib/arrow_web/components/edit_trainsformer_export_form.ex b/lib/arrow_web/components/edit_trainsformer_export_form.ex index 89776a703..c8c2a242f 100644 --- a/lib/arrow_web/components/edit_trainsformer_export_form.ex +++ b/lib/arrow_web/components/edit_trainsformer_export_form.ex @@ -263,7 +263,13 @@ defmodule ArrowWeb.EditTrainsformerExportForm do type: type, key: key, message: translate_error(error_contents), - data: if(is_atom(key), do: metadata[key], else: nil) + # If there is a metadata element with the same name as the alert `key` + # try to use that as enumerable metadata to report alongside the message + enum_metadata: + if(is_atom(key) and Enumerable.impl_for(metadata[key]) != nil, + do: metadata[key], + else: nil + ) ) |> Map.drop([:error]) @@ -272,7 +278,7 @@ defmodule ArrowWeb.EditTrainsformerExportForm do <.alert_message key={@key} message={@message} - data={@data} + enum_metadata={@enum_metadata} myself={@myself} /> @@ -307,11 +313,11 @@ defmodule ArrowWeb.EditTrainsformerExportForm do """ end - defp alert_message(%{data: data} = assigns) when not is_nil(data) do + defp alert_message(%{enum_metadata: enum_metadata} = assigns) when not is_nil(enum_metadata) do ~H""" {@message}
    -
  • {element}
    • +
  • {element}
""" end