Ziafazal/wl-328 multi-tenancy support in Comprehensive Theming - #11613
Conversation
|
@natea, FYI |
|
jenkins run all |
|
Closing and re-opening this PR to try and fix the broken Jenkins checks |
| def get_template(self, uri): | ||
| """ | ||
| Overridden method which will hand-off the template lookup to the microsite subsystem | ||
| Overridden method which will hand-off the template lookup to the theming subsystem |
There was a problem hiding this comment.
Can we add a little bit more here about how this works, ie, the scenarios -- maybe cover the use cases where the lookup would not be necessary, as well as the valid case for the try, as well as the invalid case which would trigger the except.
|
@ziafazal -- I've completed an initial pass -- overall I think this PR is on the right track and happy to see it -- we definitely need a lot more in the way of tests, but that's just a matter of time+energy. I think it would be good for @saleem-latif to assist with writing the tests because he'll be assuming responsibility for the theming enhancements going forward. Happy to take another look after you've addressed feedback and made changes. I mentioned to @bradenmacdonald that I would tag him on this PR earlier today, so going ahead and doing that now. |
|
@andy-armstrong, FYI on this PR |
| def get_template(self, uri): | ||
| """ | ||
| Overridden method which will hand-off the template lookup to the microsite subsystem | ||
| Overridden method do template lookup database or in white labe site theme. |
There was a problem hiding this comment.
"Overridden method for locating a template in either the database or the site theme."
a03a4a1 to
3eddd7b
Compare
There was a problem hiding this comment.
@ziafazal If you are removing this setting then this usage should also be removed:
https://github.com/edx/edx-platform/blob/master/lms/djangoapps/course_wiki/views.py#L62
There was a problem hiding this comment.
@symbolist I'm not sure we should remove that usage so much as protect against the scenario where the value is not available in settings -- for example:
if hasattr(settings, "SITE_ID") and site_id != str(settings.SITE_ID):
Thoughts?
There was a problem hiding this comment.
@mattdrayer It looks like this code is going to need a bit of refactoring. If the SITE_ID is not defined (which given that this was the only instance in settings, is never going to be), get_current() is going to throw a ImproperlyConfigured exception instead of DoesNotExist exception and code will not even go into this except block.
https://github.com/django/django/blob/1.8.9/django/contrib/sites/models.py#L50-L69
3eddd7b to
9a44c68
Compare
|
@felipemontoya @mtyaka @bradenmacdonald @andy-armstrong @ziafazal @saleem-latif -- I've done some housecleaning on the branch in order to resolve some conflicts and squash the intermediate commits. The PR is back open for business now. Ideally we would merge this change set on Monday (end of current sprint). So, it would be great to have all remaning reviews+feedback submitted by Sunday evening, in order to give us time to address any additional concerns. |
|
jenkins run python |
e86be7e to
b80dc6b
Compare
|
jenkins run lettuce |
|
jenkins run python |
1 similar comment
|
jenkins run python |
b80dc6b to
5a890d2
Compare
|
jenkins run python |
2 similar comments
|
jenkins run python |
|
jenkins run python |
|
@saleem-latif One of the tests is causing the suite to fail with a segmentation fault. Re-running will not fix that. I recommend running the whole suite locally with verbose mode to isolate the issue. |
42e0364 to
811a221
Compare
|
jenkins run lettuce |
1 similar comment
|
jenkins run lettuce |
|
👍 from me. Nice work @saleem-latif and @ziafazal. |
|
Fantastic, thanks much, @andy-armstrong -- I'm going to squash the commits and merge the branch! |
811a221 to
c56a633
Compare
|
jenkins run python |
|
jenkins run all |
3 similar comments
|
jenkins run all |
|
jenkins run all |
|
jenkins run all |
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
7f620c6 to
954dae5
Compare
|
Wow, that was an ex-CI-ting adventure :) Finally the build is green again, so I am going ahead with the merge. |
Ziafazal/wl-328 multi-tenancy support in Comprehensive Theming
| settings.MAKO_TEMPLATES['main'].insert(0, themes_dir) | ||
|
|
||
| for theme_dir in os.listdir(themes_dir): | ||
| staticfiles_dir = os.path.join(themes_dir, theme_dir, get_project_root_name(), "static") |
There was a problem hiding this comment.
@mattdrayer @ziafazal I think we need to remove theme_dir from the line above, otherwise we end up with paths like /edx/app/edxapp/edx-platform/themes/edx.org/cms/lms/static.
There was a problem hiding this comment.
Scratch that. I think it is get_project_root_name that needs to go.
|
@benpatterson @mattdrayer |
|
@jzoldak please add details to WL-325 instead, if you wouldn't mind -- is there a way for us to know ahead of merging to master about an impact to the sitespeed score? |
|
@mattdrayer Sure, I just added comments to WL-325. We don't have sitespeed automatically running for PRs yet, that's still an epic on the backlog PERF-185. |
8 participants
@mattdrayer @saleem-latif this PR has the work I have done so far to support multi-tenancy in Comprehensive theming. This PR has accomplished these objectives.