Render lms main navigation (tabs) with template #11076

dmitry-viskov · 2015-12-28T16:19:45Z

The reason for this pull requests is the conversation with @singingwolfboy in Slack:
https://openedx.slack.com/archives/theming/p1449519743000017

Someone wants to rename the “Courseware” tab in the LMS navigation and for now there is no ability to make it easy (only one way to do it is to override the entire template).

With this little improvement every tab could be redefined (for example in microsite theme) with adding specific template (by default courseware/tab_default.html will be taken):

courseware/tab_courseware.html 
courseware/tab_info.html 
courseware/tab_discussion.html 
courseware/tab_wiki.html 
courseware/tab_progress.html
courseware/tab_instructor.html

singingwolfboy · 2015-12-28T16:26:29Z

@strannikk: we use a rebase workflow with the Open edX codebase, not a merge workflow. You have three merge commits in this pull request. Can you take them out? The easiest way to do that would probably be to reset your branch to the tip of edx/master and then cherry-pick the one non-merge commit that you have in this pull request, and then force-push to update the pull request.

This page might also be helpful: https://github.com/edx/edx-platform/wiki/How-to-Rebase-a-Pull-Request

openedx-webhooks · 2015-12-28T16:27:15Z

Thanks for the pull request, @strannikk! I've created OSPR-1044 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:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

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.

We can't start reviewing your pull request until you've submitted a signed contributor agreement or indicated your institutional affiliation. You haven't added yourself to the AUTHORS file for this repo. Please create a new pull request to do so! Please see the CONTRIBUTING file for more information.

dmitry-viskov · 2015-12-28T19:47:59Z

@singingwolfboy I've removed merge commits, please check

singingwolfboy · 2015-12-28T20:00:51Z

common/djangoapps/pipeline_mako/templates/static_content.html

@@ -99,3 +99,18 @@
 else:
    tmpl.render_context(context)
 %></%def>
+
+<%def name="optional_include_mako_with_fallback(file, fallback, with_microsite=False, **kwargs)"><%


Why not just fold this functionality into the optional_include_mako function that already exists? If the file isn't found, check and see if a fallback was specified. If so, try to use it; if not, pass.

Also, what happens if the template author wants to define a list of fallbacks?

@singingwolfboy do you suggest to extend optional_include_mako function with fallback and kwargs params? It seemed that in slack discussion you first offered to create separated function like optional_include_mako_with_fallback with additional arguments.
Ok I will extend existed function with new arguments.

Also, what happens if the template author wants to define a list of fallbacks?

Do you suggest to provide fallback param as a list of pathes to templates? I can do it but I think that one fallback template will be sufficient for the 99% cases.

You're right that I suggested that in chat, but I was throwing out a suggestion without really considering the implications. Now that I see the code, I think it would be better to fold this improvement into the original function. (Sometimes my off-the-cuff suggestions are wrong. Sorry! 😜)

You're right that one fallback template will probably be sufficient in many cases, but probably not all cases. We should consider how we want to handle those other cases. I'm open to suggestions -- maybe we should optimize for the "one fallback" case, and have a separate argument that is used for multiple fallbacks. Maybe we shouldn't handle multiple fallbacks at all, and let the template author write the logic into the template. But I suspect there's something we can do to make it at least a little bit cleaner.

As for me there are two possible variants: 1. shouldn't handle multiple fallbacks at all (template author should provide provide the logic into his template), 2. to consider fallback argument as a list of templates.
The variant with two separate arguments (one for the first fallback and one for the others) looks strange. Also we could consider fallback param as one fallback if it was passed as a string and as a group of fallbacks if it was passed as a list. But I think this variant strange and non-intuitive as well.

FWIW, I like the solution of the fallback argument being either a string or a list of strings (which could of course have a single element.)

@dpowell string_or_list argument could be a solution but for me it looks like a little bit unpythonic. The philosophy of python is based on the couple of principles. "Explicit is better than implicit" one of them. So as for my opinion if function always expects an argument as a list it is more explicit (even this list consists of only one element).

dmitry-viskov · 2015-12-29T14:15:22Z

@singingwolfboy @dpowell I've updated the pull requests according to your comments. About fallback argument - I've left it without changes.

singingwolfboy · 2015-12-30T18:46:39Z

lms/templates/courseware/course_navigation.html

@@ -86,21 +86,11 @@
    <ol class="course-tabs">
      % for tab in get_course_tab_list(request, course):
        <%
-            tab_is_active = (tab.tab_id == active_page) or (tab.tab_id == default_tab)
+        tab_is_active = (tab.tab_id == active_page) or (tab.tab_id == default_tab)


Nitpick: this can be shortened to:

tab_is_active = tab.tab_id in (active_page, default_tab)

ok, I'll change this string. But this behavior was before my pull request :-)

jbarciauskas · 2016-01-12T15:38:18Z

@nedbat FYI - not sure if this should go to you or someone else, just FYI

dpowell · 2016-01-13T13:15:36Z

What are the next steps on this pull request?

jbarciauskas · 2016-01-13T15:00:15Z

@dpowell Since DB has left, we just need to figure out who the right person is to pick up this review. It might be @nedbat but he's pretty heavily involved in getting Dogwood out the door at the moment.

nedbat · 2016-01-27T19:30:38Z

We should give this to T&L.

cahrens · 2016-01-28T18:02:58Z

As I indicated in the JIRA ticket, I think this PR should be reviewed by @mattdrayer on the Solutions team.

mattdrayer · 2016-01-28T18:09:39Z

@ziafazal @saleem-latif -- please take a look at this PR when you have a moment

ziafazal · 2016-01-29T08:09:54Z

lms/templates/courseware/course_navigation.html

@@ -86,21 +86,11 @@
    <ol class="course-tabs">
      % for tab in get_course_tab_list(request, course):


I think we need to simply things. We should provide ability to override all course tabs instead of each single tab with a separate template file.
we can include a tabs file here like this
<%static:optional_include_mako file="courseware/tabs.html" >
Then inside tabs.html we should handle rendering of tabs like this

<ol class="course-tabs"> % for tab in get_course_tab_list(request, course): ................... ................... ................... </ol>

ziafazal · 2016-01-29T08:11:11Z

@mattdrayer @strannikk this PR lacks test coverage in specially with course tab overrides.

dmitry-viskov · 2016-01-29T11:22:26Z

@ziafazal I've already created the test "test_optional_include_mako_with_fallback": https://github.com/edx/edx-platform/pull/11076/files#diff-63196f493f5c1eaf461be34d34bf5320R18
Or do you mean that there will be needed tests in case of overriding course tabs instead of each single tab?

ziafazal · 2016-01-29T11:27:08Z

@strannikk yeah tests for tab overrides.

mattdrayer · 2016-02-01T01:29:59Z

@strannikk looks like you need a rebase as well

ziafazal · 2016-02-10T13:20:09Z

lms/templates/courseware/course_navigation.html

+          tabs_tmpl = '/' + base_tabs_tpl
+      tab_list = get_course_tab_list(request, course)
+      %>
+     <%include file="${tabs_tmpl}" args="tab_list=tab_list,active_page=active_page,default_tab=default_tab,tab_image=tab_image" />


@strannikk I don't think you need to pass everything in args. courseware/tabs.html should have the same context as the container template.

@ziafazal it doesn't work.
Exception 'Undefined' object is not iterable will always be raised.
As I could see the same approach used in many templates in edx. Example:
https://github.com/edx/edx-platform/blob/90f2fb868e0646733fad8ec615af2d2dec11801e/cms/templates/base.html#L59

dmitry-viskov · 2016-02-11T12:19:34Z

@mattdrayer @saleem-latif @ziafazal @jbarciauskas What are the next steps with this PR?

nedbat · 2016-02-11T19:48:16Z

jenkins run all

ziafazal · 2016-02-15T07:52:37Z

jenkins run bokchoy

ziafazal · 2016-02-15T07:53:24Z

jenkins run all

dmitry-viskov · 2016-02-15T13:13:09Z

@ziafazal I've updated the PR according your comments but now the test:

./manage.py lms test --verbosity=2 lms/djangoapps/courseware/tests/test_microsites.py:TestMicrosites.test_microsite_course_custom_tabs --settings=test

is always failed. So I'm waiting for your fix WL-320. As soon as it will be in master, I'll rebase it with my branch and this test will be set right.

Also I'm fixed the failed DiscussionHomePageTest:

paver test_bokchoy -t discussion/test_discussion.py:DiscussionHomePageTest --fasttest

but because of this I had to move <ol> </ol> tags from the tabs.html template

…or a specific list item

dmitry-viskov · 2016-02-16T14:32:35Z

@ziafazal I've rebased the PR with the last master (with your get_template_path fix). Could you please run tests again ?

mattdrayer · 2016-02-16T14:33:45Z

jenkins run all

openedx-webhooks · 2016-02-16T14:34:15Z

Hello @strannikk: We are unable to continue with review of your submission at this time. Please see the associated JIRA ticket for more explanation.

mattdrayer · 2016-02-16T14:34:35Z

jenkins run all

benpatterson · 2016-02-16T15:15:42Z

jenkins ok to test

benpatterson · 2016-02-16T15:16:07Z

jenkins add to whitelist

