Move logic around merge views and serializers

misago/threads/api/threadendpoints/merge.py

@@ -26,24 +26,20 @@ def thread_merge_endpoint(request, thread, viewmodel):
         },
     )
 
-    if not serializer.is_valid():
-        return Response(serializer.errors, status=400)
-
-    # interrupt merge with request for poll resolution?
-    if serializer.validated_data.get('polls'):
-        return Response({'polls': serializer.validated_data['polls']}, status=400)
+    serializer.is_valid(raise_exception=True)
 
     # merge polls
     other_thread = serializer.validated_data['other_thread']
-    poll = serializer.validated_data.get('poll')
+    poll = serializer.validated_data['poll']
 
-    if len(serializer.polls_handler.polls) == 1:
-        poll.move(other_thread)
-    elif serializer.polls_handler.is_merge_conflict():
-        if poll and poll.thread_id != other_thread.id:
+    if poll:
+        if hasattr(other_thread, 'poll') and poll != other_thread.poll:
             other_thread.poll.delete()
-            poll.move(other_thread)
-        elif not poll:
+        poll.move(other_thread)
+    else:
+        if hasattr(thread, 'poll'):
+            thread.poll.delete()
+        if hasattr(other_thread, 'poll'):
             other_thread.poll.delete()
 
     # merge thread contents
@@ -76,21 +72,18 @@ def threads_merge_endpoint(request):
 
     serializer.is_valid(raise_exception=True)
 
-    new_thread = merge_threads(request, serializer.validated_data)
-    return Response(ThreadsListSerializer(new_thread).data)
-
-
-def merge_threads(request, validated_data):
-    threads = validated_data['threads']
-    poll = validated_data.get('poll')
+    data = serializer.validated_data
+    
+    threads = data['threads']
+    poll = data['poll']
 
     new_thread = Thread(
-        category=validated_data['category'],
+        category=data['category'],
         started_on=threads[0].started_on,
         last_post_on=threads[0].last_post_on,
     )
 
-    new_thread.set_title(validated_data['title'])
+    new_thread.set_title(data['title'])
     new_thread.save()
 
     if poll:
@@ -107,7 +100,7 @@ def merge_threads(request, validated_data):
             new_thread,
             'merged',
             {
-                'merged_thread': thread.title,
+                'merged_thread': thread.title
             },
             commit=False,
         )
@@ -115,13 +108,13 @@ def merge_threads(request, validated_data):
     new_thread.synchronize()
     new_thread.save()
 
-    if validated_data.get('weight') == Thread.WEIGHT_GLOBAL:
+    if data.get('weight') == Thread.WEIGHT_GLOBAL:
         moderation.pin_thread_globally(request, new_thread)
-    elif validated_data.get('weight'):
+    elif data.get('weight'):
         moderation.pin_thread_locally(request, new_thread)
-    if validated_data.get('is_hidden', False):
+    if data.get('is_hidden', False):
         moderation.hide_thread(request, new_thread)
-    if validated_data.get('is_closed', False):
+    if data.get('is_closed', False):
         moderation.close_thread(request, new_thread)
 
     if new_thread.category not in categories:
@@ -136,4 +129,5 @@ def merge_threads(request, validated_data):
     new_thread.subscription = None
 
     add_acl(request.user, new_thread)
-    return new_thread
+
+    return Response(ThreadsListSerializer(new_thread).data)

misago/threads/serializers/moderation.py

@@ -441,25 +441,8 @@ class MergeThreadSerializer(serializers.Serializer):
         return other_thread
 
     def validate(self, data):
-        thread = self.context['thread']
-        other_thread = data['other_thread']
-
-        polls_handler = PollMergeHandler([thread, other_thread])
-
-        if len(polls_handler.polls) == 1:
-            data['poll'] = polls_handler.polls[0]
-        elif polls_handler.is_merge_conflict():
-            if 'poll' in data:
-                polls_handler.set_resolution(data['poll'])
-                if polls_handler.is_valid():
-                    data['poll'] = polls_handler.get_resolution()
-                else:
-                    raise ValidationError({'poll': _("Invalid choice.")})
-            else:
-                data['polls'] = polls_handler.get_available_resolutions()
-
-        self.polls_handler = polls_handler
-
+        threads = [self.context['thread'], data['other_thread']]
+        data['poll'] = validate_poll_merge(threads, data)
         return data
 
 
@@ -517,7 +500,7 @@ class MergeThreadsSerializer(NewThreadSerializer):
 
         return threads
     
-    def validate_threads_merge(self, threads):
+    def validate_threads_permissions(self, threads):
         user = self.context['user']
         invalid_threads = []
         for thread in threads:
@@ -525,33 +508,34 @@ class MergeThreadsSerializer(NewThreadSerializer):
                 allow_merge_thread(user, thread)
             except PermissionDenied as permission_error:
                 invalid_threads.append({
+                    'status': 403,
                     'id': thread.pk,
-                    'detail': [permission_error]
+                    'detail': permission_error
                 })
 
         if invalid_threads:
             raise ValidationError({'merge': invalid_threads})
 
-    def validate_poll_merge(self, data):
-        validated_threads = data['threads']
-        polls_handler = PollMergeHandler(validated_threads)
+    def validate(self, data):
+        self.validate_threads_permissions(data['threads'])
+        data['poll'] = validate_poll_merge(data['threads'], data)
+        return data
 
-        if len(polls_handler.polls) == 1:
-            return polls_handler.polls[0]
 
-        if polls_handler.is_merge_conflict():
-            if 'poll' in data:
-                polls_handler.set_resolution(data['poll'])
-                if polls_handler.is_valid():
-                    return polls_handler.get_resolution()
-                else:
-                    raise ValidationError({'poll': [_("Invalid choice.")]})
-            else:
-                raise ValidationError({'polls': polls_handler.get_available_resolutions()})
+def validate_poll_merge(threads, data):
+    merge_handler = PollMergeHandler(threads)
 
-        return None
+    if len(merge_handler.polls) == 1:
+        return merge_handler.polls[0]
 
-    def validate(self, data):
-        self.validate_threads_merge(data['threads'])
-        data['poll'] = self.validate_poll_merge(data)
-        return data
+    if merge_handler.is_merge_conflict():
+        if 'poll' in data:
+            merge_handler.set_resolution(data['poll'])
+            if merge_handler.is_valid():
+                return merge_handler.get_resolution()
+            else:
+                raise ValidationError({'poll': [_("Invalid choice.")]})
+        else:
+            raise ValidationError({'polls': merge_handler.get_available_resolutions()})
+
+    return None

misago/threads/tests/test_thread_merge_api.py

@@ -538,9 +538,9 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         self.assertEqual(
             response.json(), {
                 'polls': [
-                    [0, "Delete all polls"],
-                    [poll.pk, poll.question],
-                    [other_poll.pk, other_poll.question],
+                    ['0', "Delete all polls"],
+                    [str(poll.pk), poll.question],
+                    [str(other_poll.pk), other_poll.question],
                 ]
             }
         )

misago/threads/tests/test_threads_merge_api.py

@@ -1,5 +1,6 @@
 import json
 
+from django.utils import six
 from django.urls import reverse
 
 from misago.acl import add_acl
@@ -55,14 +56,16 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             }
         )
 
-    def test_merge_no_threads(self):
+    def test_empty_data(self):
         """api validates if we are trying to merge no threads"""
         response = self.client.post(self.api_link, content_type="application/json")
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json['threads'], ["You have to select at least two threads to merge."]
+            response.json(), {
+                'title': ['This field is required.'],
+                'category': ['This field is required.'],
+                'threads': ["You have to select at least two threads to merge."],
+            }
         )
 
     def test_merge_empty_threads(self):
@@ -70,15 +73,17 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
         response = self.client.post(
             self.api_link,
             json.dumps({
+                'category': self.category.pk,
+                'title': 'Lorem ipsum dolor',
                 'threads': [],
             }),
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json['threads'], ["You have to select at least two threads to merge."]
+            response.json(), {
+                'threads': ["You have to select at least two threads to merge."],
+            }
         )
 
     def test_merge_invalid_threads(self):
@@ -86,24 +91,35 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
         response = self.client.post(
             self.api_link,
             json.dumps({
+                'category': self.category.pk,
+                'title': 'Lorem ipsum dolor',
                 'threads': 'abcd',
             }),
             content_type="application/json",
         )
-        self.assertContains(response, "Expected a list of items", status_code=400)
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(
+            response.json(), {
+                'threads': [
+                    'Expected a list of items but got type "{}".'.format(six.text_type.__name__)
+                ],
+            }
+        )
 
         response = self.client.post(
             self.api_link,
             json.dumps({
+                'category': self.category.pk,
+                'title': 'Lorem ipsum dolor',
                 'threads': ['a', '-', 'c'],
             }),
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json['threads'], ["One or more thread ids received were invalid."]
+            response.json(), {
+                'threads': ["One or more thread ids received were invalid."],
+            }
         )
 
     def test_merge_single_thread(self):
@@ -111,15 +127,17 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
         response = self.client.post(
             self.api_link,
             json.dumps({
+                'category': self.category.pk,
+                'title': 'Lorem ipsum dolor',
                 'threads': [self.thread.id],
             }),
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json['threads'], ["You have to select at least two threads to merge."]
+            response.json(), {
+                'threads': ["You have to select at least two threads to merge."],
+            }
         )
 
     def test_merge_with_nonexisting_thread(self):
@@ -127,15 +145,17 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
         response = self.client.post(
             self.api_link,
             json.dumps({
+                'category': self.category.pk,
+                'title': 'Lorem ipsum dolor',
                 'threads': [self.thread.id, self.thread.id + 1000],
             }),
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json['threads'], ["One or more threads to merge could not be found."]
+            response.json(), {
+                'threads': ["One or more threads to merge could not be found."],
+            }
         )
 
     def test_merge_with_invisible_thread(self):
@@ -145,15 +165,17 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
         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)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json['threads'], ["One or more threads to merge could not be found."]
+            response.json(), {
+                'threads': ["One or more threads to merge could not be found."],
+            }
         )
 
     def test_merge_no_permission(self):
@@ -174,12 +196,14 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             response.json(), {
                 'merge': [
                     {
+                        'status': '403',
                         'id': str(thread.pk),
-                        'detail': ["You can't merge threads in this category."],
+                        'detail': "You can't merge threads in this category.",
                     },
                     {
+                        'status': '403',
                         'id': str(self.thread.pk),
-                        'detail': ["You can't merge threads in this category."],
+                        'detail': "You can't merge threads in this category.",
                     },
                 ],
             }
@@ -212,12 +236,14 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             response.json(), {
                 'merge': [
                     {
+                        'status': '403',
                         'id': str(other_thread.pk),
-                        'detail': ["This category is closed. You can't merge it's threads."],
+                        'detail': "This category is closed. You can't merge it's threads.",
                     },
                     {
+                        'status': '403',
                         'id': str(self.thread.pk),
-                        'detail': ["This category is closed. You can't merge it's threads."],
+                        'detail': "This category is closed. You can't merge it's threads.",
                     },
                 ],
             }
@@ -229,7 +255,6 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             'can_merge_threads': 1,
             'can_close_threads': 0,
         })
-        self.override_other_category()
 
         other_thread = testutils.post_thread(self.category)
 
@@ -239,7 +264,7 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
         response = self.client.post(
             self.api_link,
             json.dumps({
-                'category': self.category_b.pk,
+                'category': self.category.pk,
                 'title': 'Lorem ipsum dolor',
                 'threads': [self.thread.id, other_thread.id],
             }),
@@ -250,10 +275,9 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             response.json(), {
                 'merge': [
                     {
+                        'status': '403',
                         'id': str(other_thread.pk),
-                        'detail': [
-                            "This thread is closed. You can't merge it with other threads."
-                        ],
+                        'detail': "This thread is closed. You can't merge it with other threads.",
                     },
                 ],
             }
@@ -310,10 +334,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'title': ['This field is required.'],
                 'category': ['This field is required.'],
             }
@@ -340,10 +362,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'title': ["Thread title should be at least 5 characters long (it has 3)."],
             }
         )
@@ -369,10 +389,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'category': ["Requested category could not be found."],
             }
         )
@@ -399,10 +417,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'category': ["You can't create new threads in selected category."],
             }
         )
@@ -429,10 +445,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'weight': ["Ensure this value is less than or equal to 2."],
             }
         )
@@ -459,10 +473,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'weight': ["You don't have permission to pin threads globally in this category."],
             }
         )
@@ -489,10 +501,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'weight': ["You don't have permission to pin threads in this category."],
             }
         )
@@ -520,10 +530,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'title': ["Thread title should be at least 5 characters long (it has 3)."],
             }
         )
@@ -551,10 +559,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'title': ["Thread title should be at least 5 characters long (it has 3)."],
             }
         )
@@ -581,10 +587,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'is_closed': ["You don't have permission to close threads in this category."],
             }
         )
@@ -612,10 +616,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'title': ["Thread title should be at least 5 characters long (it has 3)."],
             }
         )
@@ -643,10 +645,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'is_hidden': ["You don't have permission to hide threads in this category."],
             }
         )
@@ -675,10 +675,8 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
             content_type="application/json",
         )
         self.assertEqual(response.status_code, 400)
-
-        response_json = response.json()
         self.assertEqual(
-            response_json, {
+            response.json(), {
                 'title': ["Thread title should be at least 5 characters long (it has 3)."],
             }
         )
@@ -708,16 +706,14 @@ class ThreadsMergeApiTests(ThreadsApiTestCase):
         self.assertEqual(response.status_code, 200)
 
         # is response json with new thread?
-        response_json = response.json()
-
-        new_thread = Thread.objects.get(pk=response_json['id'])
+        new_thread = Thread.objects.get(pk=response.json()['id'])
         new_thread.is_read = False
         new_thread.subscription = None
 
         add_acl(self.user, new_thread.category)
         add_acl(self.user, new_thread)
 
-        self.assertEqual(response_json, ThreadsListSerializer(new_thread).data)
+        self.assertEqual(response.json(), ThreadsListSerializer(new_thread).data)
 
         # did posts move to new thread?
         for post in Post.objects.filter(id__in=posts_ids):

misago/users/tests/test_user_username_api.py

@@ -33,9 +33,12 @@ class UserUsernameTests(AuthenticatedUserTestCase):
 
         response = self.client.get(self.link)
         self.assertEqual(response.status_code, 200)
-
-        self.assertEqual(response.json()['changes_left'], 0)
-        self.assertIsNotNone(response.json()['next_change_on'])
+        self.assertEqual(response.json(), {
+            'changes_left': 0,
+            'next_change_on': response.json()['next_change_on'],
+            'length_min': settings.username_length_min,
+            'length_max': settings.username_length_max,
+        })
 
     def test_change_username_no_changes_left(self):
         """api returns error 400 if there are no username changes left"""