diff --git a/src/core/types/workspace.ts b/src/core/types/workspace.ts index fd1cab5b63e7..6f44ddb08ecd 100644 --- a/src/core/types/workspace.ts +++ b/src/core/types/workspace.ts @@ -24,3 +24,7 @@ export interface WorkspaceAttributeWithPermission extends WorkspaceAttribute { export interface WorkspaceObject extends WorkspaceAttributeWithPermission { readonly?: boolean; } + +export interface WorkspaceAttributeWithPermission extends WorkspaceAttribute { + permissions?: Permissions; +} diff --git a/src/plugins/workspace/common/constants.ts b/src/plugins/workspace/common/constants.ts index e5d1cd03a166..ff66348b1f2f 100644 --- a/src/plugins/workspace/common/constants.ts +++ b/src/plugins/workspace/common/constants.ts @@ -8,7 +8,7 @@ export const WORKSPACE_LIST_APP_ID = 'workspace_list'; export const WORKSPACE_UPDATE_APP_ID = 'workspace_update'; export const WORKSPACE_OVERVIEW_APP_ID = 'workspace_overview'; // These features will be checked and disabled in checkbox on default. -export const DEFAULT_CHECKED_FEATURES_IDS = [WORKSPACE_UPDATE_APP_ID, WORKSPACE_OVERVIEW_APP_ID]; +export const DEFAULT_SELECTED_FEATURES_IDS = [WORKSPACE_UPDATE_APP_ID, WORKSPACE_OVERVIEW_APP_ID]; export const WORKSPACE_FATAL_ERROR_APP_ID = 'workspace_fatal_error'; export const WORKSPACE_SAVED_OBJECTS_CLIENT_WRAPPER_ID = 'workspace'; export const WORKSPACE_CONFLICT_CONTROL_SAVED_OBJECTS_CLIENT_WRAPPER_ID = diff --git a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx index b67870b55294..114e72f177db 100644 --- a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx +++ b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx @@ -182,15 +182,47 @@ describe('WorkspaceCreator', () => { expect(notificationToastsAddDanger).not.toHaveBeenCalled(); }); + it('should show danger toasts after create workspace failed', async () => { + workspaceClientCreate.mockReturnValueOnce({ result: { id: 'failResult' }, success: false }); + const { getByTestId } = render(); + const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText'); + fireEvent.input(nameInput, { + target: { value: 'test workspace name' }, + }); + fireEvent.click(getByTestId('workspaceForm-bottomBar-createButton')); + expect(workspaceClientCreate).toHaveBeenCalled(); + await waitFor(() => { + expect(notificationToastsAddDanger).toHaveBeenCalled(); + }); + expect(notificationToastsAddSuccess).not.toHaveBeenCalled(); + }); + + it('should show danger toasts after call create workspace API failed', async () => { + workspaceClientCreate.mockImplementationOnce(async () => { + throw new Error(); + }); + const { getByTestId } = render(); + const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText'); + fireEvent.input(nameInput, { + target: { value: 'test workspace name' }, + }); + fireEvent.click(getByTestId('workspaceForm-bottomBar-createButton')); + expect(workspaceClientCreate).toHaveBeenCalled(); + await waitFor(() => { + expect(notificationToastsAddDanger).toHaveBeenCalled(); + }); + expect(notificationToastsAddSuccess).not.toHaveBeenCalled(); + }); + it('create workspace with customized permissions', async () => { - const { getByTestId, getByText } = render(); + const { getByTestId, getByText, getAllByText } = render(); const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText'); fireEvent.input(nameInput, { target: { value: 'test workspace name' }, }); fireEvent.click(getByText('Users & Permissions')); fireEvent.click(getByTestId('workspaceForm-permissionSettingPanel-user-addNew')); - const userIdInput = getByTestId('workspaceForm-permissionSettingPanel-0-userId'); + const userIdInput = getAllByText('Select')[0]; fireEvent.click(userIdInput); fireEvent.input(getByTestId('comboBoxSearchInput'), { target: { value: 'test user id' }, @@ -215,19 +247,4 @@ describe('WorkspaceCreator', () => { }); expect(notificationToastsAddDanger).not.toHaveBeenCalled(); }); - - it('should show danger toasts after create workspace failed', async () => { - workspaceClientCreate.mockReturnValue({ result: { id: 'failResult' }, success: false }); - const { getByTestId } = render(); - const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText'); - fireEvent.input(nameInput, { - target: { value: 'test workspace name' }, - }); - fireEvent.click(getByTestId('workspaceForm-bottomBar-createButton')); - expect(workspaceClientCreate).toHaveBeenCalled(); - await waitFor(() => { - expect(notificationToastsAddDanger).toHaveBeenCalled(); - }); - expect(notificationToastsAddSuccess).not.toHaveBeenCalled(); - }); }); diff --git a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx index 4b3e6e57c486..d296ef5675a0 100644 --- a/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx +++ b/src/plugins/workspace/public/components/workspace_creator/workspace_creator.tsx @@ -11,14 +11,14 @@ import { WorkspaceForm, WorkspaceFormSubmitData, WorkspaceOperationType } from ' import { WORKSPACE_OVERVIEW_APP_ID } from '../../../common/constants'; import { formatUrlWithWorkspaceId } from '../../../../../core/public/utils'; import { WorkspaceClient } from '../../workspace_client'; -import { convertPermissionSettingsToPermissions } from '../workspace_form/utils'; +import { convertPermissionSettingsToPermissions } from '../workspace_form'; export const WorkspaceCreator = () => { const { services: { application, notifications, http, workspaceClient }, } = useOpenSearchDashboards<{ workspaceClient: WorkspaceClient }>(); - const isPermissionEnabled = application?.capabilities.workspaces.permissionEnabled; + const handleWorkspaceFormSubmit = useCallback( async (data: WorkspaceFormSubmitData) => { let result; @@ -28,6 +28,29 @@ export const WorkspaceCreator = () => { attributes, convertPermissionSettingsToPermissions(permissionSettings) ); + if (result?.success) { + notifications?.toasts.addSuccess({ + title: i18n.translate('workspace.create.success', { + defaultMessage: 'Create workspace successfully', + }), + }); + if (application && http) { + const newWorkspaceId = result.result.id; + // Redirect page after one second, leave one second time to show create successful toast. + window.setTimeout(() => { + window.location.href = formatUrlWithWorkspaceId( + application.getUrlForApp(WORKSPACE_OVERVIEW_APP_ID, { + absolute: true, + }), + newWorkspaceId, + http.basePath + ); + }, 1000); + } + return; + } else { + throw new Error(result?.error ? result?.error : 'create workspace failed'); + } } catch (error) { notifications?.toasts.addDanger({ title: i18n.translate('workspace.create.failed', { @@ -37,33 +60,6 @@ export const WorkspaceCreator = () => { }); return; } - if (result?.success) { - notifications?.toasts.addSuccess({ - title: i18n.translate('workspace.create.success', { - defaultMessage: 'Create workspace successfully', - }), - }); - if (application && http) { - const newWorkspaceId = result.result.id; - // Redirect page after one second, leave one second time to show create successful toast. - window.setTimeout(() => { - window.location.href = formatUrlWithWorkspaceId( - application.getUrlForApp(WORKSPACE_OVERVIEW_APP_ID, { - absolute: true, - }), - newWorkspaceId, - http.basePath - ); - }, 1000); - } - return; - } - notifications?.toasts.addDanger({ - title: i18n.translate('workspace.create.failed', { - defaultMessage: 'Failed to create workspace', - }), - text: result?.error, - }); }, [notifications?.toasts, http, application, workspaceClient] ); diff --git a/src/plugins/workspace/public/components/workspace_form/constants.ts b/src/plugins/workspace/public/components/workspace_form/constants.ts index 3af7f5c743e9..693f0cdce141 100644 --- a/src/plugins/workspace/public/components/workspace_form/constants.ts +++ b/src/plugins/workspace/public/components/workspace_form/constants.ts @@ -6,6 +6,17 @@ import { i18n } from '@osd/i18n'; import { WorkspacePermissionMode } from '../../../common/constants'; +export enum WorkspaceOperationType { + Create = 'create', + Update = 'update', +} + +export enum WorkspaceFormTabs { + NotSelected, + FeatureVisibility, + UsersAndPermissions, +} + export enum WorkspacePermissionItemType { User = 'user', Group = 'group', @@ -51,14 +62,3 @@ export const optionIdToWorkspacePermissionModesMap: { ], [PermissionModeId.Admin]: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write], }; - -export enum WorkspaceOperationType { - Create = 'create', - Update = 'update', -} - -export enum WorkspaceFormTabs { - NotSelected, - FeatureVisibility, - UsersAndPermissions, -} diff --git a/src/plugins/workspace/public/components/workspace_form/index.ts b/src/plugins/workspace/public/components/workspace_form/index.ts index 6531d4a1c6f7..416592ff2006 100644 --- a/src/plugins/workspace/public/components/workspace_form/index.ts +++ b/src/plugins/workspace/public/components/workspace_form/index.ts @@ -6,3 +6,7 @@ export { WorkspaceForm } from './workspace_form'; export { WorkspaceFormSubmitData } from './types'; export { WorkspaceOperationType } from './constants'; +export { + convertPermissionsToPermissionSettings, + convertPermissionSettingsToPermissions, +} from './utils'; diff --git a/src/plugins/workspace/public/components/workspace_form/types.ts b/src/plugins/workspace/public/components/workspace_form/types.ts index 5f81ba3e6daa..95c96fa3e353 100644 --- a/src/plugins/workspace/public/components/workspace_form/types.ts +++ b/src/plugins/workspace/public/components/workspace_form/types.ts @@ -3,21 +3,29 @@ * SPDX-License-Identifier: Apache-2.0 */ -import type { WorkspacePermissionItemType, WorkspaceOperationType } from './constants'; -import type { WorkspacePermissionMode } from '../../../common/constants'; import type { ApplicationStart } from '../../../../../core/public'; +import type { WorkspacePermissionMode } from '../../../common/constants'; +import type { WorkspaceOperationType, WorkspacePermissionItemType } from './constants'; export type WorkspacePermissionSetting = - | { type: WorkspacePermissionItemType.User; userId: string; modes: WorkspacePermissionMode[] } - | { type: WorkspacePermissionItemType.Group; group: string; modes: WorkspacePermissionMode[] }; + | { + id: number; + type: WorkspacePermissionItemType.User; + userId: string; + modes: WorkspacePermissionMode[]; + } + | { + id: number; + type: WorkspacePermissionItemType.Group; + group: string; + modes: WorkspacePermissionMode[]; + }; export interface WorkspaceFormSubmitData { name: string; description?: string; features?: string[]; color?: string; - icon?: string; - defaultVISTheme?: string; permissionSettings?: WorkspacePermissionSetting[]; } @@ -36,11 +44,10 @@ export interface WorkspaceFeatureGroup { features: WorkspaceFeature[]; } -export type WorkspaceFormErrors = Omit< - { [key in keyof WorkspaceFormData]?: string }, - 'permissions' -> & { - permissions?: string[]; +export type WorkspaceFormErrors = { + [key in keyof Omit]?: string; +} & { + permissionSettings?: { [key: number]: string }; }; export interface WorkspaceFormProps { diff --git a/src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts b/src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts index 00bee7b3ab37..cded7c4bcb71 100644 --- a/src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts +++ b/src/plugins/workspace/public/components/workspace_form/use_workspace_form.ts @@ -5,19 +5,12 @@ import { useCallback, useState, FormEventHandler, useRef, useMemo, useEffect } from 'react'; import { htmlIdGenerator, EuiFieldTextProps, EuiColorPickerProps } from '@elastic/eui'; -import { i18n } from '@osd/i18n'; import { useApplications } from '../../hooks'; import { featureMatchesConfig } from '../../utils'; -import { WorkspacePermissionItemType, WorkspaceFormTabs } from './constants'; -import { WorkspacePermissionSetting, WorkspaceFormProps, WorkspaceFormErrors } from './types'; -import { - appendDefaultFeatureIds, - getNumberOfErrors, - isUserOrGroupPermissionSettingDuplicated, - isValidNameOrDescription, - isValidWorkspacePermissionSetting, -} from './utils'; +import { WorkspaceFormTabs } from './constants'; +import { WorkspaceFormProps, WorkspaceFormErrors, WorkspacePermissionSetting } from './types'; +import { appendDefaultFeatureIds, getNumberOfErrors, validateWorkspaceForm } from './utils'; const workspaceHtmlIdGenerator = htmlIdGenerator(); @@ -28,7 +21,6 @@ export const useWorkspaceForm = ({ application, defaultValues, onSubmit }: Works const [color, setColor] = useState(defaultValues?.color); const [selectedTab, setSelectedTab] = useState(WorkspaceFormTabs.FeatureVisibility); - const [numberOfErrors, setNumberOfErrors] = useState(0); // The matched feature id list based on original feature config, // the feature category will be expanded to list of feature ids const defaultFeatures = useMemo(() => { @@ -44,7 +36,7 @@ export const useWorkspaceForm = ({ application, defaultValues, onSubmit }: Works appendDefaultFeatureIds(defaultFeatures) ); const [permissionSettings, setPermissionSettings] = useState< - Array> + Array & Partial> >( defaultValues?.permissionSettings && defaultValues.permissionSettings.length > 0 ? defaultValues.permissionSettings @@ -52,6 +44,7 @@ export const useWorkspaceForm = ({ application, defaultValues, onSubmit }: Works ); const [formErrors, setFormErrors] = useState({}); + const numberOfErrors = useMemo(() => getNumberOfErrors(formErrors), [formErrors]); const formIdRef = useRef(); const getFormData = () => ({ name, @@ -70,84 +63,10 @@ export const useWorkspaceForm = ({ application, defaultValues, onSubmit }: Works const handleFormSubmit = useCallback( (e) => { e.preventDefault(); - let currentFormErrors: WorkspaceFormErrors = {}; const formData = getFormDataRef.current(); - if (!formData.name) { - currentFormErrors = { - ...currentFormErrors, - name: i18n.translate('workspace.form.detail.name.empty', { - defaultMessage: "Name can't be empty.", - }), - }; - } - if (!isValidNameOrDescription(formData.name)) { - currentFormErrors = { - ...currentFormErrors, - name: i18n.translate('workspace.form.detail.name.invalid', { - defaultMessage: 'Invalid workspace name', - }), - }; - } - if (!isValidNameOrDescription(formData.description)) { - currentFormErrors = { - ...currentFormErrors, - description: i18n.translate('workspace.form.detail.description.invalid', { - defaultMessage: 'Invalid workspace description', - }), - }; - } - const permissionErrors: string[] = new Array(formData.permissionSettings.length); - for (let i = 0; i < formData.permissionSettings.length; i++) { - const permission = formData.permissionSettings[i]; - if (isValidWorkspacePermissionSetting(permission)) { - if ( - isUserOrGroupPermissionSettingDuplicated( - formData.permissionSettings.slice(0, i), - permission - ) - ) { - permissionErrors[i] = i18n.translate('workspace.form.permission.invalidate.group', { - defaultMessage: 'Duplicate permission setting', - }); - continue; - } - continue; - } - if (!permission.type) { - permissionErrors[i] = i18n.translate('workspace.form.permission.invalidate.type', { - defaultMessage: 'Invalid type', - }); - continue; - } - if (!permission.modes || permission.modes.length === 0) { - permissionErrors[i] = i18n.translate('workspace.form.permission.invalidate.modes', { - defaultMessage: 'Invalid permission modes', - }); - continue; - } - if (permission.type === WorkspacePermissionItemType.User && !permission.userId) { - permissionErrors[i] = i18n.translate('workspace.form.permission.invalidate.userId', { - defaultMessage: 'Invalid userId', - }); - continue; - } - if (permission.type === WorkspacePermissionItemType.Group && !permission.group) { - permissionErrors[i] = i18n.translate('workspace.form.permission.invalidate.group', { - defaultMessage: 'Invalid user group', - }); - continue; // this line is need for more conditions - } - } - if (permissionErrors.some((error) => !!error)) { - currentFormErrors = { - ...currentFormErrors, - permissions: permissionErrors, - }; - } - const currentNumberOfErrors = getNumberOfErrors(currentFormErrors); + const currentFormErrors: WorkspaceFormErrors = validateWorkspaceForm(formData); setFormErrors(currentFormErrors); - setNumberOfErrors(currentNumberOfErrors); - if (currentNumberOfErrors > 0) { + if (getNumberOfErrors(currentFormErrors) > 0) { return; } @@ -168,7 +87,7 @@ export const useWorkspaceForm = ({ application, defaultValues, onSubmit }: Works onSubmit?.({ ...formData, name: formData.name!, - permissionSettings: formData.permissionSettings.filter(isValidWorkspacePermissionSetting), + permissionSettings: formData.permissionSettings as WorkspacePermissionSetting[], }); }, [defaultFeatures, onSubmit, defaultValues?.features] diff --git a/src/plugins/workspace/public/components/workspace_form/utils.test.ts b/src/plugins/workspace/public/components/workspace_form/utils.test.ts index a12222cbd22b..4531b2979bc1 100644 --- a/src/plugins/workspace/public/components/workspace_form/utils.test.ts +++ b/src/plugins/workspace/public/components/workspace_form/utils.test.ts @@ -3,12 +3,134 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { WorkspacePermissionMode } from '../../../common/constants'; -import { WorkspacePermissionItemType } from './constants'; +import { AppNavLinkStatus, DEFAULT_APP_CATEGORIES } from '../../../../../core/public'; import { + validateWorkspaceForm, + convertApplicationsToFeaturesOrGroups, convertPermissionSettingsToPermissions, convertPermissionsToPermissionSettings, } from './utils'; +import { WorkspacePermissionMode } from '../../../common/constants'; +import { WorkspacePermissionItemType } from './constants'; + +describe('convertApplicationsToFeaturesOrGroups', () => { + it('should filter out invisible features', () => { + expect( + convertApplicationsToFeaturesOrGroups([ + { id: 'foo1', title: 'Foo 1', navLinkStatus: AppNavLinkStatus.hidden }, + { id: 'foo2', title: 'Foo 2', navLinkStatus: AppNavLinkStatus.visible, chromeless: true }, + { + id: 'foo3', + title: 'Foo 3', + navLinkStatus: AppNavLinkStatus.visible, + category: DEFAULT_APP_CATEGORIES.management, + }, + { + id: 'workspace_overview', + title: 'Workspace Overview', + navLinkStatus: AppNavLinkStatus.visible, + }, + { + id: 'bar', + title: 'Bar', + navLinkStatus: AppNavLinkStatus.visible, + }, + ]) + ).toEqual([ + { + id: 'bar', + name: 'Bar', + }, + ]); + }); + it('should group same category applications in same feature group', () => { + expect( + convertApplicationsToFeaturesOrGroups([ + { + id: 'foo', + title: 'Foo', + navLinkStatus: AppNavLinkStatus.visible, + category: DEFAULT_APP_CATEGORIES.opensearchDashboards, + }, + { + id: 'bar', + title: 'Bar', + navLinkStatus: AppNavLinkStatus.visible, + category: DEFAULT_APP_CATEGORIES.opensearchDashboards, + }, + { + id: 'baz', + title: 'Baz', + navLinkStatus: AppNavLinkStatus.visible, + category: DEFAULT_APP_CATEGORIES.observability, + }, + ]) + ).toEqual([ + { + name: 'OpenSearch Dashboards', + features: [ + { + id: 'foo', + name: 'Foo', + }, + { + id: 'bar', + name: 'Bar', + }, + ], + }, + { + name: 'Observability', + features: [ + { + id: 'baz', + name: 'Baz', + }, + ], + }, + ]); + }); + it('should return features if application without category', () => { + expect( + convertApplicationsToFeaturesOrGroups([ + { + id: 'foo', + title: 'Foo', + navLinkStatus: AppNavLinkStatus.visible, + }, + { + id: 'baz', + title: 'Baz', + navLinkStatus: AppNavLinkStatus.visible, + category: DEFAULT_APP_CATEGORIES.observability, + }, + { + id: 'bar', + title: 'Bar', + navLinkStatus: AppNavLinkStatus.visible, + }, + ]) + ).toEqual([ + { + id: 'foo', + name: 'Foo', + }, + { + id: 'bar', + name: 'Bar', + }, + { + name: 'Observability', + features: [ + { + id: 'baz', + name: 'Baz', + }, + ], + }, + ]); + }); +}); describe('convertPermissionSettingsToPermissions', () => { it('should return undefined if permission items not provided', () => { @@ -16,15 +138,52 @@ describe('convertPermissionSettingsToPermissions', () => { expect(convertPermissionSettingsToPermissions([])).toBeUndefined(); }); - it('should return consistent permission settings', () => { + it('should not add duplicate users and groups', () => { + expect( + convertPermissionSettingsToPermissions([ + { + id: 0, + type: WorkspacePermissionItemType.User, + userId: 'duplicate-user', + modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], + }, + { + id: 1, + type: WorkspacePermissionItemType.User, + userId: 'duplicate-user', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Read], + }, + { + id: 2, + type: WorkspacePermissionItemType.Group, + group: 'duplicate-group', + modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], + }, + { + id: 3, + type: WorkspacePermissionItemType.Group, + group: 'duplicate-group', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Read], + }, + ]) + ).toEqual({ + library_read: { users: ['duplicate-user'], groups: ['duplicate-group'] }, + library_write: { users: ['duplicate-user'], groups: ['duplicate-group'] }, + read: { users: ['duplicate-user'], groups: ['duplicate-group'] }, + }); + }); + + it('should return consistent permissions', () => { expect( convertPermissionSettingsToPermissions([ { + id: 0, type: WorkspacePermissionItemType.User, userId: 'foo', modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], }, { + id: 1, type: WorkspacePermissionItemType.Group, group: 'bar', modes: [WorkspacePermissionMode.LibraryWrite], @@ -49,17 +208,61 @@ describe('convertPermissionsToPermissionSettings', () => { }) ).toEqual([ { + id: 0, type: WorkspacePermissionItemType.User, userId: 'foo', modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], }, { + id: 1, type: WorkspacePermissionItemType.Group, group: 'bar', modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write], }, ]); }); + it('should separate to multi permission settings', () => { + expect( + convertPermissionsToPermissionSettings({ + library_read: { users: ['foo'] }, + library_write: { users: ['foo'] }, + read: { users: ['foo'] }, + }) + ).toEqual([ + { + id: 0, + type: WorkspacePermissionItemType.User, + userId: 'foo', + modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], + }, + { + id: 1, + type: WorkspacePermissionItemType.User, + userId: 'foo', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Read], + }, + ]); + expect( + convertPermissionsToPermissionSettings({ + library_read: { groups: ['bar'] }, + library_write: { groups: ['bar'] }, + read: { groups: ['bar'] }, + }) + ).toEqual([ + { + id: 0, + type: WorkspacePermissionItemType.Group, + group: 'bar', + modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], + }, + { + id: 1, + type: WorkspacePermissionItemType.Group, + group: 'bar', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Read], + }, + ]); + }); it('should only convert workspace supported permissions', () => { expect( convertPermissionsToPermissionSettings({ @@ -68,3 +271,120 @@ describe('convertPermissionsToPermissionSettings', () => { ).toEqual([]); }); }); + +describe('validateWorkspaceForm', () => { + it('should return error if name is empty', () => { + expect(validateWorkspaceForm({}).name).toEqual("Name can't be empty."); + }); + it('should return error if name is invalid', () => { + expect(validateWorkspaceForm({ name: '~' }).name).toEqual('Invalid workspace name'); + }); + it('should return error if description is invalid', () => { + expect(validateWorkspaceForm({ description: '~' }).description).toEqual( + 'Invalid workspace description' + ); + }); + it('should return error if permission setting type is invalid', () => { + expect( + validateWorkspaceForm({ + name: 'test', + permissionSettings: [{ id: 0 }], + }).permissionSettings + ).toEqual({ 0: 'Invalid type' }); + }); + it('should return error if permission setting modes is invalid', () => { + expect( + validateWorkspaceForm({ + name: 'test', + permissionSettings: [{ id: 0, type: WorkspacePermissionItemType.User, modes: [] }], + }).permissionSettings + ).toEqual({ 0: 'Invalid permission modes' }); + }); + it('should return error if permission setting user id is invalid', () => { + expect( + validateWorkspaceForm({ + name: 'test', + permissionSettings: [ + { + id: 0, + type: WorkspacePermissionItemType.User, + modes: [WorkspacePermissionMode.LibraryRead], + userId: '', + }, + ], + }).permissionSettings + ).toEqual({ 0: 'Invalid user id' }); + }); + it('should return error if permission setting group is invalid', () => { + expect( + validateWorkspaceForm({ + name: 'test', + permissionSettings: [ + { + id: 0, + type: WorkspacePermissionItemType.Group, + modes: [WorkspacePermissionMode.LibraryRead], + group: '', + }, + ], + }).permissionSettings + ).toEqual({ 0: 'Invalid user group' }); + }); + + it('should return error if permission setting is duplicate', () => { + expect( + validateWorkspaceForm({ + name: 'test', + permissionSettings: [ + { + id: 0, + type: WorkspacePermissionItemType.User, + modes: [WorkspacePermissionMode.LibraryRead], + userId: 'foo', + }, + { + id: 1, + type: WorkspacePermissionItemType.User, + modes: [WorkspacePermissionMode.LibraryRead], + userId: 'foo', + }, + ], + }).permissionSettings + ).toEqual({ 1: 'Duplicate permission setting' }); + expect( + validateWorkspaceForm({ + name: 'test', + permissionSettings: [ + { + id: 0, + type: WorkspacePermissionItemType.Group, + modes: [WorkspacePermissionMode.LibraryRead], + group: 'foo', + }, + { + id: 1, + type: WorkspacePermissionItemType.Group, + modes: [WorkspacePermissionMode.LibraryRead], + group: 'foo', + }, + ], + }).permissionSettings + ).toEqual({ 1: 'Duplicate permission setting' }); + }); + + it('should return empty object for valid for data', () => { + expect( + validateWorkspaceForm({ + name: 'test', + permissionSettings: [ + { + id: 0, + type: WorkspacePermissionItemType.Group, + modes: [WorkspacePermissionMode.LibraryRead], + group: 'foo', + }, + ], + }) + ).toEqual({}); + }); +}); diff --git a/src/plugins/workspace/public/components/workspace_form/utils.ts b/src/plugins/workspace/public/components/workspace_form/utils.ts index bb68f69c10e2..768745281cd5 100644 --- a/src/plugins/workspace/public/components/workspace_form/utils.ts +++ b/src/plugins/workspace/public/components/workspace_form/utils.ts @@ -3,20 +3,28 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { WorkspacePermissionMode, DEFAULT_CHECKED_FEATURES_IDS } from '../../../common/constants'; +import { i18n } from '@osd/i18n'; + +import { + AppNavLinkStatus, + DEFAULT_APP_CATEGORIES, + PublicAppInfo, +} from '../../../../../core/public'; import type { SavedObjectPermissions } from '../../../../../core/types'; +import { DEFAULT_SELECTED_FEATURES_IDS, WorkspacePermissionMode } from '../../../common/constants'; +import { + optionIdToWorkspacePermissionModesMap, + PermissionModeId, + WorkspacePermissionItemType, +} from './constants'; import { WorkspaceFeature, WorkspaceFeatureGroup, - WorkspacePermissionSetting, WorkspaceFormErrors, + WorkspaceFormSubmitData, + WorkspacePermissionSetting, } from './types'; -import { - WorkspacePermissionItemType, - optionIdToWorkspacePermissionModesMap, - PermissionModeId, -} from './constants'; export const isWorkspaceFeatureGroup = ( featureOrGroup: WorkspaceFeature | WorkspaceFeatureGroup @@ -31,20 +39,21 @@ export const isValidWorkspacePermissionSetting = ( (setting.type === WorkspacePermissionItemType.Group && !!setting.group)); export const isDefaultCheckedFeatureId = (id: string) => { - return DEFAULT_CHECKED_FEATURES_IDS.indexOf(id) > -1; + return DEFAULT_SELECTED_FEATURES_IDS.indexOf(id) > -1; }; export const appendDefaultFeatureIds = (ids: string[]) => { // concat default checked ids and unique the result - return Array.from(new Set(ids.concat(DEFAULT_CHECKED_FEATURES_IDS))); + return Array.from(new Set(ids.concat(DEFAULT_SELECTED_FEATURES_IDS))); }; -export const isValidNameOrDescription = (input?: string) => { - if (!input) { - return true; - } +export const isValidFormTextInput = (input?: string) => { + /** + * This regular expression is from the workspace form name and description field UI. + * It only accepts below characters. + **/ const regex = /^[0-9a-zA-Z()_\[\]\-\s]+$/; - return regex.test(input); + return typeof input === 'string' && regex.test(input); }; export const getNumberOfErrors = (formErrors: WorkspaceFormErrors) => { @@ -55,12 +64,70 @@ export const getNumberOfErrors = (formErrors: WorkspaceFormErrors) => { if (formErrors.description) { numberOfErrors += 1; } - if (formErrors.permissions) { - numberOfErrors += formErrors.permissions.length; + if (formErrors.permissionSettings) { + numberOfErrors += Object.keys(formErrors.permissionSettings).length; } return numberOfErrors; }; +export const convertApplicationsToFeaturesOrGroups = ( + applications: Array< + Pick + > +) => { + const UNDEFINED = 'undefined'; + + // Filter out all hidden applications and management applications and default selected features + const visibleApplications = applications.filter( + ({ navLinkStatus, chromeless, category, id }) => + navLinkStatus !== AppNavLinkStatus.hidden && + !chromeless && + !DEFAULT_SELECTED_FEATURES_IDS.includes(id) && + category?.id !== DEFAULT_APP_CATEGORIES.management.id + ); + + /** + * + * Convert applications to features map, the map use category label as + * map key and group all same category applications in one array after + * transfer application to feature. + * + **/ + const categoryLabel2Features = visibleApplications.reduce<{ + [key: string]: WorkspaceFeature[]; + }>((previousValue, application) => { + const label = application.category?.label || UNDEFINED; + + return { + ...previousValue, + [label]: [...(previousValue[label] || []), { id: application.id, name: application.title }], + }; + }, {}); + + /** + * + * Iterate all keys of categoryLabel2Features map, convert map to features or groups array. + * Features with category label will be converted to feature groups. Features without "undefined" + * category label will be converted to single features. Then append them to the result array. + * + **/ + return Object.keys(categoryLabel2Features).reduce< + Array + >((previousValue, categoryLabel) => { + const features = categoryLabel2Features[categoryLabel]; + if (categoryLabel === UNDEFINED) { + return [...previousValue, ...features]; + } + return [ + ...previousValue, + { + name: categoryLabel, + features, + }, + ]; + }, []); +}; + export const isUserOrGroupPermissionSettingDuplicated = ( permissionSettings: Array>, permissionSettingToCheck: WorkspacePermissionSetting @@ -75,19 +142,13 @@ export const isUserOrGroupPermissionSettingDuplicated = ( permissionSettingToCheck.group === permissionSetting.group) ); -export const generateWorkspacePermissionItemKey = ( - item: Partial, - index?: number -) => - [ - ...(item.type ?? []), - ...(item.type === WorkspacePermissionItemType.User ? [item.userId] : []), - ...(item.type === WorkspacePermissionItemType.Group ? [item.group] : []), - ...(item.modes ?? []), - index, - ].join('-'); - -// default permission mode is read +/** + * This function is for converting passed permission modes to permission option id, + * it will return Read as default if permission modes not matched. + * + * @param modes permission modes + * @returns permission option id + */ export const getPermissionModeId = (modes: WorkspacePermissionMode[]) => { for (const key in optionIdToWorkspacePermissionModesMap) { if (optionIdToWorkspacePermissionModesMap[key].every((mode) => modes?.includes(mode))) { @@ -109,11 +170,15 @@ export const convertPermissionSettingsToPermissions = ( previous[mode] = {}; } switch (current.type) { - case 'user': - previous[mode].users = [...(previous[mode].users || []), current.userId]; + case WorkspacePermissionItemType.User: + previous[mode].users = previous[mode].users?.includes(current.userId) + ? previous[mode].users + : [...(previous[mode].users || []), current.userId]; break; - case 'group': - previous[mode].groups = [...(previous[mode].groups || []), current.group]; + case WorkspacePermissionItemType.Group: + previous[mode].groups = previous[mode].groups?.includes(current.group) + ? previous[mode].groups + : [...(previous[mode].groups || []), current.group]; break; } }); @@ -128,43 +193,153 @@ const isWorkspacePermissionMode = (test: string): test is WorkspacePermissionMod test === WorkspacePermissionMode.Write; export const convertPermissionsToPermissionSettings = (permissions: SavedObjectPermissions) => { - const userPermissionSettings: WorkspacePermissionSetting[] = []; - const groupPermissionSettings: WorkspacePermissionSetting[] = []; + const permissionSettings: WorkspacePermissionSetting[] = []; + const finalPermissionSettings: WorkspacePermissionSetting[] = []; const settingType2Modes: { [key: string]: WorkspacePermissionMode[] } = {}; - Object.keys(permissions).forEach((mode) => { - if (!isWorkspacePermissionMode(mode)) { - return; - } - permissions[mode].users?.forEach((userId) => { - const settingTypeKey = `userId-${userId}`; + const processUsersOrGroups = ( + usersOrGroups: string[] | undefined, + type: WorkspacePermissionItemType, + mode: WorkspacePermissionMode + ) => { + usersOrGroups?.forEach((userOrGroup) => { + const settingTypeKey = `${type}-${userOrGroup}`; const modes = settingType2Modes[settingTypeKey] ?? []; modes.push(mode); if (modes.length === 1) { - userPermissionSettings.push({ - type: WorkspacePermissionItemType.User, - userId, + permissionSettings.push({ + // This id is for type safe, and will be overwrite in below. + id: 0, modes, + ...(type === WorkspacePermissionItemType.User + ? { type: WorkspacePermissionItemType.User, userId: userOrGroup } + : { type: WorkspacePermissionItemType.Group, group: userOrGroup }), }); settingType2Modes[settingTypeKey] = modes; } }); - permissions[mode].groups?.forEach((group) => { - const settingTypeKey = `group-${group}`; - const modes = settingType2Modes[settingTypeKey] ?? []; + }; - modes.push(mode); - if (modes.length === 1) { - groupPermissionSettings.push({ - type: WorkspacePermissionItemType.Group, - group, - modes, + Object.keys(permissions).forEach((mode) => { + if (isWorkspacePermissionMode(mode)) { + processUsersOrGroups(permissions[mode].users, WorkspacePermissionItemType.User, mode); + processUsersOrGroups(permissions[mode].groups, WorkspacePermissionItemType.Group, mode); + } + }); + + let id = 0; + /** + * One workspace permission setting may include multi setting options, + * for loop the workspace permission setting array to separate it to multi rows. + **/ + permissionSettings.forEach((currentPermissionSettings) => { + /** + * For loop the option id to workspace permission modes map, + * if one settings includes all permission modes in a specific option, + * add these permission modes to the result array. + */ + for (const key in optionIdToWorkspacePermissionModesMap) { + if (!Object.prototype.hasOwnProperty.call(optionIdToWorkspacePermissionModesMap, key)) { + continue; + } + const modesForCertainPermissionId = optionIdToWorkspacePermissionModesMap[key]; + if ( + modesForCertainPermissionId.every((mode) => currentPermissionSettings.modes?.includes(mode)) + ) { + finalPermissionSettings.push({ + ...currentPermissionSettings, + id, + modes: modesForCertainPermissionId, }); - settingType2Modes[settingTypeKey] = modes; + id++; } - }); + } }); - return [...userPermissionSettings, ...groupPermissionSettings]; + return finalPermissionSettings; +}; + +export const validateWorkspaceForm = ( + formData: Omit, 'permissionSettings'> & { + permissionSettings?: Array< + Pick & Partial + >; + } +) => { + const formErrors: WorkspaceFormErrors = {}; + const { name, description, permissionSettings } = formData; + if (name) { + if (!isValidFormTextInput(name)) { + formErrors.name = i18n.translate('workspace.form.detail.name.invalid', { + defaultMessage: 'Invalid workspace name', + }); + } + } else { + formErrors.name = i18n.translate('workspace.form.detail.name.empty', { + defaultMessage: "Name can't be empty.", + }); + } + if (description && !isValidFormTextInput(description)) { + formErrors.description = i18n.translate('workspace.form.detail.description.invalid', { + defaultMessage: 'Invalid workspace description', + }); + } + if (permissionSettings) { + const permissionSettingsErrors: { [key: number]: string } = {}; + for (let i = 0; i < permissionSettings.length; i++) { + const setting = permissionSettings[i]; + if (!setting.type) { + permissionSettingsErrors[setting.id] = i18n.translate( + 'workspace.form.permission.invalidate.type', + { + defaultMessage: 'Invalid type', + } + ); + } else if (!setting.modes || setting.modes.length === 0) { + permissionSettingsErrors[setting.id] = i18n.translate( + 'workspace.form.permission.invalidate.modes', + { + defaultMessage: 'Invalid permission modes', + } + ); + } else if (setting.type === WorkspacePermissionItemType.User && !setting.userId) { + permissionSettingsErrors[setting.id] = i18n.translate( + 'workspace.form.permission.invalidate.userId', + { + defaultMessage: 'Invalid user id', + } + ); + } else if (setting.type === WorkspacePermissionItemType.Group && !setting.group) { + permissionSettingsErrors[setting.id] = i18n.translate( + 'workspace.form.permission.invalidate.group', + { + defaultMessage: 'Invalid user group', + } + ); + } else if ( + isUserOrGroupPermissionSettingDuplicated( + permissionSettings.slice(0, i), + setting as WorkspacePermissionSetting + ) + ) { + permissionSettingsErrors[setting.id] = i18n.translate( + 'workspace.form.permission.invalidate.group', + { + defaultMessage: 'Duplicate permission setting', + } + ); + } + } + if (Object.keys(permissionSettingsErrors).length > 0) { + formErrors.permissionSettings = permissionSettingsErrors; + } + } + return formErrors; +}; + +export const generateNextPermissionSettingsId = (permissionSettings: Array<{ id: number }>) => { + return permissionSettings.length === 0 + ? 0 + : Math.max(...permissionSettings.map(({ id }) => id)) + 1; }; diff --git a/src/plugins/workspace/public/components/workspace_form/workspace_feature_selector.test.tsx b/src/plugins/workspace/public/components/workspace_form/workspace_feature_selector.test.tsx new file mode 100644 index 000000000000..0875b0d1ff10 --- /dev/null +++ b/src/plugins/workspace/public/components/workspace_form/workspace_feature_selector.test.tsx @@ -0,0 +1,92 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import React from 'react'; +import { fireEvent, render } from '@testing-library/react'; +import { + WorkspaceFeatureSelector, + WorkspaceFeatureSelectorProps, +} from './workspace_feature_selector'; +import { AppNavLinkStatus } from '../../../../../core/public'; + +const setup = (options?: Partial) => { + const onChangeMock = jest.fn(); + const applications = [ + { + id: 'app-1', + title: 'App 1', + category: { id: 'category-1', label: 'Category 1' }, + navLinkStatus: AppNavLinkStatus.visible, + }, + { + id: 'app-2', + title: 'App 2', + category: { id: 'category-1', label: 'Category 1' }, + navLinkStatus: AppNavLinkStatus.visible, + }, + { + id: 'app-3', + title: 'App 3', + category: { id: 'category-2', label: 'Category 2' }, + navLinkStatus: AppNavLinkStatus.visible, + }, + { + id: 'app-4', + title: 'App 4', + navLinkStatus: AppNavLinkStatus.visible, + }, + ]; + const renderResult = render( + + ); + return { + renderResult, + onChangeMock, + }; +}; + +describe('WorkspaceFeatureSelector', () => { + it('should call onChange with clicked feature', () => { + const { renderResult, onChangeMock } = setup(); + + expect(onChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getByText('App 1')); + expect(onChangeMock).toHaveBeenCalledWith(['app-1']); + }); + it('should call onChange with empty array after selected feature clicked', () => { + const { renderResult, onChangeMock } = setup({ + selectedFeatures: ['app-2'], + }); + + expect(onChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getByText('App 2')); + expect(onChangeMock).toHaveBeenCalledWith([]); + }); + it('should call onChange with features under clicked group', () => { + const { renderResult, onChangeMock } = setup(); + + expect(onChangeMock).not.toHaveBeenCalled(); + fireEvent.click( + renderResult.getByTestId('workspaceForm-workspaceFeatureVisibility-Category 1') + ); + expect(onChangeMock).toHaveBeenCalledWith(['app-1', 'app-2']); + }); + it('should call onChange without features under clicked group when group already selected', () => { + const { renderResult, onChangeMock } = setup({ + selectedFeatures: ['app-1', 'app-2', 'app-3'], + }); + + expect(onChangeMock).not.toHaveBeenCalled(); + fireEvent.click( + renderResult.getByTestId('workspaceForm-workspaceFeatureVisibility-Category 1') + ); + expect(onChangeMock).toHaveBeenCalledWith(['app-3']); + }); +}); diff --git a/src/plugins/workspace/public/components/workspace_form/workspace_feature_selector.tsx b/src/plugins/workspace/public/components/workspace_form/workspace_feature_selector.tsx index e172b851f1f3..da9deb174f52 100644 --- a/src/plugins/workspace/public/components/workspace_form/workspace_feature_selector.tsx +++ b/src/plugins/workspace/public/components/workspace_form/workspace_feature_selector.tsx @@ -13,21 +13,15 @@ import { EuiCheckboxGroupProps, EuiCheckboxProps, } from '@elastic/eui'; -import { i18n } from '@osd/i18n'; -import { groupBy } from 'lodash'; -import { DEFAULT_APP_CATEGORIES, PublicAppInfo } from '../../../../../core/public'; +import { PublicAppInfo } from '../../../../../core/public'; -import { WorkspaceFeature, WorkspaceFeatureGroup } from './types'; -import { isDefaultCheckedFeatureId, isWorkspaceFeatureGroup } from './utils'; -import { getAllExcludingManagementApps } from '../../utils'; +import { isWorkspaceFeatureGroup, convertApplicationsToFeaturesOrGroups } from './utils'; -const libraryCategoryLabel = i18n.translate('core.ui.libraryNavList.label', { - defaultMessage: 'Library', -}); - -interface WorkspaceFeatureSelectorProps { - applications: PublicAppInfo[]; +export interface WorkspaceFeatureSelectorProps { + applications: Array< + Pick + >; selectedFeatures: string[]; onChange: (newFeatures: string[]) => void; } @@ -37,43 +31,9 @@ export const WorkspaceFeatureSelector = ({ selectedFeatures, onChange, }: WorkspaceFeatureSelectorProps) => { - const featureOrGroups = useMemo(() => { - const transformedApplications = applications.map((app) => { - if (app.category?.id === DEFAULT_APP_CATEGORIES.opensearchDashboards.id) { - return { - ...app, - category: { - ...app.category, - label: libraryCategoryLabel, - }, - }; - } - return app; - }); - const category2Applications = groupBy(transformedApplications, 'category.label'); - return Object.keys(category2Applications).reduce< - Array - >((previousValue, currentKey) => { - const apps = category2Applications[currentKey]; - const features = getAllExcludingManagementApps(apps).map(({ id, title }) => ({ - id, - name: title, - })); - if (features.length === 0) { - return previousValue; - } - if (currentKey === 'undefined') { - return [...previousValue, ...features]; - } - return [ - ...previousValue, - { - name: apps[0].category?.label || '', - features, - }, - ]; - }, []); - }, [applications]); + const featuresOrGroups = useMemo(() => convertApplicationsToFeaturesOrGroups(applications), [ + applications, + ]); const handleFeatureChange = useCallback( (featureId) => { @@ -95,14 +55,13 @@ export const WorkspaceFeatureSelector = ({ const handleFeatureGroupChange = useCallback( (e) => { - const featureOrGroup = featureOrGroups.find( + const featureOrGroup = featuresOrGroups.find( (item) => isWorkspaceFeatureGroup(item) && item.name === e.target.id ); if (!featureOrGroup || !isWorkspaceFeatureGroup(featureOrGroup)) { return; } const groupFeatureIds = featureOrGroup.features.map((feature) => feature.id); - // setSelectedFeatureIds((previousData) => { const notExistsIds = groupFeatureIds.filter((id) => !selectedFeatures.includes(id)); // Check all not selected features if not been selected in current group. if (notExistsIds.length > 0) { @@ -112,12 +71,12 @@ export const WorkspaceFeatureSelector = ({ // Need to un-check these features, if all features in group has been selected onChange(selectedFeatures.filter((featureId) => !groupFeatureIds.includes(featureId))); }, - [featureOrGroups, selectedFeatures, onChange] + [featuresOrGroups, selectedFeatures, onChange] ); return ( <> - {featureOrGroups.map((featureOrGroup) => { + {featuresOrGroups.map((featureOrGroup) => { const features = isWorkspaceFeatureGroup(featureOrGroup) ? featureOrGroup.features : []; const selectedIds = selectedFeatures.filter((id) => (isWorkspaceFeatureGroup(featureOrGroup) @@ -129,15 +88,6 @@ export const WorkspaceFeatureSelector = ({ ? featureOrGroup.name : featureOrGroup.id; - const categoryToDescription: { [key: string]: string } = { - [libraryCategoryLabel]: i18n.translate( - 'workspace.form.featureVisibility.libraryCategory.Description', - { - defaultMessage: 'Workspace-owned library items', - } - ), - }; - return ( @@ -145,10 +95,6 @@ export const WorkspaceFeatureSelector = ({ {featureOrGroup.name} - {isWorkspaceFeatureGroup(featureOrGroup) && - categoryToDescription[featureOrGroup.name] && ( - {categoryToDescription[featureOrGroup.name]} - )} @@ -163,10 +109,6 @@ export const WorkspaceFeatureSelector = ({ features.length > 0 ? ` (${selectedIds.length}/${features.length})` : '' }`} checked={selectedIds.length > 0} - disabled={ - !isWorkspaceFeatureGroup(featureOrGroup) && - isDefaultCheckedFeatureId(featureOrGroup.id) - } indeterminate={ isWorkspaceFeatureGroup(featureOrGroup) && selectedIds.length > 0 && @@ -179,7 +121,6 @@ export const WorkspaceFeatureSelector = ({ options={featureOrGroup.features.map((item) => ({ id: item.id, label: item.name, - disabled: isDefaultCheckedFeatureId(item.id), }))} idToSelectedMap={selectedIds.reduce( (previousValue, currentValue) => ({ diff --git a/src/plugins/workspace/public/components/workspace_form/workspace_form.tsx b/src/plugins/workspace/public/components/workspace_form/workspace_form.tsx index b340a71588c9..be977578b412 100644 --- a/src/plugins/workspace/public/components/workspace_form/workspace_form.tsx +++ b/src/plugins/workspace/public/components/workspace_form/workspace_form.tsx @@ -20,11 +20,11 @@ import { import { i18n } from '@osd/i18n'; import { WorkspaceBottomBar } from './workspace_bottom_bar'; -import { WorkspacePermissionSettingPanel } from './workspace_permission_setting_panel'; import { WorkspaceFormProps } from './types'; import { WorkspaceFormTabs } from './constants'; import { useWorkspaceForm } from './use_workspace_form'; import { WorkspaceFeatureSelector } from './workspace_feature_selector'; +import { WorkspacePermissionSettingPanel } from './workspace_permission_setting_panel'; export const WorkspaceForm = (props: WorkspaceFormProps) => { const { @@ -160,15 +160,18 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => { /> )} - {selectedTab === WorkspaceFormTabs.UsersAndPermissions && ( -

{usersAndPermissionsTitle}

+

+ {i18n.translate('workspace.form.usersAndPermissions.title', { + defaultMessage: 'Users & Permissions', + })} +

) => { + const onGroupOrUserIdChangeMock = jest.fn(); + const onPermissionModesChangeMock = jest.fn(); + const onDeleteMock = jest.fn(); + const renderResult = render( + + ); + return { + renderResult, + onGroupOrUserIdChangeMock, + onPermissionModesChangeMock, + onDeleteMock, + }; +}; + +describe('WorkspacePermissionSettingInput', () => { + it('should render consistent user id and permission modes', () => { + const { renderResult } = setup({ + userId: 'foo', + modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], + }); + + expect(renderResult.getByText('foo')).toBeInTheDocument(); + expect(renderResult.getByText('Read')).toBeInTheDocument(); + expect( + renderResult.getByText('Read').closest('.euiButtonGroupButton-isSelected') + ).toBeInTheDocument(); + }); + it('should render consistent group id and permission modes', () => { + const { renderResult } = setup({ + type: WorkspacePermissionItemType.Group, + group: 'bar', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Read], + }); + + expect(renderResult.getByText('bar')).toBeInTheDocument(); + expect(renderResult.getByText('Read & Write')).toBeInTheDocument(); + expect( + renderResult.getByText('Read & Write').closest('.euiButtonGroupButton-isSelected') + ).toBeInTheDocument(); + }); + it('should call onGroupOrUserIdChange with user id', () => { + const { renderResult, onGroupOrUserIdChangeMock } = setup(); + + expect(onGroupOrUserIdChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getByText('Select')); + fireEvent.input(renderResult.getByTestId('comboBoxSearchInput'), { + target: { value: 'user1' }, + }); + fireEvent.blur(renderResult.getByTestId('comboBoxSearchInput')); + expect(onGroupOrUserIdChangeMock).toHaveBeenCalledWith({ type: 'user', userId: 'user1' }, 0); + }); + it('should call onGroupOrUserIdChange with group', () => { + const { renderResult, onGroupOrUserIdChangeMock } = setup({ + type: WorkspacePermissionItemType.Group, + }); + + expect(onGroupOrUserIdChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getByText('Select')); + fireEvent.input(renderResult.getByTestId('comboBoxSearchInput'), { + target: { value: 'group' }, + }); + fireEvent.blur(renderResult.getByTestId('comboBoxSearchInput')); + expect(onGroupOrUserIdChangeMock).toHaveBeenCalledWith({ type: 'group', group: 'group' }, 0); + }); + + it('should call onGroupOrUserIdChange without user id after clear button clicked', () => { + const { renderResult, onGroupOrUserIdChangeMock } = setup({ + userId: 'foo', + }); + + expect(onGroupOrUserIdChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getByTestId('comboBoxClearButton')); + expect(onGroupOrUserIdChangeMock).toHaveBeenCalledWith({ type: 'user' }, 0); + }); + + it('should call onPermissionModesChange with permission modes after permission modes changed', () => { + const { renderResult, onPermissionModesChangeMock } = setup({}); + + expect(onPermissionModesChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getByText('Admin')); + expect(onPermissionModesChangeMock).toHaveBeenCalledWith(['library_write', 'write'], 0); + }); + + it('should call onDelete with index after delete button clicked', () => { + const { renderResult, onDeleteMock } = setup(); + + expect(onDeleteMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getByLabelText('Delete permission setting')); + expect(onDeleteMock).toHaveBeenCalledWith(0); + }); +}); diff --git a/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_input.tsx b/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_input.tsx index e17f99b0d15b..099ae33bc23c 100644 --- a/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_input.tsx +++ b/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_input.tsx @@ -100,7 +100,6 @@ export const WorkspacePermissionSettingInput = ({ onChange={handleGroupOrUserIdChange} placeholder="Select" style={{ width: 200 }} - data-test-subj={`workspaceForm-permissionSettingPanel-${index}-userId`} />
diff --git a/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_panel.test.tsx b/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_panel.test.tsx new file mode 100644 index 000000000000..8415405e8e98 --- /dev/null +++ b/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_panel.test.tsx @@ -0,0 +1,199 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import React from 'react'; +import { fireEvent, render } from '@testing-library/react'; +import { + WorkspacePermissionSettingPanel, + WorkspacePermissionSettingPanelProps, +} from './workspace_permission_setting_panel'; +import { WorkspacePermissionItemType } from './constants'; +import { WorkspacePermissionMode } from '../../../common/constants'; + +const setup = (options?: Partial) => { + const onChangeMock = jest.fn(); + const permissionSettings = [ + { + id: 0, + type: WorkspacePermissionItemType.User, + userId: 'foo', + modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], + }, + { + id: 1, + type: WorkspacePermissionItemType.Group, + group: 'bar', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Read], + }, + ]; + const renderResult = render( + + ); + return { + renderResult, + onChangeMock, + }; +}; + +describe('WorkspacePermissionSettingInput', () => { + it('should render consistent user and group permissions', () => { + const { renderResult } = setup(); + + expect(renderResult.getByText('foo')).toBeInTheDocument(); + expect( + renderResult.getAllByText('Read')[0].closest('.euiButtonGroupButton-isSelected') + ).toBeInTheDocument(); + + expect(renderResult.getByText('bar')).toBeInTheDocument(); + expect( + renderResult.getAllByText('Read & Write')[1].closest('.euiButtonGroupButton-isSelected') + ).toBeInTheDocument(); + }); + + it('should call onChange with new user permission modes', () => { + const { renderResult, onChangeMock } = setup(); + + expect(onChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getAllByText('Read & Write')[0]); + expect(onChangeMock).toHaveBeenCalledWith([ + { + id: 0, + type: WorkspacePermissionItemType.User, + userId: 'foo', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Read], + }, + { + id: 1, + type: WorkspacePermissionItemType.Group, + group: 'bar', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Read], + }, + ]); + }); + it('should call onChange with new group permission modes', () => { + const { renderResult, onChangeMock } = setup(); + + expect(onChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getAllByText('Admin')[1]); + expect(onChangeMock).toHaveBeenCalledWith([ + { + id: 0, + type: WorkspacePermissionItemType.User, + userId: 'foo', + modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], + }, + { + id: 1, + type: WorkspacePermissionItemType.Group, + group: 'bar', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write], + }, + ]); + }); + + it('should call onChange with new user permission setting after add new button click', () => { + const { renderResult, onChangeMock } = setup({ + permissionSettings: [], + }); + + expect(onChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getByTestId('workspaceForm-permissionSettingPanel-user-addNew')); + expect(onChangeMock).toHaveBeenCalledWith([ + { + id: 0, + type: WorkspacePermissionItemType.User, + modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], + }, + ]); + }); + + it('should call onChange with new group permission setting after add new button click', () => { + const { renderResult, onChangeMock } = setup({ + permissionSettings: [], + }); + + expect(onChangeMock).not.toHaveBeenCalled(); + fireEvent.click(renderResult.getByTestId('workspaceForm-permissionSettingPanel-group-addNew')); + expect(onChangeMock).toHaveBeenCalledWith([ + { + id: 0, + type: WorkspacePermissionItemType.Group, + modes: [WorkspacePermissionMode.LibraryRead, WorkspacePermissionMode.Read], + }, + ]); + }); + + it('should not able to delete last user admin permission setting', () => { + const { renderResult } = setup({ + lastAdminItemDeletable: false, + permissionSettings: [ + { + id: 0, + type: WorkspacePermissionItemType.User, + userId: 'foo', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write], + }, + ], + }); + + expect(renderResult.getByLabelText('Delete permission setting')).toBeDisabled(); + }); + + it('should not able to delete last group admin permission setting', () => { + const { renderResult } = setup({ + lastAdminItemDeletable: false, + permissionSettings: [ + { + id: 0, + type: WorkspacePermissionItemType.Group, + group: 'bar', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write], + }, + ], + }); + + expect(renderResult.getByLabelText('Delete permission setting')).toBeDisabled(); + }); + + it('should able to delete permission setting if more than one admin permission', () => { + const { renderResult } = setup({ + lastAdminItemDeletable: false, + permissionSettings: [ + { + id: 0, + type: WorkspacePermissionItemType.User, + userId: 'foo', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write], + }, + { + id: 0, + type: WorkspacePermissionItemType.Group, + group: 'bar', + modes: [WorkspacePermissionMode.LibraryWrite, WorkspacePermissionMode.Write], + }, + ], + }); + + expect(renderResult.getAllByLabelText('Delete permission setting')[0]).not.toBeDisabled(); + expect(renderResult.getAllByLabelText('Delete permission setting')[1]).not.toBeDisabled(); + }); + + it('should render consistent errors', () => { + const { renderResult } = setup({ + errors: { '0': 'User permission setting error', '1': 'Group permission setting error' }, + }); + expect(renderResult.container.querySelectorAll('.euiFormRow')[0]).toHaveTextContent( + 'User permission setting error' + ); + expect(renderResult.container.querySelectorAll('.euiFormRow')[1]).toHaveTextContent( + 'Group permission setting error' + ); + }); +}); diff --git a/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_panel.tsx b/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_panel.tsx index 8d2dacc4165e..a4cb3b83f52b 100644 --- a/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_panel.tsx +++ b/src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_panel.tsx @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import React, { useCallback, useMemo, useState, useEffect } from 'react'; +import React, { useCallback, useEffect, useMemo, useRef } from 'react'; import { EuiButton, EuiFormRow, EuiText, EuiSpacer } from '@elastic/eui'; import { i18n } from '@osd/i18n'; import { WorkspacePermissionSetting } from './types'; @@ -16,13 +16,17 @@ import { WorkspacePermissionSettingInput, WorkspacePermissionSettingInputProps, } from './workspace_permission_setting_input'; -import { generateWorkspacePermissionItemKey, getPermissionModeId } from './utils'; +import { generateNextPermissionSettingsId, getPermissionModeId } from './utils'; -interface WorkspacePermissionSettingPanelProps { - errors?: string[]; +export interface WorkspacePermissionSettingPanelProps { + errors?: { [key: number]: string }; lastAdminItemDeletable: boolean; - permissionSettings: Array>; - onChange?: (value: Array>) => void; + permissionSettings: Array< + Pick & Partial + >; + onChange?: ( + value: Array & Partial> + ) => void; } interface UserOrGroupSectionProps @@ -30,6 +34,7 @@ interface UserOrGroupSectionProps title: string; nonDeletableIndex: number; type: WorkspacePermissionItemType; + nextIdGenerator: () => number; } const UserOrGroupSection = ({ @@ -37,61 +42,27 @@ const UserOrGroupSection = ({ title, errors, onChange, + nextIdGenerator, permissionSettings, nonDeletableIndex, }: UserOrGroupSectionProps) => { - const transformedValue = useMemo(() => { - if (!permissionSettings) { - return []; - } - const result: Array> = []; - /** - * One workspace permission setting may include multi setting options, - * for loop the workspace permission setting array to separate it to multi rows. - **/ - for (let i = 0; i < permissionSettings.length; i++) { - const valueItem = permissionSettings[i]; - // Incomplete workspace permission setting don't need to separate to multi rows - if ( - !valueItem.modes || - !valueItem.type || - (valueItem.type === 'user' && !valueItem.userId) || - (valueItem.type === 'group' && !valueItem.group) - ) { - result.push(valueItem); - continue; - } - /** - * For loop the option id to workspace permission modes map, - * if one settings includes all permission modes in a specific option, - * add these permission modes to the result array. - */ - for (const key in optionIdToWorkspacePermissionModesMap) { - if (!Object.prototype.hasOwnProperty.call(optionIdToWorkspacePermissionModesMap, key)) { - continue; - } - const modesForCertainPermissionId = optionIdToWorkspacePermissionModesMap[key]; - if (modesForCertainPermissionId.every((mode) => valueItem.modes?.includes(mode))) { - result.push({ ...valueItem, modes: modesForCertainPermissionId }); - } - } - } - return result; - }, [permissionSettings]); - // default permission mode is read const handleAddNewOne = useCallback(() => { onChange?.([ - ...(transformedValue ?? []), - { type, modes: optionIdToWorkspacePermissionModesMap[PermissionModeId.Read] }, + ...permissionSettings, + { + id: nextIdGenerator(), + type, + modes: optionIdToWorkspacePermissionModesMap[PermissionModeId.Read], + }, ]); - }, [onChange, type, transformedValue]); + }, [onChange, type, permissionSettings, nextIdGenerator]); const handleDelete = useCallback( (index: number) => { - onChange?.((transformedValue ?? []).filter((_item, itemIndex) => itemIndex !== index)); + onChange?.(permissionSettings.filter((_item, itemIndex) => itemIndex !== index)); }, - [onChange, transformedValue] + [onChange, permissionSettings] ); const handlePermissionModesChange = useCallback< @@ -99,12 +70,12 @@ const UserOrGroupSection = ({ >( (modes, index) => { onChange?.( - (transformedValue ?? []).map((item, itemIndex) => + permissionSettings.map((item, itemIndex) => index === itemIndex ? { ...item, modes } : item ) ); }, - [onChange, transformedValue] + [onChange, permissionSettings] ); const handleGroupOrUserIdChange = useCallback< @@ -112,26 +83,29 @@ const UserOrGroupSection = ({ >( (userOrGroupIdWithType, index) => { onChange?.( - (transformedValue ?? []).map((item, itemIndex) => + permissionSettings.map((item, itemIndex) => index === itemIndex - ? { ...userOrGroupIdWithType, ...(item.modes ? { modes: item.modes } : {}) } + ? { + id: item.id, + ...userOrGroupIdWithType, + ...(item.modes ? { modes: item.modes } : {}), + } : item ) ); }, - [onChange, transformedValue] + [onChange, permissionSettings] ); - // assume that group items are always deletable return (
{title} - {transformedValue?.map((item, index) => ( - - + {permissionSettings.map((item, index) => ( + + { + if ( + lastAdminItemDeletable || + // Permission setting can be deleted if there are more than one admin setting + [...userPermissionSettings, ...groupPermissionSettings].filter( + (permission) => + permission.modes && getPermissionModeId(permission.modes) === PermissionModeId.Admin + ).length > 1 + ) { + return { userNonDeletableIndex: -1, groupNonDeletableIndex: -1 }; + } + return { + userNonDeletableIndex: userPermissionSettings.findIndex( + (permission) => + permission.modes && getPermissionModeId(permission.modes) === PermissionModeId.Admin + ), + groupNonDeletableIndex: groupPermissionSettings.findIndex( + (permission) => + permission.modes && getPermissionModeId(permission.modes) === PermissionModeId.Admin + ), + }; + }, [userPermissionSettings, groupPermissionSettings, lastAdminItemDeletable]); + + const nextIdRef = useRef(generateNextPermissionSettingsId(permissionSettings)); + const handleUserPermissionSettingsChange = useCallback( (newSettings) => { onChange?.([...newSettings, ...groupPermissionSettings]); @@ -193,30 +192,18 @@ export const WorkspacePermissionSettingPanel = ({ [userPermissionSettings, onChange] ); - const nonDeletableIndex = useMemo(() => { - let userNonDeletableIndex = -1; - let groupNonDeletableIndex = -1; - const newPermissionSettings = [...userPermissionSettings, ...groupPermissionSettings]; - if (!lastAdminItemDeletable) { - const adminPermissionSettings = newPermissionSettings.filter( - (permission) => getPermissionModeId(permission.modes ?? []) === PermissionModeId.Admin - ); - if (adminPermissionSettings.length === 1) { - if (adminPermissionSettings[0].type === WorkspacePermissionItemType.User) { - userNonDeletableIndex = userPermissionSettings.findIndex( - (permission) => getPermissionModeId(permission.modes ?? []) === PermissionModeId.Admin - ); - } else { - groupNonDeletableIndex = groupPermissionSettings.findIndex( - (permission) => getPermissionModeId(permission.modes ?? []) === PermissionModeId.Admin - ); - } - } - } - return { userNonDeletableIndex, groupNonDeletableIndex }; - }, [userPermissionSettings, groupPermissionSettings, lastAdminItemDeletable]); + const nextIdGenerator = useCallback(() => { + const nextId = nextIdRef.current; + nextIdRef.current++; + return nextId; + }, []); - const { userNonDeletableIndex, groupNonDeletableIndex } = nonDeletableIndex; + useEffect(() => { + nextIdRef.current = Math.max( + nextIdRef.current, + generateNextPermissionSettingsId(permissionSettings) + ); + }, [permissionSettings]); return (
@@ -229,6 +216,7 @@ export const WorkspacePermissionSettingPanel = ({ nonDeletableIndex={userNonDeletableIndex} permissionSettings={userPermissionSettings} type={WorkspacePermissionItemType.User} + nextIdGenerator={nextIdGenerator} />
); diff --git a/src/plugins/workspace/public/components/workspace_updater/workspace_updater.test.tsx b/src/plugins/workspace/public/components/workspace_updater/workspace_updater.test.tsx index ff07076d99d1..2b56bf322388 100644 --- a/src/plugins/workspace/public/components/workspace_updater/workspace_updater.test.tsx +++ b/src/plugins/workspace/public/components/workspace_updater/workspace_updater.test.tsx @@ -130,7 +130,7 @@ describe('WorkspaceUpdater', () => { }); it('update workspace successfully', async () => { - const { getByTestId, getByText } = render(); + const { getByTestId, getByText, getAllByText } = render(); const nameInput = getByTestId('workspaceForm-workspaceDetails-nameInputText'); fireEvent.input(nameInput, { target: { value: 'test workspace name' }, @@ -153,7 +153,7 @@ describe('WorkspaceUpdater', () => { fireEvent.click(getByText('Users & Permissions')); fireEvent.click(getByTestId('workspaceForm-permissionSettingPanel-user-addNew')); - const userIdInput = getByTestId('workspaceForm-permissionSettingPanel-0-userId'); + const userIdInput = getAllByText('Select')[0]; fireEvent.click(userIdInput); fireEvent.input(getByTestId('comboBoxSearchInput'), { target: { value: 'test user id' }, diff --git a/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx b/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx index 1f67f2063d9b..201175c11c8e 100644 --- a/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx +++ b/src/plugins/workspace/public/components/workspace_updater/workspace_updater.tsx @@ -9,15 +9,17 @@ import { i18n } from '@osd/i18n'; import { useObservable } from 'react-use'; import { of } from 'rxjs'; import { useOpenSearchDashboards } from '../../../../opensearch_dashboards_react/public'; -import { WorkspaceForm, WorkspaceFormSubmitData, WorkspaceOperationType } from '../workspace_form'; import { WORKSPACE_OVERVIEW_APP_ID } from '../../../common/constants'; import { formatUrlWithWorkspaceId } from '../../../../../core/public/utils'; import { WorkspaceAttributeWithPermission } from '../../../../../core/types'; import { WorkspaceClient } from '../../workspace_client'; import { - convertPermissionSettingsToPermissions, + WorkspaceForm, + WorkspaceFormSubmitData, + WorkspaceOperationType, convertPermissionsToPermissionSettings, -} from '../workspace_form/utils'; + convertPermissionSettingsToPermissions, +} from '../workspace_form'; function getFormDataFromWorkspace( currentWorkspace: WorkspaceAttributeWithPermission | null | undefined @@ -37,7 +39,6 @@ export const WorkspaceUpdater = () => { const { services: { application, workspaces, notifications, http, workspaceClient }, } = useOpenSearchDashboards<{ workspaceClient: WorkspaceClient }>(); - const isPermissionEnabled = application?.capabilities.workspaces.permissionEnabled; const currentWorkspace = useObservable(workspaces ? workspaces.currentWorkspace$ : of(null)); @@ -45,10 +46,6 @@ export const WorkspaceUpdater = () => { getFormDataFromWorkspace(currentWorkspace) ); - useEffect(() => { - setCurrentWorkspaceFormData(getFormDataFromWorkspace(currentWorkspace)); - }, [currentWorkspace]); - const handleWorkspaceFormSubmit = useCallback( async (data: WorkspaceFormSubmitData) => { let result; @@ -68,6 +65,28 @@ export const WorkspaceUpdater = () => { attributes, convertPermissionSettingsToPermissions(permissionSettings) ); + if (result?.success) { + notifications?.toasts.addSuccess({ + title: i18n.translate('workspace.update.success', { + defaultMessage: 'Update workspace successfully', + }), + }); + if (application && http) { + // Redirect page after one second, leave one second time to show update successful toast. + window.setTimeout(() => { + window.location.href = formatUrlWithWorkspaceId( + application.getUrlForApp(WORKSPACE_OVERVIEW_APP_ID, { + absolute: true, + }), + currentWorkspace.id, + http.basePath + ); + }, 1000); + } + return; + } else { + throw new Error(result?.error ? result?.error : 'update workspace failed'); + } } catch (error) { notifications?.toasts.addDanger({ title: i18n.translate('workspace.update.failed', { @@ -77,36 +96,14 @@ export const WorkspaceUpdater = () => { }); return; } - if (result?.success) { - notifications?.toasts.addSuccess({ - title: i18n.translate('workspace.update.success', { - defaultMessage: 'Update workspace successfully', - }), - }); - if (application && http) { - // Redirect page after one second, leave one second time to show update successful toast. - window.setTimeout(() => { - window.location.href = formatUrlWithWorkspaceId( - application.getUrlForApp(WORKSPACE_OVERVIEW_APP_ID, { - absolute: true, - }), - currentWorkspace.id, - http.basePath - ); - }, 1000); - } - return; - } - notifications?.toasts.addDanger({ - title: i18n.translate('workspace.update.failed', { - defaultMessage: 'Failed to update workspace', - }), - text: result?.error, - }); }, [notifications?.toasts, currentWorkspace, http, application, workspaceClient] ); + useEffect(() => { + setCurrentWorkspaceFormData(getFormDataFromWorkspace(currentWorkspace)); + }, [currentWorkspace]); + if (!currentWorkspaceFormData) { return null; } @@ -130,7 +127,7 @@ export const WorkspaceUpdater = () => { onSubmit={handleWorkspaceFormSubmit} operationType={WorkspaceOperationType.Update} permissionEnabled={isPermissionEnabled} - permissionLastAdminItemDeletable + permissionLastAdminItemDeletable={false} /> )} diff --git a/src/plugins/workspace/public/workspace_client.ts b/src/plugins/workspace/public/workspace_client.ts index edd9613cd9c1..754064ed0c76 100644 --- a/src/plugins/workspace/public/workspace_client.ts +++ b/src/plugins/workspace/public/workspace_client.ts @@ -162,7 +162,7 @@ export class WorkspaceClient { /** * A bypass layer to get current workspace id */ - public getCurrentWorkspaceId(): IResponse { + public getCurrentWorkspaceId(): IResponse { const currentWorkspaceId = this.workspaces.currentWorkspaceId$.getValue(); if (!currentWorkspaceId) { return { @@ -182,7 +182,7 @@ export class WorkspaceClient { /** * Do a find in the latest workspace list with current workspace id */ - public async getCurrentWorkspace(): Promise> { + public async getCurrentWorkspace(): Promise> { const currentWorkspaceIdResp = this.getCurrentWorkspaceId(); if (currentWorkspaceIdResp.success) { const currentWorkspaceResp = await this.get(currentWorkspaceIdResp.result); @@ -202,10 +202,10 @@ export class WorkspaceClient { public async create( attributes: Omit, permissions?: SavedObjectPermissions - ): Promise>> { + ): Promise>> { const path = this.getPath(); - const result = await this.safeFetch(path, { + const result = await this.safeFetch(path, { method: 'POST', body: JSON.stringify({ attributes, diff --git a/src/plugins/workspace/server/integration_tests/routes.test.ts b/src/plugins/workspace/server/integration_tests/routes.test.ts index 175319074d10..002b6653a60b 100644 --- a/src/plugins/workspace/server/integration_tests/routes.test.ts +++ b/src/plugins/workspace/server/integration_tests/routes.test.ts @@ -356,6 +356,32 @@ describe('workspace service api integration test', () => { // Global workspace will be created by default after workspace list API called. expect(listResult.body.result.total).toEqual(2); }); + it('should able to update workspace with partial attributes', async () => { + const result: any = await osdTestServer.request + .post(root, `/api/workspaces`) + .send({ + attributes: omitId(testWorkspace), + }) + .expect(200); + + await osdTestServer.request + .put(root, `/api/workspaces/${result.body.result.id}`) + .send({ + attributes: { + name: 'updated', + }, + }) + .expect(200); + + const getResult = await osdTestServer.request.get( + root, + `/api/workspaces/${result.body.result.id}` + ); + + expect(getResult.body.success).toEqual(true); + expect(getResult.body.result.name).toEqual('updated'); + expect(getResult.body.result.description).toEqual(testWorkspace.description); + }); }); describe('Duplicate saved objects APIs', () => { diff --git a/src/plugins/workspace/server/routes/index.ts b/src/plugins/workspace/server/routes/index.ts index 1693e4636017..2c002539aae6 100644 --- a/src/plugins/workspace/server/routes/index.ts +++ b/src/plugins/workspace/server/routes/index.ts @@ -30,14 +30,23 @@ const workspacePermissions = schema.recordOf( schema.recordOf(principalType, schema.arrayOf(schema.string()), {}) ); -const workspaceAttributesSchema = schema.object({ +const workspaceOptionalAttributesSchema = { description: schema.maybe(schema.string()), - name: schema.string(), features: schema.maybe(schema.arrayOf(schema.string())), color: schema.maybe(schema.string()), icon: schema.maybe(schema.string()), defaultVISTheme: schema.maybe(schema.string()), reserved: schema.maybe(schema.boolean()), +}; + +const createWorkspaceAttributesSchema = schema.object({ + name: schema.string(), + ...workspaceOptionalAttributesSchema, +}); + +const updateWorkspaceAttributesSchema = schema.object({ + name: schema.maybe(schema.string()), + ...workspaceOptionalAttributesSchema, }); export function registerRoutes({ @@ -121,7 +130,7 @@ export function registerRoutes({ path: `${WORKSPACES_API_BASE_URL}`, validate: { body: schema.object({ - attributes: workspaceAttributesSchema, + attributes: createWorkspaceAttributesSchema, permissions: schema.maybe(workspacePermissions), }), }, @@ -167,7 +176,7 @@ export function registerRoutes({ id: schema.string(), }), body: schema.object({ - attributes: workspaceAttributesSchema, + attributes: updateWorkspaceAttributesSchema, permissions: schema.maybe(workspacePermissions), }), }, diff --git a/src/plugins/workspace/server/types.ts b/src/plugins/workspace/server/types.ts index bb8073b3a857..6f903389fff8 100644 --- a/src/plugins/workspace/server/types.ts +++ b/src/plugins/workspace/server/types.ts @@ -97,7 +97,7 @@ export interface IWorkspaceClientImpl { update( requestDetail: IRequestDetail, id: string, - payload: Omit + payload: Partial> ): Promise>; /** * Delete a given workspace diff --git a/src/plugins/workspace/server/workspace_client.ts b/src/plugins/workspace/server/workspace_client.ts index b4c8f2ebd637..ac16744e67d6 100644 --- a/src/plugins/workspace/server/workspace_client.ts +++ b/src/plugins/workspace/server/workspace_client.ts @@ -244,7 +244,7 @@ export class WorkspaceClient implements IWorkspaceClientImpl { public async update( requestDetail: IRequestDetail, id: string, - payload: Omit + payload: Partial> ): Promise> { const { permissions, ...attributes } = payload; try { @@ -267,12 +267,16 @@ export class WorkspaceClient implements IWorkspaceClientImpl { throw new Error(DUPLICATE_WORKSPACE_NAME_ERROR); } } - await client.create>(WORKSPACE_TYPE, attributes, { - id, - permissions, - overwrite: true, - version: workspaceInDB.version, - }); + await client.create>( + WORKSPACE_TYPE, + { ...workspaceInDB.attributes, ...attributes }, + { + id, + permissions, + overwrite: true, + version: workspaceInDB.version, + } + ); return { success: true, result: true,