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

🪟 🎉 Client-side validation of cron expressions <= 1 hr for cloud #18044

Merged
merged 12 commits into from
Oct 27, 2022
Merged
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
/* eslint-disable @typescript-eslint/no-non-null-assertion */
import { act, render as tlr } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import React from "react";
import selectEvent from "react-select-event";
import mockConnection from "test-utils/mock-data/mockConnection.json";
import mockDest from "test-utils/mock-data/mockDestinationDefinition.json";
import { TestWrapper } from "test-utils/testutils";

import { AirbyteCatalog } from "core/request/AirbyteClient";
import { defaultFeatures, FeatureItem } from "hooks/services/Feature";
import * as sourceHook from "hooks/services/useSourceHook";

import { CreateConnectionForm } from "./CreateConnectionForm";
Expand Down Expand Up @@ -67,4 +70,72 @@ describe("CreateConnectionForm", () => {
const renderResult = await render();
expect(renderResult).toMatchSnapshot();
});

describe("cron expression validation", () => {
const INVALID_CRON_EXPRESSION = "invalid cron expression";
const CRON_EXPRESSION_EVERY_MINUTE = "* * * * * * ?";

it("should display an error for an invalid cron expression", async () => {
jest.spyOn(sourceHook, "useDiscoverSchema").mockImplementationOnce(() => baseUseDiscoverSchema);

const container = tlr(
<TestWrapper>
<CreateConnectionForm source={mockConnection.source} destination={mockConnection.destination} />
</TestWrapper>
);

await selectEvent.select(container.getByTestId("scheduleData"), /cron/i);

const cronExpressionInput = container.getByTestId("cronExpression");

userEvent.clear(cronExpressionInput);
await userEvent.type(cronExpressionInput, INVALID_CRON_EXPRESSION, { delay: 1 });

const errorMessage = container.getByText("Invalid cron expression");

expect(errorMessage).toBeInTheDocument();
});

it("should allow cron expressions under one hour when feature enabled", async () => {
jest.spyOn(sourceHook, "useDiscoverSchema").mockImplementationOnce(() => baseUseDiscoverSchema);

const container = tlr(
<TestWrapper>
<CreateConnectionForm source={mockConnection.source} destination={mockConnection.destination} />
</TestWrapper>
);

await selectEvent.select(container.getByTestId("scheduleData"), /cron/i);

const cronExpressionField = container.getByTestId("cronExpression");

await userEvent.type(cronExpressionField, `{selectall}${CRON_EXPRESSION_EVERY_MINUTE}`, { delay: 1 });

const errorMessage = container.queryByTestId("cronExpressionError");

expect(errorMessage).not.toBeInTheDocument();
});

it("should not allow cron expressions under one hour when feature not enabled", async () => {
jest.spyOn(sourceHook, "useDiscoverSchema").mockImplementationOnce(() => baseUseDiscoverSchema);

const featuresToInject = defaultFeatures.filter((f) => f !== FeatureItem.AllowSyncSubOneHourCronExpressions);

const container = tlr(
<TestWrapper features={featuresToInject}>
<CreateConnectionForm source={mockConnection.source} destination={mockConnection.destination} />
</TestWrapper>
);

await selectEvent.select(container.getByTestId("scheduleData"), /cron/i);

const cronExpressionField = container.getByTestId("cronExpression");

await userEvent.type(cronExpressionField, `{selectall}${CRON_EXPRESSION_EVERY_MINUTE}`, { delay: 1 });

const errorMessage = container.getByTestId("cronExpressionError");

expect(errorMessage).toBeInTheDocument();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,18 @@ import {
tidyConnectionFormValues,
useConnectionFormService,
} from "hooks/services/ConnectionForm/ConnectionFormService";
import { FeatureItem, useFeature } from "hooks/services/Feature";
import { useFormChangeTrackerService } from "hooks/services/FormChangeTracker";
import { useCreateConnection } from "hooks/services/useConnectionHook";
import { SchemaError as SchemaErrorType, useDiscoverSchema } from "hooks/services/useSourceHook";
import { useCurrentWorkspaceId } from "services/workspaces/WorkspacesService";
import CreateControls from "views/Connection/ConnectionForm/components/CreateControls";
import { OperationsSection } from "views/Connection/ConnectionForm/components/OperationsSection";
import { ConnectionFormFields } from "views/Connection/ConnectionForm/ConnectionFormFields";
import { connectionValidationSchema, FormikConnectionFormValues } from "views/Connection/ConnectionForm/formConfig";
import {
createConnectionValidationSchema,
FormikConnectionFormValues,
} from "views/Connection/ConnectionForm/formConfig";

import styles from "./CreateConnectionForm.module.scss";
import { CreateConnectionNameField } from "./CreateConnectionNameField";
Expand All @@ -44,10 +48,11 @@ const CreateConnectionFormInner: React.FC<CreateConnectionPropsInner> = ({ schem

const { connection, initialValues, mode, formId, getErrorMessage, setSubmitError } = useConnectionFormService();
const [editingTransformation, setEditingTransformation] = useState(false);
const allowSubOneHourCronExpressions = useFeature(FeatureItem.AllowSyncSubOneHourCronExpressions);

const onFormSubmit = useCallback(
async (formValues: FormikConnectionFormValues, formikHelpers: FormikHelpers<FormikConnectionFormValues>) => {
const values = tidyConnectionFormValues(formValues, workspaceId, mode);
const values = tidyConnectionFormValues(formValues, workspaceId, mode, allowSubOneHourCronExpressions);

try {
const createdConnection = await createConnection({
Expand Down Expand Up @@ -89,6 +94,7 @@ const CreateConnectionFormInner: React.FC<CreateConnectionPropsInner> = ({ schem
afterSubmitConnection,
navigate,
setSubmitError,
allowSubOneHourCronExpressions,
]
);

Expand All @@ -101,9 +107,8 @@ const CreateConnectionFormInner: React.FC<CreateConnectionPropsInner> = ({ schem
<div className={styles.connectionFormContainer}>
<Formik
initialValues={initialValues}
validationSchema={connectionValidationSchema(mode)}
validationSchema={createConnectionValidationSchema({ mode, allowSubOneHourCronExpressions })}
onSubmit={onFormSubmit}
validateOnChange={false}
josephkmh marked this conversation as resolved.
Show resolved Hide resolved
>
{({ values, isSubmitting, isValid, dirty }) => (
<Form>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { useGetDestinationDefinitionSpecification } from "services/connector/Des
import { FormError, generateMessageFromError } from "utils/errorStatusMessage";
import {
ConnectionFormValues,
connectionValidationSchema,
createConnectionValidationSchema,
FormikConnectionFormValues,
mapFormPropsToOperation,
useInitialValues,
Expand All @@ -33,10 +33,14 @@ export const tidyConnectionFormValues = (
values: FormikConnectionFormValues,
workspaceId: string,
mode: ConnectionFormMode,
allowSubOneHourCronExpressions: boolean,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to avoid extending this function with yet another argument, but it would have required refactoring quite a lot of the connection form schema.

operations?: OperationRead[]
): ValuesProps => {
// TODO (https://github.com/airbytehq/airbyte/issues/17279): We should try to fix the types so we don't need the casting.
const formValues: ConnectionFormValues = connectionValidationSchema(mode).cast(values, {
const formValues: ConnectionFormValues = createConnectionValidationSchema({
mode,
allowSubOneHourCronExpressions,
}).cast(values, {
context: { isRequest: true },
}) as unknown as ConnectionFormValues;

Expand Down
1 change: 1 addition & 0 deletions airbyte-webapp/src/hooks/services/Feature/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ export const defaultFeatures = [
FeatureItem.AllowSync,
FeatureItem.AllowUpdateConnectors,
FeatureItem.AllowUploadCustomImage,
FeatureItem.AllowSyncSubOneHourCronExpressions,
];
1 change: 1 addition & 0 deletions airbyte-webapp/src/hooks/services/Feature/types.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export enum FeatureItem {
AllowUpdateConnectors = "ALLOW_UPDATE_CONNECTORS",
AllowOAuthConnector = "ALLOW_OAUTH_CONNECTOR",
AllowSync = "ALLOW_SYNC",
AllowSyncSubOneHourCronExpressions = "ALLOW_SYNC_SUB_ONE_HOUR_CRON_EXPRESSIONS",
}

export type FeatureSet = Record<FeatureItem, boolean>;
1 change: 1 addition & 0 deletions airbyte-webapp/src/locales/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,7 @@
"form.cronExpression": "Cron expression*",
"form.cronExpression.placeholder": "Cron expression",
"form.cronExpression.error": "Invalid cron expression",
"form.cronExpression.underOneHourNotAllowed": "Syncs cannot execute more frequently than once per hour",
"form.cronExpression.message": "Enter a <lnk>cron expression</lnk> for when syncs should run (ex. \"0 0 12 * * ?\" => Will sync at 12:00 PM every day)",
"form.frequency.placeholder": "Select a frequency",
"form.frequency.message": "Set how often data should sync to the destination",
Expand Down
8 changes: 7 additions & 1 deletion airbyte-webapp/src/packages/cloud/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,13 @@ const Services: React.FC<React.PropsWithChildren<unknown>> = ({ children }) => (
<ConfirmationModalService>
<ModalServiceProvider>
<FormChangeTrackerService>
<FeatureService features={[FeatureItem.AllowOAuthConnector, FeatureItem.AllowSync]}>
<FeatureService
features={[
FeatureItem.AllowOAuthConnector,
FeatureItem.AllowSync,
FeatureItem.AllowSyncSubOneHourCronExpressions,
]}
>
<AppServicesProvider>
<AuthenticationProvider>
<HelmetProvider>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@
/* eslint-disable @typescript-eslint/no-non-null-assertion */

import { render as tlr, act } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import React, { Suspense } from "react";
import selectEvent from "react-select-event";
import mockConnection from "test-utils/mock-data/mockConnection.json";
import mockDest from "test-utils/mock-data/mockDestinationDefinition.json";
import { TestWrapper } from "test-utils/testutils";

import { WebBackendConnectionUpdate } from "core/request/AirbyteClient";
import { ConnectionEditServiceProvider } from "hooks/services/ConnectionEdit/ConnectionEditService";
import { defaultFeatures, FeatureItem } from "hooks/services/Feature";
import * as connectionHook from "hooks/services/useConnectionHook";

import { ConnectionReplicationTab } from "./ConnectionReplicationTab";
Expand Down Expand Up @@ -83,4 +86,65 @@ describe("ConnectionReplicationTab", () => {
expect(renderResult.findByText("We are fetching the schema of your data source.", { exact: false })).toBeTruthy();
});
});

describe("cron expression validation", () => {
const INVALID_CRON_EXPRESSION = "invalid cron expression";
const CRON_EXPRESSION_EVERY_MINUTE = "* * * * * * ?";

it("should display an error for an invalid cron expression", async () => {
setupSpies();
const renderResult = await render();

await selectEvent.select(renderResult.getByTestId("scheduleData"), /cron/i);

const cronExpressionInput = renderResult.getByTestId("cronExpression");

userEvent.clear(cronExpressionInput);
await userEvent.type(cronExpressionInput, INVALID_CRON_EXPRESSION, { delay: 1 });

const errorMessage = renderResult.getByText("Invalid cron expression");

expect(errorMessage).toBeInTheDocument();
});

it("should allow cron expressions under one hour when feature enabled", async () => {
setupSpies();

const renderResult = await render();

await selectEvent.select(renderResult.getByTestId("scheduleData"), /cron/i);

const cronExpressionField = renderResult.getByTestId("cronExpression");

await userEvent.type(cronExpressionField, `{selectall}${CRON_EXPRESSION_EVERY_MINUTE}`, { delay: 1 });

const errorMessage = renderResult.queryByTestId("cronExpressionError");

expect(errorMessage).not.toBeInTheDocument();
});

it("should not allow cron expressions under one hour when feature not enabled", async () => {
setupSpies();

const featuresToInject = defaultFeatures.filter((f) => f !== FeatureItem.AllowSyncSubOneHourCronExpressions);

const container = tlr(
<TestWrapper features={featuresToInject}>
<ConnectionEditServiceProvider connectionId={mockConnection.connectionId}>
<ConnectionReplicationTab />
</ConnectionEditServiceProvider>
</TestWrapper>
);

await selectEvent.select(container.getByTestId("scheduleData"), /cron/i);

const cronExpressionField = container.getByTestId("cronExpression");

await userEvent.type(cronExpressionField, `{selectall}${CRON_EXPRESSION_EVERY_MINUTE}`, { delay: 1 });

const errorMessage = container.getByTestId("cronExpressionError");

expect(errorMessage).toBeInTheDocument();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,18 @@ import {
tidyConnectionFormValues,
useConnectionFormService,
} from "hooks/services/ConnectionForm/ConnectionFormService";
import { FeatureItem, useFeature } from "hooks/services/Feature";
import { useModalService } from "hooks/services/Modal";
import { useConnectionService, ValuesProps } from "hooks/services/useConnectionHook";
import { useCurrentWorkspaceId } from "services/workspaces/WorkspacesService";
import { equal, naturalComparatorBy } from "utils/objects";
import { useConfirmCatalogDiff } from "views/Connection/CatalogDiffModal/useConfirmCatalogDiff";
import EditControls from "views/Connection/ConnectionForm/components/EditControls";
import { ConnectionFormFields } from "views/Connection/ConnectionForm/ConnectionFormFields";
import { connectionValidationSchema, FormikConnectionFormValues } from "views/Connection/ConnectionForm/formConfig";
import {
createConnectionValidationSchema,
FormikConnectionFormValues,
} from "views/Connection/ConnectionForm/formConfig";

import styles from "./ConnectionReplicationTab.module.scss";
import { ResetWarningModal } from "./ResetWarningModal";
Expand All @@ -39,6 +43,7 @@ export const ConnectionReplicationTab: React.FC = () => {
const { connection, schemaRefreshing, schemaHasBeenRefreshed, updateConnection, setSchemaHasBeenRefreshed } =
useConnectionEditService();
const { initialValues, mode, schemaError, getErrorMessage, setSubmitError } = useConnectionFormService();
const allowSubOneHourCronExpressions = useFeature(FeatureItem.AllowSyncSubOneHourCronExpressions);

useTrackPage(PageTrackingCodes.CONNECTIONS_ITEM_REPLICATION);

Expand Down Expand Up @@ -73,7 +78,13 @@ export const ConnectionReplicationTab: React.FC = () => {

const onFormSubmit = useCallback(
async (values: FormikConnectionFormValues, _: FormikHelpers<FormikConnectionFormValues>) => {
const formValues = tidyConnectionFormValues(values, workspaceId, mode, connection.operations);
const formValues = tidyConnectionFormValues(
values,
workspaceId,
mode,
allowSubOneHourCronExpressions,
connection.operations
);

// Detect whether the catalog has any differences in its enabled streams compared to the original one.
// This could be due to user changes (e.g. in the sync mode) or due to new/removed
Expand Down Expand Up @@ -135,6 +146,7 @@ export const ConnectionReplicationTab: React.FC = () => {
setSchemaHasBeenRefreshed,
setSubmitError,
workspaceId,
allowSubOneHourCronExpressions,
]
);

Expand All @@ -147,10 +159,9 @@ export const ConnectionReplicationTab: React.FC = () => {
) : !schemaRefreshing && connection ? (
<Formik
initialValues={initialValues}
validationSchema={connectionValidationSchema(mode)}
validationSchema={createConnectionValidationSchema({ mode, allowSubOneHourCronExpressions })}
onSubmit={onFormSubmit}
enableReinitialize
validateOnChange={false}
>
{({ values, isSubmitting, isValid, dirty, resetForm }) => (
<Form>
Expand Down
13 changes: 10 additions & 3 deletions airbyte-webapp/src/test-utils/testutils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import {
} from "core/request/AirbyteClient";
import { ServicesProvider } from "core/servicesProvider";
import { ConfirmationModalService } from "hooks/services/ConfirmationModal";
import { defaultFeatures, FeatureService } from "hooks/services/Feature";
import { defaultFeatures, FeatureItem, FeatureService } from "hooks/services/Feature";
import { ModalServiceProvider } from "hooks/services/Modal";
import en from "locales/en.json";
import { AnalyticsProvider } from "views/common/AnalyticsProvider";
Expand Down Expand Up @@ -44,12 +44,19 @@ export async function render<
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return renderResult!;
}
export const TestWrapper: React.FC<React.PropsWithChildren<unknown>> = ({ children }) => (

interface TestWrapperOptions {
features?: FeatureItem[];
}
export const TestWrapper: React.FC<React.PropsWithChildren<TestWrapperOptions>> = ({
children,
features = defaultFeatures,
}) => (
<ThemeProvider theme={{}}>
<IntlProvider locale="en" messages={en} onError={() => null}>
<ConfigContext.Provider value={{ config: defaultConfig }}>
<AnalyticsProvider>
<FeatureService features={defaultFeatures}>
<FeatureService features={features}>
<ServicesProvider>
<ModalServiceProvider>
<ConfirmationModalService>
Expand Down
2 changes: 1 addition & 1 deletion airbyte-webapp/src/utils/cron/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1 @@
export { validateCronExpression } from "./validateCronExpression";
export { validateCronExpression, validateCronFrequencyOneHourOrMore } from "./validateCronExpression";
Loading