backups: retention reads the key the settings page actually writes

get_setting on BasePlugin namespaces what it reads to plugin.backups.<key>,
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

@@ -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()

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.<key>`, 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'

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):

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.<key>`; 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