From e13d66d1e4351a19df6f7def6eefc0ebfa1da685 Mon Sep 17 00:00:00 2001 From: Kristin Aoki <42981026+KristinAoki@users.noreply.github.com> Date: Fri, 1 Nov 2024 11:03:06 -0400 Subject: [PATCH] feat: support unit preview in learning MFE (#35747) * feat: update preview url to direct to mfe * fix: use url builder instead of string formatter * fix: url redirect for never published units * fix: remove 404 error when not a preview or staff * feat: update sequence metadata to allow draft branch --- .../rest_api/v1/views/tests/test_home.py | 4 +- .../rest_api/v2/views/tests/test_home.py | 18 +- cms/djangoapps/contentstore/utils.py | 27 +- lms/djangoapps/courseware/tests/test_views.py | 86 ++---- lms/djangoapps/courseware/views/index.py | 3 + lms/djangoapps/courseware/views/views.py | 261 +++++++++--------- .../core/djangoapps/courseware_api/views.py | 41 ++- .../features/course_experience/url_helpers.py | 41 ++- xmodule/modulestore/search.py | 83 +++--- .../tests/test_mixed_modulestore.py | 2 +- 10 files changed, 301 insertions(+), 265 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/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..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 @@ -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,11 @@ 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}', + '{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_BASE}/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), diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index df1d1e27b405..c0f656ec7059 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -9,7 +9,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 @@ -193,31 +193,30 @@ 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 + # 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 ) + query_string = '' 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') - ) + params = {'preview': '1'} + query_string = urlencode(params) - return "//{lms_base}/courses/{course_key}/jump_to/{location}".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), ) + url_parts[4] = query_string + + return urlunparse(url_parts) def get_lms_link_for_certificate_web_view(course_key, mode): diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 7a5e36e54904..d38110b3b045 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,34 @@ 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)}) + + 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'http://learning-mfe/course/{course.id}' + 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 +184,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 +195,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 +270,12 @@ 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 @@ -3359,7 +3333,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 c123c7f71f4c..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 diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index b182202eddaf..4fd124e1a47e 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -51,6 +51,7 @@ COURSE_VISIBILITY_PUBLIC_OUTLINE, CATALOG_VISIBILITY_CATALOG_AND_ABOUT, ) +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 @@ -439,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 @@ -452,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) @@ -1565,142 +1569,151 @@ 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', '0') == '1' - with modulestore().bulk_operations(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, - ) + store = modulestore() + branch_type = ( + ModuleStoreEnum.Branch.draft_preferred + ) if is_preview and staff_access else ( + ModuleStoreEnum.Branch.published_only + ) - # 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(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) + 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/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index 7f56133b3459..07e219f83d81 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,40 @@ 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 staff_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): diff --git a/openedx/features/course_experience/url_helpers.py b/openedx/features/course_experience/url_helpers.py index f62e879f0994..cdcb0203fc06 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -12,9 +12,10 @@ 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 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 @@ -24,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. @@ -44,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. @@ -90,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. @@ -99,7 +103,13 @@ 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 + branch_type = ( + ModuleStoreEnum.Branch.draft_preferred + ) 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) + if len(path) <= 1: # Course-run-level block: # We have no Sequence or Unit to return. @@ -120,6 +130,7 @@ def _get_new_courseware_url( course_key=course_key, sequence_key=sequence_key, unit_key=unit_key, + preview=preview, params=request.GET if request and request.GET else None, ) @@ -129,6 +140,7 @@ def make_learning_mfe_courseware_url( 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. @@ -159,7 +171,18 @@ 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: + get_params.pop('preview') + else: + get_params = None + + if (unit_key or sequence_key): + mfe_link = f'/preview/course/{course_key}' if sequence_key: mfe_link += f'/{sequence_key}' @@ -167,10 +190,14 @@ def make_learning_mfe_courseware_url( if unit_key: mfe_link += f'/{unit_key}' - if params: - mfe_link += f'?{params.urlencode()}' + if get_params: + query_string = get_params.urlencode() - 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( diff --git a/xmodule/modulestore/search.py b/xmodule/modulestore/search.py index 60317039821a..fc065078968b 100644 --- a/xmodule/modulestore/search.py +++ b/xmodule/modulestore/search.py @@ -12,7 +12,7 @@ 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 +82,47 @@ 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) + with modulestore.branch_setting(branch_type, 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/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 = (