From 6da7f588ddf0c851f49ab2f3829417fb7aec1718 Mon Sep 17 00:00:00 2001 From: Kaustav Banerjee Date: Mon, 8 Aug 2022 21:08:36 +0530 Subject: [PATCH 01/14] feat: allow switching anonymous user ID hashing algorithm from shake to md5 The hashing algorithm has been changed in cd60646. However, there are Open edX operators who maintain backward compatibility of anonymous user IDs after past rotations of their Django secret key. For them, altering the hashing algorithm was a breaking change that made their analytics inconsistent. (cherry picked from commit 746e4fed1bfeb731b2f6288fe3ba4ea056397fdb) (cherry picked from commit ff6d92fd892dff5e66bbf77eadf9b91ed2189a24) (cherry picked from commit 7245bdc7f73d9693f15acc885bee4e0d00be8245) --- cms/envs/common.py | 11 +++++++++++ common/djangoapps/student/models/user.py | 13 +++++++++++-- common/djangoapps/student/tests/tests.py | 11 +++++++++++ lms/envs/common.py | 11 +++++++++++ 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index d5e561aa9cf9..1d7adfff9175 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -531,6 +531,17 @@ # .. toggle_creation_date: 2023-03-31 # .. toggle_tickets: https://github.com/openedx/edx-platform/pull/32015 'DISABLE_ADVANCED_SETTINGS': False, + + # .. toggle_name: FEATURES['ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Whether to enable the legacy MD5 hashing algorithm to generate anonymous user id + # instead of the newer SHAKE128 hashing algorithm + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2022-08-08 + # .. toggle_target_removal_date: None + # .. toggle_tickets: 'https://github.com/openedx/edx-platform/pull/30832' + 'ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID': False, } # .. toggle_name: ENABLE_COPPA_COMPLIANCE diff --git a/common/djangoapps/student/models/user.py b/common/djangoapps/student/models/user.py index 2a4a9deb3664..cb806fed4467 100644 --- a/common/djangoapps/student/models/user.py +++ b/common/djangoapps/student/models/user.py @@ -154,12 +154,21 @@ def anonymous_id_for_user(user, course_id): # function: Rotate at will, since the hashes are stored and # will not change. # include the secret key as a salt, and to make the ids unique across different LMS installs. - hasher = hashlib.shake_128() + legacy_hash_enabled = settings.FEATURES.get('ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID', False) + if legacy_hash_enabled: + # Use legacy MD5 algorithm if flag enabled + hasher = hashlib.md5() + else: + hasher = hashlib.shake_128() hasher.update(settings.SECRET_KEY.encode('utf8')) hasher.update(str(user.id).encode('utf8')) if course_id: hasher.update(str(course_id).encode('utf-8')) - anonymous_user_id = hasher.hexdigest(16) + + if legacy_hash_enabled: + anonymous_user_id = hasher.hexdigest() + else: + anonymous_user_id = hasher.hexdigest(16) # pylint: disable=too-many-function-args try: AnonymousUserId.objects.create( diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index b41ad2f856d6..735706304e11 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -1050,6 +1050,17 @@ def test_anonymous_id_secret_key_changes_result_in_diff_values_for_same_new_user assert anonymous_id != new_anonymous_id assert self.user == user_by_anonymous_id(new_anonymous_id) + def test_enable_legacy_hash_flag(self): + """Test that different anonymous id returned if ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID enabled.""" + CourseEnrollment.enroll(self.user, self.course.id) + anonymous_id = anonymous_id_for_user(self.user, self.course.id) + with patch.dict(settings.FEATURES, ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID=True): + # Recreate user object to clear cached anonymous id. + self.user = User.objects.get(pk=self.user.id) + AnonymousUserId.objects.filter(user=self.user).filter(course_id=self.course.id).delete() + new_anonymous_id = anonymous_id_for_user(self.user, self.course.id) + assert anonymous_id != new_anonymous_id + @skip_unless_lms @patch('openedx.core.djangoapps.programs.utils.get_programs') diff --git a/lms/envs/common.py b/lms/envs/common.py index 13e031c76175..f3e3604241da 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1039,6 +1039,17 @@ # .. toggle_creation_date: 2022-06-06 # .. toggle_tickets: 'https://github.com/edx/edx-platform/pull/29538' 'DISABLE_ALLOWED_ENROLLMENT_IF_ENROLLMENT_CLOSED': False, + + # .. toggle_name: FEATURES['ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Whether to enable the legacy MD5 hashing algorithm to generate anonymous user id + # instead of the newer SHAKE128 hashing algorithm + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2022-08-08 + # .. toggle_target_removal_date: None + # .. toggle_tickets: 'https://github.com/openedx/edx-platform/pull/30832' + 'ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID': False, } # Specifies extra XBlock fields that should available when requested via the Course Blocks API From 21571c6e20459142cd5cd79585925d3d3afd2ba9 Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Mon, 6 Feb 2023 14:53:15 +0530 Subject: [PATCH 02/14] feat: Add toggle to allow redirecting to courseware after enrollment. This change adds a new waffle switch to redirect a student to coursware after enrolment instead of the dashboard. (cherry picked from commit f494586b84f8dd8b3244d8e2180f47406b738372) --- common/djangoapps/student/tests/tests.py | 14 +++++++++++++ common/djangoapps/student/toggles.py | 20 ++++++++++++++++++- common/djangoapps/student/views/management.py | 7 +++++-- 3 files changed, 38 insertions(+), 3 deletions(-) diff --git a/common/djangoapps/student/tests/tests.py b/common/djangoapps/student/tests/tests.py index 735706304e11..8f952831f12b 100644 --- a/common/djangoapps/student/tests/tests.py +++ b/common/djangoapps/student/tests/tests.py @@ -15,6 +15,7 @@ from django.test import TestCase, override_settings from django.test.client import Client from django.urls import reverse +from edx_toggles.toggles.testutils import override_waffle_switch from markupsafe import escape from opaque_keys.edx.keys import CourseKey from opaque_keys.edx.locations import CourseLocator @@ -33,6 +34,7 @@ user_by_anonymous_id ) from common.djangoapps.student.tests.factories import CourseEnrollmentFactory, UserFactory +from common.djangoapps.student.toggles import REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT from common.djangoapps.student.views import complete_course_mode_info from common.djangoapps.util.model_utils import USER_SETTINGS_CHANGED_EVENT_NAME from common.djangoapps.util.testing import EventTestMixin @@ -893,6 +895,7 @@ def test_change_enrollment_modes(self): @skip_unless_lms +@ddt.ddt class ChangeEnrollmentViewTest(ModuleStoreTestCase): """Tests the student.views.change_enrollment view""" @@ -913,6 +916,17 @@ def _enroll_through_view(self, course): ) return response + @ddt.data( + (True, 'courseware'), + (False, None), + ) + @ddt.unpack + def test_enrollment_url(self, waffle_flag_enabled, returned_view): + with override_waffle_switch(REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT, waffle_flag_enabled): + response = self._enroll_through_view(self.course) + data = reverse(returned_view, args=[str(self.course.id)]) if returned_view else '' + assert response.content.decode('utf8') == data + def test_enroll_as_default(self): """Tests that a student can successfully enroll through this view""" response = self._enroll_through_view(self.course) diff --git a/common/djangoapps/student/toggles.py b/common/djangoapps/student/toggles.py index 46e9b0408fbf..507f323a2a53 100644 --- a/common/djangoapps/student/toggles.py +++ b/common/djangoapps/student/toggles.py @@ -1,7 +1,7 @@ """ Toggles for Dashboard page. """ -from edx_toggles.toggles import WaffleFlag +from edx_toggles.toggles import WaffleFlag, WaffleSwitch # Namespace for student waffle flags. WAFFLE_FLAG_NAMESPACE = 'student' @@ -75,3 +75,21 @@ def should_show_2u_recommendations(): def should_send_enrollment_email(): return ENROLLMENT_CONFIRMATION_EMAIL.is_enabled() + + +# Waffle flag to enable control redirecting after enrolment. +# .. toggle_name: student.redirect_to_courseware_after_enrollment +# .. toggle_implementation: WaffleSwitch +# .. toggle_default: False +# .. toggle_description: Redirect to courseware after enrollment instead of dashboard. +# .. toggle_use_cases: open_edx +# .. toggle_creation_date: 2023-02-06 +# .. toggle_target_removal_date: None +# .. toggle_warning: None +REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT = WaffleSwitch( + f'{WAFFLE_FLAG_NAMESPACE}.redirect_to_courseware_after_enrollment', __name__ +) + + +def should_redirect_to_courseware_after_enrollment(): + return REDIRECT_TO_COURSEWARE_AFTER_ENROLLMENT.is_enabled() diff --git a/common/djangoapps/student/views/management.py b/common/djangoapps/student/views/management.py index d131575193b4..cb3df1627fd9 100644 --- a/common/djangoapps/student/views/management.py +++ b/common/djangoapps/student/views/management.py @@ -38,6 +38,7 @@ from rest_framework.decorators import api_view, authentication_classes, permission_classes from rest_framework.permissions import IsAuthenticated +from common.djangoapps.student.toggles import should_redirect_to_courseware_after_enrollment from common.djangoapps.track import views as track_views from lms.djangoapps.bulk_email.models import Optout from common.djangoapps.course_modes.models import CourseMode @@ -400,8 +401,10 @@ def change_enrollment(request, check_access=True): reverse("course_modes_choose", kwargs={'course_id': str(course_id)}) ) - # Otherwise, there is only one mode available (the default) - return HttpResponse() + if should_redirect_to_courseware_after_enrollment(): + return HttpResponse(reverse('courseware', args=[str(course_id)])) + else: + return HttpResponse() elif action == "unenroll": if configuration_helpers.get_value( "DISABLE_UNENROLLMENT", From d468a8c090d7a9ca29039879bc1890753734c741 Mon Sep 17 00:00:00 2001 From: Pooja Kulkarni Date: Fri, 17 Feb 2023 11:42:26 -0500 Subject: [PATCH 03/14] feat: add new endpoint for cloning course (cherry picked from commit 79c4b4a4bbb067c07b30980d9bd11ceb3f4e9af2) --- .../api/v1/serializers/course_runs.py | 33 ++++++++++++ .../v1/tests/test_views/test_course_runs.py | 51 +++++++++++++++++++ cms/djangoapps/api/v1/views/course_runs.py | 8 +++ 3 files changed, 92 insertions(+) diff --git a/cms/djangoapps/api/v1/serializers/course_runs.py b/cms/djangoapps/api/v1/serializers/course_runs.py index cbd4d09e2181..6bbbce96dd42 100644 --- a/cms/djangoapps/api/v1/serializers/course_runs.py +++ b/cms/djangoapps/api/v1/serializers/course_runs.py @@ -5,6 +5,7 @@ from django.db import transaction from django.utils.translation import gettext_lazy as _ from opaque_keys import InvalidKeyError +from opaque_keys.edx.keys import CourseKey from rest_framework import serializers from rest_framework.fields import empty @@ -203,3 +204,35 @@ def update(self, instance, validated_data): course_run = get_course_and_check_access(new_course_run_key, user) self.update_team(course_run, team) return course_run + + +class CourseCloneSerializer(serializers.Serializer): # lint-amnesty, pylint: disable=abstract-method, missing-class-docstring + source_course_id = serializers.CharField() + destination_course_id = serializers.CharField() + + def validate(self, attrs): + source_course_id = attrs.get('source_course_id') + destination_course_id = attrs.get('destination_course_id') + store = modulestore() + source_key = CourseKey.from_string(source_course_id) + dest_key = CourseKey.from_string(destination_course_id) + + # Check if the source course exists + if not store.has_course(source_key): + raise serializers.ValidationError('Source course does not exist.') + + # Check if the destination course already exists + if store.has_course(dest_key): + raise serializers.ValidationError('Destination course already exists.') + return attrs + + def create(self, validated_data): + source_course_id = validated_data.get('source_course_id') + destination_course_id = validated_data.get('destination_course_id') + user_id = self.context['request'].user.id + store = modulestore() + source_key = CourseKey.from_string(source_course_id) + dest_key = CourseKey.from_string(destination_course_id) + with store.default_store('split'): + new_course = store.clone_course(source_key, dest_key, user_id) + return new_course diff --git a/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py b/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py index 49589a473878..8366ef72941e 100644 --- a/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py +++ b/cms/djangoapps/api/v1/tests/test_views/test_course_runs.py @@ -402,3 +402,54 @@ def test_rerun_invalid_number(self): assert response.data == {'non_field_errors': [ 'Invalid key supplied. Ensure there are no special characters in the Course Number.' ]} + + def test_clone_course(self): + course = CourseFactory() + url = reverse('api:v1:course_run-clone') + data = { + 'source_course_id': str(course.id), + 'destination_course_id': 'course-v1:destination+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 201 + self.assertEqual(response.data, {"message": "Course cloned successfully."}) + + def test_clone_course_with_missing_source_id(self): + url = reverse('api:v1:course_run-clone') + data = { + 'destination_course_id': 'course-v1:destination+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + self.assertEqual(response.data, {'source_course_id': ['This field is required.']}) + + def test_clone_course_with_missing_dest_id(self): + url = reverse('api:v1:course_run-clone') + data = { + 'source_course_id': 'course-v1:source+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + self.assertEqual(response.data, {'destination_course_id': ['This field is required.']}) + + def test_clone_course_with_nonexistent_source_course(self): + url = reverse('api:v1:course_run-clone') + data = { + 'source_course_id': 'course-v1:nonexistent+source+course_id', + 'destination_course_id': 'course-v1:destination+course+id', + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + assert str(response.data.get('non_field_errors')[0]) == 'Source course does not exist.' + + def test_clone_course_with_existing_dest_course(self): + url = reverse('api:v1:course_run-clone') + course = CourseFactory() + existing_dest_course = CourseFactory() + data = { + 'source_course_id': str(course.id), + 'destination_course_id': str(existing_dest_course.id), + } + response = self.client.post(url, data, format='json') + assert response.status_code == 400 + assert str(response.data.get('non_field_errors')[0]) == 'Destination course already exists.' diff --git a/cms/djangoapps/api/v1/views/course_runs.py b/cms/djangoapps/api/v1/views/course_runs.py index a0415d4e06dc..fb1671ebef04 100644 --- a/cms/djangoapps/api/v1/views/course_runs.py +++ b/cms/djangoapps/api/v1/views/course_runs.py @@ -13,6 +13,7 @@ from cms.djangoapps.contentstore.views.course import _accessible_courses_iter, get_course_and_check_access from ..serializers.course_runs import ( + CourseCloneSerializer, CourseRunCreateSerializer, CourseRunImageSerializer, CourseRunRerunSerializer, @@ -93,3 +94,10 @@ def rerun(self, request, *args, **kwargs): # lint-amnesty, pylint: disable=miss new_course_run = serializer.save() serializer = self.get_serializer(new_course_run) return Response(serializer.data, status=status.HTTP_201_CREATED) + + @action(detail=False, methods=['post']) + def clone(self, request, *args, **kwargs): # lint-amnesty, pylint: disable=missing-function-docstring, unused-argument + serializer = CourseCloneSerializer(data=request.data, context=self.get_serializer_context()) + serializer.is_valid(raise_exception=True) + serializer.save() + return Response({"message": "Course cloned successfully."}, status=status.HTTP_201_CREATED) From c596bf3b100bcc629602f0c371663da2c0b862ac Mon Sep 17 00:00:00 2001 From: Kshitij Sobti Date: Wed, 24 May 2023 13:58:55 +0530 Subject: [PATCH 04/14] temp: Add configuration option to redirect to external site when TAP account is unlinked (cherry picked from commit e83a8c8f82849644cf95534cde3fe149e4f11916) (cherry picked from commit 0c831dc390f63ea9259668ec400df9c4bccd95c8) --- lms/static/js/student_account/views/LoginView.js | 3 +++ openedx/core/djangoapps/user_authn/views/login.py | 10 ++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/lms/static/js/student_account/views/LoginView.js b/lms/static/js/student_account/views/LoginView.js index 5832f2c088be..9b7ce2e19908 100644 --- a/lms/static/js/student_account/views/LoginView.js +++ b/lms/static/js/student_account/views/LoginView.js @@ -216,6 +216,7 @@ saveError: function(error) { var errorCode; var msg; + var redirectURL; if (error.status === 0) { msg = gettext('An error has occurred. Check your Internet connection and try again.'); } else if (error.status === 500) { @@ -245,6 +246,7 @@ } else if (error.responseJSON !== undefined) { msg = error.responseJSON.value; errorCode = error.responseJSON.error_code; + redirectURL = error.responseJSON.redirect_url; } else { msg = gettext('An unexpected error has occurred.'); } @@ -266,6 +268,7 @@ this.clearFormErrors(); this.renderThirdPartyAuthWarning(); } + window.location.href = redirectURL; } else { this.renderErrors(this.defaultFormErrorsTitle, this.errors); } diff --git a/openedx/core/djangoapps/user_authn/views/login.py b/openedx/core/djangoapps/user_authn/views/login.py index 4880e58a433c..de90770db036 100644 --- a/openedx/core/djangoapps/user_authn/views/login.py +++ b/openedx/core/djangoapps/user_authn/views/login.py @@ -76,7 +76,7 @@ def _do_third_party_auth(request): try: return pipeline.get_authenticated_user(requested_provider, username, third_party_uid) - except USER_MODEL.DoesNotExist: + except USER_MODEL.DoesNotExist as err: AUDIT_LOG.info( "Login failed - user with username {username} has no social auth " "with backend_name {backend_name}".format( @@ -98,7 +98,13 @@ def _do_third_party_auth(request): ) ) - raise AuthFailedError(message, error_code='third-party-auth-with-no-linked-account') # lint-amnesty, pylint: disable=raise-missing-from + redirect_url = configuration_helpers.get_value('OC_REDIRECT_ON_TPA_UNLINKED_ACCOUNT', None) + + raise AuthFailedError( + message, + error_code='third-party-auth-with-no-linked-account', + redirect_url=redirect_url + ) from err def _get_user_by_email(email): From 50da90148dbdac6dd1ddf923a393ff30407e094d Mon Sep 17 00:00:00 2001 From: Kaustav Banerjee Date: Mon, 12 Jun 2023 13:37:20 +0530 Subject: [PATCH 05/14] feat: default grade designations configurable from settings (#541) (cherry picked from commit 7a7af8cdc16416ec8e3289f9482e734b58145b0e) (cherry picked from commit 94754317c3f8709b7cd8980cbf2f9e5181494fb0) (cherry picked from commit 3ca85eac1228f281212b306f3e7ddfded96f7dd8) --- cms/djangoapps/contentstore/utils.py | 3 ++- cms/envs/common.py | 8 ++++++++ cms/static/js/factories/settings_graders.js | 5 +++-- cms/static/js/views/settings/grading.js | 7 +++++-- cms/static/sass/views/_settings.scss | 10 +++++----- cms/templates/settings_graders.html | 1 + 6 files changed, 24 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 1a4b709622e6..e392a32f3ad6 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -1372,7 +1372,8 @@ def get_course_grading(course_key): 'grading_url': reverse_course_url('grading_handler', course_key), 'is_credit_course': is_credit_course(course_key), 'mfe_proctored_exam_settings_url': get_proctored_exam_settings_url(course_key), - 'course_assignment_lists': dict(course_assignment_lists) + 'course_assignment_lists': dict(course_assignment_lists), + 'default_grade_designations': settings.DEFAULT_GRADE_DESIGNATIONS } return grading_context diff --git a/cms/envs/common.py b/cms/envs/common.py index 1d7adfff9175..10ec4b3aa084 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -2377,6 +2377,14 @@ # Rate limit for regrading tasks that a grading policy change can kick off POLICY_CHANGE_TASK_RATE_LIMIT = '900/h' +# .. setting_name: DEFAULT_GRADE_DESIGNATIONS +# .. setting_default: ['A', 'B', 'C', 'D'] +# .. setting_description: The default 'pass' grade cutoff designations to be used. The failure grade +# is always 'F' and should not be included in this list. +# .. setting_warning: The DEFAULT_GRADE_DESIGNATIONS list must have more than one designation, +# or else ['A', 'B', 'C', 'D'] will be used as the default grade designations. +DEFAULT_GRADE_DESIGNATIONS = ['A', 'B', 'C', 'D'] + ############## Settings for CourseGraph ############################ # .. setting_name: COURSEGRAPH_JOB_QUEUE diff --git a/cms/static/js/factories/settings_graders.js b/cms/static/js/factories/settings_graders.js index dc75029e0f26..57e811b7ed3f 100644 --- a/cms/static/js/factories/settings_graders.js +++ b/cms/static/js/factories/settings_graders.js @@ -3,7 +3,7 @@ define([ ], function($, GradingView, CourseGradingPolicyModel) { 'use strict'; - return function(courseDetails, gradingUrl, courseAssignmentLists) { + return function(courseDetails, gradingUrl, gradeDesignations, courseAssignmentLists) { var model, editor; $('form :input') @@ -19,7 +19,8 @@ define([ editor = new GradingView({ el: $('.settings-grading'), model: model, - courseAssignmentLists: courseAssignmentLists + courseAssignmentLists: courseAssignmentLists, + gradeDesignations: gradeDesignations }); editor.render(); }; diff --git a/cms/static/js/views/settings/grading.js b/cms/static/js/views/settings/grading.js index ac4d170352a2..3d383ea457af 100644 --- a/cms/static/js/views/settings/grading.js +++ b/cms/static/js/views/settings/grading.js @@ -34,6 +34,7 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { $('#course_grade_cutoff-tpl').text() ); this.setupCutoffs(); + this.setupGradeDesignations(options.gradeDesignations); this.listenTo(this.model, 'invalid', this.handleValidationError); this.listenTo(this.model, 'change', this.showNotificationBar); @@ -318,7 +319,7 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { addNewGrade: function(e) { e.preventDefault(); var gradeLength = this.descendingCutoffs.length; // cutoffs doesn't include fail/f so this is only the passing grades - if (gradeLength > 3) { + if (gradeLength > this.GRADES.length - 1) { // TODO shouldn't we disable the button return; } @@ -399,7 +400,9 @@ function(ValidatingView, _, $, ui, GraderView, StringUtils, HtmlUtils) { this.descendingCutoffs = _.sortBy(this.descendingCutoffs, function(gradeEle) { return -gradeEle.cutoff; }); }, - revertView: function() { + setupGradeDesignations: function(gradeDesignations) { + if (Array.isArray(gradeDesignations) && gradeDesignations.length > 1) { this.GRADES = gradeDesignations; } + },revertView: function() { var self = this; this.model.fetch({ success: function() { diff --git a/cms/static/sass/views/_settings.scss b/cms/static/sass/views/_settings.scss index cc62fd436f0d..c571af4b8cb8 100644 --- a/cms/static/sass/views/_settings.scss +++ b/cms/static/sass/views/_settings.scss @@ -777,23 +777,23 @@ height: 17px; } - &:nth-child(1) { + &:nth-child(5n+1) { background: #4fe696; } - &:nth-child(2) { + &:nth-child(5n+2) { background: #ffdf7e; } - &:nth-child(3) { + &:nth-child(5n+3) { background: #ffb657; } - &:nth-child(4) { + &:nth-child(5n+4) { background: #ef54a1; } - &:nth-child(5), + &:nth-child(5n+5), &.bar-fail { background: #fb336c; } diff --git a/cms/templates/settings_graders.html b/cms/templates/settings_graders.html index c3b6f8f73a2a..1c8f0019bf3e 100644 --- a/cms/templates/settings_graders.html +++ b/cms/templates/settings_graders.html @@ -36,6 +36,7 @@ { is_credit_course: ${is_credit_course | n, dump_js_escaped_json} } ), "${grading_url | n, js_escaped_string}", + ${default_grade_designations | n, dump_js_escaped_json}, ${course_assignment_lists | n, dump_js_escaped_json}, ); }); From 6de7b64ae477bf882c1ae3a97ef2e8940e773a57 Mon Sep 17 00:00:00 2001 From: 0x29a Date: Mon, 26 Jun 2023 11:08:26 +0200 Subject: [PATCH 06/14] fix: give superusers all studio permissions (cherry picked from commit 8ef55754f4a529cc6b784298320fcdb8b415bd83) (cherry picked from commit 8e281a9535b8317e24f8d2b3b5d840a3eb852865) (cherry picked from commit f55297379276ddcdfd140b43bdc54d19128744d9) --- common/djangoapps/student/roles.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index c2fd4564e420..fe2b6c91d689 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -136,7 +136,7 @@ class GlobalStaff(AccessRole): The global staff role """ def has_user(self, user): - return bool(user and user.is_staff) + return bool(user and (user.is_superuser or user.is_staff)) def add_users(self, *users): for user in users: From 05676083ce9d2d1c59200145de7d88ee615bce93 Mon Sep 17 00:00:00 2001 From: Navin Karkera Date: Tue, 10 Oct 2023 21:03:23 +0530 Subject: [PATCH 07/14] feat: allow oauth configuration per site and backend (#32656) Allows admins to configure same oauth backend for multiple sites. This change includes site_id in KEY_FIELDS for oauth configuration provider allowing a backend configuration for each site. (cherry picked from commit 565b34e4e0904d1a55e56db403893a207c62e4c3) --- common/djangoapps/third_party_auth/models.py | 30 +++++++-- .../djangoapps/third_party_auth/pipeline.py | 4 +- .../third_party_auth/tests/test_provider.py | 62 ++++++++++++++++--- common/djangoapps/third_party_auth/views.py | 2 +- 4 files changed, 82 insertions(+), 16 deletions(-) diff --git a/common/djangoapps/third_party_auth/models.py b/common/djangoapps/third_party_auth/models.py index af5159764c92..1ea265e868f2 100644 --- a/common/djangoapps/third_party_auth/models.py +++ b/common/djangoapps/third_party_auth/models.py @@ -366,13 +366,12 @@ class OAuth2ProviderConfig(ProviderConfig): .. no_pii: """ - # We are keying the provider config by backend_name here as suggested in the python social - # auth documentation. In order to reuse a backend for a second provider, a subclass can be - # created with seperate name. + # We are keying the provider config by backend_name and site_id to support configuration per site. + # In order to reuse a backend for a second provider, a subclass can be created with seperate name. # example: # class SecondOpenIDProvider(OpenIDAuth): # name = "second-openId-provider" - KEY_FIELDS = ('backend_name',) + KEY_FIELDS = ('site_id', 'backend_name') prefix = 'oa2' backend_name = models.CharField( max_length=50, blank=False, db_index=True, @@ -401,6 +400,29 @@ class Meta: verbose_name = "Provider Configuration (OAuth)" verbose_name_plural = verbose_name + @classmethod + def current(cls, *args): + """ + Get the current config model for the provider according to the given backend and the current + site. + """ + site_id = Site.objects.get_current(get_current_request()).id + return super(OAuth2ProviderConfig, cls).current(site_id, *args) + + @property + def provider_id(self): + """ + Unique string key identifying this provider. Must be URL and css class friendly. + Ignoring site_id as the config is filtered using current method which fetches the configuration for the current + site_id. + """ + assert self.prefix is not None + return "-".join((self.prefix, ) + tuple( + str(getattr(self, field)) + for field in self.KEY_FIELDS + if field != 'site_id' + )) + def clean(self): """ Standardize and validate fields """ super().clean() diff --git a/common/djangoapps/third_party_auth/pipeline.py b/common/djangoapps/third_party_auth/pipeline.py index 9135ea556bea..fde2bb9cbc0c 100644 --- a/common/djangoapps/third_party_auth/pipeline.py +++ b/common/djangoapps/third_party_auth/pipeline.py @@ -854,7 +854,7 @@ def user_details_force_sync(auth_entry, strategy, details, user=None, *args, **k This step is controlled by the `sync_learner_profile_data` flag on the provider's configuration. """ current_provider = provider.Registry.get_from_pipeline({'backend': strategy.request.backend.name, 'kwargs': kwargs}) - if user and current_provider.sync_learner_profile_data: + if user and current_provider and current_provider.sync_learner_profile_data: # Keep track of which incoming values get applied. changed = {} @@ -931,7 +931,7 @@ def set_id_verification_status(auth_entry, strategy, details, user=None, *args, Use the user's authentication with the provider, if configured, as evidence of their identity being verified. """ current_provider = provider.Registry.get_from_pipeline({'backend': strategy.request.backend.name, 'kwargs': kwargs}) - if user and current_provider.enable_sso_id_verification: + if user and current_provider and current_provider.enable_sso_id_verification: # Get previous valid, non expired verification attempts for this SSO Provider and user verifications = SSOVerification.objects.filter( user=user, diff --git a/common/djangoapps/third_party_auth/tests/test_provider.py b/common/djangoapps/third_party_auth/tests/test_provider.py index 3fa8f80f4d1a..28e95c16b8f9 100644 --- a/common/djangoapps/third_party_auth/tests/test_provider.py +++ b/common/djangoapps/third_party_auth/tests/test_provider.py @@ -11,7 +11,9 @@ from common.djangoapps.third_party_auth import provider from common.djangoapps.third_party_auth.tests import testutil from common.djangoapps.third_party_auth.tests.utils import skip_unless_thirdpartyauth -from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration +from openedx.core.djangoapps.site_configuration.tests.test_util import ( + with_site_configuration, with_site_configuration_context +) SITE_DOMAIN_A = 'professionalx.example.com' SITE_DOMAIN_B = 'somethingelse.example.com' @@ -114,13 +116,13 @@ def test_providers_displayed_for_login(self): assert no_log_in_provider.provider_id not in provider_ids assert normal_provider.provider_id in provider_ids - def test_tpa_hint_provider_displayed_for_login(self): + def test_tpa_hint_exp_hidden_provider_displayed_for_login(self): """ - Tests to ensure that an enabled-but-not-visible provider is presented + Test to ensure that an explicitly enabled-but-not-visible provider is presented for use in the UI when the "tpa_hint" parameter is specified + A hidden provider should be accessible with tpa_hint (this is the main case) """ - # A hidden provider should be accessible with tpa_hint (this is the main case) hidden_provider = self.configure_google_provider(visible=False, enabled=True) provider_ids = [ idp.provider_id @@ -128,8 +130,14 @@ def test_tpa_hint_provider_displayed_for_login(self): ] assert hidden_provider.provider_id in provider_ids - # New providers are hidden (ie, not flagged as 'visible') by default - # The tpa_hint parameter should work for these providers as well + def test_tpa_hint_hidden_provider_displayed_for_login(self): + """ + Tests to ensure that an implicitly enabled-but-not-visible provider is presented + for use in the UI when the "tpa_hint" parameter is specified. + New providers are hidden (ie, not flagged as 'visible') by default + The tpa_hint parameter should work for these providers as well. + """ + implicitly_hidden_provider = self.configure_linkedin_provider(enabled=True) provider_ids = [ idp.provider_id @@ -137,7 +145,10 @@ def test_tpa_hint_provider_displayed_for_login(self): ] assert implicitly_hidden_provider.provider_id in provider_ids - # Disabled providers should not be matched in tpa_hint scenarios + def test_tpa_hint_disabled_hidden_provider_displayed_for_login(self): + """ + Disabled providers should not be matched in tpa_hint scenarios + """ disabled_provider = self.configure_twitter_provider(visible=True, enabled=False) provider_ids = [ idp.provider_id @@ -145,7 +156,10 @@ def test_tpa_hint_provider_displayed_for_login(self): ] assert disabled_provider.provider_id not in provider_ids - # Providers not utilized for learner authentication should not match tpa_hint + def test_tpa_hint_no_log_hidden_provider_displayed_for_login(self): + """ + Providers not utilized for learner authentication should not match tpa_hint + """ no_log_in_provider = self.configure_lti_provider() provider_ids = [ idp.provider_id @@ -153,6 +167,30 @@ def test_tpa_hint_provider_displayed_for_login(self): ] assert no_log_in_provider.provider_id not in provider_ids + def test_get_current_site_oauth_provider(self): + """ + Verify that correct provider for current site is returned even if same backend is used for multiple sites. + """ + site_a = Site.objects.get_or_create(domain=SITE_DOMAIN_A, name=SITE_DOMAIN_A)[0] + site_b = Site.objects.get_or_create(domain=SITE_DOMAIN_B, name=SITE_DOMAIN_B)[0] + site_a_provider = self.configure_google_provider(visible=True, enabled=True, site=site_a) + site_b_provider = self.configure_google_provider(visible=True, enabled=True, site=site_b) + with with_site_configuration_context(domain=SITE_DOMAIN_A): + assert site_a_provider.enabled_for_current_site is True + + # Registry.displayed_for_login gets providers enabled for current site + provider_ids = provider.Registry.displayed_for_login() + # Google oauth provider for current site should be displayed + assert site_a_provider in provider_ids + assert site_b_provider not in provider_ids + + # Similarly, the other site should only see its own providers + with with_site_configuration_context(domain=SITE_DOMAIN_B): + assert site_b_provider.enabled_for_current_site is True + provider_ids = provider.Registry.displayed_for_login() + assert site_b_provider in provider_ids + assert site_a_provider not in provider_ids + def test_provider_enabled_for_current_site(self): """ Verify that enabled_for_current_site returns True when the provider matches the current site. @@ -201,7 +239,7 @@ def test_oauth2_enabled_only_for_supplied_backend(self): def test_get_returns_none_if_provider_id_is_none(self): assert provider.Registry.get(None) is None - def test_get_returns_none_if_provider_not_enabled(self): + def test_get_returns_none_if_provider_not_enabled_change(self): linkedin_provider_id = "oa2-linkedin-oauth2" # At this point there should be no configuration entries at all so no providers should be enabled assert provider.Registry.enabled() == [] @@ -209,6 +247,12 @@ def test_get_returns_none_if_provider_not_enabled(self): # Now explicitly disabled this provider: self.configure_linkedin_provider(enabled=False) assert provider.Registry.get(linkedin_provider_id) is None + + def test_get_returns_provider_if_provider_enabled(self): + """ + Test to ensure that Registry gets enabled providers. + """ + linkedin_provider_id = "oa2-linkedin-oauth2" self.configure_linkedin_provider(enabled=True) assert provider.Registry.get(linkedin_provider_id).provider_id == linkedin_provider_id diff --git a/common/djangoapps/third_party_auth/views.py b/common/djangoapps/third_party_auth/views.py index c6a406c5301c..d24ba8cfd7db 100644 --- a/common/djangoapps/third_party_auth/views.py +++ b/common/djangoapps/third_party_auth/views.py @@ -47,7 +47,7 @@ def inactive_user_view(request): if third_party_auth.is_enabled() and pipeline.running(request): running_pipeline = pipeline.get(request) third_party_provider = provider.Registry.get_from_pipeline(running_pipeline) - if third_party_provider.skip_email_verification and not activated: + if third_party_provider and third_party_provider.skip_email_verification and not activated: user.is_active = True user.save() activated = True From a21b4f0da1d6eb5240d15aa8b50fd77454b47167 Mon Sep 17 00:00:00 2001 From: 0x29a Date: Thu, 27 Jul 2023 14:31:43 +0200 Subject: [PATCH 08/14] feat: eSHE Instructor role Adds the eSHE Instructor role, which inherits Course Staff permissions, but isn't able to enroll / un-enroll students and can't assing course team roles unless in combination with Course Staff / Instructor / Discussion admin roles. (cherry picked from commit 5d160c2ecd91e58276dccfd84ca32fb689f46e5e) --- common/djangoapps/student/roles.py | 28 ++++++++-- common/djangoapps/student/tests/test_roles.py | 2 + lms/djangoapps/courseware/rules.py | 8 ++- lms/djangoapps/instructor/access.py | 2 + lms/djangoapps/instructor/permissions.py | 8 ++- .../tests/views/test_instructor_dashboard.py | 52 +++++++++++++++++++ .../instructor/views/instructor_dashboard.py | 18 ++++++- lms/envs/common.py | 10 ++++ .../instructor_dashboard/membership.html | 1 + .../instructor_dashboard_2/membership.html | 17 ++++++ 10 files changed, 137 insertions(+), 9 deletions(-) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index fe2b6c91d689..ff6be033f9f5 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -7,6 +7,7 @@ import logging from abc import ABCMeta, abstractmethod from collections import defaultdict +from contextlib import contextmanager from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from opaque_keys.edx.django.models import CourseKeyField @@ -44,6 +45,17 @@ def register_access_role(cls): return cls +@contextmanager +def strict_role_checking(): + """ + Context manager that temporarily disables role inheritance. + """ + OLD_ACCESS_ROLES_INHERITANCE = ACCESS_ROLES_INHERITANCE.copy() + ACCESS_ROLES_INHERITANCE.clear() + yield + ACCESS_ROLES_INHERITANCE.update(OLD_ACCESS_ROLES_INHERITANCE) + + class BulkRoleCache: # lint-amnesty, pylint: disable=missing-class-docstring CACHE_NAMESPACE = "student.roles.BulkRoleCache" CACHE_KEY = 'roles_by_user' @@ -78,7 +90,7 @@ def __init__(self, user): ) @staticmethod - def _get_roles(role): + def get_roles(role): """ Return the roles that should have the same permissions as the specified role. """ @@ -90,7 +102,7 @@ def has_role(self, role, course_id, org): or a role that inherits from the specified role, course_id and org. """ return any( - access_role.role in self._get_roles(role) and + access_role.role in self.get_roles(role) and access_role.course_id == course_id and access_role.org == org for access_role in self._roles @@ -286,6 +298,14 @@ class CourseLimitedStaffRole(CourseStaffRole): BASE_ROLE = CourseStaffRole.ROLE +@register_access_role +class eSHEInstructorRole(CourseStaffRole): + """A Staff member of a course without access to Studio.""" + + ROLE = 'eshe_instructor' + BASE_ROLE = CourseStaffRole.ROLE + + @register_access_role class CourseInstructorRole(CourseRole): """A course Instructor""" @@ -463,11 +483,11 @@ def remove_courses(self, *course_keys): def courses_with_role(self): """ - Return a django QuerySet for all of the courses with this user x role. You can access + Return a django QuerySet for all of the courses with this user x (or derived from x) role. You can access any of these properties on each result record: * user (will be self.user--thus uninteresting) * org * course_id * role (will be self.role--thus uninteresting) """ - return CourseAccessRole.objects.filter(role=self.role, user=self.user) + return CourseAccessRole.objects.filter(role__in=RoleCache.get_roles(self.role), user=self.user) diff --git a/common/djangoapps/student/tests/test_roles.py b/common/djangoapps/student/tests/test_roles.py index dff86047d2ff..b97ebb0e26cf 100644 --- a/common/djangoapps/student/tests/test_roles.py +++ b/common/djangoapps/student/tests/test_roles.py @@ -15,6 +15,7 @@ CourseStaffRole, CourseFinanceAdminRole, CourseSalesAdminRole, + eSHEInstructorRole, LibraryUserRole, CourseDataResearcherRole, GlobalStaff, @@ -167,6 +168,7 @@ class RoleCacheTestCase(TestCase): # lint-amnesty, pylint: disable=missing-clas ROLES = ( (CourseStaffRole(IN_KEY), ('staff', IN_KEY, 'edX')), (CourseLimitedStaffRole(IN_KEY), ('limited_staff', IN_KEY, 'edX')), + (eSHEInstructorRole(IN_KEY), ('eshe_instructor', IN_KEY, 'edX')), (CourseInstructorRole(IN_KEY), ('instructor', IN_KEY, 'edX')), (OrgStaffRole(IN_KEY.org), ('staff', None, 'edX')), (CourseFinanceAdminRole(IN_KEY), ('finance_admin', IN_KEY, 'edX')), diff --git a/lms/djangoapps/courseware/rules.py b/lms/djangoapps/courseware/rules.py index 8202418f6f4c..07cbbab9022c 100644 --- a/lms/djangoapps/courseware/rules.py +++ b/lms/djangoapps/courseware/rules.py @@ -18,7 +18,7 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.enrollments.api import is_enrollment_valid_for_proctoring from common.djangoapps.student.models import CourseAccessRole -from common.djangoapps.student.roles import CourseRole, OrgRole +from common.djangoapps.student.roles import CourseRole, OrgRole, strict_role_checking from xmodule.course_block import CourseBlock # lint-amnesty, pylint: disable=wrong-import-order from xmodule.error_block import ErrorBlock # lint-amnesty, pylint: disable=wrong-import-order @@ -47,10 +47,14 @@ class HasAccessRule(Rule): """ A rule that calls `has_access` to determine whether it passes """ - def __init__(self, action): + def __init__(self, action, strict=False): self.action = action + self.strict = strict def check(self, user, instance=None): + if self.strict: + with strict_role_checking(): + return has_access(user, self.action, instance) return has_access(user, self.action, instance) def query(self, user): diff --git a/lms/djangoapps/instructor/access.py b/lms/djangoapps/instructor/access.py index 9255d113f038..343b6923ccee 100644 --- a/lms/djangoapps/instructor/access.py +++ b/lms/djangoapps/instructor/access.py @@ -19,6 +19,7 @@ CourseInstructorRole, CourseLimitedStaffRole, CourseStaffRole, + eSHEInstructorRole, ) from lms.djangoapps.instructor.enrollment import enroll_email, get_email_params from openedx.core.djangoapps.django_comment_common.models import Role @@ -30,6 +31,7 @@ 'instructor': CourseInstructorRole, 'staff': CourseStaffRole, 'limited_staff': CourseLimitedStaffRole, + 'eshe_instructor': eSHEInstructorRole, 'ccx_coach': CourseCcxCoachRole, 'data_researcher': CourseDataResearcherRole, } diff --git a/lms/djangoapps/instructor/permissions.py b/lms/djangoapps/instructor/permissions.py index 20426761e66b..ac9b430acec1 100644 --- a/lms/djangoapps/instructor/permissions.py +++ b/lms/djangoapps/instructor/permissions.py @@ -44,7 +44,13 @@ # only global staff or those with the data_researcher role can access the data download tab # HasAccessRule('staff') also includes course staff perms[CAN_RESEARCH] = is_staff | HasRolesRule('data_researcher') -perms[CAN_ENROLL] = HasAccessRule('staff') +# eshe_instructor implicitly gets staff access, but shouldn't be able to enroll +perms[CAN_ENROLL] = ( + # can enroll if a user is an eshe_instructor and has an explicit staff role + (HasRolesRule('eshe_instructor') & HasAccessRule('staff', strict=True)) | + # can enroll if a user is just staff + (~HasRolesRule('eshe_instructor') & HasAccessRule('staff')) +) perms[CAN_BETATEST] = HasAccessRule('instructor') perms[ENROLLMENT_REPORT] = HasAccessRule('staff') | HasRolesRule('data_researcher') perms[VIEW_COUPONS] = HasAccessRule('staff') | HasRolesRule('data_researcher') diff --git a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py index bca16977a98c..bdbffbde7f68 100644 --- a/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py +++ b/lms/djangoapps/instructor/tests/views/test_instructor_dashboard.py @@ -29,6 +29,7 @@ from lms.djangoapps.courseware.tabs import get_course_tab_list from lms.djangoapps.courseware.tests.factories import StudentModuleFactory from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase +from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory from lms.djangoapps.grades.config.waffle import WRITABLE_GRADEBOOK from lms.djangoapps.instructor.toggles import DATA_DOWNLOAD_V2 from lms.djangoapps.instructor.views.gradebook_api import calculate_page_info @@ -37,6 +38,7 @@ ENABLE_PAGES_AND_RESOURCES_MICROFRONTEND, OVERRIDE_DISCUSSION_LEGACY_SETTINGS_FLAG ) +from openedx.core.djangoapps.django_comment_common.models import FORUM_ROLE_ADMINISTRATOR from openedx.core.djangoapps.site_configuration.models import SiteConfiguration @@ -308,6 +310,56 @@ def test_membership_reason_field_visibility(self, enbale_reason_field): else: self.assertNotContains(response, reason_field) + def test_membership_tab_for_eshe_instructor(self): + """ + Verify that eSHE instructors don't have access to membership tab and + work correctly with other roles. + """ + + membership_section = ( + '
  • ' + '' + '
    • ' + ) + batch_enrollment = ( + '
    ' + ) + + user = UserFactory.create() + self.client.login(username=user.username, password="test") + + # eSHE instructors shouldn't have access to membership tab + CourseAccessRoleFactory( + course_id=self.course.id, + user=user, + role='eshe_instructor', + org=self.course.id.org + ) + response = self.client.get(self.url) + self.assertNotContains(response, membership_section) + + # However if combined with forum_admin, they should have access to this + # tab, but not to batch enrollment + forum_admin_role = RoleFactory(name=FORUM_ROLE_ADMINISTRATOR, course_id=self.course.id) + forum_admin_role.users.add(user) + response = self.client.get(self.url) + self.assertContains(response, membership_section) + self.assertNotContains(response, batch_enrollment) + + # Combined with course staff, should have union of all three roles + # permissions sets + CourseAccessRoleFactory( + course_id=self.course.id, + user=user, + role='staff', + org=self.course.id.org + ) + response = self.client.get(self.url) + self.assertContains(response, membership_section) + self.assertContains(response, batch_enrollment) + def test_student_admin_staff_instructor(self): """ Verify that staff users are not able to see course-wide options, while still diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 6e886e0c3c1c..96ebca4e557d 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -31,7 +31,9 @@ CourseFinanceAdminRole, CourseInstructorRole, CourseSalesAdminRole, - CourseStaffRole + CourseStaffRole, + eSHEInstructorRole, + strict_role_checking, ) from common.djangoapps.util.json_request import JsonResponse from lms.djangoapps.bulk_email.api import is_bulk_email_feature_enabled @@ -119,6 +121,7 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable access = { 'admin': request.user.is_staff, 'instructor': bool(has_access(request.user, 'instructor', course)), + 'eshe_instructor': eSHEInstructorRole(course_key).has_user(request.user), 'finance_admin': CourseFinanceAdminRole(course_key).has_user(request.user), 'sales_admin': CourseSalesAdminRole(course_key).has_user(request.user), 'staff': bool(has_access(request.user, 'staff', course)), @@ -126,6 +129,9 @@ def instructor_dashboard_2(request, course_id): # lint-amnesty, pylint: disable 'data_researcher': request.user.has_perm(permissions.CAN_RESEARCH, course_key), } + with strict_role_checking(): + access['explicit_staff'] = bool(has_access(request.user, 'staff', course)) + if not request.user.has_perm(permissions.VIEW_DASHBOARD, course_key): raise Http404() @@ -504,7 +510,15 @@ def _section_membership(course, access): 'update_forum_role_membership', kwargs={'course_id': str(course_key)} ), - 'is_reason_field_enabled': configuration_helpers.get_value('ENABLE_MANUAL_ENROLLMENT_REASON_FIELD', False) + 'is_reason_field_enabled': configuration_helpers.get_value('ENABLE_MANUAL_ENROLLMENT_REASON_FIELD', False), + + # Membership section should be hidden for eSHE instructors. + # Since they get Course Staff role implicitly, we need to hide this + # section if the user doesn't have the Course Staff role set explicitly + # or have the Discussion Admin role. + 'is_hidden': ( + not access['forum_admin'] and (access['eshe_instructor'] and not access['explicit_staff']) + ), } return section_data diff --git a/lms/envs/common.py b/lms/envs/common.py index f3e3604241da..f535183a5e00 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1050,6 +1050,16 @@ # .. toggle_target_removal_date: None # .. toggle_tickets: 'https://github.com/openedx/edx-platform/pull/30832' 'ENABLE_LEGACY_MD5_HASH_FOR_ANONYMOUS_USER_ID': False, + + # .. toggle_name: FEATURES['ENABLE_ESHE_INSTRUCTOR_ROLE'] + # .. toggle_implementation: DjangoSetting + # .. toggle_default: False + # .. toggle_description: Whether to enable the ESHE Instructor role + # .. toggle_use_cases: open_edx + # .. toggle_creation_date: 2023-07-31 + # .. toggle_target_removal_date: None + # .. toggle_tickets: 'https://github.com/open-craft/edx-platform/pull/561/files' + 'ENABLE_ESHE_INSTRUCTOR_ROLE': False, } # Specifies extra XBlock fields that should available when requested via the Course Blocks API diff --git a/lms/static/js/fixtures/instructor_dashboard/membership.html b/lms/static/js/fixtures/instructor_dashboard/membership.html index 962bfac5b4f1..6b77a148df69 100644 --- a/lms/static/js/fixtures/instructor_dashboard/membership.html +++ b/lms/static/js/fixtures/instructor_dashboard/membership.html @@ -11,6 +11,7 @@

    Course Team Management