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

[ha-D]

Description

Intention

This PR implements OEP-15, i.e., enabling the user to add global course-wide scripts rather than having to define them separately for every XBlock. Having such a feature allow us to integrate third-party libraries such as ReadSpeaker and Gamalon which have been requested by our clients.

Approach

As suggested in the OEP, two new fields are added to Advanced Settings allowing the staff to specify a list of JS and CSS resources which they want to be included in all pages of the course.

These resources are rendered in the base templates, which will allow them to be included in all course pages.

However, for the micro-frontend React UI things will need to be a little different. There are two main things consider here:

• In the React UI, units (and all XBocks inside) are rendered in an iframe. This means scripts and styles added to the parent frame won't work for the XBlocks rendered inside the iframe and vice verca. And so if we want the scripts/styles to be available in both contexts we will have to add them to both.
• Since the React UI is not rendered with Django templates, it will need to be able to fetch the custom resource URLs via an API call and include them in the page dynamically.

The custom JS/CSS resources are included in the CourseInfoSerializer and will be available to the React UI when it is fetching the course metadata via the API. A future PR on the frontend code will use this and add the resources to the page.
As for the content inside the iframes, they are rendered using templates in django and the changes made for the legacy UI will also work for them.

The changes required for the frontend are available in frontend-app-learning#581

Points of possible debate

• The OEP only mentions adding support for course-wide scripts (JS) and not styles (CSS). I've decided to also allow adding CSS here since a it's quite common for third-party Javascript libraries to have accompanying stylesheets which also need to be added to the page.

• I've used the names Course-wide JS and Course-wide CSS for these fields. I personally preferred it over the term global, let me know if you think otherwise or if you have other suggestions.

• For the React UI, including the scripts/styles twice (once in the parent frame and once in the iframe) is obviously not ideal performance-wise. However, I think the purpose of this OEP was to enable a wide variety of scripts to run and these could be targeting either XBlock content or the course page itself depending on the use-case (correct me if I'm wrong), which will mean the scripts need to be included in both places. Another possible solution could be to have three separate options available for the user:

  • Scripts/Styles to be used in the legacy UI
  • Scripts/Styles to be used in the parent frame of the React UI
  • Scripts/Styles to be used in the iframe's of the React UI

  I feel like it's not worth the extra complexity though

Testing Instructors

Sandbox Link: LMS / Studio

1. Login to the studio (user: staff@example.com pass: edx) and open the Advanced Settings for the demo course.
2. Find the Course-wide Custom JS and Course-wide Custom CSS fields and add sample resources to test with. These could be external URLs or files added in the Files and Uploads page.
3. Visit the any course page (e.g., courseware, discussions, progress, etc. ) and these scripts should be working

(You can skip steps 1 and 2 and use the scripts I've already added. The script will show you a toast message on the upper left corner of the page.)

Reviewers

• @nizarmah
• @bradenmacdonald

[openedx-webhooks]

Thanks for the pull request, @ha-D! I've created OSPR-5952 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.

[natabene]

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

[nizarmah]

I have two main points regarding the "Points of possible debate".

The main motivation of the OEP is to "facilitate research and experimentation". Accordingly, I agree with the following two points you mentioned:

• Including the script in both the parent frame and in the iframe is beneficial because it would provide educators to different parts of the Open edX Platform, instead of limiting them to the vertical course content.
• Complicating things by separating the different domains where the content can be injected isn't necessary because the research and experimentation can prevent certain actions unless the necessary conditions are met.

Accordingly, I'm going to go ahead and provide my approval to this pull request since I've already:

• test the changes as you suggested in the pull request
• read through the code

Nice work 👍🏼

[ha-D]

Thanks for the review @nizarmah.

I realized something was missing here while submitting the frontend PR. We also need to add the fields in the course_home_api django-app since this is the API that all non-courseware pages in the frontend will be using (rather than courseware_api).
This is added in the second commit, could you take a look at this as well.

[nizarmah]

@ha-D nice work on the changes 👍🏼 I still have one minor comment I'll need you to address though, please. Sorry about that!

[nizarmah]

Thanks @ha-D for addressing the final comment I had 👍🏼!

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

[bradenmacdonald]

@Colin-Fredericks I thought I should bring this PR to your attention :)

[bradenmacdonald]

Also @msegado as the author of the original OEP, would you like to comment on this PR? Has there been any other implementation that we weren't aware of, btw?

[bradenmacdonald]

@natabene I think this is ready for review now. Is this one I can take, or do you know who else might want to look at it?

@ha-D the code looks great to me, just saw one thing that needs to be bumped and had one question. Thanks for this nice work.

[Colin-Fredericks]

@Colin-Fredericks I thought I should bring this PR to your attention :)

Yes! Thank you so much for putting this together. This will make deploying our existing javascript much faster and more effective, and it gets us access to pages we've never been able to deploy to before. Very much appreciated.

[msegado]

Also @msegado as the author of the original OEP, would you like to comment on this PR? Has there been any other implementation that we weren't aware of, btw?

