WL-325: Multi-Site Comprehensive Theming - #11885
Conversation
591dbbe to
62798da
Compare
| "MICROSITE_DATABASE_TEMPLATE_CACHE_TTL", MICROSITE_DATABASE_TEMPLATE_CACHE_TTL | ||
| ) | ||
|
|
||
| FOOTER_CACHE_TIMEOUT = ENV_TOKENS.get('FOOTER_CACHE_TIMEOUT', FOOTER_CACHE_TIMEOUT) |
There was a problem hiding this comment.
In lms/envs/aws.py this settings parameter is located in a section named 'Branded footer' -- I think it would be good to do that in this file as well, because right now it looks like it's part of the microsite configuration set and that's not really true.
|
@saleem-latif I have made an initial pass through the updated branch -- overall I am really liking what we have here! It will definitely be good if @ziafazal can also review the code, and I will take another look on my next cycle. Once we three are happy with things we can squash things up and go a second round with some other people. |
78cfac5 to
5979be1
Compare
There was a problem hiding this comment.
Should not theme paths be self.path = Path(get_base_theme_dir()) / theme_dir? if we want to get themes path with project root or component we can created a method in this class to return component path.
There was a problem hiding this comment.
It seems there is only one reference to Theme.path at the moment, so I am okay leaving it as-is for the time being.
|
@saleem-latif @mattdrayer overall LGTM but turning a huge PR. |
|
jenkins run lettuce |
|
Just a follow up to @ziafazal's comment about the PR becoming complicated. By 'complicated' here we mean there are several major components of the system being modified in the same changeset. However these changes are all required in order for the feature to be considered complete/viable. These changes are as follows:
While it would be possible to structure these changes as three separate PRs, or even three commits in the same PR, we have decided that it is best to continue bundling all of these changes into a single PR + single commit which best reflects the overall implementation of "Multi-Site Comprehensive Theming". |
|
@felipemontoya @mtyaka, FYI on this updated PR |
|
@andy-armstrong -- as discussed earlier, here is the updated PR for multi-site comprehensive theming support in LMS. It should look fairly familiar to the previous change set, with fixes for static asset caching and some all-around improvements to the code. In addition, a sandbox is available, with four sites currently configured:
|
5979be1 to
7c9c10a
Compare
|
@saleem-latif @mattdrayer Thanks for creating all the sandboxes. That is a great demonstration of the capabilities. I wasn't able to fully test due to the bug with Underscore on sandboxes (which I'm fixing here: https://github.com/edx/edx-platform/pull/11938), but everything I could reach had the correct caching behavior. I'll do a re-review of the code either later tonight or some time tomorrow. |
|
@andy-armstrong the cool thing is that all of these sites and themes are being served by just one sandbox! |
|
@saleem-latif @mattdrayer I only had a quick looks so far, but this looks great! Very nice work. The https://stanford-theme.sandbox.edx.org/ currently loads with the red theme for me. Is that a bug or just misconfiguration?
|
|
Ah, that was a left over thing from a demo I gave earlier today -- I have changed it back to 'stanford-style' which is easy enough to do via the Django admin! (note the cached theme timeout is 30 mins by default) |
7c9c10a to
762d81c
Compare
|
@mattdrayer Could you rebase the branch and update the sandbox as the problem with Underscore should be fixed. Thanks! |
762d81c to
eb49e22
Compare
|
Rebased! |
|
@saleem-latif @mattdrayer I went through the changeset carefully and I like the direction this PR went. I will be testing this changes tomorrow morning in a more complex environment, to be sure nothing new comes up. Very nice work! |
eb49e22 to
a7ee17d
Compare
|
Wow, this got huge! Very impressive work @saleem-latif. 👍 |
|
Beauty! I have to squash one commit and then this one will be good to go -- thanks much, @andy-armstrong! |
ziafazal: improvements need for multi-tenancy ziafazal: fixed broken tests ziafazal: no need to add setting in test.py ziafazal: added hostname validation ziafazal: changes after feedback from mattdrayer ziafazal: fixed branding and microsite broken tests ziafazal: make STATICFILES_DIRS to list ziafazal: added theme directory to mako lookup for tests ziafazal: added more protection in test_util saleem-latif: Enable SCSS Overrides for Comprehensive Theming saleem-latif: Incoporate feedback changes, Correct test failures, add tests and enable theming for django templates saleem-latif: Correct errors in python tests mattdrayer: Fix invalid release reference mattdrayer: Update django-wiki reference to latest release saleem-latif: Update Theme storages to work with Caching, Pipeline and collectstatic saleem-latif: Incorporate feedback changes mattdrayer: Pylint violation fix mattdrayer: Fix broken pavelib test
a7ee17d to
a796b56
Compare
|
jenkins run bokchoy |
|
We have an almost green build here -- the bok choy failure is due to a flaky test (ref. FED-99). Aside from this issue everything else looks stellar, so I'm going ahead with the merge. |
|
|
||
| # if site theme dir is not in cache and comprehensive theming is enabled then pull it from db. | ||
| if not site_theme_dir and is_comprehensive_theming_enabled(): | ||
| site_theme = site.themes.first() # pylint: disable=no-member |
There was a problem hiding this comment.
@saleem-latif This line is causing WL-377 issue which blocks a CAT-2 TNL-4155
Can you please have look at this ?
7 participants
Hi @mattdrayer , @ziafazal
Kindly review this PR, it contains changes required to support multi-tenant comprehensive theming solution. Themes enable developers to override mako and django templates, static assets, sass styling etc.
JIRA ticket WL-325 and it subtasks contain more information about the changes included in this PR.
Here is a lsit of changes included,
A sandbox is available, with four sites currently configured: