Address code review feedback

apps/web/src/app/admin-console/common/people-table-data-source.ts

@@ -77,10 +77,6 @@ export abstract class PeopleTableDataSource<T extends UserViewTypes> 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)

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,

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<Organization> = {}) =>
     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],

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 {

apps/web/src/app/admin-console/organizations/members/members.component.ts

@@ -107,6 +107,10 @@ export class MembersComponent extends BaseMembersComponent<OrganizationUserView>
   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<OrganizationUserView>
         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<OrganizationUserView>
   }
 
   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<OrganizationUserView>
     }
 
     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<OrganizationUserView>
         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,

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,