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 all 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,51 @@
/*
* 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', () => {
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');

fireEvent.change(textarea, {
target: { value: 'a'.repeat(MAX_WORKSPACE_DESCRIPTION_LENGTH + 1) },
});

expect(onChangeMock).toHaveBeenCalledWith('a'.repeat(MAX_WORKSPACE_DESCRIPTION_LENGTH + 1));
});

it('should render the correct number of characters left when value larger than MAX_WORKSPACE_DESCRIPTION_LENGTH', () => {
render(
<WorkspaceDescriptionField
value={'a'.repeat(MAX_WORKSPACE_DESCRIPTION_LENGTH + 1)}
onChange={jest.fn()}
/>
);

const helpText = screen.getByText(new RegExp(`-1.+characters left\.`));
expect(helpText).toBeInTheDocument();
});

it('should render the correct number of characters left when value 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,66 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { EuiCompressedFormRow, EuiCompressedTextArea, EuiTextColor } 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) => {
onChange(e.currentTarget.value);
},
[onChange]
);
const leftCharacters = MAX_WORKSPACE_DESCRIPTION_LENGTH - (value?.length ?? 0);
const charactersOverflow = leftCharacters < 0;
SuZhou-Joe marked this conversation as resolved.
Show resolved Hide resolved

return (
<EuiCompressedFormRow
label={
<>
Description - <i>optional</i>
</>
}
isInvalid={!!error || charactersOverflow}
error={error}
helpText={
<EuiTextColor color={charactersOverflow ? 'danger' : 'subdued'}>
{i18n.translate('workspace.form.description.charactersLeft', {
defaultMessage: '{leftCharacters} characters left.',
values: {
leftCharacters,
},
})}
</EuiTextColor>
}
>
<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}
/>
</EuiCompressedFormRow>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* 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', () => {
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');

fireEvent.change(input, { target: { value: 'a'.repeat(MAX_WORKSPACE_NAME_LENGTH + 1) } });

expect(onChangeMock).toHaveBeenCalledWith('a'.repeat(MAX_WORKSPACE_NAME_LENGTH + 1));
});

it('should render the correct number of characters left when value greater than MAX_WORKSPACE_NAME_LENGTH', () => {
render(
<WorkspaceNameField value={'a'.repeat(MAX_WORKSPACE_NAME_LENGTH + 1)} onChange={jest.fn()} />
);

const helpText = screen.getByText(new RegExp(`-1.+characters left\.`));
expect(helpText).toBeInTheDocument();
});

it('should render the correct number of characters left when value is empty', () => {
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,70 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { EuiCompressedFieldText, EuiCompressedFormRow, EuiTextColor } 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) => {
onChange(e.currentTarget.value);
},
[onChange]
);
const leftCharacters = MAX_WORKSPACE_NAME_LENGTH - (value?.length ?? 0);
const charactersOverflow = leftCharacters < 0;

return (
<EuiCompressedFormRow
label={i18n.translate('workspace.form.workspaceDetails.name.label', {
defaultMessage: 'Name',
})}
helpText={
<>
<EuiTextColor color={charactersOverflow ? 'danger' : 'subdued'}>
{i18n.translate('workspace.form.name.charactersLeft', {
defaultMessage: '{leftCharacters} characters left.',
values: {
leftCharacters,
},
})}
</EuiTextColor>
<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 || charactersOverflow}
SuZhou-Joe marked this conversation as resolved.
Show resolved Hide resolved
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',
})}
/>
</EuiCompressedFormRow>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,7 @@
*/

import { useCallback, useState, FormEventHandler, useRef, useMemo } from 'react';
import {
htmlIdGenerator,
EuiFieldTextProps,
EuiTextAreaProps,
EuiColorPickerProps,
} from '@elastic/eui';
import { htmlIdGenerator, EuiColorPickerProps } from '@elastic/eui';

import { useApplications } from '../../hooks';
import {
Expand Down Expand Up @@ -38,7 +33,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 +128,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 +139,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
@@ -0,0 +1,60 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/
import React from 'react';
import { render, screen } from '@testing-library/react';
import { applicationServiceMock } from '../../../../../core/public/mocks';
import {
MAX_WORKSPACE_DESCRIPTION_LENGTH,
MAX_WORKSPACE_NAME_LENGTH,
} from '../../../common/constants';
import { WorkspaceCreateActionPanel } from './workspace_create_action_panel';

const mockApplication = applicationServiceMock.createStartContract();

describe('WorkspaceCreateActionPanel', () => {
const formId = 'workspaceForm';
const formData = {
name: 'Test Workspace',
description: 'This is a test workspace',
};

it('should disable the "Create Workspace" button when name exceeds the maximum length', () => {
const longName = 'a'.repeat(MAX_WORKSPACE_NAME_LENGTH + 1);
render(
<WorkspaceCreateActionPanel
formId={formId}
formData={{ name: longName, description: formData.description }}
application={mockApplication}
/>
);
const createButton = screen.getByText('Create workspace');
expect(createButton.closest('button')).toBeDisabled();
});

it('should disable the "Create Workspace" button when description exceeds the maximum length', () => {
const longDescription = 'a'.repeat(MAX_WORKSPACE_DESCRIPTION_LENGTH + 1);
render(
<WorkspaceCreateActionPanel
formId={formId}
formData={{ name: formData.name, description: longDescription }}
application={mockApplication}
/>
);
const createButton = screen.getByText('Create workspace');
expect(createButton.closest('button')).toBeDisabled();
});

it('should enable the "Create Workspace" button when name and description are within the maximum length', () => {
render(
<WorkspaceCreateActionPanel
formId={formId}
formData={formData}
application={mockApplication}
/>
);
const createButton = screen.getByText('Create workspace');
expect(createButton.closest('button')).not.toBeDisabled();
});
});
Loading
Loading