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 workspace list page #6182

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

raintygao
Copy link
Contributor

@raintygao raintygao commented Mar 18, 2024

Description

In this PR, a new page will be added to workspace module. This page shows all workspaces in a table, user can view the name, id, description and features of each workspace. And users could search workspace through inputing name.

Issues Resolved

#6132

Screenshot

image
image

Testing the changes

  • Clone the latest code and run yarn osd bootstrap
  • Modify config/opensearch_dashboards.yml add workspace.enabled: true
  • Run yarn start --no-base-path
  • Visit workspace list page

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

codecov bot commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 67.44%. Comparing base (74e0df3) to head (d53b2a1).
Report is 564 commits behind head on main.

Files with missing lines Patch % Lines
...rkspace/public/components/workspace_list/index.tsx 75.00% 6 Missing and 5 partials ⚠️
.../delete_workspace_modal/delete_workspace_modal.tsx 89.47% 0 Missing and 2 partials ⚠️
src/plugins/workspace/public/plugin.ts 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6182      +/-   ##
==========================================
+ Coverage   67.42%   67.44%   +0.01%     
==========================================
  Files        3362     3366       +4     
  Lines       65258    65338      +80     
  Branches    10524    10541      +17     
==========================================
+ Hits        43999    44066      +67     
- Misses      18701    18708       +7     
- Partials     2558     2564       +6     
Flag Coverage Δ
Linux_1 32.14% <78.75%> (+0.08%) ⬆️
Linux_2 55.58% <100.00%> (+0.01%) ⬆️
Linux_3 44.82% <16.66%> (-0.02%) ⬇️
Linux_4 35.03% <16.66%> (-0.01%) ⬇️
Windows_1 32.16% <78.75%> (+0.08%) ⬆️
Windows_2 55.54% <100.00%> (+0.01%) ⬆️
Windows_3 44.84% <16.66%> (+<0.01%) ⬆️
Windows_4 35.03% <16.66%> (-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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

typo ? model instead of modal ?

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 think modal may be more appropriate because this is a modal UI component and references the modal component of OUI . What's your opinion?

Copy link
Collaborator

Choose a reason for hiding this comment

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

works then

@ZilongX
Copy link
Collaborator

ZilongX commented Mar 20, 2024

generally looking good, need to resolve the conflicts in the CHANGELOG though

@ZilongX
Copy link
Collaborator

ZilongX commented Mar 20, 2024

re-running failed checks

@raintygao raintygao requested a review from xinruiba as a code owner March 21, 2024 03:32
@ZilongX ZilongX self-requested a review March 21, 2024 16:21
ZilongX
ZilongX previously approved these changes Mar 21, 2024
}
};

export const updateWorkspace = ({ application, http }: Core, id: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The function name here is a little bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any recommendations?

Copy link
Member

@SuZhou-Joe SuZhou-Joe Mar 22, 2024

Choose a reason for hiding this comment

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

Suggested change
export const updateWorkspace = ({ application, http }: Core, id: string) => {
export const navigateToWorkspaceUpdatePage = ({ application, http }: Core, id: string) => {

Comment on lines 58 to 60
if (http && application && returnToHome) {
const homeUrl = application.getUrlForApp('home', {
path: '/',
absolute: false,
});
const targetUrl = http.basePath.prepend(http.basePath.remove(homeUrl), {
withoutWorkspace: true,
});
await application.navigateToUrl(targetUrl);
}
Copy link
Member

@SuZhou-Joe SuZhou-Joe Mar 22, 2024

Choose a reason for hiding this comment

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

DeleteWorkspaceModal should only care about if we delete the workspace successfully and gives a toast, it should be the component who embed the Modal to decide if jump to other page?

Copy link
Contributor Author

@raintygao raintygao Mar 22, 2024

Choose a reason for hiding this comment

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

Because both workspace update page and list page use this component and their delete logic is same, so I put this part of the logic in deleteModal for code reuse. There is a param called returnToHome to decide whether navigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im not sure if we could put delete logic in deleteModal to let it handle directly.

Copy link
Member

Choose a reason for hiding this comment

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

  • For update page, it will jump to the homepage when delete successfully.
  • For list page, it need to refresh the list page when delete successfully.

The DeleteWorkspaceModal should expose a function props named onDeleteSuccess so that parent component can decide what to do next. Pass returnToHome is coupling the DeleteModal and the handle logic of the parent component.

@seraphjiang
Copy link
Member

One question: How could a user find this page? From which menu could they navigate to this page?

@raintygao
Copy link
Contributor Author

One question: How could a user find this page? From which menu could they navigate to this page?

There is an entry named All workspaces in workspace dropdown menu #6150 , user could navigate to this page from this entry.
image

@seraphjiang
Copy link
Member

There is an entry named All workspaces in workspace dropdown menu #6150 , user could navigate to this page from this entry.

Got it, Thanks @raintygao for clarification

SuZhou-Joe
SuZhou-Joe previously approved these changes Mar 25, 2024
Copy link
Member

@SuZhou-Joe SuZhou-Joe left a comment

Choose a reason for hiding this comment

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

LGTM

@raintygao raintygao requested a review from ZilongX March 25, 2024 07:48
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
Signed-off-by: tygao <tygao@amazon.com>
@ZilongX ZilongX merged commit 2a94f32 into opensearch-project:main Mar 26, 2024
68 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 18, 2024
* feat: add workspace list

Signed-off-by: tygao <tygao@amazon.com>

* doc: update changelog

Signed-off-by: tygao <tygao@amazon.com>

* fix test for delete workspace modal (#299)

Signed-off-by: tygao <tygao@amazon.com>

* update function name and modal

Signed-off-by: tygao <tygao@amazon.com>

---------

Signed-off-by: tygao <tygao@amazon.com>
(cherry picked from commit 2a94f32)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
SuZhou-Joe pushed a commit that referenced this pull request Apr 18, 2024
* feat: add workspace list

Signed-off-by: tygao <tygao@amazon.com>

* doc: update changelog

Signed-off-by: tygao <tygao@amazon.com>

* fix test for delete workspace modal (#299)

Signed-off-by: tygao <tygao@amazon.com>

* update function name and modal

Signed-off-by: tygao <tygao@amazon.com>

---------

Signed-off-by: tygao <tygao@amazon.com>
(cherry picked from commit 2a94f32)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[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.

6 participants