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

Accept a lambda for context in factory #478

Merged
merged 1 commit into from Feb 17, 2013
Merged

Accept a lambda for context in factory #478

merged 1 commit into from Feb 17, 2013

Conversation

haines
Copy link
Contributor

@haines haines commented Feb 17, 2013

Like decorates_association, you can now pass a context lambda to decorates_assigned, which receives the current controller instance and returns a hash:

class ArticleController < ApplicationController
  decorates_assigned :article, context: ->(controller){ {user: controller.current_user} }
end

The actual calling of the lambda has been extracted to the Factory·

Closes #467

steveklabnik added a commit that referenced this pull request Feb 17, 2013
Accept a lambda for context in factory
@steveklabnik steveklabnik merged commit 3ab3b35 into drapergem:master Feb 17, 2013
@steveklabnik
Copy link
Member

👍 I like the symmetry.

@haines haines deleted the context_lambda branch February 19, 2013 07:49
@urbanautomaton
Copy link
Contributor

There's a minor problem here, unfortunately, due to the way ruby destructures hashes with splats:

> def splat(*args); args; end
=> nil
> splat(*{:foo => :bar})
=> [[:foo, :bar]]
> splat(*{:foo => :bar, :baz => :quux})
=> [[:foo, :bar], [:baz, :quux]]

As a result, if I have an owner with context {:foo => :bar}, the decorated association (using the default identity context lambda) will end up with context [:foo, :bar], because Factory splats the context args in #update_context:

class Thing < ActiveRecord::Base
  belongs_to :user
end

class User < ActiveRecord::Base; end

class ThingDecorator
  decorates_association :user
end

class UserDecorator; end
> Thing.first.decorate(:context => { :foo => :bar }).user
=> #<UserDecorator:0x000001030d0ad0
 @context=[:foo, :bar],
 @source=
  #<User id: 1>>

Also, because the identity lambda takes exactly one argument, if I have a more complex context, it raises an ArgumentError:

> Thing.first.decorate(:context => { :foo => :bar, :baz => :quux }).user
ArgumentError: wrong number of arguments (2 for 1)
from <gem_home>/draper/lib/draper/decorated_association.rb:14:in `block in initialize`

Rather than defaulting :context to the identity function and passing the owner's context as :context_args at decoration time, would it work to just pass the owner context in directly at decoration time, like so?

From 507860681dfebb6ee6f2405437d9622412dbec35 Mon Sep 17 00:00:00 2001
From: Simon Coffey
Date: Tue, 19 Feb 2013 14:30:59 +0000
Subject: [PATCH] Pass owner context directly in decorated association

---
 lib/draper/decorated_association.rb       | 4 ++--
 spec/draper/decorated_association_spec.rb | 8 +++-----
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/lib/draper/decorated_association.rb b/lib/draper/decorated_association.rb
index b2e8a1b..7aecd41 100644
--- a/lib/draper/decorated_association.rb
+++ b/lib/draper/decorated_association.rb
@@ -11,7 +11,7 @@ module Draper
       @scope = options[:scope]

       decorator_class = options[:with]
-      context = options.fetch(:context, ->(context){ context })
+      context = options.fetch(:context, {})
       @factory = Draper::Factory.new(with: decorator_class, context: context)
     end

@@ -28,7 +28,7 @@ module Draper
       associated = owner.source.send(association)
       associated = associated.send(scope) if scope

-      @decorated = factory.decorate(associated, context_args: owner.context)
+      @decorated = factory.decorate(associated, context: owner.context)
     end

   end
diff --git a/spec/draper/decorated_association_spec.rb b/spec/draper/decorated_association_spec.rb
index 62b4fed..f15e10b 100644
--- a/spec/draper/decorated_association_spec.rb
+++ b/spec/draper/decorated_association_spec.rb
@@ -28,10 +28,8 @@ module Draper
       end

       describe ":context option" do
-        it "defaults to the identity function" do
-          Factory.should_receive(:new).with do |options|
-            options[:context].call(:anything) == :anything
-          end
+        it "defaults to the empty hash" do
+          Factory.should_receive(:new).with(with: anything(), context: {})
           DecoratedAssociation.new(double, :association, {})
         end
       end
@@ -48,7 +46,7 @@ module Draper
         decorated_association = DecoratedAssociation.new(owner, :association, {})
         decorated = double

-        factory.should_receive(:decorate).with(associated, context_args: owner_context).and_return(decorated)
+        factory.should_receive(:decorate).with(associated, context: owner_context).and_return(decorated)
         expect(decorated_association.call).to be decorated
       end

-- 
1.7.11

@haines
Copy link
Contributor Author

haines commented Feb 19, 2013

@urbanautomaton Good catch, thanks.

The suggested patch doesn't work because we want the ability to pass a different context - either a static value decorates_association :something, context: {something: "else"}, or dynamically calculated using a lambda context: ->(owner_context) { owner_context.merge(blah: "blah") }.

I think the best fix is just to Array.wrap the args before splatting them. I think I might also rewrite the specs to be a bit less "stubby" so that things like this don't get through!

@urbanautomaton
Copy link
Contributor

Ah, cool, I wasn't aware of Array.wrap - I'd hesitated to try a fix at the Factory#update_context location because all I could come up with was some gross type inspection. :)

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.

Questions about decorates_assigned
3 participants