feat!: remove block handling from runtime initialization of ReplaceURLService [FC-0026]

cms/djangoapps/contentstore/views/preview.py

@@ -162,8 +162,6 @@ def _prepare_runtime_for_preview(request, block, field_data):
     course_id = block.location.course_key
     display_name_only = (block.category == 'static_tab')
 
-    replace_url_service = ReplaceURLService(course_id=course_id)
-
     wrappers = [
         # This wrapper wraps the block in the template specified above
         partial(
@@ -176,7 +174,7 @@ def _prepare_runtime_for_preview(request, block, field_data):
 
         # This wrapper replaces urls in the output that start with /static
         # with the correct course-specific url for the static content
-        partial(replace_urls_wrapper, replace_url_service=replace_url_service, static_replace_only=True),
+        partial(replace_urls_wrapper, replace_url_service=ReplaceURLService, static_replace_only=True),
         _studio_wrap_xblock,
     ]
 
@@ -215,7 +213,7 @@ def _prepare_runtime_for_preview(request, block, field_data):
         "teams_configuration": TeamsConfigurationService(),
         "sandbox": SandboxService(contentstore=contentstore, course_id=course_id),
         "cache": CacheService(cache),
-        'replace_urls': replace_url_service
+        'replace_urls': ReplaceURLService
     }
 
     block.runtime.get_block_for_descriptor = partial(_load_preview_block, request)

cms/djangoapps/contentstore/views/tests/test_preview.py

@@ -292,7 +292,7 @@ def test_cache(self):
 
     def test_replace_urls(self):
         html = '<a href="/static/id">'
-        assert self.block.runtime.replace_urls(html) == \
+        assert self.block.runtime.service(self.block, 'replace_urls').replace_urls(html) == \
             static_replace.replace_static_urls(html, course_id=self.course.id)
 
     def test_anonymous_user_id_preview(self):

common/djangoapps/static_replace/services.py

@@ -16,8 +16,7 @@ class ReplaceURLService(Service):
     A service for replacing static/course/jump-to-id URLs with absolute URLs in XBlocks.
 
     Args:
-        course_id: Course identifier to be used in the absolute URL
-        data_directory: (optional) Directory in which course data is stored
+        block: (optional) An XBlock instance. Used when retrieving the service from the DescriptorSystem.
         static_asset_path: (optional) Path for static assets, which overrides data_directory and course_id, if nonempty
         static_paths_out: (optional) Array to collect tuples for each static URI found:
             * the original unmodified static URI
@@ -27,21 +26,21 @@ class ReplaceURLService(Service):
     """
     def __init__(
         self,
-        data_directory=None,
-        course_id=None,
+        block=None,
         static_asset_path='',
         static_paths_out=None,
         jump_to_id_base_url=None,
         lookup_asset_url=None,
         **kwargs
     ):
         super().__init__(**kwargs)
-        self.data_directory = data_directory
-        self.course_id = course_id
         self.static_asset_path = static_asset_path
         self.static_paths_out = static_paths_out
         self.jump_to_id_base_url = jump_to_id_base_url
         self.lookup_asset_url = lookup_asset_url
+        # This is needed because the `Service` class initialization expects the XBlock passed as an `xblock` keyword
+        #  argument, but the `service` method from the `DescriptorSystem` passes a `block`.
+        self._xblock = self.xblock() or block
 
     def replace_urls(self, text, static_replace_only=False):
         """
