From f419d6b19487d7d580d57ce2cf7fd2b91451b4fe Mon Sep 17 00:00:00 2001 From: Piotr Surowiec Date: Tue, 15 Nov 2022 16:46:24 +0100 Subject: [PATCH] feat: deprecate `track_function` and `publish` in ModuleSystem [BD-13] (#30046) * feat: delete `track_function` from ModuleSystem * feat: delete `publish` argument from ModuleSystem --- cms/djangoapps/contentstore/views/preview.py | 2 - lms/djangoapps/courseware/module_render.py | 105 +----------------- .../courseware/tests/test_module_render.py | 11 +- .../lms_xblock/test/test_runtime.py | 1 - .../core/djangoapps/xblock/runtime/runtime.py | 4 +- .../core/djangoapps/xblock/runtime/shims.py | 13 --- .../xblock_integration/xblock_testcase.py | 25 +++-- xmodule/capa/responsetypes.py | 5 +- xmodule/capa/tests/helpers.py | 4 +- xmodule/capa/tests/test_hint_functionality.py | 28 +++-- xmodule/capa_module.py | 34 +++--- xmodule/services.py | 100 ++++++++++++++++- xmodule/split_test_module.py | 3 +- xmodule/tests/__init__.py | 1 - xmodule/tests/test_capa_module.py | 40 +++---- xmodule/x_module.py | 19 +--- 16 files changed, 189 insertions(+), 206 deletions(-) diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 4100c2dd47..a7b9942ae5 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -207,8 +207,6 @@ def _preview_module_system(request, descriptor, field_data): preview_anonymous_user_id = anonymous_id_for_user(request.user, course_id) return PreviewModuleSystem( - # TODO (cpennington): Do we want to track how instructors are using the preview problems? - track_function=lambda event_type, event: None, get_module=partial(_load_preview_module, request), mixins=settings.XBLOCK_MIXINS, diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 04015787c6..68eb0bbe43 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -10,8 +10,6 @@ from collections import OrderedDict from functools import partial -from completion.waffle import ENABLE_COMPLETION_TRACKING_SWITCH -from completion.models import BlockCompletion from completion.services import CompletionService from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user @@ -52,10 +50,9 @@ from xmodule.modulestore.django import ModuleI18nService, modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.partitions.partitions_service import PartitionService from xmodule.util.sandboxing import SandboxService -from xmodule.services import RebindUserService, SettingsService, TeamsConfigurationService +from xmodule.services import EventPublishingService, RebindUserService, SettingsService, TeamsConfigurationService from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.static_replace.wrapper import replace_urls_wrapper -from common.djangoapps.xblock_django.constants import ATTR_KEY_USER_ID from xmodule.capa.xqueue_interface import XQueueService # lint-amnesty, pylint: disable=wrong-import-order from lms.djangoapps.courseware.access import get_user_role, has_access from lms.djangoapps.courseware.entrance_exams import user_can_skip_entrance_exam, user_has_passed_entrance_exam @@ -69,7 +66,6 @@ from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataC from lms.djangoapps.courseware.field_overrides import OverrideFieldData from lms.djangoapps.courseware.services import UserStateService from lms.djangoapps.grades.api import GradesUtilService -from lms.djangoapps.grades.api import signals as grades_signals from lms.djangoapps.lms_xblock.field_data import LmsFieldData from lms.djangoapps.lms_xblock.runtime import LmsModuleSystem, UserTagsService from lms.djangoapps.verify_student.services import XBlockVerificationService @@ -96,7 +92,6 @@ from openedx.features.discounts.utils import offer_banner_wrapper from openedx.features.content_type_gating.services import ContentTypeGatingService from common.djangoapps.student.models import anonymous_id_for_user from common.djangoapps.student.roles import CourseBetaTesterRole -from common.djangoapps.track import contexts from common.djangoapps.util import milestones_helpers from common.djangoapps.util.json_request import JsonResponse from common.djangoapps.edxmako.services import MakoService @@ -500,22 +495,6 @@ def get_module_system_for_user( will_recheck_access=will_recheck_access, ) - def get_event_handler(event_type): - """ - Return an appropriate function to handle the event. - - Returns None if no special processing is required. - """ - handlers = { - 'grade': handle_grade_event, - } - if ENABLE_COMPLETION_TRACKING_SWITCH.is_enabled(): - handlers.update({ - 'completion': handle_completion_event, - 'progress': handle_deprecated_progress_event, - }) - return handlers.get(event_type) - # These modules store data using the anonymous_student_id as a key. # To prevent loss of data, we will continue to provide old modules with # the per-student anonymized id (as we have in the past), @@ -536,85 +515,6 @@ def get_module_system_for_user( request_country_code=user_location, ) - def publish(block, event_type, event): - """ - A function that allows XModules to publish events. - """ - handle_event = get_event_handler(event_type) - if handle_event and not is_masquerading_as_specific_student(user, course_id): - handle_event(block, event) - else: - context = contexts.course_context_from_course_id(course_id) - user_id = user_service.get_current_user().opt_attrs.get(ATTR_KEY_USER_ID) - if user_id: - context['user_id'] = user_id - - context['asides'] = {} - for aside in block.runtime.get_asides(block): - if hasattr(aside, 'get_event_context'): - aside_event_info = aside.get_event_context(event_type, event) - if aside_event_info is not None: - context['asides'][aside.scope_ids.block_type] = aside_event_info - with tracker.get_tracker().context(event_type, context): - track_function(event_type, event) - - def handle_completion_event(block, event): - """ - Submit a completion object for the block. - """ - if not ENABLE_COMPLETION_TRACKING_SWITCH.is_enabled(): # lint-amnesty, pylint: disable=no-else-raise - raise Http404 - else: - BlockCompletion.objects.submit_completion( - user=user, - block_key=block.scope_ids.usage_id, - completion=event['completion'], - ) - - def handle_grade_event(block, event): - """ - Submit a grade for the block. - """ - if not user.is_anonymous: - grades_signals.SCORE_PUBLISHED.send( - sender=None, - block=block, - user=user, - raw_earned=event['value'], - raw_possible=event['max_value'], - only_if_higher=event.get('only_if_higher'), - score_deleted=event.get('score_deleted'), - grader_response=event.get('grader_response') - ) - - def handle_deprecated_progress_event(block, event): - """ - DEPRECATED: Submit a completion for the block represented by the - progress event. - - This exists to support the legacy progress extension used by - edx-solutions. New XBlocks should not emit these events, but instead - emit completion events directly. - """ - if not ENABLE_COMPLETION_TRACKING_SWITCH.is_enabled(): # lint-amnesty, pylint: disable=no-else-raise - raise Http404 - else: - requested_user_id = event.get('user_id', user.id) - if requested_user_id != user.id: - log.warning(f"{user} tried to submit a completion on behalf of {requested_user_id}") - return - - # 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, - block_key=block.scope_ids.usage_id, - completion=1.0, - ) - # Rebind module service to deal with noauth modules getting attached to users rebind_user_service = RebindUserService( user, @@ -689,9 +589,7 @@ def get_module_system_for_user( store = modulestore() system = LmsModuleSystem( - track_function=track_function, get_module=inner_get_module, - publish=publish, # TODO: When we merge the descriptor and module systems, we can stop reaching into the mixologist (cpennington) mixins=descriptor.runtime.mixologist._mixins, # pylint: disable=protected-access wrappers=block_wrappers, @@ -726,6 +624,7 @@ def get_module_system_for_user( 'teams': TeamsService(), 'teams_configuration': TeamsConfigurationService(), 'call_to_action': CallToActionService(), + 'publish': EventPublishingService(user, course_id, track_function), }, descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access request_token=request_token, diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 75142df645..a5eafc483c 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -885,7 +885,7 @@ class TestHandleXBlockCallback(SharedModuleStoreTestCase, LoginEnrollmentTestCas request.session = {} request.user.real_user = GlobalStaffFactory.create() request.user.real_user.masquerade_settings = CourseMasquerade(course.id, user_name="jem") - with patch('lms.djangoapps.courseware.module_render.is_masquerading_as_specific_student') as mock_masq: + with patch('xmodule.services.is_masquerading_as_specific_student') as mock_masq: mock_masq.return_value = True response = render.handle_xblock_callback( request, @@ -900,7 +900,7 @@ class TestHandleXBlockCallback(SharedModuleStoreTestCase, LoginEnrollmentTestCas BlockCompletion.objects.get(block_key=block.scope_ids.usage_id) @XBlock.register_temp_plugin(GradedStatelessXBlock, identifier='stateless_scorer') - @patch('lms.djangoapps.courseware.module_render.grades_signals.SCORE_PUBLISHED.send') + @patch('xmodule.services.grades_signals.SCORE_PUBLISHED.send') def test_anonymous_user_not_be_graded(self, mock_score_signal): course = CourseFactory.create() descriptor_kwargs = { @@ -2021,7 +2021,10 @@ class TestModuleTrackingContext(SharedModuleStoreTestCase): descriptor_kwargs['display_name'] = problem_display_name descriptor = ItemFactory.create(**descriptor_kwargs) - with patch('lms.djangoapps.courseware.module_render.tracker') as mock_tracker_for_context: + mock_tracker_for_context = MagicMock() + with patch('lms.djangoapps.courseware.module_render.tracker', mock_tracker_for_context), patch( + 'xmodule.services.tracker', mock_tracker_for_context + ): render.handle_xblock_callback( self.request, str(self.course.id), @@ -2031,12 +2034,10 @@ class TestModuleTrackingContext(SharedModuleStoreTestCase): ) assert len(mock_tracker.emit.mock_calls) == 1 - # lint-amnesty, pylint: disable=deprecated-method mock_call = mock_tracker.emit.mock_calls[0] event = mock_call[2] assert event['name'] == 'problem_check' - # lint-amnesty, pylint: disable=deprecated-method # for different operations, there are different number of context calls. # We are sending this `call_idx` to get the mock call that we are interested in. diff --git a/lms/djangoapps/lms_xblock/test/test_runtime.py b/lms/djangoapps/lms_xblock/test/test_runtime.py index 08a7ff47a0..eaca83dacb 100644 --- a/lms/djangoapps/lms_xblock/test/test_runtime.py +++ b/lms/djangoapps/lms_xblock/test/test_runtime.py @@ -51,7 +51,6 @@ class TestHandlerUrl(TestCase): super().setUp() self.block = BlockMock(name='block') self.runtime = LmsModuleSystem( - track_function=Mock(), get_module=Mock(), descriptor_runtime=Mock(), ) diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index d3f98e2cb9..3f0fc79d1b 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -23,7 +23,7 @@ from xblock.runtime import KvsFieldData, MemoryIdManager, Runtime from xmodule.errortracker import make_error_tracker from xmodule.contentstore.django import contentstore from xmodule.modulestore.django import ModuleI18nService -from xmodule.services import RebindUserService +from xmodule.services import EventPublishingService, RebindUserService from xmodule.util.sandboxing import SandboxService from common.djangoapps.edxmako.services import MakoService from common.djangoapps.static_replace.services import ReplaceURLService @@ -266,6 +266,8 @@ class XBlockRuntime(RuntimeShim, Runtime): track_function=make_track_function(), request_token=request_token(crum.get_current_request()), ) + elif service_name == 'publish': + return EventPublishingService(self.user, context_key, make_track_function()) # Check if the XBlockRuntimeSystem wants to handle this: service = self.system.get_service(block, service_name) diff --git a/openedx/core/djangoapps/xblock/runtime/shims.py b/openedx/core/djangoapps/xblock/runtime/shims.py index 6b8391033d..5f9909c95d 100644 --- a/openedx/core/djangoapps/xblock/runtime/shims.py +++ b/openedx/core/djangoapps/xblock/runtime/shims.py @@ -297,19 +297,6 @@ class RuntimeShim: result['default_value'] = field.to_json(field.default) return result - def track_function(self, title, event_info): - """ - Publish an event to the tracking log. - - This is deprecated in favor of runtime.publish - See https://git.io/JeGLf and https://git.io/JeGLY for context. - """ - warnings.warn( - "runtime.track_function is deprecated. Use runtime.publish() instead.", - DeprecationWarning, stacklevel=2, - ) - self.publish(self._active_block, title, event_info) - @property def user_location(self): """ diff --git a/openedx/tests/xblock_integration/xblock_testcase.py b/openedx/tests/xblock_integration/xblock_testcase.py index a35d170eb2..eba5d3b90c 100644 --- a/openedx/tests/xblock_integration/xblock_testcase.py +++ b/openedx/tests/xblock_integration/xblock_testcase.py @@ -49,10 +49,11 @@ from bs4 import BeautifulSoup from django.conf import settings from django.urls import reverse from xblock.plugin import Plugin + +import xmodule.services from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory -import lms.djangoapps.lms_xblock.runtime from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase @@ -100,26 +101,26 @@ class XBlockEventTestMixin: """ super().setUp() - saved_init = lms.djangoapps.lms_xblock.runtime.LmsModuleSystem.__init__ + saved_init = xmodule.services.EventPublishingService.__init__ - def patched_init(runtime_self, **kwargs): + def patched_init(runtime_self, user, course_id, track_function, **kwargs): """ - Swap out publish in the __init__ + Swap out track_function in the __init__ """ - old_publish = kwargs["publish"] + old_track_function = track_function - def publish(block, event_type, event): + def new_track_function(event_type, event): """ - Log the event, and call the original publish + Log the event, and call the original track_function. """ self.events.append({"event": event, "event_type": event_type}) - old_publish(block, event_type, event) - kwargs['publish'] = publish - return saved_init(runtime_self, **kwargs) + old_track_function(event_type, event) + track_function = new_track_function + return saved_init(runtime_self, user, course_id, track_function, **kwargs) self.events = [] - lms_sys = "lms.djangoapps.lms_xblock.runtime.LmsModuleSystem.__init__" - patcher = mock.patch(lms_sys, patched_init) + publish_service = "xmodule.services.EventPublishingService.__init__" + patcher = mock.patch(publish_service, patched_init) patcher.start() self.addCleanup(patcher.stop) diff --git a/xmodule/capa/responsetypes.py b/xmodule/capa/responsetypes.py index 5c7589e540..03b502ee76 100644 --- a/xmodule/capa/responsetypes.py +++ b/xmodule/capa/responsetypes.py @@ -371,8 +371,7 @@ class LoncapaResponse(six.with_metaclass(abc.ABCMeta, object)): else: label = _('Incorrect:') - # self.runtime.track_function('get_demand_hint', event_info) - # This this "feedback hint" event + # This is the "feedback hint" event event_info = {} event_info['module_id'] = text_type(self.capa_module.location) event_info['problem_part_id'] = self.id @@ -384,7 +383,7 @@ class LoncapaResponse(six.with_metaclass(abc.ABCMeta, object)): event_info['question_type'] = question_tag if log_extra: event_info.update(log_extra) - self.capa_module.runtime.track_function('edx.problem.hint.feedback_displayed', event_info) + self.capa_module.runtime.publish(self.capa_module, 'edx.problem.hint.feedback_displayed', event_info) # Form the div-wrapped hint texts hints_wrap = HTML('').join( diff --git a/xmodule/capa/tests/helpers.py b/xmodule/capa/tests/helpers.py index 023c02b7cf..6c6106f223 100644 --- a/xmodule/capa/tests/helpers.py +++ b/xmodule/capa/tests/helpers.py @@ -85,7 +85,7 @@ def test_capa_system(render_template=None): def mock_capa_module(): """ - capa response types needs just two things from the capa_module: location and track_function. + capa response types needs just two things from the capa_module: location and publish. """ def mock_location_text(self): # lint-amnesty, pylint: disable=unused-argument """ @@ -99,7 +99,7 @@ def mock_capa_module(): else: capa_module.location.__str__ = mock_location_text # The following comes into existence by virtue of being called - # capa_module.runtime.track_function + # capa_module.runtime.publish return capa_module diff --git a/xmodule/capa/tests/test_hint_functionality.py b/xmodule/capa/tests/test_hint_functionality.py index 0b0fca53a6..2fefc046a8 100644 --- a/xmodule/capa/tests/test_hint_functionality.py +++ b/xmodule/capa/tests/test_hint_functionality.py @@ -50,7 +50,8 @@ class TextInputHintsTest(HintTest): """Test that the tracking log comes out right.""" self.problem.capa_module.reset_mock() self.get_hint('1_3_1', 'Blue') - self.problem.capa_module.runtime.track_function.assert_called_with( + self.problem.capa_module.runtime.publish.assert_called_with( + self.problem.capa_module, 'edx.problem.hint.feedback_displayed', {'module_id': 'i4x://Foo/bar/mock/abc', 'problem_part_id': '1_2', @@ -223,7 +224,8 @@ class NumericInputHintsTest(HintTest): def test_tracking_log(self): self.get_hint('1_2_1', '1.141') - self.problem.capa_module.runtime.track_function.assert_called_with( + self.problem.capa_module.runtime.publish.assert_called_with( + self.problem.capa_module, 'edx.problem.hint.feedback_displayed', {'module_id': 'i4x://Foo/bar/mock/abc', 'problem_part_id': '1_1', 'trigger_type': 'single', 'hint_label': 'Nice', @@ -361,7 +363,8 @@ class CheckboxHintsTestTracking(HintTest): """Test checkbox tracking log - by far the most complicated case""" # A -> 1 hint self.get_hint('1_2_1', ['choice_0']) - self.problem.capa_module.runtime.track_function.assert_called_with( + self.problem.capa_module.runtime.publish.assert_called_with( + self.problem.capa_module, 'edx.problem.hint.feedback_displayed', {'hint_label': 'Incorrect:', 'module_id': 'i4x://Foo/bar/mock/abc', @@ -375,9 +378,10 @@ class CheckboxHintsTestTracking(HintTest): ) # B C -> 2 hints - self.problem.capa_module.runtime.track_function.reset_mock() + self.problem.capa_module.runtime.publish.reset_mock() self.get_hint('1_2_1', ['choice_1', 'choice_2']) - self.problem.capa_module.runtime.track_function.assert_called_with( + self.problem.capa_module.runtime.publish.assert_called_with( + self.problem.capa_module, 'edx.problem.hint.feedback_displayed', {'hint_label': 'Incorrect:', 'module_id': 'i4x://Foo/bar/mock/abc', @@ -394,9 +398,10 @@ class CheckboxHintsTestTracking(HintTest): ) # A C -> 1 Compound hint - self.problem.capa_module.runtime.track_function.reset_mock() + self.problem.capa_module.runtime.publish.reset_mock() self.get_hint('1_2_1', ['choice_0', 'choice_2']) - self.problem.capa_module.runtime.track_function.assert_called_with( + self.problem.capa_module.runtime.publish.assert_called_with( + self.problem.capa_module, 'edx.problem.hint.feedback_displayed', {'hint_label': 'Correct:', 'module_id': 'i4x://Foo/bar/mock/abc', @@ -425,7 +430,8 @@ class MultpleChoiceHintsTest(HintTest): """Test that the tracking log comes out right.""" self.problem.capa_module.reset_mock() self.get_hint('1_3_1', 'choice_2') - self.problem.capa_module.runtime.track_function.assert_called_with( + self.problem.capa_module.runtime.publish.assert_called_with( + self.problem.capa_module, 'edx.problem.hint.feedback_displayed', {'module_id': 'i4x://Foo/bar/mock/abc', 'problem_part_id': '1_2', 'trigger_type': 'single', 'student_answer': ['choice_2'], 'correctness': False, 'question_type': 'multiplechoiceresponse', @@ -465,7 +471,8 @@ class MultpleChoiceHintsWithHtmlTest(HintTest): """Test that the tracking log comes out right.""" self.problem.capa_module.reset_mock() self.get_hint('1_2_1', 'choice_0') - self.problem.capa_module.runtime.track_function.assert_called_with( + self.problem.capa_module.runtime.publish.assert_called_with( + self.problem.capa_module, 'edx.problem.hint.feedback_displayed', {'module_id': 'i4x://Foo/bar/mock/abc', 'problem_part_id': '1_1', 'trigger_type': 'single', 'student_answer': ['choice_0'], 'correctness': False, 'question_type': 'multiplechoiceresponse', @@ -498,7 +505,8 @@ class DropdownHintsTest(HintTest): """Test that the tracking log comes out right.""" self.problem.capa_module.reset_mock() self.get_hint('1_3_1', 'FACES') - self.problem.capa_module.runtime.track_function.assert_called_with( + self.problem.capa_module.runtime.publish.assert_called_with( + self.problem.capa_module, 'edx.problem.hint.feedback_displayed', {'module_id': 'i4x://Foo/bar/mock/abc', 'problem_part_id': '1_2', 'trigger_type': 'single', 'student_answer': ['FACES'], 'correctness': True, 'question_type': 'optionresponse', diff --git a/xmodule/capa_module.py b/xmodule/capa_module.py index 71be841393..dde79485f5 100644 --- a/xmodule/capa_module.py +++ b/xmodule/capa_module.py @@ -1558,7 +1558,7 @@ class ProblemBlock( """ event_info = {} event_info['problem_id'] = str(self.location) - self.track_function_unmask('showanswer', event_info) + self.publish_unmasked('showanswer', event_info) if not self.answer_available(): # lint-amnesty, pylint: disable=no-else-raise raise NotFoundError('Answer is not available') else: @@ -1736,13 +1736,13 @@ class ProblemBlock( self.max_attempts, ) event_info['failure'] = 'closed' - self.track_function_unmask('problem_check_fail', event_info) + self.publish_unmasked('problem_check_fail', event_info) raise NotFoundError(_("Problem is closed.")) # Problem submitted. Student should reset before checking again if self.done and self.rerandomize == RANDOMIZATION.ALWAYS: event_info['failure'] = 'unreset' - self.track_function_unmask('problem_check_fail', event_info) + self.publish_unmasked('problem_check_fail', event_info) raise NotFoundError(_("Problem must be reset before it can be submitted again.")) # Problem queued. Students must wait a specified waittime before they are allowed to submit @@ -1839,7 +1839,7 @@ class ProblemBlock( event_info['success'] = success event_info['attempts'] = self.attempts event_info['submission'] = self.get_submission_metadata_safe(answers_without_files, correct_map) - self.track_function_unmask('problem_check', event_info) + self.publish_unmasked('problem_check', event_info) # render problem into HTML html = self.get_problem_html(encapsulate=False, submit_notification=True) @@ -1854,9 +1854,9 @@ class ProblemBlock( } # pylint: enable=too-many-statements - def track_function_unmask(self, title, event_info): + def publish_unmasked(self, title, event_info): """ - All calls to runtime.track_function route through here so that the + All calls to runtime.publish route through here so that the choice names can be unmasked. """ # Do the unmask translates on a copy of event_info, @@ -2033,7 +2033,7 @@ class ProblemBlock( # Too late. Cannot submit if self.closed() and not self.max_attempts == 0: event_info['failure'] = 'closed' - self.track_function_unmask('save_problem_fail', event_info) + self.publish_unmasked('save_problem_fail', event_info) return { 'success': False, # pylint: disable=line-too-long @@ -2046,7 +2046,7 @@ class ProblemBlock( # again. if self.done and self.rerandomize == RANDOMIZATION.ALWAYS: event_info['failure'] = 'done' - self.track_function_unmask('save_problem_fail', event_info) + self.publish_unmasked('save_problem_fail', event_info) return { 'success': False, 'msg': _("Problem needs to be reset prior to save.") @@ -2058,7 +2058,7 @@ class ProblemBlock( self.set_state_from_lcp() self.set_score(self.score_from_lcp(self.lcp)) - self.track_function_unmask('save_problem_success', event_info) + self.publish_unmasked('save_problem_success', event_info) msg = _("Your answers have been saved.") if not self.max_attempts == 0: msg = _( @@ -2089,7 +2089,7 @@ class ProblemBlock( if self.closed(): event_info['failure'] = 'closed' - self.track_function_unmask('reset_problem_fail', event_info) + self.publish_unmasked('reset_problem_fail', event_info) return { 'success': False, # pylint: disable=line-too-long @@ -2100,7 +2100,7 @@ class ProblemBlock( if not self.is_submitted(): event_info['failure'] = 'not_done' - self.track_function_unmask('reset_problem_fail', event_info) + self.publish_unmasked('reset_problem_fail', event_info) return { 'success': False, 'msg': _("You must submit an answer before you can select Reset."), @@ -2121,7 +2121,7 @@ class ProblemBlock( self.publish_grade() event_info['new_state'] = self.lcp.get_state() - self.track_function_unmask('reset_problem', event_info) + self.publish_unmasked('reset_problem', event_info) return { 'success': True, @@ -2155,7 +2155,7 @@ class ProblemBlock( if not self.lcp.supports_rescoring(): event_info['failure'] = 'unsupported' - self.track_function_unmask('problem_rescore_fail', event_info) + self.publish_unmasked('problem_rescore_fail', event_info) # pylint: disable=line-too-long # Translators: 'rescoring' refers to the act of re-submitting a student's solution so it can get a new score. raise NotImplementedError(_("Problem's definition does not support rescoring.")) @@ -2163,7 +2163,7 @@ class ProblemBlock( if not self.done: event_info['failure'] = 'unanswered' - self.track_function_unmask('problem_rescore_fail', event_info) + self.publish_unmasked('problem_rescore_fail', event_info) raise NotFoundError(_("Problem must be answered before it can be graded again.")) # get old score, for comparison: @@ -2176,12 +2176,12 @@ class ProblemBlock( except (StudentInputError, ResponseError, LoncapaProblemError) as inst: # lint-amnesty, pylint: disable=unused-variable log.warning("Input error in capa_module:problem_rescore", exc_info=True) event_info['failure'] = 'input_error' - self.track_function_unmask('problem_rescore_fail', event_info) + self.publish_unmasked('problem_rescore_fail', event_info) raise except Exception: event_info['failure'] = 'unexpected' - self.track_function_unmask('problem_rescore_fail', event_info) + self.publish_unmasked('problem_rescore_fail', event_info) raise # rescoring should have no effect on attempts, so don't @@ -2203,7 +2203,7 @@ class ProblemBlock( event_info['correct_map'] = self.lcp.correct_map.get_dict() event_info['success'] = success event_info['attempts'] = self.attempts - self.track_function_unmask('problem_rescore', event_info) + self.publish_unmasked('problem_rescore', event_info) def has_submitted_answer(self): return self.done diff --git a/xmodule/services.py b/xmodule/services.py index 4c6ed49f40..a12730d06b 100644 --- a/xmodule/services.py +++ b/xmodule/services.py @@ -5,15 +5,17 @@ Module contains various XModule/XBlock services import inspect import logging - from functools import partial from config_models.models import ConfigurationModel from django.conf import settings +from eventtracking import tracker from edx_when.field_data import DateLookupFieldData from xblock.reference.plugins import Service from xblock.runtime import KvsFieldData +from common.djangoapps.track import contexts +from lms.djangoapps.courseware.masquerade import is_masquerading_as_specific_student from xmodule.modulestore.django import modulestore from lms.djangoapps.courseware.field_overrides import OverrideFieldData @@ -21,6 +23,7 @@ from lms.djangoapps.courseware.model_data import DjangoKeyValueStore, FieldDataC from lms.djangoapps.lms_xblock.field_data import LmsFieldData from lms.djangoapps.lms_xblock.models import XBlockAsidesConfig +from lms.djangoapps.grades.api import signals as grades_signals log = logging.getLogger(__name__) @@ -222,3 +225,98 @@ class RebindUserService(Service): # now bind the module to the new ModuleSystem instance and vice-versa block.runtime = inner_system inner_system.xmodule_instance = block + + +class EventPublishingService(Service): + """ + An XBlock Service that allows XModules to publish events (e.g. grading, completion). + + We have separated it from the ModuleSystem to be able to alter its behavior when using a different context: + LMS, Studio, or Instructor tasks. + """ + def __init__(self, user, course_id, track_function, **kwargs): + super().__init__(**kwargs) + self.user = user + self.course_id = course_id + self.track_function = track_function + self.completion_service = None + + def publish(self, block, event_type, event): + """ + A function that allows XModules to publish events. + """ + self.completion_service = block.runtime.service(block, 'completion') + + handle_event = self._get_event_handler(event_type) + if handle_event and not is_masquerading_as_specific_student(self.user, self.course_id): + handle_event(block, event) + else: + context = contexts.course_context_from_course_id(self.course_id) + if not self.user.is_anonymous: + context['user_id'] = self.user.id + + context['asides'] = {} + for aside in block.runtime.get_asides(block): + if hasattr(aside, 'get_event_context'): + aside_event_info = aside.get_event_context(event_type, event) + if aside_event_info is not None: + context['asides'][aside.scope_ids.block_type] = aside_event_info + with tracker.get_tracker().context(event_type, context): + self.track_function(event_type, event) + + def _get_event_handler(self, event_type): + """ + Return an appropriate function to handle the event. + + Returns None if no special processing is required. + """ + handlers = { + 'grade': self._handle_grade_event, + } + if self.completion_service and self.completion_service.completion_tracking_enabled(): + handlers.update( + { + 'completion': lambda block, event: self.completion_service.submit_completion( + block.scope_ids.usage_id, event['completion'] + ), + 'progress': self._handle_deprecated_progress_event, + } + ) + return handlers.get(event_type) + + def _handle_grade_event(self, block, event): + """ + Submit a grade for the block. + """ + if not self.user.is_anonymous: + grades_signals.SCORE_PUBLISHED.send( + sender=None, + block=block, + user=self.user, + raw_earned=event['value'], + raw_possible=event['max_value'], + only_if_higher=event.get('only_if_higher'), + score_deleted=event.get('score_deleted'), + grader_response=event.get('grader_response'), + ) + + def _handle_deprecated_progress_event(self, block, event): + """ + DEPRECATED: Submit a completion for the block represented by the + progress event. + + This exists to support the legacy progress extension used by + edx-solutions. New XBlocks should not emit these events, but instead + emit completion events directly. + """ + requested_user_id = event.get('user_id', self.user.id) + if requested_user_id != self.user.id: + log.warning(f"{self.user} tried to submit a completion on behalf of {requested_user_id}") + return + + # 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): + self.completion_service.submit_completion(block.scope_ids.usage_id, 1.0) diff --git a/xmodule/split_test_module.py b/xmodule/split_test_module.py index 069f999548..0953993c51 100644 --- a/xmodule/split_test_module.py +++ b/xmodule/split_test_module.py @@ -399,7 +399,6 @@ class SplitTestBlock( # lint-amnesty, pylint: disable=abstract-method """ Record in the tracking logs which child was rendered """ - # TODO: use publish instead, when publish is wired to the tracking logs try: child_id = str(self.child.scope_ids.usage_id) except Exception: @@ -410,7 +409,7 @@ class SplitTestBlock( # lint-amnesty, pylint: disable=abstract-method ) raise else: - self.system.track_function('xblock.split_test.child_render', {'child_id': child_id}) + self.runtime.publish('xblock.split_test.child_render', {'child_id': child_id}) return Response() def get_icon_class(self): diff --git a/xmodule/tests/__init__.py b/xmodule/tests/__init__.py index 107d6b27bc..fddc7bef17 100644 --- a/xmodule/tests/__init__.py +++ b/xmodule/tests/__init__.py @@ -138,7 +138,6 @@ def get_test_system( return descriptor return TestModuleSystem( - track_function=Mock(name='get_test_system.track_function'), get_module=get_module, services={ 'user': user_service, diff --git a/xmodule/tests/test_capa_module.py b/xmodule/tests/test_capa_module.py index 7513c2b9ed..4e8a910869 100644 --- a/xmodule/tests/test_capa_module.py +++ b/xmodule/tests/test_capa_module.py @@ -1672,10 +1672,10 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss Test calling get_demand_hunt() results in an event being published. """ module = CapaFactory.create(xml=self.demand_xml) - with patch.object(module.runtime, 'publish') as mock_track_function: + with patch.object(module.runtime, 'publish') as mock_publish: module.get_problem_html() module.get_demand_hint(0) - mock_track_function.assert_called_with( + mock_publish.assert_called_with( module, 'edx.problem.hint.demandhint_displayed', {'hint_index': 0, 'module_id': str(module.location), 'hint_text': 'Demand 1', 'hint_len': 2} @@ -2026,13 +2026,13 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss def test_check_unmask(self): """ Check that shuffle unmasking is plumbed through: when submit_problem is called, - unmasked names should appear in the track_function event_info. + unmasked names should appear in the publish event_info. """ module = CapaFactory.create(xml=self.common_shuffle_xml) - with patch.object(module.runtime, 'publish') as mock_track_function: + with patch.object(module.runtime, 'publish') as mock_publish: get_request_dict = {CapaFactory.input_key(): 'choice_3'} # the correct choice module.submit_problem(get_request_dict) - mock_call = mock_track_function.mock_calls[1] + mock_call = mock_publish.mock_calls[1] event_info = mock_call[1][2] assert event_info['answers'][CapaFactory.answer_key()] == 'choice_3' # 'permutation' key added to record how problem was shown @@ -2042,26 +2042,26 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss @unittest.skip("masking temporarily disabled") def test_save_unmask(self): - """On problem save, unmasked data should appear on track_function.""" + """On problem save, unmasked data should appear on publish.""" module = CapaFactory.create(xml=self.common_shuffle_xml) - with patch.object(module.runtime, 'track_function') as mock_track_function: + with patch.object(module.runtime, 'publish') as mock_publish: get_request_dict = {CapaFactory.input_key(): 'mask_0'} module.save_problem(get_request_dict) - mock_call = mock_track_function.mock_calls[0] + mock_call = mock_publish.mock_calls[0] event_info = mock_call[1][1] assert event_info['answers'][CapaFactory.answer_key()] == 'choice_2' assert event_info['permutation'][CapaFactory.answer_key()] is not None @unittest.skip("masking temporarily disabled") def test_reset_unmask(self): - """On problem reset, unmask names should appear track_function.""" + """On problem reset, unmask names should appear publish.""" module = CapaFactory.create(xml=self.common_shuffle_xml) get_request_dict = {CapaFactory.input_key(): 'mask_0'} module.submit_problem(get_request_dict) # On reset, 'old_state' should use unmasked names - with patch.object(module.runtime, 'track_function') as mock_track_function: + with patch.object(module.runtime, 'publish') as mock_publish: module.reset_problem(None) - mock_call = mock_track_function.mock_calls[0] + mock_call = mock_publish.mock_calls[0] event_info = mock_call[1][1] assert mock_call[1][0] == 'reset_problem' assert event_info['old_state']['student_answers'][CapaFactory.answer_key()] == 'choice_2' @@ -2069,21 +2069,21 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss @unittest.skip("masking temporarily disabled") def test_rescore_unmask(self): - """On problem rescore, unmasked names should appear on track_function.""" + """On problem rescore, unmasked names should appear on publish.""" module = CapaFactory.create(xml=self.common_shuffle_xml) get_request_dict = {CapaFactory.input_key(): 'mask_0'} module.submit_problem(get_request_dict) # On rescore, state/student_answers should use unmasked names - with patch.object(module.runtime, 'track_function') as mock_track_function: + with patch.object(module.runtime, 'publish') as mock_publish: module.rescore_problem(only_if_higher=False) # lint-amnesty, pylint: disable=no-member - mock_call = mock_track_function.mock_calls[0] + mock_call = mock_publish.mock_calls[0] event_info = mock_call[1][1] assert mock_call[1][0] == 'problem_rescore' assert event_info['state']['student_answers'][CapaFactory.answer_key()] == 'choice_2' assert event_info['permutation'][CapaFactory.answer_key()] is not None def test_check_unmask_answerpool(self): - """Check answer-pool question track_function uses unmasked names""" + """Check answer-pool question publish uses unmasked names""" xml = textwrap.dedent(""" @@ -2097,10 +2097,10 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss """) module = CapaFactory.create(xml=xml) - with patch.object(module.runtime, 'publish') as mock_track_function: + with patch.object(module.runtime, 'publish') as mock_publish: get_request_dict = {CapaFactory.input_key(): 'choice_2'} # mask_X form when masking enabled module.submit_problem(get_request_dict) - mock_call = mock_track_function.mock_calls[1] + mock_call = mock_publish.mock_calls[1] event_info = mock_call[1][2] assert event_info['answers'][CapaFactory.answer_key()] == 'choice_2' # 'permutation' key added to record how problem was shown @@ -2982,12 +2982,12 @@ class ProblemCheckTrackingTest(unittest.TestCase): return CustomCapaFactory def get_event_for_answers(self, module, answer_input_dict): # lint-amnesty, pylint: disable=missing-function-docstring - with patch.object(module.runtime, 'publish') as mock_track_function: + with patch.object(module.runtime, 'publish') as mock_publish: module.submit_problem(answer_input_dict) - assert len(mock_track_function.mock_calls) >= 2 + assert len(mock_publish.mock_calls) >= 2 # There are potentially 2 track logs: answers and hint. [-1]=answers. - mock_call = mock_track_function.mock_calls[-1] + mock_call = mock_publish.mock_calls[-1] event = mock_call[1][2] return event diff --git a/xmodule/x_module.py b/xmodule/x_module.py index a461e6b957..0ae31ddd38 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1673,38 +1673,26 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, def __init__( self, - track_function, get_module, descriptor_runtime, - publish=None, **kwargs, ): """ Create a closure around the system environment. - track_function - function of (event_type, event), intended for logging - or otherwise tracking the event. - TODO: Not used, and has inconsistent args in different - files. Update or remove. - get_module - function that takes a descriptor and returns a corresponding module instance object. If the current user does not have access to that location, returns None. descriptor_runtime - A `DescriptorSystem` to use for loading xblocks by id - - publish(event) - A function that allows XModules to publish events (such as grade changes) """ kwargs.setdefault('id_reader', getattr(descriptor_runtime, 'id_reader', OpaqueKeyReader())) kwargs.setdefault('id_generator', getattr(descriptor_runtime, 'id_generator', AsideKeyGenerator())) super().__init__(**kwargs) - self.track_function = track_function self.get_module = get_module - if publish: - self.publish = publish self.xmodule_instance = None self.descriptor_runtime = descriptor_runtime @@ -1740,7 +1728,12 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, raise NotImplementedError("edX Platform doesn't currently implement XBlock resource urls") def publish(self, block, event_type, event): # lint-amnesty, pylint: disable=arguments-differ - pass + """ + Publish events through the `EventPublishingService`. + This ensures that the correct track method is used for Instructor tasks. + """ + if publish_service := self._services.get('publish'): + publish_service.publish(block, event_type, event) def service(self, block, service_name): """