Draw a dual-bay secondary where it actually is, and make two counts add up

Three map and aggregation defects. The first is a behaviour change you asked
for; the other two are the 2026-09-03 run's findings.

A dualpath secondary bay was excluded from /api/assets/map unconditionally
while the collapse setting is on, so positioning it was a silent no-op: the
form took the pin, stored mapx/mapy, and the map never drew it. One marker per
pair is right when the pair sits in one place, and that stays the default - but
a dual-bay machine whose second bay is physically elsewhere is exactly what
positioning it separately means. Now: a secondary with its own position draws
its own marker; one without is represented by the primary's, which carries the
combined '2007 / 2008' label. Nothing extra is needed for the fallback, because
the feed already requires mapx/mapy - an unpositioned bay never reaches the
loop.

The combined label is now suppressed when the partner draws its own marker.
Labelling two separate pins '2007 / 2008' would say one machine is in two
places, which is worse than either bay being unlabelled.

Verified against a live instance: unpositioned secondary gives one marker
labelled with both bays; positioned gives two markers each with its own name;
clearing the position puts it back to one. That third case matters - a
one-way door would be its own bug.

machines/dashboard/summary applied the dualpath exclusion to total and bytype
but not to bystatus, so the status bars summed to more than the headline above
them, by exactly the number of secondary bays. A breakdown that does not add up
to its own total is an afternoon of reconciling.

The measuring-tool calibrationstatus filter ran AFTER pagination. It is a
derived value so it cannot be a SQL filter, but applying it to the current
page meant asking for 'overdue' with one overdue tool in sixteen returned an
empty page 1, a total of 16, eight pages, and the row itself on page 3. When
that filter is in play the matching set is now built and filtered before
paging - the same tradeoff the old comment named, spent on correct answers
instead of a cheaper query.

Seven tests. The pagination one verified against the committed version, which
fails it.

Scope: product
This commit is contained in:
cproudlock
2026-09-03 15:07:49 -04:00
parent 1d2bbb3630
commit b1e5dd734d
5 changed files with 197 additions and 23 deletions

View File

@@ -553,16 +553,22 @@ def dashboard_summary():
by_type_query = by_type_query.filter(Asset.assetid.notin_(secondaryassetids)) by_type_query = by_type_query.filter(Asset.assetid.notin_(secondaryassetids))
by_type = by_type_query.group_by(MachineType.machinetype).all() 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 from shopdb.api import AssetStatus
by_status = db.session.query( by_status_query = db.session.query(
AssetStatus.status, AssetStatus.status,
db.func.count(Machine.machineid) db.func.count(Machine.machineid)
).join(Asset, Asset.assetid == Machine.assetid ).join(Asset, Asset.assetid == Machine.assetid
).join(AssetStatus, AssetStatus.statusid == Asset.statusid ).join(AssetStatus, AssetStatus.statusid == Asset.statusid
).filter(Asset.isactive == True ).filter(Asset.isactive == True)
).group_by(AssetStatus.status if secondaryassetids:
).all() by_status_query = by_status_query.filter(
Asset.assetid.notin_(secondaryassetids))
by_status = by_status_query.group_by(AssetStatus.status).all()
return success_response({ return success_response({
'total': total, 'total': total,

View File

@@ -182,19 +182,32 @@ def list_tools():
query = query.filter(Asset.statusid == status_id) query = query.filter(Asset.statusid == status_id)
query = query.order_by(Asset.assetnumber) query = query.order_by(Asset.assetnumber)
items, total = paginate_query(query, page, per_page)
today = date.today() today = date.today()
data = [_merged(tool, today) for tool in items]
# Calibration status is derived, so it cannot be a SQL filter - apply it # Calibration status is DERIVED (see derive_status), so it cannot be a SQL
# to the built rows. Filtering post-pagination is acceptable here because # filter. It used to be applied to the rows of the current page, which
# the dataset is a gage lab's worth of tools, not a fleet. # 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') calibrationstatus = request.args.get('calibrationstatus')
if calibrationstatus: if calibrationstatus:
data = [d for d in data matched = [_merged(tool, today) for tool in query.all()]
if d['measuringtool']['calibrationstatus'] == calibrationstatus] 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) return paginated_response(data, page, per_page, total)

View File

@@ -1012,17 +1012,26 @@ def get_assets_map():
Asset.mapy.isnot(None) Asset.mapy.isnot(None)
) )
# Dualpath single-machine collapse: hide the secondary bay marker so a # Dualpath single-machine collapse on the map: a secondary bay draws its
# dual-bay pair shows one marker; the primary carries the partner label. # OWN marker when somebody has positioned it, and is represented by the
# Gated on the site setting (default on). # 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 ( from shopdb.core.services.dualpath import (
resolve_dualpath_pairs, dualpath_single_machine_enabled) resolve_dualpath_pairs, dualpath_single_machine_enabled)
dualpath_collapse = None dualpath_collapse = None
if dualpath_single_machine_enabled(): if dualpath_single_machine_enabled():
dualpath_collapse = resolve_dualpath_pairs() dualpath_collapse = resolve_dualpath_pairs()
if dualpath_collapse.secondaryassetids:
query = query.filter(
Asset.assetid.notin_(dualpath_collapse.secondaryassetids))
selected_assettype = request.args.get('assettype') selected_assettype = request.args.get('assettype')
@@ -1102,6 +1111,10 @@ def get_assets_map():
assets = query.all() 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. # 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). # Prefer isprimary=True IP, fall back to any IP (comtypeid=1).
asset_ids = [a.assetid for a in assets] asset_ids = [a.assetid for a in assets]
@@ -1153,10 +1166,17 @@ def get_assets_map():
if type_data: if type_data:
item['typedata'] = 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: if dualpath_collapse:
partner = dualpath_collapse.partnerbyasset.get(asset.assetid) 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) data.append(item)

View File

@@ -94,9 +94,16 @@ def test_dashboard_machine_count_collapses(client, db, auth_headers):
assert counts['counts']['machines'] == 1 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() invalidate_settings_cache()
_seed_dualpath_pair() _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) resp = client.get('/api/assets/map', headers=auth_headers)
assert resp.status_code == 200, resp.get_json() 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 '2007' in numbers
assert '2008' not in numbers assert '2008' not in numbers
primary = next(a for a in assets if a['assetnumber'] == '2007') 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): 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']] numbers = [a['assetnumber'] for a in resp.get_json()['data']['assets']]
assert '2007' in numbers assert '2007' in numbers
assert '2008' 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

View File

@@ -586,3 +586,61 @@ def test_an_edit_is_saved_even_when_an_audited_field_holds_a_date(
headers=auth_headers).get_json()['data'] headers=auth_headers).get_json()['data']
assert after['name'] == 'After rename' assert after['name'] == 'After rename'
assert after['measuringtool']['nextcalibrationdate'] == '2028-01-15' 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