From 4df84ff3d24699ceafcbb1b4043acca9d3efdf15 Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Tue, 10 Dec 2024 11:18:34 -0500 Subject: [PATCH] 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; } }