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

Connection Form Refactor - Part Two #16748

Merged
merged 155 commits into from
Oct 7, 2022

Conversation

krishnaglick
Copy link
Contributor

@krishnaglick krishnaglick commented Sep 14, 2022

What

Renames and re-architects the Connection Edit and Create code, as well as changing how the connection edit's name and enable/disable toggle work together with it all for better consistency.

Resolves #15606
Resolves #15605
Resolves #15607
Resolves #16368

This sidecar PR from Teal isolates some of the renaming and file moving work to reduce the complexity of this PR.
This sidecar PR from Teal handles most of the styled components -> scss module changes from this PR.

How

Part One introduced the connection form service to better bridge the edit and create forms, as well as lay the groundwork for the greater refactor.
Part two takes this quite a bit further. The original ConnectionFormService was pared down to things both Create and Edit use and need, removing any code-creep from one side or the other. ConnectionEditService was made to contain the necessary edit-specific logic without cluttering up the form service. It gives the Connection Edit Name, among other components, access to the same Connection object the ConnectionReplication (formerly ReplicationView) component uses so altering the Name or toggling the status does not get reset if you save the form!

Reading Order

Major

  • ConnectionFormService.tsx - Was renamed, folder changed to ConnectionForm from Connection. Previous Version. The complexity of this service was scaled back greatly. There's no shared onSubmit logic piped through here now, that's been transformed into the tidyConnectionFormValues function. It just holds shared logic to avoid prop-drilling and keep it DRY.

  • ConnectionEditService.tsx - There was a lot of unnecessary grouped logic in the Connection Form Service. The Connection Edit Service was split out and moved way above in the component tree to encapsulate the connection name and connection toggle logic. It encapsulates the connection edit specific logic.

  • CreateConnection.tsx - Was renamed from CreateConnectionContent.tsx but there are changes.

  • ConnectionReplication.tsx - Was renamed from ReplicationView.tsx. Slimmed down the component by moving a lot of the logic into the ConnectionEditService.

  • ConnectionFormFields.tsx - Pulled out the duplicate logic from the former ConnectionForm.tsx component and encapsulated it here. Removed the use of styled components.

Minor

  • CreateConnectionNameField.tsx - This was extracted from the ConnectionForm.tsx. No changes were made except StyledComponents -> SCSS Modules.
  • SchemaError.tsx - Extracted from CreateConnectionContent.tsx
  • ResetWarningModal.tsx - Moved the reset warning modal out of the replication view. Old Location
  • ConnectionItemPage.tsx - The onAfterSaveSchema call was simply making an analytics call. It's been moved to the appropriate location. Additionally the entire component has an Inner where most of the logic is so it has access to the ConnectionEditService.
  • ConnectionName.tsx - Update call now uses PATCH-style logic, leverages the Connection Edit Service.
  • ConnectionPageTitle.tsx - Slimmed down thanks to Connection Edit Service. Prop-drilled logic moved into it as appropriate.
  • EnabledControl.tsx - Update call now uses PATCH-style logic, leverages the Connection Edit Service. onStatusUpdating was removed as it was actually unused.
  • ResetWarningModal.tsx - Logic was pulled out of the ConnectionReplication component and placed into its own file.
  • StatusMainInfo.tsx - Slimmed down thanks to Connection Edit Service.
  • CatalogDiffModal.tsx - Closing the modal on unmount logic moved into here from ConnectionReplication.

Misc

  • useConnectionHook.tsx - The logic removed from this file has been folded into the ConnectionEditService since it was only used in a single place, and it's easier to manage this way as I felt.
  • useSourceHook.tsx - Turned an interface

Notes

  • Teal made a ticket for us to fix up the Yup schema.

🚨 User Impact 🚨

This touches just about every part of the connection create and edit process. It has high potential to break things.

Copy link
Contributor

@dizel852 dizel852 left a comment

Choose a reason for hiding this comment

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

Overall - LGTM. Much cleaner 👍
Also tested locally with "Postgres <> Postgres" connections - did not find any regression bugs...
I approve, but need to fix latest comments from @lmossman

krishnaglick and others added 5 commits October 7, 2022 09:21
…mService.tsx

Co-authored-by: Lake Mossman <lake@airbyte.io>
…tService.test.tsx

Co-authored-by: Lake Mossman <lake@airbyte.io>
…ge/ConnectionReplicationTab.tsx

Co-authored-by: Lake Mossman <lake@airbyte.io>
Co-authored-by: Krishna (kc) Glick <krishna@airbyte.io>
@krishnaglick
Copy link
Contributor Author

I ran my new e2e tests against this branch and they passed! 🎉

Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

Approving, as all of my comments were addressed!

I did not take the time to test this locally, as I saw that all of the test cases in the test-it doc were already verified

@teallarson teallarson self-requested a review October 7, 2022 19:13
Copy link
Contributor

@teallarson teallarson left a comment

Choose a reason for hiding this comment

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

re-reviewed final code. did not complete a final manual test.

krishnaglick and others added 2 commits October 7, 2022 15:18
…mService.test.tsx

Co-authored-by: Teal Larson <LARSON.TEAL@GMAIL.COM>
…tService.test.tsx

Co-authored-by: Teal Larson <LARSON.TEAL@GMAIL.COM>
@krishnaglick krishnaglick merged commit 33f0cc6 into master Oct 7, 2022
@krishnaglick krishnaglick deleted the kc/connection-form-refactor-part-two branch October 7, 2022 19:49
letiescanciano added a commit that referenced this pull request Oct 10, 2022
…vation

