From 6ac0eb51b9d1e6ea243138e92d8e0a5e18367f8c Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Wed, 26 Mar 2025 11:13:17 -0400 Subject: [PATCH 1/9] feat!: Remove all trivial mentions of PREVIEW_LMS_BASE There are a few more mentions but these are all the ones that don't need major further followup. BREAKING CHANGE: The learning MFE now supports preview functionality natively and it is no longer necessary to use a different domain on the LMS to render a preview of course content. See https://github.com/openedx/frontend-app-learning/issues/1455 for more details. --- cms/envs/common.py | 1 - cms/envs/devstack-experimental.yml | 1 - cms/envs/devstack.py | 1 - cms/envs/mock.yml | 1 - cms/envs/test.py | 1 - lms/djangoapps/courseware/access_utils.py | 4 +--- lms/envs/devstack-experimental.yml | 1 - lms/envs/minimal.yml | 2 -- lms/envs/mock.yml | 1 - lms/envs/production.py | 9 --------- lms/envs/test.py | 6 +----- 11 files changed, 2 insertions(+), 26 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index 787db40bd308..84b0742cba9f 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -367,7 +367,6 @@ # Prevent auto auth from creating superusers or modifying existing users 'RESTRICT_AUTOMATIC_AUTH': True, - 'PREVIEW_LMS_BASE': "preview.localhost:18000", 'ENABLE_GRADE_DOWNLOADS': True, 'ENABLE_MKTG_SITE': False, 'ENABLE_DISCUSSION_HOME_PANEL': True, diff --git a/cms/envs/devstack-experimental.yml b/cms/envs/devstack-experimental.yml index c08b19045faa..f5d182a82b6b 100644 --- a/cms/envs/devstack-experimental.yml +++ b/cms/envs/devstack-experimental.yml @@ -324,7 +324,6 @@ FEATURES: ENABLE_SYSADMIN_DASHBOARD: false ENABLE_THIRD_PARTY_AUTH: true ENABLE_VIDEO_UPLOAD_PIPELINE: false - PREVIEW_LMS_BASE: preview.localhost:18000 SHOW_FOOTER_LANGUAGE_SELECTOR: false SHOW_HEADER_LANGUAGE_SELECTOR: false FEEDBACK_SUBMISSION_EMAIL: '' diff --git a/cms/envs/devstack.py b/cms/envs/devstack.py index 63dff1fbb9c2..d1be075007d1 100644 --- a/cms/envs/devstack.py +++ b/cms/envs/devstack.py @@ -45,7 +45,6 @@ LMS_BASE = 'localhost:18000' LMS_ROOT_URL = f'http://{LMS_BASE}' -FEATURES['PREVIEW_LMS_BASE'] = "preview." + LMS_BASE FRONTEND_REGISTER_URL = LMS_ROOT_URL + '/register' diff --git a/cms/envs/mock.yml b/cms/envs/mock.yml index fb2233501ef4..b8935dd3cc21 100644 --- a/cms/envs/mock.yml +++ b/cms/envs/mock.yml @@ -455,7 +455,6 @@ FEATURES: LTI_1P3_ENABLED: true MILESTONES_APP: true PREVENT_CONCURRENT_LOGINS: true - PREVIEW_LMS_BASE: preview.courses.localhost SEGMENT_IO_LMS: true SEPARATE_VERIFICATION_FROM_PAYMENT: true SHOW_FOOTER_LANGUAGE_SELECTOR: true diff --git a/cms/envs/test.py b/cms/envs/test.py index 500c8d538d39..6a71e94c12cd 100644 --- a/cms/envs/test.py +++ b/cms/envs/test.py @@ -131,7 +131,6 @@ LMS_BASE = "localhost:8000" LMS_ROOT_URL = f"http://{LMS_BASE}" -FEATURES["PREVIEW_LMS_BASE"] = "preview.localhost" CMS_BASE = "localhost:8001" CMS_ROOT_URL = f"http://{CMS_BASE}" diff --git a/lms/djangoapps/courseware/access_utils.py b/lms/djangoapps/courseware/access_utils.py index 99aa1e605bcc..a7a005af2600 100644 --- a/lms/djangoapps/courseware/access_utils.py +++ b/lms/djangoapps/courseware/access_utils.py @@ -162,9 +162,7 @@ def in_preview_mode(): """ Returns whether the user is in preview mode or not. """ - hostname = get_current_request_hostname() - preview_lms_base = settings.FEATURES.get("PREVIEW_LMS_BASE", None) - return bool(preview_lms_base and hostname and hostname.split(":")[0] == preview_lms_base.split(":")[0]) + return False def check_course_open_for_learner(user, course): diff --git a/lms/envs/devstack-experimental.yml b/lms/envs/devstack-experimental.yml index 43486ca024bc..6811a0e93db8 100644 --- a/lms/envs/devstack-experimental.yml +++ b/lms/envs/devstack-experimental.yml @@ -355,7 +355,6 @@ FEATURES: ENABLE_SYSADMIN_DASHBOARD: false ENABLE_THIRD_PARTY_AUTH: true ENABLE_VIDEO_UPLOAD_PIPELINE: false - PREVIEW_LMS_BASE: preview.localhost:18000 SHOW_FOOTER_LANGUAGE_SELECTOR: false SHOW_HEADER_LANGUAGE_SELECTOR: false FEEDBACK_SUBMISSION_EMAIL: '' diff --git a/lms/envs/minimal.yml b/lms/envs/minimal.yml index 21409fa61aa3..f4f9bd8a8a8a 100644 --- a/lms/envs/minimal.yml +++ b/lms/envs/minimal.yml @@ -11,8 +11,6 @@ --- SECRET_KEY: aseuothsaeotuhaseotisaotenihsaoetih -FEATURES: - PREVIEW_LMS_BASE: "http://localhost" TRACKING_BACKENDS: {} EVENT_TRACKING_BACKENDS: {} JWT_AUTH: {} diff --git a/lms/envs/mock.yml b/lms/envs/mock.yml index 4da53fadb18f..cdcbcf49e5a2 100644 --- a/lms/envs/mock.yml +++ b/lms/envs/mock.yml @@ -619,7 +619,6 @@ FEATURES: NOTICES_SNOOZE_COUNT_LIMIT: 5 NOTICES_SNOOZE_HOURS: 5 PREVENT_CONCURRENT_LOGINS: true - PREVIEW_LMS_BASE: preview-deploy_host SEND_LEARNING_CERTIFICATE_LIFECYCLE_EVENTS_TO_BUS: true SEPARATE_VERIFICATION_FROM_PAYMENT: true SHOW_FOOTER_LANGUAGE_SELECTOR: true diff --git a/lms/envs/production.py b/lms/envs/production.py index 4884e0a22c87..b44b1078bf8b 100644 --- a/lms/envs/production.py +++ b/lms/envs/production.py @@ -257,7 +257,6 @@ def get_env_setting(setting): ALLOWED_HOSTS = [ "*", _YAML_TOKENS.get('LMS_BASE'), - FEATURES['PREVIEW_LMS_BASE'], ] # Cache used for location mapping -- called many times with the same key/value @@ -326,14 +325,6 @@ def get_env_setting(setting): CORS_ALLOW_INSECURE = _YAML_TOKENS.get('CORS_ALLOW_INSECURE', False) CROSS_DOMAIN_CSRF_COOKIE_DOMAIN = _YAML_TOKENS.get('CROSS_DOMAIN_CSRF_COOKIE_DOMAIN') -# PREVIEW DOMAIN must be present in HOSTNAME_MODULESTORE_DEFAULT_MAPPINGS for the preview to show draft changes -if 'PREVIEW_LMS_BASE' in FEATURES and FEATURES['PREVIEW_LMS_BASE'] != '': - PREVIEW_DOMAIN = FEATURES['PREVIEW_LMS_BASE'].split(':')[0] - # update dictionary with preview domain regex - HOSTNAME_MODULESTORE_DEFAULT_MAPPINGS.update({ - PREVIEW_DOMAIN: 'draft-preferred' - }) - ############### Mixed Related(Secure/Not-Secure) Items ########## LMS_SEGMENT_KEY = _YAML_TOKENS.get('SEGMENT_KEY') diff --git a/lms/envs/test.py b/lms/envs/test.py index b49a71b4711a..4563b571e0bf 100644 --- a/lms/envs/test.py +++ b/lms/envs/test.py @@ -330,12 +330,8 @@ LTI_PORT = 8765 VIDEO_SOURCE_PORT = 8777 -FEATURES['PREVIEW_LMS_BASE'] = "preview.localhost" ############### Module Store Items ########## -PREVIEW_DOMAIN = FEATURES['PREVIEW_LMS_BASE'].split(':')[0] -HOSTNAME_MODULESTORE_DEFAULT_MAPPINGS = { - PREVIEW_DOMAIN: 'draft-preferred' -} +HOSTNAME_MODULESTORE_DEFAULT_MAPPINGS = {} ################### Make tests faster From 6b70b8511bff91bfef9c82ecd0aa9c2039e4128b Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 27 Mar 2025 21:25:45 -0400 Subject: [PATCH 2/9] feat: Drop the `in_preview_mode` function. Since we're no longer using a separate domain, that check always returned false. Remove it and update any places/tests where it is used. --- lms/djangoapps/courseware/access.py | 6 --- lms/djangoapps/courseware/access_utils.py | 10 +--- .../courseware/tests/test_access.py | 51 +++++-------------- lms/djangoapps/courseware/toggles.py | 5 +- setup.cfg | 5 -- 5 files changed, 16 insertions(+), 61 deletions(-) diff --git a/lms/djangoapps/courseware/access.py b/lms/djangoapps/courseware/access.py index 436cb3514a54..a1a46137fcca 100644 --- a/lms/djangoapps/courseware/access.py +++ b/lms/djangoapps/courseware/access.py @@ -34,7 +34,6 @@ check_course_open_for_learner, check_start_date, debug, - in_preview_mode ) from lms.djangoapps.courseware.masquerade import get_masquerade_role, is_masquerading_as_student from lms.djangoapps.ccx.custom_exception import CCXLocatorValidationException @@ -158,11 +157,6 @@ def has_access(user, action, obj, course_key=None): if not user: user = AnonymousUser() - # Preview mode is only accessible by staff. - if in_preview_mode() and course_key: - if not has_staff_access_to_preview_mode(user, course_key): - return ACCESS_DENIED - # delegate the work to type-specific functions. # (start with more specific types, then get more general) if isinstance(obj, CourseBlock): diff --git a/lms/djangoapps/courseware/access_utils.py b/lms/djangoapps/courseware/access_utils.py index a7a005af2600..23ec992f8189 100644 --- a/lms/djangoapps/courseware/access_utils.py +++ b/lms/djangoapps/courseware/access_utils.py @@ -25,7 +25,6 @@ from lms.djangoapps.courseware.masquerade import get_course_masquerade, is_masquerading_as_student from openedx.features.course_experience import COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, COURSE_PRE_START_ACCESS_FLAG from xmodule.course_block import COURSE_VISIBILITY_PUBLIC # lint-amnesty, pylint: disable=wrong-import-order -from xmodule.util.xmodule_django import get_current_request_hostname # lint-amnesty, pylint: disable=wrong-import-order DEBUG_ACCESS = False log = getLogger(__name__) @@ -138,7 +137,7 @@ def check_start_date(user, days_early_for_beta, start, course_key, display_error if start_dates_disabled and not masquerading_as_student: return ACCESS_GRANTED else: - if start is None or in_preview_mode() or get_course_masquerade(user, course_key): + if start is None or get_course_masquerade(user, course_key): return ACCESS_GRANTED if now is None: @@ -158,13 +157,6 @@ def check_start_date(user, days_early_for_beta, start, course_key, display_error return StartDateError(start, display_error_to_user=display_error_to_user) -def in_preview_mode(): - """ - Returns whether the user is in preview mode or not. - """ - return False - - def check_course_open_for_learner(user, course): """ Check if the course is open for learners based on the start date. diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index e0c5f59a83fb..0c7cc248ceed 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -254,26 +254,21 @@ def test_student_has_access(self): overview, chapter, ] - with patch('lms.djangoapps.courseware.access.in_preview_mode') as mock_preview: - mock_preview.return_value = False - for obj in modules: - assert bool(access.has_access(self.student, 'load', obj, course_key=self.course.id)) + # Student should have access to modules they're enrolled in + for obj in modules: + assert bool(access.has_access(self.student, 'load', obj, course_key=self.course.id)) - with patch('lms.djangoapps.courseware.access.in_preview_mode') as mock_preview: - mock_preview.return_value = True - for obj in modules: - assert not bool(access.has_access(self.student, 'load', obj, course_key=self.course.id)) - - @patch('lms.djangoapps.courseware.access.in_preview_mode', Mock(return_value=True)) - def test_has_access_with_preview_mode(self): + def test_has_access_based_on_roles(self): """ - Tests particular user's can access content via has_access in preview mode. + Tests user access to content based on their role. """ assert bool(access.has_access(self.global_staff, 'staff', self.course, course_key=self.course.id)) assert bool(access.has_access(self.course_staff, 'staff', self.course, course_key=self.course.id)) assert bool(access.has_access(self.course_instructor, 'staff', self.course, course_key=self.course.id)) assert not bool(access.has_access(self.student, 'staff', self.course, course_key=self.course.id)) - assert not bool(access.has_access(self.student, 'load', self.course, course_key=self.course.id)) + + # Student should be able to load the course even if they don't have staff access. + assert bool(access.has_access(self.student, 'load', self.course, course_key=self.course.id)) # When masquerading is true, user should not be able to access staff content with patch('lms.djangoapps.courseware.access.is_masquerading_as_student') as mock_masquerade: @@ -281,10 +276,9 @@ def test_has_access_with_preview_mode(self): assert not bool(access.has_access(self.global_staff, 'staff', self.course, course_key=self.course.id)) assert not bool(access.has_access(self.student, 'staff', self.course, course_key=self.course.id)) - @patch('lms.djangoapps.courseware.access_utils.in_preview_mode', Mock(return_value=True)) - def test_has_access_in_preview_mode_with_group(self): + def test_has_access_with_content_groups(self): """ - Test that a user masquerading as a member of a group sees appropriate content in preview mode. + Test that a user masquerading as a member of a group sees appropriate content. """ # Note about UserPartition and UserPartition Group IDs: these must not conflict with IDs used # by dynamic user partitions. @@ -423,24 +417,6 @@ def test__has_access_to_block_beta_user(self): assert bool(access._has_access_to_block(self.beta_user, 'load', mock_unit, course_key=self.course.id)) - @ddt.data(None, YESTERDAY, TOMORROW) - @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) - @patch( - 'lms.djangoapps.courseware.access_utils.get_current_request_hostname', - Mock(return_value='preview.localhost') - ) - def test__has_access_to_block_in_preview_mode(self, start): - """ - Tests that block has access in preview mode. - """ - mock_unit = Mock(location=self.course.location, user_partitions=[]) - mock_unit._class_tags = {} # Needed for detached check in _has_access_to_block - mock_unit.visible_to_staff_only = False - mock_unit.start = self.DATES[start] - mock_unit.merged_group_access = {} - - self.verify_access(mock_unit, True) - @ddt.data( (TOMORROW, access_response.StartDateError), (None, None), @@ -448,10 +424,11 @@ def test__has_access_to_block_in_preview_mode(self, start): ) # ddt throws an error if I don't put the None argument there @ddt.unpack @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) - @patch('lms.djangoapps.courseware.access_utils.get_current_request_hostname', Mock(return_value='localhost')) - def test__has_access_to_block_when_not_in_preview_mode(self, start, expected_error_type): + def test__has_access_to_block_with_start_date(self, start, expected_error_type): """ - Tests that block has no access when start date in future & without preview. + Tests that block access follows start date rules. + Access should be denied when start date is in the future and granted when + start date is in the past or not set. """ expected_access = expected_error_type is None mock_unit = Mock(location=self.course.location, user_partitions=[]) diff --git a/lms/djangoapps/courseware/toggles.py b/lms/djangoapps/courseware/toggles.py index e6070a2e3bd1..aacc19e5c459 100644 --- a/lms/djangoapps/courseware/toggles.py +++ b/lms/djangoapps/courseware/toggles.py @@ -173,10 +173,7 @@ def courseware_mfe_is_active() -> bool: """ Should we serve the Learning MFE as the canonical courseware experience? """ - from lms.djangoapps.courseware.access_utils import in_preview_mode # avoid a circular import - - # We only use legacy views for the Studio "preview mode" feature these days, while everyone else gets the MFE - return not in_preview_mode() + return True def course_exit_page_is_active(course_key): diff --git a/setup.cfg b/setup.cfg index d83ae7d2765e..df76800f5e17 100644 --- a/setup.cfg +++ b/setup.cfg @@ -98,11 +98,6 @@ ignore_imports = # -> openedx.core.djangoapps.course_groups.cohorts # -> lms.djangoapps.courseware.courses openedx.core.djangoapps.course_groups.cohorts -> lms.djangoapps.courseware.courses - # cms.djangoapps.models.settings.course_metadata - # -> openedx.features.course_experience - # -> openedx.features.course_experience.url_helpers - # -> lms.djangoapps.courseware.toggles - openedx.features.course_experience.url_helpers -> lms.djangoapps.courseware.toggles # cms.djangoapps.contentstore.[various] # -> openedx.features.content_type_gating.partitions # -> lms.djangoapps.commerce.utils From f128ee95dea3952c2aeca55cff60ca9b42493b97 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Thu, 27 Mar 2025 21:38:35 -0400 Subject: [PATCH 3/9] feat: Drop courseware_mfe_is_active function. With the removal of the preview check this function is also a no-op now so drop calls to it and update the places where it is called to not change other behavior. --- lms/djangoapps/courseware/toggles.py | 17 +------- lms/djangoapps/courseware/views/index.py | 5 +-- .../tests/test_url_helpers.py | 42 +------------------ .../features/course_experience/url_helpers.py | 7 +--- 4 files changed, 6 insertions(+), 65 deletions(-) diff --git a/lms/djangoapps/courseware/toggles.py b/lms/djangoapps/courseware/toggles.py index aacc19e5c459..f9f083cad42e 100644 --- a/lms/djangoapps/courseware/toggles.py +++ b/lms/djangoapps/courseware/toggles.py @@ -169,25 +169,12 @@ ) -def courseware_mfe_is_active() -> bool: - """ - Should we serve the Learning MFE as the canonical courseware experience? - """ - return True - - def course_exit_page_is_active(course_key): - return ( - courseware_mfe_is_active() and - COURSEWARE_MICROFRONTEND_COURSE_EXIT_PAGE.is_enabled(course_key) - ) + return COURSEWARE_MICROFRONTEND_COURSE_EXIT_PAGE.is_enabled(course_key) def courseware_mfe_progress_milestones_are_active(course_key): - return ( - courseware_mfe_is_active() and - COURSEWARE_MICROFRONTEND_PROGRESS_MILESTONES.is_enabled(course_key) - ) + return COURSEWARE_MICROFRONTEND_PROGRESS_MILESTONES.is_enabled(course_key) def streak_celebration_is_active(course_key): diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index e3bd2b34634f..800d2f518775 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -63,7 +63,7 @@ from ..model_data import FieldDataCache from ..block_render import get_block_for_descriptor, toc_for_course from ..permissions import MASQUERADE_AS_STUDENT -from ..toggles import ENABLE_OPTIMIZELY_IN_COURSEWARE, courseware_mfe_is_active +from ..toggles import ENABLE_OPTIMIZELY_IN_COURSEWARE from .views import CourseTabView log = logging.getLogger("edx.courseware.views.index") @@ -171,8 +171,7 @@ def _redirect_to_learning_mfe(self): Can the user access this sequence in the courseware MFE? If so, redirect to MFE. """ # If the MFE is active, prefer that - if courseware_mfe_is_active(): - raise Redirect(self.microfrontend_url) + raise Redirect(self.microfrontend_url) @property def microfrontend_url(self): diff --git a/openedx/features/course_experience/tests/test_url_helpers.py b/openedx/features/course_experience/tests/test_url_helpers.py index dfa84583ab28..b6f134f78002 100644 --- a/openedx/features/course_experience/tests/test_url_helpers.py +++ b/openedx/features/course_experience/tests/test_url_helpers.py @@ -1,8 +1,6 @@ """ Test some of the functions in url_helpers """ -from unittest import mock - import ddt from django.test import TestCase from django.test.client import RequestFactory @@ -14,14 +12,6 @@ from .. import url_helpers -def _patch_courseware_mfe_is_active(ret_val): - return mock.patch.object( - url_helpers, - 'courseware_mfe_is_active', - return_value=ret_val, - ) - - @ddt.ddt class IsLearningMfeTests(TestCase): """ @@ -53,8 +43,6 @@ def test_is_request_from_learning_mfe(self, mfe_url, referrer_url, is_mfe): class GetCoursewareUrlTests(SharedModuleStoreTestCase): """ Test get_courseware_url. - - Mock out `courseware_mfe_is_active`; that is tested elseware. """ @classmethod @@ -121,12 +109,10 @@ def create_test_courses(cls): @ddt.data( ( - 'mfe', 'course_run', 'http://learning-mfe/course/course-v1:TestX+UrlHelpers+split' ), ( - 'mfe', 'section', ( 'http://learning-mfe/course/course-v1:TestX+UrlHelpers+split' + @@ -134,7 +120,6 @@ def create_test_courses(cls): ), ), ( - 'mfe', 'subsection', ( 'http://learning-mfe/course/course-v1:TestX+UrlHelpers+split' + @@ -142,7 +127,6 @@ def create_test_courses(cls): ), ), ( - 'mfe', 'unit', ( 'http://learning-mfe/course/course-v1:TestX+UrlHelpers+split' + @@ -151,7 +135,6 @@ def create_test_courses(cls): ), ), ( - 'mfe', 'component', ( 'http://learning-mfe/course/course-v1:TestX+UrlHelpers+split' + @@ -159,31 +142,10 @@ def create_test_courses(cls): '/block-v1:TestX+UrlHelpers+split+type@vertical+block@Generated_Unit' ), ), - ( - 'legacy', - 'course_run', - '/courses/course-v1:TestX+UrlHelpers+split/courseware', - ), - ( - 'legacy', - 'subsection', - '/courses/course-v1:TestX+UrlHelpers+split/courseware/Generated_Section/Generated_Subsection/', - ), - ( - 'legacy', - 'unit', - '/courses/course-v1:TestX+UrlHelpers+split/courseware/Generated_Section/Generated_Subsection/1', - ), - ( - 'legacy', - 'component', - '/courses/course-v1:TestX+UrlHelpers+split/courseware/Generated_Section/Generated_Subsection/1', - ) ) @ddt.unpack def test_get_courseware_url( self, - active_experience, structure_level, expected_path, ): @@ -196,9 +158,7 @@ def test_get_courseware_url( check that the expected path (URL without querystring) is returned by `get_courseware_url`. """ block = self.items[structure_level] - with _patch_courseware_mfe_is_active(active_experience == 'mfe') as mock_mfe_is_active: - url = url_helpers.get_courseware_url(block.location) + url = url_helpers.get_courseware_url(block.location) path = url.split('?')[0] assert path == expected_path course_run = self.items['course_run'] - mock_mfe_is_active.assert_called_once() diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index e1fe4110c613..d80ac511583f 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -14,7 +14,6 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from six.moves.urllib.parse import urlencode, urlparse -from lms.djangoapps.courseware.toggles import courseware_mfe_is_active from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.search import navigation_index, path_to_location # lint-amnesty, pylint: disable=wrong-import-order @@ -42,11 +41,7 @@ def get_courseware_url( * ItemNotFoundError if no data at the `usage_key`. * NoPathToItem if we cannot build a path to the `usage_key`. """ - if courseware_mfe_is_active(): - get_url_fn = _get_new_courseware_url - else: - get_url_fn = _get_legacy_courseware_url - return get_url_fn(usage_key=usage_key, request=request, is_staff=is_staff) + return _get_new_courseware_url(usage_key=usage_key, request=request, is_staff=is_staff) def _get_legacy_courseware_url( From ab136bada589f0b3370fdfa76f6f502e221e8117 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 28 Mar 2025 09:35:29 -0400 Subject: [PATCH 4/9] feat!: Drop redirect to preview from the legacy courseware index. The CoursewareIndex view is going to be removed eventually but for now we're focusing on removing the PREVIEW_LMS_BASE setting. With this change, if someone tries to load the legacy courseware URL from the preview domain it will no longer redirect them to the MFE preview. This is not a problem that will occur for users coming from existing studio links because those links have already been updated to go directly to the new urls. The only way this path could execute is if someone goes directly to the old Preview URL that they saved off platform somewhere. eg. If they bookmarked it for some reason. BREAKING CHANGE: Saved links (including bookmarks) to the legacy preview URLs will no longer redirect to the MFE preview URLs. --- lms/djangoapps/courseware/views/index.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index 800d2f518775..02f0ef1f7f3f 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -27,7 +27,6 @@ from xmodule.course_block import COURSE_VISIBILITY_PUBLIC from xmodule.modulestore.django import modulestore from xmodule.x_module import PUBLIC_VIEW, STUDENT_VIEW -from xmodule.util.xmodule_django import get_current_request_hostname from common.djangoapps.edxmako.shortcuts import render_to_response, render_to_string from common.djangoapps.student.models import CourseEnrollment @@ -187,7 +186,7 @@ def microfrontend_url(self): unit_key = None except InvalidKeyError: unit_key = None - is_preview = settings.FEATURES.get('PREVIEW_LMS_BASE') == get_current_request_hostname() + is_preview = False url = make_learning_mfe_courseware_url( self.course_key, self.section.location if self.section else None, From 168f1b13195e7d9e8dc5f38e383599a498225725 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 28 Mar 2025 17:32:39 -0400 Subject: [PATCH 5/9] test: Drop the set_preview_mode test helper. This test helper was setting the preview mode for tests by changing the hostname that was set while tests were running. This was mostly not being used to test preview but to run a bunch of legacy courseware tests while defaulting to the new learning MFE for the courseware. This commit updates various tests in the `courseware` app to not rely on the fact that we're in preview to test legacy courseware behavior and instead directly patches either the `_redirect_to_learning_mfe` function or uses the `_get_legacy_courseware_url` or both to be able to have the tests continue to test the legacy coursewary. This will hopefully make the tests more accuarte even though hopefully we'll just be removing many of them soon as a part of the legacy courseware cleanup. We're just doing the preview removal separately to reduce the number of things that are changing at once. --- lms/djangoapps/courseware/tests/helpers.py | 12 +-- .../courseware/tests/test_masquerade.py | 8 +- .../courseware/tests/test_navigation.py | 10 +- lms/djangoapps/courseware/tests/test_views.py | 96 ++++++++----------- lms/djangoapps/courseware/testutils.py | 9 +- 5 files changed, 59 insertions(+), 76 deletions(-) diff --git a/lms/djangoapps/courseware/tests/helpers.py b/lms/djangoapps/courseware/tests/helpers.py index 85241ead4a5b..2b5a33b2ac0f 100644 --- a/lms/djangoapps/courseware/tests/helpers.py +++ b/lms/djangoapps/courseware/tests/helpers.py @@ -8,9 +8,8 @@ import json from collections import OrderedDict from datetime import timedelta -from unittest.mock import Mock, patch +from unittest.mock import Mock -from django.conf import settings from django.contrib import messages from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user from django.test import TestCase @@ -20,7 +19,6 @@ from xblock.field_data import DictFieldData from common.djangoapps.edxmako.shortcuts import render_to_string -from lms.djangoapps.courseware import access_utils from lms.djangoapps.courseware.access import has_access from lms.djangoapps.courseware.utils import verified_upgrade_deadline_link from lms.djangoapps.courseware.masquerade import MasqueradeView @@ -460,11 +458,3 @@ def get_context_dict_from_string(data): sorted(json.loads(validated_data['metadata']).items(), key=lambda t: t[0]) ) return validated_data - - -def set_preview_mode(preview_mode: bool): - """ - A decorator to force the preview mode on or off. - """ - hostname = settings.FEATURES.get('PREVIEW_LMS_BASE') if preview_mode else None - return patch.object(access_utils, 'get_current_request_hostname', new=lambda: hostname) diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index 9d211b05ea21..89f84b3fa894 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -26,7 +26,7 @@ ) from lms.djangoapps.courseware.tests.helpers import ( - LoginEnrollmentTestCase, MasqueradeMixin, masquerade_as_group_member, set_preview_mode, + LoginEnrollmentTestCase, MasqueradeMixin, masquerade_as_group_member ) from lms.djangoapps.courseware.tests.test_submitting_problems import ProblemSubmissionTestMixin from openedx.core.djangoapps.lang_pref import LANGUAGE_KEY @@ -244,14 +244,14 @@ def testMasqueradeCohortAvailable(self, target, expected): assert is_target_available == expected -@set_preview_mode(True) +@patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) class TestStaffMasqueradeAsStudent(StaffMasqueradeTestCase): """ Check for staff being able to masquerade as student. """ @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) - def test_staff_debug_with_masquerade(self): + def test_staff_debug_with_masquerade(self, mock_redirect): """ Tests that staff debug control is not visible when masquerading as a student. """ @@ -267,7 +267,7 @@ def test_staff_debug_with_masquerade(self): self.verify_staff_debug_present(True) @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) - def test_show_answer_for_staff(self): + def test_show_answer_for_staff(self, mock_redirect): """ Tests that "Show Answer" is not visible when masquerading as a student. """ diff --git a/lms/djangoapps/courseware/tests/test_navigation.py b/lms/djangoapps/courseware/tests/test_navigation.py index 7d6e8cf04365..8c2144ee2a59 100644 --- a/lms/djangoapps/courseware/tests/test_navigation.py +++ b/lms/djangoapps/courseware/tests/test_navigation.py @@ -15,12 +15,11 @@ from xmodule.modulestore.tests.factories import CourseFactory, BlockFactory from common.djangoapps.student.tests.factories import GlobalStaffFactory -from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase, set_preview_mode +from lms.djangoapps.courseware.tests.helpers import LoginEnrollmentTestCase from openedx.features.course_experience import DISABLE_COURSE_OUTLINE_PAGE_FLAG from openedx.features.course_experience.url_helpers import make_learning_mfe_courseware_url -@set_preview_mode(True) class TestNavigation(SharedModuleStoreTestCase, LoginEnrollmentTestCase): """ Check that navigation state is saved properly. @@ -73,6 +72,13 @@ def setUpTestData(cls): # lint-amnesty, pylint: disable=super-method-not-called def setUp(self): super().setUp() self.login(self.user.email, 'test') + self.patcher = patch( + 'lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + self.mock_redirect = self.patcher.start() + + def tearDown(self): + self.patcher.stop() + super().tearDown() def assertTabActive(self, tabname, response): ''' Check if the progress tab is active in the tab set ''' diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 6303f07cf351..a24a57874b69 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -46,7 +46,6 @@ from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.course_modes.tests.factories import CourseModeFactory from common.djangoapps.student.models import CourseEnrollment -from common.djangoapps.student.roles import CourseStaffRole from common.djangoapps.student.tests.factories import ( TEST_PASSWORD, AdminFactory, @@ -75,7 +74,7 @@ from lms.djangoapps.courseware.model_data import FieldDataCache, set_score from lms.djangoapps.courseware.block_render import get_block, handle_xblock_callback from lms.djangoapps.courseware.tests.factories import StudentModuleFactory -from lms.djangoapps.courseware.tests.helpers import MasqueradeMixin, get_expiration_banner_text, set_preview_mode +from lms.djangoapps.courseware.tests.helpers import MasqueradeMixin, get_expiration_banner_text from lms.djangoapps.courseware.testutils import RenderXBlockTestMixin from lms.djangoapps.courseware.toggles import ( COURSEWARE_MICROFRONTEND_ALWAYS_OPEN_AUXILIARY_SIDEBAR, @@ -108,7 +107,7 @@ ) from openedx.features.course_experience.tests.views.helpers import add_course_mode from openedx.features.course_experience.url_helpers import ( - get_courseware_url, + _get_legacy_courseware_url, get_learning_mfe_home_url, make_learning_mfe_courseware_url ) @@ -152,12 +151,10 @@ def test_jump_to_invalid_location(self, preview_mode, store_type): # This is fragile, but unfortunately the problem is that within the LMS we # can't use the reverse calls from the CMS - with set_preview_mode(False): - response = self.client.get(jumpto_url) + response = self.client.get(jumpto_url) assert response.status_code == 302 assert response.url == expected_redirect_url - @set_preview_mode(False) def test_jump_to_preview_from_sequence(self): with self.store.default_store(ModuleStoreEnum.Type.split): course = CourseFactory.create() @@ -171,7 +168,6 @@ def test_jump_to_preview_from_sequence(self): assert response.status_code == 302 assert response.url == expected_redirect_url - @set_preview_mode(False) def test_jump_to_mfe_from_sequence(self): course = CourseFactory.create() chapter = BlockFactory.create(category='chapter', parent_location=course.location) @@ -184,7 +180,6 @@ def test_jump_to_mfe_from_sequence(self): assert response.status_code == 302 assert response.url == expected_redirect_url - @set_preview_mode(False) def test_jump_to_preview_from_block(self): with self.store.default_store(ModuleStoreEnum.Type.split): course = CourseFactory.create() @@ -211,7 +206,6 @@ def test_jump_to_preview_from_block(self): assert response.status_code == 302 assert response.url == expected_redirect_url - @set_preview_mode(False) def test_jump_to_mfe_from_block(self): course = CourseFactory.create() chapter = BlockFactory.create(category='chapter', parent_location=course.location) @@ -239,7 +233,7 @@ def test_jump_to_mfe_from_block(self): # The new courseware experience does not support this sort of course structure; # it assumes a simple course->chapter->sequence->unit->component tree. - @set_preview_mode(True) + @patch.object(views, 'get_courseware_url', new=_get_legacy_courseware_url) def test_jump_to_legacy_from_nested_block(self): with self.store.default_store(ModuleStoreEnum.Type.split): course = CourseFactory.create() @@ -275,11 +269,9 @@ def test_jump_to_id_invalid_location(self, preview_mode, store_type): ) if preview_mode else ( f'/courses/{course.id}/jump_to/NoSuchPlace' ) - with set_preview_mode(False): - response = self.client.get(jumpto_url) + response = self.client.get(jumpto_url) assert response.status_code == 404 - @set_preview_mode(True) @ddt.data( (ModuleStoreEnum.Type.split, False, '1'), (ModuleStoreEnum.Type.split, True, '2'), @@ -316,17 +308,17 @@ def test_jump_to_legacy_for_learner_with_staff_only_content(self, store_type, is } ) expected_url += "?{}".format(urlencode({'activate_block_id': str(staff_only_vertical.location)})) - assert expected_url == get_courseware_url(usage_key, request) + assert expected_url == _get_legacy_courseware_url(usage_key, request) -@set_preview_mode(True) class IndexQueryTestCase(ModuleStoreTestCase): """ Tests for query count. """ NUM_PROBLEMS = 20 - def test_index_query_counts(self): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_index_query_counts(self, mock_redirect): # TODO: decrease query count as part of REVO-28 ContentTypeGatingConfig.objects.create(enabled=True, enabled_as_of=datetime(2018, 1, 1)) with self.store.default_store(ModuleStoreEnum.Type.split): @@ -447,7 +439,6 @@ def _get_urls(self): # lint-amnesty, pylint: disable=missing-function-docstring @ddt.ddt -@set_preview_mode(True) class CoursewareIndexTestCase(BaseViewsTestCase): """ Tests for the courseware index view, used for instructor previews. @@ -456,7 +447,8 @@ def setUp(self): super().setUp() self._create_global_staff_user() # this view needs staff permission - def test_index_success(self): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_index_success(self, mock_redirect): response = self._verify_index_response() self.assertContains(response, self.problem2.location.replace(branch=None, version_guid=None)) @@ -468,7 +460,6 @@ def test_index_success(self): self.assertNotContains(response, self.problem.location.replace(branch=None, version_guid=None)) self.assertContains(response, self.problem2.location.replace(branch=None, version_guid=None)) - @set_preview_mode(True) def test_index_nonexistent_chapter(self): self._verify_index_response(expected_response_code=404, chapter_name='non-existent') @@ -505,12 +496,12 @@ def test_get_redirect_url(self): assert '/courses/{course_key}/courseware?{activate_block_id}'.format( course_key=str(self.course_key), activate_block_id=urlencode({'activate_block_id': str(self.course.location)}) - ) == get_courseware_url(self.course.location) + ) == _get_legacy_courseware_url(self.course.location) # test a section location assert '/courses/{course_key}/courseware/Chapter_1/Sequential_1/?{activate_block_id}'.format( course_key=str(self.course_key), activate_block_id=urlencode({'activate_block_id': str(self.section.location)}) - ) == get_courseware_url(self.section.location) + ) == _get_legacy_courseware_url(self.section.location) def test_index_invalid_position(self): request_url = '/'.join([ @@ -542,7 +533,8 @@ def test_unicode_handling_in_url(self): # TODO: TNL-6387: Remove test @override_waffle_flag(DISABLE_COURSE_OUTLINE_PAGE_FLAG, active=True) - def test_accordion(self): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_accordion(self, mock_redirect): """ This needs a response_context, which is not included in the render_accordion's main method returning a render_to_string, so we will render via the courseware URL in order to include @@ -1148,13 +1140,22 @@ def get_response(self, course): # TODO: LEARNER-71: Delete entire TestAccordionDueDate class -@set_preview_mode(True) class TestAccordionDueDate(BaseDueDateTests): """ Test that the accordion page displays due dates correctly """ __test__ = True + def setUp(self): + super().setUp() + self.patcher = patch( + 'lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + self.mock_redirect = self.patcher.start() + + def tearDown(self): + self.patcher.stop() + super().tearDown() + def get_response(self, course): """ Returns the HTML for the accordion """ return self.client.get( @@ -2363,13 +2364,13 @@ def student_view(self, context): # pylint: disable=unused-argument @ddt.ddt -@set_preview_mode(True) class TestIndexView(ModuleStoreTestCase): """ Tests of the courseware.views.index view. """ @XBlock.register_temp_plugin(ViewCheckerBlock, 'view_checker') - def test_student_state(self): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_student_state(self, mock_redirect): """ Verify that saved student state is loaded for xblocks rendered in the index view. """ @@ -2409,7 +2410,8 @@ def test_student_state(self): self.assertContains(response, "ViewCheckerPassed", count=3) @XBlock.register_temp_plugin(ActivateIDCheckerBlock, 'id_checker') - def test_activate_block_id(self): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_activate_block_id(self, mock_redirect): course = CourseFactory.create() with self.store.bulk_operations(course.id): chapter = BlockFactory.create(parent=course, category='chapter') @@ -2517,7 +2519,6 @@ def test_should_show_enroll_button(self, course_open_for_self_enrollment, @ddt.ddt -@set_preview_mode(True) class TestIndexViewCompleteOnView(ModuleStoreTestCase, CompletionWaffleTestMixin): """ Tests CompleteOnView is set up correctly in CoursewareIndex. @@ -2590,7 +2591,8 @@ def setup_course(self, default_store): CourseEnrollmentFactory(user=self.user, course_id=self.course.id) assert self.client.login(username=self.user.username, password=self.user_password) - def test_completion_service_disabled(self): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_completion_service_disabled(self, mock_redirect): self.setup_course(ModuleStoreEnum.Type.split) @@ -2600,7 +2602,8 @@ def test_completion_service_disabled(self): response = self.client.get(self.section_2_url) self.assertNotContains(response, 'data-mark-completed-on-view-after-delay') - def test_completion_service_enabled(self): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_completion_service_enabled(self, mock_redirect): self.override_waffle_switch(True) @@ -2652,7 +2655,6 @@ def test_completion_service_enabled(self): @ddt.ddt -@set_preview_mode(True) class TestIndexViewWithVerticalPositions(ModuleStoreTestCase): """ Test the index view to handle vertical positions. Confirms that first position is loaded @@ -2704,7 +2706,8 @@ def _assert_correct_position(self, response, expected_position): @ddt.data(("-1", 1), ("0", 1), ("-0", 1), ("2", 2), ("5", 1)) @ddt.unpack - def test_vertical_positions(self, input_position, expected_position): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_vertical_positions(self, input_position, expected_position, mock_redirect): """ Tests the following cases: * Load first position when negative position inputted. @@ -3097,7 +3100,6 @@ def course_options(self): return options -@set_preview_mode(True) class TestIndexViewCrawlerStudentStateWrites(SharedModuleStoreTestCase): """ Ensure that courseware index requests do not trigger student state writes. @@ -3128,7 +3130,8 @@ def setUp(self): super().setUp() self.client.login(username=self.user.username, password=TEST_PASSWORD) - def test_write_by_default(self): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_write_by_default(self, mock_redirect): """By default, always write student state, regardless of user agent.""" with patch('lms.djangoapps.courseware.model_data.UserStateCache.set_many') as patched_state_client_set_many: # Simulate someone using Chrome @@ -3140,7 +3143,8 @@ def test_write_by_default(self): self._load_courseware('edX-downloader/0.1') assert patched_state_client_set_many.called - def test_writes_with_config(self): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_writes_with_config(self, mock_redirect): """Test state writes (or lack thereof) based on config values.""" CrawlersConfig.objects.create(known_user_agents='edX-downloader,crawler_foo', enabled=True) with patch('lms.djangoapps.courseware.model_data.UserStateCache.set_many') as patched_state_client_set_many: @@ -3158,7 +3162,10 @@ def test_writes_with_config(self): # Disabling the crawlers config should revert us to default behavior CrawlersConfig.objects.create(enabled=False) - self.test_write_by_default() + + # Disabling the violation because pylint just can't see that we'll get the mock_redirect param passed in via the + # patch. + self.test_write_by_default() # pylint: disable=no-value-for-parameter def _load_courseware(self, user_agent): """Helper to load the actual courseware page.""" @@ -3321,25 +3328,6 @@ def test_url_generation(self): ) -class PreviewTests(BaseViewsTestCase): - """ - Make sure we allow the Legacy view for course previews. - """ - def test_learner_redirect(self): - # learners will be redirected by default - lms_url, mfe_url, __ = self._get_urls() - assert self.client.get(lms_url).url == mfe_url - - def test_preview_no_redirect(self): - __, __, preview_url = self._get_urls() - with set_preview_mode(True): - # Previews server from PREVIEW_LMS_BASE will not redirect to the mfe - course_staff = UserFactory.create(is_staff=False) - CourseStaffRole(self.course_key).add_users(course_staff) - self.client.login(username=course_staff.username, password=TEST_PASSWORD) - assert self.client.get(preview_url).status_code == 200 - - class ContentOptimizationTestCase(ModuleStoreTestCase): """ Test our ability to make browser optimizations based on XBlock content. diff --git a/lms/djangoapps/courseware/testutils.py b/lms/djangoapps/courseware/testutils.py index a18074e9ccbf..b0f444f288e8 100644 --- a/lms/djangoapps/courseware/testutils.py +++ b/lms/djangoapps/courseware/testutils.py @@ -11,9 +11,8 @@ import ddt from openedx.core.djangoapps.content.course_overviews.models import CourseOverview -from lms.djangoapps.courseware.tests.helpers import set_preview_mode from lms.djangoapps.courseware.utils import is_mode_upsellable -from openedx.features.course_experience.url_helpers import get_courseware_url +from openedx.features.course_experience.url_helpers import _get_legacy_courseware_url from common.djangoapps.student.tests.factories import AdminFactory, CourseEnrollmentFactory, UserFactory from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.course_modes.tests.factories import CourseModeFactory @@ -171,8 +170,8 @@ def verify_response(self, expected_response_code=200, url_params=None, is_staff= ('html_block', 4), ) @ddt.unpack - @set_preview_mode(True) - def test_courseware_html(self, block_name, mongo_calls): + @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) + def test_courseware_html(self, block_name, mongo_calls, mock_redirect): """ To verify that the removal of courseware chrome elements is working, we include this test here to make sure the chrome elements that should @@ -186,7 +185,7 @@ def test_courseware_html(self, block_name, mongo_calls): self.setup_user(admin=True, enroll=True, login=True) with check_mongo_calls(mongo_calls): - url = get_courseware_url(self.block_to_be_tested.location) + url = _get_legacy_courseware_url(self.block_to_be_tested.location) response = self.client.get(url) expected_elements = self.block_specific_chrome_html_elements + self.COURSEWARE_CHROME_HTML_ELEMENTS for chrome_element in expected_elements: From beb9f23ef044573ab288b2a20879ce990af208dc Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Fri, 28 Mar 2025 17:41:31 -0400 Subject: [PATCH 6/9] test: Drop the `_get_urls_function` With the other recent cleanup, this function is no longer being referenced by anything so we can just drop it. --- lms/djangoapps/courseware/tests/test_views.py | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index a24a57874b69..e3f9aa0d59ab 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -419,24 +419,6 @@ def _create_global_staff_user(self): self.global_staff = GlobalStaffFactory.create() # pylint: disable=attribute-defined-outside-init assert self.client.login(username=self.global_staff.username, password=TEST_PASSWORD) - def _get_urls(self): # lint-amnesty, pylint: disable=missing-function-docstring - lms_url = reverse( - 'courseware_section', - kwargs={ - 'course_id': str(self.course_key), - 'chapter': str(self.chapter.location.block_id), - 'section': str(self.section2.location.block_id), - }, - ) + '?foo=b$r' - mfe_url = '{}/course/{}/{}?foo=b%24r'.format( - settings.LEARNING_MICROFRONTEND_URL, - self.course_key, - self.section2.location - ) - preview_url = "http://" + settings.FEATURES.get('PREVIEW_LMS_BASE') + lms_url - - return lms_url, mfe_url, preview_url - @ddt.ddt class CoursewareIndexTestCase(BaseViewsTestCase): From 4865cab5d1ad6eb92cee125d04ac414b2cf5dcb9 Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 29 Apr 2025 15:47:17 -0400 Subject: [PATCH 7/9] test: Test student access to unpublihsed content. Ensure that students can't get access to unpublished content. --- .../courseware/tests/test_access.py | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index 0c7cc248ceed..a72a8512502d 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -49,8 +49,10 @@ CATALOG_VISIBILITY_CATALOG_AND_ABOUT, CATALOG_VISIBILITY_NONE ) + from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order +from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.modulestore.tests.django_utils import ( # lint-amnesty, pylint: disable=wrong-import-order ModuleStoreTestCase, SharedModuleStoreTestCase @@ -245,18 +247,39 @@ def test_student_has_access(self): course_key = self.course.id chapter = BlockFactory.create(category="chapter", parent_location=self.course.location) overview = CourseOverview.get_from_id(course_key) + subsection = BlockFactory.create(category="sequential", parent_location=chapter.location) + unit = BlockFactory.create(category="vertical", parent_location=subsection.location) + + with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): + html_block = BlockFactory.create( + category="html", + parent_location=unit.location, + display_name = "Unpublished Block", + data = '

