diff --git a/CHANGELOG.md b/CHANGELOG.md index a05397ff0..3667b49ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ All notable changes to Bambuddy will be documented in this file. ## [0.2.5b1] - Unreleased ### Fixed +- **Quick Stats showed Filament Cost = 0 and empty Time Accuracy on pre-upgrade data after the 0.2.4.1 stats rewrite (#1390)** — Reporter IndividualGhost1905 upgraded to 0.2.4.1 (which shipped the per-event aggregation rewrite from #1378) and saw the Stats page split between consistent values (Total Prints / Print Time / Filament Used / Energy / Success Rate matched the archive list) and zero-or-empty ones (Filament Cost, Time Accuracy). Inconsistency was a migration gap: #1378 added six columns to `print_log_entries` — `archive_id`, `cost`, `energy_kwh`, `energy_cost`, `failure_reason`, `created_by_id` — but **didn't backfill any of them**. So every pre-upgrade log entry kept NULL on all six. The new Quick Stats query sums `PrintLogEntry.cost` (gets 0 for legacy data); the time-accuracy query joins `PrintArchive ON archive_id` (drops every legacy run from the average). Counts and per-row fields that already existed pre-#1378 (`status`, `duration_seconds`, `filament_used_grams`) kept working — which is why some panels looked right and others didn't. Fix is a two-step backfill in `run_migrations` next to the existing column-add block (DML, runs inside `begin_nested()` not `_safe_execute` since the latter is documented "DDL only"): step 1 links each orphan log entry to its archive via `print_name + printer_id` (highest archive `id` wins on tiebreak — newest matches the overwrite-then-stop shape that pre-#1378 reprints left behind); step 2 copies `archive.cost / energy_kwh / energy_cost` onto the latest matching log entry per archive, **but only for archives where no log entry yet carries a cost**. That second clause is the idempotency anchor and also the double-count guard for users running this migration after #1378 has already written cost-bearing rows for new runs — those archives are left untouched. Earlier reprints stay NULL, matching the "first/latest writes, rest stay NULL" convention #1378 introduced. Sum across the legacy reprint chain reproduces sum-of-archive-cost exactly, so the Quick Stats Filament Cost column matches the pre-upgrade total instead of dropping to zero. SQL is plain ANSI — correlated UPDATE with `LIMIT 1` in the SET subquery, `WHERE id IN (SELECT MAX(id) ... GROUP BY archive_id HAVING SUM(CASE WHEN cost IS NOT NULL THEN 1 ELSE 0 END) = 0)` — verified end-to-end on both SQLite (4 unit tests in `test_print_log_backfill_migration.py`) and `postgres:16-alpine + asyncpg` (live container reproduction). For the other widgets the reporter listed (Printer Stats, Filament Trends, By Material, Success by Material, Color Distribution) — those still iterate the archives list on the frontend rather than calling /stats, so they read consistent pre-upgrade data and aren't part of this fix; the inconsistency the reporter saw between Quick Stats and those widgets resolves itself once the backfill brings Quick Stats in line. - **Spoolman: spool "Color Name" edits silently never saved — Bambuddy was writing to a field Spoolman doesn't have (#1357)** — Reporter pgladel edited a spool's Color Name in Spoolman mode, hit Save, and saw the value snap back to the subtype on the next read. Martin shipped #1319 in May to handle "form round-trips the synth value back as if it were user input" — that fix's read/form-prefill half was correct (the `color_name_is_synthesized` flag, the blank-on-synth form init), but the **write half assumed Spoolman has a `color_name` field on Filament**. It doesn't. Verified against the live `FilamentUpdateParameters` schema on Spoolman 0.23.1: `name`, `vendor_id`, `material`, `price`, `density`, `diameter`, `weight`, `spool_weight`, `article_number`, `comment`, `settings_extruder_temp`, `settings_bed_temp`, `color_hex`, `multi_color_hexes`, `multi_color_direction`, `external_id`, `extra` — that's the lot. No `color_name`. Spoolman's PATCH happily returns 200 for `{"color_name": "Red"}` and just **silently discards the unknown key**. So `find_or_create_filament` was either patching a void or creating filament after filament with the same field-that-doesn't-stick (which is what produced the reporter's "BB also created a bunch of new filaments" trail of duplicates on each save attempt). The fix takes the same route as the existing BambuStudio slicer-preset storage: persist color_name on `spool.extra.bambu_color_name` as a JSON-encoded string, register the extra field via `ensure_extra_field` before write (Spoolman 400s on unknown extra keys), and read it back in `_map_spoolman_spool` with priority `spool.extra.bambu_color_name → filament.color_name (forward-compat for any future Spoolman release that adds it) → subtype synth`. Also dropped the now-dead `color_name` passing through `find_or_create_filament` and `create_filament` — Spoolman would discard it anyway and keeping the dead pipe risked the same confusion the next time someone reads this code. The previous "match by name then patch color_name" loop is gone; what survives is the name-match resilience added earlier this turn so an AMS-sync-created filament named `"Glow"` still matches the user-driven edit's composed `"PLA Glow"`, which prevents the duplicate-filament trail. The frontend form's `color_name_is_synthesized` handling is unchanged — that part already worked. Tests rewritten across the three affected suites (`test_spoolman_inventory_methods.py`, `test_spoolman_inventory_helpers.py`, `test_spoolman_inventory_api.py`) to pin the new contract: filament patch never carries `color_name`, route writes to `bambu_color_name` extra, read prefers extra over filament-field over synth. Verified end-to-end against the live Spoolman instance at the reporter's setup (PATCH /filament with color_name → field absent from response; PATCH /spool with extra.bambu_color_name → field present in response). - **Add Smart Plug (HA mode) — search dropdown let users pick entities the schema would reject, surfacing as a cryptic regex error on Save (#1388)** — Reporter MartinNYHC opened the Add Smart Plug dialog, typed a search prefix matching a multi-entity HA device (a Shelly-style outlet exposing one `switch.*` and several `sensor.*` / `binary_sensor.*` siblings under the same friendly-name prefix), clicked one of the entities, filled in the optional power/energy sensors, and clicked Save. The backend returned 422 with the raw Pydantic message `String should match pattern '^(switch|light|input_boolean|script)\.[a-z0-9_]+$'`. After the dropdown closed and the search cleared, the entity-list refetch (with no search param) returned the default-domain-filtered list — which didn't include the user's pick — so `selectedEntity = haEntities.find(...)` was undefined, the field rendered as visually empty (placeholder shown), but `haEntityId` still held the bad value the user had selected. Root cause was at `backend/app/services/homeassistant.py::list_entities`: when a search query was present, the function bypassed the domain filter entirely and returned matches across every HA domain — including ones the `SmartPlugBase.ha_entity_id` regex at `backend/app/schemas/smart_plug.py:17` could never accept. Offering a clickable choice the user can't save is broken UX; the fact that the error message then said `switch|light|input_boolean|script` made it look like a schema problem rather than a search-permissiveness problem. Fix: the allowed-domains filter (`{"switch", "light", "input_boolean", "script"}`, kept in sync with the schema regex) now always runs, and search composes on top of it as an additional substring match against `entity_id` or `friendly_name`. Whitespace-only search strings are treated as no search. Verified the smart-plug code path is unchanged between 0.2.4 and 0.2.4.1 — this bug was latent since the script-domain commit in February 2026 and was only noticed now because the reporter hadn't reopened the modal in months. 5 new regression tests in `backend/tests/unit/services/test_homeassistant_list_entities.py` cover the no-search baseline, the search-still-domain-filters case (the actual #1388 reproduction), the entity_id-or-friendly_name substring match, case-insensitivity, and the whitespace-only edge case. - **H2S with no AMS could not start a print — firmware rejected the dispatch with `07FF_8012` "Failed to get AMS mapping table" (#1386)** — Reporter krootstijn (H2S + no AMS) clicked Print and got an immediate firmware error. Two stacked misclassifications had quietly added H2S to the dual-nozzle code paths over time. The first was in `start_print_job` at `backend/app/services/bambu_mqtt.py:3168` — the `is_h2d` flag was set true for `("H2D", "H2D PRO", "H2DPRO", "H2C", "H2S", "X2D")`. That single flag controlled both the firmware bool→int format (legitimately needed for the whole H-family) *and* the external-spool routing branch (`ext_ams_id = tray_id if is_h2d else 255`) which is only correct for actual dual-nozzle printers. With no AMS, the external-spool sentinel is `254`; the dual-nozzle branch wrote `ams_id=254` into `ams_mapping2` instead of the canonical `255`. The exact failure shape (`07FF_8012`) is even called out in the comment six lines above the bad line — H2S was getting routed straight into the path the comment warned against. The second misclassification was the use_ams=False fallback at `bambu_mqtt.py:3213` (`if ams_mapping and use_ams and not is_h2d`) — meant to skip the safety drop on dual-nozzle printers where `use_ams` controls nozzle routing — also skipped H2S, so the firmware never got a chance to fall back to external-spool mode. A third site at `bambu_mqtt.py:3987` (and its sibling at `backend/app/api/routes/kprofiles.py:119`) classified dual-nozzle by serial prefix `("094", "20P9", "31B8B")`, which is wrong because H2S shares prefix `094` with H2D. Fix splits the conflated flag into two: `is_h_family` (firmware-format gate, includes H2S) and `is_dual_nozzle` (routing/use_ams gate, excludes H2S; prefers the runtime `_is_dual_nozzle` flag set from `device.extruder.info` and falls back to model name for the brief window right after connect). The K-profile delete and the edit route now use the same two-source check instead of the serial prefix. Empirically verified across 9+ stored H2S support bundles (`nozzle_count: 1` in every one) and the reporter's bug log (`07FF_8012` immediately after dispatch). Four new regression tests: `test_h2s_single_external_spool_uses_main_id`, `test_h2s_no_ams_forces_use_ams_false`, `test_h2s_keeps_integer_format_for_calibration_fields`, plus a new `test_h2s_uses_single_nozzle_format` in the K-profile suite. The K-profile detection tests were also updated to set both model name and runtime flag rather than relying on serial prefix, since the source-of-truth has shifted. diff --git a/backend/app/core/database.py b/backend/app/core/database.py index 026ed76c1..5c52b1203 100644 --- a/backend/app/core/database.py +++ b/backend/app/core/database.py @@ -2519,6 +2519,78 @@ async def run_migrations(conn): conn, "CREATE INDEX IF NOT EXISTS ix_print_log_entries_archive_id ON print_log_entries (archive_id)" ) + # Backfill PrintLogEntry → PrintArchive linkage and per-event cost/energy + # for pre-#1378 rows the column-add migration left NULL (#1390). + # + # Without this backfill the user's Quick Stats show Filament Cost = 0 and + # Time Accuracy empty even though their archives carry both, because: + # + # - the new stats queries SUM PrintLogEntry.cost (NULL for old rows) + # - the time-accuracy query JOINs PrintArchive ON archive_id (NULL for + # old rows, so old runs get excluded from the average) + # + # Pre-#1378, archive.cost / energy_kwh / energy_cost were overwritten by + # each rerun, so the current archive values represent the *latest* run. + # Backfilling them onto the latest matching PrintLogEntry per archive + # reconstructs the pre-fix total exactly (sum across archives stays + # unchanged), and leaves earlier reprints with NULL cost so they + # contribute zero — matching the "first/latest writes, rest stay NULL" + # convention #1378 introduced for new prints. + # + # DML, not DDL — use conn.execute() inside a savepoint per _safe_execute's + # own docstring. SQL is plain ANSI (correlated UPDATE, MAX/GROUP BY/HAVING, + # CASE in HAVING) and runs unchanged on SQLite + PostgreSQL; verified + # against postgres:16-alpine + asyncpg. + # + # Step 1: link old log entries to their archive via print_name + printer_id. + # Picks the highest-id matching archive when multiple share the same key + # (newest archive wins — closest to the log's overwrite-then-leave shape). + from sqlalchemy import text as _text + + async with conn.begin_nested(): + await conn.execute( + _text(""" + UPDATE print_log_entries + SET archive_id = ( + SELECT a.id + FROM print_archives a + WHERE a.print_name = print_log_entries.print_name + AND ( + a.printer_id = print_log_entries.printer_id + OR (a.printer_id IS NULL AND print_log_entries.printer_id IS NULL) + ) + ORDER BY a.id DESC + LIMIT 1 + ) + WHERE archive_id IS NULL AND print_name IS NOT NULL + """) + ) + + # Step 2: backfill cost / energy_kwh / energy_cost onto the latest linked + # log entry per archive — the row whose creation time best matches the + # value currently stored on the archive (overwrite-on-reprint semantics + # under the old design). Only fires for archives where NO log entry has + # cost set yet, which gives the migration a clean idempotency property: + # the second pass sees the archive already has a cost-bearing run and + # leaves the rest of its history NULL (instead of marching up the + # ID-ordered list of NULL runs on every pass). + async with conn.begin_nested(): + await conn.execute( + _text(""" + UPDATE print_log_entries + SET cost = (SELECT cost FROM print_archives WHERE id = print_log_entries.archive_id), + energy_kwh = (SELECT energy_kwh FROM print_archives WHERE id = print_log_entries.archive_id), + energy_cost = (SELECT energy_cost FROM print_archives WHERE id = print_log_entries.archive_id) + WHERE id IN ( + SELECT MAX(id) + FROM print_log_entries + WHERE archive_id IS NOT NULL + GROUP BY archive_id + HAVING SUM(CASE WHEN cost IS NOT NULL THEN 1 ELSE 0 END) = 0 + ) + """) + ) + async def seed_notification_templates(): """Seed default notification templates if they don't exist.""" diff --git a/backend/tests/unit/test_print_log_backfill_migration.py b/backend/tests/unit/test_print_log_backfill_migration.py new file mode 100644 index 000000000..449456d2a --- /dev/null +++ b/backend/tests/unit/test_print_log_backfill_migration.py @@ -0,0 +1,247 @@ +"""Regression test for the PrintLogEntry → PrintArchive backfill migration (#1390). + +Reporter IndividualGhost1905 upgraded to 0.2.4.1 (which shipped the per-event +aggregation rewrite from #1378) and saw Quick Stats partially break on old +data: + + - Total Filament Cost = 0 (PrintLogEntry.cost was NULL on pre-upgrade rows) + - Time Accuracy empty for pre-upgrade runs (the new query JOINs on + archive_id, which the column-add migration left NULL) + +#1378's migration added the columns but didn't backfill anything. This test +pins the backfill that the same `run_migrations` pass now performs: + + Step 1: link old log entries to their archive via print_name + printer_id. + Step 2: copy archive.cost / energy_kwh / energy_cost onto the latest + matching log entry per archive (so the sum across archives + reproduces the pre-fix total exactly — pre-#1378, archive.cost + held the LATEST run's value because reprints overwrote it). + +Earlier reprints stay with cost = NULL — matching #1378's "first/latest run +writes, the rest stay NULL" convention for new prints, so reruns don't +double-count. +""" + +from __future__ import annotations + +from datetime import datetime, timedelta, timezone + +import pytest +from sqlalchemy import text +from sqlalchemy.ext.asyncio import create_async_engine + +from backend.app.core.database import run_migrations + + +@pytest.fixture(autouse=True) +def force_sqlite_dialect(monkeypatch): + """Force the SQLite branch in run_migrations regardless of test env settings.""" + from backend.app.core import db_dialect + + monkeypatch.setattr(db_dialect, "is_sqlite", lambda: True) + monkeypatch.setattr(db_dialect, "is_postgres", lambda: False) + from backend.app.core import database as database_module + + monkeypatch.setattr(database_module, "is_sqlite", lambda: True) + + +def _register_all_models(): + """Import every model so Base.metadata knows the full schema.""" + from backend.app.models import ( # noqa: F401 + ams_history, + ams_label, + api_key, + archive, + color_catalog, + external_link, + filament, + group, + kprofile_note, + maintenance, + notification, + notification_template, + print_log, + print_queue, + printer, + project, + project_bom, + settings, + slot_preset, + smart_plug, + smart_plug_energy_snapshot, + spool, + spool_assignment, + spool_catalog, + spool_k_profile, + spool_usage_history, + spoolbuddy_device, + user, + user_email_pref, + virtual_printer, + ) + + +@pytest.fixture +async def engine_with_legacy_data(): + """Fresh schema + a legacy-shape dataset: two archives, four PrintLogEntry + rows. The cube.3mf archive carries cost+energy (the user's reprinted file); + gear.3mf has neither set. Three matching log entries simulate cube's + reprint history (status: failed → completed → completed). All log entries + start with archive_id and cost = NULL, exactly like the column-add + migration leaves on a pre-#1378 install.""" + from sqlalchemy.ext.asyncio import async_sessionmaker + + from backend.app.core.database import Base + from backend.app.models.archive import PrintArchive + + _register_all_models() + + engine = create_async_engine("sqlite+aiosqlite:///:memory:", echo=False) + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + + SessionLocal = async_sessionmaker(engine, expire_on_commit=False) + async with SessionLocal() as session: + session.add( + PrintArchive( + id=1, + filename="cube.3mf", + file_path="/x/cube.3mf", + file_size=100, + print_name="cube.3mf", + printer_id=1, + cost=4.25, + energy_kwh=0.42, + energy_cost=0.063, + status="completed", + ) + ) + session.add( + PrintArchive( + id=2, + filename="gear.3mf", + file_path="/x/gear.3mf", + file_size=100, + print_name="gear.3mf", + printer_id=1, + status="completed", + ) + ) + await session.commit() + + async with engine.begin() as conn: + # Three log entries for cube.3mf (two early reprints + a latest run), + # one for gear.3mf. All with archive_id and cost NULL — exactly the + # state the column-add migration leaves on pre-#1378 installs. + base = datetime.now(timezone.utc) - timedelta(days=10) + for i, (delta_days, status, print_name) in enumerate( + [ + (0, "failed", "cube.3mf"), + (1, "completed", "cube.3mf"), + (2, "completed", "cube.3mf"), # latest run for cube — must receive backfill + (3, "completed", "gear.3mf"), + ], + start=1, + ): + ts = (base + timedelta(days=delta_days)).isoformat() + await conn.execute( + text(""" + INSERT INTO print_log_entries + (id, print_name, printer_id, status, started_at, completed_at, + duration_seconds, filament_used_grams, created_at) + VALUES (:id, :pn, 1, :status, :ts, :ts, 3600, 25.0, :ts) + """), + {"id": i, "pn": print_name, "status": status, "ts": ts}, + ) + + # Force NULL on the columns we want the migration to touch — the + # CREATE TABLE from Base.metadata.create_all already left them NULL, + # but we set explicitly so the fixture's intent is loud. + await conn.execute( + text("UPDATE print_log_entries SET archive_id = NULL, cost = NULL, energy_kwh = NULL, energy_cost = NULL") + ) + + yield engine + await engine.dispose() + + +async def test_backfill_links_log_entries_to_their_archive(engine_with_legacy_data): + """All four entries should pick up archive_id after the migration runs.""" + async with engine_with_legacy_data.begin() as conn: + await run_migrations(conn) + + async with engine_with_legacy_data.connect() as conn: + result = await conn.execute(text("SELECT id, print_name, archive_id FROM print_log_entries ORDER BY id")) + rows = result.all() + + assert rows == [ + (1, "cube.3mf", 1), + (2, "cube.3mf", 1), + (3, "cube.3mf", 1), + (4, "gear.3mf", 2), + ] + + +async def test_backfill_copies_cost_and_energy_to_latest_run_only(engine_with_legacy_data): + """Pre-#1378 archive.cost = LAST run's value because reprints overwrote it. + The backfill attributes that cost to the latest matching log entry; earlier + runs stay NULL so summing across runs reproduces sum-of-archive-costs + exactly — what the user saw before the upgrade.""" + async with engine_with_legacy_data.begin() as conn: + await run_migrations(conn) + + async with engine_with_legacy_data.connect() as conn: + result = await conn.execute(text("SELECT id, cost, energy_kwh, energy_cost FROM print_log_entries ORDER BY id")) + rows = result.all() + + # Two earlier cube runs (id 1, 2): cost stays NULL. + assert rows[0] == (1, None, None, None) + assert rows[1] == (2, None, None, None) + # Latest cube run (id 3): receives archive 1's cost / energy. + assert rows[2] == (3, 4.25, 0.42, 0.063) + # gear run (id 4): archive 2 has no cost/energy so log stays NULL too. + assert rows[3] == (4, None, None, None) + + +async def test_backfill_is_idempotent(engine_with_legacy_data): + """Running the migration twice produces the same state — no double-backfill, + no values pulled off rows the second pass would mistakenly treat as 'new'.""" + async with engine_with_legacy_data.begin() as conn: + await run_migrations(conn) + async with engine_with_legacy_data.begin() as conn: + await run_migrations(conn) + + async with engine_with_legacy_data.connect() as conn: + result = await conn.execute(text("SELECT id, archive_id, cost FROM print_log_entries ORDER BY id")) + rows = result.all() + + assert rows == [ + (1, 1, None), + (2, 1, None), + (3, 1, 4.25), + (4, 2, None), + ] + + +async def test_backfill_skips_archives_with_any_costed_run(engine_with_legacy_data): + """If ANY log entry for an archive already has cost set — e.g. the post-#1378 + live write path filled it for a new run — the backfill leaves the entire + archive alone. This is the migration's idempotency anchor: 'cost is + accounted for somewhere on this archive's history' is the signal we use + to decide whether to inject the archive-level value. Backfilling another + row would double-count once the live writes start adding up.""" + async with engine_with_legacy_data.begin() as conn: + # Pretend run #1 was written post-fix with its own cost. + await conn.execute(text("UPDATE print_log_entries SET cost = 1.11 WHERE id = 1")) + await run_migrations(conn) + + async with engine_with_legacy_data.connect() as conn: + result = await conn.execute(text("SELECT id, cost FROM print_log_entries ORDER BY id")) + rows = result.all() + + # Run #1 keeps its live-written cost. The archive already has a costed + # run, so the migration does NOT inject archive.cost onto run #3. + # gear.3mf (archive 2) still has nothing — but archive.cost is NULL + # there too, so the backfill UPDATE would set NULL → NULL anyway, which + # is the desired no-op. + assert dict(rows) == {1: 1.11, 2: None, 3: None, 4: None}