Fix OAuth login buttons (#19135)

* Fix OAuth login buttons * Remove unnecessary "as unknown" * Prevent empty number fields from breaking * Fix issue with default values * Fix unit test
airbytehq · Nov 8, 2022 · 8b1095d · 8b1095d
1 parent 07618a0
commit 8b1095d
Show file tree

Hide file tree

Showing 10 changed files with 65 additions and 38 deletions.
diff --git a/airbyte-webapp/src/core/jsonSchema/schemaToYup.test.ts b/airbyte-webapp/src/core/jsonSchema/schemaToYup.test.ts
@@ -47,7 +47,12 @@ it("should build schema for simple case", () => {
 
   const expectedSchema = yup.object().shape({
     host: yup.string().trim().required("form.empty.error"),
-    port: yup.number().min(0).max(65536).required("form.empty.error"),
+    port: yup
+      .number()
+      .min(0)
+      .max(65536)
+      .required("form.empty.error")
+      .transform((val) => (isNaN(val) ? undefined : val)),
     user: yup.string().trim().required("form.empty.error"),
     is_sandbox: yup.boolean().default(false),
     is_field_no_default: yup.boolean().required("form.empty.error"),

diff --git a/airbyte-webapp/src/core/jsonSchema/schemaToYup.ts b/airbyte-webapp/src/core/jsonSchema/schemaToYup.ts
@@ -65,7 +65,7 @@ export const buildYupFormForJsonSchema = (
       schema = yup.boolean();
       break;
     case "integer":
-      schema = yup.number();
+      schema = yup.number().transform((value) => (isNaN(value) ? undefined : value));
 
       if (jsonSchema?.minimum !== undefined) {
         schema = schema.min(jsonSchema?.minimum);

diff --git a/airbyte-webapp/src/views/Connector/ConnectorForm/ConnectorForm.tsx b/airbyte-webapp/src/views/Connector/ConnectorForm/ConnectorForm.tsx
@@ -56,7 +56,11 @@ const PatchInitialValuesWithWidgetConfig: React.FC<{
       .reduce((acc, [key, value]) => setIn(acc, key, value.default), patchedConstValues);
 
     if (patchedDefaultValues?.connectionConfiguration) {
-      setFieldValue("connectionConfiguration", patchedDefaultValues.connectionConfiguration);
+      setTimeout(() => {
+        // We need to push this out one execution slot, so the form isn't still in its
+        // initialization status and won't react to this call but would just take the initialValues instead.
+        setFieldValue("connectionConfiguration", patchedDefaultValues.connectionConfiguration);
+      });
     }
 
     // eslint-disable-next-line react-hooks/exhaustive-deps

diff --git a/airbyte-webapp/src/views/Connector/ConnectorForm/FormRoot.tsx b/airbyte-webapp/src/views/Connector/ConnectorForm/FormRoot.tsx
@@ -38,7 +38,7 @@ export const FormRoot: React.FC<FormRootProps> = ({
   selectedConnector,
 }) => {
   const { dirty, isSubmitting, isValid } = useFormikContext<ConnectorFormValues>();
-  const { resetConnectorForm, isLoadingSchema, selectedService, isEditMode, formType } = useConnectorForm();
+  const { resetConnectorForm, isLoadingSchema, selectedConnectorDefinition, isEditMode, formType } = useConnectorForm();
 
   return (
     <Form>
@@ -47,7 +47,7 @@ export const FormRoot: React.FC<FormRootProps> = ({
         <div className={styles.loaderContainer}>
           <Spinner />
           <div className={styles.loadingMessage}>
-            <ShowLoadingMessage connector={selectedService?.name} />
+            <ShowLoadingMessage connector={selectedConnectorDefinition?.name} />
           </div>
         </div>
       )}

diff --git a/airbyte-webapp/src/views/Connector/ConnectorForm/components/Property/Control.tsx b/airbyte-webapp/src/views/Connector/ConnectorForm/components/Property/Control.tsx
@@ -129,5 +129,15 @@ export const Control: React.FC<ControlProps> = ({
   }
   const inputType = property.type === "integer" ? "number" : "text";
 
-  return <Input {...field} autoComplete="off" type={inputType} value={value ?? ""} disabled={disabled} error={error} />;
+  return (
+    <Input
+      {...field}
+      placeholder={inputType === "number" ? property.default?.toString() : undefined}
+      autoComplete="off"
+      type={inputType}
+      value={value ?? ""}
+      disabled={disabled}
+      error={error}
+    />
+  );
 };
diff --git a/...yte-webapp/src/views/Connector/ConnectorForm/components/Sections/auth/AuthButton.test.tsx b/...yte-webapp/src/views/Connector/ConnectorForm/components/Sections/auth/AuthButton.test.tsx
@@ -1,6 +1,7 @@
 import { screen, render } from "@testing-library/react";
 import { TestWrapper } from "test-utils/testutils";
 
+import { ConnectorDefinition, ConnectorDefinitionSpecification } from "core/domain/connector";
 import { useFormikOauthAdapter } from "views/Connector/ConnectorForm/components/Sections/auth/useOauthFlowAdapter";
 import { useConnectorForm } from "views/Connector/ConnectorForm/connectorFormContext";
 import { useAuthentication } from "views/Connector/ConnectorForm/useAuthentication";
@@ -36,10 +37,9 @@ const baseUseFormikOauthAdapterValues = {
 
 jest.mock("views/Connector/ConnectorForm/connectorFormContext");
 const mockUseConnectorForm = useConnectorForm as unknown as jest.Mock<Partial<typeof useConnectorForm>>;
-const baseUseConnectorFormValues = {
-  selectedConnector: "abcde",
-  allowOAuthConnector: true,
-  selectedService: undefined,
+const baseUseConnectorFormValues: Partial<ReturnType<typeof useConnectorForm>> = {
+  selectedConnectorDefinition: { sourceDefinitionId: "abcde", name: "Acme" } as ConnectorDefinition,
+  selectedConnectorDefinitionSpecification: {} as ConnectorDefinitionSpecification,
 };
 
 jest.mock("views/Connector/ConnectorForm/useAuthentication");
@@ -55,9 +55,9 @@ describe("auth button", () => {
   it("initially renders with correct message and no status message", () => {
     // no auth errors
     mockUseConnectorForm.mockImplementationOnce(() => {
-      const { selectedConnector, selectedService } = baseUseConnectorFormValues;
+      const { selectedConnectorDefinitionSpecification, selectedConnectorDefinition } = baseUseConnectorFormValues;
 
-      return { selectedConnector, selectedService };
+      return { selectedConnectorDefinitionSpecification, selectedConnectorDefinition };
     });
 
     // not done
@@ -75,7 +75,7 @@ describe("auth button", () => {
     );
 
     // correct button text
-    const button = screen.getByRole("button", { name: "Authenticate your account" });
+    const button = screen.getByRole("button", { name: "Authenticate your Acme account" });
     expect(button).toBeInTheDocument();
 
     // no error message
@@ -90,9 +90,9 @@ describe("auth button", () => {
   it("after successful authentication, it renders with correct message and success message", () => {
     // no auth errors
     mockUseConnectorForm.mockImplementationOnce(() => {
-      const { selectedConnector, selectedService } = baseUseConnectorFormValues;
+      const { selectedConnectorDefinitionSpecification, selectedConnectorDefinition } = baseUseConnectorFormValues;
 
-      return { selectedConnector, selectedService };
+      return { selectedConnectorDefinitionSpecification, selectedConnectorDefinition };
     });
 
     // done
@@ -123,9 +123,9 @@ describe("auth button", () => {
     mockUseAuthentication.mockReturnValue({ hiddenAuthFieldErrors: { field: "form.empty.error" } });
 
     mockUseConnectorForm.mockImplementationOnce(() => {
-      const { selectedConnector, selectedService } = baseUseConnectorFormValues;
+      const { selectedConnectorDefinitionSpecification, selectedConnectorDefinition } = baseUseConnectorFormValues;
 
-      return { selectedConnector, selectedService };
+      return { selectedConnectorDefinitionSpecification, selectedConnectorDefinition };
     });
 
     // not done
@@ -143,7 +143,7 @@ describe("auth button", () => {
     );
 
     // correct button
-    const button = screen.getByRole("button", { name: "Authenticate your account" });
+    const button = screen.getByRole("button", { name: "Authenticate your Acme account" });
     expect(button).toBeInTheDocument();
 
     // error message

diff --git a/airbyte-webapp/src/views/Connector/ConnectorForm/components/Sections/auth/AuthButton.tsx b/airbyte-webapp/src/views/Connector/ConnectorForm/components/Sections/auth/AuthButton.tsx
@@ -41,19 +41,19 @@ function getAuthenticateMessageId(connectorDefinitionId: string): string {
 }
 
 export const AuthButton: React.FC = () => {
-  const { selectedService, selectedConnector } = useConnectorForm();
+  const { selectedConnectorDefinition, selectedConnectorDefinitionSpecification } = useConnectorForm();
   const { hiddenAuthFieldErrors } = useAuthentication();
   const authRequiredError = Object.values(hiddenAuthFieldErrors).includes("form.empty.error");
 
   // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-  const { loading, done, run, hasRun } = useFormikOauthAdapter(selectedConnector!);
+  const { loading, done, run, hasRun } = useFormikOauthAdapter(selectedConnectorDefinitionSpecification!);
 
-  if (!selectedConnector) {
+  if (!selectedConnectorDefinitionSpecification) {
     console.error("Entered non-auth flow while no connector is selected");
     return null;
   }
 
-  const definitionId = ConnectorSpecification.id(selectedConnector);
+  const definitionId = ConnectorSpecification.id(selectedConnectorDefinitionSpecification);
   const Component = getButtonComponent(definitionId);
 
   const messageStyle = classnames(styles.message, {
@@ -63,7 +63,10 @@ export const AuthButton: React.FC = () => {
   const buttonLabel = done ? (
     <FormattedMessage id="connectorForm.reauthenticate" />
   ) : (
-    <FormattedMessage id={getAuthenticateMessageId(definitionId)} values={{ connector: selectedService?.name }} />
+    <FormattedMessage
+      id={getAuthenticateMessageId(definitionId)}
+      values={{ connector: selectedConnectorDefinition?.name }}
+    />
   );
   return (
     <div className={styles.authSectionRow}>

diff --git a/airbyte-webapp/src/views/Connector/ConnectorForm/connectorFormContext.tsx b/airbyte-webapp/src/views/Connector/ConnectorForm/connectorFormContext.tsx
@@ -16,8 +16,8 @@ interface ConnectorFormContext {
   addUnfinishedFlow: (key: string, info?: Record<string, unknown>) => void;
   removeUnfinishedFlow: (key: string) => void;
   resetConnectorForm: () => void;
-  selectedService?: ConnectorDefinition;
-  selectedConnector?: ConnectorDefinitionSpecification;
+  selectedConnectorDefinition?: ConnectorDefinition;
+  selectedConnectorDefinitionSpecification?: ConnectorDefinitionSpecification;
   isLoadingSchema?: boolean;
   isEditMode?: boolean;
   validationSchema: AnySchema;
@@ -63,7 +63,7 @@ export const ConnectorFormContextProvider: React.FC<React.PropsWithChildren<Conn
 
   const ctx = useMemo<ConnectorFormContext>(() => {
     const unfinishedFlows = widgetsInfo["_common.unfinishedFlows"] ?? {};
-    return {
+    const context: ConnectorFormContext = {
       widgetsInfo,
       getValues,
       setUiWidgetsInfo,
@@ -89,6 +89,7 @@ export const ConnectorFormContextProvider: React.FC<React.PropsWithChildren<Conn
         resetUiWidgetsInfo();
       },
     };
+    return context;
   }, [
     widgetsInfo,
     getValues,

diff --git a/airbyte-webapp/src/views/Connector/ConnectorForm/useAuthentication.test.tsx b/airbyte-webapp/src/views/Connector/ConnectorForm/useAuthentication.test.tsx
@@ -34,7 +34,8 @@ const mockContext = ({ connector, values, submitCount, fieldMeta = {} }: MockPar
   });
   mockConnectorForm.mockReturnValue({
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    selectedConnector: { ...connector, sourceDefinitionId: "12345", jobInfo: {} as any },
+    selectedConnectorDefinitionSpecification: { ...connector, sourceDefinitionId: "12345", jobInfo: {} as any },
+    getValues: (values) => values,
   });
 };
 

diff --git a/airbyte-webapp/src/views/Connector/ConnectorForm/useAuthentication.tsx b/airbyte-webapp/src/views/Connector/ConnectorForm/useAuthentication.tsx
@@ -131,29 +131,32 @@ interface AuthenticationHook {
 
 export const useAuthentication = (): AuthenticationHook => {
   const { values, getFieldMeta, submitCount } = useFormikContext<ConnectorFormValues>();
-  const { selectedConnector } = useConnectorForm();
+  const { getValues, selectedConnectorDefinitionSpecification: connectorSpec } = useConnectorForm();
 
   const allowOAuthConnector = useFeature(FeatureItem.AllowOAuthConnector);
 
-  const advancedAuth = selectedConnector?.advancedAuth;
-  const legacyOauthSpec = selectedConnector?.authSpecification?.oauth2Specification;
+  const advancedAuth = connectorSpec?.advancedAuth;
+  const legacyOauthSpec = connectorSpec?.authSpecification?.oauth2Specification;
 
-  const spec = selectedConnector?.connectionSpecification as JSONSchema7;
+  const spec = connectorSpec?.connectionSpecification as JSONSchema7;
+
+  const valuesWithDefaults = useMemo(() => getValues(values), [getValues, values]);
 
   const isAuthButtonVisible = useMemo(() => {
     const shouldShowAdvancedAuth =
-      advancedAuth && shouldShowButtonForAdvancedAuth(advancedAuth.predicateKey, advancedAuth.predicateValue, values);
+      advancedAuth &&
+      shouldShowButtonForAdvancedAuth(advancedAuth.predicateKey, advancedAuth.predicateValue, valuesWithDefaults);
     const shouldShowLegacyAuth =
-      legacyOauthSpec && shouldShowButtonForLegacyAuth(spec, legacyOauthSpec.rootObject as Path, values);
+      legacyOauthSpec && shouldShowButtonForLegacyAuth(spec, legacyOauthSpec.rootObject as Path, valuesWithDefaults);
     return Boolean(allowOAuthConnector && (shouldShowAdvancedAuth || shouldShowLegacyAuth));
-  }, [values, advancedAuth, legacyOauthSpec, spec, allowOAuthConnector]);
+  }, [advancedAuth, valuesWithDefaults, legacyOauthSpec, spec, allowOAuthConnector]);
 
   // Fields that are filled by the OAuth flow and thus won't need to be shown in the UI if OAuth is available
   const implicitAuthFieldPaths = useMemo(
     () => [
       // Fields from `advancedAuth` connectors
       ...(advancedAuth
-        ? Object.values(serverProvidedOauthPaths(selectedConnector)).map((f) =>
+        ? Object.values(serverProvidedOauthPaths(connectorSpec)).map((f) =>
             makeConnectionConfigurationPath(f.path_in_connector_config)
           )
         : []),
@@ -165,7 +168,7 @@ export const useAuthentication = (): AuthenticationHook => {
           ]
         : []),
     ],
-    [advancedAuth, legacyOauthSpec, selectedConnector]
+    [advancedAuth, legacyOauthSpec, connectorSpec]
   );
 
   const isHiddenAuthField = useCallback(
@@ -208,8 +211,8 @@ export const useAuthentication = (): AuthenticationHook => {
   );
 
   const hasAuthFieldValues: boolean = useMemo(() => {
-    return implicitAuthFieldPaths.some((path) => getIn(values, path) !== undefined);
-  }, [implicitAuthFieldPaths, values]);
+    return implicitAuthFieldPaths.some((path) => !!getIn(valuesWithDefaults, path));
+  }, [implicitAuthFieldPaths, valuesWithDefaults]);
 
   return {
     isHiddenAuthField,