From 5b3c1fcd01a8f3771f7e073ffc67aec4f03c735a Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Fri, 6 Dec 2024 15:51:54 -0500 Subject: [PATCH 1/6] [PM-13755] Decouple Invite and Edit User Flows --- .../member-dialog/member-dialog.component.ts | 136 +++++++++++------- .../members/members.component.ts | 86 ++++++----- 2 files changed, 136 insertions(+), 86 deletions(-) diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts index fe84c5097de..4956c2e67be 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts @@ -158,8 +158,21 @@ export class MemberDialogComponent implements OnDestroy { .pipe(shareReplay({ refCount: true, bufferSize: 1 })); this.editMode = this.params.organizationUserId != null; + + let userDetails$; + if (this.editMode) { + this.title = this.i18nService.t("editMember"); + userDetails$ = this.userService.get( + this.params.organizationId, + this.params.organizationUserId, + ); + } else { + this.title = this.i18nService.t("inviteMember"); + userDetails$ = of(null); + } + this.tabIndex = this.params.initialTab ?? MemberDialogTab.Role; - this.title = this.i18nService.t(this.editMode ? "editMember" : "inviteMember"); + this.isOnSecretsManagerStandalone = this.params.isOnSecretsManagerStandalone; if (this.isOnSecretsManagerStandalone) { @@ -176,10 +189,6 @@ export class MemberDialogComponent implements OnDestroy { ), ); - const userDetails$ = this.params.organizationUserId - ? this.userService.get(this.params.organizationId, this.params.organizationUserId) - : of(null); - this.allowAdminAccessToAllCollectionItems$ = this.organization$.pipe( map((organization) => { return organization.allowAdminAccessToAllCollectionItems; @@ -276,6 +285,72 @@ export class MemberDialogComponent implements OnDestroy { emailsControl.updateValueAndValidity(); } + private async handleInviteUsers(userView: OrganizationUserAdminView, organization: Organization) { + const emails = [...new Set(this.formGroup.value.emails.trim().split(/\s*,\s*/))]; + + if (this.enforceEmailCountLimit(emails, organization)) { + return; + } + + await this.userService.invite(emails, userView); + + this.toastService.showToast({ + variant: "success", + title: null, + message: this.i18nService.t("invitedUsers"), + }); + this.close(MemberDialogResult.Saved); + } + + private enforceEmailCountLimit(emails: string[], organization: Organization): boolean { + const maxEmailsCount = organization.productTierType === ProductTierType.TeamsStarter ? 10 : 20; + + if (emails.length > maxEmailsCount) { + this.formGroup.controls.emails.setErrors({ + tooManyEmails: { message: this.i18nService.t("tooManyEmails", maxEmailsCount) }, + }); + return true; + } + + return false; + } + + private async handleEditUser(userView: OrganizationUserAdminView) { + userView.id = this.params.organizationUserId; + await this.userService.save(userView); + + this.toastService.showToast({ + variant: "success", + title: null, + message: this.i18nService.t("editedUserId", this.params.name), + }); + + this.close(MemberDialogResult.Saved); + } + + private async getUserView(): Promise { + const userView = new OrganizationUserAdminView(); + userView.organizationId = this.params.organizationId; + userView.type = this.formGroup.value.type; + + userView.permissions = this.setRequestPermissions( + userView.permissions ?? new PermissionsApi(), + userView.type !== OrganizationUserType.Custom, + ); + + userView.collections = this.formGroup.value.access + .filter((v) => v.type === AccessItemType.Collection) + .map(convertToSelectionView); + + userView.groups = (await firstValueFrom(this.restrictEditingSelf$)) + ? null + : this.formGroup.value.groups.map((m) => m.id); + + userView.accessSecretsManager = this.formGroup.value.accessSecretsManager; + + return userView; + } + private loadOrganizationUser( userDetails: OrganizationUserAdminView, groups: GroupDetailsView[], @@ -418,58 +493,13 @@ export class MemberDialogComponent implements OnDestroy { return; } - const userView = new OrganizationUserAdminView(); - userView.id = this.params.organizationUserId; - userView.organizationId = this.params.organizationId; - userView.type = this.formGroup.value.type; - userView.permissions = this.setRequestPermissions( - userView.permissions ?? new PermissionsApi(), - userView.type !== OrganizationUserType.Custom, - ); - userView.collections = this.formGroup.value.access - .filter((v) => v.type === AccessItemType.Collection) - .map(convertToSelectionView); - - userView.groups = (await firstValueFrom(this.restrictEditingSelf$)) - ? null - : this.formGroup.value.groups.map((m) => m.id); - - userView.accessSecretsManager = this.formGroup.value.accessSecretsManager; + const userView = await this.getUserView(); if (this.editMode) { - await this.userService.save(userView); + await this.handleEditUser(userView); } else { - userView.id = this.params.organizationUserId; - const maxEmailsCount = - organization.productTierType === ProductTierType.TeamsStarter ? 10 : 20; - const emails = [...new Set(this.formGroup.value.emails.trim().split(/\s*,\s*/))]; - if (emails.length > maxEmailsCount) { - this.formGroup.controls.emails.setErrors({ - tooManyEmails: { message: this.i18nService.t("tooManyEmails", maxEmailsCount) }, - }); - return; - } - if ( - organization.hasReseller && - this.params.numConfirmedMembers + emails.length > organization.seats - ) { - this.formGroup.controls.emails.setErrors({ - tooManyEmails: { message: this.i18nService.t("seatLimitReachedContactYourProvider") }, - }); - return; - } - await this.userService.invite(emails, userView); + await this.handleInviteUsers(userView, organization); } - - this.toastService.showToast({ - variant: "success", - title: null, - message: this.i18nService.t( - this.editMode ? "editedUserId" : "invitedUsers", - this.params.name, - ), - }); - this.close(MemberDialogResult.Saved); }; remove = async () => { diff --git a/apps/web/src/app/admin-console/organizations/members/members.component.ts b/apps/web/src/app/admin-console/organizations/members/members.component.ts index 26e27e1249b..fe37b443384 100644 --- a/apps/web/src/app/admin-console/organizations/members/members.component.ts +++ b/apps/web/src/app/admin-console/organizations/members/members.component.ts @@ -462,9 +462,51 @@ export class MembersComponent extends BaseMembersComponent firstValueFrom(simpleDialog.closed).then(this.handleDialogClose.bind(this)); } - async edit(user: OrganizationUserView, initialTab: MemberDialogTab = MemberDialogTab.Role) { + private async handleInviteDialog(initialTab: MemberDialogTab) { + const dialog = openUserAddEditDialog(this.dialogService, { + data: { + name: null, + organizationId: this.organization.id, + organizationUserId: null, + allOrganizationUserEmails: this.dataSource.data?.map((user) => user.email) ?? [], + usesKeyConnector: null, + isOnSecretsManagerStandalone: this.orgIsOnSecretsManagerStandalone, + initialTab: initialTab, + numConfirmedMembers: this.dataSource.confirmedUserCount, + managedByOrganization: null, + }, + }); + + const result = await lastValueFrom(dialog.closed); + + if (result === MemberDialogResult.Saved) { + await this.load(); + } + } + + private async handleSeatLimitForFixedTiers() { + if (!this.organization.canEditSubscription) { + await this.showSeatLimitReachedDialog(); + return; + } + + const reference = openChangePlanDialog(this.dialogService, { + data: { + organizationId: this.organization.id, + subscription: null, + productTierType: this.organization.productTierType, + }, + }); + + const result = await lastValueFrom(reference.closed); + + if (result === ChangePlanDialogResultType.Submitted) { + await this.load(); + } + } + + async invite(initialTab: MemberDialogTab = MemberDialogTab.Role) { if ( - !user && this.organization.hasReseller && this.organization.seats === this.dataSource.confirmedUserCount ) { @@ -473,52 +515,30 @@ export class MembersComponent extends BaseMembersComponent title: this.i18nService.t("seatLimitReached"), message: this.i18nService.t("contactYourProvider"), }); - return; - } - - // Invite User: Add Flow - // Click on user email: Edit Flow - - // User attempting to invite new users in a free org with max users - if ( - !user && + } else if ( this.dataSource.data.length === this.organization.seats && (this.organization.productTierType === ProductTierType.Free || this.organization.productTierType === ProductTierType.TeamsStarter || this.organization.productTierType === ProductTierType.Families) ) { - if (!this.organization.canEditSubscription) { - await this.showSeatLimitReachedDialog(); - return; - } - - const reference = openChangePlanDialog(this.dialogService, { - data: { - organizationId: this.organization.id, - subscription: null, - productTierType: this.organization.productTierType, - }, - }); - - const result = await lastValueFrom(reference.closed); - - if (result === ChangePlanDialogResultType.Submitted) { - await this.load(); - } - return; + await this.handleSeatLimitForFixedTiers(); + } else { + await this.handleInviteDialog(initialTab); } + } + async edit(user: OrganizationUserView, initialTab: MemberDialogTab = MemberDialogTab.Role) { const dialog = openUserAddEditDialog(this.dialogService, { data: { name: this.userNamePipe.transform(user), organizationId: this.organization.id, - organizationUserId: user != null ? user.id : null, + organizationUserId: user.id, allOrganizationUserEmails: this.dataSource.data?.map((user) => user.email) ?? [], - usesKeyConnector: user?.usesKeyConnector, + usesKeyConnector: user.usesKeyConnector, isOnSecretsManagerStandalone: this.orgIsOnSecretsManagerStandalone, initialTab: initialTab, numConfirmedMembers: this.dataSource.confirmedUserCount, - managedByOrganization: user?.managedByOrganization, + managedByOrganization: user.managedByOrganization, }, }); From 4df84ff3d24699ceafcbb1b4043acca9d3efdf15 Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Tue, 10 Dec 2024 11:18:34 -0500 Subject: [PATCH 2/6] Address code review feedback. --- .../member-dialog/member-dialog.component.ts | 136 +++++++------- .../org-seat-limit-reached.validator.spec.ts | 173 +++++++++++++++++- .../org-seat-limit-reached.validator.ts | 36 ++++ .../members/members.component.ts | 22 ++- 4 files changed, 290 insertions(+), 77 deletions(-) diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts index 4956c2e67be..ab9976b985c 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts @@ -50,7 +50,10 @@ import { } from "../../../shared/components/access-selector"; import { commaSeparatedEmails } from "./validators/comma-separated-emails.validator"; -import { orgSeatLimitReachedValidator } from "./validators/org-seat-limit-reached.validator"; +import { + orgSeatLimitReachedValidator, + inputEmailLimitValidator, +} from "./validators/org-seat-limit-reached.validator"; export enum MemberDialogTab { Role = 0, @@ -285,72 +288,6 @@ export class MemberDialogComponent implements OnDestroy { emailsControl.updateValueAndValidity(); } - private async handleInviteUsers(userView: OrganizationUserAdminView, organization: Organization) { - const emails = [...new Set(this.formGroup.value.emails.trim().split(/\s*,\s*/))]; - - if (this.enforceEmailCountLimit(emails, organization)) { - return; - } - - await this.userService.invite(emails, userView); - - this.toastService.showToast({ - variant: "success", - title: null, - message: this.i18nService.t("invitedUsers"), - }); - this.close(MemberDialogResult.Saved); - } - - private enforceEmailCountLimit(emails: string[], organization: Organization): boolean { - const maxEmailsCount = organization.productTierType === ProductTierType.TeamsStarter ? 10 : 20; - - if (emails.length > maxEmailsCount) { - this.formGroup.controls.emails.setErrors({ - tooManyEmails: { message: this.i18nService.t("tooManyEmails", maxEmailsCount) }, - }); - return true; - } - - return false; - } - - private async handleEditUser(userView: OrganizationUserAdminView) { - userView.id = this.params.organizationUserId; - await this.userService.save(userView); - - this.toastService.showToast({ - variant: "success", - title: null, - message: this.i18nService.t("editedUserId", this.params.name), - }); - - this.close(MemberDialogResult.Saved); - } - - private async getUserView(): Promise { - const userView = new OrganizationUserAdminView(); - userView.organizationId = this.params.organizationId; - userView.type = this.formGroup.value.type; - - userView.permissions = this.setRequestPermissions( - userView.permissions ?? new PermissionsApi(), - userView.type !== OrganizationUserType.Custom, - ); - - userView.collections = this.formGroup.value.access - .filter((v) => v.type === AccessItemType.Collection) - .map(convertToSelectionView); - - userView.groups = (await firstValueFrom(this.restrictEditingSelf$)) - ? null - : this.formGroup.value.groups.map((m) => m.id); - - userView.accessSecretsManager = this.formGroup.value.accessSecretsManager; - - return userView; - } - private loadOrganizationUser( userDetails: OrganizationUserAdminView, groups: GroupDetailsView[], @@ -502,6 +439,71 @@ export class MemberDialogComponent implements OnDestroy { } }; + private async getUserView(): Promise { + const userView = new OrganizationUserAdminView(); + userView.organizationId = this.params.organizationId; + userView.type = this.formGroup.value.type; + + userView.permissions = this.setRequestPermissions( + userView.permissions ?? new PermissionsApi(), + userView.type !== OrganizationUserType.Custom, + ); + + userView.collections = this.formGroup.value.access + .filter((v) => v.type === AccessItemType.Collection) + .map(convertToSelectionView); + + userView.groups = (await firstValueFrom(this.restrictEditingSelf$)) + ? null + : this.formGroup.value.groups.map((m) => m.id); + + userView.accessSecretsManager = this.formGroup.value.accessSecretsManager; + + return userView; + } + + private async handleEditUser(userView: OrganizationUserAdminView) { + userView.id = this.params.organizationUserId; + await this.userService.save(userView); + + this.toastService.showToast({ + variant: "success", + title: null, + message: this.i18nService.t("editedUserId", this.params.name), + }); + + this.close(MemberDialogResult.Saved); + } + + private async handleInviteUsers(userView: OrganizationUserAdminView, organization: Organization) { + const emails = [...new Set(this.formGroup.value.emails.trim().split(/\s*,\s*/))]; + + this.setInputEmailCountValidator(organization, emails.length); + + await this.userService.invite(emails, userView); + + this.toastService.showToast({ + variant: "success", + title: null, + message: this.i18nService.t("invitedUsers"), + }); + this.close(MemberDialogResult.Saved); + } + + private setInputEmailCountValidator(organization: Organization, emailCount: number) { + const emailsControlValidators = [ + Validators.required, + commaSeparatedEmails, + inputEmailLimitValidator(organization, (maxEmailsCount: number) => + this.i18nService.t("tooManyEmails", maxEmailsCount), + ), + ]; + + const emailsControl = this.formGroup.get("emails"); + emailsControl.setValidators(emailsControlValidators); + emailsControl.updateValueAndValidity(); + } + remove = async () => { if (!this.editMode) { return; diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts index 6c693ee8f84..569aceb0107 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts @@ -4,7 +4,10 @@ import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { ProductTierType } from "@bitwarden/common/billing/enums"; -import { orgSeatLimitReachedValidator } from "./org-seat-limit-reached.validator"; +import { + inputEmailLimitValidator, + orgSeatLimitReachedValidator, +} from "./org-seat-limit-reached.validator"; const orgFactory = (props: Partial = {}) => Object.assign( @@ -131,3 +134,171 @@ describe("orgSeatLimitReachedValidator", () => { expect(result).toBeNull(); }); }); + +describe("inputEmailLimitValidator", () => { + const getErrorMessage = (max: number) => `You can only add up to ${max} unique emails.`; + + const createUniqueEmailString = (numberOfEmails: number) => { + let result = ""; + + for (let i = 1; i <= numberOfEmails; i++) { + result += `test${i}@test.com,`; + } + + // Remove the last comma and space + result = result.slice(0, -1); + + return result; + }; + + describe("TeamsStarter limit validation", () => { + const teamsStarterOrganization = orgFactory({ + productTierType: ProductTierType.TeamsStarter, + seats: 10, + }); + + it("should return null if unique email count is within the limit", () => { + // Arrange + const control = new FormControl("test1@test.com, test2@test.com, test3@test.com"); + + const validatorFn = inputEmailLimitValidator(teamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return null if unique email count is equal the limit", () => { + // Arrange + const control = new FormControl( + "test1@test.com, test2@test.com, test3@test.com, test4@test.com, test5@test.com, test6@test.com, test7@test.com, test8@test.com, test9@test.com, test10@test.com", + ); + + const validatorFn = inputEmailLimitValidator(teamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return an error if unique email count exceeds the limit", () => { + // Arrange + const control = new FormControl(createUniqueEmailString(11)); + + const validatorFn = inputEmailLimitValidator(teamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toEqual({ + tooManyEmails: { message: "You can only add up to 10 unique emails." }, + }); + }); + }); + + describe("none TeamsStarter limit validation", () => { + const noneTeamsStarterOrganization = orgFactory({ + productTierType: ProductTierType.Enterprise, + seats: 100, + }); + + it("should return null if unique email count is within the limit", () => { + // Arrange + const control = new FormControl("test1@test.com, test2@test.com, test3@test.com"); + + const validatorFn = inputEmailLimitValidator(noneTeamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return null if unique email count is equal the limit", () => { + // Arrange + const control = new FormControl( + "test1@test.com, test2@test.com, test3@test.com, test4@test.com, test5@test.com, test6@test.com, test7@test.com, test8@test.com, test9@test.com, test10@test.com", + ); + + const validatorFn = inputEmailLimitValidator(noneTeamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return an error if unique email count exceeds the limit", () => { + // Arrange + + const control = new FormControl(createUniqueEmailString(21)); + + const validatorFn = inputEmailLimitValidator(noneTeamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toEqual({ + tooManyEmails: { message: "You can only add up to 20 unique emails." }, + }); + }); + }); + + describe("input email validation", () => { + const organization = orgFactory({ + productTierType: ProductTierType.Enterprise, + seats: 100, + }); + + it("should ignore duplicate emails and validate only unique ones", () => { + // Arrange + + const sixUniqueEmails = createUniqueEmailString(6); + const sixDuplicateEmails = + "test1@test.com,test1@test.com,test1@test.com,test1@test.com,test1@test.com,test1@test.com"; + + const control = new FormControl(sixUniqueEmails + sixDuplicateEmails); + const validatorFn = inputEmailLimitValidator(organization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return null if input is null", () => { + // Arrange + const control: AbstractControl = new FormControl(null); + + const validatorFn = inputEmailLimitValidator(organization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return null if input is empty", () => { + // Arrange + const control: AbstractControl = new FormControl(""); + + const validatorFn = inputEmailLimitValidator(organization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + }); +}); diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.ts index bcd84743918..838499653c7 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.ts @@ -47,3 +47,39 @@ export function orgSeatLimitReachedValidator( : null; }; } + +function getUniqueInputEmails(control: AbstractControl): string[] { + const emails: string[] = control.value + .split(",") + .filter((email: string) => email && email.trim() !== ""); + const uniqueEmails: string[] = Array.from(new Set(emails)); + + return uniqueEmails; +} + +/** + * Ensure the number of unique emails in an input does not exceed the allowed maximum. + * @param organization An object representing the organization + * @param getErrorMessage A callback function that generates the error message. It takes the `maxEmailsCount` as a parameter. + * @returns A function that validates an `AbstractControl` and returns `ValidationErrors` or `null` + */ +export function inputEmailLimitValidator( + organization: Organization, + getErrorMessage: (maxEmailsCount: number) => string, +): ValidatorFn { + return (control: AbstractControl): ValidationErrors | null => { + if (control.value === "" || !control.value) { + return null; + } + + const maxEmailsCount = organization.productTierType === ProductTierType.TeamsStarter ? 10 : 20; + + const uniqueEmails = getUniqueInputEmails(control); + + if (uniqueEmails.length <= maxEmailsCount) { + return null; + } + + return { tooManyEmails: { message: getErrorMessage(maxEmailsCount) } }; + }; +} diff --git a/apps/web/src/app/admin-console/organizations/members/members.component.ts b/apps/web/src/app/admin-console/organizations/members/members.component.ts index fe37b443384..45e7982a847 100644 --- a/apps/web/src/app/admin-console/organizations/members/members.component.ts +++ b/apps/web/src/app/admin-console/organizations/members/members.component.ts @@ -462,7 +462,7 @@ export class MembersComponent extends BaseMembersComponent firstValueFrom(simpleDialog.closed).then(this.handleDialogClose.bind(this)); } - private async handleInviteDialog(initialTab: MemberDialogTab) { + private async handleInviteDialog() { const dialog = openUserAddEditDialog(this.dialogService, { data: { name: null, @@ -471,7 +471,7 @@ export class MembersComponent extends BaseMembersComponent allOrganizationUserEmails: this.dataSource.data?.map((user) => user.email) ?? [], usesKeyConnector: null, isOnSecretsManagerStandalone: this.orgIsOnSecretsManagerStandalone, - initialTab: initialTab, + initialTab: MemberDialogTab.Role, numConfirmedMembers: this.dataSource.confirmedUserCount, managedByOrganization: null, }, @@ -505,7 +505,7 @@ export class MembersComponent extends BaseMembersComponent } } - async invite(initialTab: MemberDialogTab = MemberDialogTab.Role) { + async invite() { if ( this.organization.hasReseller && this.organization.seats === this.dataSource.confirmedUserCount @@ -515,16 +515,22 @@ export class MembersComponent extends BaseMembersComponent title: this.i18nService.t("seatLimitReached"), message: this.i18nService.t("contactYourProvider"), }); - } else if ( + + return; + } + + if ( this.dataSource.data.length === this.organization.seats && (this.organization.productTierType === ProductTierType.Free || this.organization.productTierType === ProductTierType.TeamsStarter || this.organization.productTierType === ProductTierType.Families) ) { await this.handleSeatLimitForFixedTiers(); - } else { - await this.handleInviteDialog(initialTab); + + return; } + + await this.handleInviteDialog(); } async edit(user: OrganizationUserView, initialTab: MemberDialogTab = MemberDialogTab.Role) { @@ -550,9 +556,7 @@ export class MembersComponent extends BaseMembersComponent case MemberDialogResult.Saved: case MemberDialogResult.Revoked: case MemberDialogResult.Restored: - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - this.load(); + await this.load(); break; } } From 932129b5010262c8fb709761dc95c9ce82e200f5 Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Tue, 10 Dec 2024 11:26:27 -0500 Subject: [PATCH 3/6] Fix minor test setup to account for trim --- .../validators/org-seat-limit-reached.validator.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts index 569aceb0107..7b41c839dc0 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts @@ -142,7 +142,7 @@ describe("inputEmailLimitValidator", () => { let result = ""; for (let i = 1; i <= numberOfEmails; i++) { - result += `test${i}@test.com,`; + result += `test${i}@test.com, `; } // Remove the last comma and space From 49553ec14a8480e0c091071e693fe561c0ddf0d8 Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Wed, 11 Dec 2024 14:24:48 -0500 Subject: [PATCH 4/6] Address code review feebacks. --- .../member-dialog/member-dialog.component.ts | 35 ++-- .../input-email-limit.validator.spec.ts | 191 ++++++++++++++++++ .../validators/input-email-limit.validator.ts | 40 ++++ .../org-seat-limit-reached.validator.spec.ts | 173 +--------------- .../org-seat-limit-reached.validator.ts | 36 ---- 5 files changed, 245 insertions(+), 230 deletions(-) create mode 100644 apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/input-email-limit.validator.spec.ts create mode 100644 apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/input-email-limit.validator.ts diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts index ab9976b985c..22ca27bb311 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts @@ -50,10 +50,8 @@ import { } from "../../../shared/components/access-selector"; import { commaSeparatedEmails } from "./validators/comma-separated-emails.validator"; -import { - orgSeatLimitReachedValidator, - inputEmailLimitValidator, -} from "./validators/org-seat-limit-reached.validator"; +import { inputEmailLimitValidator } from "./validators/input-email-limit.validator"; +import { orgSeatLimitReachedValidator } from "./validators/org-seat-limit-reached.validator"; export enum MemberDialogTab { Role = 0, @@ -273,7 +271,7 @@ export class MemberDialogComponent implements OnDestroy { } private setFormValidators(organization: Organization) { - const emailsControlValidators = [ + const _orgSeatLimitReachedValidator = [ Validators.required, commaSeparatedEmails, orgSeatLimitReachedValidator( @@ -283,8 +281,17 @@ export class MemberDialogComponent implements OnDestroy { ), ]; + const _inputEmailLimitValidator = [ + Validators.required, + commaSeparatedEmails, + inputEmailLimitValidator(organization, (maxEmailsCount: number) => + this.i18nService.t("tooManyEmails", maxEmailsCount), + ), + ]; + const emailsControl = this.formGroup.get("emails"); - emailsControl.setValidators(emailsControlValidators); + emailsControl.setValidators(_orgSeatLimitReachedValidator); + emailsControl.setValidators(_inputEmailLimitValidator); emailsControl.updateValueAndValidity(); } @@ -478,8 +485,6 @@ export class MemberDialogComponent implements OnDestroy { private async handleInviteUsers(userView: OrganizationUserAdminView, organization: Organization) { const emails = [...new Set(this.formGroup.value.emails.trim().split(/\s*,\s*/))]; - this.setInputEmailCountValidator(organization, emails.length); - await this.userService.invite(emails, userView); this.toastService.showToast({ @@ -490,20 +495,6 @@ export class MemberDialogComponent implements OnDestroy { this.close(MemberDialogResult.Saved); } - private setInputEmailCountValidator(organization: Organization, emailCount: number) { - const emailsControlValidators = [ - Validators.required, - commaSeparatedEmails, - inputEmailLimitValidator(organization, (maxEmailsCount: number) => - this.i18nService.t("tooManyEmails", maxEmailsCount), - ), - ]; - - const emailsControl = this.formGroup.get("emails"); - emailsControl.setValidators(emailsControlValidators); - emailsControl.updateValueAndValidity(); - } - remove = async () => { if (!this.editMode) { return; diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/input-email-limit.validator.spec.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/input-email-limit.validator.spec.ts new file mode 100644 index 00000000000..5a9a0e128e7 --- /dev/null +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/input-email-limit.validator.spec.ts @@ -0,0 +1,191 @@ +import { AbstractControl, FormControl } from "@angular/forms"; + +import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; +import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; +import { ProductTierType } from "@bitwarden/common/billing/enums"; + +import { inputEmailLimitValidator } from "./input-email-limit.validator"; + +const orgFactory = (props: Partial = {}) => + Object.assign( + new Organization(), + { + id: "myOrgId", + enabled: true, + type: OrganizationUserType.Admin, + }, + props, + ); + +describe("inputEmailLimitValidator", () => { + const getErrorMessage = (max: number) => `You can only add up to ${max} unique emails.`; + + const createUniqueEmailString = (numberOfEmails: number) => + Array(numberOfEmails) + .fill(null) + .map((_, i) => `email${i}@example.com`) + .join(", "); + + const createIdenticalEmailString = (numberOfEmails: number) => + Array(numberOfEmails) + .fill(null) + .map(() => `email@example.com`) + .join(", "); + + describe("TeamsStarter limit validation", () => { + let teamsStarterOrganization: Organization; + + beforeEach(() => { + teamsStarterOrganization = orgFactory({ + productTierType: ProductTierType.TeamsStarter, + seats: 10, + }); + }); + + it("should return null if unique email count is within the limit", () => { + // Arrange + const control = new FormControl(createUniqueEmailString(3)); + + const validatorFn = inputEmailLimitValidator(teamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return null if unique email count is equal the limit", () => { + // Arrange + const control = new FormControl(createUniqueEmailString(10)); + + const validatorFn = inputEmailLimitValidator(teamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return an error if unique email count exceeds the limit", () => { + // Arrange + const control = new FormControl(createUniqueEmailString(11)); + + const validatorFn = inputEmailLimitValidator(teamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toEqual({ + tooManyEmails: { message: "You can only add up to 10 unique emails." }, + }); + }); + }); + + describe("Non-TeamsStarter limit validation", () => { + let nonTeamsStarterOrganization: Organization; + + beforeEach(() => { + nonTeamsStarterOrganization = orgFactory({ + productTierType: ProductTierType.Enterprise, + seats: 100, + }); + }); + + it("should return null if unique email count is within the limit", () => { + // Arrange + const control = new FormControl(createUniqueEmailString(3)); + + const validatorFn = inputEmailLimitValidator(nonTeamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return null if unique email count is equal the limit", () => { + // Arrange + const control = new FormControl(createUniqueEmailString(10)); + + const validatorFn = inputEmailLimitValidator(nonTeamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return an error if unique email count exceeds the limit", () => { + // Arrange + + const control = new FormControl(createUniqueEmailString(21)); + + const validatorFn = inputEmailLimitValidator(nonTeamsStarterOrganization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toEqual({ + tooManyEmails: { message: "You can only add up to 20 unique emails." }, + }); + }); + }); + + describe("input email validation", () => { + let organization: Organization; + + beforeEach(() => { + organization = orgFactory({ + productTierType: ProductTierType.Enterprise, + seats: 100, + }); + }); + + it("should ignore duplicate emails and validate only unique ones", () => { + // Arrange + const sixUniqueEmails = createUniqueEmailString(6); + const sixDuplicateEmails = createIdenticalEmailString(6); + + const control = new FormControl(sixUniqueEmails + sixDuplicateEmails); + const validatorFn = inputEmailLimitValidator(organization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return null if input is null", () => { + // Arrange + const control: AbstractControl = new FormControl(null); + + const validatorFn = inputEmailLimitValidator(organization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + + it("should return null if input is empty", () => { + // Arrange + const control: AbstractControl = new FormControl(""); + + const validatorFn = inputEmailLimitValidator(organization, getErrorMessage); + + // Act + const result = validatorFn(control); + + // Assert + expect(result).toBeNull(); + }); + }); +}); diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/input-email-limit.validator.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/input-email-limit.validator.ts new file mode 100644 index 00000000000..f34c2e909aa --- /dev/null +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/input-email-limit.validator.ts @@ -0,0 +1,40 @@ +import { AbstractControl, ValidationErrors, ValidatorFn } from "@angular/forms"; + +import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; +import { ProductTierType } from "@bitwarden/common/billing/enums"; + +function getUniqueInputEmails(control: AbstractControl): string[] { + const emails: string[] = control.value + .split(",") + .filter((email: string) => email && email.trim() !== ""); + const uniqueEmails: string[] = Array.from(new Set(emails)); + + return uniqueEmails; +} + +/** + * Ensure the number of unique emails in an input does not exceed the allowed maximum. + * @param organization An object representing the organization + * @param getErrorMessage A callback function that generates the error message. It takes the `maxEmailsCount` as a parameter. + * @returns A function that validates an `AbstractControl` and returns `ValidationErrors` or `null` + */ +export function inputEmailLimitValidator( + organization: Organization, + getErrorMessage: (maxEmailsCount: number) => string, +): ValidatorFn { + return (control: AbstractControl): ValidationErrors | null => { + if (!control.value?.trim()) { + return null; + } + + const maxEmailsCount = organization.productTierType === ProductTierType.TeamsStarter ? 10 : 20; + + const uniqueEmails = getUniqueInputEmails(control); + + if (uniqueEmails.length <= maxEmailsCount) { + return null; + } + + return { tooManyEmails: { message: getErrorMessage(maxEmailsCount) } }; + }; +} diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts index 7b41c839dc0..6c693ee8f84 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.spec.ts @@ -4,10 +4,7 @@ import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { ProductTierType } from "@bitwarden/common/billing/enums"; -import { - inputEmailLimitValidator, - orgSeatLimitReachedValidator, -} from "./org-seat-limit-reached.validator"; +import { orgSeatLimitReachedValidator } from "./org-seat-limit-reached.validator"; const orgFactory = (props: Partial = {}) => Object.assign( @@ -134,171 +131,3 @@ describe("orgSeatLimitReachedValidator", () => { expect(result).toBeNull(); }); }); - -describe("inputEmailLimitValidator", () => { - const getErrorMessage = (max: number) => `You can only add up to ${max} unique emails.`; - - const createUniqueEmailString = (numberOfEmails: number) => { - let result = ""; - - for (let i = 1; i <= numberOfEmails; i++) { - result += `test${i}@test.com, `; - } - - // Remove the last comma and space - result = result.slice(0, -1); - - return result; - }; - - describe("TeamsStarter limit validation", () => { - const teamsStarterOrganization = orgFactory({ - productTierType: ProductTierType.TeamsStarter, - seats: 10, - }); - - it("should return null if unique email count is within the limit", () => { - // Arrange - const control = new FormControl("test1@test.com, test2@test.com, test3@test.com"); - - const validatorFn = inputEmailLimitValidator(teamsStarterOrganization, getErrorMessage); - - // Act - const result = validatorFn(control); - - // Assert - expect(result).toBeNull(); - }); - - it("should return null if unique email count is equal the limit", () => { - // Arrange - const control = new FormControl( - "test1@test.com, test2@test.com, test3@test.com, test4@test.com, test5@test.com, test6@test.com, test7@test.com, test8@test.com, test9@test.com, test10@test.com", - ); - - const validatorFn = inputEmailLimitValidator(teamsStarterOrganization, getErrorMessage); - - // Act - const result = validatorFn(control); - - // Assert - expect(result).toBeNull(); - }); - - it("should return an error if unique email count exceeds the limit", () => { - // Arrange - const control = new FormControl(createUniqueEmailString(11)); - - const validatorFn = inputEmailLimitValidator(teamsStarterOrganization, getErrorMessage); - - // Act - const result = validatorFn(control); - - // Assert - expect(result).toEqual({ - tooManyEmails: { message: "You can only add up to 10 unique emails." }, - }); - }); - }); - - describe("none TeamsStarter limit validation", () => { - const noneTeamsStarterOrganization = orgFactory({ - productTierType: ProductTierType.Enterprise, - seats: 100, - }); - - it("should return null if unique email count is within the limit", () => { - // Arrange - const control = new FormControl("test1@test.com, test2@test.com, test3@test.com"); - - const validatorFn = inputEmailLimitValidator(noneTeamsStarterOrganization, getErrorMessage); - - // Act - const result = validatorFn(control); - - // Assert - expect(result).toBeNull(); - }); - - it("should return null if unique email count is equal the limit", () => { - // Arrange - const control = new FormControl( - "test1@test.com, test2@test.com, test3@test.com, test4@test.com, test5@test.com, test6@test.com, test7@test.com, test8@test.com, test9@test.com, test10@test.com", - ); - - const validatorFn = inputEmailLimitValidator(noneTeamsStarterOrganization, getErrorMessage); - - // Act - const result = validatorFn(control); - - // Assert - expect(result).toBeNull(); - }); - - it("should return an error if unique email count exceeds the limit", () => { - // Arrange - - const control = new FormControl(createUniqueEmailString(21)); - - const validatorFn = inputEmailLimitValidator(noneTeamsStarterOrganization, getErrorMessage); - - // Act - const result = validatorFn(control); - - // Assert - expect(result).toEqual({ - tooManyEmails: { message: "You can only add up to 20 unique emails." }, - }); - }); - }); - - describe("input email validation", () => { - const organization = orgFactory({ - productTierType: ProductTierType.Enterprise, - seats: 100, - }); - - it("should ignore duplicate emails and validate only unique ones", () => { - // Arrange - - const sixUniqueEmails = createUniqueEmailString(6); - const sixDuplicateEmails = - "test1@test.com,test1@test.com,test1@test.com,test1@test.com,test1@test.com,test1@test.com"; - - const control = new FormControl(sixUniqueEmails + sixDuplicateEmails); - const validatorFn = inputEmailLimitValidator(organization, getErrorMessage); - - // Act - const result = validatorFn(control); - - // Assert - expect(result).toBeNull(); - }); - - it("should return null if input is null", () => { - // Arrange - const control: AbstractControl = new FormControl(null); - - const validatorFn = inputEmailLimitValidator(organization, getErrorMessage); - - // Act - const result = validatorFn(control); - - // Assert - expect(result).toBeNull(); - }); - - it("should return null if input is empty", () => { - // Arrange - const control: AbstractControl = new FormControl(""); - - const validatorFn = inputEmailLimitValidator(organization, getErrorMessage); - - // Act - const result = validatorFn(control); - - // Assert - expect(result).toBeNull(); - }); - }); -}); diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.ts index 838499653c7..bcd84743918 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/validators/org-seat-limit-reached.validator.ts @@ -47,39 +47,3 @@ export function orgSeatLimitReachedValidator( : null; }; } - -function getUniqueInputEmails(control: AbstractControl): string[] { - const emails: string[] = control.value - .split(",") - .filter((email: string) => email && email.trim() !== ""); - const uniqueEmails: string[] = Array.from(new Set(emails)); - - return uniqueEmails; -} - -/** - * Ensure the number of unique emails in an input does not exceed the allowed maximum. - * @param organization An object representing the organization - * @param getErrorMessage A callback function that generates the error message. It takes the `maxEmailsCount` as a parameter. - * @returns A function that validates an `AbstractControl` and returns `ValidationErrors` or `null` - */ -export function inputEmailLimitValidator( - organization: Organization, - getErrorMessage: (maxEmailsCount: number) => string, -): ValidatorFn { - return (control: AbstractControl): ValidationErrors | null => { - if (control.value === "" || !control.value) { - return null; - } - - const maxEmailsCount = organization.productTierType === ProductTierType.TeamsStarter ? 10 : 20; - - const uniqueEmails = getUniqueInputEmails(control); - - if (uniqueEmails.length <= maxEmailsCount) { - return null; - } - - return { tooManyEmails: { message: getErrorMessage(maxEmailsCount) } }; - }; -} From d9d4b251a817dad98e89bacb198c5af4213a7358 Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Thu, 12 Dec 2024 11:44:47 -0500 Subject: [PATCH 5/6] Addressing code review 3 --- .../member-dialog/member-dialog.component.ts | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts index eac4bdbe0f3..51a35baa7c3 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.ts @@ -273,9 +273,12 @@ export class MemberDialogComponent implements OnDestroy { } private setFormValidators(organization: Organization) { - const _orgSeatLimitReachedValidator = [ + const emailsControlValidators = [ Validators.required, commaSeparatedEmails, + inputEmailLimitValidator(organization, (maxEmailsCount: number) => + this.i18nService.t("tooManyEmails", maxEmailsCount), + ), orgSeatLimitReachedValidator( organization, this.params.allOrganizationUserEmails, @@ -283,17 +286,8 @@ export class MemberDialogComponent implements OnDestroy { ), ]; - const _inputEmailLimitValidator = [ - Validators.required, - commaSeparatedEmails, - inputEmailLimitValidator(organization, (maxEmailsCount: number) => - this.i18nService.t("tooManyEmails", maxEmailsCount), - ), - ]; - const emailsControl = this.formGroup.get("emails"); - emailsControl.setValidators(_orgSeatLimitReachedValidator); - emailsControl.setValidators(_inputEmailLimitValidator); + emailsControl.setValidators(emailsControlValidators); emailsControl.updateValueAndValidity(); } From bb59fee040846b6b8822424e1693943007677763 Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Wed, 18 Dec 2024 10:39:24 -0500 Subject: [PATCH 6/6] [PM-13755] Use active user count for seat validation (#12228) --- .../member-dialog.component.html | 15 +- .../member-dialog/member-dialog.component.ts | 72 +++++--- .../org-seat-limit-reached.validator.spec.ts | 173 ++++++++++++------ .../org-seat-limit-reached.validator.ts | 77 +++++--- .../members/members.component.ts | 27 ++- .../member-access-report.component.ts | 9 +- 6 files changed, 242 insertions(+), 131 deletions(-) diff --git a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.html b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.html index c77c8fc935f..9b4c0f4290b 100644 --- a/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.html +++ b/apps/web/src/app/admin-console/organizations/members/components/member-dialog/member-dialog.component.html @@ -2,9 +2,11 @@ {{ title }} - {{ - params.name - }} + {{ params.name }} {{ "revoked" | i18n }}
@@ -263,7 +265,8 @@