Merge pull request #600 from ostcar/motion

Fixed motion bugs

openslides/motion/models.py

@@ -36,17 +36,21 @@ from .exceptions import MotionError, WorkflowError
 
 
 class Motion(SlideMixin, models.Model):
-    """The Motion Class.
+    """
+    The Motion Class.
 
     This class is the main entry point to all other classes related to a motion.
     """
 
     prefix = 'motion'
-    """Prefix for the slide system."""
+    """
+    Prefix for the slide system.
+    """
 
     active_version = models.ForeignKey('MotionVersion', null=True,
                                        related_name="active_version")
-    """Points to a specific version.
+    """
+    Points to a specific version.
 
     Used be the permitted-version-system to deside which version is the active
     version. Could also be used to only choose a specific version as a default
@@ -54,23 +58,29 @@ class Motion(SlideMixin, models.Model):
     """
 
     state = models.ForeignKey('State', null=True)  # TODO: Check whether null=True is necessary.
-    """The related state object.
+    """
+    The related state object.
 
     This attribute is to get the current state of the motion.
     """
 
     identifier = models.CharField(max_length=255, null=True, blank=True,
                                   unique=True)
-    """A string as human readable identifier for the motion."""
+    """
+    A string as human readable identifier for the motion.
+    """
 
     identifier_number = models.IntegerField(null=True)
-    """Counts the number of the motion in one category.
+    """
+    Counts the number of the motion in one category.
 
     Needed to find the next free motion-identifier.
     """
 
     category = models.ForeignKey('Category', null=True, blank=True)
-    """ForeignKey to one category of motions."""
+    """
+    ForeignKey to one category of motions.
+    """
 
     # TODO: proposal
     #master = models.ForeignKey('self', null=True, blank=True)
@@ -86,12 +96,15 @@ class Motion(SlideMixin, models.Model):
         # ordering = ('number',)
 
     def __unicode__(self):
-        """Return a human readable name of this motion."""
+        """
+        Return a human readable name of this motion.
+        """
         return self.get_title()
 
     # TODO: Use transaction
-    def save(self, *args, **kwargs):
+    def save(self, no_new_version=False, *args, **kwargs):
-        """Save the motion.
+        """
+        Save the motion.
 
         1. Set the state of a new motion to the default state.
         2. Save the motion object.
@@ -109,12 +122,20 @@ class Motion(SlideMixin, models.Model):
 
         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()
 
+        if not self.identifier and self.identifier is not None:
+            self.identifier = None
+
         super(Motion, self).save(*args, **kwargs)
 
+        if no_new_version:
+            return
+
         # Find out if the version data has changed
         for attr in ['title', 'text', 'reason']:
             if not self.versions.exists():
@@ -126,7 +147,8 @@ class Motion(SlideMixin, models.Model):
         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.
+        # 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
@@ -163,7 +185,8 @@ class Motion(SlideMixin, models.Model):
             self.save()
 
     def get_absolute_url(self, link='detail'):
-        """Return an URL for this version.
+        """
+        Return an URL for this version.
 
         The keyword argument 'link' can be 'detail', 'view', 'edit' or 'delete'.
         """
@@ -202,7 +225,8 @@ class Motion(SlideMixin, models.Model):
                 break
 
     def get_title(self):
-        """Get the title of the motion.
+        """
+        Get the title of the motion.
 
         The titel is taken from motion.version.
         """
@@ -212,7 +236,8 @@ class Motion(SlideMixin, models.Model):
             return self.version.title
 
     def set_title(self, title):
-        """Set the titel of the motion.
+        """
+        Set the titel of the motion.
 
         The titel will me saved into the version object, wenn motion.save() is
         called.
@@ -220,13 +245,15 @@ class Motion(SlideMixin, models.Model):
         self._title = title
 
     title = property(get_title, set_title)
-    """The title of the motion.
+    """
+    The title of the motion.
 
     Is saved in a MotionVersion object.
     """
 
     def get_text(self):
-        """Get the text of the motion.
+        """
+        Get the text of the motion.
 
         Simular to get_title().
         """
@@ -236,20 +263,23 @@ class Motion(SlideMixin, models.Model):
             return self.version.text
 
     def set_text(self, text):
