Add test for sidecar + upversion intersection behavior

[JSv4]

Summary

Adds test coverage for the intersection of sidecar import and document upversioning. Verifies that when a zip containing a document and its annotation sidecar is imported at a path that already has a Document, the system correctly creates a new Document version, attaches the sidecar's annotations to the new version, and preserves the prior version's annotations on the original Document row.

Changes

• New test class TestSidecarUpversioning in opencontractserver/tests/test_sidecar_import.py:
  • Tests the complete lifecycle of importing a sidecar+document pair at an existing corpus path (default pipeline route, i.e. skip_pipeline absent / falsy)
  • Verifies that v2 import creates a new Document in the same version_tree_id with parent pointing to v1
  • Confirms DocumentPath.version_number increments and the path chain is re-linked correctly
  • Validates that sidecar text annotations and doc-level labels attach to the new Document only
  • Ensures v1's annotations remain attached to the now-non-current v1 Document (not migrated or deleted)
  • Confirms that frontend queries resolving the current path only see v2's annotations
  • Validates the invariant that exactly one current Document exists per version_tree_id
• Factored out _SidecarImportTestMixin — shared in-memory zip / TemporaryFileHandle plumbing previously duplicated across TestSidecarImportTask, TestMalformedLabelsImport, and TestSidecarSchemaValidationIntegration. The new class uses it too.
• CHANGELOG.md updated to document the new test.

Implementation Notes

Sidecar title / description only land on the Document row via the skip_pipeline=True route (already covered by test_skip_pipeline_creates_document_from_export_data). The default-pipeline upversion flow used in this test uses the bare filename as the Document title, so this test does not assert on Document.title / Document.description. The contract being pinned is annotation ownership across versions, not metadata routing.

Key assertions verify:

