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

refactor: move module query-methods into collection-decorator #874

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/draper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
require 'draper/decorated_association'
require 'draper/helper_support'
require 'draper/view_context'
require 'draper/query_methods'
require 'draper/load_strategy'
require 'draper/collection_decorator'
require 'draper/undecorate'
require 'draper/decorates_assigned'
Expand Down
17 changes: 16 additions & 1 deletion lib/draper/collection_decorator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ module Draper
class CollectionDecorator
include Enumerable
include Draper::ViewHelpers
include Draper::QueryMethods
extend Draper::Delegation

# @return the collection being decorated.
Expand Down Expand Up @@ -72,6 +71,17 @@ def replace(other)
self
end

# Proxies missing query methods to the source class if the strategy allows.
def method_missing(method, *args, &block)
return super unless strategy.allowed? method
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm new here, so please forgive my stupid question 🙏

Why do we need any strategy in the first place? Isn't object.respond_to? enough?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see: it's for methods returning decoratable collections. It's not obvious for everyone, though (see #867).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return super unless strategy.allowed? method
return super unless object.respond_to? method


object.send(method, *args, &block).decorate(with: decorator_class, context: context)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
object.send(method, *args, &block).decorate(with: decorator_class, context: context)
object.send(method, *args, &block).try(:decorate, with: decorator_class, context: context)

end

def respond_to_missing?(method, include_private = false)
strategy.allowed?(method) || super
end

protected

# Decorates the given item.
Expand All @@ -88,5 +98,10 @@ def item_decorator
->(item, options) { item.decorate(options) }
end
end

# Configures the strategy used to proxy the query methods, which defaults to `:active_record`.
def strategy
@strategy ||= LoadStrategy.new(Draper.default_query_methods_strategy)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the name LoadStrategy still make sense in this new context? Should it be something like QueryMethodStrategy or something more specific to query methods? I don't have a big opinion here, but it does seem odd.

end
end
end
19 changes: 19 additions & 0 deletions lib/draper/load_strategy.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
module Draper
module LoadStrategy
def self.new(name)
const_get(name.to_s.camelize).new
end

class ActiveRecord
def allowed?(method)
::ActiveRecord::Relation::VALUE_METHODS.include? method
end
end

class Mongoid
def allowed?(method)
raise NotImplementedError
end
end
end
end
23 changes: 0 additions & 23 deletions lib/draper/query_methods.rb

This file was deleted.

21 changes: 0 additions & 21 deletions lib/draper/query_methods/load_strategy.rb

This file was deleted.

74 changes: 74 additions & 0 deletions spec/draper/collection_decorator_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
require 'spec_helper'
require 'support/shared_examples/view_helpers'

require_relative '../dummy/app/decorators/post_decorator'

Post = Struct.new(:id) { }

module Draper
describe CollectionDecorator do
it_behaves_like "view helpers", CollectionDecorator.new([])
Expand Down Expand Up @@ -286,5 +290,75 @@ module Draper
expect(decorator.replace([:foo, :bar])).to be decorator
end
end

describe '#method_missing' do
let(:collection) { [ Post.new, Post.new ] }
let(:collection_context) { { user: 'foo' } }
let(:fake_strategy) { instance_double(LoadStrategy::ActiveRecord) }

let(:collection_decorator) do
PostDecorator.decorate_collection(
collection,
context: collection_context
)
end

before { allow(LoadStrategy).to receive(:new).and_return(fake_strategy) }

context 'when strategy allows collection to call the method' do
let(:results) { spy(:results) }

before do
allow(fake_strategy).to receive(:allowed?).with(:some_query_method).and_return(true)
allow(collection).to receive(:send).with(:some_query_method).and_return(results)
end

it 'calls the method on the collection and decorate it results' do
collection_decorator.some_query_method

expect(results).to have_received(:decorate)
end

it 'calls the method on the collection and keeps the decoration options' do
collection_decorator.some_query_method

expect(results).to have_received(:decorate).with({ context: collection_context, with: PostDecorator })
end
end

context 'when strategy does not allow collection to call the method' do
before { allow(fake_strategy).to receive(:allowed?).with(:some_query_method).and_return(false) }

it 'raises NoMethodError' do
expect { collection_decorator.some_query_method }.to raise_exception(NoMethodError)
end
end
end

describe "#respond_to?" do
let(:collection) { [ Post.new, Post.new ] }
let(:collection_decorator) { PostDecorator.decorate_collection(collection) }
let(:fake_strategy) { instance_double(LoadStrategy::ActiveRecord) }

subject { collection_decorator.respond_to?(:some_query_method) }

before { allow(LoadStrategy).to receive(:new).and_return(fake_strategy) }

context 'when strategy allows collection to call the method' do
before do
allow(fake_strategy).to receive(:allowed?).with(:some_query_method).and_return(true)
end

it { is_expected.to eq(true) }
end

context 'when strategy does not allow collection to call the method' do
before do
allow(fake_strategy).to receive(:allowed?).with(:some_query_method).and_return(false)
end

it { is_expected.to eq(false) }
end
end
end
end
70 changes: 0 additions & 70 deletions spec/draper/query_methods_spec.rb

This file was deleted.