Merge pull request #1360 from ae-utbm/image_rotation

properly rotate SAS pictures

sas/api.py

@@ -126,9 +126,8 @@ class PicturesController(ControllerBase):
         if self_moderate:
             new.moderator = user
         try:
-            new.generate_thumbnails()
             new.full_clean()
-            new.save()
+            new.generate_thumbnails(save=True)
         except ValidationError as e:
             return self.create_response({"detail": dict(e)}, status_code=409)
 
@@ -177,6 +176,19 @@ class PicturesController(ControllerBase):
     def delete_picture(self, picture_id: int):
         self.get_object_or_exception(Picture, pk=picture_id).delete()
 
+    @route.post(
+        "/{picture_id}/rotate/{direction}",
+        permissions=[IsSasAdmin],
+        response=PictureSchema,
+        url_name="rotate_picture",
+    )
+    def rotate_picture(self, picture_id: int, direction: Literal["left", "right"]):
+        """Rotate the given picture and returns its edited data."""
+        angle = 90 if direction == "left" else 270
+        picture = self.get_object_or_exception(Picture, pk=picture_id)
+        picture.rotate(angle)
+        return picture
+
     @route.patch(
         "/{picture_id}/moderation",
         permissions=[IsSasAdmin],

sas/baker_recipes.py

@@ -1,13 +1,24 @@
+from django.conf import settings
 from model_bakery import seq
-from model_bakery.recipe import Recipe
+from model_bakery.recipe import Recipe, foreign_key
 
-from sas.models import Picture
+from sas.models import Album, Picture
+
+album_recipe = Recipe(
+    Album,
+    is_in_sas=True,
+    is_folder=True,
+    is_moderated=True,
+    parent_id=settings.SITH_SAS_ROOT_DIR_ID,
+    name=seq("Album "),
+)
 
 picture_recipe = Recipe(
     Picture,
     is_in_sas=True,
     is_folder=False,
     is_moderated=True,
+    parent=foreign_key(album_recipe),
     name=seq("Picture "),
 )
 """A SAS Picture fixture.

sas/models.py

@@ -15,8 +15,6 @@
 
 from __future__ import annotations
 
-import contextlib
-from io import BytesIO
 from pathlib import Path
 from typing import ClassVar, Self
 
@@ -30,7 +28,7 @@ from django.utils.translation import gettext_lazy as _
 from PIL import Image
 
 from core.models import Notification, SithFile, User
-from core.utils import exif_auto_rotate, resize_image
+from core.utils import resize_image
 
 
 class SasFile(SithFile):
@@ -92,88 +90,75 @@ class Picture(SasFile):
 
     objects = SASPictureManager.from_queryset(PictureQuerySet)()
 
-    @property
-    def is_vertical(self):
-        with open(settings.MEDIA_ROOT / self.file.name, "rb") as f:
-            im = Image.open(BytesIO(f.read()))
-            (w, h) = im.size
-            return (w / h) < 1
-
     def get_download_url(self):
-        return reverse("sas:download", kwargs={"picture_id": self.id})
+        return reverse(
+            "sas:download",
+            kwargs={"picture_id": self.id},
+            query={"date": int(self.updated_at.timestamp())},
+        )
 
     def get_download_compressed_url(self):
-        return reverse("sas:download_compressed", kwargs={"picture_id": self.id})
+        return reverse(
+            "sas:download_compressed",
+            kwargs={"picture_id": self.id},
+            query={"date": int(self.updated_at.timestamp())},
+        )
 
     def get_download_thumb_url(self):
-        return reverse("sas:download_thumb", kwargs={"picture_id": self.id})
+        return reverse(
+            "sas:download_thumb",
+            kwargs={"picture_id": self.id},
+            query={"date": int(self.updated_at.timestamp())},
+        )
 
     def get_absolute_url(self):
         return reverse("sas:picture", kwargs={"picture_id": self.id})
 
-    def generate_thumbnails(self):
-        im = Image.open(BytesIO(self.file.read()))
-        with contextlib.suppress(Exception):
-            im = exif_auto_rotate(im)
+    def generate_thumbnails(
+        self, *, img: Image.Image | None = None, save: bool = False
+    ):
+        """Generate the thumbnail and the compressed version of this picture.
+
+        Args:
+            img: if given, this will be used to generate
+            all three images (file, compressed, thumbnail).
+            Else, `self.file` will be used
+            save: if True, save the instance in database.
+        """
+        img = img or Image.open(self.file)
+        extension = self.mime_type.split("/")[-1]
+        previous_files = [
+            f.name for f in (self.file, self.thumbnail, self.compressed) if f
+        ]
         # convert the compressed image and the thumbnail into webp
         # The original image keeps its original type, because it's not
         # meant to be shown on the website, but rather to keep the real image
-        # for less frequent cases (like downloading the pictures of an user)
-        extension = self.mime_type.split("/")[-1]
+        # for less frequent cases (like downloading the pictures of a user)
         # the HD version of the image doesn't need to be optimized, because :
         # - it isn't frequently queried
-        # - optimizing large images takes a lot time, which greatly hinders the UX
+        # - optimizing large images takes a lot of time, which greatly hinders the UX
         # - photographers usually already optimize their images
-        file = resize_image(im, max(im.size), extension, optimize=False)
-        thumb = resize_image(im, 200, "webp")
-        compressed = resize_image(im, 1200, "webp")
         new_extension_name = str(Path(self.name).with_suffix(".webp"))
-        self.file = file
-        self.file.name = self.name
-        self.thumbnail = thumb
-        self.thumbnail.name = new_extension_name
-        self.compressed = compressed
-        self.compressed.name = new_extension_name
+        file = resize_image(img, max(img.size), extension, optimize=False)
+        self.file.save(self.name, file, save=False)
+        thumbnail = resize_image(img, 200, "webp")
+        self.thumbnail.save(new_extension_name, thumbnail, save=False)
+        compressed = resize_image(img, 1200, "webp")
+        self.compressed.save(new_extension_name, compressed, save=save)
+        # once the new images have been saved, delete the previous ones.
+        # The deletion of old files is done after, so that if anything goes
+        # during the whole process, no data will be lost.
+        for filename in previous_files:
+            self.file.storage.delete(filename)
 
-    def rotate(self, degree):
-        for attr in ["file", "compressed", "thumbnail"]:
-            name = self.__getattribute__(attr).name
-            with open(settings.MEDIA_ROOT / name, "r+b") as file:
-                if file:
-                    im = Image.open(BytesIO(file.read()))
-                    file.seek(0)
-                    im = im.rotate(degree, expand=True)
-                    im.save(
-                        fp=file,
-                        format=self.mime_type.split("/")[-1].upper(),
-                        quality=90,
-                        optimize=True,
-                        progressive=True,
-                    )
+    def rotate(self, degree: int | float):
+        """Rotate this picture and update its thumbnails accordingly.
 
-    def get_next(self):
-        if self.is_moderated:
-            pictures_qs = self.parent.children.filter(
-                is_moderated=True,
-                asked_for_removal=False,
-                is_folder=False,
-                id__gt=self.id,
-            )
-        else:
-            pictures_qs = Picture.objects.filter(id__gt=self.id, is_moderated=False)
-        return pictures_qs.order_by("id").first()
-
-    def get_previous(self):
-        if self.is_moderated:
-            pictures_qs = self.parent.children.filter(
-                is_moderated=True,
-                asked_for_removal=False,
-                is_folder=False,
-                id__lt=self.id,
-            )
-        else:
-            pictures_qs = Picture.objects.filter(id__lt=self.id, is_moderated=False)
-        return pictures_qs.order_by("-id").first()
+        Args:
+            degree: the rotation angle, in degree, counter-clockwise
+        """
+        img = Image.open(self.file).rotate(degree)
+        self.generate_thumbnails(img=img, save=True)
 
 
 class AlbumQuerySet(models.QuerySet):
@@ -239,7 +224,11 @@ class Album(SasFile):
         return reverse("sas:album", kwargs={"album_id": self.id})
 
     def get_download_url(self):
-        return reverse("sas:album_preview", kwargs={"album_id": self.id})
+        return reverse(
+            "sas:album_preview",
+            kwargs={"album_id": self.id},
+            query={"date": int(self.updated_at.timestamp())},
+        )
 
     def generate_thumbnail(self):
         p = self.children_pictures.order_by("?").first()

sas/schemas.py

@@ -70,7 +70,15 @@ class PictureFilterSchema(FilterSchema):
 class PictureSchema(ModelSchema):
     class Meta:
         model = Picture
-        fields = ["id", "name", "date", "size", "is_moderated", "asked_for_removal"]
+        fields = [
+            "id",
+            "name",
+            "date",
+            "updated_at",
+            "size",
+            "is_moderated",
+            "asked_for_removal",
+        ]
 
     owner: UserProfileSchema
     sas_url: str

sas/static/bundled/sas/user/pictures-index.ts

@@ -1,4 +1,4 @@
-import { paginated } from "#core:utils/api.ts";
+import { paginated } from "#core:utils/api";
 import {
   type PictureSchema,
   type PicturesFetchPicturesData,

sas/static/bundled/sas/viewer-index.ts

@@ -1,7 +1,7 @@
 import type TomSelect from "tom-select";
-import type { UserAjaxSelect } from "#core:core/components/ajax-select-index.ts";
-import { paginated } from "#core:utils/api.ts";
-import { History } from "#core:utils/history.ts";
+import type { UserAjaxSelect } from "#core:core/components/ajax-select-index";
+import { paginated } from "#core:utils/api";
+import { History } from "#core:utils/history";
 import {
   type IdentifiedUserSchema,
   type ModerationRequestSchema,
@@ -14,6 +14,7 @@ import {
   picturesFetchPictures,
   picturesIdentifyUsers,
   picturesModeratePicture,
+  picturesRotatePicture,
   type UserProfileSchema,
   usersidentifiedDeleteRelation,
 } from "#openapi";
@@ -28,18 +29,32 @@ class PictureWithIdentifications {
   identificationsLoading = false;
   moderationLoading = false;
   id: number;
-  // biome-ignore lint/style/useNamingConvention: api is in snake_case
-  compressed_url: string;
+  compressedUrl: string = "";
+  thumbUrl: string = "";
+  fullSizeUrl: string = "";
   moderationRequests: ModerationRequestSchema[] = null;
 
   constructor(picture: PictureSchema) {
     Object.assign(this, picture);
+    this.compressedUrl = picture.compressed_url;
+    this.thumbUrl = picture.thumb_url;
+    this.fullSizeUrl = picture.full_size_url;
   }
 
   static fromPicture(picture: PictureSchema): PictureWithIdentifications {
     return new PictureWithIdentifications(picture);
   }
 
+  rebuildUrls(date: Date) {
+    const buildUrl = (url: string) => {
+      const base = url.split("?", 1)[0];
+      return `${base}?date=${date.getTime().toString()}`;
+    };
+    this.compressedUrl = buildUrl(this.compressedUrl);
+    this.thumbUrl = buildUrl(this.thumbUrl);
+    this.fullSizeUrl = buildUrl(this.fullSizeUrl);
+  }
+
   /**
    * If not already done, fetch the users identified on this picture and
    * populate the identifications field
@@ -82,12 +97,25 @@ class PictureWithIdentifications {
     this.moderationLoading = false;
   }
 
+  async rotate(direction: "left" | "right") {
+    this.imageLoading = true;
+    const res = await picturesRotatePicture({
+      // biome-ignore lint/style/useNamingConvention: api is snake case
+      path: { picture_id: this.id, direction: direction },
+    });
+    // urls returned by the api include a timestamp for cache busting
+    this.fullSizeUrl = res.data.full_size_url;
+    this.compressedUrl = res.data.compressed_url;
+    this.thumbUrl = res.data.thumb_url;
+    this.imageLoading = false;
+  }
+
   /**
    * Preload the photo and the identifications
    */
   async preload(): Promise<void> {
     const img = new Image();
-    img.src = this.compressed_url;
+    img.src = this.compressedUrl;
     if (!img.complete) {
       this.imageLoading = true;
       img.addEventListener("load", () => {
@@ -140,7 +168,8 @@ document.addEventListener("alpine:init", () => {
       // biome-ignore lint/style/useNamingConvention: api is in snake_case
       full_size_url: "",
       owner: "",
-      date: new Date(),
+      // biome-ignore lint/style/useNamingConvention: api is in snake_case
+      created_at: new Date(),
       identifications: [] as IdentifiedUserSchema[],
     },
     /**
@@ -291,10 +320,8 @@ document.addEventListener("alpine:init", () => {
     async submitIdentification(): Promise<void> {
       const widget: TomSelect = this.selector.widget;
       await picturesIdentifyUsers({
-        path: {
-          // biome-ignore lint/style/useNamingConvention: api is in snake_case
-          picture_id: this.currentPicture.id,
-        },
+        // biome-ignore lint/style/useNamingConvention: api is in snake_case
+        path: { picture_id: this.currentPicture.id },
         body: widget.items.map((i: string) => Number.parseInt(i, 10)),
       });
       // refresh the identified users list

sas/static/sas/css/picture.scss

@@ -235,37 +235,34 @@
 
   >.tools {
     flex: 1;
+    .btn {
+      background-color: $primary-neutral-light-color;
+      display: flex;
+      justify-content: center;
+      align-items: center;
+      padding: 0;
+      color: black;
+      width: 40px;
+      height: 40px;
+      font-size: 20px;
 
-    >div>div {
-      >a.btn {
-        background-color: $primary-neutral-light-color;
-        display: flex;
-        justify-content: center;
-        align-items: center;
-        padding: 0;
-        color: black;
-        width: 40px;
-        height: 40px;
-        font-size: 20px;
-
-        &:hover {
-          background-color: #aaa;
-        }
+      &:hover {
+        background-color: #aaa;
       }
+    }
 
-      >a.text.danger {
-        color: red;
+    a.text.danger {
+      color: red;
 
-        &:hover {
-          color: darkred;
-        }
+      &:hover {
+        color: darkred;
       }
+    }
 
-      &.buttons {
-        display: flex;
-        flex-direction: row;
-        gap: 5px;
-      }
+    .buttons {
+      display: flex;
+      flex-direction: row;
+      gap: 5px;
     }
   }
 }

sas/templates/sas/picture.jinja

@@ -1,9 +1,9 @@
 {% extends "core/base.jinja" %}
 
 {%- block additional_css -%}
-  <link defer rel="stylesheet" href="{{ static('bundled/core/components/ajax-select-index.css') }}">
-  <link defer rel="stylesheet" href="{{ static('core/components/ajax-select.scss') }}">
-  <link defer rel="stylesheet" href="{{ static('sas/css/picture.scss') }}">
+  <link rel="stylesheet" href="{{ static('bundled/core/components/ajax-select-index.css') }}">
+  <link rel="stylesheet" href="{{ static('core/components/ajax-select.scss') }}">
+  <link rel="stylesheet" href="{{ static('sas/css/picture.scss') }}">
 {%- endblock -%}
 
 {%- block additional_js -%}
@@ -84,7 +84,7 @@
       <div class="main">
         <div class="photo" :aria-busy="currentPicture.imageLoading">
           <img
-            :src="currentPicture.compressed_url"
+            :src="currentPicture.compressedUrl"
             :alt="currentPicture.name"
             id="main-picture"
             x-ref="mainPicture"
@@ -100,7 +100,7 @@
                 <span
                   x-text="Intl.DateTimeFormat(
                           '{{ LANGUAGE_CODE }}', {dateStyle: 'long'}
-                          ).format(new Date(currentPicture.date))"
+                          ).format(Date.parse(currentPicture.date))"
                 >
                 </span>
               </div>
@@ -115,23 +115,38 @@
             <h5>{% trans %}Tools{% endtrans %}</h5>
             <div>
               <div>
-                <a class="text" :href="currentPicture.full_size_url">
+                <a class="text" :href="currentPicture.fullSizeUrl">
                   {% trans %}HD version{% endtrans %}
                 </a>
                 <a class="text danger " :href="currentPicture.report_url">
                   {% trans %}Ask for removal{% endtrans %}
                 </a>
               </div>
-              <div class="buttons">
+              <div
+                class="buttons"
+                x-show="{{ user.has_perm("sas.change_sasfile")|tojson }} || currentPicture.owner.id === {{ user.id }}"
+              >
                 <a
                   class="btn btn-no-text"
                   :href="currentPicture.edit_url"
-                  x-show="{{ user.has_perm("sas.change_sasfile")|tojson }} || currentPicture.owner.id === {{ user.id }}"
+                  :disabled="currentPicture.imageLoading"
                 >
                   <i class="fa-regular fa-pen-to-square edit-action"></i>
                 </a>
-                <a class="btn btn-no-text" href="?rotate_left"><i class="fa-solid fa-rotate-left"></i></a>
-                <a class="btn btn-no-text" href="?rotate_right"><i class="fa-solid fa-rotate-right"></i></a>
+                <button
+                  class="btn btn-no-text"
+                  @click="currentPicture.rotate('left')"
+                  :disabled="currentPicture.imageLoading"
+                >
+                  <i class="fa-solid fa-rotate-left"></i>
+                </button>
+                <button
+                  class="btn btn-no-text"
+                  @click="currentPicture.rotate('right')"
+                  :disabled="currentPicture.imageLoading"
+                >
+                  <i class="fa-solid fa-rotate-right"></i>
+                </button>
               </div>
             </div>
           </div>
@@ -146,7 +161,7 @@
                 @keyup.left.window="currentPicture = previousPicture"
                 @click="currentPicture = previousPicture"
               >
-                <img :src="previousPicture.thumb_url" alt="{% trans %}Previous picture{% endtrans %}"/>
+                <img :src="previousPicture.thumbUrl" alt="{% trans %}Previous picture{% endtrans %}"/>
                 <div class="overlay">←</div>
               </div>
             </template>
@@ -157,7 +172,7 @@
                 @keyup.right.window="currentPicture = nextPicture"
                 @click="currentPicture = nextPicture"
               >
-                <img :src="nextPicture.thumb_url" alt="{% trans %}Previous picture{% endtrans %}"/>
+                <img :src="nextPicture.thumbUrl" alt="{% trans %}Previous picture{% endtrans %}"/>
                 <div class="overlay">→</div>
               </div>
             </template>

sas/tests/test_model.py

@@ -1,6 +1,11 @@
+from io import BytesIO
+from pathlib import Path
+
 import pytest
+from django.core.files.base import ContentFile
 from django.test import TestCase
 from model_bakery import baker
+from PIL import Image
 
 from core.baker_recipes import old_subscriber_user, subscriber_user
 from core.models import User
@@ -67,3 +72,36 @@ def test_identifications_viewable_by_user():
     assert list(picture.people.viewable_by(identifications[1].user)) == [
         identifications[1]
     ]
+
+
+@pytest.mark.django_db
+@pytest.mark.parametrize("save", [True, False])
+@pytest.mark.parametrize("initially_saved", [True, False])
+@pytest.mark.parametrize("pass_img_kwarg", [True, False])
+def test_generate_thumbnail(save, initially_saved, pass_img_kwarg):
+    """Test that Picture.generate_thumbnails works properly"""
+    image = Image.new("RGB", (2, 1))
+    image.putdata([(255, 0, 0), (0, 255, 0)])
+    buffer = BytesIO()
+    image.save(buffer, format="PNG")
+    file = ContentFile(buffer.getvalue(), "img.png")
+    picture: Picture = picture_recipe.prepare(
+        file=file,
+        name=file.name,
+        mime_type="image/png",
+        _save_related=True,
+    )
+    if initially_saved:
+        picture.save()
+    picture.generate_thumbnails(img=image if pass_img_kwarg else None, save=save)
+    storage = picture.file.storage
+    for f in picture.file, picture.compressed, picture.thumbnail:
+        # the tested picture is alone in its album,
+        # so there should be a single file in each folder
+        assert storage.exists(f.name)
+        _dirs, files = storage.listdir(str(Path(f.path).parent))
+        assert files == [Path(f.name).name]
+    new_img = Image.open(picture.file)
+    assert new_img.get_flattened_data() == image.get_flattened_data()
+    assert Image.open(picture.thumbnail).size == (200, 100)
+    assert Image.open(picture.compressed).size == (1200, 600)

sas/tests/test_views.py

@@ -12,19 +12,23 @@
 # OR WITHIN THE LOCAL FILE "LICENSE"
 #
 #
-from typing import Callable
+from typing import Callable, Literal
+from unittest.mock import patch
 
 import pytest
 from bs4 import BeautifulSoup
 from django.conf import settings
 from django.core.cache import cache
+from django.core.files.base import ContentFile
 from django.test import Client, TestCase
 from django.urls import reverse
 from model_bakery import baker
+from PIL import Image
 from pytest_django.asserts import assertHTMLEqual, assertInHTML, assertRedirects
 
 from core.baker_recipes import old_subscriber_user, subscriber_user
 from core.models import Group, User
+from core.utils import RED_PIXEL_PNG
 from sas.baker_recipes import picture_recipe
 from sas.models import Album, Picture
 
@@ -162,6 +166,71 @@ class TestAlbumUpload:
         assert not album.children.exists()
 
 
+@pytest.mark.django_db
+class TestPictureRotation:
+    @pytest.fixture
+    def picture(self) -> Picture:
+        return picture_recipe.make(
+            parent_id=settings.SITH_SAS_ROOT_DIR_ID,
+            file=ContentFile(name="foo.png", content=RED_PIXEL_PNG),
+            compressed=ContentFile(name="foo.png", content=RED_PIXEL_PNG),
+            thumbnail=ContentFile(name="foo.png", content=RED_PIXEL_PNG),
+        )
+
+    @pytest.mark.parametrize(
+        "user",
+        [
+            None,
+            lambda: baker.make(User),
+            subscriber_user.make,
+            old_subscriber_user.make,
+        ],
+    )
+    def test_permission_denied(
+        self, client: Client, picture: Picture, user: Callable[[], User] | None
+    ):
+        if user:
+            client.force_login(user())
+
+        url = reverse(
+            "api:rotate_picture", kwargs={"picture_id": picture.id, "direction": "left"}
+        )
+        response = client.post(url)
+        assert response.status_code == 403 if user else 401
+
+    @pytest.mark.parametrize(
+        "user",
+        [
+            lambda: baker.make(User, is_superuser=True),
+            lambda: baker.make(
+                User, groups=[Group.objects.get(pk=settings.SITH_GROUP_SAS_ADMIN_ID)]
+            ),
+        ],
+    )
+    @pytest.mark.parametrize(("direction", "angle"), [("left", 90), ("right", 270)])
+    def test_rotation(
+        self,
+        client: Client,
+        picture: Picture,
+        user: Callable[[], User],
+        direction: Literal["left", "right"],
+        angle: Literal[90, 270],
+    ):
+        client.force_login(user())
+        url = reverse(
+            "api:rotate_picture",
+            kwargs={"picture_id": picture.id, "direction": direction},
+        )
+        with (
+            patch.object(Image.Image, "rotate") as mocked_rotate,
+            patch.object(Picture, "generate_thumbnails") as mocked_thumb,
+        ):
+            response = client.post(url)
+            assert response.status_code == 200
+            mocked_rotate.assert_called_once_with(angle)
+            mocked_thumb.assert_called_once()
+
+
 class TestSasModeration(TestCase):
     @classmethod
     def setUpTestData(cls):

sas/views.py

@@ -97,14 +97,6 @@ class PictureView(CanViewMixin, DetailView):
     pk_url_kwarg = "picture_id"
     template_name = "sas/picture.jinja"
 
-    def get(self, request, *args, **kwargs):
-        self.object = self.get_object()
-        if "rotate_right" in request.GET:
-            self.object.rotate(270)
-        if "rotate_left" in request.GET:
-            self.object.rotate(90)
-        return super().get(request, *args, **kwargs)
-
     def get_context_data(self, **kwargs):
         return super().get_context_data(**kwargs) | {
             "album": Album.objects.get(children=self.object)