Merge pull request #5882 from FinnStutzenstein/fixPointOfOrderSorting

Fix point of order sorting

server/openslides/agenda/models.py

@@ -447,34 +447,7 @@ class SpeakerManager(models.Manager):
         if config["agenda_present_speakers_only"] and not user.is_present:
             raise OpenSlidesError("Only present users can be on the lists of speakers.")
 
-        if point_of_order:
+        weight = self._get_new_speaker_weight(list_of_speakers, point_of_order)
-            # the new point of order speaker (poos) must be inserted between
-            # the last poos and the first regular waiting speaker
-            weight = self.filter(
-                list_of_speakers=list_of_speakers, point_of_order=True
-            ).aggregate(models.Max("weight"))["weight__max"]
-            if weight is None:
-                # noo poos, take the min - 1
-                weight = (
-                    self.filter(
-                        list_of_speakers=list_of_speakers, point_of_order=True
-                    ).aggregate(models.Min("weight"))["weight__min"]
-                    or 1
-                ) - 1
-            else:
-                weight += 1
-                # we have to add +1 to every weight of non-poo speakers.
-                self.filter(
-                    list_of_speakers=list_of_speakers, point_of_order=False
-                ).update(weight=F("weight") + 1)
-
-        else:
-            weight = (
-                self.filter(list_of_speakers=list_of_speakers).aggregate(
-                    models.Max("weight")
-                )["weight__max"]
-                or 0
-            ) + 1
 
         speaker = self.model(
             list_of_speakers=list_of_speakers,
@@ -489,6 +462,59 @@ class SpeakerManager(models.Manager):
         )
         return speaker
 
