-
-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
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.
96929b1
to
778d7d4
Compare
C# Unit Tests52 tests 52 ✅ 5s ⏱️ Results for commit e313001. ♻️ This comment has been updated with latest results. |
There was a problem hiding this 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.
frontend/src/routes/(authenticated)/org/[org_id]/BulkAddOrgMembers.svelte
Outdated
Show resolved
Hide resolved
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.
There was a problem hiding this 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.
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. |
Fixes #778.
This adds a "Bulk Add Members" button to the organization page:
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:
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: