From 10b51f68971606b9f48c3953506ac37c35736bdc Mon Sep 17 00:00:00 2001 From: Oskar Hahn Date: Fri, 19 Apr 2013 14:12:49 +0200 Subject: [PATCH 1/2] Fixed #595 --- openslides/motion/models.py | 125 +++++++++++++----- .../templates/motion/motion_detail.html | 6 +- .../motion/templates/motion/motion_list.html | 2 +- tests/motion/test_models.py | 4 + 4 files changed, 97 insertions(+), 40 deletions(-) diff --git a/openslides/motion/models.py b/openslides/motion/models.py index 148425d99..e4be7cfc2 100644 --- a/openslides/motion/models.py +++ b/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): - """Save the motion. + """ + Save the motion. 1. Set the state of a new motion to the default state. 2. Save the motion object. @@ -113,6 +126,9 @@ class Motion(SlideMixin, models.Model): 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) # Find out if the version data has changed @@ -163,7 +179,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 +219,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 +230,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 +239,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 +257,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 +283,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 +314,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 +325,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 +348,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 +381,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 +397,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 +421,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 +434,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 +446,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 +456,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 +466,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. @@ -463,7 +513,8 @@ class Motion(SlideMixin, models.Model): MotionLog.objects.create(motion=self, message=message, person=person) def activate_version(self, version): - """Set the active state of a version to True. + """ + Set the active state of a version to True. 'version' can be a version object, or the version_number of a version. """ @@ -476,7 +527,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 +544,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.""" diff --git a/openslides/motion/templates/motion/motion_detail.html b/openslides/motion/templates/motion/motion_detail.html index a03d51300..a7969e7c2 100644 --- a/openslides/motion/templates/motion/motion_detail.html +++ b/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 %}

- {{ motion.title }} {{ motion.category }} + {{ motion.title }} {{ motion.category|default:'' }}
{% if motion.identifier %} - {% trans "Motion" %} {{ motion.identifier }}, + {% trans "Motion" %} {{ motion.identifier|default:'' }}, {% else %} [{% trans "no number" %}], {% endif %} diff --git a/openslides/motion/templates/motion/motion_list.html b/openslides/motion/templates/motion/motion_list.html index ef4409645..f61f4311e 100644 --- a/openslides/motion/templates/motion/motion_list.html +++ b/openslides/motion/templates/motion/motion_list.html @@ -63,7 +63,7 @@ {% for motion in motion_list %} - {{ motion.identifier }} + {{ motion.identifier|default:'' }} {{ motion.title }} {% if min_supporters > 0 %} {# motion.count_supporters #} diff --git a/tests/motion/test_models.py b/tests/motion/test_models.py index aeb79809f..625264848 100644 --- a/tests/motion/test_models.py +++ b/tests/motion/test_models.py @@ -137,6 +137,10 @@ 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='') + class ConfigTest(TestCase): def test_stop_submitting(self): From 2d7bf8ca9ac82c0e50b73f64f1e61eb9463101cd Mon Sep 17 00:00:00 2001 From: Oskar Hahn Date: Fri, 19 Apr 2013 16:02:16 +0200 Subject: [PATCH 2/2] fixed #558 --- openslides/motion/models.py | 14 +++++++++---- openslides/motion/views.py | 42 ++++++++++++++++++++++++------------- tests/motion/test_models.py | 26 +++++++++++++++++++++++ tests/motion/test_views.py | 28 +++++++++++++++++++++++++ 4 files changed, 91 insertions(+), 19 deletions(-) diff --git a/openslides/motion/models.py b/openslides/motion/models.py index e4be7cfc2..44c364697 100644 --- a/openslides/motion/models.py +++ b/openslides/motion/models.py @@ -102,7 +102,7 @@ class Motion(SlideMixin, models.Model): return self.get_title() # TODO: Use transaction - def save(self, *args, **kwargs): + def save(self, no_new_version=False, *args, **kwargs): """ Save the motion. @@ -122,6 +122,8 @@ 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() @@ -131,6 +133,9 @@ class Motion(SlideMixin, models.Model): 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(): @@ -142,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 @@ -512,9 +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. """ diff --git a/openslides/motion/views.py b/openslides/motion/views.py index 9176e822c..ae59b5b75 100644 --- a/openslides/motion/views.py +++ b/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 form.cleaned_data['new_version']: + if type(self) != MotionCreateView: + 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 - self.object.save() + """ + 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) version_permit = VersionPermitView.as_view() diff --git a/tests/motion/test_models.py b/tests/motion/test_models.py index 625264848..8c89de4e7 100644 --- a/tests/motion/test_models.py +++ b/tests/motion/test_models.py @@ -141,6 +141,32 @@ class ModelTest(TestCase): 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): diff --git a/tests/motion/test_views.py b/tests/motion/test_views.py index 90519ca71..60405838a 100644 --- a/tests/motion/test_views.py +++ b/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)