admin: Fix AnnouncementItemForm image update

When submitting the form without replacing the image file, the filename
of the previous image would be replaced to something invalid.

Teach AnnouncementItemFactory how to create the File record to keep the
test data realistic.

Leave replaced files in tact for future reference

Revert "Leave replaced files in tact for future reference"

This reverts commit 4c8fc35.

Soft delete AnnouncementItem image files when replacing them

Replace File.deleted with deleted_at

itou/communications/forms.py

@@ -2,9 +2,11 @@
 import uuid
 
 from django import forms
+from django.utils import timezone
 
 from itou.communications.models import AnnouncementItem
 from itou.files.forms import ItouAdminImageInput
+from itou.files.models import File
 from itou.users.enums import UserKind
 from itou.utils import constants as global_constants
 
@@ -31,9 +33,14 @@ class Meta:
         model = AnnouncementItem
         fields = ["priority", "title", "description", "user_kind_tags", "image", "image_alt_text", "link"]
 
-    def clean_image(self):
-        image = self.cleaned_data.get("image")
-        if image:
-            extension = pathlib.Path(image.name).suffix
-            image.name = f"{uuid.uuid4()}{extension}"
-        return image
+    def save(self, commit=True):
+        if "image" in self.changed_data:
+            extension = pathlib.Path(self.instance.image.name).suffix
+            self.instance.image.name = f"{uuid.uuid4()}{extension}"
+            if image := self.initial.get("image"):
+                File.objects.filter(key=image.name).update(deleted_at=timezone.now())
+        instance = super().save(commit=commit)
+        if "image" in self.changed_data:
+            instance.image_storage = File.objects.create(key=instance.image.name)
+            instance.save()
+        return instance

itou/communications/models.py

@@ -254,20 +254,6 @@ def _update_cached_active_announcement(self):
 
     def save(self, *args, **kwargs):
         super().save(*args, **kwargs)
-
-        def get_image_storage_key():
-            return self.image.name
-
-        def create_image_storage():
-            self.image_storage = File.objects.create(key=get_image_storage_key())
-
-        if self.image:
-            if self.image_storage is None:
-                create_image_storage()
-            elif self.image_storage.key != get_image_storage_key():
-                self.image_storage.delete()
-                create_image_storage()
-
         self._update_cached_active_announcement()
 
     def delete(self, *args, **kwargs):

itou/files/migrations/0002_file_deleted_at.py

@@ -0,0 +1,19 @@
+# Generated by Django 5.1.3 on 2024-11-26 09:38
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("files", "0001_initial"),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name="file",
+            name="deleted_at",
+            field=models.DateTimeField(
+                help_text="Marqué pour suppression du stockage", null=True, verbose_name="supprimé le"
+            ),
+        ),
+    ]

itou/files/models.py

@@ -9,6 +9,9 @@ class File(models.Model):
     # encoding is at most 1024 bytes long.
     key = models.CharField(primary_key=True, max_length=1024)
     last_modified = models.DateTimeField("dernière modification sur Cellar", default=timezone.now)
+    deleted_at = models.DateTimeField(
+        verbose_name="supprimé le", help_text="Marqué pour suppression du stockage", null=True
+    )
 
     class Meta:
         verbose_name = "fichier"

tests/communications/factories.py

@@ -1,3 +1,4 @@
+import uuid
 from datetime import date
 
 import factory
@@ -7,6 +8,7 @@
 
 from itou.communications.models import AnnouncementCampaign, AnnouncementItem
 from itou.users.enums import UserKind
+from tests.files.factories import FileFactory
 
 
 faker = Faker()
@@ -38,11 +40,21 @@ def with_items_for_every_user_kind(obj, create, extracted, **kwargs):
 class AnnouncementItemFactory(factory.django.DjangoModelFactory):
     class Meta:
         model = AnnouncementItem
