[SE-4650] OEP-15 - add support for course-wide custom resources #581

ha-D · 2021-08-11T09:59:43Z

Description

Details about the purpose of this PR can be found in edx-platform#28411.

In this PR we're just taking the course-wide JS and CSS URLs from the course metadata and adding them to then pager <head>.

Testing Instructions

Run a devstack and checkout both edx-platform and frontend-app-learning repo to the hadi/se-4650/oep-15 branch
Follow the testing instructions edx-platform#28411 to add course-wide scripts to the course via the studio
Visit a course page, check that the scripts are added to the page head

(If there's a simple way for me to get the Learning MFE running on the OCIM sandbox in edx-platform#28411 let me know and I could do that instead, I didn't want to spend too much time seeing whether I could do that or not)

Reviewers

@edx/engage-squad

openedx-webhooks · 2021-08-11T09:59:47Z

Thanks for the pull request, @ha-D! I've created OSPR-5960 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

nizarmah

Hello @ha-D! Nice work on this as well. 👍🏼

I've read through the changes and I have a small nit.

Other than that, I'm building a new app server for the sandbox instance where the frontend-app-learning MFE is enabled.

I'll update my review once the new app server is ready for me to test.

nizarmah · 2021-08-11T22:08:03Z

src/tab-page/LoadedTabPage.jsx

