fix(shuttles): call Changeset.apply_action after uploading xlsx (#1083)

* fix(shuttles): call Changeset.apply_action after uploading xlsx

* refactor: use changeset from socket for update_map

* tweak: add comment to explain creation of new changeset

* fix(shuttles): error on invalid stop id on xlsx upload

* fix: parse stop_id as string in upload and fix stop_sequence start

* test: fix upload stop IDs test after rebase

lib/arrow/shuttles/definition_upload.ex

@@ -81,7 +81,11 @@ defmodule Arrow.Shuttles.DefinitionUpload do
         end)
         |> Enum.reverse()
 
-      if Enum.empty?(errors), do: {:ok, stop_ids}, else: {:errors, errors}
+      if Enum.empty?(errors) do
+        {:ok, stop_ids |> Enum.map(&Integer.to_string(&1))}
+      else
+        {:errors, errors}
+      end
     else
       {:errors, ["Unable to parse Stop ID column"]}
     end

lib/arrow_web/live/shuttle_live/shuttle_live.ex

@@ -317,10 +317,12 @@ defmodule ArrowWeb.ShuttleViewLive do
   end
 
   def handle_event("validate", %{"shuttle" => shuttle_params}, socket) do
-    change = Shuttles.change_shuttle(socket.assigns.shuttle, shuttle_params)
-    form = to_form(change, action: :validate)
+    form =
+      socket.assigns.shuttle
+      |> Shuttles.change_shuttle(shuttle_params)
+      |> to_form(action: :validate)
 
-    {:noreply, socket |> assign(form: form) |> update_map(change)}
+    {:noreply, socket |> assign(form: form) |> update_map()}
   end
 
   def handle_event("edit", %{"shuttle" => shuttle_params}, socket) do
@@ -403,7 +405,7 @@ defmodule ArrowWeb.ShuttleViewLive do
 
     changeset = Ecto.Changeset.put_assoc(changeset, :routes, new_routes)
 
-    socket = socket |> assign(:form, to_form(changeset)) |> update_map(changeset)
+    socket = socket |> assign(form: to_form(changeset)) |> update_map()
 
     {:noreply, socket}
   end
@@ -440,7 +442,7 @@ defmodule ArrowWeb.ShuttleViewLive do
          |> update(:errors, fn errors ->
            put_in(errors, [:route_stops, Access.key(direction_id_string)], nil)
          end)
