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

feat: Include ORM associations in CollectionDecorator #845

Merged
merged 2 commits into from
Feb 25, 2019
Merged

feat: Include ORM associations in CollectionDecorator #845

merged 2 commits into from
Feb 25, 2019

Conversation

brunohkbx
Copy link
Contributor

@brunohkbx brunohkbx commented Feb 20, 2019

Description

Include all QueryMethods from the ORM in CollectionDecorator. The default strategy is :active_record

  • Why was this change required?
    It was necessary to delegate or define a method of the ORM which you are using in your decorator to make an instance of CollectionDecorator able to call it.
  • Is there something you aren't happy with or that needs extra attention?
    In order to support other ORM associations, we'll need to write a method allowed? for each strategy at lib/draper/query_methods/load_strategy.rb

Testing

  1. Create a decorator for the model and its association
class OrderHistoryDecorator < Draper::Decorator
  delegate_all
end

class OrderDecorator < Draper::Decorator
  delegate_all

  decorates_association :order_histories, with: OrderHistoryDecorator
end
  1. Call any query method in the decorated instance
pry(main)> Order.last.decorate.order_histories.includes(:user)

References

before { allow(fake_strategy).to receive(:allowed?).with(:foo).and_return(true) }

it 'calls the method' do
allow(instance).to receive(:foo)
Copy link

Choose a reason for hiding this comment

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

this can't be on the before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer to write the specs using the format: setup/exercise/verify.

expect(Draper.default_controller).to be ApplicationController
end

it 'allows customizing default_controller through configure' do

Choose a reason for hiding this comment

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

Did you want to write allows customizing default_controller through configuration?

Choose a reason for hiding this comment

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

Or even, through Draper#configure, through configure method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure 🤔. I've just moved this old tests into a context, maybe these changes are out the scope of this pr?

Copy link
Collaborator

@codebycliff codebycliff left a comment

Choose a reason for hiding this comment

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

Hey guys, this looks good. I left some feedback below. I'll probably continue to play around with it over the weekend, but I think you can expect it to be merged by the end of the weekend.

README.md Outdated Show resolved Hide resolved
spec/draper/configuration_spec.rb Outdated Show resolved Hide resolved
spec/draper/configuration_spec.rb Outdated Show resolved Hide resolved
spec/draper/query_methods_spec.rb Outdated Show resolved Hide resolved
spec/draper/query_methods_spec.rb Outdated Show resolved Hide resolved
lib/draper/query_methods/load_strategy.rb Outdated Show resolved Hide resolved
lib/draper/query_methods/load_strategy.rb Outdated Show resolved Hide resolved
@brunohkbx
Copy link
Contributor Author

Hey @codebycliff. Thanks for your feedback! 😃
I've already pushed all changes and improved a test.
Let me know if everything is ok.

README.md Outdated Show resolved Hide resolved
Include all QueryMethods from the ORM in CollectionDecorator. The default adapter is :active_record

Resolves #702, #812
Copy link
Collaborator

@codebycliff codebycliff left a comment

Choose a reason for hiding this comment

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

Looks great! I have a couple applications running Draper master currently. I'll probably hold off on cutting a release to RubyGems for a week or two to make sure everything is good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants