Improve project creation #1269
Improve project creation #1269
Conversation
begedin
left a comment
begedin
left a comment
There was a problem hiding this comment.
I like the potential simplification we're getting out of this. Seeing some inconsistencies, though.
| has_many :stripe_connect_subscriptions, through: [:stripe_connect_plan, :stripe_connect_subscriptions] | ||
|
|
||
| many_to_many :categories, CodeCorps.Category, join_through: CodeCorps.ProjectCategory | ||
| many_to_many :skills, CodeCorps.Skill, join_through: CodeCorps.ProjectSkill |
There was a problem hiding this comment.
Probably wanna do an on_delete: :delete_all here. Default is :nothing according to the docs.
If this works out, I wouldn't mind doing it with tasks and users where applicable to.
|
|
||
| defp do_associate_skills(changeset, _) do | ||
| changeset |> put_assoc(:skills, []) | ||
| end |
There was a problem hiding this comment.
We could save us a clause, and a do_associate helper, by using these:
defp associate_categories(changeset, %{"categories_ids" => ids}) when is_binary(ids) do
categories =
ids |> Enum.join(",") |> coalesce_id_string() |> find_categories()
changeset |> put_assoc(:categories, categories)
end
defp associate_categories(changeset, _), do: changeset |> put_assoc(:categories, [])There was a problem hiding this comment.
Note: I'm seeing some inconsistencies with coalesce_ids string and some of our tests.
If we're supposed to be getting a list here, instead of a csv idsstring, then the first clause would guard with
when is_list(ids) when length(ids) > 0 do| "categories_ids" => [category.id], | ||
| "cloudinary_public_id" => "foo123", | ||
| "description" => "Description", | ||
| "skills_ids" => [skill.id], |
There was a problem hiding this comment.
We are using coalesce_id_string above. Wouldn't that mean we're supposed to be sending csv ids here, instead of a list?
There was a problem hiding this comment.
No, because this is how JaSerializer modifies the data.
|
|
||
| defp do_associate_categories(changeset, _) do | ||
| changeset |> put_assoc(:categories, []) | ||
| end |
There was a problem hiding this comment.
I'm reasonably sure you could still rewrite this
defp associate_categories(changeset, %{"categories_ids" => []}) do
changeset |> put_assoc(:categories, [])
end
defp associate_categories(changeset, %{"categories_ids" => ids}) do
categories = ids |> Enum.map(&String.to_integer/1) |> find_categories()
changeset |> put_assoc(:categories, categories)
end
defp associate_categories(changeset, _) do
changeset |> put_assoc(:categories, [])
endas this
defp associate_categories(changeset, %{"categories_ids" => ids}) when is_list(ids) when length(ids) > 0 do
categories = ids |> Enum.map(&String.to_integer/1) |> find_categories()
changeset |> put_assoc(:categories, categories)
end
defp associate_categories(changeset, _) do
changeset |> put_assoc(:categories, [])
endHeck, since you're just using an enum map, you don't even need the when length(ids) > 0 part.
| end | ||
| defp associate_categories(changeset, _) do | ||
| changeset |> put_assoc(:categories, []) | ||
| end |
There was a problem hiding this comment.
👍
Same thing applies to associate_skills, of course. 😬
There was a problem hiding this comment.
Oh, node version manager, it's there.
joshsmith
force-pushed
the
improve-project-creation
branch
from
64ef63e to
d23e230
Compare
- Update views, controllers, changesets - Add missing required portions to changeset - Fix JSON API helpers to allow list relationships - Clean up JSON API helpers
joshsmith
force-pushed
the
improve-project-creation
branch
from
d23e230 to
d0ea4b5
Compare
What's in this PR?
Improves the project creation process by allowing nested skills/categories.