-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Connection Form Refactor - Connection Form Service #15893
Changes from 61 commits
10b4ff1
2f0e83f
183087a
b3bc5ff
85fe161
0e063b1
d7ad611
468d840
8834c04
b99d1b3
25f28ba
8272377
e85d301
bf6a647
33caf95
68a6ad8
9bf2c73
8af1a41
4ed21ec
eb24031
1d4b3a7
e41a42f
dfcd4cc
eb54ec2
34434d2
be4001b
9d95504
b53abb8
50bbb3f
65584ca
3da331e
e841140
92b7c0c
8c5558b
d390833
3d094fb
3ec2cf2
0452458
df444fe
aec7b10
6aabc3b
8af9bd2
c9ca5b5
f809c68
61251a0
208cf78
c7f9afa
cc91839
e89f762
96b12a5
3395828
4c17657
498743a
ed26655
955848c
a0beff7
e823d60
e2dc6e1
7f4916a
cec5034
5ac6baa
184732f
50df6de
a45d98a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,28 +1,28 @@ | ||
import { faRedoAlt } from "@fortawesome/free-solid-svg-icons"; | ||
import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; | ||
import React, { Suspense, useMemo } from "react"; | ||
import React, { Suspense, useCallback, useMemo } from "react"; | ||
import { FormattedMessage } from "react-intl"; | ||
import { useNavigate } from "react-router-dom"; | ||
|
||
import { Button, Card } from "components"; | ||
import { IDataItem } from "components/base/DropDown/components/Option"; | ||
import { JobItem } from "components/JobItem/JobItem"; | ||
import LoadingSchema from "components/LoadingSchema"; | ||
|
||
import { Action, Namespace } from "core/analytics"; | ||
import { LogsRequestError } from "core/request/LogsRequestError"; | ||
import { useAnalyticsService } from "hooks/services/Analytics"; | ||
import { ConnectionFormServiceProvider } from "hooks/services/Connection/ConnectionFormService"; | ||
import { useChangedFormsById, useFormChangeTrackerService, useUniqueFormId } from "hooks/services/FormChangeTracker"; | ||
import { useCreateConnection, ValuesProps } from "hooks/services/useConnectionHook"; | ||
import { ConnectionForm, ConnectionFormProps } from "views/Connection/ConnectionForm"; | ||
import { ConnectionForm } from "views/Connection/ConnectionForm"; | ||
|
||
import { DestinationRead, SourceRead, WebBackendConnectionRead } from "../../core/request/AirbyteClient"; | ||
import { DestinationRead, SourceRead } from "../../core/request/AirbyteClient"; | ||
import { useDiscoverSchema } from "../../hooks/services/useSourceHook"; | ||
import TryAfterErrorBlock from "./components/TryAfterErrorBlock"; | ||
import styles from "./CreateConnectionContent.module.scss"; | ||
|
||
interface CreateConnectionContentProps { | ||
source: SourceRead; | ||
destination: DestinationRead; | ||
afterSubmitConnection?: (connection: WebBackendConnectionRead) => void; | ||
afterSubmitConnection?: () => void; | ||
} | ||
|
||
const CreateConnectionContent: React.FC<CreateConnectionContentProps> = ({ | ||
|
@@ -31,61 +31,51 @@ const CreateConnectionContent: React.FC<CreateConnectionContentProps> = ({ | |
afterSubmitConnection, | ||
}) => { | ||
const { mutateAsync: createConnection } = useCreateConnection(); | ||
const analyticsService = useAnalyticsService(); | ||
const navigate = useNavigate(); | ||
|
||
const formId = useUniqueFormId(); | ||
const { clearFormChange } = useFormChangeTrackerService(); | ||
const [changedFormsById] = useChangedFormsById(); | ||
const formDirty = useMemo(() => !!changedFormsById?.[formId], [changedFormsById, formId]); | ||
|
||
const { schema, isLoading, schemaErrorStatus, catalogId, onDiscoverSchema } = useDiscoverSchema( | ||
source.sourceId, | ||
true | ||
); | ||
|
||
const connection = useMemo<ConnectionFormProps["connection"]>( | ||
() => ({ | ||
syncCatalog: schema, | ||
destination, | ||
source, | ||
catalogId, | ||
}), | ||
[schema, destination, source, catalogId] | ||
); | ||
|
||
const onSubmitConnectionStep = async (values: ValuesProps) => { | ||
const connection = await createConnection({ | ||
values, | ||
source, | ||
destination, | ||
sourceDefinition: { | ||
sourceDefinitionId: source?.sourceDefinitionId ?? "", | ||
}, | ||
destinationDefinition: { | ||
name: destination?.name ?? "", | ||
destinationDefinitionId: destination?.destinationDefinitionId ?? "", | ||
}, | ||
sourceCatalogId: catalogId, | ||
}); | ||
|
||
return { | ||
onSubmitComplete: () => { | ||
afterSubmitConnection?.(connection); | ||
}, | ||
}; | ||
const connection = { | ||
syncCatalog: schema, | ||
destination, | ||
source, | ||
catalogId, | ||
}; | ||
|
||
const onSelectFrequency = (item: IDataItem | null) => { | ||
const enabledStreams = connection.syncCatalog.streams.filter((stream) => stream.config?.selected).length; | ||
|
||
if (item) { | ||
analyticsService.track(Namespace.CONNECTION, Action.FREQUENCY, { | ||
actionDescription: "Frequency selected", | ||
frequency: item.label, | ||
connector_source_definition: source?.sourceName, | ||
connector_source_definition_id: source?.sourceDefinitionId, | ||
connector_destination_definition: destination?.destinationName, | ||
connector_destination_definition_id: destination?.destinationDefinitionId, | ||
available_streams: connection.syncCatalog.streams.length, | ||
enabled_streams: enabledStreams, | ||
const onSubmitConnectionStep = useCallback( | ||
async (values: ValuesProps) => { | ||
const createdConnection = await createConnection({ | ||
values, | ||
source, | ||
destination, | ||
sourceDefinition: { | ||
sourceDefinitionId: source?.sourceDefinitionId ?? "", | ||
}, | ||
destinationDefinition: { | ||
name: destination?.name ?? "", | ||
destinationDefinitionId: destination?.destinationDefinitionId ?? "", | ||
}, | ||
sourceCatalogId: catalogId, | ||
}); | ||
} | ||
}; | ||
|
||
// We only want to go to the new connection if we _do not_ have an after submit action. | ||
if (!afterSubmitConnection) { | ||
// We have to clear the form change to prevent the dirty-form tracking modal from appearing. | ||
clearFormChange(formId); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks too high in the component tree and should be called from the Connection form. The reason is that the ConnectionForm sets up the ConnectionChangeTracker, and should be responsible for clearing it after submission. In this case, this component, which has no idea what the form does, assumes that the tracker is being used. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The next PR brings the FormikContext "out" a layer at which point this would be the right place for this call. Does it seem reasonable to leave this for now knowing it will be much more readily adressable in the next PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be changed in part 2. |
||
// This is the "default behavior", go to the created connection. | ||
navigate(`../../connections/${createdConnection.connectionId}`); | ||
} | ||
}, | ||
[afterSubmitConnection, catalogId, clearFormChange, createConnection, destination, formId, navigate, source] | ||
); | ||
|
||
if (schemaErrorStatus) { | ||
const job = LogsRequestError.extractJobInfo(schemaErrorStatus); | ||
|
@@ -102,18 +92,23 @@ const CreateConnectionContent: React.FC<CreateConnectionContentProps> = ({ | |
) : ( | ||
<Suspense fallback={<LoadingSchema />}> | ||
<div className={styles.connectionFormContainer}> | ||
<ConnectionForm | ||
mode="create" | ||
<ConnectionFormServiceProvider | ||
connection={connection} | ||
onDropDownSelect={onSelectFrequency} | ||
mode="create" | ||
formId={formId} | ||
onSubmit={onSubmitConnectionStep} | ||
additionalSchemaControl={ | ||
<Button onClick={onDiscoverSchema} type="button"> | ||
<FontAwesomeIcon className={styles.tryArrowIcon} icon={faRedoAlt} /> | ||
<FormattedMessage id="connection.refreshSchema" /> | ||
</Button> | ||
} | ||
/> | ||
onAfterSubmit={afterSubmitConnection} | ||
formDirty={formDirty} | ||
> | ||
<ConnectionForm | ||
additionalSchemaControl={ | ||
<Button onClick={onDiscoverSchema} type="button"> | ||
<FontAwesomeIcon className={styles.tryArrowIcon} icon={faRedoAlt} /> | ||
<FormattedMessage id="connection.refreshSchema" /> | ||
</Button> | ||
} | ||
/> | ||
</ConnectionFormServiceProvider> | ||
</div> | ||
</Suspense> | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,7 +19,6 @@ const AgainButton = styled(Button)` | |
interface TryAfterErrorBlockProps { | ||
message?: React.ReactNode; | ||
onClick: () => void; | ||
additionControl?: React.ReactNode; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe I forgot to remove this as part of the side-PR's. This was unused. |
||
} | ||
|
||
const TryAfterErrorBlock: React.FC<TryAfterErrorBlockProps> = ({ message, onClick }) => ( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,144 @@ | ||
/* eslint-disable @typescript-eslint/no-explicit-any */ | ||
teallarson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import { act } from "@testing-library/react"; | ||
import { renderHook } from "@testing-library/react-hooks"; | ||
import React from "react"; | ||
import { MemoryRouter } from "react-router-dom"; | ||
import mockConnection from "test-utils/mock-data/mockConnection.json"; | ||
import mockDest from "test-utils/mock-data/mockDestinationDefinition.json"; | ||
import mockWorkspace from "test-utils/mock-data/mockWorkspace.json"; | ||
import { TestWrapper } from "test-utils/testutils"; | ||
|
||
import { WebBackendConnectionRead } from "core/request/AirbyteClient"; | ||
|
||
import { ModalCancel } from "../Modal"; | ||
import { | ||
ConnectionFormServiceProvider, | ||
ConnectionServiceProps, | ||
useConnectionFormService, | ||
} from "./ConnectionFormService"; | ||
|
||
["packages/cloud/services/workspaces/WorkspacesService", "services/workspaces/WorkspacesService"].forEach((s) => | ||
jest.mock(s, () => ({ | ||
useCurrentWorkspaceId: () => mockWorkspace.workspaceId, | ||
useCurrentWorkspace: () => mockWorkspace, | ||
})) | ||
); | ||
|
||
jest.mock("../FormChangeTracker", () => ({ | ||
useFormChangeTrackerService: () => ({ clearFormChange: () => null }), | ||
useUniqueFormId: () => "blah", | ||
})); | ||
jest.mock("services/connector/DestinationDefinitionSpecificationService", () => ({ | ||
useGetDestinationDefinitionSpecification: () => mockDest, | ||
})); | ||
|
||
describe("ConnectionFormService", () => { | ||
krishnaglick marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const Wrapper: React.FC<ConnectionServiceProps> = ({ children, ...props }) => ( | ||
<TestWrapper> | ||
<MemoryRouter> | ||
<ConnectionFormServiceProvider {...props}>{children}</ConnectionFormServiceProvider> | ||
</MemoryRouter> | ||
</TestWrapper> | ||
); | ||
|
||
const onSubmit = jest.fn(); | ||
const onAfterSubmit = jest.fn(); | ||
const onCancel = jest.fn(); | ||
|
||
beforeEach(() => { | ||
onSubmit.mockReset(); | ||
onAfterSubmit.mockReset(); | ||
onCancel.mockReset(); | ||
}); | ||
|
||
it("should call onSubmit when submitted", async () => { | ||
const { result } = renderHook(useConnectionFormService, { | ||
wrapper: Wrapper, | ||
initialProps: { | ||
connection: mockConnection as WebBackendConnectionRead, | ||
mode: "create", | ||
formId: Math.random().toString(), | ||
onSubmit, | ||
onAfterSubmit, | ||
onCancel, | ||
}, | ||
}); | ||
|
||
const resetForm = jest.fn(); | ||
const testValues: any = {}; | ||
await act(async () => { | ||
await result.current.onFormSubmit(testValues, { resetForm } as any); | ||
}); | ||
|
||
expect(resetForm).toBeCalledWith({ values: testValues }); | ||
expect(onSubmit).toBeCalledWith({ | ||
operations: [], | ||
scheduleData: { | ||
basicSchedule: { | ||
timeUnit: undefined, | ||
units: undefined, | ||
}, | ||
}, | ||
scheduleType: "manual", | ||
syncCatalog: { | ||
streams: undefined, | ||
}, | ||
}); | ||
expect(onAfterSubmit).toBeCalledWith(); | ||
}); | ||
|
||
it("should catch if onSubmit throws and generate an error message", async () => { | ||
const errorMessage = "asdf"; | ||
onSubmit.mockImplementation(async () => { | ||
throw new Error(errorMessage); | ||
}); | ||
|
||
const { result } = renderHook(useConnectionFormService, { | ||
wrapper: Wrapper, | ||
initialProps: { | ||
connection: mockConnection as WebBackendConnectionRead, | ||
mode: "create", | ||
formId: Math.random().toString(), | ||
onSubmit, | ||
onAfterSubmit, | ||
onCancel, | ||
}, | ||
}); | ||
|
||
const resetForm = jest.fn(); | ||
const testValues: any = {}; | ||
await act(async () => { | ||
await result.current.onFormSubmit(testValues, { resetForm } as any); | ||
}); | ||
|
||
expect(result.current.errorMessage).toBe(errorMessage); | ||
expect(resetForm).not.toHaveBeenCalled(); | ||
}); | ||
|
||
it("should catch if onSubmit throws but not generate an error if it's a ModalCancel error", async () => { | ||
onSubmit.mockImplementation(async () => { | ||
throw new ModalCancel(); | ||
}); | ||
|
||
const { result } = renderHook(useConnectionFormService, { | ||
wrapper: Wrapper, | ||
initialProps: { | ||
connection: mockConnection as WebBackendConnectionRead, | ||
mode: "create", | ||
formId: Math.random().toString(), | ||
onSubmit, | ||
onAfterSubmit, | ||
onCancel, | ||
}, | ||
}); | ||
|
||
const resetForm = jest.fn(); | ||
const testValues: any = {}; | ||
await act(async () => { | ||
await result.current.onFormSubmit(testValues, { resetForm } as any); | ||
}); | ||
|
||
expect(result.current.errorMessage).toBe(null); | ||
expect(resetForm).not.toHaveBeenCalled(); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all will get pulled out in part 2.