diff --git a/CHANGELOG.md b/CHANGELOG.md index 49c995fba..009bc9430 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ All notable changes to Bambuddy will be documented in this file. ## [0.2.5b1] - Unreleased ### Fixed +- **Print Log "User" column now shows the user for prints started from the Queue (#1670, reported by @JmanB52D)** — Reporter on a P2S with auth enabled, Virtual Printer in Queue mode and Auto-dispatch off: a user uploads a `.3mf` to the VP (FTP, anonymous), then logs into Bambuddy and clicks ▶ on the staged queue item to start it; the print finishes and the PrintLogEntry's User column is blank. Same setup with the VP in Archive (slicer-initiated) mode correctly attributes the user. **Root cause: two-link gap on the Queue→manual-start dispatch path.** (a) `POST /queue/{id}/start` (`print_queue.py:1039`) auth-protected, but the route's user dep was bound to `_` and discarded — the clicker was never recorded. (b) `PrintScheduler._start_print` (`print_scheduler.py:1886`) dispatches the queue item directly and never calls `printer_manager.set_current_print_user(...)`. The print-complete callback (`main.py:3513`) reads `_print_user_info = printer_manager.get_current_print_user(printer_id)` — which is only ever populated by `background_dispatch.py:747/943` (the Archive→Print and Library→Print flows). Queue dispatch had no equivalent hop, so `_print_user_info` was always `None` and the PrintLogEntry's `created_by_username` landed `NULL`. **Fix (two-sided):** (1) `print_queue.py /start` now binds the auth dep to `user: User | None` and writes `item.created_by_id = user.id` when `user is not None AND item.created_by_id is None` — credits the clicker on VP-uploaded (unattributed) items without overwriting existing attribution from UI-added queue items (matches the standard "first claim wins" ownership rule in `auth.py::require_ownership_permission`). (2) `print_scheduler.py` gains a small `_propagate_owner_to_printer_manager` helper, called from `_start_print` immediately after `register_expected_print`: when `item.created_by_id` resolves to a real User row, it forwards `(printer_id, owner.id, owner.username)` into `printer_manager.set_current_print_user`. No-ops cleanly when the item has no owner (auto-dispatched VP items intrinsically) or when the user row is missing (e.g. user deleted between queue-add and dispatch — the print log row falls back to un-credited rather than crashing the dispatch). **Tests:** 6 new in `test_queue_start_user_attribution.py` — three route tests pin (a) authenticated `/start` writes `created_by_id` on an unattributed item, (b) an existing owner is preserved when a different user clicks `/start`, (c) auth-disabled leaves `created_by_id=NULL` (no synthetic placeholder user invented); three helper tests pin (d) the propagation forwards the resolved username into `set_current_print_user`, (e) a `None` owner is silently skipped, (f) a missing User row is silently skipped instead of raising. Full 63-test `test_print_queue_api.py` suite stays green. Backend ruff clean. + - **AMS drying popover's "Start Drying" button is no longer hidden behind iOS Safari's bottom URL bar on iPhone (#1669, reported via in-app bug report, iPhone 17 Safari)** — Reporter could see the temperature / duration sliders and the "Rotate spool during drying" checkbox but couldn't reach the orange Start Drying button at the bottom of the popover — only a thin sliver of it was visible just above Safari's URL bar. **Root cause:** the popover sizes its `maxHeight` against CSS `100vh` (`PrintersPage.tsx:5443`) and positions itself using `window.innerHeight` (via `computePopoverPosition`, `popoverPosition.ts:53`). On iOS Safari both of those report the **layout** viewport — the full screen ignoring the bottom URL/toolbar overlay — not the visual viewport. The popover therefore extends *behind* Safari's bottom toolbar and the footer button gets clipped. Earlier iterations of the same surface (#1447 popover-off-bottom, #1458 footer-scroll-reachability) fixed desktop / normal-viewport cases but assumed `100vh` matched the visible viewport. **Fix:** two-line change. (a) `frontend/src/pages/PrintersPage.tsx:5443` switches `maxHeight: calc(100vh - …)` → `calc(100dvh - …)` so the dynamic viewport units shrink with iOS toolbars. (b) `frontend/src/utils/popoverPosition.ts:53` defaults `viewportHeight` from `window.visualViewport?.height ?? window.innerHeight` so the flip-above decision also uses the actually-visible area; the existing optional override still wins (tests keep their explicit viewport values). Result: when the iOS toolbar is up, either the popover flips above the trigger earlier (visualViewport too short for below-placement), or the body scrolls within a capped maxHeight and the `shrink-0` footer stays pinned to the visible bottom — the Start Drying button is reachable in both cases. **Tests:** 3 new in `popoverPosition.test.ts::computePopoverPosition (#1669)` — flip-above triggers when visualViewport.height (700) is shorter than innerHeight (800) and the trigger position would only overflow under the visual viewport; falls back to innerHeight when visualViewport is unavailable (older WebViews / jsdom); an explicit `viewportHeight` override still wins over a configured visualViewport.height (test-injection contract). 8 pre-existing tests stay green. dvh / svh browser support — Safari 15.4+, Chrome 108+, Firefox 101+ — comfortably covers iPhone 17 Safari and every supported desktop browser; no behavioural change on non-iOS. - **Print queue `require_previous_success` no longer cascades indefinitely after a user-cancelled print (#1667, fully root-caused by @599w6c26tv-droid)** — Reporter on an A1 saw a single user-cancelled print block 18 downstream queue items over 3 days, all marked `skipped` with `Previous print failed or was aborted`. They captured the override log line proving Bambuddy correctly detects the cancellation (`Overriding status 'failed' -> 'cancelled' for printer 1 (print was stopped from queue by user)`) but the scheduler's gate ignored the override; they dumped the affected DB rows confirming the cascade pattern; and they reproduced from clean state in one cycle. Two distinct bugs in one function (`PrintScheduler._check_previous_success` in `services/print_scheduler.py`): **(a)** The lookback query `.in_(["completed", "failed", "skipped", "aborted"])` excluded `cancelled`, so a user cancellation was never found as the most-recent predecessor — the query walked past it to whatever real outcome existed before. **(b)** The same lookback INCLUDED `skipped`, so once one item got skipped (under any reason — bug-cascaded or genuinely failure-gated) it became the next item's "failed predecessor" and the cascade compounded. **Fix:** swap the lookback list to `["completed", "failed", "cancelled", "aborted"]` and broaden the success check to `prev_item.status in ("completed", "cancelled")`. A user cancellation is a deliberate action — treating it as neutral matches the user's intent ("I'm done with that one, move on"); `skipped` is excluded so the query always walks back to the most recent REAL print attempt and `failed` / `aborted` still gate as before. **Conservative recovery migration**: a one-shot pass in `core/database.py::run_migrations` resets only the skipped items whose immediate real predecessor (by `completed_at` desc, excluding the skipped-cascade itself) was `cancelled` — same fingerprint as the bug, narrow enough not to disturb skipped items whose true predecessor was a real `failed` / `aborted` print. Items match on `status='skipped' AND error_message='Previous print failed or was aborted'` and the predecessor check via correlated subquery; logged per-row at INFO so operators can audit the count after upgrade. Portable across SQLite and Postgres. Idempotent (post-reset rows no longer match). **Tests**: 10 new behaviour tests in `test_check_previous_success.py` pin every status/cascade combination — bug A (cancelled → True), bug B (skipped walked past), the reporter's exact failed→cancelled→skipped→skipped→pending cascade, regression guards on real failed / aborted still gating, edge cases (no-predecessor, only-skipped history, completed-then-failed). 7 new tests in `test_cancellation_cascade_recovery_migration.py` pin the migration — skipped-after-cancelled resets, skipped-after-failed stays, skipped-after-aborted stays, different-error-message untouched, reporter's multi-item cascade resets all, idempotent on re-run, per-printer isolation. All green; full scheduler + migration test suite stays green. diff --git a/backend/app/api/routes/print_queue.py b/backend/app/api/routes/print_queue.py index c26b8169f..d8a72debe 100644 --- a/backend/app/api/routes/print_queue.py +++ b/backend/app/api/routes/print_queue.py @@ -1041,7 +1041,7 @@ async def start_queue_item( item_id: int, skip_filament_check: bool = Query(default=False), db: AsyncSession = Depends(get_db), - _: User | None = RequirePermissionIfAuthEnabled(Permission.QUEUE_UPDATE_OWN), + user: User | None = RequirePermissionIfAuthEnabled(Permission.QUEUE_UPDATE_OWN), ): """Manually start a staged (manual_start) queue item. @@ -1086,6 +1086,14 @@ async def start_queue_item( # Print Anyway / no deficit: clear the flags and let the scheduler dispatch. item.manual_start = False item.filament_short = False + # Credit the clicker as the item's owner when no prior owner is set — + # VP-uploaded queue items arrive over FTP unattributed, so without this + # the print log's User column stays blank even when auth is on + # (#1670). An item that already has a creator (UI-added queue items) + # keeps that attribution; the dispatcher is not promoted over the + # original uploader. + if user is not None and item.created_by_id is None: + item.created_by_id = user.id await db.commit() await db.refresh(item, ["archive", "printer", "library_file", "created_by", "batch"]) diff --git a/backend/app/services/print_scheduler.py b/backend/app/services/print_scheduler.py index e79bbf27c..6b7e61f7e 100644 --- a/backend/app/services/print_scheduler.py +++ b/backend/app/services/print_scheduler.py @@ -1883,6 +1883,23 @@ class PrintScheduler: await db.commit() return False + async def _propagate_owner_to_printer_manager(self, db: AsyncSession, item: PrintQueueItem) -> None: + """Hand the queue item's owner to printer_manager so the + print-complete callback can credit the user in PrintLogEntry (#1670). + + No-ops when the item has no `created_by_id` or the referenced user + row is missing (e.g. user deleted between queue-add and dispatch — + in that case the print log row falls back to the existing un-credited + behaviour rather than crashing the dispatch). + """ + if not item.created_by_id: + return + from backend.app.models.user import User + + owner = await db.get(User, item.created_by_id) + if owner: + printer_manager.set_current_print_user(item.printer_id, owner.id, owner.username) + async def _start_print(self, db: AsyncSession, item: PrintQueueItem): """Upload file and start print for a queue item. @@ -2128,6 +2145,16 @@ class PrintScheduler: created_by_id=item.created_by_id, ) + # Propagate the queue item's owner into printer_manager so the + # print-complete callback can credit the user in the PrintLogEntry + # (#1670). The dispatch path in `background_dispatch.py` does the + # equivalent for archive/library "Print" flows; the queue path was + # missing this hop, which left the print log's User column blank + # for any print started from the queue. `created_by_id` is set + # either at queue-add time (UI-added items) or when the user + # clicks the manual-start button (#1670 fix in print_queue.py). + await self._propagate_owner_to_printer_manager(db, item) + # IMPORTANT: Set status to "printing" BEFORE sending the print command. # This prevents phantom reprints if the backend crashes/restarts after the # print command is sent but before the status update is committed. diff --git a/backend/tests/integration/test_queue_start_user_attribution.py b/backend/tests/integration/test_queue_start_user_attribution.py new file mode 100644 index 000000000..4d0204ac1 --- /dev/null +++ b/backend/tests/integration/test_queue_start_user_attribution.py @@ -0,0 +1,247 @@ +"""Regression tests for #1670: queue manual-start path lost user attribution. + +Before the fix, a VP-uploaded queue item (created over FTP, so unattributed) +that was then started by an authenticated user via the `/start` button would +land in the PrintLogEntry table with `created_by_username = NULL` because +the scheduler dispatch path never set `current_print_user` and the `/start` +route didn't record the clicker. + +The fix is two-sided: + - `POST /queue/{id}/start` credits the clicker as `created_by_id` when + no prior owner is set (does NOT overwrite an existing owner — a + UI-added queue item's original uploader keeps attribution). + - `PrintScheduler._start_print` propagates `item.created_by_id` into + `printer_manager.set_current_print_user` so the print-complete callback + can write the username into the PrintLogEntry row. + +These tests pin both halves so a future refactor can't silently regress +either one back to "blank User column." +""" + +from __future__ import annotations + +import pytest +from httpx import AsyncClient +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from backend.app.models.print_queue import PrintQueueItem + + +async def _read_item(test_engine, item_id: int) -> PrintQueueItem: + """Fresh-session DB read. The `db_session` fixture's connection can + look stale after a route call dispatches through its own session via + `Depends(get_db)`, so verification reads use a new session against the + same engine.""" + maker = async_sessionmaker(test_engine, class_=AsyncSession, expire_on_commit=False) + async with maker() as fresh: + return (await fresh.execute(select(PrintQueueItem).where(PrintQueueItem.id == item_id))).scalar_one() + + +async def _enable_auth_with_admin(async_client: AsyncClient) -> tuple[str, dict]: + """Boot the app's auth setup and return (admin_token, admin_user).""" + await async_client.post( + "/api/v1/auth/setup", + json={ + "auth_enabled": True, + "admin_username": "queue1670admin", + "admin_password": "AdminPass1!", + }, + ) + login = await async_client.post( + "/api/v1/auth/login", + json={"username": "queue1670admin", "password": "AdminPass1!"}, + ) + body = login.json() + return body["access_token"], body["user"] + + +@pytest.fixture +async def queue_item(db_session): + """A pending, manual-start, UNATTRIBUTED queue item — mirrors what the + VP-queue path produces (FTP upload has no user, manual_start is the + Queue-mode default).""" + from backend.app.models.archive import PrintArchive + from backend.app.models.printer import Printer + + printer = Printer( + name="P2S Test", + ip_address="192.168.2.201", + serial_number="00M00A1234567890", + access_code="12345678", + model="P2S", + ) + db_session.add(printer) + await db_session.commit() + await db_session.refresh(printer) + + archive = PrintArchive( + filename="Plate_1.gcode.3mf", + print_name="Plate 1", + file_path="/tmp/queue1670_plate.3mf", + file_size=1024, + content_hash="queue1670hash", + status="completed", + ) + db_session.add(archive) + await db_session.commit() + await db_session.refresh(archive) + + item = PrintQueueItem( + printer_id=printer.id, + archive_id=archive.id, + status="pending", + position=1, + manual_start=True, + created_by_id=None, # unattributed — VP-queue shape + ) + db_session.add(item) + await db_session.commit() + await db_session.refresh(item) + return item + + +class TestStartCreditsTheClicker: + """`/start` writes the clicker's id to `created_by_id` when none was set.""" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_start_writes_created_by_id_when_unattributed( + self, async_client: AsyncClient, test_engine, queue_item + ): + admin_token, admin_user = await _enable_auth_with_admin(async_client) + + response = await async_client.post( + f"/api/v1/queue/{queue_item.id}/start", + headers={"Authorization": f"Bearer {admin_token}"}, + ) + assert response.status_code == 200 + + refreshed = await _read_item(test_engine, queue_item.id) + assert refreshed.created_by_id == admin_user["id"] + assert refreshed.manual_start is False + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_start_preserves_existing_owner(self, async_client: AsyncClient, db_session, test_engine, queue_item): + """A queue item that was created by user A and then started by user B + keeps user A's attribution — the original uploader's claim is stronger + than the dispatcher's. (Matches the standard ownership semantics in + `auth.py::require_ownership_permission`.)""" + admin_token, admin_user = await _enable_auth_with_admin(async_client) + + # Pre-set a different owner on the queue item using a fresh session + # (the test's `db_session` is detached from the route's session pool). + prior_owner_id = admin_user["id"] + 9999 + maker = async_sessionmaker(test_engine, class_=AsyncSession, expire_on_commit=False) + async with maker() as fresh: + item = (await fresh.execute(select(PrintQueueItem).where(PrintQueueItem.id == queue_item.id))).scalar_one() + item.created_by_id = prior_owner_id + await fresh.commit() + + response = await async_client.post( + f"/api/v1/queue/{queue_item.id}/start", + headers={"Authorization": f"Bearer {admin_token}"}, + ) + assert response.status_code == 200 + + refreshed = await _read_item(test_engine, queue_item.id) + # Prior owner survives — `/start` did not promote the clicker. + assert refreshed.created_by_id == prior_owner_id + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_start_with_auth_disabled_leaves_created_by_id_null( + self, async_client: AsyncClient, test_engine, queue_item + ): + """When auth is off the route's user dep returns None — the item stays + unattributed (no synthetic 'system' user invented). Regression guard + in case a future refactor accidentally invents a placeholder user id.""" + response = await async_client.post(f"/api/v1/queue/{queue_item.id}/start") + assert response.status_code == 200 + + refreshed = await _read_item(test_engine, queue_item.id) + assert refreshed.created_by_id is None + + +class TestSchedulerPropagatesOwnerToPrinterManager: + """`PrintScheduler._propagate_owner_to_printer_manager` looks up the + user row by `created_by_id` and forwards it into + `printer_manager.set_current_print_user` so the print-complete callback + can write the username into PrintLogEntry.""" + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_propagates_when_created_by_id_resolves_to_user(self, db_session, queue_item, monkeypatch): + from backend.app.models.user import User + from backend.app.services import print_scheduler as scheduler_module + from backend.app.services.print_scheduler import PrintScheduler + + user = User(username="clickeruser", password_hash="x", is_active=True) + db_session.add(user) + await db_session.commit() + await db_session.refresh(user) + + queue_item.created_by_id = user.id + db_session.add(queue_item) + await db_session.commit() + await db_session.refresh(queue_item) + + captured: list[tuple[int, int, str]] = [] + monkeypatch.setattr( + scheduler_module.printer_manager, + "set_current_print_user", + lambda printer_id, uid, username: captured.append((printer_id, uid, username)), + ) + + await PrintScheduler()._propagate_owner_to_printer_manager(db_session, queue_item) + + assert captured == [(queue_item.printer_id, user.id, "clickeruser")] + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_noop_when_created_by_id_is_none(self, db_session, queue_item, monkeypatch): + """VP-uploaded queue items that never got manual-started (e.g. + auto-dispatch) carry no owner — the helper must stay silent rather + than synthesise a placeholder user.""" + from backend.app.services import print_scheduler as scheduler_module + from backend.app.services.print_scheduler import PrintScheduler + + assert queue_item.created_by_id is None + + captured: list = [] + monkeypatch.setattr( + scheduler_module.printer_manager, + "set_current_print_user", + lambda *args: captured.append(args), + ) + + await PrintScheduler()._propagate_owner_to_printer_manager(db_session, queue_item) + assert captured == [] + + @pytest.mark.asyncio + @pytest.mark.integration + async def test_noop_when_user_row_missing(self, db_session, queue_item, monkeypatch): + """`created_by_id` points at a user that's since been deleted — + helper must not crash the dispatch. The print log row will just be + un-credited for this run, same as auth-disabled.""" + from backend.app.services import print_scheduler as scheduler_module + from backend.app.services.print_scheduler import PrintScheduler + + queue_item.created_by_id = 999_999 # no such user row + db_session.add(queue_item) + await db_session.commit() + await db_session.refresh(queue_item) + + captured: list = [] + monkeypatch.setattr( + scheduler_module.printer_manager, + "set_current_print_user", + lambda *args: captured.append(args), + ) + + # Must not raise — the dispatch loop would otherwise lose the whole + # queue item to an exception trace for what's effectively a missing + # foreign key. + await PrintScheduler()._propagate_owner_to_printer_manager(db_session, queue_item) + assert captured == []