[BD-13] [BB-5363] refactor: deprecates replace url related properties from ModuleSystem #29880

kaustavb12 · 2022-02-07T09:37:34Z

Description

Builds on the shim added by #29190 to deprecate the following ModuleSystem attributes in favour of the added ReplaceURLService:

replace_urls,replace_course_urls,replace_jump_to_id_urls

This change should not affect any user of edx-platform

Supporting information

BD-5363 OpenCraft internal ticket

Testing instructions

LMS: https://pr29880.sandbox.opencraft.hosting/
Studio: https://studio.pr29880.sandbox.opencraft.hosting/

Studio:

Login as a demo staff ([email protected] / edx)
Open sample course Introduction to Edx
Navigate to Section1 > SubsectionA > HTML
Check the static urls in preview of HTML is correcly replaced
Navigate to the unit CAPA
Click on Hint button and verify urls are correctly replaced in the hints
Post a course update with static url and a handout and verify that the url is correctly replaced in the preview.

LMS:

Login as a demo staff ([email protected] / edx)
Open sample course Introduction to Edx
Verify the update and handout have correct urls
Click on start course
Verify all urls in the HTML xblock are correctly replaced.
Click Next to go to the CAPA unit and click on Hint
Verify the urls are correct in the hint section

Mobile API:

Setup mobile application using these instructions
Login to the App and navigate to a course
Click on the handouts, to check the handout link is pointing to correct url and handout is accessible

openedx-webhooks · 2022-02-07T09:37:39Z

Thanks for the pull request, @kaustavb12! I've created BLENDED-1088 to keep track of it in Jira. More details are on the BD-13 project page.

When this pull request is ready, tag your edX technical lead.

Agrendalath

@kaustavb12, could you please add testing instructions and prepare the sandbox?

openedx/core/lib/xblock_utils/__init__.py

common/djangoapps/static_replace/services.py

Agrendalath · 2022-02-16T14:44:39Z

common/lib/xmodule/xmodule/capa_module.py

+                hint_text=HTML(self.runtime.service(self, "replace_urls").replace_urls(
+                    get_inner_html_from_xpath(demand_hints[counter]),
+                    static_replace_only=True
+                ))


Is it the only place where static_replace_only is used? If yes, does it really make sense to keep it? Not replacing course links in the hints is an inconsistent behavior, as these links are replaced in answers.

static_replace_url is also used in studio preview

I introduced this argument, only to keep the logic consistent in these two places before and after this PR, where only the static urls were being replaced and not the course or jump-to-id urls.

In case of hints, I think replacing course or jump-to-id urls in the hints section could be useful in a scenario where the course author may want to provide a link to a portion of a course content in the assessment hints. Accordingly I have removed static_replace_only=True from here.

In the case of studio preview, I am not sure about the reasoning behind not replacing course urls (not replacing jump-to-id url make sense as the course is dynamic in the studio and the ids may change). Do you think, we should replace the course urls here as well ? In that case we can get rid of the static_replace_url argument.

@kaustavb12,

static_replace_url is also used in studio preview

Ah, that's right.

Do you think, we should replace the course urls here as well ? In that case we can get rid of the static_replace_url argument.

Right now Studio is just providing the {STUDIO_URL}/course/about URL. After this change, it would be pointing to {STUDIO_URL}/courses/{COURSE_ID/about. These pages do not exist in Studio anyway, so this won't make a too big difference. Let's leave it as-is, for consistency with the existing approach.

not replacing jump-to-id url make sense as the course is dynamic in the studio and the ids may change

It could be nice to be able to check within Studio that the ID is correct by navigating there. However, the implementation would probably need to change for the Studio preview (the URLs are different), so let's omit this.

common/lib/xmodule/xmodule/html_module.py

common/lib/xmodule/xmodule/tests/test_capa_module.py

Agrendalath · 2022-02-16T15:46:41Z

common/djangoapps/static_replace/services.py

+from common.djangoapps.static_replace import replace_course_urls, replace_jump_to_id_urls, replace_static_urls
+
+
+class ReplaceURLService(Service):


Given that we're adding this service, should we also deprecate these methods?
cc: @bradenmacdonald, as I'm not sure if these aren't used by the blockstore.

They're not called by blockstore but they are called by XBlocks, so yes.

@kaustavb12 Please make the same changes to

edx-platform/openedx/core/djangoapps/xblock/runtime/shims.py

Lines 179 to 219 in 25b275b

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}"'

"""

return self.transform_static_paths_to_urls(self._active_block, 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

"""

# TODO: implement or deprecate.

# See also the version in openedx/core/lib/xblock_utils/__init__.py

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

"""

# TODO: implement or deprecate.

# See also the version in openedx/core/lib/xblock_utils/__init__.py

return html_str

- make them print a deprecation warning and call the new service instead.

@bradenmacdonald @Agrendalath

These methods are part of the RuntimeShim class which is used with the XBlockRuntime class.
The XBlockRuntime class is a super class of the BlockstoreXBlockRuntime class

As I understand, blockstore Xblocks are not attached to courses, and so they don't have course id associated with them.

In that case would having replace_course_urls() and replace_jump_to_id_urls() methods in this shim necessary, since replacing both course urls and jump-to-id urls makes use of course ids.

@kaustavb12

As I understand, blockstore Xblocks are not attached to courses, and so they don't have course id associated with them.

That's true, and replace_course_urls is not needed for that reason. (At least not yet; blockstore will support courses in the future!)

But: blockstore XBlocks still use static files and they still need to have their /static/... asset paths rewritten. The difference is that in Blockstore, static assets are stored with the XBlock not with the course.

I actually created a new API for this, self.runtime.transform_static_paths_to_urls:

edx-platform/openedx/core/djangoapps/xblock/runtime/runtime.py

Lines 338 to 354 in 4f58ed4

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}"'

"""

And if you look at shims.py, you can see that the old APIs are just wrappers around the new API.

So, if you want this service to be the new API, you'll have to implement it for the blockstore runtime as well, and make sure that when it's called in the blockstore runtime it uses self.runtime.transform_static_paths_to_urls to do the rewriting, which will correctly rewrite the URLs to refer to assets from the XBlock's "bundle".

The easiest way to see what I'm talking about would be to set up blockstore yourself, e.g. following these instructions and then create within blockstore an HTML block, and upload an image to the HTML block, then set the image src to /static/image.png and make sure it's working.

Let me know if you want more help!

@bradenmacdonald, I'm copying the message from @kaustavb12 here.

I have used the ReplaceURLService directly here instead of adding the service in the service() method. This is because, for invoking the service from this method we need to add a declaration for each Xblock in the form of @XBlock.needs('mako'). However, I couldn't figure out the correct classes for xblocks like Completion, Drag and Drop, Feedback, Survey, etc.

If you think the correct implementation should have been through the service() method, then I will look into in further later in the sprint.

As I don't know what this change means for blockstore, I could use a second pair of eyes to check the approach from c1c323ce08ac200955e921798039a014688bb264 when you have a moment. For now, we don't need to use the attributes of ReplaceURLService here, so we could make ReplaceURLService.replace_block_urls a static method to start using it as an API. If we are going to support courses with blockstore in the future, then we could expand the service to support them, and merge replace_static_block_urls with replace_static_urls within static_replace/__init__.py. Would it make sense?

@Agrendalath Sure, I left my comments on c1c323ce08ac200955e921798039a014688bb264

@bradenmacdonald
Thanks a lot pointing me to the instructions to setup blockstore in devstack and for reviewing the changes. It really helped me gain a lot of context about xblocks and the blockstore runtime.

I have now incorporated all your suggestions, and now there is only one API in ReplaceURLService which can be called by XBlocks as needed in the form of self.runtime.service(self, 'replace_urls').replace_urls(text)

@bradenmacdonald

Also I have gone ahead and merged the new replace_static_block_urls function with the replace_static_urls function in static_replace/init.py similar to what was done for ReplaceURLService to avoid code duplication

cc. @Agrendalath

Thanks @kaustavb12, those changes look great :)

common/djangoapps/static_replace/services.py

Agrendalath

👍

I tested this: tested that replacing URLs in XBlocks works correctly with the new service
I read through the code
I checked for accessibility issues: n/a
Includes documentation: n/a
I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository: n/a

Agrendalath · 2022-03-10T19:00:19Z

@ormsbee, would you like to take a look before we merge this?

ormsbee · 2022-03-11T05:19:46Z

