diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 366d6d33d5..f123691393 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -11,7 +11,7 @@ from datetime import datetime import six.moves.urllib.parse from completion.exceptions import UnavailableCompletionData -from completion.utilities import get_key_to_last_completed_course_block +from completion.utilities import get_key_to_last_completed_block from django.conf import settings from django.contrib.auth import load_backend from django.contrib.auth.models import User @@ -686,7 +686,7 @@ def get_resume_urls_for_enrollments(user, enrollments): resume_course_urls = OrderedDict() for enrollment in enrollments: try: - block_key = get_key_to_last_completed_course_block(user, enrollment.course_id) + block_key = get_key_to_last_completed_block(user, enrollment.course_id) url_to_block = reverse( 'jump_to', kwargs={'course_id': enrollment.course_id, 'location': block_key} diff --git a/common/djangoapps/student/tests/test_views.py b/common/djangoapps/student/tests/test_views.py index fe18588917..3f956d2a7c 100644 --- a/common/djangoapps/student/tests/test_views.py +++ b/common/djangoapps/student/tests/test_views.py @@ -703,7 +703,7 @@ class StudentDashboardTests(SharedModuleStoreTestCase, MilestonesTestCaseMixin, for number in range(5) ] - submit_completions_for_testing(self.user, course_key, block_keys) + submit_completions_for_testing(self.user, block_keys) response = self.client.get(reverse('dashboard')) @@ -816,7 +816,7 @@ class StudentDashboardTests(SharedModuleStoreTestCase, MilestonesTestCaseMixin, ] last_completed_block_string = str(block_keys[-1]) - submit_completions_for_testing(self.user, course_key, block_keys) + submit_completions_for_testing(self.user, block_keys) html_for_view_buttons.append( self._get_html_for_view_course_button( diff --git a/lms/djangoapps/course_api/blocks/transformers/block_completion.py b/lms/djangoapps/course_api/blocks/transformers/block_completion.py index 0c4f5a730d..a2b0be3d2a 100644 --- a/lms/djangoapps/course_api/blocks/transformers/block_completion.py +++ b/lms/djangoapps/course_api/blocks/transformers/block_completion.py @@ -61,7 +61,7 @@ class BlockCompletionTransformer(BlockStructureTransformer): completions = BlockCompletion.objects.filter( user=usage_info.user, - course_key=usage_info.course_key, + context_key=usage_info.course_key, ).values_list( 'block_key', 'completion', diff --git a/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py b/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py index dafdbae862..ad5fd82d8e 100644 --- a/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py +++ b/lms/djangoapps/course_api/blocks/transformers/tests/test_block_completion.py @@ -82,7 +82,6 @@ class BlockCompletionTransformerTestCase(TransformerRegistryTestMixin, Completio block = ItemFactory.create(category='comp', parent=course) BlockCompletion.objects.submit_completion( user=self.user, - course_key=block.location.course_key, block_key=block.location, completion=self.COMPLETION_TEST_VALUE, ) diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 36e7036661..b6ce3b34a0 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -569,7 +569,6 @@ def get_module_system_for_user( else: BlockCompletion.objects.submit_completion( user=user, - course_key=course_id, block_key=block.scope_ids.usage_id, completion=event['completion'], ) @@ -614,7 +613,6 @@ def get_module_system_for_user( 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, ) diff --git a/lms/djangoapps/gating/signals.py b/lms/djangoapps/gating/signals.py index 0c782cf75a..00e17af2d4 100644 --- a/lms/djangoapps/gating/signals.py +++ b/lms/djangoapps/gating/signals.py @@ -37,7 +37,9 @@ def evaluate_subsection_completion_milestones(**kwargs): evaluation of any milestone which can be completed. """ instance = kwargs['instance'] - course_id = six.text_type(instance.course_key) + course_id = six.text_type(instance.context_key) + if not instance.context_key.is_course: + return # Content in a library or some other thing that doesn't support milestones block_id = six.text_type(instance.block_key) user_id = instance.user_id task_evaluate_subsection_completion_milestones(course_id, block_id, user_id) diff --git a/lms/djangoapps/lms_xblock/runtime.py b/lms/djangoapps/lms_xblock/runtime.py index 8b9d68e65c..3df3c6a899 100644 --- a/lms/djangoapps/lms_xblock/runtime.py +++ b/lms/djangoapps/lms_xblock/runtime.py @@ -142,7 +142,7 @@ class LmsModuleSystem(ModuleSystem): # pylint: disable=abstract-method services = kwargs.setdefault('services', {}) user = kwargs.get('user') if user and user.is_authenticated: - services['completion'] = CompletionService(user=user, course_key=kwargs.get('course_id')) + services['completion'] = CompletionService(user=user, context_key=kwargs.get('course_id')) services['fs'] = xblock.reference.plugins.FSService() services['i18n'] = ModuleI18nService services['library_tools'] = LibraryToolsService(store) diff --git a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py index 643bda1030..34872ec0ae 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_content_libraries.py @@ -11,6 +11,7 @@ from organizations.models import Organization from rest_framework.test import APITestCase from student.tests.factories import UserFactory +from openedx.core.djangolib.testing.utils import skip_unless_cms from openedx.core.lib import blockstore_api # Define the URLs here - don't use reverse() because we want to detect @@ -29,7 +30,7 @@ URL_BLOCK_GET_HANDLER_URL = '/api/xblock/v2/xblocks/{block_key}/handler_url/{han @unittest.skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") -@unittest.skipUnless(settings.ROOT_URLCONF == "cms.urls", "Content Libraries REST API is only available in Studio") +@skip_unless_cms # Content Libraries REST API is only available in Studio class ContentLibrariesTest(APITestCase): """ Test for Blockstore-based Content Libraries diff --git a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py index c863e7afa1..17dd2cb550 100644 --- a/openedx/core/djangoapps/content_libraries/tests/test_runtime.py +++ b/openedx/core/djangoapps/content_libraries/tests/test_runtime.py @@ -6,6 +6,7 @@ from __future__ import absolute_import, division, print_function, unicode_litera import json import unittest +from completion.test_utils import CompletionWaffleTestMixin from django.conf import settings from django.test import TestCase from organizations.models import Organization @@ -20,6 +21,7 @@ from openedx.core.djangoapps.content_libraries.tests.test_content_libraries impo URL_BLOCK_GET_HANDLER_URL, ) from openedx.core.djangoapps.xblock import api as xblock_api +from openedx.core.djangolib.testing.utils import skip_unless_lms from openedx.core.lib import blockstore_api from student.tests.factories import UserFactory from xmodule.unit_block import UnitBlock @@ -93,7 +95,7 @@ class ContentLibraryRuntimeTest(ContentLibraryContentTestMixin, TestCase): @unittest.skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") # We can remove the line below to enable this in Studio once we implement a session-backed # field data store which we can use for both studio users and anonymous users -@unittest.skipUnless(settings.ROOT_URLCONF == "lms.urls", "Student State is only saved in the LMS") +@skip_unless_lms class ContentLibraryXBlockUserStateTest(ContentLibraryContentTestMixin, TestCase): """ Test that the Blockstore-based XBlock runtime can store and retrieve student @@ -195,7 +197,7 @@ class ContentLibraryXBlockUserStateTest(ContentLibraryContentTestMixin, TestCase # Now they should be equal, because we've saved and re-loaded instance2: self.assertEqual(block_instance1.user_str, block_instance2.user_str) - @unittest.skipUnless(settings.ROOT_URLCONF == "lms.urls", "Scores are only used in the LMS") + @skip_unless_lms # Scores are only used in the LMS def test_scores_persisted(self): """ Test that a block's emitted scores are cached in StudentModule @@ -260,3 +262,58 @@ class ContentLibraryXBlockUserStateTest(ContentLibraryContentTestMixin, TestCase sm = get_score(self.student_a, block_id) self.assertEqual(sm.grade, 1) self.assertEqual(sm.max_grade, 1) + + +@unittest.skipUnless(settings.RUN_BLOCKSTORE_TESTS, "Requires a running Blockstore server") +@skip_unless_lms # No completion tracking in Studio +class ContentLibraryXBlockCompletionTest(ContentLibraryContentTestMixin, CompletionWaffleTestMixin, TestCase): + """ + Test that the Blockstore-based XBlocks can track their completion status + using the completion library. + """ + + def setUp(self): + super(ContentLibraryXBlockCompletionTest, self).setUp() + # Enable the completion waffle flag for these tests + self.override_waffle_switch(True) + + def test_mark_complete_via_handler(self): + """ + Test that a "complete on view" XBlock like the HTML block can be marked + as complete using the LmsBlockMixin.publish_completion handler. + """ + block_id = library_api.create_library_block(self.library.key, "html", "completable_html").usage_key + new_olx = """ + + This is some HTML.

+ ]]> + + """.strip() + library_api.set_library_block_olx(block_id, new_olx) + library_api.publish_changes(self.library.key) + + # We should get a REST API for retrieving completion data; for now use python + + def get_block_completion_status(): + """ Get block completion status (0 to 1) """ + block = xblock_api.load_block(block_id, self.student_a) + assert hasattr(block, 'publish_completion') + service = block.runtime.service(block, 'completion') + return service.get_completions([block_id])[block_id] + + # At first the block is not completed + self.assertEqual(get_block_completion_status(), 0) + + # Now call the 'publish_completion' handler: + client = APIClient() + client.login(username=self.student_a.username, password='edx') + result = client.get(URL_BLOCK_GET_HANDLER_URL.format(block_key=block_id, handler_name='publish_completion')) + publish_completion_url = result.data["handler_url"] + + # This will test the 'completion' service and the completion event handler: + result2 = client.post(publish_completion_url, {"completion": 1.0}, format='json') + self.assertEqual(result2.status_code, 200) + + # Now the block is completed + self.assertEqual(get_block_completion_status(), 1) diff --git a/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py b/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py index b492c015b1..e9ce9699d7 100644 --- a/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py +++ b/openedx/core/djangoapps/user_api/accounts/tests/test_utils.py @@ -106,7 +106,6 @@ class CompletionUtilsTestCase(SharedModuleStoreTestCase, CompletionWaffleTestMix for block in self.course.children[0].children[0].children: models.BlockCompletion.objects.submit_completion( user=self.engaged_user, - course_key=self.course.id, block_key=block.location, completion=1.0 ) diff --git a/openedx/core/djangoapps/xblock/rest_api/views.py b/openedx/core/djangoapps/xblock/rest_api/views.py index 425ee09394..055d070fe1 100644 --- a/openedx/core/djangoapps/xblock/rest_api/views.py +++ b/openedx/core/djangoapps/xblock/rest_api/views.py @@ -7,6 +7,8 @@ Studio APIs cover use cases like adding/deleting/editing blocks. from __future__ import absolute_import, division, print_function, unicode_literals from django.contrib.auth import get_user_model +from django.views.decorators.clickjacking import xframe_options_exempt +from django.views.decorators.csrf import csrf_exempt from rest_framework import permissions from rest_framework.decorators import api_view, permission_classes, authentication_classes from rest_framework.exceptions import PermissionDenied, AuthenticationFailed @@ -68,12 +70,19 @@ def get_handler_url(request, usage_key_str, handler_name): return Response({"handler_url": handler_url}) -@api_view(['GET', 'POST', 'PUT', 'PATCH', 'DELETE']) -@authentication_classes([]) # Disable session authentication; we don't need it and don't want CSRF checks -@permission_classes((permissions.AllowAny, )) +# We cannot use DRF for this endpoint because its Request object is incompatible +# with the API expected by XBlock handlers. +# See https://github.com/edx/edx-platform/pull/19253 +# and https://github.com/edx/XBlock/pull/383 for context. +@csrf_exempt +@xframe_options_exempt def xblock_handler(request, user_id, secure_token, usage_key_str, handler_name, suffix): """ Run an XBlock's handler and return the result + + This endpoint has a unique authentication scheme that involves a temporary + auth token included in the URL (see below). As a result it can be exempt + from CSRF, session auth, and JWT/OAuth. """ user_id = int(user_id) # User ID comes from the URL, not session auth usage_key = UsageKey.from_string(usage_key_str) diff --git a/openedx/core/djangoapps/xblock/runtime/mixin.py b/openedx/core/djangoapps/xblock/runtime/mixin.py index 3de50b4846..52086be9b4 100644 --- a/openedx/core/djangoapps/xblock/runtime/mixin.py +++ b/openedx/core/djangoapps/xblock/runtime/mixin.py @@ -2,9 +2,14 @@ A mixin that provides functionality and default attributes for all XBlocks in the new XBlock runtime. """ +from __future__ import absolute_import, division, print_function, unicode_literals + +from xblock.core import XBlock, XBlockMixin +from xblock.exceptions import JsonHandlerError -class LmsBlockMixin(object): +@XBlock.wants('completion') +class LmsBlockMixin(XBlockMixin): """ A mixin that provides functionality and default attributes for all XBlocks in the new XBlock runtime. @@ -17,3 +22,21 @@ class LmsBlockMixin(object): # static content). If it does, it should set this and provide scoring # functionality by inheriting xblock.scorable.ScorableXBlockMixin has_score = False + + @XBlock.json_handler + def publish_completion(self, data, suffix=''): # pylint: disable=unused-argument + """ + Allow the frontend app that's rendering this XBlock to mark it as + completed when the user views it, if appropriate. + + Copied from lms.djangoapps.lms_xblock.mixin.LmsBlockMixin + """ + completion_service = self.runtime.service(self, 'completion') + if completion_service is None: + raise JsonHandlerError(500, u"No completion service found") + elif not completion_service.completion_tracking_enabled(): + raise JsonHandlerError(404, u"Completion tracking is not enabled and API calls are unexpected") + if not completion_service.can_mark_block_complete_on_view(self): + raise JsonHandlerError(400, u"Block not configured for completion on view.") + self.runtime.publish(self, "completion", data) + return {'result': 'ok'} diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index e891e9698c..b0df80894f 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -5,6 +5,8 @@ from __future__ import absolute_import, division, print_function, unicode_litera import logging from completion import waffle as completion_waffle +from completion.models import BlockCompletion +from completion.services import CompletionService import crum from django.contrib.auth import get_user_model from django.utils.lru_cache import lru_cache @@ -131,8 +133,7 @@ class XBlockRuntime(RuntimeShim, Runtime): if event_type == 'grade': return self.handle_grade_event elif event_type == 'completion': - if completion_waffle.waffle().is_enabled(completion_waffle.ENABLE_COMPLETION_TRACKING): - return self.handle_completion_event + return self.handle_completion_event return None def log_event_to_tracking_log(self, block, event_type, event_data): @@ -167,16 +168,13 @@ class XBlockRuntime(RuntimeShim, Runtime): """ Submit a completion object for the block. """ - block_key = block.scope_ids.usage_id - # edx-completion needs to be updated to support learning contexts, which is coming soon in a separate PR. - # For now just log a debug statement to confirm this plumbing is ready to send those events through. - log.debug("Completion event for block {}: new completion = {}".format(block_key, event['completion'])) - # BlockCompletion.objects.submit_completion( - # user=self.user, - # course_key=block_key.context_key, - # block_key=block_key, - # completion=event['completion'], - # ) + if not completion_waffle.waffle().is_enabled(completion_waffle.ENABLE_COMPLETION_TRACKING): + return + BlockCompletion.objects.submit_completion( + user=self.user, + block_key=block.scope_ids.usage_id, + completion=event['completion'], + ) def applicable_aside_types(self, block): """ Disable XBlock asides in this runtime """ @@ -213,6 +211,9 @@ class XBlockRuntime(RuntimeShim, Runtime): self.block_field_datas[block.scope_ids] = None raise return self.block_field_datas[block.scope_ids] + elif service_name == "completion": + context_key = block.scope_ids.usage_id.context_key + return CompletionService(user=self.user, context_key=context_key) # Check if the XBlockRuntimeSystem wants to handle this: service = self.system.get_service(block, service_name) # Otherwise, fall back to the base implementation which loads services diff --git a/openedx/core/lib/gating/api.py b/openedx/core/lib/gating/api.py index 968ecc0246..85b5a522a7 100644 --- a/openedx/core/lib/gating/api.py +++ b/openedx/core/lib/gating/api.py @@ -477,7 +477,8 @@ def get_subsection_completion_percentage(subsection_usage_key, user): if not completable_blocks: return 100 subsection_completion_total = 0 - course_block_completions = BlockCompletion.get_course_completions(user, subsection_usage_key.course_key) + course_key = subsection_usage_key.course_key + course_block_completions = BlockCompletion.get_learning_context_completions(user, course_key) for block in completable_blocks: if course_block_completions.get(block): subsection_completion_total += course_block_completions.get(block) diff --git a/openedx/core/lib/gating/tests/test_api.py b/openedx/core/lib/gating/tests/test_api.py index d51f987b06..c7fbf7de00 100644 --- a/openedx/core/lib/gating/tests/test_api.py +++ b/openedx/core/lib/gating/tests/test_api.py @@ -266,7 +266,7 @@ class TestGatingApi(ModuleStoreTestCase, MilestonesTestCaseMixin): category='html', display_name='some html block' ) - with patch.object(BlockCompletion, 'get_course_completions') as course_block_completions_mock: + with patch.object(BlockCompletion, 'get_learning_context_completions') as course_block_completions_mock: course_block_completions_mock.return_value = { problem_block.location: user_problem_completion, html_block.location: user_html_completion, @@ -307,7 +307,7 @@ class TestGatingApi(ModuleStoreTestCase, MilestonesTestCaseMixin): display_name=u'{} block'.format(component_type) ) - with patch.object(BlockCompletion, 'get_course_completions') as course_block_completions_mock: + with patch.object(BlockCompletion, 'get_learning_context_completions') as course_block_completions_mock: course_block_completions_mock.return_value = { component.location: completed, } diff --git a/openedx/features/course_experience/tests/views/test_course_outline.py b/openedx/features/course_experience/tests/views/test_course_outline.py index 4b86e85ea0..66017ca6c3 100644 --- a/openedx/features/course_experience/tests/views/test_course_outline.py +++ b/openedx/features/course_experience/tests/views/test_course_outline.py @@ -364,10 +364,12 @@ class TestCourseOutlineResumeCourse(SharedModuleStoreTestCase, CompletionWaffleT course_key = CourseKey.from_string(str(course.id)) # Fake a visit to sequence2/vertical2 block_key = UsageKey.from_string(six.text_type(sequential.location)) + if block_key.course_key.run is None: + # Old mongo keys must be annotated with course run info before calling submit_completion: + block_key = block_key.replace(course_key=course_key) completion = 1.0 BlockCompletion.objects.submit_completion( user=self.user, - course_key=course_key, block_key=block_key, completion=completion ) diff --git a/openedx/features/course_experience/utils.py b/openedx/features/course_experience/utils.py index e9646bbf74..dd9bbdc01d 100644 --- a/openedx/features/course_experience/utils.py +++ b/openedx/features/course_experience/utils.py @@ -69,7 +69,7 @@ def get_course_outline_block_tree(request, course_id, user=None): if last_completed_child_position: # Mutex w/ NOT 'course_block_completions' recurse_mark_complete( - course_block_completions=BlockCompletion.get_course_completions(user, course_key), + course_block_completions=BlockCompletion.get_learning_context_completions(user, course_key), latest_completion=last_completed_child_position, block=block ) diff --git a/openedx/tests/completion_integration/test_handlers.py b/openedx/tests/completion_integration/test_handlers.py index 344d52eb3d..f6cf616831 100644 --- a/openedx/tests/completion_integration/test_handlers.py +++ b/openedx/tests/completion_integration/test_handlers.py @@ -49,7 +49,7 @@ class ScorableCompletionHandlerTestCase(CompletionSetUpMixin, TestCase): def setUp(self): super(ScorableCompletionHandlerTestCase, self).setUp() - self.block_key = self.course_key.make_usage_key(block_type='problem', block_id='red') + self.block_key = self.context_key.make_usage_key(block_type='problem', block_id='red') def call_scorable_block_completion_handler(self, block_key, score_deleted=None): """ @@ -64,7 +64,7 @@ class ScorableCompletionHandlerTestCase(CompletionSetUpMixin, TestCase): handlers.scorable_block_completion( sender=self, user_id=self.user.id, - course_id=six.text_type(self.course_key), + course_id=six.text_type(self.context_key), usage_id=six.text_type(block_key), weighted_earned=0.0, weighted_possible=3.0, @@ -83,39 +83,39 @@ class ScorableCompletionHandlerTestCase(CompletionSetUpMixin, TestCase): self.call_scorable_block_completion_handler(self.block_key, score_deleted) completion = BlockCompletion.objects.get( user=self.user, - course_key=self.course_key, + context_key=self.context_key, block_key=self.block_key, ) self.assertEqual(completion.completion, expected_completion) @XBlock.register_temp_plugin(CustomScorableBlock, 'custom_scorable') def test_handler_skips_custom_block(self): - custom_block_key = self.course_key.make_usage_key(block_type='custom_scorable', block_id='green') + custom_block_key = self.context_key.make_usage_key(block_type='custom_scorable', block_id='green') self.call_scorable_block_completion_handler(custom_block_key) completion = BlockCompletion.objects.filter( user=self.user, - course_key=self.course_key, + context_key=self.context_key, block_key=custom_block_key, ) self.assertFalse(completion.exists()) @XBlock.register_temp_plugin(ExcludedScorableBlock, 'excluded_scorable') def test_handler_skips_excluded_block(self): - excluded_block_key = self.course_key.make_usage_key(block_type='excluded_scorable', block_id='blue') + excluded_block_key = self.context_key.make_usage_key(block_type='excluded_scorable', block_id='blue') self.call_scorable_block_completion_handler(excluded_block_key) completion = BlockCompletion.objects.filter( user=self.user, - course_key=self.course_key, + context_key=self.context_key, block_key=excluded_block_key, ) self.assertFalse(completion.exists()) def test_handler_skips_discussion_block(self): - discussion_block_key = self.course_key.make_usage_key(block_type='discussion', block_id='blue') + discussion_block_key = self.context_key.make_usage_key(block_type='discussion', block_id='blue') self.call_scorable_block_completion_handler(discussion_block_key) completion = BlockCompletion.objects.filter( user=self.user, - course_key=self.course_key, + context_key=self.context_key, block_key=discussion_block_key, ) self.assertFalse(completion.exists()) @@ -125,7 +125,7 @@ class ScorableCompletionHandlerTestCase(CompletionSetUpMixin, TestCase): grades_signals.PROBLEM_WEIGHTED_SCORE_CHANGED.send_robust( sender=self, user_id=self.user.id, - course_id=six.text_type(self.course_key), + course_id=six.text_type(self.context_key), usage_id=six.text_type(self.block_key), weighted_earned=0.0, weighted_possible=3.0, @@ -145,13 +145,13 @@ class DisabledCompletionHandlerTestCase(CompletionSetUpMixin, TestCase): def setUp(self): super(DisabledCompletionHandlerTestCase, self).setUp() - self.block_key = self.course_key.make_usage_key(block_type='problem', block_id='red') + self.block_key = self.context_key.make_usage_key(block_type='problem', block_id='red') def test_disabled_handler_does_not_submit_completion(self): handlers.scorable_block_completion( sender=self, user_id=self.user.id, - course_id=six.text_type(self.course_key), + course_id=six.text_type(self.context_key), usage_id=six.text_type(self.block_key), weighted_earned=0.0, weighted_possible=3.0, @@ -161,6 +161,6 @@ class DisabledCompletionHandlerTestCase(CompletionSetUpMixin, TestCase): with self.assertRaises(BlockCompletion.DoesNotExist): BlockCompletion.objects.get( user=self.user, - course_key=self.course_key, + context_key=self.context_key, block_key=self.block_key ) diff --git a/openedx/tests/completion_integration/test_models.py b/openedx/tests/completion_integration/test_models.py index 0f6c997791..f5d725f855 100644 --- a/openedx/tests/completion_integration/test_models.py +++ b/openedx/tests/completion_integration/test_models.py @@ -44,7 +44,7 @@ class CompletionSetUpMixin(CompletionWaffleTestMixin): self.block_key = UsageKey.from_string(u'block-v1:edx+test+run+type@video+block@doggos') self.completion = models.BlockCompletion.objects.create( user=self.user, - course_key=self.block_key.course_key, + context_key=self.block_key.context_key, block_type=self.block_key.block_type, block_key=self.block_key, completion=0.5, @@ -67,7 +67,6 @@ class SubmitCompletionTestCase(CompletionSetUpMixin, TestCase): # OTHER = user exists, completion exists completion, isnew = models.BlockCompletion.objects.submit_completion( user=self.user, - course_key=self.block_key.course_key, block_key=self.block_key, completion=0.9, ) @@ -80,7 +79,6 @@ class SubmitCompletionTestCase(CompletionSetUpMixin, TestCase): with self.assertNumQueries(SELECT + 2 * SAVEPOINT): completion, isnew = models.BlockCompletion.objects.submit_completion( user=self.user, - course_key=self.block_key.course_key, block_key=self.block_key, completion=0.5, ) @@ -94,7 +92,6 @@ class SubmitCompletionTestCase(CompletionSetUpMixin, TestCase): with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT): _, isnew = models.BlockCompletion.objects.submit_completion( user=newuser, - course_key=self.block_key.course_key, block_key=self.block_key, completion=0.0, ) @@ -106,7 +103,6 @@ class SubmitCompletionTestCase(CompletionSetUpMixin, TestCase): with self.assertNumQueries(SELECT + UPDATE + 4 * SAVEPOINT): _, isnew = models.BlockCompletion.objects.submit_completion( user=self.user, - course_key=newblock.course_key, block_key=newblock, completion=1.0, ) @@ -117,7 +113,6 @@ class SubmitCompletionTestCase(CompletionSetUpMixin, TestCase): with self.assertRaises(ValidationError): models.BlockCompletion.objects.submit_completion( user=self.user, - course_key=self.block_key.course_key, block_key=self.block_key, completion=1.2 ) @@ -143,7 +138,6 @@ class CompletionDisabledTestCase(CompletionSetUpMixin, TestCase): with self.assertRaises(RuntimeError): models.BlockCompletion.objects.submit_completion( user=self.user, - course_key=self.block_key.course_key, block_key=self.block_key, completion=0.9, ) @@ -167,7 +161,7 @@ class SubmitBatchCompletionTestCase(CompletionWaffleTestMixin, TestCase): def test_submit_batch_completion(self): blocks = [(self.block_key, 1.0)] - models.BlockCompletion.objects.submit_batch_completion(self.user, self.course_key_obj, blocks) + models.BlockCompletion.objects.submit_batch_completion(self.user, blocks) self.assertEqual(models.BlockCompletion.objects.count(), 1) self.assertEqual(models.BlockCompletion.objects.last().completion, 1.0) @@ -175,19 +169,19 @@ class SubmitBatchCompletionTestCase(CompletionWaffleTestMixin, TestCase): with waffle.waffle().override(waffle.ENABLE_COMPLETION_TRACKING, False): with self.assertRaises(RuntimeError): blocks = [(self.block_key, 1.0)] - models.BlockCompletion.objects.submit_batch_completion(self.user, self.course_key_obj, blocks) + models.BlockCompletion.objects.submit_batch_completion(self.user, blocks) def test_submit_batch_completion_with_same_block_new_completion_value(self): blocks = [(self.block_key, 0.0)] self.assertEqual(models.BlockCompletion.objects.count(), 0) - models.BlockCompletion.objects.submit_batch_completion(self.user, self.course_key_obj, blocks) + models.BlockCompletion.objects.submit_batch_completion(self.user, blocks) self.assertEqual(models.BlockCompletion.objects.count(), 1) model = models.BlockCompletion.objects.first() self.assertEqual(model.completion, 0.0) blocks = [ (UsageKey.from_string('block-v1:edx+test+run+type@video+block@doggos'), 1.0), ] - models.BlockCompletion.objects.submit_batch_completion(self.user, self.course_key_obj, blocks) + models.BlockCompletion.objects.submit_batch_completion(self.user, blocks) self.assertEqual(models.BlockCompletion.objects.count(), 1) model = models.BlockCompletion.objects.first() self.assertEqual(model.completion, 1.0) @@ -207,20 +201,25 @@ class BatchCompletionMethodTests(CompletionWaffleTestMixin, TestCase): self.course_key = CourseKey.from_string("edX/MOOC101/2049_T2") self.other_course_key = CourseKey.from_string("course-v1:ReedX+Hum110+1904") self.block_keys = [UsageKey.from_string("i4x://edX/MOOC101/video/{}".format(number)) for number in range(5)] + self.block_keys_with_runs = [key.replace(course_key=self.course_key) for key in self.block_keys] + self.other_course_block_keys = [self.other_course_key.make_usage_key('html', '1')] - submit_completions_for_testing(self.user, self.course_key, self.block_keys[:3]) - submit_completions_for_testing(self.other_user, self.course_key, self.block_keys[2:]) - submit_completions_for_testing(self.user, self.other_course_key, [self.block_keys[4]]) + # Submit completions for the main course: + submit_completions_for_testing(self.user, self.block_keys_with_runs[:3]) + # Different user: + submit_completions_for_testing(self.other_user, self.block_keys_with_runs[2:]) + # Different course: + submit_completions_for_testing(self.user, self.other_course_block_keys) - def test_get_course_completions_missing_runs(self): - actual_completions = models.BlockCompletion.get_course_completions(self.user, self.course_key) - expected_block_keys = [key.replace(course_key=self.course_key) for key in self.block_keys[:3]] + def test_get_learning_context_completions_missing_runs(self): + actual_completions = models.BlockCompletion.get_learning_context_completions(self.user, self.course_key) + expected_block_keys = self.block_keys_with_runs[:3] expected_completions = dict(list(zip(expected_block_keys, [1.0, 0.8, 0.6]))) self.assertEqual(expected_completions, actual_completions) - def test_get_course_completions_empty_result_set(self): + def test_get_learning_context_completions_empty_result_set(self): self.assertEqual( - models.BlockCompletion.get_course_completions(self.other_user, self.other_course_key), + models.BlockCompletion.get_learning_context_completions(self.other_user, self.other_course_key), {} ) diff --git a/openedx/tests/completion_integration/test_services.py b/openedx/tests/completion_integration/test_services.py index 7bc096d629..166850ae93 100644 --- a/openedx/tests/completion_integration/test_services.py +++ b/openedx/tests/completion_integration/test_services.py @@ -79,7 +79,6 @@ class CompletionServiceTestCase(CompletionWaffleTestMixin, SharedModuleStoreTest # Proper completions for the given runtime BlockCompletion.objects.submit_completion( user=self.user, - course_key=self.course_key, block_key=self.html.location, completion=1.0, ) @@ -87,7 +86,6 @@ class CompletionServiceTestCase(CompletionWaffleTestMixin, SharedModuleStoreTest for idx, block_key in enumerate(self.block_keys[0:3]): BlockCompletion.objects.submit_completion( user=self.user, - course_key=self.course_key, block_key=block_key, completion=1.0 - (0.2 * idx), ) @@ -96,7 +94,6 @@ class CompletionServiceTestCase(CompletionWaffleTestMixin, SharedModuleStoreTest for idx, block_key in enumerate(self.block_keys[2:]): BlockCompletion.objects.submit_completion( user=self.other_user, - course_key=self.course_key, block_key=block_key, completion=0.9 - (0.2 * idx), ) @@ -104,8 +101,7 @@ class CompletionServiceTestCase(CompletionWaffleTestMixin, SharedModuleStoreTest # Wrong course BlockCompletion.objects.submit_completion( user=self.user, - course_key=self.other_course_key, - block_key=self.block_keys[4], + block_key=self.other_course_key.make_usage_key('problem', 'other'), completion=0.75, ) @@ -137,7 +133,6 @@ class CompletionServiceTestCase(CompletionWaffleTestMixin, SharedModuleStoreTest for block_key in self.block_keys: BlockCompletion.objects.submit_completion( user=self.user, - course_key=self.course_key, block_key=block_key, completion=1.0 ) @@ -153,7 +148,6 @@ class CompletionServiceTestCase(CompletionWaffleTestMixin, SharedModuleStoreTest # Mark all the child blocks completed except the last one BlockCompletion.objects.submit_completion( user=self.user, - course_key=self.course_key, block_key=self.block_keys[i], completion=1.0 ) diff --git a/openedx/tests/completion_integration/test_views.py b/openedx/tests/completion_integration/test_views.py index 2a9c13c75c..dd9076466f 100644 --- a/openedx/tests/completion_integration/test_views.py +++ b/openedx/tests/completion_integration/test_views.py @@ -9,9 +9,11 @@ from completion import waffle from completion.test_utils import CompletionWaffleTestMixin from django.urls import reverse from rest_framework.test import APIClient +import six from openedx.core.djangolib.testing.utils import skip_unless_lms from student.tests.factories import CourseEnrollmentFactory, UserFactory +from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory @@ -25,8 +27,11 @@ class CompletionBatchTestCase(CompletionWaffleTestMixin, ModuleStoreTestCase): """ ENROLLED_USERNAME = 'test_user' UNENROLLED_USERNAME = 'unenrolled_user' - COURSE_KEY = 'TestX/101/Test' - BLOCK_KEY = 'i4x://TestX/101/problem/Test_Problem' + COURSE_KEY = 'course-v1:TestX+101+Test' + BLOCK_KEY = 'block-v1:TestX+101+Test+type@problem+block@Test_Problem' + # And for old mongo: + COURSE_KEY_DEPRECATED = 'TestX/201/Test' + BLOCK_KEY_DEPRECATED = 'i4x://TestX/201/problem/Test_Problem' def setUp(self): """ @@ -39,12 +44,25 @@ class CompletionBatchTestCase(CompletionWaffleTestMixin, ModuleStoreTestCase): self.override_waffle_switch(True) # Create course - self.course = CourseFactory.create(org='TestX', number='101', display_name='Test') - self.problem = ItemFactory.create( - parent=self.course, - category="problem", - display_name="Test Problem", + self.course = CourseFactory.create( + org='TestX', number='101', display_name='Test', + default_store=ModuleStoreEnum.Type.split, ) + self.assertEqual(six.text_type(self.course.id), self.COURSE_KEY) + self.problem = ItemFactory.create( + parent=self.course, category="problem", display_name="Test Problem", publish_item=False, + ) + self.assertEqual(six.text_type(self.problem.location), self.BLOCK_KEY) + # And an old mongo course: + self.course_deprecated = CourseFactory.create( + org='TestX', number='201', display_name='Test', + default_store=ModuleStoreEnum.Type.mongo, + ) + self.assertEqual(six.text_type(self.course_deprecated.id), self.COURSE_KEY_DEPRECATED) + self.problem_deprecated = ItemFactory.create( + parent=self.course_deprecated, category="problem", display_name="Test Problem", + ) + self.assertEqual(six.text_type(self.problem_deprecated.location), self.BLOCK_KEY_DEPRECATED) # Create users self.staff_user = UserFactory(is_staff=True) @@ -53,6 +71,7 @@ class CompletionBatchTestCase(CompletionWaffleTestMixin, ModuleStoreTestCase): # Enrol one user in the course CourseEnrollmentFactory.create(user=self.enrolled_user, course_id=self.course.id) + CourseEnrollmentFactory.create(user=self.enrolled_user, course_id=self.course_deprecated.id) # Login the enrolled user by for all tests self.client = APIClient() @@ -81,6 +100,16 @@ class CompletionBatchTestCase(CompletionWaffleTestMixin, ModuleStoreTestCase): } }, 200, {'detail': 'ok'} ), + # Valid submission (old mongo) + ( + { + 'username': ENROLLED_USERNAME, + 'course_key': COURSE_KEY_DEPRECATED, + 'blocks': { + BLOCK_KEY_DEPRECATED: 1.0, + } + }, 200, {'detail': 'ok'} + ), # Blocks list can be empty, though it's a no-op ( { @@ -97,7 +126,7 @@ class CompletionBatchTestCase(CompletionWaffleTestMixin, ModuleStoreTestCase): 'blocks': { BLOCK_KEY: 1.0, } - }, 400, {"detail": "Invalid course key: not:a:course:key"} + }, 400, {"detail": "Invalid learning context key: not:a:course:key"} ), # Block must be a valid key ( @@ -115,13 +144,14 @@ class CompletionBatchTestCase(CompletionWaffleTestMixin, ModuleStoreTestCase): 'username': ENROLLED_USERNAME, 'course_key': COURSE_KEY, 'blocks': { - 'i4x://some/other_course/problem/Test_Problem': 1.0, + 'block-v1:TestX+101+OtherCourse+type@problem+block@other': 1.0, } }, 400, { - "detail": u"Block with key: 'i4x://some/other_course/problem/Test_Problem' is not in course {}".format( - COURSE_KEY, + "detail": ( + u"Block with key: 'block-v1:TestX+101+OtherCourse+type@problem+block@other' " + u"is not in context {}".format(COURSE_KEY) ) } ), @@ -164,7 +194,7 @@ class CompletionBatchTestCase(CompletionWaffleTestMixin, ModuleStoreTestCase): ( { 'username': ENROLLED_USERNAME, - 'course_key': 'TestX/101/Test2', + 'course_key': 'course-v1:TestX+101+Test2', 'blocks': { BLOCK_KEY: 1.0, } @@ -191,6 +221,16 @@ class CompletionBatchTestCase(CompletionWaffleTestMixin, ModuleStoreTestCase): } }, 200, {'detail': 'ok'} ), + # Staff can submit completion on behalf of other users (old mongo) + ( + { + 'username': ENROLLED_USERNAME, + 'course_key': COURSE_KEY_DEPRECATED, + 'blocks': { + BLOCK_KEY_DEPRECATED: 1.0, + } + }, 200, {'detail': 'ok'} + ), # User must be enrolled in the course ( { diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 0ee84b72b8..8069d1f102 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -99,7 +99,7 @@ edx-analytics-data-api-client==0.15.3 edx-bulk-grades==0.6.4 edx-ccx-keys==1.0.0 edx-celeryutils==0.3.0 -edx-completion==2.0.0 +edx-completion==3.0.1 edx-django-oauth2-provider==1.3.5 edx-django-release-util==0.3.1 edx-django-sites-extensions==2.3.1 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index cdbd42256a..6e63f996d6 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -122,7 +122,7 @@ edx-analytics-data-api-client==0.15.3 edx-bulk-grades==0.6.4 edx-ccx-keys==1.0.0 edx-celeryutils==0.3.0 -edx-completion==2.0.0 +edx-completion==3.0.1 edx-django-oauth2-provider==1.3.5 edx-django-release-util==0.3.1 edx-django-sites-extensions==2.3.1 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 3c2aff7cca..5e20075c96 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -118,7 +118,7 @@ edx-analytics-data-api-client==0.15.3 edx-bulk-grades==0.6.4 edx-ccx-keys==1.0.0 edx-celeryutils==0.3.0 -edx-completion==2.0.0 +edx-completion==3.0.1 edx-django-oauth2-provider==1.3.5 edx-django-release-util==0.3.1 edx-django-sites-extensions==2.3.1