Repository navigation
issue#117: per-day/month schedule selection and dated backup subdirectories - #198
Conversation
f0c0640 to
aa3c893
Compare
aa3c893 to
9b9f3a8
Compare
somethingwithproof
left a comment
somethingwithproof
left a comment
There was a problem hiding this comment.
Blocker: bulk edit bypasses the new schedule validation. plugin_routerconfigs_schedule_validate() runs only in save_devices(). The bulk edit action offers every entry in plugin_routerconfigs_sched_types(), which now includes Monthly on Day, but never sets month, monthly_week or monthly_day.
- A device bulk-set to Monthly on Day has nothing to resolve, so both year passes return false and
nextbackupbecomes 0. The device is then backed up on every daily scheduled run. - A device bulk-set to Monthly falls into the day-of-month fallback discussed on #197.
Either validate bulk changes against each device's resulting fields, or leave the two Monthly types out of the bulk Schedule Type list.
Suggestion: a 31st selected for every month adds stray runs. "April 31" becomes May 1 and "February 31" becomes March 3. Core behaves the same way, but here it adds backups on days nobody picked. Skipping dates where checkdate() fails, and pointing people to "Last", would avoid it.
Nit: when validation fails, the redirect back to the edit page throws away everything entered. For a new device (id=0) that is the whole form.
…le on change Monthly without a day of month used the day of the current run, so a Jan 31 run asked for "February 31", which strtotime() turned into March 3, and the device drifted from then on. A Monthly schedule saved, or bulk-set, without a day now stores the day it was set up on (Last, 32, after the 28th so short months still run), and the calculator falls back to the 1st rather than the run day. Days a month does not have (February 30, April 31) are skipped, as RFC 5545 section 3.3.10 requires, instead of rolling into the next month. The replica is now used on every Cacti version. Core's api_scheduler_calculate_next_start() rolls nonexistent dates over, so delegating to it on 1.3+ would give the same device different backup dates on 1.2 and 1.3. Bulk edit validated nothing: sched_type, account and devicetype were cast to int, so an unknown sched_type resolved to 0 and the device ran on every scheduled poll. plugin_routerconfigs_bulk_edit_validate() now accepts only the offered Schedule Types, Rerun Every 1-31, a known Connection Type, and existing accounts and device types; anything else raises an error and nothing is changed. save_devices() checks Schedule Type and Rerun Every the same way. Changing the schedule now recomputes nextbackup on the device form and in bulk edit, so a device moved from Daily to Monthly no longer runs once more on the old schedule. Manual stores 0, which the download query excludes. A device that has never been scheduled keeps NULL so its first backup still runs next. Bulk edit reads the selected rows once and groups devices with identical results into one UPDATE ... WHERE id IN (...). Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…sue-117-schedule-days-datedir Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Now that nonexistent dates are skipped, a Monthly schedule on February 29 alone has no date in 2026 or 2027. The calculator tried only this year and next, so it returned 0 and the device became due on every scheduled run. It now searches 8 years, the longest gap between leap years (2096 to 2104), and still returns false for a day that never exists, such as February 30. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…sue-117-schedule-days-datedir Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…p missing days Bulk edit offered every Schedule Type, including Monthly on Day, but never sets Months, Weeks of Month or Days of Week. A device bulk-set to Monthly on Day had nothing to resolve, so nextbackup became 0 and it was backed up on every scheduled run. Monthly on Day is now left out of the bulk list, a bulk Monthly gets every month when none is stored (and a fixed day of month, as on the device form), and each device's resulting row must pass plugin_routerconfigs_schedule_validate(). If any device fails, nothing is changed and the failing device ids are reported. plugin_routerconfigs_schedule_validate() also rejects a monthly selection that never occurs (only February 30 and 31, say), which would otherwise resolve to 0. Days a month lacks are skipped rather than rolled over, so a 31st selected for every month no longer adds May 1, July 1, October 1 and December 1 runs; tests cover that through the multi-select save path. Weekly with Days of Week selected runs on those days every week and ignored Rerun Every, while the field said "every X weeks". Rerun Every is now hidden once Days of Week are selected, and both descriptions say how they combine. When validation fails the redirect still returns to a fresh edit form. The plugin does not use Cacti's form_input_validate() field retention anywhere, so keeping the entered values would mean adopting it for the whole form; that is left for a separate change. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
left a comment
somethingwithproof
left a comment
There was a problem hiding this comment.
Rechecked against the previous review.
- Bulk edit vs. validation: fixed. Monthly on Day is no longer offered in bulk edit, and
bulk_edit_apply()validates every resulting row and writes nothing if any device would have no resolvable next run. - 31st selected for every month: fixed. Missing dates are skipped, and a selection that never occurs is rejected on save.
- Redirect throws away the form (nit): still open at
router-devices.php:523and:540. A failed validation on a new device loses everything entered. Not blocking.
Suggestion: includes/scheduler.php:601: plugin_routerconfigs_schedule_augment_save() only applies intval to the multi-selects. Out-of-range values are remapped by the calculator instead of rejected: month 13 becomes January, week 99 becomes "first", weekday 99 becomes Sunday, and a weekly day 99 resolves to a different weekday through % 7. schedule_validate() doesn't catch these because the result is still schedulable. Filter each field against the option arrays (sched_months(), sched_days_of_week(), etc.) before saving.
Suggestion: includes/functions.php:784: with the dated subdirectory on, each run creates a new directory, but plugin_routerconfigs_retention() only unlink()s files. Emptied date directories pile up. Remove empty parents after the unlink, or say so in the setting description.
No blockers. Only SonarCloud runs on this stacked PR (it passes); the PHP integration jobs run on #197.
|
Pushed 57f9377 for the review, after merging #197 (8aa5c54, d244c70):
Tests: 206 passed locally. The 31st-for-every-month test got 03-03, 05-01 and 07-01 runs on the previous head, and Feb 30 validated there. SonarCloud passes on 57f9377. To revisit:
|
The merge-base changed after approval.
There was a problem hiding this comment.
🟡 Changes recommended
The database column cannot hold all offered day selections, causing valid schedules to fail to save or silently lose selected days.
3 open issues
What changed in this PR
This PR builds on the #133 scheduler to address #117, adding day/month selection for router backups and optional date-based archive folders.
Changes:
- Adds weekly and monthly multi-selects, including Monthly on Day, with schedule-dependent form visibility.
- Adds schedule validation and rejects bulk changes when any resulting schedule cannot run.
- Adds dated archive subdirectories, defaulting to Off, with tests and translation updates.
| File | Description |
|---|---|
tests/Unit/RouterconfigsSchedulerTest.php |
Tests selectors, schedule validation, calendar dates, and dated-path formatting. |
tests/Unit/RouterconfigsBulkEditTest.php |
Tests bulk schedule defaults and rejection of invalid schedules. |
router-devices.php |
Wires schedule inputs, row visibility, validation, and bulk feedback. |
locales/po/cacti.pot |
Updates translatable labels, descriptions, and validation messages. |
includes/scheduler.php |
Adds selector options, date formatting, and schedule validation helpers. |
includes/functions.php |
Inserts dated archive paths and validates bulk schedule changes. |
includes/arrays.php |
Defines schedule selectors and the Dated Subdirectory setting. |
CHANGELOG.md |
Documents scheduling and dated-directory features. |
Review effort: Balanced
|
Resolve conflicts after #198 (issue#117 + issue#133) landed on develop: - scheduler.php, RouterconfigsSchedulerTest.php: take develop's version; it is a superset of this branch's issue#117-era copy (adds the DST-aware strtotime Daily/Weekly math, plugin_routerconfigs_schedule_fields_valid() and its test). issue#130 does not touch scheduling. - router-devices.php: keep develop's save_devices() flow that validates the multi-select schedule inputs (schedule_fields_valid) before saving; the issue#130 post-login-prompt handling merged cleanly alongside it. - database.php: keep develop's day_of_month varchar(128) (a full Days of Month selection is an 86-char CSV); the issue#130 device-type columns merged cleanly. - INFO: keep this branch's version bump to 1.11. - CHANGELOG: union - issue#130 entry plus develop's issue#117/#133 entries (including the DST line). - cacti.pot: regenerated from the merged source (issue#130 + issue#117 strings); per-language .po files taken from develop unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Description
Second PR in the stacked chain (#133 → #117 → #130 → #109), built on top of the #133 scheduler branch. It completes issue #117 by exposing the finer-grained schedule selection that the #133 scheduler columns already support, and adds the requested dynamic date-based backup directories.
Per-day / per-month schedule selection — the device form now shows Automation Networks style multi-selects that appear only for the relevant Schedule Type:
The selections are stored in the
day_of_week/month/day_of_month/monthly_week/monthly_daycolumns introduced in New Features: TFTP ip, Schedule, Router edit and add features. #133 and consumed by the plugin-local scheduler. Saving validates that Monthly and Monthly-on-Day schedules have the fields they need, rejecting an unschedulable combination before it is stored. A small CSP-safe script shows/hides the rows as the Schedule Type changes.Dated backup subdirectory — a new global Dated Subdirectory setting (
Off/YYYY-MM-DD/YYYY/MM/DD/DD-MM-YYYY) inserts a date-based folder into the archive path before the per-device folder, grouping each run under its backup date (e.g..../2020-11-04/hostname/...). It defaults to Off, preserving the existing layout.Related Issue
#117
Motivation and Context
Operators asked to control which day a weekly/monthly backup runs (rather than a fixed cadence from an arbitrary anchor) and to organise backups into dated folders. The scheduler model landed in #133 already carries the necessary fields, so this PR is primarily the UI, validation, and archive-path wiring on top of it.
How Has This Been Tested?
Ran the plugin's Pest suite from a Cacti checkout with the plugin as a real
plugins/routerconfigsdirectory (the stacked diff covers #133 + #117):pest --configuration phpunit.xml→ 173 passed (1108 assertions). Extendedtests/Unit/RouterconfigsSchedulerTest.phpwith coverage for the day/month option lists, the dated-subdirectory format helper (each format + the off/invalid guards + default-to-now), the multi-select save folding, and the Monthly / Monthly-on-Day validation rules.php tests/bin/patch-coverage.php coverage/clover.xml origin/develop 100→ 100.00% (196/196 lines).php tests/bin/check-i18n-pot.php origin/develop→ OK (locales/po/cacti.potregenerated; per-language.po/.moleft to Weblate).php -lclean on every changed file.php tests/bin/validate-manifest.php→ matches.The date-directory insertion lives inside
plugin_routerconfigs_download_config(), which is within the file's existing@codeCoverageIgnoreregion (it needs a live SSH/Telnet transfer); the pure helper that builds the dated path component is unit-tested.Types of changes
Checklist: