#46: user-safe catalog refresh — batch backup/restore wrappers

[reese8272]

What

Thin orchestration around the existing per-marking backup/restore so a drop_ascc_state → import_apmc_bundle refresh no longer destroys user-submitted content:

• backup_user_markings <dir> — enumerates every marking carrying user-origin content (has covers, contributions, is_reviewed=True, edit history, or --uploader <username> images) and exports each via backup_marking. Fail-fast: any export error aborts before a drop can run against a partial backup set. Writes manifest.json reporting what could NOT be exported (markings without a code, covers linked to no marking) instead of silently skipping.
• restore_user_markings <dir> [--dry-run] — restores every backup, continues past failures (each restore_marking is transactional on its own), writes restore_report.json; the failures list is the editor review queue. Exits nonzero if anything failed.
• docs/devel/USER_SAFE_REFRESH.md — the wrapped sequence + caveats (code-less markings, media binaries, editor-edits-win-over-munger semantics).

Why

drop_ascc_state cascades through CoverMarking and deletes user covers/images/contributions, and post-policy bundles contain no covers — so a refresh of woco.dev (needed so Ian stops reviewing stale edition-5 data, per his 6/28–29 emails) would wipe his and Wayne's submissions. This is also the prerequisite for any future prod re-run.

Tests

• common.tests.test_user_markings_batch (4 tests): enumeration selects only user-content markings; no-code markings land in the manifest; full round-trip backup → drop_ascc_state VA → restore brings back covers/links/markings; a corrupt file doesn't stop the batch and is reported.
• Full common suite: 134/135 — the one failure (test_top_search_ignores_catalog_text) pre-exists on staging tip 0998d6a with this branch's files removed (verified via stash) and is unrelated (top-search vs catalog_txt semantics). @grubermeister flagging it for you since it's in the search area you've been changing.

ruff clean, manage.py check clean, no schema changes.

Not in this PR

The actual woco.dev refresh run (needs coordination on who runs it + re-run bundles), and the post-refresh re-measurement of Ian's reports (Abingdon 7/7/9, FL Almirante/Almirante/Alaqua manuscripts).

🤖 Generated with Claude Code

[github-actions]

Code review overview

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

---

Syntax Errors

No issues found.

---

Code Smells

File: backup_user_markings.py
Location: _collect_marking_ids, cover-contribution loop (~lines 185–191)

Why it matters: The method iterates over every cover-contribution row in Python to call _cover_contribution_marking_id one at a time. For large datasets this is an O(N) query-per-row anti-pattern (or at minimum a large in-memory loop) when the filtering could potentially be pushed into the database or at least batched more tightly.

Suggested fix: Consider whether _cover_contribution_marking_id can be reimplemented as an annotation/subquery, or at least document the known scale limit so maintainers know when it becomes a problem.

---

File: backup_user_markings.py
Location: _region_marking_filter, prefix-matching approach (~lines 215–245)

Why it matters: Scoping markings by post_office__code__startswith=<region_prefix> is a naming convention encoded as application logic. If the PostOfficeRegion join table already expresses this relationship structurally, filtering through it would be more robust and not depend on code-string conventions.

Suggested fix: Consider filtering via post_office__regions__in=matched_regions (or equivalent) rather than string-prefix matching, so renaming a region code doesn't silently break the scope.

---

File: restore_user_markings.py
Location: handle, file discovery (~line 57)

Why it matters: The command picks up any *.json file in the directory that isn't the manifest or report. If unrelated JSON files are present (e.g. from a previous failed run, or copied in by mistake) they will be silently attempted as backup files, producing confusing error messages.

Suggested fix: Either use filenames that follow a predictable pattern (e.g. only files whose names match the backup_filename format) or validate the schema field at the top of each file before passing it to restore_marking.

---

File: backup_user_markings.py / restore_user_markings.py
Location: Module-level constants MANIFEST_SCHEMA, REPORT_SCHEMA

Why it matters: The schema strings are version-pinned (v1) but there is no version-checking on restore. If a future v2 manifest format changes fields, restore_user_markings will silently consume it without warning.

Suggested fix: Add a schema-version assertion when reading the manifest on the restore side, or at minimum log the schema string so operators can spot a mismatch.

---

Bugs

File: backup_user_markings.py
Location: backup_filename, ~line 62

Why it matters: Only / is replaced when constructing a filesystem-safe filename from Marking.code. Other characters that are problematic on common filesystems (e.g. \, :, *, ? on Windows; or simply spaces) are passed through unchanged. If code values ever contain these characters, the written file path will be invalid or the backup and restore filenames won't match.

Suggested fix: Use a whitelist replacement (e.g. re.sub(r'[^\w\-.]', '_', code)) rather than replacing only /.

---

File: test_user_markings_batch.py
Location: test_backup_selects_user_content_markings_only, ~line 105

Why it matters: The test asserts manifest["codes"] == [self.covered.code, self.reviewed.code] relying on alphabetical order_by("code") to produce a deterministic list. This is fine as long as the codes remain M0001/M0002, but the assertion is fragile — a change to either code in setUp could silently reorder results and flip the test expectation.

Suggested fix: Assert on sets (self.assertEqual(set(manifest["codes"]), {self.covered.code, self.reviewed.code})) or explicitly document the ordering dependency.

---

File: restore_user_markings.py
Location: handle, error path (~line 87)

Why it matters: When failures occur, raise CommandError(...) exits the management command before the success-path self.stdout.write(...) runs. This is correct, but the report file is always written regardless of whether the in_dir is writable, since there is no try/except around report_path.write_text(...). A write failure here would produce an unhandled exception with no report at all, obscuring which restores already succeeded.

Suggested fix: Wrap the report write in a try/except and print restored/failed counts to stdout as a fallback.

---

Security Vulnerabilities

File: backup_user_markings.py
Location: handle, manifest write (~line 165)

Why it matters: out_dir is created with parents=True, exist_ok=True using the caller-supplied path with no restriction on where it may point. In an automated/CI context a misconfigured invocation could write files to arbitrary filesystem locations (e.g. ../../etc/). This is low risk for a management command (requires Django shell access), but worth noting.

Suggested fix: Optionally resolve and validate that out_dir is within an expected base directory (e.g. the project tools/ tree), or document that the command must only be run by trusted operators.