فهرست منبع

fix(spoolman): edit-spool patches the linked filament in place when singleton (#1357 follow-up)

  Editing a Spoolman spool used to mint a brand-new filament every time
  a match-key field (subtype/material/brand/color_hex) changed, orphan
  the previous one, and re-link the spool. The reporter ended up with
  dozens of duplicate "Amazon Basics / PLA Glow" filament rows.

  PATCH /spoolman/inventory/spools/{id} now:

  - Reuses the current filament_id when no filament-shaping field
    changed (a note/weight_used edit never touches the catalogue).
  - PATCHes the existing filament in place when it's a singleton
    (only this spool points at it, archived spools included).
  - Falls back to find_or_create_filament only when the filament is
    genuinely shared with another spool.

  Mirrors internal-inventory behaviour where editing a spool updates
  the thing the spool points at instead of proliferating new entities.
maziggy 1 هفته پیش
والد
کامیت
3b552094a7

تفاوت فایلی نمایش داده نمی شود زیرا این فایل بسیار بزرگ است
+ 0 - 0
CHANGELOG.md


+ 60 - 9
backend/app/api/routes/spoolman_inventory.py

@@ -597,15 +597,66 @@ async def update_spool(
     storage_location = data.storage_location if storage_location_changed else None
     storage_location = data.storage_location if storage_location_changed else None
 
 
     color_hex = rgba[:6]
     color_hex = rgba[:6]
-    async with _translate_spoolman_errors():
-        filament_id = await client.find_or_create_filament(
-            material=material,
-            subtype=subtype or "",
-            brand=brand,
-            color_hex=color_hex,
-            label_weight=label_weight,
-            color_name=color_name,
-        )
+
+    # Resolve which filament this spool should be linked to AFTER the edit.
+    #
+    # The old behaviour was always `find_or_create_filament`, which proliferated
+    # duplicate Spoolman filaments whenever the user changed any field that
+    # made up the match key (material/subtype/brand/color) — every edit minted
+    # a fresh row and orphaned the previous one (#1357 follow-up). To match
+    # internal-mode behaviour ([[feedback_inventory_modes_parity]]: editing a
+    # spool does not proliferate new entities), prefer PATCHing the current
+    # filament in place when it's a singleton.
+    cur_filament_id = cur_filament.get("id")
+    desired_name = f"{material} {subtype}".strip() if subtype else material
+    cur_color_norm = (cur_filament.get("color_hex") or "").upper()[:6]
+    cur_vendor_name = (cur_vendor.get("name") or "").strip()
+    cur_weight_int = int(cur_filament.get("weight") or 0)
+    metadata_unchanged = (
+        cur_filament_id
+        and (cur_filament.get("name") or "").strip() == desired_name
+        and (cur_filament.get("material") or "").upper() == material.upper()
+        and cur_color_norm == color_hex.upper()
+        and cur_vendor_name.lower() == ((brand or "").strip().lower())
+        and cur_weight_int == int(label_weight)
+    )
+
+    if metadata_unchanged:
+        # No filament-side change at all — re-use the existing link, skip
+        # find_or_create entirely so a no-op edit (e.g. just changing
+        # weight_used or note) never even touches the filament catalogue.
+        filament_id = cur_filament_id
+    else:
+        async with _translate_spoolman_errors():
+            shared = await client.is_filament_shared(cur_filament_id, spool_id) if cur_filament_id else False
+        if cur_filament_id and not shared:
+            # Singleton filament — PATCH it in place so the user's edit lands
+            # on the row their spool already points at instead of orphaning it.
+            patch_body: dict = {
+                "name": desired_name,
+                "material": material,
+                "color_hex": color_hex,
+                "weight": float(label_weight),
+            }
+            if brand:
+                vendor_id = await client.find_or_create_vendor(brand)
+                patch_body["vendor_id"] = vendor_id
+            async with _translate_spoolman_errors():
+                await client.patch_filament(cur_filament_id, patch_body)
+            filament_id = cur_filament_id
+        else:
+            # Filament is shared with other spools — PATCHing it in place would
+            # silently rewrite their metadata too. Fall back to find-or-create
+            # so only this spool's link moves.
+            async with _translate_spoolman_errors():
+                filament_id = await client.find_or_create_filament(
+                    material=material,
+                    subtype=subtype or "",
+                    brand=brand,
+                    color_hex=color_hex,
+                    label_weight=label_weight,
+                    color_name=color_name,
+                )
     if not filament_id:
     if not filament_id:
         raise HTTPException(status_code=500, detail="Failed to find or create filament in Spoolman")
         raise HTTPException(status_code=500, detail="Failed to find or create filament in Spoolman")
 
 

+ 17 - 0
backend/app/services/spoolman.py

@@ -548,6 +548,23 @@ class SpoolmanClient:
         """Delete a spool from Spoolman."""
         """Delete a spool from Spoolman."""
         await self._request_spool("DELETE", spool_id, operation="delete")
         await self._request_spool("DELETE", spool_id, operation="delete")
 
 
+    async def is_filament_shared(self, filament_id: int, exclude_spool_id: int) -> bool:
+        """True if any spool other than ``exclude_spool_id`` is linked to ``filament_id``.
+
+        Used by the spool-edit path to decide between PATCHing the existing
+        filament in place (singleton) and falling back to find_or_create
+        (shared — re-linking the spool is the only safe option). Includes
+        archived spools so a shared link doesn't suddenly look singleton just
+        because the sibling spool was archived.
+        """
+        spools = await self.get_all_spools(allow_archived=True)
+        for s in spools:
+            if s.get("id") == exclude_spool_id:
+                continue
+            if ((s.get("filament") or {}).get("id")) == filament_id:
+                return True
+        return False
+
     async def set_spool_archived(self, spool_id: int, archived: bool) -> dict:
     async def set_spool_archived(self, spool_id: int, archived: bool) -> dict:
         """Archive or restore a spool in Spoolman."""
         """Archive or restore a spool in Spoolman."""
         response = await self._request_spool(
         response = await self._request_spool(

+ 70 - 0
backend/tests/integration/test_spoolman_inventory_api.py

@@ -67,6 +67,12 @@ def mock_spoolman_client():
     mock_client.update_spool_full = AsyncMock(return_value=SAMPLE_SPOOLMAN_SPOOL)
     mock_client.update_spool_full = AsyncMock(return_value=SAMPLE_SPOOLMAN_SPOOL)
     mock_client.merge_spool_extra = AsyncMock(return_value=SAMPLE_SPOOLMAN_SPOOL)
     mock_client.merge_spool_extra = AsyncMock(return_value=SAMPLE_SPOOLMAN_SPOOL)
     mock_client.find_or_create_filament = AsyncMock(return_value=7)
     mock_client.find_or_create_filament = AsyncMock(return_value=7)
+    mock_client.find_or_create_vendor = AsyncMock(return_value=3)
+    mock_client.patch_filament = AsyncMock(return_value={"id": 7})
+    # Default to singleton (only this spool uses the filament) so edits
+    # exercise the new in-place-PATCH path; tests that need the shared
+    # branch override this on the fly.
+    mock_client.is_filament_shared = AsyncMock(return_value=False)
     mock_client.ensure_extra_field = AsyncMock(return_value=True)
     mock_client.ensure_extra_field = AsyncMock(return_value=True)
 
 
     with (
     with (
@@ -323,6 +329,70 @@ class TestSpoolmanInventoryCRUD:
         assert response.status_code == 200
         assert response.status_code == 200
         mock_spoolman_client.update_spool_full.assert_called_once()
         mock_spoolman_client.update_spool_full.assert_called_once()
 
 
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_update_noop_metadata_reuses_filament(
+        self,
+        async_client: AsyncClient,
+        spoolman_settings,
+        mock_spoolman_client,
+    ):
+        """#1357 follow-up: an edit that doesn't touch any filament-shaping
+        field (only weight_used / note / color_name) must NOT hit
+        find_or_create_filament OR patch_filament — the link stays put and
+        the filament catalogue is left alone."""
+        payload = {"note": "just a note change", "weight_used": 50.0}
+        response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload)
+        assert response.status_code == 200
+        mock_spoolman_client.find_or_create_filament.assert_not_called()
+        mock_spoolman_client.patch_filament.assert_not_called()
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_update_singleton_filament_patches_in_place(
+        self,
+        async_client: AsyncClient,
+        spoolman_settings,
+        mock_spoolman_client,
+    ):
+        """#1357 follow-up: when the linked filament is only used by the
+        spool being edited (singleton), changing the subtype must PATCH that
+        filament in place — NOT create a new filament and orphan the old
+        one. This is the exact failure the reporter showed: editing Subtype
+        "Red" → "Basic" minted a new "PETG Basic" filament every time.
+        """
+        # Sample filament is "PLA Basic"; flip to "Matte" so the metadata
+        # actually changes and the singleton path engages.
+        payload = {"subtype": "Matte"}
+        response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload)
+        assert response.status_code == 200
+        # Singleton path: PATCH the existing filament, do NOT find_or_create.
+        mock_spoolman_client.patch_filament.assert_called_once()
+        mock_spoolman_client.find_or_create_filament.assert_not_called()
+        # PATCH targets the spool's current filament (id=7) with the new name.
+        call_args = mock_spoolman_client.patch_filament.call_args
+        assert call_args.args[0] == 7
+        assert call_args.args[1]["name"] == "PLA Matte"
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_update_shared_filament_falls_back_to_find_or_create(
+        self,
+        async_client: AsyncClient,
+        spoolman_settings,
+        mock_spoolman_client,
+    ):
+        """#1357 follow-up: when the linked filament is shared with another
+        spool, PATCHing in place would silently rewrite the sibling's
+        metadata too. Fall back to find_or_create — only this spool's
+        filament_id moves."""
+        mock_spoolman_client.is_filament_shared.return_value = True
+        payload = {"subtype": "Matte"}
+        response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload)
+        assert response.status_code == 200
+        mock_spoolman_client.find_or_create_filament.assert_called_once()
+        mock_spoolman_client.patch_filament.assert_not_called()
+
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
     async def test_update_with_explicit_null_color_name_clears_extra(
     async def test_update_with_explicit_null_color_name_clears_extra(

برخی فایل ها در این مقایسه diff نمایش داده نمی شوند زیرا تعداد فایل ها بسیار زیاد است