feat: update preview url to direct to mfe (openedx#35687)

* feat: update preview url to direct to mfe * fix: use url builder instead of string formatter * fix: url redirect for never published units
kdmccormick · Oct 28, 2024 · 2373dd0 · 2373dd0
1 parent e1f31fb
commit 2373dd0
Show file tree

Hide file tree

Showing 10 changed files with 274 additions and 251 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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