From 656ec5def9e211ca6b41f5ec3fd5764dd7dc3c3e Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Fri, 28 Jan 2022 14:34:39 -0500 Subject: [PATCH] fix: avoid resetting a learner schedule to before a course starts If a learner changes modes (like upgrades to a verified learner), we will reset their schedule for them. But if they did this before the course started, we would accidentally set their schedule to the current time. So when the course did start, they would already appear to be behind schedule. That's silly. So now we always look at course start time when resetting the learner's schedule. AA-426 --- openedx/core/djangoapps/schedules/signals.py | 4 +- .../djangoapps/schedules/tests/test_utils.py | 44 ++++++++++++------- openedx/core/djangoapps/schedules/utils.py | 19 ++++---- .../api/v1/tests/test_views.py | 2 +- .../tests/views/test_course_outline.py | 2 +- 5 files changed, 43 insertions(+), 28 deletions(-) diff --git a/openedx/core/djangoapps/schedules/signals.py b/openedx/core/djangoapps/schedules/signals.py index 0f3a3d944e..c88119d83f 100644 --- a/openedx/core/djangoapps/schedules/signals.py +++ b/openedx/core/djangoapps/schedules/signals.py @@ -82,8 +82,8 @@ def reset_schedule_on_mode_change(sender, user, course_key, mode, **kwargs): # # If switching to audit, reset to when the user got access to course. This is for the case where a user # upgrades to verified (resetting their date), then later refunds the order and goes back to audit. We want # to make sure that audit users go back to their normal audit schedule access. - use_availability_date = mode in CourseMode.AUDIT_MODES - reset_self_paced_schedule(user, course_key, use_availability_date=use_availability_date) + use_enrollment_date = mode in CourseMode.AUDIT_MODES + reset_self_paced_schedule(user, course_key, use_enrollment_date=use_enrollment_date) def _calculate_upgrade_deadline(course_id, content_availability_date): # lint-amnesty, pylint: disable=missing-function-docstring diff --git a/openedx/core/djangoapps/schedules/tests/test_utils.py b/openedx/core/djangoapps/schedules/tests/test_utils.py index f32ed8f753..f1d0cd9fcb 100644 --- a/openedx/core/djangoapps/schedules/tests/test_utils.py +++ b/openedx/core/djangoapps/schedules/tests/test_utils.py @@ -6,6 +6,8 @@ import datetime import ddt from pytz import utc +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory from common.djangoapps.course_modes.models import CourseMode from openedx.core.djangoapps.schedules.models import Schedule @@ -13,38 +15,39 @@ from openedx.core.djangoapps.schedules.tests.factories import ScheduleConfigFact from openedx.core.djangoapps.schedules.utils import reset_self_paced_schedule from openedx.core.djangolib.testing.utils import skip_unless_lms from common.djangoapps.student.tests.factories import CourseEnrollmentFactory -from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.modulestore.tests.factories import CourseFactory # lint-amnesty, pylint: disable=wrong-import-order @ddt.ddt @skip_unless_lms -class ResetSelfPacedScheduleTests(SharedModuleStoreTestCase): # lint-amnesty, pylint: disable=missing-class-docstring - def create_schedule(self, offset=0): # lint-amnesty, pylint: disable=missing-function-docstring - self.config = ScheduleConfigFactory() # lint-amnesty, pylint: disable=attribute-defined-outside-init +class ResetSelfPacedScheduleTests(SharedModuleStoreTestCase): + """Tests for reset_self_paced_schedule""" + def create_schedule(self, enrollment_offset=0, course_start_offset=-100): + """Makes a course, schedule, and enrollment ready to test""" + # pylint: disable=attribute-defined-outside-init + self.config = ScheduleConfigFactory() - start = datetime.datetime.now(utc) - datetime.timedelta(days=100) - self.course = CourseFactory.create(start=start, self_paced=True) # lint-amnesty, pylint: disable=attribute-defined-outside-init + start = datetime.datetime.now(utc) + datetime.timedelta(days=course_start_offset) + self.course = CourseFactory.create(start=start, self_paced=True) - self.enrollment = CourseEnrollmentFactory( # lint-amnesty, pylint: disable=attribute-defined-outside-init + self.enrollment = CourseEnrollmentFactory( course_id=self.course.id, mode=CourseMode.AUDIT, ) - self.enrollment.created = start + datetime.timedelta(days=offset) + self.enrollment.created = start + datetime.timedelta(days=enrollment_offset) self.enrollment.save() - self.schedule = self.enrollment.schedule # lint-amnesty, pylint: disable=attribute-defined-outside-init + self.schedule = self.enrollment.schedule self.schedule.start_date = self.enrollment.created self.schedule.save() - self.user = self.enrollment.user # lint-amnesty, pylint: disable=attribute-defined-outside-init + self.user = self.enrollment.user def test_reset_to_now(self): self.create_schedule() original_start = self.schedule.start_date with self.assertNumQueries(3): - reset_self_paced_schedule(self.user, self.course.id, use_availability_date=False) + reset_self_paced_schedule(self.user, self.course.id, use_enrollment_date=False) self.schedule.refresh_from_db() assert self.schedule.start_date > original_start @@ -55,11 +58,11 @@ class ResetSelfPacedScheduleTests(SharedModuleStoreTestCase): # lint-amnesty, p ) @ddt.unpack def test_reset_to_start_date(self, offset, expected_offset): - self.create_schedule(offset=offset) + self.create_schedule(enrollment_offset=offset) expected_start = self.course.start + datetime.timedelta(days=expected_offset) with self.assertNumQueries(3): - reset_self_paced_schedule(self.user, self.course.id, use_availability_date=True) + reset_self_paced_schedule(self.user, self.course.id, use_enrollment_date=True) self.schedule.refresh_from_db() assert self.schedule.start_date.replace(microsecond=0) == expected_start.replace(microsecond=0) @@ -69,7 +72,16 @@ class ResetSelfPacedScheduleTests(SharedModuleStoreTestCase): # lint-amnesty, p self.create_schedule() self.schedule.delete() - reset_self_paced_schedule(self.user, self.course.id, use_availability_date=False) - reset_self_paced_schedule(self.user, self.course.id, use_availability_date=True) + reset_self_paced_schedule(self.user, self.course.id, use_enrollment_date=False) + reset_self_paced_schedule(self.user, self.course.id, use_enrollment_date=True) assert Schedule.objects.count() == 0 + + @ddt.data(True, False) + def test_reset_before_course_starts(self, use_enrollment_date): + self.create_schedule(enrollment_offset=-5, course_start_offset=5) # starts in 5 days, enrollment is now + + reset_self_paced_schedule(self.user, self.course.id, use_enrollment_date=use_enrollment_date) + + self.schedule.refresh_from_db() + assert self.schedule.start_date == self.enrollment.course.start diff --git a/openedx/core/djangoapps/schedules/utils.py b/openedx/core/djangoapps/schedules/utils.py index d40eed3472..c2565a1c87 100644 --- a/openedx/core/djangoapps/schedules/utils.py +++ b/openedx/core/djangoapps/schedules/utils.py @@ -34,7 +34,7 @@ class PrefixedDebugLoggerMixin: # lint-amnesty, pylint: disable=missing-class-d LOG.info(self.log_prefix + ': ' + message, *args, **kwargs) -def reset_self_paced_schedule(user, course_key, use_availability_date=False): +def reset_self_paced_schedule(user, course_key, use_enrollment_date=False): """ Reset the user's schedule if self-paced. @@ -44,7 +44,7 @@ def reset_self_paced_schedule(user, course_key, use_availability_date=False): Arguments: user (User) course_key (CourseKey or str) - use_availability_date (bool): if False, reset to now, else reset to when user got access to course material + use_enrollment_date (bool): if False, reset to now, else reset to original enrollment creation date """ with transaction.atomic(savepoint=False): try: @@ -56,10 +56,13 @@ def reset_self_paced_schedule(user, course_key, use_availability_date=False): except Schedule.DoesNotExist: return - if use_availability_date: - enrollment = schedule.enrollment - schedule.start_date = max(enrollment.created, enrollment.course.start) - schedule.save() + if use_enrollment_date: + new_start_date = schedule.enrollment.created else: - schedule.start_date = datetime.datetime.now(pytz.utc) - schedule.save() + new_start_date = datetime.datetime.now(pytz.utc) + + # Make sure we don't start the clock on the learner's schedule before the course even starts + new_start_date = max(new_start_date, schedule.enrollment.course.start) + + schedule.start_date = new_start_date + schedule.save() diff --git a/openedx/features/course_experience/api/v1/tests/test_views.py b/openedx/features/course_experience/api/v1/tests/test_views.py index 73a77006cc..f67768c38f 100644 --- a/openedx/features/course_experience/api/v1/tests/test_views.py +++ b/openedx/features/course_experience/api/v1/tests/test_views.py @@ -47,7 +47,7 @@ class ResetCourseDeadlinesViewTests(EventTestMixin, BaseCourseHomeTests, Masquer def test_reset_deadlines_with_masquerade(self): """ Staff users should be able to masquerade as a learner and reset the learner's schedule """ - course = CourseFactory.create(self_paced=True) + course = CourseFactory.create(self_paced=True, start=timezone.now() - datetime.timedelta(days=1)) student_username = self.user.username student_user_id = self.user.id student_enrollment = CourseEnrollment.enroll(self.user, course.id) diff --git a/openedx/features/course_experience/tests/views/test_course_outline.py b/openedx/features/course_experience/tests/views/test_course_outline.py index 34856dc853..524b4f766b 100644 --- a/openedx/features/course_experience/tests/views/test_course_outline.py +++ b/openedx/features/course_experience/tests/views/test_course_outline.py @@ -71,7 +71,7 @@ class TestCourseOutlinePage(SharedModuleStoreTestCase, MasqueradeMixin): # pylint: disable=super-method-not-called with super().setUpClassAndTestData(): cls.courses = [] - course = CourseFactory.create(self_paced=True) + course = CourseFactory.create(self_paced=True, start=timezone.now() - datetime.timedelta(days=1)) with cls.store.bulk_operations(course.id): chapter = ItemFactory.create(category='chapter', parent_location=course.location) sequential = ItemFactory.create(category='sequential', parent_location=chapter.location, graded=True, format="Homework") # lint-amnesty, pylint: disable=line-too-long