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

🪟 🐛 🧹 Fix workspace editing issues in cloud, cleanup CloudWorkspacesService #16942

Merged
merged 10 commits into from
Sep 22, 2022

Conversation

edmundito
Copy link
Contributor

What

Fixes #16149

This change primarily fixes some issues when editing the cloud workspace name and advanced mode, including:

  • The cancel button remained enabled after saving
  • When only updating advanced mode after updating the name, the previous name would be filled out in the form
  • No validation for empty workspace name
  • Save button was always enabled
  • After updating the name, the sidebar had the old name
  • No i18n string usage in buttons

Also cleans up the cloud workspace code to distinguish it from the workspace code

How

There are two endpoints for getting workspace data: One from the platform and one specific to cloud. The issue when saving the workspace name in cloud was that the workspace data for the platform was not getting invalidated (since it's cached with react-query), and so the name was not getting updated across the app. When the cloud workspace data is updated, the platform workspace data is invalidated, so it's re-fetched.

Additionally, some of the cache keys used with suspenseQuery were not quite matching, so those were fixed. Caused by #11746

There was a lot of mixup in the cloud between what's cloud workspace and platform workspace, so a lot of the functions have been renamed so that it's clear which is which.

Recommended reading order

WorkspaceSettingsView.tsx
CloudWorkspacesService.ts
WorkspacesService.ts
Rest of code

@edmundito edmundito requested a review from a team as a code owner September 20, 2022 21:07
@github-actions github-actions bot added area/platform issues related to the platform area/frontend Related to the Airbyte webapp labels Sep 20, 2022
Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

Have not had a chance to test locally but left a few comments.

const removeWorkspace = useRemoveWorkspace();
const updateWorkspace = useUpdateWorkspace();
const removeCloudWorkspace = useRemoveCloudWorkspace();
const updateCloudWorkspace = useUpdateCloudWorkspace();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think typically we do something like:

Suggested change
const updateCloudWorkspace = useUpdateCloudWorkspace();
const { mutateAsync: updateCloudWorkspace } = useUpdateCloudWorkspace();

export const WorkspaceSettingsView: React.FC = () => {
const { formatMessage } = useIntl();
useTrackPage(PageTrackingCodes.SETTINGS_WORKSPACE);
const { exitWorkspace } = useWorkspaceService();
const workspace = useCurrentWorkspace();
const removeWorkspace = useRemoveWorkspace();
const updateWorkspace = useUpdateWorkspace();
const removeCloudWorkspace = useRemoveCloudWorkspace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const removeCloudWorkspace = useRemoveCloudWorkspace();
const const { mutateAsync: removeCloudWorkspace, isLoading } = = useRemoveCloudWorkspace();

Copy link
Contributor

@krishnaglick krishnaglick left a comment

Choose a reason for hiding this comment

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

Code LGTM, have not tested locally.

@edmundito edmundito force-pushed the edmundito/fix-workspace-settings branch from c6f5cec to cc7d002 Compare September 21, 2022 15:20
@edmundito edmundito merged commit 0ceffd7 into master Sep 22, 2022
@edmundito edmundito deleted the edmundito/fix-workspace-settings branch September 22, 2022 13:40
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
…ervice (airbytehq#16942)

* Add validation to WorkspaceSettingsView
Use i18n string sin WorkspaceSettingsView buttons

* Invalidate the workspace query when updating the workspace in cloud

* Rename Cloud WorkspacesService to CloudWorkspacesServices to avoid confusion with WorkspaceService
Update functions in CloudWorkspacesServices to include the word Cloud

* useInvalidateWorkspaceQuery -> useInvalidateWorkspace

* Remove useWorkspaceService import from CloudWorkspacesService

* Update var names using CloudWorkspacesService

* Rename useGetUsage to useGetCloudWorkspaceUsage

* tsx -> ts

* Cleanup mutateAsync usage around CloudWorkspacesService hooks

* Remove reference to CloudWorkspacesServices in ConnectionFormService test
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
…ervice (airbytehq#16942)

* Add validation to WorkspaceSettingsView
Use i18n string sin WorkspaceSettingsView buttons

* Invalidate the workspace query when updating the workspace in cloud

* Rename Cloud WorkspacesService to CloudWorkspacesServices to avoid confusion with WorkspaceService
Update functions in CloudWorkspacesServices to include the word Cloud

* useInvalidateWorkspaceQuery -> useInvalidateWorkspace

* Remove useWorkspaceService import from CloudWorkspacesService

* Update var names using CloudWorkspacesService

* Rename useGetUsage to useGetCloudWorkspaceUsage

* tsx -> ts

* Cleanup mutateAsync usage around CloudWorkspacesService hooks

* Remove reference to CloudWorkspacesServices in ConnectionFormService test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clicking "Cancel" after saving changes reverts workspace general settings form to pre-save initial data
2 participants