@@ -46,6 +48,8 @@ function LoadedTabPage({
    <>
      <Helmet>
        <title>{`${activeTab ? `${activeTab.title} | ` : ''}${title} | ${getConfig().SITE_NAME}`}</title>
+        {courseWideJs && courseWideJs.map(js => <script key={js} src={js} />)}


Small nit: would you mind please adding the script type, type="text/javascript"?

nizarmah

@ha-D thanks for addressing the comment I had 👍🏼 I've setup the frontend-app-learning MFE on the sandbox instance.

I've also tested that things are working as expected there as well.

I had some trouble with the toastr library. I didn't end up investigating it; instead, I added an alert dialog.

LGTM though. I'll pass the review over to @bradenmacdonald.

natabene · 2021-08-18T14:53:49Z

@ha-D Thank you for your contribution. Please let me know once it is ready for our review.

bradenmacdonald · 2021-08-18T21:13:41Z

@natabene I think this is ready for review now, see comment above.

Although I was asked to review, I'm not a core committer on this repo nor super up to speed with it, so I think we'll need you to find us a reviewer.

The code seems simple and straightforward though :)

natabene · 2021-08-23T16:12:28Z

@BbrSofiane Would you like to review as a core committer for frontend-app-learning?

mikix · 2021-08-24T17:55:13Z

src/tab-page/LoadedTabPage.jsx

+        {courseWideJs && courseWideJs.map(js => <script key={js} type="text/javascript" src={js} />)}
+        {courseWideCss && courseWideCss.map(css => <link key={css} rel="stylesheet" href={css} />)}


I'm very leery of adding arbitrary css/js to the react side of things. The MFE is fast moving and doesn't want to have to make guarantees about DOM structure or anything like that.

For example, when we added the course-welcome and course-handouts features to the MFE, we "sandboxed" that arbitrary html/js inside iframes.

Can we do this asset loading inside the courseware iframe, to better isolate it?

Oh I'm sorry - I'm reading the part of the platform PR that says this is explicitly your point - you want them in the parent page too.

I'm chatting with folks that are bigger frontend experts at edX - but this whole arbitrary JS approach is very much not where we are trying to heading with the MFEs. I'm trying to see what the vision for this kind of use case is.

And your use case is basically loading some external libraries that do page-wide stuff, not trying to manipulate any specific DOM structures, yeah? Because that would definitely be a problem, given how React works.

But maybe we can find a more future-proof way to inject external libraries...

@mkikix Glad to demo some of our use cases if it would help.

jmyatt

we will not be able to approve this approach. As @mikix mentioned, this works around a core edX MFE principle of sound boundary definitions.

ha-D · 2021-08-25T12:19:09Z

All right, thats understandable. We can assume the Course-wide JS/CSS should only be targeted towards the XBlocks and course material (which is handled in this PR) and not the MFE.

As for use cases, I can think of a couple examples where having something like this in the MFE would have been useful to clients:

Adding things like chatbots to all pages
Adding a client's preferred analytics service to their OpenEdx deployment

bradenmacdonald · 2021-09-02T15:40:35Z

@ha-D @jmyatt What about changing this approach to only add the JS within the iframe? Surely having that use case is better than nothing, and then there's no interference with React.

And it provides similar capabilities to the current situation, which sees authors doing things like adding HTML blocks (with the custom JS) to every unit - which has the same effect but is a giant pain to manage.

ha-D · 2021-09-02T16:55:06Z

@bradenmacdonald Yeah I agree, it's what I intended as well. It wouldn't need any changes on the MFE side though, the content rendered in the iframes should already have the JS included with the changes in the edx-platform PR, since it's using the same base templates that were changed there.

mikix · 2021-09-02T17:23:19Z

Yeah from an MFE perspective, loading the JS in the xblock iframe seems fine - that's similar to existing patterns and security boundaries.

I think medium to long term, there might need to be a discussion around a modern version of OEP-15. But in the meantime, having this JS loaded inside the xblock iframe seems reasonable (and isn't anything adding any new risk, right? - course teams can inject arbitrary JS via html xblocks - this is just a super easy way to do it across the course, yeah?)

Which would mean we can close this MFE-side PR?

Colin-Fredericks · 2021-09-02T20:36:42Z

having this JS loaded inside the xblock iframe seems reasonable (and isn't anything adding any new risk, right? - course teams can inject arbitrary JS via html xblocks - this is just a super easy way to do it across the course, yeah?)

@mikix That's correct - this will replicate existing functionality but with a centralized file reference. Not as useful as the original proposal, but still an improvement.

Presumably we'll have to keep adding JS to custom tabs by hand, unless those are secretly XBlocks these days. At least that's fewer files.

We used to be able to access the outer frame for the course, but the "new user experience" broke that and we had to pull a bunch of course navigation walkthroughs.

natabene · 2021-09-08T19:23:57Z

Owning team believes that, unfortunately, the only thing to do with this PR is to close it without merging. That latest comment seems like more of a Product question, and not something that this PR addresses.

openedx-webhooks · 2021-09-14T19:57:25Z

@ha-D Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

feat: add course-wide scripts/styles to page head

8328df1

openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Aug 11, 2021

ha-D mentioned this pull request Aug 11, 2021

[SE-4650] OEP-15 - add support for course-wide custom resources openedx/edx-platform#28411

Merged

2 tasks

nizarmah reviewed Aug 11, 2021

View reviewed changes

fixup! feat: add course-wide scripts/styles to page head

ae577c8

nizarmah approved these changes Aug 14, 2021

View reviewed changes

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Aug 18, 2021

mikix reviewed Aug 24, 2021

View reviewed changes

jmyatt suggested changes Aug 24, 2021

View reviewed changes

openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Sep 2, 2021

ha-D closed this Sep 8, 2021

openedx-webhooks added rejected and removed engineering review labels Sep 8, 2021

arbrandes mentioned this pull request Nov 17, 2022

Create a plug-in API to make it easier to modify or improve MFEs without forking openedx/axim-engineering#430

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SE-4650] OEP-15 - add support for course-wide custom resources #581

[SE-4650] OEP-15 - add support for course-wide custom resources #581

ha-D commented Aug 11, 2021 •

edited

Loading

openedx-webhooks commented Aug 11, 2021 •

edited

Loading

nizarmah left a comment •

edited

Loading

nizarmah Aug 11, 2021

ha-D Aug 12, 2021

nizarmah left a comment

natabene commented Aug 18, 2021

bradenmacdonald commented Aug 18, 2021

natabene commented Aug 23, 2021

mikix Aug 24, 2021

mikix Aug 24, 2021

mikix Aug 24, 2021

Colin-Fredericks Aug 27, 2021

jmyatt left a comment

ha-D commented Aug 25, 2021

bradenmacdonald commented Sep 2, 2021

ha-D commented Sep 2, 2021

mikix commented Sep 2, 2021

Colin-Fredericks commented Sep 2, 2021

natabene commented Sep 8, 2021

openedx-webhooks commented Sep 14, 2021

		{courseWideJs && courseWideJs.map(js => <script key={js} type="text/javascript" src={js} />)}
		{courseWideCss && courseWideCss.map(css => <link key={css} rel="stylesheet" href={css} />)}

[SE-4650] OEP-15 - add support for course-wide custom resources #581

[SE-4650] OEP-15 - add support for course-wide custom resources #581

Conversation

ha-D commented Aug 11, 2021 • edited Loading

Description

Testing Instructions

Reviewers

openedx-webhooks commented Aug 11, 2021 • edited Loading

nizarmah left a comment • edited Loading

Choose a reason for hiding this comment

nizarmah Aug 11, 2021

Choose a reason for hiding this comment

ha-D Aug 12, 2021

Choose a reason for hiding this comment

nizarmah left a comment

Choose a reason for hiding this comment

natabene commented Aug 18, 2021

bradenmacdonald commented Aug 18, 2021

natabene commented Aug 23, 2021

mikix Aug 24, 2021

Choose a reason for hiding this comment

mikix Aug 24, 2021

Choose a reason for hiding this comment

mikix Aug 24, 2021

Choose a reason for hiding this comment

Colin-Fredericks Aug 27, 2021

Choose a reason for hiding this comment

jmyatt left a comment

Choose a reason for hiding this comment

ha-D commented Aug 25, 2021

bradenmacdonald commented Sep 2, 2021

ha-D commented Sep 2, 2021

mikix commented Sep 2, 2021

Colin-Fredericks commented Sep 2, 2021

natabene commented Sep 8, 2021

openedx-webhooks commented Sep 14, 2021

ha-D commented Aug 11, 2021 •

edited

Loading

openedx-webhooks commented Aug 11, 2021 •

edited

Loading

nizarmah left a comment •

edited

Loading