WIP wiki support for microsite templates #10623
Conversation
|
Thanks for the pull request, @felipemontoya! I've created OSPR-949 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. |
|
Hi @chrisndodge, some of my work this week led me to support the microsites template overrides on the wiki pages, so I put together this PR to discuss if this is upstream material. |
|
Thanks @felipemontoya! I'll try this out. FYI, @singingwolfboy |
| @@ -33,19 +33,17 @@ | |||
| {% if IS_EDX_DOMAIN %} | |||
| {% include "navigation-edx.html" %} | |||
| {% else %} | |||
| {% include "navigation.html" %} | |||
| {% include "navigation.html"|microsite_template_path %} | |||
There was a problem hiding this comment.
nifty trick! I didn't know this was possible.
|
Also, I don't think this branch is still active in your repository. I'm trying to pull from your repo to run this locally - to give it a whirl - but I'm not seeing it in: https://github.com/eduNEXT/edunext-platform Am I looking in the right place? |
|
I guess I could simply generate a .patch file from the PR and apply it locally... |
|
Hi @chrisndodge, that is the repo we use for deployment, all the changes we submit to uptream are at the regular edx-platform repo. Here is the correct branch: https://github.com/eduNEXT/edx-platform/tree/fmo/wiki-support-for-microsites |
|
@felipemontoya thanks, got it! Giving it a spin right now... |
|
@felipemontoya hrm, I'm having trouble running that branch in my devstack. My devstack environment is close to upstream edX master, so it's likely an environment issue. I'll either need to provision a clean devstack or simple just apply the diff patch to my localdev. I'll probably do the latter to save some time and diskspace. |
|
@felipemontoya thanks. Still trying to get your PR running, I'm having some devstack issues... |
|
@felipemontoya OK, I'm seeing one problem. When I go to the wiki on one of our white label sites, I'm getting a template not found error. however I do have a <microsite_key>/templates/footer.html file defined (but it is a Mako template). I'll investigate, but do you have any tips? |
|
The fact that it is a mako template should not make any difference. The footer I am currently using is also a mako template. The only thing I have seen here are rendering errors when the file does not have unix endings, but I don't think that is the case here. Now, is the path literally |
|
thanks. still having trouble here. I do have a footer.html in my microsite, but I don't have a navigation.html (not overridden). The navigation.html loads fine from the base edx-platform, but footer.html fails. I hate to throw out the Django1.8 card, but I'm running this on 1.8, are you? I'm not sure if there might be some template loader differences between the versions. |
|
Django 1.8 might actually be the cause. I have this branch in an old devstack from django 1.4. I will launch a sandbox on a new devstack running from master and see if it works. It will probably take me a couple of hours. |
|
thanks, sorry for the hassle. I'm not even sure it'll make a difference... |
|
Hi @chrisndodge, I found the same issue on a new devstack running django 1.8. It has to do with the way the lms/startup.py:enable_microsite function works, but I'm still looking into it. I will update the PR with both a fix and the rebase to master when I make it work. |
52a1ab8 to
b5196f5
Compare
|
@chrisndodge, basically, the MakoLoader in django1.8 has an engine which gets initialized with the TEMPLATE['DIRS'] before we had the chance to enable the microsites feature. I moved the enable_microsites to make it add the microsites_root_path before that happens. |
|
@felipemontoya thanks! |
|
The test env clearly didn't like the change, now I'm wondering if that is a missing configuration on the test env. Did you tried the change in your devstack to see if it works for you? |
|
I should be able to try locally in a few hours. Thanks! |
|
@felipemontoya @mattdrayer I pulled this down tonight and applied it locally. In general, it seems to work, but there seems to need some changes in edX's production microsites for this to fully work:
I'm going to have to dig in a bit here to see how we can pull this in without causing visual regressions in our white labels. |
|
@felipemontoya let us know if you need a hand with this PR -- we have a ticket in our backlog (WL-247) reserved for providing support. |
|
Hi @mattdrayer and @chrisndodge. I has out for the holidays, but I'm back at the office now. Obviously I have no access to the repository where you keep your microsites, but I could easily add the css override to the django_main, the same way we did it at our repo. We did this so long ago that I did not even notice that it wasn't in the upstream repo. Now, since Chris mentioned the visual regressions in your white labels I will wait a word from you before I do that. P.S: I think I'm not allowed to see WL-247. |
| @@ -28,14 +28,14 @@ | |||
| <div class="window-wrap" dir="${static.dir_rtl()}"> | |||
| <a class="nav-skip" href="{% block nav_skip %}#content{% endblock %}">{% trans "Skip to main content" %}</a> | |||
| {% with course=request.course %} | |||
| {% include "header.html" %} | |||
| {% include "header.html"|microsite_template_path %} | |||
There was a problem hiding this comment.
@mattdrayer @chrisndodge I investigated it on my side after pulling down this branch. It seems me, wiki page is based on Django templates rather than mako and after running the lms on this branch , It seems me header from microsite is not loading in wiki. You can see that templates/header.html is based on mako in microsite and main_django.html in wiki page is trying to load this microsite header file.
There was a problem hiding this comment.
@felipemontoya if you can address this issue today, that would be great.
There was a problem hiding this comment.
I am not sure I understood what you mean, but the renderer is actually capable of including Mako templates from within a django_template. That is what the ## mako first line does. It helps the template engine to recognize mako templates to load them with the right library.
There was a problem hiding this comment.
then It means, our microsite templates should have ## mako in first line.
There was a problem hiding this comment.
yes, although not every single template needs it. It is only used when loading mako templates from inside django_templates. Some examples that do need it are header.html and footer.html. Once the templates is mako, it can include other mako templates without needing the ## mako line
There was a problem hiding this comment.
Okay, so I guess I'm not clear on what needs to happen next for this PR -- @asadiqbal08, do we need to update additional microsite templates in our repo with the mako tag prior to merging in order to avoid breakages?
There was a problem hiding this comment.
@mattdrayer make tag ## mako is missing in our microsite repo (header.html) that is why there is a breakage in header section of wiki. I think we need to add up this in order to load headers.
@mattdrayer @felipemontoya It seems me that microsite css is not rendering in wiki. we need to override the microsite css. I just tested it after adding the {% microsite_css_overrides_file %} in main_django.html and styles are rendering OK.
There was a problem hiding this comment.
@asadiqbal08 -- okay, let's go ahead and make the necessary changes in the edx-microsites repository. Please log a WL ticket to track the job and add it to the current sprint.
|
That and also tell me if you also want to have me add the CSS overrides file. I would say yes to that, but you need to test it along with your microsites repo to see that the styles are rendering OK. |
|
Thanks for working on this, it would be great to get microsites working correctly with django templates. I opened a very similar PR some time ago (before comprehensive theming landed). @singingwolfboy suggested to use a custom django template loader rather than filters, see: https://github.com/edx/edx-platform/pull/9282#discussion_r37007674 |
|
@mattdrayer and @asadiqbal08 I just updated the main_django file adding the css overrides. Which use to be there, but where removed at this point. After I did that, I went back to the comp_theming readme file stating that you should now place this in the Putting everything togher I am now sure, that the css overrides should not longer be there (I'll remove it next thing), but instead, you also have to place @singingwolfboy could you please correct me if I'm wrong in the last paragraph or on this PR's code? |
|
@felipemontoya I believe you are correct. I've put in a PR to remove the unused |
|
I tested my assumptions locally and it work, it required also that the Again, the |
|
Okay, we've merged @asadiqbal08's mako tag update to our microsites repository, so I think we're good to go there. @felipemontoya @singingwolfboy @asadiqbal08 @mtyaka if there are no other changes you feel are necessary for this PR after having a chance to review the changeset I think we are good to move forward -- let me know. |
|
Looks fine to me. 👍 |
|
@felipemontoya I think the only other thing I would like to see here is a handful of unit tests which cover the new djangoapp operations and the settings check in startup.py -- would it be possible to include these in this PR? |
|
@mattdrayer a pair of tests for the |
|
Hi @felipemontoya -- just pinging you re: the aforementioned tests -- once we have those I think this PR is good to go. |
|
@mattdrayer, I added one simple test for the filter, to make sure those lines are being run during the test process. Do you reckon more complicated tests are in order there? |
8ac45cc to
f0c43a2
Compare
|
jenkins run python |
|
@felipemontoya -- thanks for adding the test -- I'm still a little fuzzy on exactly what is being covered here semantically, but Jenkins is happy and the diff coverage report is showing 100% so I guess in cold world of robots and LOCs we are looking good. If there are no other changes to be made then I'd say it's time to squash, verify one last time, and merge 👍 |
Stripping the leading / for the django_templates finder Enabling the microsite configurations before running django.setup() Adding only the templates directory before startup Adding the missing overrides file at the django templates main Using the comp_theming way of overriding css Adding test for the microsite_template_path filter
f0c43a2 to
f2a6a27
Compare
|
jenkins run bokchoy |
|
@mattdrayer, squashed and tested. From my end we are ready. |
|
Alright, let's do it! Thanks very much for this contribution, @felipemontoya -- merging now! |
WIP wiki support for microsite templates
This PR would allow the templates being rendered with the django_template engine, most notably the wiki, to support the templates override used for microsites. Specially the header and footer templates.
The way this PR addresses this issue is by creating a template tag that wrapps the microsite.get_template_path function used for finding the templates. Then it uses this templatetag inside the main_django.html template.