-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Convert discussion block's html from mako template into django template format #37662
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
Thanks for the pull request, @salman2013! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
8c884bf to
b9d018e
Compare
|
Sandbox deployment failed 💥 |
|
Sandbox deployment successful 🚀 |
|
Sandbox deployment successful 🚀 |
farhan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have some queries
| <span class="forum-nav-browse-title">${_("Posts I'm Following")}</span> | ||
| <span class="forum-nav-browse-title">{% trans "Posts I'm Following" as tmsg%} {{tmsg|force_escape}}</span> | ||
| </li> | ||
| ${HTML(render_dropdown(category_map, []))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this removal intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it was intentional as django templates do not support calling Python functions directly or embedding HTML that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I converted the render_dropdown code into django template formate and include the file in replacement of this.
The code is actually recursive methods calling which is not supported in django template so the possible solution i device is to make separate templates
_dropdown_category.html
_dropdown_items.html
_dropdown_entry.html
which include them each other in replacement of method calling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing results:
To make sure the converted code is working as expected i tested it by commented the code (not including category) first and then with including the category.
When we comment the code
| {% include "discussion/_dropdown_items.html" with map=category_map topic_list="" %} |
With conversion of render_dropdown into django templates and including file the category option is working as expected.


| %> | ||
| {% load i18n %} | ||
|
|
||
| <%def name="render_dropdown(map, topic_list)"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
render_dropdown seems not converted and was used at the end of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking into it.
Interestingly functionality is working as expected without it.
Reason is this template _filter_dropdown.html is including inside discussion_board_fragment.html
edx-platform/lms/djangoapps/discussion/templates/discussion/discussion_board_fragment.html
Line 58 in 00f72bb
| <%include file="_filter_dropdown.html" /> |
it is not passing any category and topic list so this render_dropdown is doing nothing.
As this is legacy discussion UI so i did not convert that part into django template.
I looked into the actual implementation for the finding its functionality just sharing here for reference
795c116
3de6e77
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I corrected my understanding regarding the context passing, the context passes to include tag like <%include file="_filter_dropdown.html" /> from parent template automatically.
I converted the code into django template.
The code is actually recursive methods calling which is not supported in django template so the possible solution i device is to make separate templates
_dropdown_category.html
_dropdown_items.html
_dropdown_entry.html
which include them each other in replacement of method calling.
Testing results:
#37662 (comment)
| % if child in map["entries"] and c_type == TYPE_ENTRY: | ||
| ${HTML(render_entry(map["entries"], child, topic_list))} | ||
| %else: | ||
| ${HTML(render_category(map["subcategories"], child, topic_list))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
render_category and render_entry also seems not converted.
|
Sandbox deployment successful 🚀 |
|
Sandbox deployment successful 🚀 |
|
Sandbox deployment successful 🚀 |
|
Sandbox deployment successful 🚀 |
|
Sandbox deployment successful 🚀 |

Description
In this PR i converted discussion's html files from mako into django template.
I converted following files.
_discussion_inline.html
_discussion_inline_studio.html
_filter_dropdown.html
_thread_list_template.html
_underscore_templates.html
discussion_board_fragment.html
Ticket Reference:
#36539
Acceptance criteria
How to test this PR
To enable discussion xblock:
To show the builtin discussion xblock component on unit page:
To show the legacy discussion view from discussion tab:
Testing results
Sanbox testing results.
Note: The voting option is not working on master branch too, i did not look into that as this comes out of the scope of this PR, i will look into extraction of discussion block.
