diff --git a/plugins/computers/api/routes.py b/plugins/computers/api/routes.py index fc9f26a..94e3b82 100644 --- a/plugins/computers/api/routes.py +++ b/plugins/computers/api/routes.py @@ -639,6 +639,17 @@ def update_computer(computer_id: int): asset = comp.asset + # An UPDATE may not blank the asset number. Create validates it and the + # column is NOT NULL, but the conflict check below only runs when the value + # DIFFERS, and '' never collides with anything - so a payload carrying an + # empty assetnumber wrote it straight through. A form that loaded blank + # (a failed reference load, a partial fetch) then saved the blank over a + # real record. + if 'assetnumber' in data and not (data['assetnumber'] or '').strip(): + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'assetnumber cannot be empty') + # Check for conflicting assetnumber if 'assetnumber' in data and data['assetnumber'] != asset.assetnumber: if Asset.query.filter_by(assetnumber=data['assetnumber']).first(): diff --git a/plugins/computers/plugin.py b/plugins/computers/plugin.py index 4d55054..e826c42 100644 --- a/plugins/computers/plugin.py +++ b/plugins/computers/plugin.py @@ -790,7 +790,7 @@ class ComputersPlugin(BasePlugin): return bool(row) and row[0] == spec['typename'] def _ensure_device_rows(self, deviceasset, spec, pcasset, controls, label, - warnings): + warnings, active=True): """Get-or-create the extension row and the PC -> device control link. Shared by the named-device and minted paths: an adopted asset may have @@ -819,7 +819,7 @@ class ComputersPlugin(BasePlugin): targetassetid=deviceasset.assetid, relationshiptypeid=controls.relationshiptypeid).first() if controlrow is not None: - controlrow.isactive = True + controlrow.isactive = active if not controlrow.label: controlrow.label = label else: @@ -828,7 +828,58 @@ class ComputersPlugin(BasePlugin): targetassetid=deviceasset.assetid, relationshiptypeid=controls.relationshiptypeid, label=label, - isactive=True)) + isactive=active)) + + def _device_incumbents_yield(self, deviceasset, pcasset, label, warnings): + """Settle a device already controlled by a DIFFERENT PC. + + A physical instrument or marker hangs off one PC. Before this, both + paths only ever looked at links whose source was THIS PC, so a bay + naming a device another PC still holds produced two active `controls` + rows with no warning - the replaced PC kept its link forever, and a + file copied to a second bay claimed the device silently. + + The machine link settled this years ago and this reuses its rule rather + than inventing a second one: an incumbent that has gone quiet past the + claim window, or been moved off In Use, has yielded, and its link is + ARCHIVED (never deleted, so "which PC drove this in June" stays + answerable). An incumbent still alive keeps the device, and the + challenger is recorded dormant instead of contesting a link that can + only have one holder. + + Returns True when this PC may hold the device actively. + """ + from shopdb.api import AssetRelationship, RelationshipType, Asset + + controls = RelationshipType.query.filter_by( + relationshiptype='controls').first() + if not controls or not deviceasset or not pcasset: + return True + + others = AssetRelationship.query.filter( + AssetRelationship.targetassetid == deviceasset.assetid, + AssetRelationship.relationshiptypeid == controls.relationshiptypeid, + AssetRelationship.label == label, + AssetRelationship.isactive.is_(True), + AssetRelationship.sourceassetid != pcasset.assetid, + ).all() + if not others: + return True + + holding = [] + for rel in others: + if self._incumbent_has_yielded(rel.sourceassetid): + rel.isactive = False + else: + holder = db.session.get(Asset, rel.sourceassetid) + holding.append(holder.assetnumber if holder else rel.sourceassetid) + + if holding: + warnings.append( + '{} is still controlled by {}; recorded but not activated' + .format(deviceasset.assetnumber, ', '.join(str(h) for h in holding))) + return False + return True def _sync_partmarker(self, comp, pctype, machinenumber, warnings, deviceid=None): @@ -892,6 +943,16 @@ class ComputersPlugin(BasePlugin): label = spec['label'] # --- 1. an explicitly named device wins over everything ------------- + # + # NAMING A DEVICE IS A COMMITMENT. If the bay says which device it + # drives and that name does not resolve, the answer is to link nothing + # and say so - NOT to fall through and mint -PARTMARKER, which is + # the hostname-derived twin this whole path exists to prevent. The + # warning used to be true about the typo'd number and false about the + # twin: it warned, then minted anyway. + # + # Silence still means "work it out", so a bay with no file keeps the + # reuse-then-mint behaviour it has always had. named = (deviceid or '').strip() namedasset = None if named: @@ -901,15 +962,16 @@ class ComputersPlugin(BasePlugin): # device that nobody can account for. warnings.append( 'no asset for device {!r}; not linked'.format(named)) - elif not self._is_device_of_type(candidate, spec): + return [] + if not self._is_device_of_type(candidate, spec): # The name resolved, but not to this pc-type's device. Refuse # and say which asset it hit, rather than filing an unrelated # asset under a device label the collector also owns. warnings.append( 'asset {!r} is not a {}; not linked'.format( candidate.assetnumber, spec['typename'])) - else: - namedasset = candidate + return [] + namedasset = candidate existing = AssetRelationship.query.filter( AssetRelationship.sourceassetid == pcasset.assetid, @@ -920,6 +982,11 @@ class ComputersPlugin(BasePlugin): or (existing[0] if existing else None) if namedasset is not None: + # The file is authoritative FOR THIS PC, but the device may still be + # held by another one. Settle that first: a yielded incumbent is + # archived, a live one keeps it and this link is recorded dormant. + mayhold = self._device_incumbents_yield( + namedasset, pcasset, label, warnings) # The file is authoritative. A PC that previously minted its own # marker and is now told the real one keeps only the named link; # the stale link is archived by the one-marker-per-PC sweep below. @@ -929,7 +996,7 @@ class ComputersPlugin(BasePlugin): if reuse is not None: reuse.isactive = True self._ensure_device_rows(markerasset, spec, pcasset, controls, - label, warnings) + label, warnings, active=mayhold) elif reuse: reuse.isactive = True markerasset = db.session.get(Asset, reuse.targetassetid) @@ -1280,10 +1347,14 @@ class ComputersPlugin(BasePlugin): candidate = self._asset_by_number(named) if candidate is None: # Warn rather than invent: a typo in the file must not mint a - # phantom instrument that nobody can account for. + # phantom instrument that nobody can account for. RETURN, do + # not fall through - minting -CMM here would create the + # hostname-derived twin this resolution order exists to avoid, + # while the warning claimed nothing was linked. warnings.append( 'no asset for measuring tool {!r}; not linked'.format(named)) - elif candidate.assetid not in self._measuringtool_assetids(): + return [] + if candidate.assetid not in self._measuringtool_assetids(): # The name resolved, but not to an instrument. measuringtool-id # .txt holding a machine number would otherwise link the PC to # that MACHINE under a measuring-tool label - a link that reads @@ -1292,8 +1363,8 @@ class ComputersPlugin(BasePlugin): warnings.append( 'asset {!r} is not a measuring tool; not linked'.format( candidate.assetnumber)) - else: - adopted = candidate + return [] + adopted = candidate # --- 2. a prior collector link (reactivate + retype) ----------------- reuse = next((rel for rel in existing if rel.isactive), None) \ @@ -1307,6 +1378,11 @@ class ComputersPlugin(BasePlugin): pcasset, controls, machinenumber) if adopted is not None: + # Settle any OTHER PC still holding this instrument first: a yielded + # incumbent is archived, a live one keeps it and this link is + # recorded dormant rather than becoming a second active holder. + mayhold = self._device_incumbents_yield( + adopted, pcasset, MEASURINGTOOL_LINK_ORIGIN, warnings) # CLAIM the existing link rather than adding a second one. A row for # (pc, tool, controls) usually already exists - that is how the tool # was found - and assetrelationships is unique on exactly that @@ -1330,13 +1406,14 @@ class ComputersPlugin(BasePlugin): # is ours from here, so a later cycle reuses it instead of # minting. claimed.label = MEASURINGTOOL_LINK_ORIGIN - claimed.isactive = True + claimed.isactive = mayhold else: db.session.add(AssetRelationship( sourceassetid=pcasset.assetid, targetassetid=adopted.assetid, relationshiptypeid=controls.relationshiptypeid, - label=MEASURINGTOOL_LINK_ORIGIN)) + label=MEASURINGTOOL_LINK_ORIGIN, + isactive=mayhold)) if adopted.measuringtool and tooltype: adopted.measuringtool.measuringtooltypeid = \ tooltype.measuringtooltypeid diff --git a/plugins/machines/api/routes.py b/plugins/machines/api/routes.py index b6af672..7667add 100644 --- a/plugins/machines/api/routes.py +++ b/plugins/machines/api/routes.py @@ -437,6 +437,17 @@ def update_machine(machine_id: int): asset = mach.asset + # An UPDATE may not blank the asset number. Create validates it and the + # column is NOT NULL, but the conflict check below only runs when the value + # DIFFERS, and '' never collides with anything - so a payload carrying an + # empty assetnumber wrote it straight through. A form that loaded blank + # (a failed reference load, a partial fetch) then saved the blank over a + # real record. + if 'assetnumber' in data and not (data['assetnumber'] or '').strip(): + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'assetnumber cannot be empty') + # Check for conflicting assetnumber if 'assetnumber' in data and data['assetnumber'] != asset.assetnumber: if Asset.query.filter_by(assetnumber=data['assetnumber']).first(): diff --git a/plugins/machines/frontend/views/MachineForm.vue b/plugins/machines/frontend/views/MachineForm.vue index 1b396df..efe0846 100644 --- a/plugins/machines/frontend/views/MachineForm.vue +++ b/plugins/machines/frontend/views/MachineForm.vue @@ -7,6 +7,11 @@
Loading...
+ +
{{ error }}
+

