# Need help to refactor the resource creation code

Hi,

I’m trying to create a form with file field to upload a file. In order to store file into the proper folder I need to know resourse’s `id` in advance. So, I’ve split the creation transaction to two separate change-sets: `changeset` for the form fields and `audio_changeset` to upload the file.

When I call `audio_changeset` to update changeset and upload the file with invalid data, it changes the form state to update which changes form rendering as `put` action. This all happen inside the creating resource transaction. To overcome this and reset form status back to creating resource state I need to reset `changeset.data.__meta__.state` to nil.

It works but I think it has a better and prettier solution

``````  def create_recording(attrs \\ %{}) do
Repo.transaction(fn ->
%Recording{}
|> Recording.changeset(attrs)
|> Repo.insert()
|> case do
{:ok, recording} ->
Recording.audio_changeset(recording, attrs)
|> Repo.update()
|> case do
{:ok, recording_with_audio} -> recording_with_audio
{:error, changeset} ->
# make the form to a "new" state insted of "update"
meta = Map.put(changeset.data.__meta__, :state, nil)
data = Map.put(changeset.data, :__meta__, meta)

changeset
|> Map.put(:data, data)
|> Repo.rollback()
end

{:error, error} -> Repo.rollback(error)
end
end)
end
``````

One easy thing that will make this much more readable is

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

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

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.

1 Like

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
``````
3 Likes

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

``````
1 Like