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!'