COMP-6957 Add roleScope methods and indexQueryBuilder #15
Conversation
lib/rolypoly/index_role_dsl.rb
Outdated
| module InstanceMethods | ||
| extend Forwardable | ||
|
|
||
| rescue_from NoMethodError, with: :check_rails |
There was a problem hiding this comment.
Hrmm, this seems a bit too greedy. I believe any no-method-error in the application would be caught by this...
lib/rolypoly/index_role_dsl.rb
Outdated
| role_scopes.allowed_roles(current_user_roles, scope_name) | ||
| end | ||
|
|
||
| protected def check_rails |
There was a problem hiding this comment.
I feel like this should be done on start in an initializer.
lib/rolypoly/index_role_dsl.rb
Outdated
| module IndexRoleDSL | ||
|
|
||
| def self.included(base) | ||
| base.before_filter(:check_where_or) if base.respond_to? :before_filter |
There was a problem hiding this comment.
This is still going to run before every request. I don't think that is necessary or desired. Add it to the documentation if we can't add a check without affecting too much overhead.
There was a problem hiding this comment.
This will only run before every index request. Is that still too much overhead?
There was a problem hiding this comment.
I still prefer it doesn't. Running for every index request isn't fool proof anyhow.
lib/rolypoly/index_role_dsl.rb
Outdated
| def apply_scopes | ||
| return query if role_scopes.all_access?(current_user_roles) | ||
| return query.none if scope_hash.empty? | ||
| return scope_hash.inject(query) { |query, (scope_name, ids)| query.or(query.public_send(scope_name, ids)) } |
There was a problem hiding this comment.
In regards to my comments about rails/rails#24055. The main issue is that query should be an array of possible objects to return, but for some objects, query is just the general class object rather than a specific instance(s). For example in venues_controller, query will be an array of possible venues. However in reservations_controller, query just returns Reservation(id: integer, reserver_type: string, reserver_id: string, reservable_type: string, reservable_id: integer, primary: boolean). It's something to do with object relations I think...
lib/rolypoly/index_role_dsl.rb
Outdated
| q.joins(join_table) | ||
| end | ||
|
|
||
| return scope_hash.inject(object_query) do |object_query, (scope_name, ids)| |
There was a problem hiding this comment.
It is more railsy to not use return statements on the last line of the block.
http://sportngin.github.io/styleguide/ruby.html#syntax_L18
|
|
Has QA approval
What
This PR upgrades the
RolyPolygem to include theRoleScopeobject and associated methods. Additionally, this PR adds theResourceIndexQueryBuilderto allow permission checks and roleScoping forindexactions.Why
Over the course of the past year, StatNgin and VenueService have been upgraded to use RolyPoly for permission checks. These POC's were successful, therefore it was decided to simply add the new service classes to the RolyPoly gem, allowing us to replicate permission checks across our platform.
Deploy Plan
Rollback Plan
git revert -m 1 MERGE_SHAand perform another deploy.URLs
QA Plan