diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 53b6d178325e..42a4535adba7 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -28,7 +28,8 @@ from xmodule.x_module import AUTHOR_VIEW, PREVIEW_VIEWS, STUDENT_VIEW, ModuleSystem, XModule, XModuleDescriptor from cms.djangoapps.xblock_config.models import StudioConfig from cms.lib.xblock.field_data import CmsFieldData -from common.djangoapps import static_replace +from common.djangoapps.static_replace.services import ReplaceURLService +from common.djangoapps.static_replace.wrapper import replace_urls_wrapper from common.djangoapps.edxmako.shortcuts import render_to_string from common.djangoapps.edxmako.services import MakoService from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService @@ -36,7 +37,6 @@ from openedx.core.lib.license import wrap_with_license from openedx.core.lib.cache_utils import CacheService from openedx.core.lib.xblock_utils import ( - replace_static_urls, request_token, wrap_fragment, wrap_xblock, @@ -162,6 +162,8 @@ def _preview_module_system(request, descriptor, field_data): course_id = descriptor.location.course_key display_name_only = (descriptor.category == 'static_tab') + replace_url_service = ReplaceURLService(course_id=course_id) + wrappers = [ # This wrapper wraps the module in the template specified above partial( @@ -174,7 +176,7 @@ def _preview_module_system(request, descriptor, 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_static_urls, None, course_id=course_id), + partial(replace_urls_wrapper, replace_url_service=replace_url_service, static_replace_only=True), _studio_wrap_xblock, ] @@ -199,7 +201,6 @@ def _preview_module_system(request, descriptor, field_data): filestore=descriptor.runtime.resources_fs, get_module=partial(_load_preview_module, request), debug=True, - replace_urls=partial(static_replace.replace_static_urls, data_directory=None, course_id=course_id), mixins=settings.XBLOCK_MIXINS, course_id=course_id, @@ -223,6 +224,7 @@ def _preview_module_system(request, descriptor, field_data): "teams_configuration": TeamsConfigurationService(), "sandbox": SandboxService(contentstore=contentstore, course_id=course_id), "cache": CacheService(cache), + 'replace_urls': replace_url_service }, ) diff --git a/cms/djangoapps/contentstore/views/tests/test_preview.py b/cms/djangoapps/contentstore/views/tests/test_preview.py index d98621192005..1f8d9ce51fe2 100644 --- a/cms/djangoapps/contentstore/views/tests/test_preview.py +++ b/cms/djangoapps/contentstore/views/tests/test_preview.py @@ -22,6 +22,7 @@ from xmodule.modulestore.tests.test_asides import AsideTestType from cms.djangoapps.contentstore.utils import reverse_usage_url from cms.djangoapps.xblock_config.models import StudioConfig +from common.djangoapps import static_replace from common.djangoapps.student.tests.factories import UserFactory from ..preview import _preview_module_system, get_preview_fragment @@ -174,6 +175,7 @@ def test_block_branch_not_changed_by_preview_handler(self, default_store): @XBlock.needs("field-data") @XBlock.needs("i18n") @XBlock.needs("mako") +@XBlock.needs("replace_urls") @XBlock.needs("user") @XBlock.needs("teams_configuration") class PureXBlock(XBlock): @@ -205,7 +207,7 @@ def setUp(self): self.field_data = mock.Mock() @XBlock.register_temp_plugin(PureXBlock, identifier='pure') - @ddt.data("user", "i18n", "field-data", "teams_configuration") + @ddt.data("user", "i18n", "field-data", "teams_configuration", "replace_urls") def test_expected_services_exist(self, expected_service): """ Tests that the 'user' and 'i18n' services are provided by the Studio runtime. @@ -287,3 +289,8 @@ def test_no_get_python_lib_zip(self): def test_cache(self): assert hasattr(self.runtime.cache, 'get') assert hasattr(self.runtime.cache, 'set') + + def test_replace_urls(self): + html = '' + assert self.runtime.replace_urls(html) == \ + static_replace.replace_static_urls(html, course_id=self.runtime.course_id) diff --git a/common/djangoapps/static_replace/__init__.py b/common/djangoapps/static_replace/__init__.py index 55b1de1f8941..8bf44b9a6bb3 100644 --- a/common/djangoapps/static_replace/__init__.py +++ b/common/djangoapps/static_replace/__init__.py @@ -147,12 +147,20 @@ def replace(__, prefix, quote, rest): ) -def replace_static_urls(text, data_directory=None, course_id=None, static_asset_path='', static_paths_out=None): +def replace_static_urls( + text, + data_directory=None, + course_id=None, + static_asset_path='', + static_paths_out=None, + xblock=None, + lookup_asset_url=None +): """ Replace /static/$stuff urls either with their correct url as generated by collectstatic, (/static/$md5_hashed_stuff) or by the course-specific content static url /static/$course_data_dir/$stuff, or, if course_namespace is not None, by the - correct url in the contentstore (/c4x/.. or /asset-loc:..) + correct url in the contentstore (/c4x/.. or /asset-loc:..) or by lookup_asset_url text: The source text to do the substitution in data_directory: The directory in which course data is stored @@ -161,6 +169,8 @@ def replace_static_urls(text, data_directory=None, course_id=None, static_asset_ static_paths_out: (optional) pass an array to collect tuples for each static URI found: * the original unmodified static URI * the updated static URI (will match the original if unchanged) + xblock: xblock where the static assets are stored + lookup_url_func: Lookup function which returns the correct path of the asset """ if static_paths_out is None: @@ -176,6 +186,10 @@ def replace_static_url(original, prefix, quote, rest): static_paths_out.append((original_uri, original_uri)) return original + if lookup_asset_url: + new_url = lookup_asset_url(xblock, rest) or original_uri + return "".join([quote, new_url, quote]) + # In debug mode, if we can find the url as is, if settings.DEBUG and finders.find(rest, True): static_paths_out.append((original_uri, original_uri)) diff --git a/common/djangoapps/static_replace/services.py b/common/djangoapps/static_replace/services.py new file mode 100644 index 000000000000..97ce314da676 --- /dev/null +++ b/common/djangoapps/static_replace/services.py @@ -0,0 +1,69 @@ +""" +Supports replacement of static/course/jump-to-id URLs to absolute URLs in XBlocks. +""" + +from xblock.reference.plugins import Service + +from common.djangoapps.static_replace import ( + replace_course_urls, + replace_jump_to_id_urls, + replace_static_urls +) + + +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 + 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 + * the updated static URI (will match the original if unchanged) + jump_to_id_base_url: (optional) Absolute path to the base of the handler that will perform the redirect + lookup_url_func: Lookup function which returns the correct path of the asset + """ + def __init__( + self, + data_directory=None, + course_id=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 + + def replace_urls(self, text, static_replace_only=False): + """ + Replaces all static/course/jump-to-id URLs in provided text/html. + + Args: + text: String containing the URL to be replaced + static_replace_only: If True, only static urls will be replaced + """ + if self.lookup_asset_url: + text = replace_static_urls(text, xblock=self.xblock(), 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, + static_paths_out=self.static_paths_out + ) + if not static_replace_only: + text = replace_course_urls(text, self.course_id) + if self.jump_to_id_base_url: + text = replace_jump_to_id_urls(text, self.course_id, 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 4b2d8cb7f26a..093ff9de0698 100644 --- a/common/djangoapps/static_replace/test/test_static_replace.py +++ b/common/djangoapps/static_replace/test/test_static_replace.py @@ -3,6 +3,7 @@ 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 @@ -11,6 +12,7 @@ from django.test import override_settings from opaque_keys.edx.keys import CourseKey from PIL import Image +from web_fragments.fragment import Fragment from common.djangoapps.static_replace import ( _url_replace_regex, @@ -19,6 +21,8 @@ replace_course_urls, replace_static_urls ) +from common.djangoapps.static_replace.services import ReplaceURLService +from common.djangoapps.static_replace.wrapper import replace_urls_wrapper from xmodule.assetstore.assetmgr import AssetManager # lint-amnesty, pylint: disable=wrong-import-order from xmodule.contentstore.content import StaticContent # lint-amnesty, pylint: disable=wrong-import-order from xmodule.contentstore.django import contentstore # lint-amnesty, pylint: disable=wrong-import-order @@ -783,3 +787,143 @@ def test_canonical_asset_path_with_c4x_style_assets(self, base_url, start, expec with check_mongo_calls(mongo_calls): asset_path = StaticContent.get_canonicalized_asset_path(self.courses[prefix].id, start, base_url, exts) assert re.match(expected, asset_path) is not None + + +class ReplaceURLServiceTest(TestCase): + """ + Test ReplaceURLService methods + """ + def setUp(self): + super().setUp() + self.mock_replace_static_urls = self.create_patch( + 'common.djangoapps.static_replace.services.replace_static_urls' + ) + self.mock_replace_course_urls = self.create_patch( + 'common.djangoapps.static_replace.services.replace_course_urls' + ) + self.mock_replace_jump_to_id_urls = self.create_patch( + 'common.djangoapps.static_replace.services.replace_jump_to_id_urls' + ) + + def create_patch(self, name): + patcher = patch(name) + mock_method = patcher.start() + self.addCleanup(patcher.stop) + return mock_method + + 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) + 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_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") + 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") + 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") + assert not self.mock_replace_jump_to_id_urls.called + + +@ddt.ddt +class TestReplaceURLWrapper(SharedModuleStoreTestCase): + """ + Tests for replace_url_wrapper utility function. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.course_mongo = CourseFactory.create( + default_store=ModuleStoreEnum.Type.mongo, + org='TestX', + number='TS01', + run='2015' + ) + cls.course_split = CourseFactory.create( + default_store=ModuleStoreEnum.Type.split, + org='TestX', + number='TS02', + run='2015' + ) + + @ddt.data('course_mongo', 'course_split') + def test_replace_jump_to_id_urls(self, course_id): + """ + Verify that the jump-to URL has been replaced. + """ + course = getattr(self, course_id) + replace_url_service = ReplaceURLService(course_id=course.id, jump_to_id_base_url='/base_url/') + test_replace = replace_urls_wrapper( + block=course, + view='baseview', + frag=Fragment(''), + context=None, + replace_url_service=replace_url_service + ) + assert isinstance(test_replace, Fragment) + assert test_replace.content == '' + + @ddt.data( + ('course_mongo', ''), + ('course_split', '') + ) + @ddt.unpack + def test_replace_course_urls(self, course_id, anchor_tag): + """ + Verify that the course URL has been replaced. + """ + course = getattr(self, course_id) + replace_url_service = ReplaceURLService(course_id=course.id) + test_replace = replace_urls_wrapper( + block=course, + view='baseview', + frag=Fragment(''), + context=None, + replace_url_service=replace_url_service + ) + assert isinstance(test_replace, Fragment) + assert test_replace.content == anchor_tag + + @ddt.data( + ('course_mongo', ''), + ('course_split', '') + ) + @ddt.unpack + def test_replace_static_urls(self, course_id, anchor_tag): + """ + Verify that the static URL has been replaced. + """ + course = getattr(self, course_id) + replace_url_service = ReplaceURLService(course_id=course.id) + test_replace = replace_urls_wrapper( + block=course, + view='baseview', + frag=Fragment(''), + context=None, + replace_url_service=replace_url_service, + static_replace_only=True + ) + assert isinstance(test_replace, Fragment) + assert test_replace.content == anchor_tag diff --git a/common/djangoapps/static_replace/wrapper.py b/common/djangoapps/static_replace/wrapper.py new file mode 100644 index 000000000000..d9a58b20d562 --- /dev/null +++ b/common/djangoapps/static_replace/wrapper.py @@ -0,0 +1,12 @@ +""" +Wrapper function to replace static/course/jump-to-id URLs in XBlock to absolute URLs +""" + +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 + """ + return wrap_fragment(frag, replace_url_service.replace_urls(frag.content, static_replace_only)) diff --git a/common/lib/xmodule/xmodule/capa_module.py b/common/lib/xmodule/xmodule/capa_module.py index 0a7f2674f974..efff7e9011b9 100644 --- a/common/lib/xmodule/xmodule/capa_module.py +++ b/common/lib/xmodule/xmodule/capa_module.py @@ -123,6 +123,7 @@ def from_json(self, value): @XBlock.needs('mako') @XBlock.needs('cache') @XBlock.needs('sandbox') +@XBlock.needs('replace_urls') # Studio doesn't provide XQueueService, but the LMS does. @XBlock.wants('xqueue') @XBlock.wants('call_to_action') @@ -1175,7 +1176,9 @@ def get_demand_hint(self, hint_index): ) ), # Course-authored HTML demand hints are supported. - hint_text=HTML(self.runtime.replace_urls(get_inner_html_from_xpath(demand_hints[counter]))) + hint_text=HTML(self.runtime.service(self, "replace_urls").replace_urls( + get_inner_html_from_xpath(demand_hints[counter]) + )) ) counter += 1 @@ -1280,12 +1283,7 @@ def get_problem_html(self, encapsulate=True, submit_notification=False): # Now do all the substitutions which the LMS module_render normally does, but # we need to do here explicitly since we can get called for our HTML via AJAX - html = self.runtime.replace_urls(html) - if self.runtime.replace_course_urls: - html = self.runtime.replace_course_urls(html) - - if self.runtime.replace_jump_to_id_urls: - html = self.runtime.replace_jump_to_id_urls(html) + html = self.runtime.service(self, "replace_urls").replace_urls(html) return html @@ -1567,11 +1565,7 @@ def get_answer(self, _data): new_answers = {} for answer_id in answers: try: - answer_content = self.runtime.replace_urls(answers[answer_id]) - if self.runtime.replace_course_urls: - answer_content = self.runtime.replace_course_urls(answer_content) - if self.runtime.replace_jump_to_id_urls: - answer_content = self.runtime.replace_jump_to_id_urls(answer_content) + answer_content = self.runtime.service(self, "replace_urls").replace_urls(answers[answer_id]) new_answer = {answer_id: answer_content} except TypeError: log.debug('Unable to perform URL substitution on answers[%s]: %s', diff --git a/common/lib/xmodule/xmodule/html_module.py b/common/lib/xmodule/xmodule/html_module.py index de22e1a823d6..5609ea1934be 100644 --- a/common/lib/xmodule/xmodule/html_module.py +++ b/common/lib/xmodule/xmodule/html_module.py @@ -457,6 +457,7 @@ class CourseInfoFields: @XBlock.tag("detached") +@XBlock.needs('replace_urls') class CourseInfoBlock(CourseInfoFields, HtmlBlockMixin): # lint-amnesty, pylint: disable=abstract-method """ These pieces of course content are treated as HtmlBlock but we need to overload where the templates are located diff --git a/common/lib/xmodule/xmodule/tests/__init__.py b/common/lib/xmodule/xmodule/tests/__init__.py index a1a95ea220e0..bb7015315864 100644 --- a/common/lib/xmodule/xmodule/tests/__init__.py +++ b/common/lib/xmodule/xmodule/tests/__init__.py @@ -35,7 +35,7 @@ from xmodule.modulestore.draft_and_published import ModuleStoreDraftAndPublished from xmodule.modulestore.inheritance import InheritanceMixin from xmodule.modulestore.xml import CourseLocationManager -from xmodule.tests.helpers import mock_render_template, StubMakoService, StubUserService +from xmodule.tests.helpers import StubReplaceURLService, mock_render_template, StubMakoService, StubUserService from xmodule.util.sandboxing import SandboxService from xmodule.x_module import DoNothingCache, ModuleSystem, XModuleDescriptor, XModuleMixin from openedx.core.lib.cache_utils import CacheService @@ -125,6 +125,8 @@ def get_test_system( mako_service = StubMakoService(render_template=render_template) + replace_url_service = StubReplaceURLService() + descriptor_system = get_test_descriptor_system() def get_module(descriptor): @@ -147,7 +149,6 @@ def get_module(descriptor): static_url='/static', track_function=Mock(name='get_test_system.track_function'), get_module=get_module, - replace_urls=str, filestore=Mock(name='get_test_system.filestore', root_path='.'), debug=True, hostname="edx.org", @@ -162,6 +163,7 @@ def get_module(descriptor): waittime=10, construct_callback=Mock(name='get_test_system.xqueue.construct_callback', side_effect="/"), ), + 'replace_urls': replace_url_service }, node_path=os.environ.get("NODE_PATH", "/usr/local/lib/node_modules"), course_id=course_id, diff --git a/common/lib/xmodule/xmodule/tests/helpers.py b/common/lib/xmodule/xmodule/tests/helpers.py index f57f67fe97f7..8c960a2b5717 100644 --- a/common/lib/xmodule/xmodule/tests/helpers.py +++ b/common/lib/xmodule/xmodule/tests/helpers.py @@ -97,3 +97,15 @@ def get_user_by_anonymous_id(self, uid=None): # pylint: disable=unused-argument Return the original user passed into the service. """ return self.user + + +class StubReplaceURLService: + """ + Stub ReplaceURLService for testing modules. + """ + + def replace_urls(self, text, static_replace_only=False): + """ + Invokes the configured render_template method. + """ + return text diff --git a/common/lib/xmodule/xmodule/tests/test_capa_module.py b/common/lib/xmodule/xmodule/tests/test_capa_module.py index 2d3f2a4e42bb..2b2b8e342899 100644 --- a/common/lib/xmodule/xmodule/tests/test_capa_module.py +++ b/common/lib/xmodule/xmodule/tests/test_capa_module.py @@ -3157,9 +3157,10 @@ def test_get_answer_with_jump_to_id_urls(self): data = {} problem = CapaFactory.create(showanswer='always', xml=problem_xml) - problem.runtime.replace_jump_to_id_urls = Mock() + problem.runtime.service(problem, 'replace_urls').replace_urls = Mock() + problem.get_answer(data) - assert problem.runtime.replace_jump_to_id_urls.called + assert problem.runtime.service(problem, 'replace_urls').replace_urls.called class ProblemBlockReportGenerationTest(unittest.TestCase): diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 46ae9ce15682..fb59a44f03cb 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/common/lib/xmodule/xmodule/x_module.py @@ -1959,6 +1959,51 @@ def cache(self): ) return 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=3, + ) + replace_urls_service = 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=3, + ) + replace_urls_service = 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=3, + ) + replace_urls_service = self._services.get('replace_urls') + if replace_urls_service: + return partial(replace_urls_service.replace_urls) + class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, Runtime): """ @@ -1975,11 +2020,9 @@ class ModuleSystem(MetricsMixin, ConfigurableFragmentWrapper, ModuleSystemShim, def __init__( self, static_url, track_function, get_module, - replace_urls, descriptor_runtime, filestore=None, + descriptor_runtime, filestore=None, debug=False, hostname="", publish=None, node_path="", - course_id=None, - replace_course_urls=None, - replace_jump_to_id_urls=None, error_descriptor_class=None, + course_id=None, error_descriptor_class=None, field_data=None, rebind_noauth_module_to_user=None, **kwargs): """ @@ -1999,10 +2042,6 @@ def __init__( filestore - A filestore ojbect. Defaults to an instance of OSFS based at settings.DATA_DIR. - replace_urls - TEMPORARY - A function like static_replace.replace_urls - that capa_module can use to fix up the static urls in - ajax results. - descriptor_runtime - A `DescriptorSystem` to use for loading xblocks by id course_id - the course_id containing this module @@ -2029,14 +2068,11 @@ def __init__( self.get_module = get_module self.DEBUG = self.debug = debug self.HOSTNAME = self.hostname = hostname - self.replace_urls = replace_urls self.node_path = node_path self.course_id = course_id if publish: self.publish = publish - self.replace_course_urls = replace_course_urls - self.replace_jump_to_id_urls = replace_jump_to_id_urls self.error_descriptor_class = error_descriptor_class self.xmodule_instance = None diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index 4bf1a46dcd88..f9ee3c8d211d 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/lms/djangoapps/courseware/module_render.py @@ -45,7 +45,8 @@ from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import ItemNotFoundError from xmodule.util.sandboxing import SandboxService -from common.djangoapps import static_replace +from common.djangoapps.static_replace.services import ReplaceURLService +from common.djangoapps.static_replace.wrapper import replace_urls_wrapper from common.djangoapps.xblock_django.constants import ATTR_KEY_USER_ID from capa.xqueue_interface import XQueueService # lint-amnesty, pylint: disable=wrong-import-order from lms.djangoapps.courseware.access import get_user_role, has_access @@ -79,10 +80,7 @@ add_staff_markup, get_aside_from_xblock, hash_resource, - is_xblock_aside, - replace_course_urls, - replace_jump_to_id_urls, - replace_static_urls + is_xblock_aside ) from openedx.core.lib.xblock_utils import request_token as xblock_request_token from openedx.core.lib.xblock_utils import wrap_xblock @@ -692,32 +690,15 @@ def rebind_noauth_module_to_user(module, real_user): request_token=request_token, )) - # TODO (cpennington): When modules are shared between courses, the static - # prefix is going to have to be specific to the module, not the directory - # that the xml was loaded from - - # Rewrite urls beginning in /static to point to course-specific content - block_wrappers.append(partial( - replace_static_urls, - getattr(descriptor, 'data_dir', None), + replace_url_service = ReplaceURLService( + data_directory=getattr(descriptor, 'data_dir', None), course_id=course_id, - static_asset_path=static_asset_path or descriptor.static_asset_path - )) - - # Allow URLs of the form '/course/' refer to the root of multicourse directory - # hierarchy of this course - block_wrappers.append(partial(replace_course_urls, course_id)) - - # this will rewrite intra-courseware links (/jump_to_id/). This format - # is an improvement over the /course/... format for studio authored courses, - # because it is agnostic to course-hierarchy. - # NOTE: module_id is empty string here. The 'module_id' will get assigned in the replacement - # function, we just need to specify something to get the reverse() to work. - block_wrappers.append(partial( - replace_jump_to_id_urls, - course_id, - reverse('jump_to_id', kwargs={'course_id': str(course_id), 'module_id': ''}), - )) + static_asset_path=static_asset_path or descriptor.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 + block_wrappers.append(partial(replace_urls_wrapper, replace_url_service=replace_url_service)) block_wrappers.append(partial(display_access_messages, user)) block_wrappers.append(partial(course_expiration_wrapper, user)) @@ -752,24 +733,6 @@ def rebind_noauth_module_to_user(module, real_user): user=user, debug=settings.DEBUG, hostname=settings.SITE_NAME, - # TODO (cpennington): This should be removed when all html from - # a module is coming through get_html and is therefore covered - # by the replace_static_urls code below - replace_urls=partial( - static_replace.replace_static_urls, - data_directory=getattr(descriptor, 'data_dir', None), - course_id=course_id, - static_asset_path=static_asset_path or descriptor.static_asset_path, - ), - replace_course_urls=partial( - static_replace.replace_course_urls, - course_key=course_id - ), - replace_jump_to_id_urls=partial( - static_replace.replace_jump_to_id_urls, - course_id=course_id, - jump_to_id_base_url=reverse('jump_to_id', kwargs={'course_id': str(course_id), 'module_id': ''}) - ), node_path=settings.NODE_PATH, publish=publish, course_id=course_id, @@ -793,6 +756,7 @@ def rebind_noauth_module_to_user(module, real_user): 'cache': CacheService(cache), 'sandbox': SandboxService(contentstore=contentstore, course_id=course_id), 'xqueue': xqueue_service, + 'replace_urls': replace_url_service }, descriptor_runtime=descriptor._runtime, # pylint: disable=protected-access rebind_noauth_module_to_user=rebind_noauth_module_to_user, diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 87f5f64a31a9..5a9e3639f02b 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -58,6 +58,7 @@ from xmodule.modulestore.tests.test_asides import AsideTestType # lint-amnesty, pylint: disable=wrong-import-order from xmodule.video_module import VideoBlock # lint-amnesty, pylint: disable=wrong-import-order from xmodule.x_module import STUDENT_VIEW, CombinedSystem, XModule, XModuleDescriptor # 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 GlobalStaffFactory from common.djangoapps.student.tests.factories import RequestFactoryNoCsrf @@ -2770,3 +2771,19 @@ def test_no_get_python_lib_zip(self): def test_cache(self): assert hasattr(self.runtime.cache, 'get') assert hasattr(self.runtime.cache, 'set') + + def test_replace_urls(self): + html = '' + assert self.runtime.replace_urls(html) == \ + static_replace.replace_static_urls(html, course_id=self.runtime.course_id) + + def test_replace_course_urls(self): + html = '' + assert self.runtime.replace_course_urls(html) == \ + static_replace.replace_course_urls(html, course_key=self.runtime.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.runtime.course_id), 'module_id': ''}) + assert self.runtime.replace_jump_to_id_urls(html) == \ + static_replace.replace_jump_to_id_urls(html, self.runtime.course_id, jump_to_id_base_url) diff --git a/lms/djangoapps/lms_xblock/test/test_runtime.py b/lms/djangoapps/lms_xblock/test/test_runtime.py index eed0279c7184..d85c9e5816ed 100644 --- a/lms/djangoapps/lms_xblock/test/test_runtime.py +++ b/lms/djangoapps/lms_xblock/test/test_runtime.py @@ -63,7 +63,6 @@ def setUp(self): static_url='/static', track_function=Mock(), get_module=Mock(), - replace_urls=str, course_id=self.course_key, user=Mock(), descriptor_runtime=Mock(), @@ -129,7 +128,6 @@ def setUp(self): static_url='/static', track_function=Mock(), get_module=Mock(), - replace_urls=str, user=self.user, course_id=self.course_id, descriptor_runtime=Mock(), @@ -180,7 +178,6 @@ def create_runtime(self): static_url='/static', track_function=Mock(), get_module=Mock(), - replace_urls=str, course_id=self.course_id, user=self.user, descriptor_runtime=Mock(), @@ -234,7 +231,6 @@ def setUp(self): static_url='/static', track_function=Mock(), get_module=Mock(), - replace_urls=str, course_id=self.course.id, user=Mock(), descriptor_runtime=Mock(), diff --git a/lms/djangoapps/mobile_api/course_info/views.py b/lms/djangoapps/mobile_api/course_info/views.py index fb0f91cc5961..3d8aff38ec78 100644 --- a/lms/djangoapps/mobile_api/course_info/views.py +++ b/lms/djangoapps/mobile_api/course_info/views.py @@ -111,9 +111,7 @@ def apply_wrappers_to_content(content, module, request): Returns: A piece of html content containing the original content updated by each wrapper. """ - content = module.system.replace_urls(content) - content = module.system.replace_course_urls(content) - content = module.system.replace_jump_to_id_urls(content) + content = module.system.service(module, "replace_urls").replace_urls(content) return make_static_urls_absolute(request, content) diff --git a/openedx/core/djangoapps/xblock/runtime/runtime.py b/openedx/core/djangoapps/xblock/runtime/runtime.py index 119c49c1bdf6..444ba3348878 100644 --- a/openedx/core/djangoapps/xblock/runtime/runtime.py +++ b/openedx/core/djangoapps/xblock/runtime/runtime.py @@ -25,6 +25,7 @@ from xmodule.modulestore.django import ModuleI18nService from xmodule.util.sandboxing import SandboxService from common.djangoapps.edxmako.services import MakoService +from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.track import contexts as track_contexts from common.djangoapps.track import views as track_views from common.djangoapps.xblock_django.user_service import DjangoXBlockUserService @@ -37,7 +38,6 @@ from openedx.core.djangoapps.xblock.utils import get_xblock_id_for_anonymous_user from openedx.core.lib.cache_utils import CacheService from openedx.core.lib.xblock_utils import wrap_fragment, xblock_local_resource_url -from common.djangoapps.static_replace import process_static_urls from .id_managers import OpaqueKeyReader from .shims import RuntimeShim, XBlockShim @@ -249,6 +249,8 @@ def service(self, block, service_name): return SandboxService(contentstore=contentstore, course_id=context_key) elif service_name == 'cache': return CacheService(cache) + elif service_name == 'replace_urls': + return ReplaceURLService(xblock=block, lookup_asset_url=self._lookup_asset_url) # Check if the XBlockRuntimeSystem wants to handle this: service = self.system.get_service(block, service_name) @@ -307,6 +309,7 @@ def render(self, block, view_name, context=None): # than public_view. They may call any handlers though. if (self.user is None or self.user.is_anonymous) and view_name != 'public_view': raise PermissionDenied + # We also need to override this method because some XBlocks in the # edx-platform codebase use methods like add_webpack_to_fragment() # which create relative URLs (/static/studio/bundles/webpack-foo.js). @@ -331,42 +334,13 @@ def render(self, block, view_name, context=None): # Apply any required transforms to the fragment. # We could move to doing this in wrap_xblock() and/or use an array of # wrapper methods like the ConfigurableFragmentWrapper mixin does. - fragment = wrap_fragment(fragment, self.transform_static_paths_to_urls(block, fragment.content)) + fragment = wrap_fragment( + fragment, + ReplaceURLService(xblock=block, lookup_asset_url=self._lookup_asset_url).replace_urls(fragment.content) + ) return fragment - def transform_static_paths_to_urls(self, block, html_str): - """ - Given an HTML string, replace any static file paths like - /static/foo.png - (which are really pointing to block-specific assets stored in blockstore) - with working absolute URLs like - https://s3.example.com/blockstore/bundle17/this-block/assets/324.png - See common/djangoapps/static_replace/__init__.py - - This is generally done automatically for the HTML rendered by XBlocks, - but if an XBlock wants to have correct URLs in data returned by its - handlers, the XBlock must call this API directly. - - Note that the paths are only replaced if they are in "quotes" such as if - they are an HTML attribute or JSON data value. Thus, to transform only a - single path string on its own, you must pass html_str=f'"{path}"' - """ - - def replace_static_url(original, prefix, quote, rest): # pylint: disable=unused-argument - """ - Replace a single matched url. - """ - original_url = prefix + rest - # Don't mess with things that end in '?raw' - if rest.endswith('?raw'): - new_url = original_url - else: - new_url = self._lookup_asset_url(block, rest) or original_url - return "".join([quote, new_url, quote]) - - return process_static_urls(html_str, replace_static_url) - def _lookup_asset_url(self, block, asset_path): # pylint: disable=unused-argument """ Return an absolute URL for the specified static asset file that may diff --git a/openedx/core/djangoapps/xblock/runtime/shims.py b/openedx/core/djangoapps/xblock/runtime/shims.py index e6b1d91da21f..38425a89230d 100644 --- a/openedx/core/djangoapps/xblock/runtime/shims.py +++ b/openedx/core/djangoapps/xblock/runtime/shims.py @@ -13,6 +13,7 @@ from openedx.core.djangoapps.xblock.apps import get_xblock_app_config +from common.djangoapps.static_replace.services import ReplaceURLService from common.djangoapps.edxmako.shortcuts import render_to_string from common.djangoapps.student.models import anonymous_id_for_user @@ -178,44 +179,35 @@ def process_xml(self, xml): def replace_urls(self, html_str): """ - Deprecated precursor to transform_static_paths_to_urls - - Given an HTML string, replace any static file paths like - /static/foo.png - (which are really pointing to block-specific assets stored in blockstore) - with working absolute URLs like - https://s3.example.com/blockstore/bundle17/this-block/assets/324.png - See common/djangoapps/static_replace/__init__.py - - This is generally done automatically for the HTML rendered by XBlocks, - but if an XBlock wants to have correct URLs in data returned by its - handlers, the XBlock must call this API directly. - - Note that the paths are only replaced if they are in "quotes" such as if - they are an HTML attribute or JSON data value. Thus, to transform only a - single path string on its own, you must pass html_str=f'"{path}"' + Deprecated in favor of the replace_urls service. """ - return self.transform_static_paths_to_urls(self._active_block, html_str) + 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): """ - Given an HTML string, replace any course-relative URLs like - /course/blah - with working URLs like - /course/:course_id/blah - See common/djangoapps/static_replace/__init__.py + Deprecated in favor of the replace_urls service. """ - # TODO: implement or deprecate. - # See also the version in openedx/core/lib/xblock_utils/__init__.py + 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): """ - Replace /jump_to_id/ URLs in the HTML with expanded versions. - See common/djangoapps/static_replace/__init__.py + Deprecated in favor of the replace_urls service. """ - # TODO: implement or deprecate. - # See also the version in openedx/core/lib/xblock_utils/__init__.py + warnings.warn( + 'replace_jump_to_id_urls is deprecated. Please use ReplaceURLService instead.', + DeprecationWarning, stacklevel=3, + ) return html_str @property diff --git a/openedx/core/lib/tests/test_xblock_utils.py b/openedx/core/lib/tests/test_xblock_utils.py index de9f6e206112..5f446e9561e3 100644 --- a/openedx/core/lib/tests/test_xblock_utils.py +++ b/openedx/core/lib/tests/test_xblock_utils.py @@ -22,9 +22,6 @@ from openedx.core.lib.xblock_utils import ( get_aside_from_xblock, is_xblock_aside, - replace_course_urls, - replace_jump_to_id_urls, - replace_static_urls, request_token, sanitize_html_id, wrap_fragment, @@ -119,64 +116,6 @@ def test_wrap_xblock(self, course_id, data_usage_id): assert test_wrap_output.resources[0].data == 'body {background-color:red;}' assert test_wrap_output.resources[1].data == 'alert("Hi!");' - @ddt.data('course_mongo', 'course_split') - def test_replace_jump_to_id_urls(self, course_id): - """ - Verify that the jump-to URL has been replaced. - """ - course = getattr(self, course_id) - test_replace = replace_jump_to_id_urls( - course_id=course.id, - jump_to_id_base_url='/base_url/', - block=course, - view='baseview', - frag=Fragment(''), - context=None - ) - assert isinstance(test_replace, Fragment) - assert test_replace.content == '' - - @ddt.data( - ('course_mongo', ''), - ('course_split', '') - ) - @ddt.unpack - def test_replace_course_urls(self, course_id, anchor_tag): - """ - Verify that the course URL has been replaced. - """ - course = getattr(self, course_id) - test_replace = replace_course_urls( - course_id=course.id, - block=course, - view='baseview', - frag=Fragment(''), - context=None - ) - assert isinstance(test_replace, Fragment) - assert test_replace.content == anchor_tag - - @ddt.data( - ('course_mongo', ''), - ('course_split', '') - ) - @ddt.unpack - def test_replace_static_urls(self, course_id, anchor_tag): - """ - Verify that the static URL has been replaced. - """ - course = getattr(self, course_id) - test_replace = replace_static_urls( - data_dir=None, - course_id=course.id, - block=course, - view='baseview', - frag=Fragment(''), - context=None - ) - assert isinstance(test_replace, Fragment) - assert test_replace.content == anchor_tag - def test_sanitize_html_id(self): """ Verify that colons and dashes are replaced. diff --git a/openedx/core/lib/xblock_utils/__init__.py b/openedx/core/lib/xblock_utils/__init__.py index bd93050700ce..e9b7a47ca7db 100644 --- a/openedx/core/lib/xblock_utils/__init__.py +++ b/openedx/core/lib/xblock_utils/__init__.py @@ -231,48 +231,6 @@ def wrap_xblock_aside( return wrap_fragment(frag, render_to_string('xblock_wrapper.html', template_context)) -def replace_jump_to_id_urls(course_id, jump_to_id_base_url, block, view, frag, context): # pylint: disable=unused-argument - """ - This will replace a link between courseware in the format - /jump_to_id/ with a URL for a page that will correctly redirect - This is similar to replace_course_urls, but much more flexible and - durable for Studio authored courses. See more comments in static_replace.replace_jump_to_urls - - course_id: The course_id in which this rewrite happens - jump_to_id_base_url: - A app-tier (e.g. LMS) absolute path to the base of the handler that will perform the - redirect. e.g. /courses////jump_to_id. NOTE the will be appended to - the end of this URL at re-write time - - output: a new :class:`~web_fragments.fragment.Fragment` that modifies `frag` with - content that has been update with /jump_to_id links replaced - """ - return wrap_fragment(frag, static_replace.replace_jump_to_id_urls(frag.content, course_id, jump_to_id_base_url)) - - -def replace_course_urls(course_id, block, view, frag, context): # pylint: disable=unused-argument - """ - Updates the supplied module with a new get_html function that wraps - the old get_html function and substitutes urls of the form /course/... - with urls that are /courses//... - """ - return wrap_fragment(frag, static_replace.replace_course_urls(frag.content, course_id)) - - -def replace_static_urls(data_dir, block, view, frag, context, course_id=None, static_asset_path=''): # pylint: disable=unused-argument - """ - Updates the supplied module with a new get_html function that wraps - the old get_html function and substitutes urls of the form /static/... - with urls that are /static//... - """ - return wrap_fragment(frag, static_replace.replace_static_urls( - frag.content, - data_dir, - course_id, - static_asset_path=static_asset_path - )) - - def grade_histogram(module_id): ''' Print out a histogram of grades on a given problem in staff member debug info. diff --git a/openedx/features/course_experience/course_updates.py b/openedx/features/course_experience/course_updates.py index 9eb1db96e84e..3539eb9cbab4 100644 --- a/openedx/features/course_experience/course_updates.py +++ b/openedx/features/course_experience/course_updates.py @@ -70,7 +70,7 @@ def get_ordered_updates(request, course): reverse=True ) for update in ordered_updates: - update['content'] = info_block.system.replace_urls(update['content']) + update['content'] = info_block.system.service(info_block, "replace_urls").replace_urls(update['content']) return ordered_updates diff --git a/openedx/features/course_experience/views/course_updates.py b/openedx/features/course_experience/views/course_updates.py index f3ee5de4ad98..f0acff801b0c 100644 --- a/openedx/features/course_experience/views/course_updates.py +++ b/openedx/features/course_experience/views/course_updates.py @@ -81,4 +81,6 @@ def get_plain_html_updates(self, request, course): # lint-amnesty, pylint: disa """ info_module = get_course_info_section_module(request, request.user, course, 'updates') info_block = getattr(info_module, '_xmodule', info_module) - return info_block.system.replace_urls(info_module.data) if info_module else '' + return info_block.system.service( + info_block, "replace_urls" + ).replace_urls(info_module.data) if info_module else ''