Merge pull request #23 from open-craft/cliff/handle-bad-data

Handle aggregation for courses with bad data

completion_aggregator/compat.py

@@ -46,15 +46,12 @@ def get_modulestore():
     return modulestore()
 
 
-def get_item(content_key):
+def get_item_not_found_error():
     """
-    Return an item from the modulestore.
+    Return ItemNotFoundError.
     """
     from xmodule.modulestore.exceptions import ItemNotFoundError  # pylint: disable=import-error
-    try:
-        get_modulestore().get_item(content_key)
-    except ItemNotFoundError:
-        return False
+    return ItemNotFoundError
 
 
 def init_course_blocks(user, course_block_key):

completion_aggregator/tasks/aggregation_tasks.py

@@ -108,8 +108,14 @@ def _update_aggregators(user, course_key, block_keys=frozenset(), force=False):
             If True, update aggregators even if they are up-to-date.
 
     """
-    updater = AggregationUpdater(user, course_key, compat.get_modulestore())
-    updater.update(block_keys, force)
+    try:
+        updater = AggregationUpdater(user, course_key, compat.get_modulestore())
+    except compat.get_item_not_found_error():
+        log.exception("Course not found in modulestore.  Skipping aggregation for %s/%s.", user, course_key)
+    except TypeError:
+        log.exception("Could not parse modulestore data.  Skipping aggregation for %s/%s.", user, course_key)
+    else:
+        updater.update(block_keys, force)
 
 
 class AggregationUpdater(object):

test_utils/compat.py

@@ -80,6 +80,11 @@ def course_enrollment_model(self):
     def get_mobile_only_courses(self):
         return MagicMock()
 
+    def get_item_not_found_error(self):
+        """
+        Use ValueError as a replacement for modulestore's ItemNotFoundError
+        """
+        return ValueError
 
 
 CourseTreeNode = collections.namedtuple('CourseTreeNode', ['block', 'children'])

tests/test_aggregation_tasks.py

@@ -6,6 +6,7 @@
 
 from datetime import timedelta
 
+import ddt
 import mock
 import pytest
 import six
@@ -23,6 +24,7 @@
 from test_utils.xblocks import CourseBlock, HiddenBlock, HTMLBlock, InvalidModeBlock, OtherAggBlock
 
 
+@ddt.ddt
 class AggregationUpdaterTestCase(TestCase):
     """
     Test the AggregationUpdater.
@@ -147,6 +149,30 @@ def test_invalid_completion_mode(self):
         with pytest.raises(ValueError):
             self.updater.update()
 
+    @ddt.data(ValueError, TypeError)
+    @XBlock.register_temp_plugin(CourseBlock, 'course')
+    @XBlock.register_temp_plugin(InvalidModeBlock, 'html')
+    @XBlock.register_temp_plugin(HiddenBlock, 'hidden')
+    @XBlock.register_temp_plugin(OtherAggBlock, 'other')
+    def test_expected_updater_errors(self, exception_class):
+        # Verify that no exception is bubbled up when the constructor errors, but that the update method is not called.
+        with mock.patch.object(AggregationUpdater, '__init__') as mock_update_constructor:
+            mock_update_constructor.side_effect = exception_class('test')
+            with mock.patch.object(AggregationUpdater, 'update') as mock_update_action:
+                update_aggregators(username='saskia', course_key='course-v1:OpenCraft+Onboarding+2018')
+                assert not mock_update_action.called
+
+    @XBlock.register_temp_plugin(CourseBlock, 'course')
+    @XBlock.register_temp_plugin(InvalidModeBlock, 'html')
+    @XBlock.register_temp_plugin(HiddenBlock, 'hidden')
+    @XBlock.register_temp_plugin(OtherAggBlock, 'other')
+    def test_unexpected_updater_errors(self):
+        # Verify that no exception is bubbled up when the constructor errors, but that the update method is not called.
+        with mock.patch.object(AggregationUpdater, '__init__') as mock_update_constructor:
+            mock_update_constructor.side_effect = RuntimeError('test')
+            with pytest.raises(RuntimeError):
+                update_aggregators(username='saskia', course_key='course-v1:OpenCraft+Onboarding+2018')
+
 
 class PartialUpdateTest(TestCase):
     """

tox.ini

@@ -36,6 +36,7 @@ passenv =
     EDXAGG_MYSQL_PASSWORD
 
 [testenv:docs]
+basepython = python3.5
 setenv =
     DJANGO_SETTINGS_MODULE = test_settings
     PYTHONPATH = {toxinidir}
@@ -53,6 +54,7 @@ commands =
     python setup.py check --restructuredtext --strict
 
 [testenv:quality]
+basepython = python3.5
 whitelist_externals =
     make
     rm