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

Bulk-add users to existing org #887

Merged
merged 12 commits into from
Jul 23, 2024
Merged

Bulk-add users to existing org #887

merged 12 commits into from
Jul 23, 2024

Conversation

rmunn
Copy link
Contributor

@rmunn rmunn commented Jun 14, 2024

Fixes #778.

This adds a "Bulk Add Members" button to the organization page:

org-bulk-add-members-1

Currently the button is only visible to site admins. If we decide to allow org admins to use the Bulk-Add feature as well, it will be quite simple to make it visible to them; adding the correct auth requirements to the GraphQL mutation will be slightly harder but doable.

When you click the button, UI very reminiscent of the bulk-add feature from the project page will show up:

org-bulk-add-members-2

Unlike the project bulk-add feature, the bulk-add feature for orgs does NOT invite users if a username or email address is not found. Instead, it returns a "not found" list to the user and allows them to decide what to do about the not-found accounts:

org-bulk-add-members-3

Copy link

github-actions bot commented Jun 14, 2024

UI unit Tests

11 tests  ±0   11 ✅ ±0   0s ⏱️ ±0s
 3 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit e313001. ± Comparison against base commit 179fdec.

♻️ This comment has been updated with latest results.

@rmunn rmunn self-assigned this Jun 14, 2024
@rmunn
Copy link
Contributor Author

rmunn commented Jul 15, 2024

As per #778 (comment), we're not going to use the checkbox approach, and will instead be creating a bulk-add tool. So rather than fix the merge conflicts in commit 96929b1, I'm going to force-push this PR branch away from 96929b1 and abandon the checkbox idea, thereby making commit 96929b1 likely to be garbage-collected at some point. I don't think there's anything valuable there, though: most of the code is obvious and would be easy to recreate if we need to. I mean, the diff for adding the checkboxes is extremely simple (thanks to Svelte):

diff --git a/frontend/src/routes/(authenticated)/admin/UserTable.svelte b/frontend/src/routes/(authenticated)/admin/UserTable.svelte
index 6ee9adc4..5e48233a 100644
--- a/frontend/src/routes/(authenticated)/admin/UserTable.svelte
+++ b/frontend/src/routes/(authenticated)/admin/UserTable.svelte
@@ -7,17 +7,30 @@
   import type { User } from './+page';
 
   export let shownUsers: User[];
+  export let selectedUsers: User[] = [];
 
   const dispatch = createEventDispatcher<{
     openUserModal: User
     editUser: User
     filterProjectsByUser: User
   }>();
+
+  function toggleSelectAllUsers(e: Event): void {
+    if (((e.target!) as HTMLInputElement).checked) {
+      selectedUsers = [...shownUsers];
+    } else {
+      selectedUsers = [];
+    }
+  }
+
 </script>
 
 <table class="table table-lg">
   <thead>
     <tr class="bg-base-200">
+      <th class="max-w-4">
+        <input type="checkbox" on:change={toggleSelectAllUsers}/>
+      </th>
       <th>
         {$t('admin_dashboard.column_name')}
         <span class="i-mdi-sort-ascending text-xl align-[-5px] ml-2" />
@@ -32,6 +45,9 @@
   <tbody>
     {#each shownUsers as user}
       <tr>
+        <td class="max-w-4">
+          <input type="checkbox" bind:group={selectedUsers} value={user}/>
+        </td>
         <td>
           <div class="flex items-center gap-2 max-w-40 @xl:max-w-52">
             <Button variant="btn-ghost" size="btn-sm" class="max-w-full" on:click={() => dispatch('openUserModal', user)}>

Most of this code is identical to the bulk-add code in ProjectMutations,
but the org version explicitly does *not* invite members if an email
address is not found. Instead, any usernames or emails not found are
returned to the frontend so that the user can take appropriate action.
@rmunn rmunn force-pushed the feat/bulk-add-users-to-org branch from 96929b1 to 778d7d4 Compare July 15, 2024 10:01
Copy link

github-actions bot commented Jul 15, 2024

C# Unit Tests

52 tests   52 ✅  5s ⏱️
10 suites   0 💤
 1 files     0 ❌

Results for commit e313001.

♻️ This comment has been updated with latest results.

@rmunn rmunn marked this pull request as ready for review July 16, 2024 03:56
@rmunn rmunn requested a review from hahn-kev July 16, 2024 04:03
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

I realize I didn't make this clear anywhere, but the bulk add to org feature should be available to org admins. Sorry I didn't clarify that. There's a few things that need to be updated to accommodate that change.

rmunn added 5 commits July 17, 2024 16:37
Since the org page query asks for `orgById`, the GraphQL cache is
caching the type OrgById rather than the type Organization.

While we're at it, we also invalidate the OrgById cache for a few other
mutations like adding and removing members from an org. And we stop
asking for the mutation to return the list of org members, because the
org admins don't have permission to access the .members field of orgs in
normal queries, only in the orgById query. This stops the GraphQL
permissions error that was popping up when org admins tried to do these
operations.
Copy link
Collaborator

@hahn-kev hahn-kev left a comment

Choose a reason for hiding this comment

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

fixed some indenting that got missed in BulkAddOrgMembers.svelte.

Looks good to me.

@rmunn
Copy link
Contributor Author

rmunn commented Jul 23, 2024

Merging is currently blocked because "Code scanning has not received results from CodeQL". But the green checkmark is right there in the check results. It's unclear what part of GitHub's system is malfunctioning to not see the CodeQL results, but I'm not going to wait for it. PR has been approved and all checks are green, so I'm checking the "bypass branch protection" box and merging.

@rmunn rmunn merged commit 6d6e539 into develop Jul 23, 2024
14 checks passed
@rmunn rmunn deleted the feat/bulk-add-users-to-org branch July 23, 2024 01:48
@rmunn rmunn linked an issue Jul 23, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants