From 05afe7855279788ae4da6f6619d47163c86d1abd Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Tue, 26 Oct 2021 15:10:36 -0400 Subject: [PATCH] fix: AA-1063: Reset Completion data for problems on exam reset We encountered a bug where learners were sometimes not having their completion information reset when their exam is reset. It was unclear what was actually causing the completion to not be reset (it usually is via a signal listener), but the effect was learners being unable to reset their due dates in order to attempt the exam again since the exam believed it was still complete. This PR will likely be duplicating calls to the Completion API, but we believe that is worthwhile to ensure successful completion state reset. --- lms/djangoapps/instructor/services.py | 12 +++-- lms/djangoapps/instructor/tasks.py | 15 +++--- .../instructor/tests/test_services.py | 49 ++++++++++--------- 3 files changed, 43 insertions(+), 33 deletions(-) diff --git a/lms/djangoapps/instructor/services.py b/lms/djangoapps/instructor/services.py index 09150c73ba..ba23c0e2d9 100644 --- a/lms/djangoapps/instructor/services.py +++ b/lms/djangoapps/instructor/services.py @@ -16,7 +16,7 @@ from common.djangoapps.student.models import get_user_by_username_or_email from common.djangoapps.student.roles import CourseStaffRole from lms.djangoapps.commerce.utils import create_zendesk_ticket from lms.djangoapps.courseware.models import StudentModule -from lms.djangoapps.instructor.tasks import complete_student_attempt_task +from lms.djangoapps.instructor.tasks import update_exam_completion_task from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) @@ -80,9 +80,15 @@ class InstructorService: ) log.error(err_msg) + # In some cases, reset_student_attempts does not clear the entire exam's completion state. + # One example of this is an exam with multiple units (verticals) within it and the learner + # never viewing one of the units. All of the content in that unit will still be marked complete, + # but the reset code is unable to handle clearing the completion in that scenario. + update_exam_completion_task.apply_async((student_identifier, content_id, 0.0)) + def complete_student_attempt(self, user_identifier: str, content_id: str) -> None: """ - Calls the complete_student_attempt_task + Calls the update_exam_completion_task, marking the exam as complete. The task submits all completable xblocks inside of the content_id block to the Completion Service to mark them as complete. One use case of this function is @@ -94,7 +100,7 @@ class InstructorService: user_identifier (str): username or email of a user content_id (str): the block key for a piece of content """ - complete_student_attempt_task.apply_async((user_identifier, content_id)) + update_exam_completion_task.apply_async((user_identifier, content_id, 1.0)) def is_course_staff(self, user, course_id): """ diff --git a/lms/djangoapps/instructor/tasks.py b/lms/djangoapps/instructor/tasks.py index bf4c9fe1a6..d5c13b207d 100644 --- a/lms/djangoapps/instructor/tasks.py +++ b/lms/djangoapps/instructor/tasks.py @@ -1,4 +1,4 @@ -""" Celery Tasks for the Instructor App """ +""" Celery Tasks for the Instructor App. """ import logging @@ -22,9 +22,9 @@ log = logging.getLogger(__name__) @shared_task(base=LoggedTask, ignore_result=True) @set_code_owner_attribute -def complete_student_attempt_task(user_identifier: str, content_id: str) -> None: +def update_exam_completion_task(user_identifier: str, content_id: str, completion: float) -> None: """ - Marks all completable children of content_id as complete for the user + Marks all completable children of content_id as complete for the user. Submits all completable xblocks inside of the content_id block to the Completion Service to mark them as complete. One use case of this function is @@ -35,6 +35,7 @@ def complete_student_attempt_task(user_identifier: str, content_id: str) -> None params: user_identifier (str): username or email of a user content_id (str): the block key for a piece of content + completion (float): the completion percentage to send to the Completion service (either 1.0 or 0.0) """ err_msg_prefix = ( 'Error occurred while attempting to complete student attempt for user ' @@ -78,13 +79,13 @@ def complete_student_attempt_task(user_identifier: str, content_id: str) -> None log.error(err_msg) return - def _submit_completions(block, user): + def _submit_completions(block, user, completion): """ Recursively submits the children for completion to the Completion Service """ mode = XBlockCompletionMode.get_mode(block) if mode == XBlockCompletionMode.COMPLETABLE: - block.runtime.publish(block, 'completion', {'completion': 1.0, 'user_id': user.id}) + block.runtime.publish(block, 'completion', {'completion': completion, 'user_id': user.id}) elif mode == XBlockCompletionMode.AGGREGATOR: # I know this looks weird, but at the time of writing at least, there isn't a good # single way to get the children assigned for a partcular user. Some blocks define the @@ -96,6 +97,6 @@ def complete_student_attempt_task(user_identifier: str, content_id: str) -> None or (hasattr(block, 'get_children') and block.get_children()) or []) for child in block_children: - _submit_completions(child, user) + _submit_completions(child, user, completion) - _submit_completions(root_module, user) + _submit_completions(root_module, user, completion) diff --git a/lms/djangoapps/instructor/tests/test_services.py b/lms/djangoapps/instructor/tests/test_services.py index 0a52a1981b..8e52b8bfc4 100644 --- a/lms/djangoapps/instructor/tests/test_services.py +++ b/lms/djangoapps/instructor/tests/test_services.py @@ -15,7 +15,6 @@ from common.djangoapps.student.tests.factories import UserFactory from lms.djangoapps.courseware.models import StudentModule from lms.djangoapps.instructor.access import allow_access from lms.djangoapps.instructor.services import InstructorService -from lms.djangoapps.instructor.tests.test_tools import msk_from_problem_urlname from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.partitions.partitions import Group, UserPartition @@ -31,16 +30,12 @@ class InstructorServiceTests(SharedModuleStoreTestCase): super().setUpClass() cls.email = 'escalation@test.com' cls.course = CourseFactory.create(proctoring_escalation_email=cls.email) - cls.problem_location = msk_from_problem_urlname( - cls.course.id, - 'robot-some-problem-urlname' - ) - cls.other_problem_location = msk_from_problem_urlname( - cls.course.id, - 'robot-some-other_problem-urlname' - ) - cls.problem_urlname = str(cls.problem_location) - cls.other_problem_urlname = str(cls.other_problem_location) + cls.section = ItemFactory.create(parent=cls.course, category='chapter') + cls.subsection = ItemFactory.create(parent=cls.section, category='sequential') + cls.unit = ItemFactory.create(parent=cls.subsection, category='vertical') + cls.problem = ItemFactory.create(parent=cls.unit, category='problem') + cls.unit_2 = ItemFactory.create(parent=cls.subsection, category='vertical') + cls.problem_2 = ItemFactory.create(parent=cls.unit_2, category='problem') cls.complete_error_prefix = ('Error occurred while attempting to complete student attempt for ' 'user {user} for content_id {content_id}. ') @@ -54,12 +49,13 @@ class InstructorServiceTests(SharedModuleStoreTestCase): self.module_to_reset = StudentModule.objects.create( student=self.student, course_id=self.course.id, - module_state_key=self.problem_location, + module_state_key=self.problem.location, state=json.dumps({'attempts': 2}), ) @mock.patch('lms.djangoapps.grades.signals.handlers.PROBLEM_WEIGHTED_SCORE_CHANGED.send') - def test_reset_student_attempts_delete(self, _mock_signal): + @mock.patch('completion.handlers.BlockCompletion.objects.submit_completion') + def test_reset_student_attempts_delete(self, mock_submit, _mock_signal): """ Test delete student state. """ @@ -68,17 +64,23 @@ class InstructorServiceTests(SharedModuleStoreTestCase): assert StudentModule.objects.filter(student=self.module_to_reset.student, course_id=self.course.id, module_state_key=self.module_to_reset.module_state_key).count() == 1 - self.service.delete_student_attempt( - self.student.username, - str(self.course.id), - self.problem_urlname, - requesting_user=self.student, - ) + with override_waffle_switch(ENABLE_COMPLETION_TRACKING_SWITCH, True): + self.service.delete_student_attempt( + self.student.username, + str(self.course.id), + str(self.subsection.location), + requesting_user=self.student, + ) # make sure the module has been deleted assert StudentModule.objects.filter(student=self.module_to_reset.student, course_id=self.course.id, module_state_key=self.module_to_reset.module_state_key).count() == 0 + # Assert we send completion == 0.0 for both problems even though the second problem was never viewed + assert mock_submit.call_count == 2 + mock_submit.assert_any_call(user=self.student, block_key=self.problem.location, completion=0.0) + mock_submit.assert_any_call(user=self.student, block_key=self.problem_2.location, completion=0.0) + def test_reset_bad_content_id(self): """ Negative test of trying to reset attempts with bad content_id @@ -113,7 +115,7 @@ class InstructorServiceTests(SharedModuleStoreTestCase): result = self.service.delete_student_attempt( # lint-amnesty, pylint: disable=assignment-from-none self.student.username, str(self.course.id), - self.other_problem_urlname, + str(self.problem_2.location), requesting_user=self.student, ) assert result is None @@ -198,9 +200,10 @@ class InstructorServiceTests(SharedModuleStoreTestCase): Assert complete_student_attempt with a bad user raises error and returns None """ username = 'bad_user' - self.service.complete_student_attempt(username, self.problem_urlname) + block_id = str(self.problem.location) + self.service.complete_student_attempt(username, block_id) mock_logger.assert_called_once_with( - self.complete_error_prefix.format(user=username, content_id=self.problem_urlname) + 'User does not exist!' + self.complete_error_prefix.format(user=username, content_id=block_id) + 'User does not exist!' ) @mock.patch('lms.djangoapps.instructor.tasks.log.error') @@ -221,7 +224,7 @@ class InstructorServiceTests(SharedModuleStoreTestCase): raises error and returns None """ username = self.student.username - block = self.problem_urlname + block = 'i4x://org.0/course_0/problem/fake_problem' self.service.complete_student_attempt(username, block) mock_logger.assert_called_once_with( self.complete_error_prefix.format(user=username, content_id=block) + 'Block not found in the modulestore!'