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

Adds tests to cover policy overrides and when authorize is called in other namespaces #772

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

TheDevJoao
Copy link

@TheDevJoao TheDevJoao commented May 23, 2023

This PR adds more tests as suggested in this comment from the issue #723

To do

  • I have read the contributing guidelines.
  • I have added relevant tests.
  • No documentation alterations required.
  • I have made sure the individual commits are meaningful.
  • I have added relevant lines to the CHANGELOG.

PS: Thank you for contributing to Pundit ❤️

- tests when called from a controller
- tests when a record is called from another namespace
@TheDevJoao TheDevJoao marked this pull request as ready for review May 23, 2023 20:55
@TheDevJoao
Copy link
Author

Hey @dgmstuart, could you check this out in the near future? I'd appreciate any feedback, thanks! 😄

Copy link
Collaborator

@dgmstuart dgmstuart left a 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.

@TheDevJoao TheDevJoao requested a review from dgmstuart May 26, 2023 20:58
@TheDevJoao
Copy link
Author

@dgmstuart Any updates on this PR?

Copy link
Collaborator

@dgmstuart dgmstuart left a 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)
Copy link
Collaborator

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.

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.

3 participants