feat: deprecate track_function and publish in ModuleSystem [BD-13] (#30046)

* feat: delete `track_function` from ModuleSystem

* feat: delete `publish` argument from ModuleSystem
This commit is contained in:
Piotr Surowiec
2022-11-15 16:46:24 +01:00
committed by GitHub
parent eb8f3b6750
commit f419d6b194
16 changed files with 189 additions and 206 deletions

View File

@@ -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(

View File

@@ -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

View File

@@ -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',

View File

@@ -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

View File

@@ -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)

View File

@@ -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):

View File

@@ -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,

View File

@@ -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("""
<problem>
<multiplechoiceresponse>
@@ -2097,10 +2097,10 @@ class ProblemBlockTest(unittest.TestCase): # lint-amnesty, pylint: disable=miss
</problem>
""")
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

View File

@@ -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):
"""