@Agrendalath: As long as it has @bradenmacdonald's blessing, I'm fine with not reviewing it. Please arrange the timing of the merge with @jristau1984, @connorhaugh, or @doctoryes though–probably for Monday or Tuesday? In general, these deep-in-the-modulestore changes have a higher risk of introducing regressions with the wide array of interesting content data that edX has in its catalog.

Deprecates the following attributes from ModuleSystem: * replace_urls * replace_course_urls * replace_jump_to_id_urls A new ReplaceURLService is created as replacement with a unified replace_urls method

openedx-webhooks · 2022-03-14T13:35:47Z

@kaustavb12 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

edx-pipeline-bot · 2022-03-14T15:04:30Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2022-03-14T16:04:14Z

EdX Release Notice: This PR has been deployed to the production environment.

`ReplaceURLService` replaced deprecated runtime methods in Nutmeg: openedx/edx-platform#29880

openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Feb 7, 2022

kaustavb12 force-pushed the kaustav/bd_13_replace_urls branch from a37f47d to 8a7baa2 Compare February 10, 2022 14:20

Agrendalath reviewed Feb 16, 2022

View reviewed changes

Agrendalath force-pushed the kaustav/bd_13_replace_urls branch from dda52d2 to e5c75ae Compare February 21, 2022 19:47

kaustavb12 force-pushed the kaustav/bd_13_replace_urls branch 6 times, most recently from e727391 to 8fb0a90 Compare March 5, 2022 18:16

Agrendalath reviewed Mar 7, 2022

View reviewed changes

common/djangoapps/static_replace/services.py Show resolved Hide resolved

Agrendalath approved these changes Mar 10, 2022

View reviewed changes

refactor: deprecates replace url related properties from ModuleSystem

cfdf6c7

Deprecates the following attributes from ModuleSystem: * replace_urls * replace_course_urls * replace_jump_to_id_urls A new ReplaceURLService is created as replacement with a unified replace_urls method

kaustavb12 force-pushed the kaustav/bd_13_replace_urls branch from 8fb0a90 to cfdf6c7 Compare March 11, 2022 14:07

doctoryes merged commit c48c655 into openedx:master Mar 14, 2022

openedx-webhooks added merged and removed needs triage labels Mar 14, 2022

Agrendalath deleted the kaustav/bd_13_replace_urls branch March 14, 2022 20:53

Agrendalath added a commit to abodacs/xblock-drag-and-drop-v2 that referenced this pull request Oct 12, 2022

fix: use ReplaceURLService and handle exceptions in explanations

27cc6cc

`ReplaceURLService` replaced deprecated runtime methods in Nutmeg: openedx/edx-platform#29880

		from common.djangoapps.static_replace import replace_course_urls, replace_jump_to_id_urls, replace_static_urls


		class ReplaceURLService(Service):

	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}"'
	"""
	return self.transform_static_paths_to_urls(self._active_block, 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
	"""
	# TODO: implement or deprecate.
	# See also the version in openedx/core/lib/xblock_utils/__init__.py
	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
	"""
	# TODO: implement or deprecate.
	# See also the version in openedx/core/lib/xblock_utils/__init__.py
	return html_str

	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}"'
	"""

[BD-13] [BB-5363] refactor: deprecates replace url related properties from ModuleSystem #29880

[BD-13] [BB-5363] refactor: deprecates replace url related properties from ModuleSystem #29880

Uh oh!

Conversation

kaustavb12 commented Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Supporting information

Testing instructions

Uh oh!

openedx-webhooks commented Feb 7, 2022

Uh oh!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaustavb12 Feb 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaustavb12 Mar 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Agrendalath left a comment

Choose a reason for hiding this comment

Uh oh!

Agrendalath commented Mar 10, 2022

Uh oh!

ormsbee commented Mar 11, 2022

Uh oh!

openedx-webhooks commented Mar 14, 2022

Uh oh!

edx-pipeline-bot commented Mar 14, 2022

Uh oh!

edx-pipeline-bot commented Mar 14, 2022

Uh oh!

Uh oh!

kaustavb12 commented Feb 7, 2022 •

edited

Loading

kaustavb12 Feb 18, 2022 •

edited

Loading

kaustavb12 Mar 5, 2022 •

edited

Loading