force html_body, text_body and raw_blob to be updated all at once via the API

Author: sdemagny

src/backend/core/api/serializers.py

@@ -1,6 +1,7 @@
 # pylint: disable=too-many-lines
 """Client serializers for the messages core app."""
 
+from django.db import transaction
 from django.db.models import Count, Exists, OuterRef, Q
 
 from drf_spectacular.utils import extend_schema_field
@@ -960,7 +961,7 @@ class MessageTemplateSerializer(AbilitiesModelSerializer):
     is_forced = serializers.BooleanField(
         required=False, default=False, help_text="Set as forced template"
     )
-    raw_blob = serializers.CharField(required=True, help_text="Raw blob content")
+    raw_blob = serializers.CharField(required=False, help_text="Raw blob content")
 
     def to_representation(self, instance):
         """Convert model instance to serialized data."""
@@ -1002,45 +1003,64 @@ def validate(self, attrs):
                     "Either mailbox_id or maildomain_id is required."
                 )
 
-        # For creation or when both html_body and text_body are being updated
-        if "html_body" in attrs or "text_body" in attrs:
+        # For creation or when updating content fields
+        if "html_body" in attrs or "text_body" in attrs or "raw_blob" in attrs:
             html_body = attrs.get("html_body", "")
             text_body = attrs.get("text_body", "")
+            raw_blob = attrs.get("raw_blob", "")
 
-            # If we're updating both fields and both are empty, that's invalid
-            if not html_body and not text_body:
-                raise serializers.ValidationError(
-                    "At least one of html_body or text_body must be provided."
-                )
+            # If any content field is provided, all three must be provided together
+            # This ensures atomic updates and prevents inconsistent states
+            if html_body or text_body or raw_blob:
+                if not html_body or not text_body or not raw_blob:
+                    raise serializers.ValidationError(
+                        "When providing content fields, all three fields (html_body, text_body, raw_blob) "
+                        "must be provided together to ensure consistency."
+                    )
 
         return attrs
 
     def create(self, validated_data):
-        """Create template with relationships."""
+        """Create template with relationships and ensure atomic content creation."""
+
         mailbox_id = validated_data.pop("mailbox_id", None)
         maildomain_id = validated_data.pop("maildomain_id", None)
         raw_blob_content = validated_data.pop("raw_blob", None)
 
         # Set mailbox or maildomain directly
-        if mailbox_id:
+        # Update mailbox or maildomain if provided
+        if mailbox_id is not None and maildomain_id is not None:
+            # check if domain is the same as the mailbox domain
+            mailbox = models.Mailbox.objects.get(id=mailbox_id)
+            if mailbox.domain.id != maildomain_id:
+                raise serializers.ValidationError(
+                    "Mailbox domain does not match maildomain."
+                )
+            validated_data["mailbox"] = mailbox
+            validated_data["maildomain"] = models.MailDomain.objects.get(
+                id=maildomain_id
+            )
+        elif mailbox_id is not None:
             validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id)
-        elif maildomain_id:
+        elif maildomain_id is not None:
             validated_data["maildomain"] = models.MailDomain.objects.get(
                 id=maildomain_id
             )
 
-        # Create raw_blob relationship if content provided
-        if raw_blob_content:
-            blob = models.Blob.objects.create_blob(
-                content=raw_blob_content.encode("utf-8"),
-                content_type="application/json",
-                mailbox_id=mailbox_id,
-                maildomain_id=maildomain_id,
-            )
-            validated_data["raw_blob"] = blob
+        # Use atomic transaction to ensure all content fields are created together
+        with transaction.atomic():
+            # Create raw_blob relationship if content provided
+            if raw_blob_content:
+                blob = models.Blob.objects.create_blob(
+                    content=raw_blob_content.encode("utf-8"),
+                    content_type="application/json",
+                    mailbox_id=mailbox_id,
+                    maildomain_id=maildomain_id,
+                )
+                validated_data["raw_blob"] = blob
 
-        template = super().create(validated_data)
-        return template
+            template = super().create(validated_data)
+            return template
 
     def update(self, instance, validated_data):
         """Update template with relationships."""
@@ -1069,24 +1089,27 @@ def update(self, instance, validated_data):
                     "Mailbox domain does not match maildomain."
                 )
 
-        # Handle raw_blob update
-        if raw_blob_content is not None:
-            try:
-                if instance.raw_blob:
-                    instance.raw_blob.delete()
-            except models.Blob.DoesNotExist:
-                pass
-
-            if raw_blob_content:  # Only create blob if content is not empty
-                blob = models.Blob.objects.create_blob(
-                    content=raw_blob_content.encode("utf-8"),
-                    content_type="application/json",
-                    mailbox_id=mailbox_id,
-                    maildomain_id=maildomain_id,
-                )
-                validated_data["raw_blob"] = blob
-            else:
-                validated_data["raw_blob"] = None
-
-        template = super().update(instance, validated_data)
-        return template
+        # Use atomic transaction to ensure all content fields are updated together
+        with transaction.atomic():
+            # Handle raw_blob update first
+            if raw_blob_content is not None:
+                try:
+                    if instance.raw_blob:
+                        instance.raw_blob.delete()
+                except models.Blob.DoesNotExist:
+                    pass
+
+                if raw_blob_content:  # Only create blob if content is not empty
+                    blob = models.Blob.objects.create_blob(
+                        content=raw_blob_content.encode("utf-8"),
+                        content_type="application/json",
+                        mailbox_id=mailbox_id,
+                        maildomain_id=maildomain_id,
+                    )
+                    validated_data["raw_blob"] = blob
+                else:
+                    validated_data["raw_blob"] = None
+
+            # Update all fields atomically
+            template = super().update(instance, validated_data)
+            return template

src/backend/core/tests/api/test_message_template.py

