feat: Optimize the querying of course block dates when querying dates…

… for use in a

course outline, which don't need block dates below the subsection level of a course.
Also, rework the block date caching to use TieredCache, as well as using the current
published version of a course in the cache key to remove the need for cache invalidation.

TNL-8061

CHANGELOG.rst

@@ -14,6 +14,11 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[2.2.0] - 2021-08-27
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+* Added optimization when requesting course block dates for an outline, where block dates below subsections are unneeded.
+* Use current version of the course to improve the cache key, along with using the TieredCache to cache date data.
+
 [2.1.0] - 2021-07-23
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 Added Django 3.2 Support

edx_when/__init__.py

@@ -1,7 +1,7 @@
 """
-Your project description goes here.
+Central source of course block dates for the LMS.
 """
 
-__version__ = '2.1.0'
+__version__ = '2.2.0'
 
 default_app_config = 'edx_when.apps.EdxWhenConfig'  # pylint: disable=invalid-name

edx_when/migrations/0008_courseversion_block_type.py

@@ -0,0 +1,35 @@
+# Generated by Django 3.2.6 on 2021-08-26 17:33
+
+import django.db.models.deletion
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('edx_when', '0007_meta_tweaks'),
+    ]
+
+    operations = [
+        migrations.CreateModel(
+            name='CourseVersion',
+            fields=[
+                ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
+                ('published_version', models.CharField(max_length=255)),
+            ],
+        ),
+        migrations.AddField(
+            model_name='contentdate',
+            name='block_type',
+            field=models.CharField(max_length=255, null=True),
+        ),
+        migrations.AddIndex(
+            model_name='contentdate',
+            index=models.Index(fields=['course_id', 'block_type'], name='edx_when_co_course__01bc36_idx'),
+        ),
+        migrations.AddField(
+            model_name='contentdate',
+            name='course_version',
+            field=models.ForeignKey(null=True, on_delete=django.db.models.deletion.CASCADE, to='edx_when.courseversion'),
+        ),
+    ]

edx_when/models.py

@@ -84,6 +84,16 @@ def clean(self):
             raise ValidationError(_("Absolute and relative dates cannot both be used"))
 
 
+class CourseVersion(models.Model):
+    """
+    Stores published versions of a course. Used to more easily cache the block dates.
+
+    .. no_pii:
+    """
+
+    published_version = models.CharField(max_length=255, blank=False)
+
+
 class ContentDate(models.Model):
     """
     Ties a DatePolicy to a specific piece of course content. (e.g. a due date for a homework).
@@ -96,18 +106,23 @@ class ContentDate(models.Model):
     location = UsageKeyField(null=True, default=None, db_index=True, max_length=255)
     field = models.CharField(max_length=255, default='')
     active = models.BooleanField(default=True)
+    block_type = models.CharField(max_length=255, null=True)
+    course_version = models.ForeignKey(CourseVersion, null=True, on_delete=models.CASCADE)
 
     class Meta:
         """Django Metadata."""
 
         unique_together = ('policy', 'location', 'field')
+        indexes = [
+            models.Index(fields=('course_id', 'block_type')),
+        ]
 
     def __str__(self):
         """
         Get a string representation of this model instance.
         """
         # Location already holds course id
-        return f'{self.location}, {self.field}'
+        return f'ContentDate({self.policy}, {self.location}, {self.field}, {self.block_type})'
 
     def schedule_for_user(self, user):
         """

tests/test_api.py

@@ -8,9 +8,8 @@
 
 import ddt
 from django.contrib import auth
-from django.core.cache import cache
 from django.test import TestCase
-from edx_django_utils.cache.utils import DEFAULT_REQUEST_CACHE
+from edx_django_utils.cache.utils import TieredCache
 from opaque_keys.edx.locator import CourseLocator
 
 from edx_when import api, models
@@ -34,6 +33,8 @@ def setUp(self):
         self.course = DummyCourse(id='course-v1:testX+tt101+2019')
         self.course.save()
 
+        self.course_version = 'TEST_VERSION'
+
         self.user = User(username='tester', email='[email protected]')
         self.user.save()
 
@@ -50,10 +51,9 @@ def setUp(self):
         relative_dates_patcher = patch('edx_when.api._are_relative_dates_enabled', return_value=True)
         relative_dates_patcher.start()
         self.addCleanup(relative_dates_patcher.stop)
-        self.addCleanup(cache.clear)
+        self.addCleanup(TieredCache.dangerous_clear_all_tiers)
 
-        cache.clear()
-        DEFAULT_REQUEST_CACHE.clear()
+        TieredCache.dangerous_clear_all_tiers()
 
     @patch('edx_when.api.Schedule', DummySchedule)
     def test_get_schedules_with_due_date_for_abs_date(self):
@@ -71,7 +71,7 @@ def test_get_schedules_with_due_date_for_abs_date(self):
     @patch('edx_when.api.Schedule', DummySchedule)
     def test_get_schedules_with_due_date_for_rel_date(self):
         items = make_items(with_relative=False)
-        api.set_dates_for_course(items[0][0].course_key, items)
+        api.set_dates_for_course(items[0][0].course_key, items, self.course_version)
         relative_date = timedelta(days=2)
         api.set_date_for_block(items[0][0].course_key, items[0][0], 'due', relative_date)
         assignment_date = items[0][1].get('due') + relative_date
@@ -88,7 +88,7 @@ def test_get_schedules_with_due_date_for_rel_date(self):
     @patch('edx_when.api.Schedule', DummySchedule)
     def test_get_schedules_with_due_date_for_abs_user_dates(self):
         items = make_items(with_relative=True)
-        api.set_dates_for_course(items[0][0].course_key, items)
+        api.set_dates_for_course(items[0][0].course_key, items, self.course_version)
         assignment_date = items[0][1].get('due')
         api.set_date_for_block(items[0][0].course_key, items[0][0], 'due', assignment_date, user=self.user)
         models.UserDate.objects.create(
@@ -105,7 +105,7 @@ def test_get_schedules_with_due_date_for_abs_user_dates(self):
     @patch('edx_when.api.Schedule', DummySchedule)
     def test_get_schedules_with_due_date_for_rel_user_dates(self):
         items = make_items(with_relative=True)
-        api.set_dates_for_course(items[0][0].course_key, items)
+        api.set_dates_for_course(items[0][0].course_key, items, self.course_version)
         assignment_date = items[0][1].get('due')
         api.set_date_for_block(items[0][0].course_key, items[0][0], 'due', assignment_date, user=self.user)
         models.UserDate.objects.create(
@@ -121,14 +121,30 @@ def test_get_schedules_with_due_date_for_rel_user_dates(self):
 
     def test_set_dates_for_course(self):
         items = make_items()
-        api.set_dates_for_course(items[0][0].course_key, items)
+        api.set_dates_for_course(items[0][0].course_key, items, self.course_version)
 
         cdates = models.ContentDate.objects.all()
         assert len(cdates) == NUM_OVERRIDES
 
+    def test_get_dates_for_course_outline(self):
+        items = make_items()
+        course_key = items[0][0].course_key
+        items.append((make_block_id(course_key, block_type='video'), {'start': datetime(2019, 3, 21), 'test': '1'}))
+        api.set_dates_for_course(course_key, items, self.course_version)
+        # Ensure the video block *was* returned normally.
+        retrieved = api.get_dates_for_course(
+            course_key, subsection_and_higher_only=False, published_version=self.course_version
+        )
+        assert len(retrieved) == NUM_OVERRIDES + 1
+        # Ensure the video block *was not* returned with subsection and higher blocks.
+        retrieved = api.get_dates_for_course(
+            course_key, subsection_and_higher_only=True, published_version=self.course_version
+        )
+        assert len(retrieved) == NUM_OVERRIDES
+
     def test_get_dates_for_course(self):
         items = make_items()
-        api.set_dates_for_course(items[0][0].course_key, items)
+        api.set_dates_for_course(items[0][0].course_key, items, self.course_version)
         retrieved = api.get_dates_for_course(items[0][0].course_key)
         assert len(retrieved) == NUM_OVERRIDES
         first = items[0]
@@ -150,7 +166,7 @@ def test_get_dates_for_course(self):
         schedule2 = DummySchedule(enrollment=enrollment2, start_date=datetime(2019, 4, 1))
         schedule2.save()
 
-        api.set_dates_for_course(new_items[0][0].course_key, new_items)
+        api.set_dates_for_course(new_items[0][0].course_key, new_items, self.course_version)
         new_retrieved = api.get_dates_for_course(new_items[0][0].course_key)
         assert len(new_retrieved) == NUM_OVERRIDES
         first_id = list(new_retrieved.keys())[0][0]
@@ -161,7 +177,7 @@ def test_get_dates_for_course(self):
     def test_get_dates_no_schedule(self):
         items = make_items(with_relative=True)
         course_key = items[0][0].course_key
-        api.set_dates_for_course(course_key, items)
+        api.set_dates_for_course(course_key, items, self.course_version)
         retrieved = api.get_dates_for_course(course_key, user=self.user)
         assert len(retrieved) == 6
         self.schedule.delete()
@@ -171,7 +187,7 @@ def test_get_dates_no_schedule(self):
     def test_get_user_date_no_schedule(self):
         items = make_items()
         course_key = items[0][0].course_key
-        api.set_dates_for_course(course_key, items)
+        api.set_dates_for_course(course_key, items, self.course_version)
         before_override = api.get_dates_for_course(course_key, user=self.user)
         assert len(before_override) == 3
 
@@ -202,7 +218,7 @@ def test_set_user_override_invalid_block(self):
         items = make_items()
         first = items[0]
         block_id = first[0]
-        api.set_dates_for_course(str(block_id.course_key), items)
+        api.set_dates_for_course(str(block_id.course_key), items, self.course_version)
 
         with self.assertRaises(api.MissingDateError):
             # can't set a user override for content without a date
@@ -220,7 +236,7 @@ def test_set_user_override_invalid_date(self, initial_date, override_date):
         first = items[0]
         block_id = first[0]
         items[0][1]['due'] = initial_date
-        api.set_dates_for_course(str(block_id.course_key), items)
+        api.set_dates_for_course(str(block_id.course_key), items, self.course_version)
 
         with self.assertRaises(api.InvalidDateError):
             api.set_date_for_block(block_id.course_key, block_id, 'due', override_date, user=self.user)
@@ -238,11 +254,10 @@ def test_set_user_override(self, initial_date, override_date, expected_date):
         block_id = first[0]
         items[0][1]['due'] = initial_date
 
-        api.set_dates_for_course(str(block_id.course_key), items)
+        api.set_dates_for_course(str(block_id.course_key), items, self.course_version)
 
         api.set_date_for_block(block_id.course_key, block_id, 'due', override_date, user=self.user)
-        DEFAULT_REQUEST_CACHE.clear()
-        cache.clear()
+        TieredCache.dangerous_clear_all_tiers()
         retrieved = api.get_dates_for_course(block_id.course_key, user=self.user.id)
         assert len(retrieved) == NUM_OVERRIDES
         assert retrieved[block_id, 'due'] == expected_date
@@ -274,11 +289,9 @@ def test_set_date_for_block(self, initial_date, override_date, expected_date):
         block_id = first[0]
         items[0][1]['due'] = initial_date
 
-        api.set_dates_for_course(str(block_id.course_key), items)
-
+        api.set_dates_for_course(str(block_id.course_key), items, self.course_version)
         api.set_date_for_block(block_id.course_key, block_id, 'due', override_date)
-        DEFAULT_REQUEST_CACHE.clear()
-        cache.clear()
+        TieredCache.dangerous_clear_all_tiers()
         retrieved = api.get_dates_for_course(block_id.course_key, user=self.user.id)
         assert len(retrieved) == NUM_OVERRIDES
         assert retrieved[block_id, 'due'] == expected_date
@@ -296,18 +309,16 @@ def test_remove_user_override(self, initial_date, override_date, expected_date):
         block_id = first[0]
         items[0][1]['due'] = initial_date
 
-        api.set_dates_for_course(str(block_id.course_key), items)
+        api.set_dates_for_course(str(block_id.course_key), items, self.course_version)
 
         api.set_date_for_block(block_id.course_key, block_id, 'due', override_date, user=self.user)
-        DEFAULT_REQUEST_CACHE.clear()
-        cache.clear()
+        TieredCache.dangerous_clear_all_tiers()
         retrieved = api.get_dates_for_course(block_id.course_key, user=self.user.id)
         assert len(retrieved) == NUM_OVERRIDES
         assert retrieved[block_id, 'due'] == expected_date
 
         api.set_date_for_block(block_id.course_key, block_id, 'due', None, user=self.user)
-        DEFAULT_REQUEST_CACHE.clear()
-        cache.clear()
+        TieredCache.dangerous_clear_all_tiers()
         retrieved = api.get_dates_for_course(block_id.course_key, user=self.user.id)
         assert len(retrieved) == NUM_OVERRIDES
         if isinstance(initial_date, timedelta):
@@ -319,7 +330,7 @@ def test_remove_user_override(self, initial_date, override_date, expected_date):
     def test_get_date_for_block(self):
         items = make_items()
         course_id = items[0][0].course_key
-        api.set_dates_for_course(course_id, items)
+        api.set_dates_for_course(course_id, items, self.course_version)
         block_id, data = items[0]
         assert api.get_date_for_block(course_id, block_id, user=self.user) == data['due']
         assert api.get_date_for_block(course_id, 'bad', user=self.user) is None
@@ -328,7 +339,7 @@ def test_is_enabled(self):
         items = make_items()
         course_id = items[0][0].course_key
         assert not api.is_enabled_for_course(course_id)
-        api.set_dates_for_course(course_id, items)
+        api.set_dates_for_course(course_id, items, self.course_version)
         assert api.is_enabled_for_course(course_id)
 
     def test_allow_relative_dates(self):
@@ -352,7 +363,7 @@ def test_allow_relative_dates(self):
             (block3, {'due': date3_delta}),  # relative
             (block4, {'due': date4_delta}),  # relative, to be overwritten by absolute date
         ]
-        api.set_dates_for_course(course_key, items)
+        api.set_dates_for_course(course_key, items, self.course_version)
         api.set_date_for_block(course_key, block2, 'due', date2_override_delta, user=self.user)
         api.set_date_for_block(course_key, block4, 'due', date4_override, user=self.user)
 
@@ -415,7 +426,7 @@ def test_relative_date_past_end_date(self):
             (after_end_date_block, {'due': after_end_date_delta}),  # relative
             (end_block, {'end': end_date}),  # end dates are always absolute
         ]
-        api.set_dates_for_course(course_key, items)
+        api.set_dates_for_course(course_key, items, self.course_version)
 
         dates = [
             ((start_block, 'start'), start_date),
@@ -443,7 +454,7 @@ def test_relative_date_past_cutoff_date(self):
             (second_block, {'due': second_delta}),  # relative
             (end_block, {'end': end_date}),  # end dates are always absolute
         ]
-        api.set_dates_for_course(course_key, items)
+        api.set_dates_for_course(course_key, items, self.course_version)
 
         # Try one with just enough as a sanity check
         self.schedule.created = end_date - second_delta
@@ -456,8 +467,7 @@ def test_relative_date_past_cutoff_date(self):
         ]
         assert api.get_dates_for_course(course_key, schedule=self.schedule) == dict(dates)
 
-        cache.clear()
-        DEFAULT_REQUEST_CACHE.clear()
+        TieredCache.dangerous_clear_all_tiers()
 
         # Now set schedule start date too close to the end date and verify that we no longer get due dates
         self.schedule.created = datetime(2019, 4, 15)
@@ -485,7 +495,7 @@ def test_get_dates_for_course_query_counts(self, has_schedule, pass_user_object,
             (make_block_id(self.course.id), {'due': datetime(2020, 1, 1) + timedelta(days=i)})
             for i in range(item_count)
         ]
-        api.set_dates_for_course(self.course.id, items)
+        api.set_dates_for_course(self.course.id, items, self.course_version)
 
         user = self.user if pass_user_object else self.user.id
         schedule = self.schedule if pass_schedule and has_schedule else None
@@ -506,12 +516,11 @@ def test_get_dates_for_course_query_counts(self, has_schedule, pass_user_object,
             )
             assert dates == cached_dates
 
-        # Now wipe the request cache...
-        DEFAULT_REQUEST_CACHE.clear()
+        # Now wipe all cache tiers...
+        TieredCache.dangerous_clear_all_tiers()
 
-        # This time, test the external cache (which eliminates the one large
-        # query to ContentDates).
-        with self.assertNumQueries(query_count - 1):
+        # No cached values - so will do *all* queries again.
+        with self.assertNumQueries(query_count):
             externally_cached_dates = api.get_dates_for_course(
                 course_id=self.course.id, user=user, schedule=schedule
             )
@@ -527,13 +536,16 @@ def test_get_dates_for_course_query_counts(self, has_schedule, pass_user_object,
     def test_set_dates_for_course_query_counts(self):
         items = make_items()
 
-        with self.assertNumQueries(2):  # two for transaction wrappers
+        with self.assertNumQueries(6):  # four for savepoint wrappers; two for creating the CourseVersion
             with patch('edx_when.api.set_date_for_block', return_value=1) as mock_set:
                 with patch('edx_when.api.clear_dates_for_course') as mock_clear:
-                    api.set_dates_for_course(self.course.id, items)
+                    api.set_dates_for_course(self.course.id, items, self.course_version)
 
         self.assertEqual(mock_set.call_count, NUM_OVERRIDES)
-        self.assertEqual(mock_clear.call_args_list, [call(self.course.id, keep=[1] * NUM_OVERRIDES)])
+        self.assertEqual(
+            mock_clear.call_args_list,
+            [call(self.course.id, self.course_version, keep=[1] * NUM_OVERRIDES)]
+        )
 
     def test_set_date_for_block_query_counts(self):
         args = (self.course.id, make_block_id(self.course.id), 'due', datetime(2019, 3, 22))

tests/test_xblock_services.py

@@ -92,7 +92,6 @@ def test_field_data_has(self):
         assert dfd.has(block, 'due') is True
         assert dfd.has(block, 'foo') is defaults.has(block, 'foo')
         child = MockBlock('child', block)
-        # import pdb;pdb.set_trace()
         assert dfd.has(child, 'due') is False
         assert dfd.default(child, 'due') == self.items[0][1]['due']
         assert dfd.default(child, 'foo') is defaults.default(child, 'foo')