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 all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
78c9d6b
[WIP] new per stream flow
Jul 12, 2022
efa5c98
Start work on ModalSerice
Jul 12, 2022
6ab269c
Merge branch 'master' into tim/per-stream-flow
Jul 13, 2022
654d502
Continue work
Jul 13, 2022
b89cc19
Fix typo
Jul 13, 2022
2657d88
Change wording
Jul 13, 2022
6fce4f0
Add todos and remove dead code
Jul 13, 2022
5a9cbcc
Remove dead message
Jul 13, 2022
13be9e8
Merge branch 'master' into tim/per-stream-flow
Jul 13, 2022
7d1fbeb
Merge branch 'master' into tim/per-stream-flow
Jul 13, 2022
9266bb5
Adjust to new API
Jul 13, 2022
ee1c329
Merge branch 'master' into tim/per-stream-flow
Jul 14, 2022
871df8a
Adjust to new modal changes
Jul 14, 2022
f0515e8
Remove debug output
Jul 14, 2022
8de44f9
Fix e2e test
Jul 14, 2022
e08d0e5
Add data-testids
Jul 14, 2022
6fd68b1
Adjust for PR review
Jul 15, 2022
9e7fd01
Merge branch 'master' into tim/per-stream-flow
Jul 15, 2022
22036c0
Merge branch 'master' into tim/per-stream-flow
Jul 15, 2022
4787a78
Switch to new ModalFooter/Body components
Jul 15, 2022
a0cb0ce
Add ModalService tests
Jul 15, 2022
8cbe3fe
Merge branch 'master' into tim/per-stream-flow
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
Jul 19, 2022
5e759ca
Update airbyte-webapp/src/locales/en.json
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 { defaultFeatures, 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 features={defaultFeatures}>
<NotificationService>
<ConfirmationModalService>
<FormChangeTrackerService>
<ApiServices>{children}</ApiServices>
</FormChangeTrackerService>
<ModalServiceProvider>
<FormChangeTrackerService>
<ApiServices>{children}</ApiServices>
</FormChangeTrackerService>
</ModalServiceProvider>
</ConfirmationModalService>
</NotificationService>
</FeatureService>
Expand Down
6 changes: 3 additions & 3 deletions airbyte-webapp/src/components/Modal/Modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ const cardStyleBySize = {
};

const Modal: React.FC<ModalProps> = ({ children, title, onClose, clear, closeOnBackground, size }) => {
const handleUserKeyPress = useCallback((event, closeModal) => {
const { keyCode } = event;
const handleUserKeyPress = useCallback((event: KeyboardEvent, closeModal: () => void) => {
const { key } = event;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ Changed this to use key instead of the deprecated keyCode, so I could test it via @testing-library/user-events.

// Escape key
if (keyCode === 27) {
if (key === "Escape") {
closeModal();
}
}, []);
Expand Down
2 changes: 1 addition & 1 deletion airbyte-webapp/src/components/Modal/ModalBody.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@

.modalBody {
padding: variables.$spacing-lg variables.$spacing-xl;
overflow: scroll;
overflow: auto;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ @edmundito I've changed this to auto, since scroll will have scrollbars always visible (on non mac systems) inside the modal body:

screenshot-20220715-111421

Copy link
Contributor

Choose a reason for hiding this comment

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

Whoops, yes I am familiar with this. it definitely should be auto!

max-width: 100%;
}
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,
});
87 changes: 87 additions & 0 deletions airbyte-webapp/src/hooks/services/Modal/ModalService.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import { render, waitFor } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { useEffectOnce } from "react-use";

import { ModalServiceProvider, useModalService } from "./ModalService";
import { ModalResult } from "./types";

const TestComponent: React.FC<{ onModalResult?: (result: ModalResult<unknown>) => void }> = ({ onModalResult }) => {
const { openModal } = useModalService();
useEffectOnce(() => {
openModal({
title: "Test Modal Title",
content: ({ onCancel, onClose }) => (
<div data-testid="testModalContent">
<button onClick={onCancel} data-testid="cancel">
Cancel
</button>
<button onClick={() => onClose("reason1")} data-testid="close-reason1">
Close Reason 1
</button>
<button onClick={() => onClose("reason2")} data-testid="close-reason2">
Close Reason 2
</button>
</div>
),
}).then(onModalResult);
});
return null;
};

const renderModal = (resultCallback?: (reason: unknown) => void) => {
return render(
<ModalServiceProvider>
<TestComponent onModalResult={resultCallback} />
</ModalServiceProvider>
);
};

describe("ModalService", () => {
it("should open a modal on openModal", () => {
const rendered = renderModal();

expect(rendered.getByText("Test Modal Title")).toBeTruthy();
expect(rendered.getByTestId("testModalContent")).toBeTruthy();
});

it("should close the modal with escape and emit a cancel result", async () => {
const resultCallback = jest.fn();

const rendered = renderModal(resultCallback);

await waitFor(() => userEvent.keyboard("{Escape}"));

expect(rendered.queryByTestId("testModalContent")).toBeFalsy();
expect(resultCallback).toHaveBeenCalledWith({ type: "canceled" });
});

it("should allow cancelling the modal from inside", async () => {
const resultCallback = jest.fn();

const rendered = renderModal(resultCallback);

await waitFor(() => userEvent.click(rendered.getByTestId("cancel")));

expect(rendered.queryByTestId("testModalContent")).toBeFalsy();
expect(resultCallback).toHaveBeenCalledWith({ type: "canceled" });
});

it("should allow closing the button with a reason and return that reason", async () => {
const resultCallback = jest.fn();

let rendered = renderModal(resultCallback);

await waitFor(() => userEvent.click(rendered.getByTestId("close-reason1")));

expect(rendered.queryByTestId("testModalContent")).toBeFalsy();
expect(resultCallback).toHaveBeenCalledWith({ type: "closed", reason: "reason1" });

resultCallback.mockReset();
rendered = renderModal(resultCallback);

await waitFor(() => userEvent.click(rendered.getByTestId("close-reason2")));

expect(rendered.queryByTestId("testModalContent")).toBeFalsy();
expect(resultCallback).toHaveBeenCalledWith({ type: "closed", reason: "reason2" });
});
});
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, ModalServiceContext } from "./types";

const modalServiceContext = React.createContext<ModalServiceContext | undefined>(undefined);

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: ModalServiceContext = useMemo(
() => ({
openModal: (options) => {
resultSubjectRef.current = new Subject();
Copy link
Contributor 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 ModalServiceContext {
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
Contributor 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, we recommend a data reset. A reset will delete data in the destination of the affected streams and then re-sync that data. Skipping the reset is discouraged and might lead to unexpected behavior.",
"connection.streamFullResetHint": "Due to changes in the stream configuration, we recommend a data reset. A reset will delete data in the destination of the affected streams and then re-sync that data. Skipping the reset is discouraged and might lead to unexpected behavior.",
"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
25 changes: 14 additions & 11 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 { FeatureItem, 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,17 +38,19 @@ const Services: React.FC = ({ children }) => (
<ApiErrorBoundary>
<NotificationServiceProvider>
<ConfirmationModalService>
<FormChangeTrackerService>
<FeatureService
features={[FeatureItem.AllowOAuthConnector, FeatureItem.AllowCreateConnection, FeatureItem.AllowSync]}
>
<AppServicesProvider>
<AuthenticationProvider>
<IntercomProvider>{children}</IntercomProvider>
</AuthenticationProvider>
</AppServicesProvider>
</FeatureService>
</FormChangeTrackerService>
<ModalServiceProvider>
<FormChangeTrackerService>
<FeatureService
features={[FeatureItem.AllowOAuthConnector, FeatureItem.AllowCreateConnection, FeatureItem.AllowSync]}
>
<AppServicesProvider>
<AuthenticationProvider>
<IntercomProvider>{children}</IntercomProvider>
</AuthenticationProvider>
</AppServicesProvider>
</FeatureService>
</FormChangeTrackerService>
</ModalServiceProvider>
</ConfirmationModalService>
</NotificationServiceProvider>
</ApiErrorBoundary>
Expand Down
Loading