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 update workspace page #6270

Merged
merged 7 commits into from
Apr 5, 2024

Conversation

gaobinlong
Copy link
Contributor

@gaobinlong gaobinlong commented Mar 26, 2024

Description

This PR is followed by #6179, which adds the update workspace page that can be used to update the workspace's attributes such as name, description and feature set etc.

For the implementation, this new page leverages most of the existing components in the create workspace page.

Issues Resolved

#6269

Screenshot

image
2024-03-26.14.57.23.mov

Testing the changes

  1. Clone the latest code and run yarn osd bootstrap
  2. Modify config/opensearch_dashboards.yml, add workspace.enabled: true
  3. Run yarn start --no-base-path
  4. Create a workspace firstly, or click Select a workspace to enter the overview page of a workspace, the URL is like this: http://localhost:5601/w/DsNfKS/app/workspace_overview
  5. Visit the URL http://localhost:5601/w/DsNfKS/app/workspace_update.
  6. Update the name, description, feature set or other attributes for the workspace.

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: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
Copy link

codecov bot commented Mar 26, 2024

Codecov Report

Attention: Patch coverage is 71.79487% with 11 lines in your changes missing coverage. Please review.

Project coverage is 67.49%. Comparing base (7352365) to head (5b38942).
Report is 555 commits behind head on main.

Files with missing lines Patch % Lines
...components/workspace_updater/workspace_updater.tsx 75.00% 2 Missing and 5 partials ⚠️
...components/workspace_creator/workspace_creator.tsx 75.00% 0 Missing and 2 partials ⚠️
src/plugins/workspace/public/plugin.ts 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6270      +/-   ##
==========================================
- Coverage   67.50%   67.49%   -0.01%     
==========================================
  Files        3370     3371       +1     
  Lines       65467    65498      +31     
  Branches    10564    10574      +10     
==========================================
+ Hits        44192    44210      +18     
- Misses      18700    18712      +12     
- Partials     2575     2576       +1     
Flag Coverage Δ
Linux_1 32.20% <71.79%> (+0.02%) ⬆️
Linux_2 55.58% <ø> (ø)
Linux_3 44.94% <ø> (+0.01%) ⬆️
Linux_4 35.11% <ø> (ø)
Windows_1 32.23% <71.79%> (+<0.01%) ⬆️
Windows_2 55.55% <ø> (ø)
Windows_3 44.94% <ø> (ø)
Windows_4 35.11% <ø> (?)

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: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
@gaobinlong
Copy link
Contributor Author

Hi @Flyingliuhub, could you help to review this PR, thank you~

Signed-off-by: gaobinlong <gbinlong@amazon.com>
Comment on lines 51 to 89
try {
const { ...attributes } = data;
result = await workspaceClient.update(currentWorkspace.id, attributes);
} catch (error) {
notifications?.toasts.addDanger({
title: i18n.translate('workspace.update.failed', {
defaultMessage: 'Failed to update workspace',
}),
text: error instanceof Error ? error.message : JSON.stringify(error),
});
return;
}
if (result?.success) {
notifications?.toasts.addSuccess({
title: i18n.translate('workspace.update.success', {
defaultMessage: 'Update workspace successfully',
}),
});
if (application && http) {
// Redirect page after one second, leave one second time to show update successful toast.
window.setTimeout(() => {
window.location.href = formatUrlWithWorkspaceId(
application.getUrlForApp(WORKSPACE_OVERVIEW_APP_ID, {
absolute: true,
}),
currentWorkspace.id,
http.basePath
);
}, 1000);
}
return;
}
notifications?.toasts.addDanger({
title: i18n.translate('workspace.update.failed', {
defaultMessage: 'Failed to update workspace',
}),
text: result?.error,
});
},
Copy link
Member

@xinruiba xinruiba Apr 2, 2024

Choose a reason for hiding this comment

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

Thanks for this change. Looks like the error handling is duplicate, what if we change it like this:

Suggested change
try {
const { ...attributes } = data;
result = await workspaceClient.update(currentWorkspace.id, attributes);
} catch (error) {
notifications?.toasts.addDanger({
title: i18n.translate('workspace.update.failed', {
defaultMessage: 'Failed to update workspace',
}),
text: error instanceof Error ? error.message : JSON.stringify(error),
});
return;
}
if (result?.success) {
notifications?.toasts.addSuccess({
title: i18n.translate('workspace.update.success', {
defaultMessage: 'Update workspace successfully',
}),
});
if (application && http) {
// Redirect page after one second, leave one second time to show update successful toast.
window.setTimeout(() => {
window.location.href = formatUrlWithWorkspaceId(
application.getUrlForApp(WORKSPACE_OVERVIEW_APP_ID, {
absolute: true,
}),
currentWorkspace.id,
http.basePath
);
}, 1000);
}
return;
}
notifications?.toasts.addDanger({
title: i18n.translate('workspace.update.failed', {
defaultMessage: 'Failed to update workspace',
}),
text: result?.error,
});
},
try {
const { ...attributes } = data;
result = await workspaceClient.update(currentWorkspace.id, attributes);
if (result?.success) {
notifications?.toasts.addSuccess({
title: i18n.translate('workspace.update.success', {
defaultMessage: 'Update workspace successfully',
}),
});
if (application && http) {
// Redirect page after one second, leave one second time to show update successful toast.
window.setTimeout(() => {
window.location.href = formatUrlWithWorkspaceId(
application.getUrlForApp(WORKSPACE_OVERVIEW_APP_ID, {
absolute: true,
}),
currentWorkspace.id,
http.basePath
);
}, 1000);
}
return;
} else {
throw Error('workspace.update.success failed')
}
} catch (error) {
notifications?.toasts.addDanger({
title: i18n.translate('workspace.update.failed', {
defaultMessage: 'Failed to update workspace',
}),
text: error instanceof Error ? error.message : JSON.stringify(error),
});
return;
}

The disadvantage in the suggestion change is, we also include window.setTimeout() and other window API call into the error handling, which is ok to me :)

What do you think?
Thanks~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review, the suggested change looks good to me, and I've changed the code in both update workspace page and create workspace page, please help to take a look.

Signed-off-by: gaobinlong <gbinlong@amazon.com>
Signed-off-by: gaobinlong <gbinlong@amazon.com>
@xinruiba
Copy link
Member

xinruiba commented Apr 3, 2024

LGTM, thanks

@BionIT BionIT merged commit 05a29a8 into opensearch-project:main Apr 5, 2024
68 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 19, 2024
* Add update workspace page

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Modify change log

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Increase test coverage

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Add a new test case

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Optimize some code

Signed-off-by: gaobinlong <gbinlong@amazon.com>

---------

Signed-off-by: gaobinlong <gbinlong@amazon.com>
(cherry picked from commit 05a29a8)
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 19, 2024
* Add update workspace page

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Modify change log

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Increase test coverage

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Add a new test case

Signed-off-by: gaobinlong <gbinlong@amazon.com>

* Optimize some code

Signed-off-by: gaobinlong <gbinlong@amazon.com>

---------

Signed-off-by: gaobinlong <gbinlong@amazon.com>
(cherry picked from commit 05a29a8)
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