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

🪟 Per-Stream state new flow #14634

Merged
merged 29 commits into from
Jul 19, 2022
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
78c9d6b
[WIP] new per stream flow
timroes Jul 12, 2022
efa5c98
Start work on ModalSerice
timroes Jul 12, 2022
6ab269c
Merge branch 'master' into tim/per-stream-flow
timroes Jul 13, 2022
654d502
Continue work
timroes Jul 13, 2022
b89cc19
Fix typo
timroes Jul 13, 2022
2657d88
Change wording
timroes Jul 13, 2022
6fce4f0
Add todos and remove dead code
timroes Jul 13, 2022
5a9cbcc
Remove dead message
timroes Jul 13, 2022
13be9e8
Merge branch 'master' into tim/per-stream-flow
timroes Jul 13, 2022
7d1fbeb
Merge branch 'master' into tim/per-stream-flow
timroes Jul 13, 2022
9266bb5
Adjust to new API
timroes Jul 13, 2022
ee1c329
Merge branch 'master' into tim/per-stream-flow
timroes Jul 14, 2022
871df8a
Adjust to new modal changes
timroes Jul 14, 2022
f0515e8
Remove debug output
timroes Jul 14, 2022
8de44f9
Fix e2e test
timroes Jul 14, 2022
e08d0e5
Add data-testids
timroes Jul 14, 2022
6fd68b1
Adjust for PR review
timroes Jul 15, 2022
9e7fd01
Merge branch 'master' into tim/per-stream-flow
timroes Jul 15, 2022
22036c0
Merge branch 'master' into tim/per-stream-flow
timroes Jul 15, 2022
4787a78
Switch to new ModalFooter/Body components
timroes Jul 15, 2022
a0cb0ce
Add ModalService tests
timroes Jul 15, 2022
8cbe3fe
Merge branch 'master' into tim/per-stream-flow
timroes Jul 15, 2022
7cadda6
Merge branch 'master' into tim/per-stream-flow
timroes Jul 18, 2022
f736e2e
Downgrade user-events again
timroes Jul 18, 2022
4984735
Merge branch 'master' into tim/per-stream-flow
timroes Jul 19, 2022
b92807b
Only compare selected streams
timroes Jul 19, 2022
1f0b868
Update airbyte-webapp/src/locales/en.json
timroes Jul 19, 2022
5e759ca
Update airbyte-webapp/src/locales/en.json
timroes Jul 19, 2022
95a90b6
Remove redundant space
timroes Jul 19, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ describe("Connection main actions", () => {
});

