diff --git a/CHANGELOG.md b/CHANGELOG.md index e128b33..832c005 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/plugins/computers/plugin.py b/plugins/computers/plugin.py index d6c6dbc..19e9a01 100644 --- a/plugins/computers/plugin.py +++ b/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 = ('

View {1}

'.format(link, newname) + if link else '') + html = ( + '

{0} is now reporting machine ' + '{1}, which was previously driven by ' + '{2}.

' + '

The old link has been archived. {2} has NOT had its status ' + 'changed - set it to Inventory, In Repair or Retired as ' + 'appropriate.

{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. diff --git a/tests/test_core/test_collector_contract.py b/tests/test_core/test_collector_contract.py index e6a13dd..011f49a 100644 --- a/tests/test_core/test_collector_contract.py +++ b/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/, 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