Feedback on some Phoenix code

Phoenix Forum Questions / Help
,
1

I’m writing my first non-toy app in Phoenix. I’m starting grok schemas, changesets, and ecto in general, but I have a ways to go.

So I have an Organization schema living inside an Organizations context. When an Organization is created, I want to capture the user that created it and the user who owns it. On create, these two are always going to be the same user.

Is there an idiom to follow here? I had a few iterations:

The one I have landed on (for now) involves setting a virtual creator field and setting the users in the changeset. It looks like this (note, I have omitted most boilerplate):

# context
defmodule MyApp.Organizations do
  def create_organization(attrs) do
    %Organization{}
    |> Organization.create_changeset(attrs)
    |> Repo.insert()
  end
end

# schema
defmodule MyApp.Organization do
  schema "organizations" do
    field :name, :string
    belongs_to :created_by, MyApp.Accounts.User
    belongs_to :owned_by, MyApp.Accounts.User
    field :creator, :map, virtual: true

    timestamps()
  end

  def create_changeset(organization, attrs) do
    organization
    |> cast(attrs, [:name, :creator])
    |> validate_required([:name, :creator])
    |> put_users()
  end

  defp put_users(changeset) do
    creator = get_change(changeset, :creator)

    changeset
    |> put_change(:owned_by, creator)
    |> put_change(:created_by, creator)
  end
end

This next bit is an earlier iteration where I set the users in the context.

# context
defmodule MyApp.Organizations do
  def create_organization(user, attrs) do
    attrs =
      attrs
      |> Map.put(created_by: user)
      |> Map.put(owned_by: user)

    %Organization{}
    |> Organization.create_changeset(attrs)
    |> Repo.insert()
  end
end

# schema
defmodule MyApp.Organization do
  schema "organizations" do
    field :name, :string
    belongs_to :created_by, MyApp.Accounts.User
    belongs_to :owned_by, MyApp.Accounts.User

    timestamps()
  end

  def create_changeset(organization, attrs) do
    organization
    |> cast(attrs, [:name, :created_by, :owned_by])
    |> validate_required([:name, :created_by, :owned_by])
  end
end

So the only thing I like about the second one is that you Organizations.create_organization/2 takes a user as a required param which immediately calls out that a user is required to create and org. Of course, the final iteration also ensures a user exists, it’s just not immediately apparently by just looking at the context (is this important? Maybe not… I should be looking at the schemas anyway).

I think the motivation behind this question is that in Rails, we would do something like current_user.organizations.create(params) and the fact that the org is owned by a user is but only in the callsite. I like how requiring a user param in my Phoenix version makes that relationship clear in the signature but I’m not married to it. I’m really just wondering what is idiomatic.

Otherwise, I’d love to just get general feedback on ways to improve in general.

Thank you!

2

I prefer to use Ecto.build_assoc for this than casting ids in the changeset.

3

So are you advocating for using build_assoc in the context? What is being asked in this post (Why do we have Ecto.build_assoc?) is actually what I had in the first place—%Organization{created_at: user, owned_by: user} |> Organization.changeset(attrs)—but I wasn’t sure if that idiomatic or not. build_assoc seems like it cleans that up a bit. At first I really felt that logic belonged in the context and then I wasn’t so sure and now I’m thinking it should again, heh. Any advantages either way? I think I need to accept that Phoenix isn’t anywhere near as opinionated as Rails (which is fine, I just need to accept it).

4

No, not really… as I get the user in the controller, I just do it in the controller, as it is just one line.

But this could be easily hidden in the context, if You want.

5

Ah ok, I see. I’m wondering the use of build_assoc now as I’ve been playing around and this solution is the shortest. It’s also starting to feel like the clearest. I get to explicitly pass the user and there are no single-use virtual fields.

# context
  def create_organization(user, attrs) do
    %Organization{created_by: user, owned_by: user}
    |> Organization.create_changeset(attrs)
    |> Repo.insert()
  end

# schema
  def create_changeset(organization, attrs) do
    organization
    |> cast(attrs, [:name])
    |> cast_assoc(:created_by)
    |> cast_assoc(:owned_by)
    |> validate_required([:name, :created_by, :owned_by])
  end

Thanks for your responses!

1 Like
6

I also realize the cast_assoc calls are completely useless since its trusted data.

7

Yes, I prefer to do it without having to cast any external ids.

8

Not only that, but all cast* methods only apply for whatever is passed through attrs. So cast_assoc wouldn’t do anything in the best case and allow people to tamper with the owner and creator in the worst case.

1 Like
9

Right. I definitely don’t want to be sending the user along in the params—I just wasn’t thinking clearly on exactly what cast_assoc does. I’m also trying to isolate business rules although I haven’t started developing a functional core without Phoenix (which I have questions for another thread about… mostly around naming, heh).