Make custom themes work on microsites. #9282
Conversation
|
Thanks for the pull request, @mtyaka! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request. To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?repo=edx%2Fedx-platform&number=9282 |
c737517 to
20d292b
Compare
|
Thanks for the pull request, @mtyaka! I've created OSPR-748 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:
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. |
20d292b to
32de816
Compare
| ] | ||
| ) | ||
|
|
||
|
|
There was a problem hiding this comment.
Nit: how about
return {"THEME_ENABLED": settings.FEATURES.get("USE_CUSTOM_THEME", False)}
or
return dict(
THEME_ENABLED=settings.FEATURES.get("USE_CUSTOM_THEME", False)
)
just to reduce the number of parentheses a bit.
There was a problem hiding this comment.
I wrote it that way to make it consistent with other functions in that file. Your comment encouraged me to change them all to use literal dicts.
|
The code looks good, and the patch very clean. Is there a way for me to test this with reasonable effort? Would I have to set up an installation with microsites myself? |
32de816 to
c659164
Compare
|
@mtyaka @antoviaque hello, before I take a deeper look at this, I'd like to ask if OpenCraft is an operator of a microsite(s)? I ask this because @felipemontoya and I have been discussing some potential backwards incompatible changes to how configuration overrides are structured in order to make them a bit more consistent. So we wanted to see who are potential stakeholders. |
|
@chrisndodge We are only getting started now, we currently don't have a production website running microsites, only a staging one -- ie where we are currently trying to get the microsite and theming to work together : ) So it can definitely be good to be in the loop, but most likely bakward-incompatible changes would be fine until we get it in production on our side. I'll let @mtyaka comment though : ) |
|
@chrisndodge Backwards-incompatible changes are fine. We appreciate the effort to try to make microsite configuration more consistent. |
|
@mtyaka Given that you are already testing this in real deployments, I don't think it's necessary for me to try and deploy microsites myself, so 👍. |
| @@ -21,6 +21,10 @@ | |||
| {% block headextra %}{% endblock %} | |||
| {% render_block "css" %} | |||
|
|
|||
| {% if THEME_ENABLED %} | |||
| {% include "theme-head-extra.html"|microsite_template_path %} | |||
There was a problem hiding this comment.
This seems like an awkward usage pattern. Django's template lookup system is designed to handle the sort of directory lookup that the microsite.get_template_path() function does: just insert the directory at the front of the list of directories, and the template lookup system will find it first. For that matter, Mako's template lookup system is designed that way, too -- @nedbat and I are refactoring how this works in #8271. Could you take a look at what we're doing in that PR, and see if you can do the same sort of thing here? (Or we could just wait until it gets reviewed and merged, and then merge this in afterward.)
Also, you're doing a conditional include here. I think it would be better to do some kind of optional include: have the template system include the template file if it can find it, and if it can't find it, don't include anything. I implemented that in Mako in #8271, and I'm sure it would be possible to implement that in Django templates as well with a custom template tag.
|
I've put this pull request into the "blocked by other work" state on JIRA: we don't want to merge this until after #8271 is merged. This pull request has some useful work in it that I'd like to integrate into our theming system, particularly the parts that touch Django templates (rather than Mako templates), but it'll be a LOT easier to handle after #8271 is merged. |
|
@singingwolfboy Now that https://github.com/edx/edx-platform/pull/8271 is merged, what is the next step on this? Would it be worth a quick re-review pass on this before @mtyaka adapts it to comprehensive theming, now that we know the outcome of https://github.com/edx/edx-platform/pull/8271 ? |
|
@antoviaque Unfortunately, we discovered some problems with #8271 when trying to release it to production on edx.org, particularly related to microsites. As a result, we're going to revert it for now, and try to get it back in for the next release to production. :( |
|
@singingwolfboy OK - good luck with the issues :/ |
|
Hi everyone - this is now one of our top 5 oldest PRs. DB is working hard at making the theming stuff work, but this can't be reviewed until that happens. Further, this will need a lot of work I think to bring it up to speed. My preference is that this PR is closed until we're ready to start reviewing it, and communication in the meantime (questions etc) be done on email. However if that's distasteful to you, so be it. |
|
I closed this PR for now, will reopen when the new theming stuff is merged. This PR will probably have to be changed significantly to work with new theming anyway, so it doesn't make much sense to keep it open in its current form. |
|
@sarina @mtyaka Since this is an active issue for us to have this working, even if we closed it in the meantime we would still have to track it. From prior experiences with PRs closed "temporarily", they tend to be forgotten upstream. I've seen that Matjaz just closed it, but I'd rather reopen it if possible to reflect the current status on this - would you mind? |
|
@antoviaque I have the same experiences w/ PRs that stay open - they get very stale, and the only action that happens is the literally weekly checking I have to do to see what the status of the PR is. From that perspective, I prefer having PRs that aren't being actively worked on or reviewed/waiting for review not be a thing that has to take energy every week. I guess I also don't see how it being open and stale reminds you that it still needs to be done, over active tickets on your issue tracker. But, all that said, if you think it's valuable to have it open, perhaps the PR can be open but a new OSPR ticket could be made when it's time to kick the tires again? |
|
@sarina Ah, I see what you mean. That makes sense for me to save your the weekly work. How will we know that it's the right time to reopen it, though? I've a bit lost track of the status of the dependencies on this, with the merges & revert - would someone think of letting us know, if you're not checking on this PR anymore? |
|
@singingwolfboy can you please make a jira task to loop back on this PR On Thu, Nov 5, 2015 at 3:12 AM, Xavier Antoviaque notifications@github.com
|
|
@sarina @singingwolfboy Perfect - that would work well. Thank you! |
|
@singingwolfboy @mtyaka Now that comprehensive theming is merged, will it be a good time to resurrect this PR? If so we'll try to schedule this in an upcoming sprint. |
|
@singingwolfboy @sarina Ping : ) |
|
Hi @antoviaque I assume now is the right time, but I would recommend to rebase the branch and open a new PR. |
|
@sarina Sounds good, we'll do it like that, and schedule the work for one of the upcoming sprints. Thanks! |
|
We tested comprehensive theming with microsites and found out it works well on microsites. Adding microsite support to the deprecated stanford system doesn't make sense anymore, so we are done with this issue. |
Background: Custom themes currently don't work on microsites. Custom theme only works on the base site (and only if there is no default microsite configured), while requests to microsites use the default edX theme with optional microsite template overrides. In most cases this is not the desired behavior - when you install a custom theme you will most likely want it to work on the base site as well as any configured microsites. Custom themes and microsites were previously discussed here. The discussion resulted in the patch at https://github.com/edx/edx-platform/pull/3495 which enabled microsite template overrides to work on sites with custom themes, but it has a side-effect of completely disabling custom themes on microsites. This patch enables custom themes on microsites while still allowing microsites to override specific templates.
JIRA ticket: https://openedx.atlassian.net/browse/OSPR-748
Partner information: not an edX partner - 3rd party-hosted open edX instance
Reviewers: @smarnach & TBD
This patch enables custom themes on microsites. Microsites can still override specific templates, although when a custom theme is enabled, microsites have to override theme templates (theme-header.html, theme-footer.html...) rather than default edX templates (navigation.html, footer.html...). I believe this makes sense since with a custom theme installed, microsites will be building upon the theme rather that the default edX look.
This patch also makes the main_django.html template custom theme friendly, so that custom themes don't have to override the entire file. It also makes the main_django.html template microsite override friendly.
Note that this change is NOT backwards compatible. If an Open edX instance was configured with both a custom theme and microsites, the microsites will start using the custom theme after deploying this patch, while they were using the default edX theme before the patch.
If desired, we could add a flag to microsite configuration to disable custom theme on the microsite level. I didn't implement it yet, because I'm not sure how useful that would be, but will be happy to do it if there is need for it.
To test this change, install a custom theme, for example https://github.com/Stanford-Online/edx-theme. Ensure that the custom theme is working, then configure a microsite (https://github.com/edx/edx-platform/wiki/Microsites-Theming). If you visit the microsite without this patch, you will see the default edX theme. If you visit the microsite with this patch applied, you will see the custom theme.