NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

fix: Role assign ui fixes#3521

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Emyrk merged 14 commits intomainfromstevenmasley/assign_roles_ui

Aug 16, 2022

Merged

fix: Role assign ui fixes#3521

Emyrk merged 14 commits intomainfromstevenmasley/assign_roles_ui

Aug 16, 2022

Conversation

Copy link

Member

Emyrk commentedAug 15, 2022•
edited
Loading

What this does

Roles you cannot assign are disabled

Fixes

User admin also needs the ability to create organization_members to create a new user. Creating a new user assigns them to the single org atm.

Emyrk added4 commits

August 15, 2022 21:07

fix: Role asign ui

74b52de

fixup! fix: Role asign ui

daf8a98

fix: Give user admin ability to make org members

ef93908

Make fmt

f49870e

Emyrk commented

Aug 15, 2022

View reviewed changes

coderd/rbac/builtin.go

Comment on lines +126 to +129

		ResourceRoleAssignment: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
		ResourceUser: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
		// Full perms to manage org members
		ResourceOrganizationMember: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},

Copy link

MemberAuthor

EmyrkAug 15, 2022

Choose a reason for hiding this comment

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

Needs org_member role to create new users
RoleAssignment to assign roles. They can only assign org_member and member roles right now.

Emyrk marked this pull request as ready for review

August 15, 2022 21:18

Emyrk requested a review froma team as acode owner

August 15, 2022 21:18

Emyrk added2 commits

August 15, 2022 16:27

Update unit test

42ff363

Update unit test of what roles can do

bf7464b

Emyrk requested review fromKira-Pilot andjohnstcn

August 15, 2022 21:40

Kira-Pilot reviewed

Aug 15, 2022

View reviewed changes

