Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

feat!: remove block handling from runtime initialization of ReplaceURLService

Browse Source 
BREAKING CHANGE: This removes the following deprecated shims from the runtime:
`replace_urls`, `replace_course_urls`, `replace_jump_to_id_urls`. XBlocks need
to use the `replace_urls` service instead.
This commit is contained in:
Agrendalath
2023-06-05 16:31:22 +02:00
committed by Piotr Surowiec
parent a903230a74
commit 71fee4a4a0
9 changed files with 70 additions and 147 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
cms/djangoapps/contentstore/views/preview.py
View File

@@ -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)

2
cms/djangoapps/contentstore/views/tests/test_preview.py
View File

@@ -292,7 +292,7 @@ class CmsModuleSystemShimTest(ModuleStoreTestCase):
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):

24
common/djangoapps/static_replace/services.py
View File

@@ -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

66
common/djangoapps/static_replace/test/test_static_replace.py
View File

@@ -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('<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">'

4
common/djangoapps/static_replace/wrapper.py
View File

@@ -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))

19
lms/djangoapps/courseware/block_render.py
View File

@@ -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

17
lms/djangoapps/courseware/tests/test_block_render.py
View File

@@ -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 = '<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):

34
openedx/core/djangoapps/xblock/runtime/shims.py
View File

@@ -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):
"""

45
xmodule/x_module.py
View File

@@ -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):
"""