Identity

@@ -425,6 +430,10 @@ const statuses = ref([]) const vendors = ref([]) const locations = ref([]) const models = ref([]) +// Set when the record itself could not be loaded, as opposed to a reference +// list failing. The form is withheld entirely, because an empty edit form is +// indistinguishable from a record whose fields are genuinely empty. +const loadFailed = ref(false) const businessunits = ref([]) const pcs = ref([]) const relationshipTypes = ref([]) @@ -506,9 +515,29 @@ onMounted(async () => { relationshipTypeId.value = controlsType.relationshiptypeid } - // Load machine if editing + // Load machine if editing. + // + // ITS OWN try/catch, deliberately. This used to sit inside the same block + // as the eight reference loads above, so ONE transient failure among them - + // a page of listAll() timing out on a busy collector cycle - rejected the + // whole thing and rendered a fully editable EDIT form with every field + // BLANK, including the required Asset Number, next to a Save button. Typing + // a number and saving then wrote the blank-loaded values over a real + // machine. The reference lists degrade to empty dropdowns; the record must + // not degrade at all. if (isEdit.value) { - const response = await machinesApi.get(route.params.id) + let response + try { + response = await machinesApi.get(route.params.id) + } catch (err) { + console.error('Error loading machine:', err) + error.value = 'Could not load this machine. Nothing has been changed - ' + + 'reload the page rather than saving, or the blank form would ' + + 'overwrite the record.' + loadFailed.value = true + loading.value = false + return + } const data = response.data.data currentAssetId.value = data.assetid || null currentMachine.value = data diff --git a/plugins/measuringtools/api/routes.py b/plugins/measuringtools/api/routes.py index b038445..9ee2748 100644 --- a/plugins/measuringtools/api/routes.py +++ b/plugins/measuringtools/api/routes.py @@ -316,6 +316,17 @@ def update_tool(tool_id: int): data = request.get_json() or {} asset = tool.asset + # An UPDATE may not blank the asset number. Create validates it and the + # column is NOT NULL, but the conflict check below only runs when the value + # DIFFERS, and '' never collides with anything - so a payload carrying an + # empty assetnumber wrote it straight through. A form that loaded blank + # (a failed reference load, a partial fetch) then saved the blank over a + # real record. + if 'assetnumber' in data and not (data['assetnumber'] or '').strip(): + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'assetnumber cannot be empty') + if 'assetnumber' in data and data['assetnumber'] != asset.assetnumber: if Asset.query.filter_by(assetnumber=data['assetnumber']).first(): return error_response(ErrorCodes.CONFLICT, diff --git a/plugins/network/api/routes.py b/plugins/network/api/routes.py index 4c7780b..f8e1f93 100644 --- a/plugins/network/api/routes.py +++ b/plugins/network/api/routes.py @@ -483,6 +483,17 @@ def update_network_device(device_id: int): asset = netdev.asset + # An UPDATE may not blank the asset number. Create validates it and the + # column is NOT NULL, but the conflict check below only runs when the value + # DIFFERS, and '' never collides with anything - so a payload carrying an + # empty assetnumber wrote it straight through. A form that loaded blank + # (a failed reference load, a partial fetch) then saved the blank over a + # real record. + if 'assetnumber' in data and not (data['assetnumber'] or '').strip(): + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'assetnumber cannot be empty') + # Check for conflicting assetnumber if 'assetnumber' in data and data['assetnumber'] != asset.assetnumber: if Asset.query.filter_by(assetnumber=data['assetnumber']).first(): diff --git a/plugins/printers/api/asset_routes.py b/plugins/printers/api/asset_routes.py index 158ffe8..574c0d4 100644 --- a/plugins/printers/api/asset_routes.py +++ b/plugins/printers/api/asset_routes.py @@ -1963,6 +1963,17 @@ def update_printer(printer_id: int): asset = printer.asset + # An UPDATE may not blank the asset number. Create validates it and the + # column is NOT NULL, but the conflict check below only runs when the value + # DIFFERS, and '' never collides with anything - so a payload carrying an + # empty assetnumber wrote it straight through. A form that loaded blank + # (a failed reference load, a partial fetch) then saved the blank over a + # real record. + if 'assetnumber' in data and not (data['assetnumber'] or '').strip(): + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'assetnumber cannot be empty') + # Check for conflicting assetnumber if 'assetnumber' in data and data['assetnumber'] != asset.assetnumber: if Asset.query.filter_by(assetnumber=data['assetnumber']).first(): diff --git a/shopdb/core/api/assets.py b/shopdb/core/api/assets.py index fdbd3b4..57a1598 100644 --- a/shopdb/core/api/assets.py +++ b/shopdb/core/api/assets.py @@ -506,6 +506,17 @@ def update_asset(asset_id: int): if not data: return error_response(ErrorCodes.VALIDATION_ERROR, 'No data provided') + # An UPDATE may not blank the asset number. Create validates it and the + # column is NOT NULL, but the conflict check below only runs when the value + # DIFFERS, and '' never collides with anything - so a payload carrying an + # empty assetnumber wrote it straight through. A form that loaded blank + # (a failed reference load, a partial fetch) then saved the blank over a + # real record. + if 'assetnumber' in data and not (data['assetnumber'] or '').strip(): + return error_response( + ErrorCodes.VALIDATION_ERROR, + 'assetnumber cannot be empty') + # Check for conflicting assetnumber if 'assetnumber' in data and data['assetnumber'] != asset.assetnumber: if Asset.query.filter_by(assetnumber=data['assetnumber']).first(): diff --git a/tests/test_plugins/test_collector_deviceid.py b/tests/test_plugins/test_collector_deviceid.py index 5a721af..0f2f84c 100644 --- a/tests/test_plugins/test_collector_deviceid.py +++ b/tests/test_plugins/test_collector_deviceid.py @@ -102,8 +102,25 @@ def _controlled(pcname, label): # --------------------------------------------------------------- part markers +def _retire(db, hostname): + """Move a PC off In Use, which is the one-step way to yield its device.""" + from shopdb.core.models import AssetStatus + retired = AssetStatus.query.filter_by(status='Retired').first() + if retired is None: + retired = AssetStatus(status='Retired') + db.session.add(retired) + db.session.flush() + pc = Asset.query.filter(Asset.assetnumber.ilike(hostname)).first() + pc.statusid = retired.statusid + db.session.commit() + + def test_a_pc_swap_does_not_mint_a_second_marker(client, db, rig, collector_key): - """THE case this exists for. Same physical marker, two different PCs.""" + """THE case this exists for. Same physical marker, two different PCs. + + Neither PC mints a twin. Who HOLDS the marker is settled separately, by the + two tests below - this one pins only that the physical unit stays one row. + """ _asset(db, '0613') marker = _marker(db, 'PM-0613-A') @@ -120,12 +137,51 @@ def test_a_pc_swap_does_not_mint_a_second_marker(client, db, rig, collector_key) assert Asset.query.filter_by(assetnumber='FMARK100-PARTMARKER').first() is None assert Asset.query.filter_by(assetnumber='FMARK200-PARTMARKER').first() is None - assert _controlled('FMARK200', 'collector:partmarker') == ['PM-0613-A'] assert Asset.query.filter_by(assetnumber='PM-0613-A').count() == 1 assert marker.assetid == Asset.query.filter_by( assetnumber='PM-0613-A').first().assetid +def test_a_live_incumbent_keeps_the_marker_and_the_challenger_is_dormant( + client, db, rig, collector_key): + """Two PCs naming one device must not both hold it actively. + + Before this, neither device path looked at who else held the target, so a + replaced PC kept its link forever and a copied asset-id.txt claimed the same + marker from every bay, silently. + """ + _asset(db, '0613') + _marker(db, 'PM-0613-A') + _report(client, collector_key, 'FMARK100', pctype='gea-shopfloor-partmarker', + machinenumber='0613', deviceid='PM-0613-A') + resp = _report(client, collector_key, 'FMARK200', + pctype='gea-shopfloor-partmarker', + machinenumber='0613', deviceid='PM-0613-A') + + assert _controlled('FMARK100', 'collector:partmarker') == ['PM-0613-A'] + assert _controlled('FMARK200', 'collector:partmarker') == [] + warnings = ' '.join(resp.get_json()['data'].get('warnings', [])) + assert 'FMARK100' in warnings and 'PM-0613-A' in warnings + + +def test_handover_completes_once_the_incumbent_yields(client, db, rig, + collector_key): + """The swap case as it actually happens: the old PC is retired or goes quiet.""" + _asset(db, '0613') + _marker(db, 'PM-0613-A') + _report(client, collector_key, 'FMARK100', pctype='gea-shopfloor-partmarker', + machinenumber='0613', deviceid='PM-0613-A') + _retire(db, 'FMARK100') + + _report(client, collector_key, 'FMARK200', pctype='gea-shopfloor-partmarker', + machinenumber='0613', deviceid='PM-0613-A') + + assert _controlled('FMARK200', 'collector:partmarker') == ['PM-0613-A'] + # Archived, never deleted: "which PC drove this in June" stays answerable. + assert _controlled('FMARK100', 'collector:partmarker') == [] + assert AssetRelationship.query.filter_by(label='collector:partmarker').count() >= 2 + + def test_without_the_file_a_swap_still_mints_the_old_way(client, db, rig, collector_key): """The unfixed behaviour, pinned so the file's value stays visible.""" @@ -150,6 +206,11 @@ def test_an_unknown_device_warns_and_links_nothing(client, db, rig, assert Asset.query.filter_by(assetnumber='PM-TYPO').first() is None warnings = ' '.join(resp.get_json()['data'].get('warnings', [])) assert 'PM-TYPO' in warnings + # LINKS NOTHING, which is what the name claims. This used to warn and then + # mint FMARK500-PARTMARKER anyway - the twin the whole path exists to stop - + # and the test passed because it only checked that PM-TYPO was not created. + assert Asset.query.filter_by(assetnumber='FMARK500-PARTMARKER').first() is None + assert _controlled('FMARK500', 'collector:partmarker') == [] def test_a_device_of_the_wrong_type_is_refused(client, db, rig, collector_key): @@ -160,7 +221,8 @@ def test_a_device_of_the_wrong_type_is_refused(client, db, rig, collector_key): pctype='gea-shopfloor-partmarker', machinenumber='0617', deviceid='PLAIN-MACHINE') assert resp.status_code in (200, 201) - assert _controlled('FMARK600', 'collector:partmarker') != ['PLAIN-MACHINE'] + assert _controlled('FMARK600', 'collector:partmarker') == [] + assert Asset.query.filter_by(assetnumber='FMARK600-PARTMARKER').first() is None warnings = ' '.join(resp.get_json()['data'].get('warnings', [])) assert 'PLAIN-MACHINE' in warnings