Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Workspace]Add name and description characters limitation #7656

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/7656.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- [Workspace]Add name and description characters limitation ([#7656](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7656))
3 changes: 3 additions & 0 deletions src/plugins/workspace/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,3 +185,6 @@ export const WORKSPACE_USE_CASES = Object.freeze({
export const MAX_WORKSPACE_PICKER_NUM = 3;
export const RECENT_WORKSPACES_KEY = 'recentWorkspaces';
export const CURRENT_USER_PLACEHOLDER = '%me%';

export const MAX_WORKSPACE_NAME_LENGTH = 40;
export const MAX_WORKSPACE_DESCRIPTION_LENGTH = 200;
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import React from 'react';
import { render, screen, fireEvent } from '@testing-library/react';

import { MAX_WORKSPACE_DESCRIPTION_LENGTH } from '../../../../common/constants';
import { WorkspaceDescriptionField } from './workspace_description_field';

describe('<WorkspaceDescriptionField />', () => {
it('should call onChange when the new value is within MAX_WORKSPACE_DESCRIPTION_LENGTH', () => {
const onChangeMock = jest.fn();
const value = 'test';

render(<WorkspaceDescriptionField value={value} onChange={onChangeMock} />);

const textarea = screen.getByPlaceholderText('Describe the workspace');
fireEvent.change(textarea, { target: { value: 'new value' } });

expect(onChangeMock).toHaveBeenCalledWith('new value');
});

it('should not call onChange when the new value exceeds MAX_WORKSPACE_DESCRIPTION_LENGTH', () => {
const onChangeMock = jest.fn();
const value = 'a'.repeat(MAX_WORKSPACE_DESCRIPTION_LENGTH);

render(<WorkspaceDescriptionField value={value} onChange={onChangeMock} />);

const textarea = screen.getByPlaceholderText('Describe the workspace');
fireEvent.change(textarea, {
target: { value: 'a'.repeat(MAX_WORKSPACE_DESCRIPTION_LENGTH + 1) },
});

expect(onChangeMock).not.toHaveBeenCalled();
});

it('should render the correct number of characters left when value is falsy', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When description is empty?

render(<WorkspaceDescriptionField value={undefined} onChange={jest.fn()} />);

const helpText = screen.getByText(
new RegExp(`${MAX_WORKSPACE_DESCRIPTION_LENGTH}.+characters left\.`)
);
expect(helpText).toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { EuiCompressedFormRow, EuiCompressedTextArea } from '@elastic/eui';
import { i18n } from '@osd/i18n';
import React, { useCallback } from 'react';

import { MAX_WORKSPACE_DESCRIPTION_LENGTH } from '../../../../common/constants';

export interface WorkspaceDescriptionFieldProps {
value?: string;
onChange: (newValue: string) => void;
error?: string;
readOnly?: boolean;
}

export const WorkspaceDescriptionField = ({
value,
error,
readOnly,
onChange,
}: WorkspaceDescriptionFieldProps) => {
const handleChange = useCallback(
(e) => {
const newValue = e.currentTarget.value;
if (newValue.length <= MAX_WORKSPACE_DESCRIPTION_LENGTH) {
onChange(newValue);
}
},
[onChange]
);

return (
<EuiCompressedFormRow
label={
<>
Description - <i>optional</i>
</>
}
isInvalid={!!error}
error={error}
helpText={<>{MAX_WORKSPACE_DESCRIPTION_LENGTH - (value?.length ?? 0)} characters left.</>}
>
<EuiCompressedTextArea
value={value}
onChange={handleChange}
data-test-subj="workspaceForm-workspaceDetails-descriptionInputText"
rows={4}
placeholder={i18n.translate('workspace.form.workspaceDetails.description.placeholder', {
defaultMessage: 'Describe the workspace',
})}
readOnly={readOnly}
maxLength={MAX_WORKSPACE_DESCRIPTION_LENGTH}
/>
</EuiCompressedFormRow>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import React from 'react';
import { render, screen, fireEvent } from '@testing-library/react';

import { MAX_WORKSPACE_NAME_LENGTH } from '../../../../common/constants';
import { WorkspaceNameField } from './workspace_name_field';

describe('<WorkspaceNameField />', () => {
it('should call onChange when the new value is within MAX_WORKSPACE_NAME_LENGTH', () => {
const onChangeMock = jest.fn();
const value = 'test';

render(<WorkspaceNameField value={value} onChange={onChangeMock} />);

const input = screen.getByPlaceholderText('Enter a name');
fireEvent.change(input, { target: { value: 'new value' } });

expect(onChangeMock).toHaveBeenCalledWith('new value');
});

it('should not call onChange when the new value exceeds MAX_WORKSPACE_NAME_LENGTH', () => {
const onChangeMock = jest.fn();
const value = 'a'.repeat(MAX_WORKSPACE_NAME_LENGTH);

render(<WorkspaceNameField value={value} onChange={onChangeMock} />);

const input = screen.getByPlaceholderText('Enter a name');
fireEvent.change(input, { target: { value: 'a'.repeat(MAX_WORKSPACE_NAME_LENGTH + 1) } });

expect(onChangeMock).not.toHaveBeenCalled();
});

it('should render the correct number of characters left when value is falsy', () => {
render(<WorkspaceNameField value={undefined} onChange={jest.fn()} />);

const helpText = screen.getByText(
new RegExp(`${MAX_WORKSPACE_NAME_LENGTH}.+characters left\.`)
);
expect(helpText).toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { EuiCompressedFieldText, EuiCompressedFormRow } from '@elastic/eui';
import { i18n } from '@osd/i18n';
import React, { useCallback } from 'react';

import { MAX_WORKSPACE_NAME_LENGTH } from '../../../../common/constants';

export interface WorkspaceNameFieldProps {
value?: string;
onChange: (newValue: string) => void;
error?: string;
readOnly?: boolean;
}

export const WorkspaceNameField = ({
value,
error,
readOnly,
onChange,
}: WorkspaceNameFieldProps) => {
const handleChange = useCallback(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we allow user to keep editing but display error message when exceed limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to this. Can be discuss with designer about that behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this part according the latest feedback. Could you help me take a look?

(e) => {
const newValue = e.currentTarget.value;
if (newValue.length <= MAX_WORKSPACE_NAME_LENGTH) {
onChange(newValue);
}
},
[onChange]
);

return (
<EuiCompressedFormRow
label={i18n.translate('workspace.form.workspaceDetails.name.label', {
defaultMessage: 'Name',
})}
helpText={
<>
{MAX_WORKSPACE_NAME_LENGTH - (value?.length ?? 0)} characters left. <br />
{i18n.translate('workspace.form.workspaceDetails.name.helpText', {
defaultMessage:
'Use a unique name for the workspace. Valid characters are a-z, A-Z, 0-9, (), [], _ (underscore), - (hyphen) and (space).',
})}
</>
}
isInvalid={!!error}
error={error}
>
<EuiCompressedFieldText
value={value}
onChange={handleChange}
readOnly={readOnly}
data-test-subj="workspaceForm-workspaceDetails-nameInputText"
placeholder={i18n.translate('workspace.form.workspaceDetails.name.placeholder', {
defaultMessage: 'Enter a name',
})}
maxLength={MAX_WORKSPACE_NAME_LENGTH}
/>
</EuiCompressedFormRow>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export const useWorkspaceForm = ({
permissionEnabled,
}: WorkspaceFormProps) => {
const applications = useApplications(application);
const [name, setName] = useState(defaultValues?.name);
const [name, setName] = useState(defaultValues?.name ?? '');
const [description, setDescription] = useState(defaultValues?.description);
const [color, setColor] = useState(defaultValues?.color);
const defaultValuesRef = useRef(defaultValues);
Expand Down Expand Up @@ -133,14 +133,6 @@ export const useWorkspaceForm = ({
[onSubmit, permissionEnabled]
);

const handleNameInputChange = useCallback<Required<EuiFieldTextProps>['onChange']>((e) => {
setName(e.target.value);
}, []);

const handleDescriptionChange = useCallback<Required<EuiTextAreaProps>['onChange']>((e) => {
setDescription(e.target.value);
}, []);

const handleColorChange = useCallback<Required<EuiColorPickerProps>['onChange']>((text) => {
setColor(text);
}, []);
Expand All @@ -152,12 +144,12 @@ export const useWorkspaceForm = ({
applications,
numberOfErrors,
numberOfChanges,
setName,
setDescription,
handleFormSubmit,
handleColorChange,
handleUseCaseChange,
handleNameInputChange,
setPermissionSettings,
setSelectedDataSources,
handleDescriptionChange,
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,13 @@ export const WorkspaceDetailForm = (props: WorkspaceFormProps) => {
formErrors,
numberOfErrors,
numberOfChanges,
setName,
setDescription,
handleFormSubmit,
handleColorChange,
handleUseCaseChange,
setPermissionSettings,
handleNameInputChange,
setSelectedDataSources,
handleDescriptionChange,
} = useWorkspaceForm(props);

const isDashboardAdmin = application?.capabilities?.dashboards?.isDashboardAdmin ?? false;
Expand Down Expand Up @@ -101,8 +101,8 @@ export const WorkspaceDetailForm = (props: WorkspaceFormProps) => {
description={formData.description}
color={formData.color}
readOnly={!!defaultValues?.reserved}
handleNameInputChange={handleNameInputChange}
handleDescriptionChange={handleDescriptionChange}
onNameChange={setName}
onDescriptionChange={setDescription}
handleColorChange={handleColorChange}
/>
</FormGroup>
Expand Down
Loading
Loading