From ebb86ccf3b2e947ba85a5d259ef7c0e8b1368abd Mon Sep 17 00:00:00 2001 From: Cam Proudlock Date: Thu, 20 Aug 2026 17:09:02 -0400 Subject: [PATCH] Blank asset numbers, contested device claims, and honest toner forecasting --- plugins/computers/api/routes.py | 11 ++ plugins/computers/plugin.py | 103 +++++++++++-- plugins/machines/api/routes.py | 11 ++ .../machines/frontend/views/MachineForm.vue | 33 +++- plugins/measuringtools/api/routes.py | 11 ++ plugins/network/api/routes.py | 11 ++ plugins/printers/api/asset_routes.py | 11 ++ .../printers/frontend/views/TonerForecast.vue | 21 ++- plugins/printers/services/supply_history.py | 144 ++++++++++++++++-- shopdb/core/api/assets.py | 11 ++ tests/test_plugins/test_collector_deviceid.py | 68 ++++++++- tests/test_plugins/test_supply_history.py | 72 +++++++++ 12 files changed, 473 insertions(+), 34 deletions(-) diff --git a/plugins/computers/api/routes.py b/plugins/computers/api/routes.py index fc9f26a4..94e3b828 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 4d55054a..e826c422 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 b6af6721..7667add1 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 1b396df8..efe0846c 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 b0384454..9ee2748d 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 4c7780b5..f8e1f93d 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 158ffe82..574c0d43 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/plugins/printers/frontend/views/TonerForecast.vue b/plugins/printers/frontend/views/TonerForecast.vue index f9b4e2e0..98841259 100644 --- a/plugins/printers/frontend/views/TonerForecast.vue +++ b/plugins/printers/frontend/views/TonerForecast.vue @@ -90,7 +90,7 @@ Level Runs out Rate - Changed + Replacements @@ -123,11 +123,20 @@ {{ daysText(c) }} - {{ c.burnrateperday != null ? c.burnrateperday + '%/day' : '-' }} + + - {{ c.replacements || 0 }} - / {{ c.basisdays }}d + + + @@ -424,6 +433,10 @@ onMounted(load) .days.empty { color: var(--danger); } .days.soon { color: var(--warning); } .small { font-size: 0.8rem; } + +/* Marks a rate the intervals do not agree on. Deliberately quiet - it qualifies + the number beside it rather than competing with the urgency bands. */ +.unstable { margin-left: 2px; font-weight: 600; cursor: help; } .empty-state { padding: 2rem; text-align: center; } .footnote { margin-top: 1rem; font-size: 0.85rem; } diff --git a/plugins/printers/services/supply_history.py b/plugins/printers/services/supply_history.py index 9917420e..e3e6bda3 100644 --- a/plugins/printers/services/supply_history.py +++ b/plugins/printers/services/supply_history.py @@ -20,6 +20,19 @@ from datetime import datetime, timezone # new cartridge rather than noise. REPLACEMENT_RISE = 10 +# A replacement must also LAND high. A fresh cartridge reads near full, so a +# rise that stops mid-range is a gauge bouncing, not a swap. Without this the +# count was any +10 between readings, and the two common noise shapes both +# scored: a supply reading 0 or near-0 while it was out of the machine, then the +# real level again (0 -> 60 counts as +60), and a coarse gauge ticking back up +# after a reseat or a power cycle. That is how one cartridge claimed five +# changes in ninety days. +# +# A site that fits PART-USED cartridges will under-count with this rule. That is +# the right way round: a missed swap widens the run and slows the estimate, +# while a phantom swap resets the run and throws the estimate away entirely. +NEW_CARTRIDGE_LEVEL = 80 + # Below this many readings a slope is arithmetic, not evidence. Two points # through a coarse gauge can "prove" any rate at all. MIN_POINTS_FOR_ESTIMATE = 4 @@ -29,6 +42,12 @@ MIN_POINTS_FOR_ESTIMATE = 4 # says "never", which is worse than saying nothing. MIN_DROP_FOR_ESTIMATE = 2 +# How far the fastest and slowest intervals may differ before a single rate +# stops being a fair summary. A cartridge that ran at 10 percent/day for two +# days and 0.2 percent/day since is not described by any one number, and a +# precise-looking figure invites more trust than it has earned. +RATE_SPREAD_FACTOR = 5 + # At or below this, the cartridge is done and the arithmetic stops being the # useful answer. A supply sitting at 1% that drains a tenth of a point a day # computes to ten days; a printer at 1% is out of toner as far as anyone @@ -58,18 +77,65 @@ def normalise(points): continue out.append((datetime.fromtimestamp(seconds, tz=timezone.utc), level)) out.sort(key=lambda p: p[0]) + return drop_spikes(out) + + +def drop_spikes(points, rise=REPLACEMENT_RISE): + """Remove one-reading dips that RECOVER to where they came from. + + A single reading far below both neighbours, then a recovery, is a big + upward step that scores as a cartridge change. That is how a cartridge + claimed five changes in ninety days. + + NOT a Zabbix timeout: an item that does not answer records nothing, it does + not write a zero. The dip is a value the device really reported - a supply + pulled out to be shaken and reseated, a door open mid-poll, or a site whose + preprocessing maps the Printer MIB's "unknown" sentinels (-1/-2/-3, which + cannot land in the unsigned history table) onto 0. + + The filter keys on SHAPE rather than cause, which is why it holds for all of + them: a level that comes back to where it was did not get a new cartridge. + + Only a dip that comes BACK to roughly its previous level is removed. A + genuine near-empty reading before a swap (30, 5, 100) does not recover - it + jumps to full - so it is kept, and the swap after it still counts. + """ + if len(points) < 3: + return points + out = [points[0]] + for index in range(1, len(points) - 1): + previous = points[index - 1][1] + current = points[index][1] + following = points[index + 1][1] + dipped = (previous - current) >= rise and (following - current) >= rise + recovered = abs(following - previous) <= rise + if dipped and recovered: + continue + out.append(points[index]) + out.append(points[-1]) return out -def find_replacements(points, rise=REPLACEMENT_RISE): +def _is_replacement(previous, current, rise=REPLACEMENT_RISE, + newlevel=NEW_CARTRIDGE_LEVEL): + """One definition of "a cartridge was changed", used by every caller. + + The rise must be big enough to clear gauge noise AND land near full, which + is what a new cartridge reads. Both conditions, because either alone admits + a shape that is not a swap. + """ + return (current - previous) >= rise and current >= newlevel + + +def find_replacements(points, rise=REPLACEMENT_RISE, + newlevel=NEW_CARTRIDGE_LEVEL): """Timestamps where the level jumped up - one per cartridge change. - Returns [] for a series that only falls. A rise smaller than `rise` is - treated as noise, not a replacement. + Returns [] for a series that only falls. """ replacements = [] for (_, previous), (when, current) in zip(points, points[1:]): - if current - previous >= rise: + if _is_replacement(previous, current, rise, newlevel): replacements.append(when) return replacements @@ -84,28 +150,78 @@ def current_run(points, rise=REPLACEMENT_RISE): return [] start = 0 for index in range(1, len(points)): - if points[index][1] - points[index - 1][1] >= rise: + if _is_replacement(points[index - 1][1], points[index][1], rise): start = index return points[start:] +def interval_rates(points): + """Percent-per-day for each consecutive pair, falling intervals only. + + A rise inside a run is gauge noise (a swap would have ended the run), and + a flat interval is real information - a cartridge that did not move - so it + stays in at zero. + """ + rates = [] + for (whenprev, prev), (when, current) in zip(points, points[1:]): + days = (when - whenprev).total_seconds() / 86400 + if days <= 0: + continue + drop = prev - current + if drop < 0: + continue + rates.append(drop / days) + return rates + + +def _median(values): + ordered = sorted(values) + count = len(ordered) + if not count: + return None + middle = count // 2 + if count % 2: + return ordered[middle] + return (ordered[middle - 1] + ordered[middle]) / 2 + + def burn_rate(points): """Percent consumed per day over these readings, or None. + THE MEDIAN OF THE PER-INTERVAL RATES, not the slope between the first and + last reading. Two endpoints cannot tell "steady" from "burst then stopped": + a cartridge that lost 20 percent in two days and then barely moved for a + month reads as 0.83 percent/day forever after, so the report keeps promising + it will run out long after printing slowed. The burst is one interval among + many to a median, and one of two points to a secant. + None means "no honest estimate": too few readings, no elapsed time, or a drop too small to distinguish from a gauge that has not moved yet. """ if len(points) < MIN_POINTS_FOR_ESTIMATE: return None - first_when, first_level = points[0] - last_when, last_level = points[-1] - days = (last_when - first_when).total_seconds() / 86400 - if days <= 0: + total_days = (points[-1][0] - points[0][0]).total_seconds() / 86400 + if total_days <= 0: return None - drop = first_level - last_level - if drop < MIN_DROP_FOR_ESTIMATE: + # The overall drop still gates the estimate: a gauge sitting on one plateau + # has not proved anything yet, whatever the intervals say. + if points[0][1] - points[-1][1] < MIN_DROP_FOR_ESTIMATE: return None - return drop / days + rate = _median(interval_rates(points)) + if not rate: + # Every interval flat or rising, yet the run dropped overall - the + # movement is all in intervals the median discarded. Fall back to the + # whole-run slope rather than reporting nothing. + return (points[0][1] - points[-1][1]) / total_days + return rate + + +def rate_is_unstable(points, factor=RATE_SPREAD_FACTOR): + """True when the intervals disagree enough that one number oversells it.""" + rates = [r for r in interval_rates(points) if r > 0] + if len(rates) < 2: + return False + return max(rates) >= min(rates) * factor def analyse(points, rise=REPLACEMENT_RISE, currentlevel=None): @@ -131,6 +247,7 @@ def analyse(points, rise=REPLACEMENT_RISE, currentlevel=None): points = normalise(points) result = { 'currentlevel': currentlevel, 'daysleft': None, 'burnrateperday': None, + 'rateunstable': False, 'reason': None, 'replacements': 0, 'lastreplaced': None, 'basisdays': 0, 'points': [], } @@ -182,6 +299,9 @@ def analyse(points, rise=REPLACEMENT_RISE, currentlevel=None): result['burnrateperday'] = round(rate, 2) result['daysleft'] = max(0, int(level / rate)) + # Say so when the intervals disagree wildly. The number is still the best + # estimate available; the flag stops it reading as a measurement. + result['rateunstable'] = rate_is_unstable(run) return result diff --git a/shopdb/core/api/assets.py b/shopdb/core/api/assets.py index fdbd3b42..57a1598d 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 5a721afd..0f2f84cc 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 diff --git a/tests/test_plugins/test_supply_history.py b/tests/test_plugins/test_supply_history.py index 396d8d46..90d17bfd 100644 --- a/tests/test_plugins/test_supply_history.py +++ b/tests/test_plugins/test_supply_history.py @@ -267,3 +267,75 @@ def test_forecast_endpoint_says_so_when_zabbix_is_absent(client, db): assert 'not configured' in body['reason'] assert body['cartridges'] == [] assert body['orderlist'] == [] + + +# -------------------------------------------------------------------------- +# Noise that used to read as cartridge changes, and bursts that used to bias +# the rate for the life of the cartridge. Both were reported from the floor: +# "5 changes in 90 days, that's hard to believe", and a cartridge that dropped +# 20 percent in two days then barely moved. +# -------------------------------------------------------------------------- + +def test_a_poll_returning_zero_is_not_a_cartridge_change(): + """0 then a real level is an SNMP error or a calibrating printer. + + The rise is +60, which cleared the old threshold on its own. It does not + land near full, so it is not a swap. + """ + points = normalise(series([70, 60, 0, 60, 55, 50])) + assert find_replacements(points) == [] + + +def test_a_gauge_ticking_back_up_mid_range_is_not_a_change(): + """A coarse gauge after a reseat or a power cycle. Lands at 55, not full.""" + points = normalise(series([70, 60, 45, 55, 50, 45])) + assert find_replacements(points) == [] + + +def test_a_real_swap_is_still_counted(): + """Near-empty to near-full. The shape a cartridge change actually makes.""" + points = normalise(series([30, 15, 5, 100, 95, 90])) + assert len(find_replacements(points)) == 1 + + +def test_the_run_starts_at_the_real_swap_not_at_the_noise(): + """current_run and find_replacements must agree on what a change is. + + They read the same predicate now; when they did not, a phantom rise reset + the run and threw away the history the estimate needed. + """ + points = normalise(series([90, 0, 85, 80, 75, 70])) + assert find_replacements(points) == [] + assert len(current_run(points)) == len(points) + + +def test_an_early_burst_does_not_dominate_the_rate_forever(): + """20 percent in two days, then a month of almost nothing. + + The endpoint slope reads the burst forever: (100-75)/30 = 0.83 %/day, so + the report keeps promising the cartridge runs out long after printing + stopped. The median sees one fast interval among many quiet ones. + """ + levels = [100, 90, 80] + [80 - i * 0.2 for i in range(1, 28)] + rate = burn_rate(normalise(series(levels))) + assert rate is not None + assert rate < 1.0, rate + detail = analyse(series(levels)) + assert detail['rateunstable'] is True + + +def test_a_steady_cartridge_is_not_flagged_unstable(): + detail = analyse(series([100, 95, 90, 85, 80, 75, 70])) + assert detail['burnrateperday'] == 5.0 + assert detail['rateunstable'] is False + + +def test_a_real_near_empty_reading_before_a_swap_is_kept(): + """30, 5, 100 is a cartridge run to the end and changed - not a spike. + + The dip filter must not eat it: the 5 does not RECOVER to 30, it jumps to + full, which is the shape of a swap rather than of a bad poll. + """ + points = normalise(series([40, 30, 5, 100, 95, 90])) + assert 5.0 in [level for _, level in points] + assert len(find_replacements(points)) == 1