Apologies for being late to the party! I'm not aware of any other implementations, so thanks to @ha-D for putting this one together; I expect the Harvard folks in particular will find it quite helpful. I also like the name change from "global" to "course-wide" - definitely clearer that way.

The only thing I'd suggest adding (eventually, doesn't need to be in this PR) is some text clarifying the security implications and support expectations for this feature. The OEP contains some sample wording that could work in the What about security? and What about compatibility and support? sections.

[natabene]

@bradenmacdonald Thanks for asking, let me check and get back to you asap.

[natabene]

@bradenmacdonald Yes, that is fine, thank you!

[bradenmacdonald]

@gabor-boros I just tested this now on the sandbox, and have only one concern:

Right now, this is a little bit inconsistent because of the partial roll-out of MFEs in courseware.

I can see the toast on the old Course outline page but not the new course outline page.

I can see the toast on the courseware page within the iframe as expected ✅

I see the toast on the Discussion, Wiki, and Instructor tabs, but not on the Progress or Dates tabs (since those are MFEs).

Is it possible to make a quick fix to this so that the scripts only appear on the courseware pages (old or new), but not on the various other tabs? Because we've already heard that the MFEs will not allow these kind of script injections, so we know that eventually it won't be on any tabs at all other than courseware, and the current mixed situation could be confusing for course authors.

Otherwise this looks good and I'm ready to approve it.

[gabor-boros]

Thank you @bradenmacdonald for reviewing this!
I'll check what I can do here. In the meantime, I rebased this PR on the master branch to make sure everything is up to date.

[gabor-boros]

@bradenmacdonald The only reasonable way I found is to render the assets if we are rendering courseware or an xBlock. Please let me know if this is not what you had in mind. The sandbox is on its way.

[edx-status-bot]

Your PR has finished running tests. The following contexts failed:

• jenkins/django-3.2/python

[gabor-boros]

@bradenmacdonald I'm not entirely sure about the issue on the CI, but the PR is ready for your review. If you would like to see it working with non-MFE, please check this PR.

[bradenmacdonald]

Thanks for those changes, I think this is good to go now! Just have to ask about if the django3.2 test is a blocker for merge.

👍

• I tested this: MFE and legacy experiences on the sandbox
• I read through the code
• I checked for accessibility issues: n/a
• Includes documentation: some inline docs

[openedx-webhooks]

@ha-D 🎉 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]

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

[edx-pipeline-bot]

EdX Release Notice: This PR may have caused e2e tests to fail on Stage. If you're a member of the edX org, please visit #e2e-troubleshooting on Slack to help diagnose the cause of these failures. Otherwise, it is the reviewer's responsibility. E2E tests have failed. https://gocd.tools.edx.org/go/tab/pipeline/history/deploy_to_stage

[edx-pipeline-bot]

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

[alfredchavez]

Hi @gabor-boros could you port this changes to lilac.master(if it's ok to do so)? as part of BB-4877
CC: @arbrandes for review

[kevincanavan]

@ha-D @bradenmacdonald - edX Partner Support has received a request for documentation on this feature. I've seen the original proposal doc, but since that hadn't been updated in years before this was merged, I'm not sure how much might have changed. Any help you can provide would be appreciated.

Specific questions (though anything additional is also welcomed):

• proper syntax for adding both JS and CSS files/resources in their respective advanced settings fields
• Where on the platform do these scripts/styles apply? Just inside the content iframe? On the items outside the frame? On custom pages? Presumably not in discussion forums.
• Are there any specific things we should avoid – for instance, is there a built-in .invisible class that shouldn’t be overridden, or a javascript function named main() that shouldn’t be redefined?

Thanks in advance for any help you can offer.

[bradenmacdonald]

@kevincanavan I'm about to take some time off, so I'll find someone else to answer your question. If not, I'll figure it out for you when I'm back. CC @gabor-boros

[xitij2000]

@ha-D @bradenmacdonald - edX Partner Support has received a request for documentation on this feature. I've seen the original proposal doc, but since that hadn't been updated in years before this was merged, I'm not sure how much might have changed. Any help you can provide would be appreciated.

Specific questions (though anything additional is also welcomed):

* proper syntax for adding both JS and CSS files/resources in their respective advanced settings fields

* Where on the platform do these scripts/styles apply? Just inside the content iframe? On the items outside the frame? On custom pages? Presumably not in discussion forums.

* Are there any specific things we should avoid – for instance, is there a built-in .invisible class that shouldn’t be overridden, or a javascript function named main() that shouldn’t be redefined?

Thanks in advance for any help you can offer.

I've created a PR to update the OEP to match the current implementation: openedx/open-edx-proposals#396

I'm not sure if that's the right approach, if so, do tell me how/where I should document this.

• These will only apply inside the content iframe.
• I think making such an exhaustive list would be difficult, a lot of things to break the UI, so it might be better to just advise general caution. I don't know if there is any such advice when adding custom JS/CSS using raw HTML. If so we can duplicate that.