+    def _get_new_speaker_weight(self, list_of_speakers, point_of_order):
+        """
+        Calculates the weight of a newly to create speaker
+
+        - if there are no speakers, it will be 1
+        - non-poo speakers get max_weight + 1
+        - poo speakers:
+          * if the first waiting speaker is no poo speaker, insert it before it (min_weight-1)
+          * else: insert it before the first non-poo speaker. All other speakers must get a weight+1 update
+
+        Note that this method has side-effects: It might change the weight of other speakers to make
+        space for the new point of order speaker.
+        """
+        if point_of_order:
+            # get the first waiting speaker
+            first_speaker = self.filter(begin_time=None).order_by("weight").first()
+
+            if first_speaker is None:
+                return 1
+            if not first_speaker.point_of_order:
+                return first_speaker.weight - 1
+
+            # get the first non-poo speaker
+            first_non_poo_speaker = (
+                self.filter(begin_time=None, point_of_order=False)
+                .order_by("weight")
+                .first()
+            )
+
+            if first_non_poo_speaker is None:
+                # max weight + 1, the speaker is last since there are no non-poo speakers
+                return (
+                    self.filter(
+                        list_of_speakers=list_of_speakers, begin_time=None
+                    ).aggregate(models.Max("weight"))["weight__max"]
+                    + 1
+                )
+
+            weight = first_non_poo_speaker.weight
+            # add +1 to all speakers with weight >= first_non_poo_speaker.weight
+            self.filter(
+                list_of_speakers=list_of_speakers, begin_time=None, weight__gte=weight
+            ).update(weight=F("weight") + 1)
+
+            return weight
+        else:
+            return (
+                self.filter(list_of_speakers=list_of_speakers).aggregate(
+                    models.Max("weight")
+                )["weight__max"]
+                or 0
+            ) + 1
+
 
 class Speaker(RESTModelMixin, models.Model):
     """

server/tests/integration/agenda/test_viewset.py

@@ -373,7 +373,7 @@ class ManageSpeaker(TestCase):
         self.assertEqual(response.status_code, 200)
         self.assertEqual(Speaker.objects.all().count(), 1)
         self.assertTrue(Speaker.objects.get().point_of_order)
-        self.assertEqual(Speaker.objects.get().weight, 0)
+        self.assertEqual(Speaker.objects.get().weight, 1)
 
     def test_point_of_order_not_enabled(self):
         self.assertEqual(Speaker.objects.all().count(), 0)
@@ -409,6 +409,18 @@ class ManageSpeaker(TestCase):
         self.assertEqual(Speaker.objects.filter(user=self.admin).count(), 2)
 
     def test_point_of_order_two_poo_speakers(self):
+        """
+        before (poo):
+            - user (y)
+            - user2 (n)
+            - user3 (n)
+
+        after adding admin as poo speaker:
+            - user (y)
+            - admin (y)
+            - user2 (n)
+            - user3 (n)
+        """
         config["agenda_enable_point_of_order_speakers"] = True
         # user 2 and user3 are non-poo speakers
         self.user2, _ = self.create_user()
@@ -430,6 +442,92 @@ class ManageSpeaker(TestCase):
         self.assertEqual(Speaker.objects.get(user=self.user2).weight, 2)
         self.assertEqual(Speaker.objects.get(user=self.user3).weight, 3)
 
+    def test_point_of_order_two_poo_speakers_different_ordering(self):
+        """
+        before (poo):
+            - user (n)
+            - user2 (y)
+            - user3 (n)
+
+        after adding admin as poo speaker (no weight modifications of other speakers!)
+            - admin (y)
+            - user (n)
+            - user2 (y)
+            - user3 (n)
+        """
+        config["agenda_enable_point_of_order_speakers"] = True
+        # user 2 and user3 are non-poo speakers
+        self.user2, _ = self.create_user()
+        self.user3, _ = self.create_user()
+        s_user = Speaker.objects.add(self.user, self.list_of_speakers)
+        s_user2 = Speaker.objects.add(
+            self.user2, self.list_of_speakers, point_of_order=True
+        )
+        s_user3 = Speaker.objects.add(self.user3, self.list_of_speakers)
+
+        # set different ordering
+        s_user.weight = 0
+        s_user.save()
+        s_user2.weight = 1
+        s_user2.save()
+        s_user3.weight = 2
+        s_user3.save()
+
+        response = self.client.post(
+            reverse("listofspeakers-manage-speaker", args=[self.list_of_speakers.pk]),
+            {"point_of_order": True},
+        )
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(Speaker.objects.all().count(), 4)
+        self.assertEqual(Speaker.objects.get(user=self.admin).weight, -1)
+        self.assertEqual(Speaker.objects.get(user=self.user).weight, 0)
+        self.assertEqual(Speaker.objects.get(user=self.user2).weight, 1)
+        self.assertEqual(Speaker.objects.get(user=self.user3).weight, 2)
+
+    def test_point_of_order_three_poo_speakers(self):
+        """
+        before (poo):
+            - user (y)
+            - user2 (n)
+            - user3 (y)
+
+        after adding admin as poo speaker (there are weight modifications of other speakers!)
+            - user (y)
+            - admin (y)
+            - user2 (n)
+            - user3 (y)
+        """
+        config["agenda_enable_point_of_order_speakers"] = True
+        # user 2 and user3 are non-poo speakers
+        self.user2, _ = self.create_user()
+        self.user3, _ = self.create_user()
+        s_user = Speaker.objects.add(
+            self.user, self.list_of_speakers, point_of_order=True
+        )
+        s_user2 = Speaker.objects.add(self.user2, self.list_of_speakers)
+        s_user3 = Speaker.objects.add(
+            self.user3, self.list_of_speakers, point_of_order=True
+        )
+
+        # set different ordering
+        s_user.weight = 0
+        s_user.save()
+        s_user2.weight = 1
+        s_user2.save()
+        s_user3.weight = 2
+        s_user3.save()
+
+        response = self.client.post(
+            reverse("listofspeakers-manage-speaker", args=[self.list_of_speakers.pk]),
+            {"point_of_order": True},
+        )
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(Speaker.objects.all().count(), 4)
+        self.assertEqual(Speaker.objects.get(user=self.user).weight, 0)
+        self.assertEqual(Speaker.objects.get(user=self.admin).weight, 1)
+        self.assertEqual(Speaker.objects.get(user=self.user2).weight, 2)
+        self.assertEqual(Speaker.objects.get(user=self.user3).weight, 3)
+
     def test_point_of_order_twice(self):
         config["agenda_enable_point_of_order_speakers"] = True
         Speaker.objects.add(self.admin, self.list_of_speakers, point_of_order=True)