Repository navigation
fix(plugin-forms): give the digest cron task a valid name - #3979
Conversation
🦋 Changeset detectedLatest commit: 094581c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@emdash-cms/admin
@emdash-cms/auth
@emdash-cms/auth-atproto
@emdash-cms/blocks
@emdash-cms/cloudflare
@emdash-cms/contentful-to-portable-text
emdash
create-emdash
@emdash-cms/gutenberg-to-portable-text
@emdash-cms/plugin-cli
@emdash-cms/plugin-test
@emdash-cms/plugin-types
@emdash-cms/registry-client
@emdash-cms/registry-lexicons
@emdash-cms/registry-loader
@emdash-cms/registry-moderation
@emdash-cms/registry-verification
upgrade-emdash
@emdash-cms/sandbox-workerd
@emdash-cms/x402
@emdash-cms/plugin-ai-moderation
@emdash-cms/plugin-atproto
@emdash-cms/plugin-audit-log
@emdash-cms/plugin-color
@emdash-cms/plugin-embeds
@emdash-cms/plugin-field-kit
@emdash-cms/plugin-forms
@emdash-cms/plugin-webhook-notifier
commit: |
Overlapping PRsThis PR modifies files that are also changed by other open PRs: This may cause merge conflicts or duplicated work. A maintainer will coordinate. |
There was a problem hiding this comment.
This PR fixes the right bug in the right way. The digest:<formId> cron task name was always invalid under validateTaskName (letters, numbers, dashes, underscores only), so every schedule/cancel threw and forms saved with digest enabled had no actual task. Centralizing the name in src/digest-task.ts and switching to digest-<formId> eliminates that, and the new regression test exercises the create/update handlers against a cron stub that rejects the names the real cron rejects.
I verified:
- The new task name matches the allowed pattern and form IDs (ULIDs) contain only valid characters.
- All schedules, cancels, and the cron dispatcher route through
digestTaskName/formIdFromDigestTaskconsistently. - No migration is needed: an invalid
digest:...name could never be inserted becauseCronAccessImpl.schedulevalidates before writing. - Core cron scopes events by
plugin_id, so theelse { formIdFromDigestTask(...) }dispatch only ever sees this plugin's own tasks. - The changeset for
@emdash-cms/plugin-formsis technically accurate and user-facing.
The changed code is clean. One related gap remains: formsDuplicateHandler copies settings.digestEnabled but never schedules a digest for the new form, so duplicating a form with digest on leaves it in the same broken state this PR fixes for creates. That is pre-existing, but worth addressing while the digest scheduling logic is being centralized.
Findings
-
[suggestion]
packages/plugins/forms/src/handlers/forms.ts:221Duplicating a form copies the source's
digestEnabledsetting, butformsDuplicateHandlernever schedules a digest cron for the new form. A duplicate created from a form with digest on ends up withdigestEnabled: trueand no scheduled task—the same broken state this PR fixes for creates. Afterforms(ctx).put(id, duplicate), also schedule ifduplicate.settings.digestEnabledis true.await forms(ctx).put(id, duplicate); if (duplicate.settings.digestEnabled && ctx.cron) { await ctx.cron.schedule(digestTaskName(id), { schedule: `0 ${duplicate.settings.digestHour} * * *`, }); }
|
Taken the duplicate finding in 094581c: |
What does this PR do?
Makes the forms plugin's daily digest schedulable.
The plugin named its cron task
digest:<formId>.ctx.crononly accepts letters, numbers, dashes and underscores (validateTaskNameinpackages/core/src/plugins/cron.ts), so everyscheduleandcancelcall for a digest threw. Creating a form withdigestEnabled: true, or turning it on later, answered 500. On create the form had already been written, so it was left stored with the digest on and no task: it sent no per-submission email and no digest.The task is now named
digest-<formId>. The name is built and read back in one place,src/digest-task.ts, used by the create, update, duplicate and delete handlers, the digest handler and the cron dispatcher. Duplicating a form copied itsdigestEnabledsetting but never scheduled a task for the copy; it now does.No task named
digest:<formId>can exist in a database, because scheduling one always threw, so nothing needs migrating. A form that was already saved with the digest on by a failed create still has no task; turning its digest off and on again schedules it.Closes #3975
Type of change
Checklist
pnpm typecheckpassespnpm lintpassespnpm testpasses (or targeted tests for my change)pnpm formathas been runmessages.pochanges except in translation PRs — a workflow extracts catalogs on merge tomain.AI-generated code disclosure
Screenshots / test output
Not applicable (no UI change).
tests/digest-task.test.tsruns the create, update and duplicate handlers against a cron that refuses the names the real one refuses. Before the fix the create and update tests failed withInvalid task name "digest:01M4C0YKWPN6VTW8JY5PA9WGZV", and the duplicate test failed with no task scheduled; with it:Also run against the real cron on
demos/simple(SQLite) withformsPlugin()added, reading_emdash_cron_tasksafter each call: