From b0ccb1ea7e4059060fb4465eb7a67828d7bd8a7f Mon Sep 17 00:00:00 2001 From: Finn Stutzenstein Date: Wed, 10 Feb 2021 15:58:01 +0100 Subject: [PATCH] Fix point of order sorting --- server/openslides/agenda/models.py | 82 +++++++++----- .../tests/integration/agenda/test_viewset.py | 100 +++++++++++++++++- 2 files changed, 153 insertions(+), 29 deletions(-) diff --git a/server/openslides/agenda/models.py b/server/openslides/agenda/models.py index b8c8b78ed..09cacadf2 100644 --- a/server/openslides/agenda/models.py +++ b/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: - # 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 + weight = self._get_new_speaker_weight(list_of_speakers, point_of_order) 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): """ diff --git a/server/tests/integration/agenda/test_viewset.py b/server/tests/integration/agenda/test_viewset.py index 8e30f3cc2..8372d4fec 100644 --- a/server/tests/integration/agenda/test_viewset.py +++ b/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)