Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add params_with_assocs function #124

Merged
merged 1 commit into from
May 10, 2016
Merged

Add params_with_assocs function #124

merged 1 commit into from
May 10, 2016

Conversation

jsteiner
Copy link
Contributor

@jsteiner jsteiner commented May 5, 2016

Addresses: #105

@@ -66,6 +70,33 @@ defmodule ExMachina.Ecto do
|> drop_ecto_fields
end

@doc """
Same as `params_for/2`, but it inserts all associations and sets the
foreign_keys.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that it should only insert the belongs_to associations and not has_many. What do you think?

Same asparams_for/2, but it inserts all belongs_to associations and sets the foreign keys

@paulcsmith
Copy link
Contributor

This is awesome! I was actually going to ask if you had time to tackle this next :D

@jsteiner jsteiner force-pushed the js-params-with-assocs branch 3 times, most recently from ec71427 to 01dfce0 Compare May 6, 2016 17:10
@@ -66,6 +73,50 @@ defmodule ExMachina.Ecto do
|> drop_ecto_fields
end

@doc """
Same as `params_for/2`, but it inserts all belongs_to associations and sets the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this could be shortened to Same asparams_for/2, but inserts all belongs_to associations and sets the foreign keys? If you don't like that I'm fine with it as-is as well

@paulcsmith
Copy link
Contributor

A few comments, but this looks really good :) Good to merge whenever you're ready

@@ -43,14 +43,18 @@ defmodule ExMachina.EctoTest do
end
end

test "params_with_assocs/2 inserts belongs_to associations" do
test "params_with_assocs/2 inserts belongs_tos that are set by the factory" do
assert Enum.member?(ExMachina.Article.__schema__(:associations), :editor)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this, but I think extracting to a private function would be good. Something like assert has_association_in_schema?(ExMachina.Article, :editor). What do you think?

@jsteiner jsteiner force-pushed the js-params-with-assocs branch 2 times, most recently from 7377c3f to 607d082 Compare May 10, 2016 21:52
@jsteiner jsteiner merged commit 84ccf51 into master May 10, 2016
@jsteiner jsteiner deleted the js-params-with-assocs branch May 10, 2016 21:56
@paulcsmith paulcsmith mentioned this pull request Jun 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants