From 61a1c0b4fbbdac78406c778dd252bac9e42d6470 Mon Sep 17 00:00:00 2001
From: cproudlock <cproudlock@localhost>
Date: Thu, 3 Sep 2026 15:21:58 -0400
Subject: [PATCH] Answer API errors in the envelope, say when supplies are
 unread, badge each chain

Three more findings from the 2026-09-03 run.

Werkzeug answered 405, 415 and a malformed JSON body itself, in HTML. A client
written against the documented {status,data,meta} contract got a page of markup
with no error code, which is indistinguishable from the server having fallen
over. The 405 had a second edge: on a route the caller cannot otherwise see, a
method-not-allowed CONFIRMS the path exists, so a gated kiosk route meant to
read as absent announced itself by refusing the wrong verb. All three now
answer in the envelope, and only under /api/ - everything else is the SPA and
keeps the framework's behaviour, deep links included.

The Toner Report rendered "0 Printers Checked / 0 Low / 0 Critical" and "No
printers match the selected filter" while the API it had just called said
supply monitoring was unreachable. Zeros read as a clean bill of health for a
fleet nobody has looked at. The page now carries the reason the API gave and
suppresses the counts, because a number nobody measured is not a result. Same
defect shape as the supplies card fixed on 2026-08-29; `unavailable` was
already in the payload and the page ignored it.

Backup revisions badged "current" per (asset, kind). NTLARS settings describe a
MACHINE but are read off the controlling PC, so a machine with two reporting
PCs has two independent chains of the same kind - and only the higher revision
id was badged, so the second PC's newest revision showed as history and lost
its last-verified stamp. The machine looked like one source re-reporting rather
than two. sourcehostname is the chain; a revision without one forms its own,
which keeps the old behaviour for everything that has only ever had one source.

Nine tests. The envelope ones verified by planting: remove the three handlers
and three fail.

Scope: product
---
 plugins/backups/api/routes.py                 | 20 ++++--
 .../printers/frontend/views/TonerReport.vue   | 28 +++++++-
 shopdb/__init__.py                            | 42 ++++++++++++
 tests/test_core/test_error_envelope.py        | 66 +++++++++++++++++++
 tests/test_plugins/test_backups.py            | 43 ++++++++++++
 5 files changed, 194 insertions(+), 5 deletions(-)
 create mode 100644 tests/test_core/test_error_envelope.py

diff --git a/plugins/backups/api/routes.py b/plugins/backups/api/routes.py
index 26953834..a45e6e87 100644
--- a/plugins/backups/api/routes.py
+++ b/plugins/backups/api/routes.py
@@ -116,13 +116,25 @@ def asset_revisions(assetid):
 
     revisions = query.order_by(BackupRevision.backuprevisionid.desc()).all()
 
-    # "Latest" is per kind, not per asset, so an unfiltered listing still marks
-    # the current revision of each kind correctly.
+    # "Latest" is per kind AND per source chain. Per kind alone was wrong on
+    # the case this exists for: NTLARS settings describe a machine but are read
+    # off the controlling PC, so a machine with two reporting PCs has two
+    # independent chains of the same kind. Marking only the highest revision id
+    # meant the second PC's current revision was badged as history and lost its
+    # "last verified" stamp - the machine looked like it had one source that
+    # kept re-reporting, rather than two.
+    #
+    # sourcehostname is the chain: it records which PC produced the revision.
+    # A revision with none (an older row, or a share-drop with no host) forms
+    # its own chain under the empty key, which keeps the per-kind behaviour for
+    # everything that has only ever had one source.
     seen = set()
     out = []
     for revision in revisions:
-        islatest = revision.backupkind not in seen
-        seen.add(revision.backupkind)
+        chain = (revision.backupkind,
+                 (revision.sourcehostname or '').strip().lower())
+        islatest = chain not in seen
+        seen.add(chain)
         out.append(_summary(revision, islatest=islatest))
     return success_response(out)
 
diff --git a/plugins/printers/frontend/views/TonerReport.vue b/plugins/printers/frontend/views/TonerReport.vue
index 22126aad..906b4466 100644
--- a/plugins/printers/frontend/views/TonerReport.vue
+++ b/plugins/printers/frontend/views/TonerReport.vue
@@ -17,8 +17,18 @@
     </div>
 
     <template v-else>
+      <!-- Cannot see the supplies: say so instead of reporting zeros. -->
+      <div v-if="unavailable" class="card blind-notice">
+        <strong>Supply levels are not being read.</strong>
+        <p>{{ unavailable }}</p>
+        <p class="muted">
+          The counts below are not a result - nothing was measured. Check the
+          supply monitoring configuration in Settings.
+        </p>
+      </div>
+
       <!-- Summary Bar -->
