From 256644375f21a4bc754bdf9e427838c9015371b8 Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Wed, 20 Oct 2021 13:37:26 -0400 Subject: [PATCH] fix: AA-1058: Bust cache when Schedules are updated We had a bug reported where learners would reset their due dates for their Personalized Learner Schedule, but the dates wouldn't update. This was a result of the cache key not changing, so this fix adds in the learner's schedule to the transformer. --- CHANGELOG.rst | 4 +++ edx_when/__init__.py | 2 +- edx_when/api.py | 9 +++--- edx_when/models.py | 38 +++++-------------------- edx_when/utils.py | 22 +++++++++++++++ tests/test_api.py | 7 +++-- tests/test_models.py | 52 ++--------------------------------- tests/test_xblock_services.py | 2 +- 8 files changed, 46 insertions(+), 90 deletions(-) create mode 100644 edx_when/utils.py diff --git a/CHANGELOG.rst b/CHANGELOG.rst index fc9f614..11d0473 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -14,6 +14,10 @@ Change Log Unreleased ~~~~~~~~~~ +[2.2.2] - 2021-10-21 +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +* Bug fix to bust cache when Personalized Learner Schedules are updated. + [2.2.1] - 2021-09-15 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ * Bug fix for optimization in 2.2.0, to account for missing block_type data. diff --git a/edx_when/__init__.py b/edx_when/__init__.py index 45e8260..937c3e8 100644 --- a/edx_when/__init__.py +++ b/edx_when/__init__.py @@ -2,6 +2,6 @@ Central source of course block dates for the LMS. """ -__version__ = '2.2.1' +__version__ = '2.2.2' default_app_config = 'edx_when.apps.EdxWhenConfig' # pylint: disable=invalid-name diff --git a/edx_when/api.py b/edx_when/api.py index d446a7c..aa919b0 100644 --- a/edx_when/api.py +++ b/edx_when/api.py @@ -13,6 +13,7 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from . import models +from .utils import get_schedule_for_user try: from openedx.core.djangoapps.schedules.models import Schedule @@ -191,6 +192,9 @@ def get_dates_for_course( else: user_id = user.id if not user.is_anonymous else '' + if schedule is None and user is not None and user_id != '': + schedule = get_schedule_for_user(user_id, course_id) + # Construct the cache key, incorporating all parameters which would cause a different # query set to be returned. processed_results_cache_key = _processed_results_cache_key( @@ -239,14 +243,9 @@ def get_dates_for_course( dates = {} policies = {} - need_schedule = schedule is None and user is not None end_datetime, cutoff_datetime = _get_end_dates_from_content_dates(qset) for cdate in qset: - if need_schedule: - need_schedule = False - schedule = cdate.schedule_for_user(user) - key = (cdate.location.map_into_course(course_id), cdate.field) try: dates[key] = cdate.policy.actual_date(schedule, end_datetime, cutoff_datetime) diff --git a/edx_when/models.py b/edx_when/models.py index 697d5d7..e540bdf 100644 --- a/edx_when/models.py +++ b/edx_when/models.py @@ -5,17 +5,13 @@ from datetime import datetime from django.contrib.auth import get_user_model -from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.core.exceptions import ValidationError from django.db import models from django.utils.translation import gettext_lazy as _ from model_utils.models import TimeStampedModel from opaque_keys.edx.django.models import CourseKeyField, UsageKeyField -try: - from openedx.core.djangoapps.schedules.models import Schedule -# TODO: Move schedules into edx-when -except ImportError: - Schedule = None +from .utils import get_schedule_for_user class MissingScheduleError(ValueError): @@ -113,26 +109,6 @@ def __str__(self): # Location already holds course id return f'ContentDate({self.policy}, {self.location}, {self.field}, {self.block_type})' - def schedule_for_user(self, user): - """ - Return the schedule for the supplied user that applies to this piece of content or None. - """ - no_schedules_found = None - if Schedule is None: - return no_schedules_found - if isinstance(user, int): - try: - return Schedule.objects.get(enrollment__user__id=user, enrollment__course__id=self.course_id) - except ObjectDoesNotExist: - return no_schedules_found - else: - # TODO: We could contemplate using pre-fetched enrollments/schedules, - # but for the moment, just use the fastests non-prefetchable query - try: - return Schedule.objects.get(enrollment__user__id=user.id, enrollment__course__id=self.course_id) - except ObjectDoesNotExist: - return no_schedules_found - class UserDate(TimeStampedModel): """ @@ -158,9 +134,9 @@ def actual_date(self): if self.abs_date: return self.abs_date - schedule_for_user = self.content_date.schedule_for_user(self.user) - policy_date = self.content_date.policy.actual_date(schedule_for_user) - if schedule_for_user and self.rel_date: + schedule = get_schedule_for_user(self.user.id, self.content_date.course_id) # pylint: disable=no-member + policy_date = self.content_date.policy.actual_date(schedule) + if schedule and self.rel_date: return policy_date + self.rel_date else: return policy_date @@ -179,8 +155,8 @@ def clean(self): if self.abs_date and self.rel_date: raise ValidationError(_("Absolute and relative dates cannot both be used")) - user_schedule = self.content_date.schedule_for_user(self.user) - policy_date = self.content_date.policy.actual_date(schedule=user_schedule) + schedule = get_schedule_for_user(self.user.id, self.content_date.course_id) # pylint: disable=no-member + policy_date = self.content_date.policy.actual_date(schedule=schedule) if self.rel_date is not None and self.rel_date.total_seconds() < 0: raise ValidationError(_("Override date must be later than policy date")) if self.abs_date is not None and isinstance(policy_date, datetime) and self.abs_date < policy_date: diff --git a/edx_when/utils.py b/edx_when/utils.py new file mode 100644 index 0000000..6dc22ed --- /dev/null +++ b/edx_when/utils.py @@ -0,0 +1,22 @@ +""" +Utility functions to use across edx-when. +""" +from django.core.exceptions import ObjectDoesNotExist + +try: + from openedx.core.djangoapps.schedules.models import Schedule +# TODO: Move schedules into edx-when +except ImportError: + Schedule = None + + +def get_schedule_for_user(user_id, course_key): + """ + Return the schedule for the user in the course or None if it does not exist or the Schedule model is undefined. + """ + if Schedule: + try: + return Schedule.objects.get(enrollment__user__id=user_id, enrollment__course__id=course_key) + except ObjectDoesNotExist: + pass + return None diff --git a/tests/test_api.py b/tests/test_api.py index 182dd3d..bb99aed 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -44,7 +44,7 @@ def setUp(self): self.schedule = DummySchedule(enrollment=self.enrollment, start_date=datetime(2019, 4, 1)) self.schedule.save() - dummy_schedule_patcher = patch('edx_when.models.Schedule', DummySchedule) + dummy_schedule_patcher = patch('edx_when.utils.Schedule', DummySchedule) dummy_schedule_patcher.start() self.addCleanup(dummy_schedule_patcher.stop) @@ -523,8 +523,9 @@ def test_get_dates_for_course_query_counts(self, has_schedule, pass_user_object, course_id=self.course.id, user=user, schedule=schedule ) - # Second time, the request cache eliminates all querying... - with self.assertNumQueries(0): + # Second time, the request cache eliminates all querying (sometimes)... + # If a schedule is not provided, we will get the schedule to avoid caching outdated dates + with self.assertNumQueries(0 if schedule else 1): cached_dates = api.get_dates_for_course( course_id=self.course.id, user=user, schedule=schedule ) diff --git a/tests/test_models.py b/tests/test_models.py index 040a12f..fd0ebec 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -4,15 +4,14 @@ """ from datetime import datetime, timedelta -from unittest.mock import patch import ddt from django.contrib.auth import get_user_model -from django.core.exceptions import ObjectDoesNotExist, ValidationError +from django.core.exceptions import ValidationError from django.test import TestCase -from edx_when.models import ContentDate, DatePolicy, MissingScheduleError -from tests.test_models_app.models import DummyCourse, DummyEnrollment, DummySchedule +from edx_when.models import DatePolicy, MissingScheduleError +from tests.test_models_app.models import DummySchedule User = get_user_model() @@ -62,48 +61,3 @@ def test_actual_date_schedule_after_cutoff(self): def test_mixed_dates(self): with self.assertRaises(ValidationError): DatePolicy(abs_date=datetime(2020, 1, 1), rel_date=timedelta(days=1)).full_clean() - - -class TestContentDate(TestCase): - """ - Tests of the ContentDate model. - """ - def setUp(self): - super().setUp() - self.course = DummyCourse(id='course-v1:edX+Test+Course') - self.course.save() - - self.user = User() - self.user.save() - - self.enrollment = DummyEnrollment(user=self.user, course=self.course) - self.enrollment.save() - - self.schedule = DummySchedule(enrollment=self.enrollment, start_date=datetime(2020, 1, 1)) - self.schedule.save() - - self.policy = DatePolicy(abs_date=datetime(2020, 1, 1)) - self.content_date = ContentDate(course_id=self.course.id, policy=self.policy) - - @patch('edx_when.models.Schedule', DummySchedule) - def test_schedule_for_user_id(self): - assert self.content_date.schedule_for_user(self.user.id) == self.schedule - - @patch('edx_when.models.Schedule', wraps=DummySchedule) - def test_schedule_for_user_with_object_does_not_exist(self, dummy_schedule): - """Test that None is returned when schedules are fetched for user.""" - dummy_schedule.objects.get.side_effect = ObjectDoesNotExist() - assert self.content_date.schedule_for_user(self.user) is None - - @patch('edx_when.models.Schedule', None) - def test_schedule_for_user_id_no_schedule_installed(self): - assert self.content_date.schedule_for_user(self.user.id) is None - - @patch('edx_when.models.Schedule', DummySchedule) - def test_schedule_for_user_obj(self): - assert self.content_date.schedule_for_user(self.user) == self.schedule - - @patch('edx_when.models.Schedule', wraps=DummySchedule) - def test_schedule_for_user_obj_no_enrollments(self, mock_schedule): - mock_schedule.objects.get.side_effect = ObjectDoesNotExist() - assert self.content_date.schedule_for_user(self.user) is None diff --git a/tests/test_xblock_services.py b/tests/test_xblock_services.py index 563a93b..43cbbc4 100644 --- a/tests/test_xblock_services.py +++ b/tests/test_xblock_services.py @@ -49,7 +49,7 @@ def setUp(self): mock_Schedule = mock.Mock(name="Schedule") mock_Schedule.objects.get.return_value = schedule - schedule_patcher = mock.patch('edx_when.models.Schedule', mock_Schedule) + schedule_patcher = mock.patch('edx_when.utils.Schedule', mock_Schedule) schedule_patcher.start() self.addCleanup(schedule_patcher.stop)