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 better support for activerecord relations and mongoid criteria #662

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

donaldpiret
Copy link

Cleaned up implementation of #610 with Mongoid support

@mikedao
Copy link
Contributor

mikedao commented Apr 3, 2015

I think a valuable question here is if we want to explicitly continue support for Mongoid.

What does everyone think about this? I think a good discussion about it either way would be valuable, arguments for and arguments against.

@colinpetruno
Copy link

I was working with your branch and it works well but I noticed two small issues.

  1. edit my own fault
  2. RelationDecorator doesn't work with render collection: Model.decorated_associaton. It won't error but it doesn't render any of the partials so it seems unable to find the models in RelationDecorator.

The ActionView partial renderer calls to_ary on the object passed the collection parameter. I was able to resolve number two by defining this method. There may be a better way already in draper.

# lib/draper/relation_decorator.rb
def to_ary
   relation.map(&:decorate)
end

@donaldpiret
Copy link
Author

Hi @colinpetruno, awesome, thanks for reporting these. I'll try to get issue 2 resolved over the next few days or this weekend at the latest.

@saneshark
Copy link

👍 can you delegate_all from relation_decorator? I would imagine yes, and perhaps it is implicit, similar to SimpleDelegator's method missing I imagine, right? Otherwise what's the point of distinguishing it as a relation?

@saneshark
Copy link

README documentation needed before merge?

@donaldpiret
Copy link
Author

Hi @saneshark, not sure I understand your question regarding the delegate_all. If your decorator implements delegate_all, the decorated model objects returned by accessing a member of the relation will respect this as well.
The point of distinguishing it as a relations rather than turning it into a collection is to enable further chaining of scopes down the line, so your .decorate call can be called at any time, not necessarily at the end.
It also supports pagination out of the box by delegating the pagination methods to the relation directly and handling the decoration of the returned model instances.

For example you can now do:

User.decorate.page(2).per(1).first and this will return a decorated user instance.

I'm happy to write up documentation for this if there is a real intention of getting this branch merged. Let me know if this is the case and I'll write up some proper docs.

In order to properly support `render collection: Model.scope.decorate`
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.

None yet

5 participants