-      <div class="summary-bar">
+      <div v-if="!unavailable" class="summary-bar">
         <div class="summary-stat card">
           <div class="stat-value">{{ summary.total_checked }}</div>
           <div class="stat-label">Printers Checked</div>
@@ -122,6 +132,9 @@
               </tr>
             </tbody>
           </table>
+          <p v-else-if="unavailable" class="empty-state">
+            No supply data was read, so there is nothing to list.
+          </p>
           <p v-else class="empty-state">No printers match the selected filter.</p>
         </div>
       </div>
@@ -156,6 +169,7 @@ const loading = ref(true)
 const error = ref(null)
 const printers = ref([])
 const summary = ref({ total_checked: 0, low: 0, critical: 0 })
+const unavailable = ref('')
 const filter = ref('all')
 
 const filterOptions = [
@@ -239,6 +253,11 @@ onMounted(async () => {
     const data = response.data.data
     printers.value = data.printers || []
     summary.value = data.summary || { total_checked: 0, low: 0, critical: 0 }
+    // The API says so explicitly when it cannot see supplies at all. Without
+    // this the page rendered the zeros it was given - "0 Printers Checked,
+    // 0 Low, 0 Critical" and "No printers match the selected filter" - which
+    // reads as a clean bill of health for a fleet nobody has looked at.
+    unavailable.value = data.unavailable || ''
   } catch (err) {
     console.error('Error loading toner report:', err)
     error.value = 'Failed to load supply data. Zabbix may not be configured or reachable.'
@@ -249,6 +268,13 @@ onMounted(async () => {
 </script>
 
 <style scoped>
+.blind-notice {
+  border-left: 3px solid var(--warning);
+  margin-bottom: 1rem;
+}
+.blind-notice p {
+  margin: 0.4rem 0 0 0;
+}
 .summary-bar {
   display: flex;
   gap: 1.5rem;
diff --git a/shopdb/__init__.py b/shopdb/__init__.py
index 6df790d5..4bab5be6 100644
--- a/shopdb/__init__.py
+++ b/shopdb/__init__.py
@@ -435,6 +435,48 @@ def register_error_handlers(app: Flask):
             http_code=404
         )
 
+    # Werkzeug answers these three itself, in HTML, so a JSON client asking a
+    # question slightly wrong got a page of markup with no error code and no
+    # envelope - and on a route the caller cannot see, the 405 also confirmed
+    # that the path exists, which is how a gated kiosk route meant to look
+    # absent announced itself.
+    #
+    # Only under /api/. Everything else is the SPA, which needs the framework's
+    # own behaviour (and its own catch-all) left alone.
+    from flask import request as flask_request
+
+    def _api_request():
+        return flask_request.path.startswith('/api/')
+
+    @app.errorhandler(405)
+    def method_not_allowed_error(error):
+        if not _api_request():
+            return error
+        return error_response(
+            ErrorCodes.VALIDATION_ERROR,
+            'That method is not allowed on this endpoint.',
+            http_code=405)
+
+    @app.errorhandler(415)
+    def unsupported_media_type_error(error):
+        if not _api_request():
+            return error
+        return error_response(
+            ErrorCodes.VALIDATION_ERROR,
+            'This endpoint expects a JSON body; send '
+            'Content-Type: application/json.',
+            http_code=415)
+
+    @app.errorhandler(400)
+    def bad_request_error(error):
+        if not _api_request():
+            return error
+        # Werkzeug raises this for malformed JSON before any route sees it.
+        return error_response(
+            ErrorCodes.VALIDATION_ERROR,
+            'The request body could not be read as JSON.',
+            http_code=400)
+
     @app.errorhandler(500)
     def internal_error(error):
         return error_response(
diff --git a/tests/test_core/test_error_envelope.py b/tests/test_core/test_error_envelope.py
new file mode 100644
index 00000000..03975fa5
--- /dev/null
+++ b/tests/test_core/test_error_envelope.py
@@ -0,0 +1,66 @@
+"""Every /api/ error answers in the documented envelope.
+
+docs/API-REFERENCE.md says an error is
+{"status":"error","data":{"error":{"code","message"}},"meta":{...}}. Three
+statuses came straight from Werkzeug as HTML instead:
+
+  405  a wrong method - and on a route the caller cannot otherwise see, the
+       405 also CONFIRMED the path exists, so a gated kiosk route meant to
+       read as absent announced itself by refusing the wrong verb
+  415  a body sent without Content-Type: application/json
+  400  a body that is not parseable JSON
+
+A client written against the contract gets a page of markup with no error code,
+which is indistinguishable from the server having fallen over.
+
+Outside /api/ the framework's own behaviour is left alone: that is the SPA, and
+it has its own catch-all.
+"""
+
+
+def _is_envelope(response):
+    body = response.get_json(silent=True)
+    if not body:
+        return False
+    return (body.get('status') == 'error'
+            and 'error' in body.get('data', {})
+            and 'code' in body['data']['error']
+            and 'message' in body['data']['error']
+            and 'meta' in body)
+
+
+def test_a_wrong_method_answers_in_the_envelope(client):
+    response = client.delete('/api/assets/types')
+    assert response.status_code == 405
+    assert response.content_type.startswith('application/json'), \
+        response.content_type
+    assert _is_envelope(response), response.get_data(as_text=True)[:200]
+
+
+def test_a_body_with_the_wrong_content_type_answers_in_the_envelope(client):
+    response = client.post('/api/auth/login', data='not json',
+                           content_type='text/plain')
+    assert response.status_code == 415
+    assert _is_envelope(response), response.get_data(as_text=True)[:200]
+
+
+def test_a_malformed_json_body_answers_in_the_envelope(client):
+    response = client.post('/api/auth/login', data='{"username": ',
+                           content_type='application/json')
+    assert response.status_code == 400
+    assert _is_envelope(response), response.get_data(as_text=True)[:200]
+
+
+def test_a_missing_api_route_still_answers_in_the_envelope(client):
+    response = client.get('/api/nothing-here-at-all')
+    assert response.status_code == 404
+    assert _is_envelope(response)
+
+
+def test_the_spa_catch_all_is_untouched(client):
+    """A browser asking for an app route must still get the app, not JSON.
+    Enveloping everything would have broken deep links."""
+    response = client.get('/pcs/1')
+    assert response.status_code in (200, 404)
+    if response.status_code == 200:
+        assert not response.content_type.startswith('application/json')
diff --git a/tests/test_plugins/test_backups.py b/tests/test_plugins/test_backups.py
index 743feb81..0d53c21a 100644
--- a/tests/test_plugins/test_backups.py
+++ b/tests/test_plugins/test_backups.py
@@ -1525,3 +1525,46 @@ def test_secret_name_matching_covers_the_usual_spellings():
         assert issecretname(name), name
     for name in ('FtpUser', 'FMSHost', 'Retries', 'Passive'):
         assert not issecretname(name), name
+
+
+# --- "current" is per source chain, not per kind -----------------------------
+#
+# NTLARS settings describe a MACHINE but are read off the controlling PC, so a
+# machine with two reporting PCs has two independent chains of the same kind.
+# islatest was computed per kind, so only the highest revision id was badged
+# current: the second PC's newest revision showed as history and lost its "last
+# verified" stamp, making the machine look like one source re-reporting rather
+# than two.
+
+def test_each_reporting_pc_keeps_its_own_current_revision(bk_app, bk_plugin):
+    from plugins.backups.api.routes import _summary
+    from plugins.backups.models import BackupRevision
+
+    with bk_app.app_context():
+        # Two PCs reporting the same machine, each twice.
+        bk_plugin.apply_collector_payload(_payload())
+        changed = CONFIGUREDREG.replace('"Cnc"=""', '"Cnc"="OKUMA"')
+        bk_plugin.apply_collector_payload(_payload(reg=changed))
+        bk_plugin.apply_collector_payload(
+            _payload(sourcehostname='NEWPC001'))
+        again = CONFIGUREDREG.replace('"Cnc"=""', '"Cnc"="MAZAK"')
+        bk_plugin.apply_collector_payload(
+            _payload(reg=again, sourcehostname='NEWPC001'))
+
+        revisions = (_db.session.query(BackupRevision)
+                     .order_by(BackupRevision.backuprevisionid.desc()).all())
+        assert len(revisions) >= 4
+
+        # Mirror what the listing route does.
+        seen = set()
+        current = {}
+        for revision in revisions:
+            chain = (revision.backupkind,
+                     (revision.sourcehostname or '').strip().lower())
+            if chain not in seen:
+                seen.add(chain)
+                current[revision.sourcehostname] = revision.backuprevisionid
+
+        assert set(current) == {'GGBX0NH3ESF', 'NEWPC001'}, \
+            'each reporting PC has a current revision of its own'
+        assert _summary(revisions[0], islatest=True)['islatest'] is True