Remove Jinja and use Tornado templates#521
Conversation
| @web.authenticated | ||
| @gen.coroutine | ||
| def get(self): | ||
| self.render('admin/page.html') |
There was a problem hiding this comment.
I changed the name of the admin and user page files because when doing {% extends "page.html" %} they were loading themselves resulting on an infinite loop.
| @@ -1,6 +1,6 @@ | |||
| {% extends "page.html" %} | |||
|
|
|||
| {% set skin = "skin-red" %} | |||
There was a problem hiding this comment.
With tornado {% set skin = ... %} only sets a local variable which is not available in the parent scope. Unfortunately, according to tornadoweb/tornado#1323, an equivalent implementation in Tornado of the behavior of Jinja is not available yet. A workaround is to use a block.
| {{message}} | ||
| </p> | ||
| {% endif %} | ||
| {% if message_html %} |
There was a problem hiding this comment.
I couldn't find any other reference to message_html, maybe an old unused anymore variable ?
|
|
||
| <script src="{{ static_url("dist/sharedDependencies.js") }}"></script> | ||
|
|
||
| <script type="text/javascript"> |
There was a problem hiding this comment.
In tornado we don't have the possibility to call super () in blocks, moving this script out of the block script_init was the only solution that I found.
Codecov Report
@@ Coverage Diff @@
## master #521 +/- ##
==========================================
- Coverage 95.25% 95.18% -0.07%
==========================================
Files 88 85 -3
Lines 3961 3905 -56
Branches 248 247 -1
==========================================
- Hits 3773 3717 -56
Misses 137 137
Partials 51 51
Continue to review full report at Codecov.
|
| <script type="text/javascript"> | ||
| window.apidata = { | ||
| base_url: "{{base_url}}", | ||
| prefix: "{{prefix}}", |
There was a problem hiding this comment.
Same here, I couldn't find any other reference to prefix
There was a problem hiding this comment.
Jinja wasn't complaining but tornado is less permissive. With tornado code in the brackets is python code so it raises a name "prefix" is not defined
|
Checked it once. I am unsure about a few things. Leaving for later. |
| </li> | ||
| </ul> | ||
| </div> | ||
| {% if user is not None %} |
There was a problem hiding this comment.
When do we have user == None? (maybe we already talked about it, but I don't remember)
There was a problem hiding this comment.
I had it when rendering the error.html template
There was a problem hiding this comment.
ok, that's suspicious... why?
There was a problem hiding this comment.
You're right, I shouldn't because the render function sets the user. So maybe I'm wrong, maybe I had an other reason but I can't remember..
There was a problem hiding this comment.
Ok, I suspect that it's because if you login and another user connects to the server, user is not defined (you are not passing the right credentials for the process) and in that case you issue an error message. but the error message cannot be shown because it depends on a template that requires the user.
So the best solution is to have an error message with a different base template.
There was a problem hiding this comment.
Should we do that in this PR ?
There was a problem hiding this comment.
Yes please, it makes sense that we rationalise our templates. They are going away anyway, but as a stepstone it's probably the best strategy
Closes #474