Collector: machine number identifies the machine, not the PC; ntlars empty-value fix

CHANGELOG.md

@@ -12,6 +12,19 @@ ADR-007 and ADR-002.
 
 ### Fixed
 
+- A shop-floor PC that reported the machine number of a machine ShopDB already
+  knew got a 500 from the collector, on every report, forever. The reported
+  machine number was being written to the PC's own `assets.assetnumber`, which
+  is uniquely indexed and already held by the machine, so the insert failed with
+  a duplicate-key error and the whole report was lost: operating system, boot
+  time, applications, printers and access protocols never landed. The machine
+  number now identifies the machine only. A new PC takes its hostname as its
+  asset number, which is what the existing data already does, and an existing
+  PC's asset number is never overwritten.
+- A backup export containing a value with an empty right-hand side (`Name=`)
+  failed to parse, and with it the whole file, so that machine could never be
+  backed up. The form is not strictly legal but occurs in real exports. It is
+  now read as an empty string, preserving the value name.
 - The 3D parts kiosk label prefix never appeared on the kiosk. The kiosk runs
   logged out, and an unauthenticated read of a setting is limited to an
   allowlist the key was not on, so the kiosk got a 404 and fell back to no
@@ -35,6 +48,17 @@ ADR-007 and ADR-002.
 
 ### Added
 
+- A PC reporting a machine number is now linked to that machine. Until now the
+  number was collected and then discarded, so a replaced PC never took over its
+  bay: the retired PC kept the link and everything that walks PC to machine, the
+  warranty machine column and the DNC info card among them, pointed at hardware
+  that had been pulled out. The link is a `controls` relationship carrying a
+  `collector:machine` origin label, so a link made by hand is never touched.
+  Superseding a PC archives the old link rather than deleting it, keeping the
+  history of which PC ran a machine and when, and raises an alert by email and
+  webhook. The retired PC's status is deliberately left alone: the collector
+  cannot tell whether it was shelved, sent for repair or re-imaged for another
+  bay, so a person decides.
 - Plugin contract 0.16.0: `get_settings_defaults()` lets a plugin declare the
   settings it owns (key, default, type, category, description, and whether an
   unauthenticated caller may read it). See `docs/PLUGIN-HOOKS.md`.

plugins/backups/services/ntlars.py

@@ -149,6 +149,13 @@ def _parsevalue(body):
         return _parsehexvalue(body)
     if body == '-':
         return 'DELETE', None
+    if body == '':
+        # "Name="  with nothing after the sign. Not strictly legal, but it
+        # occurs in real exports (the part marker's WJPRT.reg has KRelay1 like
+        # this), and failing the whole file over it would mean that machine can
+        # never be backed up. Treat it as an empty string so the value NAME is
+        # still preserved.
+        return 'REG_SZ', ''
     raise NtlarsParseError('unrecognised value form: {!r}'.format(body))
 
 

plugins/computers/plugin.py

@@ -28,6 +28,12 @@ PRINTER_LINK_ORIGIN = 'collector:printers'
 # by a collector push, so hand-made tool links survive.
 MEASURINGTOOL_LINK_ORIGIN = 'collector:measuringtool'
 
+# Marker stamped on the PC->machine "controls" link built from the reported
+# machine number. Same discipline as the two above: only rows carrying this
+# label are archived by a collector push, so a link made by hand is never
+# touched.
+MACHINE_LINK_ORIGIN = 'collector:machine'
+
 
 class ComputersPlugin(BasePlugin):
     """
@@ -153,8 +159,15 @@ class ComputersPlugin(BasePlugin):
         if not hostname:
             raise ValueError('hostname is required')
 
-        # Machine number is the business identifier (Asset.assetnumber). Skip
-        # the imaging-time placeholder '9999' and fall back to hostname.
+        # The machine number identifies the MACHINE, not this PC. It is
+        # reported so the PC can be related to its machine; it is deliberately
+        # NOT used as the PC's assetnumber. assets.assetnumber is uniquely
+        # indexed and the machine already owns that value, so assigning it here
+        # raised "Duplicate entry '3015' for key 'ix_assets_assetnumber'" and
+        # returned 500 to the bay - forever, since every retry did the same
+        # thing. The convention this restores is what the data already shows:
+        # of 289 computers, none has a numeric assetnumber and 214 use their
+        # hostname.
         machinenumber = (payload.get('machinenumber') or '').strip()
         if machinenumber in ('', '9999'):
             machinenumber = None
@@ -169,7 +182,7 @@ class ComputersPlugin(BasePlugin):
             atype = AssetType.query.filter_by(assettype='computer').first()
             # statusid=1 is the first seeded asset status ("In Use"); a
             # collector-discovered PC is by definition in use.
-            asset = Asset(assetnumber=machinenumber or hostname,
+            asset = Asset(assetnumber=hostname,
                           assettypeid=atype.assettypeid, statusid=1)
             db.session.add(asset)
             db.session.flush()
@@ -177,8 +190,10 @@ class ComputersPlugin(BasePlugin):
             db.session.add(comp)
             db.session.flush()
             action = 'created'
-        elif machinenumber and comp.asset:
-            comp.asset.assetnumber = machinenumber
+        # NOTE: an existing PC's assetnumber is left alone. Overwriting it with
+        # the machine number renamed the PC onto the machine's identifier, which
+        # either collided with the unique index or silently changed how that PC
+        # is identified everywhere else.
 
         comp.lastreporteddate = datetime.now(timezone.utc).replace(tzinfo=None)
         if payload.get('lastboottime'):
@@ -277,6 +292,9 @@ class ComputersPlugin(BasePlugin):
         # Remote-access protocol sync (only when the payload carried the key).
         accessprotocols = self._sync_access_protocols(comp, payload, warnings)
 
+        # PC -> machine link from the reported machine number.
+        machinelinks = self._sync_machine_link(comp, machinenumber, warnings)
+
         # Printer relationship sync (only when the payload carried printer data).
         printerlinks = self._sync_printer_links(comp.asset, payload, warnings)
 
@@ -297,6 +315,7 @@ class ComputersPlugin(BasePlugin):
                 'measuringtoollinks': measuringtoollinks,
                 'measuringtoollinkcount': len(measuringtoollinks),
                 'accessprotocols': accessprotocols,
+                'machinelinks': machinelinks,
             },
         }
 
@@ -494,6 +513,140 @@ class ComputersPlugin(BasePlugin):
 
     # -- measuring-tool sync -----------------------------------------------
 
+    def _sync_machine_link(self, comp, machinenumber, warnings):
+        """Link a PC to the machine it drives, from the reported machine number.
+
+        Until this existed the machine number was collected and then discarded,
+        so a replaced PC never took over its bay: the retired PC kept the link
+        and the new one got none. Everything that walks PC->machine (the
+        warranty machine column, the DNC info card) therefore pointed at
+        hardware that had been pulled out.
+
+        ARCHIVES, never deletes. A superseded link stays with isactive=False so
+        "which PC ran 3015 in June" is still answerable. Status on the old PC is
+        deliberately NOT changed: the collector cannot tell whether it was
+        shelved, broken or re-imaged for another bay, and guessing would
+        overwrite whatever a person deliberately set.
+        """
+        from shopdb.api import AssetRelationship, RelationshipType, Asset
+
+        pcasset = comp.asset if comp else None
+        if not machinenumber or not pcasset:
+            return []
+
+        controls = RelationshipType.query.filter_by(
+            relationshiptype='controls').first()
+        if not controls:
+            warnings.append("'controls' relationship type missing; "
+                            'run flask seed reference-data')
+            return []
+
+        machine = Asset.query.filter(
+            Asset.assetnumber.ilike(machinenumber),
+            Asset.isactive.is_(True)).first()
+        if not machine:
+            # Reported a machine ShopDB does not know. Warn rather than invent
+            # an asset: a mistyped number would create a machine nobody can
+            # account for.
+            warnings.append(
+                'no asset for machine number {!r}; PC not linked'.format(
+                    machinenumber))
+            return []
+        if machine.assetid == pcasset.assetid:
+            return []
+
+        # This PC's own collector links: keep the one for the reported machine,
+        # archive any other (the PC moved bays).
+        mine = AssetRelationship.query.filter(
+            AssetRelationship.sourceassetid == pcasset.assetid,
+            AssetRelationship.relationshiptypeid == controls.relationshiptypeid,
+            AssetRelationship.label == MACHINE_LINK_ORIGIN,
+        ).all()
+
+        link = None
+        for rel in mine:
+            if rel.targetassetid == machine.assetid:
+                rel.isactive = True
+                link = rel
+            else:
+                rel.isactive = False
+        if link is None:
+            link = AssetRelationship(
+                sourceassetid=pcasset.assetid,
+                targetassetid=machine.assetid,
+                relationshiptypeid=controls.relationshiptypeid,
+                label=MACHINE_LINK_ORIGIN,
+                isactive=True)
+            db.session.add(link)
+
+        # Any OTHER pc still collector-linked to this machine has been replaced.
+        superseded = AssetRelationship.query.filter(
+            AssetRelationship.targetassetid == machine.assetid,
+            AssetRelationship.relationshiptypeid == controls.relationshiptypeid,
+            AssetRelationship.label == MACHINE_LINK_ORIGIN,
+            AssetRelationship.sourceassetid != pcasset.assetid,
+            AssetRelationship.isactive.is_(True),
+        ).all()
+        for rel in superseded:
+            rel.isactive = False
+            old = db.session.get(Asset, rel.sourceassetid)
+            oldname = old.assetnumber if old else str(rel.sourceassetid)
+            warnings.append(
+                'machine {} was taken over from {}; check that PC'.format(
+                    machine.assetnumber, oldname))
+            self._alert_pc_superseded(oldname, comp, machine)
+
+        return [{'assetid': machine.assetid,
+                 'machinenumber': machine.assetnumber,
+                 'superseded': len(superseded)}]
+
+    def _alert_pc_superseded(self, oldname, newcomp, machine):
+        """Tell a human a PC was replaced on a machine. Best effort, never raises.
+
+        Deliberately an ALERT and not a status change: the collector cannot tell
+        whether the old PC was shelved, sent for repair or re-imaged for another
+        bay, so it says what happened and lets a person decide. Uses the same
+        email + webhook path as the toner alerts, falling back to the site-wide
+        alert_recipients when nothing more specific is set.
+
+        NOT a shopfloor notification: that board is for operators (General,
+        Recertification, Recognition), and an IT asset message does not belong
+        in front of the floor.
+        """
+        import logging as _logging
+        try:
+            from shopdb.api import send_email, send_webhook, Setting
+
+            newname = newcomp.hostname if newcomp else 'a new PC'
+            machinename = machine.assetnumber or machine.name
+            subject = 'PC replaced on machine {}: {} to {}'.format(
+                machinename, oldname, newname)
+            # /pcs/:id is keyed on computerid, not assetid - an assetid here
+            # opens someone else's PC or a 404.
+            base = (Setting.get('site_base_url') or '').rstrip('/')
+            link = '{}/pcs/{}'.format(base, newcomp.computerid) if base else ''
+            linkhtml = ('<p><a href="{0}">View {1}</a></p>'.format(link, newname)
+                        if link else '')
+            html = (
+                '<p><strong>{0}</strong> is now reporting machine '
+                '<strong>{1}</strong>, which was previously driven by '
+                '<strong>{2}</strong>.</p>'
+                '<p>The old link has been archived. {2} has NOT had its status '
+                'changed - set it to Inventory, In Repair or Retired as '
+                'appropriate.</p>{3}'.format(newname, machinename, oldname,
+                                             linkhtml))
+            send_webhook(subject, subject)
+
+            raw = Setting.get('alert_recipients') or ''
+            recipients = [a.strip() for a in raw.replace(';', ',').split(',')
+                          if a.strip()]
+            if recipients:
+                send_email(recipients, subject, html)
+        except Exception:
+            _logging.getLogger(__name__).exception(
+                'PC-superseded alert failed for machine %s',
+                getattr(machine, 'assetnumber', '?'))
+
     def _sync_measuringtool_link(self, pcasset, pctype, hostname, warnings):
         """Auto-create + link the MeasuringTool a metrology PC drives.
 

