From 88222857bbd3a724930b00facfdd005d8bb7e69b Mon Sep 17 00:00:00 2001 From: Oskar Hahn Date: Sun, 19 Oct 2014 10:16:22 +0200 Subject: [PATCH] Use the new django transaction API fixes #1027 --- openslides/__main__.py | 4 +- openslides/agenda/csv_import.py | 2 +- openslides/agenda/views.py | 88 +++++++++++++-------------- openslides/motion/csv_import.py | 2 +- openslides/users/csv_import.py | 4 +- tests/agenda/test_list_of_speakers.py | 50 ++++++++++++++- tests/agenda/tests.py | 40 ++++++++++++ 7 files changed, 137 insertions(+), 53 deletions(-) diff --git a/openslides/__main__.py b/openslides/__main__.py index 47d7e6b76..d03bd3980 100644 --- a/openslides/__main__.py +++ b/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: diff --git a/openslides/agenda/csv_import.py b/openslides/agenda/csv_import.py index c0a75cfaf..65fb90ff0 100644 --- a/openslides/agenda/csv_import.py +++ b/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( diff --git a/openslides/agenda/views.py b/openslides/agenda/views.py index bef10c199..68df2b79c 100644 --- a/openslides/agenda/views.py +++ b/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() - for item in Item.objects.all(): - form = ItemOrderForm(request.POST, prefix="i%d" % item.id) - if form.is_valid(): - try: - 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: - transaction.rollback() + # Use transaction.atomic() to change all items at once. + # Raise IntegrityError if some data is invalid. + # In this case, django does not commit anything, else, the mptt-tree is + # rebuild. + try: + with transaction.atomic(): + for item in Item.objects.all(): + form = ItemOrderForm(request.POST, prefix="i%d" % item.id) + if form.is_valid(): + try: + 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.')) - break - item.parent = parent - item.weight = form.cleaned_data['weight'] - Model.save(item) - else: - transaction.rollback() - messages.error( - request, _('Errors when reordering of the agenda')) - break + request, _('Errors when reordering of the agenda')) + raise IntegrityError + except IntegrityError: + pass 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: - speaker_pk = int(speaker.split('_')[1]) - except IndexError: - transaction.rollback() - break - try: - speaker = Speaker.objects.filter(item=self.object).get(pk=speaker_pk) - except: - transaction.rollback() - break - speaker.weight = counter + 1 - speaker.save() - else: - transaction.commit() - return None - messages.error(request, _('Could not change order. Invalid data.')) + + try: + with transaction.atomic(): + for (counter, speaker) in enumerate(self.request.POST['sort_order'].split(',')): + try: + speaker_pk = int(speaker.split('_')[1]) + except IndexError: + raise IntegrityError + try: + speaker = Speaker.objects.filter(item=self.object).get(pk=speaker_pk) + except Speaker.DoesNotExist: + raise IntegrityError + speaker.weight = counter + 1 + speaker.save() + except IntegrityError: + messages.error(request, _('Could not change order. Invalid data.')) def get_url_name_args(self): return [self.object.pk] diff --git a/openslides/motion/csv_import.py b/openslides/motion/csv_import.py index 9989fc28c..ec0ae6b67 100644 --- a/openslides/motion/csv_import.py +++ b/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) diff --git a/openslides/users/csv_import.py b/openslides/users/csv_import.py index 4d99e6bc8..1bdd417c4 100644 --- a/openslides/users/csv_import.py +++ b/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, diff --git a/tests/agenda/test_list_of_speakers.py b/tests/agenda/test_list_of_speakers.py index cac93b394..aa8227081 100644 --- a/tests/agenda/test_list_of_speakers.py +++ b/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.') diff --git a/tests/agenda/tests.py b/tests/agenda/tests.py index 57c0a9c5a..812637331 100644 --- a/tests/agenda/tests.py +++ b/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/')