Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

added should_authorize_for 'macro' for the common controller expectation

  • Loading branch information...
commit 607218097eda8a44a034f293e405189a8e7c56dd 1 parent 8fae9f3
Dmytrii Nagirniak authored
Showing with 28 additions and 6 deletions.
  1. +5 −0 README.md
  2. +23 −6 lib/allowy/rspec.rb
5 README.md
View
@@ -216,6 +216,11 @@ describe PagesController do
it "will always allow no matter what" do
post(:create).should be_success
end
+
+ it "checks the authorisation" do
+ should_authorize_for(:create, page)
Dmytrii Nagirniak Owner
dnagir added a note

Ideally I'd like to write something like 'allowy.should authorize_for(:create, page)' but not sure how to write a proper matcher.

Matchers are designed to pass/fail immediately, whereas message expectations are designed to pass/fail when they receive an unexpected message or at the end of the example if they never received the expected message. What you're looking for is not really a matcher, but a way to instrument the example with message expectations using matcher-like syntax. If that's what you really want, then just always return true from matches? and always return false from does_not_match? and set the expectations you want within those methods. You're basically breaking the should/matcher contract to do this, so please consider the confusion that will set in in 6 months when this matcher works differently from all other matchers.

Personally I'd probably just wrap the expectations in a normal method. Something like what you have, but named differently. Maybe expect_authorization_request(:create, page) or some such. WDYT?

Dmytrii Nagirniak Owner
dnagir added a note

Yeah, thanks. Totally agree. What I really need here is a shortcut for setting a very common expectation.

The reason I choose "should_authorize_for" is that it seems to be inline with RSpec's "should_receive".

But the problem here is that "should_authorize_for" doesn't have an explicit subject unlike "should_receive".

Maybe I should write something like "allowy.should_authorize_for", but then I would have to add a method to RSpec mock object which I'm not sure is a good idea.

So for now settled on "macro" style with "should_autorize_for" until something better will be suggested.
What do you think?

Actually, looking at the code below where should_authorize_for is defined, I wouldn't even wrap this. It's already a one liner, so you're saving very little code, and the things that differ from line to line still differ from line to line.

Dmytrii Nagirniak Owner
dnagir added a note

That's true. However in practice writing the expectation allowy.should_receive(:authorize!).with(:edit, page) is a bit harder to write and read than just should_authorize_for(:create, page) (or other similar alternative).

That's the only motive for wrapping it. I don't care exactly what method was called, what I care about is that thing was authorized.
I guess this is more of a philosophical reason than poorly technical if that makes sense to you.

I understand what you're saying, but I have a different philosophy. object.should_receive(:message).with(arg, arg) already means something to me. I don't have to think about it because it's just like any other expectation. So for me it's actually less readable to wrap it because I have to think about what that method does.

Dmytrii Nagirniak Owner
dnagir added a note

Ok. I see, that makes sense.
I still like the macro due to its simplicity and probably will keep it.
But will show how to do it with plan RSpec expectation: ac8d4f9.

Thanks a lot for the help, David.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ post(:create)
+ end
end
```
29 lib/allowy/rspec.rb
View
@@ -3,11 +3,28 @@
module Allowy
module ControllerAuthorizationMacros
- def ignore_authorization!
- before(:each) do
- registry = double 'Registry'
- registry.stub(:can? => true, :cannot? => false, :authorize! => nil, access_control_for!: registry)
- @controller.stub(:current_allowy).and_return registry
+ extend ActiveSupport::Concern
+
+ def allowy
+ @controller.current_allowy
+ end
+
+
+ def should_authorize_for(*args)
+ allowy.should_receive(:authorize!).with(*args)
+ end
+
+ def should_not_authorize_for(*args)
+ allowy.should_not_receive(:authorize!).with(*args)
+ end
+
+ module ClassMethods
+ def ignore_authorization!
+ before(:each) do
+ registry = double 'Registry'
+ registry.stub(:can? => true, :cannot? => false, :authorize! => nil, access_control_for!: registry)
+ @controller.stub(:current_allowy).and_return registry
+ end
end
end
end
@@ -15,6 +32,6 @@ def ignore_authorization!
end
RSpec.configure do |config|
- config.extend Allowy::ControllerAuthorizationMacros, :type => :controller
+ config.include Allowy::ControllerAuthorizationMacros, :type => :controller
end

1 comment on commit 6072180

Dmytrii Nagirniak
Owner

Ideally I'd like to write something like 'allowy.should authorize_for(:create, page)' but not sure how to write a proper matcher.

Dmytrii Nagirniak
Owner

The related spec is at dc1b3c4

David Chelimsky

Matchers are designed to pass/fail immediately, whereas message expectations are designed to pass/fail when they receive an unexpected message or at the end of the example if they never received the expected message. What you're looking for is not really a matcher, but a way to instrument the example with message expectations using matcher-like syntax. If that's what you really want, then just always return true from matches? and always return false from does_not_match? and set the expectations you want within those methods. You're basically breaking the should/matcher contract to do this, so please consider the confusion that will set in in 6 months when this matcher works differently from all other matchers.

Personally I'd probably just wrap the expectations in a normal method. Something like what you have, but named differently. Maybe expect_authorization_request(:create, page) or some such. WDYT?

Dmytrii Nagirniak
Owner

Yeah, thanks. Totally agree. What I really need here is a shortcut for setting a very common expectation.

The reason I choose "should_authorize_for" is that it seems to be inline with RSpec's "should_receive".

But the problem here is that "should_authorize_for" doesn't have an explicit subject unlike "should_receive".

Maybe I should write something like "allowy.should_authorize_for", but then I would have to add a method to RSpec mock object which I'm not sure is a good idea.

So for now settled on "macro" style with "should_autorize_for" until something better will be suggested.
What do you think?

David Chelimsky

Actually, looking at the code below where should_authorize_for is defined, I wouldn't even wrap this. It's already a one liner, so you're saving very little code, and the things that differ from line to line still differ from line to line.

Dmytrii Nagirniak
Owner

That's true. However in practice writing the expectation allowy.should_receive(:authorize!).with(:edit, page) is a bit harder to write and read than just should_authorize_for(:create, page) (or other similar alternative).

That's the only motive for wrapping it. I don't care exactly what method was called, what I care about is that thing was authorized.
I guess this is more of a philosophical reason than poorly technical if that makes sense to you.

David Chelimsky

I understand what you're saying, but I have a different philosophy. object.should_receive(:message).with(arg, arg) already means something to me. I don't have to think about it because it's just like any other expectation. So for me it's actually less readable to wrap it because I have to think about what that method does.

Dmytrii Nagirniak
Owner

Ok. I see, that makes sense.
I still like the macro due to its simplicity and probably will keep it.
But will show how to do it with plan RSpec expectation: ac8d4f9.

Thanks a lot for the help, David.

Please sign in to comment.
Something went wrong with that request. Please try again.