🪟🎨 Clean up connector form form controls #21698

flash1293 · 2023-01-23T11:53:14Z

What

Fixes #20967

Remove legacy styled components
Unify look of edit connector form and new connector form
Make it possible to check connection without saving
Make "retest" button test current form state instead of saved state

Old look

Empty

Selected connector

Check in progress

Error on unsaved connector

Passed checks:

Edit form:

Error on edit form:

New look

Empty

Selected connector

Check in progress

Error on unsaved connector

Passed checks:

Edit form:

Error on edit form:

How

To pull the controls out of the card, the ConnectorForm had to be changed:
- Do not render the card as part of ConnectorCard anymore (move it down to the FormRoot level)
- Pass down the connector definition select as headerBlock, so it can be rendered inside the card within FormRoot
- Introduce a renderFooter render prop which is responsible for rendering the form controls:
  - in case of the connector card, it is rendering the testing card as well as the buttons for saving/deleting/canceling (depending on whether it's editing or a new actor)
  - in case of connector builder, it's rendering reset, cancel and save buttons
As the connector form is always rendered now even if there is no definition selected yet, the selector specification definition gets optional (which needs to be handled throughout the component)
Custom styling of various elements (success message, error message, spacings) are replaced by standard components (flex container and item, text, callout)

lmossman

Left some comments, main changes requested are about issues I found when locally testing.

Overall this is looking great though!

lmossman · 2023-01-25T02:37:05Z

airbyte-webapp/src/views/Connector/ConnectorForm/ConnectorForm.tsx

+  title?: React.ReactNode;
+  description?: React.ReactNode;
+  full?: boolean;


nit: it might be nice to group these props with the renderWithCard prop somehow, so indicate that these are being applied to the card and are only used if renderWithCard is true

(same comment for FormRoot since these props are drilled into there)

I specified this in the types using a union of an interface for props with renderWithCard: true and title/description/full and one with renderWithCard?: false without them - we use the same pattern for destinations vs. sources.

Also while doing this I deduped the type between form root and connector form and removed a bunch of stuff that's not used anymore.

lmossman · 2023-01-25T02:43:20Z

airbyte-webapp/src/components/connectorBuilder/StreamTestingPanel/ConfigMenu.tsx

+                            onClick={() => {
+                              setTestInputJson({});
+                            }}


If the inputs in this ConfigMenu are blank (e.g. first time filling them out, or Reset was clicked to clear them), then you type a value into an input, and click Reset again without saving, it doesn't clear out the value.

I think we need to also clear the formik state here to make that work.

Good catch, it needs to call resetConnectorForm which is passed in as well, forgot about that.

lmossman · 2023-01-25T02:51:39Z

airbyte-webapp/src/views/Connector/ConnectorCard/components/TestCard.tsx

+              <Button
+                variant="clear"
+                type="button"
+                icon={<FontAwesomeIcon icon={logsVisible ? faChevronDown : faChevronRight} />}
+                onClick={() => {
+                  setLogsVisible(!logsVisible);
+                }}
+              >
+                <FormattedMessage id="connector.testLogs" />
+              </Button>


It seems like these logs are only showing up when I click Test and save to perform the test.

If I click the Retest source button instead, the Connector logs tray is not rendered. I would expect as a user that the logs should be shown in both cases

This is existing behavior but I agree it doesn't make sense. Unified the connection check behavior on submit vs. the test button (we need to set the errorStatusRequest state)

lmossman · 2023-01-25T03:09:16Z

airbyte-webapp/src/views/Connector/ConnectorCard/components/TestCard.tsx

+              onClick={onRetestClick}
+              variant="secondary"
+              icon={<FontAwesomeIcon icon={faRefresh} />}
+              disabled={!isValid || (!isEditMode && !dirty)}


Suggested change

disabled={!isValid || (!isEditMode && !dirty)}

disabled={!isValid || (isEditMode && !dirty)}

I think this was a mistake. The current logic means that if you set up a new connector and don't change any of the values (e.g set up an E2E Testing destination and don't change any of the defaults), then it won't let you test or submit.

Good catch, actually it's even simpler - it should always be possible to re-test as long as the form is valid right now.

lmossman · 2023-01-25T03:10:45Z

airbyte-webapp/src/views/Connector/ConnectorCard/components/Controls.tsx

+            <FormattedMessage id="form.cancel" />
+          </Button>
+        )}
+        <Button type="submit" disabled={isSubmitting || !dirty}>


You'll need to fix this disabled logic in a similar way, so that if we arent in edit mode, the form can still be submitted even if its not dirty (see above comment)

…rols-refactor

flash1293 · 2023-01-25T12:32:10Z

Thanks for the review @lmossman , addressed your points

lmossman

Tested out the changes and it looks like all of the things I pointed out have been fixed 👍

…rols-refactor

flash1293 · 2023-01-26T11:30:14Z

I had to make a change because I realized we can't use the updated config values when testing an existing source/destination because with #21041 the check_for_update call can potentially update the actor in the database.

This is what I did for now (but we should probably re-visit this in the future):

Make the test button test the saved config again (reverting to the existing behavior)
Adjust the copy of the button to make it clearer what's happening
Disable the button when the form is dirty to avoid people thinking they test the updated state

Also, this change makes me think that we might just want to hide the testing card for new actors and only pop them in if the user tries to save - the current solution is more consistent but I'm not sure.

lmossman · 2023-01-26T23:06:05Z

@flash1293 to confirm my understanding here:

We cannot call the scheduler/sources/check_connection endpoint (i.e. the one we use when testing new connectors) for testing changes to existing connectors, because we don't have the actual secret values in the webapp, as they are saved to the database
We cannot call the check_connection_for_update endpoint for testing changes to existing connectors, because if there is a control message returned then that could result in the server writing the unsaved form values to the actor database record

If the above is correct, then I think the changes you made make sense to me as a temporary solution.

However, it still feels pretty bad to me that users have no way of testing changes to an existing connector without saving those changes. To solve this, it seems like we need a new API endpoint, or a boolean param added to check_connection_for_update, which causes the API to just run the connection check with the provided values, ignoring control messages and not updating the actor config in the database. Since that is exactly what we want to be able to do with the Test button in the UI.

cc @evantahler @pedroslopez to get your thoughts on the above

lmossman

Approving the current state of the PR as a good intermediate solution (see my comment above for my thoughts on follow-up changes here).

Also, this change makes me think that we might just want to hide the testing card for new actors and only pop them in if the user tries to save

I think the current solution is good, since as a user I like being able to test my configuration before I commit to actually saving it.

Tested locally and it works well. There is a merge conflict that needs to be fixed before merging.

pedroslopez · 2023-01-26T23:33:17Z

However, it still feels pretty bad to me that users have no way of testing changes to an existing connector without saving those changes. To solve this, it seems like we need a new API endpoint, or a boolean param added to check_connection_for_update, which causes the API to just run the connection check with the provided values, ignoring control messages and not updating the actor config in the database. Since that is exactly what we want to be able to do with the Test button in the UI.

@lmossman In general I agree that it's weird that invalid configurations could potentially be saved. However, this comes up because of single-use refresh tokens, which are part of the config. Once that refresh token is used, the previous config would be invalidated. Because of the way the UI worked (I haven't reviewed the actual changes in this PR), you could get into a situation where the check runs, refreshes tokens, and then the UI would call an "update" with the (invalid) values that were in the form. The problem is described more in #20913.

To at least make it clearer to the user we introduced a connectorConfigurationUpdated bool in the response on #21466 so that we could at least show the updated values to the user.

We could add an input to the check that avoids persisting config updates, but this would have to be on some separate button so that we make sure your credentials are valid and properly update refresh tokens if necessary on the check that runs when saving.

flash1293 · 2023-01-27T10:16:43Z

@pedroslopez thanks for taking a look!

Because of the way the UI worked (I haven't reviewed the actual changes in this PR), you could get into a situation where the check runs, refreshes tokens, and then the UI would call an "update" with the (invalid) values that were in the form.

Yes, this is handled on a separate PR

We could add an input to the check that avoids persisting config updates, but this would have to be on some separate button so that we make sure your credentials are valid and properly update refresh tokens if necessary on the check that runs when saving.

There is a separate button in the UI for this:

I can open a separate issue for this, I don't think it's a super pressing task though - the UI worked like this probably since the beginning.

flash1293 · 2023-01-27T10:39:50Z

@pedroslopez created an issue here: #21960

…rols-refactor

refactor

458bb8d

octavia-squidington-iv added the area/frontend Related to the Airbyte webapp label Jan 23, 2023

flash1293 marked this pull request as ready for review January 23, 2023 15:15

flash1293 requested a review from lmossman January 23, 2023 15:16

Joe Reuter added 2 commits January 23, 2023 16:16

Merge branch 'master' into flash1293/form-controls-refactor

5c063a7

Merge branch 'master' into flash1293/form-controls-refactor

d0a652e

lmossman requested changes Jan 25, 2023

View reviewed changes

Joe Reuter added 2 commits January 25, 2023 12:34

Merge remote-tracking branch 'origin/master' into flash1293/form-cont…

7cb7adb

…rols-refactor

review comments

5feec24

Merge branch 'master' into flash1293/form-controls-refactor

7693ea2

lmossman approved these changes Jan 25, 2023

View reviewed changes

Joe Reuter added 2 commits January 26, 2023 10:11

Merge remote-tracking branch 'origin/master' into flash1293/form-cont…

78bef81

…rols-refactor

do not pass changes to retest source

3c5def4

lmossman approved these changes Jan 26, 2023

View reviewed changes

flash1293 mentioned this pull request Jan 27, 2023

Allow to test existing actor with updated config #21960

Closed

Joe Reuter added 2 commits January 27, 2023 11:43

Merge remote-tracking branch 'origin/master' into flash1293/form-cont…

69166ce

…rols-refactor

reorder imports

9207ec1

flash1293 enabled auto-merge (squash) January 27, 2023 11:09

Joe Reuter added 3 commits January 27, 2023 12:31

Merge remote-tracking branch 'origin/master' into flash1293/form-cont…

4a87678

…rols-refactor

fix order of imports

1ff8752

Merge branch 'master' into flash1293/form-controls-refactor

d4495cc

flash1293 merged commit 1697b8e into master Jan 27, 2023

flash1293 deleted the flash1293/form-controls-refactor branch January 27, 2023 13:26

octavia-squidington-iii mentioned this pull request Jan 27, 2023

Bump Airbyte version from 0.40.29 to 0.40.30 #21991

Closed

octavia-squidington-iii mentioned this pull request Jan 27, 2023

Bump Airbyte version from 0.40.29 to 0.40.30 #22045

Merged

flash1293 mentioned this pull request Feb 1, 2023

Remove test card from empty actor form #22199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🪟🎨 Clean up connector form form controls #21698

🪟🎨 Clean up connector form form controls #21698

flash1293 commented Jan 23, 2023 •

edited

Loading

lmossman left a comment

lmossman Jan 25, 2023

flash1293 Jan 25, 2023

lmossman Jan 25, 2023

flash1293 Jan 25, 2023

lmossman Jan 25, 2023

flash1293 Jan 25, 2023

lmossman Jan 25, 2023

flash1293 Jan 25, 2023

lmossman Jan 25, 2023

flash1293 Jan 25, 2023

flash1293 commented Jan 25, 2023

lmossman left a comment

flash1293 commented Jan 26, 2023 •

edited

Loading

lmossman commented Jan 26, 2023

lmossman left a comment

pedroslopez commented Jan 26, 2023 •

edited

Loading

flash1293 commented Jan 27, 2023

flash1293 commented Jan 27, 2023

	disabled={!isValid \|\| (!isEditMode && !dirty)}
	disabled={!isValid \|\| (isEditMode && !dirty)}

🪟🎨 Clean up connector form form controls #21698

🪟🎨 Clean up connector form form controls #21698

Conversation

flash1293 commented Jan 23, 2023 • edited Loading

What

Old look

New look

How

lmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash1293 commented Jan 25, 2023

lmossman left a comment

Choose a reason for hiding this comment

flash1293 commented Jan 26, 2023 • edited Loading

lmossman commented Jan 26, 2023

lmossman left a comment

Choose a reason for hiding this comment

pedroslopez commented Jan 26, 2023 • edited Loading

flash1293 commented Jan 27, 2023

flash1293 commented Jan 27, 2023

flash1293 commented Jan 23, 2023 •

edited

Loading

flash1293 commented Jan 26, 2023 •

edited

Loading

pedroslopez commented Jan 26, 2023 •

edited

Loading