From 4a98914d4aec1ca16fb5a17eb5f6b7251add6cc9 Mon Sep 17 00:00:00 2001 From: maziggy Date: Sun, 17 May 2026 09:10:20 +0200 Subject: [PATCH] =?UTF-8?q?=20=20fix(spoolman):=20persist=20Color=20Name?= =?UTF-8?q?=20via=20spool.extra=20=E2=80=94=20Spoolman=20has=20no=20filame?= =?UTF-8?q?nt.color=5Fname=20field=20(#1357)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. The earlier #1319 fix correctly handled the read/form-prefill half (the color_name_is_synthesized flag, 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 — the accepted fields are 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. No color_name. Spoolman's PATCH happily returns 200 for {"color_name": "Red"} and 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 "BB also created a bunch of new filaments" duplicate trail on each save attempt). The fix follows the same pattern 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 extra > filament.color_name (forward-compat for any future Spoolman release that adds the field) > subtype synth. 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 that lets an AMS-sync-created filament named "Glow" still match the user-driven edit's composed "PLA Glow", which prevents re-introducing the duplicate-filament trail. The frontend form's color_name_is_synthesized handling is unchanged — that part already worked. --- CHANGELOG.md | 1 + backend/app/api/routes/_spoolman_helpers.py | 34 +++-- backend/app/api/routes/spoolman_inventory.py | 28 +++- backend/app/services/spoolman.py | 79 +++++++--- .../test_spoolman_inventory_api.py | 93 ++++++++---- .../unit/test_spoolman_inventory_helpers.py | 53 +++++++ .../unit/test_spoolman_inventory_methods.py | 142 +++++++++++------- 7 files changed, 301 insertions(+), 129 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d41f5196d..a05397ff0 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 +- **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/api/routes/_spoolman_helpers.py b/backend/app/api/routes/_spoolman_helpers.py index 0e3ef816f..e6ab5efc8 100644 --- a/backend/app/api/routes/_spoolman_helpers.py +++ b/backend/app/api/routes/_spoolman_helpers.py @@ -257,18 +257,28 @@ def _map_spoolman_spool(spool: dict) -> MappedSpoolFields: created_at: str | None = spool.get("registered") or None - # Spoolman doesn't standardise a `color_name` field — most installs only - # populate `color_hex` (the swatch) and the filament's `name` (which often - # carries the colour, e.g. "PLA Basic Red"). Without a fallback the - # frontend lists a sea of "Unknown color" entries that all look identical - # except for the swatch. Fall back to the filament name minus material - # prefix (the same string the `subtype` field already carries — typically - # "Basic Red" / "PLA+ Black" / etc.) so the user can tell spools apart at - # a glance even on Spoolman installs that don't fill color_name. - # color_name_is_synthesized: surfaced so the edit form can avoid prefilling - # the synth value back into the input, which would otherwise round-trip the - # subtype string as if it were a user-set color_name (#1319). - stored_color_name = filament.get("color_name") or None + # Spoolman has no `color_name` field on Filament — confirmed against the + # FilamentUpdateParameters schema in 0.23.1: name/vendor_id/material/price/ + # density/diameter/weight/spool_weight/article_number/comment/extruder_temp/ + # bed_temp/color_hex/multi_color_hexes/multi_color_direction/external_id/ + # extra, no color_name (#1357). The previous attempt (b8e350c3) was + # PATCHing a key Spoolman silently discards, which is why color_name + # never actually persisted from the user's edits. + # + # We persist it ourselves under spool.extra.bambu_color_name (JSON-encoded + # string, same pattern as bambu_slicer_filament). Read order: + # 1. spool.extra.bambu_color_name (the canonical store) + # 2. filament.color_name (forward-compat — picks up the value if a + # future Spoolman release adds the field, or if an admin populated + # it via a custom extra-field they registered themselves) + # 3. subtype (synth fallback so the inventory list isn't a sea of + # "Unknown color" entries on installs with neither field set) + # + # color_name_is_synthesized = True only when we fell back to subtype. + # The edit form uses it to leave the input blank, so the user doesn't + # round-trip the synth value back as if they had set it. + extra_color_name = _extract_extra_str(extra, "bambu_color_name") or None + stored_color_name = extra_color_name or (filament.get("color_name") or None) color_name: str | None = stored_color_name or subtype or None color_name_is_synthesized: bool = stored_color_name is None and color_name is not None diff --git a/backend/app/api/routes/spoolman_inventory.py b/backend/app/api/routes/spoolman_inventory.py index acfd5d6ba..e49518995 100644 --- a/backend/app/api/routes/spoolman_inventory.py +++ b/backend/app/api/routes/spoolman_inventory.py @@ -440,18 +440,24 @@ async def create_spool( spool, price_warnings = await _apply_price_if_set(client, spool, data.cost_per_kg) - # Persist slicer_filament under the spool's extra dict (mirror update_spool). - if data.slicer_filament is not None or data.slicer_filament_name is not None: + # Persist slicer_filament AND color_name under the spool's extra dict + # (mirror update_spool). Spoolman has no `color_name` field on filament + # (#1357) so we own the round-trip ourselves. + if data.slicer_filament is not None or data.slicer_filament_name is not None or data.color_name is not None: # Ensure extra fields are registered before write. if data.slicer_filament is not None: await client.ensure_extra_field("bambu_slicer_filament") if data.slicer_filament_name is not None: await client.ensure_extra_field("bambu_slicer_filament_name") + if data.color_name is not None: + await client.ensure_extra_field("bambu_color_name") new_extra: dict = {} if data.slicer_filament is not None: new_extra["bambu_slicer_filament"] = json.dumps(data.slicer_filament) if data.slicer_filament_name is not None: new_extra["bambu_slicer_filament_name"] = json.dumps(data.slicer_filament_name) + if data.color_name is not None: + new_extra["bambu_color_name"] = json.dumps(data.color_name) if new_extra: try: async with _translate_spoolman_errors(): @@ -459,7 +465,7 @@ async def create_spool( except HTTPException: # Best-effort — the spool already exists, log and continue. logger.warning( - "Failed to persist slicer_filament for spool %s", + "Failed to persist slicer_filament/color_name for spool %s", spool.get("id"), ) @@ -633,25 +639,33 @@ async def update_spool( clear_location=storage_location_changed and not storage_location, ) - # Persist BambuStudio slicer preset under the spool's extra dict. - # Spoolman doesn't have a native field for this, so we round-trip via - # extra and unpack in _map_spoolman_spool. Only writes when the request + # Persist BambuStudio slicer preset AND color_name under spool.extra. + # Spoolman has no native fields for these — color_name was confirmed + # absent from the FilamentUpdateParameters schema in 0.23.1 (#1357), so + # writing `filament.color_name` was a silent no-op that left every + # edit looking "not saved". They all round-trip via extra and get + # unpacked in _map_spoolman_spool. Only writes when the request # explicitly set the field — passing null/omitting leaves the existing # extra entry untouched (write empty string to clear). sf_set = "slicer_filament" in data.model_fields_set sfn_set = "slicer_filament_name" in data.model_fields_set - if sf_set or sfn_set: + cn_set = "color_name" in data.model_fields_set + if sf_set or sfn_set or cn_set: # Ensure extra fields are registered (Spoolman rejects PATCHes with # unknown keys with HTTP 400). Idempotent if startup already ran this. if sf_set: await client.ensure_extra_field("bambu_slicer_filament") if sfn_set: await client.ensure_extra_field("bambu_slicer_filament_name") + if cn_set: + await client.ensure_extra_field("bambu_color_name") new_extra: dict = {} if sf_set: new_extra["bambu_slicer_filament"] = json.dumps(data.slicer_filament or "") if sfn_set: new_extra["bambu_slicer_filament_name"] = json.dumps(data.slicer_filament_name or "") + if cn_set: + new_extra["bambu_color_name"] = json.dumps(data.color_name or "") async with _translate_spoolman_errors(): updated = await client.merge_spool_extra(spool_id, new_extra) diff --git a/backend/app/services/spoolman.py b/backend/app/services/spoolman.py index 8fa7ac9cf..b8f8e876c 100644 --- a/backend/app/services/spoolman.py +++ b/backend/app/services/spoolman.py @@ -75,6 +75,25 @@ class SpoolmanClientError(Exception): self.response_text = response_text +def _filament_subtype_part(name: str, material: str) -> str: + """Return the subtype portion of a filament name, lowercased. + + Mirrors the read-side derivation in + ``backend/app/api/routes/_spoolman_helpers.py::_map_spoolman_spool``: + if the filament name starts with the material prefix (e.g. ``"PLA Glow"`` + when material is ``"PLA"``), strip it; otherwise return the name as-is. + + Used by ``find_or_create_filament`` so that an existing filament saved by + the AMS-sync path with name ``"Glow"`` still matches a user-driven edit + that composes ``"PLA Glow"`` (#1357). + """ + s = (name or "").strip() + m = (material or "").strip() + if m and s.upper().startswith(m.upper() + " "): + return s[len(m) + 1 :].strip().lower() + return s.lower() + + class SpoolmanClient: """Client for interacting with Spoolman API.""" @@ -623,48 +642,58 @@ class SpoolmanClient: if brand: vendor_id = await self.find_or_create_vendor(brand) + # Normalised match keys (case-insensitive). Computed once outside the + # loop so the inner comparison stays simple. + composed_subtype = _filament_subtype_part(name, material) + material_norm = material.upper() + brand_norm = (brand or "").strip().lower() + filaments = await self.get_filaments() for f in filaments: f_material = (f.get("material") or "").upper() - f_name = (f.get("name") or "").strip() f_color = (f.get("color_hex") or "").upper()[:6] f_vendor = f.get("vendor") or {} f_vendor_name = (f_vendor.get("name") or "").strip().lower() - material_match = f_material == material.upper() - name_match = f_name.lower() == name.lower() + material_match = f_material == material_norm + # Match on the subtype portion of the filament name. AMS-sync + # auto-create (the underscore-prefixed `_find_or_create_filament` + # used during MQTT tray import) stores the filament as just + # ``tray.tray_sub_brands`` — e.g. ``"Glow"`` — while the + # user-driven edit path here composes ``" "`` + # — ``"PLA Glow"``. The old literal equality `f_name == name` + # failed to bridge the two shapes, so every edit fell through to + # `create_filament`, leaving a trail of duplicate filaments AND + # leaving the spool either still pointed at the old filament + # whose `color_name` never got patched, or pointed at a new + # filament with the colour while the inventory list kept + # showing the synth fallback from the old one (#1357). + f_subtype_part = _filament_subtype_part(f.get("name") or "", material) + name_match = f_subtype_part == composed_subtype color_match = f_color == color - vendor_match = (not brand) or f_vendor_name == (brand or "").strip().lower() + vendor_match = (not brand) or f_vendor_name == brand_norm if material_match and name_match and color_match and vendor_match: - # #1319: color_name is not part of the match key, but if the - # caller passed a value that differs from what's stored, update - # the filament — otherwise the user's edit is silently dropped - # and the inventory read falls back to subtype, making it look - # like color_name "reverts" to the subtype on every save. - # Convention: None = "don't touch"; "" = explicit clear; any - # other string = set/update. - if color_name is not None: - existing = (f.get("color_name") or "").strip() - requested = color_name.strip() - if requested != existing: - payload_value: str | None = requested if requested else None - try: - await self.patch_filament(f["id"], {"color_name": payload_value}) - except Exception as e: - logger.warning( - "Failed to update color_name on filament %s: %s", - f["id"], - e, - ) + # color_name is intentionally not part of the match key and + # is no longer patched onto the filament here: Spoolman 0.23.1 + # has no `color_name` field on Filament (#1357 — confirmed + # against the FilamentUpdateParameters schema). The earlier + # #1319 fix tried to patch it and Spoolman silently dropped + # the key, which is exactly why the user's edit looked "not + # saved". The route now persists color_name via + # spool.extra.bambu_color_name (see _map_spoolman_spool for + # the read side); find_or_create_filament's only job is to + # resolve the right filament_id for the spool link. return f["id"] + # color_name omitted: Spoolman has no such field on Filament (#1357); + # the user's color_name lands in spool.extra.bambu_color_name via the + # route after find_or_create_filament returns the new id. filament = await self.create_filament( name=name, vendor_id=vendor_id, material=material, color_hex=color, - color_name=color_name, weight=float(label_weight), ) filament_id = filament.get("id") diff --git a/backend/tests/integration/test_spoolman_inventory_api.py b/backend/tests/integration/test_spoolman_inventory_api.py index 71fb1fd25..7a405fbc8 100644 --- a/backend/tests/integration/test_spoolman_inventory_api.py +++ b/backend/tests/integration/test_spoolman_inventory_api.py @@ -66,6 +66,7 @@ def mock_spoolman_client(): mock_client.update_spool_full = 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.ensure_extra_field = AsyncMock(return_value=True) with ( patch( @@ -323,39 +324,50 @@ class TestSpoolmanInventoryCRUD: @pytest.mark.asyncio @pytest.mark.integration - async def test_update_with_explicit_null_color_name_clears( + async def test_update_with_explicit_null_color_name_clears_extra( self, async_client: AsyncClient, spoolman_settings, mock_spoolman_client, ): - """#1319 follow-up: explicit color_name=null in the PATCH body means - "clear" — route translates it to "" so find_or_create_filament patches - the matched filament with color_name=None.""" + """#1357: explicit color_name=null means "clear". The route writes a + JSON-encoded empty string to spool.extra.bambu_color_name so the read + path falls back to the synth value next time.""" payload = {"color_name": None} 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() - kwargs = mock_spoolman_client.find_or_create_filament.call_args.kwargs - assert kwargs["color_name"] == "" + mock_spoolman_client.ensure_extra_field.assert_any_call("bambu_color_name") + mock_spoolman_client.merge_spool_extra.assert_called_once() + _, kwargs = mock_spoolman_client.merge_spool_extra.call_args + # First positional arg is spool_id; second is the extra-dict patch. + args = mock_spoolman_client.merge_spool_extra.call_args.args + extra_patch = args[1] if len(args) > 1 else kwargs.get("new_fields", {}) + import json as _json + + assert _json.loads(extra_patch["bambu_color_name"]) == "" @pytest.mark.asyncio @pytest.mark.integration - async def test_update_without_color_name_keeps_current( + async def test_update_without_color_name_skips_extra_write( self, async_client: AsyncClient, spoolman_settings, mock_spoolman_client, ): - """#1319 follow-up: when color_name is omitted from the PATCH body the - current value is kept — None passed to find_or_create_filament means - "don't touch".""" + """#1357: when color_name is omitted from the PATCH body the extra + write is skipped entirely — no merge_spool_extra call, no ensure_extra + call for bambu_color_name. Only fields the request explicitly set go + through the extra round-trip.""" payload = {"note": "only updating note"} response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload) assert response.status_code == 200 - kwargs = mock_spoolman_client.find_or_create_filament.call_args.kwargs - # SAMPLE_SPOOLMAN_SPOOL has no color_name field, so cur fallback is None. - assert kwargs["color_name"] is None + # No call should target bambu_color_name when color_name wasn't in the body. + color_name_calls = [ + c + for c in mock_spoolman_client.ensure_extra_field.call_args_list + if c.args and c.args[0] == "bambu_color_name" + ] + assert color_name_calls == [] @pytest.mark.asyncio @pytest.mark.integration @@ -1117,17 +1129,26 @@ class TestStorageLocationPassthrough: class TestColorNamePassthrough: - """color_name is forwarded to find_or_create_filament on create and update (B6 / T5).""" + """color_name persistence via spool.extra.bambu_color_name (#1357). + + Spoolman 0.23.1 has no `color_name` field on Filament, so Bambuddy owns + the round-trip via the spool's extra dict — same shape as the existing + bambu_slicer_filament storage. These tests pin that the create/update + routes register the extra field and write to merge_spool_extra, NOT to + find_or_create_filament's color_name parameter. + """ @pytest.mark.asyncio @pytest.mark.integration - async def test_create_passes_color_name_to_filament( + async def test_create_writes_color_name_to_spool_extra( self, async_client: AsyncClient, spoolman_settings, mock_spoolman_client, ): - """color_name from the create payload is forwarded to find_or_create_filament.""" + """color_name from create payload lands in spool.extra.bambu_color_name.""" + import json as _json + payload = { "material": "PLA", "label_weight": 1000, @@ -1136,41 +1157,53 @@ class TestColorNamePassthrough: } response = await async_client.post("/api/v1/spoolman/inventory/spools", json=payload) assert response.status_code == 200 - mock_spoolman_client.find_or_create_filament.assert_called_once() - _, kwargs = mock_spoolman_client.find_or_create_filament.call_args - assert kwargs.get("color_name") == "Bambu Green" + mock_spoolman_client.ensure_extra_field.assert_any_call("bambu_color_name") + mock_spoolman_client.merge_spool_extra.assert_called_once() + args = mock_spoolman_client.merge_spool_extra.call_args.args + extra_patch = args[1] + assert _json.loads(extra_patch["bambu_color_name"]) == "Bambu Green" @pytest.mark.asyncio @pytest.mark.integration - async def test_update_passes_color_name_to_filament( + async def test_update_writes_color_name_to_spool_extra( self, async_client: AsyncClient, spoolman_settings, mock_spoolman_client, ): - """color_name from the update payload is forwarded to find_or_create_filament.""" + """color_name from update payload lands in spool.extra.bambu_color_name — + this is the #1357 reproduction: previously the value went to + filament.color_name which Spoolman silently dropped.""" + import json as _json + payload = {"color_name": "Jade White"} 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() - _, kwargs = mock_spoolman_client.find_or_create_filament.call_args - assert kwargs.get("color_name") == "Jade White" + mock_spoolman_client.ensure_extra_field.assert_any_call("bambu_color_name") + mock_spoolman_client.merge_spool_extra.assert_called_once() + args = mock_spoolman_client.merge_spool_extra.call_args.args + extra_patch = args[1] + assert _json.loads(extra_patch["bambu_color_name"]) == "Jade White" @pytest.mark.asyncio @pytest.mark.integration - async def test_update_omits_color_name_when_not_provided( + async def test_update_omits_color_name_skips_extra_write( self, async_client: AsyncClient, spoolman_settings, mock_spoolman_client, ): - """When color_name is not in the PATCH payload, the existing filament color_name is used.""" + """When color_name is absent from the PATCH body, the route must not + write to spool.extra at all (preserves any existing value).""" payload = {"note": "no color_name here"} response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload) assert response.status_code == 200 - _, kwargs = mock_spoolman_client.find_or_create_filament.call_args - # color_name falls back to current filament's color_name (which is None in test fixture) - assert kwargs.get("color_name") is None + color_name_calls = [ + c + for c in mock_spoolman_client.ensure_extra_field.call_args_list + if c.args and c.args[0] == "bambu_color_name" + ] + assert color_name_calls == [] class TestSpoolmanInventoryAuth: diff --git a/backend/tests/unit/test_spoolman_inventory_helpers.py b/backend/tests/unit/test_spoolman_inventory_helpers.py index f11665f99..b4c87deb1 100644 --- a/backend/tests/unit/test_spoolman_inventory_helpers.py +++ b/backend/tests/unit/test_spoolman_inventory_helpers.py @@ -218,6 +218,59 @@ class TestMapSpoolmanSpool: # color_name falls back to subtype. assert result["color_name"] == "Basic Red" + def test_color_name_read_from_spool_extra_first(self): + """#1357: the canonical store for color_name is + spool.extra.bambu_color_name (JSON-encoded). Read priority is + extra > filament.color_name > subtype-synth. The user's + Bambuddy-saved value MUST win even when Spoolman's own + filament.color_name happens to be populated from some other source. + """ + spool = { + **MINIMAL_SPOOL, + "extra": {"bambu_color_name": '"Galaxy Black"'}, + "filament": { + **MINIMAL_SPOOL["filament"], + "name": "PLA Glow", + "color_name": "Glow", # would be picked up if extra weren't preferred + }, + } + result = _map_spoolman_spool(spool) + assert result["color_name"] == "Galaxy Black" + assert result["color_name_is_synthesized"] is False + + def test_color_name_empty_extra_falls_through_to_filament(self): + """An explicit empty string in spool.extra.bambu_color_name (the + "user cleared the field" shape) must NOT mask Spoolman's own + filament.color_name if one exists — it falls through to the next + layer instead of suppressing it.""" + spool = { + **MINIMAL_SPOOL, + "extra": {"bambu_color_name": '""'}, + "filament": { + **MINIMAL_SPOOL["filament"], + "color_name": "Sunset", + }, + } + result = _map_spoolman_spool(spool) + assert result["color_name"] == "Sunset" + assert result["color_name_is_synthesized"] is False + + def test_color_name_empty_extra_falls_through_to_synth(self): + """When extra is cleared and filament has no color_name either, + fall all the way through to the subtype synth — same UX as a fresh + Spoolman install.""" + spool = { + **MINIMAL_SPOOL, + "extra": {"bambu_color_name": '""'}, + "filament": { + **MINIMAL_SPOOL["filament"], + "name": "PLA Basic Red", + }, + } + result = _map_spoolman_spool(spool) + assert result["color_name"] == "Basic Red" + assert result["color_name_is_synthesized"] is True + def test_color_name_none_when_both_fields_empty(self): """If neither color_name nor a usable subtype exists, return None — UI falls back to its own 'Unknown color' string rather than showing a diff --git a/backend/tests/unit/test_spoolman_inventory_methods.py b/backend/tests/unit/test_spoolman_inventory_methods.py index fb0d050ce..55a94be07 100644 --- a/backend/tests/unit/test_spoolman_inventory_methods.py +++ b/backend/tests/unit/test_spoolman_inventory_methods.py @@ -323,26 +323,15 @@ class TestFindOrCreateFilament: assert result == 7 @pytest.mark.asyncio - async def test_patches_color_name_on_existing_filament_when_changed(self, client): - """#1319: color_name is not part of the match key, so when a caller - updates a spool with a new color_name and the material/name/color/vendor - still match an existing filament, the existing filament's color_name - must be patched — otherwise the user's edit is silently dropped.""" - existing = {**SAMPLE_FILAMENT, "color_name": None} - with ( - patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)), - patch.object(client, "get_filaments", AsyncMock(return_value=[existing])), - patch.object(client, "patch_filament", AsyncMock(return_value={"id": 7})) as mock_patch, - ): - result = await client.find_or_create_filament( - "PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="Sunny Yellow" - ) - assert result == 7 - mock_patch.assert_called_once_with(7, {"color_name": "Sunny Yellow"}) - - @pytest.mark.asyncio - async def test_does_not_patch_when_color_name_unchanged(self, client): - existing = {**SAMPLE_FILAMENT, "color_name": "Sunny Yellow"} + async def test_color_name_does_not_trigger_filament_patch(self, client): + """#1357: Spoolman 0.23.1 has no `color_name` field on Filament + (verified against FilamentUpdateParameters schema). find_or_create_filament + must NOT attempt to PATCH it — the route now persists the user's + color_name to spool.extra.bambu_color_name instead. Any patch call + from this layer would be a silent no-op (Spoolman ignores unknown + keys) and was the original symptom of "edits never save". + """ + existing = {**SAMPLE_FILAMENT} with ( patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)), patch.object(client, "get_filaments", AsyncMock(return_value=[existing])), @@ -355,47 +344,88 @@ class TestFindOrCreateFilament: mock_patch.assert_not_called() @pytest.mark.asyncio - async def test_does_not_patch_when_color_name_empty(self, client): - """An empty/None color_name should not clobber an existing value.""" - existing = {**SAMPLE_FILAMENT, "color_name": "Sunny Yellow"} + async def test_matches_filament_named_with_just_subtype(self, client): + """#1357: AMS-sync auto-create saves the filament with name set to just + ``tray.tray_sub_brands`` (e.g. ``"Glow"`` without the material prefix), + but the user-driven edit path composes ``" "`` + (``"PLA Glow"``). Before this fix the literal `f_name == name` check + failed to bridge the two shapes, so every edit fell through to + ``create_filament`` and left a trail of duplicate filaments. Now the + name match strips the material prefix on both sides, so the two + shapes resolve to the same subtype key.""" + existing = { + **SAMPLE_FILAMENT, + "id": 11, + "name": "Glow", # AMS-sync shape: just subtype + "material": "PLA", + "color_hex": "AAF3C6", + "color_name": None, + "vendor": {"id": 3, "name": "Amazon Basics"}, + } with ( patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)), patch.object(client, "get_filaments", AsyncMock(return_value=[existing])), patch.object(client, "patch_filament", AsyncMock()) as mock_patch, - ): - result = await client.find_or_create_filament("PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name=None) - assert result == 7 - mock_patch.assert_not_called() - - @pytest.mark.asyncio - async def test_clears_color_name_when_empty_string_passed(self, client): - """#1319 follow-up: empty string means "explicit clear" — the route - layer translates a wire-level null into "" so the user can blank the - field on a previously-set spool.""" - existing = {**SAMPLE_FILAMENT, "color_name": "Sunny Yellow"} - with ( - patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)), - patch.object(client, "get_filaments", AsyncMock(return_value=[existing])), - patch.object(client, "patch_filament", AsyncMock(return_value={"id": 7})) as mock_patch, - ): - result = await client.find_or_create_filament("PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="") - assert result == 7 - mock_patch.assert_called_once_with(7, {"color_name": None}) - - @pytest.mark.asyncio - async def test_patch_failure_does_not_block_match(self, client): - """A patch_filament failure must not prevent returning the matched id — - save should still link the spool to the correct filament.""" - existing = {**SAMPLE_FILAMENT, "color_name": None} - with ( - patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)), - patch.object(client, "get_filaments", AsyncMock(return_value=[existing])), - patch.object(client, "patch_filament", AsyncMock(side_effect=SpoolmanUnavailableError("boom"))), + patch.object(client, "create_filament", AsyncMock()) as mock_create, ): result = await client.find_or_create_filament( - "PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="Sunny Yellow" + "PLA", "Glow", "Amazon Basics", "AAF3C6", 1000, color_name="Bright Glow" + ) + assert result == 11 + # color_name is no longer written via the filament — see #1357 — and + # the function must not create a duplicate filament. + mock_patch.assert_not_called() + mock_create.assert_not_called() + + @pytest.mark.asyncio + async def test_still_matches_filament_named_material_plus_subtype(self, client): + """The composed-name shape (``"PLA Basic"`` matching a Spoolman filament + also named ``"PLA Basic"``) must keep working — the normalisation strips + the prefix on both sides, so the comparison is on the subtype part.""" + existing = { + **SAMPLE_FILAMENT, + "id": 7, + "name": "PLA Basic", + "material": "PLA", + "color_hex": "FF0000", + "color_name": "Sunset", + } + with ( + patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)), + patch.object(client, "get_filaments", AsyncMock(return_value=[existing])), + patch.object(client, "patch_filament", AsyncMock(return_value={"id": 7})), + patch.object(client, "create_filament", AsyncMock()) as mock_create, + ): + result = await client.find_or_create_filament( + "PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="Sunset" ) assert result == 7 + mock_create.assert_not_called() + + @pytest.mark.asyncio + async def test_name_match_does_not_cross_materials(self, client): + """Sanity check: a filament with name=subtype must NOT match a request + with a different material that happens to share the subtype string. + material_match runs first and fails, so the iteration moves on and + ``create_filament`` is called.""" + existing = { + **SAMPLE_FILAMENT, + "id": 7, + "name": "Basic", + "material": "PETG", # different material + "color_hex": "FF0000", + } + new_filament = {"id": 99, "name": "PLA Basic"} + with ( + patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)), + patch.object(client, "get_filaments", AsyncMock(return_value=[existing])), + patch.object(client, "create_filament", AsyncMock(return_value=new_filament)) as mock_create, + ): + result = await client.find_or_create_filament( + "PLA", "Basic", "Bambu Lab", "FF0000", 1000, color_name="Sunset" + ) + assert result == 99 + mock_create.assert_called_once() @pytest.mark.asyncio async def test_creates_filament_when_no_match(self, client): @@ -407,12 +437,15 @@ class TestFindOrCreateFilament: ): result = await client.find_or_create_filament("PETG", "Pro", "Bambu Lab", "00FF00", 1000) assert result == 99 + # color_name is intentionally not forwarded to create_filament (#1357): + # Spoolman has no such field on Filament, so passing it would be a + # no-op. The route persists color_name to spool.extra.bambu_color_name + # after this returns. mock_create.assert_called_once_with( name="PETG Pro", vendor_id=3, material="PETG", color_hex="00FF00", - color_name=None, weight=1000.0, ) @@ -443,7 +476,6 @@ class TestFindOrCreateFilament: vendor_id=None, material="ABS", color_hex="FF0000", - color_name=None, weight=750.0, )