Merge pull request #819 from ae-utbm/fix-delete-picture

fix undeletable SAS pictures

core/models.py

@@ -944,40 +944,15 @@ class SithFile(models.Model):
                     param="1",
                 ).save()
 
-    def can_be_managed_by(self, user: User) -> bool:
-        """Tell if the user can manage the file (edit, delete, etc.) or not.
-        Apply the following rules:
-            - If the file is not in the SAS nor in the profiles directory, it can be "managed" by anyone -> return True
-            - If the file is in the SAS, only the SAS admins (or roots) can manage it -> return True if the user is in the SAS admin group or is a root
-            - If the file is in the profiles directory, only the roots can manage it -> return True if the user is a root.
-
-        Returns:
-            True if the file is managed by the SAS or within the profiles directory, False otherwise
-        """
-        # If the file is not in the SAS nor in the profiles directory, it can be "managed" by anyone
-        profiles_dir = SithFile.objects.filter(name="profiles").first()
-        if not self.is_in_sas and not profiles_dir in self.get_parent_list():
-            return True
-
-        # If the file is in the SAS, only the SAS admins (or roots) can manage it
-        if self.is_in_sas and (
-            user.is_in_group(settings.SITH_GROUP_SAS_ADMIN_ID) or user.is_root
-        ):
-            return True
-
-        # If the file is in the profiles directory, only the roots can manage it
-        if profiles_dir in self.get_parent_list() and (
-            user.is_root or user.is_board_member
-        ):
-            return True
-
-        return False
-
     def is_owned_by(self, user):
         if user.is_anonymous:
             return False
-        if hasattr(self, "profile_of") and user.is_board_member:
+        if user.is_root:
             return True
+        if hasattr(self, "profile_of"):
+            # if the `profile_of` attribute is set, this file is a profile picture
+            # and profile pictures may only be edited by board members
+            return user.is_board_member
         if user.is_com_admin:
             return True
         if self.is_in_sas and user.is_in_group(pk=settings.SITH_GROUP_SAS_ADMIN_ID):
@@ -993,7 +968,7 @@ class SithFile(models.Model):
             return user.can_view(self.scrub_of)
         return False
 
-    def delete(self):
+    def delete(self, *args, **kwargs):
         for c in self.children.all():
             c.delete()
         self.file.delete()

core/views/files.py

@@ -190,13 +190,6 @@ class FileEditView(CanEditMixin, UpdateView):
     template_name = "core/file_edit.jinja"
     context_object_name = "file"
 
-    def get(self, request, *args, **kwargs):
-        self.object = self.get_object()
-        if not self.object.can_be_managed_by(request.user):
-            raise PermissionDenied
-
-        return super().get(request, *args, **kwargs)
-
     def get_form_class(self):
         fields = ["name", "is_moderated"]
         if self.object.is_file:
@@ -242,13 +235,6 @@ class FileEditPropView(CanEditPropMixin, UpdateView):
     context_object_name = "file"
     form_class = FileEditPropForm
 
-    def get(self, request, *args, **kwargs):
-        self.object = self.get_object()
-        if not self.object.can_be_managed_by(request.user):
-            raise PermissionDenied
-
-        return super().get(request, *args, **kwargs)
-
     def get_form(self, form_class=None):
         form = super().get_form(form_class)
         form.fields["parent"].queryset = SithFile.objects.filter(is_folder=True)
@@ -322,9 +308,6 @@ class FileView(CanViewMixin, DetailView, FormMixin):
 
     def get(self, request, *args, **kwargs):
         self.form = self.get_form()
-        if not self.object.can_be_managed_by(request.user):
-            raise PermissionDenied
-
         if "clipboard" not in request.session.keys():
             request.session["clipboard"] = []
         return super().get(request, *args, **kwargs)
@@ -372,13 +355,6 @@ class FileDeleteView(CanEditPropMixin, DeleteView):
     template_name = "core/file_delete_confirm.jinja"
     context_object_name = "file"
 
-    def get(self, request, *args, **kwargs):
-        self.object = self.get_object()
-        if not self.object.can_be_managed_by(request.user):
-            raise PermissionDenied
-
-        return super().get(request, *args, **kwargs)
-
     def get_success_url(self):
         self.object.file.delete()  # Doing it here or overloading delete() is the same, so let's do it here
         if "next" in self.request.GET.keys():
@@ -416,6 +392,7 @@ class FileModerateView(CanEditPropMixin, SingleObjectMixin):
     model = SithFile
     pk_url_kwarg = "file_id"
 
+    # FIXME : wrong http method. This should be a POST or a DELETE request
     def get(self, request, *args, **kwargs):
         self.object = self.get_object()
         self.object.is_moderated = True

core/views/user.py

@@ -561,6 +561,8 @@ class UserListView(ListView, CanEditPropMixin):
     template_name = "core/user_list.jinja"
 
 
+# FIXME: the edit_once fields aren't displayed to the user (as expected).
+#  However, if the user re-add them manually in the form, they are saved.
 class UserUpdateProfileView(UserTabsMixin, CanEditMixin, UpdateView):
     """Edit a user's profile."""
 

sas/tests/test_views.py

@@ -17,14 +17,15 @@ from typing import Callable
 import pytest
 from django.conf import settings
 from django.core.cache import cache
-from django.test import Client
+from django.test import Client, TestCase
 from django.urls import reverse
 from model_bakery import baker
+from pytest_django.asserts import assertRedirects
 
 from core.baker_recipes import old_subscriber_user, subscriber_user
 from core.models import RealGroup, User
 from sas.baker_recipes import picture_recipe
-from sas.models import Album
+from sas.models import Album, Picture
 
 # Create your tests here.
 
@@ -64,3 +65,70 @@ def test_album_access_non_subscriber(client: Client):
     cache.clear()
     res = client.get(reverse("sas:album", kwargs={"album_id": album.id}))
     assert res.status_code == 200
+
+
+class TestSasModeration(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        album = baker.make(Album, parent_id=settings.SITH_SAS_ROOT_DIR_ID)
+        cls.pictures = picture_recipe.make(
+            parent=album, _quantity=10, _bulk_create=True
+        )
+        cls.to_moderate = cls.pictures[0]
+        cls.to_moderate.is_moderated = False
+        cls.to_moderate.save()
+        cls.moderator = baker.make(
+            User, groups=[RealGroup.objects.get(pk=settings.SITH_GROUP_SAS_ADMIN_ID)]
+        )
+        cls.simple_user = subscriber_user.make()
+
+    def test_moderation_page_sas_admin(self):
+        """Test that a moderator can see the pictures needing moderation."""
+        self.client.force_login(self.moderator)
+        res = self.client.get(reverse("sas:moderation"))
+        assert res.status_code == 200
+        assert len(res.context_data["pictures"]) == 1
+        assert res.context_data["pictures"][0] == self.to_moderate
+
+        res = self.client.post(
+            reverse("sas:moderation"),
+            data={"album_id": self.to_moderate.id, "picture_id": self.to_moderate.id},
+        )
+
+    def test_moderation_page_forbidden(self):
+        self.client.force_login(self.simple_user)
+        res = self.client.get(reverse("sas:moderation"))
+        assert res.status_code == 403
+
+    def test_moderate_picture(self):
+        self.client.force_login(self.moderator)
+        res = self.client.get(
+            reverse("core:file_moderate", kwargs={"file_id": self.to_moderate.id}),
+            data={"next": self.pictures[1].get_absolute_url()},
+        )
+        assertRedirects(res, self.pictures[1].get_absolute_url())
+        self.to_moderate.refresh_from_db()
+        assert self.to_moderate.is_moderated
+
+    def test_delete_picture(self):
+        self.client.force_login(self.moderator)
+        res = self.client.post(
+            reverse("core:file_delete", kwargs={"file_id": self.to_moderate.id})
+        )
+        assert res.status_code == 302
+        assert not Picture.objects.filter(pk=self.to_moderate.id).exists()
+
+    def test_moderation_action_non_authorized_user(self):
+        """Test that a non-authorized user cannot moderate a picture."""
+        self.client.force_login(self.simple_user)
+        res = self.client.post(
+            reverse("core:file_moderate", kwargs={"file_id": self.to_moderate.id}),
+        )
+        assert res.status_code == 403
+        self.to_moderate.refresh_from_db()
+        assert not self.to_moderate.is_moderated
+        res = self.client.post(
+            reverse("core:file_delete", kwargs={"file_id": self.to_moderate.id}),
+        )
+        assert res.status_code == 403
+        assert Picture.objects.filter(pk=self.to_moderate.id).exists()

sas/views.py

@@ -333,10 +333,9 @@ class ModerationView(TemplateView):
         kwargs["albums_to_moderate"] = Album.objects.filter(
             is_moderated=False, is_in_sas=True, is_folder=True
         ).order_by("id")
-        kwargs["pictures"] = Picture.objects.filter(is_moderated=False)
-        kwargs["albums"] = Album.objects.filter(
-            id__in=kwargs["pictures"].values("parent").distinct("parent")
-        )
+        pictures = Picture.objects.filter(is_moderated=False).select_related("parent")
+        kwargs["pictures"] = pictures
+        kwargs["albums"] = [p.parent for p in pictures]
         return kwargs