edx-platform
edx-platform copied to clipboard
feat: added new setting CUSTOM_RESOURCE_TEMPLATES_DIRECTORY
This PR backports the changes https://github.com/openedx/edx-platform/pull/30108 to maple.master. Re-using the description below.
Description
At Opencraft, we have clients that want to be able to add custom HTML templates that show up while creating course components in Studio.
Currently, there's no way to achieve this besides forking edx-platform
and adding the yaml
files to common/lib/xmodule/xmodule/templates/
This PR allows operators to set a custom directory where templates can be additionally loaded from.
- Checks if
settings.CUSTOM_RESOURCE_TEMPLATES_DIRECTORY
exists. - If so, load the
yaml
files with the same logic as in thexmodule/templates
directory.
Supporting information
- Configuration PR: https://github.com/openedx/configuration/pull/6706
- Custom Client Branch: https://github.com/open-craft/edx-platform/pull/453
- Task: https://tasks.opencraft.com/browse/BB-5885
- Sandbox: https://studio.pr30108.sandbox.opencraft.hosting [Sandbox]
Testing instructions
- Update the setting
settings.CUSTOM_RESOURCE_TEMPLATES_DIRECTORY
to a valid path. - In that directory create an
html
directory. - Copy a template from
common/lib/xmodule/xmodule/templates/html
to the above directory with a different filename. - In the Studio, edit a Unit. Under the Text component you should see your new template list (on the Sandbox I've added many).
Deadline
"None"
Other
Settings
EDXAPP_CUSTOM_RESOURCE_TEMPLATES_DIRECTORY: /edx/var/edx-themes/edx-themes/resource-templates
edx_platform_commit: keith/bb_5885_maple_backport
edx_platform_repository_url: https://github.com/open-craft/edx-platform.git
configuration_source_repo_url: https://github.com/open-craft/configuration.git
configuration_version: master
Thanks for the pull request, @keithgg! 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 can you assist with the test failures I'm getting here?
I'm getting the below error from the CI:
ImportError: cannot import name 'get_catalog_api_base_url' from 'openedx.core.djangoapps.catalog.utils' (/runner/_work/edx-platform/edx-platform/openedx/core/djangoapps/catalog/utils.py)
But that code isn't part of Maple. It was only added two months ago.
@keithgg Thank you for the contribution. Sorry, I don't know how to help here. Let's see if reviewers can help out.
@natabene can you assist with the test failures I'm getting here?
I'm getting the below error from the CI:
ImportError: cannot import name 'get_catalog_api_base_url' from 'openedx.core.djangoapps.catalog.utils' (/runner/_work/edx-platform/edx-platform/openedx/core/djangoapps/catalog/utils.py)
But that code isn't part of Maple. It was only added two months ago.
I came across this PR accidentally.
It seems the issue here is that pip dependencies are being cached and restored, however the process for installing dependencies doesn't remove existing dependencies.
So what's happening here is that newer CI builds are installing new dependencies, which includes learner-pathway-progress
, and this is being cached. When this build runs, it tries to install older dependencies on top of the existing restored cached dependencies, but doesn't remove learner-pathway-progress
. Since that is a plugin it auto-installs and edx-platform automatically tries to run the code.
I think the quick solution here would be to include the target branch name in cache key, and only restore the cache for the same branch.
I don't know that we usually take non-critical backports to older releases, but the diff is basically identical and so from that perspective I'd be happy to give an approval -- as long as tests could be made to pass.
The CI issue is weird, though. The workflow that's actually running is unit-tests.yml, which doesn't even use actions/cache
. But I also spotted the runs-on: [ edx-platform-runner ]
, which I vaguely recall is the 2U-hosted runner. I think the caching issue must be in the runner itself, compounded by the workflow file not using pip-sync to ensure that stale deps are removed.
I'm going to see if I can work around the caching issue on master, and if that works, maybe you can backport to Maple in a separate PR to unblock this one.
@timmc-edx apologies for the late reply here. Thank you for looking into this. I see that the caching task has been merged. I'll do another PR and close this one if the tests pass.
Oh sorry, I don't mean another PR for this work -- you can keep this one. I just meant that the change to using pip-sync would also needed to be backported to Maple first, to avoid the stale cache issue.
@keithgg @timmc-edx Hi there! Just checking on this to see if this has become unblocked? Looks like tests will need to be re-run prior to review. Thanks!
I think the status is still that fixes to the CI tests would still need to be backported.
@timmc-edx that looks like the case. Although the error now is with installing the requirements and no longer in the test suite itself.
Hi @keithgg and @timmc-edx - is this PR still being pursued? If so, we'll need to have a product review of it as well. Please let me know. Thanks!
@mphilbrick211 we've since migrated away from maple so this isn't important anymore. I'm happy to close it.
@timmc-edx thanks for the help so far, are you happy to close this?
I'm fine either way -- I'll go ahead and close it since it sounds like that's where things are at right now, but if anyone wants to reopen I'm happy to help out with anything that needs review or repo-owner permissions.
@keithgg 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.