Allow deleting fields with ordinal inputs (#312)

Commit f9d1bb9 (PR #208) had to revert an attempt to handle deleting
fields in ordinal inputs (something like `inputs_for`).

This commit adds the test setup from previous attempt to reproduce error
and introduce a fix.

The fix is slightly complex (not sure if we can do better) but it
compares the current HTML fields with those that have been changed
(based on what we have in memory) so long as they're index-based.
Otherwise, things remain the same (which prevents the previous
regression)

Author: germsvel

lib/phoenix_test/form_data.ex

@@ -98,6 +98,10 @@ defmodule PhoenixTest.FormData do
     value == field_data or value in List.wrap(field_data)
   end
 
+  def field_names(%__MODULE__{data: data}) do
+    Map.keys(data)
+  end
+
   def to_list(%__MODULE__{data: data}) do
     data
     |> Enum.map(fn

lib/phoenix_test/live.ex

@@ -518,8 +518,7 @@ defmodule PhoenixTest.Live do
   def submit_form(session, selector, form_data, additional_data \\ FormData.new()) do
     form = Form.find!(session.current_operation.html, selector)
 
-    form_data = remove_data_for_fields_that_have_been_removed(form_data, form)
-    form_data = FormData.override(form.form_data, form_data)
+    form_data = select_form_data_to_submit(form, form_data)
 
     additional_data =
       if form.submit_button do
@@ -548,14 +547,74 @@ defmodule PhoenixTest.Live do
     end
   end
 
-  defp remove_data_for_fields_that_have_been_removed(form_data, form) do
-    element_names = Form.form_element_names(form)
+  defp select_form_data_to_submit(form, form_data) do
+    element_names_present_in_final_form = Form.form_element_names(form)
+    form_data = remove_data_for_fields_that_have_been_removed(form_data, element_names_present_in_final_form)
+
+    FormData.override(form.form_data, form_data)
+  end
+
+  defp remove_data_for_fields_that_have_been_removed(form_data, element_names_present_in_final_form) do
+    element_names_set = MapSet.new(element_names_present_in_final_form)
+
+    removed_index_groups =
+      form_data
+      |> FormData.field_names()
+      |> Enum.filter(&name_index_based_and_not_in_current_form_names?(&1, element_names_set))
+      |> MapSet.new(&index_group_key/1)
 
     FormData.filter(form_data, fn %{name: name} ->
-      name in element_names
+      keep_field?(name, element_names_set, removed_index_groups)
     end)
   end
 
+  defp keep_field?(name, element_names_set, removed_index_groups) do
+    cond do
+      not MapSet.member?(element_names_set, name) ->
+        false
+
+      not index_based_field?(name) ->
+        true
+
+      true ->
+        not MapSet.member?(removed_index_groups, index_group_key(name))
+    end
+  end
+
+  defp name_index_based_and_not_in_current_form_names?(name, element_names_set) do
+    index_based_field?(name) and not MapSet.member?(element_names_set, name)
+  end
+
+  defp index_based_field?(name) do
+    Regex.match?(~r/\[\d+\]/, name)
+  end
+
+  defp index_group_key(name) do
+    case bracket_segments(name) do
+      [] ->
+        name
+
+      [root | segments] ->
+        normalized_segments = Enum.map(segments, &normalize_segment/1)
+        root <> Enum.join(normalized_segments)
+    end
+  end
+
+  defp normalize_segment(segment) do
+    if numeric_segment?(segment), do: "[]", else: "[#{segment}]"
+  end
+
+  defp bracket_segments(name) do
+    String.split(name, ["[", "]"], trim: true)
+  end
+
+  defp numeric_segment?(segment) do
+    case Integer.parse(segment) do
+      {_integer, ""} -> true
+      _ -> false
+    end
+  end
+
   def open_browser(%{view: view} = session, open_fun \\ &Phoenix.LiveViewTest.open_browser/1) do
     open_fun.(view)
     session

test/phoenix_test/form_data_test.exs

@@ -222,6 +222,24 @@ defmodule PhoenixTest.FormDataTest do
     end
   end
 
+  describe "field_names" do
+    test "returns unique field names from form data" do
+      form_data =
+        FormData.new()
+        |> FormData.add_data("email", "first@example.com")
+        |> FormData.add_data("email", "second@example.com")
+        |> FormData.add_data("items[]", "one")
+        |> FormData.add_data("items[]", "two")
+
+      names =
+        form_data
+        |> FormData.field_names()
+        |> Enum.sort()
+
+      assert names == ["email", "items[]"]
+    end
+  end
+
   describe "to_list" do
     test "transforms FormData into a list" do
       form_data =

test/phoenix_test/live_test.exs

@@ -964,6 +964,25 @@ defmodule PhoenixTest.LiveTest do
     end
   end
 
+  describe "general form logic" do
+    test "handles inputs_for ordinal inputs", %{conn: conn} do
+      conn
+      |> visit("/live/ordinal_inputs")
+      |> fill_in("Title", with: "Fellowship")
+      |> click_button("Add Email")
+      |> fill_in("#mailing_list_emails_0_email", "Email", with: "Bow")
+      |> click_button("Add Email")
+      |> fill_in("#mailing_list_emails_1_email", "Email", with: "Muffins")
+      |> click_button("Add Email")
+      |> fill_in("#mailing_list_emails_2_email", "Email", with: "Arrows")
+      |> click_link("a[phx-value-index='1']", "Remove")
+      |> submit()
+      |> assert_has("[data-role=email]", text: "Bow")
+      |> assert_has("[data-role=email]", text: "Arrows")
+      |> refute_has("[data-role=email]", text: "Muffins")
+    end
+  end
+
   describe "upload/4" do
     test "uploads an image", %{conn: conn} do
       conn

test/support/web_app/ordinal_inputs_live.ex

@@ -0,0 +1,127 @@
+defmodule PhoenixTest.WebApp.MailingList do
+  @moduledoc false
+  use Ecto.Schema
+
+  import Ecto.Changeset
+
+  embedded_schema do
+    field(:title, :string)
+
+    embeds_many :emails, Email, on_replace: :delete do
+      field(:email, :string)
+    end
+  end
+
+  def changeset(list, attrs) do
+    list
+    |> cast(attrs, [:title])
+    |> cast_embed(:emails,
+      with: &email_changeset/2,
+      sort_param: :emails_sort,
+      drop_param: :emails_drop
+    )
+  end
+
+  def email_changeset(email_notification, attrs) do
+    cast(email_notification, attrs, [:email])
+  end
+end
+
+defmodule PhoenixTest.WebApp.OrdinalInputsLive do
+  @moduledoc false
+  use Phoenix.LiveView
+  use Phoenix.Component
+
+  import PhoenixTest.WebApp.Components
+
+  alias PhoenixTest.WebApp.MailingList
+
+  def mount(_params, _session, socket) do
+    changeset = MailingList.changeset(%MailingList{}, %{})
+
+    {:ok,
+     assign(socket,
+       changeset: changeset,
+       form: to_form(changeset),
+       submitted: false,
+       emails: []
+     )}
+  end
+
+  def render(assigns) do
+    ~H"""
+    <.form for={@form} phx-change="validate" phx-submit="submit">
+      <.input field={@form[:title]} label="Title" />
+      <.inputs_for :let={ef} field={@form[:emails]}>
+        <input type="hidden" name="mailing_list[emails_sort][]" value={ef.index} />
+        <.input label="Email" type="text" field={ef[:email]} placeholder="email" />
+        <a class="underline" phx-click="remove-email" phx-value-index={ef.index}>
+          Remove
+        </a>
+      </.inputs_for>
+
+      <input type="hidden" name="mailing_list[emails_drop][]" />
+
+      <button phx-click="add-email">Add Email</button>
+      <button type="submit">Submit</button>
+    </.form>
+
+    <div>
+      <%= if @submitted do %>
+        <h3>Submitted Values:</h3>
+        <div>Title: {@form.params["title"]}</div>
+        <%= for email <- @emails do %>
+          <div data-role="email">{email}</div>
+        <% end %>
+      <% end %>
+    </div>
+    """
+  end
+
+  def handle_event("validate", %{"mailing_list" => params}, socket) do
+    changeset = MailingList.changeset(%MailingList{}, params)
+    {:noreply, assign(socket, changeset: changeset, form: to_form(changeset))}
+  end
+
+  def handle_event("add-email", _params, socket) do
+    changeset = socket.assigns.changeset
+
+    new_emails =
+      Ecto.Changeset.get_field(changeset, :emails) ++ [%MailingList.Email{}]
+
+    updated_changeset =
+      MailingList.changeset(%{changeset.data | emails: new_emails}, %{})
+
+    {:noreply, assign(socket, changeset: updated_changeset, form: to_form(updated_changeset))}
+  end
+
+  def handle_event("remove-email", %{"index" => index}, socket) do
+    index = String.to_integer(index)
+    current_emails = Ecto.Changeset.get_field(socket.assigns.changeset, :emails)
+
+    new_emails = List.delete_at(current_emails, index)
+
+    updated_changeset =
+      MailingList.changeset(%{socket.assigns.changeset.data | emails: new_emails}, %{})
+
+    {:noreply, assign(socket, changeset: updated_changeset, form: to_form(updated_changeset))}
+  end
+
+  def handle_event("submit", %{"mailing_list" => params}, socket) do
+    changeset = MailingList.changeset(%MailingList{}, params)
+
+    emails =
+      changeset
+      |> Ecto.Changeset.get_field(:emails)
+      |> Enum.map(fn email -> email.email end)
+      |> Enum.reject(&is_nil/1)
+
+    {:noreply,
+     assign(socket,
+       changeset: changeset,
+       form: to_form(changeset),
+       submitted: true,
+       emails: emails
+     )}
+  end
+end

test/support/web_app/router.ex

@@ -42,6 +42,7 @@ defmodule PhoenixTest.WebApp.Router do
       live "/live/page_2", Page2Live
       live "/live/async_page", AsyncPageLive
       live "/live/async_page_2", AsyncPage2Live
+      live "/live/ordinal_inputs", OrdinalInputsLive
       live "/live/dynamic_form", DynamicFormLive
       live "/live/simple_ordinal_inputs", SimpleOrdinalInputsLive
       live "/live/dynamic_inputs_add_remove", DynamicInputsAddRemoveLive