diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index bce67c76ff..ec77e1aa80 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -554,12 +554,18 @@ def get_module_system_for_user( if requested_user_id != user.id: log.warning("{} tried to submit a completion on behalf of {}".format(user, requested_user_id)) return - BlockCompletion.objects.submit_completion( - user=user, - course_key=course_id, - block_key=block.scope_ids.usage_id, - completion=1.0, - ) + + # If blocks explicitly declare support for the new completion API, + # we expect them to emit 'completion' events, + # and we ignore the deprecated 'progress' events + # in order to avoid duplicate work and possibly conflicting semantics. + if not getattr(block, 'has_custom_completion', False): + BlockCompletion.objects.submit_completion( + user=user, + course_key=course_id, + block_key=block.scope_ids.usage_id, + completion=1.0, + ) def rebind_noauth_module_to_user(module, real_user): """ diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index d61240d9fb..d4f4798513 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -26,6 +26,7 @@ from mock import MagicMock, Mock, patch from nose.plugins.attrib import attr from opaque_keys.edx.keys import CourseKey, UsageKey from pyquery import PyQuery +from xblock.completable import CompletableXBlockMixin from xblock.core import XBlock, XBlockAside from xblock.field_data import FieldData from xblock.fields import ScopeIds @@ -117,7 +118,7 @@ class GradedStatelessXBlock(XBlock): ) -class StubCompletableXBlock(XBlock): +class StubCompletableXBlock(CompletableXBlockMixin): """ This XBlock exists to test completion storage. """ @@ -143,6 +144,22 @@ class StubCompletableXBlock(XBlock): return self.runtime.publish(self, 'progress', {}) +class XBlockWithoutCompletionAPI(XBlock): + """ + This XBlock exists to test completion storage for xblocks + that don't support completion API but do emit progress signal. + """ + + @XBlock.json_handler + def progress(self, json_data, suffix): # pylint: disable=unused-argument + """ + Mark the block as complete using the deprecated progress interface. + + New code should use the completion event instead. + """ + return self.runtime.publish(self, 'progress', {}) + + @attr(shard=1) @ddt.ddt class ModuleRenderTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase): @@ -472,6 +489,26 @@ class TestHandleXBlockCallback(SharedModuleStoreTestCase, LoginEnrollmentTestCas mock_file.name = name return mock_file + def make_xblock_callback_response(self, request_data, course, block, handler): + """ + Prepares an xblock callback request and returns response to it. + """ + request = self.request_factory.post( + '/', + data=json.dumps(request_data), + content_type='application/json', + ) + request.user = self.mock_user + response = render.handle_xblock_callback( + request, + unicode(course.id), + quote_slashes(unicode(block.scope_ids.usage_id)), + handler, + '', + ) + + return response + def test_invalid_location(self): request = self.request_factory.post('dummy_url', data={'position': 1}) request.user = self.mock_user @@ -637,32 +674,46 @@ class TestHandleXBlockCallback(SharedModuleStoreTestCase, LoginEnrollmentTestCas mock_complete.assert_not_called() self.assertFalse(BlockCompletion.objects.filter(block_key=block.scope_ids.usage_id).exists()) - @ddt.data( - ('complete', {'completion': 0.625}, 0.625), - ('progress', {}, 1.0), - ) - @ddt.unpack @XBlock.register_temp_plugin(StubCompletableXBlock, identifier='comp') - def test_completion_events(self, signal, data, expected_completion): + def test_completion_signal_for_completable_xblock(self): with completion_waffle.waffle().override(completion_waffle.ENABLE_COMPLETION_TRACKING, True): course = CourseFactory.create() block = ItemFactory.create(category='comp', parent=course) - request = self.request_factory.post( - '/', - data=json.dumps(data), - content_type='application/json', - ) - request.user = self.mock_user - response = render.handle_xblock_callback( - request, - unicode(course.id), - quote_slashes(unicode(block.scope_ids.usage_id)), - signal, - '', + + response = self.make_xblock_callback_response( + {'completion': 0.625}, course, block, 'complete' ) + self.assertEqual(response.status_code, 200) completion = BlockCompletion.objects.get(block_key=block.scope_ids.usage_id) - self.assertEqual(completion.completion, expected_completion) + self.assertEqual(completion.completion, 0.625) + + @XBlock.register_temp_plugin(StubCompletableXBlock, identifier='comp') + def test_progress_signal_ignored_for_completable_xblock(self): + with completion_waffle.waffle().override(completion_waffle.ENABLE_COMPLETION_TRACKING, True): + course = CourseFactory.create() + block = ItemFactory.create(category='comp', parent=course) + + response = self.make_xblock_callback_response( + {}, course, block, 'progress' + ) + + self.assertEqual(response.status_code, 200) + self.assertFalse(BlockCompletion.objects.filter(block_key=block.scope_ids.usage_id).exists()) + + @XBlock.register_temp_plugin(XBlockWithoutCompletionAPI, identifier='no_comp') + def test_progress_signal_processed_for_xblock_without_completion_api(self): + with completion_waffle.waffle().override(completion_waffle.ENABLE_COMPLETION_TRACKING, True): + course = CourseFactory.create() + block = ItemFactory.create(category='no_comp', parent=course) + + response = self.make_xblock_callback_response( + {}, course, block, 'progress' + ) + + self.assertEqual(response.status_code, 200) + completion = BlockCompletion.objects.get(block_key=block.scope_ids.usage_id) + self.assertEqual(completion.completion, 1.0) @XBlock.register_temp_plugin(StubCompletableXBlock, identifier='comp') def test_skip_handlers_for_masquerading_staff(self):