-
Notifications
You must be signed in to change notification settings - Fork 634
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
Adds tests to cover policy overrides and when authorize is called in other namespaces #772
base: main
Are you sure you want to change the base?
Adds tests to cover policy overrides and when authorize is called in other namespaces #772
Conversation
- tests when called from a controller - tests when a record is called from another namespace
Hey @dgmstuart, could you check this out in the near future? I'd appreciate any feedback, thanks! 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the submission.
A number of these tests seem to be stubbing (eg. allow(controller).to receive(x)...
) and then testing the stub (expect(controller.x)....
).
It seems like these tests aren't actually exercising the application code. An important principle to follow is never stub the system under test.
Please remove those tests or change them so that they're actually exercising the Pundit code.
- refactors the authorization_spec and spec_helper
@dgmstuart Any updates on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi - so sorry for the absurd delay - I've been crunching on a project for a long stretch and haven't had any headspace for Pundit.
I'm afraid I don't understand what these specs are intended to cover: I need a little more explanation.
policy = controller.policy(post) | ||
expect(policy).to be_instance_of(PostPolicy) | ||
expect(policy.user).to eq(user) | ||
expect(policy.post).to eq(post) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This context
says "when the policy is overridden", but it doesn't look like anything is being overriden? What's the "policy" which is being overridden (do you mean the policy
method, or the Policy
class?), and where is it overridden?
I see there are some new things added in the spec_helper
but none of those override policy_class
, authorize
, policy
, Post
or PostPolicy
, so this spec looks like it just tests that ::PostPolicy
works - nothing to do with any overriding?
What am I missing?
If I understand it correctly, the "overriding" which the original issue was talking about defining policy
or authorize
in a specific controller - eg. in an admin controller you might want to do something like this:
def authorize(record, query = nil)
super([:admin, record], query)
end
def policy(record)
super([:admin, record])
end
...to make sure that calls to authorize(Post)
and policy(Post)
in that controller end using the Admin::PostPolicy
instead of ::PostPolicy
.
I'm actually using this exact override of authorize
in my current project.
This PR adds more tests as suggested in this comment from the issue #723
To do
PS: Thank you for contributing to Pundit ❤️