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

User permission fixes #23331

Merged
merged 4 commits into from Jun 26, 2018
Merged

User permission fixes #23331

merged 4 commits into from Jun 26, 2018

Conversation

islemaster
Copy link
Contributor

@islemaster islemaster commented Jun 26, 2018

Some updates to the UserPermissionGrantee implementation suggested by @aoby while reviewing #23329.

  • revoke_all_permissions should clear the user's permissions cache.
  • Dynamically define user permission helper methods like levelbuilder?

user.permission = UserPermission::FACILITATOR
test 'revoke_all_permissions drops permission cache' do
user = create :facilitator
user.permission?(UserPermission::LEVELBUILDER)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the point of this call? We don't check the return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This first call to permission? populates the permission cache, so we can later check that it's cleared.

I lifted this approach from the caching test that I copied over:

test 'permission? caches all permissions' do
user = create :facilitator
user.permission?(UserPermission::LEVELBUILDER)
no_database
assert user.permission?(UserPermission::FACILITATOR)
refute user.permission?(UserPermission::LEVELBUILDER)
end

I didn't come up with a way to assert that the cache has been filled without the no_database call the previous test uses. Maybe, in turn, this needs a comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. I'd either add a comment, or instead override reload_permissions to also refresh that @permissions cache and call that.

@islemaster islemaster changed the base branch from user-permission-concern to staging June 26, 2018 18:15
@islemaster islemaster merged commit c9cf393 into staging Jun 26, 2018
@islemaster islemaster deleted the user-permission-fixes branch June 29, 2018 00:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants