🪟 Show Validation Errors on Service Form for Missing Fields #15625
Conversation
This file contains hidden or 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
github-actions bot added area/platform 
teallarson force-pushed the teal/service-form-error-messaging branch from 59d7c4c to 2aae26b Compare teallarson changed the title 
teallarson commented Aug 22, 2022
Comment on lines +21 to +26
Contributor Author
There was a problem hiding this comment.
Some funky workarounds to be able to use mockImplementationOnce with TS, but I wanted to change the mocked return values for each test and I think this is the cleanest way to do it (based on this and similar threads)
teallarson force-pushed the teal/service-form-error-messaging branch from 3416176 to 2bc0696 Compare teallarson commented Aug 22, 2022
Contributor Author
There was a problem hiding this comment.
Added the base objects to keep repeated code out of these mock implementations so it was easier to tell which property was changing with each implementation.
teallarson force-pushed the teal/service-form-error-messaging branch from 2bc0696 to 6f99d3a Compare airbyte-webapp/src/views/Connector/ServiceForm/components/Sections/auth/AuthButton.tsx Outdated Show resolved Hide resolved
krishnaglick approved these changes Aug 24, 2022
timroes reviewed Aug 25, 2022
airbyte-webapp/src/views/Connector/ServiceForm/components/Sections/auth/AuthButton.module.scss Outdated Show resolved Hide resolved
timroes reviewed Aug 25, 2022
airbyte-webapp/src/views/Connector/ServiceForm/serviceFormContext.tsx Outdated Show resolved Hide resolved
timroes previously requested changes Aug 25, 2022
Contributor
timroes left a comment
timroes left a comment There was a problem hiding this comment.
Left some code style comments and SCSS suggestions. Have not yet tested locally.
teallarson force-pushed the teal/service-form-error-messaging branch from 142450a to 3cc01b9 Compare timroes reviewed Aug 25, 2022
airbyte-webapp/src/views/Connector/ServiceForm/serviceFormContext.tsx Outdated Show resolved Hide resolved
timroes dismissed their stale review Review has been addressed, dismissing to not block this until I can re-review.
teallarson force-pushed the teal/service-form-error-messaging branch from 98fefda to c85f1b2 Compare 
teallarson force-pushed the teal/service-form-error-messaging branch from 1e05535 to ec8b752 Compare rodireich pushed a commit that referenced this pull request Aug 29, 2022
* edit controls allow submit attempt for invalid form * remove unneeded calculations * add authentication required error message after submit attempt * use context instead * begin adding tests * auth button component tests * return map of field errors, not a boolean * cleanup * use scss variable * dry up repeated mock values * relative import for mock * fix casing * includes vs indexOf * changes from review * autofix space in comment
robbinhan pushed a commit to robbinhan/airbyte that referenced this pull request Sep 29, 2022
…q#15625) * edit controls allow submit attempt for invalid form * remove unneeded calculations * add authentication required error message after submit attempt * use context instead * begin adding tests * auth button component tests * return map of field errors, not a boolean * cleanup * use scss variable * dry up repeated mock values * relative import for mock * fix casing * includes vs indexOf * changes from review * autofix space in comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
What
closes #15598
This PR allows users to attempt to submit an incomplete form for setting up a connector. This allows validation errors to be surfaced which improves overall user experience (if I can't submit a form, but I don't know why, that's frustrating!).
This also adds an
Authentication requiredmessage if the validation error is OAuth-related.How
The
useServiceFormcontext now contains a specific object for errors related to hidden auth fields. TheAuthButtoncomponent grabs those and checks to see if it's due to an empty field.NOTE: This will (and can) only show an error message for Connectors that mark authentication as required in their spec. A few of these are: Mailchimp, Salesforce, and Github.
However, other Connectors such as HubSpot, Google Ads, Google Sheets, Facebook Ads, and more allow a user to test the connector without completing oauth (the check fails if there's no credentials, but that is different than the field having an error).
This is due to how the connector spec is written. If we want to surface the
Authentication requiredmessage, then the spec would need to be changed to include that auth is required.Testing
Authentication requiredAuthentication succeeded!. Currently, Salesforce and Mailchimp should work againstfrontend-devusing the workflow outlined in the "Testing OAuth Locally" Notion docRecommended reading order
AuthButton.*serviceFormContext.tsx