Browse Source

fix(spoolman): decide spool assignability from the slot-assignment ledger, not extra.tag (#1122)

  GET /spoolman/spools/unlinked hid any spool with a non-empty
  extra.tag from the AMS-slot assignment picker. extra.tag is only an
  RFID/NFC matching key -- OpenSpoolman writes its own NFC tag value
  into that same Spoolman field -- so every OpenSpoolman-tagged spool
  became un-assignable in Bambuddy even when it occupied no slot.

  get_unlinked_spools now determines assignability from the
  spoolman_slot_assignments table (the documented source of truth for
  slot assignments) and ignores extra.tag entirely. Both link_spool and
  the AMS auto-sync upsert a row there for every occupied slot, so the
  ledger is complete. get_linked_spools and find_spool_by_tag still use
  extra.tag -- they are genuine tag-match maps and are unaffected.

  Internal-inventory mode needs no parallel change: it stores tags in
  its own DB with no Spoolman extra collision.

  Updates test_get_unlinked_spools_success and adds
  test_get_unlinked_spools_excludes_slot_assigned.
maziggy 1 week ago
parent
commit
e1a236e408
3 changed files with 51 additions and 34 deletions
  1. 0 0
      CHANGELOG.md
  2. 26 19
      backend/app/api/routes/spoolman.py
  3. 25 15
      backend/tests/integration/test_spoolman_api.py

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 26 - 19
backend/app/api/routes/spoolman.py

@@ -655,7 +655,7 @@ async def get_unlinked_spools(
     db: AsyncSession = Depends(get_db),
     db: AsyncSession = Depends(get_db),
     _: User | None = RequirePermissionIfAuthEnabled(Permission.FILAMENTS_READ),
     _: User | None = RequirePermissionIfAuthEnabled(Permission.FILAMENTS_READ),
 ):
 ):
-    """Get all Spoolman spools that don't have a tag (not linked to AMS)."""
+    """Get all Spoolman spools not currently assigned to an AMS slot."""
     sm = await get_spoolman_settings(db)
     sm = await get_spoolman_settings(db)
     enabled, url = sm["enabled"], sm["url"]
     enabled, url = sm["enabled"], sm["url"]
     if not enabled:
     if not enabled:
@@ -672,27 +672,34 @@ async def get_unlinked_spools(
         raise HTTPException(status_code=503, detail="Spoolman is not reachable")
         raise HTTPException(status_code=503, detail="Spoolman is not reachable")
 
 
     spools = await client.get_spools()
     spools = await client.get_spools()
-    unlinked = []
 
 
+    # A spool is "assignable" iff it does not currently occupy an AMS slot.
+    # Assignability is decided by the spoolman_slot_assignments ledger — NOT by
+    # the presence of extra.tag. extra.tag is only an RFID/NFC matching key, and
+    # OpenSpoolman writes its own NFC tag value into that same field (#1122);
+    # treating any non-empty extra.tag as "linked" hid every OpenSpoolman-tagged
+    # spool from this picker even when it occupied no slot. Both link_spool and
+    # the AMS auto-sync upsert a row here for every occupied slot, so the ledger
+    # is a complete record of what is actually assigned.
+    assigned_result = await db.execute(select(SpoolmanSlotAssignment.spoolman_spool_id))
+    assigned_spool_ids = set(assigned_result.scalars().all())
+
+    unlinked = []
     for spool in spools:
     for spool in spools:
-        # Check if spool has a tag in extra field
-        extra = spool.get("extra", {}) or {}
-        tag = extra.get("tag", "")
-        # Remove quotes if present (JSON encoded string) and check if empty
-        clean_tag = tag.strip('"') if tag else ""
-        if not clean_tag:
-            filament = spool.get("filament", {}) or {}
-            unlinked.append(
-                UnlinkedSpool(
-                    id=spool["id"],
-                    filament_name=filament.get("name"),
-                    filament_vendor=(filament.get("vendor") or {}).get("name"),
-                    filament_material=filament.get("material"),
-                    filament_color_hex=filament.get("color_hex"),
-                    remaining_weight=spool.get("remaining_weight"),
-                    location=spool.get("location"),
-                )
+        if spool["id"] in assigned_spool_ids:
+            continue
+        filament = spool.get("filament", {}) or {}
+        unlinked.append(
+            UnlinkedSpool(
+                id=spool["id"],
+                filament_name=filament.get("name"),
+                filament_vendor=(filament.get("vendor") or {}).get("name"),
+                filament_material=filament.get("material"),
+                filament_color_hex=filament.get("color_hex"),
+                remaining_weight=spool.get("remaining_weight"),
+                location=spool.get("location"),
             )
             )
+        )
 
 
     return unlinked
     return unlinked
 
 

+ 25 - 15
backend/tests/integration/test_spoolman_api.py

@@ -205,13 +205,18 @@ class TestSpoolmanAPI:
     async def test_get_unlinked_spools_success(
     async def test_get_unlinked_spools_success(
         self, async_client: AsyncClient, spoolman_settings, mock_spoolman_client
         self, async_client: AsyncClient, spoolman_settings, mock_spoolman_client
     ):
     ):
-        """Verify get unlinked spools returns spools without tags."""
-        # Mock spool without extra.tag (unlinked)
+        """A spool with no slot assignment is assignable even when extra.tag is set.
+
+        #1122 — extra.tag is only an RFID/NFC matching key (OpenSpoolman writes
+        its own NFC tag value there too); it must NOT gate assignability. A spool
+        with a non-empty extra.tag but no spoolman_slot_assignments row still
+        appears in the picker.
+        """
         mock_spool = {
         mock_spool = {
             "id": 1,
             "id": 1,
             "remaining_weight": 800,
             "remaining_weight": 800,
             "used_weight": 200,
             "used_weight": 200,
-            "extra": {},  # No tag = unlinked
+            "extra": {"tag": '"04A1B2C3D4E5F6"'},  # OpenSpoolman-style NFC tag value
             "filament": {
             "filament": {
                 "id": 1,
                 "id": 1,
                 "name": "PLA Basic",
                 "name": "PLA Basic",
@@ -232,35 +237,40 @@ class TestSpoolmanAPI:
 
 
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
-    async def test_get_unlinked_spools_excludes_linked(
-        self, async_client: AsyncClient, spoolman_settings, mock_spoolman_client
+    async def test_get_unlinked_spools_excludes_slot_assigned(
+        self, async_client: AsyncClient, spoolman_settings, mock_spoolman_client, printer_factory, db_session
     ):
     ):
-        """Verify linked spools (with tag) are excluded."""
-        # Mock spool with extra.tag (linked)
-        mock_spool_linked = {
+        """Verify spools that currently occupy an AMS slot are excluded."""
+        from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment
+
+        printer = await printer_factory()
+
+        # Spool 1 occupies a slot; spool 2 has an extra.tag but no slot row.
+        db_session.add(SpoolmanSlotAssignment(printer_id=printer.id, ams_id=0, tray_id=1, spoolman_spool_id=1))
+        await db_session.commit()
+
+        mock_spool_assigned = {
             "id": 1,
             "id": 1,
             "remaining_weight": 800,
             "remaining_weight": 800,
             "used_weight": 200,
             "used_weight": 200,
-            "extra": {"tag": '"ABC123"'},  # Has tag = linked
+            "extra": {"tag": '"A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4"'},
             "filament": {"id": 1, "name": "PLA Red", "material": "PLA", "color_hex": "FF0000"},
             "filament": {"id": 1, "name": "PLA Red", "material": "PLA", "color_hex": "FF0000"},
         }
         }
-
-        # Mock spool without tag (unlinked)
-        mock_spool_unlinked = {
+        mock_spool_unassigned = {
             "id": 2,
             "id": 2,
             "remaining_weight": 900,
             "remaining_weight": 900,
             "used_weight": 100,
             "used_weight": 100,
-            "extra": {},  # No tag = unlinked
+            "extra": {"tag": '"04DEADBEEF1122"'},  # tagged but not slot-assigned
             "filament": {"id": 2, "name": "PLA Blue", "material": "PLA", "color_hex": "0000FF"},
             "filament": {"id": 2, "name": "PLA Blue", "material": "PLA", "color_hex": "0000FF"},
         }
         }
 
 
-        mock_spoolman_client.get_spools = AsyncMock(return_value=[mock_spool_linked, mock_spool_unlinked])
+        mock_spoolman_client.get_spools = AsyncMock(return_value=[mock_spool_assigned, mock_spool_unassigned])
 
 
         response = await async_client.get("/api/v1/spoolman/spools/unlinked")
         response = await async_client.get("/api/v1/spoolman/spools/unlinked")
         assert response.status_code == 200
         assert response.status_code == 200
         data = response.json()
         data = response.json()
         assert len(data) == 1
         assert len(data) == 1
-        assert data[0]["id"] == 2  # Only unlinked spool
+        assert data[0]["id"] == 2  # Only the spool not occupying a slot
 
 
     # =========================================================================
     # =========================================================================
     # Linked Spools Tests
     # Linked Spools Tests

Some files were not shown because too many files changed in this diff