Organization member-management #1414

simensandhaug · 2023-01-30T17:33:09Z

Changes

Implements changes from PR(change permission groups hr_group, primary_group -> admin_group, member_group)
Add mutation to delete membership
Add mutation to add member by username
Ability to change membership group from member -> administrator and vice versa

Issues

No backend tests
Have to refresh page after delete and assign membership to see changes. Fix by updating local cache when mutating? Not sure how to do this.
Button alignment

…models

…h explicit names for groups

…escriptive

vercel · 2023-01-30T17:33:13Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
indok-web	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 28, 2023 4:50pm

…e-members-in-an-organization-on-the-organizations-member-page

codecov · 2023-01-30T17:39:57Z

Codecov Report

Merging #1414 (3af4c03) into main (5bfd2a4) will decrease coverage by 0.32%.
Report is 6 commits behind head on main.
The diff coverage is 68.57%.

@@            Coverage Diff             @@
##             main    #1414      +/-   ##
==========================================
- Coverage   83.11%   82.79%   -0.32%     
==========================================
  Files         100      100              
  Lines        3524     3569      +45     
==========================================
+ Hits         2929     2955      +26     
- Misses        595      614      +19

Flag	Coverage Δ
apitests	`82.79% <68.57%> (-0.32%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
backend/apps/forms/signals.py	`100.00% <100.00%> (ø)`
backend/apps/organizations/models.py	`87.50% <100.00%> (+2.50%)`	⬆️
backend/apps/organizations/schema.py	`100.00% <100.00%> (ø)`
backend/apps/organizations/types.py	`73.77% <100.00%> (ø)`
backend/apps/permissions/constants.py	`100.00% <100.00%> (ø)`
backend/apps/permissions/signals.py	`97.50% <100.00%> (-0.07%)`	⬇️
backend/apps/organizations/signals.py	`80.00% <80.00%> (+0.58%)`	⬆️
backend/apps/organizations/mutations.py	`60.65% <57.14%> (-4.35%)`	⬇️

simensandhaug · 2023-02-02T17:05:50Z

Current problem: Clientside doesnt get updated when changes are made. (i.e when user is deleted or added). Not sure how to fix this

Merge branch 'main' into 1360-admins-can-add-delete-promote-and-demote-members-in-an-organization-on-the-organizations-member-page

sonarcloud · 2023-02-13T17:36:38Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

…e-members-in-an-organization-on-the-organizations-member-page

sonarcloud · 2023-09-28T16:49:19Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

No Coverage information
19.0% Duplication

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

cypress · 2023-09-28T16:58:46Z

Passing run #2913 ↗︎

0	2	0	0	0

Details:

Merge `3af4c03` into `5bfd2a4`...
Project: Indøk Web	Commit: `472f7ec1e3 ℹ️`
Status: Passed	Duration: 01:45 💡
Started: Sep 28, 2023 4:57 PM	Ended: Sep 28, 2023 4:58 PM

Review all test suite changes for PR #1414 ↗︎

larwaa

These are fairly old comments that I've forgotten to finish up, but hopefully there's some value here. Don't hesitate to ask if there are any questions 😊

larwaa · 2023-11-10T07:43:51Z

backend/apps/organizations/mutations.py

+    class Arguments:
+        membership_id = graphene.ID()
+
+    @permission_required("organizations.manage_organization")


Unfortunately, some poor decision making on my part to add a lot of complexity with permissions comes back to bite us 🦷 This permission check must include the ID of the organization the membership belongs to. Otherwise, you can delete memberships from other organizations than your own.

Look to get_organization_from_data for inspiration.

larwaa · 2023-11-10T07:44:28Z

backend/apps/organizations/mutations.py

+    class Arguments:
+        membership_data = ChangeMembershipInput(required=True)
+
+    @permission_required("organizations.manage_organization")


Same here, the permission check must include the organization ID

larwaa · 2023-11-10T07:46:44Z

backend/apps/permissions/migrations/0034_auto_20221114_1854.py

+            # Change ResponsibleGroup.group_type from "HR" or "PRIMARY" to "ADMIN" or "MEMBER"
+            responsible_group.name = f"{responsible_group.organization.name}:{ADMIN_GROUP_TYPE}"
+            responsible_group.group.name = (
+                f"{responsible_group.organization.name}:{ADMIN_GROUP_TYPE}:git{responsible_group.group.uuid}"


Is the git part intentional?

Suggested change

f"{responsible_group.organization.name}:{ADMIN_GROUP_TYPE}:git{responsible_group.group.uuid}"

f"{responsible_group.organization.name}:{ADMIN_GROUP_TYPE}:{responsible_group.group.uuid}"

larwaa · 2023-11-10T07:47:21Z

backend/apps/permissions/migrations/0034_auto_20221114_1854.py

This is great, looks solid

larwaa · 2023-11-10T07:49:37Z

frontend/src/components/pages/organization/OrgMembers.tsx

+  const [AssignMembershipWithUsername] = useMutation(AssignMembershipWithUsernameDocument);
+  const [DeleteMembership] = useMutation(DeleteMembershipDocument);
+  const [ChangeMembership] = useMutation(ChangeMembershipDocument);


Nitpicking

Suggested change

const [AssignMembershipWithUsername] = useMutation(AssignMembershipWithUsernameDocument);

const [DeleteMembership] = useMutation(DeleteMembershipDocument);

const [ChangeMembership] = useMutation(ChangeMembershipDocument);

const [assignMembershipWithUsername] = useMutation(AssignMembershipWithUsernameDocument);

const [deleteMembership] = useMutation(DeleteMembershipDocument);

const [changeMembership] = useMutation(ChangeMembershipDocument);

larwaa · 2023-11-10T08:10:05Z

backend/apps/organizations/mutations.py

+    group_id = graphene.ID()
+
+
+class AssignMembershipWithUsername(graphene.Mutation):


I'm going to do a longer write up here with a general suggestion, so be warned: wall of text incoming.

Early on in the life span of this project, I got carried away with permissions, thinking it would be cool to have a super-flexible permission structure that could handle as many use cases as I could imagine. This resulted in us adding the complex permission structure of the project today, which are way too complex. In reality, we just need, at most, a way to tell if a user is and ADMIN or a MEMBER in an organization.

As a result, my suggestion here is to try to hide some of the complexity of our permission structure, to prevent it from leaking out through the API, and make it easier to work with in the future. It doesn't require a huge structural change, we can just change this API slightly.

Rather than forcing the users of the API (us) to have to figure out which group_id a user should be added to, we can hide that complexity behind a choice with two options: A user should either be added as an ADMIN or as a MEMBER.

Specifically, something like this:

class MembershipType(graphene.Enum): MEMBER = "MEMBER" ADMIN = "ADMIN" class AssignMembershipWithUsernameInput(graphene.InputObjectType): username = graphene.String(required=True) organization_id = graphene.ID(required=True) membership_type = MembershipType # Not 100% on the syntax here, this is my best guess. class AssignMembershipWithUsername(graphene.Mutation): membership = graphene.Field(MembershipType) # If you want to avoid having to deal with cache mutations frontend, you can also add this organization = graphene.Field(OrganizationType) @permission_required("organizations.manage_organization", fn=get_organization_from_data) def mutate(self, _, membership_data): # Implement it similar to today, but instead of getting `group_id` directly, do something along the lines of if membership_data["membership_type"] == MembershipType.ADMIN: group = organization.admin_group elif membership_data["membership_type"] == MembershipType.MEMBER: group = organization.member_group # followed by more or less the same logic as before

The overall goal here is to reduce the complexity for the end-user of the API, placing more of the responsibility on the backend to handle the underlying (and in this case, unnecessary 😢 ) complexity.

larwaa · 2023-11-10T08:19:48Z

frontend/src/components/pages/organization/OrgMembers.tsx

+    }
+  };
+
+  const handleRemoveMembership = (membership: MembershipType | any) => {


Don't use any here, define it properly. If it's possibly undefined, use | undefined instead.

Suggested change

const handleRemoveMembership = (membership: MembershipType | any) => {

const handleRemoveMembership = (membership: MembershipType) => {

larwaa · 2023-11-10T08:20:02Z

frontend/src/components/pages/organization/OrgMembers.tsx

+    setUserInput("");
+  };
+
+  const handleGroupChange = (membership: MembershipType | any) => {


Same here, don't use any

Suggested change

const handleGroupChange = (membership: MembershipType | any) => {

const handleGroupChange = (membership: MembershipType) => {

larwaa · 2023-11-10T08:21:08Z

frontend/src/components/pages/organization/OrgMembers.tsx

+                      startIcon={<AdminPanelSettings />}
+                      sx={{ mr: 1 }}
+                    >
+                      {membership?.group?.uuid == organization.adminGroup?.uuid ? "Demoter" : "Promoter"}


Promoter/demoter aren't very obvious, how about or something along those lines?

Suggested change

{membership?.group?.uuid == organization.adminGroup?.uuid ? "Demoter" : "Promoter"}

{membership?.group?.uuid == organization.adminGroup?.uuid ? "Fjern som admin" : "Gjør til admin"}

larwaa · 2023-11-10T08:26:25Z

backend/apps/organizations/signals.py

        )
-        assign_perm("forms.add_form", hr_group.group)
+        print(admin_group.group, instance)


Suggested change

print(admin_group.group, instance)

simensandhaug added 22 commits November 7, 2022 18:12

change hr_group and primary_group to admin_group and member_group in …

c6cf298

…models

update group constants

d800ba4

change instances of hr_group and primary_group

5faef25

fix query error

1d6e42b

write custom migration

aa5c147

revert changes to previous migrations

ad93482

revert changes to previous migrations

22a5b20

revert changes to previous migrations

1eef979

move custom migrations to more appropriate folder

de5d3d7

merge migrations due to conflicts

3733a79

remove manage_organization permission from organizations/signals.py

ccd26d8

uncomment manage_organization permission in organizations/models.py

bb000da

remove autogenerated yarn.lock file

008dfdb

quality of life improvement for admin-panel, more easily readable wit…

188b95f

…h explicit names for groups

Admin-panel improvements and update signal and migration to be more d…

2cde7d2

…escriptive

update migration

30d6ad4

fix naming error

fe30038

fix typing error

6556ecf

commit before merge

1dcf3d8

merge with feat/migrate-org-groups

211c9fe

start on mutations

aacf92b

add removeMembership mutation

1415d84

simensandhaug linked an issue Jan 30, 2023 that may be closed by this pull request

Admins can Add, Delete, Promote and Demote members in an organization on the organizations member-page #1360

Open

Merge branch 'main' into 1360-admins-can-add-delete-promote-and-demot…

dfc97e5

…e-members-in-an-organization-on-the-organizations-member-page

vercel bot had a problem deploying to Preview January 30, 2023 17:35 Failure

fix unused desctructured variables

6238bd4

vercel bot deployed to Preview January 30, 2023 17:45 View deployment

fix graphql schema

2b43178

vercel bot deployed to Preview January 30, 2023 18:13 View deployment

Change role and delete membership fixed

ca93d1a

vercel bot deployed to Preview January 30, 2023 19:12 View deployment

add membership_with_username mutation

1e2b663

vercel bot deployed to Preview February 2, 2023 15:53 View deployment

add comments

8a16781

simensandhaug requested a review from larwaa February 2, 2023 17:06

vercel bot deployed to Preview February 2, 2023 17:07 View deployment

simensandhaug changed the title ~~1360 admins can add delete promote and demote members in an organization on the organizations member page~~ Organization member-management Feb 6, 2023

fix black linting

2f06296

Merge branch 'main' into 1360-admins-can-add-delete-promote-and-demote-members-in-an-organization-on-the-organizations-member-page

vercel bot deployed to Preview February 13, 2023 17:37 View deployment

Merge branch 'main' into 1360-admins-can-add-delete-promote-and-demot…

3af4c03

…e-members-in-an-organization-on-the-organizations-member-page

vercel bot deployed to Preview September 28, 2023 16:50 View deployment

MagnusHafstad requested review from larwaa and removed request for larwaa November 9, 2023 20:24

larwaa reviewed Jan 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organization member-management #1414

Organization member-management #1414

simensandhaug commented Jan 30, 2023 •

edited

Loading

vercel bot commented Jan 30, 2023 •

edited

Loading

codecov bot commented Jan 30, 2023 •

edited

Loading

simensandhaug commented Feb 2, 2023

sonarcloud bot commented Feb 13, 2023

sonarcloud bot commented Sep 28, 2023

cypress bot commented Sep 28, 2023

larwaa left a comment

larwaa Nov 10, 2023

larwaa Nov 10, 2023

larwaa Nov 10, 2023

larwaa Nov 10, 2023

larwaa Nov 10, 2023

larwaa Nov 10, 2023

larwaa Nov 10, 2023

larwaa Nov 10, 2023

larwaa Nov 10, 2023

larwaa Nov 10, 2023

	f"{responsible_group.organization.name}:{ADMIN_GROUP_TYPE}:git{responsible_group.group.uuid}"
	f"{responsible_group.organization.name}:{ADMIN_GROUP_TYPE}:{responsible_group.group.uuid}"

		group_id = graphene.ID()


		class AssignMembershipWithUsername(graphene.Mutation):

	const handleRemoveMembership = (membership: MembershipType \| any) => {
	const handleRemoveMembership = (membership: MembershipType) => {

	const handleGroupChange = (membership: MembershipType \| any) => {
	const handleGroupChange = (membership: MembershipType) => {

	{membership?.group?.uuid == organization.adminGroup?.uuid ? "Demoter" : "Promoter"}
	{membership?.group?.uuid == organization.adminGroup?.uuid ? "Fjern som admin" : "Gjør til admin"}

Organization member-management #1414

Are you sure you want to change the base?

Organization member-management #1414

Conversation

simensandhaug commented Jan 30, 2023 • edited Loading

Changes

Issues

vercel bot commented Jan 30, 2023 • edited Loading

codecov bot commented Jan 30, 2023 • edited Loading

Codecov Report

simensandhaug commented Feb 2, 2023

sonarcloud bot commented Feb 13, 2023

sonarcloud bot commented Sep 28, 2023

cypress bot commented Sep 28, 2023

Passing run #2913 ↗︎

Review all test suite changes for PR #1414 ↗︎

larwaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simensandhaug commented Jan 30, 2023 •

edited

Loading

vercel bot commented Jan 30, 2023 •

edited

Loading

codecov bot commented Jan 30, 2023 •

edited

Loading