site/src/api/api.ts

		export const getSiteRoles = async (): Promise<Array<TypesGen.Role>> => {
		const response = await axios.get<Array<TypesGen.Role>>(`/api/v2/users/roles`)
		export const getSiteRoles = async (): Promise<Array<TypesGen.AssignableRoles>> => {
		const response = await axios.get<Array<TypesGen.AssignableRoles>>(`/api/v2/users/roles`)

Copy link

Member

Kira-PilotAug 15, 2022

Choose a reason for hiding this comment

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

Eventually, it would be nice differentiate the tworoles endpoints by label a bit more. In the network tab, all I see is "roles" and it's hard to filter or figure out what I'm looking at.

Copy link

MemberAuthor

EmyrkAug 15, 2022

Choose a reason for hiding this comment

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

Not a bad idea

Copy link

Member

Kira-Pilot commentedAug 15, 2022•
edited
Loading

Nice job! Do you mind adding two tests?

a new story inRoleSelect.stories.tsx that showcases the disabled state. You can make a new test entity for assignable roles insite/src/testHelpers/entities.ts.
a new test in a new file:site/src/components/RoleSelect/RoleSelect.test.tsx that ensures non-assignable roles are disabled. You can use your new test entity here, too. You should be able to test for disabled state by doing something like:
expect(stevensListItem).toHaveProperty('disabled', true)

LMK if you need help!

Copy link

MemberAuthor

Emyrk commentedAug 15, 2022

Nice job! Do you mind adding two tests?
a new story inRoleSelect.stories.tsx that showcases the disabled state. You can make a new test entity for assignable roles insite/src/testHelpers/entities.ts.
a new test in a new file:site/src/components/RoleSelect/RoleSelect.test.tsx that ensures non-assignable roles are disabled. You can use your new test entity here, too. You should be able to test for disabled state by doing something like:
expect(stevensListItem).toHaveProperty('disabled', true)
LMK if you need help!

Will get on this tomorrow 👍

johnstcn approved these changes

Aug 16, 2022

View reviewed changes

coderd/roles_test.go OutdatedShow resolvedHide resolved

coderd/rbac/builtin_test.go Outdated

		@@ -251,8 +251,8 @@ func TestRolePermissions(t *testing.T) {
		Actions: []rbac.Action{rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete},
		Resource: rbac.ResourceRoleAssignment,
		AuthorizeMap: map[bool][]authSubject{
		true: {admin},
		false: {orgAdmin, orgMemberMe, otherOrgAdmin, otherOrgMember, memberMe, templateAdmin, userAdmin},
		true: {admin, userAdmin},

Copy link

Member

johnstcnAug 16, 2022

Choose a reason for hiding this comment

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

I thought we got rid ofadmin?

Copy link

MemberAuthor

EmyrkAug 16, 2022

Choose a reason for hiding this comment

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

admin is just the name of theowner user. Forgot to rename the var 👍

Emyrk added2 commits

August 16, 2022 08:58

PR cleanup

88a6f6b

Add frontend tests

1c7bae3

Kira-Pilot reviewed

Aug 16, 2022

View reviewed changes

site/src/testHelpers/entities.ts

		export const MockSiteRoles = [MockUserAdminRole, MockAuditorRole]
		export const MockAssignableSiteRoles = [MockAssignableOwnerRole, MockAssignableUserAdminRole, MockAssignableAuditorRole]

		export function assignableRole(role: TypesGen.Role, assignable: boolean): TypesGen.AssignableRoles {

Copy link

Member

Kira-PilotAug 16, 2022

Choose a reason for hiding this comment

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

😍

Make fmt

bad6277

Kira-Pilot reviewed

Aug 16, 2022

View reviewed changes

site/src/components/RoleSelect/RoleSelect.test.tsx Outdated

		MockOwnerRole, MockTemplateAdminRole,
		MockUserAdminRole,
		render
		} from "../../testHelpers/renderHelpers"

Copy link

Member

Kira-PilotAug 16, 2022

Choose a reason for hiding this comment

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

Suggested change

	}from"../../testHelpers/renderHelpers"
	}from"testHelpers/renderHelpers"

We're trying to favor absolute paths when we're not in the same directory as whatever we're importing.

Kira-Pilot reviewed

Aug 16, 2022

View reviewed changes

site/src/components/RoleSelect/RoleSelect.stories.tsxShow resolvedHide resolved

Remove unused code

9994ce4

Kira-Pilot reviewed

Aug 16, 2022

View reviewed changes

site/src/components/RoleSelect/RoleSelect.test.tsx OutdatedShow resolvedHide resolved

Kira-Pilot approved these changes

Aug 16, 2022

View reviewed changes

Copy link

Member

Kira-Pilot left a comment

Choose a reason for hiding this comment

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

Looking great once CI passes. Thanks for adding those tests.

Emyrkand others added4 commits

August 16, 2022 09:55

Update site/src/components/RoleSelect/RoleSelect.test.tsx

e45aec6

Co-authored-by: Kira Pilot <kira@coder.com>

Test updates

04cfad9

Fix js test assertions

74dbb33

Attribute is string not boolean

7212d7e

Emyrk changed the title~~fix: Role asign ui~~fix: Role assign ui fixes

Aug 16, 2022

Emyrk merged commit4be61d9 intomain

Aug 16, 2022

Emyrk deleted the stevenmasley/assign_roles_ui branch

August 16, 2022 15:39

Labels

None yet

3 participants

Movatterモバイル変換

fix: Role assign ui fixes#3521

fix: Role assign ui fixes#3521

Uh oh!

Conversation

Emyrk commentedAug 15, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

What this does

Fixes

Uh oh!

EmyrkAug 15, 2022

Choose a reason for hiding this comment

Uh oh!

Kira-PilotAug 15, 2022

Choose a reason for hiding this comment

Uh oh!

EmyrkAug 15, 2022

Choose a reason for hiding this comment

Uh oh!

Kira-Pilot commentedAug 15, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Emyrk commentedAug 15, 2022

Uh oh!

Uh oh!

johnstcnAug 16, 2022

Choose a reason for hiding this comment

Uh oh!

EmyrkAug 16, 2022

Choose a reason for hiding this comment

Uh oh!

Kira-PilotAug 16, 2022

Choose a reason for hiding this comment

Uh oh!

Kira-PilotAug 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Kira-Pilot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Emyrk commentedAug 15, 2022•
edited
Loading

Kira-Pilot commentedAug 15, 2022•
edited
Loading