From d3b203a4a166907fdb84cdbe619561422fec8b0c Mon Sep 17 00:00:00 2001
From: thomas girod <thgirod@hotmail.com>
Date: Tue, 6 Aug 2024 13:23:34 +0200
Subject: [PATCH] change cache on picture download

---
 core/models.py          |  2 +-
 sas/api.py              |  3 ++-
 sas/models.py           | 27 ++++++++++++-----------
 sas/tests/test_model.py | 48 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 15 deletions(-)
 create mode 100644 sas/tests/test_model.py

diff --git a/core/models.py b/core/models.py
index 3803e3ca..1f5b2109 100644
--- a/core/models.py
+++ b/core/models.py
@@ -982,7 +982,7 @@ class SithFile(models.Model):
             return True
         if self.is_in_sas and user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID):
             return True
-        return user.id == self.owner.id
+        return user.id == self.owner_id
 
     def can_be_viewed_by(self, user):
         if hasattr(self, "profile_of"):
diff --git a/sas/api.py b/sas/api.py
index f4b98fc3..dffded87 100644
--- a/sas/api.py
+++ b/sas/api.py
@@ -45,12 +45,13 @@ class PicturesController(ControllerBase):
             # User can view any moderated picture if he/she is subscribed.
             # If not, he/she can view only the one he/she has been identified on
             raise PermissionDenied
-        return (
+        pictures = list(
             filters.filter(Picture.objects.viewable_by(user))
             .distinct()
             .order_by("-parent__date", "date")
             .annotate(album=F("parent__name"))
         )
+        return pictures
 
 
 @api_controller("/sas/relation", tags="User identification on SAS pictures")
diff --git a/sas/models.py b/sas/models.py
index c93e0a0d..f7ed8b02 100644
--- a/sas/models.py
+++ b/sas/models.py
@@ -13,8 +13,9 @@
 #
 #
 
+from __future__ import annotations
+
 from io import BytesIO
-from typing import Self
 
 from django.conf import settings
 from django.core.cache import cache
@@ -29,7 +30,7 @@ from core.utils import exif_auto_rotate, resize_image
 
 
 class PictureQuerySet(models.QuerySet):
-    def viewable_by(self, user: User) -> Self:
+    def viewable_by(self, user: User) -> PictureQuerySet:
         """Filter the pictures that this user can view.
 
         Warnings:
@@ -39,7 +40,7 @@ class PictureQuerySet(models.QuerySet):
             return self.all()
         if user.was_subscribed:
             return self.filter(is_moderated=True)
-        return self.filter(people__user_id=user.id)
+        return self.filter(people__user_id=user.id, is_moderated=True)
 
 
 class SASPictureManager(models.Manager):
@@ -76,19 +77,19 @@ class Picture(SithFile):
         return perm
 
     def can_be_viewed_by(self, user: User) -> bool:
-        # SAS pictures are visible to old subscribers
-        # Result is cached 4s for this user
         if user.is_anonymous:
             return False
 
-        perm = cache.get("%d_can_view_pictures" % (user.id), False)
-        if not perm:
-            perm = user.was_subscribed
-
-        cache.set("%d_can_view_pictures" % (user.id), perm, timeout=4)
-        return (perm and self.is_moderated and self.is_in_sas) or self.can_be_edited_by(
-            user
-        )
+        cache_key = f"sas:pictures_viewable_by_{user.id}_in_{self.parent_id}"
+        viewable: list[int] | None = cache.get(cache_key)
+        if viewable is None:
+            viewable = list(
+                Picture.objects.filter(parent_id=self.parent_id)
+                .viewable_by(user)
+                .values_list("pk", flat=True)
+            )
+            cache.set(cache_key, viewable, timeout=10)
+        return self.id in viewable
 
     def get_download_url(self):
         return reverse("sas:download", kwargs={"picture_id": self.id})
diff --git a/sas/tests/test_model.py b/sas/tests/test_model.py
new file mode 100644
index 00000000..76d3b2cb
--- /dev/null
+++ b/sas/tests/test_model.py
@@ -0,0 +1,48 @@
+from django.test import TestCase
+from model_bakery import baker, seq
+
+from core.baker_recipes import old_subscriber_user, subscriber_user
+from core.models import User
+from sas.models import Picture
+
+
+class TestPictureQuerySet(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        Picture.objects.all().delete()
+        cls.pictures = baker.make(
+            Picture,
+            is_moderated=True,
+            is_in_sas=True,
+            is_folder=False,
+            name=seq(""),
+            _quantity=10,
+            _bulk_create=True,
+        )
+        Picture.objects.filter(pk=cls.pictures[0].id).update(is_moderated=False)
+
+    def test_root(self):
+        root = baker.make(User, is_superuser=True)
+        pictures = list(Picture.objects.viewable_by(root))
+        self.assertCountEqual(pictures, self.pictures)
+
+    def test_subscriber(self):
+        subscriber = subscriber_user.make()
+        old_subcriber = old_subscriber_user.make()
+        for user in (subscriber, old_subcriber):
+            pictures = list(Picture.objects.viewable_by(user))
+            self.assertCountEqual(pictures, self.pictures[1:])
+
+    def test_not_subscribed_identified(self):
+        user = baker.make(
+            # This is the guy who asked the feature of making pictures
+            # available for tagged users, even if not subscribed
+            User,
+            first_name="Pierrick",
+            last_name="Dheilly",
+            nick_name="Sahmer",
+        )
+        user.pictures.create(picture=self.pictures[0])
+        user.pictures.create(picture=self.pictures[1])
+        pictures = list(Picture.objects.viewable_by(user))
+        assert pictures == [self.pictures[1]]