-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
Force scope on default filters collection #7511
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
base: master
Are you sure you want to change the base?
Conversation
ea831c1
to
c3f8cb2
Compare
The tests are failing. Likely need some updates since this was last looked at. |
82e1817
to
26e87ed
Compare
@javierjulio This is now a breaking change. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7511 +/- ##
=======================================
Coverage 99.11% 99.11%
=======================================
Files 141 141
Lines 4069 4087 +18
=======================================
+ Hits 4033 4051 +18
Misses 36 36 ☔ View full report in Codecov by Sentry. |
a04b6ba
to
e8be50f
Compare
this should work even if default policy is not defined or does not define a default scope
e8be50f
to
0ffac23
Compare
@javierjulio took me some time to build a proper spec. Now is ready for review. |
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.
What is the benefit of adding this? If I understand right it only affects default filters, so if you set filters explicitly which is best, this would not apply. Or I guess it would since there is an update to add_filter
method. I'm not convinced on this change.
lib/active_admin/error.rb
Outdated
@@ -19,6 +19,16 @@ def message | |||
end | |||
end | |||
|
|||
class ScopeNotDefined < StandardError |
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.
I'd like to include "Policy" in the error class name so it's clear since when I think "scopes" it's something else. Perhaps "PolicyScopeNotDefined" as the class name?
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.
I understand your concern with the name. I'm open to use another one. I just want to avoid Policy
because it's a Pundit
strong word. I want to model something more abstract, the ActiveRecord::RecordNotFoundError
for scope_collection
method. I don't want to tailor the error for a specific AuthorizationAdapter implementation.
I renamed the class to ScopeAuthorizationError
. Is it better?
@javierjulio The change affects filters and default filters. I added more test scenarios to demonstrate the usage. I understand the previous commit was misleading. |
In-repo clone of #6923.
Fixes #6883.