Fixes namespace handling for policy_scope #391
Conversation
| @@ -88,7 +88,8 @@ def policy_scope(user, scope) | |||
| # @raise [NotDefinedError] if the policy scope cannot be found | |||
| # @return [Scope{#resolve}] instance of scope class which can resolve to a scope | |||
| def policy_scope!(user, scope) | |||
| PolicyFinder.new(scope).scope!.new(user, scope).resolve | |||
| model = scope.is_a?(Array) ? scope.last : scope | |||
There was a problem hiding this comment.
Note that this will break if the scope is a Rails 3 relation. See #402
There was a problem hiding this comment.
Interesting... It's been so long since I used Rails 3 I had not considered this.
There was a problem hiding this comment.
It's very unfortunate behavior. I think the only option might be to check #model_name, or drop support for Rails 3 (not my personal favorite choice, since I still maintain a Rails 3 application, but it would be understandable since even Rails doesn't support Rails 3 anymore).
|
What's the status on this? |
|
Not sure we have been using this in production at Doorman.co for months now without issue. I would vote to drop rails 3 support as rails 5 has been released and supporting 3 versions is just crazy. |
|
@ramaboo yes, please drop Rails 3 |
|
+1 |
|
+1 |
1 similar comment
|
👍 |
|
@ramaboo Can close & merge this PR with a readme update that explains the version w/ rails 3 support and that future versions will drop R3 support? |
|
+1 |
|
After reading the discussion #351 I found very interesting the @mysmallidea implementation and give the idea to apply the same fix to the scope class. class ApplicationPolicy
#index? show?....
class Scope
attr_reader :user, :scope
def initialize(user, scope)
@user = user
@scope = scope.is_a?(Array) ? scope.last : scope #This fix the problem
end
def resolve
scope
end
end
end
class Backoffice::EventPolicy < ApplicationPolicy
class Scope < Scope
def resolve
if user.is_admin?
scope.all
else
scope.not_published
end
end
end
end
class Backoffice::EventsController < ApplicationController
def index
authorize [:backoffice, Event]
@events = policy_scope([:backoffice, Event]).page params[:page]
end
end |
|
@medir, we've had to do something similar in order to make namespaces work. Had a |
|
Man.. this is really getting in my way - can we bring this thread back to life? |
|
@ramaboo A long overdue reply here :) So, this fixes the issue that the whole array is passed to the policy scope initializer by passing only the last item, right? So we need to make the same fix for the Policy class as well, as discussed in #351 I don't mean that you should add that fix in this PR, I just want to make clear that I understand what's going on here. I will wait with the merge here until I have a PR to fix the same issue for policies. As this is a breaking change it will have to go into 2.0. |
|
Is anyone going to merge this fix? |
|
Please read the comment I made before yours. |
|
@Linuus Ack sorry |
|
@ramaboo Could you rebase this on master and fix the conflicts? |
| @@ -88,7 +88,8 @@ def policy_scope(user, scope) | |||
| # @raise [NotDefinedError] if the policy scope cannot be found | |||
| # @return [Scope{#resolve}] instance of scope class which can resolve to a scope | |||
| def policy_scope!(user, scope) | |||
There was a problem hiding this comment.
The policy_scope method on line 78 needs to be updated as well.
|
Since I've not heard anything here, I added a PR which incorporates these changes and fixes the same issue for policy. See #529 |
As discussed here #351 this fixes the policy_scope finder to work with namespaces.