diff --git a/lms/djangoapps/instructor/services.py b/lms/djangoapps/instructor/services.py index 3a9e380c43..09150c73ba 100644 --- a/lms/djangoapps/instructor/services.py +++ b/lms/djangoapps/instructor/services.py @@ -12,10 +12,11 @@ from opaque_keys.edx.keys import CourseKey, UsageKey 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 lms.djangoapps.instructor.tasks import complete_student_attempt_task from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) @@ -44,15 +45,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 +76,26 @@ 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: str, content_id: str) -> None: + """ + Calls the complete_student_attempt_task + + 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 + 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 (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)) + def is_course_staff(self, user, course_id): """ Returns True if the user is the course staff diff --git a/lms/djangoapps/instructor/tasks.py b/lms/djangoapps/instructor/tasks.py new file mode 100644 index 0000000000..1ec98199ae --- /dev/null +++ b/lms/djangoapps/instructor/tasks.py @@ -0,0 +1,99 @@ +""" Celery Tasks for the Instructor App """ + +import logging + +from celery import shared_task +from celery_utils.logged_task import LoggedTask +from django.core.exceptions import ObjectDoesNotExist +from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import UsageKey +from xblock.completable import XBlockCompletionMode + +from common.djangoapps.student.models import get_user_by_username_or_email +from lms.djangoapps.courseware.model_data import FieldDataCache +from lms.djangoapps.courseware.module_render import get_module_for_descriptor +from openedx.core.lib.request_utils import get_request_or_stub +from xmodule.modulestore.django import modulestore +from xmodule.modulestore.exceptions import ItemNotFoundError + +log = logging.getLogger(__name__) + + +@shared_task(base=LoggedTask, ignore_result=True) +def complete_student_attempt_task(user_identifier: str, content_id: str) -> None: + """ + 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 + 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 (str): username or email of a user + content_id (str): 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_descriptor = 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 + + # This logic has been copied over from openedx/core/djangoapps/schedules/content_highlights.py + # in the _get_course_module function. + # I'm not sure if this is an anti-pattern or not, so if you can avoid re-copying this, please do. + # We are using it here because we ran into issues with the User service being undefined when we + # encountered a split_test xblock. + + # Fake a request to fool parts of the courseware that want to inspect it. + request = get_request_or_stub() + request.user = user + + # Now evil modulestore magic to inflate our descriptor with user state and + # permissions checks. + field_data_cache = FieldDataCache.cache_for_descriptor_descendents( + root_descriptor.course_id, user, root_descriptor, read_only=True, + ) + root_module = get_module_for_descriptor( + user, request, root_descriptor, field_data_cache, root_descriptor.course_id, + ) + if not root_module: + err_msg = err_msg_prefix + 'Module unable to be created from descriptor!' + 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. See also Split Test Module + # for another use case where user state has to be taken into account via get_child_descriptors + block_children = ((hasattr(block, 'get_child_descriptors') and block.get_child_descriptors()) + or (hasattr(block, 'get_children') and block.get_children()) + or []) + for child in block_children: + _submit_completions(child, user) + + _submit_completions(root_module, user) diff --git a/lms/djangoapps/instructor/tests/test_services.py b/lms/djangoapps/instructor/tests/test_services.py index c2161f90d6..0a52a1981b 100644 --- a/lms/djangoapps/instructor/tests/test_services.py +++ b/lms/djangoapps/instructor/tests/test_services.py @@ -5,7 +5,9 @@ import json from unittest import mock import pytest +from completion.waffle import ENABLE_COMPLETION_TRACKING_SWITCH from django.core.exceptions import ObjectDoesNotExist +from edx_toggles.toggles.testutils import override_waffle_switch from opaque_keys import InvalidKeyError from common.djangoapps.student.models import CourseEnrollment @@ -15,7 +17,8 @@ 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 +from xmodule.partitions.partitions import Group, UserPartition class InstructorServiceTests(SharedModuleStoreTestCase): @@ -38,6 +41,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 +118,128 @@ class InstructorServiceTests(SharedModuleStoreTestCase): ) assert result is None + @mock.patch('completion.handlers.BlockCompletion.objects.submit_completion') + def test_complete_student_attempt_success(self, mock_submit): + """ + 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') + + with override_waffle_switch(ENABLE_COMPLETION_TRACKING_SWITCH, True): + self.service.complete_student_attempt(self.student.username, str(subsection.location)) + + # Only Completable leaf blocks should have completion published + assert mock_submit.call_count == 2 + mock_submit.assert_any_call(user=self.student, block_key=video.location, completion=1.0) + mock_submit.assert_any_call(user=self.student, block_key=problem.location, completion=1.0) + + @mock.patch('completion.handlers.BlockCompletion.objects.submit_completion') + def test_complete_student_attempt_split_test(self, mock_submit): + """ + Asserts complete_student_attempt correctly publishes completion when a split test is involved + + This test case exists because we ran into a bug about the user_service not existing + when a split_test existed inside of a subsection. Associated with this change was adding + in the user state into the module before attempting completion and this ensures that is + working properly. + """ + partition = UserPartition( + 0, + 'first_partition', + 'First Partition', + [ + Group(0, 'alpha'), + Group(1, 'beta') + ] + ) + course = CourseFactory.create(user_partitions=[partition]) + section = ItemFactory.create(parent=course, category='chapter') + subsection = ItemFactory.create(parent=section, category='sequential') + + c0_url = course.id.make_usage_key('vertical', 'split_test_cond0') + c1_url = course.id.make_usage_key('vertical', 'split_test_cond1') + split_test = ItemFactory.create( + parent=subsection, + category='split_test', + user_partition_id=0, + group_id_to_child={'0': c0_url, '1': c1_url}, + ) + + cond0vert = ItemFactory.create(parent=split_test, category='vertical', location=c0_url) + ItemFactory.create(parent=cond0vert, category='video') + ItemFactory.create(parent=cond0vert, category='problem') + + cond1vert = ItemFactory.create(parent=split_test, category='vertical', location=c1_url) + ItemFactory.create(parent=cond1vert, category='video') + ItemFactory.create(parent=cond1vert, category='html') + + with override_waffle_switch(ENABLE_COMPLETION_TRACKING_SWITCH, True): + self.service.complete_student_attempt(self.student.username, str(subsection.location)) + + # Only the group the user was assigned to should have completion published. + # Either cond0vert's children or cond1vert's children + assert mock_submit.call_count == 2 + + @mock.patch('lms.djangoapps.instructor.tasks.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.tasks.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.tasks.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!' + ) + + @mock.patch('lms.djangoapps.instructor.tasks.log.error') + def test_complete_student_attempt_failed_module(self, mock_logger): + """ + Assert complete_student_attempt with failed get_module raises error and returns None + """ + username = self.student.username + with mock.patch('lms.djangoapps.instructor.tasks.get_module_for_descriptor', return_value=None): + self.service.complete_student_attempt(username, str(self.course.location)) + mock_logger.assert_called_once_with( + self.complete_error_prefix.format(user=username, content_id=self.course.location) + + 'Module unable to be created from descriptor!' + ) + 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 99f6b5408b..bb98de6b9b 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -86,9 +86,6 @@ 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 e6a560b688..88fc39a838 100644 --- a/requirements/edx/base.txt +++ b/requirements/edx/base.txt @@ -454,9 +454,8 @@ edx-opaque-keys[django]==2.2.1 # xmodule edx-organizations==6.9.0 # via -r requirements/edx/base.in -edx-proctoring==3.13.2 +edx-proctoring==3.14.0 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/base.in # edx-proctoring-proctortrack edx-proctoring-proctortrack==1.0.5 diff --git a/requirements/edx/development.txt b/requirements/edx/development.txt index 9325bb910b..018ba01d5f 100644 --- a/requirements/edx/development.txt +++ b/requirements/edx/development.txt @@ -542,9 +542,8 @@ edx-opaque-keys[django]==2.2.1 # xmodule edx-organizations==6.9.0 # via -r requirements/edx/testing.txt -edx-proctoring==3.13.2 +edx-proctoring==3.14.0 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/testing.txt # edx-proctoring-proctortrack edx-proctoring-proctortrack==1.0.5 diff --git a/requirements/edx/testing.txt b/requirements/edx/testing.txt index 4a512ddade..db6db89ada 100644 --- a/requirements/edx/testing.txt +++ b/requirements/edx/testing.txt @@ -528,9 +528,8 @@ edx-opaque-keys[django]==2.2.1 # xmodule edx-organizations==6.9.0 # via -r requirements/edx/base.txt -edx-proctoring==3.13.2 +edx-proctoring==3.14.0 # via - # -c requirements/edx/../constraints.txt # -r requirements/edx/base.txt # edx-proctoring-proctortrack edx-proctoring-proctortrack==1.0.5