Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP wiki support for microsite templates #10623

Merged
merged 1 commit into from
Jan 12, 2016
Merged

WIP wiki support for microsite templates #10623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions common/djangoapps/microsite_configuration/templatetags/microsite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,3 +63,14 @@ def microsite_css_overrides_file():
return "<link href='{}' rel='stylesheet' type='text/css'>".format(static(file_path))
else:
return ""


@register.filter
def microsite_template_path(template_name):
"""
Django template filter to apply template overriding to microsites.
The django_templates loader does not support the leading slash, therefore
it is stripped before returning.
"""
template_name = microsite.get_template_path(template_name)
return template_name[1:] if template_name[0] == '/' else template_name
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any risk if template_name is None? That probably wouldn't happen in practice, but it seems like we might want a quick exit at the top of the function

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is very little chances of that happening since it is used as a filter on a static string. But since the consequences of that would be annoying (500 error) we might as well prevent it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now there are only 2 known use cases of this tag: "navigation.html" and "footer.html", but eventually it could be used on variables as well and the risk would rise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey, just checking back into this PR, are you going to be able to put in those None guards? Thanks. In the meantime, I'm going to pull this down and try locally.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. I loaded up this branch on its devstack and tried to pass None in the template name. With or without the None guards it will throw an error, the difference would be if is either TemplateDoesNotExist: No exception supplied, or IndexError: string index out of range. I think it does not make a big difference, but if you think they are still required, just ping me here, and I will add them and push them right away.

7 changes: 7 additions & 0 deletions common/djangoapps/microsite_configuration/tests/test_microsites.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,10 @@ def test_breadcrumb_tag(self):
expected = u'my | less specific | Page | edX'
title = microsite.page_title_breadcrumbs_tag(None, *crumbs)
self.assertEqual(expected, title)

def test_microsite_template_path(self):
"""
When an unexistent path is passed to the filter, it should return the same path
"""
path = microsite.microsite_template_path('footer.html')
self.assertEqual("footer.html", path)
16 changes: 15 additions & 1 deletion lms/startup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ def run():
if settings.FEATURES.get('ENABLE_THIRD_PARTY_AUTH', False):
enable_third_party_auth()

if settings.FEATURES.get('USE_MICROSITES', False):
enable_microsites_pre_startup()

django.setup()

autostartup()
Expand Down Expand Up @@ -116,6 +119,18 @@ def enable_stanford_theme():
settings.LOCALE_PATHS = (theme_root / 'conf/locale',) + settings.LOCALE_PATHS


def enable_microsites_pre_startup():
"""
The TEMPLATE_ENGINE directory to search for microsite templates
in non-mako templates must be loaded before the django startup
"""
microsites_root = settings.MICROSITE_ROOT_DIR
microsite_config_dict = settings.MICROSITE_CONFIGURATION

if microsite_config_dict:
settings.DEFAULT_TEMPLATE_ENGINE['DIRS'].append(microsites_root)


def enable_microsites():
"""
Enable the use of microsites, which are websites that allow
Expand Down Expand Up @@ -151,7 +166,6 @@ def enable_microsites():

# if we have any valid microsites defined, let's wire in the Mako and STATIC_FILES search paths
if microsite_config_dict:
settings.DEFAULT_TEMPLATE_ENGINE['DIRS'].append(microsites_root)
edxmako.paths.add_lookup('main', microsites_root)

settings.STATICFILES_DIRS.insert(0, microsites_root)
Expand Down
6 changes: 3 additions & 3 deletions lms/templates/main_django.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
{% block headextra %}{% endblock %}
{% render_block "css" %}

{% optional_include "head-extra.html" %}
{% optional_include "head-extra.html"|microsite_template_path %}

<meta name="path_prefix" content="{{EDX_ROOT_URL}}">
</head>
Expand All @@ -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 %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@felipemontoya if you can address this issue today, that would be great.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then It means, our microsite templates should have ## mako in first line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

{% endwith %}
<div class="content-wrapper" id="content">
{% block body %}{% endblock %}
{% block bodyextra %}{% endblock %}
</div>
{% with course=request.course %}
{% include "footer.html" %}
{% include "footer.html"|microsite_template_path %}
{% endwith %}

</div>
Expand Down