Skip to content

Remove ThemingAware storage mixins and ComprehensiveThemeFinder - #11319

Merged
mtyaka merged 1 commit into
openedx:masterfrom
open-craft:remove-comprehensive-theme-finders
Feb 2, 2016
Merged

Remove ThemingAware storage mixins and ComprehensiveThemeFinder#11319
mtyaka merged 1 commit into
openedx:masterfrom
open-craft:remove-comprehensive-theme-finders

Conversation

@mtyaka

@mtyaka mtyaka commented Jan 25, 2016

Copy link
Copy Markdown
Contributor

Collectstatic failed in production when comprehensive theme contained custom css files.
This patch fixes that problem by removing ComprehensiveThemeFinder from STATICFILES_FINDERS and ComprehensiveThemingAware mixin from STATICFILES_STORAGE.

Comprehensive theme static dirs are added to the top of the STATICFILES_DIRS entry, which means that the default django FilesystemFinder will find theme static files, and since the theme folder is at the top of STATICFILES_DIRS, theme files will take precedence over default LMS/CMS static files.

This change means that theme static file URLs are no longer prefixed with 'themes/<theme-name>/', but since we currently only support one comprehensive theme at a time, that shouldn't be a problem.

If/when we want to make the choice of a theme dynamic per-request (comprehensive theming for microsites?), we will have to bring custom theme finders and storage mixins back, but for now, it looks like we don't need them.

Prior discussion: https://groups.google.com/forum/#!topic/edx-code/ixHe-CvE1AQ
Partner information: 3rd party-hosted open edX instance
JIRA ticket: https://openedx.atlassian.net/browse/OSPR-1086
Sandbox URLs: TBD


Settings

COMPREHENSIVE_THEME_DIR: '/edx/app/edxapp/edx-platform/themes/red-theme/'

@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @mtyaka! I've created OSPR-1086 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

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

  • supporting documentation
  • edx-code email 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 still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Jan 25, 2016
@nedbat

nedbat commented Jan 27, 2016

Copy link
Copy Markdown
Contributor

@mattdrayer will be the engineer on this.

@nedbat

nedbat commented Jan 27, 2016

Copy link
Copy Markdown
Contributor

I would have thought themable microsites are on the close horizon, and we would rather not discard work that would help with it, but @mattdrayer knows more.

@mattdrayer

Copy link
Copy Markdown
Contributor

@mtyaka -- @nedbat is correct that we are very close to combining microsites to yield dynamic per-request theming. We have not yet defined the specific approach to accomplish this outcome, however -- our work so far has been to hide all of the existing branding/theming implementations behind comprehensive theming and eliminate redundancies where possible. After this consolidation work is complete we will move to multi-site which we hope to begin within the next two weeks.

So with that said, I'm not sure we want to remove these classes now if we are going to need them back in short order -- thoughts?

@mattdrayer

Copy link
Copy Markdown
Contributor

@ziafazal @saleem-latif FYI on this PR

@mtyaka

mtyaka commented Jan 27, 2016

Copy link
Copy Markdown
Contributor Author

@mattdrayer @nedbat I can change the patch to not remove the actual files where the classes are defined, and just remove them from configuration and from DevelopmentStorage and ProductionStorage for now, because they are currently broken and not needed. Does that sound good?

@jbzdak

jbzdak commented Jan 28, 2016

Copy link
Copy Markdown
Contributor

👍 I have verified existence of the error, and that this PR solves it. After checking out this PR comprehensive themes work on my local full-stack.

@mattdrayer

Copy link
Copy Markdown
Contributor

@mtyaka that sounds good to me -- thanks!

Collectstatic failed in production when comprehensive theme contained custom css files.
This patch fixes that problem by removing ComprehensiveThemeFinder from STATICFILES_FINDERS
and ComprehensiveThemingAware mixin from STATICFILES_STORAGE.

Comprehensive theme static dirs are added to the top of the STATICFILES_DIRS entry,
which means that the default django FilesystemFinder will find theme static files,
and since the theme folder is at the top of STATICFILES_DIRS, theme files will take
precedence over default LMS/CMS static files.

This change means that theme static file URLs are no longer prefixed with themes/<theme-name>/,
but since we currently only support one comprehensive theme at a time, that shouldn't be a problem.
If/when we want to make the choice of a theme dynamic per-request (microsites?), we will have to
bring custom theme finders and storage mixins back, but for now, we don't need them.
@mtyaka
mtyaka force-pushed the remove-comprehensive-theme-finders branch from a0eeab3 to 266f593 Compare January 29, 2016 07:28
@mtyaka

mtyaka commented Jan 29, 2016

Copy link
Copy Markdown
Contributor Author

@mattdrayer @nedbat I changed the patch so that ComprehensiveTheme finders/storage mixins are only removed from the configuration. The actual files where the classes are defined are kept in the repository.

@mattdrayer

Copy link
Copy Markdown
Contributor

Thanks @mtyaka -- 👍 -- @saleem-latif @ziafazal if you can confirm that we're good to go with this PR that would be great.

@saleem-latif

Copy link
Copy Markdown
Contributor

LGTM 👍

@mattdrayer

Copy link
Copy Markdown
Contributor

👍 for me as well -- GTG @mtyaka

@mtyaka

mtyaka commented Feb 2, 2016

Copy link
Copy Markdown
Contributor Author

Thanks!

mtyaka added a commit that referenced this pull request Feb 2, 2016
…finders

Remove ThemingAware storage mixins and ComprehensiveThemeFinder
@mtyaka
mtyaka merged commit bb0c290 into openedx:master Feb 2, 2016
@bradenmacdonald
bradenmacdonald deleted the remove-comprehensive-theme-finders branch February 5, 2016 16:23
@andy-armstrong

Copy link
Copy Markdown
Contributor

@mtyaka @mattdrayer @jbzdak Can one of you verify this PR on staging when you have a chance, as we plan on pushing to production at 11am EST on Tuesday:

https://openedx.atlassian.net/wiki/display/ENG/2016-02-09+Release

@mtyaka

mtyaka commented Feb 9, 2016

Copy link
Copy Markdown
Contributor Author

@andy-armstrong @mattdrayer @jbzdak I'm on it.

@natea

natea commented Feb 21, 2016

Copy link
Copy Markdown

@mattdrayer

After this consolidation work is complete we will move to multi-site which we hope to begin within the >next two weeks.

Any update on combining microsites with the comprehensive theming?

@mattdrayer

Copy link
Copy Markdown
Contributor

@natea, yes! This work is currently in progress -- I will tag you on a PR momentarily

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants