diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index bf5f463a1e..144b92d11c 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/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) diff --git a/cms/djangoapps/contentstore/views/tests/test_preview.py b/cms/djangoapps/contentstore/views/tests/test_preview.py index 1810842d4f..0908a2eb8f 100644 --- a/cms/djangoapps/contentstore/views/tests/test_preview.py +++ b/cms/djangoapps/contentstore/views/tests/test_preview.py @@ -292,7 +292,7 @@ class CmsModuleSystemShimTest(ModuleStoreTestCase): def test_replace_urls(self): html = '' - 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): diff --git a/common/djangoapps/static_replace/services.py b/common/djangoapps/static_replace/services.py index 97ce314da6..80c284a9b5 100644 --- a/common/djangoapps/static_replace/services.py +++ b/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,8 +26,7 @@ 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, @@ -36,12 +34,13 @@ class ReplaceURLService(Service): **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 @@ class ReplaceURLService(Service): 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 diff --git a/common/djangoapps/static_replace/test/test_static_replace.py b/common/djangoapps/static_replace/test/test_static_replace.py index c780d32104..1bdb770cad 100644 --- a/common/djangoapps/static_replace/test/test_static_replace.py +++ b/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 @@ from common.djangoapps.static_replace import ( 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 @@ class CanonicalContentTest(SharedModuleStoreTestCase): 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 @@ class ReplaceURLServiceTest(TestCase): """ 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 @@ class ReplaceURLServiceTest(TestCase): """ 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('') test_replace = replace_urls_wrapper( block=self.course, view='baseview', - frag=Fragment(''), + 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 == '' 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('') test_replace = replace_urls_wrapper( block=self.course, view='baseview', - frag=Fragment(''), + 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 == '' 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('') test_replace = replace_urls_wrapper( block=self.course, view='baseview', - frag=Fragment(''), + 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 == '' diff --git a/common/djangoapps/static_replace/wrapper.py b/common/djangoapps/static_replace/wrapper.py index d9a58b20d5..5347ae6b6c 100644 --- a/common/djangoapps/static_replace/wrapper.py +++ b/common/djangoapps/static_replace/wrapper.py @@ -7,6 +7,6 @@ from openedx.core.lib.xblock_utils import wrap_fragment 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)) diff --git a/lms/djangoapps/courseware/block_render.py b/lms/djangoapps/courseware/block_render.py index 4bb7a129b1..e7675893f5 100644 --- a/lms/djangoapps/courseware/block_render.py +++ b/lms/djangoapps/courseware/block_render.py @@ -482,11 +482,20 @@ def prepare_runtime_for_user( 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 diff --git a/lms/djangoapps/courseware/tests/test_block_render.py b/lms/djangoapps/courseware/tests/test_block_render.py index e0f97f5418..33ed581c68 100644 --- a/lms/djangoapps/courseware/tests/test_block_render.py +++ b/lms/djangoapps/courseware/tests/test_block_render.py @@ -58,7 +58,6 @@ from xmodule.modulestore.tests.test_asides import AsideTestType # lint-amnesty, 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 @@ class LmsModuleSystemShimTest(SharedModuleStoreTestCase): assert hasattr(self.block.runtime.cache, 'get') assert hasattr(self.block.runtime.cache, 'set') - def test_replace_urls(self): - html = '' - 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 = '' - 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 = '' - 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): diff --git a/openedx/core/djangoapps/xblock/runtime/shims.py b/openedx/core/djangoapps/xblock/runtime/shims.py index 6215afab34..abef606bcf 100644 --- a/openedx/core/djangoapps/xblock/runtime/shims.py +++ b/openedx/core/djangoapps/xblock/runtime/shims.py @@ -11,7 +11,6 @@ from django.utils.functional import cached_property 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 @@ class RuntimeShim: # 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): """ diff --git a/xmodule/x_module.py b/xmodule/x_module.py index fbea2b2896..df29004422 100644 --- a/xmodule/x_module.py +++ b/xmodule/x_module.py @@ -1292,51 +1292,6 @@ class ModuleSystemShim: ) return self._runtime_services.get('cache') or self._services.get('cache') or DoNothingCache() - @property - def replace_urls(self): - """ - Returns a function to replace static urls with course specific urls. - - Deprecated in favor of the replace_urls service. - """ - warnings.warn( - 'runtime.replace_urls is deprecated. Please use the replace_urls service instead.', - DeprecationWarning, stacklevel=2, - ) - replace_urls_service = self._runtime_services.get('replace_urls') or self._services.get('replace_urls') - if replace_urls_service: - return partial(replace_urls_service.replace_urls, static_replace_only=True) - - @property - def replace_course_urls(self): - """ - Returns a function to replace static urls with course specific urls. - - Deprecated in favor of the replace_urls service. - """ - warnings.warn( - 'runtime.replace_course_urls is deprecated. Please use the replace_urls service instead.', - DeprecationWarning, stacklevel=2, - ) - replace_urls_service = self._runtime_services.get('replace_urls') or self._services.get('replace_urls') - if replace_urls_service: - return partial(replace_urls_service.replace_urls) - - @property - def replace_jump_to_id_urls(self): - """ - Returns a function to replace static urls with course specific urls. - - Deprecated in favor of the replace_urls service. - """ - warnings.warn( - 'runtime.replace_jump_to_id_urls is deprecated. Please use the replace_urls service instead.', - DeprecationWarning, stacklevel=2, - ) - replace_urls_service = self._runtime_services.get('replace_urls') or self._services.get('replace_urls') - if replace_urls_service: - return partial(replace_urls_service.replace_urls) - @property def filestore(self): """