Navigation Menu

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

fix PolicyFinder when given an array with policy class override #475

Merged
merged 1 commit into from May 31, 2018

Conversation

ryanhertz
Copy link
Contributor

When trying to find a policy using an array, the policy_class override method was not getting called on the last item of the array.

Copy link

@damien damien left a comment

Choose a reason for hiding this comment

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

Nice work! These changes seem to be in line with how Pundit has historically operated (e.g. Rails-like form-for arrays to specify namespaces).

@ryanhertz
Copy link
Contributor Author

@damien @jnicklas Can this be merged? It's my first real OS pr.

@mikaellisselgard
Copy link

What the status on this one? Policies are currently broken when trying to use the namespace functionality used in #331. Anything I can do?

@Linuus Linuus added the bug label May 11, 2018
@Linuus Linuus added this to the v2.0 milestone May 16, 2018
@armchairdj
Copy link

Please merge this! I have now forked Pundit twice, once last year and once just now after pulling upstream changes from the last few months, to fix this same issue. And my fix was not nearly as elegant! Kudos to @ryanhertz for tackling it!

@Linuus
Copy link
Collaborator

Linuus commented May 31, 2018

This looks good to me. Thanks @ryanhertz !

@Linuus Linuus merged commit a0124c5 into varvet:master May 31, 2018
@halo
Copy link

halo commented Jun 7, 2018

Just to mention it for posterity. I use Draper and occasionally try to authorize a Draper::CollectionDecorator like so:

def index
  @articles = authorize scope.decorate
end

For whatever reason, the Draper::CollectionDecorator masks as an Array. Since this change, Pundit tries to find a Policy with the name "Article::Article::Article::ArticlePolicy", depending on the number of records in the Array 😆

I know, I shouldn't be using Draper at all, but I thought I'd just mention the conflict here.

May I also ask how this pull request is related to #351 ? Do they interfere with each other? Does this one fix the other one? Or are they not related? I'm just trying to make sense out of the code.

Thank you for your hard work!

@Linuus
Copy link
Collaborator

Linuus commented Jun 7, 2018

@halo #351 is not really related to this one. The problem here is that if you override the policy_class method on the object being authorized it wouldn't call it if used with the namespacing functionality, e.g. authorize [:admin, @article].

We still always pass the entire array to the policy in the end. I'm not sure if that will change yet.

dgmstuart added a commit that referenced this pull request Aug 23, 2019
The specs here were "Stubbing the system under test", which is generally
considered to not be good practice, since it can result in 'Imaginary'
specs which don't exercise possible behaviours:

  https://thoughtbot.com/blog/don-t-stub-the-system-under-test

In this case it's not actually possible for the private `#find` method
to return nil.

This PR removes that stubbing and completes a set of test cases which
execute all the paths.

The case where we get a nil object should in normal cases result in an
error, but for some reason in this test suite we have a NilClassPolicy.
This spec was kept in order to be consistent with the corresponding
scope spec.

Other fixes:

- In the test data it seems like `policy_class` and `model_name` should
  only be defined on classes, not instances
- Added test cases to cover #475
dgmstuart added a commit that referenced this pull request Nov 12, 2019
The specs here were "Stubbing the system under test", which is generally
considered to not be good practice, since it can result in 'Imaginary'
specs which don't exercise possible behaviours:

  https://thoughtbot.com/blog/don-t-stub-the-system-under-test

In this case it's not actually possible for the private `#find` method
to return nil.

This PR removes that stubbing and completes a set of test cases which
execute all the paths.

The case where we get a nil object should in normal cases result in an
error, but for some reason in this test suite we have a NilClassPolicy.
This spec was kept in order to be consistent with the corresponding
scope spec.

Other fixes:

- In the test data it seems like `policy_class` and `model_name` should
  only be defined on classes, not instances
- Added test cases to cover #475
dgmstuart added a commit that referenced this pull request Nov 12, 2019
The specs here were "Stubbing the system under test", which is generally
considered to not be good practice, since it can result in 'Imaginary'
specs which don't exercise possible behaviours:

  https://thoughtbot.com/blog/don-t-stub-the-system-under-test

In this case it's not actually possible for the private `#find` method
to return nil.

This PR removes that stubbing and completes a set of test cases which
execute all the paths.

The case where we get a nil object should in normal cases result in an
error, but for some reason in this test suite we have a NilClassPolicy.
This spec was kept in order to be consistent with the corresponding
scope spec.

Other fixes:

- In the test data it seems like `policy_class` and `model_name` should
  only be defined on classes, not instances
- Added test cases to cover #475
dgmstuart added a commit that referenced this pull request Nov 12, 2019
The specs here were "Stubbing the system under test", which is generally
considered to not be good practice, since it can result in 'Imaginary'
specs which don't exercise possible behaviours:

  https://thoughtbot.com/blog/don-t-stub-the-system-under-test

In this case it's not actually possible for the private `#find` method
to return nil.

This PR removes that stubbing and completes a set of test cases which
execute all the paths.

The case where we get a nil object should in normal cases result in an
error, but for some reason in this test suite we have a NilClassPolicy.
This spec was kept in order to be consistent with the corresponding
scope spec.

Other fixes:

- In the test data it seems like `policy_class` and `model_name` should
  only be defined on classes, not instances
- Added test cases to cover #475
HappyDevman added a commit to HappyDevman/pundit that referenced this pull request May 5, 2021
The specs here were "Stubbing the system under test", which is generally
considered to not be good practice, since it can result in 'Imaginary'
specs which don't exercise possible behaviours:

  https://thoughtbot.com/blog/don-t-stub-the-system-under-test

In this case it's not actually possible for the private `#find` method
to return nil.

This PR removes that stubbing and completes a set of test cases which
execute all the paths.

The case where we get a nil object should in normal cases result in an
error, but for some reason in this test suite we have a NilClassPolicy.
This spec was kept in order to be consistent with the corresponding
scope spec.

Other fixes:

- In the test data it seems like `policy_class` and `model_name` should
  only be defined on classes, not instances
- Added test cases to cover varvet/pundit#475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants