[Workspace] Updated permission settings appearance #7652

Kapian1234 · 2024-08-08T03:44:09Z

Description

Changed permission control selector style.
Merged user section and group section, and added type switching.

Issues Resolved

Screenshot

Testing the changes

Changelog

feat: Update permission settings appearance

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

Signed-off-by: Kapian1234 <[email protected]>

codecov · 2024-08-08T04:00:47Z

Codecov Report

Attention: Patch coverage is 89.13043% with 5 lines in your changes missing coverage. Please review.

Project coverage is 63.80%. Comparing base (b4ae264) to head (668bf70).
Report is 175 commits behind head on main.

Files with missing lines	Patch %	Lines
...kspace_form/workspace_permission_setting_input.tsx	84.00%	1 Missing and 3 partials ⚠️
...kspace_form/workspace_permission_setting_panel.tsx	95.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7652   +/-   ##
=======================================
  Coverage   63.79%   63.80%           
=======================================
  Files        3656     3656           
  Lines       81190    81205   +15     
  Branches    12945    12950    +5     
=======================================
+ Hits        51794    51809   +15     
+ Misses      26217    26215    -2     
- Partials     3179     3181    +2

Flag	Coverage Δ
Linux_1	`30.08% <89.13%> (+0.02%)`	⬆️
Linux_2	`55.87% <ø> (ø)`
Linux_3	`40.42% <ø> (-0.01%)`	⬇️
Linux_4	`31.28% <ø> (ø)`
Windows_1	`30.09% <89.13%> (+0.02%)`	⬆️
Windows_2	`55.82% <ø> (ø)`
Windows_3	`40.42% <ø> (ø)`
Windows_4	`31.28% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Kapian1234 <[email protected]>

…apian1234/OpenSearch-Dashboards into workspace_permission_setting_panel_dev

Signed-off-by: Kapian1234 <[email protected]>

…ch-Dashboards into workspace_permission_setting_panel_dev

SuZhou-Joe · 2024-08-19T09:53:44Z

src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_input.tsx

+  const handleTypeChange = useCallback(
+    (options: Array<EuiSelectableOption<any>>) => {
+      for (const option of options) {
+        if (option.checked === 'on') {
+          onTypeChange(option.value, index);
+          setIsTypeListOpen(false);
+          return;
+        }
+      }
+    },
+    [index, onTypeChange]
+  );


We need a unit test for this function.

It's added here, but it doesn't seem to work src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_input.test.tsx:117

The root cause should be missing await for waitFor. The waitFor return a promise. The test case will be completed if doesn't await the promise object.

SuZhou-Joe · 2024-08-19T09:55:12Z

src/plugins/workspace/public/components/workspace_form/workspace_permission_setting_panel.tsx


  const handleDelete = useCallback(
    (index: number) => {
-      onChange?.(permissionSettings.filter((_item, itemIndex) => itemIndex !== index));
+      handlePermissionSettingsChange?.(
+        permissionSettings.filter((_item, itemIndex) => itemIndex !== index)


All the event are missing test coverage, can we add those?

Signed-off-by: Kapian1234 <[email protected]>

wanglam · 2024-08-20T05:46:18Z

...ugins/workspace/public/components/workspace_form/workspace_permission_setting_panel.test.tsx

@@ -124,12 +132,20 @@ describe('WorkspacePermissionSettingInput', () => {
    });
  });
  it('should call onChange with new group type', () => {
+    Object.defineProperty(HTMLElement.prototype, 'offsetHeight', {


Shall we move these offset mocks to the beforeEach / afterEach? I think we should clear it after test case complete.

wanglam · 2024-08-20T05:46:55Z

...ugins/workspace/public/components/workspace_form/workspace_permission_setting_input.test.tsx

@@ -112,3 +113,20 @@ describe('WorkspacePermissionSettingInput', () => {
    expect(onDeleteMock).toHaveBeenCalledWith(0);
  });
 });
+
+it('should call onTypeChange with types after types changed', () => {


Can we move this test case inside describe section?

sorry I didn't notice that

Signed-off-by: Kapian1234 <[email protected]>

…apian1234/OpenSearch-Dashboards into workspace_permission_setting_panel_dev

wanglam · 2024-08-20T08:39:50Z

...ugins/workspace/public/components/workspace_form/workspace_permission_setting_input.test.tsx

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


Should reset offset value after test case completed.

Shall we use beforeEach and afterEach here?

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

Signed-off-by: Kapian1234 <[email protected]>

wanglam · 2024-08-20T08:57:30Z

...ugins/workspace/public/components/workspace_form/workspace_permission_setting_input.test.tsx

+    const permissionToggleListButton = renderResult
+      .getAllByTestId('comboBoxToggleListButton')
+      .filter((button) => button.closest('[data-test-subj="workspace-permissionModeOptions"]'))[0];
+    fireEvent.click(permissionToggleListButton);


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.

wanglam · 2024-08-20T09:02:21Z

...ugins/workspace/public/components/workspace_form/workspace_permission_setting_input.test.tsx

+    'offsetHeight'
+  );
+  const originalOffsetWidth = Object.getOwnPropertyDescriptor(HTMLElement.prototype, 'offsetWidth');
+  Object.defineProperty(HTMLElement.prototype, 'offsetHeight', {


Shall we delete this Object.defineProperty?

Signed-off-by: Kapian1234 <[email protected]>

wanglam

LGTM

SuZhou-Joe

LGTM, good job!

* Changed permission control style Signed-off-by: Kapian1234 <[email protected]> * Merged the group section and user section, and added type switching. Signed-off-by: Kapian1234 <[email protected]> * Added isDisabled for permission control Signed-off-by: Kapian1234 <[email protected]> * Modified styles and added descriptions for permission control Signed-off-by: Kapian1234 <[email protected]> * / Signed-off-by: Kapian1234 <[email protected]> * Modified tests Signed-off-by: Kapian1234 <[email protected]> * Changeset file for PR #7652 created/updated * Modified tests Signed-off-by: Kapian1234 <[email protected]> * Modified tests Signed-off-by: Kapian1234 <[email protected]> * / Signed-off-by: Kapian1234 <[email protected]> * Modified tests Signed-off-by: Kapian1234 <[email protected]> * Remove the doc updates Signed-off-by: Kapian1234 <[email protected]> * Remove the doc updates Signed-off-by: Kapian1234 <[email protected]> * Resolve some issues Signed-off-by: Kapian1234 <[email protected]> * resolve some issues Signed-off-by: Kapian1234 <[email protected]> * Change type switch to popover style Signed-off-by: Kapian1234 <[email protected]> * Change type switch to popover style Signed-off-by: Kapian1234 <[email protected]> * Change type switch to popover style Signed-off-by: Kapian1234 <[email protected]> * Change type switch to popover style Signed-off-by: Kapian1234 <[email protected]> * Modify tests Signed-off-by: Kapian1234 <[email protected]> * Resolve some issues Signed-off-by: Kapian1234 <[email protected]> * Modify tests Signed-off-by: Kapian1234 <[email protected]> * Modify tests to increase coverage Signed-off-by: Kapian1234 <[email protected]> * Modify tests Signed-off-by: Kapian1234 <[email protected]> * Modify tests Signed-off-by: Kapian1234 <[email protected]> * Modify tests Signed-off-by: Kapian1234 <[email protected]> * Modify tests Signed-off-by: Kapian1234 <[email protected]> --------- Signed-off-by: Kapian1234 <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit aed03fa) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Changed permission control style * Merged the group section and user section, and added type switching. * Added isDisabled for permission control * Modified styles and added descriptions for permission control * / * Modified tests * Changeset file for PR #7652 created/updated * Modified tests * Modified tests * / * Modified tests * Remove the doc updates * Remove the doc updates * Resolve some issues * resolve some issues * Change type switch to popover style * Change type switch to popover style * Change type switch to popover style * Change type switch to popover style * Modify tests * Resolve some issues * Modify tests * Modify tests to increase coverage * Modify tests * Modify tests * Modify tests * Modify tests --------- (cherry picked from commit aed03fa) Signed-off-by: Kapian1234 <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

Kapian1234 added 6 commits August 2, 2024 16:02

Changed permission control style

14af74b

Signed-off-by: Kapian1234 <[email protected]>

Merged the group section and user section, and added type switching.

e28f4a9

Signed-off-by: Kapian1234 <[email protected]>

Added isDisabled for permission control

3d860e2

Signed-off-by: Kapian1234 <[email protected]>

Modified styles and added descriptions for permission control

4b7a368

Signed-off-by: Kapian1234 <[email protected]>

/

2bf47e1

Signed-off-by: Kapian1234 <[email protected]>

Modified tests

272b57f

Signed-off-by: Kapian1234 <[email protected]>

github-actions bot added the repeat-contributor label Aug 8, 2024

Changeset file for PR opensearch-project#7652 created/updated

afb5a25

Kapian1234 and others added 7 commits August 8, 2024 13:11

Modified tests

97505a9

Signed-off-by: Kapian1234 <[email protected]>

Merge branch 'workspace_permission_setting_panel_dev' of github.com:K…

3b94759

…apian1234/OpenSearch-Dashboards into workspace_permission_setting_panel_dev

Modified tests

3fe0e7b

Signed-off-by: Kapian1234 <[email protected]>

Merge branch 'main' into workspace_permission_setting_panel_dev

6b98eeb

Merge branch 'main' into workspace_permission_setting_panel_dev

7c19205

/

31a87ee

Signed-off-by: Kapian1234 <[email protected]>

Modified tests

55d8213

Signed-off-by: Kapian1234 <[email protected]>

Kapian1234 marked this pull request as ready for review August 9, 2024 06:54

Kapian1234 requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq and bandinib-amzn as code owners August 9, 2024 06:54

Merge branch 'main' of https://github.com/opensearch-project/OpenSear…

3f6247c

…ch-Dashboards into workspace_permission_setting_panel_dev

ruanyl previously approved these changes Aug 16, 2024

View reviewed changes

Kapian1234 requested a review from SuZhou-Joe August 16, 2024 09:06

SuZhou-Joe reviewed Aug 19, 2024

View reviewed changes

Modify tests to increase coverage

7a535e3

Signed-off-by: Kapian1234 <[email protected]>

Kapian1234 dismissed ruanyl’s stale review via 7a535e3 August 20, 2024 04:13

Merge branch 'main' into workspace_permission_setting_panel_dev

9292fe7

wanglam reviewed Aug 20, 2024

View reviewed changes

Kapian1234 added 2 commits August 20, 2024 14:30

Modify tests

4651d44

Signed-off-by: Kapian1234 <[email protected]>

Merge branch 'workspace_permission_setting_panel_dev' of github.com:K…

a0c8f94

…apian1234/OpenSearch-Dashboards into workspace_permission_setting_panel_dev

Kapian1234 requested review from ruanyl, SuZhou-Joe and wanglam August 20, 2024 07:49

wanglam reviewed Aug 20, 2024

View reviewed changes

Kapian1234 added 2 commits August 20, 2024 16:45

Modify tests

d58a9ca

Signed-off-by: Kapian1234 <[email protected]>

Modify tests

d266ca8

Signed-off-by: Kapian1234 <[email protected]>

wanglam reviewed Aug 20, 2024

View reviewed changes

Kapian1234 and others added 2 commits August 20, 2024 17:35

Modify tests

ed8bff2

Signed-off-by: Kapian1234 <[email protected]>

Merge branch 'main' into workspace_permission_setting_panel_dev

668bf70

wanglam approved these changes Aug 21, 2024

View reviewed changes

SuZhou-Joe approved these changes Aug 21, 2024

View reviewed changes

ruanyl approved these changes Aug 21, 2024

View reviewed changes

SuZhou-Joe merged commit aed03fa into opensearch-project:main Aug 21, 2024
66 of 67 checks passed

opensearch-trigger-bot bot mentioned this pull request Aug 21, 2024

[Backport 2.x] [Workspace] Updated permission settings appearance #7784

Merged

AMoo-Miki added the in-next label Sep 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Workspace] Updated permission settings appearance #7652

[Workspace] Updated permission settings appearance #7652

Kapian1234 commented Aug 8, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading

SuZhou-Joe Aug 19, 2024

Kapian1234 Aug 19, 2024

wanglam Aug 19, 2024

SuZhou-Joe Aug 19, 2024

Kapian1234 Aug 19, 2024

wanglam Aug 20, 2024

Kapian1234 Aug 20, 2024

wanglam Aug 20, 2024

Kapian1234 Aug 20, 2024

wanglam Aug 20, 2024

Kapian1234 Aug 20, 2024 •

edited

Loading

wanglam Aug 20, 2024

wanglam Aug 20, 2024

wanglam Aug 20, 2024

wanglam left a comment

SuZhou-Joe left a comment

[Workspace] Updated permission settings appearance #7652

[Workspace] Updated permission settings appearance #7652

Conversation

Kapian1234 commented Aug 8, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Aug 8, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kapian1234 Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanglam left a comment

Choose a reason for hiding this comment

SuZhou-Joe left a comment

Choose a reason for hiding this comment

Kapian1234 commented Aug 8, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading

Kapian1234 Aug 20, 2024 •

edited

Loading