From 4d6ab741cc4d12e09be7052870e8683c53210b4f Mon Sep 17 00:00:00 2001 From: cproudlock Date: Fri, 14 Aug 2026 13:47:19 -0400 Subject: [PATCH] backups: retention reads the key the settings page actually writes get_setting on BasePlugin namespaces what it reads to plugin.backups., while get_settings_defaults declares - and the settings page writes - the bare key. So the retention read never found the operator's value and always fell back to 0, and 0 means keep everything. Retention was configurable in the UI and did nothing. It is the only place in the codebase using the namespaced helper. The share root also stops shipping one site's file server as its default. That put a site's internal topology in a bundled plugin and in the public mirror, and pointed a second site at a server it cannot reach. Blank now, per ADR-015, and a share kind with no configured root returns nothing rather than composing a path from somebody else's hostname - a path built on an empty root is not a lesser answer, it is a wrong one. --- plugins/backups/api/routes.py | 2 +- plugins/backups/plugin.py | 16 +++++++++++--- plugins/backups/services/registry.py | 25 +++++++++++++++------- tests/test_plugins/test_backups.py | 31 +++++++++++++++++++++++++--- 4 files changed, 59 insertions(+), 15 deletions(-) diff --git a/plugins/backups/api/routes.py b/plugins/backups/api/routes.py index f4533a8..b707fa8 100644 --- a/plugins/backups/api/routes.py +++ b/plugins/backups/api/routes.py @@ -40,7 +40,7 @@ def _sitezone(): Same lookup the notifications plugin uses. A stored timestamp is UTC, so anything rendered server-side has to be converted or it shows the wrong - wall clock for the site - four hours out at West Jefferson. + wall clock for the site - four hours out at the reference site. """ from shopdb.api import Setting row = Setting.query.filter_by(key='site_timezone').first() diff --git a/plugins/backups/plugin.py b/plugins/backups/plugin.py index e26567e..b784b88 100644 --- a/plugins/backups/plugin.py +++ b/plugins/backups/plugin.py @@ -20,6 +20,7 @@ from typing import Dict, List, Optional, Type from flask import Flask, Blueprint from shopdb.plugins.base import BasePlugin, PluginMeta +from shopdb.api import Setting from .api import backups_bp from .models import BackupRevision @@ -165,7 +166,10 @@ class BackupsPlugin(BasePlugin): 'valuetype': 'string', 'category': 'backups', 'description': 'UNC root that opaque (non-JSON) backups are ' - 'written under by the collecting PC. Site-specific.', + 'written under by the collecting PC, e.g. ' + '\\\\fileserver\\shopfloor\\backups. Blank until ' + 'the site sets it: no default can be right at ' + 'more than one site (ADR-015).', }, { 'key': 'backups_staledays', @@ -395,8 +399,14 @@ class BackupsPlugin(BasePlugin): pruned = prune( assetid, kindkey, - retentioncount=self.get_setting('backups_retentioncount', 0), - retentiondays=self.get_setting('backups_retentiondays', 0), + # Setting.get, NOT self.get_setting. The BasePlugin helper + # namespaces what it reads to `plugin.backups.`, while + # get_settings_defaults declares - and the settings page writes - + # the bare key. So this read never found the value an operator had + # set, always fell back to 0, and 0 means "keep everything": the + # retention policy was configurable in the UI and did nothing. + retentioncount=Setting.get('backups_retentioncount', 0), + retentiondays=Setting.get('backups_retentiondays', 0), ) if pruned: warnings.append('pruned {} old revision(s) per retention policy' diff --git a/plugins/backups/services/registry.py b/plugins/backups/services/registry.py index e0fdd32..45df56d 100644 --- a/plugins/backups/services/registry.py +++ b/plugins/backups/services/registry.py @@ -19,14 +19,17 @@ import json from . import ntlars as ntlarscodec -# Default root of the opaque-backup tree on the SFLD share. Connected PCs write -# here directly (they need SFLD creds - a SYSTEM process hitting a UNC path +# Root of the opaque-backup tree on the site's file share. Connected PCs write +# here directly (they need share creds - a SYSTEM process hitting a UNC path # without them gets an access-denied that Test-Path reports as "not found"). # -# This is the WEST JEFFERSON path and is only a DEFAULT: the live value is the -# backups_shareroot setting, because a bundled plugin in a multi-site product -# must not hardcode one site's file server. -DEFAULTSHAREROOT = r'\\tsgwp00525.wjs.geaerospace.net\shared\dt\shopfloor\backups' +# EMPTY BY DESIGN (ADR-015). This used to ship one site's own file server +# as the default, which put one site's internal topology in a bundled plugin and +# in the public mirror, and silently pointed a second site at a server it cannot +# reach. The live value is the backups_shareroot setting; unset, the share kinds +# have no conventional location to offer and say so rather than composing a path +# from somebody else's hostname. +DEFAULTSHAREROOT = '' def canonicalhash(projection): @@ -150,10 +153,16 @@ class BackupKind: Advisory only - the authoritative path is the sharepath the collector reports, since the PC is what actually wrote the file. This builds the conventional location for display and for validating a reported path. + + Returns '' when no share root is configured. A path built on an empty + root is not a lesser answer, it is a wrong one - it would render as a + relative path and validate a reported path against nothing. """ + root = shareroot or DEFAULTSHAREROOT + if not root: + return '' return '{}\\{}\\{}\\{}'.format( - shareroot or DEFAULTSHAREROOT, - machinetype or 'unknown', identifier or 'unknown', self.key) + root, machinetype or 'unknown', identifier or 'unknown', self.key) class NtlarsKind(BackupKind): diff --git a/tests/test_plugins/test_backups.py b/tests/test_plugins/test_backups.py index 6b08680..e2123cc 100644 --- a/tests/test_plugins/test_backups.py +++ b/tests/test_plugins/test_backups.py @@ -206,12 +206,19 @@ def test_partmarker_kind_is_share_backed_and_not_renderable(): assert kind.parse(b'anything') is None -def test_partmarker_sharedir_is_under_the_sfld_backups_root(): - path = registry.getkind('partmarker').sharedir('lathe', '3204') - assert path.startswith(registry.DEFAULTSHAREROOT) +def test_partmarker_sharedir_is_under_the_configured_backups_root(): + root = r'\\fileserver\shopfloor\backups' + path = registry.getkind('partmarker').sharedir('lathe', '3204', shareroot=root) + assert path.startswith(root) assert path.endswith(r'lathe\3204\partmarker') +def test_sharedir_is_empty_when_no_root_is_configured(): + """No default root ships (ADR-015), so an unconfigured site gets nothing + rather than a path composed from another site's file server.""" + assert registry.getkind('partmarker').sharedir('lathe', '3204') == '' + + def test_getkind_is_case_insensitive_and_returns_none_when_unknown(): assert registry.getkind('NTLARS') is not None assert registry.getkind('nosuchkind') is None @@ -981,3 +988,21 @@ def test_the_panel_shows_verified_beside_captured(bk_app, bk_plugin): listpanel = next(p for p in panels if p.get('render') == 'list') labels = [m['label'] for m in listpanel['map']['meta']] assert 'Captured' in labels and 'Verified' in labels + + +def test_retention_reads_the_key_the_settings_page_writes(app, db): + """The plugin helper namespaces to `plugin.backups.`; the declared key + is bare. Reading through the helper meant retention was configurable and + inert - it always fell back to 0, which means keep everything.""" + from shopdb.core.models import Setting + from plugins.backups.plugin import BackupsPlugin + + declared = {entry['key'] for entry in BackupsPlugin().get_settings_defaults()} + assert 'backups_retentioncount' in declared + + Setting.set('backups_retentioncount', 5) + db.session.commit() + assert int(Setting.get('backups_retentioncount', 0)) == 5 + # The helper cannot see it, which is the whole defect: it looks under + # plugin.backups.backups_retentioncount, which nothing writes. + assert int(BackupsPlugin().get_setting('backups_retentioncount', 0) or 0) == 0