Merge pull request #117 from edx/ddumesnil/update-transformer-aa-1058

fix: AA-1058: Bust cache when Schedules are updated

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.

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

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)

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:

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

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
             )

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

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)