-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Frontend + Backend CRON validation #13005
Conversation
…is not explained if it is hit - still need some frontend for this, but this now means that an error is provided to users when attempting to publish, and we can re-use this validation in the automation UI. Need to have both backend and frontend validation as invalid CRONs will already exist, backend makes sure these are error'd on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - nice enhancement 😄
…e into fix/add-cron-validation
…tion' into fix/add-cron-validation
function improveErrors(errors: string[]): string[] { | ||
const finalErrors: string[] = [] | ||
|
||
for (let error of errors) { | ||
if (error.includes(INPUT_CRON_START)) { | ||
error = error.split(INPUT_CRON_START)[0].trim() | ||
} | ||
for (let [oldErr, newErr] of Object.entries(ERROR_SWAPS)) { | ||
if (error.includes(oldErr)) { | ||
error = error.replace(new RegExp(oldErr, "g"), newErr) | ||
} | ||
} | ||
finalErrors.push(error) | ||
} | ||
return finalErrors | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eep. This concerns me. We're very sensitive to the cron-validate
library changing their wording slightly and breaking our error messages. Makes me wonder if cron validation is actually complex enough to warrant using a library and munging its error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we lock the library version then this shouldn't be too much of a problem - I doubt we will need to update this often - the error messages themselves are quite hard to read so just wanted to improve them ever so slightly.
Right now the way this works is it will spit out the raw errors anyway if they were to be updated for some reason, so it'll just look a bit less clear. The alternative is to just have the raw errors from the start, but I see no reason to not at least attempt to improve them/make them more human-readable.
EDIT: I've pinned the package version now - so should avoid issues with error messages being updated.
packages/builder/src/components/automation/SetupPanel/CronBuilder.svelte
Outdated
Show resolved
Hide resolved
✅ the frontend side of it! |
packages/builder/src/components/automation/SetupPanel/CronBuilder.svelte
Show resolved
Hide resolved
…tion' into fix/add-cron-validation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend LGTM!
Description
Adding some basic CRON validation to publishing, currently the error is not explained if it is hit - still need some frontend for this, but this now means that an error is provided to users when attempting to publish, and we can re-use this validation in the automation UI. Need to have both backend and frontend validation as invalid CRONs will already exist, backend makes sure these are error'd on.
Partially addresses: https://linear.app/budibase/issue/BUDI-7928/cron-expressions-should-be-validated
Error message:
Frontend: