Use the new django transaction API

fixes #1027

openslides/__main__.py

@@ -279,7 +279,7 @@ def backupdb(settings, args):
 
     from django.db import connection, transaction
 
-    @transaction.commit_manually
+    @transaction.atomic
     def do_backup(src_path, dest_path):
         # perform a simple file-copy backup of the database
         # first we need a shared lock on the database, issuing a select()
@@ -291,8 +291,6 @@ def backupdb(settings, args):
             shutil.copy(src_path, dest_path)
         except IOError:
             raise Exception("Database backup failed.")
-        # and release the lock again
-        transaction.commit()
 
     database_path = get_database_path_from_settings()
     if database_path:

openslides/agenda/csv_import.py

@@ -25,7 +25,7 @@ def import_agenda_items(csvfile):
         dialect = csv_ext.patchup(dialect)
         csvfile.seek(0)
         # Parse CSV file
-        with transaction.commit_on_success():
+        with transaction.atomic():
             success_lines = []
             error_lines = []
             for (line_no, line) in enumerate(csv.reader(

openslides/agenda/views.py

@@ -7,7 +7,7 @@ from django.contrib import messages
 from django.contrib.contenttypes.models import ContentType
 from django.contrib.staticfiles.templatetags.staticfiles import static
 from django.core.urlresolvers import reverse
-from django.db import transaction
+from django.db import transaction, IntegrityError
 from django.db.models import Model
 from django.template.loader import render_to_string
 from django.utils.datastructures import SortedDict
@@ -117,7 +117,6 @@ class Overview(TemplateView):
             'active_type': active_type})
         return context
 
-    @transaction.commit_manually
     def post(self, request, *args, **kwargs):
         if not request.user.has_perm('agenda.can_manage_agenda'):
             messages.error(
@@ -126,38 +125,42 @@ class Overview(TemplateView):
             context = self.get_context_data(**kwargs)
             return self.render_to_response(context)
 
-        transaction.commit()
+        # Use transaction.atomic() to change all items at once.
-        for item in Item.objects.all():
+        # Raise IntegrityError if some data is invalid.
-            form = ItemOrderForm(request.POST, prefix="i%d" % item.id)
+        # In this case, django does not commit anything, else, the mptt-tree is
-            if form.is_valid():
+        # rebuild.
-                try:
+        try:
-                    parent = Item.objects.get(id=form.cleaned_data['parent'])
+            with transaction.atomic():
-                except Item.DoesNotExist:
+                for item in Item.objects.all():
-                    parent = None
+                    form = ItemOrderForm(request.POST, prefix="i%d" % item.id)
-                else:
+                    if form.is_valid():
-                    if item.type == item.AGENDA_ITEM and parent.type == item.ORGANIZATIONAL_ITEM:
+                        try:
-                        transaction.rollback()
+                            parent = Item.objects.get(id=form.cleaned_data['parent'])
+                        except Item.DoesNotExist:
+                            parent = None
+                        else:
+                            if item.type == item.AGENDA_ITEM and parent.type == item.ORGANIZATIONAL_ITEM:
+                                messages.error(
+                                    request, _('Agenda items can not be child elements of an organizational item.'))
+                                raise IntegrityError
+                        item.parent = parent
+                        item.weight = form.cleaned_data['weight']
+                        Model.save(item)
+                    else:
                         messages.error(
-                            request, _('Agenda items can not be child elements of an organizational item.'))
+                            request, _('Errors when reordering of the agenda'))
-                        break
+                        raise IntegrityError
-                item.parent = parent
+        except IntegrityError:
-                item.weight = form.cleaned_data['weight']
+            pass
-                Model.save(item)
-            else:
-                transaction.rollback()
-                messages.error(
-                    request, _('Errors when reordering of the agenda'))
-                break
         else:
             Item.objects.rebuild()
             # TODO: assure, that it is a valid tree
+
         context = self.get_context_data(**kwargs)
-        transaction.commit()
 
         if get_active_slide()['callback'] == 'agenda':
             update_projector()
         context = self.get_context_data(**kwargs)
-        transaction.commit()
         return self.render_to_response(context)
 
 
@@ -530,7 +533,6 @@ class SpeakerChangeOrderView(SingleObjectMixin, RedirectView):
     def pre_redirect(self, args, **kwargs):
         self.object = self.get_object()
 
-    @transaction.commit_manually
     def pre_post_redirect(self, request, *args, **kwargs):
         """
         Reorder the list of speaker.
@@ -538,24 +540,22 @@ class SpeakerChangeOrderView(SingleObjectMixin, RedirectView):
         Take the string 'sort_order' from the post-data, and use this order.
         """
         self.object = self.get_object()
-        transaction.commit()
+
-        for (counter, speaker) in enumerate(self.request.POST['sort_order'].split(',')):
+        try:
-            try:
+            with transaction.atomic():
-                speaker_pk = int(speaker.split('_')[1])
+                for (counter, speaker) in enumerate(self.request.POST['sort_order'].split(',')):
-            except IndexError:
+                    try:
-                transaction.rollback()
+                        speaker_pk = int(speaker.split('_')[1])
-                break
+                    except IndexError:
-            try:
+                        raise IntegrityError
-                speaker = Speaker.objects.filter(item=self.object).get(pk=speaker_pk)
+                    try:
-            except:
+                        speaker = Speaker.objects.filter(item=self.object).get(pk=speaker_pk)
-                transaction.rollback()
+                    except Speaker.DoesNotExist:
-                break
+                        raise IntegrityError
-            speaker.weight = counter + 1
+                    speaker.weight = counter + 1
-            speaker.save()
+                    speaker.save()
-        else:
+        except IntegrityError:
-            transaction.commit()
+            messages.error(request, _('Could not change order. Invalid data.'))
-            return None
-        messages.error(request, _('Could not change order. Invalid data.'))
 
     def get_url_name_args(self):
         return [self.object.pk]

openslides/motion/csv_import.py

@@ -35,7 +35,7 @@ def import_motions(csvfile, default_submitter, override, importing_person=None):
         return '', '', _('Import file has wrong character encoding, only UTF-8 is supported!')
     csvfile.seek(0)
 
-    with transaction.commit_on_success():
+    with transaction.atomic():
         dialect = csv.Sniffer().sniff(csvfile.readline().decode('utf8'))
         dialect = csv_ext.patchup(dialect)
         csvfile.seek(0)

openslides/users/csv_import.py

@@ -18,13 +18,13 @@ def import_users(csvfile):
         csvfile.read().decode('utf-8')
         csvfile.seek(0)
 
