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

feat: Improve Cron schedule support #1395

Merged
merged 2 commits into from
Apr 6, 2024

Conversation

baumandm
Copy link
Contributor

Improved support for task scheduling via cron expression by integrating it directly into the RecurrenceEditor.

Aside from simplifying the code and UI, there are a number of functional benefits:

  • A description of the cron is shown thanks to cRonstrue
  • You can mostly build a cron in one of the other modes, then switch into cron mode to continue editing what you had selected
  • If a DataDoc schedule is manually updated to a non-supported cron expression, the DataDoc schedule modal continues to work as expected (previously it crashes)
  • By default, cron schedules are still not allowed on DataDocs, but enabling it is only a one-line change

Screenshot 2024-01-12 at 2 41 18 PM

Copy link
Collaborator

@jczhong84 jczhong84 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! It really looks more easy and clean.

One issue when I tried:
When it is in the weekly, monthly and yearly tab, it can't be switched to the cron tab. Got below exception

cron.ts:97 Uncaught TypeError: Cannot read properties of undefined (reading 'join')
  at recurrenceToCron (cron.ts:97:41)

}) => {
const description = useMemo(() => {
try {
if (cron) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: when it is (!cron), it will also raise exception. can just leave it to the try/catch to handle

import cronstrue from 'cronstrue';
import React, { useMemo } from 'react';

export const DescribeCron: React.FunctionComponent<{ cron?: string }> = ({
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't feel necessary to have this as a separate component. could be just a useMemo inside the RecurrenceEditor component.

Comment on lines 89 to 98
<>
<input
{...field}
className="editor-input"
placeholder="* * * * *"
/>
<div className="editor-text mt12">
<DescribeCron cron={recurrence.cron} />
</div>
</>
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could be a separate component to me instead of DescribeCron

Comment on lines +162 to +167
/**
* Determines whether a cron string is supported by the recurrence editor.
*
* @param cron Cron string to validate
* @returns true if cron string is supported by recurrence editor, false otherwise
*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can also use cron-parser for validation, which is mentioned by package cronstrue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The backend has cron validation that prevents saving invalid crons, so I don't think we would need this here. This function is only used to determine if the RecurrenceEditor should use cron mode or one of the simple modes.

@baumandm
Copy link
Contributor Author

baumandm commented Mar 4, 2024

One issue when I tried:
When it is in the weekly, monthly and yearly tab, it can't be switched to the cron tab. Got below exception

Previously, you had to fill out the required fields in the weekly/monthly/yearly tabs before you could switch to cron, but I agree this doesn't make a lot of sense. Now, if there is any issue converting the recurrence editor selections to cron, it just resets back to ""

@jczhong84 jczhong84 merged commit 26007d6 into pinterest:master Apr 6, 2024
3 checks passed
@baumandm baumandm deleted the external/cron-schedule branch April 16, 2024 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants