[PM-28422] Client batching for member actions (#17805)

* add member action batching, feature flag, and implement batching for reinvite * clean up, fix tests, remove redundant tests * cleanup * clean up tests * bump cloud limit to 8k
2025-12-19 09:43:23 +00:00 · 2025-12-11 10:30:05 -05:00
parent dc763f6291
commit 50dff4e032
4 changed files with 378 additions and 58 deletions
--- 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
@@ -24,7 +24,7 @@ export const MaxCheckedCount = 500;
 * Maximum for bulk reinvite operations when the IncreaseBulkReinviteLimitForCloud
 * feature flag is enabled on cloud environments.
 */
-export const CloudBulkReinviteLimit = 4000;
+export const CloudBulkReinviteLimit = 8000;

 /**
 * Returns true if the user matches the status, or where the status is `null`, if the user is active (not revoked).
--- a/apps/web/src/app/admin-console/organizations/members/members.component.ts
+++ b/apps/web/src/app/admin-console/organizations/members/members.component.ts
@@ -443,7 +443,7 @@ export class MembersComponent extends BaseMembersComponent<OrganizationUserView>
    try {
      const result = await this.memberActionsService.bulkReinvite(
        organization,
-        filteredUsers.map((user) => user.id),
+        filteredUsers.map((user) => user.id as UserId),
      );

      if (!result.successful) {
--- a/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.spec.ts
+++ b/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.spec.ts
@@ -4,6 +4,7 @@ import { of } from "rxjs";
 import {
  OrganizationUserApiService,
  OrganizationUserBulkResponse,
+  OrganizationUserService,
 } from "@bitwarden/admin-console/common";
 import {
  OrganizationUserType,
@@ -22,9 +23,8 @@ import { newGuid } from "@bitwarden/guid";
 import { KeyService } from "@bitwarden/key-management";

 import { OrganizationUserView } from "../../../core/views/organization-user.view";
-import { OrganizationUserService } from "../organization-user/organization-user.service";

-import { MemberActionsService } from "./member-actions.service";
+import { REQUESTS_PER_BATCH, MemberActionsService } from "./member-actions.service";

 describe("MemberActionsService", () => {
  let service: MemberActionsService;
@@ -308,41 +308,308 @@ describe("MemberActionsService", () => {
  });

  describe("bulkReinvite", () => {
-    const userIds = [newGuid(), newGuid(), newGuid()];
+    const userIds = [newGuid() as UserId, newGuid() as UserId, newGuid() as UserId];

-    it("should successfully reinvite multiple users", async () => {
-      const mockResponse = {
-        data: userIds.map((id) => ({
-          id,
-          error: null,
-        })),
-        continuationToken: null,
-      } as ListResponse<OrganizationUserBulkResponse>;
-      organizationUserApiService.postManyOrganizationUserReinvite.mockResolvedValue(mockResponse);
-
-      const result = await service.bulkReinvite(mockOrganization, userIds);
-
-      expect(result).toEqual({
-        successful: mockResponse,
-        failed: [],
+    describe("when feature flag is false", () => {
+      beforeEach(() => {
+        configService.getFeatureFlag$.mockReturnValue(of(false));
+      });
+
+      it("should successfully reinvite multiple users", async () => {
+        const mockResponse = new ListResponse(
+          {
+            data: userIds.map((id) => ({
+              id,
+              error: null,
+            })),
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );
+
+        organizationUserApiService.postManyOrganizationUserReinvite.mockResolvedValue(mockResponse);
+
+        const result = await service.bulkReinvite(mockOrganization, userIds);
+
+        expect(result.failed).toEqual([]);
+        expect(result.successful).toBeDefined();
+        expect(result.successful).toEqual(mockResponse);
+        expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledWith(
+          organizationId,
+          userIds,
+        );
+      });
+
+      it("should handle bulk reinvite errors", async () => {
+        const errorMessage = "Bulk reinvite failed";
+        organizationUserApiService.postManyOrganizationUserReinvite.mockRejectedValue(
+          new Error(errorMessage),
+        );
+
+        const result = await service.bulkReinvite(mockOrganization, userIds);
+
+        expect(result.successful).toBeUndefined();
+        expect(result.failed).toHaveLength(3);
+        expect(result.failed[0]).toEqual({ id: userIds[0], error: errorMessage });
      });
-      expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledWith(
-        organizationId,
-        userIds,
-      );
    });

-    it("should handle bulk reinvite errors", async () => {
-      const errorMessage = "Bulk reinvite failed";
-      organizationUserApiService.postManyOrganizationUserReinvite.mockRejectedValue(
-        new Error(errorMessage),
-      );
+    describe("when feature flag is true (batching behavior)", () => {
+      beforeEach(() => {
+        configService.getFeatureFlag$.mockReturnValue(of(true));
+      });
+      it("should process users in a single batch when count equals REQUESTS_PER_BATCH", async () => {
+        const userIdsBatch = Array.from({ length: REQUESTS_PER_BATCH }, () => newGuid() as UserId);
+        const mockResponse = new ListResponse(
+          {
+            data: userIdsBatch.map((id) => ({
+              id,
+              error: null,
+            })),
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );

-      const result = await service.bulkReinvite(mockOrganization, userIds);
+        organizationUserApiService.postManyOrganizationUserReinvite.mockResolvedValue(mockResponse);

-      expect(result.successful).toBeUndefined();
-      expect(result.failed).toHaveLength(3);
-      expect(result.failed[0]).toEqual({ id: userIds[0], error: errorMessage });
+        const result = await service.bulkReinvite(mockOrganization, userIdsBatch);
+
+        expect(result.successful).toBeDefined();
+        expect(result.successful?.response).toHaveLength(REQUESTS_PER_BATCH);
+        expect(result.failed).toHaveLength(0);
+        expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledTimes(
+          1,
+        );
+        expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledWith(
+          organizationId,
+          userIdsBatch,
+        );
+      });
+
+      it("should process users in multiple batches when count exceeds REQUESTS_PER_BATCH", async () => {
+        const totalUsers = REQUESTS_PER_BATCH + 100;
+        const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId);
+
+        const mockResponse1 = new ListResponse(
+          {
+            data: userIdsBatch.slice(0, REQUESTS_PER_BATCH).map((id) => ({
+              id,
+              error: null,
+            })),
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );
+
+        const mockResponse2 = new ListResponse(
+          {
+            data: userIdsBatch.slice(REQUESTS_PER_BATCH).map((id) => ({
+              id,
+              error: null,
+            })),
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );
+
+        organizationUserApiService.postManyOrganizationUserReinvite
+          .mockResolvedValueOnce(mockResponse1)
+          .mockResolvedValueOnce(mockResponse2);
+
+        const result = await service.bulkReinvite(mockOrganization, userIdsBatch);
+
+        expect(result.successful).toBeDefined();
+        expect(result.successful?.response).toHaveLength(totalUsers);
+        expect(result.failed).toHaveLength(0);
+        expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledTimes(
+          2,
+        );
+        expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenNthCalledWith(
+          1,
+          organizationId,
+          userIdsBatch.slice(0, REQUESTS_PER_BATCH),
+        );
+        expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenNthCalledWith(
+          2,
+          organizationId,
+          userIdsBatch.slice(REQUESTS_PER_BATCH),
+        );
+      });
+
+      it("should aggregate results across multiple successful batches", async () => {
+        const totalUsers = REQUESTS_PER_BATCH + 50;
+        const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId);
+
+        const mockResponse1 = new ListResponse(
+          {
+            data: userIdsBatch.slice(0, REQUESTS_PER_BATCH).map((id) => ({
+              id,
+              error: null,
+            })),
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );
+
+        const mockResponse2 = new ListResponse(
+          {
+            data: userIdsBatch.slice(REQUESTS_PER_BATCH).map((id) => ({
+              id,
+              error: null,
+            })),
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );
+
+        organizationUserApiService.postManyOrganizationUserReinvite
+          .mockResolvedValueOnce(mockResponse1)
+          .mockResolvedValueOnce(mockResponse2);
+
+        const result = await service.bulkReinvite(mockOrganization, userIdsBatch);
+
+        expect(result.successful).toBeDefined();
+        expect(result.successful?.response).toHaveLength(totalUsers);
+        expect(result.successful?.response.slice(0, REQUESTS_PER_BATCH)).toEqual(
+          mockResponse1.data,
+        );
+        expect(result.successful?.response.slice(REQUESTS_PER_BATCH)).toEqual(mockResponse2.data);
+        expect(result.failed).toHaveLength(0);
+      });
+
+      it("should handle mixed individual errors across multiple batches", async () => {
+        const totalUsers = REQUESTS_PER_BATCH + 4;
+        const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId);
+
+        const mockResponse1 = new ListResponse(
+          {
+            data: userIdsBatch.slice(0, REQUESTS_PER_BATCH).map((id, index) => ({
+              id,
+              error: index % 10 === 0 ? "Rate limit exceeded" : null,
+            })),
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );
+
+        const mockResponse2 = new ListResponse(
+          {
+            data: [
+              { id: userIdsBatch[REQUESTS_PER_BATCH], error: null },
+              { id: userIdsBatch[REQUESTS_PER_BATCH + 1], error: "Invalid email" },
+              { id: userIdsBatch[REQUESTS_PER_BATCH + 2], error: null },
+              { id: userIdsBatch[REQUESTS_PER_BATCH + 3], error: "User suspended" },
+            ],
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );
+
+        organizationUserApiService.postManyOrganizationUserReinvite
+          .mockResolvedValueOnce(mockResponse1)
+          .mockResolvedValueOnce(mockResponse2);
+
+        const result = await service.bulkReinvite(mockOrganization, userIdsBatch);
+
+        // Count expected failures: every 10th index (0, 10, 20, ..., 490) in first batch + 2 explicit in second batch
+        // Indices 0 to REQUESTS_PER_BATCH-1 where index % 10 === 0: that's floor((BATCH_SIZE-1)/10) + 1 values
+        const expectedFailuresInBatch1 = Math.floor((REQUESTS_PER_BATCH - 1) / 10) + 1;
+        const expectedFailuresInBatch2 = 2;
+        const expectedTotalFailures = expectedFailuresInBatch1 + expectedFailuresInBatch2;
+        const expectedSuccesses = totalUsers - expectedTotalFailures;
+
+        expect(result.successful).toBeDefined();
+        expect(result.successful?.response).toHaveLength(expectedSuccesses);
+        expect(result.failed).toHaveLength(expectedTotalFailures);
+        expect(result.failed.some((f) => f.error === "Rate limit exceeded")).toBe(true);
+        expect(result.failed.some((f) => f.error === "Invalid email")).toBe(true);
+        expect(result.failed.some((f) => f.error === "User suspended")).toBe(true);
+      });
+
+      it("should aggregate all failures when all batches fail", async () => {
+        const totalUsers = REQUESTS_PER_BATCH + 100;
+        const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId);
+        const errorMessage = "All batches failed";
+
+        organizationUserApiService.postManyOrganizationUserReinvite.mockRejectedValue(
+          new Error(errorMessage),
+        );
+
+        const result = await service.bulkReinvite(mockOrganization, userIdsBatch);
+
+        expect(result.successful).toBeUndefined();
+        expect(result.failed).toHaveLength(totalUsers);
+        expect(result.failed.every((f) => f.error === errorMessage)).toBe(true);
+        expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledTimes(
+          2,
+        );
+      });
+
+      it("should handle empty data in batch response", async () => {
+        const totalUsers = REQUESTS_PER_BATCH + 50;
+        const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId);
+
+        const mockResponse1 = new ListResponse(
+          {
+            data: userIdsBatch.slice(0, REQUESTS_PER_BATCH).map((id) => ({
+              id,
+              error: null,
+            })),
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );
+
+        const mockResponse2 = new ListResponse(
+          {
+            data: [],
+            continuationToken: null,
+          },
+          OrganizationUserBulkResponse,
+        );
+
+        organizationUserApiService.postManyOrganizationUserReinvite
+          .mockResolvedValueOnce(mockResponse1)
+          .mockResolvedValueOnce(mockResponse2);
+
+        const result = await service.bulkReinvite(mockOrganization, userIdsBatch);
+
+        expect(result.successful).toBeDefined();
+        expect(result.successful?.response).toHaveLength(REQUESTS_PER_BATCH);
+        expect(result.failed).toHaveLength(0);
+      });
+
+      it("should process batches sequentially in order", async () => {
+        const totalUsers = REQUESTS_PER_BATCH * 2;
+        const userIdsBatch = Array.from({ length: totalUsers }, () => newGuid() as UserId);
+        const callOrder: number[] = [];
+
+        organizationUserApiService.postManyOrganizationUserReinvite.mockImplementation(
+          async (orgId, ids) => {
+            const batchIndex = ids.includes(userIdsBatch[0]) ? 1 : 2;
+            callOrder.push(batchIndex);
+
+            return new ListResponse(
+              {
+                data: ids.map((id) => ({
+                  id,
+                  error: null,
+                })),
+                continuationToken: null,
+              },
+              OrganizationUserBulkResponse,
+            );
+          },
+        );
+
+        await service.bulkReinvite(mockOrganization, userIdsBatch);
+
+        expect(callOrder).toEqual([1, 2]);
+        expect(organizationUserApiService.postManyOrganizationUserReinvite).toHaveBeenCalledTimes(
+          2,
+        );
+      });
    });
  });

@@ -427,14 +694,6 @@ describe("MemberActionsService", () => {
      expect(result).toBe(false);
    });

-    it("should not allow reset password when organization lacks public and private keys", () => {
-      const org = { ...mockOrganization, hasPublicAndPrivateKeys: false } as Organization;
-
-      const result = service.allowResetPassword(mockOrgUser, org, resetPasswordEnabled);
-
-      expect(result).toBe(false);
-    });
-
    it("should not allow reset password when user is not enrolled in reset password", () => {
      const user = { ...mockOrgUser, resetPasswordEnrolled: false } as OrganizationUserView;

@@ -443,12 +702,6 @@ describe("MemberActionsService", () => {
      expect(result).toBe(false);
    });

-    it("should not allow reset password when reset password is disabled", () => {
-      const result = service.allowResetPassword(mockOrgUser, mockOrganization, false);
-
-      expect(result).toBe(false);
-    });
-
    it("should not allow reset password when user status is not confirmed", () => {
      const user = {
        ...mockOrgUser,
--- a/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.ts
+++ b/apps/web/src/app/admin-console/organizations/members/services/member-actions/member-actions.service.ts
@@ -20,9 +20,12 @@ import { EncryptService } from "@bitwarden/common/key-management/crypto/abstract
 import { ListResponse } from "@bitwarden/common/models/response/list.response";
 import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service";
 import { KeyService } from "@bitwarden/key-management";
+import { UserId } from "@bitwarden/user-core";

 import { OrganizationUserView } from "../../../core/views/organization-user.view";

+export const REQUESTS_PER_BATCH = 500;
+
 export interface MemberActionResult {
  success: boolean;
  error?: string;
@@ -162,20 +165,36 @@ export class MemberActionsService {
    }
  }

-  async bulkReinvite(organization: Organization, userIds: string[]): Promise<BulkActionResult> {
-    try {
-      const result = await this.organizationUserApiService.postManyOrganizationUserReinvite(
-        organization.id,
-        userIds,
-      );
-      return { successful: result, failed: [] };
-    } catch (error) {
-      return {
-        failed: userIds.map((id) => ({ id, error: (error as Error).message ?? String(error) })),
-      };
+  async bulkReinvite(organization: Organization, userIds: UserId[]): Promise<BulkActionResult> {
+    const increaseBulkReinviteLimitForCloud = await firstValueFrom(
+      this.configService.getFeatureFlag$(FeatureFlag.IncreaseBulkReinviteLimitForCloud),
+    );
+    if (increaseBulkReinviteLimitForCloud) {
+      return await this.vNextBulkReinvite(organization, userIds);
+    } else {
+      try {
+        const result = await this.organizationUserApiService.postManyOrganizationUserReinvite(
+          organization.id,
+          userIds,
+        );
+        return { successful: result, failed: [] };
+      } catch (error) {
+        return {
+          failed: userIds.map((id) => ({ id, error: (error as Error).message ?? String(error) })),
+        };
+      }
    }
  }

+  async vNextBulkReinvite(
+    organization: Organization,
+    userIds: UserId[],
+  ): Promise<BulkActionResult> {
+    return this.processBatchedOperation(userIds, REQUESTS_PER_BATCH, (batch) =>
+      this.organizationUserApiService.postManyOrganizationUserReinvite(organization.id, batch),
+    );
+  }
+
  allowResetPassword(
    orgUser: OrganizationUserView,
    organization: Organization,
@@ -207,4 +226,52 @@ export class MemberActionsService {
      orgUser.status === OrganizationUserStatusType.Confirmed
    );
  }
+
+  /**
+   * Processes user IDs in sequential batches and aggregates results.
+   * @param userIds - Array of user IDs to process
+   * @param batchSize - Number of IDs to process per batch
+   * @param processBatch - Async function that processes a single batch and returns the result
+   * @returns Aggregated bulk action result
+   */
+  private async processBatchedOperation(
+    userIds: UserId[],
+    batchSize: number,
+    processBatch: (batch: string[]) => Promise<ListResponse<OrganizationUserBulkResponse>>,
+  ): Promise<BulkActionResult> {
+    const allSuccessful: OrganizationUserBulkResponse[] = [];
+    const allFailed: { id: string; error: string }[] = [];
+
+    for (let i = 0; i < userIds.length; i += batchSize) {
+      const batch = userIds.slice(i, i + batchSize);
+
+      try {
+        const result = await processBatch(batch);
+
+        if (result?.data) {
+          for (const response of result.data) {
+            if (response.error) {
+              allFailed.push({ id: response.id, error: response.error });
+            } else {
+              allSuccessful.push(response);
+            }
+          }
+        }
+      } catch (error) {
+        allFailed.push(
+          ...batch.map((id) => ({ id, error: (error as Error).message ?? String(error) })),
+        );
+      }
+    }
+
+    const successful =
+      allSuccessful.length > 0
+        ? new ListResponse(allSuccessful, OrganizationUserBulkResponse)
+        : undefined;
+
+    return {
+      successful,
+      failed: allFailed,
+    };
+  }
 }