[Workspace]Display duplicate collaborators error in add collaborators…

… modal (opensearch-project#8531) * Show Duplicate collaborators error Signed-off-by: Lin Wang <wonglam@amazon.com> * Changeset file for PR opensearch-project#8531 created/updated * Combine with pendingAdded and existing error details Signed-off-by: Lin Wang <wonglam@amazon.com> * Use separate error message for duplicate collaborators with list Signed-off-by: Lin Wang <wonglam@amazon.com> --------- Signed-off-by: Lin Wang <wonglam@amazon.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
RyanL1997 · Oct 15, 2024 · 64ed61b · 64ed61b
1 parent 4334ecf
commit 64ed61b
Show file tree

Hide file tree

Showing 11 changed files with 228 additions and 11 deletions.
diff --git a/changelogs/fragments/8531.yml b/changelogs/fragments/8531.yml
@@ -0,0 +1,2 @@
+feat:
+- [Workspace]Display duplicate collaborators error in add collaborators modal ([#8531](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8531))
diff --git a/...gins/workspace/public/components/add_collaborators_modal/add_collaborators_modal.test.tsx b/...gins/workspace/public/components/add_collaborators_modal/add_collaborators_modal.test.tsx
@@ -6,6 +6,7 @@
 import React from 'react';
 import { render, screen, fireEvent, waitFor } from '@testing-library/react';
 import { AddCollaboratorsModal } from './add_collaborators_modal';
+import { DuplicateCollaboratorError } from './duplicate_collaborator_error';
 
 describe('AddCollaboratorsModal', () => {
   const defaultProps = {
@@ -77,6 +78,30 @@ describe('AddCollaboratorsModal', () => {
     expect(screen.getByText('Learn more in Documentation')).toBeInTheDocument();
   });
 
+  it('should display consistent duplicate collaborators errors', async () => {
+    const mockOnAddCollaborators = () => {
+      throw new DuplicateCollaboratorError({ pendingAdded: ['user1'], existing: ['user2'] });
+    };
+    render(<AddCollaboratorsModal {...defaultProps} onAddCollaborators={mockOnAddCollaborators} />);
+    const collaboratorIdInput0 = screen.getByTestId('workspaceCollaboratorIdInput-0');
+    fireEvent.change(collaboratorIdInput0, { target: { value: 'user1' } });
+
+    fireEvent.click(screen.getByText('Add Another'));
+    const collaboratorIdInput1 = screen.getByTestId('workspaceCollaboratorIdInput-1');
+    fireEvent.change(collaboratorIdInput1, { target: { value: 'user1' } });
+
+    fireEvent.click(screen.getByText('Add Another'));
+    const collaboratorIdInput2 = screen.getByTestId('workspaceCollaboratorIdInput-2');
+    fireEvent.change(collaboratorIdInput2, { target: { value: 'user2' } });
+
+    const addCollaboratorsButton = screen.getByRole('button', { name: 'Add collaborators' });
+    fireEvent.click(addCollaboratorsButton);
+    await waitFor(() => {
+      expect(screen.getByText('This ID is already added to the list.')).toBeInTheDocument();
+      expect(screen.getByText('A collaborator with this ID already exists.')).toBeInTheDocument();
+    });
+  });
+
   it('should disable "Add collaborators" button during onAddCollaborators execution', async () => {
     const onAddCollaboratorsMock = jest.fn().mockReturnValue(
       new Promise((resolve) => {

diff --git a/src/plugins/workspace/public/components/add_collaborators_modal/add_collaborators_modal.tsx b/src/plugins/workspace/public/components/add_collaborators_modal/add_collaborators_modal.tsx
@@ -25,6 +25,21 @@ import {
   WorkspaceCollaboratorsPanel,
   WorkspaceCollaboratorInner,
 } from './workspace_collaborators_panel';
+import { DuplicateCollaboratorError } from './duplicate_collaborator_error';
+
+const DUPLICATE_COLLABORATOR_ERROR_MESSAGE = i18n.translate(
+  'workspace.addCollaboratorsModal.errors.duplicateCollaborator',
+  {
+    defaultMessage: 'A collaborator with this ID already exists.',
+  }
+);
+
+const DUPLICATE_COLLABORATOR_WITH_LIST_ERROR_MESSAGE = i18n.translate(
+  'workspace.addCollaboratorsModal.errors.duplicateCollaboratorWithList',
+  {
+    defaultMessage: 'This ID is already added to the list.',
+  }
+);
 
 export interface AddCollaboratorsModalProps {
   title: string;
@@ -58,18 +73,57 @@ export const AddCollaboratorsModal = ({
   const [collaborators, setCollaborators] = useState<WorkspaceCollaboratorInner[]>([
     { id: 0, accessLevel: 'readOnly', collaboratorId: '' },
   ]);
-  const validCollaborators = collaborators.flatMap(({ collaboratorId, accessLevel }) => {
-    if (!collaboratorId) {
+  const [errors, setErrors] = useState<{ [key: number]: string }>({});
+
+  const validInnerCollaborators = collaborators.flatMap((collaborator) => {
+    if (!collaborator.collaboratorId) {
       return [];
     }
-    return { collaboratorId, accessLevel, permissionType };
+    return collaborator;
   });
   const [isAdding, setIsAdding] = useState(false);
 
   const handleAddCollaborators = async () => {
+    const collaboratorId2IdsMap = validInnerCollaborators.reduce<{
+      [key: string]: number[];
+    }>((previousValue, collaborator) => {
+      const key = collaborator.collaboratorId;
+      const existingIds = previousValue[key];
+
+      if (!existingIds) {
+        return {
+          ...previousValue,
+          [key]: [collaborator.id],
+        };
+      }
+      existingIds.push(collaborator.id);
+      return previousValue;
+    }, {});
+    setErrors({});
     setIsAdding(true);
     try {
-      await onAddCollaborators(validCollaborators);
+      await onAddCollaborators(
+        validInnerCollaborators.map(({ id, ...collaborator }) => ({
+          ...collaborator,
+          permissionType,
+        }))
+      );
+    } catch (error) {
+      if (error instanceof DuplicateCollaboratorError) {
+        const newErrors: { [key: number]: string } = {};
+        error.details.pendingAdded.forEach((collaboratorId) => {
+          collaboratorId2IdsMap[collaboratorId].slice(1).forEach((id) => {
+            newErrors[id] = DUPLICATE_COLLABORATOR_ERROR_MESSAGE;
+          });
+        });
+        error.details.existing.forEach((collaboratorId) => {
+          collaboratorId2IdsMap[collaboratorId].forEach((id) => {
+            newErrors[id] = DUPLICATE_COLLABORATOR_WITH_LIST_ERROR_MESSAGE;
+          });
+        });
+        setErrors(newErrors);
+        return;
+      }
     } finally {
       setIsAdding(false);
     }
@@ -123,6 +177,7 @@ export const AddCollaboratorsModal = ({
           description={inputDescription}
           collaboratorIdInputPlaceholder={inputPlaceholder}
           addAnotherButtonLabel={addAnotherButtonLabel}
+          errors={errors}
         />
       </EuiModalBody>
 
@@ -133,7 +188,7 @@ export const AddCollaboratorsModal = ({
           })}
         </EuiSmallButtonEmpty>
         <EuiSmallButton
-          disabled={validCollaborators.length === 0}
+          disabled={validInnerCollaborators.length === 0}
           type="submit"
           onClick={handleAddCollaborators}
           fill

diff --git a/...ugins/workspace/public/components/add_collaborators_modal/duplicate_collaborator_error.ts b/...ugins/workspace/public/components/add_collaborators_modal/duplicate_collaborator_error.ts
@@ -0,0 +1,16 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+export class DuplicateCollaboratorError {
+  constructor(
+    private _details: {
+      pendingAdded: string[];
+      existing: string[];
+    }
+  ) {}
+  public get details() {
+    return this._details;
+  }
+}
diff --git a/src/plugins/workspace/public/components/add_collaborators_modal/index.ts b/src/plugins/workspace/public/components/add_collaborators_modal/index.ts
@@ -4,3 +4,7 @@
  */
 
 export { AddCollaboratorsModal, AddCollaboratorsModalProps } from './add_collaborators_modal';
+export {
+  DuplicateCollaboratorError,
+  CollaboratorDuplicateSource,
+} from './duplicate_collaborator_error';
diff --git a/...workspace/public/components/add_collaborators_modal/workspace_collaborator_input.test.tsx b/...workspace/public/components/add_collaborators_modal/workspace_collaborator_input.test.tsx
@@ -41,4 +41,9 @@ describe('WorkspaceCollaboratorInput', () => {
     fireEvent.click(deleteButton);
     expect(defaultProps.onDelete).toHaveBeenCalledWith(0);
   });
+
+  it('collaborator id input should be invalid when error passed', () => {
+    render(<WorkspaceCollaboratorInput {...defaultProps} error="error" />);
+    expect(screen.getByTestId('workspaceCollaboratorIdInput-0')).toBeInvalid();
+  });
 });
diff --git a/...gins/workspace/public/components/add_collaborators_modal/workspace_collaborator_input.tsx b/...gins/workspace/public/components/add_collaborators_modal/workspace_collaborator_input.tsx
@@ -20,6 +20,7 @@ export const COLLABORATOR_ID_INPUT_LABEL_ID = 'collaborator_id_input_label';
 
 export interface WorkspaceCollaboratorInputProps {
   index: number;
+  error?: string;
   collaboratorId?: string;
   accessLevel: WorkspaceCollaboratorAccessLevel;
   collaboratorIdInputPlaceholder?: string;
@@ -42,6 +43,7 @@ const isAccessLevelKey = (test: string): test is WorkspaceCollaboratorAccessLeve
 
 export const WorkspaceCollaboratorInput = ({
   index,
+  error,
   accessLevel,
   collaboratorId,
   onDelete,
@@ -79,6 +81,7 @@ export const WorkspaceCollaboratorInput = ({
           data-test-subj={`workspaceCollaboratorIdInput-${index}`}
           placeholder={collaboratorIdInputPlaceholder}
           aria-labelledby={COLLABORATOR_ID_INPUT_LABEL_ID}
+          isInvalid={!!error}
         />
       </EuiFlexItem>
       <EuiFlexItem grow={false}>

diff --git a/...orkspace/public/components/add_collaborators_modal/workspace_collaborators_panel.test.tsx b/...orkspace/public/components/add_collaborators_modal/workspace_collaborators_panel.test.tsx
@@ -91,4 +91,12 @@ describe('WorkspaceCollaboratorsPanel', () => {
       { id: 3, collaboratorId: '', accessLevel: 'readOnly' },
     ]);
   });
+
+  it('should display error message if provided', () => {
+    render(
+      <WorkspaceCollaboratorsPanel {...defaultProps} errors={{ 2: 'A test error message' }} />
+    );
+
+    expect(screen.getByText('A test error message')).toBeInTheDocument();
+  });
 });
diff --git a/...ins/workspace/public/components/add_collaborators_modal/workspace_collaborators_panel.tsx b/...ins/workspace/public/components/add_collaborators_modal/workspace_collaborators_panel.tsx
@@ -24,6 +24,7 @@ export interface WorkspaceCollaboratorInner
 
 export interface WorkspaceCollaboratorsPanelProps {
   label: string;
+  errors?: { [key: number]: string };
   description?: string;
   collaborators: WorkspaceCollaboratorInner[];
   onChange: (value: WorkspaceCollaboratorInner[]) => void;
@@ -33,6 +34,7 @@ export interface WorkspaceCollaboratorsPanelProps {
 
 export const WorkspaceCollaboratorsPanel = ({
   label,
+  errors,
   description,
   collaborators,
   addAnotherButtonLabel,
@@ -91,7 +93,12 @@ export const WorkspaceCollaboratorsPanel = ({
         </>
       )}
       {collaborators.map((item, index) => (
-        <EuiCompressedFormRow key={item.id} fullWidth>
+        <EuiCompressedFormRow
+          key={item.id}
+          fullWidth
+          error={errors?.[item.id]}
+          isInvalid={!!errors?.[item.id]}
+        >
           <WorkspaceCollaboratorInput
             index={index}
             accessLevel={item.accessLevel}
@@ -100,6 +107,7 @@ export const WorkspaceCollaboratorsPanel = ({
             onAccessLevelChange={handleAccessLevelChange}
             onDelete={handleDelete}
             collaboratorIdInputPlaceholder={collaboratorIdInputPlaceholder}
+            error={errors?.[item.id]}
           />
         </EuiCompressedFormRow>
       ))}

diff --git a/src/plugins/workspace/public/components/workspace_form/add_collaborator_button.test.tsx b/src/plugins/workspace/public/components/workspace_form/add_collaborator_button.test.tsx
@@ -7,6 +7,8 @@ import React from 'react';
 import { fireEvent, render } from '@testing-library/react';
 
 import { AddCollaboratorButton } from './add_collaborator_button';
+import { WorkspacePermissionSetting } from './types';
+import { DuplicateCollaboratorError } from '../add_collaborators_modal';
 
 describe('AddCollaboratorButton', () => {
   const mockProps = {
@@ -24,7 +26,7 @@ describe('AddCollaboratorButton', () => {
         type: 'group',
         group: 'group',
       },
-    ],
+    ] as WorkspacePermissionSetting[],
     handleSubmitPermissionSettings: jest.fn(),
   };
 
@@ -105,4 +107,65 @@ describe('AddCollaboratorButton', () => {
       { id: 2, modes: ['library_read', 'read'], type: 'user', userId: '2' },
     ]);
   });
+
+  it('should throw DuplicateCollaboratorError with consistent details', async () => {
+    let errorCached: DuplicateCollaboratorError | undefined;
+    const mockOnAdd = jest.fn(async ({ onAddCollaborators }) => {
+      try {
+        await onAddCollaborators([
+          {
+            accessLevel: 'readOnly',
+            collaboratorId: 'admin',
+            permissionType: 'user',
+          },
+          {
+            accessLevel: 'readOnly',
+            collaboratorId: 'group',
+            permissionType: 'group',
+          },
+          {
+            accessLevel: 'readOnly',
+            collaboratorId: 'new-user',
+            permissionType: 'user',
+          },
+          {
+            accessLevel: 'readOnly',
+            collaboratorId: 'new-user',
+            permissionType: 'user',
+          },
+        ]);
+      } catch (e) {
+        errorCached = e;
+      }
+    });
+    const displayedTypes = [
+      {
+        name: 'add user',
+        buttonLabel: 'add user',
+        onAdd: mockOnAdd,
+        id: 'user',
+      },
+      {
+        name: 'add group',
+        buttonLabel: 'add group',
+        onAdd: mockOnAdd,
+        id: 'group',
+      },
+    ];
+    const { getByTestId, getByText } = render(
+      <AddCollaboratorButton {...mockProps} displayedTypes={displayedTypes} />
+    );
+    const button = getByTestId('add-collaborator-button');
+    fireEvent.click(button);
+    expect(getByTestId('add-collaborator-popover')).toBeInTheDocument();
+    const addUserButton = getByText('add user');
+    fireEvent.click(addUserButton);
+    await mockOnAdd.mock.results;
+
+    expect(errorCached).toBeInstanceOf(DuplicateCollaboratorError);
+    if (errorCached instanceof DuplicateCollaboratorError) {
+      expect(errorCached.details.pendingAdded).toEqual(['new-user']);
+      expect(errorCached.details.existing).toEqual(['admin', 'group']);
+    }
+  });
 });
diff --git a/src/plugins/workspace/public/components/workspace_form/add_collaborator_button.tsx b/src/plugins/workspace/public/components/workspace_form/add_collaborator_button.tsx
@@ -15,10 +15,11 @@ import { i18n } from '@osd/i18n';
 import { WorkspaceCollaboratorType } from '../../services/workspace_collaborator_types_service';
 import { WorkspaceCollaborator } from '../../types';
 import { PermissionSetting } from './workspace_collaborator_table';
-import { generateNextPermissionSettingsId } from './utils';
+import { generateNextPermissionSettingsId, hasSameUserIdOrGroup } from './utils';
 import { accessLevelNameToWorkspacePermissionModesMap } from '../../constants';
 import { WorkspacePermissionItemType } from './constants';
 import { WorkspacePermissionSetting } from './types';
+import { DuplicateCollaboratorError } from '../add_collaborators_modal';
 
 interface Props {
   displayedTypes: WorkspaceCollaboratorType[];
@@ -44,8 +45,9 @@ export const AddCollaboratorButton = ({
   }, []);
 
   const onAddCollaborators = async (collaborators: WorkspaceCollaborator[]) => {
+    const uniqueCollaboratorIds = new Set();
     const addedSettings = collaborators.map(({ permissionType, accessLevel, collaboratorId }) => ({
-      type: permissionType as WorkspacePermissionItemType,
+      type: permissionType,
       modes: accessLevelNameToWorkspacePermissionModesMap[accessLevel],
       id: nextIdGenerator(),
       ...(permissionType === WorkspacePermissionItemType.User
@@ -55,8 +57,34 @@ export const AddCollaboratorButton = ({
         : {
             group: collaboratorId,
           }),
-    }));
-    const newPermissionSettings = [...permissionSettings, ...addedSettings];
+      collaboratorId,
+    })) as Array<WorkspacePermissionSetting & { collaboratorId: string }>;
+    const existingDuplicateSettings = addedSettings.filter((permissionSettingToAdd) =>
+      hasSameUserIdOrGroup(permissionSettings, permissionSettingToAdd)
+    );
+    const pendingAddedDuplicateCollaboratorIds = Array.from(
+      new Set(
+        collaborators.flatMap(({ collaboratorId }) => {
+          if (uniqueCollaboratorIds.has(collaboratorId)) {
+            return [collaboratorId];
+          }
+          uniqueCollaboratorIds.add(collaboratorId);
+          return [];
+        })
+      )
+    );
+    if (pendingAddedDuplicateCollaboratorIds.length > 0 || existingDuplicateSettings.length > 0) {
+      throw new DuplicateCollaboratorError({
+        pendingAdded: pendingAddedDuplicateCollaboratorIds,
+        existing: Array.from(
+          new Set(existingDuplicateSettings.map((setting) => setting.collaboratorId))
+        ),
+      });
+    }
+    const newPermissionSettings = [
+      ...permissionSettings,
+      ...addedSettings.map(({ collaboratorId, ...rest }) => rest),
+    ];
     await handleSubmitPermissionSettings(newPermissionSettings as WorkspacePermissionSetting[]);
   };
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		feat:
		- [Workspace]Display duplicate collaborators error in add collaborators modal ([#8531](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/8531))