Cleanup the serializers and views

src/documents/serialisers.py

@@ -24,6 +24,7 @@ from django.core.validators import MaxValueValidator
 from django.core.validators import MinValueValidator
 from django.core.validators import RegexValidator
 from django.core.validators import integer_validator
+from django.db import transaction
 from django.db.models import Count
 from django.db.models import Q
 from django.db.models.functions import Lower
@@ -3767,19 +3768,8 @@ class OcrTemplateZoneSerializer(serializers.ModelSerializer):
             "validation_regex",
         ]
 
-    def validate_width(self, value):
-        if value < 1:
-            raise serializers.ValidationError("Width must be at least 1.")
-        return value
-
-    def validate_height(self, value):
-        if value < 1:
-            raise serializers.ValidationError("Height must be at least 1.")
-        return value
-
     def validate_custom_field(self, value):
         if value is None:
-            # Built-in target (title/asn/created) — no custom field required.
             return value
         if value.data_type not in OCR_SUPPORTED_FIELD_TYPES:
             raise serializers.ValidationError(
@@ -3788,9 +3778,55 @@ class OcrTemplateZoneSerializer(serializers.ModelSerializer):
             )
         return value
 
+    def validate_page(self, value):
+        if value is not None and value != -1 and value < 1:
+            raise serializers.ValidationError(
+                "Page must be positive or -1 for the last page.",
+            )
+        return value
+
+
+class OcrTemplateZoneTestSerializer(OcrTemplateZoneSerializer):
+    name = serializers.CharField(default="test")
+    x = serializers.IntegerField(min_value=0, default=0)
+    y = serializers.IntegerField(min_value=0, default=0)
+    width = serializers.IntegerField(min_value=2)
+    height = serializers.IntegerField(min_value=2)
+
+    class Meta(OcrTemplateZoneSerializer.Meta):
+        fields = [
+            "name",
+            "x",
+            "y",
+            "width",
+            "height",
+            "page",
+            "ocr_language",
+            "transform",
+            "date_format",
+            "validation_regex",
+            "zone_source_width",
+            "zone_source_height",
+        ]
+        extra_kwargs = {
+            "zone_source_width": {"min_value": 1},
+            "zone_source_height": {"min_value": 1},
+        }
+
+
+class OcrTemplateTestSerializer(serializers.Serializer):
+    document = serializers.PrimaryKeyRelatedField(
+        queryset=Document.objects.select_related("root_document__owner"),
+    )
+    zone = OcrTemplateZoneTestSerializer()
+
 
 class OcrTemplateSerializer(MatchingModelSerializer, OwnedObjectSerializer):
     zones = OcrTemplateZoneSerializer(many=True, required=False)
+    combine_formats = serializers.DictField(
+        child=serializers.CharField(allow_blank=True, trim_whitespace=False),
+        required=False,
+    )
 
     # Templates are never assigned to documents
     slug = None
@@ -3826,45 +3862,38 @@ class OcrTemplateSerializer(MatchingModelSerializer, OwnedObjectSerializer):
             )
         return value
 
