Merge pull request #35 from edx/mikix/less-writes

Don't write to db if nothing changed

edx_when/__init__.py

@@ -4,6 +4,6 @@
 
 from __future__ import absolute_import, unicode_literals
 
-__version__ = '1.0.6'
+__version__ = '1.0.7'
 
 default_app_config = 'edx_when.apps.EdxWhenConfig'  # pylint: disable=invalid-name

edx_when/api.py

@@ -49,27 +49,38 @@ def is_enabled_for_course(course_key):
 
 def set_dates_for_course(course_key, items):
     """
-    Extract dates from blocks.
+    Set dates for blocks.
 
     items is an iterator of (location, field metadata dictionary)
     """
     with transaction.atomic():
-        clear_dates_for_course(course_key)
+        active_date_ids = []
+
         for location, fields in items:
             for field in FIELDS_TO_EXTRACT:
                 if field in fields:
                     val = fields[field]
                     if val:
                         log.info('Setting date for %r, %s, %r', location, field, val)
-                        set_date_for_block(course_key, location, field, val)
+                        active_date_ids.append(set_date_for_block(course_key, location, field, val))
+
+        # Now clear out old dates that we didn't touch
+        clear_dates_for_course(course_key, keep=active_date_ids)
 
 
-def clear_dates_for_course(course_key):
+def clear_dates_for_course(course_key, keep=None):
     """
     Set all dates to inactive.
+
+    Arguments:
+        course_key: either a CourseKey or string representation of same
+        keep: an iterable of ContentDate ids to keep active
     """
     course_key = _ensure_key(CourseKey, course_key)
-    models.ContentDate.objects.filter(course_id=course_key, active=True).update(active=False)
+    dates = models.ContentDate.objects.filter(course_id=course_key, active=True)
+    if keep:
+        dates = dates.exclude(id__in=keep)
+    dates.update(active=False)
 
 
 # TODO: Record dates for every block in the course, not just the ones where the block
@@ -234,6 +245,9 @@ def set_date_for_block(course_id, block_id, field, date_or_timedelta, user=None,
     user: user object to override date
     reason: explanation for override
     actor: user object of person making the override
+
+    Returns:
+        a unique id for this block date
     """
     course_id = _ensure_key(CourseKey, course_id)
     block_id = _ensure_key(UsageKey, block_id)
@@ -245,15 +259,19 @@ def set_date_for_block(course_id, block_id, field, date_or_timedelta, user=None,
     else:
         date_kwargs = {'abs_date': date_or_timedelta}
 
-    with transaction.atomic():
+    with transaction.atomic(savepoint=False):  # this is frequently called in a loop, let's avoid the savepoints
         try:
-            existing_date = models.ContentDate.objects.get(course_id=course_id, location=block_id, field=field)
+            existing_date = models.ContentDate.objects.select_related('policy').get(
+                course_id=course_id, location=block_id, field=field
+            )
+            needs_save = not existing_date.active
             existing_date.active = True
         except models.ContentDate.DoesNotExist:
             if user:
                 raise MissingDateError(block_id)
             existing_date = models.ContentDate(course_id=course_id, location=block_id, field=field)
             existing_date.policy, __ = models.DatePolicy.objects.get_or_create(**date_kwargs)
+            needs_save = True
 
         if user and not user.is_anonymous:
             userd = models.UserDate(
@@ -281,7 +299,11 @@ def set_date_for_block(course_id, block_id, field, date_or_timedelta, user=None,
                     date_or_timedelta
                 )
                 existing_date.policy = models.DatePolicy.objects.get_or_create(**date_kwargs)[0]
-        existing_date.save()
+                needs_save = True
+
+        if needs_save:
+            existing_date.save()
+        return existing_date.id
 
 
 class BaseWhenException(Exception):

tests/test_api.py

@@ -11,7 +11,7 @@
 from django.contrib.auth.models import User
 from django.test import TestCase
 from edx_django_utils.cache.utils import DEFAULT_REQUEST_CACHE
-from mock import Mock, patch
+from mock import Mock, call, patch
 from opaque_keys.edx.locator import CourseLocator
 
 from edx_when import api, models
@@ -116,9 +116,18 @@ def test_get_user_date_no_schedule(self):
 
     def test_clear_dates_for_course(self):
         items = self.test_get_dates_for_course()
-        api.clear_dates_for_course(items[0][0].course_key)
+        keep_date = models.ContentDate.objects.get(location=items[1][0])
+
+        with self.assertNumQueries(1):
+            api.clear_dates_for_course(items[0][0].course_key, keep=[keep_date.id])
+
         retrieved = api.get_dates_for_course(items[0][0].course_key, use_cached=False)
-        assert not retrieved
+        self.assertEqual(len(retrieved), 1)
+        self.assertEqual(list(retrieved.keys())[0][0], items[1][0])
+
+        with self.assertNumQueries(1):
+            api.clear_dates_for_course(items[0][0].course_key)
+        self.assertEqual(api.get_dates_for_course(items[0][0].course_key, use_cached=False), {})
 
     def test_set_user_override_invalid_block(self):
         items = make_items()
@@ -370,6 +379,30 @@ def test_get_dates_for_course_query_counts(self, has_schedule, pass_user_object,
         with self.assertNumQueries(query_count):
             api.get_dates_for_course(course_id=self.course.id, user=user, schedule=schedule)
 
+    def test_set_dates_for_course_query_counts(self):
+        items = make_items()
+
+        with self.assertNumQueries(2):  # two for transaction wrappers
+            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)
+
+        self.assertEqual(mock_set.call_count, NUM_OVERRIDES)
+        self.assertEqual(mock_clear.call_args_list, [call(self.course.id, 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))
+
+        # Each date we make has:
+        #  1 get & 1 create for the date itself
+        #  1 get & 1 create for the sub-policy (plus 2 for starting/stopping transactions)
+        with self.assertNumQueries(2 + 4):
+            api.set_date_for_block(*args)
+
+        # When setting same items, we should only do initial read
+        with self.assertNumQueries(1):
+            api.set_date_for_block(*args)
+
 
 class ApiWaffleTests(TestCase):
     """