acrolink
Code review needed: creating multiple records sharing some values on one API call
With the help and code samples from friends here (@idi527 and others) I have written this function to create multiple book related records. I pass to this function some shared data (current_user and borrower_id) along with a comma separated list of book_ids. I make some custom validations, insert if everything is fine or return error messages if something is wrong. What I receive back at the client side is the result list (as JSON):
def create_record(%{"user_object" => user, "borrower_id" => borrower_id} = attrs, book_list) do
book_list = String.split(book_list, ",")
result = []
for book_id <- book_list do
book_id = book_id |> String.to_integer
alias Ecto.Multi
record_changeset = Mango.Records.Record.changeset(%Record{user_id: user.id, book_id: book_id}, attrs)
Multi.new()
|> Mango.Records.Validators.valid_book_exists_multi(book_id)
|> Mango.Records.Validators.valid_book_belongs_to_current_user_and_borrower_multi(book_id, user, borrower_id)
|> Mango.Records.Validators.valid_last_record_closed_multi(book_id)
|> Multi.insert(:record, record_changeset)
|> Repo.transaction()
|> case do
{:ok, :first_record} -> {:ok}
{:ok, %{record: record}} -> result = result ++ record
{:error, :book_belongs_to_current_user_and_borrower, reason, _changes} -> result = result ++ reason
{:error, :book_exists, reason, _changes} -> result = result ++ reason
{:error, :last_record_closed, reason, _changes} -> result = result ++ reason
end
end
end
You’re welcome to review it and provide feedback ![]()
Most Liked
idi527
Does it work?
Note that for ... <- ... is not a for loop, but a list comprehension. You don’t need to do result = [] and result = result ++ reason.
I’d probably try and “invert” the logic and first do the list comprehension, and then all these Multi and Repo.transaction …
But without significantly changing the logic of your example, I’d probably write something like:
alias Ecto.Multi
alias Mango.Records.{Record, Validators}
# here I suppose book_list is an already parsed list of integers (book ids)
# so no `book_list = String.split(book_list, ",")` <-- cleaning up and preparing
# the inputs is the responsibility of a "boundary" (like a controller action in phoenix)
@typep multi_result :: {:ok, %Record{} | :first_record} | {:error, operation :: atom, reason :: any, reverted_changes :: map}
@spec create_records(%{(String.t | atom) => term}, %User{}, pos_integer, [pos_integer]) ::
%{(book_id :: pos_integer) => multi_result}
def create_records(attrs, %User{id: user_id} = user, borrower_id, book_ids) do
book_ids
|> Stream.map(fn book_id ->
{book_id, Record.changeset(%Record{user_id: user_id, book_id: book_id}, attrs)}
end)
|> Stream.map(fn {book_id, record_changeset} ->
multi_result =
Multi.new()
|> Validators.valid_book_exists_multi(book_id)
|> Validators.valid_book_belongs_to_current_user_and_borrower_multi(book_id, user, borrower_id)
|> Validators.valid_last_record_closed_multi(book_id)
|> Multi.insert(:record, record_changeset)
|> Repo.transaction()
{book_id, multi_result}
end)
|> Enum.into(%{})
end
It’s also not quite clear to me what this code is supposed to do and whether it should halt at invalid changesets / failed “validators” or continue …
joaquinalcerro
You might want to separate the Multi related code to a different function for code reuse. For example:
def validations(record_changeset, book_id, borrower_id) do
Multi.new()
|> Mango.Records.Validators.valid_book_exists_multi(book_id)
|> Mango.Records.Validators.valid_book_belongs_to_current_user_and_borrower_multi(book_id, user, borrower_id)
|> Mango.Records.Validators.valid_last_record_closed_multi(book_id)
|> Multi.insert(:record, record_changeset)
end
def create_record(%{"user_object" => user, "borrower_id" => borrower_id} = attrs, book_list) do
book_list = String.split(book_list, ",")
result = []
for book_id <- book_list do
book_id = book_id |> String.to_integer
record_changeset = Mango.Records.Record.changeset(%Record{user_id: user.id, book_id: book_id}, attrs)
case Repo.transaction(validations(record_changeset, book_id, borrower_id)) do
{:ok, :first_record} -> {:ok}
{:ok, %{record: record}} -> result = result ++ record
{:error, :book_belongs_to_current_user_and_borrower, reason, _changes} -> result = result ++ reason
{:error, :book_exists, reason, _changes} -> result = result ++ reason
{:error, :last_record_closed, reason, _changes} -> result = result ++ reason
end
end
end
Popular in Questions
Other popular topics
Categories:
Sub Categories:
Forums
Popular Tags
- #ecto
- #liveview
- #troubleshooting
- #learning-elixir
- #deployment
- #library
- #erlang
- #testing
- #genserver
- #mix
- #absinthe
- #remote-other
- #otp
- #plug
- #how-to-question
- #macros
- #postgres
- #channels
- #elixirconf
- #exunit
- #discussion
- #code-sync
- #javascript
- #podcasts
- #onsite
- #dialyzer
- #docker
- #authentication
- #umbrella
- #full-time-contract
- #podcasts-by-brainlid
- #ecto-query
- #elixir-ls
- #phoenix_html
- #iex
- #blog-post
- #graphql
- #genstage
- #ai
- #websockets
- #supervisor
- #advent-of-code
- #elixirconf-us
- #distillery
- #processes
- #forms
- #api
- #metaprogramming
- #security
- #performance