-        """ Set the text of the motion.
+        """
+        Set the text of the motion.
 
         Simular to set_title().
         """
         self._text = text
 
     text = property(get_text, set_text)
-    """The text of a motin.
+    """
+    The text of a motin.
 
     Is saved in a MotionVersion object.
     """
 
     def get_reason(self):
-        """Get the reason of the motion.
+        """
+        Get the reason of the motion.
 
         Simular to get_title().
         """
@@ -259,21 +289,24 @@ class Motion(SlideMixin, models.Model):
             return self.version.reason
 
     def set_reason(self, reason):
-        """Set the reason of the motion.
+        """
+        Set the reason of the motion.
 
         Simular to set_title().
         """
         self._reason = reason
 
     reason = property(get_reason, set_reason)
-    """The reason for the motion.
+    """
+    The reason for the motion.
 
     Is saved in a MotionVersion object.
     """
 
     @property
     def new_version(self):
-        """Return a version object, not saved in the database.
+        """
+        Return a version object, not saved in the database.
 
         On the first call, it creates a new version. On any later call, it
         use the existing new version.
@@ -287,7 +320,8 @@ class Motion(SlideMixin, models.Model):
             return self._new_version
 
     def get_version(self):
-        """Get the 'active' version object.
+        """
+        Get the 'active' version object.
 
         This version will be used to get the data for this motion.
         """
@@ -297,7 +331,8 @@ class Motion(SlideMixin, models.Model):
             return self.last_version
 
     def set_version(self, version):
-        """Set the 'active' version object.
+        """
+        Set the 'active' version object.
 
         The keyword argument 'version' can be a MotionVersion object or the
         version_number of a version object or None.
@@ -319,11 +354,15 @@ class Motion(SlideMixin, models.Model):
             self._version = version
 
     version = property(get_version, set_version)
-    """The active version of this motion."""
+    """
+    The active version of this motion.
+    """
 
     @property
     def last_version(self):
-        """Return the newest version of the motion."""
+        """
+        Return the newest version of the motion.
+        """
         # TODO: Fix the case, that the motion has no version
         try:
             return self.versions.order_by('-version_number')[0]
@@ -348,11 +387,15 @@ class Motion(SlideMixin, models.Model):
         MotionSubmitter.objects.create(motion=self, person=person)
 
     def is_supporter(self, person):
-        """Return True, if person is a supporter of this motion. Else: False."""
+        """
+        Return True, if person is a supporter of this motion. Else: False.
+        """
         return self.supporter.filter(person=person).exists()
 
     def support(self, person):
-        """Add 'person' as a supporter of this motion."""
+        """
+        Add 'person' as a supporter of this motion.
+        """
         if self.state.allow_support:
             if not self.is_supporter(person):
                 MotionSupporter(motion=self, person=person).save()
@@ -360,14 +403,17 @@ class Motion(SlideMixin, models.Model):
             raise WorkflowError('You can not support a motion in state %s.' % self.state.name)
 
     def unsupport(self, person):
-        """Remove 'person' as supporter from this motion."""
+        """
+        Remove 'person' as supporter from this motion.
+        """
         if self.state.allow_support:
             self.supporter.filter(person=person).delete()
         else:
             raise WorkflowError('You can not unsupport a motion in state %s.' % self.state.name)
 
     def create_poll(self):
-        """Create a new poll for this motion.
+        """
+        Create a new poll for this motion.
 
         Return the new poll object.
         """
@@ -381,7 +427,8 @@ class Motion(SlideMixin, models.Model):
             raise WorkflowError('You can not create a poll in state %s.' % self.state.name)
 
     def set_state(self, state):
-        """Set the state of the motion.
+        """
+        Set the state of the motion.
 
         State can be the id of a state object or a state object.
         """
@@ -393,7 +440,11 @@ class Motion(SlideMixin, models.Model):
         self.state = state
 
     def reset_state(self):
-        """Set the state to the default state. If the motion is new, it chooses the default workflow from config."""
+        """
+        Set the state to the default state.
+
+        If the motion is new, it chooses the default workflow from config.
+        """
         if self.state:
             self.state = self.state.workflow.first_state
         else:
@@ -401,7 +452,9 @@ class Motion(SlideMixin, models.Model):
                           Workflow.objects.get(pk=config['motion_workflow']).state_set.all()[0])
 
     def slide(self):
-        """Return the slide dict."""
+        """
+        Return the slide dict.
+        """
         data = super(Motion, self).slide()
         data['motion'] = self
         data['title'] = self.title
@@ -409,7 +462,9 @@ class Motion(SlideMixin, models.Model):
         return data
 
     def get_agenda_title(self):
-        """Return a title for the Agenda."""
+        """
+        Return a title for the Agenda.
+        """
         return self.last_version.title  # TODO: nutze active_version
 
     ## def get_agenda_title_supplement(self):
@@ -417,7 +472,8 @@ class Motion(SlideMixin, models.Model):
         ## return '(%s %s)' % (ugettext('motion'), number)
 
     def get_allowed_actions(self, person):
-        """Return a dictonary with all allowed actions for a specific person.
+        """
+        Return a dictonary with all allowed actions for a specific person.
 
         The dictonary contains the following actions.
 
@@ -462,8 +518,9 @@ class Motion(SlideMixin, models.Model):
         """
         MotionLog.objects.create(motion=self, message=message, person=person)
 
-    def activate_version(self, version):
+    def set_active_version(self, version):
-        """Set the active state of a version to True.
+        """
+        Set the active state of a version to 'version'.
 
         'version' can be a version object, or the version_number of a version.
         """
@@ -476,7 +533,8 @@ class Motion(SlideMixin, models.Model):
             version.save()
 
     def reject_version(self, version):
-        """Reject a version of this motion.
+        """
+        Reject a version of this motion.
 
         'version' can be a version object, or the version_number of a version.
         """
@@ -492,7 +550,8 @@ class Motion(SlideMixin, models.Model):
 
 class MotionVersion(models.Model):
     """
-    A MotionVersion object saves some date of the motion."""
+    A MotionVersion object saves some date of the motion.
+    """
 
     motion = models.ForeignKey(Motion, related_name='versions')
     """The motion to which the version belongs."""

openslides/motion/templates/motion/motion_detail.html

@@ -4,15 +4,15 @@
 {% load i18n %}
 {% load staticfiles %}
 
-{% block title %}{{ block.super }} – {% trans "Motion" %} {{ motion.identifier }}{% endblock %}
+{% block title %}{{ block.super }} – {% trans "Motion" %} {{ motion.identifier|default:'' }}{% endblock %}
 
 {% block content %}
 <h1>
-    {{ motion.title }} {{ motion.category }}
+    {{ motion.title }} {{ motion.category|default:'' }}
     <br>
     <small>
     {% if motion.identifier %}
-        {% trans "Motion" %} {{ motion.identifier }},
+        {% trans "Motion" %} {{ motion.identifier|default:'' }},
     {% else %}
         <i>[{% trans "no number" %}]</i>,
     {% endif %}

openslides/motion/templates/motion/motion_list.html

@@ -63,7 +63,7 @@
         </tr>
     {% for motion in motion_list %}
         <tr class="{% if motion.active %}activeline{% endif %}">
-            <td>{{ motion.identifier }}</td>
+            <td>{{ motion.identifier|default:'' }}</td>
             <td><a href="{% model_url motion %}">{{ motion.title }}</a></td>
         {% if min_supporters > 0 %}
             <td class="optional">{# motion.count_supporters #}</td>

openslides/motion/views.py

@@ -83,21 +83,22 @@ motion_detail = MotionDetailView.as_view()
 
 
 class MotionMixin(object):
-    """Mixin for MotionViewsClasses to save the version data."""
+    """
+    Mixin for MotionViewsClasses to save the version data.
+    """
 
     def manipulate_object(self, form):
-        """Save the version data into the motion object before it is saved in
+        """
-        the Database."""
+        Save the version data into the motion object before it is saved in
-
+        the Database.
+        """
         super(MotionMixin, self).manipulate_object(form)
         for attr in ['title', 'text', 'reason']:
             setattr(self.object, attr, form.cleaned_data[attr])
 
-        try:
+        if type(self) != MotionCreateView:
-            if form.cleaned_data['new_version']:
+            if self.object.state.versioning and form.cleaned_data.get('new_version', True):
                 self.object.new_version
-        except KeyError:
-            pass
 
         try:
             self.object.category = form.cleaned_data['category']
@@ -210,29 +211,40 @@ motion_delete = MotionDeleteView.as_view()
 
 
 class VersionPermitView(GetVersionMixin, SingleObjectMixin, QuestionMixin, RedirectView):
-    """View to permit a version of a motion."""
+    """
+    View to permit a version of a motion.
+    """
 
     model = Motion
     question_url_name = 'motion_version_detail'
     success_url_name = 'motion_version_detail'
+    success_message = ugettext_lazy('Version successfully permitted.')
 
     def get(self, *args, **kwargs):
-        """Set self.object to a motion."""
+        """
+        Set self.object to a motion.
+        """
         self.object = self.get_object()
         return super(VersionPermitView, 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 a string, shown to the user as question to permit the version."""
+        """
+        Return a string, shown to the user as question to permit the version.
+        """
         return _('Are you sure you want permit Version %s?') % self.object.version.version_number
 
     def case_yes(self):
-        """Activate the version, if the user chooses 'yes'."""
+        """
-        self.object.activate_version(self.object.version)  # TODO: Write log message
+        Activate the version, if the user chooses 'yes'.
-        self.object.save()
+        """
+        self.object.set_active_version(self.object.version)  # TODO: Write log message
+        self.object.save(no_new_version=True)
 
 version_permit = VersionPermitView.as_view()
 

tests/motion/test_models.py

@@ -137,6 +137,36 @@ class ModelTest(TestCase):
             state_1.next_states.add(state_2)
             state_1.save()
 
+    def test_two_empty_identifiers(self):
+        motion1 = Motion.objects.create(title='foo', text='bar', identifier='')
+        motion2 = Motion.objects.create(title='foo2', text='bar2', identifier='')
+
+    def test_do_not_create_new_version_when_permit_old_version(self):
+        motion = Motion()
+        motion.title = 'foo'
+        motion.text = 'bar'
+        first_version = motion.version
+        my_state = State.objects.create(name='automatic_versioning', workflow=self.workflow,
+                                        versioning=True, dont_set_new_version_active=True)
+        motion.state = my_state
+        motion.save()
+
+        motion = Motion.objects.get(pk=motion.pk)
+        motion.new_version
+        motion.title = 'New Title'
+        motion.save()
+        new_version = motion.last_version
+        self.assertEqual(motion.versions.count(), 2)
+
+        motion.set_active_version(new_version)
+        motion.save()
+        self.assertEqual(motion.versions.count(), 2)
+
+        motion.set_active_version(first_version)
+        motion.version = first_version
+        motion.save(no_new_version=True)
+        self.assertEqual(motion.versions.count(), 2)
+
 
 class ConfigTest(TestCase):
     def test_stop_submitting(self):

tests/motion/test_views.py

@@ -165,3 +165,31 @@ class TestMotionDeleteView(MotionViewTestCase):
         motion = Motion.objects.get(pk=2).add_submitter(self.delegate)
         response = self.delegate_client.post('/motion/2/del/', {})
         self.assertRedirects(response, '/motion/')
+
+
+class TestVersionPermitView(MotionViewTestCase):
+    def setUp(self):
+        super(TestVersionPermitView, self).setUp()
+        self.motion1.new_version
+        self.motion1.save()
+
+    def test_get(self):
+        response = self.check_url('/motion/1/version/2/permit/', self.admin_client, 302)
+        self.assertRedirects(response, '/motion/1/version/2/')
+
+    def test_post(self):
+        new_version = self.motion1.last_version
+        response = self.admin_client.post('/motion/1/version/2/permit/', {'yes': 1})
+        self.assertRedirects(response, '/motion/1/version/2/')
+        self.assertEqual(self.motion1.active_version, new_version)
+
+    def test_activate_old_version(self):
+        new_version = self.motion1.last_version
+        first_version = self.motion1.versions.order_by('version_number')[0]
+
+        self.motion1.set_active_version(new_version)
+        self.assertEqual(self.motion1.versions.count(), 2)
+        response = self.admin_client.post('/motion/1/version/1/permit/', {'yes': 1})
+        self.motion1 = Motion.objects.get(pk=1)
+        self.assertEqual(self.motion1.active_version, first_version)
+        self.assertEqual(self.motion1.versions.count(), 2)