Edgewall Software
Modify ↓

Opened 8 years ago

Closed 8 years ago

Last modified 5 years ago

#12639 closed enhancement (fixed)

Integrate Jinja2 branch

Reported by: Christian Boos Owned by: Christian Boos
Priority: high Milestone: 1.3.2
Component: rendering Version: 1.3dev
Severity: critical Keywords: jinja2 performance +testing
Cc: Branch:
Release Notes:

The template engine used by Trac is now ​Jinja2. Genshi templates rendering in plugins will be supported until Trac 1.5.1.

API Changes:

Major changes in trac.util.html, trac.web.Chrome, trac.web.RequestFilter to support Jinja2. trac.web.ITemplateStreamFilter is deprecated.

Internal Changes:

Description

The work on the switch to the Jinja2 engine is mostly complete on the experimental branch cboos.git@jinja2.

Like explained in TracDev/Proposals/Jinja, this change will adequately solve the performance issues we had with Genshi: pages render from 3x to 10x faster, at a fraction of the cost in memory.

This ticket is about the finalization and review, and the merge of the changes on trunk.

Besides the merge and catching up with the changes that happened on trunk since the merge-base (r14499), the main open points are:

  • get rid of the j* prefix used for the Jinja2 pages and find another mechanism to support both Jinja2 templates and legacy Genshi templates
  • test l10n for template pages in plugins (no domain directive)

Attachments (1)

spamfilter-jinja2-compat-r15349.diff​ (2.6 KB ) - added by Christian Boos 8 years ago.
make the SpamFilter compatible with the jinja2-trunk-r.. branch after [0f3aee46/cboos.git]

Download all attachments as: .zip

Change History (61)

comment:1 by Jun Omae, 8 years ago

Notes: 2 plugins on trac-hacks.org are using template named with j*. 

$ find . -path '*/templates/j*'
./tracticketstatsplugin/trunk/ticketstats/templates/jsondata.html
./tracticketstatsplugin/tags/2.2.0/ticketstats/templates/jsondata.html
./tracjenkinsplugin/trunk/tracjenkins/templates/jobs.html

comment:2 by Christian Boos, 8 years ago

Sure, the 'j' prefix is just a hack that allowed me to have both series of templates in the same folder while making the conversion. It's not supposed to stay.

comment:3 by Christian Boos, 8 years ago

