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.

Sign up for GitHub

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] Updated permission settings appearance #7652

Merged
merged 36 commits into from
Aug 21, 2024
Merged

[Workspace] Updated permission settings appearance #7652

Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
14af74b
Changed permission control style
Kapian1234 Aug 2, 2024
e28f4a9
Merged the group section and user section, and added type switching.
Kapian1234 Aug 5, 2024
3d860e2
Added isDisabled for permission control
Kapian1234 Aug 5, 2024
4b7a368
Modified styles and added descriptions for permission control
Kapian1234 Aug 6, 2024
2bf47e1
/
Kapian1234 Aug 7, 2024
272b57f
Modified tests
Kapian1234 Aug 8, 2024
afb5a25
Changeset file for PR #7652 created/updated
opensearch-changeset-bot[bot] Aug 8, 2024
97505a9
Modified tests
Kapian1234 Aug 8, 2024
3b94759
Merge branch 'workspace_permission_setting_panel_dev' of github.com:K…
Kapian1234 Aug 8, 2024
3fe0e7b
Modified tests
Kapian1234 Aug 8, 2024
6b98eeb
Merge branch 'main' into workspace_permission_setting_panel_dev
Kapian1234 Aug 8, 2024
7c19205
Merge branch 'main' into workspace_permission_setting_panel_dev
Kapian1234 Aug 9, 2024
31a87ee
/
Kapian1234 Aug 9, 2024
55d8213
Modified tests
Kapian1234 Aug 9, 2024
ac75a01
Remove the doc updates
Kapian1234 Aug 9, 2024
c136b48
Remove the doc updates
Kapian1234 Aug 9, 2024
7d33f1b
Resolve some issues
Kapian1234 Aug 9, 2024
b0b1b85
resolve some issues
Kapian1234 Aug 14, 2024
17ec17f
Change type switch to popover style
Kapian1234 Aug 15, 2024
432c5cd
Change type switch to popover style
Kapian1234 Aug 15, 2024
b9906d5
Merge branch 'workspace_permission_setting_panel_dev' of github.com:K…
Kapian1234 Aug 15, 2024
da3996d
Change type switch to popover style
Kapian1234 Aug 15, 2024
ed7c13a
Change type switch to popover style
Kapian1234 Aug 15, 2024
040b5e1
Modify tests
Kapian1234 Aug 15, 2024
55e9f02
Resolve some issues
Kapian1234 Aug 16, 2024
6dbcbea
Resolve some conflicts
Kapian1234 Aug 16, 2024
a0b4243
Modify tests
Kapian1234 Aug 16, 2024
3f6247c
Merge branch 'main' of https://github.com/opensearch-project/OpenSear…
Kapian1234 Aug 16, 2024
7a535e3
Modify tests to increase coverage
Kapian1234 Aug 20, 2024
9292fe7
Merge branch 'main' into workspace_permission_setting_panel_dev
Kapian1234 Aug 20, 2024
4651d44
Modify tests
Kapian1234 Aug 20, 2024
a0c8f94
Merge branch 'workspace_permission_setting_panel_dev' of github.com:K…
Kapian1234 Aug 20, 2024
d58a9ca
Modify tests
Kapian1234 Aug 20, 2024
d266ca8
Modify tests
Kapian1234 Aug 20, 2024
ed8bff2
Modify tests
Kapian1234 Aug 20, 2024
668bf70
Merge branch 'main' into workspace_permission_setting_panel_dev
Kapian1234 Aug 21, 2024
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/7652.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
feat:
- Update permission settings appearance ([#7652](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7652))
4 changes: 2 additions & 2 deletions src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,7 @@ describe('WorkspaceCreator', () => {
target: { value: 'test workspace name' },
});
fireEvent.click(getByTestId('workspaceUseCase-observability'));
fireEvent.click(getByTestId('workspaceForm-permissionSettingPanel-user-addNew'));
fireEvent.click(getByTestId('workspaceForm-permissionSettingPanel-addNew'));
fireEvent.click(getByTestId('workspaceForm-bottomBar-createButton'));
expect(workspaceClientCreate).toHaveBeenCalledWith(
expect.objectContaining({
Expand Down Expand Up @@ -309,7 +309,7 @@ describe('WorkspaceCreator', () => {
},
}
);
await waitFor(() => {
waitFor(() => {
expect(notificationToastsAddSuccess).toHaveBeenCalled();
});
expect(notificationToastsAddDanger).not.toHaveBeenCalled();
Expand Down
5 changes: 4 additions & 1 deletion src/plugins/workspace/public/components/workspace_detail/workspace_detail.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { fireEvent, render, screen } from '@testing-library/react';
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import React from 'react';
import { BehaviorSubject } from 'rxjs';
import { PublicAppInfo, WorkspaceObject } from 'opensearch-dashboards/public';
Expand Down Expand Up @@ -189,6 +189,9 @@ describe('WorkspaceDetail', () => {
const { getByText } = render(WorkspaceDetailPage({ workspacesService: workspaceService }));
fireEvent.click(getByText('Collaborators'));
expect(document.querySelector('#collaborators')).toHaveClass('euiTab-isSelected');
await waitFor(() => {
expect(screen.queryByText('Workspaces access')).not.toBeNull();
});
});

it('click on Data Sources tab when dataSource enabled', async () => {
Expand Down
5 changes: 5 additions & 0 deletions src/plugins/workspace/public/components/workspace_form/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ export const usersAndPermissionsTitle = i18n.translate('workspace.form.usersAndP
defaultMessage: 'Workspaces access',
});

export const usersAndPermissionsCreatePageTitle = i18n.translate(
'workspace.form.usersAndPermissions.createPage.title',
{ defaultMessage: 'Add collaborators' }
);

export const detailsName = i18n.translate('workspace.form.workspaceDetails.name.label', {
defaultMessage: 'Name',
});
Expand Down
16 changes: 11 additions & 5 deletions src/plugins/workspace/public/components/workspace_form/workspace_form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
*/

import React, { useRef } from 'react';
import { EuiPanel, EuiSpacer, EuiTitle, EuiForm } from '@elastic/eui';

import { EuiPanel, EuiSpacer, EuiTitle, EuiForm, EuiText } from '@elastic/eui';
import { i18n } from '@osd/i18n';
import { WorkspaceFormProps } from './types';
import { useWorkspaceForm } from './use_workspace_form';
import { WorkspacePermissionSettingPanel } from './workspace_permission_setting_panel';
Expand All @@ -16,7 +16,7 @@ import { SelectDataSourcePanel } from './select_data_source_panel';
import { EnterDetailsPanel } from './workspace_enter_details_panel';
import {
selectDataSourceTitle,
usersAndPermissionsTitle,
usersAndPermissionsCreatePageTitle,
workspaceDetailsTitle,
workspaceUseCaseTitle,
} from './constants';
Expand All @@ -36,7 +36,6 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => {
formData,
formErrors,
numberOfErrors,
numberOfChanges,
setName,
setDescription,
handleFormSubmit,
Expand Down Expand Up @@ -95,8 +94,15 @@ export const WorkspaceForm = (props: WorkspaceFormProps) => {
{permissionEnabled && (
<EuiPanel>
<EuiTitle size="s">
<h2>{usersAndPermissionsTitle}</h2>
<h2>{usersAndPermissionsCreatePageTitle}</h2>
</EuiTitle>
<EuiSpacer size="s" />
<EuiText size="xs" color="default">
{i18n.translate('workspace.form.usersAndPermissions.description', {
defaultMessage:
'You will be added as an owner to the workspace. Select additional users and user groups as workspace collaborators with different access levels.',
})}
</EuiText>
<EuiSpacer size="m" />
<WorkspacePermissionSettingPanel
errors={formErrors.permissionSettings?.fields}
Expand Down
38 changes: 28 additions & 10 deletions ...ns/workspace/public/components/workspace_form/workspace_permission_setting_input.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const setup = (options?: Partial<WorkspacePermissionSettingInputProps>) => {
const onGroupOrUserIdChangeMock = jest.fn();
const onPermissionModesChangeMock = jest.fn();
const onDeleteMock = jest.fn();
const onTypeChangeMock = jest.fn();
const renderResult = render(
<WorkspacePermissionSettingInput
index={0}
Expand All @@ -24,6 +25,7 @@ const setup = (options?: Partial<WorkspacePermissionSettingInputProps>) => {
onGroupOrUserIdChange={onGroupOrUserIdChangeMock}
onPermissionModesChange={onPermissionModesChangeMock}
onDelete={onDeleteMock}
onTypeChange={onTypeChangeMock}
{...options}
/>
);
Expand All @@ -32,6 +34,7 @@ const setup = (options?: Partial<WorkspacePermissionSettingInputProps>) => {
onGroupOrUserIdChangeMock,
onPermissionModesChangeMock,
onDeleteMock,
onTypeChangeMock,
};
};

Expand All @@ -44,9 +47,6 @@ describe('WorkspacePermissionSettingInput', () => {

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({
Expand All @@ -57,19 +57,16 @@ describe('WorkspacePermissionSettingInput', () => {

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 a user'));
fireEvent.input(renderResult.getByTestId('comboBoxSearchInput'), {
fireEvent.input(renderResult.getAllByTestId('comboBoxSearchInput')[0], {
target: { value: 'user1' },
});
fireEvent.blur(renderResult.getByTestId('comboBoxSearchInput'));
fireEvent.blur(renderResult.getAllByTestId('comboBoxSearchInput')[0]);
expect(onGroupOrUserIdChangeMock).toHaveBeenCalledWith({ type: 'user', userId: 'user1' }, 0);
});
it('should call onGroupOrUserIdChange with group', () => {
Expand All @@ -79,10 +76,10 @@ describe('WorkspacePermissionSettingInput', () => {

expect(onGroupOrUserIdChangeMock).not.toHaveBeenCalled();
fireEvent.click(renderResult.getByText('Select a user group'));
fireEvent.input(renderResult.getByTestId('comboBoxSearchInput'), {
fireEvent.input(renderResult.getAllByTestId('comboBoxSearchInput')[0], {
target: { value: 'group' },
});
fireEvent.blur(renderResult.getByTestId('comboBoxSearchInput'));
fireEvent.blur(renderResult.getAllByTestId('comboBoxSearchInput')[0]);
expect(onGroupOrUserIdChangeMock).toHaveBeenCalledWith({ type: 'group', group: 'group' }, 0);
});

Expand All @@ -100,6 +97,10 @@ describe('WorkspacePermissionSettingInput', () => {
const { renderResult, onPermissionModesChangeMock } = setup({});

expect(onPermissionModesChangeMock).not.toHaveBeenCalled();
const permissionToggleListButton = renderResult
.getAllByTestId('comboBoxToggleListButton')
.filter((button) => button.closest('[data-test-subj="workspace-permissionModeOptions"]'))[0];
fireEvent.click(permissionToggleListButton);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we refactor to below code?

import {  within } from '@testing-library/react';



    const permissionToggleListButton = within(
      renderResult.getAllByTestId('workspace-permissionModeOptions')[1]
    ).getByTestId('comboBoxToggleListButton');
    fireEvent.click(permissionToggleListButton);

That look more clarify to me.

fireEvent.click(renderResult.getByText('Owner'));
expect(onPermissionModesChangeMock).toHaveBeenCalledWith(['library_write', 'write'], 0);
});
Expand All @@ -111,4 +112,21 @@ describe('WorkspacePermissionSettingInput', () => {
fireEvent.click(renderResult.getByLabelText('Delete permission setting'));
expect(onDeleteMock).toHaveBeenCalledWith(0);
});

it('should call onTypeChange with types after types changed', () => {
const { renderResult, onTypeChangeMock } = setup({});
Object.defineProperty(HTMLElement.prototype, 'offsetHeight', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should reset offset value after test case completed.

Copy link
Contributor Author

@Kapian1234 Kapian1234 Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shall we use beforeEach and afterEach here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need put them in before / after if there only one test case use it.

configurable: true,
value: 600,
});
Object.defineProperty(HTMLElement.prototype, 'offsetWidth', {
configurable: true,
value: 600,
});
expect(onTypeChangeMock).not.toHaveBeenCalled();

fireEvent.click(renderResult.getByTestId('workspace-typeOptions'));
fireEvent.click(renderResult.getByText('Group'));
expect(onTypeChangeMock).toHaveBeenCalledWith('group', 0);
});
});
Loading
Loading