From cc7be98f9167be64361e698e5c3f6282509edede Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Fri, 4 Jun 2021 13:08:21 -0400 Subject: [PATCH] feat: AA-773: Add in logic to mark special exams as complete We have had numerous support tickets/bugs related to special exams not properly displaying completeness (see https://openedx.atlassian.net/browse/AA-773 for details). Along with a corresponding edx-proctoring PR, this will now submit completions for all completable children within a special exam upon the exam being completed for the first time (as dictated by the logic in the edx-proctoring PR). --- lms/djangoapps/instructor/services.py | 69 +++++++++++++--- .../instructor/tests/test_services.py | 82 ++++++++++++++++++- requirements/edx/base.txt | 2 +- requirements/edx/development.txt | 2 +- requirements/edx/testing.txt | 2 +- 5 files changed, 142 insertions(+), 15 deletions(-) diff --git a/lms/djangoapps/instructor/services.py b/lms/djangoapps/instructor/services.py index 3a9e380c43..9758037a88 100644 --- a/lms/djangoapps/instructor/services.py +++ b/lms/djangoapps/instructor/services.py @@ -9,14 +9,16 @@ from django.core.exceptions import ObjectDoesNotExist from django.utils.translation import ugettext as _ from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey +from xblock.completable import XBlockCompletionMode import lms.djangoapps.instructor.enrollment as enrollment from common.djangoapps.student import auth +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.views.tools import get_student_from_identifier from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError log = logging.getLogger(__name__) @@ -44,15 +46,12 @@ class InstructorService: course_id = CourseKey.from_string(course_id) try: - student = get_student_from_identifier(student_identifier) + student = get_user_by_username_or_email(student_identifier) except ObjectDoesNotExist: err_msg = ( 'Error occurred while attempting to reset student attempts for user ' - '{student_identifier} for content_id {content_id}. ' - 'User does not exist!'.format( - student_identifier=student_identifier, - content_id=content_id - ) + f'{student_identifier} for content_id {content_id}. ' + 'User does not exist!' ) log.error(err_msg) return @@ -78,13 +77,61 @@ class InstructorService: except (StudentModule.DoesNotExist, enrollment.sub_api.SubmissionError): err_msg = ( 'Error occurred while attempting to reset student attempts for user ' - '{student_identifier} for content_id {content_id}.'.format( - student_identifier=student_identifier, - content_id=content_id - ) + f'{student_identifier} for content_id {content_id}.' ) log.error(err_msg) + def complete_student_attempt(self, user_identifier, content_id): + """ + 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 + for special exams (timed/proctored) where regardless of submission status on + individual problems, we want to mark the entire exam as complete when the exam + is finished. + + params: + user_identifier (String): username or email of a user + content_id (String): the block key for a piece of content + """ + err_msg_prefix = ( + 'Error occurred while attempting to complete student attempt for user ' + f'{user_identifier} for content_id {content_id}. ' + ) + err_msg = None + try: + user = get_user_by_username_or_email(user_identifier) + block_key = UsageKey.from_string(content_id) + root_block = modulestore().get_item(block_key) + except ObjectDoesNotExist: + err_msg = err_msg_prefix + 'User does not exist!' + except InvalidKeyError: + err_msg = err_msg_prefix + 'Invalid content_id!' + except ItemNotFoundError: + err_msg = err_msg_prefix + 'Block not found in the modulestore!' + if err_msg: + log.error(err_msg) + return + + def _submit_completions(block, user): + """ + 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}) + 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 + # child descriptors method, but others don't and with blocks like Randomized Content + # (Library Content), the get_children method returns all children and not just assigned + # children. So this is our way around situations like that. + block_children = ((hasattr(block, 'get_child_descriptors') and block.get_child_descriptors()) + or (hasattr(block, 'get_children') and block.get_children())) + for child in block_children: + _submit_completions(child, user) + + _submit_completions(root_block, user) + def is_course_staff(self, user, course_id): """ Returns True if the user is the course staff diff --git a/lms/djangoapps/instructor/tests/test_services.py b/lms/djangoapps/instructor/tests/test_services.py index c2161f90d6..185db5d215 100644 --- a/lms/djangoapps/instructor/tests/test_services.py +++ b/lms/djangoapps/instructor/tests/test_services.py @@ -15,7 +15,7 @@ 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 +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory class InstructorServiceTests(SharedModuleStoreTestCase): @@ -38,6 +38,8 @@ class InstructorServiceTests(SharedModuleStoreTestCase): ) cls.problem_urlname = str(cls.problem_location) cls.other_problem_urlname = str(cls.other_problem_location) + cls.complete_error_prefix = ('Error occurred while attempting to complete student attempt for ' + 'user {user} for content_id {content_id}. ') def setUp(self): super().setUp() @@ -113,6 +115,84 @@ class InstructorServiceTests(SharedModuleStoreTestCase): ) assert result is None + # So technically, this mock publish is different than what is hit when running this code + # in production (lms.djangoapps.courseware.module_render.get_module_system_for_user.publish). + # I tried to figure out why or a way to force it to be more production like and was unsuccessful, + # so if anyone figures it out at any point, it would be greatly appreciated if you could update this. + # I thought it was acceptable because I'm still able to confirm correct behavior of the function + # and the attempted call, it is just going to the wrong publish spot ¯\_(ツ)_/¯ + @mock.patch('xmodule.x_module.DescriptorSystem.publish') + def test_complete_student_attempt_success(self, mock_publish): + """ + Assert complete_student_attempt correctly publishes completion for all + completable children of the given content_id + """ + # Section, subsection, and unit are all aggregators and not completable so should + # not be submitted. + section = ItemFactory.create(parent=self.course, category='chapter') + subsection = ItemFactory.create(parent=section, category='sequential') + unit = ItemFactory.create(parent=subsection, category='vertical') + + # should both be submitted + video = ItemFactory.create(parent=unit, category='video') + problem = ItemFactory.create(parent=unit, category='problem') + + # Not a completable block + ItemFactory.create(parent=unit, category='discussion') + + self.service.complete_student_attempt(self.student.username, str(subsection.location)) + # Only Completable leaf blocks should have completion published + assert mock_publish.call_count == 2 + + # The calls take the form of (xblock, publish_event, publish_data), which in our case + # will look like (xblock, 'completion', {'completion': 1.0, 'user_id': 1}). + # I'd prefer to be able to just assert all fields at once, but for some reason the value + # of the xblock in the mock call and the object above are different (even with a refetch) + # so I'm settling for just ensuring the location (block_key) of both are identical + video_call = mock_publish.call_args_list[0][0] + assert video_call[0].location == video.location + assert video_call[1] == 'completion' + assert video_call[2] == {'completion': 1.0, 'user_id': self.student.id} + problem_call = mock_publish.call_args_list[1][0] + assert problem_call[0].location == problem.location + assert problem_call[1] == 'completion' + assert problem_call[2] == {'completion': 1.0, 'user_id': self.student.id} + + @mock.patch('lms.djangoapps.instructor.services.log.error') + def test_complete_student_attempt_bad_user(self, mock_logger): + """ + 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) + mock_logger.assert_called_once_with( + self.complete_error_prefix.format(user=username, content_id=self.problem_urlname) + 'User does not exist!' + ) + + @mock.patch('lms.djangoapps.instructor.services.log.error') + def test_complete_student_attempt_bad_content_id(self, mock_logger): + """ + Assert complete_student_attempt with a bad content_id raises error and returns None + """ + username = self.student.username + self.service.complete_student_attempt(username, 'foo/bar/baz') + mock_logger.assert_called_once_with( + self.complete_error_prefix.format(user=username, content_id='foo/bar/baz') + 'Invalid content_id!' + ) + + @mock.patch('lms.djangoapps.instructor.services.log.error') + def test_complete_student_attempt_nonexisting_item(self, mock_logger): + """ + Assert complete_student_attempt with nonexisting item in the modulestore + raises error and returns None + """ + username = self.student.username + block = self.problem_urlname + 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!' + ) + def test_is_user_staff(self): """ Test to assert that the user is staff or not diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 81d809009a..b0cb895b58 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -455,7 +455,7 @@ edx-organizations==6.9.0 # via -r requirements/edx/base.in edx-proctoring-proctortrack==1.0.5 # via -r requirements/edx/base.in -edx-proctoring==3.13.2 +edx-proctoring==3.14.0 # via # -r requirements/edx/base.in # edx-proctoring-proctortrack diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 1e36bd5d41..a1be96207a 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -549,7 +549,7 @@ edx-organizations==6.9.0 # via -r requirements/edx/testing.txt edx-proctoring-proctortrack==1.0.5 # via -r requirements/edx/testing.txt -edx-proctoring==3.13.2 +edx-proctoring==3.14.0 # via # -r requirements/edx/testing.txt # edx-proctoring-proctortrack diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 65875fddc4..f77438c504 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -533,7 +533,7 @@ edx-organizations==6.9.0 # via -r requirements/edx/base.txt edx-proctoring-proctortrack==1.0.5 # via -r requirements/edx/base.txt -edx-proctoring==3.13.2 +edx-proctoring==3.14.0 # via # -r requirements/edx/base.txt # edx-proctoring-proctortrack