diff --git a/Makefile b/Makefile index e9d644f39c..057d9839f2 100644 --- a/Makefile +++ b/Makefile @@ -35,8 +35,6 @@ swagger: ## generate the swagger.yaml file extract_translations: ## extract localizable strings from sources i18n_tool extract --no-segment -v cd conf/locale/en/LC_MESSAGES && msgcat djangojs.po underscore.po -o djangojs.po - cd conf/locale/en/LC_MESSAGES && msgcat django.po wiki.po edx_proctoring_proctortrack.po mako.po -o django.po - cd conf/locale/en/LC_MESSAGES && rm wiki.po edx_proctoring_proctortrack.po mako.po underscore.po pull_plugin_translations: ## Pull translations for edx_django_utils.plugins for both lms and cms python manage.py lms pull_plugin_translations --verbose $(ATLAS_OPTIONS) diff --git a/cms/djangoapps/contentstore/api/tests/test_validation.py b/cms/djangoapps/contentstore/api/tests/test_validation.py index 4928d31dc1..8da8004772 100644 --- a/cms/djangoapps/contentstore/api/tests/test_validation.py +++ b/cms/djangoapps/contentstore/api/tests/test_validation.py @@ -21,7 +21,12 @@ from common.djangoapps.student.tests.factories import UserFactory @ddt.ddt -@override_settings(PROCTORING_BACKENDS={'DEFAULT': 'proctortrack', 'proctortrack': {}}) +@override_settings( + PROCTORING_BACKENDS={ + "DEFAULT": "test_proctoring_provider", + "test_proctoring_provider": {"requires_escalation_email": True}, + } +) class CourseValidationViewTest(SharedModuleStoreTestCase, APITestCase): """ Test course validation view via a RESTful API @@ -33,7 +38,7 @@ class CourseValidationViewTest(SharedModuleStoreTestCase, APITestCase): cls.course = CourseFactory.create( display_name='test course', run="Testing_course", - proctoring_provider='proctortrack', + proctoring_provider='test_proctoring_provider', proctoring_escalation_email='test@example.com', ) cls.course_key = cls.course.id diff --git a/cms/djangoapps/contentstore/api/views/course_validation.py b/cms/djangoapps/contentstore/api/views/course_validation.py index be5208430a..c55f58d945 100644 --- a/cms/djangoapps/contentstore/api/views/course_validation.py +++ b/cms/djangoapps/contentstore/api/views/course_validation.py @@ -8,6 +8,7 @@ from rest_framework.response import Response from cms.djangoapps.contentstore.course_info_model import get_course_updates from cms.djangoapps.contentstore.views.certificates import CertificateManager +from common.djangoapps.util.proctoring import requires_escalation_email from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes from xmodule.course_metadata_utils import DEFAULT_GRADING_POLICY # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order @@ -340,8 +341,8 @@ class CourseValidationView(DeveloperErrorViewMixin, GenericAPIView): return False def _proctoring_validation(self, course): - # A proctoring escalation email is currently only required for courses using Proctortrack + # A proctoring escalation email is required if 'required_escalation_email' is set on the proctoring backend return dict( - needs_proctoring_escalation_email=course.proctoring_provider == 'proctortrack', + needs_proctoring_escalation_email=requires_escalation_email(course.proctoring_provider), has_proctoring_escalation_email=bool(course.proctoring_escalation_email) ) diff --git a/cms/djangoapps/contentstore/config/waffle.py b/cms/djangoapps/contentstore/config/waffle.py index ae0e6ea467..40d1181766 100644 --- a/cms/djangoapps/contentstore/config/waffle.py +++ b/cms/djangoapps/contentstore/config/waffle.py @@ -22,10 +22,6 @@ ENABLE_CHECKLISTS_QUALITY = CourseWaffleFlag( # lint-amnesty, pylint: disable=t f'{WAFFLE_NAMESPACE}.enable_checklists_quality', __name__, LOG_PREFIX ) -SHOW_REVIEW_RULES_FLAG = CourseWaffleFlag( # lint-amnesty, pylint: disable=toggle-missing-annotation - f'{WAFFLE_NAMESPACE}.show_review_rules', __name__, LOG_PREFIX -) - # .. toggle_name: studio.custom_relative_dates # .. toggle_implementation: CourseWaffleFlag diff --git a/cms/djangoapps/contentstore/rest_api/v1/serializers/proctoring.py b/cms/djangoapps/contentstore/rest_api/v1/serializers/proctoring.py index 8398bd8ad6..f13ddd3aab 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/serializers/proctoring.py +++ b/cms/djangoapps/contentstore/rest_api/v1/serializers/proctoring.py @@ -5,7 +5,7 @@ API Serializers for proctoring from rest_framework import serializers from cms.djangoapps.contentstore.rest_api.serializers.common import ProctoringErrorListSerializer -from xmodule.course_block import get_available_providers +from xmodule.course_block import get_available_providers, get_requires_escalation_email_providers class ProctoredExamSettingsSerializer(serializers.Serializer): @@ -29,6 +29,9 @@ class ProctoredExamConfigurationSerializer(serializers.Serializer): """ Serializer for various metadata associated with proctored exam settings. """ proctored_exam_settings = ProctoredExamSettingsSerializer() available_proctoring_providers = serializers.ChoiceField(get_available_providers()) + requires_escalation_email_providers = serializers.ChoiceField( + get_requires_escalation_email_providers() + ) course_start_date = serializers.DateTimeField() diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/proctoring.py b/cms/djangoapps/contentstore/rest_api/v1/views/proctoring.py index 015ff31c29..f654ab874f 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/proctoring.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/proctoring.py @@ -14,7 +14,10 @@ from cms.djangoapps.contentstore.views.course import get_course_and_check_access from cms.djangoapps.contentstore.utils import get_proctored_exam_settings_url from cms.djangoapps.models.settings.course_metadata import CourseMetadata from common.djangoapps.student.auth import has_studio_advanced_settings_access -from xmodule.course_block import get_available_providers # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.course_block import ( + get_available_providers, + get_requires_escalation_email_providers, +) # lint-amnesty, pylint: disable=wrong-import-order from openedx.core.djangoapps.course_apps.toggles import exams_ida_enabled from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, verify_course_exists, view_auth_classes from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order @@ -49,7 +52,8 @@ class ProctoredExamSettingsView(APIView): **Response** - In the case of a 200 response code, the response will proctored exam settings data + In the case of a 200 response code, the response will proctored exam settings data, + list of proctoring backends that have 'requires_escalation_email' set to 'True' as well as other metadata about the course or the requesting user that are necessary for rendering the settings page. @@ -65,7 +69,10 @@ class ProctoredExamSettingsView(APIView): }, "available_proctoring_providers": [ "mockprock", - "proctortrack" + "software_secure", + ], + "requires_escalation_email_providers": [ + "software_secure" ], "course_start_date": "2013-02-05T05:00:00Z", } @@ -108,10 +115,15 @@ class ProctoredExamSettingsView(APIView): data['course_start_date'] = course_metadata['start'].get('value') available_providers = get_available_providers() + requires_escalation_email_providers = ( + get_requires_escalation_email_providers() + ) if not exams_ida_enabled(CourseKey.from_string(course_id)): available_providers.remove('lti_external') + requires_escalation_email_providers.remove('lti_external') data['available_proctoring_providers'] = available_providers + data['requires_escalation_email_providers'] = requires_escalation_email_providers serializer = ProctoredExamConfigurationSerializer(data) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py index 8e220a334c..9eabf03f1b 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_proctoring.py @@ -62,6 +62,7 @@ class ProctoringExamSettingsGetTests( }, "course_start_date": "2030-01-01T00:00:00Z", "available_proctoring_providers": ["null"], + "requires_escalation_email_providers": [], } def make_request(self, course_id=None, data=None): @@ -100,6 +101,7 @@ class ProctoringExamSettingsGetTests( }, "course_start_date": "2030-01-01T00:00:00Z", "available_proctoring_providers": ["null"], + "requires_escalation_email_providers": [], } assert response.data == expected_data @@ -122,6 +124,7 @@ class ProctoringExamSettingsGetTests( }, "course_start_date": "2030-01-01T00:00:00Z", "available_proctoring_providers": ["lti_external", "null"], + "requires_escalation_email_providers": ["lti_external"], } assert response.data == expected_data @@ -162,14 +165,17 @@ class ProctoringExamSettingsPostTests( return super().test_course_instructor(expect_status=expect_status) @override_settings( - PROCTORING_BACKENDS={"DEFAULT": "proctortrack", "proctortrack": {}}, + PROCTORING_BACKENDS={ + "DEFAULT": "test_proctoring_provider", + "test_proctoring_provider": {"requires_escalation_email": True}, + }, ) def test_update_exam_settings_200_escalation_email(self): - """update exam settings for provider that requires an escalation email (proctortrack)""" + """update exam settings for provider that requires an escalation email""" self.client.login(username=self.global_staff.username, password=self.password) data = self.get_request_data( enable_proctored_exams=True, - proctoring_provider="proctortrack", + proctoring_provider="test_proctoring_provider", proctoring_escalation_email="foo@bar.com", ) response = self.make_request(data=data) @@ -182,7 +188,7 @@ class ProctoringExamSettingsPostTests( "proctored_exam_settings": { "enable_proctored_exams": True, "allow_proctoring_opt_out": True, - "proctoring_provider": "proctortrack", + "proctoring_provider": "test_proctoring_provider", "proctoring_escalation_email": "foo@bar.com", "create_zendesk_tickets": True, } @@ -192,7 +198,7 @@ class ProctoringExamSettingsPostTests( # course settings have been updated updated = modulestore().get_item(self.course.location) assert updated.enable_proctored_exams is True - assert updated.proctoring_provider == "proctortrack" + assert updated.proctoring_provider == "test_proctoring_provider" assert updated.proctoring_escalation_email == "foo@bar.com" @override_settings( @@ -299,14 +305,17 @@ class ProctoringExamSettingsPostTests( assert response.status_code == status.HTTP_403_FORBIDDEN @override_settings( - PROCTORING_BACKENDS={"DEFAULT": "proctortrack", "proctortrack": {}}, + PROCTORING_BACKENDS={ + "DEFAULT": "test_proctoring_provider", + "test_proctoring_provider": {"requires_escalation_email": True}, + }, ) def test_200_for_instructor_request_compatibility(self): self.client.login(username=self.course_instructor, password=self.password) data = { "proctored_exam_settings": { "enable_proctored_exams": True, - "proctoring_provider": "proctortrack", + "proctoring_provider": "test_proctoring_provider", "proctoring_escalation_email": "foo@bar.com", } } @@ -315,16 +324,13 @@ class ProctoringExamSettingsPostTests( @override_settings( PROCTORING_BACKENDS={ - "DEFAULT": "proctortrack", - "proctortrack": {}, + "DEFAULT": "software_secure", "software_secure": {}, }, ) @patch("logging.Logger.info") @ddt.data( - ("proctortrack", False, False), ("software_secure", True, False), - ("proctortrack", True, True), ("software_secure", False, True), ) @ddt.unpack diff --git a/cms/djangoapps/contentstore/tests/test_course_settings.py b/cms/djangoapps/contentstore/tests/test_course_settings.py index ac8d9d3eba..e40d0c9256 100644 --- a/cms/djangoapps/contentstore/tests/test_course_settings.py +++ b/cms/djangoapps/contentstore/tests/test_course_settings.py @@ -1147,12 +1147,13 @@ class CourseMetadataEditingTest(CourseTestCase): @override_settings( PROCTORING_BACKENDS={ 'DEFAULT': 'test_proctoring_provider', - 'proctortrack': {} + 'test_proctoring_provider': {"requires_escalation_email": True}, }, ) def test_fetch_proctoring_escalation_email_present(self): """ - If 'proctortrack' is an available provider, show the escalation email setting + If proctoring provider has 'requires_escalation_email' set to 'True', + show the escalation email setting """ test_model = CourseMetadata.fetch(self.fullcourse) self.assertIn('proctoring_escalation_email', test_model) @@ -1160,12 +1161,13 @@ class CourseMetadataEditingTest(CourseTestCase): @override_settings( PROCTORING_BACKENDS={ 'DEFAULT': 'test_proctoring_provider', - 'alternate_provider': {} + 'test_proctoring_provider': {} }, ) def test_fetch_proctoring_escalation_email_not_present(self): """ - If 'proctortrack' is not an available provider, don't show the escalation email setting + If proctoring provider does not have 'requires_escalation_email' set to 'True', + don't show the escalation email setting """ test_model = CourseMetadata.fetch(self.fullcourse) self.assertNotIn('proctoring_escalation_email', test_model) @@ -1526,13 +1528,12 @@ class CourseMetadataEditingTest(CourseTestCase): @override_settings( PROCTORING_BACKENDS={ 'DEFAULT': 'test_proctoring_provider', - 'test_proctoring_provider': {}, - 'proctortrack': {} + 'test_proctoring_provider': {"requires_escalation_email": True}, }, ) - def test_validate_update_requires_escalation_email_for_proctortrack(self, include_blank_email): + def test_validate_update_requires_escalation_email_if_relevant_flag_is_set(self, include_blank_email): json_data = { - "proctoring_provider": {"value": 'proctortrack'}, + "proctoring_provider": {"value": 'test_proctoring_provider'}, } if include_blank_email: json_data["proctoring_escalation_email"] = {"value": ""} @@ -1549,14 +1550,13 @@ class CourseMetadataEditingTest(CourseTestCase): self.assertIsNone(test_model) self.assertEqual( errors[0].get('message'), - 'Provider \'proctortrack\' requires an exam escalation contact.' + 'Provider \'test_proctoring_provider\' requires an exam escalation contact.' ) @override_settings( PROCTORING_BACKENDS={ 'DEFAULT': 'test_proctoring_provider', 'test_proctoring_provider': {}, - 'proctortrack': {} } ) def test_validate_update_does_not_require_escalation_email_by_default(self): @@ -1573,14 +1573,14 @@ class CourseMetadataEditingTest(CourseTestCase): @override_settings( PROCTORING_BACKENDS={ - 'DEFAULT': 'proctortrack', - 'proctortrack': {} + 'DEFAULT': 'test_proctoring_provider', + 'test_proctoring_provider': {"requires_escalation_email": True}, }, ) - def test_validate_update_cannot_unset_escalation_email_when_proctortrack_is_provider(self): + def test_validate_update_cannot_unset_escalation_email_when_requires_escalation_email_set_on_provider(self): course = CourseFactory.create() CourseMetadata.update_from_dict( - {"proctoring_provider": 'proctortrack', "enable_proctored_exams": True}, + {"proctoring_provider": 'test_proctoring_provider', "enable_proctored_exams": True}, course, self.user ) @@ -1596,20 +1596,20 @@ class CourseMetadataEditingTest(CourseTestCase): self.assertIsNone(test_model) self.assertEqual( errors[0].get('message'), - 'Provider \'proctortrack\' requires an exam escalation contact.' + 'Provider \'test_proctoring_provider\' requires an exam escalation contact.' ) @override_settings( PROCTORING_BACKENDS={ - 'DEFAULT': 'proctortrack', - 'proctortrack': {} + 'DEFAULT': 'test_proctoring_provider', + 'test_proctoring_provider': {"requires_escalation_email": True}, } ) - def test_validate_update_set_proctortrack_provider_with_valid_escalation_email(self): + def test_validate_update_set_proctoring_provider_with_valid_escalation_email(self): did_validate, errors, test_model = CourseMetadata.validate_and_update_from_json( self.course, { - "proctoring_provider": {"value": "proctortrack"}, + "proctoring_provider": {"value": "test_proctoring_provider"}, "proctoring_escalation_email": {"value": "foo@bar.com"}, }, user=self.user @@ -1621,16 +1621,20 @@ class CourseMetadataEditingTest(CourseTestCase): @override_settings( PROCTORING_BACKENDS={ - 'DEFAULT': 'proctortrack', - 'proctortrack': {} + 'DEFAULT': 'test_proctoring_provider', + 'test_proctoring_provider': {"requires_escalation_email": True}, } ) def test_validate_update_disable_proctoring_with_no_escalation_email(self): course = CourseFactory.create() CourseMetadata.update_from_dict( - {"proctoring_provider": 'proctortrack', "proctoring_escalation_email": '', "enable_proctored_exams": True}, + { + "proctoring_provider": "test_proctoring_provider", + "proctoring_escalation_email": "", + "enable_proctored_exams": True, + }, course, - self.user + self.user, ) did_validate, errors, test_model = CourseMetadata.validate_and_update_from_json( course, @@ -1645,15 +1649,15 @@ class CourseMetadataEditingTest(CourseTestCase): @override_settings( PROCTORING_BACKENDS={ - 'DEFAULT': 'proctortrack', - 'proctortrack': {} + 'DEFAULT': 'test_proctoring_provider', + 'test_proctoring_provider': {"requires_escalation_email": True}, } ) def test_validate_update_disable_proctoring_and_change_escalation_email(self): did_validate, errors, test_model = CourseMetadata.validate_and_update_from_json( self.course, { - "proctoring_provider": {"value": "proctortrack"}, + "proctoring_provider": {"value": "test_proctoring_provider"}, "proctoring_escalation_email": {"value": ""}, "enable_proctored_exams": {"value": False}, }, @@ -1667,14 +1671,14 @@ class CourseMetadataEditingTest(CourseTestCase): @override_settings( PROCTORING_BACKENDS={ - 'DEFAULT': 'proctortrack', - 'proctortrack': {} + 'DEFAULT': 'test_proctoring_provider', + 'test_proctoring_provider': {"requires_escalation_email": True}, } ) def test_validate_update_disabled_proctoring_and_unset_escalation_email(self): course = CourseFactory.create() CourseMetadata.update_from_dict( - {"proctoring_provider": 'proctortrack', "enable_proctored_exams": False}, + {"proctoring_provider": 'test_proctoring_provider', "enable_proctored_exams": False}, course, self.user ) diff --git a/cms/djangoapps/contentstore/views/tests/test_block.py b/cms/djangoapps/contentstore/views/tests/test_block.py index bdab677981..46c1f83198 100644 --- a/cms/djangoapps/contentstore/views/tests/test_block.py +++ b/cms/djangoapps/contentstore/views/tests/test_block.py @@ -12,6 +12,7 @@ from django.http import Http404 from django.test import TestCase from django.test.client import RequestFactory from django.urls import reverse +from django.test.utils import override_settings from edx_toggles.toggles.testutils import override_waffle_flag from openedx.core.djangoapps.video_config.toggles import PUBLIC_VIDEO_SHARE from openedx_events.content_authoring.data import DuplicatedXBlockData @@ -3701,10 +3702,49 @@ class TestSpecialExamXBlockInfo(ItemTest): assert xblock_info["proctoring_exam_configuration_link"] == "test_url" assert xblock_info["supports_onboarding"] is True assert xblock_info["is_onboarding_exam"] is False + assert xblock_info["show_review_rules"] is True mock_get_exam_configuration_dashboard_url.assert_called_with( self.course.id, xblock_info["id"] ) + @patch_get_exam_configuration_dashboard_url + @patch_does_backend_support_onboarding + @patch_get_exam_by_content_id_success + @override_settings( + PROCTORING_BACKENDS={ + "DEFAULT": "null", + # By default "show_review_rules" is True unless you explicitly set it to False. + "test_proctoring_provider": {"show_review_rules": False}, + } + ) + def test_show_review_rules_xblock_info( + self, + mock_get_exam_by_content_id, + _mock_does_backend_support_onboarding, + mock_get_exam_configuration_dashboard_url, + ): + # Set course.proctoring_provider to test_proctoring_provider + self.course.proctoring_provider = 'test_proctoring_provider' + sequential = BlockFactory.create( + parent_location=self.chapter.location, + category="sequential", + display_name="Test Lesson 1", + user_id=self.user.id, + is_proctored_enabled=True, + is_time_limited=True, + default_time_limit_minutes=100, + is_onboarding_exam=False, + ) + sequential = modulestore().get_item(sequential.location) + xblock_info = create_xblock_info( + sequential, + include_child_info=True, + include_children_predicate=ALWAYS, + course=self.course, + ) + + assert xblock_info["show_review_rules"] is False + @patch_get_exam_configuration_dashboard_url @patch_does_backend_support_onboarding @patch_get_exam_by_content_id_success @@ -3757,7 +3797,7 @@ class TestSpecialExamXBlockInfo(ItemTest): (None, False), ) @ddt.unpack - def test_xblock_was_ever_proctortrack_proctored_exam( + def test_xblock_was_ever_linked_to_external_exam( self, external_id, expected_value, @@ -3787,7 +3827,7 @@ class TestSpecialExamXBlockInfo(ItemTest): @patch_get_exam_configuration_dashboard_url @patch_does_backend_support_onboarding @patch_get_exam_by_content_id_not_found - def test_xblock_was_never_proctortrack_proctored_exam( + def test_xblock_was_never_linked_to_external_exam( self, mock_get_exam_by_content_id, _mock_does_backend_support_onboarding_patch, @@ -3846,6 +3886,7 @@ class TestSpecialExamXBlockInfo(ItemTest): assert xblock_info["proctoring_exam_configuration_link"] is None assert xblock_info["supports_onboarding"] is True assert xblock_info["is_onboarding_exam"] is False + assert xblock_info["show_review_rules"] is True class TestLibraryXBlockInfo(ModuleStoreTestCase): diff --git a/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py b/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py index 88c4aa27eb..735c59acd2 100644 --- a/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py +++ b/cms/djangoapps/contentstore/views/tests/test_exam_settings_view.py @@ -87,7 +87,7 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): @override_settings( PROCTORING_BACKENDS={ 'DEFAULT': 'test_proctoring_provider', - 'proctortrack': {} + 'test_proctoring_provider': {"requires_escalation_email": True} }, ) @ddt.data( @@ -98,9 +98,9 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): An alert should appear if current exam settings are invalid. The exam settings alert should direct users to the exam settings page. """ - # create an error by setting proctortrack as the provider and not setting + # create an error by setting 'requires_escalation_email' as 'True' and not setting # the (required) escalation contact - self.course.proctoring_provider = 'proctortrack' + self.course.proctoring_provider = 'test_proctoring_provider' self.course.enable_proctored_exams = True self.save_course() @@ -123,7 +123,7 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): @override_settings( PROCTORING_BACKENDS={ 'DEFAULT': 'test_proctoring_provider', - 'proctortrack': {} + 'test_proctoring_provider': {"requires_escalation_email": True} }, ) @ddt.data( @@ -136,9 +136,9 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): The exam settings alert should direct users to the advanced settings page if the exam settings feature is not available. """ - # create an error by setting proctortrack as the provider and not setting + # create an error by setting 'requires_escalation_email' as 'True' and not setting # the (required) escalation contact - self.course.proctoring_provider = 'proctortrack' + self.course.proctoring_provider = 'test_proctoring_provider' self.course.enable_proctored_exams = True self.save_course() @@ -164,7 +164,6 @@ class TestExamSettingsView(CourseTestCase, UrlResetMixin): @override_settings( PROCTORING_BACKENDS={ 'DEFAULT': 'test_proctoring_provider', - 'proctortrack': {}, 'test_proctoring_provider': {}, }, ) diff --git a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py index 78c3935323..747641a184 100644 --- a/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py +++ b/cms/djangoapps/contentstore/xblock_storage_handlers/view_handlers.py @@ -35,7 +35,6 @@ from xblock.core import XBlock from xblock.fields import Scope from .xblock_helpers import get_block_key_string -from cms.djangoapps.contentstore.config.waffle import SHOW_REVIEW_RULES_FLAG from cms.djangoapps.contentstore.helpers import StaticFileNotices from cms.djangoapps.models.settings.course_grading import CourseGradingModel from cms.lib.ai_aside_summary_config import AiAsideSummaryConfig @@ -49,6 +48,7 @@ from common.djangoapps.student.auth import ( ) from common.djangoapps.util.date_utils import get_default_time_display from common.djangoapps.util.json_request import JsonResponse, expect_json +from common.djangoapps.util.proctoring import show_review_rules from openedx.core.djangoapps.bookmarks import api as bookmarks_api from openedx.core.djangoapps.content_tagging.toggles import is_tagging_feature_disabled from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration @@ -1302,13 +1302,6 @@ def create_xblock_info( # lint-amnesty, pylint: disable=too-many-statements f"Error while getting proctoring exam configuration link: {e}" ) - if course.proctoring_provider == "proctortrack": - show_review_rules = SHOW_REVIEW_RULES_FLAG.is_enabled( - xblock.location.course_key - ) - else: - show_review_rules = True - xblock_info.update( { "is_proctored_exam": xblock.is_proctored_exam, @@ -1323,7 +1316,7 @@ def create_xblock_info( # lint-amnesty, pylint: disable=too-many-statements "default_time_limit_minutes": xblock.default_time_limit_minutes, "proctoring_exam_configuration_link": proctoring_exam_configuration_link, "supports_onboarding": supports_onboarding, - "show_review_rules": show_review_rules, + "show_review_rules": show_review_rules(course.proctoring_provider), } ) diff --git a/cms/djangoapps/models/settings/course_metadata.py b/cms/djangoapps/models/settings/course_metadata.py index 1cd87ed5a0..c66f5b65e4 100644 --- a/cms/djangoapps/models/settings/course_metadata.py +++ b/cms/djangoapps/models/settings/course_metadata.py @@ -14,6 +14,7 @@ from xblock.fields import Scope from cms.djangoapps.contentstore import toggles from common.djangoapps.util.db import MYSQL_MAX_INT, generate_int_id +from common.djangoapps.util.proctoring import requires_escalation_email from common.djangoapps.xblock_django.models import XBlockStudioConfigurationFlag from openedx.core.djangoapps.course_apps.toggles import exams_ida_enabled from openedx.core.djangoapps.discussions.config.waffle_utils import legacy_discussion_experience_enabled @@ -144,10 +145,15 @@ class CourseMetadata: if not COURSE_ENABLE_UNENROLLED_ACCESS_FLAG.is_enabled(course_key=course_key): exclude_list.append('course_visibility') - # Do not show "Proctortrack Exam Escalation Contact" if Proctortrack is not - # an available proctoring backend. - if not settings.PROCTORING_BACKENDS or settings.PROCTORING_BACKENDS.get('proctortrack') is None: - exclude_list.append('proctoring_escalation_email') + # Do not show "Proctoring Exam Escalation Contact" if 'requires_escalation_email' + # is not set on any of the the proctoring backends. + escalation_email_required = False + for provider in get_available_providers(): + if requires_escalation_email(provider): + escalation_email_required = True + break + if not escalation_email_required: + exclude_list.append("proctoring_escalation_email") if not legacy_discussion_experience_enabled(course_key): exclude_list.append('discussion_blackouts') @@ -512,7 +518,7 @@ class CourseMetadata: 'model': proctoring_provider_model }) - # Require a valid escalation email if Proctortrack is chosen as the proctoring provider + # Require a valid escalation email if 'requires_escalation_email' is set in the proctoring backend escalation_email_model = settings_dict.get('proctoring_escalation_email') if escalation_email_model: escalation_email = escalation_email_model.get('value') @@ -520,7 +526,7 @@ class CourseMetadata: escalation_email = block.proctoring_escalation_email missing_escalation_email_msg = 'Provider \'{provider}\' requires an exam escalation contact.' - if proctoring_provider_model and proctoring_provider == 'proctortrack': + if proctoring_provider_model and requires_escalation_email(proctoring_provider): if not escalation_email: message = missing_escalation_email_msg.format(provider=proctoring_provider) errors.append({ @@ -531,7 +537,7 @@ class CourseMetadata: if ( escalation_email_model and not proctoring_provider_model and - proctoring_provider == 'proctortrack' + requires_escalation_email(proctoring_provider) ): if not escalation_email: message = missing_escalation_email_msg.format(provider=proctoring_provider) @@ -548,10 +554,7 @@ class CourseMetadata: else: create_zendesk_tickets = block.create_zendesk_tickets - if ( - (proctoring_provider == 'proctortrack' and create_zendesk_tickets) - or (proctoring_provider == 'software_secure' and not create_zendesk_tickets) - ): + if proctoring_provider == 'software_secure' and not create_zendesk_tickets: LOGGER.info( 'create_zendesk_tickets set to {ticket_value} but proctoring ' 'provider is {provider} for course {course_id}. create_zendesk_tickets ' diff --git a/cms/envs/mock.yml b/cms/envs/mock.yml index 3d6c92d288..980151d3e5 100644 --- a/cms/envs/mock.yml +++ b/cms/envs/mock.yml @@ -638,12 +638,12 @@ PLATFORM_TWITTER_ACCOUNT: '@name' POLICY_CHANGE_GRADES_ROUTING_KEY: edx.lms.core.grades_policy_change PRESS_EMAIL: press@example.com PROCTORING_BACKENDS: - DEFAULT: proctortrack - proctortrack: - base_url: https://testing.verificient.com + DEFAULT: mockprock + mockprock: + base_url: https://testing.mockprock.com client_id: test_client_id client_secret: test_client_secret - integration_specific_email: proctortrack-support@example.com + integration_specific_email: mockprock-support@example.com PROCTORING_SETTINGS: ALLOW_CALLBACK_SIMULATION: true LINK_URLS: diff --git a/common/djangoapps/util/proctoring.py b/common/djangoapps/util/proctoring.py new file mode 100644 index 0000000000..34359d721e --- /dev/null +++ b/common/djangoapps/util/proctoring.py @@ -0,0 +1,26 @@ +""" +Utility methods related to proctoring configurations. +""" + + +from django.conf import settings + + +def requires_escalation_email(proctoring_provider): + """ + Returns the value of 'requires_escalation_email' in the given proctoring backend. + The default value for 'requires_escalation_email' is False. + """ + return settings.PROCTORING_BACKENDS.get(proctoring_provider, {}).get( + "requires_escalation_email", False + ) + + +def show_review_rules(proctoring_provider): + """ + Returns the value of 'show_review_rules' in the given proctoring backend. + The default value for 'show_review_rules' is True. + """ + return settings.PROCTORING_BACKENDS.get(proctoring_provider, {}).get( + "show_review_rules", True + ) diff --git a/docs/decisions/0018-standarize-django-po-files.rst b/docs/decisions/0018-standarize-django-po-files.rst index 4020b7c493..7f008f5545 100644 --- a/docs/decisions/0018-standarize-django-po-files.rst +++ b/docs/decisions/0018-standarize-django-po-files.rst @@ -92,7 +92,6 @@ pofiles into the four files and then combine them into two files respectively: - django-partial.po - mako.po - wiki.po - - edx_proctoring_proctortrack.po platform-js.po: - djangojs-partial.po - djangojs-account-settings-view.po diff --git a/lms/djangoapps/discussion/django_comment_client/base/tests_v2.py b/lms/djangoapps/discussion/django_comment_client/base/tests_v2.py index 7bc84e5038..1f5ae78057 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/tests_v2.py +++ b/lms/djangoapps/discussion/django_comment_client/base/tests_v2.py @@ -180,7 +180,8 @@ class ThreadActionGroupIdTestCase( with mock.patch( "openedx.core.djangoapps.django_comment_common.signals.thread_flagged.send" ) as signal_mock: - response = self.call_view("flag_abuse_for_thread", "update_thread_flag") + with self.captureOnCommitCallbacks(execute=True): + response = self.call_view("flag_abuse_for_thread", "update_thread_flag") self._assert_json_response_contains_group_info(response) self.assertEqual(signal_mock.call_count, 1) response = self.call_view("un_flag_abuse_for_thread", "update_thread_flag") @@ -471,10 +472,15 @@ class ViewsTestCase( def assert_discussion_signals(self, signal, user=None): if user is None: user = self.student + # Use captureOnCommitCallbacks to execute on_commit callbacks during tests, + # since signals are now deferred until after transaction commit. + # Order matters: assert_signal_sent must be outer context so the signal + # fires (via captureOnCommitCallbacks) before the assertion check. with self.assert_signal_sent( views, signal, sender=None, user=user, exclude_args=("post",) ): - yield + with self.captureOnCommitCallbacks(execute=True): + yield def test_create_thread(self): with self.assert_discussion_signals("thread_created"): @@ -1218,7 +1224,8 @@ class CommentActionTestCase(CohortedTestCase, MockForumApiMixin): with mock.patch( "openedx.core.djangoapps.django_comment_common.signals.comment_flagged.send" ) as signal_mock: - self.call_view("flag_abuse_for_comment", "update_comment_flag") + with self.captureOnCommitCallbacks(execute=True): + self.call_view("flag_abuse_for_comment", "update_comment_flag") self.assertEqual(signal_mock.call_count, 1) diff --git a/lms/djangoapps/discussion/django_comment_client/base/views.py b/lms/djangoapps/discussion/django_comment_client/base/views.py index 95d5a02010..14ce9c4b57 100644 --- a/lms/djangoapps/discussion/django_comment_client/base/views.py +++ b/lms/djangoapps/discussion/django_comment_client/base/views.py @@ -50,6 +50,7 @@ from lms.djangoapps.discussion.django_comment_client.utils import ( prepare_content, sanitize_body ) +from lms.djangoapps.discussion.rest_api.utils import send_signal_after_commit from openedx.core.djangoapps.django_comment_common.signals import ( comment_created, comment_deleted, @@ -587,7 +588,10 @@ def create_thread(request, course_id, commentable_id): thread.save() - thread_created.send(sender=None, user=user, post=thread) + # Use send_signal_after_commit() to ensure the signal is sent only after the transaction commits. + send_signal_after_commit( + lambda: thread_created.send(sender=None, user=user, post=thread) + ) # patch for backward compatibility to comments service if 'pinned' not in thread.attributes: @@ -598,7 +602,9 @@ def create_thread(request, course_id, commentable_id): if follow: cc_user = cc.User.from_django_user(user) cc_user.follow(thread, course_id) - thread_followed.send(sender=None, user=user, post=thread) + send_signal_after_commit( + lambda: thread_followed.send(sender=None, user=user, post=thread) + ) data = thread.to_dict() @@ -645,7 +651,9 @@ def update_thread(request, course_id, thread_id): thread.save() - thread_edited.send(sender=None, user=user, post=thread) + send_signal_after_commit( + lambda: thread_edited.send(sender=None, user=user, post=thread) + ) track_thread_edited_event(request, course, thread, None) if request.headers.get('x-requested-with') == 'XMLHttpRequest': @@ -688,7 +696,9 @@ def _create_comment(request, course_key, thread_id=None, parent_id=None): ) comment.save(params={"course_id": str(course_key)}) - comment_created.send(sender=None, user=user, post=comment) + send_signal_after_commit( + lambda: comment_created.send(sender=None, user=user, post=comment) + ) followed = post.get('auto_subscribe', 'false').lower() == 'true' @@ -729,7 +739,9 @@ def delete_thread(request, course_id, thread_id): course = get_course_with_access(request.user, 'load', course_key) thread = cc.Thread.find(thread_id) thread.delete(course_id=course_id) - thread_deleted.send(sender=None, user=request.user, post=thread) + send_signal_after_commit( + lambda: thread_deleted.send(sender=None, user=request.user, post=thread) + ) track_thread_deleted_event(request, course, thread) return JsonResponse(prepare_content(thread.to_dict(), course_key)) @@ -751,7 +763,9 @@ def update_comment(request, course_id, comment_id): comment.body = sanitize_body(request.POST["body"]) comment.save(params={"course_id": course_id}) - comment_edited.send(sender=None, user=request.user, post=comment) + send_signal_after_commit( + lambda: comment_edited.send(sender=None, user=request.user, post=comment) + ) track_comment_edited_event(request, course, comment, None) if request.headers.get('x-requested-with') == 'XMLHttpRequest': @@ -776,7 +790,9 @@ def endorse_comment(request, course_id, comment_id): comment.endorsed = endorsed comment.endorsement_user_id = user.id comment.save(params={"course_id": course_id}) - comment_endorsed.send(sender=None, user=user, post=comment) + send_signal_after_commit( + lambda: comment_endorsed.send(sender=None, user=user, post=comment) + ) track_forum_response_mark_event(request, course, comment, endorsed) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -828,7 +844,9 @@ def delete_comment(request, course_id, comment_id): course = get_course_with_access(request.user, 'load', course_key) comment = cc.Comment.find(comment_id) comment.delete(course_id=course_id) - comment_deleted.send(sender=None, user=request.user, post=comment) + send_signal_after_commit( + lambda: comment_deleted.send(sender=None, user=request.user, post=comment) + ) track_comment_deleted_event(request, course, comment) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -847,7 +865,9 @@ def _vote_or_unvote(request, course_id, obj, value='up', undo_vote=False): # (People could theoretically downvote by handcrafting AJAX requests.) else: user.vote(obj, value, course_id) - thread_voted.send(sender=None, user=request.user, post=obj) + send_signal_after_commit( + lambda: thread_voted.send(sender=None, user=request.user, post=obj) + ) track_voted_event(request, course, obj, value, undo_vote) return JsonResponse(prepare_content(obj.to_dict(), course_key)) @@ -861,7 +881,9 @@ def vote_for_comment(request, course_id, comment_id, value): """ comment = cc.Comment.find(comment_id) result = _vote_or_unvote(request, course_id, comment, value) - comment_voted.send(sender=None, user=request.user, post=comment) + send_signal_after_commit( + lambda: comment_voted.send(sender=None, user=request.user, post=comment) + ) return result @@ -914,7 +936,9 @@ def flag_abuse_for_thread(request, course_id, thread_id): thread = cc.Thread.find(thread_id) thread.flagAbuse(user, thread, course_id) track_discussion_reported_event(request, course, thread) - thread_flagged.send(sender='flag_abuse_for_thread', user=request.user, post=thread) + send_signal_after_commit( + lambda: thread_flagged.send(sender='flag_abuse_for_thread', user=request.user, post=thread) + ) return JsonResponse(prepare_content(thread.to_dict(), course_key)) @@ -953,7 +977,9 @@ def flag_abuse_for_comment(request, course_id, comment_id): comment = cc.Comment.find(comment_id) comment.flagAbuse(user, comment, course_id) track_discussion_reported_event(request, course, comment) - comment_flagged.send(sender='flag_abuse_for_comment', user=request.user, post=comment) + send_signal_after_commit( + lambda: comment_flagged.send(sender='flag_abuse_for_comment', user=request.user, post=comment) + ) return JsonResponse(prepare_content(comment.to_dict(), course_key)) @@ -1019,7 +1045,9 @@ def follow_thread(request, course_id, thread_id): # lint-amnesty, pylint: disab course = get_course_by_id(course_key) thread = cc.Thread.find(thread_id) user.follow(thread, course_id=course_id) - thread_followed.send(sender=None, user=request.user, post=thread) + send_signal_after_commit( + lambda: thread_followed.send(sender=None, user=request.user, post=thread) + ) track_thread_followed_event(request, course, thread, True) return JsonResponse({}) @@ -1051,7 +1079,9 @@ def unfollow_thread(request, course_id, thread_id): # lint-amnesty, pylint: dis user = cc.User.from_django_user(request.user) thread = cc.Thread.find(thread_id) user.unfollow(thread, course_id=course_id) - thread_unfollowed.send(sender=None, user=request.user, post=thread) + send_signal_after_commit( + lambda: thread_unfollowed.send(sender=None, user=request.user, post=thread) + ) track_thread_followed_event(request, course, thread, False) return JsonResponse({}) diff --git a/lms/djangoapps/discussion/rest_api/api.py b/lms/djangoapps/discussion/rest_api/api.py index b87852c16c..1c0b057352 100644 --- a/lms/djangoapps/discussion/rest_api/api.py +++ b/lms/djangoapps/discussion/rest_api/api.py @@ -128,6 +128,7 @@ from .utils import ( discussion_open_for_user, get_usernames_for_course, get_usernames_from_search_string, + send_signal_after_commit, set_attribute, is_posting_allowed, can_user_notify_all_learners, is_captcha_enabled, get_captcha_site_key_by_platform @@ -1382,7 +1383,9 @@ def _handle_following_field(form_value, user, cc_content, request): else: user.unfollow(cc_content) signal = thread_followed if form_value else thread_unfollowed - signal.send(sender=None, user=user, post=cc_content) + send_signal_after_commit( + lambda: signal.send(sender=None, user=user, post=cc_content) + ) track_thread_followed_event(request, course, cc_content, form_value) @@ -1395,9 +1398,13 @@ def _handle_abuse_flagged_field(form_value, user, cc_content, request): track_discussion_reported_event(request, course, cc_content) if ENABLE_DISCUSSIONS_MFE.is_enabled(course_key): if cc_content.type == 'thread': - thread_flagged.send(sender='flag_abuse_for_thread', user=user, post=cc_content) + send_signal_after_commit( + lambda: thread_flagged.send(sender='flag_abuse_for_thread', user=user, post=cc_content) + ) else: - comment_flagged.send(sender='flag_abuse_for_comment', user=user, post=cc_content) + send_signal_after_commit( + lambda: comment_flagged.send(sender='flag_abuse_for_comment', user=user, post=cc_content) + ) else: remove_all = bool(is_privileged_user(course_key, User.objects.get(id=user.id))) cc_content.unFlagAbuse(user, cc_content, remove_all) @@ -1407,7 +1414,9 @@ def _handle_abuse_flagged_field(form_value, user, cc_content, request): def _handle_voted_field(form_value, cc_content, api_content, request, context): """vote or undo vote on thread/comment""" signal = thread_voted if cc_content.type == 'thread' else comment_voted - signal.send(sender=None, user=context["request"].user, post=cc_content) + send_signal_after_commit( + lambda: signal.send(sender=None, user=context["request"].user, post=cc_content) + ) if form_value: context["cc_requester"].vote(cc_content, "up") api_content["vote_count"] += 1 @@ -1452,7 +1461,9 @@ def _handle_comment_signals(update_data, comment, user, sender=None): """ for key, value in update_data.items(): if key == "endorsed" and value is True: - comment_endorsed.send(sender=sender, user=user, post=comment) + send_signal_after_commit( + lambda: comment_endorsed.send(sender=sender, user=user, post=comment) + ) def create_thread(request, thread_data): @@ -1502,7 +1513,10 @@ def create_thread(request, thread_data): raise ValidationError(dict(list(serializer.errors.items()) + list(actions_form.errors.items()))) serializer.save() cc_thread = serializer.instance - thread_created.send(sender=None, user=user, post=cc_thread, notify_all_learners=notify_all_learners) + # Use send_signal_after_commit() to ensure the signal is sent only after the transaction commits. + send_signal_after_commit( + lambda: thread_created.send(sender=None, user=user, post=cc_thread, notify_all_learners=notify_all_learners) + ) api_thread = serializer.data _do_extra_actions(api_thread, cc_thread, list(thread_data.keys()), actions_form, context, request) @@ -1550,7 +1564,9 @@ def create_comment(request, comment_data): context["cc_requester"].follow(cc_thread) serializer.save() cc_comment = serializer.instance - comment_created.send(sender=None, user=request.user, post=cc_comment) + send_signal_after_commit( + lambda: comment_created.send(sender=None, user=request.user, post=cc_comment) + ) api_comment = serializer.data _do_extra_actions(api_comment, cc_comment, list(comment_data.keys()), actions_form, context, request) track_comment_created_event(request, course, cc_comment, cc_thread["commentable_id"], followed=False, @@ -1586,7 +1602,9 @@ def update_thread(request, thread_id, update_data): if set(update_data) - set(actions_form.fields): serializer.save() # signal to update Teams when a user edits a thread - thread_edited.send(sender=None, user=request.user, post=cc_thread) + send_signal_after_commit( + lambda: thread_edited.send(sender=None, user=request.user, post=cc_thread) + ) api_thread = serializer.data _do_extra_actions(api_thread, cc_thread, list(update_data.keys()), actions_form, context, request) @@ -1635,7 +1653,9 @@ def update_comment(request, comment_id, update_data): # Only save comment object if some of the edited fields are in the comment data, not extra actions if set(update_data) - set(actions_form.fields): serializer.save() - comment_edited.send(sender=None, user=request.user, post=cc_comment) + send_signal_after_commit( + lambda: comment_edited.send(sender=None, user=request.user, post=cc_comment) + ) api_comment = serializer.data _do_extra_actions(api_comment, cc_comment, list(update_data.keys()), actions_form, context, request) _handle_comment_signals(update_data, cc_comment, request.user) @@ -1823,7 +1843,9 @@ def delete_thread(request, thread_id): cc_thread, context = _get_thread_and_context(request, thread_id) if can_delete(cc_thread, context): cc_thread.delete() - thread_deleted.send(sender=None, user=request.user, post=cc_thread) + send_signal_after_commit( + lambda: thread_deleted.send(sender=None, user=request.user, post=cc_thread) + ) track_thread_deleted_event(request, context["course"], cc_thread) else: raise PermissionDenied @@ -1848,7 +1870,9 @@ def delete_comment(request, comment_id): cc_comment, context = _get_comment_and_context(request, comment_id) if can_delete(cc_comment, context): cc_comment.delete() - comment_deleted.send(sender=None, user=request.user, post=cc_comment) + send_signal_after_commit( + lambda: comment_deleted.send(sender=None, user=request.user, post=cc_comment) + ) track_comment_deleted_event(request, context["course"], cc_comment) else: raise PermissionDenied diff --git a/lms/djangoapps/discussion/rest_api/tests/test_api_v2.py b/lms/djangoapps/discussion/rest_api/tests/test_api_v2.py index 900d52017c..df4ac947bf 100644 --- a/lms/djangoapps/discussion/rest_api/tests/test_api_v2.py +++ b/lms/djangoapps/discussion/rest_api/tests/test_api_v2.py @@ -273,7 +273,8 @@ class CreateThreadTest( with self.assert_signal_sent( api, "thread_created", sender=None, user=self.user, exclude_args=("post", "notify_all_learners") ): - actual = create_thread(self.request, self.minimal_data) + with self.captureOnCommitCallbacks(execute=True): + actual = create_thread(self.request, self.minimal_data) expected = self.expected_thread_data( { "id": "test_id", @@ -352,7 +353,8 @@ class CreateThreadTest( with self.assert_signal_sent( api, "thread_created", sender=None, user=self.user, exclude_args=("post", "notify_all_learners") ): - actual = create_thread(self.request, self.minimal_data) + with self.captureOnCommitCallbacks(execute=True): + actual = create_thread(self.request, self.minimal_data) expected = self.expected_thread_data( { "author_label": "Moderator", @@ -428,7 +430,8 @@ class CreateThreadTest( with self.assert_signal_sent( api, "thread_created", sender=None, user=self.user, exclude_args=("post", "notify_all_learners") ): - create_thread(self.request, data) + with self.captureOnCommitCallbacks(execute=True): + create_thread(self.request, data) event_name, event_data = mock_emit.call_args[0] assert event_name == "edx.forum.thread.created" assert event_data == { @@ -678,7 +681,8 @@ class CreateCommentTest( with self.assert_signal_sent( api, "comment_created", sender=None, user=self.user, exclude_args=("post",) ): - actual = create_comment(self.request, data) + with self.captureOnCommitCallbacks(execute=True): + actual = create_comment(self.request, data) expected = { "id": "test_comment", "thread_id": "test_thread", @@ -785,7 +789,8 @@ class CreateCommentTest( with self.assert_signal_sent( api, "comment_created", sender=None, user=self.user, exclude_args=("post",) ): - actual = create_comment(self.request, data) + with self.captureOnCommitCallbacks(execute=True): + actual = create_comment(self.request, data) expected = { "id": "test_comment", "thread_id": "test_thread", @@ -1118,9 +1123,10 @@ class UpdateThreadTest( with self.assert_signal_sent( api, "thread_edited", sender=None, user=self.user, exclude_args=("post",) ): - actual = update_thread( - self.request, "test_thread", {"raw_body": "Edited body"} - ) + with self.captureOnCommitCallbacks(execute=True): + actual = update_thread( + self.request, "test_thread", {"raw_body": "Edited body"} + ) assert actual == self.expected_thread_data( { @@ -1436,13 +1442,13 @@ class UpdateThreadTest( self.register_thread() data = {"following": new_following} signal_name = "thread_followed" if new_following else "thread_unfollowed" - mock_path = ( - f"openedx.core.djangoapps.django_comment_common.signals.{signal_name}.send" - ) + # Patch at the api module level where the signal is imported and used + mock_path = f"lms.djangoapps.discussion.rest_api.api.{signal_name}" with mock.patch(mock_path) as signal_patch: - result = update_thread(self.request, "test_thread", data) + with self.captureOnCommitCallbacks(execute=True): + result = update_thread(self.request, "test_thread", data) if old_following != new_following: - self.assertEqual(signal_patch.call_count, 1) + self.assertEqual(signal_patch.send.call_count, 1) assert result["following"] == new_following if old_following == new_following: @@ -1782,9 +1788,10 @@ class UpdateCommentTest( with self.assert_signal_sent( api, "comment_edited", sender=None, user=self.user, exclude_args=("post",) ): - actual = update_comment( - self.request, "test_comment", {"raw_body": "Edited body"} - ) + with self.captureOnCommitCallbacks(execute=True): + actual = update_comment( + self.request, "test_comment", {"raw_body": "Edited body"} + ) expected = { "anonymous": False, "anonymous_to_peers": False, @@ -2207,7 +2214,7 @@ class UpdateCommentTest( ) @ddt.unpack @mock.patch( - "openedx.core.djangoapps.django_comment_common.signals.comment_endorsed.send" + "lms.djangoapps.discussion.rest_api.api.comment_endorsed.send" ) def test_endorsed_access( self, role_name, is_thread_author, thread_type, is_comment_author, endorsed_mock @@ -2226,7 +2233,8 @@ class UpdateCommentTest( thread_type == "discussion" or not is_thread_author ) try: - update_comment(self.request, "test_comment", {"endorsed": True}) + with self.captureOnCommitCallbacks(execute=True): + update_comment(self.request, "test_comment", {"endorsed": True}) self.assertEqual(endorsed_mock.call_count, 1) assert not expected_error except ValidationError as err: @@ -2354,7 +2362,8 @@ class DeleteThreadTest( with self.assert_signal_sent( api, "thread_deleted", sender=None, user=self.user, exclude_args=("post",) ): - assert delete_thread(self.request, self.thread_id) is None + with self.captureOnCommitCallbacks(execute=True): + assert delete_thread(self.request, self.thread_id) is None self.check_mock_called("delete_thread") params = { "thread_id": self.thread_id, @@ -2540,7 +2549,8 @@ class DeleteCommentTest( with self.assert_signal_sent( api, "comment_deleted", sender=None, user=self.user, exclude_args=("post",) ): - assert delete_comment(self.request, self.comment_id) is None + with self.captureOnCommitCallbacks(execute=True): + assert delete_comment(self.request, self.comment_id) is None self.check_mock_called("delete_comment") params = { "comment_id": self.comment_id, diff --git a/lms/djangoapps/discussion/rest_api/utils.py b/lms/djangoapps/discussion/rest_api/utils.py index 0f02a0dcdc..a2591655ad 100644 --- a/lms/djangoapps/discussion/rest_api/utils.py +++ b/lms/djangoapps/discussion/rest_api/utils.py @@ -3,13 +3,14 @@ Utils for discussion API. """ import logging from datetime import datetime -from typing import Dict, List +from typing import Callable, Dict, List import requests from crum import get_current_request from django.conf import settings from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.core.paginator import Paginator +from django.db import transaction from django.db.models.functions import Length from pytz import UTC @@ -496,3 +497,24 @@ def get_captcha_site_key_by_platform(platform: str) -> str | None: Get reCAPTCHA site key based on the platform. """ return settings.RECAPTCHA_SITE_KEYS.get(platform, None) + + +def send_signal_after_commit(signal_func: Callable): + """ + Schedule a signal to be sent after the current database transaction commits. + + This helper ensures that signals are only sent after the transaction commits, + preventing race conditions where async tasks (like Celery workers) may try to + access database records before they are visible (especially important for MySQL + backend with transaction isolation). + + Args: + signal_func: A callable that sends the signal. This will be executed + after the transaction commits. + + Example: + send_signal_after_commit( + lambda: thread_created.send(sender=None, user=user, post=thread, notify_all_learners=False) + ) + """ + transaction.on_commit(signal_func) diff --git a/lms/djangoapps/instructor/tests/test_proctoring.py b/lms/djangoapps/instructor/tests/test_proctoring.py index 4b196f307b..6a87538826 100644 --- a/lms/djangoapps/instructor/tests/test_proctoring.py +++ b/lms/djangoapps/instructor/tests/test_proctoring.py @@ -141,7 +141,6 @@ class TestProctoringDashboardViews(SharedModuleStoreTestCase): { 'DEFAULT': 'test_proctoring_provider', 'test_proctoring_provider': {}, - 'proctortrack': {} }, ) @ddt.data( @@ -149,9 +148,9 @@ class TestProctoringDashboardViews(SharedModuleStoreTestCase): ('test_proctoring_provider', 'foo@bar.com') ) @ddt.unpack - def test_non_proctortrack_provider(self, proctoring_provider, escalation_email): + def test_requires_escalation_email_unset(self, proctoring_provider, escalation_email): """ - Escalation email will not be visible if proctortrack is not the proctoring provider, with or without + Escalation email will not be visible if 'requires_escalation_email' is not set, with or without escalation email provided. """ self.setup_course_with_proctoring_backend(proctoring_provider, escalation_email) @@ -164,17 +163,16 @@ class TestProctoringDashboardViews(SharedModuleStoreTestCase): settings.PROCTORING_BACKENDS, { 'DEFAULT': 'test_proctoring_provider', - 'test_proctoring_provider': {}, - 'proctortrack': {} + 'test_proctoring_provider': {"requires_escalation_email": True}, }, ) @patch.dict(settings.FEATURES, {'ENABLE_PROCTORED_EXAMS': True}) - def test_proctortrack_provider_with_email(self): + def test_requires_escalation_email_set_with_email(self): """ - Escalation email will be visible if proctortrack is the proctoring provider, and there + Escalation email will be visible if 'requires_escalation_email' is set, and there is an escalation email provided. """ - self.setup_course_with_proctoring_backend('proctortrack', 'foo@bar.com') + self.setup_course_with_proctoring_backend('test_proctoring_provider', 'foo@bar.com') self.instructor.is_staff = True self.instructor.save() diff --git a/lms/djangoapps/instructor/views/instructor_dashboard.py b/lms/djangoapps/instructor/views/instructor_dashboard.py index 2e1703fa1a..f3b2dc9bc7 100644 --- a/lms/djangoapps/instructor/views/instructor_dashboard.py +++ b/lms/djangoapps/instructor/views/instructor_dashboard.py @@ -34,6 +34,7 @@ from common.djangoapps.student.roles import ( CourseStaffRole ) from common.djangoapps.util.json_request import JsonResponse +from common.djangoapps.util.proctoring import requires_escalation_email from lms.djangoapps.bulk_email.api import is_bulk_email_feature_enabled from lms.djangoapps.bulk_email.models_api import is_bulk_email_disabled_for_course from lms.djangoapps.certificates import api as certs_api @@ -320,7 +321,7 @@ def _section_special_exams(course, access): else: # Only call does_backend_support_onboarding if not using an LTI proctoring provider show_onboarding = does_backend_support_onboarding(course.proctoring_provider) - if proctoring_provider == 'proctortrack': + if requires_escalation_email(course.proctoring_provider): escalation_email = course.proctoring_escalation_email from edx_proctoring.api import is_backend_dashboard_available show_dashboard = is_backend_dashboard_available(course_key) diff --git a/lms/envs/common.py b/lms/envs/common.py index 85b4b8af06..03eb67cd5d 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -3343,14 +3343,6 @@ VIDEO_UPLOAD_PIPELINE = { 'ROOT_PATH': '', } -### Proctoring configuration (redirct URLs and keys shared between systems) #### -PROCTORING_BACKENDS = { - 'DEFAULT': 'null', - # The null key needs to be quoted because - # null is a language independent type in YAML - 'null': {} -} - PROCTORED_EXAM_VIEWABLE_PAST_DUE = False ######################### rate limit for yt_video_metadata api ############## diff --git a/lms/envs/mock.yml b/lms/envs/mock.yml index 9c6b077dbe..47a10eea94 100644 --- a/lms/envs/mock.yml +++ b/lms/envs/mock.yml @@ -977,7 +977,7 @@ PRESS_EMAIL: press@example.com PROCTORING_BACKENDS: DEFAULT: 'null' 'null': {} - proctortrack: + mockprock: base_url: hello client_id: '[encrypted]' client_secret: '[encrypted]' diff --git a/lms/templates/instructor/instructor_dashboard_2/special_exams.html b/lms/templates/instructor/instructor_dashboard_2/special_exams.html index 2658af0bc7..d652842df2 100644 --- a/lms/templates/instructor/instructor_dashboard_2/special_exams.html +++ b/lms/templates/instructor/instructor_dashboard_2/special_exams.html @@ -11,7 +11,7 @@ import pytz % else: % if section_data.get('escalation_email'): -
${_('Proctortrack Escalation Email:')} ${ section_data['escalation_email'] }
+
${_('Proctoring Escalation Email:')} ${ section_data['escalation_email'] }
% endif
diff --git a/openedx/envs/common.py b/openedx/envs/common.py index 86f6d9e1dc..4d8c223d25 100644 --- a/openedx/envs/common.py +++ b/openedx/envs/common.py @@ -2390,7 +2390,33 @@ ENTRANCE_EXAM_MIN_SCORE_PCT = 50 # Initialize to 'release', but read from JSON in production.py EDX_PLATFORM_REVISION = 'release' -# Proctoring configuration (redirct URLs and keys shared between systems) +# .. setting_name: PROCTORING_BACKENDS +# .. setting_description: A dictionary describing all available proctoring provider configurations. +# Structure: +# { +# "": { +# "show_review_rules": , +# "requires_escalation_email": , +# ... additional provider-specific options ... +# }, +# "": { ... } +# ... +# "DEFAULT": "", +# } +# +# Keys: +# +# **show_review_rules** (bool): +# Whether studio would show a "Review Rules" field as part of proctoring configuration. +# Default is True. +# +# **requires_escalation_email** (bool): +# Providers with this flag set to True require that an escalation email address be +# specified in the advanced course settings. Default is False. +# .. setting_default: { +# 'DEFAULT': 'null', +# 'null': {} +# } PROCTORING_BACKENDS = { 'DEFAULT': 'null', # The null key needs to be quoted because diff --git a/requirements/edx/kernel.in b/requirements/edx/kernel.in index 38bf64f8ed..4c2ed2c145 100644 --- a/requirements/edx/kernel.in +++ b/requirements/edx/kernel.in @@ -80,7 +80,6 @@ edx-opaque-keys>=2.12.0 edx-organizations edx-proctoring>=2.0.1 # using hash to support django42 -# edx-proctoring-proctortrack==1.2.1 # Intentionally and permanently pinned to ensure code changes are reviewed edx-rest-api-client edx-search edx-submissions diff --git a/xmodule/course_block.py b/xmodule/course_block.py index 9d24f15306..0ebd00a3e5 100644 --- a/xmodule/course_block.py +++ b/xmodule/course_block.py @@ -283,7 +283,10 @@ class ProctoringProvider(String): return default -def get_available_providers(): # lint-amnesty, pylint: disable=missing-function-docstring +def get_available_providers() -> list[str]: + """ + Return list of available proctoring providers. + """ proctoring_backend_settings = getattr( settings, 'PROCTORING_BACKENDS', @@ -296,6 +299,24 @@ def get_available_providers(): # lint-amnesty, pylint: disable=missing-function return available_providers +def get_requires_escalation_email_providers() -> list[str]: + """ + Return list of available proctoring providers that require an escalation email. + """ + requires_escalation_email_providers = [ + provider + for provider in settings.PROCTORING_BACKENDS + if provider != "DEFAULT" + and settings.PROCTORING_BACKENDS[provider].get( + "requires_escalation_email", False + ) + ] + # Add lti_external unconditionally since it always requires an escalation email + requires_escalation_email_providers.append('lti_external') + requires_escalation_email_providers.sort() + return requires_escalation_email_providers + + class TeamsConfigField(Dict): """ XBlock field for teams configuration, including definitions for teamsets. @@ -878,9 +899,9 @@ class CourseFields: # lint-amnesty, pylint: disable=missing-class-docstring ) proctoring_escalation_email = EmailString( - display_name=_("Proctortrack Exam Escalation Contact"), + display_name=_("Proctoring Exam Escalation Contact"), help=_( - "Required if 'proctortrack' is selected as your proctoring provider. " + "Required if 'requires_escalation_email' is set in the proctoring backend." "Enter an email address to be contacted by the support team whenever there are escalations " "(e.g. appeals, delayed reviews, etc.)." ), diff --git a/xmodule/static/css-builtin-blocks/VideoBlockDisplay.css b/xmodule/static/css-builtin-blocks/VideoBlockDisplay.css index 9584a1dc76..401a798804 100644 --- a/xmodule/static/css-builtin-blocks/VideoBlockDisplay.css +++ b/xmodule/static/css-builtin-blocks/VideoBlockDisplay.css @@ -708,6 +708,10 @@ line-height: lh(); } +.xmodule_display.xmodule_VideoBlock .video .subtitles .subtitles-menu li:has(> span:empty) { + display: none; +} + .xmodule_display.xmodule_VideoBlock .video .subtitles .subtitles-menu li span { display: block; }