Quick overview of the Python code churn, as it stands (related to #12130): 

$ git diff trunk...jinja2 --stat | grep ".py"
 contrib/jinjachecker.py                            |  433 +++++++
 contrib/jinjastats.py                              |   63 +
 doc/conf.py                                        |    1 +
 sample-plugins/HelloWorld.py                       |    6 +-
 sample-plugins/Timestamp.py                        |    2 +-
 sample-plugins/revision_links.py                   |    3 +-
 sample-plugins/workflow/CodeReview.py              |    3 +-
 sample-plugins/workflow/StatusFixer.py             |    5 +-
 sample-plugins/workflow/VoteOperation.py           |    3 +-
 setup.py                                           |   29 +-
 trac/about.py                                      |    8 +-
 trac/admin/tests/functional.py                     |   20 +-
 trac/admin/web_ui.py                               |    4 +-
 trac/attachment.py                                 |    3 +-
 trac/config.py                                     |    3 +-
 trac/core.py                                       |    2 +-
 trac/db/api.py                                     |    2 +-
 trac/db/mysql_backend.py                           |    3 +-
 trac/db/postgres_backend.py                        |    3 +-
 trac/db/sqlite_backend.py                          |    3 +-
 trac/db/tests/api.py                               |    2 +-
 trac/dist.py                                       |  229 +++-
 trac/env.py                                        |   10 +-
 trac/mimeview/api.py                               |  105 +-
 trac/mimeview/patch.py                             |    6 +-
 trac/mimeview/pygments.py                          |   58 +-
 trac/mimeview/rst.py                               |    4 +-
 trac/mimeview/templates/jprefs_pygments.html       |   72 ++
 trac/mimeview/tests/api.py                         |  113 +-
 trac/mimeview/tests/functional.py                  |    4 +-
 trac/mimeview/tests/patch.py                       |   24 +-
 trac/mimeview/tests/pygments.data                  |   91 ++
 trac/mimeview/tests/pygments.html                  |   57 -
 trac/mimeview/tests/pygments.py                    |   51 +-
 trac/notification/compat.py                        |   20 +-
 trac/notification/mail.py                          |    3 +-
 trac/notification/prefs.py                         |    5 +-
 trac/notification/tests/mail.py                    |    3 +-
 trac/prefs/tests/functional.py                     |    2 +-
 trac/prefs/web_ui.py                               |   24 +-
 trac/resource.py                                   |    2 +-
 trac/search/web_ui.py                              |    9 +-
 trac/tests/compat.py                               |   12 +-
 trac/tests/functional/better_twill.py              |    1 +
 trac/tests/functional/testcases.py                 |    6 +-
 trac/tests/functional/tester.py                    |    2 +-
 trac/tests/resource.py                             |    2 +-
 trac/ticket/api.py                                 |    3 +-
 trac/ticket/batch.py                               |    3 +-
 trac/ticket/default_workflow.py                    |    3 +-
 trac/ticket/notification.py                        |   21 +-
 trac/ticket/query.py                               |   17 +-
 trac/ticket/report.py                              |    3 +-
 trac/ticket/roadmap.py                             |    8 +-
 trac/ticket/tests/conversion.py                    |    8 +-
 trac/ticket/tests/default_workflow.py              |    4 +-
 trac/ticket/tests/functional/admin.py              |   69 +-
 trac/ticket/tests/functional/default_workflow.py   |   40 +-
 trac/ticket/tests/functional/main.py               |  168 +--
 trac/ticket/tests/query.py                         |   87 +-
 trac/ticket/tests/web_ui.py                        |   14 +-
 trac/ticket/web_ui.py                              |   19 +-
 trac/timeline/tests/web_ui.py                      |    6 +-
 trac/timeline/web_ui.py                            |   24 +-
 trac/util/html.py                                  |  905 ++++++++++++--
 trac/util/presentation.py                          |  225 +++-
 trac/util/tests/__init__.py                        |    4 +-
 trac/util/tests/html.py                            |  254 ++--
 trac/util/tests/presentation.py                    |   17 +
 trac/util/text.py                                  |   48 +
 trac/util/translation.py                           |   21 +-
 trac/versioncontrol/admin.py                       |   12 +-
 trac/versioncontrol/diff.py                        |    5 +-
 trac/versioncontrol/tests/functional.py            |   10 +-
 trac/versioncontrol/web_ui/browser.py              |    6 +-
 trac/versioncontrol/web_ui/changeset.py            |   19 +-
 trac/versioncontrol/web_ui/log.py                  |    4 +-
 trac/versioncontrol/web_ui/util.py                 |    3 +-
 trac/web/__init__.py                               |   19 +-
 trac/web/api.py                                    |   35 +-
 trac/web/auth.py                                   |   16 +-
 trac/web/chrome.py                                 |  657 +++++++---
 trac/web/main.py                                   |   36 +-
 trac/web/tests/api.py                              |   19 +-
 trac/web/tests/chrome.py                           |    2 +-
 trac/wiki/api.py                                   |    3 +-
 trac/wiki/formatter.py                             |   43 +-
 trac/wiki/intertrac.py                             |    4 +-
 trac/wiki/interwiki.py                             |    3 +-
 trac/wiki/macros.py                                |    5 +-
 trac/wiki/tests/formatter.py                       |   35 +-
 trac/wiki/tests/functional.py                      |   41 +-
 trac/wiki/tests/macros.py                          |   44 +-
 trac/wiki/web_ui.py                                |   14 +-
 tracopt/ticket/clone.py                            |   61 +-
 tracopt/ticket/commit_updater.py                   |    3 +-
 tracopt/ticket/deleter.py                          |   73 +-
 tracopt/versioncontrol/git/git_fs.py               |    4 +-
 tracopt/versioncontrol/svn/svn_prop.py             |    3 +-
 tracopt/versioncontrol/svn/tests/svn_fs.py         |  148 ++-

comment:4 by Christian Boos, 8 years ago

Owner: set to Christian Boos
Status: new → assigned

I've completed the rebase on latest trunk.

Here's a commented break down of the changesets, to ease reviewing. The changesets are listed oldest first.

Note that I started by squashing all the changes, then created smaller commits according to functional units. While I rebased on trunk, I also added some bug fixes.

Here's the initial infrastructure for the switch to Jinja2:

Quite a few utility functions and classes were added, the lowest level is in trac.util.text (Jinja2 being primarily a text based template engine), then trac.util.html and finally trac.util.presentation:

The integration of Jinja2 then continues in the higher-level common layers of Trac, the wiki formatter and the mimeview API:

All the old direct Genshi imports are converted to the new (backward compatible) ones from trac.util.html:

Now the big piece, enabling the Jinja2 template to be loaded, populated with data and generated:

At this point, we can start to work on the templates proper:

Now we start the conversion module by module, with the occasional commit reflecting the changes on trunk since… last year:

So far we have just a refresh of the original Jinja2 branch, with all the back and forth eliminated.

As I did the merge, I worked on finding a way to be able to render indifferently Jinja2 and legacy Genshi templates, without resorting to the "j" prefixing hack that was convenient during the migration period.

This resulted in some quite important API changes, the main trick being that the "old" API is still supported but if used, it is an indication that Genshi templates should be used. For using Jinja2 templates, the newer API should be used. All the details in the following log message of the following commits:

That's it, thanks for having followed so far, and please give it a shot. The integration is really overdue, I've been using the branch successfully for the past 6 months without major issues, and it's time to spread to good news ;-)

I'm particularly interested in feedback on the last two important API changes commits, as it's quite new and I suspect there are still a few improvements possible.

Another area which still needs work is i18n, as I haven't looked closely there since a while. TODO

Finally, Jinja 2.9 is out (or will be out soon), and it comes with a bug fix I've been waiting for all along, so I plan to adapt to that version and make it a minimal requirement. TODO 

$ git diff --stat trunk jinja2-trunk-r15341
 ...
 214 files changed, 15353 insertions(+), 1877 deletions(-)

Well, when we'll remove most of the Genshi templates, that will also give some more deletions!

in reply to:  4 comment:5 by Christian Boos, 8 years ago

Replying to Christian Boos:

Finally, Jinja 2.9 is out

It's out and already used for the AppVeyor and Travis builds. I guess the errors in the functional tests seen there must be due to this new version, as I've not tested 2.9 myself yet.

comment:6 by Christian Boos, 8 years ago

Keywords: +testing added

The branch can now be seen in action in testing.

For example, this are the timings for 00faab14 with 30 lines of context:

testing t.e.o
Waiting (TTFB) 9.65 s 27.25 s
Content Download 409.63 ms 598.59 ms

(measured with the network panel in Chrome)

At my first try, I didn't see much speed difference between the two, so I increased the context to 30 lines, hoping the difference would be more noticeable. It was, as I got a MemoryError as a result! Then I realized we have the SpamFilter installed there, and one of its filter uses the ITemplateStreamFilter interface. Once I disabled that tracspamfilter.filters.trapfield.* component, the results were again in line with the expectations (i.e. what you see in the table above).

So I hope that this will be a compelling argument for people to upgrade their plugins and get rid of the ITemplateStreamFilter interface support. As soon as you have one active, you lose the performance improvements gained with the switch to Jinja2. And the hack for supporting the stream filters seems to be quite memory hungry.

by Christian Boos, 8 years ago

Attachment: spamfilter-jinja2-compat-r15349.diff​ added

make the SpamFilter compatible with the jinja2-trunk-r.. branch after [0f3aee46/cboos.git]

comment:7 by Christian Boos, 8 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: → fixed
Status: assigned → closed

Branch landed on trunk today [15406-15474].

comment:8 by Jun Omae, 8 years ago

I think we should use extractor marker (${_('...')}) for the following title attributes except Genshi templates, at least. 

$ git grep 'title="[^$]' -- $(find trac tracopt -name '*.html') | cat
trac/templates/genshi/history_view.html:                <a href="${item.url or url_of(resource, version=item.version)}" title="View this version">$item.version</a>
trac/templates/genshi/list_of_attachments.html:      <a href="${url_of(attachment.resource)}" title="View attachment">${attachment.filename
trac/templates/genshi/list_of_attachments.html:                class="trac-rawlink" title="Download">&#8203;</a><!--!
trac/templates/genshi/theme.html:        <a class="trac-close-msg" href="#" title="Hide this warning"><span>close</span></a>
trac/templates/genshi/theme.html:        <a class="trac-close-msg" href="#" title="Hide this notice"><span>close</span></a>
trac/ticket/templates/admin_milestones.html:        <input type="submit" title="Clear default ticket milestone and default retargeting milestone"
trac/ticket/templates/query_results.html:        <td class="id"><a href="${result.href}" title="View ticket"
trac/ticket/templates/report_view.html:              <a title="View milestone" href="${href.milestone(cell.value)}">${
trac/ticket/templates/ticket_change.html:                 title="Revert this change">${
trac/ticket/templates/ticket_change.html:           title="Cancel comment edit"/>
trac/versioncontrol/templates/browser.html:            <a href="${rcset}" title="View differences">Last change</a>
trac/versioncontrol/templates/changeset_content.html:  <a title="Show entry in browser" href="${item.new.href}">
Last edited 8 years ago by Jun Omae (previous) (diff)

comment:9 by Christian Boos, 8 years ago

API Changes: modified (diff)

comment:10 by Ryan J Ollos, 8 years ago

I updated 1.3/TracInstall, but it looks like there are some changes in r15406 that may need to be added to 1.3/TracInstall and 1.3/TracUpgrade (INSTALL.rst and UPGRADE.rst are generated from the wiki pages).

in reply to:  10 comment:11 by Ryan J Ollos, 8 years ago

Replying to Ryan J Ollos:

I updated 1.3/TracInstall, but it looks like there are some changes in r15406 that may need to be added to 1.3/TracInstall and 1.3/TracUpgrade (INSTALL.rst and UPGRADE.rst are generated from the wiki pages).

Applied r15406 changes in 1.3/TracInstall@5 and 1.3/TracUpgrade@2.

UPGRADE.rst and INSTALL.rst updated in r15482.

in reply to:  8 comment:12 by anonymous, 8 years ago

Replying to Jun Omae:

I think we should use extractor marker (${_('...')}) for the following title attributes except Genshi templates, at least.

Thanks! Fixed that in r15483.

Also, you're right, better standardize on single quotes here, as we're already within the double quotes of the attributes. This is fixed in r15484.

comment:13 by Christian Boos, 8 years ago

As I fixed one glitch seen in the roadmap (r15486), I was wondering if instead of having both _() and tag_(), we shouldn't instead have only one (_()), but doing what _tag() does, i.e. tgettext.

It's probably slightly less efficient, but would simplify life for template writers and be less error prone.

comment:14 by Jun Omae, 8 years ago

I received notification mail with Re: $prefix #$ticket.id: $summary in the subject. The subject template is not expanded.

Order of buttons in each ticket change is changed with Trac 1.2. I think the order should be the same with Trac 1.2.

  • Trac 1.2 - [Reley] [Edit] [Delete]
  • Trac 1.3dev - [Clone] [Delete] [Reply] [Edit]

comment:15 by Christian Boos, 8 years ago

Another glitch with the ticketclone.js: the old_values dict is not present for new tickets. The ticket clone feature should probably not kick in for /newticket. FIXED in r15509

Last edited 8 years ago by Christian Boos (previous) (diff)

in reply to:  14 ; comment:16 by Christian Boos, 8 years ago

Replying to Jun Omae:

I received notification mail with Re: $prefix #$ticket.id: $summary in the subject. The subject template is not expanded.

Should be fixed? Yes.

Last edited 8 years ago by Christian Boos (previous) (diff)

in reply to:  14 ; comment:17 by Christian Boos, 8 years ago

Replying to Jun Omae:

Order of buttons in each ticket change is changed with Trac 1.2. I think the order should be the same with Trac 1.2.

  • Trac 1.2 - [Reley] [Edit] [Delete]
  • Trac 1.3dev - [Clone] [Delete] [Reply] [Edit]

The rightmost action is also the easier to click on, so I'm not sure it's best to have [Delete] there. I'd rather have [Clone] [Delete] [Edit] [Reply]. I'll address this topic in #12671.

comment:18 by Ryan J Ollos, 8 years ago

Found an issue with /report/6?asc=1&format=rss&USER=anonymous through the logs: 

Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/trac/web/main.py", line 630, in _dispatch_request
    dispatcher.dispatch(req)
  File "build/bdist.linux-x86_64/egg/trac/web/main.py", line 273, in dispatch
    method=method)
  File "build/bdist.linux-x86_64/egg/trac/web/chrome.py", line 1374, in render_template
    fragment, iterable, method)
  File "build/bdist.linux-x86_64/egg/trac/web/chrome.py", line 1724, in _render_genshi_template
    doctype, data, iterable)
  File "build/bdist.linux-x86_64/egg/trac/web/chrome.py", line 1751, in _send_genshi_content
    encoding='utf-8')
  File "/usr/local/lib/python2.7/dist-packages/genshi/core.py", line 184, in render
    return encode(generator, method=method, encoding=encoding, out=out)
  File "/usr/local/lib/python2.7/dist-packages/genshi/output.py", line 58, in encode
    for chunk in iterator:
  File "/usr/local/lib/python2.7/dist-packages/genshi/output.py", line 241, in __call__
    for kind, data, pos in stream:
  File "/usr/local/lib/python2.7/dist-packages/genshi/output.py", line 669, in __call__
    for kind, data, pos in stream:
  File "/usr/local/lib/python2.7/dist-packages/genshi/output.py", line 774, in __call__
    for kind, data, pos in chain(stream, [(None, None, None)]):
  File "/usr/local/lib/python2.7/dist-packages/genshi/output.py", line 594, in __call__
    for ev in stream:
  File "/usr/local/lib/python2.7/dist-packages/genshi/core.py", line 289, in _ensure
    for event in stream:
  File "/usr/local/lib/python2.7/dist-packages/genshi/template/base.py", line 618, in _include
    for event in stream:
  File "/usr/local/lib/python2.7/dist-packages/genshi/template/markup.py", line 326, in _match
    for event in stream:
  File "/usr/local/lib/python2.7/dist-packages/genshi/template/base.py", line 578, in _flatten
    result = _eval_expr(data, ctxt, vars)
  File "/usr/local/lib/python2.7/dist-packages/genshi/template/base.py", line 289, in _eval_expr
    retval = expr.evaluate(ctxt)
  File "/usr/local/lib/python2.7/dist-packages/genshi/template/eval.py", line 177, in evaluate
    return eval(self.code, _globals, {'__data__': data})
  File "/tmp/.eggs-1.3dev/Trac-1.3.2.dev0-py2.7.egg-tmp/trac/ticket/templates/report.rss", line 30, in <Expression u'http_date(from_utimestamp(long(cell.value)))'>
    <pubDate>${http_date(from_utimestamp(long(cell.value)))}</pubDate>
  File "/usr/local/lib/python2.7/dist-packages/genshi/template/eval.py", line 317, in lookup_attr
    val = getattr(obj, key)
  File "/usr/local/lib/python2.7/dist-packages/genshi/template/eval.py", line 278, in _die
    raise UndefinedError(self._name, self._owner)
