From 1c5f2262f4aefd9bf20ba414ba5576574e270bc9 Mon Sep 17 00:00:00 2001 From: Jimmy Vo Date: Fri, 13 Dec 2024 15:58:00 -0500 Subject: [PATCH] Address code review feedback --- .../common/people-table-data-source.ts | 4 - .../member-dialog/member-dialog.component.ts | 32 ++-- .../org-seat-limit-reached.validator.spec.ts | 174 +++++++++--------- .../org-seat-limit-reached.validator.ts | 9 +- .../members/members.component.ts | 16 +- .../member-access-report.component.ts | 3 - 6 files changed, 115 insertions(+), 123 deletions(-) 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 index 9a9942d7e6c..c1ece833d83 100644 --- 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 @@ -77,10 +77,6 @@ export abstract class PeopleTableDataSource extends Tab return super.data; } - get occupiedSeatCount(): number { - return this.activeUserCount; - } - /** * Check or uncheck a user in the table * @param select check the user (true), uncheck the user (false), or toggle the current state (null) 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 87ddf11ac16..08c62cd1c86 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 @@ -61,26 +61,24 @@ export enum MemberDialogTab { Collections = 2, } -export interface AddMemberDialogParams { - kind: "Add"; - organizationId: string; - occupiedSeatCount: number; - allOrganizationUserEmails: string[]; +interface CommonMemberDialogParams { isOnSecretsManagerStandalone: boolean; - initialTab: MemberDialogTab; + organizationId: string; } -export interface EditMemberDialogParams { +export interface AddMemberDialogParams extends CommonMemberDialogParams { + kind: "Add"; + occupiedSeatCount: number; + allOrganizationUserEmails: string[]; +} + +export interface EditMemberDialogParams extends CommonMemberDialogParams { kind: "Edit"; name: string; - organizationId: string; organizationUserId: string; - allOrganizationUserEmails: string[]; usesKeyConnector: boolean; - occupiedSeatCount: number; - isOnSecretsManagerStandalone: boolean; - initialTab: MemberDialogTab; managedByOrganization?: boolean; + initialTab: MemberDialogTab; } export type MemberDialogParams = EditMemberDialogParams | AddMemberDialogParams; @@ -155,7 +153,7 @@ export class MemberDialogComponent implements OnDestroy { isEditDialogParams( params: EditMemberDialogParams | AddMemberDialogParams, ): params is EditMemberDialogParams { - return (params as EditMemberDialogParams).organizationUserId !== undefined; + return this.editMode; } constructor( @@ -187,14 +185,14 @@ export class MemberDialogComponent implements OnDestroy { this.params.organizationId, this.params.organizationUserId, ); + this.tabIndex = this.params.initialTab; } else { this.editMode = false; this.title = this.i18nService.t("inviteMember"); userDetails$ = of(null); + this.tabIndex = MemberDialogTab.Role; } - this.tabIndex = this.params.initialTab ?? MemberDialogTab.Role; - this.isOnSecretsManagerStandalone = this.params.isOnSecretsManagerStandalone; if (this.isOnSecretsManagerStandalone) { @@ -292,6 +290,10 @@ export class MemberDialogComponent implements OnDestroy { } private setFormValidators(organization: Organization) { + if (this.isEditDialogParams(this.params)) { + return; + } + const emailsControlValidators = [ Validators.required, commaSeparatedEmails, 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 5157851e52d..c975ae5ee5a 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 @@ -21,17 +21,35 @@ const orgFactory = (props: Partial = {}) => props, ); +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("orgSeatLimitReachedValidator", () => { - const allOrganizationUserEmails = ["user1@example.com"]; - const dummyOrg: Organization = null; - const dummyOccupiedSeatCount = 1; + let organization: Organization; + let allOrganizationUserEmails: string[]; + let occupiedSeatCount: number; + + beforeEach(() => { + allOrganizationUserEmails = [createUniqueEmailString(1)]; + occupiedSeatCount = 1; + organization = null; + }); it("should return null when control value is empty", () => { const validatorFn = orgSeatLimitReachedValidator( - dummyOrg, + organization, allOrganizationUserEmails, "You cannot invite more than 2 members without upgrading your plan.", - dummyOccupiedSeatCount, + occupiedSeatCount, ); const control = new FormControl(""); @@ -42,10 +60,10 @@ describe("orgSeatLimitReachedValidator", () => { it("should return null when control value is null", () => { const validatorFn = orgSeatLimitReachedValidator( - dummyOrg, + organization, allOrganizationUserEmails, "You cannot invite more than 2 members without upgrading your plan.", - dummyOccupiedSeatCount, + occupiedSeatCount, ); const control = new FormControl(null); @@ -54,83 +72,8 @@ describe("orgSeatLimitReachedValidator", () => { expect(result).toBeNull(); }); - it("should return null when max seats are not exceeded on free plan", () => { - const organization = orgFactory({ - productTierType: ProductTierType.Free, - seats: 2, - }); - - const OccupiedSeatCount = 1; - - const validatorFn = orgSeatLimitReachedValidator( - organization, - allOrganizationUserEmails, - "You cannot invite more than 2 members without upgrading your plan.", - OccupiedSeatCount, - ); - const control = new FormControl("user2@example.com"); - - const result = validatorFn(control); - - expect(result).toBeNull(); - }); - - it("should return null when max seats are not exceeded on teams starter plan", () => { - const organization = orgFactory({ - productTierType: ProductTierType.TeamsStarter, - seats: 10, - }); - - const occupiedSeatCount = 0; - - const validatorFn = orgSeatLimitReachedValidator( - organization, - allOrganizationUserEmails, - "You cannot invite more than 10 members without upgrading your plan.", - occupiedSeatCount, - ); - - const control = new FormControl( - "user2@example.com," + - "user3@example.com," + - "user4@example.com," + - "user5@example.com," + - "user6@example.com," + - "user7@example.com," + - "user8@example.com," + - "user9@example.com," + - "user10@example.com", - ); - - const result = validatorFn(control); - - expect(result).toBeNull(); - }); - - it("should return validation error when max seats are exceeded on free plan", () => { - const organization = orgFactory({ - productTierType: ProductTierType.Free, - seats: 2, - }); - const errorMessage = "You cannot invite more than 2 members without upgrading your plan."; - - const occupiedSeatCount = 1; - - const validatorFn = orgSeatLimitReachedValidator( - organization, - allOrganizationUserEmails, - "You cannot invite more than 2 members without upgrading your plan.", - occupiedSeatCount, - ); - const control = new FormControl("user2@example.com,user3@example.com"); - - const result = validatorFn(control); - - expect(result).toStrictEqual({ seatLimitReached: { message: errorMessage } }); - }); - it("should return null when on dynamic seat plan", () => { - const control = new FormControl("user2@example.com,user3@example.com"); + const control = new FormControl(createUniqueEmailString(1)); const organization = orgFactory({ productTierType: ProductTierType.Enterprise, seats: 100, @@ -140,7 +83,7 @@ describe("orgSeatLimitReachedValidator", () => { organization, allOrganizationUserEmails, "Enterprise plan dummy error.", - dummyOccupiedSeatCount, + occupiedSeatCount, ); const result = validatorFn(control); @@ -149,9 +92,9 @@ describe("orgSeatLimitReachedValidator", () => { }); it("should only count unique input email addresses", () => { - const control = new FormControl( - "sameUser1@example.com,sameUser1@example.com,user2@example.com,user3@example.com", - ); + const twoUniqueEmails = createUniqueEmailString(2); + const sixDuplicateEmails = createIdenticalEmailString(6); + const control = new FormControl(twoUniqueEmails + sixDuplicateEmails); const organization = orgFactory({ productTierType: ProductTierType.Families, seats: 6, @@ -169,6 +112,62 @@ describe("orgSeatLimitReachedValidator", () => { expect(result).toBeNull(); }); + + describe("when total occupied seat count is below plan's max count", () => { + test.each([ + [ProductTierType.Free, 2], + [ProductTierType.Families, 6], + [ProductTierType.TeamsStarter, 10], + ])(`should return null on plan %s`, (plan, planSeatCount) => { + const organization = orgFactory({ + productTierType: plan, + seats: planSeatCount, + }); + + const occupiedSeatCount = 0; + + const validatorFn = orgSeatLimitReachedValidator( + organization, + allOrganizationUserEmails, + "Generic error message", + occupiedSeatCount, + ); + + const control = new FormControl(createUniqueEmailString(1)); + + const result = validatorFn(control); + + expect(result).toBeNull(); + }); + }); + + describe("when total occupied seat count is at plan's max count", () => { + test.each([ + [ProductTierType.Free, 2, 1], + [ProductTierType.Families, 6, 5], + [ProductTierType.TeamsStarter, 10, 9], + ])(`should return null on plan %s`, (plan, planSeatCount, newEmailCount) => { + const organization = orgFactory({ + productTierType: plan, + seats: planSeatCount, + }); + + const occupiedSeatCount = 1; + + const validatorFn = orgSeatLimitReachedValidator( + organization, + allOrganizationUserEmails, + "Generic error message", + occupiedSeatCount, + ); + + const control = new FormControl(createUniqueEmailString(newEmailCount)); + + const result = validatorFn(control); + + expect(result).toBeNull(); + }); + }); }); describe("isFixedSeatPlan", () => { @@ -187,6 +186,7 @@ describe("isFixedSeatPlan", () => { describe("isDynamicSeatPlan", () => { test.each([ [ProductTierType.Enterprise, true], + [ProductTierType.Teams, true], [ProductTierType.Free, false], [ProductTierType.Families, false], [ProductTierType.TeamsStarter, false], 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 bbe9c5c47e0..5b94f591df6 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 @@ -39,12 +39,11 @@ export function orgSeatLimitReachedValidator( } export function isDynamicSeatPlan(productTierType: ProductTierType): boolean { - switch (productTierType) { - case ProductTierType.Enterprise: - return true; - default: - return false; + if (!productTierType) { + return false; } + + return !isFixedSeatPlan(productTierType); } export function isFixedSeatPlan(productTierType: ProductTierType): boolean { 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 d14a634c127..3d9eb32ec2b 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 @@ -107,6 +107,10 @@ export class MembersComponent extends BaseMembersComponent protected rowHeight = 69; protected rowHeightClass = `tw-h-[69px]`; + get occupiedSeatCount(): number { + return this.dataSource.acceptedUserCount; + } + constructor( apiService: ApiService, i18nService: I18nService, @@ -471,9 +475,8 @@ export class MembersComponent extends BaseMembersComponent kind: "Add", organizationId: this.organization.id, allOrganizationUserEmails: this.dataSource.data?.map((user) => user.email) ?? [], - occupiedSeatCount: this.dataSource.occupiedSeatCount, + occupiedSeatCount: this.occupiedSeatCount, isOnSecretsManagerStandalone: this.orgIsOnSecretsManagerStandalone, - initialTab: MemberDialogTab.Role, }, }); @@ -506,10 +509,7 @@ export class MembersComponent extends BaseMembersComponent } async invite() { - if ( - this.organization.hasReseller && - this.organization.seats === this.dataSource.occupiedSeatCount - ) { + if (this.organization.hasReseller && this.organization.seats === this.occupiedSeatCount) { this.toastService.showToast({ variant: "error", title: this.i18nService.t("seatLimitReached"), @@ -520,7 +520,7 @@ export class MembersComponent extends BaseMembersComponent } if ( - this.dataSource.occupiedSeatCount === this.organization.seats && + this.occupiedSeatCount === this.organization.seats && isFixedSeatPlan(this.organization.productTierType) ) { await this.handleSeatLimitForFixedTiers(); @@ -538,8 +538,6 @@ export class MembersComponent extends BaseMembersComponent name: this.userNamePipe.transform(user), organizationId: this.organization.id, organizationUserId: user.id, - occupiedSeatCount: this.dataSource.occupiedSeatCount, - allOrganizationUserEmails: this.dataSource.data?.map((user) => user.email) ?? [], usesKeyConnector: user.usesKeyConnector, isOnSecretsManagerStandalone: this.orgIsOnSecretsManagerStandalone, initialTab: initialTab, diff --git a/bitwarden_license/bit-web/src/app/tools/reports/member-access-report/member-access-report.component.ts b/bitwarden_license/bit-web/src/app/tools/reports/member-access-report/member-access-report.component.ts index 40e944544a1..52ec2901031 100644 --- a/bitwarden_license/bit-web/src/app/tools/reports/member-access-report/member-access-report.component.ts +++ b/bitwarden_license/bit-web/src/app/tools/reports/member-access-report/member-access-report.component.ts @@ -99,10 +99,7 @@ export class MemberAccessReportComponent implements OnInit { kind: "Edit", name: this.userNamePipe.transform(user), organizationId: this.organizationId, - // This occupiedSeatCount is not needed for path. - occupiedSeatCount: 0, organizationUserId: user.userGuid, - allOrganizationUserEmails: this.dataSource.data?.map((user) => user.email) ?? [], usesKeyConnector: user.usesKeyConnector, isOnSecretsManagerStandalone: this.orgIsOnSecretsManagerStandalone, initialTab: MemberDialogTab.Role,