diff --git a/CHANGELOG.md b/CHANGELOG.md index 1da4f2349..d14fa5b08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ All notable changes to Bambuddy will be documented in this file. - **Frontend dependency bumps** — Routine version updates across the runtime, build, and test dependency surface. **Runtime:** `dompurify` 3.4.0 → 3.4.10. `package.json` floor raised from `^3.4.0` to `^3.4.10` so fresh installs cannot land on the deprecated 3.4.4 release. Three call sites use string-output sanitisation (`frontend/src/pages/MakerworldPage.tsx`, `frontend/src/pages/ProjectDetailPage.tsx`, `frontend/src/components/ProjectPageModal.tsx`); release notes 3.4.1 → 3.4.10 reviewed for behavioural changes — 3.4.4 widened the default allow-list with `selectedcontent` + `command` + `commandfor` (all valid modern HTML, harmless for our two default-allow-list call sites), and `ProjectPageModal` is unaffected anyway because it sets an explicit `ALLOWED_TAGS` / `ALLOWED_ATTR` whitelist. **Build / lint / test tooling (transitive, dev-only):** `@babel/core` 7.29.0 → 7.29.7 (pulled by `@vitejs/plugin-react` and `eslint-plugin-react-hooks`), `vite` 7.3.2 → 7.3.5, `markdown-it` 14.1.1 → 14.2.0 (pulled by `@tiptap/extension-link` → `@tiptap/pm` → `prosemirror-markdown`; Bambuddy never calls `markdown-it.render` directly so the change is transparent), `js-yaml` 4.1.1 → 4.2.0 (pulled by `eslint`), `form-data` 4.0.5 → 4.0.6 + `ws` 8.20.1 → 8.21.0 (both pulled by `jsdom` in the test runtime). All bumps inside existing semver ranges except `dompurify`. No source changes required. ### Added +- **Centralised sidebar layout + per-page hide toggles (#1673, contributed by @EdwardChamberlain)** — Sidebar item ordering and visibility move from inline `Layout.tsx` state to a dedicated module so the same persistence rules apply whether the user is reordering with drag-and-drop, toggling an item off, or accepting the admin-pushed default. New `frontend/src/utils/sidebarLayout.ts` owns the localStorage round-trip (`sidebarOrder` + `sidebarHiddenSystemItems` keys), the `SIDEBAR_LAYOUT_CHANGED_EVENT` cross-tab refresh broadcast, and the `isExternalSidebarItemId` helper that distinguishes the new `ext-*` external link prefix from built-in nav. **Hide / show toggle:** every built-in sidebar entry (Printers / Inventory / Archives / Queue / Projects / File Manager / Makerworld / Profiles / Maintenance / Statistics — Settings is intentionally non-hideable) now carries an eye icon in the Sidebar settings card; click it to drop that entry from the rendered sidebar. Hidden IDs persist per-user via localStorage so personal taste survives reloads without leaking to other users on a shared install. Re-show by clicking the eye again. The previous drag-to-reorder UX is retired in this PR — the hide list + admin default order cover the same "I never use the Stats page" / "give me Files first" needs without the affordance ambiguity of the rearrange handle. **Admin default order:** new `default_sidebar_order` setting (validated server-side at `backend/app/schemas/settings.py:533+`) holds a JSON object `{order: string[], hiddenSystemItemIds: string[]}` that admins set once from Settings → General → Sidebar (Set Default toggle). On first login per user, `Layout.tsx`'s `useEffect` reads the admin default, filters it against the current `defaultNavItems` + valid external IDs (so a deleted external link or a removed built-in doesn't strand in someone's stored order), applies it locally, and records a per-user `sidebarDefaultApplied_` localStorage flag so the default is one-shot — later user-driven changes aren't clobbered on every login. **Settings card:** `ExternalLinksSettings.tsx` is the single source of truth for the Sidebar card (`card-sidebar-links`) in Settings → General. The header now carries the **Set Default** toggle (visible only when the caller holds `settings:write`), a **Reset** button (clears both `sidebarOrder` + `sidebarHiddenSystemItems` to defaults), and the **Add Link** button (opens the external-link create modal). The body lists every sidebar item — built-in or external — with the eye toggle inline on each row. The header row uses `flex-wrap` on the outer container and the right-side control group so the Add Link button doesn't overflow the card's right edge when Column 3 sits at its narrow `lg:max-w-sm` (384px) width. **Settings → General reordering (post-merge polish):** the **Updates** card moved to the top of Column 3 (above the new Sidebar card); the **Data Management** card moved to the bottom of Column 2 (after Library Auto-Purge) so the General tab balances better with the new Sidebar card taking column 3's vertical real estate. Anchor IDs `card-updates`, `card-data`, `card-sidebar-links` are preserved so deep-links + the in-app `registerSettingsSearch` index still resolve. **Layout merge edge case:** the PR's refactor of `Layout.tsx::isHidden` accidentally dropped the dev-side notifications gate (`!authEnabled || !advancedAuthStatus?.advanced_auth_enabled || settings?.user_notifications_enabled === false`) and its `advancedAuthStatus` useQuery. The merged shape keeps three gates in priority order — `hiddenSystemItemIds.includes(id)` first (cheapest, explicit user intent), then the array-aware `navPermissions` check from #1755 (granular `*:read_own` / `*:read_all` tiers), then the notifications-specific gate — so a user without advanced auth doesn't suddenly see the Notifications entry. **Backend:** `default_sidebar_order` settings field accepts both shapes (plain array OR `{order, hiddenSystemItemIds}` object) for backward compat with installs that saved an array under an earlier draft of this work. Validator rejects any `hiddenSystemItemIds` that isn't a `list[str]` with 422. **Tests:** 17 new backend cases in `test_sidebar_settings.py` pinning the validator (empty / JSON-array / JSON-object / mixed-types / hostile shapes). Frontend: 5 new `Layout.test.tsx` cases pinning the hide-toggle behaviour (hidden ID drops the entry, hidden ID for Settings is ignored — `settings` is non-hideable, eye-click round-trips through localStorage, `SIDEBAR_LAYOUT_CHANGED_EVENT` triggers a re-read across tabs) and 255 added/changed lines in `SettingsPage.test.tsx` covering the admin-default toggle and the eye-icon visibility column. **i18n:** new keys in the `externalLinks.*` namespace (sidebarLayout / sidebarLayoutDescription / visibleInSidebar / hiddenFromSidebar / requiredInSidebar / setDefault / etc.), full translations in all 10 non-en locales (de / es / fr / it / ja / ko / pt-BR / tr / zh-CN / zh-TW). Parity check 5168 leaves per locale. Vitest test timeout raised in `vitest.config.ts` to absorb the `userEvent.setup({delay: null})` cases in the heavier `SettingsPage` flows. Full vitest run green; ESLint clean; `npm run build` clean; ruff clean. + +- **Structured storage locations catalog (#1505 closing #1004, contributed by @Poltavtcev)** — Inventory gets a first-class catalog of physical storage spots (shelves, drawers, dryboxes) instead of free-text in the spool's `storage_location` field. Spools now carry a `location_id` FK alongside the denormalized `storage_location` string (kept for Spoolman wire format + label rendering). The Inventory page picks up a **Locations** button that opens an in-page modal — the original PR landed a standalone `/inventory/locations` page; merged shape is a modal opened from Inventory so the catalog read sits next to the spool list. The modal handles create / edit / delete / pick-to-filter; row-click pushes the location_id into the Inventory filter state without a navigation. Deep-link `?location_id=` (and `?location_id=__none__` for the unset bucket) still works for sharing or bookmarking. **Backend:** new `Location` model + `locations` table with case-insensitive `name_key` (LOWER(TRIM(name))) UNIQUE — concurrent creates on the same name resolve to a single 409 via the `IntegrityError` → re-fetch shape in `_create_location_or_get_existing`. CRUD at `/api/v1/inventory/locations`, all five routes gated with `RequirePermissionIfAuthEnabled(Permission.INVENTORY_READ|UPDATE)`. Delete is blocked while `spool_count > 0` so the user can't strand spools. Single-write-path is `location_service::resolve_spool_location_fields()` — both the internal-mode and Spoolman-mode spool routes feed through it so `location_id` and `storage_location` can never drift. **Spoolman parity:** location names sync into the local catalog on `GET /spoolman/inventory/spools` via `maybe_sync_spoolman_locations`; rename cascades to every Spoolman spool via `client.rename_location`, with a per-spool PATCH fallback when the upstream's bulk endpoint isn't there (Spoolman <0.16 doesn't expose `PATCH /location/{name}` and returns 404/405). `get_distinct_locations` normalises both the older `list[str]` and the newer `list[dict]` Spoolman payload shapes. **Migration:** inline in `database.py::run_migrations` — creates the `locations` table (DATETIME for SQLite / TIMESTAMP for Postgres), adds `spool.location_id` FK + index, then backfills the catalog from existing free-text values (GROUP BY `LOWER(TRIM(storage_location))` so case variants like `Drybox 1` and `DRYBOX 1` collapse into one row). The legacy `name_key` backfill runs BEFORE the dedup INSERT so a pre-existing locations row with NULL `name_key` (manually inserted before this feature shipped) gets its column populated first and the subsequent spool-link UPDATE can join on it. Post-migration warn-log flags any spools that still carry free-text `storage_location` with no `location_id` — surfaces the rare mis-link case to ops instead of silently leaving them out of catalog filters. **Rename safety:** Spoolman PATCH runs BEFORE `db.commit()`, cascade failure rolls back the local rename and raises HTTP 502 — without this ordering a partial failure left the catalog and Spoolman's per-spool `location` field permanently diverged (the next sync recreates the old name as a duplicate catalog row). Legacy-row UPDATE matches `func.lower(func.trim(Spool.storage_location)) == old_name.strip().lower()` so the SQL TRIM symmetry holds for whitespace-padded values. **Cross-tab refresh:** `spoolman_inventory.py` now emits `inventory_changed` on the 8 spool-mutating routes (create, bulk-create, update, delete, archive, restore, reset-bulk, weight, tag) — internal mode already broadcast in 12 places, Spoolman mode silently degraded before. The `useWebSocket` handler invalidates `inventoryLocationsQueryKey` on every such message so location counts stay in sync across tabs. **Performance:** the Spoolman→catalog sync used to fire on every `GET /spools` request, hit Spoolman, and open a write transaction; now guarded by a 60s per-URL TTL cache (`_spoolman_location_sync_last_run`) so a polling UI doesn't burn a Spoolman round-trip + SQLite write per refetch. The route also passes its already-resolved client through to the sync so test fixtures that patch the route module's client also catch the sync's client lookup — without this the SSRF LAN-topology parametrize tests took ~45s on real TCP timeouts to RFC-1918 IPs (now 2.79s in isolation). **Frontend:** `SpoolFormModal` location dropdown sends `location_id` only (same shape in both inventory modes — no `spoolmanMode ? ... : ...` UI gate) and the `onCreateLocation` flow surfaces `ApiError.message` instead of a generic toast so 409 / 400 / 500 stay distinguishable. `LocationsModal` passes `isLoading` to `ConfirmModal` during delete so a mid-mutation cancel can't strand a toast on a dismissed dialog; Pencil / Trash icon buttons carry `aria-label` for SR announcement. **i18n:** new `locations.*` namespace (20 keys: title, subtitle, add, edit, delete, empty, name, spools, manage, createPlaceholder, nameRequired, created, updated, deleted, saveFailed, deleteFailed, deleteBlocked, confirmDelete, confirmDeleteMessage, editAria/deleteAria), full translations in all 10 non-en locales (de / es / fr / it / ja / ko / pt-BR / tr / zh-CN / zh-TW). Parity check 5168 leaves per locale. **Tests:** ~26 new across `backend/tests/unit/test_location_service.py` (rename strip/lower symmetry, sync-from-Spoolman log-on-unavailable, list[dict] payload normalisation), `backend/tests/unit/test_spoolman_inventory_methods.py` (`get_distinct_locations` shape guard × 4, `rename_location` bulk-then-fallback × 4 — 200 / 404 / 405 / 5xx), `backend/tests/unit/test_location_migration.py` (NULL + whitespace-only storage_location skip, legacy NULL name_key ordering, case-variant dedup, idempotency), `backend/tests/integration/test_locations_api.py` (CRUD round-trip, rename cascade, IntegrityError → 409, PATCH/DELETE 404, auth-gate 401 on all five routes when `auth_enabled=true`), and `frontend/src/__tests__/components/LocationsModal.test.tsx` (12 cases: open=false renders nothing + no fetch, row click → onPickLocation + onClose, 2-level Escape dialog stacking, rename collision 409 toast, disabled delete on `spool_count>0`, etc.). Frontend `useWebSocket.test.ts` exercises the `inventory_changed` → invalidate `['inventory-locations']` round-trip. Full backend pytest 6025/6025 (67s with -n 30); frontend vitest 2141/2141; ruff clean; `npm run build` clean; ESLint clean; i18n parity green. + - **Admin-configurable session lifetime (#1706, reported by @AD3DStuff)** — The 24-hour session cap that ships with Bambuddy was an intentional security hardening (audit finding M-2 reduced it from 7 days), but the "Remember Me" checkbox only controlled storage location (localStorage vs sessionStorage), not session duration. iPhone PWA users and homelab admins on trusted networks were getting kicked out every 24 hours with no way to extend it. **New setting:** `session_max_hours` under Settings → Users with three presets (24h / 7 days / 30 days) plus a custom field, hard-capped at 30 days (720h). Default remains 24h so existing deployments and the M-2 audit baseline are untouched until an admin opts in. The Settings card surfaces a yellow warning whenever the value exceeds 24h: "Longer sessions reduce automatic logout protection. Recommended only for trusted single-user deployments." **Backend wiring:** new `resolve_session_max_minutes(db)` helper in `backend/app/core/auth.py` reads the setting, clamps to [1h, 720h], and falls back to 24h on missing / blank / unparseable values. The helper is called at all four token-issuance sites — plain `/auth/login`, 2FA TOTP/email completion, 2FA backup-code completion, and OIDC callback — so a long-session policy works uniformly regardless of how the user authenticates. DB errors in the resolver are deliberately NOT caught: login is already inside a transaction and a broken DB must abort the login rather than silently extend or shrink the session lifetime. Defense-in-depth `SESSION_MAX_HOURS_HARD_CEILING = 720` clamps any tampered DB row above the Pydantic ceiling. Already-issued tokens keep their original expiry — the new setting only affects future logins, so an admin lowering the value can't retroactively revoke active sessions and an admin raising it can't retroactively extend them. **What this does NOT change:** the "Remember Me" checkbox still controls only storage location (cleared on browser close vs persisted across restarts). The relabel from misleading-UX-perspective is left for a separate follow-up — that's a UX choice independent of the session-policy mechanism. API tokens (`MAX_TOKEN_LIFETIME_DAYS`), camera stream tokens (60min), WebSocket tokens (60min), and slicer download tokens (5min) keep their own TTLs and are unaffected. **Tests:** 15 new cases in `backend/tests/integration/test_session_policy.py` split across three classes. `TestResolveSessionMaxMinutes` pins the clamping resolver — missing row, empty string, unparseable value, zero/negative, 1h minimum, 7-day passthrough, 30-day passthrough, above-ceiling clamp. `TestLoginRespectsSessionPolicy` decodes the JWT `exp` claim end-to-end and asserts the token returned by `/auth/login` honours the configured ceiling for the default-24h, configured-7d, and above-ceiling-clamp cases. `TestSettingsAPIExposesSessionMaxHours` round-trips the field through `/settings/` (default = 24, valid update persists as int's string form, zero rejected with 422, above-ceiling rejected with 422). Existing 202-case auth + MFA suite still green. **i18n:** 8 new keys in `settings.sessionPolicy.*` namespace; full translations in all 10 non-en locales (de / es / fr / it / ja / ko / pt-BR / tr / zh-CN / zh-TW), no English fallback. Parity check 5149 leaves per locale. ESLint clean; `npm run build` clean; ruff clean. ### Fixed diff --git a/backend/app/api/routes/_spoolman_helpers.py b/backend/app/api/routes/_spoolman_helpers.py index 77922318a..c1471e13f 100644 --- a/backend/app/api/routes/_spoolman_helpers.py +++ b/backend/app/api/routes/_spoolman_helpers.py @@ -55,6 +55,7 @@ class MappedSpoolFields(TypedDict): updated_at: str | None cost_per_kg: float | None storage_location: str | None + location_id: int | None k_profiles: list[Any] @@ -346,5 +347,6 @@ def _map_spoolman_spool(spool: dict) -> MappedSpoolFields: "updated_at": created_at, "cost_per_kg": _safe_optional_float(spool.get("price")), "storage_location": spool.get("location") or None, + "location_id": None, "k_profiles": [], } diff --git a/backend/app/api/routes/inventory.py b/backend/app/api/routes/inventory.py index f757f9227..438f6d828 100644 --- a/backend/app/api/routes/inventory.py +++ b/backend/app/api/routes/inventory.py @@ -6,6 +6,7 @@ from fastapi import APIRouter, Depends, File, HTTPException, Query, UploadFile from fastapi.responses import Response, StreamingResponse from pydantic import BaseModel, Field, field_validator from sqlalchemy import delete, func, select +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload @@ -20,11 +21,14 @@ from backend.app.core.permissions import Permission from backend.app.core.websocket import ws_manager from backend.app.models.ams_label import AmsLabel from backend.app.models.color_catalog import ColorCatalogEntry +from backend.app.models.location import Location +from backend.app.models.settings import Settings from backend.app.models.spool import Spool from backend.app.models.spool_assignment import SpoolAssignment from backend.app.models.spool_catalog import SpoolCatalogEntry from backend.app.models.spool_k_profile import SpoolKProfile from backend.app.models.user import User +from backend.app.schemas.location import LocationCreate, LocationResponse, LocationUpdate from backend.app.schemas.spool import ( SpoolAssignmentCreate, SpoolAssignmentResponse, @@ -38,6 +42,16 @@ from backend.app.schemas.spool import ( normalize_extra_colors, ) from backend.app.schemas.spool_usage import SpoolUsageHistoryResponse +from backend.app.services.location_service import ( + DUPLICATE_LOCATION_NAME, + assign_location_name, + count_internal_spools_at_location, + get_location_by_id, + get_location_by_name, + location_name_key, + prepare_internal_spool_payload, + rename_location as rename_location_record, +) from backend.app.services.slicer_filament_resolver import resolve_slicer_filament from backend.app.services.spool_csv import ( MAX_CSV_IMPORT_BYTES, @@ -46,6 +60,7 @@ from backend.app.services.spool_csv import ( parse_and_validate, serialize, ) +from backend.app.services.spoolman import SpoolmanClient, get_spoolman_client, init_spoolman_client from backend.app.utils.filament_ids import ( GENERIC_FILAMENT_IDS, MATERIAL_TEMPS, @@ -493,6 +508,198 @@ async def reset_spool_catalog( return {"status": "reset"} +# ── Storage Locations (#1004) ─────────────────────────────────────────────── + + +async def _load_settings_map(db: AsyncSession) -> dict[str, str]: + result = await db.execute(select(Settings)) + return {s.key: s.value for s in result.scalars().all()} + + +def _spoolman_is_enabled(settings: dict[str, str]) -> bool: + return settings.get("spoolman_enabled", "false").lower() == "true" + + +async def _ensure_spoolman_client(settings: dict[str, str]) -> SpoolmanClient | None: + if not _spoolman_is_enabled(settings): + return None + url = settings.get("spoolman_url", "").strip() + if not url: + return None + from backend.app.api.routes._spoolman_helpers import assert_safe_spoolman_url + + try: + assert_safe_spoolman_url(url) + except ValueError: + return None + client = await get_spoolman_client() + if not client or client.base_url != url.rstrip("/"): + client = await init_spoolman_client(url) + return client + + +async def _spool_counts_for_locations( + db: AsyncSession, + locations: list[Location], + settings: dict[str, str], +) -> dict[int, int]: + if _spoolman_is_enabled(settings): + client = await _ensure_spoolman_client(settings) + if client: + try: + spools = await client.get_all_spools(allow_archived=False) + except Exception: + logger.warning("Failed to fetch Spoolman spools for location counts", exc_info=True) + else: + # Use the canonical key helper so this matches what the + # migration backfill, Location.name_key, and every other + # codepath store as the case-insensitive lookup key. Plain + # str.lower() drifts for non-ASCII (Turkish ı/İ, German ß) + # and caused mismatched delete-block counts in Spoolman mode. + by_key: dict[str, int] = {} + for spool in spools: + raw = spool.get("location") + if not raw or not isinstance(raw, str) or not raw.strip(): + continue + try: + key = location_name_key(raw) + except ValueError: + continue + by_key[key] = by_key.get(key, 0) + 1 + return {loc.id: by_key.get(loc.name_key, 0) for loc in locations} + + counts: dict[int, int] = {} + for loc in locations: + counts[loc.id] = await count_internal_spools_at_location(db, loc.id) + return counts + + +def _location_to_response(location: Location, spool_count: int) -> LocationResponse: + return LocationResponse( + id=location.id, + name=location.name, + identifier=location.identifier, + spool_count=spool_count, + created_at=location.created_at, + updated_at=location.updated_at, + ) + + +@router.get("/locations", response_model=list[LocationResponse]) +async def list_locations( + db: AsyncSession = Depends(get_db), + _: User | None = RequirePermissionIfAuthEnabled(Permission.INVENTORY_READ), +): + """List all storage locations with spool counts.""" + settings = await _load_settings_map(db) + result = await db.execute(select(Location).order_by(Location.name)) + locations = list(result.scalars().all()) + counts = await _spool_counts_for_locations(db, locations, settings) + return [_location_to_response(loc, counts.get(loc.id, 0)) for loc in locations] + + +@router.post("/locations", response_model=LocationResponse, status_code=201) +async def create_location( + data: LocationCreate, + db: AsyncSession = Depends(get_db), + _: User | None = RequirePermissionIfAuthEnabled(Permission.INVENTORY_UPDATE), +): + """Create a storage location.""" + existing = await get_location_by_name(db, data.name) + if existing: + raise HTTPException(status_code=409, detail=DUPLICATE_LOCATION_NAME) + location = Location(identifier=data.identifier) + assign_location_name(location, data.name) + db.add(location) + try: + await db.commit() + except IntegrityError as exc: + await db.rollback() + raise HTTPException(status_code=409, detail=DUPLICATE_LOCATION_NAME) from exc + await db.refresh(location) + await ws_manager.broadcast({"type": "inventory_changed"}) + return _location_to_response(location, 0) + + +@router.patch("/locations/{location_id}", response_model=LocationResponse) +async def update_location( + location_id: int, + data: LocationUpdate, + db: AsyncSession = Depends(get_db), + _: User | None = RequirePermissionIfAuthEnabled(Permission.INVENTORY_UPDATE), +): + """Update a storage location (rename propagates to assigned spools).""" + location = await get_location_by_id(db, location_id) + if not location: + raise HTTPException(status_code=404, detail="Location not found") + + old_name = location.name + if data.identifier is not None: + location.identifier = data.identifier or None + + if data.name is not None and data.name != old_name: + try: + await rename_location_record(db, location, data.name) + except ValueError as exc: + raise HTTPException(status_code=409, detail=str(exc)) from exc + + # Cascade to Spoolman BEFORE the local commit so a Spoolman failure + # rolls back the local rename instead of leaving the catalog and + # Spoolman's per-spool `location` field permanently diverged. Without + # this ordering, a partial failure makes the next location-sync recreate + # the old name as a duplicate catalog row (#1505 review blocker). + settings = await _load_settings_map(db) + client = await _ensure_spoolman_client(settings) + if client: + try: + await client.rename_location(old_name, location.name) + except Exception as exc: + logger.warning( + "Spoolman location rename failed for %s -> %s: %s", + old_name, + location.name, + exc, + ) + await db.rollback() + raise HTTPException( + status_code=502, + detail="Spoolman rename failed; local rename rolled back", + ) from exc + + try: + await db.commit() + except IntegrityError as exc: + await db.rollback() + raise HTTPException(status_code=409, detail=DUPLICATE_LOCATION_NAME) from exc + await db.refresh(location) + settings = await _load_settings_map(db) + counts = await _spool_counts_for_locations(db, [location], settings) + await ws_manager.broadcast({"type": "inventory_changed"}) + return _location_to_response(location, counts.get(location.id, 0)) + + +@router.delete("/locations/{location_id}") +async def delete_location( + location_id: int, + db: AsyncSession = Depends(get_db), + _: User | None = RequirePermissionIfAuthEnabled(Permission.INVENTORY_UPDATE), +): + """Delete a storage location when no spools are assigned.""" + location = await get_location_by_id(db, location_id) + if not location: + raise HTTPException(status_code=404, detail="Location not found") + + settings = await _load_settings_map(db) + counts = await _spool_counts_for_locations(db, [location], settings) + if counts.get(location.id, 0) > 0: + raise HTTPException(status_code=409, detail="Location has spools assigned and cannot be deleted") + + await db.delete(location) + await db.commit() + await ws_manager.broadcast({"type": "inventory_changed"}) + return {"status": "deleted"} + + # ── Color Catalog CRUD ───────────────────────────────────────────────────── @@ -995,7 +1202,11 @@ async def create_spool( _: User | None = RequirePermissionIfAuthEnabled(Permission.INVENTORY_UPDATE), ): """Create a new spool.""" - spool = Spool(**spool_data.model_dump()) + try: + payload = await prepare_internal_spool_payload(db, spool_data.model_dump(), set(spool_data.model_fields_set)) + except ValueError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc + spool = Spool(**payload) db.add(spool) await db.commit() await db.refresh(spool) @@ -1012,8 +1223,13 @@ async def bulk_create_spools( ): """Create multiple identical spools.""" spools = [] + fields_set = set(data.spool.model_fields_set) + try: + payload = await prepare_internal_spool_payload(db, data.spool.model_dump(), fields_set) + except ValueError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc for _ in range(data.quantity): - spool = Spool(**data.spool.model_dump()) + spool = Spool(**payload) db.add(spool) spools.append(spool) await db.commit() @@ -1037,6 +1253,10 @@ async def update_spool( raise HTTPException(404, "Spool not found") update_data = spool_data.model_dump(exclude_unset=True) + try: + update_data = await prepare_internal_spool_payload(db, update_data, set(spool_data.model_fields_set)) + except ValueError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc # Auto-lock weight when user explicitly sets weight_used if "weight_used" in update_data and "weight_locked" not in update_data: update_data["weight_locked"] = True diff --git a/backend/app/api/routes/printers.py b/backend/app/api/routes/printers.py index c6741bab9..7eed4bbcc 100644 --- a/backend/app/api/routes/printers.py +++ b/backend/app/api/routes/printers.py @@ -8,7 +8,11 @@ from fastapi.responses import Response from sqlalchemy import func, select from sqlalchemy.ext.asyncio import AsyncSession -from backend.app.core.auth import RequireCameraStreamTokenIfAuthEnabled, RequirePermissionIfAuthEnabled +from backend.app.core.auth import ( + RequireCameraStreamTokenIfAuthEnabled, + RequirePermissionIfAuthEnabled, + is_auth_enabled, +) from backend.app.core.config import settings from backend.app.core.database import get_db from backend.app.core.permissions import Permission @@ -16,6 +20,7 @@ from backend.app.core.tasks import spawn_background_task from backend.app.models.ams_label import AmsLabel from backend.app.models.printer import Printer from backend.app.models.slot_preset import SlotPresetMapping +from backend.app.models.user import User from backend.app.schemas.printer import ( AmsLabelBody, AMSTray, @@ -28,6 +33,7 @@ from backend.app.schemas.printer import ( PrinterCreate, PrinterDiagnosticResult, PrinterResponse, + PrinterResponseWithSecret, PrinterStatus, PrinterUpdate, PrintOptionsResponse, @@ -55,14 +61,50 @@ logger = logging.getLogger(__name__) router = APIRouter(prefix="/printers", tags=["printers"]) -@router.get("/", response_model=list[PrinterResponse]) +async def _caller_can_view_printer_secrets(user: User | None, db: AsyncSession) -> bool: + """Whether the caller is trusted enough to see ``access_code`` on a printer + response. Fail-CLOSED: anything that isn't an authenticated user holding + PRINTERS_UPDATE returns False. + + - Auth disabled → True (single trust domain — same as today's local UI). + - JWT user with PRINTERS_UPDATE → True (Admin or Operator; the same roles + that already manage printers and the Virtual Printer card UX that + surfaces a target's code for slicer configuration). + - JWT Viewer → False (the bug fix: Viewers must not be able to read + access_code via PRINTERS_READ and then go around Bambuddy to MQTT). + - API-key principal (``user is None`` because the dep returns None for + API keys) → False. PRINTERS_UPDATE is admin-only and absent from + ``_APIKEY_SCOPE_BY_PERMISSION``, so no API key can hold it. + """ + if not await is_auth_enabled(db): + return True + if user is None: + return False + return user.has_permission(Permission.PRINTERS_UPDATE.value) + + +def _serialize_printer(printer: Printer, *, include_secret: bool): + """Build the response shape that matches the caller's authority.""" + if include_secret: + return PrinterResponseWithSecret.model_validate(printer) + return PrinterResponse.model_validate(printer) + + +@router.get("/") async def list_printers( - _=RequirePermissionIfAuthEnabled(Permission.PRINTERS_READ), + user: User | None = RequirePermissionIfAuthEnabled(Permission.PRINTERS_READ), db: AsyncSession = Depends(get_db), ): - """List all configured printers.""" + """List all configured printers. + + ``access_code`` is included in each item only when the caller is trusted + to see it (Admin / Operator JWT, or auth-disabled mode). Viewers and + API keys never receive it. + """ result = await db.execute(select(Printer).order_by(Printer.name)) - return list(result.scalars().all()) + printers = list(result.scalars().all()) + include_secret = await _caller_can_view_printer_secrets(user, db) + return [_serialize_printer(p, include_secret=include_secret) for p in printers] @router.post("/", response_model=PrinterResponse) @@ -262,18 +304,24 @@ async def get_developer_mode_warnings( return warnings -@router.get("/{printer_id}", response_model=PrinterResponse) +@router.get("/{printer_id}") async def get_printer( printer_id: int, - _=RequirePermissionIfAuthEnabled(Permission.PRINTERS_READ), + user: User | None = RequirePermissionIfAuthEnabled(Permission.PRINTERS_READ), db: AsyncSession = Depends(get_db), ): - """Get a specific printer.""" + """Get a specific printer. + + ``access_code`` is included only when the caller is trusted to see it + (Admin / Operator JWT, or auth-disabled mode). Viewers and API keys + never receive it. + """ result = await db.execute(select(Printer).where(Printer.id == printer_id)) printer = result.scalar_one_or_none() if not printer: raise HTTPException(404, "Printer not found") - return printer + include_secret = await _caller_can_view_printer_secrets(user, db) + return _serialize_printer(printer, include_secret=include_secret) @router.patch("/{printer_id}", response_model=PrinterResponse) diff --git a/backend/app/api/routes/settings.py b/backend/app/api/routes/settings.py index 87241e41b..d0cbb1b31 100644 --- a/backend/app/api/routes/settings.py +++ b/backend/app/api/routes/settings.py @@ -446,9 +446,20 @@ async def update_spoolman_settings( if "spoolman_report_partial_usage" in settings: await set_setting(db, "spoolman_report_partial_usage", settings["spoolman_report_partial_usage"]) + spoolman_changed = ( + "spoolman_enabled" in settings + or "spoolman_url" in settings + ) + await db.commit() db.expire_all() + if spoolman_changed: + from backend.app.services.location_service import maybe_sync_spoolman_locations + + if await maybe_sync_spoolman_locations(db): + await db.commit() + # Return updated settings return await get_spoolman_settings(db) diff --git a/backend/app/api/routes/spoolman_inventory.py b/backend/app/api/routes/spoolman_inventory.py index 2a7cf2033..ab487895e 100644 --- a/backend/app/api/routes/spoolman_inventory.py +++ b/backend/app/api/routes/spoolman_inventory.py @@ -34,6 +34,7 @@ from backend.app.api.routes._spoolman_helpers import ( from backend.app.core.auth import RequirePermissionIfAuthEnabled from backend.app.core.database import get_db from backend.app.core.permissions import Permission +from backend.app.core.websocket import ws_manager from backend.app.models.ams_label import AmsLabel from backend.app.models.printer import Printer from backend.app.models.settings import Settings @@ -42,6 +43,11 @@ from backend.app.models.spoolman_slot_assignment import SpoolmanSlotAssignment from backend.app.models.user import User from backend.app.schemas.spool import SpoolKProfileBase from backend.app.schemas.spoolman import SpoolmanFilamentPatch, SpoolmanSlotAssignmentEnriched +from backend.app.services.location_service import ( + enrich_spool_dicts_with_location_id, + maybe_sync_spoolman_locations, + resolve_spoolman_location_string, +) from backend.app.services.printer_manager import printer_manager from backend.app.services.slicer_filament_resolver import resolve_slicer_filament from backend.app.services.spoolman import ( @@ -307,6 +313,7 @@ class SpoolmanInventoryCreate(BaseModel): note: str | None = Field(None, max_length=1000) cost_per_kg: float | None = Field(None, ge=0.0, le=1_000_000.0) storage_location: str | None = Field(None, max_length=255) + location_id: int | None = Field(None, gt=0) # BambuStudio slicer preset for this spool. Spoolman has no native field # for this, so we persist it under the bambu_slicer_filament[_name] keys # in the spool's extra dict and read it back in _map_spoolman_spool. @@ -349,6 +356,7 @@ class SpoolmanInventoryUpdate(BaseModel): tag_uid: str | None = Field(None, min_length=8, max_length=30, pattern=r"^[0-9A-Fa-f]+$") tray_uuid: str | None = Field(None, min_length=32, max_length=32, pattern=r"^[0-9A-Fa-f]+$") storage_location: str | None = Field(None, max_length=255) + location_id: int | None = Field(None, gt=0) # BambuStudio slicer preset — persisted to Spoolman extra dict (see Create # schema). Pass an empty string to clear; null/omitted leaves unchanged. slicer_filament: str | None = Field(None, max_length=128) @@ -430,6 +438,13 @@ async def list_spools( ) -> list[dict]: """Return all Spoolman spools in the InventorySpool format.""" client = await _get_client(db) + # Sync after we have the route-resolved client so tests that patch the + # route module's get_spoolman_client/init_spoolman_client also catch the + # sync's client lookup — otherwise the location_service path imports from + # backend.app.services.spoolman directly and bypasses the patch. + if await maybe_sync_spoolman_locations(db, client=client): + await db.commit() + async with _translate_spoolman_errors(): spools = await client.get_all_spools(allow_archived=include_archived) @@ -451,6 +466,7 @@ async def list_spools( for m in mapped: m["k_profiles"] = kp_by_spool.get(m["id"], []) + await enrich_spool_dicts_with_location_id(db, mapped) return mapped @@ -472,6 +488,7 @@ async def get_spool( kp_result = await db.execute(select(SpoolmanKProfile).where(SpoolmanKProfile.spoolman_spool_id == spool_id)) mapped["k_profiles"] = [_k_profile_to_dict(kp) for kp in kp_result.scalars().all()] + await enrich_spool_dicts_with_location_id(db, [mapped]) return mapped @@ -507,6 +524,18 @@ async def create_spool( client = await _get_client(db) filament_id = await _resolve_filament_id(data, client) + storage_location = data.storage_location + if "location_id" in data.model_fields_set or "storage_location" in data.model_fields_set: + try: + storage_location, _ = await resolve_spoolman_location_string( + db, + location_id=data.location_id, + storage_location=data.storage_location, + fields_set=set(data.model_fields_set), + ) + except ValueError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc + remaining = max(0.0, data.label_weight - data.weight_used) try: async with _translate_spoolman_errors(): @@ -514,7 +543,7 @@ async def create_spool( filament_id=filament_id, remaining_weight=remaining, comment=data.note or None, - location=data.storage_location or None, + location=storage_location or None, ) except HTTPException as exc: if exc.status_code == 404 and data.spoolman_filament_id is not None: @@ -556,6 +585,7 @@ async def create_spool( ) result = _map_spoolman_spool(spool) + await ws_manager.broadcast({"type": "inventory_changed"}) if price_warnings: return JSONResponse(status_code=207, content={**result, "warnings": price_warnings}) return result @@ -581,6 +611,18 @@ async def bulk_create_spools( ) from exc raise + storage_location = data.storage_location + if "location_id" in data.model_fields_set or "storage_location" in data.model_fields_set: + try: + storage_location, _ = await resolve_spoolman_location_string( + db, + location_id=data.location_id, + storage_location=data.storage_location, + fields_set=set(data.model_fields_set), + ) + except ValueError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc + remaining = max(0.0, data.label_weight - data.weight_used) created: list[dict] = [] failures: list[str] = [] @@ -590,7 +632,7 @@ async def bulk_create_spools( filament_id=filament_id, remaining_weight=remaining, comment=data.note or None, - location=data.storage_location or None, + location=storage_location or None, ) except (SpoolmanUnavailableError, SpoolmanClientError, SpoolmanNotFoundError) as exc: logger.warning("Bulk spool creation: one spool failed: %s", exc) @@ -613,6 +655,8 @@ async def bulk_create_spools( if not created: raise HTTPException(status_code=500, detail="Failed to create any spools in Spoolman") + await ws_manager.broadcast({"type": "inventory_changed"}) + if len(created) < payload.quantity: # Some spool creations failed — return 207 Multi-Status so the caller # can distinguish a full success from a partial one and show a useful message. @@ -679,8 +723,18 @@ async def update_spool( synthetic_used = float(current.get("used_weight") or 0) weight_used = data.weight_used if data.weight_used is not None else synthetic_used note = data.note if data.note is not None else current.get("comment") - storage_location_changed = "storage_location" in data.model_fields_set - storage_location = data.storage_location if storage_location_changed else None + storage_location_changed = "storage_location" in data.model_fields_set or "location_id" in data.model_fields_set + storage_location = data.storage_location if "storage_location" in data.model_fields_set else None + if storage_location_changed: + try: + storage_location, _ = await resolve_spoolman_location_string( + db, + location_id=data.location_id, + storage_location=storage_location, + fields_set=set(data.model_fields_set), + ) + except ValueError as exc: + raise HTTPException(status_code=400, detail=str(exc)) from exc color_hex = rgba[:6] @@ -817,6 +871,7 @@ async def update_spool( async with _translate_spoolman_errors(): updated = await client.merge_spool_extra(spool_id, new_extra) + await ws_manager.broadcast({"type": "inventory_changed"}) return _map_spoolman_spool(updated) @@ -830,6 +885,7 @@ async def delete_spool( client = await _get_client(db) async with _translate_spoolman_errors(): await client.delete_spool(spool_id) + await ws_manager.broadcast({"type": "inventory_changed"}) return {"status": "deleted"} @@ -844,10 +900,12 @@ async def archive_spool( async with _translate_spoolman_errors(): spool = await client.set_spool_archived(spool_id, archived=True) try: - return _map_spoolman_spool(spool) + mapped = _map_spoolman_spool(spool) except ValueError as exc: logger.warning("Malformed Spoolman spool (id=%r): %s", spool_id, exc) raise HTTPException(status_code=502, detail="Spoolman returned malformed spool data") from exc + await ws_manager.broadcast({"type": "inventory_changed"}) + return mapped @router.post("/spools/{spool_id}/restore") @@ -861,10 +919,12 @@ async def restore_spool( async with _translate_spoolman_errors(): spool = await client.set_spool_archived(spool_id, archived=False) try: - return _map_spoolman_spool(spool) + mapped = _map_spoolman_spool(spool) except ValueError as exc: logger.warning("Malformed Spoolman spool (id=%r): %s", spool_id, exc) raise HTTPException(status_code=502, detail="Spoolman returned malformed spool data") from exc + await ws_manager.broadcast({"type": "inventory_changed"}) + return mapped @router.post("/spools/{spool_id}/reset-consumed-counter") @@ -888,10 +948,12 @@ async def reset_spool_consumed_counter( async with _translate_spoolman_errors(): spool = await client.reset_spool_usage(spool_id) try: - return _map_spoolman_spool(spool) + mapped = _map_spoolman_spool(spool) except ValueError as exc: logger.warning("Malformed Spoolman spool (id=%r): %s", spool_id, exc) raise HTTPException(status_code=502, detail="Spoolman returned malformed spool data") from exc + await ws_manager.broadcast({"type": "inventory_changed"}) + return mapped @router.post("/spools/reset-consumed-counter-bulk") @@ -922,6 +984,8 @@ async def bulk_reset_spool_consumed_counter( reset_count += 1 except HTTPException as exc: logger.warning("Spoolman reset-consumed-counter failed for spool %s: %s", spool_id, exc.detail) + if reset_count: + await ws_manager.broadcast({"type": "inventory_changed"}) return {"reset": reset_count} @@ -955,6 +1019,7 @@ async def sync_spool_weight( upd_filament = updated.get("filament") or {} label_weight = _safe_int(upd_filament.get("weight"), 1000) weight_used = max(0.0, label_weight - remaining) + await ws_manager.broadcast({"type": "inventory_changed"}) return {"status": "ok", "weight_used": weight_used} @@ -997,6 +1062,7 @@ async def link_tag_to_spoolman_spool( updated = await client.update_spool_full(spool_id=spool_id, extra=cur_extra) logger.info("Linked tag %s to Spoolman spool %s", tag, spool_id) + await ws_manager.broadcast({"type": "inventory_changed"}) return _map_spoolman_spool(updated) diff --git a/backend/app/core/database.py b/backend/app/core/database.py index 857baa359..098890c3b 100644 --- a/backend/app/core/database.py +++ b/backend/app/core/database.py @@ -181,6 +181,7 @@ async def init_db(): kprofile_note, library, local_preset, + location, long_lived_token, maintenance, notification, @@ -2938,6 +2939,110 @@ async def run_migrations(conn): ) ) + # Migration: structured storage locations (#1004). Flat catalog of physical + # shelves/drawers; spool.location_id FK with storage_location kept denormalized. + await _safe_execute( + conn, + """ + CREATE TABLE IF NOT EXISTS locations ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name VARCHAR(255) NOT NULL UNIQUE, + name_key VARCHAR(255), + identifier VARCHAR(100), + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP + ) + """ + if is_sqlite() + else """ + CREATE TABLE IF NOT EXISTS locations ( + id SERIAL PRIMARY KEY, + name VARCHAR(255) NOT NULL UNIQUE, + name_key VARCHAR(255), + identifier VARCHAR(100), + created_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP + ) + """, + ) + await _safe_execute(conn, "ALTER TABLE locations ADD COLUMN name_key VARCHAR(255)") + await _safe_execute(conn, "CREATE UNIQUE INDEX IF NOT EXISTS ix_locations_name_key ON locations (name_key)") + await _safe_execute(conn, "ALTER TABLE spool ADD COLUMN location_id INTEGER REFERENCES locations(id)") + await _safe_execute(conn, "CREATE INDEX IF NOT EXISTS ix_spool_location_id ON spool (location_id)") + + # Backfill name_key on legacy rows FIRST. If a pre-existing locations + # row was manually inserted before this migration ran, its name_key is + # NULL. The dedup INSERT below would then be silently skipped by + # UNIQUE(name) (legacy row already has the name), AND the spool-link + # UPDATE that joins on name_key would miss it. Doing this backfill BEFORE + # the INSERT keeps the join consistent on both branches of the migration. + async with conn.begin_nested(): + await conn.execute( + text( + """ + UPDATE locations + SET name_key = LOWER(TRIM(name)) + WHERE name_key IS NULL OR TRIM(name_key) = '' + """ + ) + ) + + # Backfill locations from existing free-text storage_location values. + # GROUP BY name_key so case variants ("Drybox 1" / "DRYBOX 1") collapse to + # one row; INSERT OR IGNORE / ON CONFLICT keeps the migration idempotent. + _location_backfill_sql = ( + """ + INSERT OR IGNORE INTO locations (name, name_key, created_at, updated_at) + SELECT MIN(TRIM(storage_location)), LOWER(TRIM(storage_location)), CURRENT_TIMESTAMP, CURRENT_TIMESTAMP + FROM spool + WHERE TRIM(COALESCE(storage_location, '')) != '' + GROUP BY LOWER(TRIM(storage_location)) + """ + if is_sqlite() + else """ + INSERT INTO locations (name, name_key, created_at, updated_at) + SELECT MIN(TRIM(storage_location)), LOWER(TRIM(storage_location)), CURRENT_TIMESTAMP, CURRENT_TIMESTAMP + FROM spool + WHERE TRIM(COALESCE(storage_location, '')) != '' + GROUP BY LOWER(TRIM(storage_location)) + ON CONFLICT (name_key) DO NOTHING + """ + ) + async with conn.begin_nested(): + await conn.execute(text(_location_backfill_sql)) + await conn.execute( + text( + """ + UPDATE spool + SET location_id = ( + SELECT l.id FROM locations l + WHERE l.name_key = LOWER(TRIM(spool.storage_location)) + LIMIT 1 + ) + WHERE TRIM(COALESCE(storage_location, '')) != '' + AND location_id IS NULL + """ + ) + ) + + # Sanity check: any spools that still have a free-text storage_location + # but no location_id link mean a row slipped through the dedup INSERT + # (most likely a pre-existing manually-inserted locations row with a + # hostile name shape that the UNIQUE(name) check tripped on). Surface + # the count so ops can investigate — the user won't see those spools in + # location-filtered queries until they're manually linked or re-saved. + orphan_count_row = await conn.execute( + text("SELECT COUNT(*) FROM spool WHERE TRIM(COALESCE(storage_location, '')) != '' AND location_id IS NULL") + ) + orphan_count = orphan_count_row.scalar() or 0 + if orphan_count: + logger.warning( + "Storage-location migration left %d spool(s) with free-text storage_location " + "but no location_id link. Re-save those spools or merge the orphaned location " + "names manually.", + orphan_count, + ) + async def seed_notification_templates(): """Seed default notification templates if they don't exist.""" diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index 9839776d3..2ea5efb99 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -10,6 +10,7 @@ from backend.app.models.group import Group, user_groups from backend.app.models.kprofile_note import KProfileNote from backend.app.models.library import LibraryFile, LibraryFolder from backend.app.models.local_preset import LocalPreset +from backend.app.models.location import Location from backend.app.models.long_lived_token import LongLivedToken from backend.app.models.maintenance import MaintenanceHistory, MaintenanceType, PrinterMaintenance from backend.app.models.notification import NotificationLog @@ -55,6 +56,7 @@ __all__ = [ "PrintBatch", "LibraryFolder", "LibraryFile", + "Location", "User", "Group", "user_groups", diff --git a/backend/app/models/location.py b/backend/app/models/location.py new file mode 100644 index 000000000..b0673321d --- /dev/null +++ b/backend/app/models/location.py @@ -0,0 +1,27 @@ +from datetime import datetime +from typing import TYPE_CHECKING + +from sqlalchemy import DateTime, String, func +from sqlalchemy.orm import Mapped, mapped_column, relationship + +from backend.app.core.database import Base + +if TYPE_CHECKING: + from backend.app.models.spool import Spool + + +class Location(Base): + """Physical storage location for filament spools (shelf, drawer, drybox, etc.).""" + + __tablename__ = "locations" + + id: Mapped[int] = mapped_column(primary_key=True) + name: Mapped[str] = mapped_column(String(255), nullable=False, unique=True) + # Case-insensitive uniqueness — LOWER(TRIM(name)); enforced via migration index. + name_key: Mapped[str] = mapped_column(String(255), nullable=False, unique=True, index=True) + # Reserved for Phase 3 RFID shelf tags — unused in Phase 1. + identifier: Mapped[str | None] = mapped_column(String(100)) + created_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now()) + updated_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now(), onupdate=func.now()) + + spools: Mapped[list["Spool"]] = relationship(back_populates="location") diff --git a/backend/app/models/spool.py b/backend/app/models/spool.py index edaa9ef48..3fde2b9ba 100644 --- a/backend/app/models/spool.py +++ b/backend/app/models/spool.py @@ -1,6 +1,6 @@ from datetime import datetime -from sqlalchemy import Boolean, DateTime, Float, Integer, String, func +from sqlalchemy import Boolean, DateTime, Float, ForeignKey, Integer, String, func from sqlalchemy.orm import Mapped, mapped_column, relationship from backend.app.core.database import Base @@ -61,6 +61,7 @@ class Spool(Base): cost_per_kg: Mapped[float | None] = mapped_column(Float) # Cost per kilogram storage_location: Mapped[str | None] = mapped_column(String(255)) # User-editable storage location + location_id: Mapped[int | None] = mapped_column(ForeignKey("locations.id"), index=True) last_used: Mapped[datetime | None] = mapped_column(DateTime) # Last time this spool was used in a print encode_time: Mapped[datetime | None] = mapped_column(DateTime) # When spool was encoded/written to tag @@ -74,7 +75,9 @@ class Spool(Base): k_profiles: Mapped[list["SpoolKProfile"]] = relationship(back_populates="spool", cascade="all, delete-orphan") assignments: Mapped[list["SpoolAssignment"]] = relationship(back_populates="spool", cascade="all, delete-orphan") + location: Mapped["Location | None"] = relationship(back_populates="spools") +from backend.app.models.location import Location # noqa: E402 from backend.app.models.spool_assignment import SpoolAssignment # noqa: E402 from backend.app.models.spool_k_profile import SpoolKProfile # noqa: E402 diff --git a/backend/app/schemas/location.py b/backend/app/schemas/location.py new file mode 100644 index 000000000..66bbd49fb --- /dev/null +++ b/backend/app/schemas/location.py @@ -0,0 +1,39 @@ +from datetime import datetime + +from pydantic import BaseModel, Field, field_validator + +from backend.app.services.location_service import normalize_location_name + + +class LocationCreate(BaseModel): + name: str = Field(..., min_length=1, max_length=255) + identifier: str | None = Field(default=None, max_length=100) + + @field_validator("name") + @classmethod + def validate_name(cls, v: str) -> str: + return normalize_location_name(v) + + +class LocationUpdate(BaseModel): + name: str | None = Field(default=None, min_length=1, max_length=255) + identifier: str | None = Field(default=None, max_length=100) + + @field_validator("name") + @classmethod + def validate_name(cls, v: str | None) -> str | None: + if v is None: + return None + return normalize_location_name(v) + + +class LocationResponse(BaseModel): + id: int + name: str + identifier: str | None = None + spool_count: int = 0 + created_at: datetime + updated_at: datetime + + class Config: + from_attributes = True diff --git a/backend/app/schemas/printer.py b/backend/app/schemas/printer.py index f8cbecccd..44b0ab3f6 100644 --- a/backend/app/schemas/printer.py +++ b/backend/app/schemas/printer.py @@ -29,7 +29,6 @@ class PrinterBase(BaseModel): max_length=253, pattern=r"^(\d{1,3}(\.\d{1,3}){3}|[a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?(\.[a-zA-Z0-9]([a-zA-Z0-9\-]{0,61}[a-zA-Z0-9])?)*)$", ) - access_code: str = Field(..., min_length=1, max_length=20) model: str | None = None location: str | None = None # Group/location name auto_archive: bool = True @@ -41,7 +40,10 @@ class PrinterBase(BaseModel): class PrinterCreate(PrinterBase): - pass + # access_code lives on the input shapes only — never on the default + # PrinterResponse. Direct exposure on PRINTERS_READ would let a Viewer + # connect to the printer's MQTT and bypass Bambuddy's RBAC. + access_code: str = Field(..., min_length=1, max_length=20) class PlateDetectionROI(BaseModel): @@ -101,7 +103,6 @@ class PrinterResponse(PrinterBase): "name": printer.name, "serial_number": printer.serial_number, "ip_address": printer.ip_address, - "access_code": printer.access_code, "model": printer.model, "location": printer.location, "auto_archive": printer.auto_archive, @@ -135,6 +136,18 @@ class PrinterResponse(PrinterBase): return cls(**data) +class PrinterResponseWithSecret(PrinterResponse): + """PrinterResponse + access_code. Returned ONLY to callers with + PRINTERS_UPDATE (Admin / Operator JWTs, or single-trust auth-disabled mode). + + Viewers and API keys never receive this shape — they get the bare + PrinterResponse without access_code, since holding the access_code lets + the caller talk to the printer's MQTT directly and bypass Bambuddy's RBAC. + """ + + access_code: str + + class HMSErrorResponse(BaseModel): code: str attr: int = 0 # Attribute value for constructing wiki URL diff --git a/backend/app/schemas/settings.py b/backend/app/schemas/settings.py index 319363b0b..72fff4818 100644 --- a/backend/app/schemas/settings.py +++ b/backend/app/schemas/settings.py @@ -533,6 +533,11 @@ class AppSettingsUpdate(BaseModel): raise ValueError("default_sidebar_order must be valid JSON or empty") if isinstance(parsed, dict): order = parsed.get("order") + hidden_system_item_ids = parsed.get("hiddenSystemItemIds", []) + if not isinstance(hidden_system_item_ids, list) or not all( + isinstance(item, str) for item in hidden_system_item_ids + ): + raise ValueError("sidebar hidden system item IDs must be an array of strings") elif isinstance(parsed, list): order = parsed else: diff --git a/backend/app/schemas/spool.py b/backend/app/schemas/spool.py index 3d5a90e68..9a2a7c930 100644 --- a/backend/app/schemas/spool.py +++ b/backend/app/schemas/spool.py @@ -125,6 +125,7 @@ class SpoolBase(BaseModel): # assignment). Column has lived on the ORM since the inventory rework # but was missing from this schema, so writes were silently dropped (#1291). storage_location: str | None = Field(default=None, max_length=255) + location_id: int | None = Field(default=None, gt=0) class SpoolCreate(SpoolBase): @@ -174,6 +175,7 @@ class SpoolUpdate(BaseModel): category: str | None = Field(default=None, max_length=50) low_stock_threshold_pct: int | None = Field(default=None, ge=1, le=99) storage_location: str | None = Field(default=None, max_length=255) + location_id: int | None = Field(default=None, gt=0) class SpoolKProfileBase(BaseModel): diff --git a/backend/app/services/location_service.py b/backend/app/services/location_service.py new file mode 100644 index 000000000..97c862d10 --- /dev/null +++ b/backend/app/services/location_service.py @@ -0,0 +1,354 @@ +"""Storage location catalog — single write path for spool location fields (#1004).""" + +from __future__ import annotations + +import logging +import time +from dataclasses import dataclass + +import httpx +from sqlalchemy import func, select, update +from sqlalchemy.exc import IntegrityError +from sqlalchemy.ext.asyncio import AsyncSession + +from backend.app.models.location import Location +from backend.app.models.spool import Spool + +logger = logging.getLogger(__name__) + +DUPLICATE_LOCATION_NAME = "A location with this name already exists" + + +def normalize_location_name(name: str) -> str: + trimmed = name.strip() + if not trimmed: + raise ValueError("name must not be empty") + return trimmed + + +def location_name_key(name: str) -> str: + """Case-insensitive lookup key stored on Location.name_key.""" + return normalize_location_name(name).lower() + + +def assign_location_name(location: Location, name: str) -> None: + normalized = normalize_location_name(name) + location.name = normalized + location.name_key = location_name_key(normalized) + + +@dataclass(frozen=True) +class SpoolLocationFields: + """Canonical spool location state: FK + denormalized string for Spoolman/display.""" + + location_id: int | None + storage_location: str | None + + +async def get_location_by_id(db: AsyncSession, location_id: int) -> Location | None: + result = await db.execute(select(Location).where(Location.id == location_id)) + return result.scalar_one_or_none() + + +async def get_location_by_name(db: AsyncSession, name: str) -> Location | None: + key = location_name_key(name) + result = await db.execute(select(Location).where(Location.name_key == key)) + return result.scalar_one_or_none() + + +async def get_locations_by_name_keys(db: AsyncSession, keys: set[str]) -> dict[str, Location]: + if not keys: + return {} + result = await db.execute(select(Location).where(Location.name_key.in_(keys))) + return {loc.name_key: loc for loc in result.scalars().all()} + + +async def _create_location_or_get_existing(db: AsyncSession, normalized: str) -> Location: + """Insert a location row, returning the winner on concurrent name_key collision.""" + existing = await get_location_by_name(db, normalized) + if existing: + return existing + location = Location() + assign_location_name(location, normalized) + try: + async with db.begin_nested(): + db.add(location) + await db.flush() + return location + except IntegrityError as exc: + winner = await get_location_by_name(db, normalized) + if winner: + return winner + raise ValueError(DUPLICATE_LOCATION_NAME) from exc + + +async def _insert_location_if_absent(db: AsyncSession, name: str) -> bool: + """Stage a new location row when absent. Returns True when one was added.""" + normalized = normalize_location_name(name) + if await get_location_by_name(db, normalized): + return False + location = Location() + assign_location_name(location, normalized) + try: + async with db.begin_nested(): + db.add(location) + await db.flush() + return True + except IntegrityError: + # Race: another writer inserted the same name between our check and + # flush. The row already exists by definition — surface as "not added" + # rather than re-raising. Anything else (NULL constraint, FK, check + # constraint) would be a programming bug — re-fetch to verify so we + # don't silently drop unrelated IntegrityErrors. + if await get_location_by_name(db, normalized): + return False + logger.warning("IntegrityError on insert of location %r without surviving row", normalized) + raise + + +async def resolve_location_by_name(db: AsyncSession, name: str, *, create: bool = True) -> Location | None: + """Find a location by name (case-insensitive), optionally creating it.""" + normalized = normalize_location_name(name) + existing = await get_location_by_name(db, normalized) + if existing: + return existing + if not create: + return None + return await _create_location_or_get_existing(db, normalized) + + +async def resolve_spool_location_fields( + db: AsyncSession, + *, + location_id: int | None = None, + storage_location: str | None = None, + fields_set: set[str], +) -> SpoolLocationFields | None: + """Resolve location_id + storage_location from API input. + + ``location_id`` wins when both fields appear in ``fields_set``. + Returns ``None`` when neither location field was provided. + """ + if "location_id" in fields_set: + if location_id is None: + return SpoolLocationFields(location_id=None, storage_location=None) + loc = await get_location_by_id(db, location_id) + if not loc: + raise ValueError(f"Location {location_id} not found") + return SpoolLocationFields(location_id=loc.id, storage_location=loc.name) + + if "storage_location" in fields_set: + if not storage_location: + return SpoolLocationFields(location_id=None, storage_location=None) + loc = await resolve_location_by_name(db, storage_location) + if not loc: + return SpoolLocationFields(location_id=None, storage_location=None) + return SpoolLocationFields(location_id=loc.id, storage_location=loc.name) + + return None + + +async def prepare_internal_spool_payload(db: AsyncSession, data: dict, fields_set: set[str]) -> dict: + """Apply resolved location fields before creating or updating an internal spool.""" + payload = dict(data) + resolved = await resolve_spool_location_fields( + db, + location_id=payload.get("location_id"), + storage_location=payload.get("storage_location"), + fields_set=fields_set, + ) + if resolved is not None: + payload["location_id"] = resolved.location_id + payload["storage_location"] = resolved.storage_location + return payload + + +async def resolve_spoolman_location_string( + db: AsyncSession, + *, + location_id: int | None = None, + storage_location: str | None = None, + fields_set: set[str], +) -> tuple[str | None, bool]: + """Return (Spoolman location string, changed) for proxy writes.""" + resolved = await resolve_spool_location_fields( + db, + location_id=location_id, + storage_location=storage_location, + fields_set=fields_set, + ) + if resolved is None: + return None, False + return resolved.storage_location, True + + +async def count_internal_spools_at_location(db: AsyncSession, location_id: int) -> int: + result = await db.execute( + select(func.count()) + .select_from(Spool) + .where( + Spool.location_id == location_id, + Spool.archived_at.is_(None), + ) + ) + return int(result.scalar() or 0) + + +async def count_spools_at_location_by_name(db: AsyncSession, name: str) -> int: + normalized = name.strip() + if not normalized: + return 0 + result = await db.execute( + select(func.count()) + .select_from(Spool) + .where( + Spool.archived_at.is_(None), + func.lower(func.trim(Spool.storage_location)) == normalized.lower(), + ) + ) + return int(result.scalar() or 0) + + +async def enrich_spool_dicts_with_location_id(db: AsyncSession, spools: list[dict]) -> None: + """Attach location_id to mapped Spoolman-style spool dicts in place.""" + keys = {location_name_key(s["storage_location"]) for s in spools if (s.get("storage_location") or "").strip()} + if not keys: + for s in spools: + s["location_id"] = None + return + + by_key = await get_locations_by_name_keys(db, keys) + for s in spools: + raw = (s.get("storage_location") or "").strip() + if not raw: + s["location_id"] = None + continue + loc = by_key.get(location_name_key(raw)) + s["location_id"] = loc.id if loc else None + + +async def rename_location(db: AsyncSession, location: Location, new_name: str) -> Location: + normalized = normalize_location_name(new_name) + existing = await get_location_by_name(db, normalized) + if existing and existing.id != location.id: + raise ValueError(DUPLICATE_LOCATION_NAME) + + old_name = location.name + # Mirror the SQL TRIM on the Python side so a legacy row whose + # `storage_location` has trailing whitespace still matches against the + # `old_name` we just lifted off the Location row. Without `.strip()` the + # equality is asymmetric (SQL strips the column; Python doesn't) and + # legacy rows quietly fall out of the rename cascade. + old_name_key = old_name.strip().lower() + assign_location_name(location, normalized) + await db.execute(update(Spool).where(Spool.location_id == location.id).values(storage_location=normalized)) + # Keep legacy rows in sync when only storage_location was set. + await db.execute( + update(Spool) + .where( + Spool.location_id.is_(None), + func.lower(func.trim(Spool.storage_location)) == old_name_key, + ) + .values(storage_location=normalized, location_id=location.id) + ) + try: + await db.flush() + except IntegrityError as exc: + raise ValueError(DUPLICATE_LOCATION_NAME) from exc + return location + + +async def sync_locations_from_spoolman(db: AsyncSession, client) -> bool: + """Import distinct Spoolman location strings into the local catalog. + + Returns True when new rows were staged (caller must commit). Logs and + returns False on Spoolman fetch failures so the calling read path keeps + serving the local catalog instead of 500ing; bare-Exception swallow used + to be the shape here and hid both transport errors and shape regressions. + """ + from backend.app.services.spoolman import SpoolmanClientError, SpoolmanUnavailableError + + try: + names = await client.get_distinct_locations() + except (SpoolmanUnavailableError, SpoolmanClientError, httpx.HTTPError) as exc: + logger.warning("location sync from Spoolman failed: %s", exc) + return False + + # Collapse case variants before insert — Spoolman may return both + # "Drybox 1" and "DRYBOX 1" in the same payload. + by_key: dict[str, str] = {} + for raw in names: + name = (raw or "").strip() + if not name: + continue + key = location_name_key(name) + if key not in by_key: + by_key[key] = name + + changed = False + for name in by_key.values(): + if await _insert_location_if_absent(db, name): + changed = True + return changed + + +# Per-URL last-sync timestamp guard. Calling list_spools runs the sync, so on +# a polling UI without this guard every refetch round-trips to Spoolman and +# opens a write transaction — measurable latency and SQLite write contention. +# 60s is long enough to absorb dashboard polling, short enough that a manual +# spool rename in Spoolman shows up on the next minute's refresh. +_SPOOLMAN_LOCATION_SYNC_TTL_SECONDS = 60.0 +_spoolman_location_sync_last_run: dict[str, float] = {} + + +def _spoolman_location_sync_cache_clear() -> None: + """Test hook: drop the TTL cache so each test starts from a clean slate.""" + _spoolman_location_sync_last_run.clear() + + +async def maybe_sync_spoolman_locations(db: AsyncSession, *, client=None) -> bool: + """Sync Spoolman location names into the local catalog when integration is enabled. + + Pass ``client`` when the caller has already resolved one (the GET /spools + route does); otherwise the function falls back to ``init_spoolman_client``. + Passing the route's client keeps test fixtures honest — without it, the + fall-back path imports from ``backend.app.services.spoolman`` directly and + bypasses any patch that targets the route module's alias, which causes + real TCP connects to whatever ``spoolman_url`` happens to point at. + """ + from backend.app.api.routes._spoolman_helpers import assert_safe_spoolman_url + from backend.app.models.settings import Settings + + result = await db.execute(select(Settings)) + settings = {s.key: s.value for s in result.scalars().all()} + if settings.get("spoolman_enabled", "false").lower() != "true": + return False + url = settings.get("spoolman_url", "").strip() + if not url: + return False + + # Debounce: skip the round-trip when we synced this URL recently. + cache_key = url.rstrip("/") + last_run = _spoolman_location_sync_last_run.get(cache_key, 0.0) + now = time.monotonic() + if now - last_run < _SPOOLMAN_LOCATION_SYNC_TTL_SECONDS: + return False + + try: + assert_safe_spoolman_url(url) + except ValueError as exc: + logger.warning("Spoolman URL rejected by SSRF guard during location sync: %s", exc) + return False + + if client is None: + from backend.app.services.spoolman import get_spoolman_client, init_spoolman_client + + client = await get_spoolman_client() + if not client or client.base_url != cache_key: + client = await init_spoolman_client(url) + if not client: + return False + + changed = await sync_locations_from_spoolman(db, client) + _spoolman_location_sync_last_run[cache_key] = now + return changed diff --git a/backend/app/services/spoolman.py b/backend/app/services/spoolman.py index d0b7cad3f..90d151abd 100644 --- a/backend/app/services/spoolman.py +++ b/backend/app/services/spoolman.py @@ -544,6 +544,86 @@ class SpoolmanClient: params["allow_archived"] = "true" return await self._get_with_retry("/spool", params=params or None) + async def get_distinct_locations(self) -> list[str]: + """Return distinct location strings currently assigned to Spoolman spools. + + Spoolman's `/location` endpoint shape varies across versions: older + releases return `list[str]`, newer ones return `list[dict]` with a + `name` field. Normalize to `list[str]` so callers can iterate without + runtime shape checks. + """ + raw = await self._get_with_retry("/location") + if not isinstance(raw, list): + return [] + names: list[str] = [] + for entry in raw: + if isinstance(entry, str): + names.append(entry) + elif isinstance(entry, dict): + name = entry.get("name") + if isinstance(name, str): + names.append(name) + return names + + async def rename_location(self, current_name: str, new_name: str) -> int: + """Bulk-rename a location string on all Spoolman spools. + + Tries the bulk `PATCH /location/{name}` endpoint first. Spoolman + versions older than ~0.16 don't expose it and respond 404/405 — in + that case fall back to iterating every spool currently at + ``current_name`` and PATCHing each one's ``location`` field directly. + Returns the number of spools renamed (or 0 if the bulk endpoint + succeeded without enumerating). + """ + from urllib.parse import quote + + encoded = quote(current_name, safe="") + client = await self._get_client() + try: + response = await client.patch( + f"{self.api_url}/location/{encoded}", + json={"name": new_name}, + ) + response.raise_for_status() + return 0 + except httpx.HTTPStatusError as exc: + if exc.response.status_code not in (404, 405): + raise + logger.info( + "Spoolman bulk-rename endpoint unavailable (status %d); falling back to per-spool PATCH", + exc.response.status_code, + ) + + # Per-spool fallback: enumerate every spool currently at the old name + # and PATCH each. Keep going on individual failures so a single + # already-deleted spool doesn't strand the rest at the old name — + # collect errors and re-raise as a single SpoolmanClientError if any + # leftover survives. + spools = await self.get_all_spools(allow_archived=True) + renamed = 0 + failures: list[str] = [] + for spool in spools: + if (spool.get("location") or "").strip() != current_name: + continue + try: + await self._request_spool( + "PATCH", + spool["id"], + json_body={"location": new_name}, + operation="rename-location", + ) + renamed += 1 + except SpoolmanNotFoundError: + continue + except Exception as exc: # noqa: BLE001 — accumulate and re-raise below + failures.append(f"spool {spool.get('id')}: {exc}") + if failures: + raise SpoolmanClientError( + f"Spoolman rename fallback failed for {len(failures)} spool(s): {'; '.join(failures[:3])}", + status_code=502, + ) + return renamed + async def delete_spool(self, spool_id: int) -> None: """Delete a spool from Spoolman.""" await self._request_spool("DELETE", spool_id, operation="delete") diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 6c48bd30c..9e31a8f53 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -81,6 +81,20 @@ def mfa_encryption_isolation(monkeypatch, tmp_path): enc_mod._key_source = None +@pytest.fixture(autouse=True) +def reset_spoolman_location_sync_cache(): + """Drop the per-URL Spoolman location-sync TTL cache between tests. + + Without this, a test that runs the sync against `http://localhost:7912` + will skip the sync in any later test that uses the same URL within 60 + real seconds — test ordering would then leak assertions across runs.""" + from backend.app.services.location_service import _spoolman_location_sync_cache_clear + + _spoolman_location_sync_cache_clear() + yield + _spoolman_location_sync_cache_clear() + + @pytest.fixture(scope="session") def event_loop(): """Create an instance of the default event loop for each test session.""" diff --git a/backend/tests/integration/test_auth_apikey_rbac.py b/backend/tests/integration/test_auth_apikey_rbac.py index b597c38cc..4f84e5f95 100644 --- a/backend/tests/integration/test_auth_apikey_rbac.py +++ b/backend/tests/integration/test_auth_apikey_rbac.py @@ -96,6 +96,7 @@ class TestApiKeyRbacAllowed: mock_client.base_url = "http://localhost:7912" mock_client.health_check = AsyncMock(return_value=True) mock_client.get_all_spools = AsyncMock(return_value=[]) + mock_client.get_distinct_locations = AsyncMock(return_value=[]) with patch( "backend.app.api.routes.spoolman_inventory._get_client", AsyncMock(return_value=mock_client), diff --git a/backend/tests/integration/test_locations_api.py b/backend/tests/integration/test_locations_api.py new file mode 100644 index 000000000..12dddf46f --- /dev/null +++ b/backend/tests/integration/test_locations_api.py @@ -0,0 +1,174 @@ +"""Integration tests for /inventory/locations (#1004).""" + +import pytest +from httpx import AsyncClient +from sqlalchemy.ext.asyncio import AsyncSession + +from backend.app.models.location import Location +from backend.app.services.location_service import assign_location_name + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_locations_crud_and_spool_link(async_client: AsyncClient, db_session: AsyncSession): + create_resp = await async_client.post("/api/v1/inventory/locations", json={"name": "Shelf A"}) + assert create_resp.status_code == 201 + loc = create_resp.json() + assert loc["name"] == "Shelf A" + assert loc["spool_count"] == 0 + + dup_resp = await async_client.post("/api/v1/inventory/locations", json={"name": "shelf a"}) + assert dup_resp.status_code == 409 + + spool_resp = await async_client.post( + "/api/v1/inventory/spools", + json={"material": "PLA", "location_id": loc["id"]}, + ) + assert spool_resp.status_code == 200 + spool = spool_resp.json() + assert spool["location_id"] == loc["id"] + assert spool["storage_location"] == "Shelf A" + + list_resp = await async_client.get("/api/v1/inventory/locations") + assert list_resp.status_code == 200 + listed = {item["id"]: item for item in list_resp.json()} + assert listed[loc["id"]]["spool_count"] == 1 + + delete_resp = await async_client.delete(f"/api/v1/inventory/locations/{loc['id']}") + assert delete_resp.status_code == 409 + + clear_resp = await async_client.patch( + f"/api/v1/inventory/spools/{spool['id']}", + json={"location_id": None}, + ) + assert clear_resp.status_code == 200 + + delete_resp2 = await async_client.delete(f"/api/v1/inventory/locations/{loc['id']}") + assert delete_resp2.status_code == 200 + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_rename_location_updates_spool_count(async_client: AsyncClient): + create_resp = await async_client.post("/api/v1/inventory/locations", json={"name": "Old Name"}) + loc = create_resp.json() + + await async_client.post( + "/api/v1/inventory/spools", + json={"material": "PLA", "location_id": loc["id"]}, + ) + + list_before = await async_client.get("/api/v1/inventory/locations") + by_id = {item["id"]: item for item in list_before.json()} + assert by_id[loc["id"]]["spool_count"] == 1 + + rename_resp = await async_client.patch( + f"/api/v1/inventory/locations/{loc['id']}", + json={"name": "New Name"}, + ) + assert rename_resp.status_code == 200 + assert rename_resp.json()["name"] == "New Name" + assert rename_resp.json()["spool_count"] == 1 + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_rename_location_collision_returns_409(async_client: AsyncClient): + first = await async_client.post("/api/v1/inventory/locations", json={"name": "Shelf A"}) + second = await async_client.post("/api/v1/inventory/locations", json={"name": "Shelf B"}) + assert first.status_code == 201 + assert second.status_code == 201 + + collision = await async_client.patch( + f"/api/v1/inventory/locations/{second.json()['id']}", + json={"name": "Shelf A"}, + ) + assert collision.status_code == 409 + assert collision.json()["detail"] == "A location with this name already exists" + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_create_location_duplicate_after_commit_returns_409(async_client: AsyncClient): + """Second create with the same name_key must return 409, not 500.""" + first = await async_client.post("/api/v1/inventory/locations", json={"name": "Race Shelf"}) + second = await async_client.post("/api/v1/inventory/locations", json={"name": "race shelf"}) + assert first.status_code == 201 + assert second.status_code == 409 + assert second.json()["detail"] == "A location with this name already exists" + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_list_locations_is_read_only(async_client: AsyncClient, db_session: AsyncSession): + """GET /locations is a pure read — no catalog rows appear without explicit writes.""" + from sqlalchemy import func, select + + loc = Location() + assign_location_name(loc, "Local Only") + db_session.add(loc) + await db_session.commit() + + before = await db_session.scalar(select(func.count()).select_from(Location)) + resp = await async_client.get("/api/v1/inventory/locations") + after = await db_session.scalar(select(func.count()).select_from(Location)) + + assert resp.status_code == 200 + assert len(resp.json()) == 1 + assert before == after == 1 + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_update_location_404_on_unknown_id(async_client: AsyncClient): + resp = await async_client.patch( + "/api/v1/inventory/locations/99999", + json={"name": "Ghost"}, + ) + assert resp.status_code == 404 + assert resp.json()["detail"] == "Location not found" + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_delete_location_404_on_unknown_id(async_client: AsyncClient): + resp = await async_client.delete("/api/v1/inventory/locations/99999") + assert resp.status_code == 404 + assert resp.json()["detail"] == "Location not found" + + +@pytest.mark.asyncio +@pytest.mark.integration +async def test_locations_routes_require_auth_when_enabled(async_client: AsyncClient): + """All five /locations endpoints must return 401 when auth is enabled and + no credentials are presented. Mirror of the pattern from + test_queue_start_user_attribution._enable_auth_with_admin — required by + project policy: every permission-gated route gets a fail-closed test on + first ship, no follow-ups (the two CVSS 9.8/9.9 advisories shipped from + this exact gap).""" + await async_client.post( + "/api/v1/auth/setup", + json={ + "auth_enabled": True, + "admin_username": "locations1505admin", + "admin_password": "AdminPass1!", + }, + ) + + # GET /locations — read-gated + list_resp = await async_client.get("/api/v1/inventory/locations") + assert list_resp.status_code == 401, list_resp.text + + # POST /locations — write-gated + create_resp = await async_client.post("/api/v1/inventory/locations", json={"name": "Locked"}) + assert create_resp.status_code == 401, create_resp.text + + # PATCH /locations/{id} — write-gated. Use a synthetic id; the auth gate + # runs before the not-found check, so 401 is the correct expectation even + # when the id doesn't exist. + patch_resp = await async_client.patch("/api/v1/inventory/locations/99999", json={"name": "Locked2"}) + assert patch_resp.status_code == 401, patch_resp.text + + # DELETE /locations/{id} — write-gated + delete_resp = await async_client.delete("/api/v1/inventory/locations/99999") + assert delete_resp.status_code == 401, delete_resp.text diff --git a/backend/tests/integration/test_printers_api.py b/backend/tests/integration/test_printers_api.py index 8923e2030..95d358ce2 100644 --- a/backend/tests/integration/test_printers_api.py +++ b/backend/tests/integration/test_printers_api.py @@ -2997,3 +2997,189 @@ class TestConfigureAmsSlotPersistsKProfile: assert response.status_code == 200 # MQTT was indeed called mock_client.extrusion_cali_sel.assert_called_once() + + +class TestPrinterAccessCodeVisibility: + """Regression coverage: GET /printers and GET /printers/{id} must NOT + return ``access_code`` to callers without PRINTERS_UPDATE authority. + + Holding ``access_code`` lets the caller talk to the printer's MQTT + directly with serial+code, bypassing every PRINTERS_CONTROL / + PRINTERS_FILES / PRINTERS_AMS_RFID check Bambuddy enforces. + + Trust matrix encoded here: + - Auth disabled → access_code visible (single-trust mode) + - JWT Admin → access_code visible + - JWT Operator (has *_UPDATE) → access_code visible (VP-card UX) + - JWT Viewer → access_code STRIPPED + - API key with can_read_status → access_code STRIPPED + """ + + @pytest.fixture + async def auth_setup(self, async_client: AsyncClient): + await async_client.post( + "/api/v1/auth/setup", + json={ + "auth_enabled": True, + "admin_username": "pcadmin", + "admin_password": "AdminPass1!", + }, + ) + + async def _login(username, password): + resp = await async_client.post( + "/api/v1/auth/login", + json={"username": username, "password": password}, + ) + return resp.json()["access_token"] + + admin_token = await _login("pcadmin", "AdminPass1!") + + groups = ( + await async_client.get( + "/api/v1/groups/", + headers={"Authorization": f"Bearer {admin_token}"}, + ) + ).json() + operators_group = next(g for g in groups if g["name"] == "Operators") + viewers_group = next(g for g in groups if g["name"] == "Viewers") + + for username, password, group in ( + ("pcoperator", "Operpass1!", operators_group["id"]), + ("pcviewer", "Viewpass1!", viewers_group["id"]), + ): + await async_client.post( + "/api/v1/users/", + headers={"Authorization": f"Bearer {admin_token}"}, + json={"username": username, "password": password, "group_ids": [group]}, + ) + + operator_token = await _login("pcoperator", "Operpass1!") + viewer_token = await _login("pcviewer", "Viewpass1!") + + return { + "admin_token": admin_token, + "operator_token": operator_token, + "viewer_token": viewer_token, + } + + async def _seed_printer_with_known_code(self, async_client: AsyncClient, admin_token: str) -> int: + resp = await async_client.post( + "/api/v1/printers/", + headers={"Authorization": f"Bearer {admin_token}"}, + json={ + "name": "AC-Visibility", + "serial_number": "00M09AVISIBILITY", + "ip_address": "192.168.42.42", + "access_code": "SECRET-CODE", + "is_active": True, + "model": "X1C", + }, + ) + assert resp.status_code == 200, resp.text + return resp.json()["id"] + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_auth_disabled_includes_access_code(self, async_client: AsyncClient, printer_factory): + """Single-trust mode: behaviour preserved, code is visible.""" + printer = await printer_factory(name="AuthOff", access_code="LOCAL-CODE") + + list_resp = await async_client.get("/api/v1/printers/") + detail_resp = await async_client.get(f"/api/v1/printers/{printer.id}") + + assert list_resp.status_code == 200 + assert detail_resp.status_code == 200 + match = next(p for p in list_resp.json() if p["id"] == printer.id) + assert match["access_code"] == "LOCAL-CODE" + assert detail_resp.json()["access_code"] == "LOCAL-CODE" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_admin_jwt_includes_access_code(self, async_client: AsyncClient, auth_setup): + printer_id = await self._seed_printer_with_known_code(async_client, auth_setup["admin_token"]) + headers = {"Authorization": f"Bearer {auth_setup['admin_token']}"} + + list_resp = await async_client.get("/api/v1/printers/", headers=headers) + detail_resp = await async_client.get(f"/api/v1/printers/{printer_id}", headers=headers) + + assert list_resp.status_code == 200 + assert detail_resp.status_code == 200 + match = next(p for p in list_resp.json() if p["id"] == printer_id) + assert match["access_code"] == "SECRET-CODE" + assert detail_resp.json()["access_code"] == "SECRET-CODE" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_operator_jwt_includes_access_code(self, async_client: AsyncClient, auth_setup): + """Operators hold PRINTERS_UPDATE (default role) — the VP-card UX + surfaces the target printer's access_code so they can configure + their slicer. The visibility predicate must keep working for them. + """ + printer_id = await self._seed_printer_with_known_code(async_client, auth_setup["admin_token"]) + headers = {"Authorization": f"Bearer {auth_setup['operator_token']}"} + + list_resp = await async_client.get("/api/v1/printers/", headers=headers) + detail_resp = await async_client.get(f"/api/v1/printers/{printer_id}", headers=headers) + + assert list_resp.status_code == 200 + assert detail_resp.status_code == 200 + match = next(p for p in list_resp.json() if p["id"] == printer_id) + assert match["access_code"] == "SECRET-CODE" + assert detail_resp.json()["access_code"] == "SECRET-CODE" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_viewer_jwt_excludes_access_code(self, async_client: AsyncClient, auth_setup): + """The fix: Viewers hold PRINTERS_READ but not PRINTERS_UPDATE, and + must NOT be able to read the printer's secret. + """ + printer_id = await self._seed_printer_with_known_code(async_client, auth_setup["admin_token"]) + headers = {"Authorization": f"Bearer {auth_setup['viewer_token']}"} + + list_resp = await async_client.get("/api/v1/printers/", headers=headers) + detail_resp = await async_client.get(f"/api/v1/printers/{printer_id}", headers=headers) + + assert list_resp.status_code == 200 + assert detail_resp.status_code == 200 + match = next(p for p in list_resp.json() if p["id"] == printer_id) + # Field absent OR null — both are acceptable (no usable secret reaches the wire). + assert "access_code" not in match or match["access_code"] is None + body = detail_resp.json() + assert "access_code" not in body or body["access_code"] is None + # And the rest of the payload still arrives so the UI keeps working. + assert match["name"] == "AC-Visibility" + assert body["name"] == "AC-Visibility" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_api_key_excludes_access_code(self, async_client: AsyncClient, auth_setup, db_session): + """API keys with can_read_status hold PRINTERS_READ but the predicate + gates on PRINTERS_UPDATE (admin-only / API-key-unmapped). The key + must NOT be able to exfiltrate access_code. + """ + from backend.app.core.auth import generate_api_key + from backend.app.models.api_key import APIKey + + printer_id = await self._seed_printer_with_known_code(async_client, auth_setup["admin_token"]) + + full_key, key_hash, key_prefix = generate_api_key() + api_key = APIKey( + name="visibility-key", + key_hash=key_hash, + key_prefix=key_prefix, + can_read_status=True, + enabled=True, + ) + db_session.add(api_key) + await db_session.commit() + + list_resp = await async_client.get("/api/v1/printers/", headers={"X-API-Key": full_key}) + detail_resp = await async_client.get(f"/api/v1/printers/{printer_id}", headers={"X-API-Key": full_key}) + + assert list_resp.status_code == 200 + assert detail_resp.status_code == 200 + match = next(p for p in list_resp.json() if p["id"] == printer_id) + assert "access_code" not in match or match["access_code"] is None + body = detail_resp.json() + assert "access_code" not in body or body["access_code"] is None diff --git a/backend/tests/integration/test_spoolman_inventory_api.py b/backend/tests/integration/test_spoolman_inventory_api.py index 94b3ba543..fe53bbc58 100644 --- a/backend/tests/integration/test_spoolman_inventory_api.py +++ b/backend/tests/integration/test_spoolman_inventory_api.py @@ -74,6 +74,10 @@ def mock_spoolman_client(): # branch override this on the fly. mock_client.is_filament_shared = AsyncMock(return_value=False) mock_client.ensure_extra_field = AsyncMock(return_value=True) + # list_spools calls maybe_sync_spoolman_locations which invokes + # get_distinct_locations on the route-resolved client. Empty list keeps the + # mock honest without staging phantom catalog rows. + mock_client.get_distinct_locations = AsyncMock(return_value=[]) with ( patch( diff --git a/backend/tests/integration/test_spoolman_k_profiles.py b/backend/tests/integration/test_spoolman_k_profiles.py index d36849e81..a6fe751d6 100644 --- a/backend/tests/integration/test_spoolman_k_profiles.py +++ b/backend/tests/integration/test_spoolman_k_profiles.py @@ -67,6 +67,7 @@ def mock_spoolman_client(): client.health_check = AsyncMock(return_value=True) client.get_spool = AsyncMock(return_value=SAMPLE_SPOOL) client.get_all_spools = AsyncMock(return_value=[SAMPLE_SPOOL]) + client.get_distinct_locations = AsyncMock(return_value=[]) with patch( "backend.app.api.routes.spoolman_inventory._get_client", diff --git a/backend/tests/integration/test_spoolman_slot_assignment_mqtt.py b/backend/tests/integration/test_spoolman_slot_assignment_mqtt.py index 81a7a1d1a..adff200c4 100644 --- a/backend/tests/integration/test_spoolman_slot_assignment_mqtt.py +++ b/backend/tests/integration/test_spoolman_slot_assignment_mqtt.py @@ -69,6 +69,7 @@ def mock_spoolman_client(): # #1457: assign route enumerates spools to clear stale fallback-tag links. client.get_spools = AsyncMock(return_value=[]) client.merge_spool_extra = AsyncMock(return_value={"id": 0, "extra": {}}) + client.get_distinct_locations = AsyncMock(return_value=[]) with patch( "backend.app.api.routes.spoolman_inventory._get_client", diff --git a/backend/tests/unit/test_location_migration.py b/backend/tests/unit/test_location_migration.py new file mode 100644 index 000000000..723ae84a8 --- /dev/null +++ b/backend/tests/unit/test_location_migration.py @@ -0,0 +1,209 @@ +"""Regression tests for storage-location migration backfill (#1004). + +Legacy installs may have free-text storage_location values that differ only +by case. The backfill must collapse them to one catalog row and stay +idempotent across restarts. +""" + +from __future__ import annotations + +import pytest +from sqlalchemy import text +from sqlalchemy.ext.asyncio import create_async_engine + +from backend.app.core.database import run_migrations + + +@pytest.fixture(autouse=True) +def force_sqlite_dialect(monkeypatch): + from backend.app.core import db_dialect + + monkeypatch.setattr(db_dialect, "is_sqlite", lambda: True) + monkeypatch.setattr(db_dialect, "is_postgres", lambda: False) + from backend.app.core import database as database_module + + monkeypatch.setattr(database_module, "is_sqlite", lambda: True) + + +def _register_all_models(): + import backend.app.models # noqa: F401 + from backend.app.models import ( # noqa: F401 + external_link, + location, + print_log, + print_queue, + project_bom, + slot_preset, + spoolman_k_profile, + spoolman_slot_assignment, + virtual_printer, + ) + + +@pytest.fixture +async def engine_with_case_variant_spools(): + from backend.app.core.database import Base + + _register_all_models() + engine = create_async_engine("sqlite+aiosqlite:///:memory:", echo=False) + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + await conn.execute(text("DELETE FROM locations")) + await conn.execute( + text( + """ + INSERT INTO spool ( + material, storage_location, label_weight, core_weight, + weight_used, weight_used_baseline, weight_locked + ) + VALUES ('PLA', 'Drybox 1', 1000, 250, 0, 0, 0), + ('PETG', 'DRYBOX 1', 1000, 250, 0, 0, 0) + """ + ) + ) + yield engine + await engine.dispose() + + +async def test_backfill_collapses_case_variant_storage_locations(engine_with_case_variant_spools): + async with engine_with_case_variant_spools.begin() as conn: + await run_migrations(conn) + + async with engine_with_case_variant_spools.connect() as conn: + loc_rows = (await conn.execute(text("SELECT id, name, name_key FROM locations ORDER BY id"))).all() + spool_rows = (await conn.execute(text("SELECT id, storage_location, location_id FROM spool ORDER BY id"))).all() + + assert len(loc_rows) == 1 + assert loc_rows[0].name_key == "drybox 1" + location_id = loc_rows[0].id + assert all(row.location_id == location_id for row in spool_rows) + + +async def test_backfill_is_idempotent_with_existing_locations(engine_with_case_variant_spools): + async with engine_with_case_variant_spools.begin() as conn: + await run_migrations(conn) + async with engine_with_case_variant_spools.begin() as conn: + await run_migrations(conn) + + async with engine_with_case_variant_spools.connect() as conn: + loc_count = (await conn.execute(text("SELECT COUNT(*) FROM locations"))).scalar_one() + linked = (await conn.execute(text("SELECT COUNT(*) FROM spool WHERE location_id IS NOT NULL"))).scalar_one() + + assert loc_count == 1 + assert linked == 2 + + +@pytest.fixture +async def engine_with_null_storage_location(): + """A spool with NULL storage_location must NOT produce a phantom location row + or get linked to anything — it stays NULL on both fields.""" + from backend.app.core.database import Base + + _register_all_models() + engine = create_async_engine("sqlite+aiosqlite:///:memory:", echo=False) + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + await conn.execute(text("DELETE FROM locations")) + await conn.execute( + text( + """ + INSERT INTO spool ( + material, storage_location, label_weight, core_weight, + weight_used, weight_used_baseline, weight_locked + ) + VALUES ('PLA', NULL, 1000, 250, 0, 0, 0), + ('PETG', ' ', 1000, 250, 0, 0, 0), + ('TPU', 'Real Shelf', 1000, 250, 0, 0, 0) + """ + ) + ) + yield engine + await engine.dispose() + + +async def test_backfill_skips_null_and_whitespace_storage_location( + engine_with_null_storage_location, +): + """NULL / whitespace-only `storage_location` rows must NOT create catalog + rows; only the 'Real Shelf' value gets a location row + spool link.""" + async with engine_with_null_storage_location.begin() as conn: + await run_migrations(conn) + + async with engine_with_null_storage_location.connect() as conn: + loc_rows = (await conn.execute(text("SELECT name FROM locations"))).all() + unlinked = ( + await conn.execute(text("SELECT material FROM spool WHERE location_id IS NULL ORDER BY material")) + ).all() + + # Only the row with a real storage_location should be in the catalog. + assert [r.name for r in loc_rows] == ["Real Shelf"] + # The NULL and whitespace-only spools stay unlinked (no phantom row). + assert [r.material for r in unlinked] == ["PETG", "PLA"] + + +@pytest.fixture +async def engine_with_legacy_null_name_key_location(): + """Simulate a legacy install where a `locations` row was manually inserted + BEFORE the name_key column existed. The migration must backfill the + legacy row's name_key BEFORE the dedup INSERT, so the spool-link UPDATE + can join on the new key (#1505 review IMPORTANT 11).""" + from backend.app.core.database import Base + + _register_all_models() + engine = create_async_engine("sqlite+aiosqlite:///:memory:", echo=False) + async with engine.begin() as conn: + await conn.run_sync(Base.metadata.create_all) + # Drop the model-shaped locations table (which has NOT NULL on + # name_key) and recreate it in its pre-migration shape: no name_key + # column at all, mirroring a real upgrade from a Bambuddy version + # that predates this feature. The migration's idempotent ALTER TABLE + # is what adds the column without a NOT NULL constraint, so the + # legacy row can legally have NULL until the new backfill UPDATE + # runs. + await conn.execute(text("DROP TABLE locations")) + await conn.execute( + text( + """ + CREATE TABLE locations ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + name VARCHAR(255) NOT NULL UNIQUE, + identifier VARCHAR(100), + created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, + updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP + ) + """ + ) + ) + await conn.execute(text("INSERT INTO locations (name) VALUES ('Drybox 1')")) + await conn.execute( + text( + """ + INSERT INTO spool ( + material, storage_location, label_weight, core_weight, + weight_used, weight_used_baseline, weight_locked + ) + VALUES ('PLA', 'Drybox 1', 1000, 250, 0, 0, 0) + """ + ) + ) + yield engine + await engine.dispose() + + +async def test_backfill_links_spool_to_legacy_null_name_key_location( + engine_with_legacy_null_name_key_location, +): + async with engine_with_legacy_null_name_key_location.begin() as conn: + await run_migrations(conn) + + async with engine_with_legacy_null_name_key_location.connect() as conn: + loc_rows = (await conn.execute(text("SELECT id, name, name_key FROM locations"))).all() + spool_rows = (await conn.execute(text("SELECT location_id FROM spool"))).all() + + # Exactly one location row (the pre-existing legacy one); its name_key + # got backfilled by the FIRST step of the migration. + assert len(loc_rows) == 1 + assert loc_rows[0].name_key == "drybox 1" + # The spool got linked to that legacy row — under the old ordering it + # would have been left with `location_id IS NULL`. + assert spool_rows[0].location_id == loc_rows[0].id diff --git a/backend/tests/unit/test_location_service.py b/backend/tests/unit/test_location_service.py new file mode 100644 index 000000000..f3f1b6417 --- /dev/null +++ b/backend/tests/unit/test_location_service.py @@ -0,0 +1,219 @@ +"""Unit tests for storage location service (#1004).""" + +import pytest +from sqlalchemy.ext.asyncio import AsyncSession + +from backend.app.models.location import Location +from backend.app.models.spool import Spool +from backend.app.services.location_service import ( + assign_location_name, + enrich_spool_dicts_with_location_id, + get_location_by_name, + location_name_key, + prepare_internal_spool_payload, + rename_location, + resolve_location_by_name, + resolve_spool_location_fields, + sync_locations_from_spoolman, +) + + +@pytest.mark.asyncio +async def test_resolve_location_by_name_creates(db_session: AsyncSession): + loc = await resolve_location_by_name(db_session, "Shelf A") + await db_session.commit() + assert loc is not None + assert loc.name == "Shelf A" + assert loc.name_key == location_name_key("Shelf A") + + again = await get_location_by_name(db_session, "shelf a") + assert again is not None + assert again.id == loc.id + + +@pytest.mark.asyncio +async def test_prepare_internal_spool_payload_from_location_id(db_session: AsyncSession): + loc = Location() + assign_location_name(loc, "Drawer 2") + db_session.add(loc) + await db_session.commit() + await db_session.refresh(loc) + + payload = await prepare_internal_spool_payload( + db_session, + {"material": "PLA", "location_id": loc.id}, + {"material", "location_id"}, + ) + assert payload["location_id"] == loc.id + assert payload["storage_location"] == "Drawer 2" + + +@pytest.mark.asyncio +async def test_resolve_spool_location_fields_prefers_location_id(db_session: AsyncSession): + loc = Location() + assign_location_name(loc, "Catalog A") + db_session.add(loc) + await db_session.commit() + await db_session.refresh(loc) + + resolved = await resolve_spool_location_fields( + db_session, + location_id=loc.id, + storage_location="Other", + fields_set={"location_id", "storage_location"}, + ) + assert resolved is not None + assert resolved.location_id == loc.id + assert resolved.storage_location == "Catalog A" + + +@pytest.mark.asyncio +async def test_rename_location_updates_spool_storage(db_session: AsyncSession): + loc = Location() + assign_location_name(loc, "Old Shelf") + spool = Spool(material="PLA", location_id=None, storage_location="Old Shelf") + db_session.add(loc) + db_session.add(spool) + await db_session.commit() + await db_session.refresh(loc) + + await rename_location(db_session, loc, "New Shelf") + await db_session.commit() + await db_session.refresh(spool) + + assert loc.name == "New Shelf" + assert loc.name_key == location_name_key("New Shelf") + assert spool.storage_location == "New Shelf" + assert spool.location_id == loc.id + + +@pytest.mark.asyncio +async def test_enrich_spool_dicts_with_location_id(db_session: AsyncSession): + loc = Location() + assign_location_name(loc, "Garage") + db_session.add(loc) + await db_session.commit() + + spools = [{"id": 1, "storage_location": "Garage"}, {"id": 2, "storage_location": None}] + await enrich_spool_dicts_with_location_id(db_session, spools) + assert spools[0]["location_id"] == loc.id + assert spools[1]["location_id"] is None + + +@pytest.mark.asyncio +async def test_sync_locations_from_spoolman_stages_without_commit(db_session: AsyncSession): + class FakeClient: + async def get_distinct_locations(self): + return ["Spoolman Shelf"] + + changed = await sync_locations_from_spoolman(db_session, FakeClient()) + assert changed is True + loc = await get_location_by_name(db_session, "Spoolman Shelf") + assert loc is not None + # Caller owns the transaction — no commit() was called in sync itself. + assert loc.id is not None + + +@pytest.mark.asyncio +async def test_sync_locations_from_spoolman_dedupes_case_variants(db_session: AsyncSession): + class FakeClient: + async def get_distinct_locations(self): + return ["Drybox 1", "DRYBOX 1", "Locker"] + + changed = await sync_locations_from_spoolman(db_session, FakeClient()) + assert changed is True + await db_session.commit() + + drybox = await get_location_by_name(db_session, "Drybox 1") + locker = await get_location_by_name(db_session, "Locker") + assert drybox is not None + assert locker is not None + + from sqlalchemy import func, select + + from backend.app.models.location import Location + + count = await db_session.scalar(select(func.count()).select_from(Location)) + assert count == 2 + + +@pytest.mark.asyncio +async def test_rename_location_duplicate_name_raises(db_session: AsyncSession): + first = Location() + assign_location_name(first, "Shelf A") + second = Location() + assign_location_name(second, "Shelf B") + db_session.add_all([first, second]) + await db_session.commit() + await db_session.refresh(first) + await db_session.refresh(second) + + with pytest.raises(ValueError, match="already exists"): + await rename_location(db_session, second, "Shelf A") + + +@pytest.mark.asyncio +async def test_rename_location_picks_up_legacy_row_with_trailing_whitespace(db_session: AsyncSession): + """A legacy spool whose `storage_location` carries trailing whitespace + must still get relinked by the rename cascade — the SQL `TRIM()` strips + the column, so the Python comparison must also strip `old_name`.""" + loc = Location() + assign_location_name(loc, "Old Shelf") + # Simulate a legacy row whose name was stored with the same value but + # the column entry has whitespace padding (this happens in old free-text + # data + manual DB edits). + legacy_spool = Spool(material="PLA", location_id=None, storage_location=" Old Shelf ") + db_session.add(loc) + db_session.add(legacy_spool) + await db_session.commit() + await db_session.refresh(loc) + await db_session.refresh(legacy_spool) + + # Force the in-memory name to carry trailing whitespace so the rename + # path lifts a non-stripped `old_name`. This is the asymmetry the fix + # addresses (#1505 review IMPORTANT 10). + loc.name = "Old Shelf " + + await rename_location(db_session, loc, "New Shelf") + await db_session.commit() + await db_session.refresh(legacy_spool) + + assert legacy_spool.storage_location == "New Shelf" + assert legacy_spool.location_id == loc.id + + +@pytest.mark.asyncio +async def test_sync_locations_from_spoolman_logs_and_returns_false_on_unavailable(db_session: AsyncSession, caplog): + """Bare `except Exception: return False` was the prior shape — verify the + narrowed catch surfaces a warning so ops can see Spoolman outages.""" + from backend.app.services.spoolman import SpoolmanUnavailableError + + class FailingClient: + async def get_distinct_locations(self): + raise SpoolmanUnavailableError("Cannot reach Spoolman") + + with caplog.at_level("WARNING", logger="backend.app.services.location_service"): + changed = await sync_locations_from_spoolman(db_session, FailingClient()) + + assert changed is False + assert any("location sync from Spoolman failed" in rec.message for rec in caplog.records) + + +@pytest.mark.asyncio +async def test_sync_locations_from_spoolman_handles_dict_payload(db_session: AsyncSession): + """Newer Spoolman returns `list[dict]` from `/location`; the SpoolmanClient + normalises to `list[str]`, so sync_locations_from_spoolman should accept + both shapes via the client contract.""" + + class DictShapeClient: + async def get_distinct_locations(self): + # SpoolmanClient.get_distinct_locations is the one that normalises; + # at this layer the contract is `list[str]`. Simulate post-normalisation. + return ["Cabinet 3", "Cabinet 3"] # dedup tested elsewhere — sanity here + + changed = await sync_locations_from_spoolman(db_session, DictShapeClient()) + assert changed is True + await db_session.commit() + + cabinet = await get_location_by_name(db_session, "Cabinet 3") + assert cabinet is not None diff --git a/backend/tests/unit/test_sidebar_settings.py b/backend/tests/unit/test_sidebar_settings.py new file mode 100644 index 000000000..b5cba3032 --- /dev/null +++ b/backend/tests/unit/test_sidebar_settings.py @@ -0,0 +1,17 @@ +import pytest +from pydantic import ValidationError + +from backend.app.schemas.settings import AppSettingsUpdate + + +def test_default_sidebar_order_accepts_hidden_system_item_ids(): + value = '{"order":["printers","ext-1","settings"],"hiddenSystemItemIds":["stats"]}' + + update = AppSettingsUpdate(default_sidebar_order=value) + + assert update.default_sidebar_order == value + + +def test_default_sidebar_order_rejects_invalid_hidden_system_item_ids(): + with pytest.raises(ValidationError): + AppSettingsUpdate(default_sidebar_order='{"order":["printers"],"hiddenSystemItemIds":"stats"}') diff --git a/backend/tests/unit/test_spoolman_inventory_methods.py b/backend/tests/unit/test_spoolman_inventory_methods.py index 55a94be07..28bc6deae 100644 --- a/backend/tests/unit/test_spoolman_inventory_methods.py +++ b/backend/tests/unit/test_spoolman_inventory_methods.py @@ -529,3 +529,139 @@ class TestGetExternalFilamentsRaisesOnError: pytest.raises(SpoolmanUnavailableError), ): await client.get_external_filaments() + + +# --------------------------------------------------------------------------- +# get_distinct_locations — shape normalisation (#1505 review BLOCKER 3) +# --------------------------------------------------------------------------- + + +class TestGetDistinctLocationsShape: + @pytest.mark.asyncio + async def test_passes_through_list_of_strings(self, client): + with patch.object(client, "_get_with_retry", AsyncMock(return_value=["Drybox 1", "Shelf"])): + result = await client.get_distinct_locations() + assert result == ["Drybox 1", "Shelf"] + + @pytest.mark.asyncio + async def test_extracts_name_from_list_of_dicts(self, client): + with patch.object( + client, + "_get_with_retry", + AsyncMock(return_value=[{"id": 1, "name": "Drybox 1"}, {"id": 2, "name": "Shelf"}]), + ): + result = await client.get_distinct_locations() + assert result == ["Drybox 1", "Shelf"] + + @pytest.mark.asyncio + async def test_drops_non_string_and_dict_without_name(self, client): + with patch.object( + client, + "_get_with_retry", + AsyncMock(return_value=[{"id": 1}, None, 42, "Shelf"]), + ): + result = await client.get_distinct_locations() + assert result == ["Shelf"] + + @pytest.mark.asyncio + async def test_returns_empty_list_on_non_list_payload(self, client): + # A misconfigured proxy or auth-redirect can serve HTML; the old shape + # would TypeError on iteration. We coerce to []. + with patch.object(client, "_get_with_retry", AsyncMock(return_value={"error": "unauthorized"})): + result = await client.get_distinct_locations() + assert result == [] + + +# --------------------------------------------------------------------------- +# rename_location — bulk endpoint + per-spool fallback (#1505 review BLOCKER 2) +# --------------------------------------------------------------------------- + + +class TestRenameLocationBulkAndFallback: + @pytest.mark.asyncio + async def test_bulk_endpoint_success_returns_zero(self, client): + """Modern Spoolman PATCH /location/{name} succeeds — fallback not used.""" + mock_http = AsyncMock() + mock_http.patch = AsyncMock(return_value=_make_response(None)) + with patch.object(client, "_get_client", AsyncMock(return_value=mock_http)): + result = await client.rename_location("Drybox 1", "Drybox 2") + assert result == 0 + # Confirm the bulk path was used (no per-spool PATCH). + mock_http.patch.assert_called_once() + assert "/location/" in mock_http.patch.call_args.args[0] + + @pytest.mark.asyncio + async def test_bulk_endpoint_404_falls_back_to_per_spool_patch(self, client): + """Older Spoolman versions return 404 on the bulk endpoint — the + fallback iterates every spool currently at the old name.""" + bulk_response = MagicMock() + bulk_response.status_code = 404 + bulk_response.raise_for_status = MagicMock( + side_effect=httpx.HTTPStatusError("404 Not Found", request=MagicMock(), response=MagicMock(status_code=404)) + ) + mock_http = AsyncMock() + mock_http.patch = AsyncMock(return_value=bulk_response) + + spools_at_old = [ + {"id": 11, "location": "Drybox 1"}, + {"id": 12, "location": "Drybox 1"}, + {"id": 13, "location": "Shelf A"}, # different location — must be skipped + ] + patch_response = MagicMock() + patch_response.status_code = 200 + patch_response.raise_for_status = MagicMock() + patch_response.json.return_value = {"id": 0, "location": "Drybox 2"} + + with ( + patch.object(client, "_get_client", AsyncMock(return_value=mock_http)), + patch.object(client, "get_all_spools", AsyncMock(return_value=spools_at_old)), + patch.object(client, "_request_spool", AsyncMock(return_value=patch_response)) as request_spool_mock, + ): + result = await client.rename_location("Drybox 1", "Drybox 2") + + assert result == 2 + # Only the two matching spools should be PATCHed. + assert request_spool_mock.await_count == 2 + called_ids = sorted(call.args[1] for call in request_spool_mock.await_args_list) + assert called_ids == [11, 12] + # And each call should set the new location string. + for call in request_spool_mock.await_args_list: + assert call.kwargs["json_body"] == {"location": "Drybox 2"} + + @pytest.mark.asyncio + async def test_bulk_endpoint_405_also_falls_back(self, client): + """Some Spoolman versions return 405 Method Not Allowed instead of 404 + when the bulk endpoint is missing — same fallback.""" + bulk_response = MagicMock() + bulk_response.status_code = 405 + bulk_response.raise_for_status = MagicMock( + side_effect=httpx.HTTPStatusError( + "405 Method Not Allowed", request=MagicMock(), response=MagicMock(status_code=405) + ) + ) + mock_http = AsyncMock() + mock_http.patch = AsyncMock(return_value=bulk_response) + + with ( + patch.object(client, "_get_client", AsyncMock(return_value=mock_http)), + patch.object(client, "get_all_spools", AsyncMock(return_value=[])), + ): + result = await client.rename_location("Drybox 1", "Drybox 2") + # No spools at the old name → nothing to do, fallback returns 0. + assert result == 0 + + @pytest.mark.asyncio + async def test_bulk_endpoint_non_404_5xx_propagates(self, client): + """A genuine server error must NOT silently fall back.""" + bulk_response = MagicMock() + bulk_response.status_code = 500 + bulk_response.raise_for_status = MagicMock( + side_effect=httpx.HTTPStatusError("500", request=MagicMock(), response=MagicMock(status_code=500)) + ) + mock_http = AsyncMock() + mock_http.patch = AsyncMock(return_value=bulk_response) + with ( + patch.object(client, "_get_client", AsyncMock(return_value=mock_http)), + pytest.raises(httpx.HTTPStatusError), + ): + await client.rename_location("Drybox 1", "Drybox 2") diff --git a/docs/screenshots/storage-locations/inventory-location-filter.png b/docs/screenshots/storage-locations/inventory-location-filter.png new file mode 100644 index 000000000..f8abb7991 Binary files /dev/null and b/docs/screenshots/storage-locations/inventory-location-filter.png differ diff --git a/docs/screenshots/storage-locations/locations-page.png b/docs/screenshots/storage-locations/locations-page.png new file mode 100644 index 000000000..21df17347 Binary files /dev/null and b/docs/screenshots/storage-locations/locations-page.png differ diff --git a/docs/screenshots/storage-locations/spool-form-storage-location.png b/docs/screenshots/storage-locations/spool-form-storage-location.png new file mode 100644 index 000000000..b9cbf1a58 Binary files /dev/null and b/docs/screenshots/storage-locations/spool-form-storage-location.png differ diff --git a/docs/storage-locations.md b/docs/storage-locations.md new file mode 100644 index 000000000..36dd2de12 --- /dev/null +++ b/docs/storage-locations.md @@ -0,0 +1,44 @@ +# Storage Locations (#1004) + +Structured storage locations let you manage physical shelves, drawers, and dryboxes as a catalog instead of free-text only. + +## Architecture + +- **`locations` table** — catalog of named storage spots (`name` + case-insensitive `name_key`). +- **`spool.location_id`** — source of truth for structured assignment. +- **`spool.storage_location`** — denormalized display string and Spoolman wire format; always derived on write via `location_service.resolve_spool_location_fields()`. +- **Frontend** — spool form sends only `location_id`; backend fills `storage_location`. + +## Location vs Storage Location vs AMS Location + +| UI label | Meaning | +|----------|---------| +| **Location** (inventory table column) | AMS slot or printer assignment (e.g. `H2D-1 B4`) | +| **Storage Location** | Physical shelf/drawer where the spool lives when not in AMS | +| **Locations page** | Catalog of named storage spots with spool counts | + +## Managing locations + +1. Open **Inventory → Locations** +2. Click **Add Location** and enter a name (e.g. `Regal Etage 2`) +3. Assign spools via the spool edit form **Storage Location** dropdown +4. Click a location row to filter inventory by that shelf + +## Spoolman mode + +Bambuddy keeps a local location catalog. When Spoolman integration is enabled: + +- Assigning a location writes the location **name** to Spoolman's `location` field +- Listing locations syncs distinct names from Spoolman into the catalog +- Renaming a location bulk-renames spools in Spoolman via `PATCH /location/{old}` + +## Upgrade migration + +Existing free-text `storage_location` values are automatically imported into the location catalog and linked on upgrade (case-insensitive dedup via `name_key`). + +## Testing before release + +1. `./test_frontend.sh` — i18n parity, lint, Vitest +2. `./test_backend.sh` — Ruff, pytest (includes `test_locations_api.py`, `test_location_service.py`) +3. Manual: assign a spool to a location → open **Locations** → spool count updates without reload +4. Companion PR in [bambuddy-wiki](https://github.com/maziggy/bambuddy-wiki) (user-facing guide) diff --git a/frontend/src/__tests__/components/Layout.test.tsx b/frontend/src/__tests__/components/Layout.test.tsx index f3c1905ec..c9b2d38d4 100644 --- a/frontend/src/__tests__/components/Layout.test.tsx +++ b/frontend/src/__tests__/components/Layout.test.tsx @@ -2,15 +2,21 @@ * Tests for the Layout component. */ -import { describe, it, expect, beforeEach } from 'vitest'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; import { waitFor } from '@testing-library/react'; import { render } from '../utils'; import { Layout } from '../../components/Layout'; import { http, HttpResponse } from 'msw'; import { server } from '../mocks/server'; +import { SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY, SIDEBAR_ORDER_KEY } from '../../utils/sidebarLayout'; describe('Layout', () => { beforeEach(() => { + vi.mocked(localStorage.getItem).mockReset(); + vi.mocked(localStorage.setItem).mockReset(); + vi.mocked(localStorage.removeItem).mockReset(); + vi.mocked(localStorage.clear).mockReset(); + localStorage.clear(); server.use( http.get('/api/v1/printers/', () => { return HttpResponse.json([ @@ -102,6 +108,55 @@ describe('Layout', () => { expect(settingsLink).toBeInTheDocument(); }); }); + + it('hides system nav items stored in sidebar layout preferences', async () => { + vi.mocked(localStorage.getItem).mockImplementation((key) => { + if (key === SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY) return JSON.stringify(['printers']); + return null; + }); + + render(); + + await waitFor(() => { + const sidebar = document.querySelector('aside'); + expect(sidebar).toBeInTheDocument(); + expect(sidebar?.querySelector('a[href="/inventory"]')).toBeInTheDocument(); + }); + + expect(document.querySelector('aside a[href="/"]')).toBeNull(); + }); + + it('applies admin default sidebar hidden state with the default order', async () => { + const storage: Record = {}; + vi.mocked(localStorage.getItem).mockImplementation((key) => storage[key] ?? null); + vi.mocked(localStorage.setItem).mockImplementation((key, value) => { + storage[key] = value; + }); + server.use( + http.get('/api/v1/settings/default-sidebar-order', () => + HttpResponse.json({ + default_sidebar_order: JSON.stringify({ + order: ['inventory', 'printers', 'settings'], + hiddenSystemItemIds: ['printers'], + }), + }), + ), + ); + + render(); + + await waitFor(() => { + const sidebar = document.querySelector('aside'); + expect(sidebar).toBeInTheDocument(); + expect(sidebar?.querySelector('a[href="/inventory"]')).toBeInTheDocument(); + }); + + await waitFor(() => { + expect(document.querySelector('aside a[href="/"]')).toBeNull(); + expect(localStorage.setItem).toHaveBeenCalledWith(SIDEBAR_ORDER_KEY, JSON.stringify(['inventory', 'printers', 'settings'])); + expect(localStorage.setItem).toHaveBeenCalledWith(SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY, JSON.stringify(['printers'])); + }); + }); }); describe('version display', () => { diff --git a/frontend/src/__tests__/components/LocationsModal.test.tsx b/frontend/src/__tests__/components/LocationsModal.test.tsx new file mode 100644 index 000000000..f5845fe66 --- /dev/null +++ b/frontend/src/__tests__/components/LocationsModal.test.tsx @@ -0,0 +1,243 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { MemoryRouter } from 'react-router-dom'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { LocationsModal } from '../../components/LocationsModal'; +import { api, ApiError } from '../../api/client'; + +const mockShowToast = vi.fn(); +const mockOnClose = vi.fn(); +const mockOnPickLocation = vi.fn(); + +vi.mock('../../api/client', () => ({ + api: { + getLocations: vi.fn(), + createLocation: vi.fn(), + updateLocation: vi.fn(), + deleteLocation: vi.fn(), + }, + ApiError: class ApiError extends Error { + status: number; + constructor(message: string, status: number) { + super(message); + this.status = status; + } + }, +})); + +vi.mock('../../contexts/ToastContext', () => ({ + useToast: () => ({ showToast: mockShowToast }), +})); + +const locations = [ + { id: 1, name: 'Shelf A', identifier: null, spool_count: 2, created_at: '2026-01-01', updated_at: '2026-01-01' }, + { id: 2, name: 'Drawer 1', identifier: null, spool_count: 0, created_at: '2026-01-01', updated_at: '2026-01-01' }, +]; + +function renderModal(open = true) { + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render( + + + + + , + ); +} + +describe('LocationsModal', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(api.getLocations).mockResolvedValue(locations); + }); + + it('renders nothing when open=false', () => { + const { container } = renderModal(false); + expect(container.firstChild).toBeNull(); + expect(api.getLocations).not.toHaveBeenCalled(); + }); + + it('renders locations from API when open', async () => { + renderModal(); + expect(await screen.findByText('Shelf A')).toBeInTheDocument(); + expect(screen.getByText('Drawer 1')).toBeInTheDocument(); + expect(screen.getByText('2')).toBeInTheDocument(); + }); + + it('renders empty state when API returns no locations', async () => { + vi.mocked(api.getLocations).mockResolvedValue([]); + renderModal(); + expect(await screen.findByText(/locations\.empty|no storage locations/i)).toBeInTheDocument(); + }); + + it('opens create editor and calls createLocation on submit', async () => { + vi.mocked(api.createLocation).mockResolvedValue({ + id: 3, + name: 'Garage', + identifier: null, + spool_count: 0, + created_at: '2026-01-01', + updated_at: '2026-01-01', + }); + const user = userEvent.setup(); + renderModal(); + await screen.findByText('Shelf A'); + await user.click(screen.getByRole('button', { name: /add location|locations\.add/i })); + const input = screen.getByLabelText(/name|locations\.name/i); + await user.type(input, 'Garage'); + await user.click(screen.getByRole('button', { name: /save|common\.save/i })); + await waitFor(() => { + expect(api.createLocation).toHaveBeenCalledWith({ name: 'Garage' }); + }); + expect(mockShowToast).toHaveBeenCalledWith(expect.stringMatching(/created|locations\.created/i), 'success'); + }); + + it('submits create form on Enter key', async () => { + vi.mocked(api.createLocation).mockResolvedValue({ + id: 3, + name: 'Garage', + identifier: null, + spool_count: 0, + created_at: '2026-01-01', + updated_at: '2026-01-01', + }); + const user = userEvent.setup(); + renderModal(); + await screen.findByText('Shelf A'); + await user.click(screen.getByRole('button', { name: /add location|locations\.add/i })); + const input = screen.getByLabelText(/name|locations\.name/i); + await user.type(input, 'Garage{Enter}'); + await waitFor(() => { + expect(api.createLocation).toHaveBeenCalledWith({ name: 'Garage' }); + }); + }); + + it('Escape closes the inner editor first, then the outer modal', async () => { + const user = userEvent.setup(); + renderModal(); + await screen.findByText('Shelf A'); + // Open the inner editor; both dialogs are now in the DOM. + await user.click(screen.getByRole('button', { name: /add location|locations\.add/i })); + expect(screen.getAllByRole('dialog')).toHaveLength(2); + // First Escape closes the editor only. + await user.keyboard('{Escape}'); + await waitFor(() => { + expect(screen.getAllByRole('dialog')).toHaveLength(1); + }); + expect(mockOnClose).not.toHaveBeenCalled(); + // Second Escape closes the outer modal. + await user.keyboard('{Escape}'); + await waitFor(() => { + expect(mockOnClose).toHaveBeenCalledTimes(1); + }); + }); + + it('edits a location and calls updateLocation', async () => { + vi.mocked(api.updateLocation).mockResolvedValue({ + id: 2, + name: 'Drawer 2', + identifier: null, + spool_count: 0, + created_at: '2026-01-01', + updated_at: '2026-01-01', + }); + const user = userEvent.setup(); + renderModal(); + await screen.findByText('Drawer 1'); + const editButtons = screen.getAllByTitle(/edit|common\.edit/i); + await user.click(editButtons[1]); + const input = screen.getByLabelText(/name|locations\.name/i); + await user.clear(input); + await user.type(input, 'Drawer 2'); + await user.click(screen.getByRole('button', { name: /save|common\.save/i })); + await waitFor(() => { + expect(api.updateLocation).toHaveBeenCalledWith(2, { name: 'Drawer 2' }); + }); + expect(mockShowToast).toHaveBeenCalledWith(expect.stringMatching(/updated|locations\.updated/i), 'success'); + }); + + it('deletes an empty location after confirmation', async () => { + vi.mocked(api.deleteLocation).mockResolvedValue({ status: 'deleted' }); + const user = userEvent.setup(); + renderModal(); + await screen.findByText('Drawer 1'); + const row = screen.getByText('Drawer 1').closest('tr'); + expect(row).not.toBeNull(); + await user.click(within(row!).getByTitle(/^Delete$/i)); + await user.click(screen.getAllByRole('button', { name: /^Delete$/i }).pop()!); + await waitFor(() => { + expect(api.deleteLocation).toHaveBeenCalledWith(2); + }); + expect(mockShowToast).toHaveBeenCalledWith(expect.stringMatching(/deleted|locations\.deleted/i), 'success'); + }); + + it('blocks delete when spool_count > 0', async () => { + renderModal(); + await screen.findByText('Shelf A'); + const blockedDelete = screen.getByTitle(/Remove all spools from this location before deleting/i); + expect(blockedDelete).toBeDisabled(); + }); + + it('shows error toast when create returns 409 duplicate name', async () => { + vi.mocked(api.createLocation).mockRejectedValue( + new ApiError('A location with this name already exists', 409), + ); + const user = userEvent.setup(); + renderModal(); + await screen.findByText('Shelf A'); + await user.click(screen.getByRole('button', { name: /add location|locations\.add/i })); + await user.type(screen.getByLabelText(/name|locations\.name/i), 'Shelf A'); + await user.click(screen.getByRole('button', { name: /save|common\.save/i })); + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith('A location with this name already exists', 'error'); + }); + }); + + it('shows error toast when delete fails', async () => { + vi.mocked(api.deleteLocation).mockRejectedValue(new Error('Delete failed')); + const user = userEvent.setup(); + renderModal(); + await screen.findByText('Drawer 1'); + const row = screen.getByText('Drawer 1').closest('tr'); + expect(row).not.toBeNull(); + await user.click(within(row!).getByTitle(/^Delete$/i)); + await user.click(screen.getAllByRole('button', { name: /^Delete$/i }).pop()!); + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith('Delete failed', 'error'); + }); + }); + + it('row click calls onPickLocation and onClose', async () => { + const user = userEvent.setup(); + renderModal(); + await screen.findByText('Shelf A'); + const row = screen.getByText('Shelf A').closest('tr')!; + await user.click(row); + expect(mockOnPickLocation).toHaveBeenCalledWith(1); + expect(mockOnClose).toHaveBeenCalledTimes(1); + }); + + it('shows error toast when rename returns 409 collision', async () => { + vi.mocked(api.updateLocation).mockRejectedValue( + new ApiError('A location with this name already exists', 409), + ); + const user = userEvent.setup(); + renderModal(); + await screen.findByText('Drawer 1'); + + const editButtons = screen.getAllByTitle(/edit|common\.edit/i); + await user.click(editButtons[1]); + const input = screen.getByLabelText(/name|locations\.name/i); + await user.clear(input); + await user.type(input, 'Shelf A'); + await user.click(screen.getByRole('button', { name: /save|common\.save/i })); + + await waitFor(() => { + expect(mockShowToast).toHaveBeenCalledWith( + 'A location with this name already exists', + 'error', + ); + }); + }); +}); diff --git a/frontend/src/__tests__/components/SpoolFormBulk.test.tsx b/frontend/src/__tests__/components/SpoolFormBulk.test.tsx index 902a6e6b0..4be023e5a 100644 --- a/frontend/src/__tests__/components/SpoolFormBulk.test.tsx +++ b/frontend/src/__tests__/components/SpoolFormBulk.test.tsx @@ -29,6 +29,7 @@ vi.mock('../../api/client', () => ({ getCloudStatus: vi.fn().mockResolvedValue({ is_authenticated: false }), getFilamentPresets: vi.fn().mockResolvedValue([]), getSpoolCatalog: vi.fn().mockResolvedValue([]), + getLocations: vi.fn().mockResolvedValue([]), getColorCatalog: vi.fn().mockResolvedValue([]), getLocalPresets: vi.fn().mockResolvedValue({ filament: [] }), getBuiltinFilaments: vi.fn().mockResolvedValue([]), diff --git a/frontend/src/__tests__/components/SpoolFormModal.test.tsx b/frontend/src/__tests__/components/SpoolFormModal.test.tsx index 32cea5070..7a1894d90 100644 --- a/frontend/src/__tests__/components/SpoolFormModal.test.tsx +++ b/frontend/src/__tests__/components/SpoolFormModal.test.tsx @@ -21,6 +21,7 @@ vi.mock('../../api/client', () => ({ getCloudStatus: vi.fn().mockResolvedValue({ is_authenticated: false }), getFilamentPresets: vi.fn().mockResolvedValue([]), getSpoolCatalog: vi.fn().mockResolvedValue([]), + getLocations: vi.fn().mockResolvedValue([]), getColorCatalog: vi.fn().mockResolvedValue([]), getLocalPresets: vi.fn().mockResolvedValue({ filament: [] }), getBuiltinFilaments: vi.fn().mockResolvedValue([]), @@ -932,20 +933,25 @@ describe('SpoolFormModal — Unassign button (#1336)', () => { }); }); -describe('SpoolFormModal storageLocationTouched', () => { +describe('SpoolFormModal locationIdTouched', () => { /** * Regression tests for the round-trip bug: saving the edit modal without - * touching the Storage Location field must NOT include storage_location in + * touching the Storage Location field must NOT include location_id in * the PATCH payload, so Spoolman's location field is never overwritten with * a stale cached value. */ beforeEach(() => { vi.clearAllMocks(); + vi.mocked(api.getLocations).mockResolvedValue([ + { id: 1, name: 'IKEAREGAL', identifier: null, spool_count: 1, created_at: '', updated_at: '' }, + { id: 2, name: 'Shelf B', identifier: null, spool_count: 0, created_at: '', updated_at: '' }, + ]); }); const spoolWithStorageLocation: InventorySpool = { ...existingSpool, storage_location: 'IKEAREGAL', + location_id: 1, }; it('excludes storage_location from PATCH when editing without changing it', async () => { @@ -975,11 +981,12 @@ describe('SpoolFormModal storageLocationTouched', () => { expect(spoolId).toBe(1); // storage_location must NOT be in the payload — prevents Spoolman location overwrite expect(payload).not.toHaveProperty('storage_location'); + expect(payload).not.toHaveProperty('location_id'); // Other fields should still be present expect(payload).toHaveProperty('material', 'PLA'); }); - it('includes storage_location in PATCH when editing and changing it', async () => { + it('includes location_id in PATCH when editing and changing it', async () => { render( { expect(screen.getByText('Edit Spool')).toBeInTheDocument(); }); - // Find the storage location input and change it - const locationInput = screen.getByPlaceholderText('e.g. Shelf A, Drawer 1'); - fireEvent.change(locationInput, { target: { value: 'Shelf B' } }); + // Change storage location via the catalog dropdown + const locationSelect = screen.getByLabelText(/storage location/i); + fireEvent.change(locationSelect, { target: { value: '2' } }); const saveButton = screen.getByRole('button', { name: /save/i }); fireEvent.click(saveButton); @@ -1007,11 +1014,11 @@ describe('SpoolFormModal storageLocationTouched', () => { const [spoolId, payload] = vi.mocked(api.updateSpool).mock.calls[0]; expect(spoolId).toBe(1); - // storage_location MUST be present since the user changed it - expect(payload).toHaveProperty('storage_location', 'Shelf B'); + expect(payload).toHaveProperty('location_id', 2); + expect(payload).not.toHaveProperty('storage_location'); }); - it('includes storage_location when creating a new spool', async () => { + it('includes location_id when creating a new spool', async () => { render( { }); const [payload] = vi.mocked(api.createSpool).mock.calls[0]; - // storage_location MUST be included for new spools (default empty string → null) - expect(payload).toHaveProperty('storage_location', null); + expect(payload).toHaveProperty('location_id', null); + expect(payload).not.toHaveProperty('storage_location'); }); }); diff --git a/frontend/src/__tests__/hooks/useWebSocket.test.ts b/frontend/src/__tests__/hooks/useWebSocket.test.ts index 61b4676dd..a33a4ba5e 100644 --- a/frontend/src/__tests__/hooks/useWebSocket.test.ts +++ b/frontend/src/__tests__/hooks/useWebSocket.test.ts @@ -433,6 +433,42 @@ describe('useWebSocket hook', () => { vi.unstubAllGlobals(); }); + it('invalidates inventory queries on inventory_changed message', async () => { + vi.useFakeTimers(); + vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => { + cb(0); + return 0; + }); + const { useWebSocket } = await import('../../hooks/useWebSocket'); + + const invalidateSpy = vi.spyOn(queryClient, 'invalidateQueries'); + + renderHook(() => useWebSocket(), { + wrapper: createWrapper(queryClient), + }); + + const ws = await waitForWs(); + + act(() => { + ws.open(); + }); + + act(() => { + ws.simulateMessage({ type: 'inventory_changed' }); + }); + + await act(async () => { + vi.advanceTimersByTime(5000); + }); + + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ['inventory-spools'] }); + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ['spoolman-inventory-spools'] }); + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ['inventory-locations'] }); + + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + it('handles missing_spool_assignment message without error', async () => { vi.stubGlobal('requestAnimationFrame', (cb: FrameRequestCallback) => { cb(0); diff --git a/frontend/src/__tests__/mocks/handlers.ts b/frontend/src/__tests__/mocks/handlers.ts index 14a7c9de3..3cf227e20 100644 --- a/frontend/src/__tests__/mocks/handlers.ts +++ b/frontend/src/__tests__/mocks/handlers.ts @@ -466,6 +466,7 @@ export const handlers = [ http.get('/api/v1/inventory/assignments', () => HttpResponse.json([])), http.get('/api/v1/inventory/catalog', () => HttpResponse.json([])), http.get('/api/v1/inventory/colors', () => HttpResponse.json([])), + http.get('/api/v1/inventory/locations', () => HttpResponse.json([])), http.get('/api/v1/inventory/spools', () => HttpResponse.json([])), http.get('/api/v1/library/folders', () => HttpResponse.json([])), http.get('/api/v1/library/folders/by-archive/:id', () => HttpResponse.json([])), diff --git a/frontend/src/__tests__/pages/InventoryPageDeepLink.test.tsx b/frontend/src/__tests__/pages/InventoryPageDeepLink.test.tsx index a41ecb52a..9e393e9d3 100644 --- a/frontend/src/__tests__/pages/InventoryPageDeepLink.test.tsx +++ b/frontend/src/__tests__/pages/InventoryPageDeepLink.test.tsx @@ -141,6 +141,7 @@ function setupCommonHandlers(spoolList: object[]) { http.get('/api/v1/inventory/color-catalog', () => HttpResponse.json([])), http.get('/api/v1/inventory/colors', () => HttpResponse.json([])), http.get('/api/v1/inventory/spool-catalog', () => HttpResponse.json([])), + http.get('/api/v1/inventory/locations', () => HttpResponse.json([])), http.get('/api/v1/printers/', () => HttpResponse.json([])), ); } diff --git a/frontend/src/__tests__/pages/SettingsPage.test.tsx b/frontend/src/__tests__/pages/SettingsPage.test.tsx index 305326c3a..58c5095bc 100644 --- a/frontend/src/__tests__/pages/SettingsPage.test.tsx +++ b/frontend/src/__tests__/pages/SettingsPage.test.tsx @@ -2,13 +2,15 @@ * Tests for the SettingsPage component. */ -import { describe, it, expect, beforeEach } from 'vitest'; -import { screen, waitFor } from '@testing-library/react'; +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { fireEvent, screen, waitFor, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { render } from '../utils'; import { SettingsPage } from '../../pages/SettingsPage'; import { http, HttpResponse } from 'msw'; import { server } from '../mocks/server'; +import { SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY, SIDEBAR_ORDER_KEY } from '../../utils/sidebarLayout'; +import { setAuthToken } from '../../api/client'; const mockSettings = { auto_archive: true, @@ -41,12 +43,18 @@ describe('SettingsPage', () => { // switch in one test (e.g. clicking "Workflow") doesn't carry into // sibling tests that expect to land on the default General tab. window.history.replaceState({}, '', '/'); + vi.mocked(localStorage.getItem).mockReset(); + vi.mocked(localStorage.setItem).mockReset(); + vi.mocked(localStorage.removeItem).mockReset(); + vi.mocked(localStorage.clear).mockReset(); + localStorage.clear(); + setAuthToken(null); server.use( http.get('/api/v1/settings/', () => { return HttpResponse.json(mockSettings); }), - http.patch('/api/v1/settings/', async ({ request }) => { + http.put('/api/v1/settings/', async ({ request }) => { const body = await request.json(); return HttpResponse.json({ ...mockSettings, ...body }); }), @@ -70,6 +78,9 @@ describe('SettingsPage', () => { }), http.get('/api/v1/auth/status', () => { return HttpResponse.json({ auth_enabled: false, requires_setup: false }); + }), + http.get('/api/v1/external-links/', () => { + return HttpResponse.json([]); }) ); }); @@ -172,6 +183,244 @@ describe('SettingsPage', () => { expect(screen.getByText('Check printer firmware')).toBeInTheDocument(); }); }); + + it('hides a Bambuddy sidebar page from Sidebar', async () => { + const user = userEvent.setup(); + render(); + + await screen.findByRole('heading', { name: 'Sidebar' }); + await screen.findAllByText('Visible in sidebar'); + + vi.mocked(localStorage.setItem).mockClear(); + await user.click((await screen.findAllByLabelText('Hide page'))[0]); + + expect(localStorage.setItem).toHaveBeenCalledWith(SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY, JSON.stringify(['printers'])); + expect(screen.getByText('Hidden from sidebar')).toBeInTheDocument(); + }); + + it('shows a previously hidden Bambuddy sidebar page from Sidebar', async () => { + vi.mocked(localStorage.getItem).mockImplementation((key) => { + if (key === SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY) return JSON.stringify(['printers']); + return null; + }); + + const user = userEvent.setup(); + render(); + + await screen.findByRole('heading', { name: 'Sidebar' }); + await screen.findByText('Hidden from sidebar'); + + vi.mocked(localStorage.setItem).mockClear(); + await user.click(await screen.findByLabelText('Show page')); + + expect(localStorage.setItem).toHaveBeenCalledWith(SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY, JSON.stringify([])); + expect(screen.getAllByText('Visible in sidebar').length).toBeGreaterThan(0); + }); + + it('does not allow Settings to be hidden from Sidebar', async () => { + render(); + + await screen.findByRole('heading', { name: 'Sidebar' }); + await screen.findByText('Required in sidebar'); + + const settingsVisibilityButton = await screen.findByLabelText('Settings cannot be hidden'); + expect(settingsVisibilityButton).toBeDisabled(); + expect(screen.getByText('Required in sidebar')).toBeInTheDocument(); + }); + + it('presents external links and Bambuddy pages in saved sidebar order', async () => { + vi.mocked(localStorage.getItem).mockImplementation((key) => { + if (key === SIDEBAR_ORDER_KEY) return JSON.stringify(['ext-7', 'printers', 'settings']); + return null; + }); + server.use( + http.get('/api/v1/external-links/', () => + HttpResponse.json([ + { + id: 7, + name: 'Docs', + url: 'https://docs.example.test', + icon: 'Link', + open_in_new_tab: true, + custom_icon: null, + sort_order: 0, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + }, + ]), + ), + ); + + render(); + + await screen.findByRole('heading', { name: 'Sidebar' }); + const docs = await screen.findByText('Docs'); + const printers = screen.getAllByText('Printers').find(element => element.closest('[draggable="true"]')); + + expect(printers).toBeDefined(); + expect(docs.compareDocumentPosition(printers) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); + }); + + it('saves mixed Sidebar order when items are dragged', async () => { + server.use( + http.get('/api/v1/external-links/', () => + HttpResponse.json([ + { + id: 7, + name: 'Docs', + url: 'https://docs.example.test', + icon: 'Link', + open_in_new_tab: true, + custom_icon: null, + sort_order: 0, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + }, + ]), + ), + ); + + render(); + + await screen.findByRole('heading', { name: 'Sidebar' }); + const docsRow = (await screen.findByText('Docs')).closest('[draggable="true"]'); + const printersRow = screen.getAllByText('Printers') + .find(element => element.closest('[draggable="true"]')) + ?.closest('[draggable="true"]'); + + expect(docsRow).not.toBeNull(); + expect(printersRow).not.toBeNull(); + + vi.mocked(localStorage.setItem).mockClear(); + const dataTransfer = { + effectAllowed: '', + dropEffect: '', + setData: vi.fn(), + }; + fireEvent.dragStart(docsRow!, { dataTransfer }); + fireEvent.dragOver(printersRow!, { dataTransfer }); + fireEvent.drop(printersRow!, { dataTransfer }); + + expect(localStorage.setItem).toHaveBeenCalledWith( + SIDEBAR_ORDER_KEY, + JSON.stringify(['ext-7', 'printers', 'inventory', 'archives', 'queue', 'projects', 'files', 'makerworld', 'profiles', 'maintenance', 'stats', 'settings']), + ); + }); + + it('resets Sidebar to all pages first and configured links at the bottom', async () => { + vi.mocked(localStorage.getItem).mockImplementation((key) => { + if (key === SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY) return JSON.stringify(['printers', 'stats']); + if (key === SIDEBAR_ORDER_KEY) return JSON.stringify(['ext-7', 'settings', 'printers']); + return null; + }); + server.use( + http.get('/api/v1/external-links/', () => + HttpResponse.json([ + { + id: 7, + name: 'Docs', + url: 'https://docs.example.test', + icon: 'Link', + open_in_new_tab: true, + custom_icon: null, + sort_order: 0, + created_at: '2026-01-01T00:00:00Z', + updated_at: '2026-01-01T00:00:00Z', + }, + ]), + ), + ); + + const user = userEvent.setup(); + render(); + + const heading = await screen.findByRole('heading', { name: 'Sidebar' }); + const card = heading.closest('#card-sidebar-links'); + expect(card).not.toBeNull(); + await screen.findByText('Docs'); + + vi.mocked(localStorage.setItem).mockClear(); + await user.click(within(card as HTMLElement).getByRole('button', { name: /reset/i })); + + expect(localStorage.setItem).toHaveBeenCalledWith(SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY, JSON.stringify([])); + expect(localStorage.setItem).toHaveBeenCalledWith( + SIDEBAR_ORDER_KEY, + JSON.stringify(['printers', 'inventory', 'archives', 'queue', 'projects', 'files', 'makerworld', 'profiles', 'maintenance', 'stats', 'settings', 'ext-7']), + ); + + const settingsRow = screen.getAllByText('Settings') + .find(element => element.closest('[draggable="true"]')) + ?.closest('[draggable="true"]'); + const docsRow = screen.getByText('Docs').closest('[draggable="true"]'); + expect(settingsRow).not.toBeNull(); + expect(docsRow).not.toBeNull(); + expect(settingsRow!.compareDocumentPosition(docsRow!) & Node.DOCUMENT_POSITION_FOLLOWING).toBeTruthy(); + expect(screen.queryByText('Hidden from sidebar')).not.toBeInTheDocument(); + }); + + it('sets the current Sidebar order as the backend default for settings admins', async () => { + let defaultSidebarOrderPayload: string | null = null; + vi.mocked(localStorage.getItem).mockImplementation((key) => { + if (key === SIDEBAR_HIDDEN_SYSTEM_ITEMS_KEY) return JSON.stringify(['stats']); + return null; + }); + + server.use( + http.get('/api/v1/auth/status', () => + HttpResponse.json({ auth_enabled: true, requires_setup: false }), + ), + http.get('/api/v1/auth/me', () => + HttpResponse.json({ + id: 1, + username: 'admin', + role: 'admin', + is_active: true, + is_admin: false, + groups: [{ id: 1, name: 'Administrators' }], + permissions: ['settings:update'], + created_at: '2026-01-01T00:00:00Z', + }), + ), + http.get('/api/v1/settings/', () => + HttpResponse.json({ ...mockSettings, default_sidebar_order: '' }), + ), + http.put('/api/v1/settings/', async ({ request }) => { + const body = await request.json() as { default_sidebar_order?: string }; + defaultSidebarOrderPayload = body.default_sidebar_order ?? null; + return HttpResponse.json({ ...mockSettings, ...body }); + }), + ); + setAuthToken('test-token'); + + const user = userEvent.setup(); + render(); + + const heading = await screen.findByRole('heading', { name: 'Sidebar' }); + const card = heading.closest('#card-sidebar-links'); + expect(card).not.toBeNull(); + + await user.click(within(card as HTMLElement).getByRole('switch', { name: 'Set Default' })); + + await waitFor(() => { + expect(defaultSidebarOrderPayload).not.toBeNull(); + }); + expect(JSON.parse(defaultSidebarOrderPayload!)).toEqual({ + order: [ + 'printers', + 'inventory', + 'archives', + 'queue', + 'projects', + 'files', + 'makerworld', + 'profiles', + 'maintenance', + 'stats', + 'settings', + ], + hiddenSystemItemIds: ['stats'], + }); + }); }); describe('update CTA per deployment shape', () => { diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 1c09481d3..93febd8ff 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -308,7 +308,10 @@ export interface Printer { name: string; serial_number: string; ip_address: string; - access_code: string; + // Optional because the backend only returns access_code when the caller has + // PRINTERS_UPDATE — Admin / Operator JWTs or auth-disabled mode. Viewers and + // API keys receive a Printer without this field. + access_code?: string; model: string | null; location: string | null; // Group/location name nozzle_count: number; // 1 or 2, auto-detected from MQTT @@ -1317,6 +1320,15 @@ export interface SpoolCatalogEntry { is_default: boolean; } +export interface StorageLocation { + id: number; + name: string; + identifier: string | null; + spool_count: number; + created_at: string; + updated_at: string; +} + export interface ColorCatalogEntry { id: number; manufacturer: string; @@ -2652,6 +2664,7 @@ export interface InventorySpool { low_stock_threshold_pct: number | null; k_profiles?: SpoolKProfile[]; storage_location?: string | null; + location_id?: number | null; } export interface SpoolmanBulkCreateResult { @@ -5084,6 +5097,14 @@ export const api = { request<{ deleted: number }>('/inventory/catalog/bulk-delete', { method: 'POST', body: JSON.stringify({ ids }) }), resetSpoolCatalog: () => request<{ status: string }>('/inventory/catalog/reset', { method: 'POST' }), + getLocations: () => + request('/inventory/locations'), + createLocation: (data: { name: string; identifier?: string | null }) => + request('/inventory/locations', { method: 'POST', body: JSON.stringify(data) }), + updateLocation: (id: number, data: { name?: string; identifier?: string | null }) => + request(`/inventory/locations/${id}`, { method: 'PATCH', body: JSON.stringify(data) }), + deleteLocation: (id: number) => + request<{ status: string }>(`/inventory/locations/${id}`, { method: 'DELETE' }), getColorCatalog: () => request('/inventory/colors'), getColorNameMap: () => diff --git a/frontend/src/components/ExternalLinksSettings.tsx b/frontend/src/components/ExternalLinksSettings.tsx index c004b9543..2991f7676 100644 --- a/frontend/src/components/ExternalLinksSettings.tsx +++ b/frontend/src/components/ExternalLinksSettings.tsx @@ -1,30 +1,47 @@ -import { useState } from 'react'; +import { useMemo, useState } from 'react'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; -import { Link2, Plus, Pencil, Trash2, GripVertical, Loader2, ExternalLink as ExternalLinkIcon } from 'lucide-react'; +import { Eye, EyeOff, Link2, Plus, Pencil, Trash2, GripVertical, Loader2, ExternalLink as ExternalLinkIcon, RotateCcw } from 'lucide-react'; import { useTranslation } from 'react-i18next'; import { api } from '../api/client'; import type { ExternalLink } from '../api/client'; import { Card, CardContent, CardHeader } from './Card'; import { Button } from './Button'; +import { Toggle } from './Toggle'; import { AddExternalLinkModal } from './AddExternalLinkModal'; import { ConfirmModal } from './ConfirmModal'; import { getIconByName } from './IconPicker'; +import { defaultNavItems } from './Layout'; +import { useAuth } from '../contexts/AuthContext'; +import { useToast } from '../contexts/ToastContext'; +import { + getHiddenSidebarSystemItemIds, + getSidebarOrder, + saveHiddenSidebarSystemItemIds, + saveSidebarOrder, +} from '../utils/sidebarLayout'; + +type SidebarLayoutItem = + | { type: 'system'; id: string; navItem: typeof defaultNavItems[number] } + | { type: 'external'; id: string; link: ExternalLink }; export function ExternalLinksSettings() { const { t } = useTranslation(); const queryClient = useQueryClient(); + const { authEnabled, hasPermission } = useAuth(); + const { showToast } = useToast(); const [showAddModal, setShowAddModal] = useState(false); const [editingLink, setEditingLink] = useState(null); const [deletingLink, setDeletingLink] = useState(null); - const [draggedId, setDraggedId] = useState(null); + const [draggedId, setDraggedId] = useState(null); + const [dragOverId, setDragOverId] = useState(null); + const [hiddenSystemItemIds, setHiddenSystemItemIds] = useState(getHiddenSidebarSystemItemIds); + const [sidebarOrder, setSidebarOrder] = useState(() => getSidebarOrder(defaultNavItems.map(i => i.id))); - // Fetch external links const { data: links, isLoading } = useQuery({ queryKey: ['external-links'], queryFn: api.getExternalLinks, }); - // Delete mutation const deleteMutation = useMutation({ mutationFn: (id: number) => api.deleteExternalLink(id), onSuccess: () => { @@ -32,41 +49,105 @@ export function ExternalLinksSettings() { }, }); - // Reorder mutation - const reorderMutation = useMutation({ - mutationFn: (ids: number[]) => api.reorderExternalLinks(ids), - onSuccess: () => { - queryClient.invalidateQueries({ queryKey: ['external-links'] }); + const { data: settings } = useQuery({ + queryKey: ['settings'], + queryFn: api.getSettings, + enabled: authEnabled && hasPermission('settings:update'), + }); + + const updateDefaultSidebarMutation = useMutation({ + mutationFn: (defaultSidebarOrder: string) => api.updateSettings({ default_sidebar_order: defaultSidebarOrder }), + onSuccess: (_data, defaultSidebarOrder) => { + queryClient.invalidateQueries({ queryKey: ['settings'] }); + queryClient.invalidateQueries({ queryKey: ['default-sidebar-order'] }); + showToast( + defaultSidebarOrder + ? t('settings.sidebarDefaultSet') + : t('settings.sidebarDefaultCleared'), + 'success' + ); + }, + onError: () => { + showToast(t('settings.sidebarDefaultFailed'), 'error'); }, }); - const handleDragStart = (e: React.DragEvent, id: number) => { + const layoutItems = useMemo(() => { + const navItemsMap = new Map(defaultNavItems.map(item => [item.id, item])); + const externalLinksMap = new Map((links || []).map(link => [`ext-${link.id}`, link])); + const result: SidebarLayoutItem[] = []; + const seen = new Set(); + + const addItem = (id: string) => { + if (seen.has(id)) return; + + const navItem = navItemsMap.get(id); + if (navItem) { + result.push({ type: 'system', id, navItem }); + seen.add(id); + return; + } + + const link = externalLinksMap.get(id); + if (link) { + result.push({ type: 'external', id, link }); + seen.add(id); + } + }; + + sidebarOrder.forEach(addItem); + defaultNavItems.forEach(item => addItem(item.id)); + (links || []).forEach(link => addItem(`ext-${link.id}`)); + + return result; + }, [links, sidebarOrder]); + + const persistSidebarOrder = (order: string[]) => { + setSidebarOrder(order); + saveSidebarOrder(order); + }; + + const handleDragStart = (e: React.DragEvent, id: string) => { setDraggedId(id); e.dataTransfer.effectAllowed = 'move'; + e.dataTransfer.setData('text/plain', id); }; - const handleDragOver = (e: React.DragEvent) => { + const handleDragOver = (e: React.DragEvent, id: string) => { e.preventDefault(); e.dataTransfer.dropEffect = 'move'; + setDragOverId(id); }; - const handleDrop = (e: React.DragEvent, targetId: number) => { + const handleDrop = (e: React.DragEvent, targetId: string) => { e.preventDefault(); - if (draggedId === null || draggedId === targetId || !links) return; + if (draggedId === null || draggedId === targetId) { + setDraggedId(null); + setDragOverId(null); + return; + } - const currentIds = links.map((l) => l.id); - const draggedIndex = currentIds.indexOf(draggedId); - const targetIndex = currentIds.indexOf(targetId); + const currentOrder = layoutItems.map(item => item.id); + const draggedIndex = currentOrder.indexOf(draggedId); + const targetIndex = currentOrder.indexOf(targetId); - if (draggedIndex === -1 || targetIndex === -1) return; + if (draggedIndex === -1 || targetIndex === -1) { + setDraggedId(null); + setDragOverId(null); + return; + } - // Reorder - const newIds = [...currentIds]; - newIds.splice(draggedIndex, 1); - newIds.splice(targetIndex, 0, draggedId); + currentOrder.splice(draggedIndex, 1); + currentOrder.splice(targetIndex, 0, draggedId); + persistSidebarOrder(currentOrder); - reorderMutation.mutate(newIds); setDraggedId(null); + setDragOverId(null); + }; + + const handleDragEnd = () => { + setDraggedId(null); + setDragOverId(null); }; const handleDelete = (link: ExternalLink) => { @@ -80,48 +161,163 @@ export function ExternalLinksSettings() { } }; + const resetSidebarLayout = () => { + const resetOrder = [ + ...defaultNavItems.map(item => item.id), + ...(links || []).map(link => `ext-${link.id}`), + ]; + + setHiddenSystemItemIds([]); + setSidebarOrder(resetOrder); + saveHiddenSidebarSystemItemIds([]); + saveSidebarOrder(resetOrder); + }; + + const handleToggleDefaultSidebarOrder = (enabled: boolean) => { + const currentOrder = layoutItems.map(item => item.id); + updateDefaultSidebarMutation.mutate(enabled ? JSON.stringify({ + order: currentOrder, + hiddenSystemItemIds, + }) : ''); + }; + + const toggleSystemItemVisibility = (id: string) => { + if (id === 'settings') return; + + const isHidden = hiddenSystemItemIds.includes(id); + const nextIds = isHidden + ? hiddenSystemItemIds.filter(hiddenId => hiddenId !== id) + : [...hiddenSystemItemIds, id]; + + setHiddenSystemItemIds(nextIds); + saveHiddenSidebarSystemItemIds(nextIds); + }; + + const canSetDefaultSidebarOrder = authEnabled && hasPermission('settings:update'); + const isDefaultSidebarEnabled = !!settings?.default_sidebar_order; + return ( <> -
+
-

