-
Notifications
You must be signed in to change notification settings - Fork 916
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
Conversation
Signed-off-by: gaobinlong <[email protected]>
Signed-off-by: gaobinlong <[email protected]>
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: gaobinlong <[email protected]>
Signed-off-by: gaobinlong <[email protected]>
Hi @Flyingliuhub, could you help to review this PR, thank you~ |
Signed-off-by: gaobinlong <[email protected]>
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, | ||
}); | ||
}, |
There was a problem hiding this comment.
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:
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~
There was a problem hiding this comment.
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 <[email protected]>
Signed-off-by: gaobinlong <[email protected]>
LGTM, thanks |
* Add update workspace page Signed-off-by: gaobinlong <[email protected]> * Modify change log Signed-off-by: gaobinlong <[email protected]> * Increase test coverage Signed-off-by: gaobinlong <[email protected]> * Add a new test case Signed-off-by: gaobinlong <[email protected]> * Optimize some code Signed-off-by: gaobinlong <[email protected]> --------- Signed-off-by: gaobinlong <[email protected]> (cherry picked from commit 05a29a8) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
* Add update workspace page Signed-off-by: gaobinlong <[email protected]> * Modify change log Signed-off-by: gaobinlong <[email protected]> * Increase test coverage Signed-off-by: gaobinlong <[email protected]> * Add a new test case Signed-off-by: gaobinlong <[email protected]> * Optimize some code Signed-off-by: gaobinlong <[email protected]> --------- Signed-off-by: gaobinlong <[email protected]> (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>
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
2024-03-26.14.57.23.mov
Testing the changes
config/opensearch_dashboards.yml
, addworkspace.enabled: true
yarn start --no-base-path
Select a workspace
to enter the overview page of a workspace, the URL is like this:http://localhost:5601/w/DsNfKS/app/workspace_overview
http://localhost:5601/w/DsNfKS/app/workspace_update
.Check List
yarn test:jest
yarn test:jest_integration