Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ function TestWrapper({ formErrors, formData }: TestProps) {

return (
<WorkflowActionResetForm
trigger={methods.trigger}
control={methods.control}
clearErrors={methods.clearErrors}
fieldErrors={formErrors}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ function TestWrapper({ formErrors, formData }: TestProps) {

return (
<WorkflowActionSignalForm
trigger={methods.trigger}
control={methods.control}
clearErrors={methods.clearErrors}
fieldErrors={formErrors}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,26 @@ describe('WorkflowActionStartForm', () => {
);

await user.click(screen.getByRole('radio', { name: 'Cron' }));
expect(
screen.getByRole('textbox', { name: 'Cron Schedule (UTC)' })
).toHaveAttribute('aria-invalid', 'true');
expect(screen.getByLabelText('Minute')).toHaveAttribute(
'aria-invalid',
'true'
);
expect(screen.getByLabelText('Hour')).toHaveAttribute(
'aria-invalid',
'true'
);
expect(screen.getByLabelText('Day of Month')).toHaveAttribute(
'aria-invalid',
'true'
);
expect(screen.getByLabelText('Month')).toHaveAttribute(
'aria-invalid',
'true'
);
expect(screen.getByLabelText('Day of Week')).toHaveAttribute(
'aria-invalid',
'true'
);
});

it('handles input changes correctly', async () => {
Expand Down Expand Up @@ -201,6 +218,7 @@ function TestWrapper({ formErrors, formData }: TestProps) {

return (
<WorkflowActionStartForm
trigger={methods.trigger}
control={methods.control}
clearErrors={methods.clearErrors}
fieldErrors={formErrors}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
import { type Result } from 'cron-validate/lib/result';
import { type Options } from 'cron-validate/lib/types';

import { cronValidate } from '@/utils/cron-validate/cron-validate';
import { type CronData } from '@/utils/cron-validate/cron-validate.types';

import { getCronFieldsError } from '../get-cron-fields-error';

jest.mock('@/utils/cron-validate/cron-validate');

const mockCronValidate = cronValidate as jest.MockedFunction<
typeof cronValidate
>;

describe('getCronFieldsError', () => {
beforeEach(() => {
jest.clearAllMocks();
});

it('should return null for valid cron expression', () => {
const { result } = setup({
isValid: true,
});

expect(result).toBeNull();
});

it('should handle field-specific errors correctly', () => {
const { result } = setup({
isValid: false,
errors: [
'Invalid value for minutes field: x',
'Invalid value for hours field: x',
'Invalid value for daysOfMonth field: x',
'Invalid value for months field: x',
'Invalid value for daysOfWeek field: x',
],
});

expect(result).toEqual({
minutes: 'Invalid value for minutes field: x',
hours: 'Invalid value for hours field: x',
daysOfWeek: 'Invalid value for daysOfWeek field: x',
daysOfMonth: 'Invalid value for daysOfMonth field: x',
months: 'Invalid value for months field: x',
});
});

it('should return general error when errors array has no field-specific matches', () => {
const { result } = setup({
isValid: false,
errors: ['Malformed cron expression', 'Cannot parse input'],
});

expect(result).toEqual({
general: 'Malformed cron expression',
});
});

it('should handle mixed field-specific and general errors', () => {
const { result } = setup({
isValid: false,
errors: [
'Invalid value for minutes field: abc',
'Syntax error in expression',
'Invalid value for hours field: xyz',
],
});

// Should prioritize field-specific errors over general ones
expect(result).toEqual({
minutes: 'Invalid value for minutes field: abc',
hours: 'Invalid value for hours field: xyz',
});
});
});

function setup({
cronText = '* * * * *',
isValid = true,
errors = [],
}: {
cronText?: string;
isValid?: boolean;
errors?: string[];
}) {
// Giving type for cron result is not straightforward, because of the conditional type within it
// So we make sure we are correctly creating partial mock and cast it to any
const mockCronResult = {
isValid: jest.fn(() => isValid),
getError: jest.fn(() => errors),
} satisfies Partial<Result<Options | CronData, string[]>> as any;

mockCronValidate.mockReturnValue(mockCronResult);

const result = getCronFieldsError(cronText);

return {
result,
mockCronResult,
mockIsValid: mockCronResult.isValid,
mockGetError: mockCronResult.getError,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,13 @@ describe('transformStartWorkflowFormToSubmission', () => {
const formData: StartWorkflowFormData = {
...baseFormData,
scheduleType: 'CRON',
cronSchedule: '0 9 * * 1-5',
cronSchedule: {
minutes: '0',
hours: '9',
daysOfMonth: '*',
months: '*',
daysOfWeek: '1-5',
},
};

const result = transformStartWorkflowFormToSubmission(formData);
Expand All @@ -50,7 +56,13 @@ describe('transformStartWorkflowFormToSubmission', () => {
...baseFormData,
scheduleType: 'NOW',
firstRunAt: '2024-01-01T10:00:00Z',
cronSchedule: '0 9 * * 1-5',
cronSchedule: {
minutes: '0',
hours: '9',
daysOfMonth: '*',
months: '*',
daysOfWeek: '1-5',
},
};

const result = transformStartWorkflowFormToSubmission(formData);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { CRON_FIELD_ORDER } from '@/components/cron-schedule-input/cron-schedule-input.constants';
import { cronValidate } from '@/utils/cron-validate/cron-validate';
import { type CronData } from '@/utils/cron-validate/cron-validate.types';

export const getCronFieldsError = (
cronString: string
): Partial<Record<keyof CronData | 'general', string>> | null => {
const cronObj = cronValidate(cronString);

if (!cronObj.isValid()) {
const errors = cronObj.getError();
const errorFieldsKeys = CRON_FIELD_ORDER;
const fieldsErrors: Partial<Record<keyof CronData, string>> = {};
errors.forEach((e) => {
const errorKey = errorFieldsKeys.find((key) =>
e.includes(`${key} field`)
);
if (errorKey) fieldsErrors[errorKey] = e;
});

if (!Object.keys(fieldsErrors).length) return { general: errors[0] };
else return fieldsErrors;
}

return null;
};
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { CRON_FIELD_ORDER } from '@/components/cron-schedule-input/cron-schedule-input.constants';
import { type Json } from '@/route-handlers/start-workflow/start-workflow.types';

import {
Expand All @@ -22,7 +23,9 @@ export default function transformStartWorkflowFormToSubmission(
firstRunAt: formData.firstRunAt,
}),
...(formData.scheduleType === 'CRON' && {
cronSchedule: formData.cronSchedule,
cronSchedule: CRON_FIELD_ORDER.map(
(key) => formData.cronSchedule?.[key]
).join(' '),
}),
...(formData.enableRetryPolicy && {
retryPolicy: {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import { z } from 'zod';

import { CRON_FIELD_ORDER } from '@/components/cron-schedule-input/cron-schedule-input.constants';
import {
WORKER_SDK_LANGUAGES,
WORKFLOW_ID_REUSE_POLICIES,
} from '@/route-handlers/start-workflow/start-workflow.constants';

import { getCronFieldsError } from '../helpers/get-cron-fields-error';

const baseSchema = z.object({
workflowType: z.object({
name: z.string().min(1, 'Workflow type name is required'),
Expand Down Expand Up @@ -81,8 +84,45 @@ const baseSchema = z.object({
// Schedule type fields
scheduleType: z.enum(['NOW', 'LATER', 'CRON']),
firstRunAt: z.string().optional(),
cronSchedule: z.string().optional(),
cronSchedule: z
.object({
minutes: z.string().min(1, 'Minutes is required'),
hours: z.string().min(1, 'Hours is required'),
daysOfMonth: z.string().min(1, 'Days of month is required'),
months: z.string().min(1, 'Months is required'),
daysOfWeek: z.string().min(1, 'Days of week is required'),
})
.superRefine((data, ctx) => {
const allFieldsHasValue = Object.values(data).every((value) =>
Boolean(value)
);

// If there are missing fields, no need to validate the cron schedule format.
if (!allFieldsHasValue) {
return;
}

const cronString = CRON_FIELD_ORDER.map((key) => data[key]).join(' ');
const cronFieldsErrors = getCronFieldsError(cronString);

if (!cronFieldsErrors) return;

if (cronFieldsErrors?.general) {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: 'Invalid cron schedule format',
});
} else {
Object.entries(cronFieldsErrors).forEach(([errorKey, errorMessage]) => {
ctx.addIssue({
code: z.ZodIssueCode.custom,
message: errorMessage,
path: [errorKey],
});
});
}
})
.optional(),
// Retry policy fields
enableRetryPolicy: z.boolean().optional(),
limitRetries: z.enum(['ATTEMPTS', 'DURATION']).optional(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import { DatePicker } from 'baseui/datepicker';
import { FormControl } from 'baseui/form-control';
import { Input } from 'baseui/input';
import { RadioGroup, Radio } from 'baseui/radio';
import { get } from 'lodash';
import { get, isObjectLike } from 'lodash';
import { Controller, useWatch } from 'react-hook-form';

import CronScheduleInput from '@/components/cron-schedule-input/cron-schedule-input';
import MultiJsonInput from '@/components/multi-json-input/multi-json-input';
import { WORKER_SDK_LANGUAGES } from '@/route-handlers/start-workflow/start-workflow.constants';

Expand All @@ -19,13 +20,16 @@ export default function WorkflowActionStartForm({
control,
clearErrors,
formData: _formData,
trigger,
}: Props) {
const now = useMemo(() => new Date(), []);

const getErrorMessage = (field: string) => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we're not returning a string in L32, this function is not an error message

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can also add a return type annotation to make things clearer

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, sometimes it is more than one message (Array/object of error messages). So adding an s to the name and type should make it clear

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Typing the error turned out to be tricky as the errors are generated using superRefine which makes them almost dynamic. Changed its name and fixed one issue in it.

const error = get(fieldErrors, field);
if (Array.isArray(error)) {
return error.map((err) => err?.message);
} else if (isObjectLike(error) && !error.message) {
return error;
}
return error?.message;
};
Expand Down Expand Up @@ -221,20 +225,15 @@ export default function WorkflowActionStartForm({
<Controller
name="cronSchedule"
control={control}
defaultValue=""
render={({ field: { ref, ...field } }) => (
<Input
{...field}
// @ts-expect-error - inputRef expects ref object while ref is a callback. It should support both.
inputRef={ref}
aria-label="Cron Schedule (UTC)"
size="compact"
onChange={(e) => {
field.onChange(e.target.value);
render={({ field }) => (
<CronScheduleInput
value={field.value}
onChange={(value) => {
field.onChange(value);
trigger('cronSchedule');
}}
onBlur={field.onBlur}
error={Boolean(getErrorMessage('cronSchedule'))}
placeholder="* * * * *"
error={getErrorMessage('cronSchedule')}
/>
)}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export default function WorkflowActionsModalContent<
control,
watch,
clearErrors,
trigger,
} = useForm<OptionalFormData>({
resolver: action.modal.formSchema
? zodResolver(action.modal.formSchema)
Expand Down Expand Up @@ -122,6 +123,7 @@ export default function WorkflowActionsModalContent<
fieldErrors={validationErrors}
clearErrors={clearErrors}
control={control}
trigger={trigger}
cluster={params.cluster}
domain={params.domain}
workflowId={params.workflowId}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,10 @@ export const overrides = {
right: $theme.sizing.scale800,
}),
},
Dialog: {
style: (): StyleObject => ({
width: '600px',
}),
},
} satisfies ModalOverrides,
};
2 changes: 2 additions & 0 deletions src/views/workflow-actions/workflow-actions.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
type Control,
type FieldErrors,
type FieldValues,
type UseFormTrigger,
} from 'react-hook-form';
import { type z } from 'zod';

Expand Down Expand Up @@ -33,6 +34,7 @@ export type WorkflowActionFormProps<FormData extends FieldValues> = {
fieldErrors: FieldErrors<FormData>;
control: Control<FormData>;
clearErrors: UseFormClearErrors<FormData>;
trigger: UseFormTrigger<FormData>;
cluster: string;
domain: string;
workflowId: string;
Expand Down