Merge pull request #638 from normanjaeckel/MotionRework

Motion identifier setting and versioning.

openslides/motion/forms.py

@@ -99,10 +99,27 @@ class MotionDisableVersioningMixin(forms.ModelForm):
     last_version will be used."""
 
 
+# TODO: Add category and identifier to the form as normal fields (the django way),
+# not as 'new' field from 'new' forms.
+
 class MotionCategoryMixin(forms.ModelForm):
-    """Mixin to let the user choose the category for the motion."""
+    """
+    Mixin to let the user choose the category for the motion.
+    """
 
     category = forms.ModelChoiceField(queryset=Category.objects.all(), required=False, label=ugettext_lazy("Category"))
+    """
+    Category of the motion.
+    """
+
+    def __init__(self, *args, **kwargs):
+        """
+        Fill in the category of the motion as default value.
+        """
+        if self.motion is not None:
+            category = self.motion.category
+            self.initial['category'] = category
+        super(MotionCategoryMixin, self).__init__(*args, **kwargs)
 
 
 class MotionIdentifierMixin(forms.ModelForm):
@@ -112,15 +129,9 @@ class MotionIdentifierMixin(forms.ModelForm):
 
     identifier = forms.CharField(required=False, label=ugettext_lazy('Identifier'))
 
-    def clean_identifier(self):
+    class Meta:
-        """
+        model = Motion
-        Test, that the identifier is unique
+        fields = ('identifier',)
-        """
-        identifier = self.cleaned_data['identifier']
-        if Motion.objects.filter(identifier=identifier).exists():
-            raise forms.ValidationError(_('The Identifier is not unique.'))
-        else:
-            return identifier
 
 
 class MotionImportForm(CssClassMixin, forms.Form):

openslides/motion/models.py

@@ -102,87 +102,61 @@ class Motion(SlideMixin, models.Model):
         return self.get_title()
 
     # TODO: Use transaction
-    def save(self, no_new_version=False, *args, **kwargs):
+    def save(self, ignore_version_data=False, *args, **kwargs):
         """
         Save the motion.
 
         1. Set the state of a new motion to the default state.
-        2. Save the motion object.
+        2. Ensure that the identifier is not an empty string.
-        3. Save the version data.
+        3. Save the motion object.
-        4. Set the active version for the motion.
+        4. Save the version data, if ignore_version_data == False.
-
+        5. Set the active version for the motion, if ignore_version_data == False.
-        A new version will be saved if motion.new_version was called
-        between the creation of this object and the last call of motion.save()
-
-            or
-
-        If the motion has new version data (title, text, reason)
-
-            and
-
-        the config 'motion_create_new_version' is set to
-        'ALWAYS_CREATE_NEW_VERSION'.
-
-        If no_new_version is True, a new version will never be used.
         """
         if not self.state:
             self.reset_state()
+            # TODO: Bad hack here to make Motion.objects.create() work
+            # again. We have to remove the flag to force an INSERT given
+            # by Django's create() method without knowing its advantages
+            # because of our misuse of the save() method in the
+            # set_identifier() method.
+            kwargs.pop('force_insert', None)
 
-        if not self.identifier and self.identifier is not None:
+        if not self.identifier and self.identifier is not None:  # TODO: Why not >if self.identifier is '':<
             self.identifier = None
 
         super(Motion, self).save(*args, **kwargs)
 
-        if no_new_version:
+        if not ignore_version_data:
-            return
+            # Select version object
-
-        # Find out if the version data has changed
-        for attr in ['title', 'text', 'reason']:
-            if not self.versions.exists():
-                new_data = True
-                break
-            if getattr(self, attr) != getattr(self.last_version, attr):
-                new_data = True
-                break
-        else:
-            new_data = False
-
-        # TODO: Check everything here. The decision whether to create a new
-        #       version has to be done in the view. Update docstings too.
-        need_new_version = self.state.versioning
-        if hasattr(self, '_new_version') or (new_data and need_new_version):
-            version = self.new_version
-            del self._new_version
-            version.motion = self  # TODO: Test if this line is really neccessary.
-        elif new_data and not need_new_version:
             version = self.last_version
-        else:
+            if hasattr(self, '_new_version'):
-            # We do not need to save the motion version.
+                version = self.new_version
-            return
+                del self._new_version
+                version.motion = self  # TODO: Test if this line is really neccessary.
 
-        # Save title, text and reason in the version object
+            # Save title, text and reason in the version object
-        for attr in ['title', 'text', 'reason']:
+            for attr in ['title', 'text', 'reason']:
-            _attr = '_%s' % attr
+                _attr = '_%s' % attr
-            try:
+                try:
-                setattr(version, attr, getattr(self, _attr))
+                    setattr(version, attr, getattr(self, _attr))
-                delattr(self, _attr)
+                    delattr(self, _attr)
-            except AttributeError:
+                except AttributeError:
-                if self.versions.exists():
+                    if self.versions.exists():
-                    # If the _attr was not set, use the value from last_version
+                        # If the _attr was not set, use the value from last_version
-                    setattr(version, attr, getattr(self.last_version, attr))
+                        setattr(version, attr, getattr(self.last_version, attr))
 
-        # Set version_number of the new Version (if neccessary) and save it into the DB
+            # Set version_number of the new Version (if neccessary) and save it into the DB
-        if version.id is None:
+            if version.id is None:
-            # TODO: auto increment the version_number in the Database
+                # TODO: auto increment the version_number in the Database
-            version_number = self.versions.aggregate(Max('version_number'))['version_number__max'] or 0
+                version_number = self.versions.aggregate(Max('version_number'))['version_number__max'] or 0
-            version.version_number = version_number + 1
+                version.version_number = version_number + 1
-        version.save()
+            version.save()
 
-        # Set the active version of this motion. This has to be done after the
+            # Set the active version of this motion. This has to be done after the
-        # version is saved to the database
+            # version is saved to the database
-        if self.active_version is None or not self.state.leave_old_version_active:
+            if self.active_version is None or not self.state.leave_old_version_active:
-            self.active_version = version
+                self.active_version = version
-            self.save()
+                self.save(ignore_version_data=True)
 
     def get_absolute_url(self, link='detail'):
         """
@@ -198,12 +172,16 @@ class Motion(SlideMixin, models.Model):
             return reverse('motion_delete', args=[str(self.id)])
 
     def set_identifier(self):
-        if config['motion_identifier'] == 'manually':
+        """
+        Sets the motion identifier automaticly according to the config
+        value, if it is not set yet.
+        """
+        if config['motion_identifier'] == 'manually' or self.identifier:
             # Do not set an identifier.
             return
         elif config['motion_identifier'] == 'per_category':
             motions = Motion.objects.filter(category=self.category)
-        else:
+        else:  # That means: config['motion_identifier'] == 'serially_numbered'
             motions = Motion.objects.all()
 
         number = motions.aggregate(Max('identifier_number'))['identifier_number__max'] or 0
@@ -212,23 +190,24 @@ class Motion(SlideMixin, models.Model):
         else:
             prefix = self.category.prefix + ' '
 
+        # TODO: Do not use the save() method in this method, see note in
+        # the save() method above.
         while True:
             number += 1
             self.identifier = '%s%d' % (prefix, number)
+            self.identifier_number = number
             try:
-                self.save()
+                self.save(ignore_version_data=True)
             except IntegrityError:
                 continue
             else:
-                self.number = number
-                self.save()
                 break
 
     def get_title(self):
         """
         Get the title of the motion.
 
-        The titel is taken from motion.version.
+        The title is taken from motion.version.
         """
         try:
             return self._title
@@ -239,7 +218,7 @@ class Motion(SlideMixin, models.Model):
         """
         Set the titel of the motion.
 
-        The titel will me saved into the version object, wenn motion.save() is
+        The title will be saved into the version object, wenn motion.save() is
         called.
         """
         self._title = title
@@ -449,10 +428,11 @@ class Motion(SlideMixin, models.Model):
         If the motion is new, it chooses the default workflow from config.
         """
         if self.state:
-            self.state = self.state.workflow.first_state
+            new_state = self.state.workflow.first_state
         else:
-            self.state = (Workflow.objects.get(pk=config['motion_workflow']).first_state or
+            new_state = (Workflow.objects.get(pk=config['motion_workflow']).first_state or
-                          Workflow.objects.get(pk=config['motion_workflow']).state_set.all()[0])
+                         Workflow.objects.get(pk=config['motion_workflow']).state_set.all()[0])
+        self.set_state(new_state)
 
     def slide(self):
         """

openslides/motion/signals.py

@@ -98,15 +98,15 @@ def setup_motion_config_page(sender, **kwargs):
             choices=[(workflow.pk, ugettext_lazy(workflow.name)) for workflow in Workflow.objects.all()]))
     motion_identifier = ConfigVariable(
         name='motion_identifier',
-        default_value='manually',
+        default_value='serially_numbered',
         form_field=forms.ChoiceField(
             widget=forms.Select(),
-            required=False,
+            required=True,
             label=ugettext_lazy('Identifier'),
             choices=[
-                ('manually', ugettext_lazy('Set it manually')),
+                ('serially_numbered', ugettext_lazy('Serially numbered')),
                 ('per_category', ugettext_lazy('Numbered per category')),
-                ('serially_numbered', ugettext_lazy('Serially numbered'))]))
+                ('manually', ugettext_lazy('Set it manually'))]))
 
     return ConfigPage(title=ugettext_noop('Motion'),
                       url='motion',

openslides/motion/urls.py

@@ -40,11 +40,6 @@ urlpatterns = patterns('openslides.motion.views',
         name='motion_delete',
     ),
 
-    url(r'^(?P<pk>\d+)/set_identifier/',
-        'set_identifier',
-        name='motion_set_identifier',
-    ),
-
     url(r'^(?P<pk>\d+)/version/(?P<version_number>\d+)/$',
         'motion_detail',
         name='motion_version_detail',

openslides/motion/views.py

@@ -99,8 +99,16 @@ class MotionMixin(object):
         for attr in ['title', 'text', 'reason']:
             setattr(self.object, attr, form.cleaned_data[attr])
 
-        if type(self) != MotionCreateView:
+        if type(self) == MotionCreateView:
-            if self.object.state.versioning and form.cleaned_data.get('new_version', True):
+            self.object.new_version
+        else:
+            for attr in ['title', 'text', 'reason']:
+                if getattr(self.object, attr) != getattr(self.object.last_version, attr):
+                    new_data = True
+                    break
+            else:
+                new_data = False
+            if new_data and self.object.state.versioning and not form.cleaned_data.get('disable_versioning', False):
                 self.object.new_version
 
         try:
@@ -114,7 +122,9 @@ class MotionMixin(object):
             pass
 
     def post_save(self, form):
-        """Save the submitter an the supporter so the motion."""
+        """
+        Save the submitter an the supporter so the motion.
+        """
         super(MotionMixin, self).post_save(form)
         # TODO: only delete and save neccessary submitters and supporter
         if 'submitter' in form.cleaned_data:
@@ -129,7 +139,8 @@ class MotionMixin(object):
                  for person in form.cleaned_data['supporter']])
 
     def get_form_class(self):
-        """Return the FormClass to Create or Update the Motion.
+        """
+        Return the FormClass to Create or Update the Motion.
 
         forms.BaseMotionForm is the base for the Class, and some FormMixins
         will be mixed in dependence of some config values. See motion.forms
@@ -138,7 +149,7 @@ class MotionMixin(object):
         form_classes = []
 
         if (self.request.user.has_perm('motion.can_manage_motion') and
-                config['motion_identifier'] == 'manually'):
+                (config['motion_identifier'] == 'manually' or type(self) == MotionUpdateView)):
             form_classes.append(MotionIdentifierMixin)
 
         form_classes.append(BaseMotionForm)
