diff --git a/backend/app/api/routes/_spoolman_helpers.py b/backend/app/api/routes/_spoolman_helpers.py index cb95115e1..d0d3bf6b4 100644 --- a/backend/app/api/routes/_spoolman_helpers.py +++ b/backend/app/api/routes/_spoolman_helpers.py @@ -91,7 +91,10 @@ def assert_safe_spoolman_url(url: str) -> None: assert_safe_lan_service_url(url, label="Spoolman URL") -_COLOR_HEX_RE = re.compile(r"^[0-9A-Fa-f]{6}$") +# Six characters, or eight when the filament carries an alpha byte. The write +# side stores eight only for genuinely translucent spools (#2912); rejecting +# them here turned every clear spool into neutral grey on read. +_COLOR_HEX_RE = re.compile(r"^[0-9A-Fa-f]{6}(?:[0-9A-Fa-f]{2})?$") _TAG_HEX_RE = re.compile(r"^[0-9A-F]+$") @@ -197,10 +200,12 @@ def _map_spoolman_spool(spool: dict) -> MappedSpoolFields: else: subtype = filament_name or None - # Colour: validate as 6-char hex; fall back to neutral grey for invalid values + # Colour: validate as 6- or 8-char hex; fall back to neutral grey for invalid + # values. An 8-char value already carries its alpha, so appending the opaque + # byte would push it to ten and lose the translucency it was stored to keep. raw_color = (filament.get("color_hex") or "").upper().removeprefix("#") color_hex: str = raw_color if _COLOR_HEX_RE.match(raw_color) else "808080" - rgba: str = color_hex + "FF" + rgba: str = color_hex if len(color_hex) == 8 else color_hex + "FF" label_weight: int = _safe_int(filament.get("weight"), 1000) real_used_weight: float = _safe_float(spool.get("used_weight"), 0.0) diff --git a/backend/app/api/routes/spoolman_inventory.py b/backend/app/api/routes/spoolman_inventory.py index bc3a8907f..97e239449 100644 --- a/backend/app/api/routes/spoolman_inventory.py +++ b/backend/app/api/routes/spoolman_inventory.py @@ -59,6 +59,7 @@ from backend.app.services.spoolman import ( init_spoolman_client, ) from backend.app.services.spoolman_tracking import get_fallback_spool_tag_for_slot +from backend.app.utils.color_utils import spoolman_color_hex from backend.app.utils.filament_ids import ( GENERIC_FILAMENT_IDS, filament_id_to_setting_id, @@ -502,7 +503,10 @@ async def _resolve_filament_id(data: SpoolmanInventoryCreate, client: SpoolmanCl return data.spoolman_filament_id # Validator guarantees material is non-None when spoolman_filament_id is None assert data.material is not None # noqa: S101 - color_hex = (data.rgba or "808080FF")[:6] + # `or "808080"` on the result rather than on the input: spoolman_color_hex + # returns None only for a missing value, so this is the same neutral grey the + # old inline default produced, without handing an Optional to a str parameter. + color_hex = spoolman_color_hex(data.rgba) or "808080" async with _translate_spoolman_errors(): return await client.find_or_create_filament( material=data.material, @@ -708,7 +712,10 @@ async def update_spool( else: color_name = cur_filament.get("color_name") or None cur_color = (cur_filament.get("color_hex") or "808080").upper().removeprefix("#") - rgba = data.rgba if data.rgba is not None else (cur_color + "FF") + # Handed over as stored. The opaque alpha this used to append was folded + # straight back off by `spoolman_color_hex` below, so the two paths landed on + # the same string and the append only obscured which shape was in hand (#2912). + rgba = data.rgba if data.rgba is not None else cur_color label_weight = data.label_weight if data.label_weight is not None else int(cur_filament.get("weight") or 1000) # Default weight_used from the synthetic mapping (label - remaining) so an # edit that doesn't touch the weight field preserves Spoolman's real @@ -736,7 +743,7 @@ async def update_spool( except ValueError as exc: raise HTTPException(status_code=400, detail=str(exc)) from exc - color_hex = rgba[:6] + color_hex = spoolman_color_hex(rgba) or rgba # Resolve which filament this spool should be linked to AFTER the edit. # @@ -749,14 +756,18 @@ async def update_spool( # 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] + # Compare the stored shapes, not raw strings and not bare RGB prefixes. Raw + # strings make an opaque spool's six characters differ from an incoming eight + # and PATCH the filament on every no-op edit; bare prefixes make an + # alpha-only edit invisible so the change never lands (#2912). + cur_color_norm = spoolman_color_hex(cur_filament.get("color_hex")) or "" 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_color_norm == (color_hex or "").upper() and cur_vendor_name.lower() == ((brand or "").strip().lower()) and cur_weight_int == int(label_weight) ) diff --git a/backend/app/services/spoolman.py b/backend/app/services/spoolman.py index 6c2a415e4..7a9b60a45 100644 --- a/backend/app/services/spoolman.py +++ b/backend/app/services/spoolman.py @@ -9,6 +9,8 @@ from typing import Literal import httpx +from backend.app.utils.color_utils import color_match_key, spoolman_color_hex + logger = logging.getLogger(__name__) BAMBU_RFID_TAG_LENGTH = 32 @@ -356,9 +358,10 @@ class SpoolmanClient: if material: data["material"] = material if color_hex: - # Strip alpha channel if present (RRGGBBAA -> RRGGBB) - color_hex = color_hex[:6] if len(color_hex) >= 6 else color_hex - data["color_hex"] = color_hex + # Every create funnels through here, so this is where the stored shape + # is decided: six characters for an opaque spool, eight only when the + # alpha byte says the filament is translucent. See #2912. + data["color_hex"] = spoolman_color_hex(color_hex) or color_hex if color_name: data["color_name"] = color_name if weight: @@ -757,7 +760,12 @@ class SpoolmanClient: ) -> int: """Return the filament ID matching material/name/brand/color, creating it if absent.""" name = f"{material} {subtype}".strip() if subtype else material - color = color_hex[:6].upper() if len(color_hex) >= 6 else color_hex.upper() + # One value in both roles. `color_match_key` returns the shape the colour + # would be stored as, so the key the loop below compares on and the value + # a new filament is created with are the same string by construction: an + # opaque spool keys and stores as six characters, a translucent one as + # eight, and neither can be conflated with the other (#2912). + color = color_match_key(color_hex) vendor_id: int | None = None if brand: @@ -772,7 +780,7 @@ class SpoolmanClient: filaments = await self.get_filaments() for f in filaments: f_material = (f.get("material") or "").upper() - f_color = (f.get("color_hex") or "").upper()[:6] + f_color = color_match_key(f.get("color_hex")) f_vendor = f.get("vendor") or {} f_vendor_name = (f_vendor.get("name") or "").strip().lower() @@ -1199,7 +1207,7 @@ class SpoolmanClient: material=tray.tray_type, subtype="", brand=brand, - color_hex=tray.tray_color[:6], + color_hex=tray.tray_color, label_weight=tray.tray_weight, ) except (SpoolmanNotFoundError, SpoolmanUnavailableError, SpoolmanClientError): @@ -1249,9 +1257,12 @@ class SpoolmanClient: async def _find_or_create_filament(self, tray: AMSTray) -> dict | None: """Return a Bambu Lab filament matching the tray's material/color, creating it if absent.""" bambu_vendor_id = await self.ensure_bambu_vendor() - color_hex = tray.tray_color[:6] # Strip alpha channel material_upper = tray.tray_type.upper() - color_upper = color_hex.upper() + # Same single value as the user-driven path: the match key is the stored + # shape. That is what lets an opaque tray still find the six-character + # filaments every existing instance is full of, while a clear tray keys + # to eight and gets its own record (#2912). + color = color_match_key(tray.tray_color) # Search internal filaments - only match Bambu Lab vendor filaments = await self.get_filaments() @@ -1260,8 +1271,7 @@ class SpoolmanClient: if fil_vendor_id != bambu_vendor_id: continue fil_material = filament.get("material") or "" - fil_color = filament.get("color_hex") or "" - if fil_material.upper() == material_upper and fil_color.upper() == color_upper: + if fil_material.upper() == material_upper and color_match_key(filament.get("color_hex")) == color: return filament # Search external filaments (SpoolmanDB) — restrict to Bambu Lab only. @@ -1277,8 +1287,7 @@ class SpoolmanClient: if manufacturer != "bambu lab" and not ext_id.startswith("bambulab_"): continue fil_material = filament.get("material") or "" - fil_color = filament.get("color_hex") or "" - if fil_material.upper() == material_upper and fil_color.upper() == color_upper: + if fil_material.upper() == material_upper and color_match_key(filament.get("color_hex")) == color: bambu_candidates.append(filament) if bambu_candidates: @@ -1296,7 +1305,7 @@ class SpoolmanClient: name=tray.tray_sub_brands or tray.tray_type, vendor_id=bambu_vendor_id, material=tray.tray_type, - color_hex=color_hex, + color_hex=color, weight=tray.tray_weight, ) @@ -1307,7 +1316,11 @@ class SpoolmanClient: name=external.get("name", tray.tray_sub_brands), vendor_id=vendor_id, material=external.get("material", tray.tray_type), - color_hex=external.get("color_hex", tray.tray_color[:6]), + # `or`, not a two-argument get: an entry that carries the key with an + # explicit null would hand None to create_filament rather than reach + # the tray fallback. Only a candidate when the tray colour is empty + # too, so this is a correctness tidy, not a fix for a live path. + color_hex=external.get("color_hex") or color_match_key(tray.tray_color), weight=external.get("weight", tray.tray_weight), density=external.get("density"), ) diff --git a/backend/app/utils/color_utils.py b/backend/app/utils/color_utils.py index 6d119770e..fa504e008 100644 --- a/backend/app/utils/color_utils.py +++ b/backend/app/utils/color_utils.py @@ -2,6 +2,60 @@ import math +# Alpha byte that means "fully opaque". Bambu's firmware reports every opaque +# spool as RRGGBBFF, so this is the overwhelmingly common value. +_OPAQUE_ALPHA = "FF" + + +def spoolman_color_hex(rgba: str | None) -> str | None: + """Normalise an RRGGBB(AA) value to what Spoolman's ``color_hex`` should hold. + + Eight characters only when the spool is genuinely translucent. Bambuddy used + to truncate to six unconditionally, which turned a clear spool's ``00000000`` + into opaque black (#2912); passing everything through instead would rewrite + the ``color_hex`` of every opaque spool on its next touch, churning records in + people's Spoolman for no benefit. Keeping the opaque case at six characters + leaves existing data byte-identical. + + Returns ``None`` for a missing value. A value shorter than six characters is + passed through unchanged so a malformed colour is not reshaped into something + that looks valid; a value between six and eight is truncated to six, which is + what the pre-#2912 behaviour did and what the six-character path still means. + Neither is reachable through ``_validate_rgba``, which admits only 6 or 8. + """ + if not rgba: + return None + clean = rgba.strip().removeprefix("#").upper() + if len(clean) < 6: + return clean or None + if len(clean) >= 8 and clean[6:8] != _OPAQUE_ALPHA: + return clean[:8] + return clean[:6] + + +def color_match_key(color_hex: str | None) -> str: + """Return the key two colours are compared on: **the shape they would be stored as**. + + Deliberately the same rule as :func:`spoolman_color_hex`, so two colours match + exactly when storing them would produce the same value. That settles both + directions of the alpha question at once (#2912): + + ============== ============ ================================================== + value key consequence + ============== ============ ================================================== + ``000000`` ``000000`` existing six-character data + ``000000FF`` ``000000`` still matches it — the upgrade guard, without which + the next AMS sync mints a duplicate filament for + every spool on the instance + ``00000000`` ``00000000`` a clear spool gets its own filament and is never + conflated with the black one, in either direction + ============== ============ ================================================== + + Returns ``""`` rather than ``None`` for a missing value so callers can compare + without guarding, which is the only reason this is not simply an alias. + """ + return spoolman_color_hex(color_hex) or "" + def colors_similar(hex_a: str, hex_b: str, threshold: int = 50) -> bool: """Compare two RRGGBB(AA) hex colors with tolerance for RFID/firmware variations. diff --git a/backend/tests/integration/test_spoolman_inventory_api.py b/backend/tests/integration/test_spoolman_inventory_api.py index fe53bbc58..dbdd718c5 100644 --- a/backend/tests/integration/test_spoolman_inventory_api.py +++ b/backend/tests/integration/test_spoolman_inventory_api.py @@ -378,6 +378,88 @@ class TestSpoolmanInventoryCRUD: assert call_args.args[0] == 7 assert call_args.args[1]["name"] == "PLA Matte" + @pytest.mark.asyncio + @pytest.mark.integration + async def test_create_spool_keeps_a_clear_colour_translucent( + self, + async_client: AsyncClient, + spoolman_settings, + mock_spoolman_client, + ): + """#2912: the create route truncated rgba to six characters, so entering + "fully transparent" by hand landed on the same opaque black as the AMS + case in the report.""" + payload = { + "material": "PLA", + "rgba": "00000000", + "label_weight": 1000, + "weight_used": 0, + } + response = await async_client.post("/api/v1/spoolman/inventory/spools", json=payload) + + assert response.status_code == 200 + assert mock_spoolman_client.find_or_create_filament.call_args.kwargs["color_hex"] == "00000000" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_create_spool_keeps_an_opaque_colour_at_six( + self, + async_client: AsyncClient, + spoolman_settings, + mock_spoolman_client, + ): + """The opaque case has to stay six characters or every create starts + writing a shape the rest of the instance does not hold.""" + payload = { + "material": "PLA", + "rgba": "FF0000FF", + "label_weight": 1000, + "weight_used": 0, + } + response = await async_client.post("/api/v1/spoolman/inventory/spools", json=payload) + + assert response.status_code == 200 + assert mock_spoolman_client.find_or_create_filament.call_args.kwargs["color_hex"] == "FF0000" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_update_alpha_only_edit_reaches_the_filament( + self, + async_client: AsyncClient, + spoolman_settings, + mock_spoolman_client, + ): + """#2912: making a spool translucent is a real change to the filament's + colour. Comparing bare RGB prefixes would call it a no-op and the edit + would never land.""" + # Sample filament is FF0000; make it half-transparent. + payload = {"rgba": "FF000080"} + response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload) + + assert response.status_code == 200 + mock_spoolman_client.patch_filament.assert_called_once() + assert mock_spoolman_client.patch_filament.call_args.args[1]["color_hex"] == "FF000080" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_update_with_the_round_tripped_opaque_rgba_is_a_no_op( + self, + async_client: AsyncClient, + spoolman_settings, + mock_spoolman_client, + ): + """#2912: the read side hands the frontend FF0000FF for a filament stored + as FF0000, and the edit form sends it straight back. Comparing raw strings + would make metadata_unchanged permanently False and PATCH the filament on + every no-op edit. + """ + payload = {"rgba": "FF0000FF", "note": "unrelated change"} + response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload) + + assert response.status_code == 200 + mock_spoolman_client.patch_filament.assert_not_called() + mock_spoolman_client.find_or_create_filament.assert_not_called() + @pytest.mark.asyncio @pytest.mark.integration async def test_update_shared_filament_falls_back_to_find_or_create( diff --git a/backend/tests/unit/services/test_spoolman_service.py b/backend/tests/unit/services/test_spoolman_service.py index 5e6d223a7..6188903b3 100644 --- a/backend/tests/unit/services/test_spoolman_service.py +++ b/backend/tests/unit/services/test_spoolman_service.py @@ -900,3 +900,208 @@ class TestFindOrCreateFilament: mock_create.assert_called_once() assert mock_create.call_args.kwargs["density"] == 1.31 + + +class TestColorHexAlphaHandling: + """#2912 — a clear spool must not be stored as opaque black, and widening the + stored value must not mint duplicates against inventories that hold six + characters everywhere. + """ + + @pytest.fixture + def client(self): + return SpoolmanClient("http://localhost:7912") + + def _tray(self, tray_color: str) -> AMSTray: + return AMSTray( + ams_id=0, + tray_id=0, + tray_type="PLA", + tray_sub_brands="PLA Basic", + tray_color=tray_color, + remain=100, + tag_uid="", + tray_uuid="A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4", + tray_info_idx="GFA00", + tray_weight=1000, + ) + + async def _posted_payload(self, client, color_hex: str) -> dict: + """Run create_filament and return the JSON body it actually sent.""" + with patch.object(client, "_get_client") as mock_get_client: + mock_http_client = AsyncMock() + mock_response = Mock() + mock_response.status_code = 200 + mock_response.raise_for_status = Mock() + mock_response.json = Mock(return_value={"id": 99}) + mock_http_client.post = AsyncMock(return_value=mock_response) + mock_get_client.return_value = mock_http_client + + await client.create_filament(name="PLA Basic", material="PLA", color_hex=color_hex) + + return mock_http_client.post.call_args.kwargs["json"] + + @pytest.mark.asyncio + async def test_create_filament_stores_alpha_for_a_translucent_spool(self, client): + """create_filament is the chokepoint every create funnels through — it + truncated to six characters unconditionally, which is what turned a clear + spool into opaque black.""" + payload = await self._posted_payload(client, "00000000") + assert payload["color_hex"] == "00000000" + + @pytest.mark.asyncio + async def test_create_filament_keeps_an_opaque_spool_at_six(self, client): + """Passing everything through would rewrite the color_hex of every opaque + spool on its next touch. Existing data has to stay byte-identical.""" + payload = await self._posted_payload(client, "FF0000FF") + assert payload["color_hex"] == "FF0000" + + @pytest.mark.asyncio + async def test_clear_tray_creates_a_translucent_filament(self, client): + """End-to-end through the AMS auto-create path with nothing to match.""" + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=[])), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client._find_or_create_filament(self._tray("00000000")) + + assert mock_create.call_args.kwargs["color_hex"] == "00000000" + + @pytest.mark.asyncio + async def test_opaque_tray_still_matches_an_existing_six_char_filament(self, client): + """The upgrade hazard neither the report nor the original patch mentioned. + + Every filament already in a user's Spoolman is stored six characters. If + the match compared full strings, an 8-char tray colour would stop matching + them and the next AMS sync would mint a duplicate filament for every spool + on the instance. An opaque tray keys to six characters and still matches. + """ + existing = {"id": 6, "name": "Black", "material": "PLA", "color_hex": "000000", "vendor_id": 2} + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[existing])), + patch.object(client, "get_external_filaments", AsyncMock()) as mock_external, + patch.object(client, "create_filament", AsyncMock()) as mock_create, + ): + result = await client._find_or_create_filament(self._tray("000000FF")) + + assert result is existing + mock_external.assert_not_called() + mock_create.assert_not_called() + + @pytest.mark.asyncio + async def test_clear_tray_does_not_attach_to_the_black_filament(self, client): + """A translucent tray keys to eight characters, so it does not match the + opaque filament of the same RGB and gets its own record instead.""" + black = {"id": 6, "name": "Black", "material": "PLA", "color_hex": "000000", "vendor_id": 2} + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[black])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=[])), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client._find_or_create_filament(self._tray("00000000")) + + assert mock_create.call_args.kwargs["color_hex"] == "00000000" + + @pytest.mark.asyncio + async def test_black_tray_does_not_attach_to_a_clear_filament(self, client): + """The inverse direction, which only became possible once 8-char values + were storable at all: without the alpha in the key, an opaque black roll + would match the clear filament, then render as the transparency + checkerboard and be named Clear. Whichever roll synced first would decide + and the other would be mislabelled. + """ + clear = {"id": 6, "name": "Clear", "material": "PLA", "color_hex": "00000000", "vendor_id": 2} + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[clear])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=[])), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client._find_or_create_filament(self._tray("000000FF")) + + assert mock_create.call_args.kwargs["color_hex"] == "000000" + + @pytest.mark.asyncio + async def test_clear_tray_does_not_take_a_same_rgb_external_entry(self, client): + """The reported path on a fresh Spoolman with the external library + reachable. Candidates are built with the same key, so SpoolmanDB's opaque + "PLA Basic Black" is no longer a candidate for a clear tray and the + filament is created from the tray data with its alpha intact. + """ + external = [ + { + "id": "bambulab_pla_black_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "PLA Basic Black", + "material": "PLA", + "color_hex": "000000", + }, + ] + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=[])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=external)), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client._find_or_create_filament(self._tray("00000000")) + + assert mock_create.call_args.kwargs["color_hex"] == "00000000" + + @pytest.mark.asyncio + async def test_find_or_create_filament_creates_a_clear_filament_with_its_alpha(self, client): + """The user-driven path, with nothing to match. There is no split to pin + here: the key and the created value are the same string, and for a clear + spool that string is eight characters.""" + with ( + patch.object(client, "find_or_create_vendor", AsyncMock(return_value=3)), + patch.object(client, "get_filaments", AsyncMock(return_value=[])), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client.find_or_create_filament( + material="PLA", + subtype="Basic", + brand="Bambu Lab", + color_hex="00000000", + label_weight=1000, + ) + + assert mock_create.call_args.kwargs["color_hex"] == "00000000" + + @pytest.mark.asyncio + async def test_find_or_create_filament_matches_an_existing_six_char_filament(self, client): + """The upgrade guard on the *other* match loop. + + `test_opaque_tray_still_matches_an_existing_six_char_filament` pins it for + the AMS path. The public `find_or_create_filament` has its own loop, and + the non-BL RFID auto-create (spoolman.py, `_sync_tray_to_spoolman`) now + hands it `tray.tray_color` whole where it used to hand over + `tray.tray_color[:6]`. If the opaque fold ever came off this key, every + non-Bambu RFID spool on an instance would mint a duplicate filament on the + next sync and nothing would go red. + """ + existing = { + "id": 7, + "name": "PLA Basic", + "material": "PLA", + "color_hex": "FF0000", + "vendor": {"id": 3, "name": "Bambu Lab"}, + } + 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()) as mock_create, + ): + result = await client.find_or_create_filament( + material="PLA", + subtype="Basic", + brand="Bambu Lab", + color_hex="FF0000FF", + label_weight=1000, + ) + + assert result == 7 + mock_create.assert_not_called() diff --git a/backend/tests/unit/test_color_utils.py b/backend/tests/unit/test_color_utils.py index 822d03c21..7f5b041cb 100644 --- a/backend/tests/unit/test_color_utils.py +++ b/backend/tests/unit/test_color_utils.py @@ -1,6 +1,6 @@ """Unit tests for color_utils — hex color similarity comparison.""" -from backend.app.utils.color_utils import colors_similar +from backend.app.utils.color_utils import color_match_key, colors_similar, spoolman_color_hex class TestColorsSimilar: @@ -52,3 +52,67 @@ class TestColorsSimilar: def test_white_and_off_white(self): assert colors_similar("FFFFFF", "F0F0F0") is True + + +class TestSpoolmanColorHex: + """#2912 — what a colour is stored as in Spoolman's color_hex.""" + + def test_opaque_value_stays_six_characters(self): + """The common case must be byte-identical to what is already stored, or + every opaque spool gets rewritten on its next touch.""" + assert spoolman_color_hex("FF0000FF") == "FF0000" + + def test_translucent_value_keeps_its_alpha(self): + assert spoolman_color_hex("FF000080") == "FF000080" + + def test_fully_transparent_keeps_its_alpha(self): + """The reported case: a clear spool reads as 00000000 and must not be + stored as opaque black.""" + assert spoolman_color_hex("00000000") == "00000000" + + def test_six_character_input_passes_through(self): + assert spoolman_color_hex("00FF00") == "00FF00" + + def test_normalises_case_and_hash_prefix(self): + assert spoolman_color_hex("#ff000080") == "FF000080" + + def test_none_and_empty_return_none(self): + assert spoolman_color_hex(None) is None + assert spoolman_color_hex("") is None + + def test_short_value_passes_through_rather_than_being_padded(self): + """A malformed value is reported as it is, not reshaped into something + that looks valid.""" + assert spoolman_color_hex("FFF") == "FFF" + + +class TestColorMatchKey: + """#2912 — two colours match exactly when storing them would give the same value.""" + + def test_opaque_value_matches_its_six_character_twin(self): + """The upgrade hazard: a user's existing filaments are all stored six + characters. If an opaque 8-char value stopped matching them, the next AMS + sync would mint a duplicate filament for every spool on the instance.""" + assert color_match_key("FF0000FF") == color_match_key("FF0000") + + def test_translucent_value_does_not_match_its_opaque_twin(self): + """Both directions. A clear roll must not attach to the black filament, + and — the case that only exists once 8-char values are storable — a black + roll must not attach to a clear one and inherit its swatch and name.""" + assert color_match_key("00000000") != color_match_key("000000") + assert color_match_key("00000000") != color_match_key("000000FF") + + def test_differs_when_the_rgb_differs(self): + assert color_match_key("FF0000FF") != color_match_key("00FF00FF") + + def test_is_the_stored_shape(self): + """Stated as an invariant because three separate comparisons rely on it.""" + for value in ("FF0000", "FF0000FF", "FF000080", "00000000"): + assert color_match_key(value) == spoolman_color_hex(value) + + def test_normalises_case_and_hash_prefix(self): + assert color_match_key("#ff0000") == "FF0000" + + def test_missing_value_is_empty_string(self): + assert color_match_key(None) == "" + assert color_match_key("") == "" diff --git a/backend/tests/unit/test_spoolman_inventory_helpers.py b/backend/tests/unit/test_spoolman_inventory_helpers.py index b3b86134b..463defbe6 100644 --- a/backend/tests/unit/test_spoolman_inventory_helpers.py +++ b/backend/tests/unit/test_spoolman_inventory_helpers.py @@ -192,9 +192,37 @@ class TestMapSpoolmanSpool: result = _map_spoolman_spool(spool) assert result["rgba"] == "808080FF" - def test_eight_char_color_hex_falls_back(self): - # Only 6-char hex is valid from Spoolman; 8-char (RGBA) should fall back - spool = {**MINIMAL_SPOOL, "filament": {**MINIMAL_SPOOL["filament"], "color_hex": "FF0000FF"}} + def test_eight_char_color_hex_is_read_back_with_its_alpha(self): + """#2912: 8-char color_hex is a value the write side stores on purpose for a + translucent spool, so the read must return it rather than grey it out. + + This test previously asserted the opposite, on the premise that only 6-char + hex was valid from Spoolman. Spoolman stores whatever it is given, and + Bambuddy's own rgba fields advertise RRGGBBAA — the read was what turned a + clear spool into neutral grey. + """ + spool = {**MINIMAL_SPOOL, "filament": {**MINIMAL_SPOOL["filament"], "color_hex": "FF000080"}} + result = _map_spoolman_spool(spool) + assert result["rgba"] == "FF000080" + + def test_fully_transparent_color_hex_survives_the_read(self): + """The reported case: a clear spool stored as 00000000 must not come back + as opaque black.""" + spool = {**MINIMAL_SPOOL, "filament": {**MINIMAL_SPOOL["filament"], "color_hex": "00000000"}} + result = _map_spoolman_spool(spool) + assert result["rgba"] == "00000000" + + def test_six_char_color_hex_still_gains_the_opaque_alpha(self): + """Existing data is 6-char and must keep round-tripping unchanged — the + opaque byte is appended, not doubled onto an alpha that is already there.""" + spool = {**MINIMAL_SPOOL, "filament": {**MINIMAL_SPOOL["filament"], "color_hex": "FF0000"}} + result = _map_spoolman_spool(spool) + assert result["rgba"] == "FF0000FF" + + def test_seven_char_color_hex_falls_back(self): + """Only 6 or 8 are valid lengths; a 7-char value is malformed and still + greys out rather than being padded into something plausible.""" + spool = {**MINIMAL_SPOOL, "filament": {**MINIMAL_SPOOL["filament"], "color_hex": "FF0000F"}} result = _map_spoolman_spool(spool) assert result["rgba"] == "808080FF" diff --git a/frontend/src/__tests__/components/AssignSpoolModal.test.tsx b/frontend/src/__tests__/components/AssignSpoolModal.test.tsx index 96789b76e..750ce4e5d 100644 --- a/frontend/src/__tests__/components/AssignSpoolModal.test.tsx +++ b/frontend/src/__tests__/components/AssignSpoolModal.test.tsx @@ -109,6 +109,27 @@ describe('AssignSpoolModal', () => { expect(screen.getByText(/Jade White/)).toBeInTheDocument(); }); + it('renders a clear tray as the transparency checkerboard, not an invisible circle (#2912)', async () => { + render( + , + ); + + await waitFor(() => { + expect(screen.getByText(/PETG/)).toBeInTheDocument(); + }); + + // The tray swatch used to be painted `#00000000` directly, which is a valid + // colour the browser renders as nothing at all — a clear spool showed an + // empty circle. getSwatchStyle draws the checkerboard instead. + const swatches = Array.from(document.querySelectorAll('[style]')); + expect( + swatches.some((el) => el.style.backgroundImage.includes('repeating-conic-gradient')), + ).toBe(true); + }); + it('filters out spools already assigned to other slots', async () => { (api.getAssignments as ReturnType).mockResolvedValue([ { id: 1, spool_id: 3, printer_id: 1, ams_id: 0, tray_id: 1 }, // spool 3 assigned to different slot diff --git a/frontend/src/__tests__/components/ConfigureAmsSlotModal.test.tsx b/frontend/src/__tests__/components/ConfigureAmsSlotModal.test.tsx index e1623985b..ac896a3c7 100644 --- a/frontend/src/__tests__/components/ConfigureAmsSlotModal.test.tsx +++ b/frontend/src/__tests__/components/ConfigureAmsSlotModal.test.tsx @@ -96,6 +96,23 @@ describe('ConfigureAmsSlotModal', () => { expect(screen.queryByText('Configure AMS Slot')).not.toBeInTheDocument(); }); + it('renders a clear tray as the transparency checkerboard rather than solid black (#2912)', () => { + render( + , + ); + + const styled = Array.from(document.querySelectorAll('[style]')); + expect( + styled.some((el) => el.style.backgroundImage.includes('repeating-conic-gradient')), + ).toBe(true); + // The regression this pins: the alpha byte was sliced off before painting, + // so a clear tray rendered as opaque black — the reported symptom, in the UI. + expect(styled.some((el) => el.style.backgroundColor === 'rgb(0, 0, 0)')).toBe(false); + }); + it('renders modal when open', async () => { render(); await waitFor(() => { diff --git a/frontend/src/__tests__/components/LinkSpoolModal.test.tsx b/frontend/src/__tests__/components/LinkSpoolModal.test.tsx index a7a50bcb1..96a9dd63e 100644 --- a/frontend/src/__tests__/components/LinkSpoolModal.test.tsx +++ b/frontend/src/__tests__/components/LinkSpoolModal.test.tsx @@ -100,6 +100,25 @@ describe('LinkSpoolModal', () => { }); }); + it('paints a clear spool as the checkerboard, not an invisible swatch', async () => { + // Same swatch bug as the filament picker: this row built its background as + // `#${filament_color_hex}`, so a clear spool — which only reaches the list + // with its alpha intact because of #2912 — rendered as an empty circle. + vi.mocked(api.getUnlinkedSpools).mockResolvedValue([ + { ...mockSpools[0], id: 3, filament_name: 'Bambu PLA Clear', filament_color_hex: '00000000' }, + ]); + + render(); + + await waitFor(() => { + expect(screen.getByText(/Bambu PLA Clear/)).toBeInTheDocument(); + }); + + const swatch = document.querySelector('span.rounded-full') as HTMLElement; + expect(swatch.style.backgroundImage).toContain('repeating-conic-gradient'); + expect(swatch.style.backgroundColor).toBe(''); + }); + it('displays unlinked spools from Spoolman', async () => { render(); diff --git a/frontend/src/__tests__/components/SpoolFormModal.test.tsx b/frontend/src/__tests__/components/SpoolFormModal.test.tsx index 7a1894d90..8dbbf6266 100644 --- a/frontend/src/__tests__/components/SpoolFormModal.test.tsx +++ b/frontend/src/__tests__/components/SpoolFormModal.test.tsx @@ -703,6 +703,18 @@ vi.mock('../../components/spool-form/SpoolmanFilamentPicker', () => ({ })}> Select Filament + ); }, @@ -765,6 +777,66 @@ describe('SpoolFormModal — SpoolmanFilamentPicker integration (T2)', () => { }); }); + it('prefills a translucent filament with its own alpha, not 808080FF (#2912)', async () => { + // The guard here required exactly 6 hex chars and then appended FF. That was + // unreachable while Bambuddy never wrote 8 characters; once a clear filament + // is storable, picking it out of the Spoolman catalogue prefilled the form + // with neutral grey — the frontend twin of the read-side regex. + render( + + ); + + await waitFor(() => { + expect(screen.getByTestId('picker-select-clear-btn')).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByTestId('picker-select-clear-btn')); + + const saveButton = screen.getByRole('button', { name: /save|add spool/i }); + fireEvent.click(saveButton); + + await waitFor(() => { + expect(api.createSpoolmanInventorySpool).toHaveBeenCalledTimes(1); + }); + + const payload = vi.mocked(api.createSpoolmanInventorySpool).mock.calls[0][0] as Record; + expect(payload.rgba).toBe('00000000'); + }); + + it('still appends the opaque alpha to a 6-char catalogue colour', async () => { + render( + + ); + + await waitFor(() => { + expect(screen.getByTestId('picker-select-btn')).toBeInTheDocument(); + }); + + fireEvent.click(screen.getByTestId('picker-select-btn')); + + const saveButton = screen.getByRole('button', { name: /save|add spool/i }); + fireEvent.click(saveButton); + + await waitFor(() => { + expect(api.createSpoolmanInventorySpool).toHaveBeenCalledTimes(1); + }); + + const payload = vi.mocked(api.createSpoolmanInventorySpool).mock.calls[0][0] as Record; + expect(payload.rgba).toBe('FF0000FF'); + }); + it('includes spoolman_filament_id in the submit payload when a filament is pre-selected', async () => { render( { // PLA Basic has color_name 'Red' — should be filtered out expect(screen.queryByText(/PLA Basic/)).toBeNull(); }); + + it('paints a clear filament as the checkerboard, not an invisible swatch', () => { + // Storing the alpha is what puts 00000000 into this list in the first place, + // and the hand-rolled `#${hex}` background this swatch used to build renders + // that as nothing at all — a fully transparent circle (#2912). + const clear: SpoolmanFilamentEntry[] = [ + { ...FILAMENTS[0], id: 4, name: 'PLA Basic Clear', color_hex: '00000000', color_name: 'Clear' }, + ]; + render( + + ); + const swatch = screen.getByLabelText('inventory.spoolmanFilamentColorSwatch'); + expect(swatch.style.backgroundImage).toContain('repeating-conic-gradient'); + expect(swatch.style.backgroundColor).toBe(''); + }); }); diff --git a/frontend/src/__tests__/utils/colors.test.ts b/frontend/src/__tests__/utils/colors.test.ts index 8162e94a8..17f3370fc 100644 --- a/frontend/src/__tests__/utils/colors.test.ts +++ b/frontend/src/__tests__/utils/colors.test.ts @@ -1,6 +1,7 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { disambiguateColorNames, + getSwatchStyle, colorFamily, colorSortKey, hexToColorName, @@ -343,3 +344,48 @@ describe('disambiguateColorNames', () => { expect(disambiguateColorNames({}, {})).toEqual(['', '']); }); }); + + +describe('getSwatchStyle (#1545, #2912)', () => { + const CHECKERBOARD = 'repeating-conic-gradient(#979797 0% 25%, #f5f5f5 0% 50%)'; + + it('falls back to neutral grey for missing or unparseable input', () => { + expect(getSwatchStyle(null)).toEqual({ backgroundColor: '#808080' }); + expect(getSwatchStyle(undefined)).toEqual({ backgroundColor: '#808080' }); + expect(getSwatchStyle('')).toEqual({ backgroundColor: '#808080' }); + expect(getSwatchStyle('ABC')).toEqual({ backgroundColor: '#808080' }); + }); + + it('paints an opaque colour flat, with or without the FF byte', () => { + expect(getSwatchStyle('FF0000')).toEqual({ backgroundColor: '#FF0000' }); + expect(getSwatchStyle('FF0000FF')).toEqual({ backgroundColor: '#FF0000' }); + expect(getSwatchStyle('#FF0000FF')).toEqual({ backgroundColor: '#FF0000' }); + }); + + it('shows the checkerboard alone for a fully transparent colour', () => { + expect(getSwatchStyle('00000000')).toEqual({ + backgroundImage: CHECKERBOARD, + backgroundSize: '8px 8px', + }); + }); + + it('layers a partly translucent colour over the checkerboard (#2912)', () => { + // Regression: this used to fall through to the RGB prefix, so a 50%-alpha + // spool rendered identically to an opaque one. Spoolman mode can now store + // any non-FF alpha, so the in-between case is reachable in normal use. + const style = getSwatchStyle('FF000080'); + expect(style.backgroundColor).toBeUndefined(); + expect(style.backgroundImage).toBe( + `linear-gradient(#FF000080, #FF000080), ${CHECKERBOARD}`, + ); + expect(style.backgroundSize).toBe('100% 100%, 8px 8px'); + }); + + it('treats the alpha byte case-insensitively', () => { + expect(getSwatchStyle('ff0000ff')).toEqual({ backgroundColor: '#ff0000' }); + expect(getSwatchStyle('ff000000')).toEqual({ + backgroundImage: CHECKERBOARD, + backgroundSize: '8px 8px', + }); + }); +}); diff --git a/frontend/src/components/AssignSpoolModal.tsx b/frontend/src/components/AssignSpoolModal.tsx index f4045ff3b..f88af20b4 100644 --- a/frontend/src/components/AssignSpoolModal.tsx +++ b/frontend/src/components/AssignSpoolModal.tsx @@ -381,7 +381,7 @@ export function AssignSpoolModal({ isOpen, onClose, printerId, amsId, trayId, tr {trayInfo.color && ( )} {trayInfo.type || t('ams.emptySlot')} diff --git a/frontend/src/components/ConfigureAmsSlotModal.tsx b/frontend/src/components/ConfigureAmsSlotModal.tsx index 691d48d38..b1c47f68b 100644 --- a/frontend/src/components/ConfigureAmsSlotModal.tsx +++ b/frontend/src/components/ConfigureAmsSlotModal.tsx @@ -8,6 +8,7 @@ import { matchesPrinterModelSuffix, presetCompatibility, buildCompatibilityIndex import { toFilamentId } from './spool-form/utils'; import { Button } from './Button'; import { getAmsLabel } from '../utils/amsHelpers'; +import { getSwatchStyle } from '../utils/colors'; import { useCancellableTimeout } from '../hooks/useCancellableTimeout'; interface SlotInfo { @@ -1179,7 +1180,10 @@ export function ConfigureAmsSlotModal({ const canSave = selectedPresetId && !configureMutation.isPending; // Get display color (custom or slot default) - const displayColor = colorHex || slotInfo.trayColor?.slice(0, 6) || 'FFFFFF'; + // Not sliced to six: a clear tray reports RRGGBB00, and cutting the alpha off + // here previewed it as solid black. `colorHex` is the edited form value and is + // always six characters, so only the tray fallback ever carries an alpha (#2912). + const displayColor = colorHex || slotInfo.trayColor || 'FFFFFF'; return (
@@ -1208,7 +1212,7 @@ export function ConfigureAmsSlotModal({ {slotInfo.trayColor && ( )} @@ -1249,7 +1253,7 @@ export function ConfigureAmsSlotModal({ {slotInfo.trayColor && ( )} @@ -1468,7 +1472,7 @@ export function ConfigureAmsSlotModal({
diff --git a/frontend/src/components/SpoolFormModal.tsx b/frontend/src/components/SpoolFormModal.tsx index a6e873a75..12e5acd78 100644 --- a/frontend/src/components/SpoolFormModal.tsx +++ b/frontend/src/components/SpoolFormModal.tsx @@ -450,15 +450,18 @@ export function SpoolFormModal({ const name = filament.name || ''; const subtype = material && name.startsWith(material) ? name.slice(material.length).trim() : name; const rawHex = (filament.color_hex ?? '').replace('#', '').toUpperCase(); - // Guard against short/malformed hex values — must be exactly 6 hex chars - const colorHex = /^[0-9A-F]{6}$/.test(rawHex) ? rawHex : '808080'; + // Guard against short/malformed hex values — 6 chars (RRGGBB), or 8 when the + // filament is translucent and carries its own alpha (#2912). Rejecting 8 here + // prefilled a clear filament picked from the Spoolman catalogue as 808080FF. + const colorHex = /^[0-9A-F]{6}(?:[0-9A-F]{2})?$/.test(rawHex) ? rawHex : '808080'; + const prefillRgba = colorHex.length === 8 ? colorHex : `${colorHex}FF`; setFormData(prev => ({ ...prev, spoolman_filament_id: filament.id, material, subtype, brand: filament.vendor?.name || '', - rgba: `${colorHex}FF`, + rgba: prefillRgba, color_name: filament.color_name || '', label_weight: filament.weight ?? prev.label_weight, })); diff --git a/frontend/src/components/spool-form/SpoolmanFilamentPicker.tsx b/frontend/src/components/spool-form/SpoolmanFilamentPicker.tsx index 5b8fa42a5..c143abc52 100644 --- a/frontend/src/components/spool-form/SpoolmanFilamentPicker.tsx +++ b/frontend/src/components/spool-form/SpoolmanFilamentPicker.tsx @@ -2,6 +2,7 @@ import { useState, useRef, useEffect, useMemo } from 'react'; import { ChevronDown, Loader2, Package } from 'lucide-react'; import { useTranslation } from 'react-i18next'; import type { SpoolmanFilamentEntry } from '../../api/client'; +import { getSwatchStyle } from '../../utils/colors'; interface SpoolmanFilamentPickerProps { filaments: SpoolmanFilamentEntry[]; @@ -65,9 +66,6 @@ export function SpoolmanFilamentPicker({ setSearch(''); }; - const colorStyle = (hex: string | null): string => - hex ? `#${hex.replace('#', '')}` : '#808080'; - return (