From 7101bc099c6652b8d0cf26c54c35fbfaf2b3ab41 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Mon, 21 Oct 2024 17:09:50 -0400 Subject: [PATCH 01/16] feat: update preview url to direct ot mfe --- cms/djangoapps/contentstore/utils.py | 16 ++++++---------- cms/djangoapps/contentstore/views/component.py | 1 + lms/djangoapps/courseware/views/index.py | 1 + lms/djangoapps/courseware/views/views.py | 11 ++++++++++- .../features/course_experience/url_helpers.py | 9 ++++++++- 5 files changed, 26 insertions(+), 12 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 214193918eb4..9cf1f9ec0f9e 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -190,30 +190,26 @@ def get_lms_link_for_item(location, preview=False): """ assert isinstance(location, UsageKey) - # checks LMS_BASE value in site configuration for the given course_org_filter(org) - # if not found returns settings.LMS_BASE lms_base = SiteConfiguration.get_value_for_org( location.org, "LMS_BASE", settings.LMS_BASE ) + params = '' if lms_base is None: return None if preview: - # checks PREVIEW_LMS_BASE value in site configuration for the given course_org_filter(org) - # if not found returns settings.FEATURES.get('PREVIEW_LMS_BASE') - lms_base = SiteConfiguration.get_value_for_org( - location.org, - "PREVIEW_LMS_BASE", - settings.FEATURES.get('PREVIEW_LMS_BASE') - ) + # checks LMS_BASE value in site configuration for the given course_org_filter(org) + # if not found returns settings.LMS_BASE + params = '?preview=1' - return "//{lms_base}/courses/{course_key}/jump_to/{location}".format( + return "//{lms_base}/courses/{course_key}/jump_to/{location}{params}".format( lms_base=lms_base, course_key=str(location.course_key), location=str(location), + params=params, ) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index ba767df78dc9..787995ccaedf 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -466,6 +466,7 @@ def _get_item_in_course(request, usage_key): item = modulestore().get_item(usage_key, depth=1) lms_link = get_lms_link_for_item(item.location) preview_lms_link = get_lms_link_for_item(item.location, preview=True) + print(preview_lms_link) return course, item, lms_link, preview_lms_link diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index c123c7f71f4c..6e6d0958f6e2 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -402,6 +402,7 @@ def _create_courseware_context(self, request): """ course_url = default_course_url(self.course.id) + print(course_url, self.chapter, self.section) show_search = ( settings.FEATURES.get('ENABLE_COURSEWARE_SEARCH') or (settings.FEATURES.get('ENABLE_COURSEWARE_SEARCH_FOR_COURSE_STAFF') and self.is_staff) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 79a52db8a0b6..3947ce0ec9ff 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -47,6 +47,7 @@ from token_utils.api import unpack_token_for from web_fragments.fragment import Fragment from xmodule.course_block import COURSE_VISIBILITY_PUBLIC, COURSE_VISIBILITY_PUBLIC_OUTLINE +from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from xmodule.tabs import CourseTabList @@ -437,6 +438,7 @@ def jump_to(request, course_id, location): usage_key=usage_key, request=request, ) + print(redirect_url) except (ItemNotFoundError, NoPathToItem): # We used to 404 here, but that's ultimately a bad experience. There are real world use cases where a user # hits a no-longer-valid URL (for example, "resume" buttons that link to no-longer-existing block IDs if the @@ -1559,8 +1561,15 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_sta ) staff_access = has_access(request.user, 'staff', course_key) + is_preview = request.GET.get('preview') == '1' - with modulestore().bulk_operations(course_key): + store = modulestore() + branchType = ModuleStoreEnum.Branch.draft_preferred if is_preview else ModuleStoreEnum.Branch.published_only + + if is_preview and not staff_access: + return HttpResponseBadRequest("You do not have access to preview this xblock") + + with store.branch_setting(branchType, course_key): # verify the user has access to the course, including enrollment check try: course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=check_if_enrolled) diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index f62e879f0994..81ce5b04ce20 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -120,12 +120,14 @@ def _get_new_courseware_url( course_key=course_key, sequence_key=sequence_key, unit_key=unit_key, + preview=request.GET.get('preview') if request and request.GET else False, params=request.GET if request and request.GET else None, ) def make_learning_mfe_courseware_url( course_key: CourseKey, + preview: bool, sequence_key: Optional[UsageKey] = None, unit_key: Optional[UsageKey] = None, params: Optional[QueryDict] = None, @@ -160,6 +162,9 @@ def make_learning_mfe_courseware_url( `params` is an optional QueryDict object (e.g. request.GET) """ mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/course/{course_key}' + + if preview: + mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/preview/course/{course_key}' if sequence_key: mfe_link += f'/{sequence_key}' @@ -168,7 +173,9 @@ def make_learning_mfe_courseware_url( mfe_link += f'/{unit_key}' if params: - mfe_link += f'?{params.urlencode()}' + get_params = params.copy() + del get_params['preview'] + mfe_link += f'?{get_params.urlencode()}' return mfe_link From a4f6a26a23364506633a032e71972ba7017170d3 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Tue, 22 Oct 2024 11:22:03 -0400 Subject: [PATCH 02/16] fix: code styling errors --- cms/djangoapps/contentstore/utils.py | 2 +- openedx/features/course_experience/url_helpers.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index d02de61d1df0..03e322a3a2de 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -204,7 +204,7 @@ def get_lms_link_for_item(location, preview=False): if preview: # checks LMS_BASE value in site configuration for the given course_org_filter(org) - # if not found returns settings.LMS_BASE + # if not found returns settings.LMS_BASE params = '?preview=1' return "//{lms_base}/courses/{course_key}/jump_to/{location}{params}".format( diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index 81ce5b04ce20..be693f2f141e 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -162,7 +162,7 @@ def make_learning_mfe_courseware_url( `params` is an optional QueryDict object (e.g. request.GET) """ mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/course/{course_key}' - + if preview: mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/preview/course/{course_key}' From 07d8e9c7a0e250bfb69d78e7feb60263a7ec7d5e Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Tue, 22 Oct 2024 12:23:12 -0400 Subject: [PATCH 03/16] fix: failing test --- lms/djangoapps/courseware/tests/test_views.py | 6 +++++- openedx/features/course_experience/url_helpers.py | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index cde0e8a34ec2..c8351234cc07 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -467,7 +467,11 @@ def _get_urls(self): # lint-amnesty, pylint: disable=missing-function-docstring self.course_key, self.section2.location ) - preview_url = "http://" + settings.FEATURES.get('PREVIEW_LMS_BASE') + lms_url + preview_url = '{}/preview/course/{}/{}?foo=b%24r'.format( + settings.LEARNING_MICROFRONTEND_URL, + self.course_key, + self.section2.location + ) return lms_url, mfe_url, preview_url diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index be693f2f141e..a01f53a510c6 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -127,7 +127,7 @@ def _get_new_courseware_url( def make_learning_mfe_courseware_url( course_key: CourseKey, - preview: bool, + preview: bool = None, sequence_key: Optional[UsageKey] = None, unit_key: Optional[UsageKey] = None, params: Optional[QueryDict] = None, From c38940b2f337bf1fd922995a24fc24a09978768c Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Wed, 23 Oct 2024 15:19:33 -0400 Subject: [PATCH 04/16] fix: courseware view tests --- .../contentstore/views/component.py | 1 - lms/djangoapps/courseware/tests/test_views.py | 89 ++----- lms/djangoapps/courseware/views/index.py | 4 +- lms/djangoapps/courseware/views/views.py | 250 +++++++++--------- .../features/course_experience/url_helpers.py | 17 +- 5 files changed, 166 insertions(+), 195 deletions(-) diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 0a198fad8963..b89bef0f6709 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -514,7 +514,6 @@ def _get_item_in_course(request, usage_key): item = modulestore().get_item(usage_key, depth=1) lms_link = get_lms_link_for_item(item.location) preview_lms_link = get_lms_link_for_item(item.location, preview=True) - print(preview_lms_link) return course, item, lms_link, preview_lms_link diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index c8351234cc07..19b19d633fb5 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -132,38 +132,6 @@ class TestJumpTo(ModuleStoreTestCase): """ Check the jumpto link for a course. """ - @ddt.data( - (True, False), # preview -> Legacy experience - (False, True), # no preview -> MFE experience - ) - @ddt.unpack - def test_jump_to_legacy_vs_mfe(self, preview_mode, expect_mfe): - """ - Test that jump_to and jump_to_id correctly choose which courseware frontend to redirect to. - - Can be removed when the MFE supports a preview mode. - """ - course = CourseFactory.create() - chapter = BlockFactory.create(category='chapter', parent_location=course.location) - if expect_mfe: - expected_url = f'http://learning-mfe/course/{course.id}/{chapter.location}' - else: - expected_url = f'/courses/{course.id}/courseware/{chapter.url_name}/' - - jumpto_url = f'/courses/{course.id}/jump_to/{chapter.location}' - with set_preview_mode(preview_mode): - response = self.client.get(jumpto_url) - assert response.status_code == 302 - # Check the response URL, but chop off the querystring; we don't care here. - assert response.url.split('?')[0] == expected_url - - jumpto_id_url = f'/courses/{course.id}/jump_to_id/{chapter.url_name}' - with set_preview_mode(preview_mode): - response = self.client.get(jumpto_id_url) - assert response.status_code == 302 - # Check the response URL, but chop off the querystring; we don't care here. - assert response.url.split('?')[0] == expected_url - @ddt.data( (False, ModuleStoreEnum.Type.split), (True, ModuleStoreEnum.Type.split), @@ -174,32 +142,31 @@ def test_jump_to_invalid_location(self, preview_mode, store_type): with self.store.default_store(store_type): course = CourseFactory.create() location = course.id.make_usage_key(None, 'NoSuchPlace') - expected_redirect_url = ( - f'/courses/{course.id}/courseware?' + urlencode({'activate_block_id': str(course.location)}) - ) if preview_mode else ( - f'http://learning-mfe/course/{course.id}' - ) + + expected_redirect_url = f'http://learning-mfe/course/{course.id}' + jumpto_url = f'/courses/{course.id}/jump_to/{location}?preview=1' if preview_mode else f'/courses/{course.id}/jump_to/{location}' + # This is fragile, but unfortunately the problem is that within the LMS we # can't use the reverse calls from the CMS - jumpto_url = f'/courses/{course.id}/jump_to/{location}' - with set_preview_mode(preview_mode): + + with set_preview_mode(False): response = self.client.get(jumpto_url) assert response.status_code == 302 assert response.url == expected_redirect_url - @set_preview_mode(True) - def test_jump_to_legacy_from_sequence(self): + @set_preview_mode(False) + def test_jump_to_preview_from_sequence(self): with self.store.default_store(ModuleStoreEnum.Type.split): course = CourseFactory.create() chapter = BlockFactory.create(category='chapter', parent_location=course.location) sequence = BlockFactory.create(category='sequential', parent_location=chapter.location) - activate_block_id = urlencode({'activate_block_id': str(sequence.location)}) + jumpto_url = f'/courses/{course.id}/jump_to/{sequence.location}?preview=1' expected_redirect_url = ( - f'/courses/{course.id}/courseware/{chapter.url_name}/{sequence.url_name}/?{activate_block_id}' + f'http://learning-mfe/preview/course/{course.id}/{sequence.location}' ) - jumpto_url = f'/courses/{course.id}/jump_to/{sequence.location}' response = self.client.get(jumpto_url) - self.assertRedirects(response, expected_redirect_url, status_code=302, target_status_code=302) + assert response.status_code == 302 + assert response.url == expected_redirect_url @set_preview_mode(False) def test_jump_to_mfe_from_sequence(self): @@ -214,8 +181,8 @@ def test_jump_to_mfe_from_sequence(self): assert response.status_code == 302 assert response.url == expected_redirect_url - @set_preview_mode(True) - def test_jump_to_legacy_from_block(self): + @set_preview_mode(False) + def test_jump_to_preview_from_block(self): with self.store.default_store(ModuleStoreEnum.Type.split): course = CourseFactory.create() chapter = BlockFactory.create(category='chapter', parent_location=course.location) @@ -225,21 +192,21 @@ def test_jump_to_legacy_from_block(self): block1 = BlockFactory.create(category='html', parent_location=vertical1.location) block2 = BlockFactory.create(category='html', parent_location=vertical2.location) - activate_block_id = urlencode({'activate_block_id': str(block1.location)}) + jumpto_url = f'/courses/{course.id}/jump_to/{block1.location}?preview=1' expected_redirect_url = ( - f'/courses/{course.id}/courseware/{chapter.url_name}/{sequence.url_name}/1?{activate_block_id}' + f'http://learning-mfe/preview/course/{course.id}/{sequence.location}/{vertical1.location}' ) - jumpto_url = f'/courses/{course.id}/jump_to/{block1.location}' response = self.client.get(jumpto_url) - self.assertRedirects(response, expected_redirect_url, status_code=302, target_status_code=302) + assert response.status_code == 302 + assert response.url == expected_redirect_url - activate_block_id = urlencode({'activate_block_id': str(block2.location)}) + jumpto_url = f'/courses/{course.id}/jump_to/{block2.location}?preview=1' expected_redirect_url = ( - f'/courses/{course.id}/courseware/{chapter.url_name}/{sequence.url_name}/2?{activate_block_id}' + f'http://learning-mfe/preview/course/{course.id}/{sequence.location}/{vertical2.location}' ) - jumpto_url = f'/courses/{course.id}/jump_to/{block2.location}' response = self.client.get(jumpto_url) - self.assertRedirects(response, expected_redirect_url, status_code=302, target_status_code=302) + assert response.status_code == 302 + assert response.url == expected_redirect_url @set_preview_mode(False) def test_jump_to_mfe_from_block(self): @@ -300,8 +267,8 @@ def test_jump_to_legacy_from_nested_block(self): def test_jump_to_id_invalid_location(self, preview_mode, store_type): with self.store.default_store(store_type): course = CourseFactory.create() - jumpto_url = f'/courses/{course.id}/jump_to/NoSuchPlace' - with set_preview_mode(preview_mode): + jumpto_url = f'/courses/{course.id}/jump_to/NoSuchPlace?preview=1' if preview_mode else f'/courses/{course.id}/jump_to/NoSuchPlace' + with set_preview_mode(False): response = self.client.get(jumpto_url) assert response.status_code == 404 @@ -467,11 +434,7 @@ def _get_urls(self): # lint-amnesty, pylint: disable=missing-function-docstring self.course_key, self.section2.location ) - preview_url = '{}/preview/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 @@ -3363,7 +3326,7 @@ def test_learner_redirect(self): def test_preview_no_redirect(self): __, __, preview_url = self._get_urls() with set_preview_mode(True): - # Previews will not redirect to the mfe + # 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) diff --git a/lms/djangoapps/courseware/views/index.py b/lms/djangoapps/courseware/views/index.py index 6e6d0958f6e2..7a9242f595ef 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -27,6 +27,7 @@ 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 @@ -188,11 +189,13 @@ def microfrontend_url(self): unit_key = None except InvalidKeyError: unit_key = None + is_preview = settings.FEATURES.get('PREVIEW_LMS_BASE') == get_current_request_hostname() url = make_learning_mfe_courseware_url( self.course_key, self.section.location if self.section else None, unit_key, params=self.request.GET, + preview=is_preview, ) return url @@ -402,7 +405,6 @@ def _create_courseware_context(self, request): """ course_url = default_course_url(self.course.id) - print(course_url, self.chapter, self.section) show_search = ( settings.FEATURES.get('ENABLE_COURSEWARE_SEARCH') or (settings.FEATURES.get('ENABLE_COURSEWARE_SEARCH_FOR_COURSE_STAFF') and self.is_staff) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 10636dac9e33..46525628f6e5 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -445,7 +445,6 @@ def jump_to(request, course_id, location): usage_key=usage_key, request=request, ) - print(redirect_url) except (ItemNotFoundError, NoPathToItem): # We used to 404 here, but that's ultimately a bad experience. There are real world use cases where a user # hits a no-longer-valid URL (for example, "resume" buttons that link to no-longer-existing block IDs if the @@ -1576,140 +1575,141 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_sta if is_preview and not staff_access: return HttpResponseBadRequest("You do not have access to preview this xblock") - with store.branch_setting(branchType, course_key): - # verify the user has access to the course, including enrollment check - try: - course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=check_if_enrolled) - except CourseAccessRedirect: - raise Http404("Course not found.") # lint-amnesty, pylint: disable=raise-missing-from - - # with course access now verified: - # assume masquerading role, if applicable. - # (if we did this *before* the course access check, then course staff - # masquerading as learners would often be denied access, since course - # staff are generally not enrolled, and viewing a course generally - # requires enrollment.) - _course_masquerade, request.user = setup_masquerade( - request, - course_key, - staff_access, - ) - - # Record user activity for tracking progress towards a user's course goals (for mobile app) - UserActivity.record_user_activity( - request.user, usage_key.course_key, request=request, only_if_mobile_app=True - ) + with store.bulk_operations(course_key): + with store.branch_setting(branchType, course_key): + # verify the user has access to the course, including enrollment check + try: + course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=check_if_enrolled) + except CourseAccessRedirect: + raise Http404("Course not found.") # lint-amnesty, pylint: disable=raise-missing-from + + # with course access now verified: + # assume masquerading role, if applicable. + # (if we did this *before* the course access check, then course staff + # masquerading as learners would often be denied access, since course + # staff are generally not enrolled, and viewing a course generally + # requires enrollment.) + _course_masquerade, request.user = setup_masquerade( + request, + course_key, + staff_access, + ) - # get the block, which verifies whether the user has access to the block. - recheck_access = request.GET.get('recheck_access') == '1' - block, _ = get_block_by_usage_id( - request, - str(course_key), - str(usage_key), - disable_staff_debug_info=disable_staff_debug_info, - course=course, - will_recheck_access=recheck_access, - ) + # Record user activity for tracking progress towards a user's course goals (for mobile app) + UserActivity.record_user_activity( + request.user, usage_key.course_key, request=request, only_if_mobile_app=True + ) - student_view_context = request.GET.dict() - student_view_context['show_bookmark_button'] = request.GET.get('show_bookmark_button', '0') == '1' - student_view_context['show_title'] = request.GET.get('show_title', '1') == '1' - - is_learning_mfe = is_request_from_learning_mfe(request) - # Right now, we only care about this in regards to the Learning MFE because it results - # in a bad UX if we display blocks with access errors (repeated upgrade messaging). - # If other use cases appear, consider removing the is_learning_mfe check or switching this - # to be its own query parameter that can toggle the behavior. - student_view_context['hide_access_error_blocks'] = is_learning_mfe and recheck_access - is_mobile_app = is_request_from_mobile_app(request) - student_view_context['is_mobile_app'] = is_mobile_app - - enable_completion_on_view_service = False - completion_service = block.runtime.service(block, 'completion') - if completion_service and completion_service.completion_tracking_enabled(): - if completion_service.blocks_to_mark_complete_on_view({block}): - enable_completion_on_view_service = True - student_view_context['wrap_xblock_data'] = { - 'mark-completed-on-view-after-delay': completion_service.get_complete_on_view_delay_ms() - } + # get the block, which verifies whether the user has access to the block. + recheck_access = request.GET.get('recheck_access') == '1' + block, _ = get_block_by_usage_id( + request, + str(course_key), + str(usage_key), + disable_staff_debug_info=disable_staff_debug_info, + course=course, + will_recheck_access=recheck_access, + ) - missed_deadlines, missed_gated_content = dates_banner_should_display(course_key, request.user) - - # Some content gating happens only at the Sequence level (e.g. "has this - # timed exam started?"). - ancestor_sequence_block = enclosing_sequence_for_gating_checks(block) - if ancestor_sequence_block: - context = {'specific_masquerade': is_masquerading_as_specific_student(request.user, course_key)} - # If the SequenceModule feels that gating is necessary, redirect - # there so we can have some kind of error message at any rate. - if ancestor_sequence_block.descendants_are_gated(context): - return redirect( - reverse( - 'render_xblock', - kwargs={'usage_key_string': str(ancestor_sequence_block.location)} + student_view_context = request.GET.dict() + student_view_context['show_bookmark_button'] = request.GET.get('show_bookmark_button', '0') == '1' + student_view_context['show_title'] = request.GET.get('show_title', '1') == '1' + + is_learning_mfe = is_request_from_learning_mfe(request) + # Right now, we only care about this in regards to the Learning MFE because it results + # in a bad UX if we display blocks with access errors (repeated upgrade messaging). + # If other use cases appear, consider removing the is_learning_mfe check or switching this + # to be its own query parameter that can toggle the behavior. + student_view_context['hide_access_error_blocks'] = is_learning_mfe and recheck_access + is_mobile_app = is_request_from_mobile_app(request) + student_view_context['is_mobile_app'] = is_mobile_app + + enable_completion_on_view_service = False + completion_service = block.runtime.service(block, 'completion') + if completion_service and completion_service.completion_tracking_enabled(): + if completion_service.blocks_to_mark_complete_on_view({block}): + enable_completion_on_view_service = True + student_view_context['wrap_xblock_data'] = { + 'mark-completed-on-view-after-delay': completion_service.get_complete_on_view_delay_ms() + } + + missed_deadlines, missed_gated_content = dates_banner_should_display(course_key, request.user) + + # Some content gating happens only at the Sequence level (e.g. "has this + # timed exam started?"). + ancestor_sequence_block = enclosing_sequence_for_gating_checks(block) + if ancestor_sequence_block: + context = {'specific_masquerade': is_masquerading_as_specific_student(request.user, course_key)} + # If the SequenceModule feels that gating is necessary, redirect + # there so we can have some kind of error message at any rate. + if ancestor_sequence_block.descendants_are_gated(context): + return redirect( + reverse( + 'render_xblock', + kwargs={'usage_key_string': str(ancestor_sequence_block.location)} + ) ) - ) - # For courses using an LTI provider managed by edx-exams: - # Access to exam content is determined by edx-exams and passed to the LMS using a - # JWT url param. There is no longer a need for exam gating or logic inside the - # sequence block or its render call. descendants_are_gated shoule not return true - # for these timed exams. Instead, sequences are assumed gated by default and we look for - # an access token on the request to allow rendering to continue. - if course.proctoring_provider == 'lti_external': - seq_block = ancestor_sequence_block if ancestor_sequence_block else block - if getattr(seq_block, 'is_time_limited', None): - if not _check_sequence_exam_access(request, seq_block.location): - return HttpResponseForbidden("Access to exam content is restricted") + # For courses using an LTI provider managed by edx-exams: + # Access to exam content is determined by edx-exams and passed to the LMS using a + # JWT url param. There is no longer a need for exam gating or logic inside the + # sequence block or its render call. descendants_are_gated shoule not return true + # for these timed exams. Instead, sequences are assumed gated by default and we look for + # an access token on the request to allow rendering to continue. + if course.proctoring_provider == 'lti_external': + seq_block = ancestor_sequence_block if ancestor_sequence_block else block + if getattr(seq_block, 'is_time_limited', None): + if not _check_sequence_exam_access(request, seq_block.location): + return HttpResponseForbidden("Access to exam content is restricted") + + context = { + 'course': course, + 'block': block, + 'disable_accordion': True, + 'allow_iframing': True, + 'disable_header': True, + 'disable_footer': True, + 'disable_window_wrap': True, + 'enable_completion_on_view_service': enable_completion_on_view_service, + 'edx_notes_enabled': is_feature_enabled(course, request.user), + 'staff_access': staff_access, + 'xqa_server': settings.FEATURES.get('XQA_SERVER', 'http://your_xqa_server.com'), + 'missed_deadlines': missed_deadlines, + 'missed_gated_content': missed_gated_content, + 'has_ended': course.has_ended(), + 'web_app_course_url': get_learning_mfe_home_url(course_key=course.id, url_fragment='home'), + 'on_courseware_page': True, + 'verified_upgrade_link': verified_upgrade_deadline_link(request.user, course=course), + 'is_learning_mfe': is_learning_mfe, + 'is_mobile_app': is_mobile_app, + 'render_course_wide_assets': True, + } - context = { - 'course': course, - 'block': block, - 'disable_accordion': True, - 'allow_iframing': True, - 'disable_header': True, - 'disable_footer': True, - 'disable_window_wrap': True, - 'enable_completion_on_view_service': enable_completion_on_view_service, - 'edx_notes_enabled': is_feature_enabled(course, request.user), - 'staff_access': staff_access, - 'xqa_server': settings.FEATURES.get('XQA_SERVER', 'http://your_xqa_server.com'), - 'missed_deadlines': missed_deadlines, - 'missed_gated_content': missed_gated_content, - 'has_ended': course.has_ended(), - 'web_app_course_url': get_learning_mfe_home_url(course_key=course.id, url_fragment='home'), - 'on_courseware_page': True, - 'verified_upgrade_link': verified_upgrade_deadline_link(request.user, course=course), - 'is_learning_mfe': is_learning_mfe, - 'is_mobile_app': is_mobile_app, - 'render_course_wide_assets': True, - } + try: + # .. filter_implemented_name: RenderXBlockStarted + # .. filter_type: org.openedx.learning.xblock.render.started.v1 + context, student_view_context = RenderXBlockStarted.run_filter( + context=context, student_view_context=student_view_context + ) + except RenderXBlockStarted.PreventXBlockBlockRender as exc: + log.info("Halted rendering block %s. Reason: %s", usage_key_string, exc.message) + return render_500(request) + except RenderXBlockStarted.RenderCustomResponse as exc: + log.info("Rendering custom exception for block %s. Reason: %s", usage_key_string, exc.message) + context.update({ + 'fragment': Fragment(exc.response) + }) + return render_to_response('courseware/courseware-chromeless.html', context, request=request) + + fragment = block.render(requested_view, context=student_view_context) + optimization_flags = get_optimization_flags_for_content(block, fragment) - try: - # .. filter_implemented_name: RenderXBlockStarted - # .. filter_type: org.openedx.learning.xblock.render.started.v1 - context, student_view_context = RenderXBlockStarted.run_filter( - context=context, student_view_context=student_view_context - ) - except RenderXBlockStarted.PreventXBlockBlockRender as exc: - log.info("Halted rendering block %s. Reason: %s", usage_key_string, exc.message) - return render_500(request) - except RenderXBlockStarted.RenderCustomResponse as exc: - log.info("Rendering custom exception for block %s. Reason: %s", usage_key_string, exc.message) context.update({ - 'fragment': Fragment(exc.response) + 'fragment': fragment, + **optimization_flags, }) - return render_to_response('courseware/courseware-chromeless.html', context, request=request) - - fragment = block.render(requested_view, context=student_view_context) - optimization_flags = get_optimization_flags_for_content(block, fragment) - context.update({ - 'fragment': fragment, - **optimization_flags, - }) - - return render_to_response('courseware/courseware-chromeless.html', context, request=request) + return render_to_response('courseware/courseware-chromeless.html', context, request=request) def get_optimization_flags_for_content(block, fragment): diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index a01f53a510c6..d7b64a5f89dd 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -127,10 +127,10 @@ def _get_new_courseware_url( def make_learning_mfe_courseware_url( course_key: CourseKey, - preview: bool = None, sequence_key: Optional[UsageKey] = None, unit_key: Optional[UsageKey] = None, params: Optional[QueryDict] = None, + preview: bool = None, ) -> str: """ Return a str with the URL for the specified courseware content in the Learning MFE. @@ -162,9 +162,16 @@ def make_learning_mfe_courseware_url( `params` is an optional QueryDict object (e.g. request.GET) """ mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/course/{course_key}' + get_params = params.copy() if params else None if preview: - mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/preview/course/{course_key}' + if len(get_params.keys()) > 1: + del get_params['preview'] + else: + get_params = None + + if (unit_key or sequence_key): + mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/preview/course/{course_key}' if sequence_key: mfe_link += f'/{sequence_key}' @@ -172,11 +179,11 @@ def make_learning_mfe_courseware_url( if unit_key: mfe_link += f'/{unit_key}' - if params: - get_params = params.copy() - del get_params['preview'] + if get_params: mfe_link += f'?{get_params.urlencode()}' + print(mfe_link) + return mfe_link From 4002c996195b91ecf33af67a218b785c95e538c0 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Wed, 23 Oct 2024 16:28:31 -0400 Subject: [PATCH 05/16] fix: use url builder instead of string formater --- cms/djangoapps/contentstore/utils.py | 23 +++++++++++-------- .../features/course_experience/url_helpers.py | 18 ++++++++++----- 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 03e322a3a2de..6a9d11e9325a 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -8,7 +8,7 @@ from collections import defaultdict from contextlib import contextmanager from datetime import datetime, timezone -from urllib.parse import quote_plus +from urllib.parse import quote_plus, urlencode, urlunparse, urlparse from uuid import uuid4 from bs4 import BeautifulSoup @@ -192,27 +192,30 @@ def get_lms_link_for_item(location, preview=False): """ assert isinstance(location, UsageKey) + # checks LMS_ROOT_URL value in site configuration for the given course_org_filter(org) + # if not found returns settings.LMS_ROOT_URL lms_base = SiteConfiguration.get_value_for_org( location.org, - "LMS_BASE", - settings.LMS_BASE + "LMS_ROOT_URL", + settings.LMS_ROOT_URL ) - params = '' + query_string = '' if lms_base is None: return None if preview: - # checks LMS_BASE value in site configuration for the given course_org_filter(org) - # if not found returns settings.LMS_BASE - params = '?preview=1' + params = {'preview': '1'} + query_string = urlencode(params) - return "//{lms_base}/courses/{course_key}/jump_to/{location}{params}".format( - lms_base=lms_base, + url_parts = list(urlparse(lms_base)) + url_parts[2]= '/courses/{course_key}/jump_to/{location}'.format( course_key=str(location.course_key), location=str(location), - params=params, ) + url_parts[4] = query_string + + return urlunparse(url_parts) def get_lms_link_for_certificate_web_view(course_key, mode): diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index d7b64a5f89dd..10602fba1e7b 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -12,7 +12,7 @@ from django.http.request import QueryDict from django.urls import reverse from opaque_keys.edx.keys import CourseKey, UsageKey -from six.moves.urllib.parse import urlencode, urlparse +from six.moves.urllib.parse import urlencode, urlparse, urlunparse from lms.djangoapps.courseware.toggles import courseware_mfe_is_active from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order @@ -161,8 +161,9 @@ def make_learning_mfe_courseware_url( strings. They're only ever used to concatenate a URL string. `params` is an optional QueryDict object (e.g. request.GET) """ - mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/course/{course_key}' + mfe_link = f'/course/{course_key}' get_params = params.copy() if params else None + query_string = '' if preview: if len(get_params.keys()) > 1: @@ -171,7 +172,7 @@ def make_learning_mfe_courseware_url( get_params = None if (unit_key or sequence_key): - mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/preview/course/{course_key}' + mfe_link = f'/preview/course/{course_key}' if sequence_key: mfe_link += f'/{sequence_key}' @@ -180,11 +181,16 @@ def make_learning_mfe_courseware_url( mfe_link += f'/{unit_key}' if get_params: - mfe_link += f'?{get_params.urlencode()}' + query_string = get_params.urlencode() - print(mfe_link) + # url_parts = ['', , mfe_link, '', query_string, ''] + # url = urlunparse(url_parts) + # print(url) - return mfe_link + url_parts = list(urlparse(settings.LEARNING_MICROFRONTEND_URL)) + url_parts[2]= mfe_link + url_parts[4] = query_string + return urlunparse(url_parts) def get_learning_mfe_home_url( From 3a9085d9932ae006cbc26a95a76a30a4a01b1656 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Wed, 23 Oct 2024 16:39:30 -0400 Subject: [PATCH 06/16] fix: code styling errors --- cms/djangoapps/contentstore/utils.py | 2 +- lms/djangoapps/courseware/tests/test_views.py | 15 +++++++++++---- openedx/features/course_experience/url_helpers.py | 9 +++------ 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 6a9d11e9325a..7b1a8db5282e 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -209,7 +209,7 @@ def get_lms_link_for_item(location, preview=False): query_string = urlencode(params) url_parts = list(urlparse(lms_base)) - url_parts[2]= '/courses/{course_key}/jump_to/{location}'.format( + url_parts[2] = '/courses/{course_key}/jump_to/{location}'.format( course_key=str(location.course_key), location=str(location), ) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 19b19d633fb5..2ff1b8c256ba 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -143,12 +143,15 @@ def test_jump_to_invalid_location(self, preview_mode, store_type): course = CourseFactory.create() location = course.id.make_usage_key(None, 'NoSuchPlace') - expected_redirect_url = f'http://learning-mfe/course/{course.id}' - jumpto_url = f'/courses/{course.id}/jump_to/{location}?preview=1' if preview_mode else f'/courses/{course.id}/jump_to/{location}' + expected_redirect_url = f'http://learning-mfe/course/{course.id}' + jumpto_url = ( + f'/courses/{course.id}/jump_to/{location}?preview=1' + ) if preview_mode else ( + f'/courses/{course.id}/jump_to/{location}' + ) # 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) assert response.status_code == 302 @@ -267,7 +270,11 @@ def test_jump_to_legacy_from_nested_block(self): def test_jump_to_id_invalid_location(self, preview_mode, store_type): with self.store.default_store(store_type): course = CourseFactory.create() - jumpto_url = f'/courses/{course.id}/jump_to/NoSuchPlace?preview=1' if preview_mode else f'/courses/{course.id}/jump_to/NoSuchPlace' + jumpto_url = ( + f'/courses/{course.id}/jump_to/NoSuchPlace?preview=1' + ) if preview_mode else ( + f'/courses/{course.id}/jump_to/NoSuchPlace' + ) with set_preview_mode(False): response = self.client.get(jumpto_url) assert response.status_code == 404 diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index 10602fba1e7b..fde34832aae3 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -170,7 +170,7 @@ def make_learning_mfe_courseware_url( del get_params['preview'] else: get_params = None - + if (unit_key or sequence_key): mfe_link = f'/preview/course/{course_key}' @@ -183,13 +183,10 @@ def make_learning_mfe_courseware_url( if get_params: query_string = get_params.urlencode() - # url_parts = ['', , mfe_link, '', query_string, ''] - # url = urlunparse(url_parts) - # print(url) - url_parts = list(urlparse(settings.LEARNING_MICROFRONTEND_URL)) - url_parts[2]= mfe_link + url_parts[2] = mfe_link url_parts[4] = query_string + return urlunparse(url_parts) From c51a88c1e4a91f5547a26d8a747913cf823c044f Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Thu, 24 Oct 2024 10:07:38 -0400 Subject: [PATCH 07/16] fix: failing cms tests --- .../contentstore/rest_api/v1/views/tests/test_home.py | 4 ++-- openedx/features/course_experience/url_helpers.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py index 69eee524373c..73e3fe5ec3ad 100644 --- a/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v1/views/tests/test_home.py @@ -117,7 +117,7 @@ def test_home_page_response(self): "courses": [{ "course_key": course_id, "display_name": self.course.display_name, - "lms_link": f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}', + "lms_link": f'{settings.LMS_ROOT_URL}/courses/{course_id}/jump_to/{self.course.location}', "number": self.course.number, "org": self.course.org, "rerun_link": f'/course_rerun/{course_id}', @@ -144,7 +144,7 @@ def test_home_page_response_with_api_v2(self): OrderedDict([ ("course_key", course_id), ("display_name", self.course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}'), + ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{course_id}/jump_to/{self.course.location}'), ("number", self.course.number), ("org", self.course.org), ("rerun_link", f'/course_rerun/{course_id}'), diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index fde34832aae3..9df327029a6e 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -167,7 +167,7 @@ def make_learning_mfe_courseware_url( if preview: if len(get_params.keys()) > 1: - del get_params['preview'] + get_params.pop('preview') else: get_params = None From 4003655bf104262d69991cb051131c17cac09ace Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Thu, 24 Oct 2024 10:54:15 -0400 Subject: [PATCH 08/16] fix: failing v2 home tests --- .../contentstore/rest_api/v2/views/tests/test_home.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py index c0ffa50903cd..e6ef1a2b6dd7 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py @@ -62,7 +62,7 @@ def test_home_page_response(self): OrderedDict([ ("course_key", course_id), ("display_name", self.course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{course_id}/jump_to/{self.course.location}'), + ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{course_id}/jump_to/{self.course.location}'), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'), ("number", self.course.number), ("org", self.course.org), @@ -76,7 +76,7 @@ def test_home_page_response(self): ("display_name", self.archived_course.display_name), ( "lms_link", - f'//{settings.LMS_BASE}/courses/{archived_course_id}/jump_to/{self.archived_course.location}' + f'{settings.LMS_ROOT_URL}/courses/{archived_course_id}/jump_to/{self.archived_course.location}' ), ( "cms_link", @@ -139,7 +139,7 @@ def test_active_only_query_if_passed(self): self.assertEqual(response.data["results"]["courses"], [OrderedDict([ ("course_key", str(self.course.id)), ("display_name", self.course.display_name), - ("lms_link", f'//{settings.LMS_BASE}/courses/{str(self.course.id)}/jump_to/{self.course.location}'), + ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{str(self.course.id)}/jump_to/{self.course.location}'), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.course.id)}'), ("number", self.course.number), ("org", self.course.org), @@ -164,7 +164,7 @@ def test_archived_only_query_if_passed(self): ("display_name", self.archived_course.display_name), ( "lms_link", - f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', + f'{settings.LMS_ROOT_URL}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), @@ -190,7 +190,7 @@ def test_search_query_if_passed(self): ("display_name", self.archived_course.display_name), ( "lms_link", - f'//{settings.LMS_BASE}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', + f'{settings.LMS_ROOT_URL}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), From 7df4ba3c69e91ec817d248a6fc8e3659728304a8 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Thu, 24 Oct 2024 13:42:51 -0400 Subject: [PATCH 09/16] fix: line too long errors --- .../rest_api/v2/views/tests/test_home.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py index e6ef1a2b6dd7..6905de254f3e 100644 --- a/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py +++ b/cms/djangoapps/contentstore/rest_api/v2/views/tests/test_home.py @@ -164,7 +164,11 @@ def test_archived_only_query_if_passed(self): ("display_name", self.archived_course.display_name), ( "lms_link", - f'{settings.LMS_ROOT_URL}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', + '{url_root}/courses/{course_id}/jump_to/{location}'.format( + url_root=settings.LMS_ROOT_URL, + course_id=str(self.archived_course.id), + location=self.archived_course.location + ), ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), @@ -190,7 +194,11 @@ def test_search_query_if_passed(self): ("display_name", self.archived_course.display_name), ( "lms_link", - f'{settings.LMS_ROOT_URL}/courses/{str(self.archived_course.id)}/jump_to/{self.archived_course.location}', + '{url_root}/courses/{course_id}/jump_to/{location}'.format( + url_root=settings.LMS_ROOT_URL, + course_id=str(self.archived_course.id), + location=self.archived_course.location + ), ), ("cms_link", f'//{settings.CMS_BASE}{reverse_course_url("course_handler", self.archived_course.id)}'), ("number", self.archived_course.number), From 17764bd0e0fc6151b8b6fc6812a0b472070aa61c Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Fri, 25 Oct 2024 11:02:12 -0400 Subject: [PATCH 10/16] fix: url redirect for never published units --- lms/djangoapps/courseware/views/views.py | 2 +- .../features/course_experience/url_helpers.py | 14 ++- xmodule/modulestore/search.py | 85 ++++++++++--------- xmodule/video_block/video_block.py | 2 +- 4 files changed, 58 insertions(+), 45 deletions(-) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 46525628f6e5..e7f37b86bdae 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -1566,7 +1566,7 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_sta f"Rendering of the xblock view '{nh3.clean(requested_view)}' is not supported." ) - staff_access = has_access(request.user, 'staff', course_key) + staff_access = bool(has_access(request.user, 'staff', course_key)) is_preview = request.GET.get('preview') == '1' store = modulestore() diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index 9df327029a6e..0c7a704ee8ad 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -14,7 +14,9 @@ from opaque_keys.edx.keys import CourseKey, UsageKey from six.moves.urllib.parse import urlencode, urlparse, urlunparse +from lms.djangoapps.courseware.access import has_access 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 @@ -99,7 +101,15 @@ def _get_new_courseware_url( * NoPathToItem if we cannot build a path to the `usage_key`. """ course_key = usage_key.course_key.replace(version_guid=None, branch=None) - path = path_to_location(modulestore(), usage_key, request, full_path=True) + preview = request.GET.get('preview') if request and request.GET else False + staff_access = bool(has_access(request.user, 'staff', course_key)) + + branch_type = ( + ModuleStoreEnum.Branch.draft_preferred + ) if preview and staff_access else ModuleStoreEnum.Branch.published_only + + path = path_to_location(modulestore(), usage_key, request, full_path=True, branch_type=branch_type) + if len(path) <= 1: # Course-run-level block: # We have no Sequence or Unit to return. @@ -120,7 +130,7 @@ def _get_new_courseware_url( course_key=course_key, sequence_key=sequence_key, unit_key=unit_key, - preview=request.GET.get('preview') if request and request.GET else False, + preview=preview, params=request.GET if request and request.GET else None, ) diff --git a/xmodule/modulestore/search.py b/xmodule/modulestore/search.py index 60317039821a..628f79124e35 100644 --- a/xmodule/modulestore/search.py +++ b/xmodule/modulestore/search.py @@ -8,11 +8,12 @@ from lms.djangoapps.courseware.masquerade import MASQUERADE_SETTINGS_KEY from common.djangoapps.student.roles import GlobalStaff # lint-amnesty, pylint: disable=unused-import from .exceptions import ItemNotFoundError, NoPathToItem +from . import ModuleStoreEnum LOGGER = getLogger(__name__) -def path_to_location(modulestore, usage_key, request=None, full_path=False): +def path_to_location(modulestore, usage_key, request=None, full_path=False, branch_type=None): ''' Try to find a course_id/chapter/section[/position] path to location in modulestore. The courseware insists that the first level in the course is @@ -82,46 +83,48 @@ def find_path_to_course(): queue.append((parent, newpath)) with modulestore.bulk_operations(usage_key.course_key): - if not modulestore.has_item(usage_key): - raise ItemNotFoundError(usage_key) - - path = find_path_to_course() - if path is None: - raise NoPathToItem(usage_key) - - if full_path: - return path - - n = len(path) - course_id = path[0].course_key - # pull out the location names - chapter = path[1].block_id if n > 1 else None - section = path[2].block_id if n > 2 else None - vertical = path[3].block_id if n > 3 else None - # Figure out the position - position = None - - # This block of code will find the position of a block within a nested tree - # of blocks. If a problem is on tab 2 of a sequence that's on tab 3 of a - # sequence, the resulting position is 3_2. However, no positional blocks - # (e.g. sequential) currently deal with this form of representing nested - # positions. This needs to happen before jumping to a block nested in more - # than one positional block will work. - - if n > 3: - position_list = [] - for path_index in range(2, n - 1): - category = path[path_index].block_type - if category == 'sequential': - section_desc = modulestore.get_item(path[path_index]) - # this calls get_children rather than just children b/c old mongo includes private children - # in children but not in get_children - child_locs = get_child_locations(section_desc, request, course_id) - # positions are 1-indexed, and should be strings to be consistent with - # url parsing. - if path[path_index + 1] in child_locs: - position_list.append(str(child_locs.index(path[path_index + 1]) + 1)) - position = "_".join(position_list) + branch_setting = branch_type if branch_type else ModuleStoreEnum.Branch.published_only + with modulestore.branch_setting(branch_setting, usage_key.course_key): + if not modulestore.has_item(usage_key): + raise ItemNotFoundError(usage_key) + + path = find_path_to_course() + if path is None: + raise NoPathToItem(usage_key) + + if full_path: + return path + + n = len(path) + course_id = path[0].course_key + # pull out the location names + chapter = path[1].block_id if n > 1 else None + section = path[2].block_id if n > 2 else None + vertical = path[3].block_id if n > 3 else None + # Figure out the position + position = None + + # This block of code will find the position of a block within a nested tree + # of blocks. If a problem is on tab 2 of a sequence that's on tab 3 of a + # sequence, the resulting position is 3_2. However, no positional blocks + # (e.g. sequential) currently deal with this form of representing nested + # positions. This needs to happen before jumping to a block nested in more + # than one positional block will work. + + if n > 3: + position_list = [] + for path_index in range(2, n - 1): + category = path[path_index].block_type + if category == 'sequential': + section_desc = modulestore.get_item(path[path_index]) + # this calls get_children rather than just children b/c old mongo includes private children + # in children but not in get_children + child_locs = get_child_locations(section_desc, request, course_id) + # positions are 1-indexed, and should be strings to be consistent with + # url parsing. + if path[path_index + 1] in child_locs: + position_list.append(str(child_locs.index(path[path_index + 1]) + 1)) + position = "_".join(position_list) return (course_id, chapter, section, vertical, position, path[-1]) diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index 782645c373b0..73ad13c0bbed 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -179,7 +179,7 @@ def get_transcripts_for_student(self, transcripts, dest_lang=None): track_url = self.track elif sub or other_lang: track_url = self.runtime.handler_url(self, 'transcript', 'download').rstrip('/?') - + print('GETTING THE TRANSCRIPT DEFAULT LANGUAGW',dest_lang) transcript_language = self.get_default_transcript_language(transcripts, dest_lang) native_languages = {lang: label for lang, label in settings.LANGUAGES if len(lang) == 2} languages = { From cf7057210be8ba73f5fd3ae81cbbfe6ec7b11ba0 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Fri, 25 Oct 2024 14:51:56 -0400 Subject: [PATCH 11/16] fix: forbidden lms import for cms code --- lms/djangoapps/courseware/views/views.py | 3 +++ openedx/features/course_experience/url_helpers.py | 10 +++++----- xmodule/modulestore/search.py | 3 +-- xmodule/modulestore/tests/test_mixed_modulestore.py | 2 +- xmodule/video_block/video_block.py | 1 - 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index e7f37b86bdae..4e89dc296590 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -440,10 +440,12 @@ def jump_to(request, course_id, location): except InvalidKeyError as exc: raise Http404("Invalid course_key or usage_key") from exc + staff_access = has_access(request.user, 'staff', course_key) try: redirect_url = get_courseware_url( usage_key=usage_key, request=request, + is_staff=staff_access, ) except (ItemNotFoundError, NoPathToItem): # We used to 404 here, but that's ultimately a bad experience. There are real world use cases where a user @@ -453,6 +455,7 @@ def jump_to(request, course_id, location): redirect_url = get_courseware_url( usage_key=course_location_from_key(course_key), request=request, + is_staff=staff_access, ) return redirect(redirect_url) diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index 0c7a704ee8ad..cdcb0203fc06 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, urlunparse -from lms.djangoapps.courseware.access import has_access 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 @@ -26,6 +25,7 @@ def get_courseware_url( usage_key: UsageKey, request: Optional[HttpRequest] = None, + is_staff: bool = False, ) -> str: """ Return the URL to the canonical learning experience for a given block. @@ -46,12 +46,13 @@ def get_courseware_url( 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) + return get_url_fn(usage_key=usage_key, request=request, is_staff=is_staff) def _get_legacy_courseware_url( usage_key: UsageKey, request: Optional[HttpRequest] = None, + is_staff: bool = None ) -> str: """ Return the URL to Legacy (LMS-rendered) courseware content. @@ -92,6 +93,7 @@ def _get_legacy_courseware_url( def _get_new_courseware_url( usage_key: UsageKey, request: Optional[HttpRequest] = None, + is_staff: bool = None, ) -> str: """ Return the URL to the "new" (Learning Micro-Frontend) experience for a given block. @@ -102,11 +104,9 @@ def _get_new_courseware_url( """ course_key = usage_key.course_key.replace(version_guid=None, branch=None) preview = request.GET.get('preview') if request and request.GET else False - staff_access = bool(has_access(request.user, 'staff', course_key)) - branch_type = ( ModuleStoreEnum.Branch.draft_preferred - ) if preview and staff_access else ModuleStoreEnum.Branch.published_only + ) if preview and is_staff else ModuleStoreEnum.Branch.published_only path = path_to_location(modulestore(), usage_key, request, full_path=True, branch_type=branch_type) diff --git a/xmodule/modulestore/search.py b/xmodule/modulestore/search.py index 628f79124e35..418c7ed9384a 100644 --- a/xmodule/modulestore/search.py +++ b/xmodule/modulestore/search.py @@ -83,8 +83,7 @@ def find_path_to_course(): queue.append((parent, newpath)) with modulestore.bulk_operations(usage_key.course_key): - branch_setting = branch_type if branch_type else ModuleStoreEnum.Branch.published_only - with modulestore.branch_setting(branch_setting, usage_key.course_key): + with modulestore.branch_setting(branch_type, usage_key.course_key): if not modulestore.has_item(usage_key): raise ItemNotFoundError(usage_key) diff --git a/xmodule/modulestore/tests/test_mixed_modulestore.py b/xmodule/modulestore/tests/test_mixed_modulestore.py index 8adbfcb911a4..0928ab253b9c 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1752,7 +1752,7 @@ def test_path_to_location(self, default_ms, num_mysql, num_finds, num_sends): for location, expected in should_work: # each iteration has different find count, pop this iter's find count with check_mongo_calls(num_finds.pop(0), num_sends), self.assertNumQueries(num_mysql.pop(0)): - path = path_to_location(self.store, location) + path = path_to_location(self.store, location, branch_type=ModuleStoreEnum.Branch.published_only) assert path == expected not_found = ( diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index 73ad13c0bbed..0dd1fd59354a 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -179,7 +179,6 @@ def get_transcripts_for_student(self, transcripts, dest_lang=None): track_url = self.track elif sub or other_lang: track_url = self.runtime.handler_url(self, 'transcript', 'download').rstrip('/?') - print('GETTING THE TRANSCRIPT DEFAULT LANGUAGW',dest_lang) transcript_language = self.get_default_transcript_language(transcripts, dest_lang) native_languages = {lang: label for lang, label in settings.LANGUAGES if len(lang) == 2} languages = { From df5d985fc5cef84693662122443b52f1e7123ca9 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Mon, 28 Oct 2024 09:18:57 -0400 Subject: [PATCH 12/16] fix: remove used import --- xmodule/modulestore/search.py | 1 - 1 file changed, 1 deletion(-) diff --git a/xmodule/modulestore/search.py b/xmodule/modulestore/search.py index 418c7ed9384a..fc065078968b 100644 --- a/xmodule/modulestore/search.py +++ b/xmodule/modulestore/search.py @@ -8,7 +8,6 @@ from lms.djangoapps.courseware.masquerade import MASQUERADE_SETTINGS_KEY from common.djangoapps.student.roles import GlobalStaff # lint-amnesty, pylint: disable=unused-import from .exceptions import ItemNotFoundError, NoPathToItem -from . import ModuleStoreEnum LOGGER = getLogger(__name__) From 7f681a545e72aa269b9ffe73a46a50e03f1df2e3 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Wed, 30 Oct 2024 09:29:34 -0400 Subject: [PATCH 13/16] fix: remove 404 error when not a preview or staff --- lms/djangoapps/courseware/views/views.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 4e89dc296590..1c2559bed928 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -1570,16 +1570,13 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_sta ) staff_access = bool(has_access(request.user, 'staff', course_key)) - is_preview = request.GET.get('preview') == '1' + is_preview = request.GET.get('preview', '0') == '1' store = modulestore() - branchType = ModuleStoreEnum.Branch.draft_preferred if is_preview else ModuleStoreEnum.Branch.published_only - - if is_preview and not staff_access: - return HttpResponseBadRequest("You do not have access to preview this xblock") + branch_type = ModuleStoreEnum.Branch.draft_preferred if is_preview and staff_access else ModuleStoreEnum.Branch.published_only with store.bulk_operations(course_key): - with store.branch_setting(branchType, course_key): + with store.branch_setting(branch_type, course_key): # verify the user has access to the course, including enrollment check try: course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=check_if_enrolled) From d45a3b19a1f1d2a74f1b54b71039080cfd237b5b Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Wed, 30 Oct 2024 09:33:52 -0400 Subject: [PATCH 14/16] feat: update sequence metadata to allow draft branch --- .../core/djangoapps/courseware_api/views.py | 37 +++++++++++-------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index 7f56133b3459..c7ab594cc6da 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -19,6 +19,7 @@ from rest_framework.response import Response from rest_framework.views import APIView +from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError, NoPathToItem from xmodule.modulestore.search import path_to_location @@ -594,30 +595,36 @@ def get(self, request, usage_key_string, *args, **kwargs): # lint-amnesty, pyli usage_key = UsageKey.from_string(usage_key_string) except InvalidKeyError as exc: raise NotFound(f"Invalid usage key: '{usage_key_string}'.") from exc + + staff_access = has_access(request.user, 'staff', usage_key.course_key) + is_preview = request.GET.get('preview', '0') == '1' _, request.user = setup_masquerade( request, usage_key.course_key, - staff_access=has_access(request.user, 'staff', usage_key.course_key), + staff_access=staff_access, reset_masquerade_data=True, ) - sequence, _ = get_block_by_usage_id( - self.request, - str(usage_key.course_key), - str(usage_key), - disable_staff_debug_info=True, - will_recheck_access=True) + branch_type = ModuleStoreEnum.Branch.draft_preferred if is_preview and has_access else ModuleStoreEnum.Branch.published_only + + with modulestore().branch_setting(branch_type, usage_key.course_key): + sequence, _ = get_block_by_usage_id( + self.request, + str(usage_key.course_key), + str(usage_key), + disable_staff_debug_info=True, + will_recheck_access=True) - if not hasattr(sequence, 'get_metadata'): - # Looks like we were asked for metadata on something that is not a sequence (or section). - return Response(status=status.HTTP_422_UNPROCESSABLE_ENTITY) + if not hasattr(sequence, 'get_metadata'): + # Looks like we were asked for metadata on something that is not a sequence (or section). + return Response(status=status.HTTP_422_UNPROCESSABLE_ENTITY) - view = STUDENT_VIEW - if request.user.is_anonymous: - view = PUBLIC_VIEW + view = STUDENT_VIEW + if request.user.is_anonymous: + view = PUBLIC_VIEW - context = {'specific_masquerade': is_masquerading_as_specific_student(request.user, usage_key.course_key)} - return Response(sequence.get_metadata(view=view, context=context)) + context = {'specific_masquerade': is_masquerading_as_specific_student(request.user, usage_key.course_key)} + return Response(sequence.get_metadata(view=view, context=context)) class Resume(DeveloperErrorViewMixin, APIView): From 680125772712c0fca77de5f04e57693eabe34a66 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Wed, 30 Oct 2024 10:48:20 -0400 Subject: [PATCH 15/16] fix: line too long errors --- lms/djangoapps/courseware/views/views.py | 6 +++++- openedx/core/djangoapps/courseware_api/views.py | 8 ++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 1c2559bed928..280170687eed 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -1573,7 +1573,11 @@ def render_xblock(request, usage_key_string, check_if_enrolled=True, disable_sta is_preview = request.GET.get('preview', '0') == '1' store = modulestore() - branch_type = ModuleStoreEnum.Branch.draft_preferred if is_preview and staff_access else ModuleStoreEnum.Branch.published_only + branch_type = ( + ModuleStoreEnum.Branch.draft_preferred + ) if is_preview and staff_access else ( + ModuleStoreEnum.Branch.published_only + ) with store.bulk_operations(course_key): with store.branch_setting(branch_type, course_key): diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index c7ab594cc6da..07e219f83d81 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -595,7 +595,7 @@ def get(self, request, usage_key_string, *args, **kwargs): # lint-amnesty, pyli usage_key = UsageKey.from_string(usage_key_string) except InvalidKeyError as exc: raise NotFound(f"Invalid usage key: '{usage_key_string}'.") from exc - + staff_access = has_access(request.user, 'staff', usage_key.course_key) is_preview = request.GET.get('preview', '0') == '1' _, request.user = setup_masquerade( @@ -605,7 +605,11 @@ def get(self, request, usage_key_string, *args, **kwargs): # lint-amnesty, pyli reset_masquerade_data=True, ) - branch_type = ModuleStoreEnum.Branch.draft_preferred if is_preview and has_access else ModuleStoreEnum.Branch.published_only + branch_type = ( + ModuleStoreEnum.Branch.draft_preferred + ) if is_preview and staff_access else ( + ModuleStoreEnum.Branch.published_only + ) with modulestore().branch_setting(branch_type, usage_key.course_key): sequence, _ = get_block_by_usage_id( From 4007cab5f9c0e2e0eaeccb40e0fffda7c1bbff79 Mon Sep 17 00:00:00 2001 From: KristinAoki Date: Thu, 31 Oct 2024 16:59:39 -0400 Subject: [PATCH 16/16] fix: revert unnecessary line deletion --- xmodule/video_block/video_block.py | 1 + 1 file changed, 1 insertion(+) diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index 0dd1fd59354a..782645c373b0 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -179,6 +179,7 @@ def get_transcripts_for_student(self, transcripts, dest_lang=None): track_url = self.track elif sub or other_lang: track_url = self.runtime.handler_url(self, 'transcript', 'download').rstrip('/?') + transcript_language = self.get_default_transcript_language(transcripts, dest_lang) native_languages = {lang: label for lang, label in settings.LANGUAGES if len(lang) == 2} languages = {