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 22 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
23 changes: 9 additions & 14 deletions airbyte-webapp/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion airbyte-webapp/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
"@testing-library/jest-dom": "^5.16.4",
"@testing-library/react": "^12.1.3",
"@testing-library/react-hooks": "^7.0.2",
"@testing-library/user-event": "^13.5.0",
"@testing-library/user-event": "^14.2.4",
"@types/flat": "^5.0.2",
"@types/jest": "^27.4.1",
"@types/json-schema": "^7.0.11",
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
Collaborator 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
Collaborator 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,
});
93 changes: 93 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,93 @@
import { render } 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 user = userEvent.setup();

const resultCallback = jest.fn();

const rendered = renderModal(resultCallback);

await user.keyboard("{Escape}");

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

it("should allow cancelling the modal from inside", async () => {
const user = userEvent.setup();

const resultCallback = jest.fn();

const rendered = renderModal(resultCallback);

await user.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 user = userEvent.setup();

const resultCallback = jest.fn();

let rendered = renderModal(resultCallback);

await user.click(rendered.getByTestId("close-reason1"));

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

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

await user.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
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 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
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
Loading