🏹🚆 Trainsformer Validations: Make sure all errors are reported to frontend by firestack · Pull Request #1354 · mbta/arrow

firestack · 2026-02-03T22:22:54Z

Summary of changes

Asana Ticket: 🏹🚆 Trainsformer Validations: Make sure all errors are reported to frontend

This is a somewhat large refactor of the way we're reporting errors and warnings from the Arrow.Trainsformer.ExportUpload validation module, and how ArrowWeb Trainsformer pages use those errors & warnings.

This change bundles reporting invalid CSV errors and reporting multiple errors and warnings at once because implementing multiple errors without also implementing changes on the other functions was a bit of a mess to manage multiple error "shapes" on top of the existing ways errors were reported.

I tried a few different ways of returning multiple errors & warnings to the frontend, and a few different ways of returning errors (4 element tuple for warnings & errors, rather than nested tuples) and I found that I think this is the implementation I favored the most in terms of friction and extensibility.

I tried to make the commits make logical sense in the order of refactors and changes, so you should be able to read the commits in order to understand the intention and reasoning.

Reviewer Checklist

Meets ticket's acceptance criteria
Any new or changed functions have typespecs
Tests were added for any new functionality (don't just rely on Codecov)
This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

…inment

…h special error

…into func

firestack · 2026-02-03T22:44:13Z

lib/arrow_web/components/edit_trainsformer_export_form.ex

+  defp alert_message(%{data: data} = assigns) when not is_nil(data) do
+    ~H"""
+    {@message}
+    <ul>
+      <li :for={element <- @data}>{element}</li>
+    </ul>
+    """
+  end


note: one issue I have with this implementation is that there's this undocumented/implicit behavior where if there's a metadata key that matches the key of the error/warning, it uses this implementation to render the contents nicely as a list.

I wanted to use translate_error/1 or some Gettext function to do this, but I haven't been able to figure out how to blend HTML/HEEx with Gettext so this was the best idea I could come up with :/

I tried to make it a bit more clear with 704ecfa (this PR)

But that doesn't help with discoverability of the "convention" in export_upload.ex

…on frontend

…ith list data to generic function

…ement case more clear

jzimbel-mbta

I think this is good overall, though it does add some complexity (or consistent structure, under a more positive light) to the error handling logic.

My comments are mostly on minor things like warning/error message content and small code improvement opportunities.

I made the mistake of commenting along the way as I worked through your changes commit by commit, which resulted in a lot of comments on outdated code where the issue was addressed in a subsequent commit. Took me a bit at the end to clear those out. Oops!

jzimbel-mbta · 2026-02-04T15:58:30Z

lib/arrow/trainsformer/export_upload.ex


    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)


nit:
Prefer avoiding negated boolean conditions when possible, for clarity.
Also, any reason for using MapSet.member?/2 instead of just in/2?

My suggested change:

Enum.reject(stop_ids, fn stop -> stop in gtfs_stop_ids end)

