Fix param handling for nested embeds

lib/polymorphic_embed/html/helpers.ex

@@ -49,45 +49,24 @@ if Code.ensure_loaded?(Phoenix.HTML) && Code.ensure_loaded?(Phoenix.HTML.Form) d
       form.source.data.__struct__
     end
 
-    def to_form(%{action: parent_action} = source_changeset, form, field, options) do
+    def to_form(source_changeset, %{action: parent_action} = form, field, options) do
       id = to_string(form.id <> "_#{field}")
       name = to_string(form.name <> "[#{field}]")
 
       params = Map.get(source_changeset.params || %{}, to_string(field), %{}) |> List.wrap()
 
       struct = Ecto.Changeset.apply_changes(source_changeset)
 
-      list_data =
-        case Map.get(struct, field) do
-          nil ->
-            type = Keyword.get(options, :polymorphic_type, get_polymorphic_type(form, field))
-            module = PolymorphicEmbed.get_polymorphic_module(struct.__struct__, field, type)
-            if module, do: [struct(module)], else: []
-
-          data ->
-            List.wrap(data)
-        end
-
-      list_data
+      resolve_field_data(source_changeset, struct, form, field, options)
       |> Enum.with_index()
-      |> Enum.map(fn {data, i} ->
-        params = Enum.at(params, i) || %{}
-
-        %Ecto.Changeset{} =
-          changeset =
-          data
-          |> Ecto.Changeset.change()
-          |> apply_action(parent_action)
-
-        errors = get_errors(changeset)
-
-        changeset = %Ecto.Changeset{
-          changeset
-          | action: parent_action,
-            params: params,
-            errors: errors,
-            valid?: errors == []
-        }
+      |> Enum.map(&prepare_changeset(&1, params, parent_action))
+      |> Enum.map(fn prepared_data ->
+        %{
+          changeset: changeset,
+          params: params,
+          errors: errors,
+          index: i
+        } = prepared_data
 
         %schema{} = source_changeset.data
 
@@ -107,7 +86,7 @@ if Code.ensure_loaded?(Phoenix.HTML) && Code.ensure_loaded?(Phoenix.HTML.Form) d
           name: if(array?, do: name <> "[" <> index_string <> "]", else: name),
           index: if(array?, do: i),
           errors: errors,
-          data: data,
+          data: changeset.data,
           action: parent_action,
           params: params,
           hidden: [{type_field_name, to_string(type)}],
@@ -116,6 +95,57 @@ if Code.ensure_loaded?(Phoenix.HTML) && Code.ensure_loaded?(Phoenix.HTML.Form) d
       end)
     end
 
+    defp resolve_field_data(source_changeset, struct, form, field, options) do
+      case Map.get(source_changeset.changes, field) do
+        nil ->
+          case Map.get(struct, field) do
+            nil ->
+              type = Keyword.get(options, :polymorphic_type, get_polymorphic_type(form, field))
+              module = PolymorphicEmbed.get_polymorphic_module(struct.__struct__, field, type)
+              if module, do: [struct(module)], else: []
+
+            data ->
+              List.wrap(data)
+          end
+
+        data ->
+          List.wrap(data)
+      end
+    end
+
+    defp prepare_changeset({%Ecto.Changeset{} = changeset, i}, _params, parent_action) do
+      params = changeset.params || %{}
+      changeset = apply_action(changeset, parent_action)
+      errors = get_errors(changeset)
+
+      %{
+        changeset: changeset,
+        params: params,
+        errors: errors,
+        index: i
+      }
+    end
+
+    defp prepare_changeset({data, i}, params, parent_action) do
+      params = Enum.at(params, i) || %{}
+
+      changeset =
+        data
+        |> Ecto.Changeset.change()
+        |> apply_action(parent_action)
+
+      errors = get_errors(changeset)
+
+      changeset = %Ecto.Changeset{
+        changeset
+        | params: params,
+          errors: errors,
+          valid?: errors == []
+      }
+
+      %{changeset: changeset, params: params, errors: errors, index: i}
+    end
+
     # If the parent changeset had no action, we need to remove the action
     # from children changeset so we ignore all errors accordingly.
     defp apply_action(changeset, nil), do: %{changeset | action: nil}

test/polymorphic_embed_test.exs

@@ -2770,25 +2770,58 @@
     for generator <- @generators do
       reminder_module = get_module(Reminder, generator)
 
-      attrs = %{
-        date: ~U[2020-05-28 02:57:19Z],
-        text: "This is an Email reminder",
-        channel: %{
-          address: "a",
-          valid: true,
-          confirmed: true
-        }
-      }
+      attrs =
+        if polymorphic?(generator) do
+          %{
+            date: ~U[2020-05-28 02:57:19Z],
+            text: "This is an Email reminder",
+            channel: %{
+              address: "a",
+              valid: true,
+              confirmed: true
+            }
+          }
+        else
+          %{
+            number: ~U[2020-05-28 02:57:19Z],
+            text: "This is a non polymorphic reminder",
+            channel: %{
+              number: "a"
+            }
+          }
+        end
 
       changeset =
         struct(reminder_module)
         |> reminder_module.changeset(attrs)
 
+      # Without parent action, errors should be empty in both cases
+      # (mirroring standard Ecto embed behavior)
+      safe_inputs_for(changeset, :channel, generator, fn f ->
+        assert f.impl == Phoenix.HTML.FormData.Ecto.Changeset
+        assert is_struct(f.data), "f.data should be a struct, not a changeset"
+        refute match?(%Ecto.Changeset{}, f.data), "f.data should not be a changeset"
+        assert f.errors == []
+        assert f.action == nil
+
+        if polymorphic?(generator), do: text_input(f, :address), else: text_input(f, :number)
+      end)
+
+      changeset = Map.put(changeset, :action, :insert)
+
+      # With parent action, validation errors should surface in both cases
       contents =
         safe_inputs_for(changeset, :channel, generator, fn f ->
           assert f.impl == Phoenix.HTML.FormData.Ecto.Changeset
-          assert f.errors == []
-          text_input(f, :address)
+          assert is_struct(f.data), "f.data should be a struct, not a changeset"
+          refute match?(%Ecto.Changeset{}, f.data), "f.data should not be a changeset"
+
+          assert f.errors != [],
+                 "errors should be present when parent has action and data is invalid"
+
+          assert f.action == :insert
+
+          if polymorphic?(generator), do: text_input(f, :address), else: text_input(f, :number)
         end)
 
       expected_contents =
@@ -2798,7 +2831,7 @@
           <input id="reminder_channel_address" name="reminder[channel][address]" type="text" value="a">
           """,
           else: ~s"""
-          <input id="reminder_channel_address" name="reminder[channel][address]" type="text" value="a">
+          <input id="reminder_channel_number" name="reminder[channel][number]" type="text" value="a">
           """
         )
 
@@ -2811,7 +2844,12 @@
           generator,
           fn f ->
             assert f.impl == Phoenix.HTML.FormData.Ecto.Changeset
-            text_input(f, :address)
+
+            if polymorphic?(generator) do
+              text_input(f, :address)
+            else
+              text_input(f, :number)
+            end
           end
         )
 
@@ -2822,14 +2860,65 @@
           <input id="reminder_channel_address" name="reminder[channel][address]" type="text" value="a">
           """,
           else: ~s"""
-          <input id="reminder_channel_address" name="reminder[channel][address]" type="text" value="a">
+          <input id="reminder_channel_number" name="reminder[channel][number]" type="text" value="a">
           """
         )
 
       assert contents == String.replace(expected_contents, "\n", "")
     end
   end
 
+  test "polymorphic_embed_inputs_for ignores child errors when parent action is nil" do
+    reminder_module = get_module(Reminder, :polymorphic)
+
+    attrs = %{
+      date: ~U[2020-05-28 02:57:19Z],
+      text: "This is an Email reminder",
+      channel: %{
+        address: "a",
+        valid: true,
+        confirmed: true
+      }
+    }
+
+    changeset =
+      struct(reminder_module)
+      |> reminder_module.changeset(attrs)
+
+    safe_inputs_for(changeset, :channel, :polymorphic, fn f ->
+      assert f.impl == Phoenix.HTML.FormData.Ecto.Changeset
+      assert f.errors == []
+      text_input(f, :address)
+    end)
+  end
+
+  test "polymorphic_embed_inputs_for/4 applies parent action to a prebuilt child changeset" do
+    child_changeset =
+      PolymorphicEmbed.Channel.Email.changeset(
+        %PolymorphicEmbed.Channel.Email{},
+        %{address: "a", valid: true, confirmed: true}
+      )
+
+    changeset =
+      Ecto.Changeset.change(%PolymorphicEmbed.Reminder{})
+      |> Ecto.Changeset.put_change(:channel, child_changeset)
+      |> Map.put(:action, :insert)
+
+    safe_inputs_for(changeset, :channel, :polymorphic, fn f ->
+      assert f.impl == Phoenix.HTML.FormData.Ecto.Changeset
+      assert f.action == :insert
+      assert f.source.action == :insert
+
+      assert f.errors == [
+               {:address,
+                {"should be at least %{count} character(s)",
+                 [count: 3, validation: :length, kind: :min, type: :string]}}
+             ]
+
+      text_input(f, :address)
+    end)
+  end
+
   test "polymorphic_embed_inputs_for/4 for list of embeds" do
     for generator <- @generators do
       reminder_module = get_module(Reminder, generator)
@@ -2908,6 +2997,64 @@
     end
   end
 
+  test "to_form/4 keeps sorted embeds_many params aligned with each row" do
+    reminder_module = get_module(Reminder, :polymorphic)
+
+    attrs = %{
+      "date" => ~U[2020-05-28 02:57:19Z],
+      "text" => "This is a reminder with sorted contexts",
+      "channel" => %{
+        "my_type_field" => "sms",
+        "number" => "02/807.05.53",
+        "country_code" => 1,
+        "provider" => %{
+          "__type__" => "twilio",
+          "api_key" => "foo"
+        }
+      },
+      "contexts" => %{
+        "0" => %{
+          "__type__" => "device",
+          "_persistent_id" => "device-row",
+          "ref" => "12345",
+          "type" => "cellphone"
+        },
+        "1" => %{
+          "__type__" => "age",
+          "_persistent_id" => "age-row",
+          "age" => "aquarius"
+        }
+      },
+      "contexts_sort" => ["1", "0"]
+    }
+
+    changeset =
+      struct(reminder_module)
+      |> reminder_module.changeset(attrs)
+
+    form = Phoenix.Component.to_form(changeset)
+    forms = PolymorphicEmbed.HTML.Helpers.to_form(changeset, form, :contexts, [])
+
+    assert Enum.map(forms, & &1.data.__struct__) == [
+             PolymorphicEmbed.Reminder.Context.Age,
+             PolymorphicEmbed.Reminder.Context.Device
+           ]
+
+    assert Enum.map(forms, & &1.params) == [
+             %{
+               "__type__" => "age",
+               "_persistent_id" => "age-row",
+               "age" => "aquarius"
+             },
+             %{
+               "__type__" => "device",
+               "_persistent_id" => "device-row",
+               "ref" => "12345",
+               "type" => "cellphone"
+             }
+           ]
+  end
+
   test "polymorphic_embed_inputs_for/4 after invalid insert" do
     for generator <- @generators do
       reminder_module = get_module(Reminder, generator)