@@ -51,19 +50,20 @@ def replace_urls(self, text, static_replace_only=False):
             text: String containing the URL to be replaced
             static_replace_only: If True, only static urls will be replaced
         """
+        block = self.xblock()
         if self.lookup_asset_url:
-            text = replace_static_urls(text, xblock=self.xblock(), lookup_asset_url=self.lookup_asset_url)
+            text = replace_static_urls(text, xblock=block, lookup_asset_url=self.lookup_asset_url)
         else:
             text = replace_static_urls(
                 text,
-                data_directory=self.data_directory,
-                course_id=self.course_id,
-                static_asset_path=self.static_asset_path,
+                data_directory=getattr(block, 'data_dir', None),
+                course_id=block.scope_ids.usage_id.context_key,
+                static_asset_path=self.static_asset_path or block.static_asset_path,
                 static_paths_out=self.static_paths_out
             )
             if not static_replace_only:
-                text = replace_course_urls(text, self.course_id)
+                text = replace_course_urls(text, block.scope_ids.usage_id.context_key)
                 if self.jump_to_id_base_url:
-                    text = replace_jump_to_id_urls(text, self.course_id, self.jump_to_id_base_url)
+                    text = replace_jump_to_id_urls(text, block.scope_ids.usage_id.context_key, self.jump_to_id_base_url)
 
         return text

common/djangoapps/static_replace/test/test_static_replace.py

@@ -1,9 +1,8 @@
 """Tests for static_replace"""
-
+from functools import partial
 
 import re
 from io import BytesIO
-from unittest import TestCase
 from unittest.mock import Mock, patch
 from urllib.parse import parse_qsl, quote, urlparse, urlunparse, urlencode
 
@@ -19,7 +18,8 @@
     make_static_urls_absolute,
     process_static_urls,
     replace_course_urls,
-    replace_static_urls
+    replace_static_urls,
+    replace_jump_to_id_urls,
 )
 from common.djangoapps.static_replace.services import ReplaceURLService
 from common.djangoapps.static_replace.wrapper import replace_urls_wrapper
@@ -593,10 +593,15 @@ def test_canonical_asset_path_with_new_style_assets(self, base_url, start, expec
             assert re.match(expected, asset_path) is not None
 
 
-class ReplaceURLServiceTest(TestCase):
+class ReplaceURLServiceTest(SharedModuleStoreTestCase):
     """
     Test ReplaceURLService methods
     """
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.course = CourseFactory.create(org='TestX', number='TS02', run='2015')
+
     def setUp(self):
         super().setUp()
         self.mock_replace_static_urls = self.create_patch(
@@ -619,8 +624,16 @@ def test_replace_static_url_only(self):
         """
         Test only replace_static_urls method called when static_replace_only is passed as True.
         """
-        replace_url_service = ReplaceURLService(course_id=COURSE_KEY)
-        return_text = replace_url_service.replace_urls("text", static_replace_only=True)
+        replace_url_service = ReplaceURLService(xblock=self.course)
+        replace_url_service.replace_urls("text", static_replace_only=True)
+        assert self.mock_replace_static_urls.called
+        assert not self.mock_replace_course_urls.called
+        assert not self.mock_replace_jump_to_id_urls.called
+
+    def test_service_block_argument(self):
+        """This service accepts either `block` or `xblock` keyword argument."""
+        replace_url_service = ReplaceURLService(block=self.course)
+        replace_url_service.replace_urls("text", static_replace_only=True)
         assert self.mock_replace_static_urls.called
         assert not self.mock_replace_course_urls.called
         assert not self.mock_replace_jump_to_id_urls.called
@@ -629,24 +642,24 @@ def test_replace_course_urls_called(self):
         """
         Test replace_course_urls method called static_replace_only is passed as False.
         """
-        replace_url_service = ReplaceURLService(course_id=COURSE_KEY)
-        return_text = replace_url_service.replace_urls("text")
+        replace_url_service = ReplaceURLService(xblock=self.course)
+        replace_url_service.replace_urls("text")
         assert self.mock_replace_course_urls.called
 
     def test_replace_jump_to_id_urls_called(self):
         """
         Test replace_jump_to_id_urls method called jump_to_id_base_url is provided.
         """
-        replace_url_service = ReplaceURLService(course_id=COURSE_KEY, jump_to_id_base_url="/course/course_id")
-        return_text = replace_url_service.replace_urls("text")
+        replace_url_service = ReplaceURLService(xblock=self.course, jump_to_id_base_url="/course/course_id")
+        replace_url_service.replace_urls("text")
         assert self.mock_replace_jump_to_id_urls.called
 
     def test_replace_jump_to_id_urls_not_called(self):
         """
         Test replace_jump_to_id_urls method called jump_to_id_base_url is not provided.
         """
-        replace_url_service = ReplaceURLService(course_id=COURSE_KEY)
-        return_text = replace_url_service.replace_urls("text")
+        replace_url_service = ReplaceURLService(xblock=self.course)
+        replace_url_service.replace_urls("text")
         assert not self.mock_replace_jump_to_id_urls.called
 
 
@@ -659,54 +672,53 @@ class TestReplaceURLWrapper(SharedModuleStoreTestCase):
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
-        cls.course = CourseFactory.create(
-            org='TestX',
-            number='TS02',
-            run='2015'
-        )
+        cls.course = CourseFactory.create(org='TestX', number='TS02', run='2015')
 
     def test_replace_jump_to_id_urls(self):
         """
         Verify that the jump-to URL has been replaced.
         """
-        replace_url_service = ReplaceURLService(course_id=self.course.id, jump_to_id_base_url='/base_url/')
+        fragment = Fragment('<a href="/jump_to_id/id">')
         test_replace = replace_urls_wrapper(
             block=self.course,
             view='baseview',
-            frag=Fragment('<a href="/jump_to_id/id">'),
+            frag=fragment,
             context=None,
-            replace_url_service=replace_url_service
+            replace_url_service=partial(ReplaceURLService, jump_to_id_base_url='/base_url/'),
         )
         assert isinstance(test_replace, Fragment)
+        assert test_replace.content == replace_jump_to_id_urls(fragment.content, self.course.id, '/base_url/')
         assert test_replace.content == '<a href="/base_url/id">'
 
     def test_replace_course_urls(self):
         """
         Verify that the course URL has been replaced.
         """
-        replace_url_service = ReplaceURLService(course_id=self.course.id)
+        fragment = Fragment('<a href="/course/id">')
         test_replace = replace_urls_wrapper(
             block=self.course,
             view='baseview',
-            frag=Fragment('<a href="/course/id">'),
+            frag=fragment,
             context=None,
-            replace_url_service=replace_url_service
+            replace_url_service=ReplaceURLService,
         )
         assert isinstance(test_replace, Fragment)
+        assert test_replace.content == replace_course_urls(fragment.content, course_key=self.course.id)
         assert test_replace.content == '<a href="/courses/course-v1:TestX+TS02+2015/id">'
 
     def test_replace_static_urls(self):
         """
         Verify that the static URL has been replaced.
         """
-        replace_url_service = ReplaceURLService(course_id=self.course.id)
+        fragment = Fragment('<a href="/static/id">')
         test_replace = replace_urls_wrapper(
             block=self.course,
             view='baseview',
-            frag=Fragment('<a href="/static/id">'),
+            frag=fragment,
             context=None,
-            replace_url_service=replace_url_service,
-            static_replace_only=True
+            replace_url_service=ReplaceURLService,
+            static_replace_only=True,
         )
         assert isinstance(test_replace, Fragment)
+        assert test_replace.content == replace_static_urls(fragment.content, course_id=self.course.id)
         assert test_replace.content == '<a href="/asset-v1:TestX+TS02+2015+type@asset+block/id">'

common/djangoapps/static_replace/wrapper.py

@@ -7,6 +7,6 @@
 
 def replace_urls_wrapper(block, view, frag, context, replace_url_service, static_replace_only=False):  # pylint: disable=unused-argument
     """
