fix: update static url for image and link by leangseu-edx · Pull Request #1794 · openedx/edx-ora2

leangseu-edx · 2022-01-26T15:19:33Z

TL;DR - correctly update static url for image and link

JIRA: AU-490

What changed?

Modify static url on insert/edit image and anchor link (update logic)
Modify static url on save contents (getcontent in this case)
Remove Sanitize url on init because it would be redundant with on save.

Developer Checklist

Reviewed the release process
Translations and JS/SASS compiled
Bumped version number in setup.py and package.json

Testing Instructions

Follow the jira ticket instruction

Reviewer Checklist

Collectively, these should be completed by reviewers of this PR:

I've done a visual code review
I've tested the new functionality

FYI: @edx/masters-devs-gta

codecov · 2022-01-26T15:25:01Z

Codecov Report

Merging #1794 (ec4f1c5) into master (b7dd0c0) will not change coverage.
The diff coverage is n/a.

❗ Current head ec4f1c5 differs from pull request most recent head d5d2be8. Consider uploading reports for the commit d5d2be8 to get more accurate results

@@           Coverage Diff           @@
##           master    #1794   +/-   ##
=======================================
  Coverage   93.91%   93.91%           
=======================================
  Files         151      151           
  Lines       16592    16592           
  Branches     1828     1828           
=======================================
  Hits        15583    15583           
  Misses        807      807           
  Partials      202      202