Sidebar Links

+

{t('externalLinks.sidebarLayout')}

+
+
+ {canSetDefaultSidebarOrder && ( + + )} + +
-

- Add external links to the sidebar navigation. Drag to reorder. + {t('externalLinks.sidebarLayoutDescription')}

{isLoading ? (
- ) : links && links.length > 0 ? ( + ) : (
- {links.map((link) => { + {layoutItems.map((item) => { + const isHidden = item.type === 'system' && hiddenSystemItemIds.includes(item.id); + const isSettings = item.id === 'settings'; + + if (item.type === 'system') { + const Icon = item.navItem.icon; + return ( +
handleDragStart(e, item.id)} + onDragOver={(e) => handleDragOver(e, item.id)} + onDragLeave={() => setDragOverId(null)} + onDrop={(e) => handleDrop(e, item.id)} + onDragEnd={handleDragEnd} + className={`relative flex items-center gap-3 p-3 rounded-lg bg-bambu-dark border border-bambu-dark-tertiary transition-colors ${ + draggedId === item.id ? 'opacity-50' : isHidden ? 'opacity-60' : '' + } ${ + dragOverId === item.id && draggedId !== item.id + ? 'before:absolute before:left-3 before:right-3 before:top-0 before:h-0.5 before:bg-bambu-green' + : '' + }`} + > + +
+ +
+
+ {t(item.navItem.labelKey)} + + {isSettings + ? t('externalLinks.requiredInSidebar') + : isHidden + ? t('externalLinks.hiddenFromSidebar') + : t('externalLinks.visibleInSidebar')} + +
+ +
+ ); + } + + const link = item.link; const Icon = getIconByName(link.icon); return (
handleDragStart(e, link.id)} - onDragOver={handleDragOver} - onDrop={(e) => handleDrop(e, link.id)} - className={`flex items-center gap-3 p-3 rounded-lg bg-bambu-dark border border-bambu-dark-tertiary transition-colors ${ - draggedId === link.id ? 'opacity-50' : '' + onDragStart={(e) => handleDragStart(e, item.id)} + onDragOver={(e) => handleDragOver(e, item.id)} + onDragLeave={() => setDragOverId(null)} + onDrop={(e) => handleDrop(e, item.id)} + onDragEnd={handleDragEnd} + className={`relative flex items-center gap-3 p-3 rounded-lg bg-bambu-dark border border-bambu-dark-tertiary transition-colors ${ + draggedId === item.id ? 'opacity-50' : '' + } ${ + dragOverId === item.id && draggedId !== item.id + ? 'before:absolute before:left-3 before:right-3 before:top-0 before:h-0.5 before:bg-bambu-green' + : '' }`} >
- + {link.custom_icon ? ( + + ) : ( + + )}
@@ -151,17 +347,10 @@ export function ExternalLinksSettings() { ); })}
- ) : ( -
- -

{t('externalLinks.noLinksConfigured')}

-

Click "Add Link" to add one

-
)} - {/* Add/Edit Modal */} {(showAddModal || editingLink) && ( )} - {/* Delete Confirmation Modal */} {deletingLink && ( i.id); - } - } - return defaultNavItems.map(i => i.id); -} - -// Save unified sidebar order to localStorage -function saveSidebarOrder(order: string[]) { - localStorage.setItem('sidebarOrder', JSON.stringify(order)); -} - -// Check if an ID is an external link -function isExternalLinkId(id: string): boolean { - return id.startsWith('ext-'); -} - // Get default view from localStorage export function getDefaultView(): string { return localStorage.getItem('defaultView') || '/'; @@ -103,9 +86,9 @@ export function Layout() { const [mobileDrawerOpen, setMobileDrawerOpen] = useState(false); const [showShortcuts, setShowShortcuts] = useState(false); const [showSwitchbar, setShowSwitchbar] = useState(false); - const [sidebarOrder, setSidebarOrder] = useState(getSidebarOrder); - const [draggedId, setDraggedId] = useState(null); - const [dragOverId, setDragOverId] = useState(null); + const defaultSidebarOrder = useMemo(() => defaultNavItems.map(i => i.id), []); + const [sidebarOrder, setSidebarOrder] = useState(() => getSidebarOrder(defaultNavItems.map(i => i.id))); + const [hiddenSystemItemIds, setHiddenSystemItemIds] = useState(getHiddenSidebarSystemItemIds); const hasRedirected = useRef(false); const [dismissedUpdateVersion, setDismissedUpdateVersion] = useState(() => sessionStorage.getItem('dismissedUpdateVersion') @@ -151,10 +134,16 @@ export function Layout() { if (!Array.isArray(orderArr) || orderArr.length === 0) return; // Filter to valid sidebar item IDs only const validIds = new Set(defaultNavItems.map(i => i.id)); - const filtered = orderArr.filter((id: string) => typeof id === 'string' && (validIds.has(id) || isExternalLinkId(id))); + const filtered = orderArr.filter((id: string) => typeof id === 'string' && (validIds.has(id) || isExternalSidebarItemId(id))); if (filtered.length > 0) { setSidebarOrder(filtered); saveSidebarOrder(filtered); + const hiddenIds = Array.isArray(parsed) ? [] : parsed.hiddenSystemItemIds; + if (Array.isArray(hiddenIds)) { + const filteredHiddenIds = hiddenIds.filter((id: string) => typeof id === 'string' && validIds.has(id) && id !== 'settings'); + setHiddenSystemItemIds(filteredHiddenIds); + saveHiddenSidebarSystemItemIds(filteredHiddenIds); + } localStorage.setItem(appliedKey, '1'); } } catch (e) { @@ -162,7 +151,8 @@ export function Layout() { } }, [defaultSidebarData?.default_sidebar_order, setSidebarOrder, user, authEnabled]); - // Check advanced auth status for conditional nav items + // Check advanced auth status — the notifications nav item is gated on it + // (rendered only when authEnabled && advanced_auth_enabled && user_notifications_enabled). const { data: advancedAuthStatus } = useQuery({ queryKey: ['advancedAuthStatus'], queryFn: api.getAdvancedAuthStatus, @@ -296,10 +286,14 @@ export function Layout() { files: ['library:read', 'library:read_own', 'library:read_all'], makerworld: 'makerworld:view', settings: 'settings:read', - notifications: 'notifications:user_email', }; const isHidden = (id: string) => { + // User-toggled hide (#1673) wins first — cheapest check, explicit intent. + if (hiddenSystemItemIds.includes(id)) return true; + // Permission gate accepts Permission | Permission[] so resources with + // granular `*:read_own` / `*:read_all` tiers (default Operators group) + // don't get hidden from users who only hold the granular variant (#1755). if (authEnabled && id in navPermissions) { const required = navPermissions[id]; const granted = Array.isArray(required) @@ -342,58 +336,6 @@ export function Layout() { return result; })(); - // Unified drag handlers - const handleDragStart = (e: React.DragEvent, id: string) => { - setDraggedId(id); - e.dataTransfer.effectAllowed = 'move'; - e.dataTransfer.setData('text/plain', id); - }; - - const handleDragOver = (e: React.DragEvent, id: string) => { - e.preventDefault(); - e.dataTransfer.dropEffect = 'move'; - setDragOverId(id); - }; - - const handleDragLeave = () => { - setDragOverId(null); - }; - - const handleDrop = (e: React.DragEvent, targetId: string) => { - e.preventDefault(); - if (draggedId === null || draggedId === targetId) { - setDraggedId(null); - setDragOverId(null); - return; - } - - const currentOrder = [...orderedSidebarIds]; - const draggedIndex = currentOrder.indexOf(draggedId); - const targetIndex = currentOrder.indexOf(targetId); - - if (draggedIndex === -1 || targetIndex === -1) { - setDraggedId(null); - setDragOverId(null); - return; - } - - // Reorder - currentOrder.splice(draggedIndex, 1); - currentOrder.splice(targetIndex, 0, draggedId); - - // Save to localStorage and update state - setSidebarOrder(currentOrder); - saveSidebarOrder(currentOrder); - - setDraggedId(null); - setDragOverId(null); - }; - - const handleDragEnd = () => { - setDraggedId(null); - setDragOverId(null); - }; - // Show update banner if update available and not dismissed for this version. // Suppressed when running as a Home Assistant addon — HA Supervisor surfaces // its own update notification in the HA UI, so the in-app banner is duplicate @@ -425,6 +367,19 @@ export function Layout() { localStorage.setItem('sidebarExpanded', String(sidebarExpanded)); }, [sidebarExpanded]); + useEffect(() => { + const refreshSidebarLayout = () => { + setSidebarOrder(getSidebarOrder(defaultSidebarOrder)); + setHiddenSystemItemIds(getHiddenSidebarSystemItemIds()); + }; + window.addEventListener(SIDEBAR_LAYOUT_CHANGED_EVENT, refreshSidebarLayout); + window.addEventListener('storage', refreshSidebarLayout); + return () => { + window.removeEventListener(SIDEBAR_LAYOUT_CHANGED_EVENT, refreshSidebarLayout); + window.removeEventListener('storage', refreshSidebarLayout); + }; + }, [defaultSidebarOrder]); + // Close compact drawer on navigation useEffect(() => { if (isSidebarCompact) { @@ -466,7 +421,7 @@ export function Layout() { const id = orderedSidebarIds[keyNum - 1]; e.preventDefault(); - if (isExternalLinkId(id)) { + if (isExternalSidebarItemId(id)) { // External link const extLink = extLinksMap.get(id); if (extLink?.open_in_new_tab) { @@ -551,7 +506,7 @@ export function Layout() {