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

Use correct policy class in search controller #2827

Conversation

martinemde
Copy link
Contributor

@martinemde martinemde commented Jun 1, 2024

Description

The authorization_policy on the resource should be used when set in the search controller.

Fixes something similar to #2799 (related to #2805)

#2805 fixed most of the test failures for us, but this PR is still necessary to get them all passing.

Here's the test run that failed.
This is the commit without the fix applied that fails: rubygems/rubygems.org@4726858

Specifically, the search controller test failure is caused in test/integration/avo/gem_name_reservations_controller_test.rb:15

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

Screenshots & recording

Manual review steps

  1. This change was necessary to get rubygems.org tests working with a custom authorization policy.

Manual reviewer: please leave a comment with output from the test if that's the case.

The authorization_policy on the resource should be used when set.
Copy link

codeclimate bot commented Jun 1, 2024

Code Climate has analyzed commit 868fb02 and detected 0 issues on this pull request.

View more on Code Climate.

@martinemde
Copy link
Contributor Author

I'm concerned that all the places where authorize_action is called might need to have this added, which would imply a refactor is needed. I see that there aren't any tests that use a resource with a custom policy so I couldn't easy add something for this.

@adrianthedev
Copy link
Collaborator

Thank you @martinemde
We'll add some tests for this use-case in #2828

AFAIK you are still using Avo 2 on rubygems.org
This PR will be merged in main which will be released for Avo 3.x

We'll need to backport it to https://github.com/avo-hq/avo/tree/2.x

@martinemde
Copy link
Contributor Author

Thanks @adrianthedev! I know we've looked at the Avo 3 upgrade. I'm not sure what is blocking it at the moment. I'll check again when I have a moment.

@martinemde martinemde changed the title Use correct policy class is search controller Use correct policy class in search controller Jun 2, 2024
@adrianthedev adrianthedev merged commit 148ce51 into avo-hq:main Jun 4, 2024
22 checks passed
@martinemde martinemde deleted the martinemde/use-correct-policy-class-for-search branch June 4, 2024 23:46
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.

2 participants