From 8f66d60a7ef8fe47986a6bc3fe7a89a92b61cc52 Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Mon, 1 Jul 2024 06:50:42 +1000 Subject: [PATCH] [AC-2791] Members page - finish component library refactors (#9727) * Replace PlatformUtilsService with ToastService * Remove unneeded templates * Implement table filtering function * Move member-only methods from base class to subclass * Move utility functions inside new MemberTableDataSource * Rename PeopleComponent to MembersComponent --- .../common/new-base.people.component.ts | 277 +++--------------- .../common/people-table-data-source.ts | 133 +++++++++ .../members/members-routing.module.ts | 4 +- ....component.html => members.component.html} | 29 +- ...ople.component.ts => members.component.ts} | 136 +++++---- .../organizations/members/members.module.ts | 4 +- 6 files changed, 282 insertions(+), 301 deletions(-) create mode 100644 apps/web/src/app/admin-console/common/people-table-data-source.ts rename apps/web/src/app/admin-console/organizations/members/{people.component.html => members.component.html} (93%) rename apps/web/src/app/admin-console/organizations/members/{people.component.ts => members.component.ts} (88%) diff --git a/apps/web/src/app/admin-console/common/new-base.people.component.ts b/apps/web/src/app/admin-console/common/new-base.people.component.ts index 17f504c74aa..90c25e840c0 100644 --- a/apps/web/src/app/admin-console/common/new-base.people.component.ts +++ b/apps/web/src/app/admin-console/common/new-base.people.component.ts @@ -1,7 +1,7 @@ -import { Directive, ViewChild, ViewContainerRef } from "@angular/core"; +import { Directive } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { FormControl } from "@angular/forms"; -import { firstValueFrom, lastValueFrom, debounceTime } from "rxjs"; +import { firstValueFrom, lastValueFrom, debounceTime, combineLatest, BehaviorSubject } from "rxjs"; import { UserNamePipe } from "@bitwarden/angular/pipes/user-name.pipe"; import { ModalService } from "@bitwarden/angular/services/modal.service"; @@ -18,88 +18,47 @@ import { ListResponse } from "@bitwarden/common/models/response/list.response"; import { CryptoService } from "@bitwarden/common/platform/abstractions/crypto.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; -import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { ValidationService } from "@bitwarden/common/platform/abstractions/validation.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; -import { DialogService, TableDataSource } from "@bitwarden/components"; +import { DialogService, ToastService } from "@bitwarden/components"; import { OrganizationUserView } from "../organizations/core/views/organization-user.view"; import { UserConfirmComponent } from "../organizations/manage/user-confirm.component"; -type StatusType = OrganizationUserStatusType | ProviderUserStatusType; +import { PeopleTableDataSource, peopleFilter } from "./people-table-data-source"; -const MaxCheckedCount = 500; +export type StatusType = OrganizationUserStatusType | ProviderUserStatusType; +export type UserViewTypes = ProviderUserUserDetailsResponse | OrganizationUserView; /** * A refactored copy of BasePeopleComponent, using the component library table and other modern features. * This will replace BasePeopleComponent once all subclasses have been changed over to use this class. */ @Directive() -export abstract class NewBasePeopleComponent< - UserView extends ProviderUserUserDetailsResponse | OrganizationUserView, -> { - @ViewChild("confirmTemplate", { read: ViewContainerRef, static: true }) - confirmModalRef: ViewContainerRef; - - get allCount() { - return this.activeUsers != null ? this.activeUsers.length : 0; - } - - get invitedCount() { - return this.statusMap.has(this.userStatusType.Invited) - ? this.statusMap.get(this.userStatusType.Invited).length - : 0; - } - - get acceptedCount() { - return this.statusMap.has(this.userStatusType.Accepted) - ? this.statusMap.get(this.userStatusType.Accepted).length - : 0; - } - - get confirmedCount() { - return this.statusMap.has(this.userStatusType.Confirmed) - ? this.statusMap.get(this.userStatusType.Confirmed).length - : 0; - } - - get revokedCount() { - return this.statusMap.has(this.userStatusType.Revoked) - ? this.statusMap.get(this.userStatusType.Revoked).length - : 0; - } - +export abstract class NewBasePeopleComponent { /** * Shows a banner alerting the admin that users need to be confirmed. */ get showConfirmUsers(): boolean { return ( - this.activeUsers != null && - this.statusMap != null && - this.activeUsers.length > 1 && - this.confirmedCount > 0 && - this.confirmedCount < 3 && - this.acceptedCount > 0 + this.dataSource.activeUserCount > 1 && + this.dataSource.confirmedUserCount > 0 && + this.dataSource.confirmedUserCount < 3 && + this.dataSource.acceptedUserCount > 0 ); } get showBulkConfirmUsers(): boolean { - return this.acceptedCount > 0; + return this.dataSource.acceptedUserCount > 0; } abstract userType: typeof OrganizationUserType | typeof ProviderUserType; abstract userStatusType: typeof OrganizationUserStatusType | typeof ProviderUserStatusType; - protected dataSource = new TableDataSource(); + protected abstract dataSource: PeopleTableDataSource; firstLoaded: boolean; - /** - * A hashmap that groups users by their status (invited/accepted/etc). This is used by the toggles to show - * user counts and filter data by user status. - */ - statusMap = new Map(); - /** * The currently selected status filter, or null to show all active users. */ @@ -110,22 +69,12 @@ export abstract class NewBasePeopleComponent< */ actionPromise: Promise; - /** - * All users, loaded from the server, before any filtering has been applied. - */ - protected allUsers: UserView[] = []; - - /** - * Active users only, that is, users that are not in the revoked status. - */ - protected activeUsers: UserView[] = []; - protected searchControl = new FormControl("", { nonNullable: true }); + protected statusToggle = new BehaviorSubject(null); constructor( protected apiService: ApiService, protected i18nService: I18nService, - protected platformUtilsService: PlatformUtilsService, protected cryptoService: CryptoService, protected validationService: ValidationService, protected modalService: ModalService, @@ -133,18 +82,19 @@ export abstract class NewBasePeopleComponent< protected userNamePipe: UserNamePipe, protected dialogService: DialogService, protected organizationManagementPreferencesService: OrganizationManagementPreferencesService, + protected toastService: ToastService, ) { - // Connect the search input to the table dataSource filter input - this.searchControl.valueChanges - .pipe(debounceTime(200), takeUntilDestroyed()) - .subscribe((v) => (this.dataSource.filter = v)); + // Connect the search input and status toggles to the table dataSource filter + combineLatest([this.searchControl.valueChanges.pipe(debounceTime(200)), this.statusToggle]) + .pipe(takeUntilDestroyed()) + .subscribe( + ([searchText, status]) => (this.dataSource.filter = peopleFilter(searchText, status)), + ); } abstract edit(user: UserView): void; abstract getUsers(): Promise | UserView[]>; abstract deleteUser(id: string): Promise; - abstract revokeUser(id: string): Promise; - abstract restoreUser(id: string): Promise; abstract reinviteUser(id: string): Promise; abstract confirmUser(user: UserView, publicKey: Uint8Array): Promise; @@ -152,70 +102,16 @@ export abstract class NewBasePeopleComponent< // Load new users from the server const response = await this.getUsers(); - // Reset and repopulate the statusMap - this.statusMap.clear(); - this.activeUsers = []; - for (const status of Utils.iterateEnum(this.userStatusType)) { - this.statusMap.set(status, []); - } - + // GetUsers can return a ListResponse or an Array if (response instanceof ListResponse) { - this.allUsers = response.data != null && response.data.length > 0 ? response.data : []; + this.dataSource.data = response.data != null && response.data.length > 0 ? response.data : []; } else if (Array.isArray(response)) { - this.allUsers = response; + this.dataSource.data = response; } - this.allUsers.forEach((u) => { - if (!this.statusMap.has(u.status)) { - this.statusMap.set(u.status, [u]); - } else { - this.statusMap.get(u.status).push(u); - } - if (u.status !== this.userStatusType.Revoked) { - this.activeUsers.push(u); - } - }); - - // Filter based on UserStatus - this also populates the table on first load - this.filter(this.status); - this.firstLoaded = true; } - /** - * Filter the data source by user status. - * This overwrites dataSource.data because this filtering needs to apply first, before the search input - */ - filter(status: StatusType | null) { - this.status = status; - if (this.status != null) { - this.dataSource.data = this.statusMap.get(this.status); - } else { - this.dataSource.data = this.activeUsers; - } - // Reset checkbox selection - this.selectAll(false); - } - - checkUser(user: UserView, select?: boolean) { - (user as any).checked = select == null ? !(user as any).checked : select; - } - - selectAll(select: boolean) { - if (select) { - // Reset checkbox selection first so we know nothing else is selected - this.selectAll(false); - } - - const filteredUsers = this.dataSource.filteredData; - - const selectCount = - select && filteredUsers.length > MaxCheckedCount ? MaxCheckedCount : filteredUsers.length; - for (let i = 0; i < selectCount; i++) { - this.checkUser(filteredUsers[i], select); - } - } - invite() { this.edit(null); } @@ -237,59 +133,12 @@ export abstract class NewBasePeopleComponent< this.actionPromise = this.deleteUser(user.id); try { await this.actionPromise; - this.platformUtilsService.showToast( - "success", - null, - this.i18nService.t("removedUserId", this.userNamePipe.transform(user)), - ); - this.removeUser(user); - } catch (e) { - this.validationService.showError(e); - } - this.actionPromise = null; - } - - protected async revokeUserConfirmationDialog(user: UserView) { - return this.dialogService.openSimpleDialog({ - title: { key: "revokeAccess", placeholders: [this.userNamePipe.transform(user)] }, - content: this.revokeWarningMessage(), - acceptButtonText: { key: "revokeAccess" }, - type: "warning", - }); - } - - async revoke(user: UserView) { - const confirmed = await this.revokeUserConfirmationDialog(user); - - if (!confirmed) { - return false; - } - - this.actionPromise = this.revokeUser(user.id); - try { - await this.actionPromise; - this.platformUtilsService.showToast( - "success", - null, - this.i18nService.t("revokedUserId", this.userNamePipe.transform(user)), - ); - await this.load(); - } catch (e) { - this.validationService.showError(e); - } - this.actionPromise = null; - } - - async restore(user: UserView) { - this.actionPromise = this.restoreUser(user.id); - try { - await this.actionPromise; - this.platformUtilsService.showToast( - "success", - null, - this.i18nService.t("restoredUserId", this.userNamePipe.transform(user)), - ); - await this.load(); + this.toastService.showToast({ + variant: "success", + title: null, + message: this.i18nService.t("removedUserId", this.userNamePipe.transform(user)), + }); + this.dataSource.removeUser(user); } catch (e) { this.validationService.showError(e); } @@ -304,11 +153,11 @@ export abstract class NewBasePeopleComponent< this.actionPromise = this.reinviteUser(user.id); try { await this.actionPromise; - this.platformUtilsService.showToast( - "success", - null, - this.i18nService.t("hasBeenReinvited", this.userNamePipe.transform(user)), - ); + this.toastService.showToast({ + variant: "success", + title: null, + message: this.i18nService.t("hasBeenReinvited", this.userNamePipe.transform(user)), + }); } catch (e) { this.validationService.showError(e); } @@ -316,25 +165,18 @@ export abstract class NewBasePeopleComponent< } async confirm(user: UserView) { - function updateUser(self: NewBasePeopleComponent) { - user.status = self.userStatusType.Confirmed; - const mapIndex = self.statusMap.get(self.userStatusType.Accepted).indexOf(user); - if (mapIndex > -1) { - self.statusMap.get(self.userStatusType.Accepted).splice(mapIndex, 1); - self.statusMap.get(self.userStatusType.Confirmed).push(user); - } - } - const confirmUser = async (publicKey: Uint8Array) => { try { this.actionPromise = this.confirmUser(user, publicKey); await this.actionPromise; - updateUser(this); - this.platformUtilsService.showToast( - "success", - null, - this.i18nService.t("hasBeenConfirmed", this.userNamePipe.transform(user)), - ); + user.status = this.userStatusType.Confirmed; + this.dataSource.replaceUser(user); + + this.toastService.showToast({ + variant: "success", + title: null, + message: this.i18nService.t("hasBeenConfirmed", this.userNamePipe.transform(user)), + }); } catch (e) { this.validationService.showError(e); throw e; @@ -379,37 +221,4 @@ export abstract class NewBasePeopleComponent< this.logService.error(`Handled exception: ${e}`); } } - - protected revokeWarningMessage(): string { - return this.i18nService.t("revokeUserConfirmation"); - } - - protected getCheckedUsers() { - return this.dataSource.data.filter((u) => (u as any).checked); - } - - /** - * Remove a user row from the table and all related data sources - */ - protected removeUser(user: UserView) { - let index = this.dataSource.data.indexOf(user); - if (index > -1) { - // Clone the array so that the setter for dataSource.data is triggered to update the table rendering - const updatedData = [...this.dataSource.data]; - updatedData.splice(index, 1); - this.dataSource.data = updatedData; - } - - index = this.allUsers.indexOf(user); - if (index > -1) { - this.allUsers.splice(index, 1); - } - - if (this.statusMap.has(user.status)) { - index = this.statusMap.get(user.status).indexOf(user); - if (index > -1) { - this.statusMap.get(user.status).splice(index, 1); - } - } - } } diff --git a/apps/web/src/app/admin-console/common/people-table-data-source.ts b/apps/web/src/app/admin-console/common/people-table-data-source.ts new file mode 100644 index 00000000000..db357b4dbcd --- /dev/null +++ b/apps/web/src/app/admin-console/common/people-table-data-source.ts @@ -0,0 +1,133 @@ +import { + OrganizationUserStatusType, + ProviderUserStatusType, +} from "@bitwarden/common/admin-console/enums"; +import { TableDataSource } from "@bitwarden/components"; + +import { StatusType, UserViewTypes } from "./new-base.people.component"; + +const MaxCheckedCount = 500; + +/** + * Returns true if the user matches the status, or where the status is `null`, if the user is active (not revoked). + */ +function statusFilter(user: UserViewTypes, status: StatusType) { + if (status == null) { + return user.status != OrganizationUserStatusType.Revoked; + } + + return user.status === status; +} + +/** + * Returns true if the string matches the user's id, name, or email. + * (The default string search includes all properties, which can return false positives for collection names etc.) + */ +function textFilter(user: UserViewTypes, text: string) { + const normalizedText = text?.toLowerCase(); + return ( + !normalizedText || // null/empty strings should be ignored, i.e. always return true + user.email.toLowerCase().includes(normalizedText) || + user.id.toLowerCase().includes(normalizedText) || + user.name?.toLowerCase().includes(normalizedText) + ); +} + +export function peopleFilter(searchText: string, status: StatusType) { + return (user: UserViewTypes) => statusFilter(user, status) && textFilter(user, searchText); +} + +/** + * An extended TableDataSource class for managing people (organization members and provider users). + * It includes a tally of different statuses, utility methods, and other common functionality. + */ +export abstract class PeopleTableDataSource extends TableDataSource { + protected abstract statusType: typeof OrganizationUserStatusType | typeof ProviderUserStatusType; + + /** + * The number of 'active' users, that is, all users who are not in a revoked status. + */ + activeUserCount: number; + + invitedUserCount: number; + acceptedUserCount: number; + confirmedUserCount: number; + revokedUserCount: number; + + override set data(data: T[]) { + super.data = data; + + this.activeUserCount = + this.data?.filter((u) => u.status !== this.statusType.Revoked).length ?? 0; + + this.invitedUserCount = + this.data?.filter((u) => u.status === this.statusType.Invited).length ?? 0; + this.acceptedUserCount = + this.data?.filter((u) => u.status === this.statusType.Accepted).length ?? 0; + this.confirmedUserCount = + this.data?.filter((u) => u.status === this.statusType.Confirmed).length ?? 0; + this.revokedUserCount = + this.data?.filter((u) => u.status === this.statusType.Revoked).length ?? 0; + } + + override get data() { + // If you override a setter, you must also override the getter + return super.data; + } + + /** + * Check or uncheck a user in the table + * @param select check the user (true), uncheck the user (false), or toggle the current state (null) + */ + checkUser(user: T, select?: boolean) { + (user as any).checked = select == null ? !(user as any).checked : select; + } + + getCheckedUsers() { + return this.data.filter((u) => (u as any).checked); + } + + /** + * Check all filtered users (i.e. those rows that are currently visible) + * @param select check the filtered users (true) or uncheck the filtered users (false) + */ + checkAllFilteredUsers(select: boolean) { + if (select) { + // Reset checkbox selection first so we know nothing else is selected + this.uncheckAllUsers(); + } + + const filteredUsers = this.filteredData; + + const selectCount = + filteredUsers.length > MaxCheckedCount ? MaxCheckedCount : filteredUsers.length; + for (let i = 0; i < selectCount; i++) { + this.checkUser(filteredUsers[i], select); + } + } + + uncheckAllUsers() { + this.data.forEach((u) => ((u as any).checked = false)); + } + + /** + * Remove a user from the data source. Use this to ensure the table is re-rendered after the change. + */ + removeUser(user: T) { + // Note: use immutable functions so that we trigger setters to update the table + this.data = this.data.filter((u) => u != user); + } + + /** + * Replace a user in the data source by matching on user.id. Use this to ensure the table is re-rendered after the change. + */ + replaceUser(user: T) { + const index = this.data.findIndex((u) => u.id === user.id); + if (index > -1) { + // Clone the array so that the setter for dataSource.data is triggered to update the table rendering + const updatedData = this.data.slice(); + updatedData[index] = user; + this.data = updatedData; + } + } +} diff --git a/apps/web/src/app/admin-console/organizations/members/members-routing.module.ts b/apps/web/src/app/admin-console/organizations/members/members-routing.module.ts index a1d010deef5..d722ad6c0f9 100644 --- a/apps/web/src/app/admin-console/organizations/members/members-routing.module.ts +++ b/apps/web/src/app/admin-console/organizations/members/members-routing.module.ts @@ -5,12 +5,12 @@ import { canAccessMembersTab } from "@bitwarden/common/admin-console/abstraction import { OrganizationPermissionsGuard } from "../guards/org-permissions.guard"; -import { PeopleComponent } from "./people.component"; +import { MembersComponent } from "./members.component"; const routes: Routes = [ { path: "", - component: PeopleComponent, + component: MembersComponent, canActivate: [OrganizationPermissionsGuard], data: { titleId: "members", diff --git a/apps/web/src/app/admin-console/organizations/members/people.component.html b/apps/web/src/app/admin-console/organizations/members/members.component.html similarity index 93% rename from apps/web/src/app/admin-console/organizations/members/people.component.html rename to apps/web/src/app/admin-console/organizations/members/members.component.html index 9123b39a375..99afe8099a6 100644 --- a/apps/web/src/app/admin-console/organizations/members/people.component.html +++ b/apps/web/src/app/admin-console/organizations/members/members.component.html @@ -14,26 +14,35 @@
- {{ "all" | i18n }} {{ allCount }} + {{ "all" | i18n }} + {{ + allCount + }} {{ "invited" | i18n }} - {{ invitedCount }} + {{ + invitedCount + }} {{ "needsConfirmation" | i18n }} - {{ acceptedCount }} + {{ + acceptedUserCount + }} {{ "revoked" | i18n }} - {{ revokedCount }} + {{ + revokedCount + }}
@@ -67,7 +76,7 @@ type="checkbox" bitCheckbox class="tw-mr-1" - (change)="selectAll($any($event.target).checked)" + (change)="dataSource.checkAllFilteredUsers($any($event.target).checked)" id="selectAll" />