Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
firestack wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5df3c47
test: assert error messages present when export has no routes or serv…
firestack Feb 2, 2026
5ebc43e
fix: convert "manual" service ids error into changeset error
firestack Feb 2, 2026
efd54a0
cleanup: remove unnessicary `services` processing in `create_export`
firestack Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions lib/arrow/trainsformer/export.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,16 @@ defmodule Arrow.Trainsformer.Export do
def changeset(export, attrs) do
export
|> cast(attrs, [:s3_path, :disruption_id, :name])
|> cast_assoc(:services, with: &Service.changeset/2, required: true)
|> cast_assoc(:routes, with: &Route.changeset/2, required: true)
|> cast_assoc(:services,
with: &Service.changeset/2,
required: true,
required_message: "Export must contain at least one Service ID"
)
|> cast_assoc(:routes,
with: &Route.changeset/2,
required: true,
required_message: "Export must contain at least one route"
)
|> validate_required([:s3_path])
|> assoc_constraint(:disruption)
end
Expand Down
31 changes: 31 additions & 0 deletions lib/arrow_web/components/core_components.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,37 @@ defmodule ArrowWeb.CoreComponents do
"""
end

@doc """
Utility for showing errors on a `Phoenix.HTML.Form`

Takes a form field as `field` and renders [`<.error>`](`error/1`) for each error

<.errors field={@form[:services]} />

Can specify `always_show` to always show errors regardless of if the
field is considered "used" by `Phoenix.Component`

<.errors field={@form[:services]} always_show />
"""
attr :field, Phoenix.HTML.FormField,
doc: "a form field struct retrieved from the form, for example: `@form[:email]`"

attr :always_show, :boolean,
default: false,
doc: "a flag that forces any errors to be displayed regardless of it's used state"

def errors(%{field: field, always_show: show?} = assigns) do
errors =
if(Phoenix.Component.used_input?(field) or show?, do: field.errors, else: [])

assigns =
assign(assigns, errors: Enum.map(errors, &translate_error(&1)))

~H"""
<.error :for={msg <- @errors} class="d-block alert alert-danger">{msg}</.error>
"""
end

def custom_normalize_value("text", value) when is_map(value) do
iodata = Jason.encode_to_iodata!(value)
Phoenix.HTML.Form.normalize_value("text", iodata)
Expand Down
39 changes: 13 additions & 26 deletions lib/arrow_web/components/edit_trainsformer_export_form.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@ defmodule ArrowWeb.EditTrainsformerExportForm do
</div>
</div>
<% end %>
<.errors field={@form[:routes]} always_show />
</div>
<div class="col-lg-10">
<b class="mb-3">Service ID</b>
Expand Down Expand Up @@ -250,6 +251,7 @@ defmodule ArrowWeb.EditTrainsformerExportForm do
</.button>
</div>
</.inputs_for>
<.errors field={@form[:services]} always_show />
</div>
</div>

Expand Down Expand Up @@ -537,35 +539,21 @@ defmodule ArrowWeb.EditTrainsformerExportForm do
end

defp update_export(export_params, socket) do
imported_services =
for {key, value} <- export_params["services"],
into: %{},
do: {key, value}
export = Trainsformer.get_export!(socket.assigns.export.id)

if imported_services == %{} do
{:noreply, assign(socket, error: "You must import at least one service")}
Copy link
Member Author

@firestack firestack Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason for this PR is that I needed to get rid of this "error", because it's being used in a way that is both handled by Changeset's and doesn't match the way I want to return errors from the export validations, and conveniently, it actually makes it simpler to remove this code.

else
export = Trainsformer.get_export!(socket.assigns.export.id)

case Trainsformer.update_export(export, export_params) do
{:ok, _} ->
{:noreply,
socket
|> push_patch(to: "/disruptions/#{socket.assigns.disruption.id}")
|> put_flash(:info, "Trainsformer service schedules updated successfully!")}

{:error, %Ecto.Changeset{} = changeset} ->
{:noreply, assign(socket, form: to_form(changeset))}
end
case Trainsformer.update_export(export, export_params) do
{:ok, _} ->
{:noreply,
socket
|> push_patch(to: "/disruptions/#{socket.assigns.disruption.id}")
|> put_flash(:info, "Trainsformer service schedules updated successfully!")}

{:error, %Ecto.Changeset{} = changeset} ->
{:noreply, assign(socket, form: to_form(changeset))}
end
end

defp create_export(export_params, socket) do
imported_services =
for {key, value} <- export_params["services"],
into: %{},
do: {key, value}

export_params = Map.put(export_params, "routes", socket.assigns.uploaded_file_routes)

with {:ok, s3_path} <-
Expand All @@ -578,8 +566,7 @@ defmodule ArrowWeb.EditTrainsformerExportForm do
Trainsformer.create_export(%{
export_params
| "s3_path" => s3_path,
"name" => socket.assigns.uploaded_file_name,
"services" => imported_services
"name" => socket.assigns.uploaded_file_name
}) do
{:noreply,
socket
Expand Down
22 changes: 22 additions & 0 deletions test/integration/disruptionsv2/trainsformer_export_section_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,28 @@ defmodule Arrow.Integration.Disruptionsv2.TrainsformerExportSectionTest do
)
end

feature "reports errors about missing service ids and route ids", %{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,reasons=no-trips,no-stop-times,no-multi-route-trips.zip"
)
|> assert_text(
"Successfully imported export invalid,reasons=no-trips,no-stop-times,no-multi-route-trips.zip!"
Copy link
Member Author

@firestack firestack Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I don't like the "successfully imported" but actually there's errors, but I think we can fix that later/follow up PR? thoughts?

Copy link
Member

@rudiejd rudiejd Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer something like "Imported export with warnings" if possible. I personally think it would be fine to do that as part of this PR since it should be a relatively small change, but happy to put it off if it ends up being difficult.

)
|> assert_text("Export must contain at least one Service ID")
|> assert_text("Export must contain at least one route")
|> click(Query.css("#save-export-button"))
|> assert_text("Export must contain at least one Service ID")
|> assert_text("Export must contain at least one route")
|> assert_has(Query.css("#save-export-button"))
end

feature "can cancel uploading a Trainsformer export", %{session: session} do
disruption = disruption_v2_fixture(%{mode: :commuter_rail})

Expand Down
Binary file added ...ort/fixtures/trainsformer/invalid,reasons=no-trips,no-stop-times,no-multi-route-trips.zip
View file
Open in desktop
Copy link
Member Author

@firestack firestack Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contents:

: unzip -c test/support/fixtures/trainsformer/invalid,reasons=no-trips,no-stop-times,no-multi-route-trips.zip
Archive:  test/support/fixtures/trainsformer/invalid,reasons=no-trips,no-stop-times,no-multi-route-trips.zip
 extracting: multi_route_trips.txt
added_route_id,trip_id

  inflating: stop_times.txt
trip_id,arrival_time,departure_time,stop_id,stop_sequence,stop_headsign,pickup_type,drop_off_type,timepoint,bikes_allowed,nonstandard_track

  inflating: trips.txt
route_id,service_id,trip_id,trip_headsign,trip_short_name,shape_id,direction_id,bikes_allowed,event_time

Binary file not shown.
Loading