-         |> assign(:form, to_form(changeset))}
+         |> assign(form: to_form(changeset))}
 
       {:error, error} ->
         {:noreply,
@@ -451,32 +453,6 @@ defmodule ArrowWeb.ShuttleViewLive do
     end
   end
 
-  defp update_route_changeset_with_uploaded_stops(route_changeset, stop_ids, direction_id) do
-    if Ecto.Changeset.get_field(route_changeset, :direction_id) == direction_id do
-      new_route_stops =
-        stop_ids
-        |> Enum.with_index()
-        |> Enum.map(fn {stop_id, i} ->
-          Arrow.Shuttles.RouteStop.changeset(
-            %Arrow.Shuttles.RouteStop{},
-            %{
-              direction_id: direction_id,
-              stop_sequence: i,
-              display_stop_id: Integer.to_string(stop_id)
-            }
-          )
-        end)
-
-      Ecto.Changeset.put_assoc(
-        route_changeset,
-        :route_stops,
-        new_route_stops
-      )
-    else
-      route_changeset
-    end
-  end
-
   @spec get_stop_travel_times(list({:ok, any()})) ::
           {:ok, list(number())} | {:error, any()}
   defp get_stop_travel_times(stop_coordinates) do
@@ -572,7 +548,9 @@ defmodule ArrowWeb.ShuttleViewLive do
     end
   end
 
-  defp update_map(socket, changeset) do
+  defp update_map(socket) do
+    changeset = socket.assigns.form.source
+
     layers =
       changeset
       |> Ecto.Changeset.get_assoc(:routes, :struct)
@@ -600,23 +578,98 @@ defmodule ArrowWeb.ShuttleViewLive do
     end
   end
 
+  defp get_new_route_stops_changeset_with_uploaded_stops(stop_ids, direction_id) do
+    new_route_stops =
+      stop_ids
+      |> Enum.with_index(1)
+      |> Enum.map(fn {stop_id, i} ->
+        Arrow.Shuttles.RouteStop.changeset(
+          %Arrow.Shuttles.RouteStop{},
+          %{
+            direction_id: direction_id,
+            stop_sequence: i,
+            display_stop_id: stop_id
+          }
+        )
+      end)
+
+    if Enum.all?(new_route_stops, & &1.valid?) do
+      {:ok, new_route_stops}
+    else
+      {:error,
+       new_route_stops
+       |> Enum.flat_map(fn %Ecto.Changeset{errors: errors} -> errors end)
+       |> Enum.map(&elem(&1, 1))}
+    end
+  end
+
   defp populate_stop_ids(socket, stop_ids) do
     changeset = socket.assigns.form.source
     existing_routes = Ecto.Changeset.get_assoc(changeset, :routes)
 
-    new_routes =
-      Enum.map(existing_routes, fn route_changeset ->
+    new_route_stops =
+      existing_routes
+      |> Enum.map(fn route_changeset ->
         direction_id = Ecto.Changeset.get_field(route_changeset, :direction_id)
 
-        update_route_changeset_with_uploaded_stops(
-          route_changeset,
-          elem(stop_ids, direction_id |> Atom.to_string() |> String.to_integer()),
-          direction_id
-        )
+        stop_ids
+        |> elem(direction_id |> Atom.to_string() |> String.to_integer())
+        |> get_new_route_stops_changeset_with_uploaded_stops(direction_id)
       end)
+      |> Enum.split_with(fn
+        {:ok, _} -> true
+        _ -> false
+      end)
+      |> case do
+        {valid_route_stops, []} ->
+          {:ok, valid_route_stops |> Enum.flat_map(&elem(&1, 1))}
 
-    changeset = Ecto.Changeset.put_assoc(changeset, :routes, new_routes)
+        {_, errors} ->
+          {:error, errors |> Enum.flat_map(&elem(&1, 1))}
+      end
 
-    socket |> assign(:form, to_form(changeset)) |> update_map(changeset)
+    case new_route_stops do
+      {:ok, route_stops} ->
+        new_routes =
+          Enum.map(existing_routes, fn route_changeset ->
+            direction_id = Ecto.Changeset.get_field(route_changeset, :direction_id)
+
+            direction_new_route_stops =
+              route_stops
+              |> Enum.filter(&(Ecto.Changeset.get_field(&1, :direction_id) == direction_id))
+
+            Ecto.Changeset.put_assoc(
+              route_changeset,
+              :route_stops,
+              direction_new_route_stops
+            )
+          end)
+
+        changeset = Ecto.Changeset.put_assoc(changeset, :routes, new_routes)
+
+        case Ecto.Changeset.apply_action(changeset, :update) do
+          {:ok, shuttle} ->
+            # We replaced any existing associated stops
+            # so we create a new changeset here to track additional changes
+            # Related Ecto error:
+            # https://github.com/elixir-ecto/ecto/blob/18288287f18ce205b03b3b3dc8cb80f0f1b06dbe/lib/ecto/changeset/relation.ex#L448-L453
+            new_changeset = Shuttles.change_shuttle(shuttle)
+            socket |> assign(form: to_form(new_changeset)) |> update_map()
+
+          {:error, _invalid_changeset} ->
+            # The changeset from the upload data wasn't valid, so we don't retain it
+            socket |> assign(form: to_form(changeset)) |> update_map()
+        end
+
+      {:error, errors} ->
+        socket
+        |> put_flash(
+          :errors,
+          {
+            "Failed to upload definition: ",
+            errors |> Enum.map(&translate_error/1)
+          }
+        )
+    end
   end
 end

test/arrow_web/live/shuttle_live/shuttle_live_test.exs

@@ -465,22 +465,76 @@ defmodule ArrowWeb.ShuttleLiveTest do
 
       direction_0_stop_sequence = ~w(9328 5327 5271)
       direction_1_stop_sequence = ~w(5271 5072 9328)
+
+      (direction_0_stop_sequence ++ direction_1_stop_sequence)
+      |> Enum.uniq()
+      |> Enum.map(fn stop_id ->
+        insert(:gtfs_stop, %{id: stop_id})
+      end)
+
+      html = render_upload(definition, "valid.xlsx")
+
+      direction_0_stop_rows = Floki.find(html, "#stops-dir-0 > .row")
+      direction_1_stop_rows = Floki.find(html, "#stops-dir-1 > .row")
+
+      for {stop_id, index} <- Enum.with_index(direction_0_stop_sequence, 1) do
+        [stop] =
+          Floki.attribute(
+            direction_0_stop_rows,
+            "[data-stop_sequence=#{index}] > div > div.form-group > div > div > div > input[type=text]",
+            "value"
+          )
+
+        assert stop =~ stop_id
+      end
+
+      for {stop_id, index} <- Enum.with_index(direction_1_stop_sequence, 1) do
+        [stop] =
+          Floki.attribute(
+            direction_1_stop_rows,
+            "[data-stop_sequence=#{index}] > div > div.form-group > div > div > div > input[type=text]",
+            "value"
+          )
+
+        assert stop =~ stop_id
+      end
+    end
+
+    @tag :authenticated_admin
+    test "displays error if uploaded stop IDs contain invalid stop IDs", %{conn: conn} do
+      {:ok, new_live, _html} = live(conn, ~p"/shuttles/new")
+
+      definition =
+        file_input(new_live, "#shuttle-form", :definition, [
+          %{
+            name: "valid.xlsx",
+            content: File.read!("test/support/fixtures/xlsx/valid.xlsx")
+          }
+        ])
+
+      direction_0_stop_sequence = ~w(9328 5327 5271)
+      direction_1_stop_sequence = ~w(5271 5072 9328)
+
       html = render_upload(definition, "valid.xlsx")
 
+      assert html =~ "Failed to upload definition:"
+      assert html =~ "not a valid stop ID &#39;9328"
+      assert html =~ "not a valid stop ID &#39;5072"
+
       direction_0_stop_rows = Floki.find(html, "#stops-dir-0 > .row")
       direction_1_stop_rows = Floki.find(html, "#stops-dir-1 > .row")
 
-      for {stop_id, index} <- Enum.with_index(direction_0_stop_sequence) do
-        assert [^stop_id] =
+      for {_stop_id, index} <- Enum.with_index(direction_0_stop_sequence, 1) do
+        assert [] =
                  Floki.attribute(
                    direction_0_stop_rows,
                    "[data-stop_sequence=#{index}] > div > div.form-group > div > div > div > input[type=text]",
                    "value"
                  )
       end
 
-      for {stop_id, index} <- Enum.with_index(direction_1_stop_sequence) do
-        assert [^stop_id] =
+      for {_stop_id, index} <- Enum.with_index(direction_1_stop_sequence, 1) do
+        assert [] =
                  Floki.attribute(
                    direction_1_stop_rows,
                    "[data-stop_sequence=#{index}] > div > div.form-group > div > div > div > input[type=text]",