dmitry-viskov · 2016-02-16T16:22:53Z

@mattdrayer @saleem-latif @ziafazal @jbarciauskas @benpatterson all tests are green. Is there anything else I can do with this PR?

jbarciauskas · 2016-02-16T17:57:16Z

@strannikk I'll put this back into awaiting review. @mattdrayer @ziafazal who will give the final thumbs up on this?

mattdrayer · 2016-02-16T22:15:23Z

I'll leave the final thumbs up to @ziafazal on this one -- he has the most context at this point.

ziafazal · 2016-02-17T07:08:14Z

LGTM 👍

jbarciauskas · 2016-02-17T14:29:44Z

k, who's going to click the green button? :) @ziafazal @mattdrayer

…ion-with-template Render lms main navigation (tabs) with template

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Dec 28, 2015

dmitry-viskov force-pushed the render-lms-main-navigation-with-template branch from 02a463e to 1179c82 Compare December 28, 2015 19:31

singingwolfboy reviewed Dec 28, 2015
View reviewed changes

openedx-webhooks added engineering review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Dec 28, 2015

dmitry-viskov force-pushed the render-lms-main-navigation-with-template branch from 8d616c5 to 5bfa11b Compare December 29, 2015 15:01

singingwolfboy reviewed Dec 30, 2015
View reviewed changes

dmitry-viskov force-pushed the render-lms-main-navigation-with-template branch from 5bfa11b to bad82a6 Compare December 31, 2015 11:21

openedx-webhooks added awaiting prioritization and removed engineering review labels Jan 27, 2016

ziafazal reviewed Jan 29, 2016
View reviewed changes

ziafazal reviewed Feb 10, 2016
View reviewed changes

ziafazal mentioned this pull request Feb 15, 2016

WL-320: fixed get_template_path to work with absolute path also #11548

Merged

dmitry-viskov force-pushed the render-lms-main-navigation-with-template branch from 55335c5 to f5c80f0 Compare February 15, 2016 12:59

dmitry-viskov force-pushed the render-lms-main-navigation-with-template branch from f5c80f0 to bd0af7f Compare February 15, 2016 13:16

Render LMS main navigation (tabs) with template, override templates f…

d532f8d

…or a specific list item

dmitry-viskov force-pushed the render-lms-main-navigation-with-template branch from bd0af7f to d532f8d Compare February 16, 2016 14:30

mattdrayer closed this Feb 16, 2016

mattdrayer reopened this Feb 16, 2016

openedx-webhooks closed this Feb 16, 2016

mattdrayer reopened this Feb 16, 2016

openedx-webhooks added the awaiting prioritization label Feb 16, 2016

ziafazal added a commit that referenced this pull request Feb 18, 2016

Merge pull request #11076 from CredoReference/render-lms-main-navigat…

e8ccc2f

…ion-with-template Render lms main navigation (tabs) with template

ziafazal merged commit e8ccc2f into openedx:master Feb 18, 2016

dmitry-viskov deleted the render-lms-main-navigation-with-template branch May 19, 2017 12:53

		@@ -86,21 +86,11 @@
		<ol class="course-tabs">
		% for tab in get_course_tab_list(request, course):

Render lms main navigation (tabs) with template #11076

Render lms main navigation (tabs) with template #11076

Conversation

dmitry-viskov commented Dec 28, 2015

singingwolfboy commented Dec 28, 2015

openedx-webhooks commented Dec 28, 2015

dmitry-viskov commented Dec 28, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-viskov commented Dec 29, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbarciauskas commented Jan 12, 2016

dpowell commented Jan 13, 2016

jbarciauskas commented Jan 13, 2016

nedbat commented Jan 27, 2016

cahrens commented Jan 28, 2016

mattdrayer commented Jan 28, 2016

Choose a reason for hiding this comment

ziafazal commented Jan 29, 2016

dmitry-viskov commented Jan 29, 2016

ziafazal commented Jan 29, 2016

mattdrayer commented Feb 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-viskov commented Feb 11, 2016

nedbat commented Feb 11, 2016

ziafazal commented Feb 15, 2016

ziafazal commented Feb 15, 2016

dmitry-viskov commented Feb 15, 2016

dmitry-viskov commented Feb 16, 2016

mattdrayer commented Feb 16, 2016

openedx-webhooks commented Feb 16, 2016

mattdrayer commented Feb 16, 2016

benpatterson commented Feb 16, 2016

benpatterson commented Feb 16, 2016

dmitry-viskov commented Feb 16, 2016

jbarciauskas commented Feb 16, 2016

mattdrayer commented Feb 16, 2016

ziafazal commented Feb 17, 2016

jbarciauskas commented Feb 17, 2016