Slideshow management fixes: delete inactive items, duplicate slideshows - #23
Merged
Conversation
…ent. Documents the root-cause analysis and a three-milestone implementation plan for the two slideshow management fixes: - Milestone 1: Allow deletion of inactive slideshow items. The DELETE /api/v1/slideshow-items/<id> endpoint currently returns 404 for inactive items; the fix makes deletion two-stage (active items soft-delete as today, inactive items are hard-deleted), beginning with initially-failing regression tests. - Milestone 2: Add a duplicate-slideshow capability via a new POST /api/v1/slideshows/<id>/duplicate endpoint and a Duplicate action on the Slideshows admin page, copying all items (including inactive) and physically copying uploaded media files into the new slideshow's upload directory. - Milestone 3: Acceptance criteria (documentation, test coverage, full nox pass, minor version bump, PR). Also records the human-approved design decisions: hard delete for inactive items, prompt for the duplicate's name, copy media files on disk, and copy all items including inactive ones. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ctive item deletion.
Adds regression tests that document the desired two-stage deletion behavior
for slideshow items:
- tests/unit/test_api.py:
- test_delete_inactive_slideshow_item: deleting an inactive item should
return 200 and permanently remove the row from the database.
- test_delete_active_then_inactive_slideshow_item: first delete soft-deletes
(is_active=False, row remains), second delete permanently removes.
- tests/integration/test_slideshow_items.py:
- test_delete_inactive_item_permanently: soft-deletes an item via the API,
then deletes it from the inactive state via the admin UI and verifies the
row disappears from the item list.
All three tests currently fail because DELETE /api/v1/slideshow-items/<id>
returns 404 for inactive items ("if not item or not item.is_active" in
delete_slideshow_item). They will pass once the endpoint implements the
two-stage delete in task 1.2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… items via two-stage delete. Fixes the bug where inactive slideshow items could never be removed: the DELETE /api/v1/slideshow-items/<id> endpoint returned 404 for any item with is_active=False, while the admin UI displays inactive items with a Delete button, so deleting them always failed. The endpoint now implements a two-stage delete: - Deleting an active item soft-deletes it (sets is_active=False), preserving the existing behavior so items can still be reactivated via the edit form. - Deleting an already-inactive item permanently removes the row from the database via db.session.delete(). The item's title is captured before commit since a hard-deleted instance's attributes are inaccessible after the commit; the SSE broadcast now uses the item_id route parameter for the same reason. The endpoint docstring documents the two-stage behavior. The regression tests added in task 1.1 now pass; full unit suite passes (671 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…is permanent. Since deletion is now two-stage (active items are soft-deleted, inactive items are permanently removed), the admin UI's delete confirmation should distinguish the two cases. SlideshowDetail's handleDeleteItem now takes the item's is_active flag and shows "Are you sure you want to permanently delete ... This cannot be undone." for inactive items, while keeping the original confirmation message for active items. Adds a frontend unit test covering the permanent-delete confirmation message and DELETE call for an inactive item. Full frontend suite passes (133 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…py at Python 3.14. Side quest from the Slideshow Management Fixes feature: running nox -s format with a freshly created environment installed black 26.5.1, which (with our target-version of py314) rewrites "except (X, Y):" to the PEP 758 style "except X, Y:" and hugs multiline string arguments. The previous safeguard of excluding black 26.1.0 in noxfile.py no longer protects against this, and mypy rejected the new except syntax because mypy.ini set python_version to 3.13. Per human decision, embrace the new black style rather than pinning an aging formatter version: - mypy.ini: python_version 3.13 -> 3.14, matching the project's Python version and the (previously shadowed) [tool.mypy] setting in pyproject.toml, so mypy accepts PEP 758 except clauses. - noxfile.py: drop the now-moot black!=26.1.0 exclusion. - Apply black 26.5.1 formatting churn: PEP 758 except clause in ical_service.py and multiline-string hugging in two integration test files. nox -s format, lint, and type_check all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… document. Records Milestone 1 progress: regression tests (initially failing, now passing), the two-stage delete implementation in the API, the permanent-delete confirmation in the admin UI, the full test pass (671 unit, 173 integration, 133 frontend; format/lint/type_check clean), and the human-approved black 26.5.x / mypy 3.14 toolchain side quest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…eshow(). Adds a storage helper used by the upcoming duplicate-slideshow endpoint: copy_file_to_slideshow() copies an existing uploaded file (identified by its relative path as stored in SlideshowItem.content_file_path) into another slideshow's upload directory, generating a fresh secure filename, and returns the new relative path. This lets a duplicated slideshow own independent copies of media files rather than referencing the original slideshow's files, which live in a per-slideshow directory. Leading "/" and "uploads/" prefixes on the stored path are tolerated to match the lenient handling in SlideshowItem.display_url. A missing source file or copy failure logs a warning/error and returns None so callers can degrade gracefully instead of failing an entire duplicate operation. Adds unit tests covering the happy path, the uploads/-prefixed path form, and a missing source file. Full unit suite passes (674 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Implements Milestone 1 of the “Slideshow management fixes” feature by aligning backend delete semantics, frontend UX, and test coverage so inactive slideshow items can be permanently removed (while active items remain soft-deleted on first delete). Also updates the Python 3.14 toolchain configuration (black + mypy target version) and records the feature plan/progress in docs.
Changes:
- Backend:
DELETE /api/v1/slideshow-items/<id>now performs a two-stage delete (active → soft delete, inactive → hard delete) and preserves SSE/audit logging behavior. - Frontend: Delete confirmation messaging now warns when an inactive item will be permanently deleted; corresponding unit and Playwright tests added.
- Tooling/docs: remove stale black exclusion, set mypy to Python 3.14, and add feature specification/progress doc.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/unit/test_api.py |
Adds unit regression coverage for two-stage delete behavior (inactive hard delete + active-then-inactive flow). |
tests/integration/test_slideshow_items.py |
Adds Playwright coverage for permanently deleting inactive items; includes minor formatting adjustments to page.evaluate calls. |
tests/integration/test_display_playback.py |
Formatting-only adjustments to page.evaluate calls for consistency with updated formatter behavior. |
noxfile.py |
Updates formatter install step to use black without the prior !=26.1.0 exclusion. |
mypy.ini |
Targets mypy at Python 3.14 to match the project/tooling baseline. |
kiosk_show_replacement/ical_service.py |
Formatter-driven update to exception clause formatting compatible with the Python 3.14 target. |
kiosk_show_replacement/api/v1.py |
Implements two-stage deletion semantics for slideshow items and adjusts SSE payload/logging to work for hard deletes. |
frontend/src/test/SlideshowDetail.test.tsx |
Adds frontend unit test ensuring inactive-item delete uses the permanent-delete confirmation message. |
frontend/src/pages/SlideshowDetail.tsx |
Updates delete handler to vary confirmation text based on item active/inactive status. |
docs/features/slideshow-management-fixes.md |
Adds/updates feature spec and progress tracking for slideshow management fixes and upcoming duplication work. |
…licate endpoint. Adds a duplicate-slideshow API endpoint that creates a copy of a slideshow under a new caller-supplied name: - Validates the name exactly like create_slideshow (required, trimmed, globally unique among active slideshows, with the unique-constraint IntegrityError as backstop) and returns 404 for missing or inactive source slideshows. - Copies description, default_item_duration, and transition_type; the duplicate is owned by the current user and is never the default slideshow. - Copies all items, including inactive ones (which stay inactive), preserving title, content fields, display_duration, order_index, scale_factor, and iCal settings. - Uploaded media files are copied into the duplicate's upload directory via StorageManager.copy_file_to_slideshow() so the copy is independent of the original; a missing source file degrades to an item without a file reference rather than failing the duplicate. - The whole operation is one transaction; on failure, already-copied files are cleaned up best-effort and the database is rolled back. Adds nine unit tests covering settings/item copying (including inactive items and order preservation), the is_default flag not being copied, duplicate/missing name validation, 404s for missing and soft-deleted sources, authentication, on-disk file copying, and missing-file tolerance. Full unit suite passes (683 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ws admin page.
Adds a Duplicate button (copy icon) to each row's action group on
/admin/slideshows. Clicking it prompts for the new slideshow's name via
window.prompt() (matching the page's existing native-dialog pattern),
pre-filled with "{name} (Copy)" as a convenience. Cancelling the prompt is a
no-op; a blank name shows the page's error alert without calling the API;
API errors (e.g. duplicate name) are surfaced in the same alert. On success
the slideshow list is refreshed.
Supporting changes:
- apiClient.duplicateSlideshow(id, name) calling
POST /api/v1/slideshows/<id>/duplicate (no-retry, like other mutations).
- useApi hook routes the duplicate endpoint to the new apiClient method.
Adds four frontend unit tests: successful duplicate (prompt pre-fill and API
call), cancelled prompt (no API call), blank name (error alert, no API
call), and API failure (error alert). Full frontend suite passes
(137 passed); type-check and lint clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… slideshow duplication. Adds three browser integration tests for the Duplicate action on the Slideshows admin page: - test_duplicate_slideshow_success: creates a slideshow with one active and one soft-deleted item via the API, duplicates it through the UI (accepting the name prompt with a new name), verifies the duplicate appears in the list, and confirms via the API that both items were copied with their active/inactive states preserved. - test_duplicate_slideshow_cancel: dismissing the name prompt creates nothing. - test_duplicate_slideshow_duplicate_name_error: accepting the prompt with an existing slideshow name surfaces the "already exists" error alert. Full integration suite passes (176 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…in storage helper. nox -s format reflowed two expressions in the new duplicate API tests, and pycodestyle flagged E203 (whitespace before ':') on the black-formatted slice in copy_file_to_slideshow(); replace the startswith/slice pair with str.removeprefix(), which is clearer and sidesteps the black/pycodestyle conflict. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… document. Records Milestone 2 progress: the storage copy helper, the duplicate API endpoint, the admin UI Duplicate action, Playwright integration tests, and the full test pass (683 unit, 176 integration, 137 frontend; format/lint/type_check and frontend type-check/lint clean). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…elete and duplication. docs/api.rst: - Document the new POST /api/v1/slideshows/<id>/duplicate endpoint (request body, 201 response, side effects). - Update DELETE /api/v1/slideshow-items/<id> to describe the two-stage behavior (active items are soft-deleted; inactive items are permanently removed). docs/usage.rst: - Add a "Duplicating Slideshows" section describing the Duplicate action, independent file copies, and the default-slideshow flag not being copied. - Note in "Managing Slides" that removing a slide deactivates it and that deleting an inactive slide is permanent. - Add "duplicate" to the admin interface feature list. README.md and CLAUDE.md need no changes: neither enumerates per-action slideshow capabilities or API endpoints. Sphinx docs build succeeds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…error paths. Coverage review of the feature's new code found two duplicate-endpoint paths without tests: - test_duplicate_slideshow_no_data: a request without a JSON body returns 400. - test_duplicate_slideshow_name_conflict_with_inactive: an inactive slideshow's name passes the active-only duplicate-name check but violates the (name, owner_id) unique constraint at flush time, exercising the IntegrityError backstop. - test_duplicate_slideshow_cleanup_on_failure: failing the final commit after items are processed exercises the generic failure handler, verifying the response is 500, the copied media file is removed from disk, and no duplicate slideshow row is persisted. The only remaining uncovered line in the new code is the file-cleanup loop inside the IntegrityError handler, which cannot execute in practice because the constraint violation surfaces at flush (before any file copies); it is kept as defense-in-depth. Full unit suite passes (686 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…he feature. Final acceptance-criteria tasks for the Slideshow Management Fixes feature: - Minor version bump in pyproject.toml: 0.3.5 -> 0.4.0 (two-stage slideshow item deletion and the new duplicate-slideshow capability). - Move the feature document to docs/features/completed/ with Milestone 3 progress recorded. - Apply one black reflow in tests/unit/test_api.py from the final nox -s format run. Final verification, all passing: 686 backend unit tests, 176 Playwright integration tests, 137 frontend tests; nox format, lint, type_check, and docs sessions clean; frontend type-check, lint, and production build clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…al guard and test mock hygiene. Addresses two Copilot review comments on PR #23: 1. StorageManager.copy_file_to_slideshow() now resolves the source path and refuses to copy anything that resolves outside the upload folder. Since SlideshowItem.content_file_path is writable via the API, a crafted value like "images/../../etc/secret" could previously have copied arbitrary server files into the uploads directory (and thus exposed them via /uploads/). Adds a unit test verifying traversal attempts return None and copy nothing. 2. The frontend tests added by this branch no longer override window.confirm / window.prompt by direct assignment, which could leak into later tests if an assertion threw before the manual restore. They now use vi.spyOn(), and the SlideshowDetail and Slideshows suites gained an afterEach(() => vi.restoreAllMocks()) so spies are restored even on test failure. Pre-existing tests using the manual save/restore pattern are left unchanged. Backend: 687 unit tests pass. Frontend: 137 tests pass; type-check and lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nox -s format collapsed a list comprehension assertion in the new test_copy_file_to_slideshow_rejects_path_traversal onto one line. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…es to vi.spyOn. Follow-up to Copilot review feedback on PR #23: the previous commit added afterEach(() => vi.restoreAllMocks()) to the Slideshows and SlideshowDetail test suites, but each file still had one pre-existing test mocking window.confirm by direct assignment, which vi.restoreAllMocks() does not restore. If such a test failed before its manual cleanup line, the mock could leak into later tests and make the suites order-dependent. Convert those two remaining tests ('handles delete slideshow' and 'handles delete item') to vi.spyOn(window, 'confirm').mockReturnValue(true) and drop their manual save/restore, so every window.confirm/window.prompt mock in both suites is now a spy covered by the afterEach restore. Full frontend suite passes (137 passed); type-check and lint clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the
slideshow-management-fixesfeature (plan and progress indocs/features/completed/slideshow-management-fixes.md):DELETE /api/v1/slideshow-items/<id>previously returned 404 for inactiveitems, so an item that had been soft-deleted could never be removed even
though the admin UI offers a Delete button on inactive items. Deletion is
now two-stage: deleting an active item soft-deletes it (unchanged), and
deleting an already-inactive item permanently removes it. The admin UI
warns that deleting an inactive item is permanent.
POST /api/v1/slideshows/<id>/duplicateendpoint and a Duplicate action on the Slideshows admin page that prompts
for the copy's name (pre-filled "{name} (Copy)"). The duplicate copies all
items (including inactive ones, which stay inactive), physically copies
uploaded media files into the new slideshow's upload directory so the copy
is fully independent, is owned by the current user, and is never the
default slideshow.
Also includes a human-approved toolchain update: embrace black 26.5.x
formatting for Python 3.14 (PEP 758 except clauses) and target mypy at
Python 3.14 (
mypy.ini), replacing the staleblack!=26.1.0exclusion.Bumps version 0.3.5 → 0.4.0 and updates
docs/api.rst/docs/usage.rst.Test plan
type-check,lint, and production build cleannox -s format,lint,type_check,docsall passing🤖 Generated with Claude Code