• Path tree structure (version numbers, parent links, is_current flags)
• Content tree structure (version_tree_id, parent links, is_current flags)
• Annotation isolation (each Document version owns only its own annotations)
• Query visibility (current path resolves to v2 with only v2's annotations visible)
• Database consistency (all annotations still exist, just scoped to their Document versions)

This pins the contract that annotations are not migrated between versions — each Document row owns its own annotation set via FK relationship.

[claude]

Code Review

Overview

This PR adds TestSidecarUpversioning — a new integration test class that pins the contract for the sidecar-import + path-collision upversioning intersection. The scenario (v1 import → v2 import at same path) and the assertions (path tree, content tree, metadata isolation, annotation isolation, "frontend query" visibility) are all well-chosen and cover a real gap in the existing test suite. The CHANGELOG entry is accurate and well-scoped.

---

Issues

Critical — CLAUDE.md violation

The PR description ends with a bare Claude Code session URL:

https://claude.ai/code/session_012XuvTrHKXiMGPRwHxv1jGJ

CLAUDE.md explicitly says "Never credit Claude or Claude Code in commit messages, PR messages, comments, or any other artifacts." Please remove this URL from the description before merging.

---

Should Fix — DRY violation in helper methods

_create_test_zip and _create_temp_file_handle are near-exact copies of the same methods on TestSidecarImportTask (line ~352 in the existing class). The only difference is the zip filename string.

# TestSidecarImportTask (existing)
def _create_temp_file_handle(self, zip_buffer):
    zip_content = ContentFile(zip_buffer.read(), name="test_sidecar_import.zip")
    handle = TemporaryFileHandle.objects.create(file=zip_content)
    return handle

# TestSidecarUpversioning (new) — nearly identical
def _create_temp_file_handle(self, zip_buffer):
    zip_content = ContentFile(zip_buffer.read(), name="test_sidecar_upversion.zip")
    return TemporaryFileHandle.objects.create(file=zip_content)

CLAUDE.md calls out DRY as a first-class principle. A shared SidecarImportTestMixin (or having TestSidecarUpversioning inherit from TestSidecarImportTask) would eliminate the duplication. The setUp structure is also nearly identical — same transaction.atomic() blocks, same _load_pawls_subset(2) call, same permission grant pattern.

---

Should Fix — Local import inside test method

def test_sidecar_metadata_and_annotations_attach_to_new_version(self):
    from opencontractserver.documents.models import Document

Document should be imported at module level with the other model imports. Burying it inside the test method makes it harder to spot at a glance what the test depends on, and is inconsistent with the rest of the file.

---

Suggestions

Monolithic test method

At ~250 lines, test_sidecar_metadata_and_annotations_attach_to_new_version does a lot. The sequential dependency (v1 must exist before v2) makes splitting hard, but setUpClass could run both imports once, leaving individual test methods for each assertion group (path tree, content tree, metadata isolation, annotation isolation, query visibility). This would give clearer failure messages when a specific assertion breaks.

Example sketch:

@classmethod
def setUpClass(cls):
    super().setUpClass()
    # run v1 import, capture cls.v1_doc / cls.v1_path
    # run v2 import, capture cls.v2_doc / cls.v2_path

def test_path_tree_version_numbers(self): ...
def test_content_tree_parent_links(self): ...
def test_metadata_isolation(self): ...
def test_annotation_isolation(self): ...
def test_current_path_query_visibility(self): ...

This is a "nice to have" — the existing single-method approach is functional and consistent with tests elsewhere in the file.

TestCase vs TransactionTestCase

CLAUDE.md recommends TransactionTestCase for tests involving signals. The import task almost certainly fires signals (document save, annotation creation). The existing TestSidecarImportTask also uses TestCase, so this isn't a regression introduced by this PR — but it may be worth evaluating if this test ever becomes flaky due to signal-related transaction isolation.

Annotation type strings

"TOKEN_LABEL" and "DOC_TYPE_LABEL" are used as string literals throughout, but they're defined as module-level constants in opencontractserver.annotations.models (line 63–64). The constants are already imported from that module (Annotation, LabelSet, Relationship are imported but not TOKEN_LABEL/DOC_TYPE_LABEL). This is a pre-existing pattern in the file, not introduced here, but worth noting for a future cleanup pass.

---

Summary

The test logic itself is solid — the assertions cover exactly the right invariants and the scenario is well-constructed. The two blocking items are the CLAUDE.md attribution violation in the PR description and the DRY violations in the helper methods. Everything else is improvement-level feedback.

[claude]

Code Review

Overview

This PR adds TestSidecarUpversioning — an integration test that pins the contract for the sidecar-import + path-collision upversioning intersection. The scenario (v1 import → v2 import at same corpus path, both with sidecars) and the assertions (path tree, content tree, annotation isolation, "frontend query" visibility, cross-tree count invariant) fill a real gap that the existing TestDocumentUpversioning tests in test_zip_import_integration.py didn't exercise.

The _SidecarImportTestMixin refactor is the right move — it properly DRYs up _create_test_zip and _create_temp_file_handle across all four consumer classes. Good call.

---

Issues

Minor — handle_name on TestSidecarImportTask is redundant

_SidecarImportTestMixin already defaults handle_name = "test_sidecar_import.zip", and TestSidecarImportTask overrides it with the exact same string:

class TestSidecarImportTask(_SidecarImportTestMixin, TestCase):
    handle_name = "test_sidecar_import.zip"  # same as mixin default — redundant

Either remove the override from TestSidecarImportTask (simplest), or change the mixin default to "" and make it mandatory on each subclass (safer). As-is it's just noise that will confuse the next reader.

Minor — visible_texts check is one-sided

visible_texts = set(
    Annotation.objects.filter(
        document=current_doc, corpus=self.corpus
    ).values_list("raw_text", flat=True)
)
# Only asserts what's NOT there:
self.assertNotIn("Original Heading", visible_texts)

The negative assertion confirms v1's annotation is absent, but doesn't confirm v2's annotations are present. A ghost current doc (or empty filter) would pass. Add the positive assertion too:

self.assertEqual(visible_texts & {"Revised Heading", "New Section"}, {"Revised Heading", "New Section"})
# or more directly:
self.assertIn("Revised Heading", visible_texts)
self.assertIn("New Section", visible_texts)

Nit — _shared_labels_json can be a @staticmethod

The method never references self. Marking it @staticmethod makes the intent explicit and avoids callers wondering whether instance state is involved.

Nit — CHANGELOG entry is very long

The entry in CHANGELOG.md is ~280 words for a test-only addition. The body of the PR description covers all this context already. The changelog entry could be trimmed to 2–3 sentences covering the key point (new test class, what contract it pins, what was factored out).

---

Strengths

• Sequential narrative in a single test method works well here — each phase builds on the previous one, and the phase comments (# --- v1 import ---, # --- v2 import ---, etc.) make the progression easy to follow without needing separate test methods and shared fixture state.
• Cross-tree annotation count (Annotation.objects.filter(document__version_tree_id=...).count() == 3) is a strong invariant — it proves neither side lost annotations to the other.
• Rule C3 reference in the final assertion is a useful breadcrumb.
• Fixture divergence (pdf_bytes_v1 vs pdf_bytes_v2) correctly ensures pdf_file_hash will differ, making the hash assertion meaningful.
• The mixin's handle_name class attribute cleanly solves the filename-distinction problem without adding constructor complexity.

---

Summary

One real (minor) bug risk (one-sided visible_texts assertion), one redundant line, and two nits. The core test logic and the mixin refactor are solid — this is ready to merge after the visible_texts assertion is strengthened.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[claude]

Code Review: PR #1650 — Sidecar + Upversion Intersection Test

Overview

This PR adds TestSidecarUpversioning to pin the contract that annotations are not migrated across document versions during a sidecar+upversion import. It also refactors duplicated zip/handle helpers across three test classes into a _SidecarImportTestMixin. The scope is well-defined and the implementation is clean overall.

---

Strengths

• Thorough assertion coverage: The test validates the full surface area — path tree structure, content tree structure, annotation isolation per document version, DB-level count consistency, and the simulated frontend query (current-path resolution). This is exactly the kind of contract test that prevents silent regressions.
• _SidecarImportTestMixin refactor is correct: Leading underscore prevents accidental test discovery, handle_name class attribute gives per-class traceability for log/debug, and @staticmethod on _create_test_zip is appropriate since it has no instance state dependency.
• Inline comment for the magic number (# 1 from v1 + 2 from v2) on the count assertion at line ~1861 is the right level of explanation — it would otherwise be opaque.
• _shared_labels_json as @staticmethod: Correct — it constructs a pure value and doesn't need self.

---

Issues & Suggestions

Minor: Unnecessary nested transaction.atomic() in setUp

def setUp(self):
    with transaction.atomic():
        self.user = User.objects.create_user(...)
    with transaction.atomic():
        self.corpus = Corpus.objects.create(...)

TestCase already wraps each test method in a transaction (rolled back after). The nested atomic() blocks create savepoints rather than real transactions, so they add no isolation here. They're harmless but add visual noise. The other test classes in this file (TestSidecarImportTask, TestMalformedLabelsImport) don't use them. Consider removing for consistency.

Minor: Missing assertion on v1 path having no parent

The path-chain assertions confirm v2_path.parent == v1_path, but there's no corresponding check that v1_path.parent is None. A regression that sets a spurious parent on the first version wouldn't be caught. One line would seal this:

self.assertIsNone(v1_path.parent)

Minor: result["files_upversioned"] accessed with direct indexing

self.assertEqual(result_v1["files_upversioned"], 0)
self.assertEqual(result_v1["annotation_sidecars_processed"], 1)

If the task ever omits one of these keys on a code path reached before the failure that result_v1["success"] would catch, the test raises KeyError instead of AssertionError, which can obscure the failure. result_v1.get("files_upversioned") with an explicit default would produce a cleaner failure message. This is a nit — the current behavior is still caught by the test run — but worth considering for test maintainability.

Nit: Verbose CHANGELOG entry

The CHANGELOG entry (~10 lines) essentially duplicates the PR description verbatim. Per CLAUDE.md, changelog entries should be concise and focused on what changed and why. Consider trimming to the essential facts: what was added, what contract it pins, and the DRY refactor. The PR description is the right place for the full implementation notes.

Observation: TestSidecarImportTask.handle_name not overridden

TestSidecarImportTask doesn't set handle_name, so it inherits the mixin's default "test_sidecar_import.zip". This happens to match the previously hardcoded string, so behavior is preserved. However, explicitly declaring handle_name = "test_sidecar_import.zip" in TestSidecarImportTask would make the mapping self-documenting (matching the pattern established by TestMalformedLabelsImport and TestSidecarSchemaValidationIntegration).

---

Assessment

The production logic is not changed — this is a pure test addition + refactor. The new test class covers a meaningful, previously untested code path (sidecar payload meeting a path collision), and the mixin refactor correctly eliminates the three duplicated helper implementations. The issues noted above are all minor or nits. The PR is in good shape to merge once the changelog verbosity is addressed.