#48: move images between markings and covers (both directions)

[reese8272]

What

Michael's ask: "You should also be able to move images between existing marking and cover entries, as all uploaded covers are currently attached to the marking as an image since v1 had no cover record table."

Backend — subject_type/subject_id were already writable on ImageSerializer, but unguarded:

• Update-path validate(): the target subject must actually exist (subject_id is a bare integer column — before this, an image could be PATCHed onto Cover #99999 silently), and image_view must match the new subject type (400 with a field error instead of the DB check constraint's 500).
• perform_update(): on a subject change, the moved image lands at the end of the target's display order — never displacing the target's default thumbnail — and the source subject promotes a new display_order=0, mirroring the delete path's promotion logic from PR Requested fixes for Ian, 6/28 #74.

Frontend

• Marking page: Move to cover action on marking-subject thumbnails (the Charlestown red/black cross-contamination case). Dialog restricts targets to covers already linked to the marking + cover view select (Front/Back/Interior/Detail).
• Cover page: reverse Move to marking via a new optional onMoveImage action on EntryAssociatedThumbnailsCard; targets the cover's linked markings with Full/Detail views.
• Both gated editor/admin, matching IsEditorOrAdminWrite.

Stacked on #84

This branch includes #47 (reese/issue-47-link-existing) because both features rework the same detail pages — merge #84 first and this diff reduces to the image-move commit (eb62ac8).

Tests

+5 backend (test_image_move): both move directions, display-order bookkeeping on source and target, 400 on missing target, 400 on view mismatch, 403 for contributors. Backend suite 135/136 (the 1 failure is the pre-existing test_top_search_ignores_catalog_text flagged in #83). jest 53/53, tsc/eslint/ruff/vite build clean.

🤖 Generated with Claude Code

[github-actions]

Code review overview

• Syntax Errors - Score - 9/10
• Code Smells - Score - 7/10
• Bugs - Score - 7/10
• Security Vulnerabilities - Score - 8/10

---

Syntax Errors

Minor issues only; nothing that would prevent the code from running.

File: frontend/src/pages/RecordDetail.tsx, import line
Location: Top of file, lucide-react import
Why it matters: MoveRight is imported in RecordDetail.tsx directly but in EntryAssociatedThumbnailsCard.tsx it is also imported—both are fine individually, but the icon is used in both the card component and inline in RecordDetail, which is an inconsistency rather than a syntax error.
Suggested fix: Use the card component's onMoveImage prop in RecordDetail rather than duplicating the button inline, removing the need for the direct icon import there.

---

Code Smells

1. File: frontend/src/pages/RecordDetail.tsx and frontend/src/pages/CoverDetail.tsx
Location: Move-image UI in both pages
Why it matters: The dialog, state variables (moveImageBusy, moveImageError, moveImageView, etc.), and confirmation flow are duplicated nearly verbatim across two page components. Future changes (e.g. adding a confirmation step) must be made in both places.
Suggested fix: Extract a <MoveImageDialog> component that accepts open, onConfirm, targetOptions, allowedViews, and onClose props.

2. File: frontend/src/pages/CoverDetail.tsx
Location: handleMoveImageToMarking, the image view options (FULL, DETAIL)
Why it matters: The allowed marking views are hardcoded in the JSX <SelectContent> and also implicitly assumed in the default state setMoveImageView("FULL"). The same list exists on the backend (MARKING_VIEW_CHOICES). A mismatch will produce a 400 silently surfaced only as an error string.
Suggested fix: Share view-choice constants in a single place (a shared imageViews.ts or driven by the API), and derive the <SelectItem> list from it.

3. File: backend/common/api/v2/serializers.py
Location: _validate_update, allowed_views check
Why it matters: allowed_views is used in an in membership test but its type depends on what Image.MARKING_VIEW_CHOICES / Image.COVER_VIEW_CHOICES actually are. If these are lists of 2-tuples (the Django convention, e.g. [("FULL", "Full"), ...]) the in test checks against tuples, not the string values, so every image_view string would fail validation.
Suggested fix: Extract just the value part: allowed_views = [v for v, _ in Image.MARKING_VIEW_CHOICES if ...] or use a dedicated set/list of raw values.

4. File: frontend/src/services/markings.ts
Location: moveImageSubject error handler cast
Why it matters: Casting err to a hand-rolled shape instead of using an Axios type guard (axios.isAxiosError) is fragile and will silently swallow non-Axios errors (network timeouts, etc.) as generic "Could not reassign image."
Suggested fix: Use import axios from "axios"; if (axios.isAxiosError(err)) { ... } before accessing .response.

5. File: frontend/src/pages/RecordDetail.tsx
Location: canMoveToCover condition, record.images[idx]?.subjectType === "MARKING" check
Why it matters: This guard is re-checking something that should always be true on this page (images for a marking), suggesting defensive code that may hide a deeper data-model ambiguity, and adds noise.
Suggested fix: Remove the redundant subjectType guard or document clearly why a non-MARKING image might appear on the marking detail page.

---

Bugs

1. File: backend/common/api/v2/views.py
Location: perform_update, instance.save(update_fields=["display_order", "modified_date"])
Why it matters: modified_date is set on save but modified_by is not included in update_fields for the moved image itself—only the source-subject's promoted default gets modified_by set. If the audit trail requires modified_by to be updated when display_order changes, this field is silently skipped.
Suggested fix: Add "modified_by" to the first update_fields list and assign instance.modified_by = self.request.user before that save.

2. File: backend/common/api/v2/serializers.py
Location: _validate_update, allowed_views check (see Code Smells #3 above)
Why it matters: If MARKING_VIEW_CHOICES / COVER_VIEW_CHOICES are Django-style 2-tuples, every update where image_view is not changed will raise a spurious 400 error, breaking all non-subject-change PATCH requests routed through _validate_update.
Suggested fix: As noted above, use the value portion of the choices tuples.

3. File: frontend/src/pages/CoverDetail.tsx
Location: handleMoveImageToMarking, after setMoveImageIndex(null)
Why it matters: If getImagesForSubject throws (network error), the finally block clears the busy flag but the error is not surfaced to the user—it is silently swallowed. The moved image may or may not appear in the refreshed list.
Suggested fix: Add a catch block that calls setMoveImageError("Failed to refresh images after move.").

4. File: frontend/src/pages/RecordDetail.tsx
Location: handleMoveImageToCover, refresh via getMarkingById
Why it matters: Same silent-swallow risk as above. Also, after a successful move the image list on the page is refreshed by re-fetching the entire marking record, while CoverDetail only refreshes images. The two pages use inconsistent refresh strategies, which could cause stale state for other fields.
Suggested fix: Unify: either always refresh the full record, or always refresh just images. Add a catch block for error surfacing.

---

Security Vulnerabilities

1. File: backend/common/api/v2/serializers.py
Location: _validate_update, Marking.all_objects / Cover.all_objects
Why it matters: Using all_objects (which presumably includes soft-deleted records) means an editor can move an image onto a soft-deleted marking or cover. The image would then be associated with a logically deleted record, potentially making it invisible or producing unexpected behavior.
Suggested fix: Use the default manager (Marking.objects) which should exclude soft-deleted rows, or add an explicit is_removed=False filter.

2. File: frontend/src/services/markings.ts
Location: moveImageSubject, no IDOR protection beyond session auth
Why it matters: The function accepts any imageId, subjectType, and subjectId from the caller. While the backend presumably checks that the authenticated user has editor rights, there is no frontend guard preventing a UI bug from passing an arbitrary subjectId for an unrelated record.
Why it matters (lower severity): This is standard for API clients, but worth noting that backend authorization must be the sole enforcement point and should be verified in tests.
Suggested fix: Confirm backend permission checks cover cross-record moves explicitly (the test suite covers 403 for contributors but not an editor moving to a completely unrelated, unlinked record).