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] Fix workspace update issue #8570

Merged

Conversation

yubonluo
Copy link
Contributor

@yubonluo yubonluo commented Oct 12, 2024

Description

  1. After updating workspace detail, should not show the navigate away modal.
  2. After updating Collaborators, switch to other tab should not show the navigate away modal
2024-10-12.13.29.04.mp4

Issues Resolved

Screenshot

2024-10-12.14.40.25.mp4

Testing the changes

Changelog

  • fix: Fix workspace update issue

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

@SuZhou-Joe
Copy link
Member

SuZhou-Joe commented Oct 12, 2024

It looks a little bit weird to me that we are refreshing the page after update the workspace detail, I remember we already have the refresh logic for currentWorkspace$ and workspaces$ in workspace client when calling update interface and the only thing we need to do is to update the state of the detail panel component. By only updating the state, we won't have the issue of the navigate away modal for workspace detail tab at least.

Copy link

codecov bot commented Oct 12, 2024

Codecov Report

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

Project coverage is 60.92%. Comparing base (c4049eb) to head (8bcc684).
Report is 77 commits behind head on main.

Files with missing lines Patch % Lines
...ic/components/workspace_form/use_workspace_form.ts 66.66% 2 Missing and 1 partial ⚠️
...c/components/workspace_detail/workspace_detail.tsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8570      +/-   ##
==========================================
- Coverage   60.96%   60.92%   -0.04%     
==========================================
  Files        3776     3777       +1     
  Lines       89680    89843     +163     
  Branches    14055    14083      +28     
==========================================
+ Hits        54671    54737      +66     
- Misses      31593    31672      +79     
- Partials     3416     3434      +18     
Flag Coverage Δ
Linux_1 29.13% <60.00%> (+0.01%) ⬆️
Linux_2 56.28% <ø> (ø)
Linux_3 37.81% <ø> (+<0.01%) ⬆️
Linux_4 29.95% <ø> (?)
Windows_1 29.14% <60.00%> (+0.01%) ⬆️
Windows_2 56.23% <ø> (ø)
Windows_3 37.81% <ø> (-0.01%) ⬇️
Windows_4 29.95% <ø> (ø)

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.

@wanglam
Copy link
Contributor

wanglam commented Oct 12, 2024

It looks a little bit weird to me that we are refreshing the page after update the workspace detail, I remember we already have the refresh logic for currentWorkspace$ and workspaces$ in workspace client when calling update interface and the only thing we need to do is to update the state of the detail panel component. By only updating the state, we won't have the issue of the navigate away modal for workspace detail tab at least.

Agree with you. I think the navigate away modal issue should come from default values not updated. I'm prefer to add some method to update the defaultValuesRef in useWorkspaceForm hook.

Signed-off-by: yubonluo <[email protected]>
…/OpenSearch-Dashboards into 2.17/fix-workspace-update-issue
@yubonluo
Copy link
Contributor Author

It looks a little bit weird to me that we are refreshing the page after update the workspace detail, I remember we already have the refresh logic for currentWorkspace$ and workspaces$ in workspace client when calling update interface and the only thing we need to do is to update the state of the detail panel component. By only updating the state, we won't have the issue of the navigate away modal for workspace detail tab at least.

Agree with you. I think the navigate away modal issue should come from default values not updated. I'm prefer to add some method to update the defaultValuesRef in useWorkspaceForm hook.

Sure, the code has been updated. When user updates the workspace, the detail page will not be refreshed and defaultValuesRef.current will be updated as currentFormData

Signed-off-by: yubonluo <[email protected]>
SuZhou-Joe
SuZhou-Joe previously approved these changes Oct 12, 2024
Signed-off-by: yubonluo <[email protected]>
Comment on lines +92 to +101
const getSubmitFormData = (submitFormData: WorkspaceFormDataState) => {
return {
name: submitFormData.name!,
description: submitFormData.description,
color: submitFormData.color || '#FFFFFF',
features: submitFormData.features,
permissionSettings: submitFormData.permissionSettings as WorkspacePermissionSetting[],
selectedDataSourceConnections: submitFormData.selectedDataSourceConnections,
};
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like the return value is same as WorkspaceFormDataState? can we use WorkspaceFormDataState diectly? or {...WorkspaceFormDataState}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, there are some differences between the type of return values and WorkspaceFormDataState, such as the useCase attribute.

@SuZhou-Joe SuZhou-Joe merged commit 9cad0a3 into opensearch-project:main Oct 14, 2024
67 of 68 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 14, 2024
* fix workspace update issue

Signed-off-by: yubonluo <[email protected]>

* Changeset file for PR #8570 created/updated

* optimize the code

Signed-off-by: yubonluo <[email protected]>

* optimize the code

Signed-off-by: yubonluo <[email protected]>

* optimize the code

Signed-off-by: yubonluo <[email protected]>

---------

Signed-off-by: yubonluo <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
(cherry picked from commit 9cad0a3)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
SuZhou-Joe pushed a commit that referenced this pull request Oct 14, 2024
* fix workspace update issue



* Changeset file for PR #8570 created/updated

* optimize the code



* optimize the code



* optimize the code



---------



(cherry picked from commit 9cad0a3)

Signed-off-by: yubonluo <[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>
sejli pushed a commit to sejli/OpenSearch-Dashboards that referenced this pull request Oct 21, 2024
* fix workspace update issue

Signed-off-by: yubonluo <[email protected]>

* Changeset file for PR opensearch-project#8570 created/updated

* optimize the code

Signed-off-by: yubonluo <[email protected]>

* optimize the code

Signed-off-by: yubonluo <[email protected]>

* optimize the code

Signed-off-by: yubonluo <[email protected]>

---------

Signed-off-by: yubonluo <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Qxisylolo pushed a commit to Qxisylolo/OpenSearch-Dashboards that referenced this pull request Oct 30, 2024
* fix workspace update issue

Signed-off-by: yubonluo <[email protected]>

* Changeset file for PR opensearch-project#8570 created/updated

* optimize the code

Signed-off-by: yubonluo <[email protected]>

* optimize the code

Signed-off-by: yubonluo <[email protected]>

* optimize the code

Signed-off-by: yubonluo <[email protected]>

---------

Signed-off-by: yubonluo <[email protected]>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
@ananzh ananzh added the v2.18.0 label Oct 30, 2024
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.

5 participants