-    Replace any static/course/jump-to-id URLs in XBlock to absolute URLs
+    Replace any static/course/jump-to-id URLs in XBlock to absolute URLs.
     """
-    return wrap_fragment(frag, replace_url_service.replace_urls(frag.content, static_replace_only))
+    return wrap_fragment(frag, replace_url_service(xblock=block).replace_urls(frag.content, static_replace_only))

lms/djangoapps/courseware/block_render.py

@@ -482,11 +482,20 @@ def inner_get_block(block):
             request_token=request_token,
         ))
 
-    replace_url_service = ReplaceURLService(
-        data_directory=getattr(block, 'data_dir', None),
-        course_id=course_id,
-        static_asset_path=static_asset_path or block.static_asset_path,
-        jump_to_id_base_url=reverse('jump_to_id', kwargs={'course_id': str(course_id), 'module_id': ''})
+    # HACK: The following test fails when we do not access this attribute.
+    #  lms/djangoapps/courseware/tests/test_views.py::TestRenderXBlock::test_success_enrolled_staff
+    #  This happens because accessing `field_data` caches the XBlock's parents through the inheritance mixin.
+    #  If we do this operation before assigning `inner_get_block` to the `get_block_for_descriptor` attribute, these
+    #  parents will be initialized without binding the user data (that's how the `get_block` works in `x_module.py`).
+    #  If we retrieve the parents after this operation (e.g., in the `enclosing_sequence_for_gating_checks` function),
+    #  they will have their runtimes initialized, which can lead to an altered behavior of the XBlock-specific
+    #  parts of other runtimes. We will keep this workaround until we remove all XBlock-specific code from here.
+    block.static_asset_path  # pylint: disable=pointless-statement
+
+    replace_url_service = partial(
+        ReplaceURLService,
+        static_asset_path=static_asset_path,
+        jump_to_id_base_url=reverse('jump_to_id', kwargs={'course_id': str(course_id), 'module_id': ''}),
     )
 
     # Rewrite static urls with course-specific absolute urls

lms/djangoapps/courseware/tests/test_block_render.py

@@ -58,7 +58,6 @@
 from xmodule.services import RebindUserServiceError
 from xmodule.video_block import VideoBlock  # lint-amnesty, pylint: disable=wrong-import-order
 from xmodule.x_module import STUDENT_VIEW, DescriptorSystem  # lint-amnesty, pylint: disable=wrong-import-order
-from common.djangoapps import static_replace
 from common.djangoapps.course_modes.models import CourseMode  # lint-amnesty, pylint: disable=reimported
 from common.djangoapps.student.tests.factories import (
     BetaTesterFactory,
@@ -2896,22 +2895,6 @@ def test_cache(self):
         assert hasattr(self.block.runtime.cache, 'get')
         assert hasattr(self.block.runtime.cache, 'set')
 
-    def test_replace_urls(self):
-        html = '<a href="/static/id">'
-        assert self.block.runtime.replace_urls(html) == \
-            static_replace.replace_static_urls(html, course_id=self.course.id)
-
-    def test_replace_course_urls(self):
-        html = '<a href="/course/id">'
-        assert self.block.runtime.replace_course_urls(html) == \
-            static_replace.replace_course_urls(html, course_key=self.course.id)
-
-    def test_replace_jump_to_id_urls(self):
-        html = '<a href="/jump_to_id/id">'
-        jump_to_id_base_url = reverse('jump_to_id', kwargs={'course_id': str(self.course.id), 'module_id': ''})
-        assert self.block.runtime.replace_jump_to_id_urls(html) == \
-            static_replace.replace_jump_to_id_urls(html, self.course.id, jump_to_id_base_url)
-
     @XBlock.register_temp_plugin(PureXBlock, 'pure')
     @XBlock.register_temp_plugin(PureXBlockWithChildren, identifier='xblock')
     def test_course_id(self):

openedx/core/djangoapps/xblock/runtime/shims.py

@@ -11,7 +11,6 @@
 from fs.memoryfs import MemoryFS
 
 from common.djangoapps.edxmako.shortcuts import render_to_string
-from common.djangoapps.static_replace.services import ReplaceURLService
 from common.djangoapps.student.models import anonymous_id_for_user
 from openedx.core.djangoapps.xblock.apps import get_xblock_app_config
 
@@ -162,39 +161,6 @@ def process_xml(self, xml):
         # as part of the runtime.
         raise NotImplementedError("This newer runtime does not support process_xml()")
 
-    def replace_urls(self, html_str):
-        """
-        Deprecated in favor of the replace_urls service.
-        """
-        warnings.warn(
-            'replace_urls is deprecated. Please use ReplaceURLService instead.',
-            DeprecationWarning, stacklevel=3,
-        )
-        return ReplaceURLService(
-            xblock=self._active_block,
-            lookup_asset_url=self._lookup_asset_url
-        ).replace_urls(html_str)
-
-    def replace_course_urls(self, html_str):
-        """
-        Deprecated in favor of the replace_urls service.
-        """
-        warnings.warn(
-            'replace_course_urls is deprecated. Please use ReplaceURLService instead.',
-            DeprecationWarning, stacklevel=3,
-        )
-        return html_str
-
-    def replace_jump_to_id_urls(self, html_str):
-        """
-        Deprecated in favor of the replace_urls service.
-        """
-        warnings.warn(
-            'replace_jump_to_id_urls is deprecated. Please use ReplaceURLService instead.',
-            DeprecationWarning, stacklevel=3,
-        )
-        return html_str
-
     @property
     def resources_fs(self):
         """