From 64ed61bd9709b62d4fbc3de98656570e7e6bc782 Mon Sep 17 00:00:00 2001 From: Lin Wang Date: Tue, 15 Oct 2024 16:40:50 +0800 Subject: [PATCH] [Workspace]Display duplicate collaborators error in add collaborators modal (#8531) * Show Duplicate collaborators error Signed-off-by: Lin Wang * Changeset file for PR #8531 created/updated * Combine with pendingAdded and existing error details Signed-off-by: Lin Wang * Use separate error message for duplicate collaborators with list Signed-off-by: Lin Wang --------- Signed-off-by: Lin Wang Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> --- changelogs/fragments/8531.yml | 2 + .../add_collaborators_modal.test.tsx | 25 +++++++ .../add_collaborators_modal.tsx | 65 +++++++++++++++++-- .../duplicate_collaborator_error.ts | 16 +++++ .../add_collaborators_modal/index.ts | 4 ++ .../workspace_collaborator_input.test.tsx | 5 ++ .../workspace_collaborator_input.tsx | 3 + .../workspace_collaborators_panel.test.tsx | 8 +++ .../workspace_collaborators_panel.tsx | 10 ++- .../add_collaborator_button.test.tsx | 65 ++++++++++++++++++- .../add_collaborator_button.tsx | 36 ++++++++-- 11 files changed, 228 insertions(+), 11 deletions(-) create mode 100644 changelogs/fragments/8531.yml create mode 100644 src/plugins/workspace/public/components/add_collaborators_modal/duplicate_collaborator_error.ts diff --git a/changelogs/fragments/8531.yml b/changelogs/fragments/8531.yml new file mode 100644 index 000000000000..e23c5eba1faa --- /dev/null +++ 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)) \ No newline at end of file diff --git a/src/plugins/workspace/public/components/add_collaborators_modal/add_collaborators_modal.test.tsx b/src/plugins/workspace/public/components/add_collaborators_modal/add_collaborators_modal.test.tsx index 80cbc4f09c7e..fdc56756dcf2 100644 --- a/src/plugins/workspace/public/components/add_collaborators_modal/add_collaborators_modal.test.tsx +++ b/src/plugins/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(); + 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 index e75524faca1b..07d2d8a90054 100644 --- 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([ { 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} /> @@ -133,7 +188,7 @@ export const AddCollaboratorsModal = ({ })} { fireEvent.click(deleteButton); expect(defaultProps.onDelete).toHaveBeenCalledWith(0); }); + + it('collaborator id input should be invalid when error passed', () => { + render(); + expect(screen.getByTestId('workspaceCollaboratorIdInput-0')).toBeInvalid(); + }); }); diff --git a/src/plugins/workspace/public/components/add_collaborators_modal/workspace_collaborator_input.tsx b/src/plugins/workspace/public/components/add_collaborators_modal/workspace_collaborator_input.tsx index 93a53c3fbcc8..3d849992f169 100644 --- a/src/plugins/workspace/public/components/add_collaborators_modal/workspace_collaborator_input.tsx +++ b/src/plugins/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} /> diff --git a/src/plugins/workspace/public/components/add_collaborators_modal/workspace_collaborators_panel.test.tsx b/src/plugins/workspace/public/components/add_collaborators_modal/workspace_collaborators_panel.test.tsx index f5e695cc59a7..45d50710c8e1 100644 --- a/src/plugins/workspace/public/components/add_collaborators_modal/workspace_collaborators_panel.test.tsx +++ b/src/plugins/workspace/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( + + ); + + expect(screen.getByText('A test error message')).toBeInTheDocument(); + }); }); diff --git a/src/plugins/workspace/public/components/add_collaborators_modal/workspace_collaborators_panel.tsx b/src/plugins/workspace/public/components/add_collaborators_modal/workspace_collaborators_panel.tsx index 4c71d1c6be0e..80ca9666ab16 100644 --- a/src/plugins/workspace/public/components/add_collaborators_modal/workspace_collaborators_panel.tsx +++ b/src/plugins/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) => ( - + ))} 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 index f5466a1f0996..82d4bae43c66 100644 --- 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( + + ); + 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 index 195f92d98a28..8cbf34cbccfe 100644 --- 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; + 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[]); };