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