Merge pull request #4868 from bradenmacdonald/fix-idde-bugs

Fixed: Can set earlier due date as an individual due date extension and related bugs

AUTHORS

@@ -171,3 +171,4 @@ Filippo Valsorda <hi@filippo.io>
 Ivica Ceraj <ceraj@mit.edu>
 Jason Zhu <fmyzjs@gmail.com>
 Marceau Cnudde <marceau.cnudde@gmail.com>
+Braden MacDonald <mail@bradenm.com>

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.

lms/djangoapps/instructor/tests/test_api.py

@@ -47,7 +47,8 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey
 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
@@ -2219,11 +2220,13 @@ class TestDueDateExtensions(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         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 +2286,7 @@ class TestDueDateExtensions(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.week1 = week1
         self.homework = homework
         self.week2 = week2
+        self.week3 = week3
         self.user1 = user1
         self.user2 = user2
 
@@ -2300,6 +2304,32 @@ class TestDueDateExtensions(ModuleStoreTestCase, LoginEnrollmentTestCase):
         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 +2338,38 @@ class TestDueDateExtensions(ModuleStoreTestCase, LoginEnrollmentTestCase):
             '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()

lms/djangoapps/instructor/tests/test_tools.py

@@ -170,6 +170,7 @@ class TestSetDueDateExtension(ModuleStoreTestCase):
         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,10 +193,11 @@ class TestSetDueDateExtension(ModuleStoreTestCase):
         self.week1 = week1
         self.homework = homework
         self.week2 = week2
+        self.week3 = week3
         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)
@@ -207,13 +209,26 @@ class TestSetDueDateExtension(ModuleStoreTestCase):
         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)
 
+    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):
+        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)
@@ -329,22 +344,6 @@ class TestDataDumps(ModuleStoreTestCase):
              "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)

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

lms/djangoapps/instructor/views/tools.py

@@ -159,10 +159,40 @@ 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.
+    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."))
+    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):
         """
         Recursively set the due date on a node and all of its children.