-    def validate_source_width(self, value):
-        if value < 1:
-            raise serializers.ValidationError("Source width must be at least 1.")
-        return value
-
-    def validate_source_height(self, value):
-        if value < 1:
-            raise serializers.ValidationError("Source height must be at least 1.")
-        return value
-
-    def validate_zones(self, zones_data):
-        """Validate zone coordinates are within the source dimensions."""
-        # source_width/height may not be in initial_data during partial updates
-        source_width = self.initial_data.get("source_width") or (
-            self.instance.source_width if self.instance else None
+    def validate(self, data):
+        data = super().validate(data)
+        zones = data.get("zones", [])
+        if (
+            self.instance
+            and "zones" not in data
+            and ("source_width" in data or "source_height" in data)
+        ):
+            zones = self.instance.zones.values()
+        source_width = data.get(
+            "source_width",
+            getattr(self.instance, "source_width", None),
         )
-        source_height = self.initial_data.get("source_height") or (
-            self.instance.source_height if self.instance else None
+        source_height = data.get(
+            "source_height",
+            getattr(self.instance, "source_height", None),
         )
+        for zone in zones:
+            width = zone.get("zone_source_width") or source_width
+            height = zone.get("zone_source_height") or source_height
+            if (
+                zone.get("x", 0) + zone.get("width", 0) > width
+                or zone.get("y", 0) + zone.get("height", 0) > height
+            ):
+                raise serializers.ValidationError(
+                    {
+                        "zones": f"Zone '{zone.get('name', '?')}' extends beyond its source dimensions.",
+                    },
+                )
+        return data
 
-        if source_width and source_height:
-            for zone in zones_data:
-                x = zone.get("x", 0)
-                y = zone.get("y", 0)
-                w = zone.get("width", 0)
-                h = zone.get("height", 0)
-                if x + w > int(source_width):
-                    raise serializers.ValidationError(
-                        f"Zone '{zone.get('name', '?')}' extends beyond source width "
-                        f"({x + w} > {source_width}).",
-                    )
-                if y + h > int(source_height):
-                    raise serializers.ValidationError(
-                        f"Zone '{zone.get('name', '?')}' extends beyond source height "
-                        f"({y + h} > {source_height}).",
-                    )
-
-        return zones_data
-
+    @transaction.atomic
     def create(self, validated_data):
         zones_data = validated_data.pop("zones", [])
         template = super().create(validated_data)
@@ -3872,12 +3901,12 @@ class OcrTemplateSerializer(MatchingModelSerializer, OwnedObjectSerializer):
             OcrTemplateZone.objects.create(template=template, **zone_data)
         return template
 
+    @transaction.atomic
     def update(self, instance, validated_data):
         zones_data = validated_data.pop("zones", None)
         instance = super().update(instance, validated_data)
 
         if zones_data is not None:
-            # Replace all zones with the new set
             instance.zones.all().delete()
             for zone_data in zones_data:
                 OcrTemplateZone.objects.create(template=instance, **zone_data)

src/documents/views.py

@@ -158,6 +158,7 @@ from documents.models import Document
 from documents.models import DocumentType
 from documents.models import Note
 from documents.models import OcrTemplate
+from documents.models import OcrTemplateZone
 from documents.models import PaperlessTask
 from documents.models import SavedView
 from documents.models import ShareLink
@@ -208,6 +209,7 @@ from documents.serialisers import MergeDocumentsAsVersionsSerializer
 from documents.serialisers import MergeDocumentsSerializer
 from documents.serialisers import NotesSerializer
 from documents.serialisers import OcrTemplateSerializer
+from documents.serialisers import OcrTemplateTestSerializer
 from documents.serialisers import PostDocumentSerializer
 from documents.serialisers import RemovePasswordDocumentsSerializer
 from documents.serialisers import ReprocessDocumentsSerializer
@@ -2310,12 +2312,10 @@ class DocumentViewSet(
         serializer.is_valid(raise_exception=True)
         template = serializer.validated_data.get("template")
 
-        try:
-            document = Document.objects.select_related(
-                "root_document__owner",
-            ).get(pk=pk)
-        except Document.DoesNotExist:
-            raise Http404
+        document = get_object_or_404(
+            Document.objects.select_related("root_document__owner"),
+            pk=pk,
+        )
         if not request.user.has_perm(
             "documents.change_document",
         ) or not has_perms_owner_aware(request.user, "change_document", document):
@@ -2349,27 +2349,28 @@ class DocumentViewSet(
 
         run_zone_extraction(document, None, templates=templates)
 
-        # Collect results
         results = []
         builtin_labels = {"title": "Title", "asn": "ASN", "created": "Created"}
+        builtin_values = {
+            "title": document.title,
+            "asn": document.archive_serial_number,
+            "created": document.created.isoformat() if document.created else None,
+        }
+        field_values = {
+            instance.field_id: instance.value
+            for instance in document.custom_fields.select_related("field")
+        }
         for template in templates:
             for zone in template.zones.all():
-                target = getattr(zone, "target", None) or "custom_field"
-                if target == "custom_field" and zone.custom_field_id:
-                    cf_instance = document.custom_fields.filter(
-                        field=zone.custom_field,
-                    ).first()
+                if (
+                    zone.target == OcrTemplateZone.TargetType.CUSTOM_FIELD
+                    and zone.custom_field_id
+                ):
                     field_name = zone.custom_field.name
-                    value = cf_instance.value if cf_instance else None
+                    value = field_values.get(zone.custom_field_id)
                 else:
-                    field_name = builtin_labels.get(target, target)
-                    value = {
-                        "title": document.title,
-                        "asn": document.archive_serial_number,
-                        "created": document.created.isoformat()
-                        if document.created
-                        else None,
-                    }.get(target)
+                    field_name = builtin_labels.get(zone.target, zone.target)
+                    value = builtin_values.get(zone.target)
                 results.append(
                     {
                         "template": template.name,
@@ -5887,24 +5888,17 @@ class OcrTemplateViewSet(BulkPermissionMixin, PassUserMixin, ModelViewSet[OcrTem
         url_path=r"document-page-image/(?P<doc_id>[0-9]+)/(?P<page>[0-9]+)",
     )
     def document_page_image(self, request, doc_id=None, page=None):
-        """Render a specific page of a document as a PNG image.
-
-        Used by the frontend template editor to display document pages
-        as images that users can draw zones on.
-        """
-        try:
-            document = Document.objects.select_related(
-                "root_document__owner",
-            ).get(pk=doc_id)
-        except Document.DoesNotExist:
-            raise Http404("Document not found")
+        """Render a page for the template editor's zone overlay."""
+        document = get_object_or_404(
+            Document.objects.select_related("root_document__owner"),
+            pk=doc_id,
+        )
 
         if not has_perms_owner_aware(request.user, "view_document", document):
             return HttpResponseForbidden("Insufficient permissions")
 
         page_num = int(page)
 
-        # Validate page number
         if document.page_count and page_num >= document.page_count:
             raise Http404(
                 f"Page {page_num} out of range (document has {document.page_count} pages)",
@@ -5914,7 +5908,6 @@ class OcrTemplateViewSet(BulkPermissionMixin, PassUserMixin, ModelViewSet[OcrTem
         if not doc_path or not Path(doc_path).is_file():
             raise Http404("Document file not found")
 
-        # Check if document is an image (single page, no PDF rendering needed)
         if document.mime_type and document.mime_type.startswith("image/"):
             content = Path(doc_path).read_bytes()
             return HttpResponse(content, content_type=document.mime_type)
@@ -5927,7 +5920,7 @@ class OcrTemplateViewSet(BulkPermissionMixin, PassUserMixin, ModelViewSet[OcrTem
                         "pdftoppm",
                         "-png",
                         "-r",
-                        "150",  # Lower DPI for preview
+                        "150",
                         "-f",
                         str(page_num + 1),
                         "-l",
@@ -5939,14 +5932,17 @@ class OcrTemplateViewSet(BulkPermissionMixin, PassUserMixin, ModelViewSet[OcrTem
                     capture_output=True,
                     timeout=30,
                 )
-            except subprocess.CalledProcessError as e:
+            except (subprocess.CalledProcessError, subprocess.TimeoutExpired) as error:
                 logger.warning(
-                    f"Failed to render page {page_num} of document {document.pk}: "
-                    f"{e.stderr.decode(errors='replace')[:200]}",
+                    "Failed to render page %s of document %s: %s",
+                    page_num,
+                    document.pk,
+                    error,
                 )
-                raise Http404("Failed to render page")
-            except FileNotFoundError:
-                raise Http404("pdftoppm not available - is poppler-utils installed?")
+                raise Http404("Failed to render page") from error
+            except FileNotFoundError as error:
+                logger.warning("pdftoppm is not available")
+                raise Http404("Failed to render page") from error
 
             rendered = sorted(Path(tmp_dir).glob("page-*.png"))
             if not rendered:
@@ -5956,31 +5952,15 @@ class OcrTemplateViewSet(BulkPermissionMixin, PassUserMixin, ModelViewSet[OcrTem
 
         return HttpResponse(content, content_type="image/png")
 
+    @extend_schema(request=OcrTemplateTestSerializer)
     @action(detail=False, methods=["post"], url_path="test-zone")
     def test_zone(self, request):
-        """Run OCR on a single ad-hoc zone of a document and return what it
-        yields: the raw OCR text, the transformed value, and whether the
-        validation regex matches. Non-destructive - writes nothing. Used by the
-        editor's per-zone test so a user can tune the zone/regex before saving.
-
-        Accepts: {"document": <id>, "zone": {x, y, width, height, page,
-        ocr_language, transform, validation_regex, zone_source_width,
-        zone_source_height}}.
-        """
-        from documents.models import OcrTemplateZone
+        """Extract an ad-hoc zone without changing the document."""
         from documents.zone_ocr import extract_zone_preview
 
-        zone_data = request.data.get("zone") or {}
-
-        try:
-            document = Document.objects.select_related(
-                "root_document__owner",
-            ).get(pk=request.data.get("document"))
-        except (Document.DoesNotExist, ValueError, TypeError):
-            return Response(
-                {"error": "Document not found"},
-                status=status.HTTP_404_NOT_FOUND,
-            )
+        serializer = OcrTemplateTestSerializer(data=request.data)
+        serializer.is_valid(raise_exception=True)
+        document = serializer.validated_data["document"]
 
         if not has_perms_owner_aware(request.user, "view_document", document):
             return HttpResponseForbidden("Insufficient permissions")
@@ -5992,36 +5972,13 @@ class OcrTemplateViewSet(BulkPermissionMixin, PassUserMixin, ModelViewSet[OcrTem
                 status=status.HTTP_404_NOT_FOUND,
             )
 
-        try:
-            zone = OcrTemplateZone(
-                name=zone_data.get("name") or "test",
-                x=int(zone_data.get("x", 0)),
-                y=int(zone_data.get("y", 0)),
-                width=int(zone_data.get("width", 0)),
-                height=int(zone_data.get("height", 0)),
-                page=zone_data.get("page"),
-                ocr_language=zone_data.get("ocr_language") or "eng",
-                transform=zone_data.get("transform") or "strip",
-                date_format=zone_data.get("date_format") or "",
-                validation_regex=zone_data.get("validation_regex") or "",
-            )
-        except (ValueError, TypeError):
-            return Response(
-                {"error": "Invalid zone definition"},
-                status=status.HTTP_400_BAD_REQUEST,
-            )
-
-        if zone.width < 2 or zone.height < 2:
-            return Response(
-                {"error": "Zone is too small to test"},
-                status=status.HTTP_400_BAD_REQUEST,
-            )
+        zone = OcrTemplateZone(**serializer.validated_data["zone"])
 
         result = extract_zone_preview(
             Path(doc_path),
             zone,
-            int(zone_data.get("zone_source_width") or 0),
-            int(zone_data.get("zone_source_height") or 0),
+            zone.zone_source_width or 0,
+            zone.zone_source_height or 0,
             document.page_count,
         )