it("Update connection", () => {
cy.intercept("/api/v1/web_backend/connections/update").as("updateConnection");
cy.intercept("/api/v1/web_backend/connections/updateNew").as("updateConnection");

createTestConnection("Test update connection source cypress", "Test update connection destination cypress");

Expand Down
9 changes: 6 additions & 3 deletions airbyte-webapp/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { ServicesProvider } from "core/servicesProvider";
import { ConfirmationModalService } from "hooks/services/ConfirmationModal";
import { FeatureService } from "hooks/services/Feature";
import { FormChangeTrackerService } from "hooks/services/FormChangeTracker";
import { ModalServiceProvider } from "hooks/services/Modal";
import NotificationService from "hooks/services/Notification";
import { AnalyticsProvider } from "views/common/AnalyticsProvider";
import { StoreProvider } from "views/common/StoreProvider";
Expand Down Expand Up @@ -44,9 +45,11 @@ const Services: React.FC = ({ children }) => (
<FeatureService>
<NotificationService>
<ConfirmationModalService>
<FormChangeTrackerService>
<ApiServices>{children}</ApiServices>
</FormChangeTrackerService>
<ModalServiceProvider>
<FormChangeTrackerService>
<ApiServices>{children}</ApiServices>
</FormChangeTrackerService>
</ModalServiceProvider>
</ConfirmationModalService>
</NotificationService>
</FeatureService>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { deleteConnection, resetConnection, syncConnection, getState } from "../../request/AirbyteClient";
import { deleteConnection, resetConnection, syncConnection, getState, getStateType } from "../../request/AirbyteClient";
import { AirbyteRequestService } from "../../request/AirbyteRequestService";

export class ConnectionService extends AirbyteRequestService {
Expand All @@ -17,4 +17,8 @@ export class ConnectionService extends AirbyteRequestService {
public getState(connectionId: string) {
return getState({ connectionId }, this.requestOptions);
}

public getStateType(connectionId: string) {
return getStateType({ connectionId }, this.requestOptions);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
webBackendCreateConnection,
webBackendGetConnection,
webBackendListConnectionsForWorkspace,
webBackendUpdateConnection,
webBackendUpdateConnectionNew as webBackendUpdateConnection,
Copy link
Contributor

Choose a reason for hiding this comment

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

webBackendUpdateConnectionNew is temporary?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it is; this endpoint was created so that we could merge backend changes related to this new flow in before the frontend changes were merged. We have a ticket here to remove this temporary endpoint after the launch of per-stream: #14733

} from "../../request/AirbyteClient";
import { AirbyteRequestService } from "../../request/AirbyteRequestService";

Expand Down
1 change: 1 addition & 0 deletions airbyte-webapp/src/core/domain/connection/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export const buildConnectionUpdate = (
connection: WebBackendConnectionRead,
connectionUpdate: Partial<WebBackendConnectionUpdate>
): WebBackendConnectionUpdate => ({
skipReset: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing that skipReset is default to false if it's not sent? Seems a bit sneaky to put this here

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct, it defaults to false in the backend if it is not present

...toWebBackendConnectionUpdate(connection),
...connectionUpdate,
});
64 changes: 64 additions & 0 deletions airbyte-webapp/src/hooks/services/Modal/ModalService.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import React, { useContext, useMemo, useRef, useState } from "react";
import { firstValueFrom, Subject } from "rxjs";

import { Modal } from "components";

import { ModalOptions, ModalResult, ModalServiceContextType } from "./types";

const ModalServiceContext = React.createContext<ModalServiceContextType | undefined>(undefined);
timroes marked this conversation as resolved.
Show resolved Hide resolved

export const ModalServiceProvider: React.FC = ({ children }) => {
// The any here is due to the fact, that every call to open a modal might come in with
// a different type, thus we can't type this with unknown or a generic.
// The consuming code of this service though is properly typed, so that this `any` stays
// encapsulated within this component.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const [modalOptions, setModalOptions] = useState<ModalOptions<any>>();
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const resultSubjectRef = useRef<Subject<ModalResult<any>>>();

const service: ModalServiceContextType = useMemo(
() => ({
openModal: (options) => {
resultSubjectRef.current = new Subject();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ Why solving this with an observable here, despite only needing a one time resolved value. The problem is, that we can't control a Promise from the outside, so we'd need to instantiate a promise here and store a reference to it's resolve/reject method in a ref for later consumption. I had this first, and the whole component read way worse imho, thus I switched this over to use a simple subject and convert that later to a promise.

setModalOptions(options);

return firstValueFrom(resultSubjectRef.current).then((reason) => {
setModalOptions(undefined);
resultSubjectRef.current = undefined;
return reason;
});
},
closeModal: () => {
resultSubjectRef.current?.next({ type: "canceled" });
},
}),
[]
);

return (
<ModalServiceContext.Provider value={service}>
{children}
{modalOptions && (
<Modal
title={modalOptions.title}
size={modalOptions.size}
onClose={() => resultSubjectRef.current?.next({ type: "canceled" })}
>
<modalOptions.content
onCancel={() => resultSubjectRef.current?.next({ type: "canceled" })}
onClose={(reason) => resultSubjectRef.current?.next({ type: "closed", reason })}
/>
</Modal>
)}
</ModalServiceContext.Provider>
);
};

export const useModalService = () => {
const context = useContext(ModalServiceContext);
if (!context) {
throw new Error("Can't use ModalService outside ModalServiceProvider");
}
return context;
};
1 change: 1 addition & 0 deletions airbyte-webapp/src/hooks/services/Modal/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from "./ModalService";
21 changes: 21 additions & 0 deletions airbyte-webapp/src/hooks/services/Modal/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import React from "react";

import { ModalProps } from "components/Modal/Modal";

export interface ModalOptions<T> {
title: ModalProps["title"];
size?: ModalProps["size"];
content: React.ComponentType<ModalContentProps<T>>;
}

export type ModalResult<T> = { type: "canceled" } | { type: "closed"; reason: T };

interface ModalContentProps<T> {
onClose: (reason: T) => void;
onCancel: () => void;
}

export interface ModalServiceContextType {
openModal: <ResultType>(options: ModalOptions<ResultType>) => Promise<ModalResult<ResultType>>;
closeModal: () => void;
}
19 changes: 5 additions & 14 deletions airbyte-webapp/src/hooks/services/useConnectionHook.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ function useWebConnectionService() {
);
}

function useConnectionService() {
export function useConnectionService() {
const config = useConfig();
const middlewares = useDefaultRequestMiddlewares();
return useInitService(() => new ConnectionService(config.apiUrl, middlewares), [config.apiUrl, middlewares]);
Expand Down Expand Up @@ -190,20 +190,11 @@ const useUpdateConnection = () => {
const service = useWebConnectionService();
const queryClient = useQueryClient();

return useMutation(
(connectionUpdate: WebBackendConnectionUpdate) => {
const withRefreshedCatalogCleaned = connectionUpdate.withRefreshedCatalog
? { withRefreshedCatalog: connectionUpdate.withRefreshedCatalog }
: null;
Comment on lines -195 to -197
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you think this was here in the first place?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not really. This was in there since 2 years, so I fear that knowledge is lost. Looking at the old code it seems this was a separate parameter to this hook earlier, and thus spreaded like this into it (trying to preventing actually setting the value when it was undefined), but it's really just guessing.


return service.update({ ...connectionUpdate, ...withRefreshedCatalogCleaned });
return useMutation((connectionUpdate: WebBackendConnectionUpdate) => service.update(connectionUpdate), {
onSuccess: (connection) => {
queryClient.setQueryData(connectionsKeys.detail(connection.connectionId), connection);
},
{
onSuccess: (connection) => {
queryClient.setQueryData(connectionsKeys.detail(connection.connectionId), connection);
},
}
);
});
};

const useConnectionList = (): ListConnection => {
Expand Down
12 changes: 6 additions & 6 deletions airbyte-webapp/src/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@
"form.noNeed": "No need!",
"form.reset": "Reset",
"form.resetData": "Reset your data",
"form.changedColumns": "You have changed which column is used to detect new records for a stream. You may want to reset the data in your stream.",
"form.resetDataText": "Resetting your data will delete all the data for this connection in your destination and start syncs from scratch. Are you sure you want to do this?",
"form.dockerError": "Could not find docker image",
"form.edit": "Edit",
Expand All @@ -106,8 +105,6 @@
"form.destinationConnector": "Destination connector",
"form.addItems": "Add",
"form.items": "Items",
"form.toSaveSchema": "To save the new schema, click on Save changes.",
"form.noteStartSync": "Note that it will delete all the data in your destination and start syncing from scratch. ",
"form.pkSelected": "{count, plural, =0 { } one {{items}} other {# keys selected}}",
"form.url.error": "field must be a valid URL",
"form.setupGuide": "Setup Guide",
Expand Down Expand Up @@ -317,7 +314,12 @@
"syncMode.full_refresh": "Full refresh",
"syncMode.incremental": "Incremental",

"connection.warningUpdateSchema": "WARNING! Updating the schema will delete all the data for this connection in your destination and start syncing from scratch.",
"connection.resetModalTitle": "Stream configuration changed",
"connection.streamResetHint": "Due to changes in the stream configuration the affected streams should be resetted. This will delete all data of those streams in the destination and sync it again. Skipping this might lead to unexpected behavior and is discouraged.",
timroes marked this conversation as resolved.
Show resolved Hide resolved
"connection.streamFullResetHint": "Due to changes in the stream configuration all streams should be resetted. This will delete all data in the destination and sync it again. Skipping this might lead to unexpected behavior and is discouraged.",
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
"connection.streamResetHint": "Due to changes in the stream configuration the affected streams should be resetted. This will delete all data of those streams in the destination and sync it again. Skipping this might lead to unexpected behavior and is discouraged.",
"connection.streamFullResetHint": "Due to changes in the stream configuration all streams should be resetted. This will delete all data in the destination and sync it again. Skipping this might lead to unexpected behavior and is discouraged.",
"connection.streamResetHint": "Due to changes in the stream configuration, the affected streams should be reset. This will delete all data of those streams in the destination and sync it again. Skipping this is discouraged and might lead to unexpected behavior.",
"connection.streamFullResetHint": "Due to changes in the stream configuration, all streams should be reset. This will delete all data in the destination and sync it again. Skipping this is discouraged and might lead to unexpected behavior.",

Copy link
Contributor

Choose a reason for hiding this comment

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

Is 'stream' a term that users understand? I don't think it's necessarily intuitive that 'stream' corresponds to db table or API endpoint.

My suggested wording:
Your data schema has changed, so a data reset is recommended. This will delete all data in the destination and resync it. Skipping this reset is discouraged, as it may lead to unexpected behavior.

timroes marked this conversation as resolved.
Show resolved Hide resolved
"connection.saveWithReset": "Reset affected streams (recommended)",
"connection.saveWithFullReset": "Reset all streams (recommended)",
"connection.save": "Save connection",
"connection.title": "Connection",
"connection.description": "<b>Connections</b> link Sources to Destinations.",
"connection.fromTo": "{source} → {destination}",
Expand All @@ -326,12 +328,10 @@
"connection.testsFailed": "The connection tests failed.",
"connection.destinationCheckSettings": "Check destination settings",
"connection.sourceCheckSettings": "Check source settings",
"connection.saveAndReset": "Save changes & reset data",
"connection.destinationTestAgain": "Test destination connection again",
"connection.sourceTestAgain": "Test source connection again",
"connection.resetData": "Reset your data",
"connection.updateSchema": "Refresh source schema",
"connection.updateSchemaText": "WARNING! Updating the schema will delete all the data for this connection in your destination and start syncing from scratch. Are you sure you want to do this?",
"connection.newConnection": "+ New connection",
"connection.newConnectionTitle": "New connection",
"connection.noConnections": "Connection list is empty",
Expand Down
21 changes: 12 additions & 9 deletions airbyte-webapp/src/packages/cloud/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { I18nProvider } from "core/i18n";
import { ConfirmationModalService } from "hooks/services/ConfirmationModal";
import { FeatureService } from "hooks/services/Feature";
import { FormChangeTrackerService } from "hooks/services/FormChangeTracker";
import { ModalServiceProvider } from "hooks/services/Modal";
import NotificationServiceProvider from "hooks/services/Notification";
import en from "locales/en.json";
import { Routing } from "packages/cloud/cloudRoutes";
Expand Down Expand Up @@ -37,15 +38,17 @@ const Services: React.FC = ({ children }) => (
<ApiErrorBoundary>
<NotificationServiceProvider>
<ConfirmationModalService>
<FormChangeTrackerService>
<FeatureService>
<AppServicesProvider>
<AuthenticationProvider>
<IntercomProvider>{children}</IntercomProvider>
</AuthenticationProvider>
</AppServicesProvider>
</FeatureService>
</FormChangeTrackerService>
<ModalServiceProvider>
<FormChangeTrackerService>
<FeatureService>
<AppServicesProvider>
<AuthenticationProvider>
<IntercomProvider>{children}</IntercomProvider>
</AuthenticationProvider>
</AppServicesProvider>
</FeatureService>
</FormChangeTrackerService>
</ModalServiceProvider>
</ConfirmationModalService>
</NotificationServiceProvider>
</ApiErrorBoundary>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
@use "../../../../../scss/variables" as vars;

.resetWarningModal {
padding: vars.$spacing-xl;
}

.resetWarningModalButtons {
display: flex;
justify-content: flex-end;
gap: vars.$spacing-md;
margin-top: vars.$spacing-lg;
}
Loading