@@ -247,33 +258,42 @@ class VersionPermitView(GetVersionMixin, SingleObjectMixin, QuestionMixin, Redir
         Activate the version, if the user chooses 'yes'.
         """
         self.object.set_active_version(self.object.version)  # TODO: Write log message
-        self.object.save(no_new_version=True)
+        self.object.save(ignore_version_data=True)
 
 version_permit = VersionPermitView.as_view()
 
 
 class VersionRejectView(GetVersionMixin, SingleObjectMixin, QuestionMixin, RedirectView):
-    """View to reject a version."""
+    """
+    View to reject a version.
+    """
+
     model = Motion
     question_url_name = 'motion_version_detail'
     success_url_name = 'motion_version_detail'
 
     def get(self, *args, **kwargs):
-        """Set self.object to a motion."""
+        """
+        Set self.object to a motion.
+        """
         self.object = self.get_object()
         return super(VersionRejectView, self).get(*args, **kwargs)
 
     def get_url_name_args(self):
-        """Return a list with arguments to create the success- and question_url."""
+        """
+        Return a list with arguments to create the success- and question_url.
+        """
         return [self.object.pk, self.object.version.version_number]
 
     def get_question(self):
         return _('Are you sure you want reject Version %s?') % self.object.version.version_number
 
     def case_yes(self):
-        """Reject the version, if the user chooses 'yes'."""
+        """
+        Reject the version, if the user chooses 'yes'.
+        """
         self.object.reject_version(self.object.version)  # TODO: Write log message
-        self.object.save()
+        self.object.save(ignore_version_data=True)
 
 version_reject = VersionRejectView.as_view()
 
@@ -311,30 +331,6 @@ class VersionDiffView(DetailView):
 version_diff = VersionDiffView.as_view()
 
 
-class SetIdentifierView(SingleObjectMixin, RedirectView):
-    """Set the identifier of the motion.
-
-    See motion.set_identifier for more informations
-    """
-    permission_required = 'motion.can_manage_motion'
-    model = Motion
-    url_name = 'motion_detail'
-
-    def get(self, request, *args, **kwargs):
-        """Set self.object to a motion."""
-        self.object = self.get_object()
-        return super(SetIdentifierView, self).get(request, *args, **kwargs)
-
-    def pre_redirect(self, request, *args, **kwargs):
-        """Set the identifier."""
-        self.object.set_identifier()
-
-    def get_url_name_args(self):
-        return [self.object.id]
-
-set_identifier = SetIdentifierView.as_view()
-
-
 class SupportView(SingleObjectMixin, QuestionMixin, RedirectView):
     """View to support or unsupport a motion.
 
@@ -544,10 +540,10 @@ poll_pdf = PollPDFView.as_view()
 
 
 class MotionSetStateView(SingleObjectMixin, RedirectView):
-    """View to set the state of a motion.
+    """
+    View to set the state of a motion.
 
     If self.reset is False, the new state is taken from url.
-
     If self.reset is True, the default state is taken.
     """
     permission_required = 'motion.can_manage_motion'
@@ -556,7 +552,9 @@ class MotionSetStateView(SingleObjectMixin, RedirectView):
     reset = False
 
     def pre_redirect(self, request, *args, **kwargs):
-        """Save the new state and write a log message."""
+        """
+        Save the new state and write a log message.
+        """
         self.object = self.get_object()
         try:
             if self.reset:
@@ -566,17 +564,13 @@ class MotionSetStateView(SingleObjectMixin, RedirectView):
         except WorkflowError, e:  # TODO: Is a WorkflowError still possible here?
             messages.error(request, e)
         else:
-            self.object.save()
+            self.object.save(ignore_version_data=True)
             # TODO: the state is not translated
             self.object.write_log(ugettext_noop('State changed to %s') %
                                   self.object.state.name, self.request.user)
             messages.success(request, _('Motion status was set to: %s.'
                                         % html_strong(self.object.state)))
 
-    def get_url_name_args(self):
-        """Return the arguments to generate the redirect_url."""
-        return [self.object.pk]
-
 set_state = MotionSetStateView.as_view()
 reset_state = MotionSetStateView.as_view(reset=True)
 

tests/motion/test_models.py

@@ -36,8 +36,8 @@ class ModelTest(TestCase):
         motion.save()
         self.assertEqual(motion.versions.count(), 2)
 
-        motion.state = State.objects.create(name='automatic_versioning', workflow=self.workflow, versioning=True)
         motion.text = 'new text'
+        motion.new_version
         motion.save()
         self.assertEqual(motion.versions.count(), 3)
 
@@ -58,11 +58,13 @@ class ModelTest(TestCase):
 
     def test_version(self):
         motion = Motion.objects.create(title='v1')
-        motion.state = State.objects.create(name='automatic_versioning', workflow=self.workflow, versioning=True)
+
         motion.title = 'v2'
+        motion.new_version
         motion.save()
         v2_version = motion.version
         motion.title = 'v3'
+        motion.new_version
         motion.save()
         with self.assertRaises(AttributeError):
             self._title
@@ -146,9 +148,6 @@ class ModelTest(TestCase):
         motion.title = 'foo'
         motion.text = 'bar'
         first_version = motion.version
-        my_state = State.objects.create(name='automatic_versioning', workflow=self.workflow,
-                                        versioning=True, leave_old_version_active=True)
-        motion.state = my_state
         motion.save()
 
         motion = Motion.objects.get(pk=motion.pk)
@@ -164,7 +163,7 @@ class ModelTest(TestCase):
 
         motion.set_active_version(first_version)
         motion.version = first_version
-        motion.save(no_new_version=True)
+        motion.save(ignore_version_data=True)
         self.assertEqual(motion.versions.count(), 2)
 
 

tests/motion/test_views.py

@@ -13,7 +13,7 @@ from django.test.client import Client
 from openslides.config.api import config
 from openslides.utils.test import TestCase
 from openslides.participant.models import User, Group
-from openslides.motion.models import Motion
+from openslides.motion.models import Motion, State
 
 
 class MotionViewTestCase(TestCase):
@@ -120,12 +120,13 @@ class TestMotionCreateView(MotionViewTestCase):
         self.assertContains(response, 'href="/motion/new/"', status_code=200)
 
     def test_identifier_not_unique(self):
-        Motion.objects.create(identifier='foo')
+        Motion.objects.create(title='Another motion 3', identifier='uufag5faoX0thahBi8Fo')
-        response = self.admin_client.post(self.url, {'title': 'foo',
+        config['motion_identifier'] = 'manually'
+        response = self.admin_client.post(self.url, {'title': 'something',
                                                      'text': 'bar',
                                                      'submitter': self.admin,
-                                                     'identifier': 'foo'})
+                                                     'identifier': 'uufag5faoX0thahBi8Fo'})
-        self.assertFormError(response, 'form', 'identifier', 'The Identifier is not unique.')
+        self.assertFormError(response, 'form', 'identifier', 'Motion with this Identifier already exists.')
 
     def test_empty_text_field(self):
         response = self.admin_client.post(self.url, {'title': 'foo',
@@ -162,6 +163,62 @@ class TestMotionUpdateView(MotionViewTestCase):
         motion = Motion.objects.get(pk=1)
         self.assertEqual(motion.title, 'my title')
 
+    def test_versioning(self):
+        self.assertFalse(self.motion1.state.versioning)
+        versioning_state = State.objects.create(name='automatic_versioning', workflow=self.motion1.state.workflow, versioning=True)
+        self.motion1.state = versioning_state
+        self.motion1.save()
+        motion = Motion.objects.get(pk=self.motion1.pk)
+        self.assertTrue(self.motion1.state.versioning)
+
+        self.assertEqual(motion.versions.count(), 1)
+        response = self.admin_client.post(self.url, {'title': 'another new motion_title',
+                                                     'text': 'another motion text',
+                                                     'reason': 'another motion reason',
+                                                     'submitter': self.admin})
+        self.assertRedirects(response, '/motion/1/')
+        motion = Motion.objects.get(pk=self.motion1.pk)
+        self.assertEqual(motion.versions.count(), 2)
+
+    def test_disable_versioning(self):
+        self.assertFalse(self.motion1.state.versioning)
+        versioning_state = State.objects.create(name='automatic_versioning', workflow=self.motion1.state.workflow, versioning=True)
+        self.motion1.state = versioning_state
+        self.motion1.save()
+        motion = Motion.objects.get(pk=self.motion1.pk)
+        self.assertTrue(self.motion1.state.versioning)
+
+        config['motion_allow_disable_versioning'] = True
+        self.assertEqual(motion.versions.count(), 1)
+        response = self.admin_client.post(self.url, {'title': 'another new motion_title',
+                                                     'text': 'another motion text',
+                                                     'reason': 'another motion reason',
+                                                     'submitter': self.admin,
+                                                     'disable_versioning': 'true'})
+        self.assertRedirects(response, '/motion/1/')
+        motion = Motion.objects.get(pk=self.motion1.pk)
+        self.assertEqual(motion.versions.count(), 1)
+
+    def test_no_versioning_without_new_data(self):
+        self.assertFalse(self.motion1.state.versioning)
+        versioning_state = State.objects.create(name='automatic_versioning', workflow=self.motion1.state.workflow, versioning=True)
+        self.motion1.state = versioning_state
+        self.motion1.title = 'Chah4kaaKasiVuishi5x'
+        self.motion1.text = 'eedieFoothae2iethuo3'
+        self.motion1.reason = 'ier2laiy1veeGoo0mau2'
+        self.motion1.save()
+        motion = Motion.objects.get(pk=self.motion1.pk)
+        self.assertTrue(self.motion1.state.versioning)
+
+        self.assertEqual(motion.versions.count(), 1)
+        response = self.admin_client.post(self.url, {'title': 'Chah4kaaKasiVuishi5x',
+                                                     'text': 'eedieFoothae2iethuo3',
+                                                     'reason': 'ier2laiy1veeGoo0mau2',
+                                                     'submitter': self.admin})
+        self.assertRedirects(response, '/motion/1/')
+        motion = Motion.objects.get(pk=self.motion1.pk)
+        self.assertEqual(motion.versions.count(), 1)
+
 
 class TestMotionDeleteView(MotionViewTestCase):
     def test_get(self):