diff --git a/plugins/printers/api/asset_routes.py b/plugins/printers/api/asset_routes.py index 574c0d4..8181a74 100644 --- a/plugins/printers/api/asset_routes.py +++ b/plugins/printers/api/asset_routes.py @@ -2109,7 +2109,9 @@ def _annotate_supply(supply, vendor_name, modelnumberid): Waste cartridge direction depends on vendor, so classification lives in the supply_parts helper. Part numbers come from the modelsupplies table. """ - level = supply.get('level', 0) + # .get('level') with no default: a supply with no reading carries None, and + # a 0 default would put back the very substitution this reports around. + level = supply.get('level') name = supply.get('name', 'Unknown') supplytype = derivesupplytype(name) color = derivecolor(name, supply.get('color')) @@ -2117,6 +2119,7 @@ def _annotate_supply(supply, vendor_name, modelnumberid): return { 'name': name, 'level': level, + 'lastseen': supply.get('lastseen'), 'color': color, 'supplytype': supplytype, 'status': cls['status'], @@ -2160,6 +2163,7 @@ def _get_low_supplies_data(): results = [] total_checked = 0 + unknown_printers = 0 for printer, asset, comm, vendor, model in unique_printers: supplies = service.getsuppliesbyip_cached(comm.ipaddress) @@ -2178,13 +2182,24 @@ def _get_low_supplies_data(): # shown it. The whole report exists to answer "what needs replacing". annotated = [] has_low = False + has_unknown = False for s in supplies: item = _annotate_supply(s, vendor_name, modelnumberid) if item['status'] == 'ok': continue + # A supply nobody could read is not a supply that needs replacing, + # so it does not join the list of things to act on. It is counted + # instead: a printer that has gone quiet should be visible as that, + # not disappear from the report and not masquerade as empty. + if item['status'] == 'unknown': + has_unknown = True + continue has_low = True annotated.append(item) + if has_unknown: + unknown_printers += 1 + if has_low: # location name for the report row # Via the relationship, the way the printers list does it. The @@ -2222,7 +2237,12 @@ def _get_low_supplies_data(): 'summary': { 'total_checked': total_checked, 'low': low_count, - 'critical': critical_count + 'critical': critical_count, + # Printers with at least one unreadable supply. They are NOT in + # 'printers' above - nothing about them needs ordering - but a + # count that only ever went up when a cartridge ran low would hide + # a fleet quietly going dark. + 'unknown': unknown_printers } } diff --git a/plugins/printers/frontend/views/PrinterDetail.vue b/plugins/printers/frontend/views/PrinterDetail.vue index a57b780..88e0550 100644 --- a/plugins/printers/frontend/views/PrinterDetail.vue +++ b/plugins/printers/frontend/views/PrinterDetail.vue @@ -215,8 +215,14 @@
{{ supply.name }} - - {{ supply.level !== null ? `${supply.level}%` : 'N/A' }} + + + {{ supply.level !== null && supply.level !== undefined ? `${supply.level}%` : 'N/A' }}
@@ -227,7 +233,7 @@ >
- {{ formatSupplyType(supply.supplytype) }} + {{ formatSupplyType(supply.supplytype) }} str: return "none" -def classifysupply(level: float, name: str, vendor: Optional[str]) -> Dict: - """Classify one supply item into ok/low/critical. +def classifysupply(level: Optional[float], name: str, + vendor: Optional[str]) -> Dict: + """Classify one supply item into ok/low/critical, or unknown. Waste cartridge fill is inverted vs a toner level: a full waste cartridge is bad. Standard vendors report waste as percent FULL (high = bad). Xerox EC/AltaLink series report waste as percent capacity REMAINING (low = bad), same direction as toner. Normalise everything to percent remaining first. + + A level of None means Zabbix has no usable reading; the status is 'unknown' + and remaining is None. Callers must not treat that as a low supply. """ lowername = (name or "").lower() iswaste = "waste" in lowername isdrum = "drum" in lowername or "imaging" in lowername isxerox = bool(vendor) and "xerox" in vendor.lower() + # No reading is its own answer, not a level. It cannot be classified ok, + # low or critical without asserting something nobody measured - and every + # one of those three is a claim the reader would act on. + if level is None: + return { + "status": "unknown", + "remaining": None, + "iswaste": iswaste, + "isdrum": isdrum, + } + if iswaste and not isxerox: remaining = 100 - level else: diff --git a/plugins/printers/services/zabbix_service.py b/plugins/printers/services/zabbix_service.py index eb5123f..da72cdd 100644 --- a/plugins/printers/services/zabbix_service.py +++ b/plugins/printers/services/zabbix_service.py @@ -208,18 +208,64 @@ class ZabbixService: continue if str(item.get("state", "0")) != "0": continue - try: - level = int(float(item.get("lastvalue", 0))) - except (ValueError, TypeError): - level = 0 + level, lastseen = self._readlevel(item) supplies.append({ "name": item.get("name", "Unknown"), "level": level, + "lastseen": lastseen, "color": self._extract_color(item), "itemid": item.get("itemid"), }) return supplies + # How old the newest reading may be and still be presented as the level. + # Generous on purpose: supply items are polled in hours, not seconds, and + # calling a slow poller "unknown" would be its own lie. This exists for the + # host that stopped answering, not for one that answers rarely. + STALE_AFTER_SECONDS = 24 * 3600 + + @classmethod + def _readlevel(cls, item): + """(level, lastseen) for one supply item; level is None when unknown. + + An item that has never collected returns lastvalue as an EMPTY STRING, + and one whose host went quiet keeps a lastvalue that is arbitrarily old. + Both used to become 0 - the empty string because float('') raises and + the handler substituted zero, the stale one because nothing ever read + lastclock, which was fetched and then ignored. + + Zero is not a safe stand-in for "no reading". It is a legitimate value + meaning the cartridge is spent, so the substitution was indistinguishable + from a real empty - and it hit every item on the host at once, which is + what a printer that has gone dark looks like. Downstream that reached + `level <= EMPTY_LEVEL`, so the forecast gave it daysleft 0 and the order + list asked purchasing to buy a full set of cartridges for a printer + nobody had measured. + + None says "unknown" and every consumer already handles it: the detail + page renders N/A, the alert loop skips it, and analyse() falls back to + the last real level in history rather than inventing one. + """ + raw = item.get("lastvalue") + if raw is None or str(raw).strip() == "": + return None, None + try: + level = int(float(raw)) + except (ValueError, TypeError): + return None, None + + try: + lastseen = int(item.get("lastclock") or 0) + except (ValueError, TypeError): + lastseen = 0 + if lastseen <= 0: + # A value with no clock cannot be aged. Trust the value and say the + # age is unknown rather than discarding a reading that may be fine. + return level, None + if time.time() - lastseen > cls.STALE_AFTER_SECONDS: + return None, lastseen + return level, lastseen + # Per item, per call. Zabbix applies `limit` to the WHOLE result set, not # per item, so the budget is multiplied by the number of items asked for. HISTORY_PER_ITEM = 2000 diff --git a/tests/test_plugins/test_supply_unknown_level.py b/tests/test_plugins/test_supply_unknown_level.py new file mode 100644 index 0000000..329b029 --- /dev/null +++ b/tests/test_plugins/test_supply_unknown_level.py @@ -0,0 +1,163 @@ +"""A supply nobody could read is unknown, not empty. + +Reported as "CSF04 now reports all 0%", on the printer's own page as well as the +reports. Every supply on one host reading zero at the same moment is not what a +set of cartridges does; it is what a host that stopped answering looks like. + +`getsuppliesbyip` turned every unreadable item into 0: + + try: + level = int(float(item.get("lastvalue", 0))) + except (ValueError, TypeError): + level = 0 + +An item that has never collected returns lastvalue as an EMPTY STRING, so +float('') raised and the handler substituted zero. `lastclock` was fetched in +the same request and never read by anything, so a reading from three weeks ago +was presented as current. + +Zero is a legitimate level meaning "spent", which is what made the substitution +dangerous rather than merely wrong: it reached `level <= EMPTY_LEVEL` in the +forecast, which set daysleft to 0, which put the printer on the order list. The +report asked purchasing to buy a full set of cartridges for a printer nobody had +measured. + +None means unknown, and it has to stay None the whole way out. +""" + +import time + +import pytest + +from plugins.printers.services.zabbix_service import ZabbixService +from plugins.printers.services.supply_parts import classifysupply +from plugins.printers.services.supply_history import analyse, EMPTY_LEVEL + + +def item(lastvalue, lastclock=None, name='Black Cartridge'): + """One Zabbix item.get row, shaped as the API returns it.""" + if lastclock is None: + lastclock = int(time.time()) - 600 + return {'itemid': '1', 'name': name, + 'lastvalue': lastvalue, 'lastclock': str(lastclock)} + + +# --- reading one item ------------------------------------------------------- + +def test_an_item_that_never_collected_is_unknown_not_zero(): + """The reported case. Zabbix returns '' for an item with no data.""" + level, lastseen = ZabbixService._readlevel(item('')) + assert level is None + assert lastseen is None + + +def test_a_missing_lastvalue_is_unknown_not_zero(): + level, _ = ZabbixService._readlevel({'lastvalue': None, 'lastclock': '0'}) + assert level is None + + +def test_an_unparseable_lastvalue_is_unknown_not_zero(): + level, _ = ZabbixService._readlevel(item('n/a')) + assert level is None + + +def test_a_real_zero_is_still_zero(): + """The whole point of the distinction: an empty cartridge must survive it. + + If this returned None, an actually-spent cartridge would stop being ordered. + """ + level, _ = ZabbixService._readlevel(item('0')) + assert level == 0 + + +def test_a_normal_reading_comes_back_with_its_clock(): + when = int(time.time()) - 300 + level, lastseen = ZabbixService._readlevel(item('47', lastclock=when)) + assert level == 47 + assert lastseen == when + + +def test_a_float_reading_is_truncated_to_an_int(): + level, _ = ZabbixService._readlevel(item('47.8')) + assert level == 47 + + +# --- staleness -------------------------------------------------------------- + +def test_a_reading_older_than_the_window_is_unknown(): + """lastclock was fetched and then ignored, so a host that went quiet weeks + ago kept presenting its last level as current.""" + stale = int(time.time()) - (ZabbixService.STALE_AFTER_SECONDS + 60) + level, lastseen = ZabbixService._readlevel(item('47', lastclock=stale)) + assert level is None + assert lastseen == stale, 'the age must survive, so the UI can say how old' + + +def test_a_reading_inside_the_window_is_kept(): + recent = int(time.time()) - (ZabbixService.STALE_AFTER_SECONDS - 3600) + level, _ = ZabbixService._readlevel(item('47', lastclock=recent)) + assert level == 47 + + +def test_a_value_with_no_clock_is_trusted(): + """A reading that cannot be aged is not thereby wrong. Discarding it would + lose real levels wherever lastclock is absent.""" + level, lastseen = ZabbixService._readlevel( + {'lastvalue': '47', 'lastclock': '0'}) + assert level == 47 + assert lastseen is None + + +# --- classification --------------------------------------------------------- + +def test_an_unknown_level_classifies_as_unknown(): + result = classifysupply(None, 'Black Cartridge', 'HP') + assert result['status'] == 'unknown' + assert result['remaining'] is None + + +def test_an_unknown_level_is_not_critical(): + """'critical' is a claim someone acts on. It may not be made about a supply + nobody measured.""" + assert classifysupply(None, 'Black Cartridge', 'HP')['status'] != 'critical' + + +def test_a_real_zero_is_still_critical(): + assert classifysupply(0, 'Black Cartridge', 'HP')['status'] == 'critical' + + +def test_an_unknown_waste_cartridge_does_not_invert_into_full(): + """Waste is scored as 100 - level. With level 0 standing in for unknown, + an unreadable waste cartridge read as 100% full, which is the alarm state.""" + result = classifysupply(None, 'Waste Toner Container', 'HP') + assert result['status'] == 'unknown' + assert result['remaining'] is None + + +# --- the forecast ----------------------------------------------------------- + +def test_an_unknown_level_with_no_history_gets_no_countdown(): + """0 reached `level <= EMPTY_LEVEL` and became daysleft 0 - the step that + put CSF04 on the order list.""" + result = analyse([], currentlevel=None) + assert result['daysleft'] is None + assert result['reason'] == 'no history' + + +def test_a_real_zero_with_no_history_still_counts_as_due_now(): + assert analyse([], currentlevel=0)['daysleft'] == 0 + assert EMPTY_LEVEL >= 0 + + +def test_an_unknown_level_falls_back_to_the_last_real_reading(): + """History is evidence the live read lacks. Preferring it beats inventing a + level, and beats throwing the cartridge off the report. + + Clock is unix seconds and level falls over time, which is the only direction + toner goes; a rising series would read as a cartridge change. + """ + now = int(time.time()) + points = [(now - n * 86400, 50 + n * 2) for n in range(20, -1, -1)] + result = analyse(points, currentlevel=None) + assert result['currentlevel'] == 50, 'the newest stored reading' + assert result['daysleft'] is not None, 'a forecast is still possible'