Add cron-expression scheduling (RunMode.CRON) - #5
Open
cmcau wants to merge 1 commit into
Open
Conversation
Adds a "Cron expression" run mode so a script can be driven by a standard 5-field cron string (e.g. "0 9 * * 1-5" for weekday mornings), alongside the existing manual/interval/daily/weekly/monthly modes. The engine change is deliberately small: the daily/weekly/monthly modes already compile down to a django-q2 Schedule with schedule_type=CRON, so cron mode just passes the user's expression straight through instead of generating one. - Model: new RunMode.CRON + cron_expression field (migration 0039, additive). - Validation: 5-field expressions only; rejects "@daily"-style shortcuts and anything croniter can't parse. croniter is already a dependency. - UI: the schedule form validates as you type and previews the next three run times before you save, with a link out to crontab.guru. - Plugin SDK: ScheduleAPI.sync() accepts a cron= kwarg. - Backup/restore and the dashboard's upcoming-runs list cover cron schedules. - Docs: docs/scheduling.md documents all six run modes, cron syntax, and the server-timezone behaviour. A .gitignore exception keeps it tracked, matching the existing docs/plugins.md carve-out. - Tests: 17 tests in core/test_cron_scheduling.py. Two pre-existing gaps surfaced while working in this area and are fixed here, since cron would otherwise inherit both: - The dashboard's upcoming-runs query only matched INTERVAL and DAILY, so weekly and monthly schedules never appeared. - Backup export/import dropped the weekly/monthly config fields. Note: as with the existing modes, the schedule's timezone field remains a display label - generated cron schedules run in the cluster's timezone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesCron scheduling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ScriptForm
participant cron_preview_view
participant ScheduleService
User->>ScriptForm: Enter cron expression
ScriptForm->>cron_preview_view: Request preview
cron_preview_view->>ScheduleService: Validate and calculate next runs
ScheduleService-->>cron_preview_view: Validation result and run times
cron_preview_view-->>ScriptForm: Return JSON preview
ScriptForm-->>User: Display status and upcoming runs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/views/scripts.py (1)
224-247: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude weekly and monthly configuration fields in schedule history.
The
previous_configandnew_configdictionaries omit the weekly and monthly schedule fields. Althoughcron_expressionwas successfully added, updates to weekly or monthly schedules will still silently skip being tracked inScheduleHistoryif these fields remain absent.🐛 Proposed fix
# Capture previous config for history previous_config = { "run_mode": schedule.run_mode, "interval_minutes": schedule.interval_minutes, "daily_times": schedule.daily_times, + "weekly_days": schedule.weekly_days, + "weekly_times": schedule.weekly_times, + "monthly_days": schedule.monthly_days, + "monthly_times": schedule.monthly_times, "cron_expression": schedule.cron_expression, "timezone": schedule.timezone, "is_active": schedule.is_active, } script = form.save(commit=False) script.save() form.save_m2m() if script.injection_mode == Script.InjectionMode.SELECTED: _reconcile_grants(script, request.POST.getlist("granted_secret_ids"), request.workspace) schedule = schedule_form.save() # Capture new config new_config = { "run_mode": schedule.run_mode, "interval_minutes": schedule.interval_minutes, "daily_times": schedule.daily_times, + "weekly_days": schedule.weekly_days, + "weekly_times": schedule.weekly_times, + "monthly_days": schedule.monthly_days, + "monthly_times": schedule.monthly_times, "cron_expression": schedule.cron_expression, "timezone": schedule.timezone, "is_active": schedule.is_active, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/views/scripts.py` around lines 224 - 247, Update the previous_config and new_config dictionaries in the schedule history flow to include the schedule’s weekly and monthly configuration fields, alongside the existing run_mode, interval, daily, cron, timezone, and active fields. Use the exact weekly/monthly attributes exposed by schedule and preserve identical keys in both dictionaries so changes are tracked by ScheduleHistory.
🧹 Nitpick comments (1)
core/test_cron_scheduling.py (1)
42-45: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert the unpacked
errvariable.The unpacked variable
erris never used. Consider asserting that it contains an error message to ensure validation fails for the right reason.✨ Proposed fix
def test_garbage_rejected(self): ok, err = ScheduleService.validate_cron_expression("99 99 * * *") self.assertFalse(ok) + self.assertIsNotNone(err)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/test_cron_scheduling.py` around lines 42 - 45, Update test_garbage_rejected to assert that the unpacked err value contains a validation error message, while preserving the existing assertion that ok is false.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/services/schedule_service.py`:
- Around line 361-375: Update the CRON branch in
core/services/schedule_service.py at lines 361-375 to pass
timezone.localtime(now) to croniter while preserving the existing error
handling. Update the expectation in core/test_cron_scheduling.py at lines 99-108
to compute the expected result using timezone.localtime(timezone.now()), so the
test matches local-time evaluation.
In `@docs/scheduling.md`:
- Around line 42-50: Update the fenced ASCII diagram in the scheduling
documentation to specify the text language identifier, changing the opening
fence to use text while preserving the diagram content unchanged.
In `@README.md`:
- Line 13: Update the “Flexible Scheduling” bullet in README.md to describe cron
support as standard five-field expressions, avoiding the inaccurate claim that
any cron expression is accepted. Keep the existing scheduling options and link
unchanged, and do not imply support for `@daily` shortcuts or six-field
expressions.
In `@static/js/script_form.js`:
- Around line 47-68: Prevent stale live-preview responses from updating the DOM
in the fetch flow: capture the requested expression when starting the request,
and before modifying e.preview, e.status, or e.runs, confirm the current input
value still matches it. Leave newer-request behavior unchanged and ignore
responses for outdated expressions.
---
Outside diff comments:
In `@core/views/scripts.py`:
- Around line 224-247: Update the previous_config and new_config dictionaries in
the schedule history flow to include the schedule’s weekly and monthly
configuration fields, alongside the existing run_mode, interval, daily, cron,
timezone, and active fields. Use the exact weekly/monthly attributes exposed by
schedule and preserve identical keys in both dictionaries so changes are tracked
by ScheduleHistory.
---
Nitpick comments:
In `@core/test_cron_scheduling.py`:
- Around line 42-45: Update test_garbage_rejected to assert that the unpacked
err value contains a validation error message, while preserving the existing
assertion that ok is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2dccf2b-32fb-4f62-a9bd-84e4c3bd8a7f
📒 Files selected for processing (15)
.gitignoreREADME.mdcore/forms.pycore/migrations/0039_scriptschedule_cron.pycore/models/schedule.pycore/plugins/api.pycore/services/backup_service.pycore/services/dashboard_service.pycore/services/schedule_service.pycore/test_cron_scheduling.pycore/urls/cpanel.pycore/views/scripts.pydocs/scheduling.mdstatic/js/script_form.jstemplates/cpanel/scripts/_form_sidebar.html
Comment on lines
+361
to
375
| elif script_schedule.run_mode == ScriptSchedule.RunMode.CRON: | ||
| cron_expr = (script_schedule.cron_expression or "").strip() | ||
| if not cron_expr: | ||
| return None | ||
| try: | ||
| from croniter import croniter | ||
|
|
||
| return croniter(cron_expr, now).get_next(datetime) | ||
| except (ValueError, KeyError) as exc: | ||
| logger.warning( | ||
| f"Could not compute next run for cron '{cron_expr}': {exc}" | ||
| ) | ||
| return None | ||
|
|
||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use local time for CRON next-run computations.
django-q2 evaluates cron expressions in the local timezone, and preview_cron_runs correctly aligns with this by using timezone.localtime(timezone.now()). However, the next-run calculation here directly passes now (which evaluates to UTC) to croniter. This mismatch will cause the next_run stored in the database and shown on the dashboard to differ dramatically from the actual execution time.
core/services/schedule_service.py#L361-L375: Update the base datetime so that cron logic evaluates against the cluster's local time:return croniter(cron_expr, timezone.localtime(now)).get_next(datetime)core/test_cron_scheduling.py#L99-L108: Update the test expectation to mirror the local timezone behavior:expected = croniter("30 4 * * *", timezone.localtime(timezone.now())).get_next(datetime)
📍 Affects 2 files
core/services/schedule_service.py#L361-L375(this comment)core/test_cron_scheduling.py#L99-L108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/services/schedule_service.py` around lines 361 - 375, Update the CRON
branch in core/services/schedule_service.py at lines 361-375 to pass
timezone.localtime(now) to croniter while preserving the existing error
handling. Update the expectation in core/test_cron_scheduling.py at lines 99-108
to compute the expected result using timezone.localtime(timezone.now()), so the
test matches local-time evaluation.
Comment on lines
+42
to
+50
| ``` | ||
| ┌───────────── minute (0-59) | ||
| │ ┌───────────── hour (0-23) | ||
| │ │ ┌───────────── day of month (1-31) | ||
| │ │ │ ┌───────────── month (1-12) | ||
| │ │ │ │ ┌───────────── day of week (0-6, 0 = Sunday) | ||
| │ │ │ │ │ | ||
| * * * * * | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced diagram.
markdownlint reports MD040 for this unlabeled code fence. Use text for the ASCII diagram.
Proposed fix
-```
+```text📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| ``` | |
| ┌───────────── minute (0-59) | |
| │ ┌───────────── hour (0-23) | |
| │ │ ┌───────────── day of month (1-31) | |
| │ │ │ ┌───────────── month (1-12) | |
| │ │ │ │ ┌───────────── day of week (0-6, 0 = Sunday) | |
| │ │ │ │ │ | |
| * * * * * | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 42-42: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/scheduling.md` around lines 42 - 50, Update the fenced ASCII diagram in
the scheduling documentation to specify the text language identifier, changing
the opening fence to use text while preserving the diagram content unchanged.
Source: Linters/SAST tools
|
|
||
| - **Script Management** — Create, edit, and organize Python scripts from your browser | ||
| - **Flexible Scheduling** — Run scripts manually, at intervals, or daily at specific times | ||
| - **Flexible Scheduling** — Run scripts manually, at intervals, daily/weekly/monthly at specific times, or on any [cron expression](docs/scheduling.md) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe the supported cron syntax precisely.
The implementation accepts standard five-field expressions, not “any cron expression”; @daily shortcuts and six-field expressions are explicitly rejected in docs/scheduling.md.
Proposed wording
-- **Flexible Scheduling** — Run scripts manually, at intervals, daily/weekly/monthly at specific times, or on any [cron expression](docs/scheduling.md)
+- **Flexible Scheduling** — Run scripts manually, at intervals, daily/weekly/monthly at specific times, or with any standard 5-field [cron expression](docs/scheduling.md)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Flexible Scheduling** — Run scripts manually, at intervals, daily/weekly/monthly at specific times, or on any [cron expression](docs/scheduling.md) | |
| - **Flexible Scheduling** — Run scripts manually, at intervals, daily/weekly/monthly at specific times, or with any standard 5-field [cron expression](docs/scheduling.md) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 13, Update the “Flexible Scheduling” bullet in README.md
to describe cron support as standard five-field expressions, avoiding the
inaccurate claim that any cron expression is accepted. Keep the existing
scheduling options and link unchanged, and do not imply support for `@daily`
shortcuts or six-field expressions.
Comment on lines
+47
to
+68
| fetch(url, { headers: { 'X-Requested-With': 'XMLHttpRequest' } }) | ||
| .then(function (r) { return r.json(); }) | ||
| .then(function (data) { | ||
| e.preview.classList.remove('hidden'); | ||
| if (!data.valid) { | ||
| e.status.textContent = data.error || 'Invalid cron expression.'; | ||
| e.status.className = 'font-medium text-fail'; | ||
| e.runs.innerHTML = ''; | ||
| return; | ||
| } | ||
| e.status.textContent = 'Next runs (server time):'; | ||
| e.status.className = 'font-medium text-ok'; | ||
| e.runs.innerHTML = ''; | ||
| (data.runs || []).forEach(function (run) { | ||
| var li = document.createElement('li'); | ||
| li.textContent = run; | ||
| e.runs.appendChild(li); | ||
| }); | ||
| }) | ||
| .catch(function () { | ||
| e.preview.classList.add('hidden'); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prevent race conditions in the live preview.
If a user types quickly, an older, slower network response could resolve after a newer, faster request completes. This would cause the preview UI to display results for an outdated expression.
Check if the current input value still matches the requested expression before updating the DOM, or use an AbortController to cancel stale requests.
🐛 Proposed fix (validation check)
.then(function (r) { return r.json(); })
.then(function (data) {
+ if (e.input.value.trim() !== expr) return;
+
e.preview.classList.remove('hidden');
if (!data.valid) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fetch(url, { headers: { 'X-Requested-With': 'XMLHttpRequest' } }) | |
| .then(function (r) { return r.json(); }) | |
| .then(function (data) { | |
| e.preview.classList.remove('hidden'); | |
| if (!data.valid) { | |
| e.status.textContent = data.error || 'Invalid cron expression.'; | |
| e.status.className = 'font-medium text-fail'; | |
| e.runs.innerHTML = ''; | |
| return; | |
| } | |
| e.status.textContent = 'Next runs (server time):'; | |
| e.status.className = 'font-medium text-ok'; | |
| e.runs.innerHTML = ''; | |
| (data.runs || []).forEach(function (run) { | |
| var li = document.createElement('li'); | |
| li.textContent = run; | |
| e.runs.appendChild(li); | |
| }); | |
| }) | |
| .catch(function () { | |
| e.preview.classList.add('hidden'); | |
| }); | |
| fetch(url, { headers: { 'X-Requested-With': 'XMLHttpRequest' } }) | |
| .then(function (r) { return r.json(); }) | |
| .then(function (data) { | |
| if (e.input.value.trim() !== expr) return; | |
| e.preview.classList.remove('hidden'); | |
| if (!data.valid) { | |
| e.status.textContent = data.error || 'Invalid cron expression.'; | |
| e.status.className = 'font-medium text-fail'; | |
| e.runs.innerHTML = ''; | |
| return; | |
| } | |
| e.status.textContent = 'Next runs (server time):'; | |
| e.status.className = 'font-medium text-ok'; | |
| e.runs.innerHTML = ''; | |
| (data.runs || []).forEach(function (run) { | |
| var li = document.createElement('li'); | |
| li.textContent = run; | |
| e.runs.appendChild(li); | |
| }); | |
| }) | |
| .catch(function () { | |
| e.preview.classList.add('hidden'); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@static/js/script_form.js` around lines 47 - 68, Prevent stale live-preview
responses from updating the DOM in the fetch flow: capture the requested
expression when starting the request, and before modifying e.preview, e.status,
or e.runs, confirm the current input value still matches it. Leave newer-request
behavior unchanged and ignore responses for outdated expressions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this adds
A "Cron expression" run mode, so a script can be scheduled from a standard 5-field cron string (e.g.
0 9 * * 1-5for weekday mornings) instead of only the preset manual/interval/daily/weekly/monthly modes.This came out of my own self-hosted use: the presets cover most cases, but "weekdays at 9" or "every 15 minutes during business hours" aren't expressible, and cron already is the lingua franca for that.
Why the change is small
The existing daily/weekly/monthly modes already compile down to a django-q2
Schedulewithschedule_type=CRON— they generate a cron string internally. So cron mode isn't a new scheduling engine, it just passes the user's expression through instead of generating one.croniteris already a dependency (django-q2 uses it for the CRON type), so there's no new package.What's included
RunMode.CRON+ acron_expressionfield. Migration0039is purely additive (AddField+ anAlterFieldfor the choices list); no data migration, no backfill, safe on existing installs.@daily-style shortcuts and anythingcronitercan't parse, with a specific error message per failure.ScheduleAPI.sync()takes acron=kwarg.docs/scheduling.mdcovers all six run modes, cron syntax, and timezone behaviour. It needed a.gitignoreexception to stay tracked, following the existing!docs/plugins.mdcarve-out.core/test_cron_scheduling.py.Two pre-existing bugs fixed along the way
Both sit directly in the code cron touches, and cron would have inherited them. Happy to split these into a separate PR if you'd prefer:
INTERVALandDAILY, so weekly and monthly schedules never showed up there.weekly_days,weekly_times,monthly_days,monthly_times), so those schedules didn't round-trip through a restore.Notes
timezonefield stays a display label — generated cron schedules run in the cluster's timezone. I documented the current behaviour rather than changing it, since fixing it properly affects every mode and felt like a separate discussion.Testing
makemigrations --checkreports no drift,manage.py checkis clean, and the cron (17) plus plugin-SDK/backup (33) suites pass on Python 3.13 / Django 6.Summary by CodeRabbit
New Features
Documentation