diff --git a/plugins/machines/api/routes.py b/plugins/machines/api/routes.py index f8726a7f..1d1dcf7e 100644 --- a/plugins/machines/api/routes.py +++ b/plugins/machines/api/routes.py @@ -553,16 +553,22 @@ def dashboard_summary(): by_type_query = by_type_query.filter(Asset.assetid.notin_(secondaryassetids)) by_type = by_type_query.group_by(MachineType.machinetype).all() - # Count by status + # Count by status. The dualpath exclusion applies here too: it was applied + # to total and bytype and NOT to this one, so the status bars summed to + # more than the total they sit under - by exactly the number of dualpath + # secondaries. A breakdown that does not add up to its own headline is the + # kind of thing somebody spends an afternoon reconciling. from shopdb.api import AssetStatus - by_status = db.session.query( + by_status_query = db.session.query( AssetStatus.status, db.func.count(Machine.machineid) ).join(Asset, Asset.assetid == Machine.assetid ).join(AssetStatus, AssetStatus.statusid == Asset.statusid - ).filter(Asset.isactive == True - ).group_by(AssetStatus.status - ).all() + ).filter(Asset.isactive == True) + if secondaryassetids: + by_status_query = by_status_query.filter( + Asset.assetid.notin_(secondaryassetids)) + by_status = by_status_query.group_by(AssetStatus.status).all() return success_response({ 'total': total, diff --git a/plugins/measuringtools/api/routes.py b/plugins/measuringtools/api/routes.py index f24f0a84..f836de36 100644 --- a/plugins/measuringtools/api/routes.py +++ b/plugins/measuringtools/api/routes.py @@ -182,19 +182,32 @@ def list_tools(): query = query.filter(Asset.statusid == status_id) query = query.order_by(Asset.assetnumber) - items, total = paginate_query(query, page, per_page) - today = date.today() - data = [_merged(tool, today) for tool in items] - # Calibration status is derived, so it cannot be a SQL filter - apply it - # to the built rows. Filtering post-pagination is acceptable here because - # the dataset is a gage lab's worth of tools, not a fleet. + # Calibration status is DERIVED (see derive_status), so it cannot be a SQL + # filter. It used to be applied to the rows of the current page, which + # meant the page showed the matches that happened to fall on it while + # meta.pagination.total counted the unfiltered set: asking for 'overdue' + # with one overdue tool in sixteen returned an empty page 1, a total of 16 + # and eight pages, with the actual row on page 3. A filter that hides + # matching rows and miscounts the rest is worse than no filter. + # + # So when this filter is in play the whole matching set is built and + # filtered before paging. That is the same tradeoff the previous comment + # named - a gage lab's worth of tools, not a fleet - spent where it buys + # correct answers rather than a cheaper query. calibrationstatus = request.args.get('calibrationstatus') if calibrationstatus: - data = [d for d in data - if d['measuringtool']['calibrationstatus'] == calibrationstatus] + matched = [_merged(tool, today) for tool in query.all()] + matched = [row for row in matched + if row['measuringtool']['calibrationstatus'] == calibrationstatus] + total = len(matched) + start = (page - 1) * per_page + data = matched[start:start + per_page] + return paginated_response(data, page, per_page, total) + items, total = paginate_query(query, page, per_page) + data = [_merged(tool, today) for tool in items] return paginated_response(data, page, per_page, total) diff --git a/shopdb/core/api/assets.py b/shopdb/core/api/assets.py index 2ad24b1e..edb7cc16 100644 --- a/shopdb/core/api/assets.py +++ b/shopdb/core/api/assets.py @@ -1012,17 +1012,26 @@ def get_assets_map(): Asset.mapy.isnot(None) ) - # Dualpath single-machine collapse: hide the secondary bay marker so a - # dual-bay pair shows one marker; the primary carries the partner label. - # Gated on the site setting (default on). + # Dualpath single-machine collapse on the map: a secondary bay draws its + # OWN marker when somebody has positioned it, and is represented by the + # primary's marker when nobody has. + # + # It used to be excluded unconditionally, which made setting a position on + # a secondary bay a silent no-op: the form accepted the pin, stored it, and + # the map never drew it. One marker per pair is right when the pair sits in + # one place, which is the common case and still the default - but a + # dual-bay machine whose second bay is physically somewhere else is exactly + # what positioning it separately means. + # + # No extra filtering is needed for the fallback half: the query above + # already requires mapx/mapy, so an unpositioned secondary never reaches + # here and the primary's marker stands for both, carrying the combined + # label. from shopdb.core.services.dualpath import ( resolve_dualpath_pairs, dualpath_single_machine_enabled) dualpath_collapse = None if dualpath_single_machine_enabled(): dualpath_collapse = resolve_dualpath_pairs() - if dualpath_collapse.secondaryassetids: - query = query.filter( - Asset.assetid.notin_(dualpath_collapse.secondaryassetids)) selected_assettype = request.args.get('assettype') @@ -1102,6 +1111,10 @@ def get_assets_map(): assets = query.all() + # Every asset that ends up with a marker. Used to decide whether a dual-bay + # partner draws its own pin or is represented by this one. + positioned_assetids = {asset.assetid for asset in assets} + # Batch-load primary IPs in a single query instead of N+1 per asset. # Prefer isprimary=True IP, fall back to any IP (comtypeid=1). asset_ids = [a.assetid for a in assets] @@ -1153,10 +1166,17 @@ def get_assets_map(): if type_data: item['typedata'] = type_data - # combined-label partner for a collapsed dual-bay pair (label only) + # Combined label for a dual-bay pair, and ONLY when this marker really + # stands for both bays. A partner that draws its own marker gets its + # own name: labelling two separate pins '2007 / 2008' would say the + # same machine is in two places. if dualpath_collapse: partner = dualpath_collapse.partnerbyasset.get(asset.assetid) - item['dualpathpartner'] = partner['assetnumber'] if partner else None + partner_has_marker = bool( + partner and partner['assetid'] in positioned_assetids) + item['dualpathpartner'] = ( + partner['assetnumber'] if partner and not partner_has_marker + else None) data.append(item) diff --git a/tests/test_plugins/test_dualpath_single.py b/tests/test_plugins/test_dualpath_single.py index f8c128d7..3c95f2eb 100644 --- a/tests/test_plugins/test_dualpath_single.py +++ b/tests/test_plugins/test_dualpath_single.py @@ -94,9 +94,16 @@ def test_dashboard_machine_count_collapses(client, db, auth_headers): assert counts['counts']['machines'] == 1 -def test_map_excludes_secondary_marker(client, db, auth_headers): +def test_an_unpositioned_secondary_is_represented_by_the_primary( + client, db, auth_headers): + """The common case: the pair sits in one place, so it draws one marker and + that marker is labelled with both bays.""" invalidate_settings_cache() _seed_dualpath_pair() + secondary = Asset.query.filter_by(assetnumber='2008').first() + secondary.mapx = None + secondary.mapy = None + _db.session.commit() resp = client.get('/api/assets/map', headers=auth_headers) assert resp.status_code == 200, resp.get_json() @@ -106,7 +113,30 @@ def test_map_excludes_secondary_marker(client, db, auth_headers): assert '2007' in numbers assert '2008' not in numbers primary = next(a for a in assets if a['assetnumber'] == '2007') - assert primary['dualpathpartner'] == '2008' + assert primary['dualpathpartner'] == '2008', \ + 'the one marker has to say it stands for both bays' + + +def test_a_positioned_secondary_draws_its_own_marker(client, db, auth_headers): + """A second bay that is physically somewhere else. Positioning it used to + be a silent no-op: the form took the pin, stored it, and the map never drew + it, because the secondary was excluded from the feed unconditionally.""" + invalidate_settings_cache() + _seed_dualpath_pair() # fixture positions BOTH bays + + resp = client.get('/api/assets/map', headers=auth_headers) + assert resp.status_code == 200, resp.get_json() + assets = resp.get_json()['data']['assets'] + numbers = [a['assetnumber'] for a in assets] + + assert '2007' in numbers + assert '2008' in numbers, 'a positioned secondary bay draws its own marker' + + # Two pins, two names. Labelling both '2007 / 2008' would say the same + # machine is in two places. + for assetnumber in ('2007', '2008'): + marker = next(a for a in assets if a['assetnumber'] == assetnumber) + assert marker['dualpathpartner'] is None, assetnumber def test_detail_banner_partner_shown_regardless_of_toggle(client, db, auth_headers): @@ -161,3 +191,50 @@ def test_map_shows_both_bays_when_disabled(client, db, auth_headers): numbers = [a['assetnumber'] for a in resp.get_json()['data']['assets']] assert '2007' in numbers assert '2008' in numbers + + +def _give_both_bays_a_status(): + """The pair fixture leaves statusid null, so the bystatus join matches + nothing. Give both bays the same status, which is what a real site has and + what makes the breakdown meaningful.""" + from shopdb.core.models import AssetStatus + + status = AssetStatus(status='In Use') + _db.session.add(status) + _db.session.flush() + for assetnumber in ('2007', '2008'): + asset = Asset.query.filter_by(assetnumber=assetnumber).first() + asset.statusid = status.statusid + _db.session.commit() + + +def test_the_status_breakdown_adds_up_to_the_total(client, db, auth_headers): + """The dualpath exclusion was applied to total and bytype but not to + bystatus, so the status bars summed to more than the headline they sit + under - by exactly the number of secondary bays. A breakdown that does not + add up to its own total is an afternoon of reconciling.""" + invalidate_settings_cache() + _seed_dualpath_pair() + _give_both_bays_a_status() + + resp = client.get('/api/machines/dashboard/summary', headers=auth_headers) + assert resp.status_code == 200, resp.get_json() + summary = resp.get_json()['data'] + + assert summary['total'] == 1, 'two bays, one physical machine' + assert sum(row['count'] for row in summary['bystatus']) == summary['total'] + + +def test_the_status_breakdown_counts_both_bays_when_collapse_is_off( + client, db, auth_headers): + """With the setting off, every bay is its own machine everywhere - the fix + must not start hiding rows a site asked to see.""" + invalidate_settings_cache() + _seed_dualpath_pair() + _give_both_bays_a_status() + _set_toggle('false') + + summary = client.get('/api/machines/dashboard/summary', + headers=auth_headers).get_json()['data'] + assert summary['total'] == 2 + assert sum(row['count'] for row in summary['bystatus']) == 2 diff --git a/tests/test_plugins/test_measuringtools.py b/tests/test_plugins/test_measuringtools.py index 77c2ab3f..0ebf1d7f 100644 --- a/tests/test_plugins/test_measuringtools.py +++ b/tests/test_plugins/test_measuringtools.py @@ -586,3 +586,61 @@ def test_an_edit_is_saved_even_when_an_audited_field_holds_a_date( headers=auth_headers).get_json()['data'] assert after['name'] == 'After rename' assert after['measuringtool']['nextcalibrationdate'] == '2028-01-15' + + +# --- a derived filter must page over the MATCHES ---------------------------- +# +# calibrationstatus was applied to the rows of the current page, so a request +# for 'overdue' returned whatever matched on that page while the total counted +# every tool. With one overdue tool in sixteen: page 1 empty, total 16, eight +# pages, and the row itself on page 3. The filter hid matching rows and +# miscounted the rest. + +def test_a_calibration_filter_pages_over_the_matches_not_the_page( + client, auth_headers): + from datetime import date, timedelta + + overdue = (date.today() - timedelta(days=5)).isoformat() + current = (date.today() + timedelta(days=400)).isoformat() + + # One overdue tool, well past the first page of a small page size. + for index in range(6): + created = client.post('/api/measuringtools', headers=auth_headers, json={ + 'assetnumber': 'MT-PAGE-%02d' % index, + 'name': 'Paged tool %d' % index, + 'statusid': _status_id(client), + 'nextcalibrationdate': overdue if index == 5 else current, + }) + assert created.status_code == 201, created.get_json() + + # Scoped with search as well, so earlier tests in this module cannot + # change the expected set - the point is paging over matches, not how many + # overdue tools the whole fixture happens to hold. + response = client.get( + '/api/measuringtools?calibrationstatus=overdue&search=MT-PAGE&perpage=2', + headers=auth_headers) + assert response.status_code == 200, response.get_json() + body = response.get_json() + rows = body['data'] + pagination = body['meta']['pagination'] + + assert [row['assetnumber'] for row in rows] == ['MT-PAGE-05'], \ + 'the overdue tool must be on page 1 of a filtered list' + assert pagination['total'] == 1, \ + 'the total counts the MATCHES, not every tool' + assert pagination['totalpages'] == 1 + + +def test_an_unfiltered_list_still_pages_normally(client, auth_headers): + for index in range(3): + client.post('/api/measuringtools', headers=auth_headers, json={ + 'assetnumber': 'MT-PLAIN-%02d' % index, + 'name': 'Plain tool %d' % index, + 'statusid': _status_id(client), + }) + + response = client.get('/api/measuringtools?perpage=2', headers=auth_headers) + assert response.status_code == 200 + pagination = response.get_json()['meta']['pagination'] + assert pagination['perpage'] == 2 + assert pagination['total'] >= 3