Skip to content

Pull THEME_NAME from envs tokens if it's there. - #2494

Merged
feanil merged 1 commit into
masterfrom
feanil/fix_cms_gather_assets
Feb 6, 2014
Merged

Pull THEME_NAME from envs tokens if it's there.#2494
feanil merged 1 commit into
masterfrom
feanil/fix_cms_gather_assets

Conversation

@feanil

@feanil feanil commented Feb 6, 2014

Copy link
Copy Markdown
Contributor

If the FEATURES['USE_CUSTOM_THEME'] flag is set but the
'THEME_NAME' is not set. We get a compilation error during
cms gather assets.

@cahrens @jzoldak

If the FEATURES['USE_CUSTOM_THEME'] flag is set but the
'THEME_NAME' is not set.  We get a compilation error during
cms gather assets.
@cahrens

cahrens commented Feb 6, 2014

Copy link
Copy Markdown

Feanil, I've never seen this option before. Can you tell from the git logs who worked on THEME_NAME?

@feanil

feanil commented Feb 6, 2014

Copy link
Copy Markdown
Contributor Author

@cahrens This is one of the files that fails when this flag is missing. Looks like @talbs added the original THEME_NAME logic and @singingwolfboy wrapped it in the feature flag. All feature flags get pulled automatically but THEME_NAME doesn't.

$ git blame lms/static/sass/application-extend1.scss.mako
85453f70 (David Baumgold   2014-01-10 10:36:11 -0500 26) % if env["FEATURES"].get("USE_CUSTOM_THEME", False):
ef221829 (Brian Talbot     2013-10-02 16:51:04 -0400 27)   // import theme's Sass overrides
ef221829 (Brian Talbot     2013-10-02 16:51:04 -0400 28)   @import '${env.get('THEME_NAME')}';
ef221829 (Brian Talbot     2013-10-02 16:51:04 -0400 29) % endif

@cahrens

cahrens commented Feb 6, 2014

Copy link
Copy Markdown

So should have the feature flag been created in a different way so that it would automatically get pulled over?

@feanil

feanil commented Feb 6, 2014

Copy link
Copy Markdown
Contributor Author

Well, the issue is that the features flags are all in a dictionary('FEATURES') but the other parameters related to a feature aren't. Everything in the 'FEATURES' dictionary gets read in but there is no logic in aws.py or any where else that I can see, that verifies that all parameters needed for a feature are set.

@yarko

yarko commented Feb 6, 2014

Copy link
Copy Markdown
Contributor

I think there is more to this.

cms/startup.py also needs to be updated for this to matter, and the associated rake (or paver) commands need to get this environment (i.e. outside of django) - see #2387, please.
I was working this basically all last night. The parts which I needed for a "well behaved" cms, in the face of the theme feature (tested w/ Stanford's edx-theme) you can see in https://github.com/yarko/edx-platform/tree/yarko/cms-compatible-theming (not a pull yet)

In other words, I don't think this is sufficient to get it.

If you look at https://github.com/yarko/edx-platform/compare/yarko;cms-compatible-theming, the changes (hack) to preprocess_assets.py are not part of this (debugging relic) and cms/startup.py is mostly a copy of lms/startup.py (needs a cleanup); the rakefile & rakelib/assets.rake change is needed, or the settings (THEME_NAME, path, etc.) are not passed to coffee and sass.

Your change to aws.py (rather than devstack.py, above) is of course a better place (that, or common.py).

@feanil

feanil commented Feb 6, 2014

Copy link
Copy Markdown
Contributor Author

@yarko This is more of a bugfix to get the gather assets working without compilation failures. I'm sure there is more to theming than this but my goal is to be consistent in how the environment vars are pulled in so that gather assets doesn't fail for cms.

@singingwolfboy

Copy link
Copy Markdown
Contributor

👍

feanil added a commit that referenced this pull request Feb 6, 2014
Pull THEME_NAME from envs tokens if it's there.
@feanil
feanil merged commit 4636b27 into master Feb 6, 2014
@feanil
feanil deleted the feanil/fix_cms_gather_assets branch February 6, 2014 16:25
@yarko

yarko commented Feb 6, 2014

Copy link
Copy Markdown
Contributor

right - for example, rake cms:gather_assets:devstack - causes (among other things) these to run:

  • ./manage.py cms --settings devstack preprocess_assets
  • coffee
  • sass

cms/startup.py is involved in the preprocess_assets stage;
the changes to rakefile, rakelib/assets are needed, or the various related theme settings (from /edx/app/edxapp/cms.env.json and lms.evn.json) don't get passed on to coffee and sass, and the theme "doesn't take effect".

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants