From 42eaa4b00578a2ee6db9c9e68304d4d70219d9ab Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 16 Aug 2014 16:07:18 -0700 Subject: [PATCH 1/6] Unit tests for bugs in IDDE API, including LMS-6563 --- lms/djangoapps/instructor/tests/test_api.py | 65 ++++++++++++++++++- lms/djangoapps/instructor/tests/test_tools.py | 12 ++++ 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 59227e9825a2..998208cdafe7 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -2219,11 +2219,13 @@ def setUp(self): """ Fixtures. """ + super(TestDueDateExtensions, self).setUp() + due = datetime.datetime(2010, 5, 12, 2, 42, tzinfo=utc) course = CourseFactory.create() week1 = ItemFactory.create(due=due) week2 = ItemFactory.create(due=due) - week3 = ItemFactory.create(due=due) + week3 = ItemFactory.create() # No due date course.children = [week1.location.to_deprecated_string(), week2.location.to_deprecated_string(), week3.location.to_deprecated_string()] @@ -2283,6 +2285,7 @@ def setUp(self): self.week1 = week1 self.homework = homework self.week2 = week2 + self.week3 = week3 self.user1 = user1 self.user2 = user2 @@ -2300,6 +2303,32 @@ def test_change_due_date(self): self.assertEqual(datetime.datetime(2013, 12, 30, 0, 0, tzinfo=utc), get_extended_due(self.course, self.week1, self.user1)) + def test_change_to_invalid_due_date(self): + url = reverse('change_due_date', kwargs={'course_id': self.course.id.to_deprecated_string()}) + response = self.client.get(url, { + 'student': self.user1.username, + 'url': self.week1.location.to_deprecated_string(), + 'due_datetime': '01/01/2009 00:00' + }) + self.assertEqual(response.status_code, 400, response.content) + self.assertEqual( + None, + get_extended_due(self.course, self.week1, self.user1) + ) + + def test_change_nonexistent_due_date(self): + url = reverse('change_due_date', kwargs={'course_id': self.course.id.to_deprecated_string()}) + response = self.client.get(url, { + 'student': self.user1.username, + 'url': self.week3.location.to_deprecated_string(), + 'due_datetime': '12/30/2013 00:00' + }) + self.assertEqual(response.status_code, 400, response.content) + self.assertEqual( + None, + get_extended_due(self.course, self.week3, self.user1) + ) + def test_reset_date(self): self.test_change_due_date() url = reverse('reset_due_date', kwargs={'course_id': self.course.id.to_deprecated_string()}) @@ -2308,8 +2337,38 @@ def test_reset_date(self): 'url': self.week1.location.to_deprecated_string(), }) self.assertEqual(response.status_code, 200, response.content) - self.assertEqual(None, - get_extended_due(self.course, self.week1, self.user1)) + self.assertEqual( + None, + get_extended_due(self.course, self.week1, self.user1) + ) + + def test_reset_nonexistent_extension(self): + url = reverse('reset_due_date', kwargs={'course_id': self.course.id.to_deprecated_string()}) + response = self.client.get(url, { + 'student': self.user1.username, + 'url': self.week1.location.to_deprecated_string(), + }) + self.assertEqual(response.status_code, 400, response.content) + + def test_reset_extension_to_deleted_date(self): + """ + Test that we can delete a due date extension after deleting the normal + due date, without causing an error. + """ + self.test_change_due_date() + self.week1.due = None + self.week1 = self.store.update_item(self.week1, self.user1.id) + # Now, week1's normal due date is deleted but the extension still exists. + url = reverse('reset_due_date', kwargs={'course_id': self.course.id.to_deprecated_string()}) + response = self.client.get(url, { + 'student': self.user1.username, + 'url': self.week1.location.to_deprecated_string(), + }) + self.assertEqual(response.status_code, 200, response.content) + self.assertEqual( + None, + get_extended_due(self.course, self.week1, self.user1) + ) def test_show_unit_extensions(self): self.test_change_due_date() diff --git a/lms/djangoapps/instructor/tests/test_tools.py b/lms/djangoapps/instructor/tests/test_tools.py index 722b57a81c02..cdda7f95c29b 100644 --- a/lms/djangoapps/instructor/tests/test_tools.py +++ b/lms/djangoapps/instructor/tests/test_tools.py @@ -170,6 +170,7 @@ def setUp(self): course = CourseFactory.create() week1 = ItemFactory.create(due=due, parent=course) week2 = ItemFactory.create(due=due, parent=course) + week3 = ItemFactory.create(parent=course) homework = ItemFactory.create( parent=week1, @@ -192,6 +193,7 @@ def setUp(self): self.week1 = week1 self.homework = homework self.week2 = week2 + self.week3 = week3 self.user = user self.extended_due = functools.partial( @@ -211,6 +213,16 @@ def test_set_due_date_extension_create_studentmodule(self): self.assertEqual(extended_due(self.week1), extended) self.assertEqual(extended_due(self.homework), extended) + def test_set_due_date_extension_invalid_date(self): + extended = datetime.datetime(2009, 1, 1, 0, 0, tzinfo=utc) + with self.assertRaises(tools.DashboardError): + tools.set_due_date_extension(self.course, self.week1, self.user, extended) + + def test_set_due_date_extension_no_date(self): + extended = datetime.datetime(2013, 12, 25, 0, 0, tzinfo=utc) + with self.assertRaises(tools.DashboardError): + tools.set_due_date_extension(self.course, self.week3, self.user, extended) + def test_reset_due_date_extension(self): tools.set_due_date_extension(self.course, self.week1, self.user, None) self.assertEqual(self.extended_due(self.week1), None) From a08ab9ff11c5133a83c2a7c0d98a324eda1a92a8 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 16 Aug 2014 14:49:15 -0700 Subject: [PATCH 2/6] Fix: an individual extended due date must not precede the normal due date LMS-6563 --- lms/djangoapps/instructor/views/tools.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/tools.py b/lms/djangoapps/instructor/views/tools.py index d195db7a4c41..dd33db64cc56 100644 --- a/lms/djangoapps/instructor/views/tools.py +++ b/lms/djangoapps/instructor/views/tools.py @@ -161,8 +161,16 @@ def title_or_url(node): def set_due_date_extension(course, unit, student, due_date): """ - Sets a due date extension. + Sets a due date extension. Raises DashboardError if the extended + due date is invalid. """ + if due_date: + # Check that the new due date is valid: + original_due_date = getattr(unit, 'due', None) + + if due_date < original_due_date: + raise DashboardError(_("An extended due date must be later than the original due date.")) + def set_due_date(node): """ Recursively set the due date on a node and all of its children. From 3ab87791909944e80f098a132a8bc9a3ae754b6a Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 16 Aug 2014 14:50:39 -0700 Subject: [PATCH 3/6] Fix: API allowed an individual due date extension on units with no due date --- lms/djangoapps/instructor/views/tools.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/tools.py b/lms/djangoapps/instructor/views/tools.py index dd33db64cc56..567255703b09 100644 --- a/lms/djangoapps/instructor/views/tools.py +++ b/lms/djangoapps/instructor/views/tools.py @@ -161,13 +161,15 @@ def title_or_url(node): def set_due_date_extension(course, unit, student, due_date): """ - Sets a due date extension. Raises DashboardError if the extended + Sets a due date extension. Raises DashboardError if the unit or extended due date is invalid. """ if due_date: # Check that the new due date is valid: original_due_date = getattr(unit, 'due', None) + if not original_due_date: + raise DashboardError(_("Unit {0} has no due date to extend.").format(unit.location)) if due_date < original_due_date: raise DashboardError(_("An extended due date must be later than the original due date.")) From f4e021093a5f255dfb7063c7351343edc9d94951 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 16 Aug 2014 15:00:41 -0700 Subject: [PATCH 4/6] Fix: 500 response if indiv. due date deleted after normal due date was deleted --- lms/djangoapps/instructor/views/api.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 49a5a4248c5f..72990cbea456 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1416,11 +1416,17 @@ def reset_due_date(request, course_id): student = get_student_from_identifier(request.GET.get('student')) unit = find_unit(course, request.GET.get('url')) set_due_date_extension(course, unit, student, None) + if not getattr(unit, "due", None): + # It's possible the normal due date was deleted after an extension was granted: + return JsonResponse( + _("Successfully removed invalid due date extension (unit has no due date).") + ) + original_due_date_str = unit.due.strftime('%Y-%m-%d %H:%M') return JsonResponse(_( 'Successfully reset due date for student {0} for {1} ' 'to {2}').format(student.profile.name, _display_unit(unit), - unit.due.strftime('%Y-%m-%d %H:%M'))) + original_due_date_str)) @handle_dashboard_error From 9c93cd8dd630988ac6b9b91eac41a5df7fe773bf Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Tue, 19 Aug 2014 22:15:12 -0700 Subject: [PATCH 5/6] Fix: Display error when attempting to reset nonexistent due date extension --- lms/djangoapps/instructor/tests/test_api.py | 3 ++- lms/djangoapps/instructor/tests/test_tools.py | 23 ++++--------------- lms/djangoapps/instructor/views/tools.py | 20 ++++++++++++++++ 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 998208cdafe7..a776a07fc7a5 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -47,7 +47,8 @@ from shoppingcart.models import CourseRegistrationCode, RegistrationCodeRedemption, Order, PaidCourseRegistration, Coupon from course_modes.models import CourseMode -from .test_tools import msk_from_problem_urlname, get_extended_due +from .test_tools import msk_from_problem_urlname +from ..views.tools import get_extended_due @common_exceptions_400 diff --git a/lms/djangoapps/instructor/tests/test_tools.py b/lms/djangoapps/instructor/tests/test_tools.py index cdda7f95c29b..aa216625a7ee 100644 --- a/lms/djangoapps/instructor/tests/test_tools.py +++ b/lms/djangoapps/instructor/tests/test_tools.py @@ -197,7 +197,7 @@ def setUp(self): self.user = user self.extended_due = functools.partial( - get_extended_due, course, student=user) + tools.get_extended_due, course, student=user) def test_set_due_date_extension(self): extended = datetime.datetime(2013, 12, 25, 0, 0, tzinfo=utc) @@ -209,7 +209,7 @@ def test_set_due_date_extension_create_studentmodule(self): extended = datetime.datetime(2013, 12, 25, 0, 0, tzinfo=utc) user = UserFactory.create() # No student modules for this user tools.set_due_date_extension(self.course, self.week1, user, extended) - extended_due = functools.partial(get_extended_due, self.course, student=user) + extended_due = functools.partial(tools.get_extended_due, self.course, student=user) self.assertEqual(extended_due(self.week1), extended) self.assertEqual(extended_due(self.homework), extended) @@ -224,8 +224,11 @@ def test_set_due_date_extension_no_date(self): tools.set_due_date_extension(self.course, self.week3, self.user, extended) def test_reset_due_date_extension(self): + extended = datetime.datetime(2013, 12, 25, 0, 0, tzinfo=utc) + tools.set_due_date_extension(self.course, self.week1, self.user, extended) tools.set_due_date_extension(self.course, self.week1, self.user, None) self.assertEqual(self.extended_due(self.week1), None) + self.assertEqual(self.extended_due(self.homework), None) @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -341,22 +344,6 @@ def test_dump_student_extensions(self): "Extended Due Date": "2013-12-25 00:00"}]) -def get_extended_due(course, unit, student): - """ - Get the extended due date out of a student's state for a particular unit. - """ - student_module = StudentModule.objects.get( - student_id=student.id, - course_id=course.id, - module_state_key=unit.location - ) - - state = json.loads(student_module.state) - extended = state.get('extended_due', None) - if extended: - return DATE_FIELD.from_json(extended) - - def msk_from_problem_urlname(course_id, urlname, block_type='problem'): """ Convert a 'problem urlname' to a module state key (db field) diff --git a/lms/djangoapps/instructor/views/tools.py b/lms/djangoapps/instructor/views/tools.py index 567255703b09..44919ca06479 100644 --- a/lms/djangoapps/instructor/views/tools.py +++ b/lms/djangoapps/instructor/views/tools.py @@ -159,6 +159,22 @@ def title_or_url(node): return title +def get_extended_due(course, unit, student): + """ + Get the extended due date out of a student's state for a particular unit. + """ + student_module = StudentModule.objects.get( + student_id=student.id, + course_id=course.id, + module_state_key=unit.location + ) + + state = json.loads(student_module.state) + extended = state.get('extended_due', None) + if extended: + return DATE_FIELD.from_json(extended) + + def set_due_date_extension(course, unit, student, due_date): """ Sets a due date extension. Raises DashboardError if the unit or extended @@ -172,6 +188,10 @@ def set_due_date_extension(course, unit, student, due_date): raise DashboardError(_("Unit {0} has no due date to extend.").format(unit.location)) if due_date < original_due_date: raise DashboardError(_("An extended due date must be later than the original due date.")) + else: + # We are deleting a due date extension. Check that it exists: + if not get_extended_due(course, unit, student): + raise DashboardError(_("No due date extension is set for that student and unit.")) def set_due_date(node): """ From b2be62cf1e38c48e55fb27731de03fc1f5e5fa35 Mon Sep 17 00:00:00 2001 From: Braden MacDonald Date: Sat, 16 Aug 2014 16:56:35 -0700 Subject: [PATCH 6/6] Add myself to AUTHORS, update changelog --- AUTHORS | 1 + CHANGELOG.rst | 2 ++ 2 files changed, 3 insertions(+) diff --git a/AUTHORS b/AUTHORS index 2feb253c1ecd..37baa411e1cb 100644 --- a/AUTHORS +++ b/AUTHORS @@ -171,3 +171,4 @@ Filippo Valsorda Ivica Ceraj Jason Zhu Marceau Cnudde +Braden MacDonald diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 4ce1290b666c..f10ef6215b66 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +LMS: Do not allow individual due dates to be earlier than the normal due date. LMS-6563 + Blades: Course teams can turn off Chinese Caching from Studio. BLD-1207 LMS: Instructors can request and see content of previous bulk emails sent in the instructor dashboard.