mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-21 10:15:04 +02:00
fix(archives): #1608 suppress card time-accuracy badge for multi-run archives
compute_time_accuracy in routes/archives.py compares the archive row's own started_at / completed_at (which reflect the latest run only) against archive.print_time_seconds (which the #1593 parser fix correctly stores as the sum across plates). For a 3-plate file printed plate-by-plate the ratio is ~300%, producing a "+188%" card badge that means nothing — apples to oranges. The 5-500% sanity band catches truly broken values but lets this deterministic N×100% shape through. Reporter's archive #65 was 3 plates over 9 runs. compute_time_accuracy gains an optional run_aggregate argument and returns both actual_time_seconds and time_accuracy as null when the aggregate reports more than one logged run. The frontend already falls through to print_time_seconds for the time display (actual_time_seconds || print_time_seconds) and gates the badge on time_accuracy being truthy, so multi-run archives now show the slicer estimate with no badge. Single-run archives keep the original behaviour verbatim. The fix is applied at every call site that renders an archive card: archive_to_response now threads run_aggregate through, and the three endpoints that previously didn't load the aggregate (archives.py search fast-path and FTS path, single-archive PATCH, and projects.list_project_archives) now batch-load it via the existing _load_run_aggregates helper. The stats endpoint's per-run accuracy aggregation at archives.py:940 already uses PrintLogEntry.duration_seconds with its own 50-200% band filter and is untouched.
This commit is contained in:
maziggy
@@ -149,7 +149,7 @@ def _apply_run_user_filter(conditions: list, created_by_id: int | None):
|
||||
conditions.append(PrintLogEntry.created_by_id == created_by_id)
|
||||
|
||||
|
||||
def compute_time_accuracy(archive: PrintArchive) -> dict:
|
||||
def compute_time_accuracy(archive: PrintArchive, run_aggregate: dict | None = None) -> dict:
|
||||
"""Compute actual print time and accuracy for an archive.
|
||||
|
||||
Returns dict with actual_time_seconds and time_accuracy.
|
||||
@@ -157,8 +157,26 @@ def compute_time_accuracy(archive: PrintArchive) -> dict:
|
||||
- 100% = perfect estimate
|
||||
- >100% = print was faster than estimated
|
||||
- <100% = print took longer than estimated
|
||||
|
||||
When ``run_aggregate`` indicates the archive has more than one logged
|
||||
run (multi-plate file printed plate-by-plate, or reprints), both
|
||||
fields are suppressed: ``archive.started_at / completed_at`` reflect
|
||||
the LATEST run only, while ``archive.print_time_seconds`` is the
|
||||
whole-file estimate (post-#1593 the parser sums across plates), so
|
||||
comparing the two describes different scopes. The card-rendering
|
||||
frontend falls through to ``archive.print_time_seconds`` for the
|
||||
time display and hides the badge when ``time_accuracy`` is null —
|
||||
that's the desired "show estimate, no badge" presentation for
|
||||
multi-run archives (#1608). Single-run archives keep the original
|
||||
badge behaviour verbatim.
|
||||
"""
|
||||
result = {"actual_time_seconds": None, "time_accuracy": None}
|
||||
result: dict[str, int | float | None] = {"actual_time_seconds": None, "time_accuracy": None}
|
||||
|
||||
# Multi-run archives: the per-run actual (started_at..completed_at on
|
||||
# the archive row) is incommensurable with the whole-file estimate.
|
||||
# Both fields are cleared so the card shows estimate + no badge.
|
||||
if run_aggregate and (run_aggregate.get("run_count") or 0) > 1:
|
||||
return result
|
||||
|
||||
if archive.started_at and archive.completed_at and archive.status == "completed":
|
||||
actual_seconds = int((archive.completed_at - archive.started_at).total_seconds())
|
||||
@@ -271,8 +289,11 @@ def archive_to_response(
|
||||
"created_by_username": archive.created_by.username if archive.created_by else None,
|
||||
}
|
||||
|
||||
# Add computed time accuracy fields
|
||||
accuracy_data = compute_time_accuracy(archive)
|
||||
# Add computed time accuracy fields. ``run_aggregate`` lets
|
||||
# ``compute_time_accuracy`` suppress the badge for multi-run archives
|
||||
# where the per-run actual / whole-file estimate scopes don't match
|
||||
# (#1608).
|
||||
accuracy_data = compute_time_accuracy(archive, run_aggregate)
|
||||
data.update(accuracy_data)
|
||||
|
||||
if run_aggregate:
|
||||
@@ -580,7 +601,10 @@ async def search_archives(
|
||||
query = query.limit(limit).offset(offset)
|
||||
result = await db.execute(query)
|
||||
archives = result.scalars().all()
|
||||
return [archive_to_response(a) for a in archives]
|
||||
# Load run aggregates so multi-run archives' time/accuracy badge is
|
||||
# suppressed consistently with the main list endpoint (#1608).
|
||||
run_aggregates = await _load_run_aggregates(db, [a.id for a in archives])
|
||||
return [archive_to_response(a, run_aggregate=run_aggregates.get(a.id)) for a in archives]
|
||||
|
||||
if not matched_ids:
|
||||
return []
|
||||
@@ -607,7 +631,10 @@ async def search_archives(
|
||||
ordered_archives = [archives_dict[id] for id in matched_ids if id in archives_dict]
|
||||
paginated = ordered_archives[offset : offset + limit]
|
||||
|
||||
return [archive_to_response(a) for a in paginated]
|
||||
# Load run aggregates so multi-run archives' time/accuracy badge is
|
||||
# suppressed consistently with the main list endpoint (#1608).
|
||||
run_aggregates = await _load_run_aggregates(db, [a.id for a in paginated])
|
||||
return [archive_to_response(a, run_aggregate=run_aggregates.get(a.id)) for a in paginated]
|
||||
|
||||
|
||||
@router.post("/search/rebuild-index")
|
||||
@@ -1416,7 +1443,11 @@ async def update_archive(
|
||||
)
|
||||
archive = result.scalar_one_or_none()
|
||||
|
||||
return archive_to_response(archive)
|
||||
# Load run aggregate so the time/accuracy badge stays consistent with
|
||||
# the list / detail endpoints when the frontend re-renders the card
|
||||
# after a PATCH (#1608).
|
||||
run_aggregates = await _load_run_aggregates(db, [archive.id]) if archive else {}
|
||||
return archive_to_response(archive, run_aggregate=run_aggregates.get(archive.id) if archive else None)
|
||||
|
||||
|
||||
@router.post("/{archive_id}/favorite", response_model=ArchiveResponse)
|
||||
|
||||
@@ -701,9 +701,13 @@ async def list_project_archives(
|
||||
archives = result.scalars().all()
|
||||
|
||||
# Import the response converter from archives module
|
||||
from backend.app.api.routes.archives import archive_to_response
|
||||
from backend.app.api.routes.archives import _load_run_aggregates, archive_to_response
|
||||
|
||||
return [archive_to_response(a) for a in archives]
|
||||
# Load run aggregates so multi-run archives' time/accuracy badge is
|
||||
# suppressed consistently with the main archives list endpoint (#1608).
|
||||
run_aggregates = await _load_run_aggregates(db, [a.id for a in archives])
|
||||
|
||||
return [archive_to_response(a, run_aggregate=run_aggregates.get(a.id)) for a in archives]
|
||||
|
||||
|
||||
@router.get("/{project_id}/queue")
|
||||
|
||||
@@ -312,3 +312,187 @@ async def test_time_accuracy_excludes_multi_plate_plate_by_plate_outliers(
|
||||
body = (await async_client.get("/api/v1/archives/stats")).json()
|
||||
assert body["average_time_accuracy"] == pytest.approx(97.3, abs=0.1)
|
||||
assert body["time_accuracy_by_printer"][str(printer.id)] == pytest.approx(97.3, abs=0.1)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# #1608: compute_time_accuracy suppresses the per-card badge for multi-run
|
||||
# archives where the whole-file estimate is incommensurable with the
|
||||
# latest-run actual.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestComputeTimeAccuracyMultiRun:
|
||||
"""The card-level ``compute_time_accuracy`` runs against the archive's own
|
||||
``started_at`` / ``completed_at`` (latest run only) and
|
||||
``print_time_seconds`` (post-#1593 sum across plates). For multi-run
|
||||
archives those describe different scopes — a 3-plate file printed
|
||||
plate-by-plate over 3 runs produces estimate/actual = 300% → +200% badge,
|
||||
which is pure noise. The reporter (#1608, archive #65) verified the
|
||||
bug surfaces at +188% for a 3-plate file with 9 runs.
|
||||
|
||||
The fix: when the archive has more than one logged run, suppress BOTH
|
||||
fields. The frontend then falls through to ``print_time_seconds`` for
|
||||
the time display (so the user sees the slicer's whole-file estimate
|
||||
instead of one run's wall-clock) and hides the badge.
|
||||
"""
|
||||
|
||||
def _make_archive(self, *, print_time_seconds, started_at, completed_at, status="completed"):
|
||||
from types import SimpleNamespace
|
||||
|
||||
return SimpleNamespace(
|
||||
print_time_seconds=print_time_seconds,
|
||||
started_at=started_at,
|
||||
completed_at=completed_at,
|
||||
status=status,
|
||||
)
|
||||
|
||||
def test_single_run_keeps_original_behaviour(self):
|
||||
"""``run_count == 1`` is the case the badge was designed for —
|
||||
compute and return both actual + accuracy as before."""
|
||||
from backend.app.api.routes.archives import compute_time_accuracy
|
||||
|
||||
archive = self._make_archive(
|
||||
print_time_seconds=3600,
|
||||
started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc),
|
||||
completed_at=datetime(2026, 5, 1, 11, 0, tzinfo=timezone.utc),
|
||||
)
|
||||
result = compute_time_accuracy(archive, run_aggregate={"run_count": 1})
|
||||
|
||||
assert result["actual_time_seconds"] == 3600
|
||||
assert result["time_accuracy"] == 100.0
|
||||
|
||||
def test_no_run_aggregate_keeps_original_behaviour(self):
|
||||
"""Endpoints that don't yet load run_aggregates (legacy callers, or
|
||||
contexts where the data isn't relevant) must keep the pre-fix
|
||||
per-archive computation — never silently drop the badge."""
|
||||
from backend.app.api.routes.archives import compute_time_accuracy
|
||||
|
||||
archive = self._make_archive(
|
||||
print_time_seconds=3600,
|
||||
started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc),
|
||||
completed_at=datetime(2026, 5, 1, 10, 50, tzinfo=timezone.utc),
|
||||
)
|
||||
result = compute_time_accuracy(archive) # no run_aggregate
|
||||
|
||||
assert result["actual_time_seconds"] == 3000
|
||||
assert result["time_accuracy"] == 120.0 # 3600/3000
|
||||
|
||||
def test_multi_run_archive_suppresses_both_fields(self):
|
||||
"""Reporter's case (archive #65, 3 plates, 9 logged runs): one-run
|
||||
actual (6364s) vs whole-file estimate (18354s) → +188% badge that
|
||||
means nothing. Multi-run must clear both fields so the card falls
|
||||
through to the estimate display with no badge."""
|
||||
from backend.app.api.routes.archives import compute_time_accuracy
|
||||
|
||||
archive = self._make_archive(
|
||||
print_time_seconds=18354,
|
||||
started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc),
|
||||
completed_at=datetime(2026, 5, 1, 11, 46, 4, tzinfo=timezone.utc), # ~6364s
|
||||
)
|
||||
result = compute_time_accuracy(archive, run_aggregate={"run_count": 9})
|
||||
|
||||
assert result["actual_time_seconds"] is None
|
||||
assert result["time_accuracy"] is None
|
||||
|
||||
def test_run_count_zero_keeps_original_behaviour(self):
|
||||
"""A run_aggregate that exists but reports zero runs (edge case from
|
||||
the LEFT JOIN-style helper) must not trigger suppression — that's
|
||||
not the multi-run shape, it's the no-runs shape, and the
|
||||
per-archive timestamps are still meaningful (the archive was
|
||||
marked completed without a PrintLogEntry trail, e.g. legacy
|
||||
imports)."""
|
||||
from backend.app.api.routes.archives import compute_time_accuracy
|
||||
|
||||
archive = self._make_archive(
|
||||
print_time_seconds=3600,
|
||||
started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc),
|
||||
completed_at=datetime(2026, 5, 1, 11, 0, tzinfo=timezone.utc),
|
||||
)
|
||||
result = compute_time_accuracy(archive, run_aggregate={"run_count": 0})
|
||||
|
||||
assert result["actual_time_seconds"] == 3600
|
||||
assert result["time_accuracy"] == 100.0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_archive_list_suppresses_time_accuracy_for_multi_run_archives(
|
||||
async_client: AsyncClient, archive_factory, printer_factory, db_session
|
||||
):
|
||||
"""#1608 integration: the card response from the main list endpoint
|
||||
must report ``actual_time_seconds = null`` and ``time_accuracy = null``
|
||||
for an archive with multiple logged runs, so the frontend renders the
|
||||
slicer estimate without the misleading +N% badge."""
|
||||
printer = await printer_factory()
|
||||
|
||||
# 3-plate file: estimate is the whole-file sum, latest run is one plate.
|
||||
archive = await archive_factory(
|
||||
printer.id,
|
||||
status="completed",
|
||||
print_time_seconds=18354, # all-plates estimate (post-#1593 parser)
|
||||
started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc),
|
||||
completed_at=datetime(2026, 5, 1, 11, 46, 4, tzinfo=timezone.utc), # ~6364s = one plate
|
||||
with_run=False,
|
||||
)
|
||||
# Three runs each ~6364s — plate-by-plate.
|
||||
for day in (1, 2, 3):
|
||||
db_session.add(
|
||||
PrintLogEntry(
|
||||
archive_id=archive.id,
|
||||
printer_id=archive.printer_id,
|
||||
status="completed",
|
||||
started_at=datetime(2026, 5, day, 10, 0, tzinfo=timezone.utc),
|
||||
completed_at=datetime(2026, 5, day, 11, 46, 4, tzinfo=timezone.utc),
|
||||
duration_seconds=6364,
|
||||
)
|
||||
)
|
||||
await db_session.commit()
|
||||
|
||||
response = await async_client.get("/api/v1/archives/")
|
||||
assert response.status_code == 200
|
||||
row = next(r for r in response.json() if r["id"] == archive.id)
|
||||
|
||||
# Frontend renders archive.actual_time_seconds || archive.print_time_seconds —
|
||||
# with actual cleared, it falls through to the estimate; with accuracy
|
||||
# cleared, no badge renders.
|
||||
assert row["actual_time_seconds"] is None, "multi-run actual is incommensurable with the estimate — must be null"
|
||||
assert row["time_accuracy"] is None, "no badge for multi-run archives — the scopes don't match"
|
||||
# The estimate itself is preserved so the card has something to display.
|
||||
assert row["print_time_seconds"] == 18354
|
||||
assert row["run_count"] == 3
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.integration
|
||||
async def test_archive_list_keeps_time_accuracy_for_single_run_archives(
|
||||
async_client: AsyncClient, archive_factory, printer_factory, db_session
|
||||
):
|
||||
"""Sanity check for the #1608 fix: single-run archives (the case the
|
||||
badge was designed for) keep their original badge behaviour."""
|
||||
printer = await printer_factory()
|
||||
|
||||
archive = await archive_factory(
|
||||
printer.id,
|
||||
status="completed",
|
||||
print_time_seconds=3600,
|
||||
started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc),
|
||||
completed_at=datetime(2026, 5, 1, 11, 0, tzinfo=timezone.utc),
|
||||
with_run=False,
|
||||
)
|
||||
db_session.add(
|
||||
PrintLogEntry(
|
||||
archive_id=archive.id,
|
||||
printer_id=archive.printer_id,
|
||||
status="completed",
|
||||
started_at=datetime(2026, 5, 1, 10, 0, tzinfo=timezone.utc),
|
||||
completed_at=datetime(2026, 5, 1, 11, 0, tzinfo=timezone.utc),
|
||||
duration_seconds=3600,
|
||||
)
|
||||
)
|
||||
await db_session.commit()
|
||||
|
||||
row = next(r for r in (await async_client.get("/api/v1/archives/")).json() if r["id"] == archive.id)
|
||||
|
||||
assert row["actual_time_seconds"] == 3600
|
||||
assert row["time_accuracy"] == 100.0
|
||||
assert row["run_count"] == 1
|
||||
|
||||
Reference in New Issue
Block a user