Merge pull request #31 from open-craft/cliff/hotload-percent

Ensure new percent is included in live-updated aggregators.

completion_aggregator/__init__.py

@@ -4,6 +4,6 @@
 
 from __future__ import absolute_import, unicode_literals
 
-__version__ = '1.5.1'
+__version__ = '1.5.2'
 
 default_app_config = 'completion_aggregator.apps.CompletionAggregatorAppConfig'  # pylint: disable=invalid-name

completion_aggregator/serializers.py

@@ -148,14 +148,11 @@ def update_aggregators(self, iterable, is_stale=False):
         """
         if is_stale:
             log.info("Stale completions found for %s+%s, recalculating.", self.user, self.course_key)
-            updated_aggregators = calculate_updated_aggregators(
+            iterable = calculate_updated_aggregators(
                 self.user,
                 self.course_key,
                 force=True,
             )
-            updated_dict = {aggregator.block_key: aggregator for aggregator in updated_aggregators}
-            iterable = (updated_dict.get(agg.block_key, agg) for agg in iterable)
-
         for aggregator in iterable:
             self.add_aggregator(aggregator)
 

completion_aggregator/tasks/aggregation_tasks.py

@@ -233,7 +233,10 @@ def update_for_aggregator(self, block, affected_aggregators, force):
             if modified is not None:
                 last_modified = max(last_modified, modified)
         if self._aggregator_needs_update(block, last_modified, force):
-
+            if total_possible == 0.0:
+                percent = 1.0
+            else:
+                percent = total_earned / total_possible
             Aggregator.objects.validate(self.user, self.course_key, block)
             if block not in self.aggregators:
                 aggregator = Aggregator(
@@ -243,13 +246,15 @@ def update_for_aggregator(self, block, affected_aggregators, force):
                     aggregation_name=block.block_type,
                     earned=total_earned,
                     possible=total_possible,
+                    percent=percent,
                     last_modified=last_modified,
                 )
                 self.aggregators[block] = aggregator
             else:
                 aggregator = self.aggregators[block]
                 aggregator.earned = total_earned
                 aggregator.possible = total_possible
+                aggregator.percent = percent
                 aggregator.last_modified = last_modified
                 aggregator.modified = timezone.now()
             self.updated_aggregators.append(aggregator)

test_settings.py

@@ -20,7 +20,7 @@ def root(*args):
 
 AUTH_USER_MODEL = 'auth.User'
 CELERY_ALWAYS_EAGER = True
-COMPLETION_AGGREGATOR_BLOCK_TYPES = {'course', 'chapter'}
+COMPLETION_AGGREGATOR_BLOCK_TYPES = {'course', 'chapter', 'sequential'}
 COMPLETION_AGGREGATOR_ASYNC_AGGREGATION = True
 
 DATABASES = {
@@ -63,6 +63,12 @@ def root(*args):
 
 ROOT_URLCONF = 'completion_aggregator.urls'
 SECRET_KEY = 'insecure-secret-key'
+TEMPLATES = [
+    {
+        'BACKEND': 'django.template.backends.django.DjangoTemplates',
+        'APP_DIRS': True,
+    },
+]
 USE_TZ = True
 
 # pylint: disable=unused-import,wrong-import-position

test_utils/test_blocks.py

@@ -20,3 +20,10 @@ class StubSequential(XBlock):
     Stub sequential block
     """
     completion_mode = XBlockCompletionMode.AGGREGATOR
+
+
+class StubHTML(XBlock):
+    """
+    Stub HTML block
+    """
+    completion_mode = XBlockCompletionMode.COMPLETABLE

tests/test_aggregation_tasks.py

@@ -4,6 +4,7 @@
 
 from __future__ import absolute_import, division, print_function, unicode_literals
 
+from collections import namedtuple
 from datetime import timedelta
 
 import ddt
@@ -174,6 +175,154 @@ def test_unexpected_updater_errors(self):
                 update_aggregators(username='saskia', course_key='course-v1:OpenCraft+Onboarding+2018')
 
 
+class CalculateUpdatedAggregatorsTestCase(TestCase):
+    """
+    Test that AggragationUpdater.calculate_updated_aggregators() finds the latest completions.
+    """
+    expected_result = namedtuple('expected_result', ['block_key', 'earned', 'updated_earned', 'possible'])
+
+    def setUp(self):
+        self.user = get_user_model().objects.create(username='testuser', email='[email protected]')
+        self.course_key = CourseKey.from_string('OpenCraft/Onboarding/2018')
+        self.blocks = [
+            self.course_key.make_usage_key('course', 'course'),
+            self.course_key.make_usage_key('chapter', 'course-chapter1'),
+            self.course_key.make_usage_key('chapter', 'course-chapter2'),
+            self.course_key.make_usage_key('html', 'course-chapter1-block1'),
+            self.course_key.make_usage_key('html', 'course-chapter1-block2'),
+            self.course_key.make_usage_key('html', 'course-chapter2-block1'),
+            self.course_key.make_usage_key('html', 'course-chapter2-block2'),
+            self.course_key.make_usage_key('chapter', 'course-zeropossible'),
+        ]
+        patch = mock.patch('completion_aggregator.tasks.aggregation_tasks.compat', StubCompat(self.blocks))
+        patch.start()
+        self.addCleanup(patch.stop)
+
+        BlockCompletion.objects.create(
+            user=self.user,
+            course_key=self.course_key,
+            block_key=self.blocks[3],
+            completion=1.0,
+            modified=now(),
+        )
+
+    def _get_updater(self):
+        """
+        Return a fresh instance of an AggregationUpdater.
+        """
+        return AggregationUpdater(self.user, self.course_key, mock.MagicMock())
+
+    def assert_expected_results(self, updated, expected):
+        updated_dict = {agg.block_key: agg for agg in updated}
+        for outcome in expected:
+            if outcome.earned is None:
+                with self.assertRaises(Aggregator.DoesNotExist):
+                    Aggregator.objects.get(block_key=outcome.block_key)
+            else:
+                agg = Aggregator.objects.get(block_key=outcome.block_key)
+                assert agg.earned == outcome.earned
+                assert agg.possible == outcome.possible
+                assert agg.percent == outcome.earned / outcome.possible
+            updated_agg = updated_dict[outcome.block_key]
+            assert updated_agg.earned == outcome.updated_earned
+            assert updated_agg.possible == outcome.possible
+            assert updated_agg.percent == outcome.updated_earned / outcome.possible
+
+    @XBlock.register_temp_plugin(CourseBlock, 'course')
+    @XBlock.register_temp_plugin(OtherAggBlock, 'chapter')
+    @XBlock.register_temp_plugin(HTMLBlock, 'html')
+    def test_unmodified_course(self):
+        self._get_updater().update()
+        self.assert_expected_results(
+            self._get_updater().calculate_updated_aggregators(),
+            [
+                self.expected_result(
+                    block_key=self.blocks[0],
+                    earned=1.0,
+                    updated_earned=1.0,
+                    possible=4.0,
+                ),
+                self.expected_result(
+                    block_key=self.blocks[1],
+                    earned=1.0,
+                    updated_earned=1.0,
+                    possible=2.0,
+                ),
+                self.expected_result(
+                    block_key=self.blocks[2],
+                    earned=0.0,
+                    updated_earned=0.0,
+                    possible=2.0,
+                ),
+            ]
+        )
+
+    @XBlock.register_temp_plugin(CourseBlock, 'course')
+    @XBlock.register_temp_plugin(OtherAggBlock, 'chapter')
+    @XBlock.register_temp_plugin(HTMLBlock, 'html')
+    def test_modified_course(self):
+        self._get_updater().update()
+        for block in self.blocks[4], self.blocks[6]:
+            BlockCompletion.objects.create(
+                user=self.user,
+                course_key=self.course_key,
+                block_key=block,
+                completion=1.0,
+                modified=now(),
+            )
+        self.assert_expected_results(
+            self._get_updater().calculate_updated_aggregators(),
+            [
+                self.expected_result(
+                    block_key=self.blocks[0],
+                    earned=1.0,
+                    updated_earned=3.0,
+                    possible=4.0,
+                ),
+                self.expected_result(
+                    block_key=self.blocks[1],
+                    earned=1.0,
+                    updated_earned=2.0,
+                    possible=2.0,
+                ),
+                self.expected_result(
+                    block_key=self.blocks[2],
+                    earned=0.0,
+                    updated_earned=1.0,
+                    possible=2.0,
+                ),
+            ]
+        )
+
+    @XBlock.register_temp_plugin(CourseBlock, 'course')
+    @XBlock.register_temp_plugin(OtherAggBlock, 'chapter')
+    @XBlock.register_temp_plugin(HTMLBlock, 'html')
+    def test_never_aggregated(self):
+        self.assert_expected_results(
+            self._get_updater().calculate_updated_aggregators(),
+            [
+                self.expected_result(
+                    block_key=self.blocks[0],
+                    earned=None,
+                    updated_earned=1.0,
+                    possible=4.0,
+                ),
+                self.expected_result(
+                    block_key=self.blocks[1],
+                    earned=None,
+                    updated_earned=1.0,
+                    possible=2.0,
+                ),
+                self.expected_result(
+                    block_key=self.blocks[2],
+                    earned=None,
+                    updated_earned=0.0,
+                    possible=2.0,
+                ),
+            ]
+        )
+
+
 class PartialUpdateTest(TestCase):
     """
     Test that when performing an update for a particular block or subset of

tests/test_serializers.py

@@ -83,7 +83,7 @@ def assert_serialized_completions(self, serializer_cls_args, extra_body, recalc_
         for this set of submitted completions.
         """
         serializer_cls = course_completion_serializer_factory(serializer_cls_args)
-        completions = [
+        aggregators = [
             models.Aggregator.objects.submit_completion(
                 user=self.test_user,
                 course_key=self.course_key,
@@ -112,21 +112,27 @@ def assert_serialized_completions(self, serializer_cls_args, extra_body, recalc_
                 last_modified=timezone.now(),
             )[0],
         ]
+        is_stale = recalc_stale and models.StaleCompletion.objects.filter(
+            username=self.test_user.username,
+            course_key=self.course_key,
+            resolved=False
+        )
         completion = AggregatorAdapter(
             user=self.test_user,
             course_key=self.course_key,
-            aggregators=completions,
+            aggregators=aggregators,
             recalculate_stale=recalc_stale,
         )
         serial = serializer_cls(completion)
         expected = {
             'course_key': str(self.course_key),
             'completion': {
-                'earned': 16.0,
-                'possible': 19.0,
-                'percent': 16 / 19,
+                'earned': 0.0 if is_stale else 16.0,
+                'possible': None if is_stale else 19.0,
+                'percent': 0.0 if is_stale else 16 / 19,
             },
         }
+
         expected.update(extra_body)
         # Need to allow for rounding error when retrieving the percent from the test database
         self.assertEqual(serial.data['course_key'], expected['course_key'])
@@ -177,11 +183,9 @@ def assert_serialized_completions(self, serializer_cls_args, extra_body, recalc_
     @ddt.unpack
     @XBlock.register_temp_plugin(StubCourse, 'course')
     @XBlock.register_temp_plugin(StubSequential, 'sequential')
-    @patch.object(AggregationUpdater, 'update')
-    def test_serialize_student_progress_object(self, serializer_cls_args, extra_body, recalc_stale, mock_update):
+    def test_serialize_aggregators(self, serializer_cls_args, extra_body, recalc_stale):
+        assert not models.StaleCompletion.objects.filter(resolved=False).exists()
         self.assert_serialized_completions(serializer_cls_args, extra_body, recalc_stale)
-        # no stale completions, so aggregations are not updated
-        assert mock_update.call_count == 0
 
     @ddt.data(
         (None, True, False),
@@ -207,10 +211,9 @@ def test_serialize_student_progress_object(self, serializer_cls_args, extra_body
     @patch.object(AggregationUpdater, 'calculate_updated_aggregators')
     def test_aggregation_recalc_stale_completions(self, stale_block_key, stale_force, recalc_stale, mock_calculate):
         """
-        Ensure that requesting aggregation when recalculating stale completions causes the aggregations to be
-        recalculated once, and the stale completion resolved.
-
-        If not recalculating the stale completion, then it should remain unresolved.
+        Ensure that requesting aggregation when recalculating stale completions
+        causes the aggregations to be recalculated once, but does not resolve
+        stale completions.
         """
         models.StaleCompletion.objects.create(
             username=self.test_user.username,