From 19d930a81aa410dc2c4bf11a9d0bb8b0031d6d5a Mon Sep 17 00:00:00 2001 From: Trenton H <797416+stumpylog@users.noreply.github.com> Date: Tue, 14 Apr 2026 08:05:34 -0700 Subject: [PATCH] feat: update conditionals.py to use resolve_requested_version and DocumentVersion - Replace resolve_effective_document_by_pk stub with direct get_object_or_404 + resolve_requested_version calls in all five conditional functions - Switch from .modified (shim) to .added (DocumentVersion native field) - Switch thumbnail cache key to use version.id instead of document id - Re-add get_root_document/get_latest_version_for_root stubs to versioning.py (bulk_edit.py still needs them; Task 10 will remove them) - Update TestVersionAwareFilters tests to reflect simplified filter behavior (no more FieldError fallback; filters query content directly) Co-Authored-By: Claude Sonnet 4.6 --- src/documents/conditionals.py | 56 ++++++++++--------- .../tests/test_api_document_versions.py | 27 +++------ src/documents/versioning.py | 19 +++++++ 3 files changed, 58 insertions(+), 44 deletions(-) diff --git a/src/documents/conditionals.py b/src/documents/conditionals.py index fa10ff58a..b358a4ba0 100644 --- a/src/documents/conditionals.py +++ b/src/documents/conditionals.py @@ -4,6 +4,7 @@ from typing import Any from django.conf import settings from django.core.cache import cache +from django.shortcuts import get_object_or_404 from documents.caching import CACHE_5_MINUTES from documents.caching import CACHE_50_MINUTES @@ -13,7 +14,7 @@ from documents.caching import CLASSIFIER_VERSION_KEY from documents.caching import get_thumbnail_modified_key from documents.classifier import DocumentClassifier from documents.models import Document -from documents.versioning import resolve_effective_document_by_pk +from documents.versioning import resolve_requested_version def suggestions_etag(request, pk: int) -> str | None: @@ -73,48 +74,53 @@ def metadata_etag(request, pk: int) -> str | None: Metadata is extracted from the original file, so use its checksum as the ETag """ - doc = resolve_effective_document_by_pk(pk, request).document - if doc is None: + doc = get_object_or_404(Document, pk=pk) + resolution = resolve_requested_version(doc, request) + version = resolution.version + if version is None: return None - return doc.checksum + return version.checksum def metadata_last_modified(request, pk: int) -> datetime | None: """ - Metadata is extracted from the original file, so use its modified. Strictly speaking, this is - not the modification of the original file, but of the database object, but might as well - error on the side of more cautious + Metadata is extracted from the original file, so use its added time. """ - doc = resolve_effective_document_by_pk(pk, request).document - if doc is None: + doc = get_object_or_404(Document, pk=pk) + resolution = resolve_requested_version(doc, request) + version = resolution.version + if version is None: return None - return doc.modified + return version.added def preview_etag(request, pk: int) -> str | None: """ ETag for the document preview, using the original or archive checksum, depending on the request """ - doc = resolve_effective_document_by_pk(pk, request).document - if doc is None: + doc = get_object_or_404(Document, pk=pk) + resolution = resolve_requested_version(doc, request) + version = resolution.version + if version is None: return None use_original = ( hasattr(request, "query_params") and "original" in request.query_params and request.query_params["original"] == "true" ) - return doc.checksum if use_original else doc.archive_checksum + return version.checksum if use_original else version.archive_checksum def preview_last_modified(request, pk: int) -> datetime | None: """ - Uses the documents modified time to set the Last-Modified header. Not strictly - speaking correct, but close enough and quick + Uses the version added time to set the Last-Modified header. """ - doc = resolve_effective_document_by_pk(pk, request).document - if doc is None: + doc = get_object_or_404(Document, pk=pk) + resolution = resolve_requested_version(doc, request) + version = resolution.version + if version is None: return None - return doc.modified + return version.added def thumbnail_last_modified(request: Any, pk: int) -> datetime | None: @@ -123,22 +129,22 @@ def thumbnail_last_modified(request: Any, pk: int) -> datetime | None: Cache should be (slightly?) faster than filesystem """ try: - doc = resolve_effective_document_by_pk(pk, request).document - if doc is None: + doc = get_object_or_404(Document, pk=pk) + resolution = resolve_requested_version(doc, request) + version = resolution.version + if version is None: return None - if not doc.thumbnail_path.exists(): + if not version.thumbnail_path.exists(): return None - # Use the effective document id for cache key - doc_key = get_thumbnail_modified_key(doc.id) + doc_key = get_thumbnail_modified_key(version.id) cache_hit = cache.get(doc_key) if cache_hit is not None: cache.touch(doc_key, CACHE_50_MINUTES) return cache_hit - # No cache, get the timestamp and cache the datetime last_modified = datetime.fromtimestamp( - doc.thumbnail_path.stat().st_mtime, + version.thumbnail_path.stat().st_mtime, tz=UTC, ) cache.set(doc_key, last_modified, CACHE_50_MINUTES) diff --git a/src/documents/tests/test_api_document_versions.py b/src/documents/tests/test_api_document_versions.py index 116b4c625..f5f99ee2a 100644 --- a/src/documents/tests/test_api_document_versions.py +++ b/src/documents/tests/test_api_document_versions.py @@ -7,7 +7,6 @@ from auditlog.models import LogEntry # type: ignore[import-untyped] from django.contrib.auth.models import Permission from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType -from django.core.exceptions import FieldError from django.core.files.uploadedfile import SimpleUploadedFile from rest_framework import status from rest_framework.test import APITestCase @@ -619,31 +618,21 @@ class TestDocumentVersioningApi(DirectoriesMixin, APITestCase): class TestVersionAwareFilters: - def test_title_content_filter_falls_back_to_content(self) -> None: + def test_title_content_filter_queries_content_directly(self) -> None: queryset = mock.Mock() - fallback_queryset = mock.Mock() - queryset.filter.side_effect = [FieldError("missing field"), fallback_queryset] - result = TitleContentFilter().filter(queryset, " latest ") + TitleContentFilter().filter(queryset, " latest ") - assert result is fallback_queryset - assert queryset.filter.call_count == 2 + assert queryset.filter.call_count == 1 - def test_effective_content_filter_falls_back_to_content_lookup(self) -> None: + def test_effective_content_filter_queries_content_directly(self) -> None: queryset = mock.Mock() - fallback_queryset = mock.Mock() - queryset.filter.side_effect = [FieldError("missing field"), fallback_queryset] - result = EffectiveContentFilter(lookup_expr="icontains").filter( - queryset, - " latest ", - ) + EffectiveContentFilter(lookup_expr="icontains").filter(queryset, " latest ") - assert result is fallback_queryset - first_kwargs = queryset.filter.call_args_list[0].kwargs - second_kwargs = queryset.filter.call_args_list[1].kwargs - assert first_kwargs == {"effective_content__icontains": "latest"} - assert second_kwargs == {"content__icontains": "latest"} + assert queryset.filter.call_count == 1 + kwargs = queryset.filter.call_args_list[0].kwargs + assert kwargs == {"content__icontains": "latest"} def test_effective_content_filter_returns_input_for_empty_values(self) -> None: queryset = mock.Mock() diff --git a/src/documents/versioning.py b/src/documents/versioning.py index efd73cb2a..ef7a66aa3 100644 --- a/src/documents/versioning.py +++ b/src/documents/versioning.py @@ -41,6 +41,25 @@ def get_version_by_pk(doc: Document, version_pk: int) -> DocumentVersion | None: return DocumentVersion.objects.filter(pk=version_pk, document=doc).first() +def get_root_document(doc: Document) -> Document: + """Return the root document. + + In the new model, every Document IS the root. This is a compatibility stub + used by bulk_edit.py; Task 10 will refactor the callers. + """ + return doc + + +def get_latest_version_for_root(doc: Document) -> Document: + """Return the document to use as the version source. + + In the new model, DocumentVersion holds per-version files. This stub + returns the document itself so that bulk_edit callers that have not yet + been updated to the new model do not crash. Task 10 will replace this. + """ + return doc + + def resolve_requested_version( doc: Document, request: Any,