#893: moved delete threads to serializers

misago/threads/api/threadendpoints/delete.py

@@ -1,20 +1,12 @@
 from rest_framework.response import Response
 
-from django.core.exceptions import PermissionDenied
 from django.db import transaction
-from django.http import Http404
-from django.utils.six import text_type
-from django.utils.translation import ugettext as _
-from django.utils.translation import ungettext
 
-from misago.conf import settings
-from misago.core.utils import clean_ids_list
-from misago.threads.permissions import allow_delete_thread
 from misago.threads.moderation import threads as moderation
+from misago.threads.permissions import allow_delete_thread
+from misago.threads.serializers import DeleteThreadsSerializer
 
 
-DELETE_LIMIT = settings.MISAGO_THREADS_PER_PAGE + settings.MISAGO_THREADS_TAIL
-
 
 @transaction.atomic
 def delete_thread(request, thread):
@@ -24,43 +16,35 @@ def delete_thread(request, thread):
 
 
 def delete_bulk(request, viewmodel):
-    threads_ids = clean_threads_ids(request)
+    serializer = DeleteThreadsSerializer(
+        data={
+            'threads': request.data,
+        },
+        context={
+            'request': request,
+            'viewmodel': viewmodel,
+        },
+    )
 
-    errors = []
-    for thread_id in threads_ids:
-        try:
-            thread = viewmodel(request, thread_id).unwrap()
+    if not serializer.is_valid():
+        if 'threads' in serializer.errors:
+            errors = serializer.errors['threads']
+            if 'details' in errors:
+                return Response(
+                    hydrate_error_details(errors['details']), status=400)
+            return Response({'detail': errors[0]}, status=403)
+        else:
+            errors = list(serializer.errors)[0][0]
+            return Response({'detail': errors}, status=400)
+
+    for thread in serializer.validated_data['threads']:
+        with transaction.atomic():
             delete_thread(request, thread)
-        except PermissionDenied as e:
-            errors.append({
-                'thread': {
-                    'id': thread.id,
-                    'title': thread.title
-                },
-                'error': text_type(e)
-            })
-        except Http404:
-            pass # skip invisible threads
 
-    if errors:
-        return Response(errors, status=400)
     return Response([])
 
 
-def clean_threads_ids(request):
-    threads_ids = clean_ids_list(
-        request.data or [],
-        _("One or more thread ids received were invalid."),
-    )
-
-    if not threads_ids:
-        raise PermissionDenied(_("You have to specify at least one thread to delete."))
-    elif len(threads_ids) > DELETE_LIMIT:
-        message = ungettext(
-            "No more than %(limit)s thread can be deleted at single time.",
-            "No more than %(limit)s threads can be deleted at single time.",
-            DELETE_LIMIT,
-        )
-        raise PermissionDenied(message % {'limit': DELETE_LIMIT})
-
-    return sorted(set(threads_ids), reverse=True)
+def hydrate_error_details(errors):
+    for error in errors:
+        error['thread']['id'] = int(error['thread']['id'])
+    return errors

misago/threads/serializers/moderation.py

@@ -2,6 +2,7 @@ from rest_framework import serializers
 
 from django.core.exceptions import PermissionDenied, ValidationError
 from django.http import Http404
+from django.utils import six
 from django.utils.translation import ugettext as _, ugettext_lazy, ungettext
 
 from misago.acl import add_acl
@@ -9,8 +10,10 @@ from misago.categories import THREADS_ROOT_NAME
 from misago.conf import settings
 from misago.threads.models import Thread
 from misago.threads.permissions import (
-    allow_delete_event, allow_delete_post, allow_merge_post, allow_merge_thread,
-    allow_move_post, allow_split_post, can_reply_thread, can_see_thread,
+    allow_delete_event, allow_delete_post, allow_delete_thread,
+    allow_merge_post, allow_merge_thread,
+    allow_move_post, allow_split_post,
+    can_reply_thread, can_see_thread,
     can_start_thread, exclude_invisible_posts)
 from misago.threads.pollmergehandler import PollMergeHandler
 from misago.threads.threadtypes import trees_map
@@ -19,11 +22,12 @@ from misago.threads.validators import validate_category, validate_title
 
 
 POSTS_LIMIT = settings.MISAGO_POSTS_PER_PAGE + settings.MISAGO_POSTS_TAIL
-THREADS_LIMIT = 20
+THREADS_LIMIT = settings.MISAGO_THREADS_PER_PAGE + settings.MISAGO_THREADS_TAIL
 
 
 __all__ = [
     'DeletePostsSerializer',
+    'DeleteThreadsSerializer',
     'MergePostsSerializer',
     'MergeThreadSerializer',
     'MergeThreadsSerializer',
@@ -345,6 +349,63 @@ class SplitPostsSerializer(NewThreadSerializer):
         return posts
 
 
+class DeleteThreadsSerializer(serializers.Serializer):
+    error_empty_or_required = ugettext_lazy("You have to specify at least one thread to delete.")
+
+    threads = serializers.ListField(
+        allow_empty=False,
+        child=serializers.IntegerField(
+            error_messages={
+                'invalid': ugettext_lazy("One or more thread ids received were invalid."),
+            },
+        ),
+        error_messages={
+            'required': error_empty_or_required,
+            'null': error_empty_or_required,
+            'empty': error_empty_or_required,
+        },
+    )
+
+    def validate_threads(self, data):
+        if len(data) > THREADS_LIMIT:
+            message = ungettext(
+                "No more than %(limit)s thread can be deleted at single time.",
+                "No more than %(limit)s threads can be deleted at single time.",
+                THREADS_LIMIT,
+            )
+            raise ValidationError(message % {'limit': THREADS_LIMIT})
+
+        request = self.context['request']
+        viewmodel = self.context['viewmodel']
+
+        threads = []
+        errors = []
+
+        for thread_id in data:
+            try:
+                thread = viewmodel(request, thread_id).unwrap()
+                allow_delete_thread(request.user, thread)
+                threads.append(thread)
+            except PermissionDenied as e:
+                errors.append({
+                    'thread': {
+                        'id': thread.id,
+                        'title': thread.title
+                    },
+                    'error': six.text_type(e)
+                })
+            except Http404 as e:
+                pass # skip invisible threads
+
+        if errors:
+            raise serializers.ValidationError({'details': errors})
+
+        if len(threads) != len(data):
+            raise ValidationError(_("One or more threads to delete could not be found."))
+
+        return threads
+
+
 class MergeThreadSerializer(serializers.Serializer):
     other_thread = serializers.CharField(
         error_messages={

misago/threads/tests/test_threads_bulkdelete_api.py

@@ -7,6 +7,7 @@ from misago.categories import PRIVATE_THREADS_ROOT_NAME
 from misago.categories.models import Category
 from misago.threads import testutils
 from misago.threads.models import Thread
+from misago.threads.serializers.moderation import THREADS_LIMIT
 from misago.threads.threadtypes import trees_map
 
 from .test_threads_api import ThreadsApiTestCase
@@ -58,10 +59,10 @@ class ThreadsBulkDeleteApiTests(ThreadsApiTestCase):
         })
 
         response = self.delete(self.api_link, True)
-        self.assertContains(response, "One or more thread ids received were invalid.", status_code=403)
+        self.assertContains(response, "Expected a list of items", status_code=403)
 
         response = self.delete(self.api_link, 'abbss')
-        self.assertContains(response, "One or more thread ids received were invalid.", status_code=403)
+        self.assertContains(response, "Expected a list of items", status_code=403)
 
         response = self.delete(self.api_link, [1, 2, 3, 'a', 'b', 'x'])
         self.assertContains(response, "One or more thread ids received were invalid.", status_code=403)
@@ -73,8 +74,12 @@ class ThreadsBulkDeleteApiTests(ThreadsApiTestCase):
             'can_hide_threads': 2,
         })
 
-        response = self.delete(self.api_link, list(range(100)))
-        self.assertContains(response, "No more than 40 threads can be deleted at single time.", status_code=403)
+        response = self.delete(self.api_link, list(range(THREADS_LIMIT + 1)))
+        self.assertContains(
+            response,
+            "No more than {} threads can be deleted at single time.".format(THREADS_LIMIT),
+            status_code=403,
+        )
 
     def test_validate_thread_visibility(self):
         """api valdiates if user can see deleted thread"""
@@ -91,20 +96,11 @@ class ThreadsBulkDeleteApiTests(ThreadsApiTestCase):
         threads_ids = [p.id for p in self.threads]
 
         response = self.delete(self.api_link, threads_ids)
+        self.assertContains(response, "threads to delete could not be found", status_code=403)
 
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response.json(), [])
-
-        # unapproved thread wasn't deleted
-        Thread.objects.get(pk=unapproved_thread.pk)
-
-        deleted_threads = [self.threads[0], self.threads[2]]
-        for thread in deleted_threads:
-            with self.assertRaises(Thread.DoesNotExist):
-                Thread.objects.get(pk=thread.pk)
-
-        category = Category.objects.get(pk=self.category.pk)
-        self.assertNotIn(category.last_thread_id, threads_ids)
+        # no thread was deleted
+        for thread in self.threads:
+            Thread.objects.get(pk=thread.pk)
 
     def test_delete_other_user_thread_no_permission(self):
         """api valdiates if user can delete other users threads"""
@@ -128,15 +124,9 @@ class ThreadsBulkDeleteApiTests(ThreadsApiTestCase):
             }
         ])
 
-        Thread.objects.get(pk=self.threads[1].pk)
-
-        deleted_threads = [self.threads[0], self.threads[2]]
-        for thread in deleted_threads:
-            with self.assertRaises(Thread.DoesNotExist):
-                Thread.objects.get(pk=thread.pk)
-
-        category = Category.objects.get(pk=self.category.pk)
-        self.assertEqual(category.last_thread_id, self.threads[1].pk)
+        # no threads are removed on failed attempt
+        for thread in self.threads:
+            Thread.objects.get(pk=thread.pk)
 
     def test_delete_thread_closed_category_no_permission(self):
         """api tests category's closed state"""
@@ -159,7 +149,7 @@ class ThreadsBulkDeleteApiTests(ThreadsApiTestCase):
                     'title': thread.title
                 },
                 'error': "This category is closed. You can't delete threads in it."
-            } for thread in sorted(self.threads, key=lambda i: i.pk, reverse=True)
+            } for thread in sorted(self.threads, key=lambda i: i.pk)
         ])
 
     def test_delete_thread_closed_no_permission(self):
@@ -210,15 +200,7 @@ class ThreadsBulkDeleteApiTests(ThreadsApiTestCase):
 
         response = self.delete(self.api_link, threads_ids)
 
-        self.assertEqual(response.status_code, 200)
-        self.assertEqual(response.json(), [])
+        self.assertEqual(response.status_code, 403)
+        self.assertContains(response, "threads to delete could not be found", status_code=403)
 
         Thread.objects.get(pk=private_thread.pk)
-
-        deleted_threads = [self.threads[1], self.threads[2]]
-        for thread in deleted_threads:
-            with self.assertRaises(Thread.DoesNotExist):
-                Thread.objects.get(pk=thread.pk)
-
-        category = Category.objects.get(pk=self.category.pk)
-        self.assertNotIn(category.last_thread_id, threads_ids)