* master: (22 commits)
  Update full-refresh-append.md (#17784)
  Update full-refresh-overwrite.md (#17783)
  Update incremental-append.md (#17785)
  Update incremental-deduped-history.md (#17786)
  Update cdc.md (#17787)
  🪟 🔧 Ignore classnames during jest snapshot comparison (#17773)
  feat: replace openjdk with amazoncorretto:17.0.4 on connectors for seсurity compliance (#17511)
  Start testing buildpulse. (#17712)
  Add missing types to the registry (#17763)
  jobs db descriptions (#16543)
  config db data catalog (#16427)
  Update lowcode docs (#17752)
  db migrations to support new webhook operations (#17671)
  Bump Airbyte version from 0.40.13 to 0.40.14 (#17762)
  September Release Notes (#17754)
  Revert "Use java-datadog-tracer-base image (#17625)" (#17759)
  Add connection migrations for schema changes (#17651)
  Connection Form Refactor - Part Two (#16748)
  Improve E2E testing around the Connection Form (#17577)
  Bump strict encrypt version (#17747)
  ...
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
* Work started

* Minor cleanup

* Some cleanup

* Lots moved into context

* WIP, stepping in the right direction

* WIP testing

* Post-merge fix

* Observables!

* WIP tests

* Tests!

* CI test

* CI?

* perhaps

* Only show name field during create

* Fix Build

* Fix build

* Fixing a bug

* Fix failing test

* Fixes e2e

* Type consolidation

* useCallback, improvements to connection create onAfter, and removing dirty tracking

* cleanup

* Removing an unused prop

* errorStatusMessage and mapFormPropsToOperation tests

* useUniqueFormId and useInitialValues tests

* Cleanup, onFrequencySelect is moved to its use location, better test wrapper,  and expanding use of FormError

* Better formSubmit handling for new connection

* Commenting and some cleanup

* Comments!

* Fixing errors from the merge

* mock data cleanup

* Better TODO

* onFrequencySelect is now always called

* Edmundo CR

* Remove whitespace

* Move connection into state so it gets updated

* Consolidate on connection object

* Remove ModalCancel throw + form clearing in create submit function

* Some cleanup

* Fixing error

* Fixing build error

* Rename file

* Bridging changes to bring things inline

* Builds and tests run

* replication view almost works as expected

* About to embark on another huge change! Committing here before I break more things.

* slowly, unity

* Code appears to be working

* Some minor cleanup

* Form dirty change tracking

* WIP styling moving and fixing

* skip refresh on update if status !== active

* Fixing styles

* Minor

* More cleanup and improvements

* Rename CreateConnectionContent -> CreateConnection

* refreshSchema moved into the connection form service

* Re-add TODO

* schemaErrorStatus => schemaError

* CreateConnectionName -> CreateConnectionNameField

* Logic for readonly vs edit

* Added TODO, fixed a re-initialize issue

* Remove unused ConnectionForm component

* Improved refresh schema logic

* undoing some of the cancel work as it felt like the wrong place to do it

* No longer need to know if the status is updating, that info is available from the form edit context

* Move close modal & confirmation modal logic to where it's used

* Remove unnecessary useUnmount

* Cleanup and removal of unused

* Somehow missed removing this

* Fixing failing test

* Added missing initialvalues logic. Added connection form service tests.

* Connection Edit tests!

* connection replication test pt 1

* Moving files to make master merge easier

* Remove snapshot

* non-default export

* Remove unused

* Minor improvements

* Edmundo CR.

* name now optional for connection editing

* Schema refresh errors handled on connection replication view

* schema refreshing logic, confirm catalog dialog abstraction

* CreateConnection -> CreateConnectionForm

* form config test cleanup

* connection replication tab tests

* Create Connection Form Tests

* Better jest mocking!

* Fixing issue setting schedule type to manual

* Tests fixed, and test-utils is better!

* Changing back to Once

* Fixing build from master merge

* Updating snapshots. If these change a lot it may be worth altering the tests to avoid snapshots.

* Edmundo & Lake Code Review Comments

* Tim CR

* Fixing a styling issue

* ?? is less clever than I thought

* Form change tracking has been improved to not leave dirty forms around. As well as it has been moved into ConnectionFormFields for DRY purposes

* Update airbyte-webapp/src/hooks/services/ConnectionForm/ConnectionFormService.tsx

Co-authored-by: Lake Mossman <lake@airbyte.io>

* Update airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.test.tsx

Co-authored-by: Lake Mossman <lake@airbyte.io>

* Update airbyte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/ConnectionReplicationTab.tsx

Co-authored-by: Lake Mossman <lake@airbyte.io>

* DRY out tests

* remove unknowns and fix mocked hook to be patch-style (airbytehq#17698)

Co-authored-by: Krishna (kc) Glick <krishna@airbyte.io>

* Self-CR

* idk why this was an issue but whatever

* Update airbyte-webapp/src/hooks/services/ConnectionForm/ConnectionFormService.test.tsx

Co-authored-by: Teal Larson <LARSON.TEAL@GMAIL.COM>

* Update airbyte-webapp/src/hooks/services/ConnectionEdit/ConnectionEditService.test.tsx

Co-authored-by: Teal Larson <LARSON.TEAL@GMAIL.COM>

Co-authored-by: Lake Mossman <lake@airbyte.io>
Co-authored-by: tealjulia <teal@airbyte.io>
Co-authored-by: Teal Larson <LARSON.TEAL@GMAIL.COM>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend Related to the Airbyte webapp area/platform issues related to the platform team/platform-move
Projects
None yet
7 participants