fix: allow to preview images #1403

Ian2012 · 2024-10-18T14:55:07Z

Description

This PR allows to preview images on the text editor for Libraries.

Supporting information

Closes #1358
Depends on openedx/edx-platform#35681

Testing instructions

Please provide detailed step-by-step instructions for testing this change.

Screenshots

openedx-webhooks · 2024-10-18T14:55:15Z

Thanks for the pull request, @Ian2012!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/2u-tnl. Tag them in a comment and let them know that your changes are ready for review.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

src/editors/containers/TextEditor/index.jsx

codecov · 2024-10-21T18:01:10Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.16%. Comparing base (11470f2) to head (9bb02c6).
Report is 4 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1403      +/-   ##
==========================================
+ Coverage   93.15%   93.16%   +0.01%     
==========================================
  Files        1051     1053       +2     
  Lines       20328    20413      +85     
  Branches     4335     4280      -55     
==========================================
+ Hits        18936    19018      +82     
- Misses       1332     1335       +3     
  Partials       60       60

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/editors/sharedComponents/TinyMceWidget/hooks.js

ormsbee · 2024-10-22T17:22:22Z

src/editors/sharedComponents/TinyMceWidget/hooks.js

+        // This function doesn't currently know how to deal with Library assets.
+        // Libraries don't mangle the path into an asset key–it might be sufficient
+        // to remove the initial "/" in a "/static/images/foo.png" link, and then
+        // set the base URL to the correct ComponentVersion base. If we let this
+        // function try to deal with Library assets, it would convert them in such a
+        // way that it wouldn't convert back later on, and we'd end up storing the
+        // incorrect OLX and breaking the display from that point forward.
+        //
+        // So until we handle it better, just disable static asset URL substitutions
+        // when dealing with Library content.


This is no longer accurate, right? Now that your code is dealing with the case properly?

Yes, I'll update this comment with the details of the solution

src/editors/sharedComponents/TinyMceWidget/hooks.js

ormsbee · 2024-10-22T17:41:33Z

I think the code looks fine. I defer to you folks on whether this is impractical to test because of TinyMCE. I'll approve and merge as soon as the comments are updated, unless @bradenmacdonald has additional feedback to give.

bradenmacdonald · 2024-10-22T17:45:18Z

I'm gonna test this in a few minutes.

bradenmacdonald · 2024-10-22T18:15:23Z

✅ This works well for existing images, but I don't see any option to upload a new image. Was that out of scope?

bradenmacdonald · 2024-10-22T18:27:59Z

src/editors/sharedComponents/TinyMceWidget/hooks.js

