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

feat: update create workspace page UI and implement unit tests #226

Merged
merged 37 commits into from
Oct 17, 2023
Merged

feat: update create workspace page UI and implement unit tests #226

merged 37 commits into from
Oct 17, 2023

Conversation

yuye-aws
Copy link
Collaborator

@yuye-aws yuye-aws commented Oct 13, 2023

Description

Issues Resolved

  1. Update create workspace page UI
  2. Implement unit tests

Screenshot

image image

image

Testing the changes

Check List

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

@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2023

Codecov Report

Merging #226 (8b22c35) into workspace (7a72a73) will decrease coverage by 5.81%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##           workspace     #226      +/-   ##
=============================================
- Coverage      66.19%   60.39%   -5.81%     
=============================================
  Files           3431     2162    -1269     
  Lines          65952    42121   -23831     
  Branches       10616     7728    -2888     
=============================================
- Hits           43659    25437   -18222     
+ Misses         19645    14993    -4652     
+ Partials        2648     1691     -957     
Flag Coverage Δ
Linux_1 ?
Linux_2 55.41% <ø> (ø)
Linux_3 42.77% <ø> (ø)
Linux_4 ?

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

see 1569 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yuye-aws yuye-aws requested review from ruanyl and gaobinlong October 13, 2023 04:45
Comment on lines 6 to 12
import React from 'react';
import { WorkspaceCreator } from './workspace_creator';
import { PublicAppInfo } from 'opensearch-dashboards/public';
import { fireEvent, render, waitFor } from '@testing-library/react';
import * as opensearchReactExports from '../../../../opensearch_dashboards_react/public';
import { BehaviorSubject } from 'rxjs';
import { WorkspaceFormSubmitData } from './workspace_form';
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better that we import absolute module in the beginning and then relative module. I do not know why OSD lint do not automatically format that but we'd better follow the standard pattern.

Comment on lines 14 to 17
jest.mock('../../../../opensearch_dashboards_react/public', () => ({
...jest.requireActual('../../../../opensearch_dashboards_react/public'),
__esModule: true,
}));
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we return the actual implementation of the target module, why do we doMock on the path?

Comment on lines 35 to 59
services: {
application: {
navigateToApp,
getUrlForApp: jest.fn(),
applications$: new BehaviorSubject<Map<string, PublicAppInfo>>(PublicAPPInfoMap as any),
},
http: {
basePath: {
remove: jest.fn(),
prepend: jest.fn(),
},
},
notifications: {
toasts: {
addDanger: notificationToastsAddDanger,
addSuccess: notificationToastsAddSuccess,
},
},
workspaceClient: {
create: workspaceClientCreate,
},
},
Copy link
Collaborator

@SuZhou-Joe SuZhou-Joe Oct 13, 2023

Choose a reason for hiding this comment

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

can we use

...coreServiceMock.create()
...{
   application: {...},
   notifications: {...},
   workspaceClient: {...}
}

In this case, if coreStart adds any new fields, we do not need to modify the mock here.

grow={false}
>
<EuiIcon size="l" type={item} color={value === item ? color : undefined} />
const icons = ['Glasses', 'Search', 'Bell', 'Package'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend moving this statement out of the function body if it is a static property.

/>
aria-label="Delete permission setting"
>
Remove
Copy link
Collaborator

Choose a reason for hiding this comment

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

i18n wrapper maybe?

color={'text'}
data-test-subj="workspaceForm-permissionSettingPanel-addNew"
>
Add New
Copy link
Collaborator

Choose a reason for hiding this comment

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

i18n wrapper maybe.

Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
yuye-aws and others added 19 commits October 16, 2023 19:27
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
…space_updater.tsx

Co-authored-by: SuZhou-Joe <suzhou@amazon.com>
…space_permission_setting_panel.tsx

Co-authored-by: SuZhou-Joe <suzhou@amazon.com>
…space_permission_setting_panel.tsx

Co-authored-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: SuZhou-Joe <suzhou@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
Signed-off-by: yuye-aws <yuyezhu@amazon.com>
@yuye-aws yuye-aws requested a review from wanglam October 17, 2023 07:33
@yuye-aws yuye-aws merged commit f00cb3c into ruanyl:workspace Oct 17, 2023
20 of 21 checks passed
@yuye-aws yuye-aws deleted the update-create branch January 3, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants