diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index e60b72012ab628d0439ca55a90bd38f6f3d0735e..a4fd7da5e70fddcaf75f9b7edd6429110363a7d7 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -688,13 +688,11 @@ def get_module_system_for_user( # the result would always be "False". masquerade_settings = user.real_user.masquerade_settings del user.real_user.masquerade_settings - instructor_access = bool(has_access(user.real_user, 'instructor', descriptor, course_id)) user.real_user.masquerade_settings = masquerade_settings else: staff_access = has_access(user, 'staff', descriptor, course_id) - instructor_access = bool(has_access(user, 'instructor', descriptor, course_id)) if staff_access: - block_wrappers.append(partial(add_staff_markup, user, instructor_access, disable_staff_debug_info)) + block_wrappers.append(partial(add_staff_markup, user, disable_staff_debug_info)) # These modules store data using the anonymous_student_id as a key. # To prevent loss of data, we will continue to provide old modules with diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 1eb3abfe0e32f10aa7916a26ddc9276f451e4409..07e6b2e5fffd4c16e03627aaeb2834e2834e94ac 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/lms/djangoapps/instructor/tests/test_api.py @@ -205,6 +205,7 @@ INSTRUCTOR_POST_ENDPOINTS = set([ 'spent_registration_codes', 'students_update_enrollment', 'update_forum_role_membership', + 'override_problem_score', ]) @@ -386,11 +387,37 @@ class TestInstructorAPIDenyLevels(SharedModuleStoreTestCase, LoginEnrollmentTest def setUpClass(cls): super(TestInstructorAPIDenyLevels, cls).setUpClass() cls.course = CourseFactory.create() - cls.problem_location = msk_from_problem_urlname( - cls.course.id, - 'robot-some-problem-urlname' + cls.chapter = ItemFactory.create( + parent=cls.course, + category='chapter', + display_name="Chapter", + publish_item=True, + start=datetime.datetime(2018, 3, 10, tzinfo=UTC), ) - cls.problem_urlname = text_type(cls.problem_location) + cls.sequential = ItemFactory.create( + parent=cls.chapter, + category='sequential', + display_name="Lesson", + publish_item=True, + start=datetime.datetime(2018, 3, 10, tzinfo=UTC), + metadata={'graded': True, 'format': 'Homework'}, + ) + cls.vertical = ItemFactory.create( + parent=cls.sequential, + category='vertical', + display_name='Subsection', + publish_item=True, + start=datetime.datetime(2018, 3, 10, tzinfo=UTC), + ) + cls.problem = ItemFactory.create( + category="problem", + parent=cls.vertical, + display_name="A Problem Block", + weight=1, + publish_item=True, + ) + + cls.problem_urlname = text_type(cls.problem.location) BulkEmailFlag.objects.create(enabled=True, require_course_email_auth=False) @classmethod @@ -406,7 +433,7 @@ class TestInstructorAPIDenyLevels(SharedModuleStoreTestCase, LoginEnrollmentTest _module = StudentModule.objects.create( student=self.user, course_id=self.course.id, - module_state_key=self.problem_location, + module_state_key=self.problem.location, state=json.dumps({'attempts': 10}), ) @@ -417,8 +444,6 @@ class TestInstructorAPIDenyLevels(SharedModuleStoreTestCase, LoginEnrollmentTest ('get_grading_config', {}), ('get_students_features', {}), ('get_student_progress_url', {'unique_student_identifier': self.user.username}), - ('reset_student_attempts', - {'problem_to_reset': self.problem_urlname, 'unique_student_identifier': self.user.email}), ('update_forum_role_membership', {'unique_student_identifier': self.user.email, 'rolename': 'Moderator', 'action': 'allow'}), ('list_forum_members', {'rolename': FORUM_ROLE_COMMUNITY_TA}), @@ -435,15 +460,28 @@ class TestInstructorAPIDenyLevels(SharedModuleStoreTestCase, LoginEnrollmentTest ('get_proctored_exam_results', {}), ('get_problem_responses', {}), ('export_ora2_data', {}), - + ('rescore_problem', + {'problem_to_reset': self.problem_urlname, 'unique_student_identifier': self.user.email}), + ('override_problem_score', + {'problem_to_reset': self.problem_urlname, 'unique_student_identifier': self.user.email, 'score': 0}), + ('reset_student_attempts', + {'problem_to_reset': self.problem_urlname, 'unique_student_identifier': self.user.email}), + ( + 'reset_student_attempts', + { + 'problem_to_reset': self.problem_urlname, + 'unique_student_identifier': self.user.email, + 'delete_module': True + } + ), ] # Endpoints that only Instructors can access self.instructor_level_endpoints = [ ('bulk_beta_modify_access', {'identifiers': 'foo@example.org', 'action': 'add'}), ('modify_access', {'unique_student_identifier': self.user.email, 'rolename': 'beta', 'action': 'allow'}), ('list_course_role_members', {'rolename': 'beta'}), - ('rescore_problem', - {'problem_to_reset': self.problem_urlname, 'unique_student_identifier': self.user.email}), + ('rescore_problem', {'problem_to_reset': self.problem_urlname, 'all_students': True}), + ('reset_student_attempts', {'problem_to_reset': self.problem_urlname, 'all_students': True}), ] def _access_endpoint(self, endpoint, args, status_code, msg): @@ -569,10 +607,6 @@ class TestInstructorAPIDenyLevels(SharedModuleStoreTestCase, LoginEnrollmentTest for endpoint, args in self.instructor_level_endpoints: expected_status = 200 - - # TODO: make this work - if endpoint in ['rescore_problem']: - continue self._access_endpoint( endpoint, args, diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 1d1d5fbbe1c6124465a447787519e1ec730bb4e1..95adfcba7902ceb455dba401e9db38cb7ceddb7b 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/lms/djangoapps/instructor/views/api.py @@ -1907,13 +1907,16 @@ def reset_student_attempts(request, course_id): course = get_course_with_access( request.user, 'staff', course_id, depth=None ) + all_students = _get_boolean_param(request, 'all_students') + + if all_students and not has_access(request.user, 'instructor', course): + return HttpResponseForbidden("Requires instructor access.") problem_to_reset = strip_if_string(request.POST.get('problem_to_reset')) student_identifier = request.POST.get('unique_student_identifier', None) student = None if student_identifier is not None: student = get_student_from_identifier(student_identifier) - all_students = _get_boolean_param(request, 'all_students') delete_module = _get_boolean_param(request, 'delete_module') # parameter combinations @@ -1926,11 +1929,6 @@ def reset_student_attempts(request, course_id): "all_students and delete_module are mutually exclusive." ) - # instructor authorization - if all_students or delete_module: - if not has_access(request.user, 'instructor', course): - return HttpResponseForbidden("Requires instructor access.") - try: module_state_key = UsageKey.from_string(problem_to_reset).map_into_course(course_id) except InvalidKeyError: @@ -2044,13 +2042,13 @@ def reset_student_attempts_for_entrance_exam(request, course_id): # pylint: dis @require_POST @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_level('instructor') +@require_level('staff') @require_post_params(problem_to_reset="problem urlname to reset") @common_exceptions_400 def rescore_problem(request, course_id): """ Starts a background process a students attempts counter. Optionally deletes student state for a problem. - Limited to instructor access. + Rescore for all students is limited to instructor access. Takes either of the following query paremeters - problem_to_reset is a urlname of a problem @@ -2060,15 +2058,19 @@ def rescore_problem(request, course_id): all_students and unique_student_identifier cannot both be present. """ course_id = CourseKey.from_string(course_id) + course = get_course_with_access(request.user, 'staff', course_id) + all_students = _get_boolean_param(request, 'all_students') + + if all_students and not has_access(request.user, 'instructor', course): + return HttpResponseForbidden("Requires instructor access.") + + only_if_higher = _get_boolean_param(request, 'only_if_higher') problem_to_reset = strip_if_string(request.POST.get('problem_to_reset')) student_identifier = request.POST.get('unique_student_identifier', None) student = None if student_identifier is not None: student = get_student_from_identifier(student_identifier) - all_students = _get_boolean_param(request, 'all_students') - only_if_higher = _get_boolean_param(request, 'only_if_higher') - if not (problem_to_reset and (all_students or student)): return HttpResponseBadRequest("Missing query parameters.") @@ -2116,7 +2118,7 @@ def rescore_problem(request, course_id): @require_POST @ensure_csrf_cookie @cache_control(no_cache=True, no_store=True, must_revalidate=True) -@require_level('instructor') +@require_level('staff') @require_post_params(problem_to_reset="problem urlname to reset", score='overriding score') @common_exceptions_400 def override_problem_score(request, course_id): @@ -2142,7 +2144,7 @@ def override_problem_score(request, course_id): return _create_error_response(request, "Unable to parse problem id {}.".format(problem_to_reset)) # check the user's access to this specific problem - if not has_access(request.user, "instructor", modulestore().get_item(usage_key)): + if not has_access(request.user, "staff", modulestore().get_item(usage_key)): _create_error_response(request, "User {} does not have permission to override scores for problem {}.".format( request.user.id, problem_to_reset diff --git a/lms/templates/staff_problem_info.html b/lms/templates/staff_problem_info.html index 0085648ebdb98d3d93d5a6b3882293196fcabdc9..bd3b82c849b8e6e1f9eaa9320d05bf0c0ea224f2 100644 --- a/lms/templates/staff_problem_info.html +++ b/lms/templates/staff_problem_info.html @@ -80,7 +80,6 @@ ${block_content} <button type="button" class="btn-link staff-debug-reset">${_('Reset Learner\'s Attempts to Zero')}</button> | % endif - % if has_instructor_access: <button type="button" class="btn-link staff-debug-sdelete">${_('Delete Learner\'s State')}</button> % if can_rescore_problem: | @@ -92,7 +91,6 @@ ${block_content} % if can_override_problem_score: <button type="button" class="btn-link staff-debug-override-score">${_('Override Score')}</button> % endif - % endif ] </div> <div id="result_${location.block_id | h}"></div> diff --git a/openedx/core/lib/xblock_utils/__init__.py b/openedx/core/lib/xblock_utils/__init__.py index 3001b755e89daa1bf8b15c1ebae9158df07ca114..7de04b70bdff8a72ba58bc8ea5f77d281c638221 100644 --- a/openedx/core/lib/xblock_utils/__init__.py +++ b/openedx/core/lib/xblock_utils/__init__.py @@ -288,8 +288,8 @@ def sanitize_html_id(html_id): return sanitized_html_id -@contract(user=User, has_instructor_access=bool, block=XBlock, view=basestring, frag=Fragment, context="dict|None") -def add_staff_markup(user, has_instructor_access, disable_staff_debug_info, block, view, frag, context): # pylint: disable=unused-argument +@contract(user=User, block=XBlock, view=basestring, frag=Fragment, context="dict|None") +def add_staff_markup(user, disable_staff_debug_info, block, view, frag, context): # pylint: disable=unused-argument """ Updates the supplied module with a new get_html function that wraps the output of the old get_html function with additional information @@ -383,7 +383,6 @@ def add_staff_markup(user, has_instructor_access, disable_staff_debug_info, bloc 'render_histogram': render_histogram, 'block_content': frag.content, 'is_released': is_released, - 'has_instructor_access': has_instructor_access, 'can_reset_attempts': 'attempts' in block.fields, 'can_rescore_problem': hasattr(block, 'rescore'), 'can_override_problem_score': isinstance(block, ScorableXBlockMixin),