+        skip_postgeneration_save = True
 
     class Params:
         with_image = factory.Trait(
-            image=factory.django.ImageField(width=1, height=1, format="JPEG"),
+            image=factory.django.ImageField(
+                filename=factory.LazyFunction(lambda: f"{uuid.uuid4()}.png"),
+                width=1,
+                height=1,
+                format="JPEG",
+            ),
             image_alt_text=factory.Faker("sentence", locale="fr_FR"),
+            image_storage=factory.RelatedFactory(
+                FileFactory,
+                key=factory.LazyAttribute(lambda obj: f"news-images/{obj.factory_parent.image.name}"),
+            ),
         )
         for_snapshot = factory.Trait(
             title="Nouvelle fonctionnalité sur notre site",

tests/communications/test_admin.py

@@ -0,0 +1,80 @@
+import io
+import pathlib
+import uuid
+
+import pytest
+from django.core.files.uploadedfile import SimpleUploadedFile
+from django.urls import reverse
+from PIL import Image
+from pytest_django.asserts import assertRedirects
+
+from itou.communications.models import AnnouncementCampaign
+from itou.files.models import File
+from tests.communications.factories import AnnouncementItemFactory
+
+
+class TestAnnouncementItemAdmin:
+    @pytest.fixture
+    def black_pixel(self):
+        with io.BytesIO() as buf:
+            image = Image.new(mode="RGB", size=(1, 1), color=(0, 0, 0))
+            image.save(buf, format="png")
+            buf.seek(0)
+            yield buf.getvalue()
+
+    def test_add_image(self, admin_client, black_pixel):
+        response = admin_client.post(
+            reverse("admin:communications_announcementcampaign_add"),
+            {
+                "max_items": 3,
+                "start_date": "01/11/2024",
+                "live": "on",
+                "items-TOTAL_FORMS": "1",
+                "items-INITIAL_FORMS": "0",
+                "items-0-priority": "0",
+                "items-0-title": "Bla",
+                "items-0-description": "Ho",
+                "items-0-image": SimpleUploadedFile("my-image.png", black_pixel, content_type="image/png"),
+                "items-0-image_alt_text": "aaa",
+                "items-0-link": "",
+                "_save": "Enregistrer",
+            },
+        )
+        assertRedirects(response, reverse("admin:communications_announcementcampaign_changelist"))
+        campaign = AnnouncementCampaign.objects.get()
+        [item] = campaign.items.all()
+        filename = pathlib.Path(item.image.name)
+        assert uuid.UUID(filename.stem)  # Did not use the provided filename
+        assert filename.suffix == ".png"
+        file = File.objects.get()
+        assert file.key.startswith("news-images/")
+
+    def test_change_image(self, admin_client, black_pixel):
+        item = AnnouncementItemFactory(with_image=True)
+        url = reverse("admin:communications_announcementcampaign_change", args=(item.campaign_id,))
+        response = admin_client.post(
+            url,
+            {
+                "max_items": 3,
+                "start_date": "01/11/2024",
+                "live": "on",
+                "items-TOTAL_FORMS": "1",
+                "items-INITIAL_FORMS": "1",
+                "items-0-id": f"{item.pk}",
+                "items-0-priority": "0",
+                "items-0-title": "Bla",
+                "items-0-description": "Ho",
+                "items-0-image": SimpleUploadedFile("my-image.png", black_pixel, content_type="image/png"),
+                "items-0-image_alt_text": "aaa",
+                "items-0-link": "",
+                "_save": "Enregistrer",
+            },
+        )
+        assertRedirects(response, reverse("admin:communications_announcementcampaign_changelist"))
+        item.refresh_from_db()
+        filename = pathlib.Path(item.image.name)
+        assert uuid.UUID(filename.stem)  # Did not use the provided filename
+        assert filename.suffix == ".png"
+        assert File.objects.filter(deleted_at__isnull=False).count() == 1
+        assert item.image_storage.deleted_at is None
+        assert item.image_storage.key.startswith("news-images/")

tests/communications/test_campaigns.py

@@ -1,14 +1,10 @@
-import uuid
 from datetime import date
-from unittest import mock
 
 import pytest
-from django.core.files.uploadedfile import SimpleUploadedFile
 from django.forms import ModelForm
 from django.forms.models import model_to_dict
 from django.urls import reverse
 
-from itou.communications.forms import AnnouncementItemForm
 from itou.communications.models import AnnouncementCampaign
 from tests.communications.factories import AnnouncementCampaignFactory, AnnouncementItemFactory
 from tests.users.factories import ItouStaffFactory
@@ -108,15 +104,3 @@ def test_campaign_not_rendered_draft(self, client):
         assert response.status_code == 200
         content = parse_response_to_soup(response)
         assert len(content.select("#news-modal")) == 0
-
-
-class TestAnnouncementItemForm:
-    @mock.patch("uuid.uuid4", return_value=uuid.UUID("6971c4bb-217f-4e84-b8a0-3f055ed19b72"))
-    def test_form_image_upload(self, client):
-        form = AnnouncementItemForm({}, instance=AnnouncementItemFactory())
-        form.is_valid()
-
-        form.cleaned_data["image"] = SimpleUploadedFile("image.jpg", b"", content_type="image/jpeg")
-        image = form.clean_image()
-
-        assert image.name == "6971c4bb-217f-4e84-b8a0-3f055ed19b72.jpg"

tests/communications/test_models.py

@@ -7,8 +7,7 @@
 from itou.communications.apps import sync_notifications
 from itou.communications.dispatch.base import BaseNotification
 from itou.communications.models import NotificationRecord, NotificationSettings
-from itou.files.models import File
-from tests.communications.factories import AnnouncementCampaignFactory, AnnouncementItemFactory
+from tests.communications.factories import AnnouncementCampaignFactory
 from tests.companies.factories import CompanyMembershipFactory
 from tests.users.factories import EmployerFactory, JobSeekerFactory, PrescriberFactory
 
@@ -130,16 +129,3 @@ def test_start_date_conflict_constraint(self):
         # cannot conflict existing date with a new instance
         with pytest.raises(IntegrityError):
             AnnouncementCampaignFactory(start_date=existing_campaign.start_date)
-
-
-class TestAnnouncementItemModel:
-    def test_file_update(self):
-        item = AnnouncementItemFactory(image_storage=None, with_image=True)
-        item.save()
-        assert item.image_storage.key == item.image.name
-
-        item.image.name = "news-images/new-image.jpg"
-        item.save()
-
-        file = File.objects.get()  # only one should exist in the database
-        assert file.key == "news-images/new-image.jpg"