@@ -261,9 +262,12 @@ export const editorConfig = ({
  content,
  minHeight,
  learningContextId,
+  documentURL,


documentUrl doesn't mean much to me. staticRootUrl would be more clear

bradenmacdonald · 2024-10-22T18:28:37Z

LGTM, no blockers from me. Just needs updated comments.

bradenmacdonald · 2024-10-22T21:12:38Z

@Ian2012 @ormsbee Not sure what's wrong yet, but this PR is deployed to the sandbox, yet it's not working on there:

https://app.tagging-preview.staging.do.opencraft.hosting/course-authoring/library/lib:OpenCraft:TL/components

The exact commit running on the sandbox at the moment is https://github.com/open-craft/frontend-app-authoring/commits/taxonomy-sandbox-20241022/ if that helps.

The URL should be rewritten to https://cms.tagging-preview.staging.do.opencraft.hosting/library_assets/blocks/lb:OpenCraft:TL:html:cc695559-2cff-43ab-a463-02346b7ab9b8/static/deer.jpg which is working

ormsbee · 2024-10-22T21:19:19Z

@bradenmacdonald: The usage key is undefined. It shows it trying to load this for me: https://cms.tagging-preview.staging.do.opencraft.hosting/library_assets/blocks/undefined/static/deer.jpg

bradenmacdonald · 2024-10-22T21:22:12Z

@ormsbee Ah, hmm, you're right. Now I see it has to do with a recent refactor on master such that sidebarComponentUsageKey is no longer available from the context.

Though we have to fix it anyways because even without that conflict, if you use the "Edit" menu to open the editor without opening the sidebar, you get the same problem:

src/editors/containers/TextEditor/index.jsx

fix: dinamically get the component UUID chore: quality fixes chore: quality fixes chore: quality fixes chore: quality fixes fix: add optional library context hook chore: update comment fix: add blockID parameter to textEditor

pomegranited · 2024-10-24T06:28:04Z

@bradenmacdonald @ormsbee CC @Ian2012 This PR is deployed and working on the tagging sandbox, however:

The user must be explicitly logged in to the CMS or they get a 403 when trying to view these assets. See e.g deer.jpg.

So I guess we need to proxy these requests via the MFE, so the JWT tokens can be used for auth? Will have to do the same in the Learning MFE too. Raised #1434.

Also -- the sandbox seems to have a really small maximum file size that it'll allow uploads for. I didn't find the exact limit, but it looks like >100k won't upload, and silently fails.

ormsbee · 2024-10-24T12:37:54Z

The user must be explicitly logged in to the CMS or they get a 403 when trying to view these assets. See e.g deer.jpg.

So I guess we need to proxy these requests via the MFE, so the JWT tokens can be used for auth? Will have to do the same in the Learning MFE too. Raised #1434.

But they have to be logged into CMS for the XBlock preview to work anyway, right?

Also, we don't need to invoke any of this from the Learning MFE for a little while, since assets are copied to contentstore before they're served to anyone in the LMS.

pomegranited · 2024-10-25T00:43:08Z

@ormsbee

we don't need to invoke any of this from the Learning MFE for a little while, since assets are copied to contentstore before they're served to anyone in the LMS.

Ah and -- whenever you're in a MFE, you're logged into the LMS anyway, so that part of my comment was mistaken.

But they have to be logged into CMS for the XBlock preview to work anyway, right?

The preview iframe works fine for some reason.. but the image doesn't load. Have added testing instructions to #1434.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 18, 2024

Ian2012 force-pushed the cag/library-image-preview branch 4 times, most recently from ca9c6f1 to 68a673c Compare October 18, 2024 15:41

Ian2012 commented Oct 18, 2024

View reviewed changes

src/editors/containers/TextEditor/index.jsx Outdated Show resolved Hide resolved

Ian2012 mentioned this pull request Oct 21, 2024

feat: add endpoint to get draft version of a library component asset openedx/edx-platform#35681

Merged

Ian2012 force-pushed the cag/library-image-preview branch 4 times, most recently from 494f6d2 to 6a52f3c Compare October 21, 2024 17:48

Ian2012 marked this pull request as ready for review October 21, 2024 18:14

Ian2012 requested a review from a team as a code owner October 21, 2024 18:14

Ian2012 force-pushed the cag/library-image-preview branch 3 times, most recently from 9ffa8d0 to b55811d Compare October 22, 2024 15:11

Ian2012 requested review from ormsbee and bradenmacdonald October 22, 2024 15:24

ormsbee reviewed Oct 22, 2024

View reviewed changes

src/editors/sharedComponents/TinyMceWidget/hooks.js Show resolved Hide resolved

ormsbee reviewed Oct 22, 2024

View reviewed changes

src/editors/sharedComponents/TinyMceWidget/hooks.js Show resolved Hide resolved

bradenmacdonald reviewed Oct 22, 2024

View reviewed changes

src/editors/containers/TextEditor/index.jsx Outdated Show resolved Hide resolved

Ian2012 force-pushed the cag/library-image-preview branch 5 times, most recently from 95e7014 to 042c4ba Compare October 22, 2024 22:28

ormsbee mentioned this pull request Oct 23, 2024

Support static assets when copy/pasting between courses and libraries openedx/edx-platform#35668

Merged

fix: allow to preview images

d64bfa5

fix: dinamically get the component UUID chore: quality fixes chore: quality fixes chore: quality fixes chore: quality fixes fix: add optional library context hook chore: update comment fix: add blockID parameter to textEditor

Ian2012 force-pushed the cag/library-image-preview branch 2 times, most recently from 47000cb to 9e8fe74 Compare October 23, 2024 14:20

bradenmacdonald added a commit to open-craft/frontend-app-authoring that referenced this pull request Oct 23, 2024

Merge fix: allow to preview images openedx#1403

ad433c5

Ian2012 force-pushed the cag/library-image-preview branch 2 times, most recently from 4d013d7 to 3d1ec42 Compare October 23, 2024 21:32

test: add blockId to mock data

9bb02c6

Ian2012 force-pushed the cag/library-image-preview branch from 3d1ec42 to 9bb02c6 Compare October 23, 2024 21:33

ormsbee approved these changes Oct 24, 2024

View reviewed changes

ormsbee merged commit e1ce3eb into openedx:master Oct 24, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow to preview images #1403

fix: allow to preview images #1403

Ian2012 commented Oct 18, 2024 •

edited

Loading

openedx-webhooks commented Oct 18, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

ormsbee Oct 22, 2024

Ian2012 Oct 22, 2024

ormsbee commented Oct 22, 2024

bradenmacdonald commented Oct 22, 2024

bradenmacdonald commented Oct 22, 2024

bradenmacdonald Oct 22, 2024

bradenmacdonald commented Oct 22, 2024

bradenmacdonald commented Oct 22, 2024 •

edited

Loading

ormsbee commented Oct 22, 2024

bradenmacdonald commented Oct 22, 2024

pomegranited commented Oct 24, 2024 •

edited

Loading

ormsbee commented Oct 24, 2024

pomegranited commented Oct 25, 2024

fix: allow to preview images #1403

fix: allow to preview images #1403

Conversation

Ian2012 commented Oct 18, 2024 • edited Loading

Description

Supporting information

Testing instructions

Screenshots

openedx-webhooks commented Oct 18, 2024 • edited Loading

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

ormsbee Oct 22, 2024

Choose a reason for hiding this comment

Ian2012 Oct 22, 2024

Choose a reason for hiding this comment

ormsbee commented Oct 22, 2024

bradenmacdonald commented Oct 22, 2024

bradenmacdonald commented Oct 22, 2024

bradenmacdonald Oct 22, 2024

Choose a reason for hiding this comment

bradenmacdonald commented Oct 22, 2024

bradenmacdonald commented Oct 22, 2024 • edited Loading

ormsbee commented Oct 22, 2024

bradenmacdonald commented Oct 22, 2024

pomegranited commented Oct 24, 2024 • edited Loading

ormsbee commented Oct 24, 2024

pomegranited commented Oct 25, 2024

Ian2012 commented Oct 18, 2024 •

edited

Loading

openedx-webhooks commented Oct 18, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

bradenmacdonald commented Oct 22, 2024 •

edited

Loading

pomegranited commented Oct 24, 2024 •

edited

Loading