Skip to content

feat: add custom roles #14069

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

Merged
merged 28 commits into from
Aug 9, 2024
Merged

feat: add custom roles #14069

merged 28 commits into from
Aug 9, 2024

Conversation

jaaydenh
Copy link
Contributor

@jaaydenh jaaydenh commented Jul 31, 2024

Resolve #13911

  • Design custom roles table when empty
  • Setup custom table loader skeleton for roles table
  • Hide custom roles tab if experiment is not enabled
  • Display a message that the name field cannot be modified
  • When editing a custom, the name field should not be editable
  • Create permission check for resource assign_org_role with action create
  • Set the checked state for permissions checkboxes to the current resource and actions for a role
  • Edit existing custom role page
  • Create new custom role page
  • Retrieve organization roles and display in a table
  • EditRolesButton falls back to role name when no display name is present
  • Add cancel and save buttons to top of create/edit role form
  • Display a subset of resources, add a checkbox to display all resources (audit_log, group, template, organization_member, provisioner_daemon, workspace)
  • Storybook stories
Screenshot 2024-08-02 at 2 58 56 PM Screenshot 2024-08-02 at 3 00 03 PM Screenshot 2024-08-02 at 3 01 56 PM

@jaaydenh jaaydenh self-assigned this Jul 31, 2024
Copy link

alwaysmeticulous bot commented Jul 31, 2024

🤖 Meticulous replayed 100 user sessions and took 1258 visual snapshots. Meticulous has not yet run on 238e995 of the main branch and so there was nothing to compare against.
If you recently setup Meticulous, this is expected. Meticulous will start reporting comparisons for new pull requests after the next commit to the main branch.

Last updated for commit 72f1bab. This comment will update as new commits are pushed.

@jaaydenh jaaydenh changed the title feat: custom roles feat: add custom roles Aug 2, 2024
@jaaydenh jaaydenh requested a review from aslilac August 2, 2024 19:03
@jaaydenh jaaydenh marked this pull request as ready for review August 2, 2024 19:35
Copy link
Member

@aslilac aslilac left a comment

Choose a reason for hiding this comment

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

couple smalls things that could be cleaned up before merging, but overall looks great! 😄

Copy link
Member

@aslilac aslilac left a comment

Choose a reason for hiding this comment

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

my main concern is the assign_org_role check in AuthProvider. I think we should remove that and just use the check in the organizationPermissions query everywhere. I tagged Steven so he can give a second opinion, because I'm not super familiar with the permissions checks stuff.

other than that, just some minor things! looking good :)

: filteredRBACResourceActions;

return (
<>
Copy link
Member

Choose a reason for hiding this comment

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

unnecessary fragment?

Copy link
Member

Choose a reason for hiding this comment

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

marking as unresolved again because I still see this in the code :p

@BrunoQuaresma
Copy link
Collaborator

I will rely on @aslilac review for the FE code and focus on the design and QA.

  1. If a form does not have more than one section, it should be a simple vertical form. I also would add a subtitle to the header.

Form

  1. For the checkbox group, what do you think about having something like what we are using for notifications? Of course, replace it with the permissions primary and secondary text, and remove the icon on the right.

Form (1)

BrunoQuaresma

This comment was marked as resolved.

@aslilac

This comment was marked as resolved.

@jaaydenh
Copy link
Contributor Author

jaaydenh commented Aug 7, 2024

@BrunoQuaresma Regarding using switches for organization roles similar to notifications.

  1. FYI, I have been using the github example below as a reference. I feel like roles are conceptually different to notifications because roles are a set of things a users does or does not possess, whereas notifications feel like something that is turned on/off. I also did a quick survey of other tools and found that Azure, Github and many others use check boxes so my feeling is that it is a more common design pattern for managing custom roles.

  2. I am guessing that an API call is made every time a switch is changed for notifications. For roles, I imagined that the user would go through the flow of checking all the relevant roles and submitting the changes because it felt like creating a set of things as opposed to switching individual settings on/off. Although, I do think there could be value in making each change in the checkbox send the patch request instead of waiting to press the submit button if others agree.

github

@jaaydenh
Copy link
Contributor Author

jaaydenh commented Aug 7, 2024

@BrunoQuaresma regarding the form only having one section, I still need to add the delete role functionality and was thinking of doing it like organization settings. Unless, you have any thoughts about a better place to delete roles?

Screenshot 2024-08-07 at 3 35 58 PM

@BrunoQuaresma
Copy link
Collaborator

Related to the checkboxes: I think using GitHub as a reference is a good idea, but I missed a few things like the alignment of the descriptions and the "header checkbox" to check or uncheck the children options.

Related to the delete section: I don't think we need it on the create page right? Also, the width page for those settings pages is smaller because the sidebar does not give proper space to a horizontal form. Even using two sections, we can still use vertical forms. GH does this very well on their settings pages.

I will leave those decisions up to you.

@BrunoQuaresma
Copy link
Collaborator

While QAing it I recorded a video to share a few design things that could be improved.

Screen.Recording.2024-08-08.at.10.20.25.mp4

@jaaydenh jaaydenh requested a review from aslilac August 8, 2024 19:35
@jaaydenh
Copy link
Contributor Author

jaaydenh commented Aug 8, 2024

@aslilac I made all the changes planned for this PR based on the suggestions. Design improvements for checkboxes and delete custom role will be completed in a separate PR.

align="right"
sx={{ paddingTop: 0.4, paddingBottom: 0.4 }}
>
<FormControlLabel
Copy link
Member

Choose a reason for hiding this comment

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

I love the addition of this! could we add something similar to the bottom of the table too, just to make sure people can find it? a small extra row that just says "show all permissions" like this does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the show all permissions checkbox in a table footer row

: filteredRBACResourceActions;

return (
<>
Copy link
Member

Choose a reason for hiding this comment

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

marking as unresolved again because I still see this in the code :p

@jaaydenh jaaydenh merged commit 2e05329 into main Aug 9, 2024
30 checks passed
@jaaydenh jaaydenh deleted the custom-roles branch August 9, 2024 01:05
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add the ability to create custom roles in the UI
4 participants