Skip to content

feat: added new setting CUSTOM_RESOURCE_TEMPLATES_DIRECTORY - #30759

Closed
keithgg wants to merge 1 commit into
openedx:open-release/maple.masterfrom
open-craft:keith/bb_5885_maple_backport
Closed

feat: added new setting CUSTOM_RESOURCE_TEMPLATES_DIRECTORY#30759
keithgg wants to merge 1 commit into
openedx:open-release/maple.masterfrom
open-craft:keith/bb_5885_maple_backport

Conversation

@keithgg

@keithgg keithgg commented Jul 21, 2022

Copy link
Copy Markdown
Contributor

This PR backports the changes #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 the xmodule/templates directory.

Supporting information

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

@openedx-webhooks

Copy link
Copy Markdown

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jul 21, 2022
@keithgg
keithgg force-pushed the keith/bb_5885_maple_backport branch from 5b47f36 to b96da20 Compare July 21, 2022 14:50
@keithgg

keithgg commented Jul 21, 2022

Copy link
Copy Markdown
Contributor Author

@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.

@natabene

Copy link
Copy Markdown
Contributor

@keithgg Thank you for the contribution. Sorry, I don't know how to help here. Let's see if reviewers can help out.

@xitij2000

Copy link
Copy Markdown
Contributor

@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.

@timmc-edx timmc-edx self-assigned this Sep 28, 2022
@timmc-edx

Copy link
Copy Markdown
Contributor

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.

@timmc-edx

Copy link
Copy Markdown
Contributor

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.

@keithgg

keithgg commented Oct 6, 2022

Copy link
Copy Markdown
Contributor Author

@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.

@timmc-edx

Copy link
Copy Markdown
Contributor

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.

@jmakowski1123 jmakowski1123 added the product review PR requires product review before merging label Oct 27, 2022
@mphilbrick211

Copy link
Copy Markdown

@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!

@mphilbrick211 mphilbrick211 added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 16, 2022
@timmc-edx

Copy link
Copy Markdown
Contributor

I think the status is still that fixes to the CI tests would still need to be backported.

This setting allows loading of Resource Templates from outside the
edx-platform codebase.

Operators will be able to add their own custom resource templates
without needing to fork the codebase.

(cherry picked from commit ed81774)
@keithgg
keithgg force-pushed the keith/bb_5885_maple_backport branch from b96da20 to 219da77 Compare December 19, 2022 07:13
@openedx-webhooks openedx-webhooks removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. product review PR requires product review before merging labels Dec 19, 2022
@keithgg

keithgg commented Dec 19, 2022

Copy link
Copy Markdown
Contributor Author

@timmc-edx that looks like the case. Although the error now is with installing the requirements and no longer in the test suite itself.

@mphilbrick211 mphilbrick211 added the product review PR requires product review before merging label Dec 27, 2022
@mphilbrick211

Copy link
Copy Markdown

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!

@keithgg

keithgg commented Jan 24, 2023

Copy link
Copy Markdown
Contributor Author

@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?

@timmc-edx

Copy link
Copy Markdown
Contributor

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.

@timmc-edx timmc-edx closed this Jan 24, 2023
@openedx-webhooks

Copy link
Copy Markdown

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U product review PR requires product review before merging

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

7 participants