Need help to refactor the resource creation code

One easy thing that will make this much more readable is

a
|> b()
|> c()
|> case do
  :foo -> nil
  :bar -> nil
end

instead of

case a |> b() |> c() do
  :foo -> nil
  :bar -> nil
end

You could use with statements if there are multiple failures possible.

However i think this does not fundamentally solves what you are trying to achieve. Maybe Ecto.Multi would be a better choice if you need to move files around as part of the transaction.

Its kind of hard to understand what is going on exactly in the snippet you posted. If it is one form, i guess you probably should have 1 changeset, also prepare_changes/2 allows you to run code in the transaction.

Using with instead of nested case statements could look like this: (i did not mess with the changeset)

    def create_recording(attrs \\ %{}) do
      Repo.transaction(fn ->
        with {:ok, recording} <- insert_recording(attrs),
             {:ok, with_audio} <- update_recording(recording) do
          with_audio
        else
          {:error, error} ->
            Repo.rollback(error)
        end
      end)
    end

    defp insert_recording(attrs \\ %{}) do
      %Recording{}
      |> Recording.changeset(attrs)
      |> Repo.insert()
    end

    defp update_recording(recording, attrs \\ %{}) do
      recording
      |> Recording.audio_changeset(attrs)
      |> Repo.update()
    end

If you are uploading a file and don’t want to depend on ids from the db, generate a random path for the uploaded file (eg uuid as filename). Maybe this helps you to get rid of the second changeset … You would probably still need a transaction to move/upload the file. Just be aware that storing a lot of files inside 1 directory is a performance killer.

Thank you for the answer. Your code looks great, I think with is the way to go. Also, we still need to reset the state filed to make the form render properly.

I can’t use prepare_changes/2 because resource’s id is not available on this stage yet.

try this:

  def create_recording(attrs \\ %{}) do
    Ecto.Multi.new()
    |> Ecto.Multi.insert(:recording, Recording.changeset(%Recording{}, attrs))
    |> Ecto.Multi.update(:recording_with_audio, &Recording.audio_changeset(&1.recording, attrs))
    |> Repo.transaction()
    |> case do
      {:ok, %{recording_with_audio: recording}} -> {:ok, recording}
      {:error, _operation, changeset, _} -> Ecto.Changeset.apply_action(changeset, :insert)
    end
  end

final version

  def create_recording(attrs \\ %{}) do
    Ecto.Multi.new()
    |> Ecto.Multi.insert(:recording, Recording.changeset(%Recording{}, attrs))
    |> Ecto.Multi.update(:recording_with_audio, &Recording.audio_changeset(&1.recording, attrs))
    |> Repo.transaction()
    |> case do
         {:ok, %{recording_with_audio: recording}} -> {:ok, recording}
         {:error, _operation, changeset, _} ->
           # make the form to a "new" state insted of "update"
           # https://github.com/phoenixframework/phoenix_ecto/blob/master/lib/phoenix_ecto/html.ex#L300
           meta = Map.put(changeset.data.__meta__, :state, nil)
           data = Map.put(changeset.data, :__meta__, meta)

           Map.put(changeset, :data, data)
           |> Ecto.Changeset.apply_action(:insert)
       end
  end