Clarify the display of an admin user permissions #5658
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brianteeman
requested review from
crossan007,
DawoudIO and
MrClever
as code owners
MrClever
changed the title
MrClever
approved these changes
Feb 25, 2021
There was a problem hiding this comment.
Clarity is a good thing, and I'm happy to merge this.
I tweaked the wording of the PR though ;) I don't think the original behaviour was "incorrect" as an "Admin" has all permissions by default, so all permissions are implied to be "yes", which meant all other explicit permissions are ignored for admin users (hence why they display "no" because they don't get set). However, for people unfamiliar with the security model in use (which is different from the more common "role-based" model) the original behaviour could be a source of confusion.
|
Thank you |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What's this PR do?
The display of an admin user settings is confusing as it says you have no permissions. This PR changes that by changing the conditional check to include displaying Yes for each permission if the user is an admin. Users who are not admin are not impacted. This is purely a visual change so is completely backwards compatible
Screenshots (if appropriate)
What Issues does it Close?
Closes #5657
What are the relevant tickets?
Any background context you want to provide?
After user testing the users wondered why it said they had no permission to do anything and then went looking to see how to add the permission.
Where should the reviewer start?
How should this be manually tested?
How should the automated tests treat this?
Questions:
Is there a related website / article to substantiate / explain this change?
Does the development wiki need an update?
Does the user documentation wiki need an update?
Does this add new dependencies?
Does this need to add new data to the demo database