diff --git a/lms/djangoapps/instructor/services.py b/lms/djangoapps/instructor/services.py index 9758037a88..3a9e380c43 100644 --- a/lms/djangoapps/instructor/services.py +++ b/lms/djangoapps/instructor/services.py @@ -9,16 +9,14 @@ 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__) @@ -46,12 +44,15 @@ class InstructorService: course_id = CourseKey.from_string(course_id) try: - student = get_user_by_username_or_email(student_identifier) + student = get_student_from_identifier(student_identifier) except ObjectDoesNotExist: err_msg = ( 'Error occurred while attempting to reset student attempts for user ' - f'{student_identifier} for content_id {content_id}. ' - 'User does not exist!' + '{student_identifier} for content_id {content_id}. ' + 'User does not exist!'.format( + student_identifier=student_identifier, + content_id=content_id + ) ) log.error(err_msg) return @@ -77,61 +78,13 @@ class InstructorService: except (StudentModule.DoesNotExist, enrollment.sub_api.SubmissionError): err_msg = ( 'Error occurred while attempting to reset student attempts for user ' - f'{student_identifier} for content_id {content_id}.' + '{student_identifier} for content_id {content_id}.'.format( + student_identifier=student_identifier, + 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 185db5d215..c2161f90d6 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, ItemFactory +from xmodule.modulestore.tests.factories import CourseFactory class InstructorServiceTests(SharedModuleStoreTestCase): @@ -38,8 +38,6 @@ 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() @@ -115,84 +113,6 @@ 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/constraints.txt b/requirements/constraints.txt index d4b9e7b584..d3dfc50509 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -86,6 +86,9 @@ social-auth-core<4.0.0 # social-auth-core>=4.0.0 requires PYJWT>=2.0.0 # celery requires click<8.0.0 which would be fixed once https://github.com/celery/celery/issues/6753 is done. click<8.0.0 +# https://openedx.atlassian.net/servicedesk/customer/portal/9/CR-3776 +edx-proctoring==3.13.2 + # constraints present due to Python35 support. Need to be tested and removed independently. celery<5.0 # celery 5.0 has dropped python3.5 support. diff --git a/requirements/edx/base.txt b/requirements/edx/base.txt index 785ee7ef61..ae4982fd00 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -457,7 +457,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.14.0 +edx-proctoring==3.13.2 # via # -r requirements/edx/base.in # edx-proctoring-proctortrack diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index aa7b342f04..6c8c2d41d4 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -547,7 +547,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.14.0 +edx-proctoring==3.13.2 # via # -r requirements/edx/testing.txt # edx-proctoring-proctortrack diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index bcd84f538a..35709defa9 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.14.0 +edx-proctoring==3.13.2 # via # -r requirements/edx/base.txt # edx-proctoring-proctortrack