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

users: change user role #1506

Merged
merged 44 commits into from
May 12, 2021
Merged

users: change user role #1506

merged 44 commits into from
May 12, 2021

Conversation

arurao
Copy link
Contributor

@arurao arurao commented May 6, 2021

  • Identified the issue which this PR solves.
  • Read the CONTRIBUTING document.
  • Code builds clean without any errors or warnings.
  • Added appropriate tests for any new functionality.
  • All new and existing tests passed.
  • Added comments in the code, where necessary.
  • Ran make check to catch common errors. Fixed any that came up.

Description:
This PR provides the ability for admins to change user roles.

Which issue(s) this PR fixes:
Part of #1247

Describe any introduced user-facing changes:
Admins will be able to see 'Edit' icon on the user details page.

Screen Shot 2021-05-11 at 4 54 34 PM

After that, there will be an option to check/uncheck the admin role option.

Screen Shot 2021-05-10 at 3 26 37 PM

Checking it will set the user as admin, unchecking will set the role to user.
If the logged-in user is already an admin, unchecking the checkbox will show a warning notice mentioning the change in role and it's effects.

Screen Shot 2021-05-10 at 3 27 38 PM

Describe any introduced API changes:
A split logic has been applied to the updateUser mutation.
Whenever the role field is specified, the logic will go to a different store method that updates the user role, else it will go to the store update a user logic.
This decision was made since even though there is currently no UI for updateUser fields like name, email, etc that might change in the future (and is possible today via the graphql playground) editing user fields like name, email, etc could be done by the user themselves. But in the case of changing user's role, this operation is strictly limited to admins only. No non-admin user should be allowed to make themselves as an admin. So, in order to keep the two different permission checks separate for updating user fields and changing user role, the logic is split up accordingly.

Copy link
Member

@mastercactapus mastercactapus left a comment

Choose a reason for hiding this comment

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

A few high-level things we could

Copy link
Contributor

@Forfold Forfold left a comment

Choose a reason for hiding this comment

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

Edit: Fixed in latest commit

I got this error when trying to update a user other than myself to admin, in the UI console:

Screen Shot 2021-05-06 at 1 42 10 PM

The network tab showed this response on the request:

{
  "errors": [
    {
      "message": "Cannot query field \"setUserRole\" on type \"Mutation\".",
      "locations": [{ "line": 2, "column": 3 }],
      "extensions": { "code": "GRAPHQL_VALIDATION_FAILED" }
    },
    {
      "message": "Unknown type \"SetUserRoleInput\".",
      "locations": [{ "line": 1, "column": 1 }],
      "extensions": { "code": "GRAPHQL_VALIDATION_FAILED" }
    }
  ],
  "data": null
}

@arurao arurao requested review from mastercactapus and Forfold May 10, 2021 21:05
Forfold
Forfold previously approved these changes May 10, 2021
Forfold
Forfold previously approved these changes May 11, 2021
Forfold
Forfold previously approved these changes May 11, 2021
@mastercactapus mastercactapus merged commit b7ba43d into master May 12, 2021
@mastercactapus mastercactapus deleted the edit-user-role branch May 12, 2021 14:44
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.

3 participants