diff --git a/CHANGELOG.md b/CHANGELOG.md index 72fe530c4..a18151cc3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,8 @@ All notable changes to Bambuddy will be documented in this file. - **GitHub backup refuses to save against a non-private repository** — While auditing real-world Bambuddy backup repos on GitHub I found several that were left public by their owners. That's a serious data leak: the settings backup only filtered `bambu_cloud_token` and `auth_secret_key`, so `mqtt_username`, `mqtt_password`, `ha_token`, `prometheus_token`, `bambu_cloud_email`, `external_url`, and the printer access codes (via K-profiles, which carry the serial number) were going to whatever visibility the user picked when they created the repo. Fix is a hard guard at every save and re-checked on every push: **`POST /github-backup/config` and `PATCH /github-backup/config`** (when the URL, token, or provider changes) run a connection test internally and return HTTP 400 unless `is_private` comes back True. Same check fires inside `run_backup()` before every scheduled or manual push, so a repository that was private at config time but later flipped to public in the provider's UI gets a clear "Backup aborted: the target repository is no longer private" failure entry instead of leaking the next backup. Implementation: each provider's `test_connection` (`GitHubBackend`, `ForgejoBackend` override, `GitLabBackend` override; `GiteaBackend` inherits unchanged) now returns `is_private: bool | None` — `True` for confirmed private, `False` for public (or GitLab's `internal`), `None` for "couldn't determine" (older self-hosted APIs, non-2xx responses). The route helper `_enforce_private_repo` rejects anything that isn't `True`, with separate error messages for the public case ("Make the repository private...") vs the unknown-visibility case ("...could not confirm..."). **Frontend** test-connection UI now renders the visibility result inline — green check + "Repository is private — safe to back up to" when confirmed, red banner with the full list of credentials at risk + "Saving is blocked until..." when public, yellow banner + "could not determine" when null. Three new i18n keys (`repoIsPrivate`, `repoIsPublicWarning`, `repoVisibilityUnknown`) translated across all 8 locales; parity holds at 4830 leaves. **Wiki** `docs/features/backup.md` gains a top-level `!!! danger "Private repositories only"` block listing what's at stake and what to do if the user already has a public backup repo, plus every per-provider setup step is updated from "(can be private)" to "(**must be private**)". **Tests**: 5 new in `test_github_backup_api.py::TestGitHubBackupPrivateRepoGuard` — create rejects public (400 + "not private" in detail), create rejects unknown visibility (400 + "could not confirm"), create rejects failed test_connection (400 + propagates the underlying message), PATCH that changes the URL re-runs the check and rejects on public, PATCH that touches an unrelated field (e.g. `schedule_enabled`) does NOT call `test_connection` (proven via a mock that raises if called — without the field-change gate, every benign toggle would trigger a live API call). The existing 15 tests now use an autouse fixture that mocks `test_connection` to return private-success so they don't try to reach github.com. 4905 backend tests green. ### Fixed +- **Spoolman edit-spool: editing a spool no longer mints duplicate filaments in the Spoolman catalogue (#1357 follow-up, reported by @pgladel)** — After the initial #1357 close, the reporter showed that BB was still spawning new Spoolman filament rows on every subsequent edit. The previous fix taught `find_or_create_filament` to bridge the AMS-sync name shape (`"Glow"`) with the user-edit shape (`"PLA Glow"`), but only on the *find* path — the moment the user changed any field that fed the match key (subtype/material/brand/color_hex) the lookup missed and a brand-new filament was created, the spool was re-linked to it, and the previous filament was orphaned. Repeating the loop produced the spread the reporter screenshotted (IDs 126/127/128/129 all "Amazon Basics / PLA Glow / PLA", slight color variants). Root fix is a behaviour change in `PATCH /spoolman/inventory/spools/{id}`: before calling `find_or_create_filament`, the route now computes whether the desired metadata still matches the current linked filament and, if so, skips the lookup entirely (a no-op metadata edit — just `note` or `weight_used` — never touches the filament catalogue). When metadata IS changing it consults a new `SpoolmanClient.is_filament_shared(filament_id, exclude_spool_id)` helper: if the current filament is a *singleton* (only this spool points at it, archived spools included so a sibling-archive doesn't fake singleton-ness), the route PATCHes that filament in place via `patch_filament` — `name`, `material`, `color_hex`, `weight`, plus a `vendor_id` resolved via `find_or_create_vendor` when the brand changed. Only when the filament is genuinely shared with another spool does the route fall back to the legacy `find_or_create_filament` path, because PATCHing a shared filament would silently rewrite every sibling spool's metadata. Net effect mirrors internal-inventory behaviour ([[feedback_inventory_modes_parity]] saved this session): editing a spool updates the thing the spool already points at, instead of proliferating new entities. Three new tests in `test_spoolman_inventory_api.py::TestSpoolmanInventoryCRUD` cover the new contract: a no-op metadata edit (only `note`/`weight_used`) does NOT call `find_or_create_filament` OR `patch_filament`; a subtype change against a singleton filament calls `patch_filament(7, {...name: "PLA Matte"})` and NOT `find_or_create_filament`; the same change with `is_filament_shared` mocked to True falls back to `find_or_create_filament` and does NOT call `patch_filament`. 162 spoolman-inventory tests + 192 broader spoolman tests green; ruff clean. + - **Inventory: "Print labels…" now works in Spoolman mode** — Both endpoints already exist (`POST /inventory/labels` for the built-in table, `POST /spoolman/labels` for Spoolman), and the `LabelTemplatePickerModal` correctly branches on a `spoolmanMode` prop. But the modal was instantiated in `InventoryPage.tsx` with `spoolmanMode={false}` hard-coded, with a stale comment from the original PR claiming "Spoolman path hands users an iframe straight to Spoolman so the per-spool button never shows in that context". That assumption stopped being true when the unified inventory UI shipped — the per-spool button DOES show in Spoolman mode now, but every click resolved to `/inventory/labels` with Spoolman spool IDs and returned `404 Spool(s) not found`. Fix passes the actual `spoolmanMode` value through to the modal (one-line change, plus removing the stale comment block). The existing `LabelTemplatePickerModal.test.tsx` already covers both branches at the component level — the gap was that no test exercised the InventoryPage wiring. This is another instance of the parity rule from [#1390 follow-up]: inventory features must ship the same UX in both modes; per the new feedback memory, any future inventory change gets a mental checklist of both routes + both client methods + both UI gates before being considered shipped. ### Added diff --git a/backend/app/api/routes/spoolman_inventory.py b/backend/app/api/routes/spoolman_inventory.py index f1302859a..47d5a9784 100644 --- a/backend/app/api/routes/spoolman_inventory.py +++ b/backend/app/api/routes/spoolman_inventory.py @@ -597,15 +597,66 @@ async def update_spool( storage_location = data.storage_location if storage_location_changed else None color_hex = rgba[:6] - async with _translate_spoolman_errors(): - filament_id = await client.find_or_create_filament( - material=material, - subtype=subtype or "", - brand=brand, - color_hex=color_hex, - label_weight=label_weight, - color_name=color_name, - ) + + # Resolve which filament this spool should be linked to AFTER the edit. + # + # The old behaviour was always `find_or_create_filament`, which proliferated + # duplicate Spoolman filaments whenever the user changed any field that + # made up the match key (material/subtype/brand/color) — every edit minted + # a fresh row and orphaned the previous one (#1357 follow-up). To match + # internal-mode behaviour ([[feedback_inventory_modes_parity]]: editing a + # spool does not proliferate new entities), prefer PATCHing the current + # filament in place when it's a singleton. + cur_filament_id = cur_filament.get("id") + desired_name = f"{material} {subtype}".strip() if subtype else material + cur_color_norm = (cur_filament.get("color_hex") or "").upper()[:6] + cur_vendor_name = (cur_vendor.get("name") or "").strip() + cur_weight_int = int(cur_filament.get("weight") or 0) + metadata_unchanged = ( + cur_filament_id + and (cur_filament.get("name") or "").strip() == desired_name + and (cur_filament.get("material") or "").upper() == material.upper() + and cur_color_norm == color_hex.upper() + and cur_vendor_name.lower() == ((brand or "").strip().lower()) + and cur_weight_int == int(label_weight) + ) + + if metadata_unchanged: + # No filament-side change at all — re-use the existing link, skip + # find_or_create entirely so a no-op edit (e.g. just changing + # weight_used or note) never even touches the filament catalogue. + filament_id = cur_filament_id + else: + async with _translate_spoolman_errors(): + shared = await client.is_filament_shared(cur_filament_id, spool_id) if cur_filament_id else False + if cur_filament_id and not shared: + # Singleton filament — PATCH it in place so the user's edit lands + # on the row their spool already points at instead of orphaning it. + patch_body: dict = { + "name": desired_name, + "material": material, + "color_hex": color_hex, + "weight": float(label_weight), + } + if brand: + vendor_id = await client.find_or_create_vendor(brand) + patch_body["vendor_id"] = vendor_id + async with _translate_spoolman_errors(): + await client.patch_filament(cur_filament_id, patch_body) + filament_id = cur_filament_id + else: + # Filament is shared with other spools — PATCHing it in place would + # silently rewrite their metadata too. Fall back to find-or-create + # so only this spool's link moves. + async with _translate_spoolman_errors(): + filament_id = await client.find_or_create_filament( + material=material, + subtype=subtype or "", + brand=brand, + color_hex=color_hex, + label_weight=label_weight, + color_name=color_name, + ) if not filament_id: raise HTTPException(status_code=500, detail="Failed to find or create filament in Spoolman") diff --git a/backend/app/services/spoolman.py b/backend/app/services/spoolman.py index 40309d366..6e0da8f25 100644 --- a/backend/app/services/spoolman.py +++ b/backend/app/services/spoolman.py @@ -548,6 +548,23 @@ class SpoolmanClient: """Delete a spool from Spoolman.""" await self._request_spool("DELETE", spool_id, operation="delete") + async def is_filament_shared(self, filament_id: int, exclude_spool_id: int) -> bool: + """True if any spool other than ``exclude_spool_id`` is linked to ``filament_id``. + + Used by the spool-edit path to decide between PATCHing the existing + filament in place (singleton) and falling back to find_or_create + (shared — re-linking the spool is the only safe option). Includes + archived spools so a shared link doesn't suddenly look singleton just + because the sibling spool was archived. + """ + spools = await self.get_all_spools(allow_archived=True) + for s in spools: + if s.get("id") == exclude_spool_id: + continue + if ((s.get("filament") or {}).get("id")) == filament_id: + return True + return False + async def set_spool_archived(self, spool_id: int, archived: bool) -> dict: """Archive or restore a spool in Spoolman.""" response = await self._request_spool( diff --git a/backend/tests/integration/test_spoolman_inventory_api.py b/backend/tests/integration/test_spoolman_inventory_api.py index ab0f14882..ef183845c 100644 --- a/backend/tests/integration/test_spoolman_inventory_api.py +++ b/backend/tests/integration/test_spoolman_inventory_api.py @@ -67,6 +67,12 @@ def mock_spoolman_client(): mock_client.update_spool_full = AsyncMock(return_value=SAMPLE_SPOOLMAN_SPOOL) mock_client.merge_spool_extra = AsyncMock(return_value=SAMPLE_SPOOLMAN_SPOOL) mock_client.find_or_create_filament = AsyncMock(return_value=7) + mock_client.find_or_create_vendor = AsyncMock(return_value=3) + mock_client.patch_filament = AsyncMock(return_value={"id": 7}) + # Default to singleton (only this spool uses the filament) so edits + # exercise the new in-place-PATCH path; tests that need the shared + # branch override this on the fly. + mock_client.is_filament_shared = AsyncMock(return_value=False) mock_client.ensure_extra_field = AsyncMock(return_value=True) with ( @@ -323,6 +329,70 @@ class TestSpoolmanInventoryCRUD: assert response.status_code == 200 mock_spoolman_client.update_spool_full.assert_called_once() + @pytest.mark.asyncio + @pytest.mark.integration + async def test_update_noop_metadata_reuses_filament( + self, + async_client: AsyncClient, + spoolman_settings, + mock_spoolman_client, + ): + """#1357 follow-up: an edit that doesn't touch any filament-shaping + field (only weight_used / note / color_name) must NOT hit + find_or_create_filament OR patch_filament — the link stays put and + the filament catalogue is left alone.""" + payload = {"note": "just a note change", "weight_used": 50.0} + response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload) + assert response.status_code == 200 + mock_spoolman_client.find_or_create_filament.assert_not_called() + mock_spoolman_client.patch_filament.assert_not_called() + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_update_singleton_filament_patches_in_place( + self, + async_client: AsyncClient, + spoolman_settings, + mock_spoolman_client, + ): + """#1357 follow-up: when the linked filament is only used by the + spool being edited (singleton), changing the subtype must PATCH that + filament in place — NOT create a new filament and orphan the old + one. This is the exact failure the reporter showed: editing Subtype + "Red" → "Basic" minted a new "PETG Basic" filament every time. + """ + # Sample filament is "PLA Basic"; flip to "Matte" so the metadata + # actually changes and the singleton path engages. + payload = {"subtype": "Matte"} + response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload) + assert response.status_code == 200 + # Singleton path: PATCH the existing filament, do NOT find_or_create. + mock_spoolman_client.patch_filament.assert_called_once() + mock_spoolman_client.find_or_create_filament.assert_not_called() + # PATCH targets the spool's current filament (id=7) with the new name. + call_args = mock_spoolman_client.patch_filament.call_args + assert call_args.args[0] == 7 + assert call_args.args[1]["name"] == "PLA Matte" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_update_shared_filament_falls_back_to_find_or_create( + self, + async_client: AsyncClient, + spoolman_settings, + mock_spoolman_client, + ): + """#1357 follow-up: when the linked filament is shared with another + spool, PATCHing in place would silently rewrite the sibling's + metadata too. Fall back to find_or_create — only this spool's + filament_id moves.""" + mock_spoolman_client.is_filament_shared.return_value = True + payload = {"subtype": "Matte"} + response = await async_client.patch("/api/v1/spoolman/inventory/spools/42", json=payload) + assert response.status_code == 200 + mock_spoolman_client.find_or_create_filament.assert_called_once() + mock_spoolman_client.patch_filament.assert_not_called() + @pytest.mark.asyncio @pytest.mark.integration async def test_update_with_explicit_null_color_name_clears_extra(