(GH doesn't seem to support making one-click-commit suggestions when working through the PR commit by commit)

I also agree with that, I'll go ahead and modify that.

jzimbel-mbta · 2026-02-04T18:31:06Z

lib/arrow_web/components/edit_trainsformer_export_form.ex

    """
  end

+  defp export_alert(%{error: {type, {key, {_, metadata} = error_contents}}} = assigns) do


suggestion:
Not sure if it's possible for a private function component, but if it is, would you mind documenting the component's attrs?

I'll see 👍🏻

jzimbel-mbta · 2026-02-04T18:38:52Z

lib/arrow_web/components/edit_trainsformer_export_form.ex

+
+  defp alert_message(%{key: :stop_id_not_in_gtfs} = assigns) do
+    ~H"""
+    Some stops are not present in GTFS!


suggestion:
I would have trouble digging up the relevant slack message, but Shanti has previously expressed a preference for no "!"'s in error messages to keep the UX a bit more chill. 😎

Recommend changing this and the other ones to periods.

Agreed, and I wasn't sure if I should change it, but it goes along with this #1354 (comment), so I'll make it use @message from the validations!

jzimbel-mbta · 2026-02-05T16:24:52Z

lib/arrow_web/components/edit_trainsformer_export_form.ex

+
+    ~H"""
+    <%= if not is_nil(@title) do %>
+      <span :if={not is_nil(@title)} class="font-bold">{@title}</span>


suggestion:
Since this tag is inside the if not is_nil(@title) check, I think you don't need to check it again--you can remove :if={not is_nil(@title)}.

Oh whoops, you're right, this was a last second refactor, thanks for catching this!!

jzimbel-mbta · 2026-02-05T16:37:02Z

test/integration/disruptionsv2/trainsformer_export_section_test.exs

      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")


nit:
I missed this change in the code, but I think "service ids" or even "service IDs" is more appropriate for error message copy.

service_id is how it's labeled in in code, but that's mainly because we can't have spaces in identifiers.

jzimbel-mbta · 2026-02-05T16:49:56Z

lib/arrow_web/components/edit_trainsformer_export_form.ex

+        # 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,


question:
What type(s) of Enumerable besides list can metadata[key] be?

Is there any reason why we can't require a list only, given that in the end it's used by this template code:

<li :for={element <- @enum_metadata}>{element}</li>

which I believe would fail if @enum_metadata were any map or keyword list, or any collection that produces values that don't implement the String.Chars protocol when enumerated.

We could require a list, I figured Enumerable was better to be less restrictive, we do send MapSet's instead of just lists, but I could flatten it at the validation layer.

jzimbel-mbta · 2026-02-05T17:04:14Z

lib/arrow/trainsformer/export_upload.ex

-          missing_routes: [String.t()],
-          invalid_routes: [String.t()],
-          trips_missing_transfers: MapSet.t()
+          warnings: list({:warning, {any(), {binary(), keyword()}}})


question:
Any reason for binary() instead of String.t()?

Oh, nope! good catch!

jzimbel-mbta · 2026-02-05T17:16:06Z

lib/arrow/trainsformer/export_upload.ex

-        :both
+        new_warning(
+          :invalid_sides,
+          "Export contains trips serving North and South Station"


suggestion:
Maybe "both North Station and South Station", for clarity?

jzimbel-mbta · 2026-02-05T17:17:19Z

lib/arrow/trainsformer/export_upload.ex

-    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",


nit:
Prefer "service IDs"

jzimbel-mbta · 2026-02-05T17:22:12Z

lib/arrow_web/components/edit_trainsformer_export_form.ex

+  defp alert_message(%{key: :stop_id_not_in_gtfs} = assigns) do
+    ~H"""
+    Some stops are not present in GTFS!


question/issue:
I'm a little confused at this point, doesn't this message already get supplied at export_upload.ex:234? Why not render the supplied message instead of writing out a new (and slightly different) one?

This question may also apply to the other alert_message/1 component's clauses that render literal copy based on the error key.

firestack added 3 commits February 3, 2026 16:13

test: assert that invalid csv errors are surfaced to the frontend

b488098

refactor: wrap Unzip.LocalFile.open in own function for error conta…

418581b

…inment

refactor: validate_stop_times_in_gtfs => validate_stop_ids_in_gtfs

d027ecb

firestack marked this pull request as ready for review February 3, 2026 22:27

firestack requested a review from a team as a code owner February 3, 2026 22:27

firestack requested review from jzimbel-mbta and removed request for a team February 3, 2026 22:27

firestack force-pushed the kf/feat/trainsformer-export-error-refactor branch from 94dbfb2 to 9af52e1 Compare February 3, 2026 22:32

firestack added 4 commits February 3, 2026 17:34

refactor: refactor error handling in export_upload

bd32dda

refactor(ex/export_upload): convert File.read/1 to own function wit…

7635ab6

…h special error

refactor(ex/export_upload): pull out errors and warnings validations …

cfc19ac

…into func

feat(test/export_upload): add test for southside routes

21b543d

firestack force-pushed the kf/feat/trainsformer-export-error-refactor branch from 9af52e1 to 3e78eef Compare February 3, 2026 22:34

firestack commented Feb 3, 2026

View reviewed changes

firestack force-pushed the kf/feat/trainsformer-export-error-refactor branch from 3a41102 to 704ecfa Compare February 3, 2026 22:57

firestack mentioned this pull request Feb 3, 2026

fix: convert "manual" service ids error into changeset error #1352

Open

4 tasks

firestack force-pushed the kf/feat/trainsformer-export-error-refactor branch from 704ecfa to d54c3bc Compare February 4, 2026 12:51

firestack added 3 commits February 4, 2026 07:53

refactor(ex/edit_trainsformer_export_form): consume new error format …

9d8735d

…on frontend

refactor(ex/edit_trainsformer_export_form): convert export messages w…

bc0c811

…ith list data to generic function

refactor(ex/edit_trainsformer_export_form): try to make alert list el…

621fbc6

…ement case more clear

firestack force-pushed the kf/feat/trainsformer-export-error-refactor branch from d54c3bc to 621fbc6 Compare February 4, 2026 12:53

jzimbel-mbta reviewed Feb 5, 2026

View reviewed changes

Conversation

firestack commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Reviewer Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzimbel-mbta left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

firestack commented Feb 3, 2026 •

edited

Loading