From cb924e5c4a3b98f10f0b2634f027d843df4b29e7 Mon Sep 17 00:00:00 2001 From: Kouki Ojima Date: Wed, 30 Sep 2026 17:15:42 +0900 Subject: [PATCH] Match a Bambu roll to its own product line, not just its colour (issue #2907) (#2944) --- backend/app/api/routes/spoolman.py | 3 + backend/app/main.py | 1 + backend/app/services/color_catalog_lookup.py | 158 ++++++++ backend/app/services/spool_tag_matcher.py | 47 +-- backend/app/services/spoolman.py | 83 ++++- .../tests/integration/test_spoolman_api.py | 2 +- .../services/test_color_catalog_lookup.py | 246 +++++++++++++ ..._spoolman_extra_field_registration_2903.py | 22 +- .../test_spoolman_product_line_parity_2907.py | 155 ++++++++ .../unit/services/test_spoolman_service.py | 339 +++++++++++++++--- 10 files changed, 953 insertions(+), 103 deletions(-) create mode 100644 backend/app/services/color_catalog_lookup.py create mode 100644 backend/tests/unit/services/test_color_catalog_lookup.py create mode 100644 backend/tests/unit/services/test_spoolman_product_line_parity_2907.py diff --git a/backend/app/api/routes/spoolman.py b/backend/app/api/routes/spoolman.py index 59ce05e29..964399438 100644 --- a/backend/app/api/routes/spoolman.py +++ b/backend/app/api/routes/spoolman.py @@ -363,6 +363,7 @@ async def sync_printer_ams( sync_result = await client.sync_ams_tray( tray, printer.name, + db, # Per-print tracking owns weight updates (#1119); manual sync # only refreshes spool metadata + slot assignments here. disable_weight_sync=True, @@ -580,6 +581,7 @@ async def sync_all_printers( sync_result = await client.sync_ams_tray( tray, printer.name, + db, # Per-print tracking owns weight updates (#1119); manual # sync-all only refreshes spool metadata + slot assignments. disable_weight_sync=True, @@ -1293,6 +1295,7 @@ async def create_spool_from_slot( sync_result = await client.sync_ams_tray( tray, printer.name, + db, disable_weight_sync=True, auto_add_unknown_rfid=True, ) diff --git a/backend/app/main.py b/backend/app/main.py index 97e4939b1..577a045c9 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -3013,6 +3013,7 @@ async def on_ams_change(printer_id: int, ams_data: list): result = await client.sync_ams_tray( tray, printer_name, + db, # Per-print tracking is the only weight writer (#1119). # AMS auto-sync still maintains spool metadata / slot # assignments but no longer touches remaining_weight. diff --git a/backend/app/services/color_catalog_lookup.py b/backend/app/services/color_catalog_lookup.py new file mode 100644 index 000000000..b8426631a --- /dev/null +++ b/backend/app/services/color_catalog_lookup.py @@ -0,0 +1,158 @@ +"""Resolve a Bambu Lab roll's colour from the colour catalogue, and recognise its product line. + +Extracted so the two inventory modes stop answering the same question in two +places. ``spool_tag_matcher.create_spool_from_tray`` had this logic inline since +#857, and three separate fixes landed there without crossing to the Spoolman +side -- the sub-brand filter (#1227), the alpha guard (#1545) and the translucent +handling. #2907 is the fourth. Both callers now go through here. +""" + +from dataclasses import dataclass + +from sqlalchemy import func, select +from sqlalchemy.ext.asyncio import AsyncSession + +from backend.app.models.color_catalog import ColorCatalogEntry + +BAMBU_MANUFACTURER = "BAMBU LAB" + + +@dataclass(frozen=True) +class CatalogColor: + """What the catalogue row says about a roll's colour. + + The name alone is not enough for the built-in path: a row can carry gradient + stops (``extra_colors``) and a rendering hint (``effect_type``), which is how + a roll the AMS identified draws the same as one picked by hand from that row. + """ + + name: str + extra_colors: str | None = None + effect_type: str | None = None + + +async def resolve_bambu_color(db: AsyncSession, rgba: str | None, sub_brand: str | None) -> CatalogColor | None: + """The catalogue's colour for this roll, or None when it has no row for it. + + Hex alone is not an identity: ``#FFFFFF`` is "Jade White" in PLA Basic, + "Ivory White" in PLA Matte and "White" in PLA Silk. The printer reports which + product line the roll belongs to as ``tray_sub_brands``, and the catalogue + stores the same string in its ``material`` column -- so the two compare + directly, with no need to rebuild one from a type plus a subtype. + + None is a real answer and not a failure. The catalogue is seeded from Bambu's + published hex list and lags new colours, so a roll it has never heard of has + no name to give and the caller has to cope rather than pick something. + + Alpha 00 short-circuits to "Clear" (#1545) before the catalogue is consulted. + The catalogue stores RGB only, so a clear roll's ``00000000`` would look up + ``#000000`` and come back "Black" -- the exact bug #1545 was filed for. + + That short-circuit is also a limit worth stating. "Clear" is the answer for + every product line, so a PLA Matte clear roll and a PLA Basic clear roll get + the same name, and nothing downstream that keys on the name can tell them + apart. The Spoolman path's product-line criterion is inert for translucent + rolls for exactly this reason. Naming a genuinely translucent roll from the + catalogue would very likely beat "Clear", but that changes both modes and + what the built-in path has returned since #1545, so it is not something an + extraction should decide on its way past. + """ + if not rgba: + return None + # #1545, and the same test the built-in path has always applied. + if len(rgba) == 8 and rgba[6:8].lower() == "00": + return CatalogColor("Clear") + if len(rgba) < 6: + return None + hex_prefix = f"#{rgba[:6].upper()}" + query = ( + select(ColorCatalogEntry) + .where(func.upper(ColorCatalogEntry.hex_color) == hex_prefix) + .where(func.upper(ColorCatalogEntry.manufacturer) == BAMBU_MANUFACTURER) + ) + if sub_brand: + query = query.where(func.upper(ColorCatalogEntry.material) == sub_brand.upper()) + # Deterministic tiebreak for the case the sub-brand filter cannot settle -- + # a roll reporting no sub-brand at all. + query = query.order_by(ColorCatalogEntry.id).limit(1) + entry = (await db.execute(query)).scalar_one_or_none() + if entry is None: + return None + return CatalogColor(entry.color_name, entry.extra_colors, entry.effect_type) + + +def _normalise(name: str | None) -> str: + # "Tough+ Black" and "Silk+ Gold" are the library's spelling of lines the + # printer reports as "PLA Tough" and "PLA Silk". The plus is part of the + # product name, not the line, so it is dropped before comparing. + return " ".join((name or "").replace("+", " ").lower().split()) + + +def _line_word(sub_brand: str | None, material: str | None) -> str: + """The product line with the base material taken off: "PLA Matte" -> "matte".""" + line = _normalise(sub_brand) + base = _normalise(material) + if base and line.startswith(base + " "): + return line[len(base) + 1 :] + return "" + + +def product_line_rank( + filament_name: str | None, + color_name: str | None, + sub_brand: str | None, + material: str | None, +) -> int | None: + """How specifically a filament name identifies this roll's product line, or None if it does not. + + Material and colour alone cannot answer this: PLA Basic Black and PLA Matte + Charcoal are both PLA at ``#000000``, which is why a Matte roll was linked to + the Basic filament (#2907). The name is the only field on a Spoolman filament + that carries the line. + + Three spellings count, ranked by how much of the line they carry: + + 0. the sub-brand itself ("PLA Matte") -- what Bambuddy names the filaments + it creates, and has since before this fix; + 1. the line folded into the colour ("Matte Charcoal", "Tough+ Black", + "Brown Galaxy") -- how SpoolmanDB names every Bambu Lab entry outside PLA + Basic, and so what a filament taken from the library used to be called; + 2. the bare colour name ("Black") -- how SpoolmanDB names PLA Basic, and the + lines that have a base material of their own (ABS, ASA, PLA-CF ...). + + Rank 2 is the weak one and callers must prefer anything better. "Black" is + the bare name for PLA Basic, PLA Tough, PETG HF and TPU 90A alike, so on its + own it cannot tell two lines of the same material apart -- it is accepted + because an existing instance is full of filaments called that and refusing + it would duplicate every one of them. + """ + name = _normalise(filament_name) + if not name: + return None + if sub_brand and name == _normalise(sub_brand): + return 0 + color = _normalise(color_name) + if not color: + return None + line = _line_word(sub_brand, material) + if line and name in (f"{line} {color}", f"{color} {line}"): + return 1 + if name == color: + return 2 + return None + + +def best_product_line_match( + filaments: list[dict], + color_name: str | None, + sub_brand: str | None, + material: str | None, +) -> dict | None: + """The filament whose name identifies this roll's line most specifically; first wins a tie.""" + best: dict | None = None + best_rank: int | None = None + for filament in filaments: + rank = product_line_rank(filament.get("name"), color_name, sub_brand, material) + if rank is not None and (best_rank is None or rank < best_rank): + best, best_rank = filament, rank + return best diff --git a/backend/app/services/spool_tag_matcher.py b/backend/app/services/spool_tag_matcher.py index 0392f12b5..9d50c6d8b 100644 --- a/backend/app/services/spool_tag_matcher.py +++ b/backend/app/services/spool_tag_matcher.py @@ -10,6 +10,7 @@ from backend.app.models.spool import Spool from backend.app.models.spool_assignment import SpoolAssignment from backend.app.schemas.spool import normalize_effect_type from backend.app.services import slot_unlink_grace +from backend.app.services.color_catalog_lookup import resolve_bambu_color from backend.app.services.slot_nozzle import resolve_slot_nozzle from backend.app.services.spool_filament_preset import printer_safe_filament_id, resolve_spool_preset from backend.app.utils.tag_normalization import ( @@ -54,7 +55,6 @@ async def create_spool_from_tray(db: AsyncSession, tray_data: dict) -> Spool: Extracts material, subtype, color, temps, and tag info from the tray dict. Looks up core_weight from the spool catalog if a Bambu Lab entry matches. """ - from backend.app.models.color_catalog import ColorCatalogEntry from backend.app.models.spool_catalog import SpoolCatalogEntry tray_type = tray_data.get("tray_type", "") # "PLA" @@ -108,42 +108,17 @@ async def create_spool_from_tray(db: AsyncSession, tray_data: dict) -> Spool: # the printer-reported material variant (`tray_sub_brands`, e.g. "PLA Matte") # so a new Ivory White roll doesn't get auto-named Jade White just because # PLA Basic happens to come first in catalog insertion order. See #1227. + # + # Clear rolls (#1545) and the catalogue's gradient stops and effect hint + # come back from the same call: the lookup is shared with the Spoolman path + # so the two modes stop answering this question differently (#2907). The + # stops and hint are the same row the spool form's colour picker reads, so + # a roll the AMS identified renders like one picked by hand from that row. rgba = tray_color if tray_color else None - color_name = None - extra_colors = None - effect_type = None - - # Transparent filament (#1545): the AMS reports alpha=00 for clear spools. - # Skip the catalog lookup — the catalog only stores RGB so 000000 would - # resolve to "Black" (or whatever else lives at that RGB), which is exactly - # the bug the cream rewrite in parse_ams_tray used to paper over. Store - # "Clear" directly and let the frontend's resolveSpoolColorName + - # hexToColorName render the swatch as a checkerboard. - if rgba and len(rgba) == 8 and rgba[6:8].lower() == "00": - color_name = "Clear" - elif rgba and len(rgba) >= 6: - hex_prefix = f"#{rgba[:6].upper()}" - cat_query = ( - select(ColorCatalogEntry) - .where(func.upper(ColorCatalogEntry.hex_color) == hex_prefix) - .where(func.upper(ColorCatalogEntry.manufacturer) == "BAMBU LAB") - ) - if tray_sub_brands: - cat_query = cat_query.where(func.upper(ColorCatalogEntry.material) == tray_sub_brands.upper()) - # Deterministic tiebreak when the material filter can't disambiguate - # (e.g. third-party spools with empty tray_sub_brands). - cat_query = cat_query.order_by(ColorCatalogEntry.id).limit(1) - cat_result = await db.execute(cat_query) - entry = cat_result.scalar_one_or_none() - if entry: - color_name = entry.color_name - # The same row the spool form's colour picker reads. It hands - # `extra_colors` and `effect_type` to the new spool when a user - # picks a colour by hand (ColorSection.selectColor), and this path - # was taking the name alone -- so a roll added by hand rendered - # its gradient and a roll the AMS identified for you did not. - extra_colors = entry.extra_colors - effect_type = entry.effect_type + catalog_color = await resolve_bambu_color(db, rgba, tray_sub_brands) + color_name = catalog_color.name if catalog_color else None + extra_colors = catalog_color.extra_colors if catalog_color else None + effect_type = catalog_color.effect_type if catalog_color else None # If tray_id_name is a human-readable name (no "-" code), fall back to it. if not color_name and tray_id_name and "-" not in tray_id_name: diff --git a/backend/app/services/spoolman.py b/backend/app/services/spoolman.py index 139b7d883..15afb646a 100644 --- a/backend/app/services/spoolman.py +++ b/backend/app/services/spoolman.py @@ -10,7 +10,9 @@ from datetime import datetime, timezone from typing import Literal import httpx +from sqlalchemy.ext.asyncio import AsyncSession +from backend.app.services.color_catalog_lookup import CatalogColor, best_product_line_match, resolve_bambu_color from backend.app.utils.color_utils import color_match_key, spoolman_color_hex logger = logging.getLogger(__name__) @@ -1245,6 +1247,7 @@ class SpoolmanClient: self, tray: AMSTray, printer_name: str, + db: AsyncSession, disable_weight_sync: bool = False, cached_spools: list[dict] | None = None, inventory_remaining: float | None = None, @@ -1309,8 +1312,10 @@ class SpoolmanClient: return None logger.info("Creating new spool in Spoolman for %s (tag: %s...)", tray.tray_sub_brands, spool_tag[:16]) + catalog_color = None if self.is_bambu_lab_spool(tray.tray_uuid, tray.tag_uid, tray.tray_info_idx): - filament = await self._find_or_create_filament(tray) + catalog_color = await resolve_bambu_color(db, tray.tray_color, (tray.tray_sub_brands or "").strip()) + filament = await self._find_or_create_filament(tray, catalog_color) filament_id = filament["id"] if filament else None else: # Non-BL spool with custom RFID: use generic vendor lookup @@ -1331,11 +1336,20 @@ class SpoolmanClient: logger.error("Failed to find or create filament for %s", tray.tray_sub_brands) return None + extra = {"tag": json.dumps(spool_tag)} + # The colour name goes where #1357 put it, not into the filament. A + # Spoolman Filament has no color_name, so the route stores the user's + # choice under spool.extra.bambu_color_name and _map_spoolman_spool + # reads that first. This path never wrote it, which is why the colour + # ended up in filament.name and the product line fell out of subtype. + # _ensure_extra_fields registers the key from the payload (#2903). + if catalog_color is not None: + extra["bambu_color_name"] = json.dumps(catalog_color.name) return await self.create_spool( filament_id=filament_id, remaining_weight=remaining, comment="Created by Bambuddy", - extra={"tag": json.dumps(spool_tag)}, + extra=extra, ) # No-RFID fallback: use the spool ID resolved from the local slot-assignment table. @@ -1365,8 +1379,21 @@ class SpoolmanClient: ) return None - 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.""" + async def _find_or_create_filament(self, tray: AMSTray, catalog_color: CatalogColor | None) -> dict | None: + """Return the Bambu Lab filament for this tray's product line, creating it if absent. + + Material plus colour is not an identity. PLA Basic Black and PLA Matte + Charcoal are both PLA at ``#000000``, so a Matte roll was linked to the + Basic filament and inherited its name (#2907). The product line lives in + ``tray_sub_brands``, so the filament's name carries it -- the same place + internal mode keeps it, as the subtype _map_spoolman_spool derives from + the name. The colour name belongs to the spool (``bambu_color_name``), + which the caller writes from ``catalog_color``. + + ``catalog_color`` is what the colour catalogue says about this roll, or + None when it has no row. It is resolved by the caller rather than here so + this client stays pure HTTP and the caller can store the name too. + """ bambu_vendor_id = await self.ensure_bambu_vendor() material_upper = tray.tray_type.upper() # Same single value as the user-driven path: the match key is the stored @@ -1374,23 +1401,27 @@ class SpoolmanClient: # 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) + sub_brand = (tray.tray_sub_brands or "").strip() + color_name = catalog_color.name if catalog_color else None # Search internal filaments - only match Bambu Lab vendor - filaments = await self.get_filaments() - for filament in filaments: + same_colour = [] + for filament in await self.get_filaments(): fil_vendor_id = filament.get("vendor_id") or filament.get("vendor", {}).get("id") if fil_vendor_id != bambu_vendor_id: continue fil_material = filament.get("material") or "" if fil_material.upper() == material_upper and color_match_key(filament.get("color_hex")) == color: - return filament + same_colour.append(filament) + existing = best_product_line_match(same_colour, color_name, sub_brand, tray.tray_type) + if existing is not None: + return existing # Search external filaments (SpoolmanDB) — restrict to Bambu Lab only. # The /api/v1/external/filament endpoint returns the full multi-vendor catalog # with no server-side filter, so without a manufacturer check the first PLA/black # hit is typically 3DJAKE or 3DXTECH, not Bambu Lab. external = await self.get_external_filaments() - sub_brand = (tray.tray_sub_brands or "").strip().lower() bambu_candidates = [] for filament in external: manufacturer = (filament.get("manufacturer") or "").strip().lower() @@ -1401,17 +1432,23 @@ class SpoolmanClient: if fil_material.upper() == material_upper and color_match_key(filament.get("color_hex")) == color: bambu_candidates.append(filament) - if bambu_candidates: - # Prefer the entry whose `name` matches the AMS `tray_sub_brands` - # (e.g. "PLA Basic", "Support for PLA/PETG Black") so the more specific - # variant wins over a generic "Black" entry when both are present. - chosen = next( - (f for f in bambu_candidates if (f.get("name") or "").strip().lower() == sub_brand), - bambu_candidates[0], - ) + # SpoolmanDB folds the product line into the name ("Matte Charcoal", + # "Tough+ Black", "Brown Galaxy") everywhere except PLA Basic and the + # lines with a base material of their own, which get the bare colour + # ("Black"). The catalogue keeps the line in its material column and a + # bare colour in color_name. So neither side's name equals the other's + # for most lines, and the selector has to rebuild the library's spelling + # from the line and the colour -- preferring it over the bare colour, + # which several lines share at the same hex. + chosen = best_product_line_match(bambu_candidates, color_name, sub_brand, tray.tray_type) + if chosen is not None: return await self._create_filament_from_external(chosen, tray) - # Not found in either source - create a new Bambu Lab filament from scratch. + # Either the catalogue has no row for this colour -- it is seeded from + # Bambu's published list and lags new releases -- or it has one and no + # external entry carries that name. Both mean the library cannot say what + # this roll is, so build it from what the printer reported rather than + # attaching to whichever candidate happened to come first. return await self.create_filament( name=tray.tray_sub_brands or tray.tray_type, vendor_id=bambu_vendor_id, @@ -1421,10 +1458,18 @@ class SpoolmanClient: ) async def _create_filament_from_external(self, external: dict, tray: AMSTray) -> dict | None: - """Create an internal Spoolman filament from an external library entry.""" + """Create an internal Spoolman filament from an external library entry. + + The entry supplies what the printer cannot: density and label weight. The + name comes from the tray, not the entry, so a filament taken from the + library is named the same way as one built from scratch and the two modes + agree on the subtype. The library's own name ("Matte Charcoal") is a + colour name with the line folded in, and the colour name is stored on the + spool instead. + """ vendor_id = await self.ensure_bambu_vendor() return await self.create_filament( - name=external.get("name", tray.tray_sub_brands), + name=tray.tray_sub_brands or external.get("name") or tray.tray_type, vendor_id=vendor_id, material=external.get("material", tray.tray_type), # `or`, not a two-argument get: an entry that carries the key with an diff --git a/backend/tests/integration/test_spoolman_api.py b/backend/tests/integration/test_spoolman_api.py index 483919ff7..37e8e0297 100644 --- a/backend/tests/integration/test_spoolman_api.py +++ b/backend/tests/integration/test_spoolman_api.py @@ -833,7 +833,7 @@ class TestSpoolmanAPI: captured_hints: list = [] - async def capturing_sync(tray, printer_name, **kwargs): + async def capturing_sync(tray, printer_name, db, **kwargs): captured_hints.append(kwargs.get("spoolman_spool_id_hint")) return None diff --git a/backend/tests/unit/services/test_color_catalog_lookup.py b/backend/tests/unit/services/test_color_catalog_lookup.py new file mode 100644 index 000000000..577301a57 --- /dev/null +++ b/backend/tests/unit/services/test_color_catalog_lookup.py @@ -0,0 +1,246 @@ +"""Direct tests for the shared colour-catalogue lookup (#2907). + +The module was extracted so the two inventory modes stop answering "what colour +is this roll" in two places -- three fixes had landed on the built-in side +without crossing to the Spoolman side. Tests that reach it only through +``_find_or_create_filament`` cannot pin what it does, because the fake session +those use ignores the query and answers with a fixed row: the manufacturer +filter, the sub-brand filter, the id tie-break and the six-character prefix are +all invisible to them. These drive a real session against seeded rows instead. +""" + +from __future__ import annotations + +import pytest + +from backend.app.models.color_catalog import ColorCatalogEntry +from backend.app.services.color_catalog_lookup import ( + CatalogColor, + best_product_line_match, + product_line_rank, + resolve_bambu_color, +) + + +async def resolve_bambu_color_name(db, rgba, sub_brand): + """The name alone, for the tests that are only about which row is chosen.""" + color = await resolve_bambu_color(db, rgba, sub_brand) + return color.name if color else None + + +async def _seed(db, rows: list[dict]) -> None: + for row in rows: + db.add(ColorCatalogEntry(**row)) + await db.commit() + + +BAMBU_BLACK = {"manufacturer": "Bambu Lab", "color_name": "Black", "hex_color": "#000000", "material": "PLA Basic"} +# As catalog_defaults.py seeds it: the line in `material`, a bare colour in +# `color_name`. An earlier version of this fixture said "Matte Charcoal" here, +# which is SpoolmanDB's spelling, not the catalogue's. +BAMBU_CHARCOAL = { + "manufacturer": "Bambu Lab", + "color_name": "Charcoal", + "hex_color": "#000000", + "material": "PLA Matte", +} + + +@pytest.mark.asyncio +async def test_the_sub_brand_picks_between_two_rows_at_the_same_hex(db_session): + """The reported defect. #000000 is two different colours depending on the line.""" + await _seed(db_session, [BAMBU_BLACK, BAMBU_CHARCOAL]) + + assert await resolve_bambu_color_name(db_session, "000000FF", "PLA Basic") == "Black" + assert await resolve_bambu_color_name(db_session, "000000FF", "PLA Matte") == "Charcoal" + + +@pytest.mark.asyncio +async def test_another_manufacturer_at_the_same_hex_is_not_an_answer(db_session): + """Only Bambu rows may name a Bambu roll, whatever else sits at that hex. + + Seeded first, so it wins the id tie-break: with the manufacturer filter gone + this returns "Panchroma Black". The sub-brand is left empty on purpose -- + with one supplied the material filter drops the Polymaker row anyway and the + test would pass whether or not the manufacturer is checked, which is no test + at all. + """ + await _seed( + db_session, + [ + {"manufacturer": "Polymaker", "color_name": "Panchroma Black", "hex_color": "#000000", "material": "PLA"}, + BAMBU_BLACK, + ], + ) + + assert await resolve_bambu_color_name(db_session, "000000FF", None) == "Black" + assert await resolve_bambu_color_name(db_session, "000000FF", "PLA Basic") == "Black" + + +@pytest.mark.asyncio +async def test_a_sub_brand_the_catalogue_does_not_carry_has_no_answer(db_session): + """A line with no row is None, not the nearest row at that hex.""" + await _seed(db_session, [BAMBU_BLACK]) + + assert await resolve_bambu_color_name(db_session, "000000FF", "PLA Nonesuch") is None + + +@pytest.mark.asyncio +async def test_no_sub_brand_falls_back_to_the_lowest_id(db_session): + """With no sub-brand the filter cannot settle it, so the lowest id wins. + + Not a third-party roll, which was the reasoning here before and was wrong: + such a roll cannot reach this module from its only caller. ``is_bambu_lab_spool`` + gates non-Bambu rolls out, and ``parse_ams_tray`` substitutes ``tray_type`` + when ``tray_sub_brands`` is empty (spoolman.py:1116-1118), so ``sub_brand`` + is never empty on that path. The built-in side does reach it, since + ``create_spool_from_tray`` passes ``tray_sub_brands`` through as it arrives. + + The ordering itself is not pinned here, and no better test exists to write. + Deleting the ``order_by`` leaves this passing; only reversing it to + ``.desc()`` fails. On SQLite the id is the rowid, so an unordered scan comes + back in id order anyway -- assigning ids against insertion order does not + separate them either -- and the suite is SQLite-only with no Postgres job, + so nothing in CI can notice if that ORDER BY goes. The determinism it buys + is real and untested; saying so is more honest than a docstring that claims + the two modes cannot disagree about which row wins. + """ + await _seed(db_session, [BAMBU_BLACK, BAMBU_CHARCOAL]) + + assert await resolve_bambu_color_name(db_session, "000000FF", None) == "Black" + assert await resolve_bambu_color_name(db_session, "000000FF", "") == "Black" + + +@pytest.mark.asyncio +async def test_only_the_first_six_characters_are_matched(db_session): + """The catalogue stores RGB; the tray reports RGBA. The alpha is not part of + the key, so an opaque roll matches on its RGB regardless of what follows.""" + await _seed(db_session, [BAMBU_BLACK]) + + assert await resolve_bambu_color_name(db_session, "000000FF", "PLA Basic") == "Black" + assert await resolve_bambu_color_name(db_session, "000000", "PLA Basic") == "Black" + assert await resolve_bambu_color_name(db_session, "0000", "PLA Basic") is None + assert await resolve_bambu_color_name(db_session, None, "PLA Basic") is None + + +@pytest.mark.asyncio +async def test_the_match_is_case_insensitive_on_both_sides(db_session): + await _seed( + db_session, + [{"manufacturer": "bambu lab", "color_name": "Black", "hex_color": "#000000", "material": "pla basic"}], + ) + + assert await resolve_bambu_color_name(db_session, "000000ff", "PLA BASIC") == "Black" + + +@pytest.mark.asyncio +async def test_an_alpha_00_roll_is_clear_and_never_reaches_the_catalogue(db_session): + """#1545, carried over deliberately -- see the docstring. + + The catalogue stores RGB only, so a clear roll's ``00000000`` would look up + ``#000000`` and come back "Black". The seeded row here is exactly that trap: + without the guard this returns "Black" and Spoolman mode names a clear roll + black while the built-in path calls it "Clear" on the same printer. + """ + await _seed(db_session, [BAMBU_BLACK]) + + assert await resolve_bambu_color_name(db_session, "00000000", "PLA Basic") == "Clear" + # Any hex with alpha 00, not just the black one. + assert await resolve_bambu_color_name(db_session, "FFFFFF00", "PLA Basic") == "Clear" + # Alpha FF is opaque and must still be looked up. + assert await resolve_bambu_color_name(db_session, "000000FF", "PLA Basic") == "Black" + + +@pytest.mark.asyncio +async def test_the_row_hands_over_its_gradient_and_effect_not_just_its_name(db_session): + """The built-in path takes all three off the row, so the shared lookup has to + return all three or that caller could not use it. The shipped seed carries no + stops on any row; a user-edited row does, and that is the case this is for.""" + await _seed( + db_session, + [ + { + "manufacturer": "Bambu Lab", + "color_name": "Arctic Whisper", + "hex_color": "#A2C8E6", + "material": "PLA Basic Gradient", + "extra_colors": "A2C8E6,FFFFFF", + "effect_type": "gradient", + } + ], + ) + + assert await resolve_bambu_color(db_session, "A2C8E6FF", "PLA Basic Gradient") == CatalogColor( + "Arctic Whisper", "A2C8E6,FFFFFF", "gradient" + ) + + +@pytest.mark.asyncio +async def test_an_unknown_colour_has_no_name_rather_than_a_wrong_one(db_session): + """The catalogue lags new releases; None is a real answer.""" + await _seed(db_session, [BAMBU_BLACK]) + + assert await resolve_bambu_color_name(db_session, "ABCDEFFF", "PLA Basic") is None + + +class TestProductLineRank: + """The three spellings a Bambu filament's name can take, ranked, and nothing else.""" + + def test_the_sub_brand_is_the_strongest(self): + """What Bambuddy names the filaments it creates.""" + assert product_line_rank("PLA Matte", "Charcoal", "PLA Matte", "PLA") == 0 + assert product_line_rank("pla matte", None, "PLA Matte", "PLA") == 0 + + def test_the_library_spelling_folds_the_line_into_the_colour(self): + """SpoolmanDB's names for these rolls, taken from a live 0.26.1 instance.""" + assert product_line_rank("Matte Charcoal", "Charcoal", "PLA Matte", "PLA") == 1 + assert product_line_rank("Translucent Teal", "Teal", "PLA Translucent", "PLA") == 1 + assert product_line_rank("HF Yellow", "Yellow", "PETG HF", "PETG") == 1 + # The plus is part of the product name, not the line the printer reports. + assert product_line_rank("Tough+ Black", "Black", "PLA Tough", "PLA") == 1 + assert product_line_rank("Silk+ Gold", "Gold", "PLA Silk", "PLA") == 1 + # Galaxy puts the line after the colour. + assert product_line_rank("Brown Galaxy", "Brown", "PLA Galaxy", "PLA") == 1 + + def test_the_bare_colour_is_the_weakest(self): + """PLA Basic's spelling in the library, and the lines with a base material + of their own -- and shared across lines, which is why it ranks last.""" + assert product_line_rank("Black", "Black", "PLA Basic", "PLA") == 2 + assert product_line_rank("Black", "Black", "PLA Tough", "PLA") == 2 + + def test_a_different_product_line_does_not_match(self): + """The reported defect: a Matte roll must not attach to PLA Basic Black.""" + assert product_line_rank("Black", "Charcoal", "PLA Matte", "PLA") is None + assert product_line_rank("PLA Basic", "Charcoal", "PLA Matte", "PLA") is None + assert product_line_rank("Tough+ Black", "Black", "PLA Basic", "PLA") is None + + def test_an_unnamed_filament_never_matches(self): + assert product_line_rank(None, "Black", "PLA Basic", "PLA") is None + assert product_line_rank("", "Black", "PLA Basic", "PLA") is None + assert product_line_rank(" ", "Black", "PLA Basic", "PLA") is None + + def test_with_neither_criterion_available_nothing_matches(self): + """No catalogue row and no sub-brand: there is nothing to compare on, and + guessing is what #2907 is about.""" + assert product_line_rank("Black", None, None, "PLA") is None + assert product_line_rank("Black", None, "", "PLA") is None + + +class TestBestProductLineMatch: + def test_the_line_spelling_beats_the_bare_colour_whatever_the_order(self): + """The PLA Tough case from review: "Black" is PLA Basic's entry and comes + first in the library, "Tough+ Black" is the roll's own.""" + basic = {"name": "Black"} + tough = {"name": "Tough+ Black"} + + assert best_product_line_match([basic, tough], "Black", "PLA Tough", "PLA") is tough + assert best_product_line_match([tough, basic], "Black", "PLA Tough", "PLA") is tough + + def test_the_bare_colour_is_taken_when_nothing_better_is_there(self): + basic = {"name": "Black"} + + assert best_product_line_match([basic], "Black", "PLA Basic", "PLA") is basic + + def test_nothing_is_better_than_the_wrong_line(self): + assert best_product_line_match([{"name": "Black"}], "Charcoal", "PLA Matte", "PLA") is None + assert best_product_line_match([], "Charcoal", "PLA Matte", "PLA") is None diff --git a/backend/tests/unit/services/test_spoolman_extra_field_registration_2903.py b/backend/tests/unit/services/test_spoolman_extra_field_registration_2903.py index 3c005c0b1..5c9bd63de 100644 --- a/backend/tests/unit/services/test_spoolman_extra_field_registration_2903.py +++ b/backend/tests/unit/services/test_spoolman_extra_field_registration_2903.py @@ -22,6 +22,24 @@ import pytest from backend.app.services.spoolman import AMSTray, SpoolmanClient +class _NoCatalog: + """A DB session whose colour catalogue has no row for anything. + + ``_find_or_create_filament`` resolves the expected colour name from the + catalogue now (#2907), so it needs a session. These tests predate the + catalogue and were written against the state where it cannot name the + colour, which is this. Tests that care about the name pass a real session. + """ + + async def execute(self, *_args, **_kwargs): + class _Result: + @staticmethod + def scalar_one_or_none(): + return None + + return _Result() + + class FakeSpoolman: """A Spoolman that rejects unregistered extra keys, the way the real one does.""" @@ -141,7 +159,7 @@ class TestTheReportedCase: fake = FakeSpoolman() # GET /field/spool returns nothing: no custom fields at all client = _client(fake) - result = await client.sync_ams_tray(_tray("D144798DEF394926ACAE9D69ABA910CC"), "OJIMPO-X2D-01") + result = await client.sync_ams_tray(_tray("D144798DEF394926ACAE9D69ABA910CC"), "OJIMPO-X2D-01", _NoCatalog()) assert result is not None, "spool creation was rejected -- this is the reported 400" assert result["extra"]["tag"] == json.dumps("D144798DEF394926ACAE9D69ABA910CC") @@ -158,7 +176,7 @@ class TestTheReportedCase: "1D1F3C49046246DBBADBC3631B7F1B61", ] - synced = [await client.sync_ams_tray(_tray(tag), "OJIMPO-X2D-01") for tag in tags] + synced = [await client.sync_ams_tray(_tray(tag), "OJIMPO-X2D-01", _NoCatalog()) for tag in tags] assert all(s is not None for s in synced) assert [s["extra"]["tag"] for s in synced] == [json.dumps(t) for t in tags] diff --git a/backend/tests/unit/services/test_spoolman_product_line_parity_2907.py b/backend/tests/unit/services/test_spoolman_product_line_parity_2907.py new file mode 100644 index 000000000..931c254ad --- /dev/null +++ b/backend/tests/unit/services/test_spoolman_product_line_parity_2907.py @@ -0,0 +1,155 @@ +"""The same Bambu roll must come out the same in both inventory modes (#2907). + +The earlier tests for this fix staged a catalogue value the seed does not +contain ("Matte Charcoal" for PLA Matte at #000000, where ``catalog_defaults.py`` +has "Charcoal") and so ran a branch the shipped catalogue never reaches. These +seed the real ``DEFAULT_COLOR_CATALOG`` and offer the real SpoolmanDB entries +for Bambu Lab PLA at #000000, as a live 0.26.1 instance returns them. The three +densities differ, which is what shows which library entry was taken. + +Each roll then goes through both modes: ``create_spool_from_tray`` for the +built-in inventory, and ``sync_ams_tray`` plus ``_map_spoolman_spool`` for +Spoolman. Material, subtype and colour name have to agree. +""" + +from __future__ import annotations + +from unittest.mock import AsyncMock, patch + +import pytest + +from backend.app.api.routes._spoolman_helpers import _map_spoolman_spool +from backend.app.core.catalog_defaults import DEFAULT_COLOR_CATALOG +from backend.app.models.color_catalog import ColorCatalogEntry +from backend.app.services.spool_tag_matcher import create_spool_from_tray +from backend.app.services.spoolman import AMSTray, SpoolmanClient + +# From GET /api/v1/external/filament on Spoolman 0.26.1, trimmed to the fields +# the selector and create_filament read. +LIBRARY = [ + { + "id": "bambulab_pla_black_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.24, + "weight": 1000.0, + }, + { + "id": "bambulab_pla_mattecharcoal_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "Matte Charcoal", + "material": "PLA", + "color_hex": "000000", + "density": 1.31, + "weight": 1000.0, + }, + { + "id": "bambulab_pla_tough+black_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "Tough+ Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.21, + "weight": 1000.0, + }, +] + +ROLLS = [ + # sub-brand, the library entry that is this roll's, subtype, colour name + ("PLA Basic", 1.24, "Basic", "Black"), + ("PLA Matte", 1.31, "Matte", "Charcoal"), + ("PLA Tough", 1.21, "Tough", "Black"), +] + + +async def _seed_the_shipped_catalogue(db) -> None: + for manufacturer, color_name, hex_color, material in DEFAULT_COLOR_CATALOG: + db.add( + ColorCatalogEntry(manufacturer=manufacturer, color_name=color_name, hex_color=hex_color, material=material) + ) + await db.commit() + + +def _tray(sub_brand: str) -> AMSTray: + return AMSTray( + ams_id=0, + tray_id=0, + tray_type="PLA", + tray_sub_brands=sub_brand, + tray_color="000000FF", + remain=100, + tag_uid="", + tray_uuid="A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4", + tray_info_idx="GFA00", + tray_weight=1000, + ) + + +async def _through_spoolman(db, sub_brand: str) -> tuple[dict, dict]: + """What Spoolman mode creates for this roll, and how the inventory then reads it.""" + client = SpoolmanClient("http://localhost:7912") + with ( + patch.object(client, "find_spool_by_tag", AsyncMock(return_value=None)), + 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=LIBRARY)), + patch.object(client, "create_filament", AsyncMock(return_value={"id": 9})) as create_filament, + patch.object(client, "create_spool", AsyncMock(return_value={"id": 99})) as create_spool, + ): + await client.sync_ams_tray(_tray(sub_brand), "TestPrinter", db) + + filament_kwargs = create_filament.call_args.kwargs + stored = { + "id": 99, + "filament": { + "id": 9, + "name": filament_kwargs["name"], + "material": filament_kwargs["material"], + "color_hex": filament_kwargs["color_hex"], + "vendor": {"id": 2, "name": "Bambu Lab"}, + }, + "initial_weight": 1000.0, + "used_weight": 0.0, + "remaining_weight": 1000.0, + "extra": create_spool.call_args.kwargs["extra"], + } + return filament_kwargs, _map_spoolman_spool(stored) + + +@pytest.mark.asyncio +@pytest.mark.parametrize(("sub_brand", "density", "subtype", "color_name"), ROLLS) +async def test_the_roll_takes_its_own_library_entry(db_session, sub_brand, density, subtype, color_name): + """Before round three: Basic took "Black", Matte reached no entry at all and + lost the library's density, and Tough took PLA Basic's "Black".""" + await _seed_the_shipped_catalogue(db_session) + + filament_kwargs, _ = await _through_spoolman(db_session, sub_brand) + + assert filament_kwargs.get("density") == density + + +@pytest.mark.asyncio +@pytest.mark.parametrize(("sub_brand", "density", "subtype", "color_name"), ROLLS) +async def test_both_modes_store_the_roll_the_same_way(db_session, sub_brand, density, subtype, color_name): + """The review's table, as an assertion. Round two had Spoolman mode put the + colour in the filament name, so its subtype read "Charcoal" where the + built-in inventory has "Matte".""" + await _seed_the_shipped_catalogue(db_session) + + internal = await create_spool_from_tray( + db_session, + { + "tray_type": "PLA", + "tray_sub_brands": sub_brand, + "tray_color": "000000FF", + "tray_uuid": "A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4", + "tray_info_idx": "GFA00", + "tray_weight": 1000, + }, + ) + _, spoolman = await _through_spoolman(db_session, sub_brand) + + assert (internal.material, internal.subtype, internal.color_name) == ("PLA", subtype, color_name) + assert (spoolman["material"], spoolman["subtype"], spoolman["color_name"]) == ("PLA", subtype, color_name) diff --git a/backend/tests/unit/services/test_spoolman_service.py b/backend/tests/unit/services/test_spoolman_service.py index 6188903b3..fadcdf5cf 100644 --- a/backend/tests/unit/services/test_spoolman_service.py +++ b/backend/tests/unit/services/test_spoolman_service.py @@ -5,13 +5,50 @@ functionality that controls whether remaining_weight is updated. Also includes tests for is_bambu_lab_spool RFID detection. """ +import json from unittest.mock import AsyncMock, Mock, patch import pytest +from backend.app.services.color_catalog_lookup import CatalogColor from backend.app.services.spoolman import AMSTray, SpoolmanClient, init_spoolman_client +class _NoCatalog: + """A DB session whose colour catalogue has no row for anything. + + ``sync_ams_tray`` resolves the roll's colour from the catalogue now (#2907), + so it needs a session. These tests predate the catalogue and were written + against the state where it cannot name the colour, which is this. + """ + + async def execute(self, *_args, **_kwargs): + class _Result: + @staticmethod + def scalar_one_or_none(): + return None + + return _Result() + + +class _BlackCatalog: + """A DB session whose colour catalogue answers "Black" for anything. + + The fake ignores the query, so it stands for the trap rather than the lookup: + a catalogue that would name a clear roll's ``000000`` if it were asked. + """ + + async def execute(self, *_args, **_kwargs): + entry = type("_Entry", (), {"color_name": "Black", "extra_colors": None, "effect_type": None})() + + class _Result: + @staticmethod + def scalar_one_or_none(): + return entry + + return _Result() + + class TestIsBambuLabSpool: """Tests for is_bambu_lab_spool — detects BL spools via RFID hardware identifiers only.""" @@ -127,7 +164,7 @@ class TestSpoolmanClient: patch.object(client, "find_spool_by_tag", AsyncMock(return_value=existing_spool)), patch.object(client, "update_spool", AsyncMock(return_value={"id": 42})) as mock_update, ): - await client.sync_ams_tray(sample_tray, "TestPrinter") + await client.sync_ams_tray(sample_tray, "TestPrinter", _NoCatalog()) mock_update.assert_called_once() call_kwargs = mock_update.call_args.kwargs @@ -142,7 +179,7 @@ class TestSpoolmanClient: patch.object(client, "find_spool_by_tag", AsyncMock(return_value=existing_spool)), patch.object(client, "update_spool", AsyncMock(return_value={"id": 42})) as mock_update, ): - await client.sync_ams_tray(sample_tray, "TestPrinter", disable_weight_sync=True) + await client.sync_ams_tray(sample_tray, "TestPrinter", _NoCatalog(), disable_weight_sync=True) mock_update.assert_called_once() call_kwargs = mock_update.call_args.kwargs @@ -159,7 +196,7 @@ class TestSpoolmanClient: patch.object(client, "_find_or_create_filament", AsyncMock(return_value=mock_filament)), patch.object(client, "create_spool", AsyncMock(return_value={"id": 99})) as mock_create, ): - await client.sync_ams_tray(sample_tray, "TestPrinter", disable_weight_sync=True) + await client.sync_ams_tray(sample_tray, "TestPrinter", _NoCatalog(), disable_weight_sync=True) mock_create.assert_called_once() call_kwargs = mock_create.call_args.kwargs @@ -167,6 +204,41 @@ class TestSpoolmanClient: assert "remaining_weight" in call_kwargs assert call_kwargs["remaining_weight"] == 500.0 # 50% of 1000g + @pytest.mark.asyncio + async def test_a_new_spool_carries_the_catalogue_colour_name(self, client, sample_tray, mock_filament): + """Where #1357 put the colour name on this side: Spoolman has no color_name + on Filament, so it lives in spool.extra.bambu_color_name and + _map_spoolman_spool reads that first. The AMS path never wrote it, which + is why the colour ended up in the filament name and the product line fell + out of the subtype (#2907, round three).""" + with ( + patch.object(client, "find_spool_by_tag", AsyncMock(return_value=None)), + patch.object(client, "_find_or_create_filament", AsyncMock(return_value=mock_filament)) as mock_find, + patch.object(client, "create_spool", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client.sync_ams_tray(sample_tray, "TestPrinter", _BlackCatalog()) + + # The same answer goes to the filament lookup and onto the spool. + assert mock_find.call_args.args[1] == CatalogColor("Black") + extra = mock_create.call_args.kwargs["extra"] + assert extra["bambu_color_name"] == json.dumps("Black") + assert "tag" in extra + + @pytest.mark.asyncio + async def test_no_catalogue_row_means_no_colour_name_rather_than_an_empty_one( + self, client, sample_tray, mock_filament + ): + """An empty string would read back as a name. Leaving the key out lets + _map_spoolman_spool fall through to the subtype, as it did before.""" + with ( + patch.object(client, "find_spool_by_tag", AsyncMock(return_value=None)), + patch.object(client, "_find_or_create_filament", AsyncMock(return_value=mock_filament)), + patch.object(client, "create_spool", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client.sync_ams_tray(sample_tray, "TestPrinter", _NoCatalog()) + + assert "bambu_color_name" not in mock_create.call_args.kwargs["extra"] + @pytest.mark.asyncio async def test_sync_ams_tray_does_not_write_location(self, client, sample_tray, existing_spool): """Verify sync_ams_tray never writes location= to Spoolman (user-managed field).""" @@ -174,7 +246,7 @@ class TestSpoolmanClient: patch.object(client, "find_spool_by_tag", AsyncMock(return_value=existing_spool)), patch.object(client, "update_spool", AsyncMock(return_value={"id": 42})) as mock_update, ): - await client.sync_ams_tray(sample_tray, "My Printer", disable_weight_sync=True) + await client.sync_ams_tray(sample_tray, "My Printer", _NoCatalog(), disable_weight_sync=True) call_kwargs = mock_update.call_args.kwargs # Bambuddy must never auto-set spool.location — it is user-managed in Spoolman @@ -216,7 +288,7 @@ class TestSpoolmanClient: AsyncMock(side_effect=SpoolmanUnavailableError("timeout")), ), ): - result = await client.sync_ams_tray(tray, "TestPrinter") + result = await client.sync_ams_tray(tray, "TestPrinter", _NoCatalog()) assert result is None @@ -250,6 +322,7 @@ class TestSpoolmanClient: result = await client.sync_ams_tray( tray, "TestPrinter", + _NoCatalog(), cached_spools=cached_spools, spoolman_spool_id_hint=99, ) @@ -285,6 +358,7 @@ class TestSpoolmanClient: result = await client.sync_ams_tray( tray, "TestPrinter", + _NoCatalog(), spoolman_spool_id_hint=99, ) @@ -309,7 +383,7 @@ class TestSpoolmanClient: tray_weight=1000, ) - result = await client.sync_ams_tray(tray, "TestPrinter") + result = await client.sync_ams_tray(tray, "TestPrinter", _NoCatalog()) assert result is None @pytest.mark.asyncio @@ -332,7 +406,7 @@ class TestSpoolmanClient: with patch.object(client, "update_spool", new_callable=AsyncMock) as mock_update: mock_update.return_value = {"id": 99} result = await client.sync_ams_tray( - tray, "TestPrinter", cached_spools=cached_spools, spoolman_spool_id_hint=99 + tray, "TestPrinter", _NoCatalog(), cached_spools=cached_spools, spoolman_spool_id_hint=99 ) assert result is not None @@ -370,7 +444,7 @@ class TestSpoolmanClient: patch.object(client, "find_spool_by_tag", AsyncMock(return_value=existing_spool)), patch.object(client, "update_spool", AsyncMock(return_value={"id": 42})) as mock_update, ): - await client.sync_ams_tray(tray, "TestPrinter", disable_weight_sync=False) + await client.sync_ams_tray(tray, "TestPrinter", _NoCatalog(), disable_weight_sync=False) call_kwargs = mock_update.call_args.kwargs assert call_kwargs["remaining_weight"] == expected, ( @@ -429,7 +503,7 @@ class TestSpoolmanClient: patch.object(client, "get_spools", AsyncMock()) as mock_get, patch.object(client, "update_spool", AsyncMock(return_value={"id": 42})), ): - await client.sync_ams_tray(sample_tray, "TestPrinter", cached_spools=cached) + await client.sync_ams_tray(sample_tray, "TestPrinter", _NoCatalog(), cached_spools=cached) mock_get.assert_not_called() # Should NOT call get_spools @pytest.mark.asyncio @@ -701,34 +775,84 @@ class TestFindOrCreateFilament: tray_weight=1000, ) - @pytest.mark.asyncio - async def test_returns_existing_internal_bambu_lab_filament(self, client, tray_pla_black): - """When a Bambu Lab filament matching material+color already exists internally, - return it as-is — never touch the external library or create a new entry. + @pytest.fixture + def tray_matte(self): + """The reported roll: PLA Matte Charcoal, same material and hex as PLA Basic Black.""" + return AMSTray( + ams_id=0, + tray_id=0, + tray_type="PLA", + tray_sub_brands="PLA Matte", + tray_color="000000FF", + remain=100, + tag_uid="", + tray_uuid="A1B2C3D4E5F6A1B2C3D4E5F6A1B2C3D4", + tray_info_idx="GFA01", + tray_weight=1000, + ) - This is the short-circuit that makes the workaround on #1309 necessary: once - a wrong name is on disk, subsequent AMS reads keep reusing it and the user has - to delete the mis-named entry manually for the corrected name to take effect. + async def _run(self, client, tray, catalog_color, *, filaments=None, external=None): + with ( + patch.object(client, "ensure_bambu_vendor", AsyncMock(return_value=2)), + patch.object(client, "get_filaments", AsyncMock(return_value=filaments or [])), + patch.object(client, "get_external_filaments", AsyncMock(return_value=external or [])) as mock_external, + patch.object(client, "create_filament", AsyncMock(return_value={"id": 99})) as mock_create, + ): + result = await client._find_or_create_filament(tray, catalog_color) + return result, mock_external, mock_create + + @pytest.mark.asyncio + async def test_reuses_an_existing_filament_of_the_same_product_line(self, client, tray_pla_black): + """The short-circuit still short-circuits — for a filament that is actually + this roll's. + + This test used to pin the version that matched on material and colour alone, + with a filament named "Black" standing in for a PLA Basic roll. That is the + defect in #2907: PLA Basic Black and PLA Matte Charcoal are both PLA at + #000000, so a Matte roll was linked to the Basic filament. The name is the + only field on a Spoolman filament that carries the product line, so it is + now part of the match. + + Named for the sub-brand here because that is what Bambuddy has been calling + its own creations (``name=tray.tray_sub_brands``) — which is what stops this + change minting a duplicate filament for every spool on an existing instance. """ existing = { "id": 6, - "name": "Black", + "name": "PLA Basic", "material": "PLA", "color_hex": "000000", # alpha stripped by create_filament at insert time "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(tray_pla_black) + result, mock_external, mock_create = await self._run(client, tray_pla_black, None, filaments=[existing]) assert result is existing mock_external.assert_not_called() mock_create.assert_not_called() + @pytest.mark.asyncio + async def test_reuses_a_filament_named_for_the_catalogue_colour(self, client, tray_pla_black): + """The bare-colour spelling: what an entry taken from the external library + used to be called, and what PLA Basic is called there. An existing instance + is full of these, so refusing it would duplicate every one.""" + existing = {"id": 6, "name": "Black", "material": "PLA", "color_hex": "000000", "vendor_id": 2} + + result, _, mock_create = await self._run(client, tray_pla_black, CatalogColor("Black"), filaments=[existing]) + + assert result is existing + mock_create.assert_not_called() + + @pytest.mark.asyncio + async def test_does_not_reuse_a_filament_from_a_different_product_line(self, client, tray_matte): + """The reported case. A PLA Matte Charcoal roll must not attach to the + PLA Basic Black filament that happens to share its material and hex.""" + pla_basic_black = {"id": 6, "name": "Black", "material": "PLA", "color_hex": "000000", "vendor_id": 2} + + _, _, mock_create = await self._run(client, tray_matte, CatalogColor("Charcoal"), filaments=[pla_basic_black]) + + mock_create.assert_called_once() + assert mock_create.call_args.kwargs["name"] == "PLA Matte" + @pytest.mark.asyncio async def test_skips_non_bambu_lab_external_entries(self, client, tray_pla_black): """Regression for #1309: the external-library loop must filter out non-Bambu-Lab @@ -768,20 +892,39 @@ class TestFindOrCreateFilament: 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(tray_pla_black) + await client._find_or_create_filament(tray_pla_black, CatalogColor("Black")) mock_create.assert_called_once() kwargs = mock_create.call_args.kwargs # The Bambu Lab entry must win — not 3DJAKE / 3DXTECH which sort earlier. - assert kwargs["name"] == "Black" + # The name is the tray's either way, so the density says which entry it was. + assert kwargs["name"] == "PLA Basic" assert kwargs["density"] == 1.26 @pytest.mark.asyncio - async def test_prefers_external_entry_matching_tray_sub_brands(self, client, tray_pla_black): - """When SpoolmanDB has multiple Bambu Lab entries for the same material+color - (e.g. a "PLA Basic" variant alongside a generic "Black"), prefer the entry - whose `name` equals the AMS `tray_sub_brands` so the more specific variant wins. - Per maintainer's request on #1309. + async def test_prefers_the_external_entry_that_carries_the_line(self, client, tray_matte): + """The tie-break selects on the line and the catalogue's colour together. + + The fixture is the real pair. A current SpoolmanDB carries, for Bambu Lab + PLA at #000000, ``bambulab_pla_black_1000_175_n`` named "Black" and + ``bambulab_pla_mattecharcoal_1000_175_n`` named "Matte Charcoal" -- both + material "PLA", because the line lives in the id and the name, never in + the material column. The previous version of this test staged an entry + named "PLA Basic" instead, which is a row SpoolmanDB does not contain: + none of its 269 Bambu Lab entries is named for a sub-brand. + + That matters for what the old code did. Against this fixture the old + equality (``name == tray_sub_brands``, i.e. "pla matte") matches nothing, + so it falls through to candidates[0] -- "Black" -- and a Matte roll is + created as PLA Basic Black. That is #2907. Against the invented fixture + it matched, which is why the old test passed. + + The catalogue value is the real one too. ``catalog_defaults.py`` seeds + #000000 under PLA Matte as "Charcoal", with the line in its material + column; the round-two version of this test staged "Matte Charcoal", which + is the library's spelling, and so exercised a branch the seed never + reaches. Neither side's name equals the other's here, which is why the + selector rebuilds the library's spelling from the line and the colour. """ external = [ { @@ -793,9 +936,9 @@ class TestFindOrCreateFilament: "density": 1.24, }, { - "id": "bambulab_plabasic_black_1000_175_n", + "id": "bambulab_pla_mattecharcoal_1000_175_n", "manufacturer": "Bambu Lab", - "name": "PLA Basic", + "name": "Matte Charcoal", "material": "PLA", "color_hex": "000000", "density": 1.26, @@ -807,13 +950,16 @@ class TestFindOrCreateFilament: 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(tray_pla_black) + await client._find_or_create_filament(tray_matte, CatalogColor("Charcoal")) mock_create.assert_called_once() kwargs = mock_create.call_args.kwargs - # "PLA Basic" wins over generic "Black" because it matches tray_sub_brands. - assert kwargs["name"] == "PLA Basic" + # "Matte Charcoal" is the entry, and the density that comes with it says + # so. candidates[0] is "Black", which the old tie-break would have taken. + # The filament is named for the line either way, as internal mode's + # subtype is. assert kwargs["density"] == 1.26 + assert kwargs["name"] == "PLA Matte" @pytest.mark.asyncio async def test_falls_back_to_create_when_no_bambu_match_anywhere(self, client, tray_pla_black): @@ -837,7 +983,7 @@ class TestFindOrCreateFilament: 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(tray_pla_black) + await client._find_or_create_filament(tray_pla_black, None) mock_create.assert_called_once() kwargs = mock_create.call_args.kwargs @@ -847,6 +993,80 @@ class TestFindOrCreateFilament: assert kwargs["color_hex"] == "000000" # alpha channel stripped from tray_color assert kwargs["vendor_id"] == 2 + @pytest.mark.asyncio + async def test_a_colour_the_catalogue_lags_is_built_from_the_tray_not_a_bambu_entry(self, client, tray_pla_black): + """A real Bambu candidate is present and deliberately not used. + + This is the first half of the decision the fall-through makes, and the + half ``test_falls_back_to_create_when_no_bambu_match_anywhere`` does not + reach: its only external entry is 3DJAKE, which the manufacturer filter + drops before ``bambu_candidates`` is built, so that test arrives at this + branch with an empty candidate list. An empty list exercises nothing + about the choice. + + Here the library does carry this roll's material and colour under Bambu + Lab, and the catalogue -- seeded from Bambu's published list, so it lags + new releases -- has no row to name it. Without a name there is nothing to + select on, and attaching to whichever candidate came first is the + misattribution #2907 is about, so the roll is built from what the printer + reported instead. + """ + external = [ + { + "id": "bambulab_pla_black_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.31, + }, + ] + _, _, mock_create = await self._run(client, tray_pla_black, None, external=external) + + mock_create.assert_called_once() + kwargs = mock_create.call_args.kwargs + # The tray's own sub-brand, not the candidate's colour name. + assert kwargs["name"] == "PLA Basic" + assert kwargs["material"] == "PLA" + assert kwargs["color_hex"] == "000000" + assert kwargs["weight"] == 1000 + # `density` reaches create_filament only via _create_filament_from_external, + # so its absence is what separates the two paths -- the name alone would + # not, since the candidate here is called "Black" for other reasons too. + assert "density" not in kwargs + + @pytest.mark.asyncio + async def test_a_name_the_library_does_not_carry_is_built_from_the_tray_too(self, client, tray_matte): + """The other half: the catalogue answers and no candidate carries that name. + + The catalogue names this roll "Charcoal" under PLA Matte; the library + offers only "Black" for Bambu Lab PLA at #000000. That is the state an instance sits + in between a colour shipping and SpoolmanDB catching up. The name is the + only field separating the two product lines at this hex, so a candidate + that does not carry it is not this roll -- and "Black" is exactly the + entry the old tie-break took. + """ + external = [ + { + "id": "bambulab_pla_black_1000_175_n", + "manufacturer": "Bambu Lab", + "name": "Black", + "material": "PLA", + "color_hex": "000000", + "density": 1.24, + }, + ] + _, _, mock_create = await self._run(client, tray_matte, CatalogColor("Charcoal"), external=external) + + mock_create.assert_called_once() + kwargs = mock_create.call_args.kwargs + # Named for the line, as every filament this path creates is; what it + # must not take is the "Black" entry's identity or the density with it. + assert kwargs["name"] == "PLA Matte" + assert kwargs["material"] == "PLA" + assert kwargs["color_hex"] == "000000" + assert "density" not in kwargs + @pytest.mark.asyncio async def test_accepts_external_entry_via_id_prefix_when_manufacturer_missing(self, client, tray_pla_black): """Defensive fallback: if `manufacturer` is absent or empty but the entry's `id` @@ -868,10 +1088,12 @@ class TestFindOrCreateFilament: 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(tray_pla_black) + await client._find_or_create_filament(tray_pla_black, CatalogColor("Black")) mock_create.assert_called_once() - assert mock_create.call_args.kwargs["name"] == "Black" + # Only the external path forwards a density, so this is what shows the + # entry was accepted rather than the roll being built from scratch. + assert mock_create.call_args.kwargs["density"] == 1.24 @pytest.mark.asyncio async def test_external_density_propagates_to_create_filament(self, client, tray_pla_black): @@ -896,7 +1118,7 @@ class TestFindOrCreateFilament: 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(tray_pla_black) + await client._find_or_create_filament(tray_pla_black, CatalogColor("Black")) mock_create.assert_called_once() assert mock_create.call_args.kwargs["density"] == 1.31 @@ -965,10 +1187,31 @@ class TestColorHexAlphaHandling: 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")) + await client._find_or_create_filament(self._tray("00000000"), None) assert mock_create.call_args.kwargs["color_hex"] == "00000000" + @pytest.mark.asyncio + async def test_a_clear_tray_is_not_named_from_the_catalogue_even_when_it_can_answer(self, client): + """#1545 at this level: the catalogue must not get to name a clear roll. + + The catalogue here does have a row -- the trap is that a clear roll + reports ``00000000`` and the catalogue stores RGB, so the lookup would hit + #000000 and come back "Black". Without the guard the spool is stored as + "Black" for a clear roll, while the built-in path calls the same roll + "Clear" on the same printer. The colour is resolved in ``sync_ams_tray`` + and handed down, so that is where this has to be driven from. + """ + with ( + patch.object(client, "find_spool_by_tag", AsyncMock(return_value=None)), + patch.object(client, "_find_or_create_filament", AsyncMock(return_value={"id": 7})) as mock_find, + patch.object(client, "create_spool", AsyncMock(return_value={"id": 99})) as mock_create, + ): + await client.sync_ams_tray(self._tray("00000000"), "TestPrinter", _BlackCatalog()) + + assert mock_find.call_args.args[1] == CatalogColor("Clear") + assert mock_create.call_args.kwargs["extra"]["bambu_color_name"] == json.dumps("Clear") + @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. @@ -985,7 +1228,10 @@ class TestColorHexAlphaHandling: 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")) + # Named after the catalogue's colour name, so the product-line + # criterion (#2907) is satisfied and the six/eight-character key is + # the only thing this test can fail on. + result = await client._find_or_create_filament(self._tray("000000FF"), CatalogColor("Black")) assert result is existing mock_external.assert_not_called() @@ -1002,7 +1248,7 @@ class TestColorHexAlphaHandling: 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")) + await client._find_or_create_filament(self._tray("00000000"), CatalogColor("Black")) assert mock_create.call_args.kwargs["color_hex"] == "00000000" @@ -1021,7 +1267,7 @@ class TestColorHexAlphaHandling: 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")) + await client._find_or_create_filament(self._tray("000000FF"), CatalogColor("Clear")) assert mock_create.call_args.kwargs["color_hex"] == "000000" @@ -1047,7 +1293,10 @@ class TestColorHexAlphaHandling: 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")) + # The catalogue is made to name this colour exactly what the external + # entry is called, so the selector (#2907) would take it. Only the + # colour key keeps it out of the candidate list at all. + await client._find_or_create_filament(self._tray("00000000"), CatalogColor("PLA Basic Black")) assert mock_create.call_args.kwargs["color_hex"] == "00000000"