Bug 1032343 - task end date prior to start date #151

Merged
merged 1 commit into from Jul 10, 2014

2 participants

@mjzffr

Display error message above create-task/edit-task forms if start
date is after end date.

@bobsilverberg @bitgeeky r?

@bobsilverberg bobsilverberg and 1 other commented on an outdated diff Jul 7, 2014
oneanddone/tasks/forms.py
@@ -39,6 +39,15 @@ def _process_keywords(self, creator):
if len(keyword.strip()):
self.instance.keyword_set.create(name=keyword.strip(), creator=creator)
+ def clean(self):
+ cleaned_data = super(TaskForm, self).clean()
+ start_date = cleaned_data.get('start_date')
+ end_date = cleaned_data.get('end_date')
+ if start_date and start_date:
@bobsilverberg
bobsilverberg added a line comment Jul 7, 2014

Should this be if start_date and end_date?

@mjzffr
mjzffr added a line comment Jul 7, 2014

Yes, thanks for catching that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bobsilverberg bobsilverberg and 1 other commented on an outdated diff Jul 7, 2014
oneanddone/tasks/forms.py
@@ -39,6 +39,15 @@ def _process_keywords(self, creator):
if len(keyword.strip()):
self.instance.keyword_set.create(name=keyword.strip(), creator=creator)
+ def clean(self):
+ cleaned_data = super(TaskForm, self).clean()
+ start_date = cleaned_data.get('start_date')
+ end_date = cleaned_data.get('end_date')
+ if start_date and start_date:
+ if start_date.date() >= end_date.date():
@bobsilverberg
bobsilverberg added a line comment Jul 7, 2014

You should not have to apply .date() to each value here as clean() should have already converted them to the appropriate Python objects. Also, should this be just >? It seems like start_date == end_date would be valid, and would yield a single valid date.

@mjzffr
mjzffr added a line comment Jul 7, 2014

Re .date(): you're right. I thought I was dealing with DateField here; my mistake.

Re >: right now a task with same start date and end date is never considered available. (See [1]) To determine availability, the current time (including hours, minutes) is compared to midnight of the end date, etc.

@bobsilverberg I think how availability is defined now makes sense so the validation should stay as >=. (It makes sense to me that start date of Jul 6 and end date of Jul 7 means 24 hours of availability, so equal start and end should be rejected.)

[1] https://github.com/mozilla/oneanddone/blob/master/oneanddone/tasks/models.py#L99

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bobsilverberg bobsilverberg commented on an outdated diff Jul 7, 2014
oneanddone/tasks/forms.py
@@ -39,6 +39,15 @@ def _process_keywords(self, creator):
if len(keyword.strip()):
self.instance.keyword_set.create(name=keyword.strip(), creator=creator)
+ def clean(self):
+ cleaned_data = super(TaskForm, self).clean()
+ start_date = cleaned_data.get('start_date')
+ end_date = cleaned_data.get('end_date')
+ if start_date and start_date:
+ if start_date.date() >= end_date.date():
+ raise forms.ValidationError("'End date' must be after 'Start date'")
@bobsilverberg
bobsilverberg added a line comment Jul 7, 2014

Please apply i18n to the validation failure message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bobsilverberg

Looks good and works well, @mjzffr. Just one more thing: please add a unit test for this new functionality. Take a look at https://github.com/mozilla/oneanddone/blob/master/oneanddone/tasks/tests/test_forms.py for some examples of unit tests for the Task form.

@mjzffr mjzffr and 1 other commented on an outdated diff Jul 8, 2014
oneanddone/tasks/tests/test_forms.py
+
+ def test_validation_start_date_before_end_date(self):
+ """
+ The form is valid if start date is before end date and all other
+ field requirements are respected.
+ """
+ task = TaskFactory.create()
+ data = {
+ 'start_date': '2013-07-01',
+ 'end_date': '2013-08-15',
+ 'team': task.team.id,
+ }
+ for field in ('name', 'short_description', 'execution_time', 'difficulty',
+ 'repeatable', 'instructions', 'is_draft'):
+ data[field] = getattr(task, field)
+ form = TaskForm(instance=task, data=data)
@mjzffr
mjzffr added a line comment Jul 8, 2014

@bobsilverberg Lines 90-99 are repeated in many test cases. I propose to move this code to a helper function in the test_forms module. Sounds good?

@bobsilverberg
bobsilverberg added a line comment Jul 9, 2014

Yes, please do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mjzffr

@bobsilverberg Could you let me know if I'm on the right track, please? I'm not sure if the 3 test cases I added so far are too much/too little in terms of coverage. I could also add a test case or two to check that the right error message shows up, with assertFormError. What do you think?

@bobsilverberg

Looks good, @mjzffr. A great start. I don't think it's too much coverage - I think all three cases you've identified should be covered. I do think you should add a test case to check that the correct error message is thrown as well.

@mjzffr mjzffr commented on the diff Jul 9, 2014
oneanddone/tasks/tests/test_forms.py
+ start_date='2014-07-01',
+ end_date='2013-08-15')
+
+ self.assertFalse(form.is_valid())
+ eq_(form.non_field_errors(), ["'End date' must be after 'Start date'"])
+
+ def test_validation_same_start_date_as_end_date(self):
+ """
+ The form is not valid if start date is same as end date.
+ """
+ form = get_filled_taskform(TaskFactory.create(),
+ start_date='2013-08-15',
+ end_date='2013-08-15')
+
+ self.assertFalse(form.is_valid())
+ eq_(form.non_field_errors(), ["'End date' must be after 'Start date'"])
@mjzffr
mjzffr added a line comment Jul 9, 2014

Thanks for the feedback, Bob.
In the end, I stepped away from using assertFormError to instead check non_field_errors directly in each of the original date validation tests. (This direct approach seems better anyway, but in case you're curious: I got stuck on simulating an authenticated admin user when generating a response for assertFormError to test; if you happen to know how to do that, I'd love to hear about it.)

@bobsilverberg
bobsilverberg added a line comment Jul 10, 2014

Off the top of my head I'm not sure how to do that. If I need to do it in the future and figure it out I'll let you know. Checking non_field_errors instead seems fine to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@bobsilverberg bobsilverberg commented on an outdated diff Jul 10, 2014
oneanddone/tasks/tests/test_forms.py
@@ -11,6 +11,19 @@
from oneanddone.tasks.tests import TaskFactory, TaskKeywordFactory
from oneanddone.users.tests import UserFactory
+def get_filled_taskform(task, **new_data):
@bobsilverberg
bobsilverberg added a line comment Jul 10, 2014

I like the refactor, but could you change **new_data to **kwargs as that is more Pythonic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mjzffr mjzffr Bug 1032343 - task end date prior to start date
Display error message above create-task/edit-task forms if start
date is after end date. Test validation and error message for
invalid dates.
a9a4a28
@bobsilverberg

Looks good. Thanks @mjzffr!

@bobsilverberg bobsilverberg merged commit d1109e1 into mozilla:master Jul 10, 2014
@mjzffr mjzffr deleted the mjzffr:validtaskdate branch Jul 22, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment