Improve merge conflict handling, test merge thread into other best answers

misago/threads/api/threadendpoints/merge.py

@@ -33,14 +33,14 @@ def thread_merge_endpoint(request, thread, viewmodel):
             errors = serializer.errors['poll']
         elif 'polls' in serializer.errors:
             return Response({'polls': serializer.errors['polls']}, status=400)
+        elif 'best_answer' in serializer.errors:
+            errors = serializer.errors['best_answer']
+        elif 'best_answers' in serializer.errors:
+            return Response({'best_answers': serializer.errors['best_answers']}, status=400)
         else:
             errors = list(serializer.errors.values())[0]
         return Response({'detail': errors[0]}, status=400)
 
-    # interrupt merge with request for poll resolution?
-    if serializer.validated_data.get('polls'):
-        return Response({'polls': serializer.validated_data['polls']}, status=400)
-
     # merge conflict
     other_thread = serializer.validated_data['other_thread']
 
@@ -54,6 +54,17 @@ def thread_merge_endpoint(request, thread, viewmodel):
     elif poll:
         poll.move(other_thread)
 
+    best_answer = serializer.validated_data.get('best_answer')
+    if 'best_answer' in serializer.merge_conflict and not best_answer:
+        other_thread.clear_best_answer()
+    if best_answer and best_answer != other_thread:
+        other_thread.best_answer_id = thread.best_answer_id
+        other_thread.best_answer_is_protected = thread.best_answer_is_protected
+        other_thread.best_answer_marked_on = thread.best_answer_marked_on
+        other_thread.best_answer_marked_by_id = thread.best_answer_marked_by_id
+        other_thread.best_answer_marked_by_name = thread.best_answer_marked_by_name
+        other_thread.best_answer_marked_by_slug = thread.best_answer_marked_by_slug
+
     # merge thread contents
     moderation.merge_thread(request, other_thread, thread)
 

misago/threads/mergeconflict.py

@@ -51,7 +51,7 @@ class BestAnswerMergeHandler(MergeConflictHandler):
             if thread.best_answer_id:
                 self.items.append(thread)
                 self.choices[thread.pk] = thread
-        self.items.sort(key=lambda choice: choice.id, reverse=True)
+        self.items.sort(key=lambda choice: (thread.title, thread.id))
 
     def get_available_resolutions(self):
         resolutions = [[0, _("Unmark all best answers")]]
@@ -70,12 +70,12 @@ class PollMergeHandler(MergeConflictHandler):
                 self.choices[thread.poll.id] = thread.poll
             except Poll.DoesNotExist:
                 pass
-        self.items.sort(key=lambda choice: choice.thread_id, reverse=True)
+        self.items.sort(key=lambda poll: poll.question)
 
     def get_available_resolutions(self):
         resolutions = [[0, _("Delete all polls")]]
         for poll in self.items:
-            resolutions.append([poll.id, poll.question])
+            resolutions.append([poll.id, u'{} ({})'.format(poll.question, poll.thread.title)])
         return resolutions
 
 

misago/threads/serializers/moderation.py

@@ -418,6 +418,12 @@ class MergeThreadSerializer(serializers.Serializer):
             'required': ugettext_lazy("Enter link to new thread."),
         },
     )
+    best_answer = serializers.IntegerField(
+        required=False,
+        error_messages={
+            'invalid': ugettext_lazy("Invalid choice."),
+        },
+    )
     poll = serializers.IntegerField(
         required=False,
         error_messages={

misago/threads/tests/test_mergeconflict.py

@@ -117,7 +117,7 @@ class MergeConflictTests(TestCase):
                     [
                         str(thread.id),
                         thread.title,
-                    ] for thread in reversed(best_answers)
+                    ] for thread in best_answers
                 ]
             })
 
@@ -174,8 +174,8 @@ class MergeConflictTests(TestCase):
                 'polls': [['0', 'Delete all polls']] + [
                     [
                         str(thread.poll.id),
-                        thread.poll.question,
-                    ] for thread in reversed(polls)
+                        u'{} ({})'.format(thread.poll.question, thread.title),
+                    ] for thread in polls
                 ]
             })
 
@@ -233,13 +233,13 @@ class MergeConflictTests(TestCase):
                     [
                         str(thread.id),
                         thread.title,
-                    ] for thread in reversed(best_answers)
+                    ] for thread in best_answers
                 ],
                 'polls': [['0', 'Delete all polls']] + [
                     [
                         str(thread.poll.id),
-                        thread.poll.question,
-                    ] for thread in reversed(polls)
+                        u'{} ({})'.format(thread.poll.question, thread.title),
+                    ] for thread in polls
                 ]
             })
 
@@ -319,4 +319,4 @@ class MergeConflictTests(TestCase):
         self.assertEqual(merge_conflict.get_resolution(), {
             'best_answer': None,
             'poll': polls[0].poll,
-        })
+        })

misago/threads/tests/test_thread_merge_api.py

@@ -103,7 +103,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_other_thread_exists(self):
         """api validates if other thread exists"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl()
 
         other_thread = testutils.post_thread(self.category_b)
@@ -120,7 +119,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_other_thread_is_invisible(self):
         """api validates if other thread is visible"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_see': 0})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -137,7 +135,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_other_thread_isnt_mergeable(self):
         """api validates if other thread can be merged"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 0})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -275,7 +272,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_merge_threads(self):
         """api merges two threads successfully"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -287,7 +283,7 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         )
         self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
 
-        # other thread has two posts now
+        # other thread has two posts and an event now
         self.assertEqual(other_thread.post_set.count(), 3)
 
         # first thread is gone
@@ -297,7 +293,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_merge_threads_kept_reads(self):
         """api keeps both threads readtrackers after merge"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -325,7 +320,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_merge_threads_kept_subs(self):
         """api keeps other thread's subscription after merge"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -356,7 +350,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_merge_threads_moved_subs(self):
         """api keeps other thread's subscription after merge"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -387,7 +380,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_merge_threads_handle_subs_colision(self):
         """api resolves conflicting thread subscriptions after merge"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         self.user.subscription_set.create(
@@ -424,10 +416,228 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         self.user.subscription_set.get(thread=other_thread)
         self.user.subscription_set.get(category=self.category_b)
 
+    def test_merge_threads_kept_best_answer(self):
+        """api merges two threads successfully, keeping best answer from old thread"""
+        self.override_acl({'can_merge_threads': 1})
+        self.override_other_acl({'can_merge_threads': 1})
+
+        other_thread = testutils.post_thread(self.category_b)
+        best_answer = testutils.reply_thread(other_thread)
+        other_thread.set_best_answer(self.user, best_answer)
+        other_thread.save()
+
+        response = self.client.post(
+            self.api_link, {
+                'other_thread': other_thread.get_absolute_url(),
+            }
+        )
+        self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
+
+        # other thread has three posts and an event now
+        self.assertEqual(other_thread.post_set.count(), 4)
+
+        # first thread is gone
+        with self.assertRaises(Thread.DoesNotExist):
+            Thread.objects.get(pk=self.thread.pk)
+
+        # best answer is kept sin other thread
+        other_thread = Thread.objects.get(pk=other_thread.pk)
+        self.assertEqual(other_thread.best_answer, best_answer)
+
+    def test_merge_threads_moved_best_answer(self):
+        """api merges two threads successfully, moving best answer to old thread"""
+        self.override_acl({'can_merge_threads': 1})
+        self.override_other_acl({'can_merge_threads': 1})
+
+        other_thread = testutils.post_thread(self.category_b)
+
+        best_answer = testutils.reply_thread(self.thread)
+        self.thread.set_best_answer(self.user, best_answer)
+        self.thread.save()
+
+        response = self.client.post(
+            self.api_link, {
+                'other_thread': other_thread.get_absolute_url(),
+            }
+        )
+        self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
+
+        # other thread has three posts and an event now
+        self.assertEqual(other_thread.post_set.count(), 4)
+
+        # first thread is gone
+        with self.assertRaises(Thread.DoesNotExist):
+            Thread.objects.get(pk=self.thread.pk)
+
+        # best answer is kept sin other thread
+        other_thread = Thread.objects.get(pk=other_thread.pk)
+        self.assertEqual(other_thread.best_answer, best_answer)
+
+    def test_merge_threads_merge_conflict_best_answer(self):
+        """api errors on merge conflict, returning list of available best answers"""
+        self.override_acl({'can_merge_threads': 1})
+        self.override_other_acl({'can_merge_threads': 1})
+
+        best_answer = testutils.reply_thread(self.thread)
+        self.thread.set_best_answer(self.user, best_answer)
+        self.thread.save()
+        
+        other_thread = testutils.post_thread(self.category_b)
+        other_best_answer = testutils.reply_thread(other_thread)
+        other_thread.set_best_answer(self.user, other_best_answer)
+        other_thread.save()
+
+        response = self.client.post(
+            self.api_link, {
+                'other_thread': other_thread.get_absolute_url(),
+            }
+        )
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.json(), {
+            'best_answers': [
+                ['0', "Unmark all best answers"],
+                [str(self.thread.id), self.thread.title],
+                [str(other_thread.id), other_thread.title],
+            ]
+        })
+
+        # best answers were untouched
+        self.assertEqual(self.thread.post_set.count(), 2)
+        self.assertEqual(other_thread.post_set.count(), 2)
+        self.assertEqual(Thread.objects.get(pk=self.thread.pk).best_answer_id, best_answer.id)
+        self.assertEqual(
+            Thread.objects.get(pk=other_thread.pk).best_answer_id, other_best_answer.id)
+
+    def test_threads_merge_conflict_best_answer_invalid_resolution(self):
+        """api errors on invalid merge conflict resolution"""
+        self.override_acl({'can_merge_threads': 1})
+        self.override_other_acl({'can_merge_threads': 1})
+
+        best_answer = testutils.reply_thread(self.thread)
+        self.thread.set_best_answer(self.user, best_answer)
+        self.thread.save()
+        
+        other_thread = testutils.post_thread(self.category_b)
+        other_best_answer = testutils.reply_thread(other_thread)
+        other_thread.set_best_answer(self.user, other_best_answer)
+        other_thread.save()
+
+        response = self.client.post(
+            self.api_link, {
+                'other_thread': other_thread.get_absolute_url(),
+                'best_answer': other_thread.id + 10,
+            }
+        )
+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.json(), {'detail': "Invalid choice."})
+
+        # best answers were untouched
+        self.assertEqual(self.thread.post_set.count(), 2)
+        self.assertEqual(other_thread.post_set.count(), 2)
+        self.assertEqual(Thread.objects.get(pk=self.thread.pk).best_answer_id, best_answer.id)
+        self.assertEqual(
+            Thread.objects.get(pk=other_thread.pk).best_answer_id, other_best_answer.id)
+
+    def test_threads_merge_conflict_delete_all_best_answers(self):
+        """api unmarks all best answers when delete all choice is selected"""
+        self.override_acl({'can_merge_threads': 1})
+        self.override_other_acl({'can_merge_threads': 1})
+
+        best_answer = testutils.reply_thread(self.thread)
+        self.thread.set_best_answer(self.user, best_answer)
+        self.thread.save()
+        
+        other_thread = testutils.post_thread(self.category_b)
+        other_best_answer = testutils.reply_thread(other_thread)
+        other_thread.set_best_answer(self.user, other_best_answer)
+        other_thread.save()
+
+        response = self.client.post(
+            self.api_link, {
+                'other_thread': other_thread.get_absolute_url(),
+                'best_answer': 0,
+            }
+        )
+        self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
+
+        # other thread has four posts and an event now
+        self.assertEqual(other_thread.post_set.count(), 5)
+
+        # first thread is gone
+        with self.assertRaises(Thread.DoesNotExist):
+            Thread.objects.get(pk=self.thread.pk)
+
+        # final thread has no marked best answer
+        self.assertIsNone(Thread.objects.get(pk=other_thread.pk).best_answer_id)
+
+    def test_threads_merge_conflict_keep_first_best_answer(self):
+        """api unmarks other best answer on merge"""
+        self.override_acl({'can_merge_threads': 1})
+        self.override_other_acl({'can_merge_threads': 1})
+
+        best_answer = testutils.reply_thread(self.thread)
+        self.thread.set_best_answer(self.user, best_answer)
+        self.thread.save()
+        
+        other_thread = testutils.post_thread(self.category_b)
+        other_best_answer = testutils.reply_thread(other_thread)
+        other_thread.set_best_answer(self.user, other_best_answer)
+        other_thread.save()
+
+        response = self.client.post(
+            self.api_link, {
+                'other_thread': other_thread.get_absolute_url(),
+                'best_answer': self.thread.pk,
+            }
+        )
+        self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
+
+        # other thread has four posts and an event now
+        self.assertEqual(other_thread.post_set.count(), 5)
+
+        # first thread is gone
+        with self.assertRaises(Thread.DoesNotExist):
+            Thread.objects.get(pk=self.thread.pk)
+
+        # other thread's best answer was unchanged
+        self.assertEqual(Thread.objects.get(pk=other_thread.pk).best_answer_id, best_answer.id)
+
+    def test_threads_merge_conflict_keep_other_best_answer(self):
+        """api unmarks first best answer on merge"""
+        self.override_acl({'can_merge_threads': 1})
+        self.override_other_acl({'can_merge_threads': 1})
+
+        best_answer = testutils.reply_thread(self.thread)
+        self.thread.set_best_answer(self.user, best_answer)
+        self.thread.save()
+        
+        other_thread = testutils.post_thread(self.category_b)
+        other_best_answer = testutils.reply_thread(other_thread)
+        other_thread.set_best_answer(self.user, other_best_answer)
+        other_thread.save()
+
+        response = self.client.post(
+            self.api_link, {
+                'other_thread': other_thread.get_absolute_url(),
+                'best_answer': other_thread.pk,
+            }
+        )
+        self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
+
+        # other thread has four posts and an event now
+        self.assertEqual(other_thread.post_set.count(), 5)
+
+        # first thread is gone
+        with self.assertRaises(Thread.DoesNotExist):
+            Thread.objects.get(pk=self.thread.pk)
+
+        # other thread's best answer was changed to merged in thread's answer
+        self.assertEqual(
+            Thread.objects.get(pk=other_thread.pk).best_answer_id, other_best_answer.id)
+
     def test_merge_threads_kept_poll(self):
         """api merges two threads successfully, keeping poll from old thread"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -440,7 +650,7 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         )
         self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
 
-        # other thread has two posts now
+        # other thread has two posts and an event now
         self.assertEqual(other_thread.post_set.count(), 3)
 
         # first thread is gone
@@ -454,7 +664,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_merge_threads_moved_poll(self):
         """api merges two threads successfully, moving poll from other thread"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -467,7 +676,7 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         )
         self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
 
-        # other thread has two posts now
+        # other thread has two posts and an event now
         self.assertEqual(other_thread.post_set.count(), 3)
 
         # first thread is gone
@@ -478,10 +687,9 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         self.assertEqual(Poll.objects.filter(pk=poll.pk, thread=other_thread).count(), 1)
         self.assertEqual(PollVote.objects.filter(poll=poll, thread=other_thread).count(), 4)
 
-    def test_threads_merge_conflict(self):
+    def test_threads_merge_conflict_polls(self):
         """api errors on merge conflict, returning list of available polls"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -498,8 +706,14 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
             response.json(), {
                 'polls': [
                     ['0', "Delete all polls"],
-                    [str(poll.pk), poll.question],
-                    [str(other_poll.pk), other_poll.question],
+                    [
+                        str(poll.pk),
+                        u'{} ({})'.format(poll.question, poll.thread.title),
+                    ],
+                    [
+                        str(other_poll.pk),
+                        u'{} ({})'.format(other_poll.question, other_poll.thread.title),
+                    ],
                 ]
             }
         )
@@ -508,10 +722,9 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         self.assertEqual(Poll.objects.count(), 2)
         self.assertEqual(PollVote.objects.count(), 8)
 
-    def test_threads_merge_conflict_invalid_resolution(self):
+    def test_threads_merge_conflict_poll_invalid_resolution(self):
         """api errors on invalid merge conflict resolution"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -522,7 +735,7 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         response = self.client.post(
             self.api_link, {
                 'other_thread': other_thread.get_absolute_url(),
-                'poll': 'jhdkajshdsak',
+                'poll': Poll.objects.all()[0].pk + 10,
             }
         )
         self.assertEqual(response.status_code, 400)
@@ -532,10 +745,9 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         self.assertEqual(Poll.objects.count(), 2)
         self.assertEqual(PollVote.objects.count(), 8)
 
-    def test_threads_merge_conflict_delete_all(self):
+    def test_threads_merge_conflict_delete_all_polls(self):
         """api deletes all polls when delete all choice is selected"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -550,7 +762,7 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         )
         self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
 
-        # other thread has two posts now
+        # other thread has two posts and an event now
         self.assertEqual(other_thread.post_set.count(), 3)
 
         # first thread is gone
@@ -564,7 +776,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_threads_merge_conflict_keep_first_poll(self):
         """api deletes other poll on merge"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -579,7 +790,7 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         )
         self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
 
-        # other thread has two posts now
+        # other thread has two posts and an event now
         self.assertEqual(other_thread.post_set.count(), 3)
 
         # first thread is gone
@@ -600,7 +811,6 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
     def test_threads_merge_conflict_keep_other_poll(self):
         """api deletes first poll on merge"""
         self.override_acl({'can_merge_threads': 1})
-
         self.override_other_acl({'can_merge_threads': 1})
 
         other_thread = testutils.post_thread(self.category_b)
@@ -615,7 +825,7 @@ class ThreadMergeApiTests(ThreadsApiTestCase):
         )
         self.assertContains(response, other_thread.get_absolute_url(), status_code=200)
 
-        # other thread has two posts now
+        # other thread has two posts and an event now
         self.assertEqual(other_thread.post_set.count(), 3)
 
         # first thread is gone