Flag	Coverage Δ
unittests	`93.91% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7dd0c0...d5d2be8. Read the comment docs.

nsprenkle · 2022-01-27T17:53:26Z

+
+  const sanitizeContent = (content) => {
+    // rewrite anchor link
+    let newContent = rewriteStaticLinks(


Where does rewriteStaticLinks come from?

edx-platform a global js function

looks like here: https://github.com/openedx/edx-platform/blob/db32ff2cdf678fa8edd12c9da76a76eef0478614/common/static/js/src/utility.js#L21

matthugs · 2022-01-27T21:48:20Z

+      `<a href="${staticUrl}`,
+      `<a href="${baseAssetUrl}`,
+    );
+
+    // rewrite image src
+    newContent = rewriteStaticLinks(
+      newContent,
+      `<img src="${staticUrl}`,
+      `<img src="${baseAssetUrl}`,


This is a bit of a hack, which would fail on html like, say <img class="w-50" src="..." />

Why not use this the same way we do elsewhere in this library?

edx-ora2/openassessment/xblock/static/js/src/studio/oa_tiny_mce.js

Line 42 in dd6adea

ed.setContent(rewriteStaticLinks(

getContent is actually get everything in html. If the user put anything even text that has /static/ will get replace. It can be quite a confusing experience. This is just an improvement to what it was.

For image tag, it always generate <img src="....." alt .... />. I tested with multiple iteration of input.

Ok, so it sounds like we only support the rewriting of static links if folks insert the image via the WYSIWYG widget for adding images, and we don't rewrite static links if the user types html manually. Is that right?

If I have that right, I think a comment explaining what you've explained here would be helpful. (I would also urge you to make the comment longer than you'd consider necessary.)

Could you expand your comment to include the assumptions you expressed in this thread? Especially that the specific placement of src or href is assured in image or anchor tags generated via the WYSIWYG editor.

I am encouraging you to document these assumptions because the behavior of other code may change in the future, causing this assumption to no longer be true.

matthugs · 2022-01-28T17:55:44Z

  const baseAssetUrl = options.base_asset_url;
+  const staticUrl = '/static';
+
+  const sanitizeContent = (content) => {


I would recommend choosing a different name for this function. sanitize is a word often used specifically for security-related changes to input. The use of that word in this context could easily give the wrong idea to the reader of your code.

matthugs · 2022-01-28T18:09:08Z

+      `<a href="${staticUrl}`,
+      `<a href="${baseAssetUrl}`,
+    );
+
+    // rewrite image src
+    newContent = rewriteStaticLinks(
+      newContent,
+      `<img src="${staticUrl}`,
+      `<img src="${baseAssetUrl}`,


Could you expand your comment to include the assumptions you expressed in this thread? Especially that the specific placement of src or href is assured in image or anchor tags generated via the WYSIWYG editor.

I am encouraging you to document these assumptions because the behavior of other code may change in the future, causing this assumption to no longer be true.

matthugs · 2022-02-02T19:55:30Z

+    // These url are generated by WYSIWYG editor, it is assumed to be formatted
+    // <a href='.....' >...</a> for link.


I think a future reader of this code would be able to read the lines below it and know that this code would only work for things formatted as in the example in this comment. So they can probably figure out what your comment tells them, even without you having commented it.

The part that is most non-obvious about this code is why anchor tags (and image tags, below) can safely be assumed to be in that format. Why that is is probably the most important thing to include in a comment here.

- update url on insert/edit and save. It is less confusing than doing it on init. chore: update comments for sanitize content method chore: update comments and method name chore: update translation chore: version bump

fix: restore original tinymce behavior, fix trailing slash In PR #1512 we introduced the bug by improperly combining the "dataHandler" methods into one method and only setting/checking src rather than also including href In PR #1794 we attempted to fix the issue by fixing the order of the to->from in the dataHandler. We also did a more specific replacement of <a href=<url> and <img src=<url> This fixed the issue of inserting bad links and images, but the static url const used in the "load all content" transform was missing a slash resulting in incorrect "resolved" URLs. Thankfully according to the read replica, there are only 3 of those that currently exist. Removing the handlers for EditImage and EditLink. Originally their purpose was so that you would only ever see the "static" link. When the image/link was saved, we'd convert it from the static link to the real link, and then when you would click "edit" we'd transition back from the real link to the "static" link. I'm getting rid of that. The URL presented in the edit dialog should actually be the URL and having it go back and forth hides the actual real value of the URL. Unnecessary and potentially confusing. Removing the specific <a href=<url> and <img src=<url> filtering for the full content transform. It's nice, and it will potentially prevent issues where we have a /static/ that we don't want transformed, but unfortunately having the global replace is the current only way we can support resolving static URLs in audio or video embeds. (see https://openedx.atlassian.net/browse/AU-544) Rather than adding those specific "special cases" it seems like it's probably just easier to do the more general resolve?

leangseu-edx requested a review from a team January 26, 2022 15:20

leangseu-edx requested a review from matthugs January 27, 2022 17:25

nsprenkle reviewed Jan 27, 2022

View reviewed changes

leangseu-edx requested a review from nsprenkle January 27, 2022 19:17

matthugs reviewed Jan 27, 2022

View reviewed changes

matthugs approved these changes Jan 28, 2022

View reviewed changes

leangseu-edx force-pushed the leangseu-edx/au-490 branch 2 times, most recently from ec4f1c5 to 7a8cfa6 Compare February 2, 2022 17:45

leangseu-edx requested a review from jansenk February 2, 2022 19:39

jansenk approved these changes Feb 2, 2022

View reviewed changes

matthugs reviewed Feb 2, 2022

View reviewed changes

fix: update static url for image and link

d5d2be8

- update url on insert/edit and save. It is less confusing than doing it on init. chore: update comments for sanitize content method chore: update comments and method name chore: update translation chore: version bump

leangseu-edx force-pushed the leangseu-edx/au-490 branch from 7a8cfa6 to d5d2be8 Compare February 2, 2022 20:03

leangseu-edx merged commit 68a8ba0 into master Feb 2, 2022

leangseu-edx deleted the leangseu-edx/au-490 branch February 2, 2022 20:50

jansenk mentioned this pull request Feb 24, 2022

fix: restore original tinymce behavior, fix trailing slash #1818

Merged

8 tasks

Navigation Menu

Uh oh!

fix: update static url for image and link - #1794

fix: update static url for image and link#1794
leangseu-edx merged 1 commit into
masterfrom
leangseu-edx/au-490

leangseu-edx commented Jan 26, 2022 •

edited

Loading

Uh oh!

codecov Bot commented Jan 26, 2022 •

edited

Loading

Uh oh!

nsprenkle Jan 27, 2022

Uh oh!

leangseu-edx Jan 27, 2022

Uh oh!

matthugs Jan 27, 2022

Uh oh!

matthugs Jan 27, 2022

Uh oh!

leangseu-edx Jan 27, 2022 •

edited

Loading

Uh oh!

matthugs Jan 28, 2022

Uh oh!

matthugs Jan 28, 2022

Uh oh!

Uh oh!

matthugs Jan 28, 2022

Uh oh!

matthugs Jan 28, 2022

Uh oh!

Uh oh!

matthugs Feb 2, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		// These url are generated by WYSIWYG editor, it is assumed to be formatted
		// <a href='.....' >...</a> for link.

Uh oh!

Conversation

leangseu-edx commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

leangseu-edx Jan 27, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthugs Feb 2, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

leangseu-edx commented Jan 26, 2022 •

edited

Loading

codecov Bot commented Jan 26, 2022 •

edited

Loading

leangseu-edx Jan 27, 2022 •

edited

Loading

matthugs Feb 2, 2022 •

edited

Loading