diff --git a/CHANGELOG.md b/CHANGELOG.md index 69979dbe..c7f54a08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,6 +128,20 @@ ADR-007 and ADR-002. ### Fixed +- **The printer supplies card was ordered by printer name, so the emptiest + cartridge was not first and sometimes was not shown at all.** Rows sorted on + "is critical" and then alphabetically, and critical is everything at or below + 5% - so a cartridge at 5% and one at 0% tied on the first key and the alphabet + broke the tie. That is the wrong order on its own, and worse than it looks: + the dashboard queue keeps only the first few rows per source, so a printer + that was genuinely empty got truncated off the card while a fuller one held + its place. The card exists to answer "what do I walk out and change today" and + was answering it with the alphabet. Rows now sort by the emptiest cartridge on + the printer, ascending. Waste cartridges were already normalised to percent + remaining, so one ascending sort is worst-first for every supply type and the + critical flag falls out of it rather than needing its own key; the name only + breaks ties. + - **A permission became grantable to anonymous visitors because of how it was spelled.** `guest_grantable_permissions()` opened any permission whose name ended `.view` or `.read` to the guest role, on the reasoning that reading is diff --git a/plugins/printers/api/asset_routes.py b/plugins/printers/api/asset_routes.py index da463726..3b3e7d39 100644 --- a/plugins/printers/api/asset_routes.py +++ b/plugins/printers/api/asset_routes.py @@ -2659,6 +2659,9 @@ def dashboard_supplies(): # dashboard card and the toner report were both doing. 'levelid': printer.get('levelid'), 'iscritical': any(s['status'] == 'critical' for s in depleted), + # The emptiest cartridge on this printer, which is what decides the + # row's place in the queue. depleted is sorted and non-empty here. + 'worstremaining': depleted[0]['remaining'], 'supplies': [{ 'text': '{} {}%'.format(_shortsupplyname(supply.get('name')), supply.get('remaining')), @@ -2667,7 +2670,18 @@ def dashboard_supplies(): } for supply in depleted], }) - rows.sort(key=lambda r: (not r['iscritical'], r['printername'])) + # Emptiest first. This sorted on iscritical then NAME, and critical is + # everything at or below 5%, so a cartridge at 5% and one at 0% tied on the + # first key and the alphabet decided which came first. That is bad ordering + # on its own and worse than it looks: the dashboard queue takes only the + # first few rows per source, so the printer that is actually empty was + # being truncated away while a fuller one held its place. + # + # remaining is already normalised for waste cartridges (a full waste bin + # reads as low remaining), so one ascending sort is worst-first for every + # supply type, and iscritical falls out of it rather than needing its own + # key. Name only breaks ties. + rows.sort(key=lambda r: (r['worstremaining'], r['printername'])) # Envelope rather than a bare list, so the card can say WHY it has no rows. # An empty card and a blind card must not look the same. return success_response({'rows': rows, diff --git a/tests/test_plugins/test_supplies_card_ordering.py b/tests/test_plugins/test_supplies_card_ordering.py new file mode 100644 index 00000000..1eb40fef --- /dev/null +++ b/tests/test_plugins/test_supplies_card_ordering.py @@ -0,0 +1,101 @@ +"""The supplies card must put the emptiest cartridge first. + +It sorted on iscritical, then on PRINTER NAME. Critical is everything at or +below 5%, so a cartridge at 5% and one at 0% tied on the first key and the +alphabet decided the order. + +That is bad ordering on its own, and worse than it looks: the dashboard queue +takes only the first few rows per source, so a printer that is genuinely empty +was truncated out of the card while a fuller one held its place. The card +existed to answer "what do I walk out and change today" and was answering it +with the alphabet. +""" +from unittest.mock import patch + +import pytest + + +def _call(client, auth_headers): + response = client.get('/api/printers/dashboard/supplies', + headers=auth_headers) + assert response.status_code == 200 + return response.get_json()['data'] + + +@pytest.fixture(autouse=True) +def _clear_cache(app): + from shopdb.extensions import cache + cache.delete('printers_low_supplies') + yield + cache.delete('printers_low_supplies') + + +def _printer(printerid, name, remaining, status='critical'): + return { + 'printerid': printerid, + 'printername': name, + 'assetnumber': 'A{}'.format(printerid), + 'location': 'Shop floor', + 'mapx': None, 'mapy': None, 'levelid': None, + 'supplies': [{ + 'name': 'Black Toner', 'level': remaining, 'remaining': remaining, + 'status': status, 'iswaste': False, 'isdrum': False, + 'partnumbers': [], 'lastseen': None, 'color': 'black', + 'supplytype': 'toner', + }], + } + + +def _patched(rows): + return patch('plugins.printers.api.asset_routes._get_low_supplies_data', + return_value={'printers': rows, + 'summary': {'total_checked': len(rows), + 'low': 0, 'critical': len(rows)}}) + + +def test_the_empty_cartridge_comes_before_the_nearly_empty_one(client, + auth_headers): + """'Alpha' at 5% used to precede 'Zulu' at 0% purely on the name.""" + with _patched([_printer(1, 'Alpha', 5), _printer(2, 'Zulu', 0)]): + data = _call(client, auth_headers) + + order = [row['printername'] for row in data['rows']] + assert order == ['Zulu', 'Alpha'] + + +def test_rows_are_ordered_by_how_empty_they_are(client, auth_headers): + with _patched([_printer(1, 'Alpha', 4), + _printer(2, 'Bravo', 0), + _printer(3, 'Charlie', 2), + _printer(4, 'Delta', 5)]): + data = _call(client, auth_headers) + + remaining = [row['worstremaining'] for row in data['rows']] + assert remaining == sorted(remaining) + assert remaining == [0, 2, 4, 5] + + +def test_a_zero_percent_printer_survives_the_queue_truncation(client, + auth_headers): + """The dashboard queue keeps only the first few rows per source. + + With five fuller printers whose names sort earlier, the empty one used to + fall off the end of the card entirely. + """ + rows = [_printer(index, name, 5) for index, name in enumerate( + ['Alpha', 'Bravo', 'Charlie', 'Delta', 'Echo'], start=1)] + rows.append(_printer(6, 'Zulu', 0)) + + with _patched(rows): + data = _call(client, auth_headers) + + # QUEUEPERSOURCE is 5 on the frontend; first place is what matters here. + assert data['rows'][0]['printername'] == 'Zulu' + assert [r['printername'] for r in data['rows'][:5]].count('Zulu') == 1 + + +def test_the_name_still_breaks_a_tie(client, auth_headers): + with _patched([_printer(1, 'Bravo', 3), _printer(2, 'Alpha', 3)]): + data = _call(client, auth_headers) + + assert [row['printername'] for row in data['rows']] == ['Alpha', 'Bravo']