Revert "Revert 'saleem-latif/WL-328: Multi-Site Comprehensive Theming'" - #12184
Revert "Revert 'saleem-latif/WL-328: Multi-Site Comprehensive Theming'"#12184mattdrayer wants to merge 1 commit into
Conversation
d228031 to
30954b8
Compare
4ec9ec4 to
0b15bbb
Compare
|
@mattdrayer I have also added changes for WL-418 an WL-419 into this PR. |
| SiteTheme object for given site or a default site set by `DEFAULT_SITE_THEME` | ||
| """ | ||
|
|
||
| theme = site.themes.first() |
There was a problem hiding this comment.
This seems like the perfect place to insert the ability to provide a theme override through URL parameter.
https://openedx.atlassian.net/browse/FEDX-31
It would be incredibly useful to be able to dynamically switch themes on a sandbox when verifying changes. I'm thinking this would be equivalent to the current preview_lang URL parameter for dynamically switching locale.
There was a problem hiding this comment.
The only problem I can see here is dealing with the cache timeout, which makes it hard to switch back and forth quickly between themes even using the Django admin -- so if we can work around (ie, ignore) that mechanism I think a URL parameter would be a useful add, yes -- if it ends up being a lot of work I think we'll need to log it as a follow-on story/feature, though.
There was a problem hiding this comment.
@andy-armstrong I think it would be better to do this after this PR, so that it can be properly tested and verified, I have logged a story to get this done https://openedx.atlassian.net/browse/WL-434,
There was a problem hiding this comment.
@saleem-latif Makes sense. It would be great if possible if we could get it in this week so that we can use it to diagnose any issues that come up during the release. It seems like it should be a very simple change.
There was a problem hiding this comment.
I really don't want this to roll back again!
|
@mattdrayer @saleem-latif FYI, the JS test failures are a known bug on master: https://openedx.atlassian.net/browse/TE-1280 |
|
@mattdrayer Can you make a sandbox again with the various themes installed on it. Since I gave thumbs to this before and it had to be reverted, I'd like to really think through what we need to test. |
|
@mattdrayer @saleem-latif I'm done with my first pass review. This all looks great, but then I thought that last time. :-) Let me know when there's a sandbox I can look at, and when my feedback has been addressed. |
|
I believe @saleem-latif has a sandbox in the works so we should be able to verify this branch over the course of the next cycle |
|
@andy-armstrong I have update sandbox (theming.sandbox.edx.org) with the latest code, here are the links for different themes
|
|
@mattdrayer Hard for me to check it at a code level because it was an OSPR, but it looks like |
|
@saleem-latif Thanks for setting up the sandboxes. Everything looks great from my side (but I did say that last time too!). 👍 |
|
@mattdrayer Is it worth trying to land this as a separate release so that it doesn't get tangled in with everything else? How else can we ensure that we don't have to rollback? Also, we should probably do an RCA on the issues that we ran into here. |
2029577 to
fb7a7dc
Compare
|
@andy-armstrong -- sorry for the delayed reply -- we are indeed planning to deploy this feature via patch release, versus a ride on the regular release train. |
|
Closing this inactive PR in favor of #12218 |
4 participants
@saleem-latif, FYI -- in order to rebase the WL-328 changes submitted and merged as #11885, then reverted in #12116, I had to 'revert the revert' and this branch+PR represents this process. It looks mainly correct although I had to address a couple of small conflicts which were also present during the initial revert. Let's see what Jenkins has to say.
This reverts commit 392ee7d.
Conflicts:
cms/envs/common.py
cms/envs/devstack.py
lms/envs/common.py
lms/envs/devstack.py
openedx/core/storage.py
pavelib/assets.py
pavelib/paver_tests/test_assets.py