-        with transaction.commit_on_success():
+        with transaction.atomic():
             dialect = csv.Sniffer().sniff(csvfile.readline().decode('utf-8'))
             dialect = csv_ext.patchup(dialect)
             csvfile.seek(0)
 
             for (line_no, line) in enumerate(csv.reader(
-                    (line.decode('utf8') for line in csvfile.readlines()),                                                        dialect=dialect)):
+                    (line.decode('utf8') for line in csvfile.readlines()), dialect=dialect)):
                 if line_no:
                     try:
                         (title, first_name, last_name, gender, email, groups,

tests/agenda/test_list_of_speakers.py

@@ -122,9 +122,9 @@ class SpeakerViewTestCase(TestCase):
         self.item1 = Item.objects.create(title='item1')
         self.item2 = Item.objects.create(title='item2')
 
-    def check_url(self, url, test_client, response_cose):
+    def check_url(self, url, test_client, response_code):
         response = test_client.get(url)
-        self.assertEqual(response.status_code, response_cose)
+        self.assertEqual(response.status_code, response_code)
         return response
 
     def assertMessage(self, response, message):
@@ -345,3 +345,49 @@ class TestCurrentListOfSpeakersOnProjectorView(SpeakerViewTestCase):
         self.assertContains(response, 'List of speakers')
         self.assertContains(response, 'title_gupooDee8ahahnaxoo2a')
         self.assertContains(response, 'speaker1')
+
+
+class TestSpeakerChangeOrderView(SpeakerViewTestCase):
+    def setUp(self):
+        super().setUp()
+        Speaker.objects.add(self.speaker1, self.item1)
+        Speaker.objects.add(self.speaker2, self.item1)
+
+    def test_post(self):
+        """
+        Tests to change the order of two speakers.
+        """
+        data = {'sort_order': 'speaker_2,speaker_1'}
+        self.admin_client.post('/agenda/1/speaker/change_order/',
+                               data)
+
+        self.assertEqual(Speaker.objects.get(pk=1).weight, 2)
+        self.assertEqual(Speaker.objects.get(pk=2).weight, 1)
+
+    def test_invalid_data1(self):
+        """
+        Tests to send invalid data.
+
+        The order should not change.
+        """
+        data = {'sort_order': 'speaker_2,speaker:1'}
+        response = self.admin_client.post('/agenda/1/speaker/change_order/',
+                                          data)
+
+        self.assertEqual(Speaker.objects.get(pk=1).weight, 1)
+        self.assertEqual(Speaker.objects.get(pk=2).weight, 2)
+        self.assertMessage(response, 'Could not change order. Invalid data.')
+
+    def test_invalid_data2(self):
+        """
+        Tests to send a speaker that does not exist.
+
+        The order should not change.
+        """
+        data = {'sort_order': 'speaker_2,speaker_10'}
+        response = self.admin_client.post('/agenda/1/speaker/change_order/',
+                                          data)
+
+        self.assertEqual(Speaker.objects.get(pk=1).weight, 1)
+        self.assertEqual(Speaker.objects.get(pk=2).weight, 2)
+        self.assertMessage(response, 'Could not change order. Invalid data.')

tests/agenda/tests.py

@@ -1,4 +1,5 @@
 from unittest.mock import patch
+from unittest import skip
 
 from django.contrib.auth.models import Permission
 from django.contrib.contenttypes.models import ContentType
@@ -230,6 +231,7 @@ class ViewTest(TestCase):
     def test_change_item_order_with_orga_item(self):
         self.item1.type = 2
         self.item1.save()
+
         data = {
             'i1-self': 1,
             'i1-weight': 50,
@@ -238,9 +240,47 @@ class ViewTest(TestCase):
             'i2-weight': 50,
             'i2-parent': 1}
         response = self.adminClient.post('/agenda/', data)
+
         self.assertNotEqual(Item.objects.get(pk=2).parent_id, 1)
         self.assertContains(response, 'Agenda items can not be child elements of an organizational item.')
 
+    def test_change_item_order_with_form_error(self):
+        """
+        Sends invalid data to the view. The expected behavior is to change
+        nothing.
+        """
+        data = {
+            'i1-self': 1,
+            'i1-weight': 50,
+            'i1-parent': 2,
+            'i2-self': 2,
+            'i2-weight': "invalid",
+            'i2-parent': "invalid"}
+
+        self.adminClient.post('/agenda/', data)
+
+        self.assertIsNone(Item.objects.get(pk=1).parent_id, 0)
+        self.assertIsNone(Item.objects.get(pk=2).parent_id, 0)
+
+    @skip('Check the tree for integrety in the openslides code')
+    def test_change_item_order_with_tree_error(self):
+        """
+        Sends invalid data to the view. The expected behavior is to change
+        nothing.
+        """
+        data = {
+            'i1-self': 1,
+            'i1-weight': 50,
+            'i1-parent': 2,
+            'i2-self': 2,
+            'i2-weight': 50,
+            'i2-parent': 1}
+
+        self.adminClient.post('/agenda/', data)
+
+        self.assertEqual(Item.objects.get(pk=1).parent_id, 0)
+        self.assertEqual(Item.objects.get(pk=2).parent_id, 0)
+
     def test_delete(self):
         response = self.adminClient.get('/agenda/%s/del/' % self.item1.pk)
         self.assertRedirects(response, '/agenda/')