@@ -1165,28 +1165,82 @@ def test_create_template_with_raw_blob(self, user, mailbox):
         template = models.MessageTemplate.objects.get()
         assert template.raw_blob.get_content().decode("utf-8") == "test blob content"
 
-    def test_update_template_raw_blob(self, user, mailbox):
-        """Test updating a template with raw_blob."""
+    def test_update_content_fields_atomic_validation(self, user, mailbox):
+        """Test that content fields must be updated together atomically."""
         factories.MailboxAccessFactory(
             mailbox=mailbox,
             user=user,
             role=models.MailboxRoleChoices.ADMIN,
         )
 
-        # Create template with blob
+        # Create a template
         template = factories.MessageTemplateFactory(
-            name="Blob Template",
-            html_body="<p>Content</p>",
-            text_body="Content",
+            name="Test Template",
+            html_body="<p>Original content</p>",
+            text_body="Original content",
             type=models.MessageTemplateTypeChoices.SIGNATURE,
             mailbox=mailbox,
         )
 
         client = APIClient()
         client.force_authenticate(user=user)
 
+        # Try to update only html_body - should fail
         data = {
-            "raw_blob": "new blob content",
+            "html_body": "<p>Updated content</p>",
+            "mailbox_id": str(mailbox.id),
+        }
+
+        response = client.patch(
+            reverse("message-templates-detail", kwargs={"pk": template.id}),
+            data,
+            format="json",
+        )
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert (
+            "all three fields (html_body, text_body, raw_blob) must be provided together"
+            in str(response.data)
+        )
+
+        # Try to update only text_body - should fail
+        data = {
+            "text_body": "Updated content",
+            "mailbox_id": str(mailbox.id),
+        }
+
+        response = client.patch(
+            reverse("message-templates-detail", kwargs={"pk": template.id}),
+            data,
+            format="json",
+        )
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert (
+            "all three fields (html_body, text_body, raw_blob) must be provided together"
+            in str(response.data)
+        )
+
+        # Try to update only raw_blob - should fail
+        data = {
+            "raw_blob": "updated raw content",
+            "mailbox_id": str(mailbox.id),
+        }
+
+        response = client.patch(
+            reverse("message-templates-detail", kwargs={"pk": template.id}),
+            data,
+            format="json",
+        )
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert (
+            "all three fields (html_body, text_body, raw_blob) must be provided together"
+            in str(response.data)
+        )
+
+        # Update all three fields together - should succeed
+        data = {
+            "html_body": "<p>Updated content</p>",
+            "text_body": "Updated content",
+            "raw_blob": "updated raw content",
             "mailbox_id": str(mailbox.id),
         }
 
@@ -1196,9 +1250,100 @@ def test_update_template_raw_blob(self, user, mailbox):
             format="json",
         )
         assert response.status_code == status.HTTP_200_OK
-        # raw_blob should contain the new content
-        assert response.data["raw_blob"] == "new blob content"
 
-        # check that blob was created with new content
+        # Verify all fields were updated
         template.refresh_from_db()
-        assert template.raw_blob.get_content().decode("utf-8") == "new blob content"
+        assert template.html_body == "<p>Updated content</p>"
+        assert template.text_body == "Updated content"
+        assert template.raw_blob.get_content().decode("utf-8") == "updated raw content"
+
+    def test_create_content_fields_atomic_validation(self, user, mailbox):
+        """Test that content fields must be created together atomically."""
+        factories.MailboxAccessFactory(
+            mailbox=mailbox,
+            user=user,
+            role=models.MailboxRoleChoices.ADMIN,
+        )
+
+        client = APIClient()
+        client.force_authenticate(user=user)
+
+        # Try to create with only html_body - should fail
+        data = {
+            "name": "Test Template",
+            "html_body": "<p>Content</p>",
+            "type": "signature",
+            "mailbox_id": str(mailbox.id),
+        }
+
+        response = client.post(
+            reverse("message-templates-list"),
+            data,
+            format="json",
+        )
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert (
+            "all three fields (html_body, text_body, raw_blob) must be provided together"
+            in str(response.data)
+        )
+
+        # Try to create with only text_body - should fail
+        data = {
+            "name": "Test Template",
+            "text_body": "Content",
+            "type": "signature",
+            "mailbox_id": str(mailbox.id),
+        }
+
+        response = client.post(
+            reverse("message-templates-list"),
+            data,
+            format="json",
+        )
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert (
+            "all three fields (html_body, text_body, raw_blob) must be provided together"
+            in str(response.data)
+        )
+
+        # Try to create with only raw_blob - should fail
+        data = {
+            "name": "Test Template",
+            "raw_blob": "raw content",
+            "type": "signature",
+            "mailbox_id": str(mailbox.id),
+        }
+
+        response = client.post(
+            reverse("message-templates-list"),
+            data,
+            format="json",
+        )
+        assert response.status_code == status.HTTP_400_BAD_REQUEST
+        assert (
+            "all three fields (html_body, text_body, raw_blob) must be provided together"
+            in str(response.data)
+        )
+
+        # Create with all three fields together - should succeed
+        data = {
+            "name": "Test Template",
+            "html_body": "<p>Content</p>",
+            "text_body": "Content",
+            "raw_blob": "raw content",
+            "type": "signature",
+            "mailbox_id": str(mailbox.id),
+        }
+
+        response = client.post(
+            reverse("message-templates-list"),
+            data,
+            format="json",
+        )
+        assert response.status_code == status.HTTP_201_CREATED
+
+        # Verify all fields were created
+        template = models.MessageTemplate.objects.get(name="Test Template")
+        assert template.html_body == "<p>Content</p>"
+        assert template.text_body == "Content"
+        assert template.raw_blob.get_content().decode("utf-8") == "raw content"