tests/test_core/test_collector_contract.py

@@ -145,7 +145,12 @@ def test_complete_asset_payload_maps_enrollment_data(client, db, collector_key,
     with client.application.app_context():
         comp = Computer.query.filter(Computer.hostname.ilike('WJSF1234')).first()
         assert comp is not None
-        assert comp.asset.assetnumber == '0615'          # machinenumber -> assetnumber
+        # The PC keeps its OWN assetnumber (hostname). machinenumber names the
+        # MACHINE, and machines are assets too (ADR-001), so assigning it here
+        # hit the unique index on assets.assetnumber and returned 500 to the bay
+        # on every retry. The machine number now builds a controls link instead,
+        # which is what the reported number was actually for.
+        assert comp.asset.assetnumber == 'WJSF1234'
         assert comp.asset.serialnumber == 'SN-ENROLL'
         assert comp.computertype.computertype == 'Shopfloor PC'  # pctype mapped
         assert comp.vendor.vendor == 'Dell'              # created
@@ -528,3 +533,192 @@ def test_unknown_protocol_warns_and_does_not_create_one(client, db, collector_ke
     assert any('TeamViewer' in w for w in body['warnings'])
     assert AccessProtocol.query.filter(
         AccessProtocol.name.ilike('TeamViewer')).first() is None
+
+
+# =============================================================================
+# PC -> machine link, and the PC-swap case
+# =============================================================================
+
+@pytest.fixture
+def machine_3015(db):
+    """A machine asset that already owns the number a PC will report."""
+    from shopdb.core.models import AssetType, Asset, RelationshipType
+
+    mt = AssetType(assettype='machine', pluginname='machines',
+                   tablename='machines', description='Machines')
+    db.session.add(mt)
+    db.session.add(RelationshipType(relationshiptype='controls',
+                                    isdirectional=True))
+    db.session.flush()
+    asset = Asset(assetnumber='3015', name='Machine 3015',
+                  assettypeid=mt.assettypeid, statusid=1)
+    db.session.add(asset)
+    db.session.commit()
+    return asset
+
+
+def _links(machineassetid, activeonly=True):
+    from shopdb.core.models import AssetRelationship
+    q = AssetRelationship.query.filter_by(targetassetid=machineassetid)
+    if activeonly:
+        q = q.filter_by(isactive=True)
+    return q.all()
+
+
+def test_pc_reporting_an_existing_machine_number_does_not_500(
+        client, db, collector_key, computer_assettype, machine_3015):
+    """The machine owns assetnumber 3015. Naming the PC 3015 too hit the unique
+    index and returned 500 to the bay on every retry, forever."""
+    r = client.post('/api/collector/computers',
+                    json={'hostname': 'FFBWTH63', 'machinenumber': '3015'},
+                    headers={'X-API-Key': collector_key})
+    assert r.status_code == 200, r.get_json()
+
+
+def test_pc_gets_its_own_assetnumber_not_the_machines(
+        client, db, collector_key, computer_assettype, machine_3015):
+    from plugins.computers.models import Computer
+    client.post('/api/collector/computers',
+                json={'hostname': 'FFBWTH63', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+    comp = Computer.query.filter(Computer.hostname.ilike('FFBWTH63')).first()
+    assert comp.asset.assetnumber == 'FFBWTH63'
+
+
+def test_reported_machine_number_creates_the_link(
+        client, db, collector_key, computer_assettype, machine_3015):
+    client.post('/api/collector/computers',
+                json={'hostname': 'FFBWTH63', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+    assert len(_links(machine_3015.assetid)) == 1
+
+
+def test_replacing_a_pc_hands_the_machine_over(
+        client, db, collector_key, computer_assettype, machine_3015):
+    """The point of the whole thing: the new PC takes the bay and the retired
+    one stops being shown as its controller."""
+    from shopdb.core.models import Asset
+    client.post('/api/collector/computers',
+                json={'hostname': 'OLDPC', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+    r = client.post('/api/collector/computers',
+                    json={'hostname': 'NEWPC', 'machinenumber': '3015'},
+                    headers={'X-API-Key': collector_key})
+
+    active = _links(machine_3015.assetid)
+    assert len(active) == 1
+    owner = db.session.get(Asset, active[0].sourceassetid)
+    assert owner.assetnumber == 'NEWPC'
+
+    # and it says so, so a person can go and look at the old one
+    assert any('taken over' in w for w in r.get_json()['data']['warnings'])
+
+
+def test_the_old_link_is_archived_not_deleted(
+        client, db, collector_key, computer_assettype, machine_3015):
+    """History has to survive: 'which PC ran 3015 in June' stays answerable."""
+    client.post('/api/collector/computers',
+                json={'hostname': 'OLDPC', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+    client.post('/api/collector/computers',
+                json={'hostname': 'NEWPC', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+    assert len(_links(machine_3015.assetid, activeonly=False)) == 2
+
+
+def test_the_old_pc_status_is_left_alone(
+        client, db, collector_key, computer_assettype, machine_3015):
+    """The collector cannot tell if it was shelved, broken or re-imaged, so it
+    must not guess - and must not overwrite what a person set."""
+    from plugins.computers.models import Computer
+    client.post('/api/collector/computers',
+                json={'hostname': 'OLDPC', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+    old = Computer.query.filter(Computer.hostname.ilike('OLDPC')).first()
+    before = old.asset.statusid
+    client.post('/api/collector/computers',
+                json={'hostname': 'NEWPC', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+    db.session.refresh(old.asset)
+    assert old.asset.statusid == before
+
+
+def test_unknown_machine_number_warns_and_invents_nothing(
+        client, db, collector_key, computer_assettype, machine_3015):
+    from shopdb.core.models import Asset
+    r = client.post('/api/collector/computers',
+                    json={'hostname': 'FFBWTH63', 'machinenumber': '9911'},
+                    headers={'X-API-Key': collector_key})
+    assert any('9911' in w for w in r.get_json()['data']['warnings'])
+    assert Asset.query.filter(Asset.assetnumber == '9911').first() is None
+
+
+def test_the_placeholder_machine_number_links_nothing(
+        client, db, collector_key, computer_assettype, machine_3015):
+    """9999 is the imaging-time placeholder, not a real bay."""
+    r = client.post('/api/collector/computers',
+                    json={'hostname': 'FFBWTH63', 'machinenumber': '9999'},
+                    headers={'X-API-Key': collector_key})
+    assert r.status_code == 200
+    assert _links(machine_3015.assetid) == []
+
+
+def test_supersede_alerts_email_and_webhook(client, db, collector_key,
+                                            computer_assettype, machine_3015,
+                                            monkeypatch):
+    """A person has to find out. Email goes to the site alert_recipients and the
+    webhook fires, the same path the toner alerts use."""
+    from shopdb.core.models import Setting
+    import shopdb.api as shopdbapi
+
+    # Patch on shopdb.api, which is where the plugin imports them from - the
+    # names are bound there at import time, so patching shopdb.utils.mailer
+    # would silently have no effect and the real (disabled) mailer would run.
+    sent = {}
+    monkeypatch.setattr(
+        shopdbapi, 'send_email',
+        lambda to, subj, html, **k: sent.update(email=(to, subj, html)))
+    monkeypatch.setattr(shopdbapi, 'send_webhook',
+                        lambda subj, text, **k: sent.update(webhook=subj))
+    db.session.add(Setting(key='alert_recipients', value='it@example.com'))
+    db.session.add(Setting(key='site_base_url', value='https://shopdb/shopdb'))
+    db.session.commit()
+
+    client.post('/api/collector/computers',
+                json={'hostname': 'OLDPC', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+    client.post('/api/collector/computers',
+                json={'hostname': 'NEWPC', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+
+    assert 'webhook' in sent, 'webhook not fired on supersede'
+    assert 'email' in sent, 'no email sent on supersede'
+    assert sent['email'][0] == ['it@example.com']
+    assert '3015' in sent['email'][1] and 'OLDPC' in sent['email'][1]
+
+    # The link must be keyed on computerid: /pcs/:id resolves through
+    # GET /api/computers/<computerid>, so an assetid opens the wrong PC.
+    from plugins.computers.models import Computer
+    newpc = Computer.query.filter_by(hostname='NEWPC').first()
+    assert '/pcs/{}"'.format(newpc.computerid) in sent['email'][2]
+
+
+def test_alert_failure_never_breaks_the_collector(client, db, collector_key,
+                                                  computer_assettype,
+                                                  machine_3015, monkeypatch):
+    """A broken mail server must not stop a bay reporting its inventory."""
+    import shopdb.api as shopdbapi
+
+    def boom(*a, **k):
+        raise RuntimeError('smtp down')
+    monkeypatch.setattr(shopdbapi, 'send_email', boom)
+    monkeypatch.setattr(shopdbapi, 'send_webhook', boom)
+
+    client.post('/api/collector/computers',
+                json={'hostname': 'OLDPC', 'machinenumber': '3015'},
+                headers={'X-API-Key': collector_key})
+    r = client.post('/api/collector/computers',
+                    json={'hostname': 'NEWPC', 'machinenumber': '3015'},
+                    headers={'X-API-Key': collector_key})
+    assert r.status_code == 200
+    assert len(_links(machine_3015.assetid)) == 1

tests/test_plugins/test_backups.py

@@ -710,3 +710,50 @@ def test_panel_label_is_rendered_in_the_site_zone(bk_app, bk_plugin):
         revision = _db.session.get(BackupRevision, result['backuprevisionid'])
         # 16:30 UTC on 2026-08-07 is 12:30 EDT.
         assert '12:30' in _label(revision)
+
+
+# =============================================================================
+# REGEDIT4 / part marker exports
+# =============================================================================
+
+REGEDIT4REG = (
+    'REGEDIT4\r\n\r\n'
+    r'[HKEY_LOCAL_MACHINE\SOFTWARE\GE Aircraft Engines\DNC\General]' '\r\n'
+    '"MachineNo"="WJPRT"\r\n'
+    '"Cnc"="MARKER"\r\n\r\n'
+    r'[HKEY_LOCAL_MACHINE\SOFTWARE\GE Aircraft Engines\DNC\Mark]' '\r\n'
+    '"Port Id"="COM4"\r\n'
+    '"Baud"="9600"\r\n'
+    '"KRelay1"=\r\n'
+)
+
+
+def test_regedit4_header_is_accepted():
+    """The part marker's per-machine file is REGEDIT4, not 'Windows Registry
+    Editor Version 5.00'."""
+    projection = ntlars.parse(REGEDIT4REG.encode('utf-8'))
+    assert registry.NtlarsKind.embeddedmachineno(projection) == 'WJPRT'
+
+
+def test_value_with_an_empty_right_hand_side_does_not_fail_the_file():
+    """A real export contains "KRelay1=" with nothing after the sign. Raising on
+    it would mean that machine could never be backed up at all."""
+    projection = ntlars.parse(REGEDIT4REG.encode('utf-8'))
+    mark = next(k for k in projection['keys'] if k['path'] == 'Mark')
+    assert mark['values']['KRelay1'] == {'type': 'REG_SZ', 'data': ''}
+
+
+def test_part_marker_serial_port_survives_the_roundtrip():
+    """The COM port the marker hangs off is the point of keeping this config."""
+    first = ntlars.parse(REGEDIT4REG.encode('utf-8'))
+    second = ntlars.parse(ntlars.render(first, dialect='wow6432node'))
+    mark = next(k for k in second['keys'] if k['path'] == 'Mark')
+    assert mark['values']['Port Id']['data'] == 'COM4'
+
+
+def test_a_marker_export_shows_the_mark_tab():
+    card = dncinfo.build(ntlars.parse(REGEDIT4REG.encode('utf-8')), assetid=0)
+    labels = [s['label'] for s in card['sections']]
+    assert any('MARK' in label for label in labels)
+    mark = next(s for s in card['sections'] if 'MARK' in s['label'])
+    assert any(f['label'] == 'Port Id' and f['value'] == 'COM4' for f in mark['fields'])