From 36f2427e9a17d952a1f87ce20f2cbe5950125af0 Mon Sep 17 00:00:00 2001 From: Dillard Blom Date: Sat, 7 Nov 2026 00:23:28 +0100 Subject: [PATCH] Tell administrators about a location without a schedule A notification when a schedule is turned off, then a reminder: once a week while another location is backed up, every day when none is. The round trip now also checks the text of these notifications and of the one about names, and that one location never gets two runs. --- CHANGELOG.md | 2 ++ PLAN.md | 12 +++++----- lib/BackgroundJob/BackupJob.php | 5 +++- lib/Notification/Notifier.php | 9 ++++++- lib/Service/AlertService.php | 24 +++++++++++++++++++ lib/Service/TargetService.php | 11 +++++++-- .../Unit/Service/ScheduleOffReminderTest.php | 22 +++++++++++++++++ tests/integration/roundtrip.sh | 24 +++++++++++++++++-- 8 files changed, 97 insertions(+), 12 deletions(-) create mode 100644 tests/Unit/Service/ScheduleOffReminderTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 024943c..d69f736 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,8 @@ shown under 1.0.0. The git tag of a release candidate goes after the date. point has the time its backup really began, not the time it was queued. - Choosing a time no longer starts a backup right away when that time has already passed today. - A location without a schedule no longer gets the notification that it has had no recent backup. + Instead, all administrators are told when a schedule is turned off, and reminded once a week while + another location is still backed up, or every day when none is. - A file or folder that is left out because of its name now gets a notification of its own, in the log and in `occ backup:run`. It says the item is missing from the backup, gives example paths and asks to rename it; before, it was reported as unreadable, with advice about permissions. diff --git a/PLAN.md b/PLAN.md index 9823465..175d94c 100644 --- a/PLAN.md +++ b/PLAN.md @@ -581,8 +581,9 @@ in README "Known limitations": is de keuze van de beheerder (besluit 2026-11-06). **Stand op branch `1.1.0` (2026-11-07):** klaar: betere melding, punten 4, 5, 6 en 8. Pin en notitie staan in de catalogus, met een kopie in de database voor de lijsten. Een moment van vóór het instellen van - een schema start niets. Geen schema is een keuze: dan ook geen melding "geen recente backup". Nog niet - getest: de update vanaf 1.0.3, de pagina in een browser, de tekst van de nieuwe melding in beeld. + een schema start niets. Geen schema: geen melding "geen recente backup", wel een melding aan alle + beheerders bij het uitzetten en een herinnering, wekelijks zolang een andere locatie een schema heeft, + anders dagelijks (besluit 2026-11-07). Nog niet getest: de update vanaf 1.0.3, de pagina in een browser. 1. `backup:key:import` zonder kit: de sleutel-slots van de locatie (`config`) lezen, met de kit als reserve als `config` ontbreekt of niet opent. Vergt een andere volgorde van `target:add` en sleutelimport (het gevoeligste pad), daarom niet in rc1. Daarna de @@ -614,11 +615,10 @@ in README "Known limitations": nooit opruimt (bijv. "vóór de upgrade naar NC 35"), in de lijst en met `occ backup:pin ` / `--unpin`. Op de locatie bewaren (catalogus), zodat de opschoning het ziet, ook na een DR; vastgepinde punten tellen niet mee in last/daily/weekly. - 9. **Feedbackvraag weghalen** in 1.1.0 of 1.2.0 (besluit 2026-10-27): de regel "We would like + 9. **Feedbackvraag weghalen in 1.2.0** (besluit 2026-11-07, niet in 1.1.0): de regel "We would like to hear how NG Backup works for you ... Share your experience" in de beheerpagina blijft tot - dan staan (de vermelding "in beta" is er in 1.0.1 al af). Zijn er tegen die - tijd geen noemenswaardige meldingen, dan werkt het: mensen melden eerder wat niet werkt dan - wat gewoon werkt. Het formulier (`experience.yml`) mag blijven voor wie het zelf opzoekt. + dan staan. Er komt weinig feedback bij zo'n 150 downloads per versie; geen bericht is goed + bericht. Het formulier (`experience.yml`) in de issues blijft. - **Bevindingen DR-test op PVE02 (2026-10-17, snapshot van nc01, 1 TB, AIO → gewone NC 34 met PostgreSQL 18)** — voor **1.0.0-rc.2**: 1. Tabellen van apps die bij de backup **niet aanstonden** (restanten: Deck, ONLYOFFICE, diff --git a/lib/BackgroundJob/BackupJob.php b/lib/BackgroundJob/BackupJob.php index 99fa12d..f91acf8 100644 --- a/lib/BackgroundJob/BackupJob.php +++ b/lib/BackgroundJob/BackupJob.php @@ -59,8 +59,11 @@ final class BackupJob extends TimedJob { } $this->failStaleRestores(); $this->weeklyPrune(); - foreach ($this->targets->list() as $t) { + $targets = $this->targets->list(); + foreach ($targets as $t) { $this->alerts->checkStale($t); + $othersActive = array_filter($targets, static fn ($o) => $o->getId() !== $t->getId() && (string)$o->getSchedule() !== '') !== []; + $this->alerts->checkOff($t, $othersActive); } } diff --git a/lib/Notification/Notifier.php b/lib/Notification/Notifier.php index 87a2f3b..f6e5d5d 100644 --- a/lib/Notification/Notifier.php +++ b/lib/Notification/Notifier.php @@ -61,13 +61,20 @@ final class Notifier implements INotifier { : $l->t('No successful backup to %1$s since %2$s', [$p['target'], $p['since']])); $notification->setParsedMessage($l->t('Check the backup settings and the server log.')); break; + case 'schedule_off': + $notification->setParsedSubject($l->t('Backup location %s has no schedule', [$p['target']])); + $notification->setParsedMessage(($p['others'] ?? '') === '1' + ? $l->t('No backups run to this location until it gets a schedule or someone starts one. If that is what you want, you can ignore this reminder.') + : $l->t('No location has a schedule, so no backups run at all unless someone starts one. Set a schedule under Backups.')); + break; case 'security_event': $who = (string)($p['actor'] ?? ''); $notification->setParsedSubject(match ($p['event'] ?? '') { 'kit_downloaded' => $l->t('%s downloaded the backup recovery kit', [$who]), 'target_removed' => $l->t('%1$s removed backup location %2$s', [$who, $p['target'] ?? '']), 'target_changed' => $l->t('%1$s changed the connection settings of backup location %2$s', [$who, $p['target'] ?? '']), - 'append_only_off' => $l->t('%1$s turned off append-only for backup location %2$s', [$who, $p['target'] ?? '']), + 'schedule_off' => $l->t('%1$s turned off the schedule of backup location %2$s', [$who, $p['target'] ?? '']), + 'append_only_off' =>$l->t('%1$s turned off append-only for backup location %2$s', [$who, $p['target'] ?? '']), 'retention_changed' => $l->t('%1$s changed the backup retention to %2$s', [$who, $p['policy'] ?? '']), 'snapshots_forgotten' => $l->t('%1$s moved %2$s snapshots of %3$s to the trash; they are deleted permanently after %4$s unless restored', [$who, $p['count'] ?? '', $p['target'] ?? '', $p['until'] ?? '']), 'snapshot_untrashed' => $l->t('%1$s restored snapshot %2$s of %3$s from the trash', [$who, $p['snapshot'] ?? '', $p['target'] ?? '']), diff --git a/lib/Service/AlertService.php b/lib/Service/AlertService.php index 5f53271..8bc249e 100644 --- a/lib/Service/AlertService.php +++ b/lib/Service/AlertService.php @@ -101,6 +101,30 @@ final class AlertService { return $v === '' ? null : (int)$v; } + /** + * Whether it is time to remind administrators of a location without a schedule: once a week + * while another location is still backed up, every day when none is. An hour of margin, so + * that the moment of the cron run does not push a reminder to the next day. + */ + public static function offReminderDue(int $last, int $now, bool $othersActive): bool { + return $now - $last >= ($othersActive ? 7 * 86400 : 86400) - 3600; + } + + public function checkOff(Target $target, bool $othersActive): void { + if ((string)$target->getSchedule() !== '') { + return; + } + $key = 'off_notified_' . $target->getId(); + $now = time(); + $last = max((int)$target->getScheduleSince(), $this->appConfig->getValueInt(Application::APP_ID, $key, 0)); + if (!self::offReminderDue($last, $now, $othersActive)) { + return; + } + $this->appConfig->setValueInt(Application::APP_ID, $key, $now); + $this->notifyAdmins('schedule_off', 'off-' . $target->getId() . '-' . date('Y-m-d', $now), + ['target' => $target->getName(), 'others' => $othersActive ? '1' : '0']); + } + /** At most one notification per location per day. */ public function checkStale(Target $target): void { if ((string)$target->getSchedule() === '') { diff --git a/lib/Service/TargetService.php b/lib/Service/TargetService.php index 80ddc18..84858d5 100644 --- a/lib/Service/TargetService.php +++ b/lib/Service/TargetService.php @@ -35,6 +35,7 @@ final class TargetService { private \OCA\NgBackup\Db\DbIndexCache $indexCache, private \OCA\NgBackup\Db\AppConfigCatalogAnchor $anchor, private SnapshotMapper $snapshots, + private AlertService $alerts, ) { } @@ -281,10 +282,16 @@ final class TargetService { if (!Schedule::validZone($zone)) { throw new \InvalidArgumentException('Unknown time zone: ' . $zone); } - $target->setSchedule(LocationSchedule::normalize($schedule)); + $new = LocationSchedule::normalize($schedule); + $turnedOff = $new === '' && (string)$target->getSchedule() !== ''; + $target->setSchedule($new); $target->setScheduleZone($zone); $target->setScheduleSince(time()); - return $this->mapper->update($target); + $target = $this->mapper->update($target); + if ($turnedOff) { + $this->alerts->securityEvent('schedule_off', ['target' => $target->getName()]); + } + return $target; } public function setMaxBytes(Target $target, ?int $maxBytes): Target { diff --git a/tests/Unit/Service/ScheduleOffReminderTest.php b/tests/Unit/Service/ScheduleOffReminderTest.php new file mode 100644 index 0000000..e74c477 --- /dev/null +++ b/tests/Unit/Service/ScheduleOffReminderTest.php @@ -0,0 +1,22 @@ +assertFalse(AlertService::offReminderDue(0, 6 * 86400, true)); + $this->assertTrue(AlertService::offReminderDue(0, 7 * 86400, true)); + } + + public function testEveryDayWhenNoLocationIsActive(): void { + $this->assertFalse(AlertService::offReminderDue(0, 3600, false)); + $this->assertTrue(AlertService::offReminderDue(0, 86400, false)); + $this->assertTrue(AlertService::offReminderDue(0, 86400 - 60, false), 'a cron run a minute earlier than yesterday'); + } +} diff --git a/tests/integration/roundtrip.sh b/tests/integration/roundtrip.sh index 9e0644d..20d40c4 100755 --- a/tests/integration/roundtrip.sh +++ b/tests/integration/roundtrip.sh @@ -108,6 +108,12 @@ for L in 2 3; do done IDS=$(api "$API/status" | php -r 'foreach (json_decode(stream_get_contents(STDIN), true)["targets"] as $t) { if ($t["name"] === "ci2" || $t["name"] === "ci3") { echo $t["id"], " "; } }') for ID in $IDS; do api -X POST -d "{\"targetId\": $ID, \"label\": \"turn\"}" "$API/backups" >/dev/null || fail "start a backup to location $ID"; done +# Never two runs to one location: a second start gets the run that is there. +ID=${IDS%% *} +RUNS=$(api "$API/status" | php -r 'echo count(array_filter(json_decode(stream_get_contents(STDIN), true)["runs"], fn ($r) => $r["status"] === "running"));') +api -X POST -d "{\"targetId\": $ID, \"label\": \"again\"}" "$API/backups" >/dev/null || fail "second start" +AGAIN=$(api "$API/status" | php -r 'echo count(array_filter(json_decode(stream_get_contents(STDIN), true)["runs"], fn ($r) => $r["status"] === "running"));') +[ "$RUNS" = 2 ] && [ "$AGAIN" = 2 ] || fail "a second start to one location: $RUNS, then $AGAIN running" occ backup:status | grep "queued" >/dev/null || fail "the second backup is not queued" QUEUED=$(api "$API/status" | php -r 'echo count(array_filter(json_decode(stream_get_contents(STDIN), true)["runs"], fn ($r) => $r["queued"]));') [ "$QUEUED" = 1 ] || fail "$QUEUED backups queued instead of 1" @@ -125,7 +131,6 @@ occ backup:status | grep "system cron" >/dev/null && fail "backup:status still w sleep 8 # APCu app config cache of the web server api "$API/status" | php -r 'exit(json_decode(stream_get_contents(STDIN), true)["cron"]["ok"] ? 0 : 1);' || fail "the status API still reports a missing cron after cron ran" ok "with a system cron that ran: no warning in backup:status or the API" -occ user:delete "$ADM" >/dev/null # A schedule per location. Choosing a time starts nothing; the moment itself does, and only there. JOB=$(occ background-job:list --class='OCA\NgBackup\BackgroundJob\BackupJob' --output=json | php -r 'echo json_decode(stream_get_contents(STDIN), true)[0]["id"];') @@ -144,9 +149,24 @@ occ backup:list ci2 | grep " scheduled " >/dev/null || fail "the scheduled momen job [ "$(occ backup:list ci2 | grep -c ' scheduled ')" = 1 ] || fail "one moment started more than one backup" occ backup:target:schedule ci2 nonsense >/dev/null 2>&1 && fail "a bad schedule was accepted" -for L in 2 3; do occ backup:target:remove ci$L >/dev/null || fail "target remove ci$L"; done ok "schedule per location: nothing on choosing a time, one backup at the moment, none when off" +# What administrators are told, as Nextcloud shows it. +notes() { api "http://localhost/ocs/v2.php/apps/notifications/api/v2/notifications?format=json"; } +occ backup:target:schedule ci2 off >/dev/null +$X sh -c "touch '$DATA/ngb-rt-bad\\name.txt'" +occ backup:run ci3 -l names | grep "their name is not supported" >/dev/null || fail "occ does not report the name" +$X sh -c "rm '$DATA/ngb-rt-bad\\name.txt'" +NOTES=$(notes) || fail "read the notifications" +echo "$NOTES" | grep "turned off the schedule of backup location ci2" >/dev/null || fail "no notification about the schedule that was turned off" +echo "$NOTES" | grep "is missing because of its name" >/dev/null || fail "no notification about the name" +echo "$NOTES" | grep "Rename them" >/dev/null || fail "the notification about the name does not ask to rename" +job +notes | grep "has no schedule" >/dev/null && fail "a reminder right after turning it off" +for L in 2 3; do occ backup:target:remove ci$L >/dev/null || fail "target remove ci$L"; done +occ user:delete "$ADM" >/dev/null +ok "notifications: schedule turned off, name not supported, no early reminder" + # Checksum audit: shallow (index presence) and deep (download, decrypt, verify every blob). occ backup:verify ci "$SNAP" | grep "^OK$" >/dev/null || fail "shallow verify" occ backup:verify ci "$SNAP" --deep | grep "^OK$" >/dev/null || fail "deep verify"