Merge pull request #3304 from CatoTH/Issue3298-PreventCollidingChangeRecommendations

Prevent colliding change recommendations - fixes #3298

CHANGELOG

@@ -28,6 +28,8 @@ Motions:
   client [#3212].
 - Reworked DOCX export parser and added comments to DOCX [#3258].
 - New PDF export for personal note and comments [#3239].
+- Bugfix: Creating colliding change recommendation is now prevented
+  on server side [#3304]
 
 Users:
 - User without permission to see users can now see agenda item speakers,

openslides/motions/models.py

@@ -1,6 +1,6 @@
 from django.conf import settings
 from django.contrib.contenttypes.fields import GenericRelation
-from django.core.exceptions import ImproperlyConfigured
+from django.core.exceptions import ImproperlyConfigured, ValidationError
 from django.db import IntegrityError, models, transaction
 from django.db.models import Max
 from django.utils import formats, timezone
@@ -771,6 +771,25 @@ class MotionChangeRecommendation(RESTModelMixin, models.Model):
     creation_time = models.DateTimeField(auto_now=True)
     """Time when the change recommendation was saved."""
 
+    def collides_with_other_recommendation(self, recommendations):
+        for recommendation in recommendations:
+            if (not (self.line_from < recommendation.line_from and self.line_to <= recommendation.line_from) and
+                    not (self.line_from >= recommendation.line_to and self.line_to > recommendation.line_to)):
+                return True
+
+        return False
+
+    def save(self, *args, **kwargs):
+        recommendations = (MotionChangeRecommendation.objects
+                           .filter(motion_version=self.motion_version)
+                           .exclude(pk=self.pk))
+
+        if self.collides_with_other_recommendation(recommendations):
+            raise ValidationError('The recommendation collides with an existing one (line %s - %s).' %
+                                  (self.line_from, self.line_to))
+
+        return super().save(*args, **kwargs)
+
     class Meta:
         default_permissions = ()
 

openslides/motions/views.py

@@ -3,6 +3,7 @@ import re
 
 from django.conf import settings
 from django.contrib.staticfiles import finders
+from django.core.exceptions import ValidationError as DjangoValidationError
 from django.db import IntegrityError, transaction
 from django.http import Http404
 from django.utils.translation import ugettext as _
@@ -449,6 +450,15 @@ class MotionChangeRecommendationViewSet(ModelViewSet):
             result = False
         return result
 
+    def create(self, request, *args, **kwargs):
+        """
+        Creating a Change Recommendation, custom exception handling
+        """
+        try:
+            return super().create(request, *args, **kwargs)
+        except DjangoValidationError as err:
+            return Response({'detail': err.message}, status=400)
+
 
 class CategoryViewSet(ModelViewSet):
     """

tests/integration/motions/test_viewset.py

@@ -607,6 +607,83 @@ class ManageVersion(TestCase):
         self.assertEqual(response.data, {'detail': 'You can not delete the active version of a motion.'})
 
 
+class CreateMotionChangeRecommendation(TestCase):
+    """
+    Tests motion change recommendation creation.
+    """
+    def setUp(self):
+        self.client = APIClient()
+        self.client.login(username='admin', password='admin')
+
+        self.client.post(
+            reverse('motion-list'),
+            {'title': 'test_title_OoCoo3MeiT9li5Iengu9',
+             'text': 'test_text_thuoz0iecheiheereiCi'})
+
+    def test_simple(self):
+        """
+        Creating a change plain, simple change recommendation
+        """
+        response = self.client.post(
+            reverse('motionchangerecommendation-list'),
+            {'line_from': '5',
+             'line_to': '7',
+             'motion_version_id': '1',
+             'text': '<p>New test</p>',
+             'type': '0'})
+        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+
+    def test_collission(self):
+        """
+        Two change recommendations with overlapping lines should lead to a Bad Request
+        """
+        response = self.client.post(
+            reverse('motionchangerecommendation-list'),
+            {'line_from': '5',
+             'line_to': '7',
+             'motion_version_id': '1',
+             'text': '<p>New test</p>',
+             'type': '0'})
+        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+
+        response = self.client.post(
+            reverse('motionchangerecommendation-list'),
+            {'line_from': '3',
+             'line_to': '6',
+             'motion_version_id': '1',
+             'text': '<p>New test</p>',
+             'type': '0'})
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        self.assertEqual(response.data, {'detail': 'The recommendation collides with an existing one (line 3 - 6).'})
+
+    def test_no_collission_different_motions(self):
+        """
+        Two change recommendations with overlapping lines, but affecting different motions, should not interfere
+        """
+        self.client.post(
+            reverse('motion-list'),
+            {'title': 'test_title_OoCoo3MeiT9li5Iengu9',
+             'text': 'test_text_thuoz0iecheiheereiCi'})
+
+        response = self.client.post(
+            reverse('motionchangerecommendation-list'),
+            {'line_from': '5',
+             'line_to': '7',
+             'motion_version_id': '1',
+             'text': '<p>New test</p>',
+             'type': '0'})
+        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+
+        response = self.client.post(
+            reverse('motionchangerecommendation-list'),
+            {'line_from': '3',
+             'line_to': '6',
+             'motion_version_id': '2',
+             'text': '<p>New test</p>',
+             'type': '0'})
+        self.assertEqual(response.status_code, status.HTTP_201_CREATED)
+
+
 class SupportMotion(TestCase):
     """
     Tests supporting a motion.

tests/unit/motions/test_models.py

@@ -0,0 +1,41 @@
+from unittest import TestCase
+
+from openslides.motions.models import MotionChangeRecommendation, MotionVersion
+
+
+class MotionChangeRecommendationTest(TestCase):
+    def test_overlapping_line_numbers(self):
+        """
+        Tests that a change recommendation directly before another one can be created
+        """
+        version = MotionVersion()
+        existing_recommendation = MotionChangeRecommendation()
+        existing_recommendation.line_from = 5
+        existing_recommendation.line_to = 7
+        existing_recommendation.rejected = False
+        existing_recommendation.motion_version = version
+        other_recommendations = [existing_recommendation]
+
+        new_recommendation1 = MotionChangeRecommendation()
+        new_recommendation1.line_from = 3
+        new_recommendation1.line_to = 5
+        collides = new_recommendation1.collides_with_other_recommendation(other_recommendations)
+        self.assertFalse(collides)
+
+        new_recommendation2 = MotionChangeRecommendation()
+        new_recommendation2.line_from = 3
+        new_recommendation2.line_to = 6
+        collides = new_recommendation2.collides_with_other_recommendation(other_recommendations)
+        self.assertTrue(collides)
+
+        new_recommendation3 = MotionChangeRecommendation()
+        new_recommendation3.line_from = 6
+        new_recommendation3.line_to = 8
+        collides = new_recommendation3.collides_with_other_recommendation(other_recommendations)
+        self.assertTrue(collides)
+
+        new_recommendation4 = MotionChangeRecommendation()
+        new_recommendation4.line_from = 7
+        new_recommendation4.line_to = 9
+        collides = new_recommendation4.collides_with_other_recommendation(other_recommendations)
+        self.assertFalse(collides)