diff --git a/CHANGELOG.md b/CHANGELOG.md index 135fd3452..2ecc4dbfc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ All notable changes to Bambuddy will be documented in this file. - **The Windows installer build is split in two so a signing request can wait for a human (SignPath Foundation)** — Release tags are Authenticode-signed through the SignPath Foundation OSS programme, and the production certificate does not sign on demand the way the self-signed test certificate does: every request has to be approved by hand in the SignPath UI, because the Foundation verifies what is being signed and which build it came from. The submitting action waits for that approval with a default timeout of 600 seconds, which is ample when the test policy approves automatically in seconds and far too short once the wait is a person noticing a tag went out. A tag pushed at night would have failed the run ten minutes later with the installer already compiled and thrown away. The compile now ends in its own job that uploads the unsigned artifact and stops; a second job downloads it, signs it, and does the release-facing work, with the wait raised to an hour. Because the artifact is uploaded before the wait begins and is addressed by id, a missed approval window is recovered by re-running the second job alone rather than rebuilding the installer — which is the reason to separate them rather than simply raise the timeout in place. The second job runs for unsigned builds too, so the daily prereleases that are deliberately left unsigned to preserve the signing quota keep going out through exactly one set of alias, artifact and release steps. The property that matters is unchanged and now recorded next to the steps that depend on it: none of the alias, upload or release-attach steps carry `always()`, so GitHub skips all three when signing fails or times out, and an unsigned `.exe` cannot reach a release. Nothing about the signed output changes, and the restructure behaves identically under the test policy — the request simply completes immediately instead of waiting — so it can be proven green before the production certificate arrives. ### Fixed +- **AMS slots were offered as places to store a spool** — The Storage Location dropdown in the spool editor listed entries like "H2D-1 - AMS A1" alongside real locations, and they could not be removed. They are not locations at all: Bambuddy used to record which slot a spool was loaded into by writing that string into Spoolman's `location` field, and although that writer went away when Storage Location became something the user picks, the strings stayed on people's Spoolman spools — where the location sync, which imports every distinct one it finds, has been reading them back ever since. A printer slot is where a spool is loaded, not where it is put away, and Bambuddy already tracks the first through slot assignments. Deleting one by hand did not work either, which is what made this a dead end rather than an annoyance: the delete route refuses a location that has spools, and in Spoolman mode it counts them by matching that same string, so every marker still sitting on a loaded spool answered 409 — and the two that were empty came back on the next sync a minute later. The import now skips them, and the ones already in the catalogue are removed on upgrade. The filter is deliberately narrow, matching only the shape Bambuddy itself wrote — an optional printer-name prefix followed by `AMS A1`, `AMS-HT A1` or `External Spool` — so "AMS Drybox" and "Spare AMS trays" are left alone; anything it swallowed would be a place the user could no longer file a spool under. A row is only removed when no spool in Bambuddy's own database points at it, by id or by legacy free-text name, so an internal-mode user who has deliberately filed spools under such a name keeps it. Spools in Spoolman are not touched: their location strings are the user's data on the user's server, and one that still reads "H2D-1 - AMS A1" in the inventory list is telling the truth about what Spoolman holds — it simply stops being offered as a destination. - **A wood, silk or gradient roll the AMS added for you was drawn as a flat disc** — A spool's swatch is composed from `effect_type` and `extra_colors`, and the RFID auto-add set neither. It reads the colour catalogue to name the colour and took the name alone, even though the row it had in hand also carries those two columns — the spool form's own colour picker hands both to a spool a user adds by hand, so the same roll rendered one way when you typed it in and another when the printer identified it for you. Both columns now travel with the name. That alone would have changed nothing on a stock install, because the shipped catalogue carries an effect on none of its 600-odd rows, so the subtype is read where the catalogue has none: it is already derived from what the printer reports, and the two vocabularies line up — Wood, Silk, Sparkle, Marble, Glow, Galaxy, Metal, Rainbow, Translucent, Matte, and the Gradient, Dual Color and Tri Color that the M*/T* colour codes upgrade a subtype to. "Silk+" is read as Silk, since the plus is on the product name rather than the finish. A subtype that names no effect — Basic, Tough, CF — leaves the column empty rather than inventing an overlay, and a value already set is never overwritten, so the column stays what it is documented to be: a rendering hint the user can override without touching Bambu's categorical label. - **The spool tare an RFID roll was added with is corrected on upgrade (#2909, diagnosed by @ojimpo)** — The lookup that gave an auto-added spool its `core_weight` asked for the first catalogue row whose name starts "Bambu Lab" and took whatever came back. There are three, and which is first is the database's business: SQLite returns insertion order in practice, Postgres promises nothing once a table has seen an update, so the same roll was recorded with the 216 g High Temp tare on one install and correctly with the 250 g Low Temp one on another. The forward fix picks the row by name; this repairs the rows already written, which the forward fix cannot reach. The tare is not cosmetic: a spool weighed on SpoolBuddy has its remaining filament worked out as the scale reading minus the tare, so a 34 g low tare credits the roll with 34 g that is not there and writes a used weight 34 g short. That error is a constant — every later print adds to the used weight on top of it — so adding the difference back is exact however much has been printed since, and it is applied only to spools that have actually been on the scale; one that never was has a used weight derived from the AMS remaining percentage, which the tare never entered into. The rows to repair are identified by the signature of the broken lookup — added by RFID, carrying the weight of one of the *other* Bambu catalogue rows — with the weights read out of the catalogue rather than hardcoded, so an install whose rows have been re-measured is repaired to its own numbers. One case cannot be told apart and is stated rather than hidden: someone who moved an RFID roll onto a genuine High Temp spool and set 216 g by hand looks identical to a row the lookup got wrong and is normalised with them. Keying on whether a catalogue row had been recorded would not have rescued them either — the weight picker auto-selects the only row matching the weight and writes its id on the next save, so that column says only whether the form was ever opened. Runs exactly once, so a tare set afterwards is kept. - **A wood-filled spool was named as plain PLA on the slot it was assigned to** — A spool's subtype is half of what it is called: "PLA" and "PLA Wood" are different filaments, and the AMS slot's hover card built the assigned-spool line out of brand, material and colour name with the subtype left out. A roll of Bambu PLA Wood Classic Birch in an H2C's A4 was therefore announced as "Bambu Lab PLA - Classic Birch". Everything else named it correctly at the same moment — the RFID read, the inventory row, the slot's own profile line, which is built from the spool's slicer preset rather than reassembled, and Bambu Studio — so the one wrong line read like a bad tag read rather than a display fault. It was not only the render: the card's `assignedSpool` prop had no subtype field at all, and the six places the printer card fills it in (regular AMS, AMS-HT and external spool, each in both Spoolman and internal-inventory mode) never passed one, so the value could not reach the component. The field is required now rather than optional, which is what stops the next call site from quietly omitting it — that omission is the whole of this bug. Three more surfaces were rebuilding the name the same way and are fixed with it: the SpoolBuddy AMS slot panel in both inventory modes, and the write-tag confirmation. Every other place a spool is named — the assign dialogs, the inventory cards, the forecast rows, the label picker — already included the subtype, so these four were the outliers. This is the display-side half of #2902, which stopped the backend reducing a filled or foamed filament onto its base material; the card was doing the same thing to the same spools, one layer further out. diff --git a/backend/app/core/database.py b/backend/app/core/database.py index 24ec8d463..0b4553377 100644 --- a/backend/app/core/database.py +++ b/backend/app/core/database.py @@ -4432,6 +4432,81 @@ async def run_migrations(conn): # whatever this database actually holds. await _migrate_repair_rfid_core_weight(conn) + # Migration: drop the AMS slot markers an older Bambuddy wrote into + # Spoolman and the location sync then imported as storage locations. + await _migrate_drop_ams_slot_locations(conn) + + +async def _migrate_drop_ams_slot_locations(conn) -> None: + """Remove imported AMS slot markers from the storage-location catalogue. + + Bambuddy used to record which slot a spool was loaded into by writing + " - AMS A1" into Spoolman's ``location`` field. That writer went + away when Storage Location became something the user picks (#1114), but the + strings stayed on people's Spoolman spools, and + ``sync_locations_from_spoolman`` imported every distinct one -- so a printer + slot turned up in the Storage Location dropdown as somewhere to put a spool + away. Worse, they could not be cleared by hand: the delete route refuses a + location that has spools, and in Spoolman mode it counts them by matching + that same string, so every marker still on a loaded spool answered 409. + + The import now skips them (``is_ams_slot_location``); this clears the ones + already in the catalogue. A row is only deleted when no spool in this + database points at it -- neither by ``location_id`` nor by a legacy + free-text ``storage_location`` -- so an internal-mode user who has + deliberately filed spools under such a name keeps it, dropdown entry and + all. + + Spools in Spoolman are not consulted and not touched: their ``location`` + strings are the user's data on the user's server, and one that still reads + "H2D-1 - AMS A1" in the inventory list is telling the truth about what + Spoolman holds. It simply stops being offered as a destination, which is + the whole point -- those are exactly the markers this cleans up. + """ + from sqlalchemy import text + + from backend.app.services.location_service import is_ams_slot_location, location_name_key + + flag = "_cleanup_ams_slot_locations_done" + + async with conn.begin_nested(): + already = ( + await conn.execute(text('SELECT value FROM settings WHERE "key" = :k'), {"k": flag}) + ).scalar_one_or_none() + if already: + return + + rows = (await conn.execute(text("SELECT id, name FROM locations"))).fetchall() + removed = [] + for row in rows: + if not is_ams_slot_location(row.name): + continue + in_use = ( + await conn.execute( + text( + "SELECT COUNT(*) FROM spool WHERE location_id = :id " + "OR LOWER(TRIM(COALESCE(storage_location, ''))) = :key" + ), + {"id": row.id, "key": location_name_key(row.name)}, + ) + ).scalar_one() + if in_use: + continue + await conn.execute(text("DELETE FROM locations WHERE id = :id"), {"id": row.id}) + removed.append(row.name) + + if removed: + logger.info( + "Removed %d AMS slot marker(s) from the storage-location catalogue: %s", + len(removed), + ", ".join(sorted(removed)), + ) + + await conn.execute( + text('INSERT INTO settings ("key", value) VALUES (:k, :v)'), + {"k": flag, "v": "true"}, + ) + async def _migrate_repair_rfid_core_weight(conn) -> None: """Correct the tare of RFID-added spools that took the wrong catalogue row (#2909). diff --git a/backend/app/services/location_service.py b/backend/app/services/location_service.py index 97c862d10..8ddcb4b8c 100644 --- a/backend/app/services/location_service.py +++ b/backend/app/services/location_service.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging +import re import time from dataclasses import dataclass @@ -18,6 +19,24 @@ logger = logging.getLogger(__name__) DUPLICATE_LOCATION_NAME = "A location with this name already exists" +# AMS residency markers, not storage locations. Bambuddy used to write the slot +# a spool was loaded into -- " - AMS A1", the shape +# `SpoolmanClient.convert_ams_slot_to_location` still produces -- straight into +# Spoolman's `location` field. That writer went away when Storage Location +# became a place the user chooses (#1114), but the strings survive on people's +# Spoolman spools, and importing them offers a printer slot as somewhere to put +# a spool away. A slot is where a spool is loaded, not where it is stored, and +# Bambuddy tracks that separately through slot assignments. +_AMS_SLOT_LOCATION_RE = re.compile( + r"^(?:.+\s-\s)?(?:AMS[- ]HT [A-Z]\d+|AMS [A-Z]\d+|External Spool)$", + re.IGNORECASE, +) + + +def is_ams_slot_location(name: str) -> bool: + """True when a location string names a printer slot rather than a storage place.""" + return bool(_AMS_SLOT_LOCATION_RE.match(name.strip())) + def normalize_location_name(name: str) -> str: trimmed = name.strip() @@ -281,6 +300,9 @@ async def sync_locations_from_spoolman(db: AsyncSession, client) -> bool: name = (raw or "").strip() if not name: continue + if is_ams_slot_location(name): + logger.debug("Skipping AMS slot marker %r from the Spoolman location import", name) + continue key = location_name_key(name) if key not in by_key: by_key[key] = name diff --git a/backend/tests/unit/test_ams_slot_location_cleanup.py b/backend/tests/unit/test_ams_slot_location_cleanup.py new file mode 100644 index 000000000..e8a21075d --- /dev/null +++ b/backend/tests/unit/test_ams_slot_location_cleanup.py @@ -0,0 +1,148 @@ +"""Cleanup of AMS slot markers imported into the storage-location catalogue. + +Bambuddy used to write the slot a spool was loaded into -- " - AMS A1" +-- into Spoolman's ``location`` field, and the location sync then imported every +distinct one as a storage location. ``_migrate_drop_ams_slot_locations`` clears +the rows that already landed; the import side is covered in +``test_location_service.py``. +""" + +import pytest +from sqlalchemy import text +from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine + +import backend.app.models # noqa: F401 - populate Base.metadata +from backend.app.core.database import Base, _migrate_drop_ams_slot_locations +from backend.app.models.location import Location +from backend.app.models.spool import Spool +from backend.app.services.location_service import assign_location_name + +FLAG = "_cleanup_ams_slot_locations_done" + + +@pytest.fixture +async def engine(tmp_path): + eng = create_async_engine(f"sqlite+aiosqlite:///{tmp_path}/t.db") + async with eng.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + try: + yield eng + finally: + await eng.dispose() + + +def _location(name: str) -> Location: + loc = Location() + assign_location_name(loc, name) + return loc + + +async def _names(db) -> set[str]: + return {r[0] for r in (await db.execute(text("SELECT name FROM locations"))).fetchall()} + + +async def _run(engine): + async with engine.begin() as conn: + await _migrate_drop_ams_slot_locations(conn) + + +@pytest.mark.asyncio +async def test_removes_the_slot_markers_and_keeps_real_locations(engine): + sm = async_sessionmaker(engine, expire_on_commit=False) + async with sm() as db: + db.add_all( + [ + _location("H2D-1 - AMS A1"), + _location("H2D-1 - AMS C3"), + _location("X1C-2 - AMS-HT A1"), + _location("P1S - External Spool"), + _location("Drybox 1"), + _location("Shelf A"), + ] + ) + await db.commit() + + await _run(engine) + + async with sm() as db: + assert await _names(db) == {"Drybox 1", "Shelf A"} + + +@pytest.mark.asyncio +async def test_keeps_a_marker_a_spool_is_actually_filed_under(engine): + """Deleting it would strand the spool's location, and someone who has + deliberately filed spools under that name meant it.""" + sm = async_sessionmaker(engine, expire_on_commit=False) + async with sm() as db: + loc = _location("H2D-1 - AMS A1") + db.add(loc) + await db.flush() + db.add( + Spool( + material="PLA", + label_weight=1000, + location_id=loc.id, + storage_location="H2D-1 - AMS A1", + ) + ) + await db.commit() + + await _run(engine) + + async with sm() as db: + assert await _names(db) == {"H2D-1 - AMS A1"} + + +@pytest.mark.asyncio +async def test_keeps_a_marker_a_legacy_free_text_spool_still_names(engine): + """Rows predating the location catalogue carry the name without the FK, and + the rename cascade still matches them on it.""" + sm = async_sessionmaker(engine, expire_on_commit=False) + async with sm() as db: + db.add(_location("H2D-1 - AMS A1")) + await db.flush() + # Whitespace and case around the name are the legacy shape the rename + # cascade already has to cope with, so the guard has to match it too. + db.add(Spool(material="PLA", label_weight=1000, storage_location=" h2d-1 - ams a1 ")) + await db.commit() + + await _run(engine) + + async with sm() as db: + assert await _names(db) == {"H2D-1 - AMS A1"} + + +@pytest.mark.asyncio +async def test_runs_exactly_once(engine): + """A location the user creates afterwards is theirs, whatever it is named.""" + sm = async_sessionmaker(engine, expire_on_commit=False) + async with sm() as db: + db.add(_location("H2D-1 - AMS A1")) + await db.commit() + + await _run(engine) + + async with sm() as db: + db.add(_location("H2D-1 - AMS B2")) + await db.commit() + + await _run(engine) + + async with sm() as db: + assert await _names(db) == {"H2D-1 - AMS B2"} + + +@pytest.mark.asyncio +async def test_marks_itself_done_on_an_install_with_nothing_to_remove(engine): + """Otherwise the whole catalogue is rescanned on every boot for ever.""" + sm = async_sessionmaker(engine, expire_on_commit=False) + async with sm() as db: + db.add(_location("Drybox 1")) + await db.commit() + + await _run(engine) + + async with sm() as db: + done = (await db.execute(text('SELECT value FROM settings WHERE "key" = :k'), {"k": FLAG})).scalar_one_or_none() + assert done == "true" + assert await _names(db) == {"Drybox 1"} diff --git a/backend/tests/unit/test_location_service.py b/backend/tests/unit/test_location_service.py index f3f1b6417..960b9ae7c 100644 --- a/backend/tests/unit/test_location_service.py +++ b/backend/tests/unit/test_location_service.py @@ -9,6 +9,7 @@ from backend.app.services.location_service import ( assign_location_name, enrich_spool_dicts_with_location_id, get_location_by_name, + is_ams_slot_location, location_name_key, prepare_internal_spool_payload, rename_location, @@ -217,3 +218,74 @@ async def test_sync_locations_from_spoolman_handles_dict_payload(db_session: Asy cabinet = await get_location_by_name(db_session, "Cabinet 3") assert cabinet is not None + + +class TestIsAmsSlotLocation: + """A printer slot is where a spool is loaded, not where it is stored.""" + + @pytest.mark.parametrize( + "name", + [ + "H2D-1 - AMS A1", + "X1C-2 - AMS C3", + "P1S - AMS-HT A1", + "H2D-1 - AMS HT B1", + "AMS A1", + "AMS-HT A1", + "External Spool", + "H2D-1 - External Spool", + "h2d-1 - ams a1", + " H2D-1 - AMS A1 ", + ], + ) + def test_slot_markers_are_recognised(self, name): + assert is_ams_slot_location(name) is True + + @pytest.mark.parametrize( + "name", + [ + "Drybox 1", + "Shelf A", + "AMS Drybox", + "Spare AMS trays", + "Locker - Top", + "AMS A1 spares", + "dadadad", + ], + ) + def test_real_storage_locations_are_kept(self, name): + """The filter has to be narrow: anything it swallows is a place the user + can no longer file a spool under.""" + assert is_ams_slot_location(name) is False + + +@pytest.mark.asyncio +async def test_sync_locations_from_spoolman_skips_ams_slot_markers(db_session: AsyncSession): + """Bambuddy used to write the loaded slot into Spoolman's `location` field. + Importing those back offered a printer slot as a storage location, and in + Spoolman mode they could not even be deleted -- the delete route counts + spools by that same string and answered 409.""" + + class FakeClient: + async def get_distinct_locations(self): + return ["H2D-1 - AMS A1", "X1C-2 - AMS A1", "H2D-1 - External Spool", "Drybox 1"] + + changed = await sync_locations_from_spoolman(db_session, FakeClient()) + assert changed is True + await db_session.commit() + + assert await get_location_by_name(db_session, "Drybox 1") is not None + for marker in ("H2D-1 - AMS A1", "X1C-2 - AMS A1", "H2D-1 - External Spool"): + assert await get_location_by_name(db_session, marker) is None + + +@pytest.mark.asyncio +async def test_sync_locations_from_spoolman_reports_no_change_when_only_markers(db_session: AsyncSession): + """`changed` drives the caller's commit — claiming a change for rows that + were all filtered out would open a write transaction on every poll.""" + + class FakeClient: + async def get_distinct_locations(self): + return ["H2D-1 - AMS A1", "H2D-1 - AMS A2"] + + assert await sync_locations_from_spoolman(db_session, FakeClient()) is False