This block should not be published.

', + publish_item = False, + ) + print(self.store.get_branch_setting()) + print(self.store.get_branch_setting()) # Enroll student to the course CourseEnrollmentFactory(user=self.student, course_id=self.course.id) modules = [ self.course, - overview, chapter, + overview, + subsection, + unit, + ] # Student should have access to modules they're enrolled in for obj in modules: - assert bool(access.has_access(self.student, 'load', obj, course_key=self.course.id)) + assert bool(access.has_access(self.student, 'load', self.store.get_item(obj.location), course_key=self.course.id)) + + # If the document is not published yet, it should return an error when we try to fetch it from + # the store. This check confirms that the student would not be able to access it. + with pytest.raises(ItemNotFoundError): + self.store.get_item(html_block.location) def test_has_access_based_on_roles(self): """ From ab4a81ec8634055949da6daad23aba5cfab2cb7b Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 13 May 2025 11:53:59 -0400 Subject: [PATCH 8/9] docs: Update lms/djangoapps/courseware/tests/test_masquerade.py --- lms/djangoapps/courseware/tests/test_masquerade.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index 89f84b3fa894..fb245df9e8bf 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -244,6 +244,12 @@ def testMasqueradeCohortAvailable(self, target, expected): assert is_target_available == expected +# These tests are testing a capability of the old courseware page. We have to not +# force redirect to the new MFE in order to be able to load the old pages which are +# being tested by this page. +# +# This is a temporary change, until we can remove the old courseware pages +# all together. @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) class TestStaffMasqueradeAsStudent(StaffMasqueradeTestCase): """ From 3f809248c82e820fd755c1db59483375a31397da Mon Sep 17 00:00:00 2001 From: Feanil Patel Date: Tue, 13 May 2025 15:16:14 -0400 Subject: [PATCH 9/9] style: Fix various lint issues. Also remove some errant debugging print statements. --- .../courseware/tests/test_access.py | 19 +++++++++++-------- .../courseware/tests/test_masquerade.py | 2 +- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_access.py b/lms/djangoapps/courseware/tests/test_access.py index a72a8512502d..bb6d42025ea6 100644 --- a/lms/djangoapps/courseware/tests/test_access.py +++ b/lms/djangoapps/courseware/tests/test_access.py @@ -254,13 +254,11 @@ def test_student_has_access(self): html_block = BlockFactory.create( category="html", parent_location=unit.location, - display_name = "Unpublished Block", - data = '

This block should not be published.

', - publish_item = False, + display_name="Unpublished Block", + data='

This block should not be published.

', + publish_item=False, ) - print(self.store.get_branch_setting()) - print(self.store.get_branch_setting()) # Enroll student to the course CourseEnrollmentFactory(user=self.student, course_id=self.course.id) @@ -274,10 +272,15 @@ def test_student_has_access(self): ] # Student should have access to modules they're enrolled in for obj in modules: - assert bool(access.has_access(self.student, 'load', self.store.get_item(obj.location), course_key=self.course.id)) + assert bool(access.has_access( + self.student, + 'load', + self.store.get_item(obj.location), + course_key=self.course.id) + ) - # If the document is not published yet, it should return an error when we try to fetch it from - # the store. This check confirms that the student would not be able to access it. + # If the document is not published yet, it should return an error when we try to fetch + # it from the store. This check confirms that the student would not be able to access it. with pytest.raises(ItemNotFoundError): self.store.get_item(html_block.location) diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index fb245df9e8bf..33d100c7a298 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -249,7 +249,7 @@ def testMasqueradeCohortAvailable(self, target, expected): # being tested by this page. # # This is a temporary change, until we can remove the old courseware pages -# all together. +# all together. @patch('lms.djangoapps.courseware.views.index.CoursewareIndex._redirect_to_learning_mfe', return_value=None) class TestStaffMasqueradeAsStudent(StaffMasqueradeTestCase): """