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

Activerecord relations #610

Closed

Conversation

donaldpiret
Copy link

Better handling of activerecord relations. One can now call the decorator anywhere in the scope chain, and keep your relation decorated. Once you call methods that access individual records or arrays of records it automatically converts your result to a collection decorator or a regular decorator.

Would love to get some feedback on this.

@@ -49,15 +49,15 @@ def decorated?

module ClassMethods

# Decorates a collection of objects. Used at the end of a scope chain.
# Decorates an activerecord relation. Used at any point of the scope chain.
Copy link
Member

Choose a reason for hiding this comment

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

ActiveRecord

@steveklabnik
Copy link
Member

I really like this idea overall. I don't have enough time to give it a proper review right now, though. :/

@steveklabnik
Copy link
Member

Could you rebase this, please? I've updated the travis file to actually build the correct rubies, and I'd like to see if it passes. Thanks!

@donaldpiret
Copy link
Author

Looks like it isn't passing. I'll have some free time after wednesday so will look into getting everything passing.

@donaldpiret
Copy link
Author

Well that was easier than expected. Seems to be passing now.

@voondo
Copy link

voondo commented Sep 28, 2014

I confirm it works very well. Thanks @donaldpiret !

@magicmarkker
Copy link

Any idea when this will be merged? I'm having an issue where I do @current_user = User.find(user_id).decorate. But then when I call @current_user.some_associated_model, it's telling me that the method doesn't exist. This pull request seems like it would fix that...

@steveklabnik
Copy link
Member

It no longer merges correctly, and needs a rebase

@magicmarkker
Copy link

@donaldpiret

@donaldpiret
Copy link
Author

@magicmarkker Thanks for the head's-up. I'll try to rebase it later today or tomorrow when I have a bit of time.

@donaldpiret
Copy link
Author

@magicmarkker @steveklabnik Rebased. Some of the builds are failing on Travis but those don't seem to have anything to do with my changes, and apparently has been failing since https://travis-ci.org/drapergem/draper/builds/35338408

@chibicode
Copy link

.idea directory should probably be removed :)

@donaldpiret
Copy link
Author

Done!

@jcasimir
Copy link
Member

I'm interested in this. Let's see if we can get it passing on CI.

@donaldpiret
Copy link
Author

I'll try to spend some time on this tomorrow to bring it up to date. Using my branch in production so would love to see it merged in as well.

@donaldpiret
Copy link
Author

Getting one failure with mongoid. Not super familiar with it so might take me a little bit of time to get around.

@donaldpiret
Copy link
Author

Closing in favor of cleaned up version: #662

@saneshark
Copy link

👍 @donaldpiret, DecoratorRelation will be able to do delegate_all, yes? Otherwise this would be a problem for say, pagination params that are only part of the CollectionProxy object. Currently CollectionDecorator does not allow delegate_all

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.

7 participants