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 the has_many association for reviews from a user #10

Merged

Conversation

dgra
Copy link
Contributor

@dgra dgra commented May 25, 2017

No description provided.

@dgra
Copy link
Contributor Author

dgra commented May 30, 2017

@tvdeyen I was wondering if I could get this pull request looked at. It just adds the Spree::User#reviews has_many relation.

@dgra dgra mentioned this pull request Jun 16, 2017
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

As we can't know which user class is actually used in a store you should use Spree.user_class instead. And please try to avoid class_eval. This association above is easily written as:

Spree.user_class.has_many :reviews, class_name: 'Spree::Review'

As you can see you also need to define the reviews class, otherwise Rails won't be able to resolve it just from the association name.

@dgra
Copy link
Contributor Author

dgra commented Jun 19, 2017

Thank you for giving me feedback.
It is very helpful and I will try harder in the future to avoid class_eval.

Also, if this is approved, I will squash these two commits into one.

@tvdeyen
Copy link
Member

tvdeyen commented Jun 19, 2017

Now that we merged the Solidus 2.3 fixes, could you please rebase your branch on current master? This should fix the build.

Thanks for addressing the feedback

@dgra dgra force-pushed the user_reviews_association branch from ac8e7cc to bfb026b Compare June 19, 2017 21:59
@dgra
Copy link
Contributor Author

dgra commented Jun 19, 2017

I have rebased onto current master and squashed my commits.

Thank you again for providing feedback!

@tvdeyen tvdeyen merged commit 33c1ccf into solidusio-contrib:master Jun 20, 2017
@tvdeyen
Copy link
Member

tvdeyen commented Jun 20, 2017

Thank you!

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