diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 4561926c1b8d..65cbc682a365 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -1345,23 +1345,23 @@ def test_create_course_with_course_creation_disabled_not_staff(self): self.user.save() self.assert_course_permission_denied() + @override_settings(ENABLE_CREATOR_GROUP=True) def test_create_course_no_course_creators_staff(self): """Test new course creation -- course creation group enabled, staff, group is empty.""" - with mock.patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}): - self.assert_created_course() + self.assert_created_course() + @override_settings(ENABLE_CREATOR_GROUP=True) def test_create_course_no_course_creators_not_staff(self): """Test new course creation -- error path for course creator group enabled, not staff, group is empty.""" - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - self.user.is_staff = False - self.user.save() - self.assert_course_permission_denied() + self.user.is_staff = False + self.user.save() + self.assert_course_permission_denied() + @override_settings(ENABLE_CREATOR_GROUP=True) def test_create_course_with_course_creator(self): """Test new course creation -- use course creator group""" - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - auth.add_users(self.user, CourseCreatorRole(), self.user) - self.assert_created_course() + auth.add_users(self.user, CourseCreatorRole(), self.user) + self.assert_created_course() def test_create_course_with_unicode_in_id_disabled(self): """ @@ -2014,13 +2014,13 @@ def test_rerun_course_fail_duplicate_course(self): # Verify that the existing course continues to be in the course listing self.assertInCourseListing(existent_course_key) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_rerun_with_permission_denied(self): - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - source_course = CourseFactory.create(default_store=ModuleStoreEnum.Type.split) - auth.add_users(self.user, CourseCreatorRole(), self.user) - self.user.is_staff = False - self.user.save() - self.post_rerun_request(source_course.id, response_code=403, expect_error=True) + source_course = CourseFactory.create(default_store=ModuleStoreEnum.Type.split) + auth.add_users(self.user, CourseCreatorRole(), self.user) + self.user.is_staff = False + self.user.save() + self.post_rerun_request(source_course.id, response_code=403, expect_error=True) def test_rerun_error(self): error_message = "Mock Error Message" diff --git a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py index fbcf56067ac1..8d274c1c9564 100644 --- a/cms/djangoapps/contentstore/tests/test_course_create_rerun.py +++ b/cms/djangoapps/contentstore/tests/test_course_create_rerun.py @@ -195,7 +195,7 @@ def test_course_creation_for_known_organization(self, organizations_autocreate): self.assertEqual(len(course_orgs), 1) # noqa: PT009 self.assertEqual(course_orgs[0]['short_name'], 'orgX') # noqa: PT009 - @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_course_creation_when_user_not_in_org(self): """ Tests course creation when user doesn't have the required role. @@ -208,7 +208,7 @@ def test_course_creation_when_user_not_in_org(self): }) self.assertEqual(response.status_code, 403) # noqa: PT009 - @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @override_settings(ENABLE_CREATOR_GROUP=True) @mock.patch( 'cms.djangoapps.course_creators.admin.render_to_string', mock.Mock(side_effect=mock_render_to_string, autospec=True) @@ -235,7 +235,7 @@ def test_course_creation_when_user_in_org_with_creator_role(self): }) self.assertEqual(response.status_code, 200) # noqa: PT009 - @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @override_settings(ENABLE_CREATOR_GROUP=True) @mock.patch( 'cms.djangoapps.course_creators.admin.render_to_string', mock.Mock(side_effect=mock_render_to_string, autospec=True) @@ -262,7 +262,7 @@ def test_course_creation_with_all_org_checked(self): }) self.assertEqual(response.status_code, 200) # noqa: PT009 - @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @override_settings(ENABLE_CREATOR_GROUP=True) @mock.patch( 'cms.djangoapps.course_creators.admin.render_to_string', mock.Mock(side_effect=mock_render_to_string, autospec=True) @@ -291,7 +291,7 @@ def test_course_creation_with_permission_for_specific_organization(self): }) self.assertEqual(response.status_code, 200) # noqa: PT009 - @override_settings(FEATURES={'ENABLE_CREATOR_GROUP': True}) + @override_settings(ENABLE_CREATOR_GROUP=True) @mock.patch( 'cms.djangoapps.course_creators.admin.render_to_string', mock.Mock(side_effect=mock_render_to_string, autospec=True) diff --git a/cms/djangoapps/contentstore/tests/test_libraries.py b/cms/djangoapps/contentstore/tests/test_libraries.py index 29a194fd017e..9ce85d525d26 100644 --- a/cms/djangoapps/contentstore/tests/test_libraries.py +++ b/cms/djangoapps/contentstore/tests/test_libraries.py @@ -574,12 +574,12 @@ def test_creation(self): # Now check that logged-in users without CourseCreator role cannot create libraries self._login_as_non_staff_user(logout_first=False) - with patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}): + with override_settings(ENABLE_CREATOR_GROUP=True): self._assert_cannot_create_library(expected_code=403) # 403 user is not CourseCreator # Now check that logged-in users with CourseCreator role can create libraries add_user_with_status_granted(self.user, self.non_staff_user) - with patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}): + with override_settings(ENABLE_CREATOR_GROUP=True): lib_key2 = self._create_library(library="lib2", display_name="Test Library 2") library2 = modulestore().get_library(lib_key2) self.assertIsNotNone(library2) # noqa: PT009 diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 569871b95bea..89eac5762cc3 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -2021,7 +2021,7 @@ def _get_course_creator_status(user): course_creator_status = 'granted' elif settings.FEATURES.get('DISABLE_COURSE_CREATION', False): course_creator_status = 'disallowed_for_this_site' - elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + elif getattr(settings, 'ENABLE_CREATOR_GROUP', False): course_creator_status = get_course_creator_status(user) if course_creator_status is None: # User not grandfathered in as an existing user, has not previously visited the dashboard page. @@ -2038,7 +2038,7 @@ def get_allowed_organizations(user): """ Helper method for returning the list of organizations for which the user is allowed to create courses. """ - if settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + if getattr(settings, 'ENABLE_CREATOR_GROUP', False): return get_organizations(user) else: return [] @@ -2058,7 +2058,7 @@ def get_allowed_organizations_for_libraries(user): # This allows people in the course creator group for an org to create # libraries, which mimics course behavior. - if settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + if getattr(settings, 'ENABLE_CREATOR_GROUP', False): organizations_set.update(get_organizations(user)) return sorted(organizations_set) @@ -2068,7 +2068,7 @@ def user_can_create_organizations(user): """ Returns True if the user can create organizations. """ - return user.is_staff or not settings.FEATURES.get('ENABLE_CREATOR_GROUP', False) + return user.is_staff or not getattr(settings, 'ENABLE_CREATOR_GROUP', False) def get_organizations_for_non_course_creators(user): diff --git a/cms/djangoapps/contentstore/views/library.py b/cms/djangoapps/contentstore/views/library.py index 70ae0bce2c2a..31ae21ce7d60 100644 --- a/cms/djangoapps/contentstore/views/library.py +++ b/cms/djangoapps/contentstore/views/library.py @@ -72,7 +72,7 @@ def _user_can_create_library_for_org(user, org=None): return False elif user.is_staff: return True - elif settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + elif getattr(settings, 'ENABLE_CREATOR_GROUP', False): is_course_creator = get_course_creator_status(user) == 'granted' if is_course_creator: return True diff --git a/cms/djangoapps/contentstore/views/tests/test_library.py b/cms/djangoapps/contentstore/views/tests/test_library.py index 172f87addb56..203f2b765a52 100644 --- a/cms/djangoapps/contentstore/views/tests/test_library.py +++ b/cms/djangoapps/contentstore/views/tests/test_library.py @@ -70,7 +70,7 @@ def test_library_creator_status_with_no_course_creator_role(self): @mock.patch("cms.djangoapps.contentstore.toggles.libraries_v1_enabled", True) def test_library_creator_status_for_enabled_creator_group_setting_for_non_staff_users(self): _, nostaff_user = self.create_non_staff_authed_user_client() - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): + with override_settings(ENABLE_CREATOR_GROUP=True): self.assertEqual(user_can_create_library(nostaff_user, None), False) # noqa: PT009 # Global staff can create libraries for any org, even ones that don't exist. @@ -88,14 +88,14 @@ def test_library_creator_status_with_is_staff_user_no_org(self): # When creator groups are enabled, global staff can create libraries in any org @mock.patch("cms.djangoapps.contentstore.toggles.libraries_v1_enabled", True) def test_library_creator_status_for_enabled_creator_group_setting_with_is_staff_user(self): - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): + with override_settings(ENABLE_CREATOR_GROUP=True): self.assertEqual(user_can_create_library(self.user, 'RandomOrg'), True) # noqa: PT009 # When creator groups are enabled, course creators can create libraries in any org. @mock.patch("cms.djangoapps.contentstore.toggles.libraries_v1_enabled", True) def test_library_creator_status_with_course_creator_role_for_enabled_creator_group_setting(self): _, nostaff_user = self.create_non_staff_authed_user_client() - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): + with override_settings(ENABLE_CREATOR_GROUP=True): grant_course_creator_status(self.user, nostaff_user) self.assertEqual(user_can_create_library(nostaff_user, 'soMeRandOmoRg'), True) # noqa: PT009 @@ -104,7 +104,7 @@ def test_library_creator_status_with_course_creator_role_for_enabled_creator_gro @mock.patch("cms.djangoapps.contentstore.toggles.libraries_v1_enabled", True) def test_library_creator_status_with_course_staff_role_for_enabled_creator_group_setting(self): _, nostaff_user = self.create_non_staff_authed_user_client() - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): + with override_settings(ENABLE_CREATOR_GROUP=True): auth.add_users(self.user, CourseStaffRole(self.course.id), nostaff_user) self.assertEqual(user_can_create_library(nostaff_user, self.course.org), True) # noqa: PT009 self.assertEqual(user_can_create_library(nostaff_user, 'SomEOtherOrg'), False) # noqa: PT009 @@ -114,7 +114,7 @@ def test_library_creator_status_with_course_staff_role_for_enabled_creator_group @mock.patch("cms.djangoapps.contentstore.toggles.libraries_v1_enabled", True) def test_library_creator_status_with_course_instructor_role_for_enabled_creator_group_setting(self): _, nostaff_user = self.create_non_staff_authed_user_client() - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): + with override_settings(ENABLE_CREATOR_GROUP=True): auth.add_users(self.user, CourseInstructorRole(self.course.id), nostaff_user) self.assertEqual(user_can_create_library(nostaff_user, self.course.org), True) # noqa: PT009 self.assertEqual(user_can_create_library(nostaff_user, 'SomEOtherOrg'), False) # noqa: PT009 @@ -206,7 +206,7 @@ def test_create_library(self): self.assertEqual(response.status_code, 200) # noqa: PT009 # That's all we check. More detailed tests are in contentstore.tests.test_libraries... - @patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_lib_create_permission(self): """ Users who are given course creator roles should be able to create libraries. @@ -220,7 +220,7 @@ def test_lib_create_permission(self): }) self.assertEqual(response.status_code, 200) # noqa: PT009 - @patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': False}) + @override_settings(ENABLE_CREATOR_GROUP=False) def test_lib_create_permission_no_course_creator_role_and_no_course_creator_group(self): """ Users who are not given course creator roles should still be able to create libraries @@ -234,7 +234,7 @@ def test_lib_create_permission_no_course_creator_role_and_no_course_creator_grou }) self.assertEqual(response.status_code, 200) # noqa: PT009 - @patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_lib_create_permission_no_course_creator_role_and_no_course_creator_group_and_no_course_staff_role(self): """ Users who are not given course creator roles or course staff role should not be able to create libraries @@ -248,7 +248,7 @@ def test_lib_create_permission_no_course_creator_role_and_no_course_creator_grou }) self.assertEqual(response.status_code, 403) # noqa: PT009 - @patch.dict('django.conf.settings.FEATURES', {'ENABLE_CREATOR_GROUP': True}) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_lib_create_permission_course_staff_role(self): """ Users who are staff on any existing course should able to create libraries @@ -492,14 +492,11 @@ def test_allowed_organizations_for_library(self): 'django.conf.settings.FEATURES', {"ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES": False} ): - with mock.patch.dict( - 'django.conf.settings.FEATURES', - {"ENABLE_CREATOR_GROUP": False} - ): + with override_settings(ENABLE_CREATOR_GROUP=False): organizations = get_allowed_organizations_for_libraries(self.user) # Assert that the method returned the expected value self.assertEqual(organizations, []) # noqa: PT009 - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): + with override_settings(ENABLE_CREATOR_GROUP=True): # Assert that correct org values are returned based on course creator state for course_creator_state in CourseCreator.STATES: course_creator.state = course_creator_state diff --git a/cms/djangoapps/contentstore/views/tests/test_organizations.py b/cms/djangoapps/contentstore/views/tests/test_organizations.py index d88420eac8c2..be9feb2c3000 100644 --- a/cms/djangoapps/contentstore/views/tests/test_organizations.py +++ b/cms/djangoapps/contentstore/views/tests/test_organizations.py @@ -89,44 +89,44 @@ def setUpTestData(cls): ) @override_settings( + ENABLE_CREATOR_GROUP=False, FEATURES={ **settings.FEATURES, 'ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES': False, - 'ENABLE_CREATOR_GROUP': False, - } + }, ) def test_both_toggles_disabled(self): allowed_orgs = get_allowed_organizations_for_libraries(self.library_author) assert allowed_orgs == [] @override_settings( + ENABLE_CREATOR_GROUP=True, FEATURES={ **settings.FEATURES, 'ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES': True, - 'ENABLE_CREATOR_GROUP': True, - } + }, ) def test_both_toggles_enabled(self): allowed_orgs = get_allowed_organizations_for_libraries(self.library_author) assert allowed_orgs == ["CreatorOrg", "OrgStaffOrg"] @override_settings( + ENABLE_CREATOR_GROUP=False, FEATURES={ **settings.FEATURES, 'ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES': True, - 'ENABLE_CREATOR_GROUP': False, - } + }, ) def test_org_staff_enabled(self): allowed_orgs = get_allowed_organizations_for_libraries(self.library_author) assert allowed_orgs == ["OrgStaffOrg"] @override_settings( + ENABLE_CREATOR_GROUP=True, FEATURES={ **settings.FEATURES, 'ENABLE_ORGANIZATION_STAFF_ACCESS_FOR_CONTENT_LIBRARIES': False, - 'ENABLE_CREATOR_GROUP': True, - } + }, ) def test_creator_group_enabled(self): allowed_orgs = get_allowed_organizations_for_libraries(self.library_author) diff --git a/cms/djangoapps/course_creators/tests/test_admin.py b/cms/djangoapps/course_creators/tests/test_admin.py index 28b9a6a61862..48fcc7ae7569 100644 --- a/cms/djangoapps/course_creators/tests/test_admin.py +++ b/cms/djangoapps/course_creators/tests/test_admin.py @@ -9,6 +9,7 @@ from django.core import mail from django.http import HttpRequest from django.test import TestCase +from django.test.utils import override_settings from cms.djangoapps.course_creators.admin import CourseCreatorAdmin from cms.djangoapps.course_creators.models import CourseCreator @@ -52,10 +53,10 @@ def setUp(self): self.studio_request_email = 'mark@marky.mark' self.enable_creator_group_patch = { - "ENABLE_CREATOR_GROUP": True, - "STUDIO_REQUEST_EMAIL": self.studio_request_email + "STUDIO_REQUEST_EMAIL": self.studio_request_email, } + @override_settings(ENABLE_CREATOR_GROUP=True) @mock.patch( 'cms.djangoapps.course_creators.admin.render_to_string', mock.Mock(side_effect=mock_render_to_string, autospec=True) @@ -103,6 +104,7 @@ def change_state_and_verify_email(state, is_creator): change_state_and_verify_email(CourseCreator.DENIED, False) + @override_settings(ENABLE_CREATOR_GROUP=True) @mock.patch( 'cms.djangoapps.course_creators.admin.render_to_string', mock.Mock(side_effect=mock_render_to_string, autospec=True) diff --git a/cms/djangoapps/course_creators/tests/test_views.py b/cms/djangoapps/course_creators/tests/test_views.py index 49b7631861dc..d4554263a8bd 100644 --- a/cms/djangoapps/course_creators/tests/test_views.py +++ b/cms/djangoapps/course_creators/tests/test_views.py @@ -3,10 +3,9 @@ """ -from unittest import mock - from django.core.exceptions import PermissionDenied from django.test import TestCase +from django.test.utils import override_settings from django.urls import reverse from cms.djangoapps.course_creators.views import ( @@ -64,35 +63,35 @@ def test_add_unrequested(self): add_user_with_status_granted(self.admin, self.user) self.assertEqual('unrequested', get_course_creator_status(self.user)) # noqa: PT009 + @override_settings(ENABLE_CREATOR_GROUP=True) def test_add_granted(self): - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - # Calling add_user_with_status_granted impacts is_user_in_course_group_role. - self.assertFalse(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 + # Calling add_user_with_status_granted impacts is_user_in_course_group_role. + self.assertFalse(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 - add_user_with_status_granted(self.admin, self.user) - self.assertEqual('granted', get_course_creator_status(self.user)) # noqa: PT009 + add_user_with_status_granted(self.admin, self.user) + self.assertEqual('granted', get_course_creator_status(self.user)) # noqa: PT009 - # Calling add again will be a no-op (even if state is different). - add_user_with_status_unrequested(self.user) - self.assertEqual('granted', get_course_creator_status(self.user)) # noqa: PT009 + # Calling add again will be a no-op (even if state is different). + add_user_with_status_unrequested(self.user) + self.assertEqual('granted', get_course_creator_status(self.user)) # noqa: PT009 - self.assertTrue(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 + self.assertTrue(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 + @override_settings(ENABLE_CREATOR_GROUP=True) def test_update_creator_group(self): - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - self.assertFalse(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 - update_course_creator_group(self.admin, self.user, True) - self.assertTrue(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 - update_course_creator_group(self.admin, self.user, False) - self.assertFalse(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 + self.assertFalse(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 + update_course_creator_group(self.admin, self.user, True) + self.assertTrue(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 + update_course_creator_group(self.admin, self.user, False) + self.assertFalse(auth.user_has_role(self.user, CourseCreatorRole())) # noqa: PT009 + @override_settings(ENABLE_CREATOR_GROUP=True) def test_update_org_content_creator_role(self): - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - self.assertFalse(auth.user_has_role(self.user, OrgContentCreatorRole(self.org))) # noqa: PT009 - update_org_content_creator_role(self.admin, self.user, [self.org]) - self.assertTrue(auth.user_has_role(self.user, OrgContentCreatorRole(self.org))) # noqa: PT009 - update_org_content_creator_role(self.admin, self.user, []) - self.assertFalse(auth.user_has_role(self.user, OrgContentCreatorRole(self.org))) # noqa: PT009 + self.assertFalse(auth.user_has_role(self.user, OrgContentCreatorRole(self.org))) # noqa: PT009 + update_org_content_creator_role(self.admin, self.user, [self.org]) + self.assertTrue(auth.user_has_role(self.user, OrgContentCreatorRole(self.org))) # noqa: PT009 + update_org_content_creator_role(self.admin, self.user, []) + self.assertFalse(auth.user_has_role(self.user, OrgContentCreatorRole(self.org))) # noqa: PT009 def test_user_requested_access(self): add_user_with_status_unrequested(self.user) diff --git a/common/djangoapps/student/auth.py b/common/djangoapps/student/auth.py index d7fdb0bcae3c..378fae50cf35 100644 --- a/common/djangoapps/student/auth.py +++ b/common/djangoapps/student/auth.py @@ -64,7 +64,7 @@ def user_has_role(user, role): if settings.FEATURES.get('DISABLE_COURSE_CREATION', False): return False # wide open course creation setting - if not settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): + if not getattr(settings, 'ENABLE_CREATOR_GROUP', False): return True if role.has_user(user): diff --git a/common/djangoapps/student/management/tests/test_recover_account.py b/common/djangoapps/student/management/tests/test_recover_account.py index b0cacdb5d77e..bd1cbf3a8e9b 100644 --- a/common/djangoapps/student/management/tests/test_recover_account.py +++ b/common/djangoapps/student/management/tests/test_recover_account.py @@ -18,8 +18,6 @@ from openedx.core.djangolib.testing.utils import skip_unless_lms LOGGER_NAME = 'common.djangoapps.student.management.commands.recover_account' -FEATURES_WITH_AUTHN_MFE_ENABLED = settings.FEATURES.copy() -FEATURES_WITH_AUTHN_MFE_ENABLED['ENABLE_AUTHN_MICROFRONTEND'] = True class RecoverAccountTests(TestCase): @@ -67,7 +65,7 @@ def test_account_recovery(self): # try to login with previous password assert not self.client.login(username=self.user.username, password='password') - @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True) @skip_unless_lms def test_authn_mfe_url_in_reset_link(self): """ diff --git a/common/djangoapps/student/tests/test_activate_account.py b/common/djangoapps/student/tests/test_activate_account.py index 12fef9f4de89..3fd6a1b432e8 100644 --- a/common/djangoapps/student/tests/test_activate_account.py +++ b/common/djangoapps/student/tests/test_activate_account.py @@ -17,9 +17,6 @@ from openedx.core.djangolib.testing.utils import skip_unless_lms from openedx.features.enterprise_support.tests.factories import EnterpriseCustomerUserFactory -FEATURES_WITH_AUTHN_MFE_ENABLED = settings.FEATURES.copy() -FEATURES_WITH_AUTHN_MFE_ENABLED['ENABLE_AUTHN_MICROFRONTEND'] = True - @skip_unless_lms class TestActivateAccount(TestCase): @@ -180,7 +177,7 @@ def test_email_confirmation_notification_on_logistration(self): self.assertContains(response, 'Your email could not be confirmed') @override_settings(LOGIN_REDIRECT_WHITELIST=['localhost:1991']) - @override_settings(FEATURES={**FEATURES_WITH_AUTHN_MFE_ENABLED, 'ENABLE_ENTERPRISE_INTEGRATION': True}) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True, ENABLE_ENTERPRISE_INTEGRATION=True) def test_authenticated_account_activation_with_valid_next_url(self): """ Verify that an activation link with a valid next URL will redirect @@ -234,7 +231,7 @@ def test_account_activation_invalid_next_url_redirects_dashboard(self): self.assertRedirects(response, expected_destination) self._assert_user_active_state(expected_active_state=True) - @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True) def test_unauthenticated_user_redirects_to_mfe(self): """ Verify that if Authn MFE is enabled then authenticated user redirects to @@ -259,7 +256,7 @@ def test_unauthenticated_user_redirects_to_mfe(self): assert response.url == (login_page_url + 'error') @override_settings(LOGIN_REDIRECT_WHITELIST=['localhost:1991']) - @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True) def test_unauthenticated_user_redirects_to_mfe_with_valid_next_url(self): """ Verify that if Authn MFE is enabled then authenticated user redirects to diff --git a/common/djangoapps/student/tests/test_authz.py b/common/djangoapps/student/tests/test_authz.py index d349da4b1bf6..54ee9dbd526b 100644 --- a/common/djangoapps/student/tests/test_authz.py +++ b/common/djangoapps/student/tests/test_authz.py @@ -53,33 +53,33 @@ def test_creator_group_not_enabled(self): """ assert user_has_role(self.user, CourseCreatorRole()) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_creator_group_enabled_but_empty(self): """ Tests creator group feature on, but group empty. """ - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - assert not user_has_role(self.user, CourseCreatorRole()) + assert not user_has_role(self.user, CourseCreatorRole()) - # Make user staff. This will cause CourseCreatorRole().has_user to return True. - self.user.is_staff = True - assert user_has_role(self.user, CourseCreatorRole()) + # Make user staff. This will cause CourseCreatorRole().has_user to return True. + self.user.is_staff = True + assert user_has_role(self.user, CourseCreatorRole()) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_creator_group_enabled_nonempty(self): """ Tests creator group feature on, user added. """ - with mock.patch.dict('django.conf.settings.FEATURES', {"ENABLE_CREATOR_GROUP": True}): - add_users(self.admin, CourseCreatorRole(), self.user) - assert user_has_role(self.user, CourseCreatorRole()) + add_users(self.admin, CourseCreatorRole(), self.user) + assert user_has_role(self.user, CourseCreatorRole()) - # check that a user who has not been added to the group still returns false - user_not_added = UserFactory.create(username='testuser2', email='test+courses2@edx.org', password='foo2') - assert not user_has_role(user_not_added, CourseCreatorRole()) + # check that a user who has not been added to the group still returns false + user_not_added = UserFactory.create(username='testuser2', email='test+courses2@edx.org', password='foo2') + assert not user_has_role(user_not_added, CourseCreatorRole()) - # remove first user from the group and verify that CourseCreatorRole().has_user now returns false - remove_users(self.admin, CourseCreatorRole(), self.user) - assert not user_has_role(self.user, CourseCreatorRole()) + # remove first user from the group and verify that CourseCreatorRole().has_user now returns false + remove_users(self.admin, CourseCreatorRole(), self.user) + assert not user_has_role(self.user, CourseCreatorRole()) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_course_creation_disabled(self): """ Tests that the COURSE_CREATION_DISABLED flag overrides course creator group settings. """ - with mock.patch.dict('django.conf.settings.FEATURES', - {'DISABLE_COURSE_CREATION': True, "ENABLE_CREATOR_GROUP": True}): + with mock.patch.dict('django.conf.settings.FEATURES', {'DISABLE_COURSE_CREATION': True}): # Add user to creator group. add_users(self.admin, CourseCreatorRole(), self.user) @@ -94,27 +94,23 @@ def test_course_creation_disabled(self): remove_users(self.admin, CourseCreatorRole(), self.user) assert user_has_role(self.user, CourseCreatorRole()) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_add_user_not_authenticated(self): """ Tests that adding to creator group fails if user is not authenticated """ - with mock.patch.dict( - 'django.conf.settings.FEATURES', - {'DISABLE_COURSE_CREATION': False, "ENABLE_CREATOR_GROUP": True} - ): + with mock.patch.dict('django.conf.settings.FEATURES', {'DISABLE_COURSE_CREATION': False}): anonymous_user = AnonymousUser() role = CourseCreatorRole() add_users(self.admin, role, anonymous_user) assert not user_has_role(anonymous_user, role) + @override_settings(ENABLE_CREATOR_GROUP=True) def test_add_user_not_active(self): """ Tests that adding to creator group fails if user is not active """ - with mock.patch.dict( - 'django.conf.settings.FEATURES', - {'DISABLE_COURSE_CREATION': False, "ENABLE_CREATOR_GROUP": True} - ): + with mock.patch.dict('django.conf.settings.FEATURES', {'DISABLE_COURSE_CREATION': False}): self.user.is_active = False add_users(self.admin, CourseCreatorRole(), self.user) assert not user_has_role(self.user, CourseCreatorRole()) diff --git a/openedx/core/djangoapps/content_libraries/permissions.py b/openedx/core/djangoapps/content_libraries/permissions.py index 1d0c7f9c0544..96b8615c0efa 100644 --- a/openedx/core/djangoapps/content_libraries/permissions.py +++ b/openedx/core/djangoapps/content_libraries/permissions.py @@ -228,7 +228,7 @@ def check(self, user, instance, *args, **kwargs): # pylint: disable=arguments-d # Is the user allowed to create content libraries? CAN_CREATE_CONTENT_LIBRARY = 'content_libraries.create_library' -if settings.FEATURES.get('ENABLE_CREATOR_GROUP', False): +if getattr(settings, 'ENABLE_CREATOR_GROUP', False): perms[CAN_CREATE_CONTENT_LIBRARY] = is_global_staff | (is_user_active & is_course_creator) else: perms[CAN_CREATE_CONTENT_LIBRARY] = is_global_staff diff --git a/openedx/core/djangoapps/user_authn/toggles.py b/openedx/core/djangoapps/user_authn/toggles.py index 67da464cbb5f..fb9e928bf5f4 100644 --- a/openedx/core/djangoapps/user_authn/toggles.py +++ b/openedx/core/djangoapps/user_authn/toggles.py @@ -22,7 +22,7 @@ def should_redirect_to_authn_microfrontend(): if request and request.GET.get('skip_authn_mfe'): return False return configuration_helpers.get_value( - 'ENABLE_AUTHN_MICROFRONTEND', settings.FEATURES.get('ENABLE_AUTHN_MICROFRONTEND') + 'ENABLE_AUTHN_MICROFRONTEND', getattr(settings, 'ENABLE_AUTHN_MICROFRONTEND', None) ) diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_login.py b/openedx/core/djangoapps/user_authn/views/tests/test_login.py index 4e4d0a9e8894..98db117aca20 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_login.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_login.py @@ -93,9 +93,6 @@ def test_login_success(self): self._assert_response(response, success=True) self._assert_audit_log(mock_audit_log, 'info', ['Login success', self.user_email]) - FEATURES_WITH_AUTHN_MFE_ENABLED = settings.FEATURES.copy() - FEATURES_WITH_AUTHN_MFE_ENABLED['ENABLE_AUTHN_MICROFRONTEND'] = True - @override_settings(MARKETING_EMAILS_OPT_IN=True) def test_login_success_with_opt_in_flag_enabled(self): self.user.is_active = False @@ -188,7 +185,7 @@ def test_public_login_failure_with_only_third_part_auth_enabled(self): ) @ddt.unpack @override_settings(LOGIN_REDIRECT_WHITELIST=['openedx.service']) - @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True) @skip_unless_lms def test_login_success_with_redirect(self, next_url, course_id, expected_redirect): post_params = {} @@ -209,7 +206,7 @@ def test_login_success_with_redirect(self, next_url, course_id, expected_redirec @ddt.data(('/dashboard', False), ('/enterprise/select/active/?success_url=/dashboard', True)) @ddt.unpack - @patch.dict(settings.FEATURES, {'ENABLE_AUTHN_MICROFRONTEND': True, 'ENABLE_ENTERPRISE_INTEGRATION': True}) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True, ENABLE_ENTERPRISE_INTEGRATION=True) @override_settings(LOGIN_REDIRECT_WHITELIST=['openedx.service']) @patch('openedx.features.enterprise_support.api.EnterpriseApiClient') @patch('openedx.core.djangoapps.user_authn.views.login.reverse') @@ -259,7 +256,7 @@ def test_login_success_for_multiple_enterprises( @ddt.data(('', True), ('/enterprise/select/active/?success_url=', False)) @ddt.unpack - @patch.dict(settings.FEATURES, {'ENABLE_AUTHN_MICROFRONTEND': True, 'ENABLE_ENTERPRISE_INTEGRATION': True}) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True, ENABLE_ENTERPRISE_INTEGRATION=True) @patch('openedx.features.enterprise_support.api.EnterpriseApiClient') @patch('openedx.core.djangoapps.user_authn.views.login.activate_learner_enterprise') @patch('openedx.core.djangoapps.user_authn.views.login.reverse') diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py index b94da138ea5d..15c90e5dea71 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_logistration.py @@ -64,16 +64,13 @@ def setUp(self): # pylint: disable=arguments-differ ) self.hidden_disabled_provider = self.configure_azure_ad_provider() - FEATURES_WITH_AUTHN_MFE_ENABLED = settings.FEATURES.copy() - FEATURES_WITH_AUTHN_MFE_ENABLED['ENABLE_AUTHN_MICROFRONTEND'] = True - @ddt.data( ("signin_user", "/login"), ("register_user", "/register"), ("password_assistance", "/reset"), ) @ddt.unpack - @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True) def test_logistration_mfe_redirects(self, url_name, path): """ Test that if Logistration MFE is enabled, then we redirect to @@ -96,7 +93,7 @@ def test_logistration_mfe_redirects(self, url_name, path): ) ) @ddt.unpack - @override_settings(FEATURES=FEATURES_WITH_AUTHN_MFE_ENABLED) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True) def test_logistration_redirect_params(self, url_name, path, query_params): """ Test that if request is redirected to logistration MFE, diff --git a/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py b/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py index 7388e51edddc..1acc9eeaaa55 100644 --- a/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py +++ b/openedx/core/djangoapps/user_authn/views/tests/test_reset_password.py @@ -47,9 +47,6 @@ ) from openedx.core.djangolib.testing.utils import CacheIsolationTestCase, skip_unless_lms -ENABLE_AUTHN_MICROFRONTEND = settings.FEATURES.copy() -ENABLE_AUTHN_MICROFRONTEND['ENABLE_AUTHN_MICROFRONTEND'] = True - def process_request(request): middleware = SessionMiddleware(get_response=lambda request: None) @@ -370,7 +367,7 @@ def test_reset_password_email_https(self, is_secure, protocol): SETTING_CHANGE_INITIATED, user_id=self.user.id, setting='password', old=None, new=None ) - @override_settings(FEATURES=ENABLE_AUTHN_MICROFRONTEND) + @override_settings(ENABLE_AUTHN_MICROFRONTEND=True) @skip_unless_lms @ddt.data(('Crazy Awesome Site', 'Crazy Awesome Site'), ('edX', 'edX')) @ddt.unpack diff --git a/openedx/core/lib/features_setting_proxy.py b/openedx/core/lib/features_setting_proxy.py index eb38ad7f9100..6ff7c872888c 100644 --- a/openedx/core/lib/features_setting_proxy.py +++ b/openedx/core/lib/features_setting_proxy.py @@ -1,6 +1,7 @@ """ Features Proxy Implementation """ +import traceback import warnings from collections.abc import Mapping, MutableMapping @@ -24,18 +25,65 @@ def __init__(self, namespace=None): """Store the namespace (as a dict)""" self.ns = namespace or {} + _MISSING = object() + + def _resolve(self, key): + """Return key's value if it's been overridden via @override_settings, else self._MISSING. + + We deliberately walk only the UserSettingsHolder chain (the layers that + @override_settings pushes onto django.conf.settings._wrapped) rather + than reading django.conf.settings.X directly. Django's bottom Settings + layer is a *snapshot* taken at init time, so it doesn't reflect runtime + mutations of the settings module's globals (which is what + proxy.ns mutations and legacy patch.dict(settings.FEATURES, ...) do). + Walking only the explicit override layers lets the new + @override_settings(X=Y) path work while leaving the legacy patch.dict + path untouched. + """ + from django.conf import settings as django_settings + wrapped = django_settings._wrapped # pylint: disable=protected-access + # UserSettingsHolder has a `default_settings` attribute and stores + # explicit overrides in its own __dict__; the bottom Settings has no + # default_settings, so the loop terminates there. + while hasattr(wrapped, 'default_settings'): + if key in wrapped.__dict__: + return wrapped.__dict__[key] + wrapped = wrapped.default_settings + return self._MISSING + def __getitem__(self, key): - """Retrieve a feature flag by key""" + """Retrieve a feature flag by key, preferring @override_settings overrides.""" + value = self._resolve(key) + if value is not self._MISSING: + return value return self.ns[key] - def __setitem__(self, key, value): - """Sets a key-value pair while emitting a deprecation warning about using FEATURES as a dict.""" + # Frames to skip when walking the stack for the [caller: ...] suffix. + # mock.patch.dict's machinery is several frames deep, so simple stacklevel + # arithmetic can't reach the user test code on its own. + _STACK_SKIP_PATTERNS = ('features_setting_proxy.py', '/unittest/mock.py', '/contextlib.py') + + def _warn_dict_access(self, key, value): + """Emit the FEATURES-as-dict deprecation warning, attributed to the caller of __setitem__/update.""" + # stacklevel=3 walks past warnings.warn -> _warn_dict_access -> caller-of-_warn_dict_access, + # so the warning is reported from the line that actually mutated FEATURES (test code or app code). + stack = traceback.extract_stack() + user_frame = next( + (f for f in reversed(stack[:-1]) if not any(p in f.filename for p in self._STACK_SKIP_PATTERNS)), + None, + ) + suffix = f" [caller: {user_frame.filename}:{user_frame.lineno}]" if user_frame else "" warnings.warn( f"Accessing FEATURES as a dict is deprecated. " - f"Add '{key} = {value!r}' to your Django settings module instead of modifying FEATURES.", + f"Add '{key} = {value!r}' to your Django settings module instead of modifying FEATURES." + f"{suffix}", DeprecationWarning, - stacklevel=2 + stacklevel=3, ) + + def __setitem__(self, key, value): + """Sets a key-value pair while emitting a deprecation warning about using FEATURES as a dict.""" + self._warn_dict_access(key, value) self.ns[key] = value def __delitem__(self, key): @@ -49,14 +97,17 @@ def __len__(self): return len(self.ns) def __contains__(self, key): - return key in self.ns + return self._resolve(key) is not self._MISSING or key in self.ns def clear(self): """Remove all feature flags from the namespace.""" self.ns.clear() def get(self, key, default=None): - """Standard dict-style get with default""" + """Standard dict-style get with default; prefers @override_settings overrides.""" + value = self._resolve(key) + if value is not self._MISSING: + return value return self.ns.get(key, default) def update(self, other=(), /, **kwds): @@ -82,21 +133,27 @@ def update(self, other=(), /, **kwds): proxy.update({'FEATURE_A': True}, FEATURE_B=False) -> other={'FEATURE_A': True}; kwds = {'FEATURE_B': False} """ + # We bypass __setitem__ and emit the warning here so that stacklevel + # points at the caller of update() rather than at this method's body. if isinstance(other, Mapping): # Handles objects that formally conform to the Mapping interface # Mapping-like types: defaultdict, OrderedDict, Counter for key in other: - self[key] = other[key] + self._warn_dict_access(key, other[key]) + self.ns[key] = other[key] elif hasattr(other, "keys"): # Fallback for objects that implement a .keys() method but # may not formally subclass Mapping for key in other.keys(): - self[key] = other[key] + self._warn_dict_access(key, other[key]) + self.ns[key] = other[key] else: for key, value in other: - self[key] = value + self._warn_dict_access(key, value) + self.ns[key] = value for key, value in kwds.items(): - self[key] = value + self._warn_dict_access(key, value) + self.ns[key] = value def copy(self): """