UndefinedError: "cell" not defined

comment:19 by Ryan J Ollos, 8 years ago

Here's another traceback found in logs: 

[pid 23091 140352200652544] 2017-02-09 15:07:52,026 Trac[chrome] ERROR: Jinja2 UnicodeEncodeError error while rendering XML/HTML template 
Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/trac/web/chrome.py", line 1624, in _iterable_jinja_content
    for chunk in stream:
  File "/usr/local/virtualenv/1.3dev/lib/python2.7/site-packages/jinja2/environment.py", line 1271, in __next__
    return self._next()
  File "/usr/local/virtualenv/1.3dev/lib/python2.7/site-packages/jinja2/environment.py", line 1248, in _buffered_generator
    c = next(self._gen)
  File "/usr/local/virtualenv/1.3dev/lib/python2.7/site-packages/jinja2/environment.py", line 1045, in generate
    yield self.environment.handle_exception(exc_info, True)
  File "/usr/local/virtualenv/1.3dev/lib/python2.7/site-packages/jinja2/environment.py", line 780, in handle_exception
    reraise(exc_type, exc_value, tb)
  File "/tmp/.eggs-1.3dev/Trac-1.3.2.dev0-py2.7.egg-tmp/trac/ticket/templates/ticket.rss", line 53, in top-level template code
    ${_("changed from %(old)s to %(new)s",
UnicodeEncodeError: 'ascii' codec can't encode character u'\xea' in position 41: ordinal not in range(128)

comment:20 by Christian Boos, 8 years ago

Both issues (comment:18 and comment:19) should be fixed with r15521.

comment:21 by Peter Suter, 8 years ago

The Genshi template​ for notification preferences had a form and a special Save button with action replace_all. The Jinja template​ seems to be missing that, so it falls back to the default Save button​ with action save. But that action is not expected​ so saving notification preference changes doesn't work.

in reply to:  21 comment:22 by Christian Boos, 8 years ago

Replying to Peter Suter:

You're right, sorry for the mistake. Fixed in r15528, and as in retrospect I found the name of the prefpanel_has_forms flag to be misleading, I renamed it in r15529.

in reply to:  17 comment:23 by Jun Omae, 8 years ago

Replying to Christian Boos:

Replying to Jun Omae:

Order of buttons in each ticket change is changed with Trac 1.2. I think the order should be the same with Trac 1.2.

  • Trac 1.2 - [Reley] [Edit] [Delete]
  • Trac 1.3dev - [Clone] [Delete] [Reply] [Edit]

The rightmost action is also the easier to click on, so I'm not sure it's best to have [Delete] there. I'd rather have [Clone] [Delete] [Edit] [Reply]. I'll address this topic in #12671.

Agreed [Clone] [Delete] [Edit] [Reply]. I think the reply button as common used button should be leftmost or rightmost action.

(I just clicked edit button when I'm deleting spam comment because the order has been changed. It annoyed me a bit.)

Last edited 8 years ago by Jun Omae (previous) (diff)

comment:24 by Ryan J Ollos, 8 years ago

The rows of the id column on the query results page can have class="". Proposed fix (TracDev/PortingFromGenshiToJinja#htmlattr):

  • trac/ticket/templates/query_results.html 

    diff --git a/trac/ticket/templates/query_results.html b/trac/ticket/templates/query_results.html
index fddcad310..b071f40b4 100644
    a b  
    118118        #       set wikitextarea = (header.field.type == 'textarea' and
    119119                                    header.field.format == 'wiki')
    120120        #       if name == 'id':
    121         <td class="id"><a href="${result.href}" title="${_('View ticket')}"
    122                           class="${classes(closed=result.status == 'closed')}"
    123                           >#${result.id}</a></td>
     121        <td class="id"><a href="${result.href}" title="${_('View ticket')}"${
     122                          {'class': 'closed' if result.status == 'closed'}
     123                          |htmlattr}>#${result.id}</a></td>
    124124        #       else:
    125125        <td class="${name}">
    126126          #       if name == 'summary':
Last edited 8 years ago by Ryan J Ollos (previous) (diff)

comment:25 by Ryan J Ollos, 8 years ago

comment:24 changes committed in r15565.

comment:26 by Ryan J Ollos, 8 years ago

SpamFilter r15353 seems to be working well. Do you think it's ready to merge back to plugins/trunk/spam-filter​?

in reply to:  26 ; comment:27 by Ryan J Ollos, 8 years ago

Replying to Ryan J Ollos:

SpamFilter r15353 seems to be working well. Do you think it's ready to merge back to plugins/trunk/spam-filter​?

Reintegrated in r15652.

in reply to:  27 comment:28 by Christian Boos, 8 years ago

Replying to Ryan J Ollos:

Replying to Ryan J Ollos:

SpamFilter r15353 seems to be working well. Do you think it's ready to merge back to plugins/trunk/spam-filter​?

Reintegrated in r15652.

Thanks! Yes, that was the idea.

comment:29 by Ryan J Ollos, 8 years ago

Minor issue: The Attachments section will not expand when in Wiki page edit mode.

comment:30 by Ryan J Ollos, 8 years ago

comment:29 issue fixed in r15697. enableFolding is called in wiki.js: trunk/trac/htdocs/js/wiki.js@15588:77#L72​.

comment:31 by Ryan J Ollos, 8 years ago

We could change the default value for ticket_subject_template and batch_subject_template on 1.0-stable and 1.2-stable to be compatible with the Jinja2 changes:

  • trac/ticket/notification.py 

    diff --git a/trac/ticket/notification.py b/trac/ticket/notification.py
index 23cb9ec32..92957d016 100644
    a b class TicketNotificationSystem(Component):  
    6161        pass
    6262
    6363    ticket_subject_template = Option('notification', 'ticket_subject_template',
    64                                      '$prefix #$ticket.id: $summary',
     64                                     '${prefix} #${ticket.id}: ${summary}',
    6565        """A Genshi text template snippet used to get the notification
    6666        subject.
    6767
    … … class TicketNotificationSystem(Component):  
    7070        """)
    7171
    7272    batch_subject_template = Option('notification', 'batch_subject_template',
    73                                     '$prefix Batch modify: $tickets_descr',
     73                                    '${prefix} Batch modify: ${tickets_descr}',
    7474        """Like `ticket_subject_template` but for batch modifications.
    7575        (''since 1.0'')""")
    7676

comment:32 by Ryan J Ollos, 8 years ago

comment:31 change committed in r15714 and r15715. Edited TracNotification@95.

in reply to:  16 comment:33 by anonymous, 8 years ago

Replying to Christian Boos:

Replying to Jun Omae:

I received notification mail with Re: $prefix #$ticket.id: $summary in the subject. The subject template is not expanded.

Should be fixed? Yes.

I got this after upgrading to Trac 1.3. How was it fixed?

comment:34 by anonymous, 8 years ago

Ah, I see now, ticket_subject_template must be changed to ${prefix} #${ticket.id}: ${summary}.

comment:35 by anonymous, 8 years ago

Could this be done in an db44.py? 

from trac.upgrades import backup_config_file


def do_upgrade(env, version, cursor):
    """Change [notification] ticket_subject_template and
    [notification] batch_subject_template from Genshi to Jinja2.
    """

    config = env.config

    def upgrade_template(name):
        old = config.get('notification', name)
        if old:
            new = re.sub(r'\$([\w.]+)', r'${\1}', old)
            config.set(section, name, new)

    upgrade_template('ticket_subject_template')
    upgrade_template('batch_subject_template')

    backup_config_file(env, '.db44.bak')
    config.save()

comment:36 by Ryan J Ollos, 8 years ago

Yes, thank you. I've been planning to add an upgrade step.

comment:37 by Ryan J Ollos, 8 years ago

Two minor changes: r15867 and r15868.

comment:38 by Ryan J Ollos, 8 years ago

Somewhere we discussed logging warnings just once per template. Proposed changes in [ef131ef64/rjollos.git].

in reply to:  35 comment:39 by Ryan J Ollos, 8 years ago

Replying to anonymous:

Could this be done in an db44.py?

Proposed change in [7d0435f52/rjollos.git].

comment:40 by Ryan J Ollos, 8 years ago

comment:38 changes committed to trunk in r15940.

in reply to:  40 comment:41 by Ryan J Ollos, 7 years ago

Replying to Ryan J Ollos:

comment:38 changes committed to trunk in r15940.

We still get duplicate log messages that are fairly close in time, but the pid is different for each so I guess it must have something to do with the multiprocess Apache server. 

[pid 5973 139801362245376] 2017-06-18 18:08:28,206 Trac[chrome] WARNING: Rendering Genshi template 'verify_captcha.html' (convert to Jinja2 before Trac 1.5.1)
[pid 6064 139801362245376] 2017-06-18 18:20:06,806 Trac[chrome] WARNING: Rendering Genshi template 'verify_captcha.html' (convert to Jinja2 before Trac 1.5.1)
[pid 6926 139801362245376] 2017-06-18 18:39:05,709 Trac[chrome] WARNING: Rendering Genshi template 'verify_captcha.html' (convert to Jinja2 before Trac 1.5.1)
[pid 7032 139801362245376] 2017-06-18 18:44:40,308 Trac[chrome] WARNING: Rendering Genshi template 'verify_captcha.html' (convert to Jinja2 before Trac 1.5.1)
[pid 7052 139801362245376] 2017-06-18 18:48:42,438 Trac[chrome] WARNING: Rendering Genshi template 'verify_captcha.html' (convert to Jinja2 before Trac 1.5.1)
[pid 7009 139801362245376] 2017-06-18 18:50:00,372 Trac[chrome] WARNING: Rendering Genshi template 'verify_captcha.html' (convert to Jinja2 before Trac 1.5.1)
[pid 8474 139801362245376] 2017-06-18 19:04:35,258 Trac[chrome] WARNING: Rendering Genshi template 'verify_captcha.html' (convert to Jinja2 before Trac 1.5.1)
[pid 9691 139801362245376] 2017-06-18 19:42:06,341 Trac[chrome] WARNING: Rendering Genshi template 'verify_captcha.html' (convert to Jinja2 before Trac 1.5.1)

comment:42 by Jun Omae, 7 years ago

In [15414], j_, jgettext and jngettext functions have been added to babel.js. However, those functions are not documented. I think those functions are not needed.

in reply to:  42 ; comment:43 by Ryan J Ollos, 7 years ago

Replying to Jun Omae:

In [15414], j_, jgettext and jngettext functions have been added to babel.js. However, those functions are not documented. I think those functions are not needed.

Looks unused to me and probably an artifact of the migration process. In absence of additional feedback I would say to just remove them before we release 1.4.

comment:44 by Ryan J Ollos, 7 years ago

I thought that I'd already added an upgrade step for batch_subject_template and ticket_subject_template, at least on a development branch, but I cannot find it.

Proposed changes in [3dbc63bb5/rjollos.git].

DONE revise 1.3/TracUpgrade after the changes are committed.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

in reply to:  44 comment:45 by anonymous, 7 years ago

I thought that I'd already added an upgrade step for batch_subject_template and ticket_subject_template, at least on a development branch, but I cannot find it.

comment:39?

comment:46 by Ryan J Ollos, 7 years ago

Yes, that's it. Thanks. Committed in [16297:16298].

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

in reply to:  43 ; comment:47 by Ryan J Ollos, 7 years ago

Replying to Ryan J Ollos:

Replying to Jun Omae:

In [15414], j_, jgettext and jngettext functions have been added to babel.js. However, those functions are not documented. I think those functions are not needed.

Looks unused to me and probably an artifact of the migration process. In absence of additional feedback I would say to just remove them before we release 1.4.

Removed in r16299.

in reply to:  47 comment:48 by Jun Omae, 7 years ago

Replying to Ryan J Ollos:

Replying to Ryan J Ollos:

Replying to Jun Omae:

In [15414], j_, jgettext and jngettext functions have been added to babel.js. However, those functions are not documented. I think those functions are not needed.

Looks unused to me and probably an artifact of the migration process. In absence of additional feedback I would say to just remove them before we release 1.4.

Removed in r16299.

Sorry. I've missed uses of j_ function in templates. I just replaced j_ with _ in [16302], and those keywords from extract_messages_js in setup.cfg in [16303].

Last edited 7 years ago by Jun Omae (previous) (diff)

comment:49 by Ryan J Ollos, 7 years ago

While modifying ​AnnouncerPlugin I noticed that PreferencesModule.process_request doesn't correctly handle the 3 value tuple returned by process_request for legacy Genshi templates (TracDev/PortingFromGenshiToJinja#IRequestHandler). Proposed fix:

  • trac/prefs/web_ui.py 

    diff --git a/trac/prefs/web_ui.py b/trac/prefs/web_ui.py
index c42166a2d..c5acbab9f 100644
    a b class PreferencesModule(Component):  
    109109                children.append((name, label, rendered))
    110110
    111111        resp = chosen_provider.render_preference_panel(req, panel_id)
    112         template, data = resp
     112        data = resp[1]
    113113
    114114        data.update(session_data)
    115115        data.update({

I will look at adding a test.

Last edited 7 years ago by Ryan J Ollos (previous) (diff)

comment:50 by Ryan J Ollos, 7 years ago

Committed in r16374. It didn't seem worth adding a test for since the compatibility code will be removed in 1.5.1.

comment:51 by Ryan J Ollos, 6 years ago

From r15413, are these imports needed?:

  • trac/util/presentation.py 

    diff --git a/trac/util/presentation.py b/trac/util/presentation.py
index 357d519ff..c042f34e1 100644
    a b from datetime import datetime  
    2121from math import ceil
    2222import re
    2323
    24 from jinja2 import (Markup, Undefined, contextfilter, evalcontextfilter,
    25                     escape as escape_quotes)
     24from jinja2 import Markup, Undefined, contextfilter, evalcontextfilter
    2625from jinja2.filters import make_attrgetter
    2726from jinja2.utils import soft_unicode
    28 from jinja2._compat import iteritems
    2927
    3028from trac.core import TracError
    3129from .datefmt import to_utimestamp

comment:52 by Jun Omae, 6 years ago

It seems to be no needs. If we want to export it as an utility method, it should be exported in trac.util.html or trac.util.text. Also, the escape_quotes is added in [f163ed6c6/cboos.git] but is no longer used in [fea04e89d/cboos.git]. 

$ git grep -w escape_quotes mirror/trunk
mirror/trunk:trac/util/html.py:from markupsafe import Markup, escape as escape_quotes
mirror/trunk:trac/util/html.py:    e = escape_quotes(str)
mirror/trunk:trac/util/presentation.py:                    escape as escape_quotes)
$ git log --all --oneline --graph --decorate -Sescape_quotes -- trac/util/presentation.py
...
* 17ac01b6f (#12639) presentation utils
| * fef6ae343 (#12639) presentation utils
|/
| * fe110250a (#12639) presentation utils
|/
| * aaa1b03be (#12639) presentation utils
|/
...
| * fea04e89d html: introduce `xml` builder
...
| * f163ed6c6 presentation: when importing `escape` from markupsafe or jinja2, use the `escape_quotes` alias for clarity.

comment:53 by Ryan J Ollos, 6 years ago

Thanks, comment:51 change committed in r16674.

comment:54 by Ryan J Ollos, 5 years ago

#10418 was resolved in this ticket.

comment:55 by Ryan J Ollos, 5 years ago

Follow-up to r15445: I'm thinking we should log at error level: branches/1.4-stable/trac/ticket/notification.py@17177:320#L311​.

in reply to:  55 comment:56 by Jun Omae, 5 years ago

Replying to Ryan J Ollos:

Follow-up to r15445: I'm thinking we should log at error level: branches/1.4-stable/trac/ticket/notification.py@17177:320#L311​.

EmailDistributor catches exceptions and logs as a warning while executing INotificationFormatter.format() at tags/trac-1.4/trac/notification/mail.py@:384-390#L377​. I consider it is not needed to catch exceptions in format() method of INotificationFormatter components.

Last edited 5 years ago by Jun Omae (previous) (diff)

comment:57 by Ryan J Ollos, 5 years ago

Good point. I set template = None to force an exception:

Before change: 

21:28:26 Trac[mail] DEBUG: EmailDistributor has found the following formats capable of handling 'email' of 'ticket': text/plain
21:28:26 Trac[notification] DEBUG: Failed to format body of notification mail:
Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/ticket/notification.py", line 318, in _format_body
    body = chrome.render_template_string(template, data, text=True)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/chrome.py", line 1675, in render_template_string
    string = template.render(data)
AttributeError: 'NoneType' object has no attribute 'render'
21:28:26 Trac[mail] DEBUG: EmailDistributor is sending event as 'text/plain' to: rjollos@...
21:28:26 Trac[mail] INFO: Sending notification through SMTP at smtp.sendgrid.net:587 to ['rjollos@...']

After change: 

21:29:52 Trac[mail] DEBUG: EmailDistributor has found the following formats capable of handling 'email' of 'ticket': text/plain
21:29:52 Trac[mail] WARNING: EmailDistributor caught exception while formatting ticket to text/plain for email: <class 'trac.ticket.notification.TicketFormatter'>
Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/notification/mail.py", line 487, in distribute
    outputs[fmt] = formatter.format(transport, fmt, event)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/ticket/notification.py", line 143, in format
    return self._format_plaintext(event)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/ticket/notification.py", line 248, in _format_plaintext
    return self._format_body(data, 'ticket_notify_email.txt')
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/ticket/notification.py", line 317, in _format_body
    body = chrome.render_template_string(template, data, text=True)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/chrome.py", line 1675, in render_template_string
    string = template.render(data)
AttributeError: 'NoneType' object has no attribute 'render'
21:29:52 Trac[mail] DEBUG: EmailDistributor is sending event as 'text/plain' to: rjollos@...
21:29:52 Trac[mail] WARNING: EmailDistributor cannot send event 'ticket' as 'text/plain': rjollos@...

  • trac/ticket/notification.py 

    diff --git a/trac/ticket/notification.py b/trac/ticket/notification.py
index 5c311bf8e..c916e74f6 100644
    a b class TicketFormatter(Component):  
    311311    def _format_body(self, data, template_name):
    312312        chrome = Chrome(self.env)
    313313        template = chrome.load_template(template_name, text=True)
    314         # don't translate the e-mail stream
    315         with translation_deactivated():
    316             try:
    317                 body = chrome.render_template_string(template, data, text=True)
    318                 return body.encode('utf-8')
    319             except Exception as e:
    320                 self.log.debug("Failed to format body of notification mail: %s",
    321                                exception_to_unicode(e, traceback=True))
     314        template = None
     315        with translation_deactivated():  # don't translate the e-mail stream
     316            body = chrome.render_template_string(template, data, text=True)
     317            return body.encode('utf-8')
    322318
    323319    def _format_subj(self, event):
    324320        is_newticket = event.category == 'created'

comment:58 by Ryan J Ollos, 5 years ago

comment:57 change committed in r17179, merged in r17180.

comment:59 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

comment:60 by Ryan J Ollos, 5 years ago

API Changes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Christian Boos to the specified user.

Add Comment
E-mail address and name can be saved in the Preferences .
Attachments ↑ Description ↑
 
Note: See TracTickets for help on using tickets.