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] use registered nav groups in workspace form #7221

Merged

Conversation

wanglam
Copy link
Contributor

@wanglam wanglam commented Jul 11, 2024

Description

In 2.15, we add the workspace use case selector in the workspace form. It used a fixed use cases list. In #7060 , we added a new concept nav group. The workspace form should use these registered nav groups as use cases and displayed in the workspace use case selector. This PR mainly includes below changes:

  1. Add available use cases property in workspace selector
  2. Subscribe registered use cases in the workspace plugin
  3. Pass registered use cases to workspace selector
  4. Remove not used workspaceConfigurableApps$

Issues Resolved

#7222

Screenshot

No UI changes

Testing the changes

The nav groups have not been displayed in the navigation, so we need to use unit tests to verify all changes.

  • For getRegisteredUseCases$ inside use case service
    Can refer src/plugins/workspace/public/services/use_case_service.test.ts
  • For app updater and registeredUseCase subscription
    Can refer src/plugins/workspace/public/plugin.test.ts
  • For workspace creator / updater / overview functionality
    Can refer below files:
    src/plugins/workspace/public/components/workspace_creator/workspace_creator.test.tsx
    src/plugins/workspace/public/components/workspace_updater/workspace_updater.test.tsx
    src/plugins/workspace/public/components/workspace_overview/workspace_overview.test.tsx

Changelog

  • feat: Use registered nav group as workspace use case

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

Copy link
Contributor

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Copy link

codecov bot commented Jul 11, 2024

Codecov Report

Attention: Patch coverage is 94.52055% with 4 lines in your changes missing coverage. Please review.

Project coverage is 67.53%. Comparing base (38ae65b) to head (6aafb87).
Report is 313 commits behind head on main.

Files with missing lines Patch % Lines
src/plugins/workspace/public/plugin.ts 90.47% 0 Missing and 2 partials ⚠️
...rkspace/public/components/workspace_list/index.tsx 83.33% 0 Missing and 1 partial ⚠️
...gins/workspace/public/services/use_case_service.ts 92.85% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7221      +/-   ##
==========================================
+ Coverage   67.52%   67.53%   +0.01%     
==========================================
  Files        3501     3502       +1     
  Lines       69340    69372      +32     
  Branches    11303    11311       +8     
==========================================
+ Hits        46824    46853      +29     
+ Misses      19764    19762       -2     
- Partials     2752     2757       +5     
Flag Coverage Δ
Linux_1 33.15% <94.52%> (+0.04%) ⬆️
Linux_2 55.46% <ø> (ø)
Linux_3 43.05% <ø> (-0.02%) ⬇️
Linux_4 34.71% <ø> (ø)
Windows_1 33.20% <94.52%> (+0.04%) ⬆️
Windows_2 55.41% <ø> (ø)
Windows_3 43.06% <ø> (ø)
Windows_4 34.71% <ø> (-0.01%) ⬇️

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.

order,
});

export const isEqualWorkspaceUseCase = (a: WorkspaceUseCase, b: WorkspaceUseCase) => {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check if the use case from constant equal to the one from navGroup? I thought match on id should be good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now, the isEqualWorkspaceUseCase only been used in UseCaseService. It's just for avoid duplicate changes fired. The appUpdater$ in filterNavLinks will generate new nav links. Then the getNavGroupsMap$ will emit new nav groups map. It will generate an infinite loop. So we add distinctUntilChanged after getNavGroupsMap$ to avoid these.

Copy link
Member

Choose a reason for hiding this comment

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

I c, thanks.

SuZhou-Joe
SuZhou-Joe previously approved these changes Jul 17, 2024
@ruanyl ruanyl merged commit fceba91 into opensearch-project:main Jul 19, 2024
66 of 67 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 19, 2024
* Add registered use cases

Signed-off-by: Lin Wang <[email protected]>

* Separate use case service and fix UTs

Signed-off-by: Lin Wang <[email protected]>

* Add test case for workspace plugin

Signed-off-by: Lin Wang <[email protected]>

* Fix workspace unit tests

Signed-off-by: Lin Wang <[email protected]>

* Changeset file for PR #7221 created/updated

* Remove workspaceConfigurableApp$ in component

Signed-off-by: Lin Wang <[email protected]>

* Remove no need workspaceConfigurableApps$ and add navGroupUpdater ut

Signed-off-by: Lin Wang <[email protected]>

* Fix type error

Signed-off-by: Lin Wang <[email protected]>

* Remove centered horizontal position

Signed-off-by: Lin Wang <[email protected]>

* Fix isDashboardAdmin in workspace creator unit tests

Signed-off-by: Lin Wang <[email protected]>

* Fix dynamic nav groups missing in workspace list

Signed-off-by: Lin Wang <[email protected]>

---------

Signed-off-by: Lin Wang <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit fceba91)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SuZhou-Joe pushed a commit that referenced this pull request Jul 20, 2024
* Add registered use cases



* Separate use case service and fix UTs



* Add test case for workspace plugin



* Fix workspace unit tests



* Changeset file for PR #7221 created/updated

* Remove workspaceConfigurableApp$ in component



* Remove no need workspaceConfigurableApps$ and add navGroupUpdater ut



* Fix type error



* Remove centered horizontal position



* Fix isDashboardAdmin in workspace creator unit tests



* Fix dynamic nav groups missing in workspace list



---------



(cherry picked from commit fceba91)

Signed-off-by: Lin Wang <[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>
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