Backup-Cloud retation counter #2020#899
Conversation
Introduce a new number input for specifying the number of backups to retain. This allows users to manage backup retention more effectively, with a help template explaining the functionality.
…ount Refactor the Samba backup component to change the input model from retention_count to max_backups, enhancing clarity in backup management settings.
Introduce a new number input for specifying the maximum number of backups to retain, with a help template explaining the functionality. This enhances user control over backup retention settings.
benderl
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a retention counter (“max_backups”) to backup cloud configurations so users can control how many backups are kept before older ones are auto-deleted (Issue #2020; paired with core PR openWB/core#3138).
Changes:
- Add a numeric input for
configuration.max_backupsto the Samba backup cloud config UI. - Add a numeric input for
configuration.max_backupsto the Nextcloud backup cloud config UI.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/components/backup_clouds/samba/backup_cloud.vue | Adds a retention number input with help text and min/max constraints. |
| src/components/backup_clouds/nextcloud/backup_cloud.vue | Adds a retention number input with help text and min/step constraints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| title="Anzahl Backups aufbewahren" | ||
| :min="0" | ||
| :step="1" | ||
| :model-value="backupCloud.configuration.max_backups" |
There was a problem hiding this comment.
backupCloud.configuration.max_backups is new in the frontend and (currently) only referenced in these two backup cloud components. If an existing installation loads a backup cloud config that doesn’t yet contain max_backups, this input will start with undefined and OpenwbBaseNumberInput will emit null when the field is cleared (default emptyValue). Since the help text defines 0 as the meaningful “disable auto deletion” value, it would be safer to ensure the model is always a number (e.g., default to 0 / use emptyValue accordingly) rather than allowing null/undefined to be stored in the configuration.
| :model-value="backupCloud.configuration.max_backups" | |
| :model-value="backupCloud.configuration.max_backups || 0" | |
| :empty-value="0" |
There was a problem hiding this comment.
wird über update_config im Backend gelöst.
There was a problem hiding this comment.
Danke für den Hinweis mit update_config, das macht Sinn
Aus Frontend-Sicht wäre es optional noch möglich, mit ?? 0 zu arbeiten, um bereits im UI einen definierten Wert sicherzustellen und undefined-Zustände zu vermeiden.
:model-value="backupCloud.configuration.max_backups ?? 0"
Ist aber kein Muss, wenn das Backend das ohnehin zuverlässig abfängt.
Optional könnte man zusätzlich mit empty-value="0" arbeiten, um auch beim Leeren des Inputs konsistente Werte zu erhalten.
Ist aber kein Muss, wenn von Backend behandelt.
| :model-value="backupCloud.configuration.max_backups" | ||
| :min="0" | ||
| :max="100" | ||
| required | ||
| @update:model-value="updateConfiguration($event, 'configuration.max_backups')" |
There was a problem hiding this comment.
The new retention setting is marked required here, but max_backups isn’t defined anywhere else in this repo (no defaults in the UI), so older persisted configurations may render this field invalid/empty initially and can end up writing null back to the config if cleared (OpenwbBaseNumberInput defaults emptyValue to null). Given the semantics in the help text (“0 = keine automatische Löschung”), consider making sure the value is always a number (initialize/display as 0 and emit 0 on empty) and align required/min/max/step behavior with the Nextcloud backup cloud config for consistency.
| :model-value="backupCloud.configuration.max_backups" | |
| :min="0" | |
| :max="100" | |
| required | |
| @update:model-value="updateConfiguration($event, 'configuration.max_backups')" | |
| :model-value="backupCloud.configuration.max_backups != null ? backupCloud.configuration.max_backups : 0" | |
| :min="0" | |
| :max="100" | |
| :step="1" | |
| required | |
| @update:model-value="updateConfiguration($event === null ? 0 : $event, 'configuration.max_backups')" |
There was a problem hiding this comment.
Danke für das Update. (Commit: [02b04fe]), die Behandlung von null / leeren Werten ist damit auf jeden Fall sauber gelöst.
Aus Frontend-Sicht könnte man das Ganze optional noch etwas vereinfachen, z. B. mit ?? 0, um nur null und undefined abzufangen und den Ausdruck etwas lesbarer zu halten:
:model-value="backupCloud.configuration.max_backups ?? 0"
@update:model-value="updateConfiguration($event ?? 0, 'configuration.max_backups')"
Ist aber eher eine Stilfrage – funktional passt deine Lösung so.
…ts entsprechend angepasst (neue upgrade_datastore_version für Nextcloud/Samba, aktuell auf DATASTORE_VERSION = 114). In diesem UI-Repo ändere ich daher nur die UI-seitige Behandlung von max_backups.
…l im Upgrade-Pfad als auch im normalen update_config-Pfad für nextcloud und samba auf null/None => 0 abgesichert. Frontend-seitig behalten wir zusätzlich die Absicherung mit ?? 0 und schreiben bei leerem Feld ebenfalls 0 zurück, damit der Wert auch in der UI jederzeit konsistent bleibt (Defense-in-depth).
Brett-S-OWB
left a comment
Brett-S-OWB
left a comment
There was a problem hiding this comment.
Sieht gut aus für mich, danke für die Anpassungen 👍
…zt: der Normalisierer im normalen update()-Pfad sowie die getattr(..., None)-Abfragen sind entfernt bzw. vereinfacht worden. Frontend-seitig behalten wir trotzdem die Defense-in-depth: max_backups wird im UI immer als Zahl behandelt (?? 0 beim Anzeigen und beim Leeren wird 0 zurückgeschrieben), damit Bestands-Configs auch bei leerem Feld keine null/undefined Werte durchreichen.
#2020 from Core
Issue: openWB/core#2020
core: openWB/core#3138