From d4506b73f459d407fece6b4eb2fb275fe8670adf Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Tue, 16 Jun 2020 15:42:03 -0400 Subject: [PATCH] AA-184: Fix extension dashboard for self-paced courses - Have it load dates from edx-when, not just write to it. This fixes self-paced courses where edx-when is only place dates are kept. - Have it read original date for a homework from edx-when when resetting a date. This fixes the message it gives the instructor about whether it was successfully reset. - Have it recursively set a date, rather than assuming that dates are only ever set on the subsection layer. This fixes setting dates on self-paced courses (where dates are set all the way down) and just in case somebody somewhere edits the course xml to have a date where it's not expected. --- lms/djangoapps/instructor/tests/test_api.py | 36 ++++++++++++-- lms/djangoapps/instructor/tests/test_tools.py | 41 ++++++++++------ lms/djangoapps/instructor/views/api.py | 13 +++-- lms/djangoapps/instructor/views/tools.py | 47 +++++++++++++++---- 4 files changed, 101 insertions(+), 36 deletions(-) diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 86585492e2..756ea92174 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -22,10 +22,9 @@ from django.core import mail from django.core.files.uploadedfile import SimpleUploadedFile from django.http import HttpRequest, HttpResponse from django.test import RequestFactory, TestCase -from django.test.utils import override_settings from django.urls import reverse as django_reverse from django.utils.translation import ugettext as _ -from edx_when.api import get_overrides_for_user +from edx_when.api import get_dates_for_course, get_overrides_for_user, set_date_for_block from mock import Mock, NonCallableMock, patch from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locator import UsageKey @@ -3901,11 +3900,18 @@ def get_extended_due(course, unit, user): for override in dates: if text_type(override['location']) == location: return override['actual_date'] - print(unit.location) - print(dates) return None +def get_date_for_block(course, unit, user): + """ + Gets the due date for the given user on the given unit (overridden or original). + Returns `None` if there is no date set. + (Differs from edx-when's get_date_for_block only in that we skip the cache. + """ + return get_dates_for_course(course.id, user=user, use_cached=False).get((unit.location, 'due'), None) + + class TestDueDateExtensions(SharedModuleStoreTestCase, LoginEnrollmentTestCase): """ Test data dumps for reporting. @@ -4042,6 +4048,28 @@ class TestDueDateExtensions(SharedModuleStoreTestCase, LoginEnrollmentTestCase): get_extended_due(self.course, self.week1, self.user1) ) + @RELATIVE_DATES_FLAG.override(True) + def test_reset_date_only_in_edx_when(self): + # Start with a unit that only has a date in edx-when + self.assertEqual(get_date_for_block(self.course, self.week3, self.user1), None) + original_due = datetime.datetime(2010, 4, 1, tzinfo=UTC) + set_date_for_block(self.course.id, self.week3.location, 'due', original_due) + self.assertEqual(get_date_for_block(self.course, self.week3, self.user1), original_due) + + # set override, confirm it took + override = datetime.datetime(2010, 7, 1, tzinfo=UTC) + set_date_for_block(self.course.id, self.week3.location, 'due', override, user=self.user1) + self.assertEqual(get_date_for_block(self.course, self.week3, self.user1), override) + + # Now test that we noticed the edx-when date + url = reverse('reset_due_date', kwargs={'course_id': text_type(self.course.id)}) + response = self.client.post(url, { + 'student': self.user1.username, + 'url': text_type(self.week3.location), + }) + self.assertContains(response, 'Successfully reset due date for student') + self.assertEqual(get_date_for_block(self.course, self.week3, self.user1), original_due) + def test_show_unit_extensions(self): self.test_change_due_date() url = reverse('show_unit_extensions', diff --git a/lms/djangoapps/instructor/tests/test_tools.py b/lms/djangoapps/instructor/tests/test_tools.py index 13294fc97d..9febba7ccc 100644 --- a/lms/djangoapps/instructor/tests/test_tools.py +++ b/lms/djangoapps/instructor/tests/test_tools.py @@ -15,6 +15,7 @@ from django.test import TestCase from opaque_keys.edx.keys import CourseKey from pytz import UTC +from edx_when.api import set_dates_for_course from edx_when.field_data import DateLookupFieldData from openedx.core.djangoapps.course_date_signals import handlers from openedx.core.djangoapps.schedules.tests.factories import ScheduleFactory @@ -137,17 +138,19 @@ class TestGetUnitsWithDueDate(ModuleStoreTestCase): """ Fixtures. """ - super(TestGetUnitsWithDueDate, self).setUp() + super().setUp() + + course = CourseFactory.create() + week1 = ItemFactory.create(parent=course) + week2 = ItemFactory.create(parent=course) + child = ItemFactory.create(parent=week1) due = datetime.datetime(2010, 5, 12, 2, 42, tzinfo=UTC) - course = CourseFactory.create() - week1 = ItemFactory.create(due=due, parent=course) - week2 = ItemFactory.create(due=due, parent=course) - - ItemFactory.create( - parent=week1, - due=due - ) + set_dates_for_course(course.id, [ + (week1.location, {'due': due}), + (week2.location, {'due': due}), + (child.location, {'due': due}), + ]) self.course = course self.week1 = week1 @@ -242,13 +245,21 @@ class TestSetDueDateExtension(ModuleStoreTestCase): block.fields['due']._del_cached_value(block) # pylint: disable=protected-access def test_set_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.assignment, self.user, extended) + # First, extend the leaf assignment date + extended_hw = datetime.datetime(2013, 10, 25, 0, 0, tzinfo=UTC) + tools.set_due_date_extension(self.course, self.assignment, self.user, extended_hw) self._clear_field_data_cache() - self.assertEqual(self.week1.due, extended) - self.assertEqual(self.homework.due, extended) - self.assertEqual(self.assignment.due, extended) + self.assertEqual(self.week1.due, self.due) + self.assertEqual(self.homework.due, self.due) + self.assertEqual(self.assignment.due, extended_hw) + + # Now, extend the whole section that the assignment was in. Both it and all under it should change + extended_week = datetime.datetime(2013, 12, 25, 0, 0, tzinfo=UTC) + tools.set_due_date_extension(self.course, self.week1, self.user, extended_week) + self._clear_field_data_cache() + self.assertEqual(self.week1.due, extended_week) + self.assertEqual(self.homework.due, extended_week) + self.assertEqual(self.assignment.due, extended_week) def test_set_due_date_extension_invalid_date(self): extended = datetime.datetime(2009, 1, 1, 0, 0, tzinfo=UTC) diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 31ad3def55..369e06d2a2 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -8,20 +8,17 @@ Many of these GETs may become PUTs in the future. import csv -import decimal import json import logging import random import re import string -import time import six import unicodecsv from django.conf import settings from django.contrib.auth.models import User from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist, PermissionDenied, ValidationError -from django.core.mail.message import EmailMessage from django.core.validators import validate_email from django.db import IntegrityError, transaction from django.http import HttpResponse, HttpResponseBadRequest, HttpResponseForbidden, HttpResponseNotFound @@ -35,6 +32,7 @@ from django.views.decorators.csrf import ensure_csrf_cookie from django.views.decorators.http import require_http_methods, require_POST from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication from edx_rest_framework_extensions.auth.session.authentication import SessionAuthenticationAllowInactiveUser +from edx_when.api import get_date_for_block from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from rest_framework import status @@ -51,7 +49,6 @@ import instructor_analytics.distributions from bulk_email.api import is_bulk_email_feature_enabled from bulk_email.models import CourseEmail from course_modes.models import CourseMode -from edxmako.shortcuts import render_to_string from lms.djangoapps.certificates import api as certs_api from lms.djangoapps.certificates.models import ( CertificateInvalidation, @@ -93,7 +90,7 @@ from openedx.core.djangoapps.django_comment_common.models import ( Role ) from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers -from openedx.core.djangoapps.user_api.preferences.api import get_user_preference, set_user_preference +from openedx.core.djangoapps.user_api.preferences.api import get_user_preference from openedx.core.djangolib.markup import HTML, Text from openedx.core.lib.api.authentication import BearerAuthenticationAllowInactiveUser from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin @@ -2408,14 +2405,16 @@ def reset_due_date(request, course_id): unit = find_unit(course, request.POST.get('url')) reason = strip_tags(request.POST.get('reason', '')) + original_due_date = get_date_for_block(course_id, unit.location) + set_due_date_extension(course, unit, student, None, request.user, reason=reason) - if not getattr(unit, "due", None): + if not original_due_date: # 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(u'%Y-%m-%d %H:%M') + original_due_date_str = original_due_date.strftime(u'%Y-%m-%d %H:%M') return JsonResponse(_( u'Successfully reset due date for student {0} for {1} ' u'to {2}').format(student.profile.name, _display_unit(unit), diff --git a/lms/djangoapps/instructor/views/tools.py b/lms/djangoapps/instructor/views/tools.py index 70c8f7a0a8..7cbc0987f5 100644 --- a/lms/djangoapps/instructor/views/tools.py +++ b/lms/djangoapps/instructor/views/tools.py @@ -17,6 +17,7 @@ from pytz import UTC from six import string_types, text_type from six.moves import zip +from openedx.core.djangoapps.schedules.models import Schedule from student.models import get_user_by_username_or_email, CourseEnrollment @@ -127,13 +128,19 @@ def get_units_with_due_date(course): """ units = [] + # Pass in a schedule here so that we get back any relative dates in the course, but actual value + # doesn't matter, since we don't care about the dates themselves, just whether they exist. + # Thus we don't save or care about this temporary schedule object. + schedule = Schedule(start_date=course.start) + course_dates = api.get_dates_for_course(course.id, schedule=schedule) + def visit(node): """ Visit a node. Checks to see if node has a due date and appends to `units` if it does. Otherwise recurses into children to search for nodes with due dates. """ - if getattr(node, 'due', None): + if (node.location, 'due') in course_dates: units.append(node) else: for child in node.get_children(): @@ -166,15 +173,35 @@ def set_due_date_extension(course, unit, student, due_date, actor=None, reason=' if not mode: raise DashboardError(_("Could not find student enrollment in the course.")) - if due_date: - try: - api.set_date_for_block(course.id, unit.location, 'due', due_date, user=student, reason=reason, actor=actor) - except api.MissingDateError: - raise DashboardError(_(u"Unit {0} has no due date to extend.").format(unit.location)) - except api.InvalidDateError: - raise DashboardError(_("An extended due date must be later than the original due date.")) - else: - api.set_date_for_block(course.id, unit.location, 'due', None, user=student, reason=reason, actor=actor) + # We normally set dates at the subsection level. But technically dates can be anywhere down the tree (and + # usually are in self paced courses, where the subsection date gets propagated down). + # So find all children that we need to set the date on, then set those dates. + course_dates = api.get_dates_for_course(course.id, user=student) + blocks_to_set = {unit} # always include the requested unit, even if it doesn't appear to have a due date now + + def visit(node): + """ + Visit a node. Checks to see if node has a due date and appends to + `blocks_to_set` if it does. And recurses into children to search for + nodes with due dates. + """ + if (node.location, 'due') in course_dates: + blocks_to_set.add(node) + for child in node.get_children(): + visit(child) + visit(unit) + + for block in blocks_to_set: + if due_date: + try: + api.set_date_for_block(course.id, block.location, 'due', due_date, user=student, reason=reason, + actor=actor) + except api.MissingDateError: + raise DashboardError(_(u"Unit {0} has no due date to extend.").format(unit.location)) + except api.InvalidDateError: + raise DashboardError(_("An extended due date must be later than the original due date.")) + else: + api.set_date_for_block(course.id, block.location, 'due', None, user=student, reason=reason, actor=actor) def dump_module_extensions(course, unit):