-
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
Merged
Merged
Changes from 54 commits
Commits
Show all changes
64 commits
Select commit
Hold shift + click to select a range
10b4ff1
Work started
krishnaglick 2f0e83f
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick 183087a
Minor cleanup
krishnaglick b3bc5ff
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick 85fe161
Some cleanup
krishnaglick 0e063b1
Lots moved into context
krishnaglick d7ad611
WIP, stepping in the right direction
krishnaglick 468d840
WIP testing
krishnaglick 8834c04
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick b99d1b3
Post-merge fix
krishnaglick 25f28ba
Observables!
krishnaglick 8272377
WIP tests
krishnaglick e85d301
Tests!
krishnaglick bf6a647
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick 33caf95
Merge branch 'master' into kc/connection-form-refactor
krishnaglick 68a6ad8
CI test
krishnaglick 9bf2c73
CI?
krishnaglick 8af1a41
perhaps
krishnaglick 4ed21ec
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick eb24031
Only show name field during create
krishnaglick 1d4b3a7
Merge branch 'master' into kc/connection-form-refactor
krishnaglick e41a42f
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick dfcd4cc
Fix Build
krishnaglick eb54ec2
Merge branch 'master' into kc/connection-form-refactor
krishnaglick 34434d2
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick be4001b
Fix build
krishnaglick 9d95504
Fixing a bug
krishnaglick b53abb8
Merge branch 'master' into kc/connection-form-refactor
krishnaglick 50bbb3f
Fix failing test
krishnaglick 65584ca
Fixes e2e
krishnaglick 3da331e
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick e841140
Type consolidation
krishnaglick 92b7c0c
useCallback, improvements to connection create onAfter, and removing …
krishnaglick 8c5558b
cleanup
krishnaglick d390833
Removing an unused prop
krishnaglick 3d094fb
errorStatusMessage and mapFormPropsToOperation tests
krishnaglick 3ec2cf2
useUniqueFormId and useInitialValues tests
krishnaglick 0452458
Cleanup, onFrequencySelect is moved to its use location, better test …
krishnaglick df444fe
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick aec7b10
Better formSubmit handling for new connection
krishnaglick 6aabc3b
Commenting and some cleanup
krishnaglick 8af9bd2
Comments!
krishnaglick c9ca5b5
Merge branch 'master' into kc/connection-form-refactor
krishnaglick f809c68
Merge branch 'master' into kc/connection-form-refactor
krishnaglick 61251a0
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick 208cf78
Fixing errors from the merge
krishnaglick c7f9afa
mock data cleanup
krishnaglick cc91839
Better TODO
krishnaglick e89f762
Merge branch 'master' into kc/connection-form-refactor
krishnaglick 96b12a5
Merge branch 'master' into kc/connection-form-refactor
krishnaglick 3395828
Merge branch 'master' into kc/connection-form-refactor
krishnaglick 4c17657
Merge branch 'master' into kc/connection-form-refactor
krishnaglick 498743a
Merge branch 'master' into kc/connection-form-refactor
krishnaglick ed26655
Merge branch 'master' into kc/connection-form-refactor
krishnaglick 955848c
onFrequencySelect is now always called
krishnaglick a0beff7
Edmundo CR
krishnaglick e823d60
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick e2dc6e1
Merge branch 'master' into kc/connection-form-refactor
lmossman 7f4916a
Remove whitespace
krishnaglick cec5034
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick 5ac6baa
Bridging changes to bring things inline
krishnaglick 184732f
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick 50df6de
Builds and tests run
krishnaglick a45d98a
Merge branch 'master' of github.com:airbytehq/airbyte into kc/connect…
krishnaglick File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 }) => ( | ||
|
144 changes: 144 additions & 0 deletions
144
airbyte-webapp/src/hooks/services/Connection/ConnectionFormService.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
This will be changed in part 2.