From 9c5779ad9c432db851d3d7cfa7abcdbae973cfe6 Mon Sep 17 00:00:00 2001 From: Jansen Kantor Date: Wed, 3 Apr 2024 10:08:29 -0400 Subject: [PATCH] feat: add flag to silence signals and events when clearing state (#34434) * feat: add flag to silence signals and events when clearing state * feat: don't emit signals and events when clearing entire course --- .../grades/tests/integration/test_events.py | 97 +++++++++++-------- lms/djangoapps/instructor/enrollment.py | 36 ++++++- lms/djangoapps/support/tasks.py | 9 +- lms/djangoapps/support/tests/test_tasks.py | 21 ++-- 4 files changed, 110 insertions(+), 53 deletions(-) diff --git a/lms/djangoapps/grades/tests/integration/test_events.py b/lms/djangoapps/grades/tests/integration/test_events.py index 881ad049b3..f058cc8179 100644 --- a/lms/djangoapps/grades/tests/integration/test_events.py +++ b/lms/djangoapps/grades/tests/integration/test_events.py @@ -1,6 +1,7 @@ """ Test grading events across apps. """ +import ddt from unittest.mock import call as mock_call from unittest.mock import patch @@ -20,6 +21,7 @@ from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory # l from ... import events +@ddt.ddt class GradesEventIntegrationTest(ProblemSubmissionTestMixin, SharedModuleStoreTestCase): """ Tests integration between the eventing in various layers @@ -113,56 +115,67 @@ class GradesEventIntegrationTest(ProblemSubmissionTestMixin, SharedModuleStoreTe any_order=True, ) - def test_delete_student_state(self): + @ddt.data(True, False) + def test_delete_student_state(self, emit_signals): self.submit_question_answer('p1', {'2_1': 'choice_choice_2'}) with patch('lms.djangoapps.instructor.enrollment.tracker') as enrollment_tracker: with patch('lms.djangoapps.grades.events.tracker') as events_tracker: reset_student_attempts( - self.course.id, self.student, self.problem.location, self.instructor, delete_module=True, + self.course.id, + self.student, + self.problem.location, + self.instructor, + delete_module=True, + emit_signals_and_events=emit_signals ) course = self.store.get_course(self.course.id, depth=0) - event_transaction_id = enrollment_tracker.method_calls[0][1][1]['event_transaction_id'] - enrollment_tracker.emit.assert_called_with( - events.STATE_DELETED_EVENT_TYPE, - { - 'user_id': str(self.student.id), - 'course_id': str(self.course.id), - 'problem_id': str(self.problem.location), - 'instructor_id': str(self.instructor.id), - 'event_transaction_id': event_transaction_id, - 'event_transaction_type': events.STATE_DELETED_EVENT_TYPE, - } - ) - events_tracker.emit.assert_has_calls( - [ - mock_call( - events.COURSE_GRADE_CALCULATED, - { - 'percent_grade': 0.0, - 'grading_policy_hash': 'ChVp0lHGQGCevD0t4njna/C44zQ=', - 'user_id': str(self.student.id), - 'letter_grade': '', - 'event_transaction_id': event_transaction_id, - 'event_transaction_type': events.STATE_DELETED_EVENT_TYPE, - 'course_id': str(self.course.id), - 'course_edited_timestamp': str(course.subtree_edited_on), - 'course_version': str(course.course_version), - } - ), - mock_call( - events.COURSE_GRADE_NOW_FAILED_EVENT_TYPE, - { - 'user_id': str(self.student.id), - 'event_transaction_id': event_transaction_id, - 'event_transaction_type': events.STATE_DELETED_EVENT_TYPE, - 'course_id': str(self.course.id), - } - ), - ], - any_order=True, - ) + if not emit_signals: + enrollment_tracker.assert_not_called() + enrollment_tracker.emit.assert_not_called() + events_tracker.emit.assert_not_called() + else: + event_transaction_id = enrollment_tracker.method_calls[0][1][1]['event_transaction_id'] + enrollment_tracker.emit.assert_called_with( + events.STATE_DELETED_EVENT_TYPE, + { + 'user_id': str(self.student.id), + 'course_id': str(self.course.id), + 'problem_id': str(self.problem.location), + 'instructor_id': str(self.instructor.id), + 'event_transaction_id': event_transaction_id, + 'event_transaction_type': events.STATE_DELETED_EVENT_TYPE, + } + ) + events_tracker.emit.assert_has_calls( + [ + mock_call( + events.COURSE_GRADE_CALCULATED, + { + 'percent_grade': 0.0, + 'grading_policy_hash': 'ChVp0lHGQGCevD0t4njna/C44zQ=', + 'user_id': str(self.student.id), + 'letter_grade': '', + 'event_transaction_id': event_transaction_id, + 'event_transaction_type': events.STATE_DELETED_EVENT_TYPE, + 'course_id': str(self.course.id), + 'course_edited_timestamp': str(course.subtree_edited_on), + 'course_version': str(course.course_version), + } + ), + mock_call( + events.COURSE_GRADE_NOW_FAILED_EVENT_TYPE, + { + 'user_id': str(self.student.id), + 'event_transaction_id': event_transaction_id, + 'event_transaction_type': events.STATE_DELETED_EVENT_TYPE, + 'course_id': str(self.course.id), + } + ), + ], + any_order=True, + ) def test_rescoring_events(self): self.submit_question_answer('p1', {'2_1': 'choice_choice_3'}) diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index f8bf1dd866..1dd451a41c 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/lms/djangoapps/instructor/enrollment.py @@ -7,6 +7,7 @@ Does not include any access control, be sure to check access before calling. import json import logging +from contextlib import ExitStack, contextmanager from datetime import datetime import pytz @@ -19,7 +20,7 @@ from edx_ace import ace from edx_ace.recipient import Recipient from eventtracking import tracker from submissions import api as sub_api # installed from the edx-submissions repository -from submissions.models import score_set +from submissions.models import score_set, score_reset from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import ( # lint-amnesty, pylint: disable=line-too-long @@ -238,7 +239,28 @@ def send_beta_role_email(action, user, email_params): send_mail_to_student(user.email, email_params, language=get_user_email_language(user)) -def reset_student_attempts(course_id, student, module_state_key, requesting_user, delete_module=False): +@contextmanager +def _conditionally_disconnect_submissions_signal_recievers(emit_score_reset): + """ + Context manager helper. + - Disconnects the score_set signal on enter, reconnects on exit + - If emit_score_reset, disconnects score_reset signal on enter, reconnects on exit + """ + with ExitStack() as context_stack: + context_stack.enter_context(disconnect_submissions_signal_receiver(score_set)) + if not emit_score_reset: + context_stack.enter_context(disconnect_submissions_signal_receiver(score_reset)) + yield + + +def reset_student_attempts( + course_id, + student, + module_state_key, + requesting_user, + delete_module=False, + emit_signals_and_events=True, +): """ Reset student attempts for a problem. Optionally deletes all student state for the specified problem. @@ -248,6 +270,10 @@ def reset_student_attempts(course_id, student, module_state_key, requesting_user `student` is a User `problem_to_reset` is the name of a problem e.g. 'L2Node1'. To build the module_state_key 'problem/' and course information will be appended to `problem_to_reset`. + `delete_module`: Instead of resetting attempts, delete the learner's StudentModule + `emit_signals_and_events`: If this is False, don't fire django signals or emit events. This is intended for + the case where we are calling this function many times, and want to handle the signalling + and eventing at a bulk level rather than firing every individual call to this function Raises: ValueError: `problem_state` is invalid JSON. @@ -275,7 +301,7 @@ def reset_student_attempts(course_id, student, module_state_key, requesting_user # Inform these blocks of the reset and allow them to handle their data. clear_student_state = getattr(block, "clear_student_state", None) if callable(clear_student_state): - with disconnect_submissions_signal_receiver(score_set): + with _conditionally_disconnect_submissions_signal_recievers(emit_signals_and_events): clear_student_state( user_id=user_id, course_id=str(course_id), @@ -300,11 +326,15 @@ def reset_student_attempts(course_id, student, module_state_key, requesting_user user_id, str(course_id), str(module_state_key), + emit_signal=emit_signals_and_events, ) def _reset_or_delete_module(studentmodule): if delete_module: studentmodule.delete() + if not emit_signals_and_events: + return + create_new_event_transaction_id() set_event_transaction_type(grades_events.STATE_DELETED_EVENT_TYPE) tracker.emit( diff --git a/lms/djangoapps/support/tasks.py b/lms/djangoapps/support/tasks.py index 62b78c3751..cf00823d98 100644 --- a/lms/djangoapps/support/tasks.py +++ b/lms/djangoapps/support/tasks.py @@ -62,7 +62,14 @@ def reset_student_course(course_id, learner_email, reset_by_user_email): # Clear student state and score for data in blocks: try: - reset_student_attempts(course.id, user, data.scope_ids.usage_id, reset_by_user, True) + reset_student_attempts( + course.id, + user, + data.scope_ids.usage_id, + reset_by_user, + delete_module=True, + emit_signals_and_events=False + ) except StudentModule.DoesNotExist: pass diff --git a/lms/djangoapps/support/tests/test_tasks.py b/lms/djangoapps/support/tests/test_tasks.py index 67a268cdf3..181f9022f4 100644 --- a/lms/djangoapps/support/tests/test_tasks.py +++ b/lms/djangoapps/support/tests/test_tasks.py @@ -123,28 +123,32 @@ class ResetStudentCourse(TestSubmittingProblems): self.student_user, self.p1.location, self.user, - True + delete_module=True, + emit_signals_and_events=False, ), call( self.course.id, self.student_user, self.p2.location, self.user, - True + delete_module=True, + emit_signals_and_events=False, ), call( self.course.id, self.student_user, self.p3.location, self.user, - True + delete_module=True, + emit_signals_and_events=False, ), call( self.course.id, self.student_user, self.video.location, self.user, - True + delete_module=True, + emit_signals_and_events=False, ) ]) @@ -168,21 +172,24 @@ class ResetStudentCourse(TestSubmittingProblems): self.student_user, self.p1.location, self.user, - True + delete_module=True, + emit_signals_and_events=False, ), call( self.course.id, self.student_user, self.p2.location, self.user, - True + delete_module=True, + emit_signals_and_events=False, ), call( self.course.id, self.student_user, self.p3.location, self.user, - True + delete_module=True, + emit_signals_and_events=False, ) ])