From afd1394112a60227a3392ea40455de355c192dc2 Mon Sep 17 00:00:00 2001 From: Kristin Aoki <42981026+KristinAoki@users.noreply.github.com> Date: Mon, 28 Oct 2024 13:26:29 -0400 Subject: [PATCH] Revert "feat: update preview url to direct to mfe (#35687)" (#35732) This reverts commit 2373dd02f9ab4edce560f182847c663122dacccf. --- .../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 | 252 +++++++++--------- .../features/course_experience/url_helpers.py | 41 +-- xmodule/modulestore/search.py | 73 +++-- .../tests/test_mixed_modulestore.py | 2 +- xmodule/video_block/video_block.py | 1 + 10 files changed, 242 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 73e3fe5ec3..69eee52437 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 @@ class HomePageCoursesViewTest(CourseTestCase): "courses": [{ "course_key": course_id, "display_name": self.course.display_name, - "lms_link": f'{settings.LMS_ROOT_URL}/courses/{course_id}/jump_to/{self.course.location}', + "lms_link": f'//{settings.LMS_BASE}/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 @@ class HomePageCoursesViewTest(CourseTestCase): OrderedDict([ ("course_key", course_id), ("display_name", self.course.display_name), - ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{course_id}/jump_to/{self.course.location}'), + ("lms_link", f'//{settings.LMS_BASE}/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 6905de254f..c0ffa50903 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 @@ class HomePageCoursesViewV2Test(CourseTestCase): OrderedDict([ ("course_key", course_id), ("display_name", self.course.display_name), - ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{course_id}/jump_to/{self.course.location}'), + ("lms_link", f'//{settings.LMS_BASE}/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 @@ class HomePageCoursesViewV2Test(CourseTestCase): ("display_name", self.archived_course.display_name), ( "lms_link", - f'{settings.LMS_ROOT_URL}/courses/{archived_course_id}/jump_to/{self.archived_course.location}' + f'//{settings.LMS_BASE}/courses/{archived_course_id}/jump_to/{self.archived_course.location}' ), ( "cms_link", @@ -139,7 +139,7 @@ class HomePageCoursesViewV2Test(CourseTestCase): self.assertEqual(response.data["results"]["courses"], [OrderedDict([ ("course_key", str(self.course.id)), ("display_name", self.course.display_name), - ("lms_link", f'{settings.LMS_ROOT_URL}/courses/{str(self.course.id)}/jump_to/{self.course.location}'), + ("lms_link", f'//{settings.LMS_BASE}/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,11 +164,7 @@ class HomePageCoursesViewV2Test(CourseTestCase): ("display_name", self.archived_course.display_name), ( "lms_link", - '{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 - ), + f'//{settings.LMS_BASE}/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), @@ -194,11 +190,7 @@ class HomePageCoursesViewV2Test(CourseTestCase): ("display_name", self.archived_course.display_name), ( "lms_link", - '{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 - ), + f'//{settings.LMS_BASE}/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), diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index c0f656ec70..df1d1e27b4 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -9,7 +9,7 @@ import re from collections import defaultdict from contextlib import contextmanager from datetime import datetime, timezone -from urllib.parse import quote_plus, urlencode, urlunparse, urlparse +from urllib.parse import quote_plus from uuid import uuid4 from bs4 import BeautifulSoup @@ -193,30 +193,31 @@ 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 + # 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_ROOT_URL", - settings.LMS_ROOT_URL + "LMS_BASE", + settings.LMS_BASE ) - query_string = '' if lms_base is None: return None if preview: - params = {'preview': '1'} - query_string = urlencode(params) + # 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') + ) - url_parts = list(urlparse(lms_base)) - url_parts[2] = '/courses/{course_key}/jump_to/{location}'.format( + return "//{lms_base}/courses/{course_key}/jump_to/{location}".format( + lms_base=lms_base, 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 2ff1b8c256..cde0e8a34e 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -132,6 +132,38 @@ 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), @@ -142,34 +174,32 @@ class TestJumpTo(ModuleStoreTestCase): with self.store.default_store(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' + expected_redirect_url = ( + f'/courses/{course.id}/courseware?' + urlencode({'activate_block_id': str(course.location)}) ) if preview_mode else ( - f'/courses/{course.id}/jump_to/{location}' + f'http://learning-mfe/course/{course.id}' ) - # 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): + jumpto_url = f'/courses/{course.id}/jump_to/{location}' + with set_preview_mode(preview_mode): response = self.client.get(jumpto_url) assert response.status_code == 302 assert response.url == expected_redirect_url - @set_preview_mode(False) - def test_jump_to_preview_from_sequence(self): + @set_preview_mode(True) + def test_jump_to_legacy_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) - jumpto_url = f'/courses/{course.id}/jump_to/{sequence.location}?preview=1' + activate_block_id = urlencode({'activate_block_id': str(sequence.location)}) expected_redirect_url = ( - f'http://learning-mfe/preview/course/{course.id}/{sequence.location}' + f'/courses/{course.id}/courseware/{chapter.url_name}/{sequence.url_name}/?{activate_block_id}' ) + jumpto_url = f'/courses/{course.id}/jump_to/{sequence.location}' response = self.client.get(jumpto_url) - assert response.status_code == 302 - assert response.url == expected_redirect_url + self.assertRedirects(response, expected_redirect_url, status_code=302, target_status_code=302) @set_preview_mode(False) def test_jump_to_mfe_from_sequence(self): @@ -184,8 +214,8 @@ class TestJumpTo(ModuleStoreTestCase): assert response.status_code == 302 assert response.url == expected_redirect_url - @set_preview_mode(False) - def test_jump_to_preview_from_block(self): + @set_preview_mode(True) + def test_jump_to_legacy_from_block(self): with self.store.default_store(ModuleStoreEnum.Type.split): course = CourseFactory.create() chapter = BlockFactory.create(category='chapter', parent_location=course.location) @@ -195,21 +225,21 @@ class TestJumpTo(ModuleStoreTestCase): block1 = BlockFactory.create(category='html', parent_location=vertical1.location) block2 = BlockFactory.create(category='html', parent_location=vertical2.location) - jumpto_url = f'/courses/{course.id}/jump_to/{block1.location}?preview=1' + activate_block_id = urlencode({'activate_block_id': str(block1.location)}) expected_redirect_url = ( - f'http://learning-mfe/preview/course/{course.id}/{sequence.location}/{vertical1.location}' + f'/courses/{course.id}/courseware/{chapter.url_name}/{sequence.url_name}/1?{activate_block_id}' ) + jumpto_url = f'/courses/{course.id}/jump_to/{block1.location}' response = self.client.get(jumpto_url) - assert response.status_code == 302 - assert response.url == expected_redirect_url + self.assertRedirects(response, expected_redirect_url, status_code=302, target_status_code=302) - jumpto_url = f'/courses/{course.id}/jump_to/{block2.location}?preview=1' + activate_block_id = urlencode({'activate_block_id': str(block2.location)}) expected_redirect_url = ( - f'http://learning-mfe/preview/course/{course.id}/{sequence.location}/{vertical2.location}' + f'/courses/{course.id}/courseware/{chapter.url_name}/{sequence.url_name}/2?{activate_block_id}' ) + jumpto_url = f'/courses/{course.id}/jump_to/{block2.location}' response = self.client.get(jumpto_url) - assert response.status_code == 302 - assert response.url == expected_redirect_url + self.assertRedirects(response, expected_redirect_url, status_code=302, target_status_code=302) @set_preview_mode(False) def test_jump_to_mfe_from_block(self): @@ -270,12 +300,8 @@ class TestJumpTo(ModuleStoreTestCase): 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' - ) - with set_preview_mode(False): + jumpto_url = f'/courses/{course.id}/jump_to/NoSuchPlace' + with set_preview_mode(preview_mode): response = self.client.get(jumpto_url) assert response.status_code == 404 @@ -3333,7 +3359,7 @@ class PreviewTests(BaseViewsTestCase): def test_preview_no_redirect(self): __, __, preview_url = self._get_urls() with set_preview_mode(True): - # Previews server from PREVIEW_LMS_BASE will not redirect to the mfe + # Previews 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 7a9242f595..c123c7f71f 100644 --- a/lms/djangoapps/courseware/views/index.py +++ b/lms/djangoapps/courseware/views/index.py @@ -27,7 +27,6 @@ from web_fragments.fragment import Fragment 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 @@ -189,13 +188,11 @@ class CoursewareIndex(View): 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 4e89dc2965..1c57b23d9b 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -51,7 +51,6 @@ from xmodule.course_block import ( 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 @@ -440,12 +439,10 @@ 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 @@ -455,7 +452,6 @@ 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) @@ -1569,151 +1565,143 @@ 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 = bool(has_access(request.user, 'staff', course_key)) - is_preview = request.GET.get('preview') == '1' + staff_access = has_access(request.user, 'staff', course_key) - store = modulestore() - branchType = ModuleStoreEnum.Branch.draft_preferred if is_preview else ModuleStoreEnum.Branch.published_only + 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 - if is_preview and not staff_access: - return HttpResponseBadRequest("You do not have access to preview this xblock") + # 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, + ) - 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 + # 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 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' - # 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, - ) + 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 - 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' + 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() + } - 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 + missed_deadlines, missed_gated_content = dates_banner_should_display(course_key, request.user) - 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)} - ) + # 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") - - 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) + # 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, + } + + 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, - **optimization_flags, + '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) + + context.update({ + 'fragment': fragment, + **optimization_flags, + }) + + 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 cdcb0203fc..f62e879f09 100644 --- a/openedx/features/course_experience/url_helpers.py +++ b/openedx/features/course_experience/url_helpers.py @@ -12,10 +12,9 @@ from django.http import HttpRequest 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, urlunparse +from six.moves.urllib.parse import urlencode, urlparse from lms.djangoapps.courseware.toggles import courseware_mfe_is_active -from xmodule.modulestore import ModuleStoreEnum # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.django import modulestore # lint-amnesty, pylint: disable=wrong-import-order from xmodule.modulestore.search import navigation_index, path_to_location # lint-amnesty, pylint: disable=wrong-import-order @@ -25,7 +24,6 @@ User = get_user_model() 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,13 +44,12 @@ 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, is_staff=is_staff) + return get_url_fn(usage_key=usage_key, request=request) 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. @@ -93,7 +90,6 @@ 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. @@ -103,13 +99,7 @@ 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) - 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) - + path = path_to_location(modulestore(), usage_key, request, full_path=True) if len(path) <= 1: # Course-run-level block: # We have no Sequence or Unit to return. @@ -130,7 +120,6 @@ 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, ) @@ -140,7 +129,6 @@ 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. @@ -171,18 +159,7 @@ 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'/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}' + mfe_link = f'{settings.LEARNING_MICROFRONTEND_URL}/course/{course_key}' if sequence_key: mfe_link += f'/{sequence_key}' @@ -190,14 +167,10 @@ def make_learning_mfe_courseware_url( if unit_key: mfe_link += f'/{unit_key}' - if get_params: - query_string = get_params.urlencode() + if params: + mfe_link += f'?{params.urlencode()}' - url_parts = list(urlparse(settings.LEARNING_MICROFRONTEND_URL)) - url_parts[2] = mfe_link - url_parts[4] = query_string - - return urlunparse(url_parts) + return mfe_link def get_learning_mfe_home_url( diff --git a/xmodule/modulestore/search.py b/xmodule/modulestore/search.py index fc06507896..6031703982 100644 --- a/xmodule/modulestore/search.py +++ b/xmodule/modulestore/search.py @@ -12,7 +12,7 @@ from .exceptions import ItemNotFoundError, NoPathToItem LOGGER = getLogger(__name__) -def path_to_location(modulestore, usage_key, request=None, full_path=False, branch_type=None): +def path_to_location(modulestore, usage_key, request=None, full_path=False): ''' 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,47 +82,46 @@ def path_to_location(modulestore, usage_key, request=None, full_path=False, bran queue.append((parent, newpath)) with modulestore.bulk_operations(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) + if not modulestore.has_item(usage_key): + raise ItemNotFoundError(usage_key) - path = find_path_to_course() - if path is None: - raise NoPathToItem(usage_key) + path = find_path_to_course() + if path is None: + raise NoPathToItem(usage_key) - if full_path: - return path + 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 + 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. + # 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) + 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 0928ab253b..8adbfcb911 100644 --- a/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -1752,7 +1752,7 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): 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, branch_type=ModuleStoreEnum.Branch.published_only) + path = path_to_location(self.store, location) assert path == expected not_found = ( diff --git a/xmodule/video_block/video_block.py b/xmodule/video_block/video_block.py index 12f0b2fb2a..ea9d1a4428 100644 --- a/xmodule/video_block/video_block.py +++ b/xmodule/video_block/video_block.py @@ -179,6 +179,7 @@ class VideoBlock( 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 = {