Revamp threads validation to return list of id's that 404'd

misago/threads/serializers/moderation.py

@@ -479,45 +479,50 @@ class MergeThreadsSerializer(NewThreadSerializer):
                 POSTS_LIMIT,
             )
             raise ValidationError(message % {'limit': THREADS_LIMIT})
+        return data
+    
+    def get_valid_threads(self, threads_ids):
+        user = self.context['user']
 
         threads_tree_id = trees_map.get_tree_id_for_root(THREADS_ROOT_NAME)
-
         threads_queryset = Thread.objects.filter(
-            id__in=data,
+            id__in=threads_ids,
             category__tree_id=threads_tree_id,
         ).select_related('category').order_by('-id')
 
-        user = self.context['user']
-
-        threads = []
+        invalid_threads = []
+        valid_threads = []
         for thread in threads_queryset:
             add_acl(user, thread)
             if can_see_thread(user, thread):
-                threads.append(thread)
-
-        if len(threads) != len(data):
-            raise ValidationError(_("One or more threads to merge could not be found."))
-
-        return threads
-    
-    def validate_threads_permissions(self, threads):
-        user = self.context['user']
-        invalid_threads = []
-        for thread in threads:
-            try:
-                allow_merge_thread(user, thread)
-            except PermissionDenied as permission_error:
-                invalid_threads.append({
-                    'status': 403,
-                    'id': thread.pk,
-                    'detail': permission_error
-                })
+                valid_threads.append(thread)
+                try:
+                    allow_merge_thread(user, thread)
+                except PermissionDenied as permission_error:
+                    invalid_threads.append({
+                        'id': thread.id,
+                        'status': 403,
+                        'detail': permission_error
+                    })
+
+        not_found_ids = set(threads_ids) - set([t.id for t in valid_threads])
+        for not_found_id in not_found_ids:
+            invalid_threads.append({
+                'id': not_found_id,
+                'status': 404,
+                'detail': _(
+                    "Requested thread doesn't exist or you don't have permission to see it."
+                ),
+            })
 
         if invalid_threads:
+            invalid_threads.sort(key=lambda e: e['id'])
             raise ValidationError({'merge': invalid_threads})
 
+        return valid_threads
+
     def validate(self, data):
-        self.validate_threads_permissions(data['threads'])
+        data['threads'] = self.get_valid_threads(data['threads'])
         data['poll'] = validate_poll_merge(data['threads'], data)
         return data
 

misago/threads/tests/test_threads_merge_api.py

@@ -142,6 +142,10 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
 
     def test_merge_with_nonexisting_thread(self):
         """api validates if we are trying to merge with invalid thread"""
+        self.override_acl({
+            'can_merge_threads': True,
+        })
+
         response = self.client.post(
             self.api_link,
             json.dumps({
@@ -153,13 +157,26 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
         )
         self.assertEqual(response.status_code, 400)
         self.assertEqual(
-            response.json(), {
-                'threads': ["One or more threads to merge could not be found."],
-            }
+            response.json(),
+            {
+                'merge': [
+                    {
+                        'id': str(self.thread.id + 1000),
+                        'status': '404',
+                        'detail': (
+                            "Requested thread doesn't exist or you "
+                            "don't have permission to see it."
+                        )
+                    },
+                ],
+            },
         )
 
     def test_merge_with_invisible_thread(self):
         """api validates if we are trying to merge with inaccesible thread"""
+        self.override_acl({
+            'can_merge_threads': True,
+        })
         unaccesible_thread = testutils.post_thread(category=self.category_b)
 
         response = self.client.post(
@@ -173,9 +190,19 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
         )
         self.assertEqual(response.status_code, 400)
         self.assertEqual(
-            response.json(), {
-                'threads': ["One or more threads to merge could not be found."],
-            }
+            response.json(),
+            {
+                'merge': [
+                    {
+                        'id': str(unaccesible_thread.id),
+                        'status': '404',
+                        'detail': (
+                            "Requested thread doesn't exist or you "
+                            "don't have permission to see it."
+                        )
+                    },
+                ],
+            },
         )
 
     def test_merge_no_permission(self):
@@ -196,19 +223,56 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             response.json(), {
                 'merge': [
                     {
+                        'id': str(self.thread.pk),
                         'status': '403',
-                        'id': str(thread.pk),
                         'detail': "You can't merge threads in this category.",
                     },
                     {
+                        'id': str(thread.pk),
                         'status': '403',
-                        'id': str(self.thread.pk),
                         'detail': "You can't merge threads in this category.",
                     },
                 ],
             }
         )
 
+    def test_merge_no_permission_with_invisible_thread(self):
+        """api validates if we are trying to merge with inaccesible thread without permission"""
+        unaccesible_thread = testutils.post_thread(category=self.category_b)
+
+        response = self.client.post(
+            self.api_link,
+            json.dumps({
+                'category': self.category.pk,
+                'title': 'Lorem ipsum dolor',
+                'threads': [self.thread.id, unaccesible_thread.id],
+            }),
+            content_type="application/json",
+        )
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(
+            response.json(),
+            {
+                'merge': [
+                    {
+                        'id': str(self.thread.id),
+                        'status': '403',
+                        'detail': (
+                            "You can't merge threads in this category."
+                        )
+                    },
+                    {
+                        'id': str(unaccesible_thread.id),
+                        'status': '404',
+                        'detail': (
+                            "Requested thread doesn't exist or you "
+                            "don't have permission to see it."
+                        )
+                    },
+                ],
+            },
+        )
+
     def test_thread_category_is_closed(self):
         """api validates if thread's category is open"""
         self.override_acl({
@@ -237,12 +301,12 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
                 'merge': [
                     {
                         'status': '403',
-                        'id': str(other_thread.pk),
+                        'id': str(self.thread.pk),
                         'detail': "This category is closed. You can't merge it's threads.",
                     },
                     {
                         'status': '403',
-                        'id': str(self.thread.pk),
+                        'id': str(other_thread.pk),
                         'detail': "This category is closed. You can't merge it's threads.",
                     },
                 ],
@@ -275,8 +339,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             response.json(), {
                 'merge': [
                     {
-                        'status': '403',
                         'id': str(other_thread.pk),
+                        'status': '403',
                         'detail': "This thread is closed. You can't merge it with other threads.",
                     },
                 ],