Attachment description should be formatted from wiki to html when html email is used

[Ryan J Ollos]

The attachment description should be formatted from wiki markup to HTML when HTML email is used.

[Ryan J Ollos]

Attached is a patch which I'm requesting feedback on. Some notes on the changes:

• Renamed method-scope function render_wiki_to_html_without_req to render_wiki_to_html. The long name seems unnecessary and makes it difficult to limit the width of expressions to 80 chars.
• The comment variable is used for both normal comments and formatted attachment descriptions, since an attachment description is effectively a comment on that attachment. Since you can't have a normal comment and attachment within the same change there is no conflict. I felt like this was better than adding another entry to the data dictionary.

I'll rework the patch based on feedback if any of these changes seem undesirable.

[Ryan J Ollos]

Patch against r12265 of the AnnouncerPlugin trunk.

[Steffen Hoffmann]

Replying to rjollos:

Attached is a patch which I'm requesting feedback on. Some notes on the changes:

• Renamed method-scope function render_wiki_to_html_without_req to render_wiki_to_html.

Sure, but how about cutting it down more, think wiki_to_html is the essence, and there is not much else you can do than do then to render it anyway?

• The comment variable ... better than adding another entry to the data dictionary.

Good find. Fine, and looks reasonable to me. Please commit.

[Ryan J Ollos]

(In [12294]) Refs #10584: Format attachment description to HTML when HTML templates are being rendered.

[Ryan J Ollos]

Replying to hasienda:

Sure, but how about cutting it down more, think wiki_to_html is the essence, and there is not much else you can do than do then to render it anyway?

Good idea. I included that change in [12294].

I'm working on adding a unit test, but ran into a some issues. I'll follow-up on that tomorrow.

[Ryan J Ollos]

Replying to rjollos:

I'm working on adding a unit test, but ran into a some issues. I'll follow-up on that tomorrow.

I added a unit test, and in the process of getting the unit test working, discovered some other issues that the attached patch addresses:

• The unit test for TicketFormatter.format was failing with TemplateNotFound: Template "ticket_email_mimic.html" not found (full traceback below). TicketFormatter and WikiFormatter were not implementing ITemplateProvider, so they must have been replying on the AnnouncerPreferences and SubscriptionManagementPanel making the templates available, since those are the only two components in the plugin that implement ITemplateProvider. I suspect this would lead to an error in the case that the admin chose to not enable the preferences panel. With this patch, TicketFormatter and WikiFormatter implement ITemplateProvider and can make the necessary templates available without relying on other components being enabled.
• Renamed tests/formatter.py to tests/formatters.py since the test module name should match name of the module under test.
• Other minor changes to the test harness.
• The expected output for the attachment notification HTML is contained in a text file. Maybe there is a better way?

======================================================================
ERROR: test_add_attachment_html_notification (announcer.tests.formatters.TicketFormatterTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/Workspace/trac-hacks/announcerplugin/trunk/announcer/tests/formatters.py", line 58, in test_add_attachment_html_notification
    output = self.tf.format([], 'ticket', 'text/html', event)
  File "/home/user/Workspace/trac-hacks/announcerplugin/trunk/announcer/formatters.py", line 80, in format
    return self._format_html(event)
  File "/home/user/Workspace/trac-hacks/announcerplugin/trunk/announcer/formatters.py", line 245, in _format_html
    cls=MarkupTemplate)
  File "/home/user/Workspace/th10083/genshi-0.6.0/genshi/template/loader.py", line 246, in load
    raise TemplateNotFound(filename, search_path)
TemplateNotFound: Template "ticket_email_mimic.html" not found

[Ryan J Ollos]

My changes are getting sufficiently complex that I plan to move to developing from a BitBucket fork following completion of work on this patch.

[Ryan J Ollos]

I just pulled in the recent changes and found my patch will apply with minimal modifications (minor conflict in announcer/tests/__init__.py.

[Steffen Hoffmann]

Replying to rjollos:

My changes are getting sufficiently complex that I plan to move to developing from a BitBucket fork following completion of work on this patch.

Yes, why now? I'm working on a Mercurial repository clone of t-h.o SVN repo, what is similar to the standard (hg) setup with BitBucket.

Regarding your latest patch, I'd rather prefer to implement ITemplateProvider for only one Component per plugin, make it an abstract one and inherit classes from there as required. I'll add a suitable implementation in announcer.api tomorrow, so you can re-base your changes from there, please.

+1 for the test module name change.

What reason do you see for the changes to announcer/tests/__init__.py? I'm especially curious since I've see a pattern with extra import statements i.e. in TagsPlugin before as well, but couldn't figure out, why it would be preferable over the simpler code here.

Other (test) changes look good to me too, and I do especially appreciate your work on the test harness, because by mow both of us know the tremendous hidden value of it.

[Ryan J Ollos]

Replying to hasienda:

Yes, why now?

I'm still not comfortable enough with Hg to be working effectively, but I think it's time I bite the bullet and take some short term loses in favor of long term efficiency gains. I could just work locally with an Hg clone I suppose. Recently I completed a patch to the Trac core, working from a Git fork in GitHub, and it was very valuable to get some direct code review from Christian via comments on the changeset in GitHub. So I'd like to both move to DVCS, and have a place where you can directly comment on my proposed changes if you like (more on that when I reply to your email).

Regarding your latest patch, I'd rather prefer to implement ITemplateProvider for only one Component per plugin,

I agree, that seems like a better way to go.

I'll add a suitable implementation in announcer.api tomorrow, so you can re-base your changes from there, please.

I'll wait for your changes and rebase.

What reason do you see for the changes to announcer/tests/__init__.py? I'm especially curious since I've see a pattern with extra import statements i.e. in TagsPlugin before as well, but couldn't figure out, why it would be preferable over the simpler code here.

I'm not sure, I'm just sort of fumbling around here, but I was seeing intermittent failures prior to making that change. To debug, I studied osimons implementation for the TagsPlugin, made changes to setup.py and __init__.py that I saw there, and those changes seemed to fix the problem.

This part does look a little suspicious to me though:

suite.addTest(announcer.api.suite())

Shouldn't it be?:

suite.addTest(announcer.tests.api.suite())

[Steffen Hoffmann]

(In [12313]) AnnouncerPlugin: Use a single ITemplateProvider across all plugin classes, refs #10584.

Done a bit of PEP8 code clean-up as well as correcting test references as per Ryan's suggestion - all tests passing for Trac >= 0.12, counted up to a total of 19 tests now.

[Ryan J Ollos]

(In [12349]) Refs #10584: Renamed announcer/tests/formatter.py to announcer/tests/formatters.py, so that the test module name agrees with the name of the module under test.

[Ryan J Ollos]

[12349] was a mistake. I'll roll it out and recommit just the change that I had intended to commit.

[Ryan J Ollos]

(In [12350]) Refs #10584: Reverse merge incorrect commit in [12349].

[Ryan J Ollos]

(In [12352]) Refs #10584: Replaced tabs with spaces in ticket_email_mimic.html.

[Ryan J Ollos]

Sorry for the mess in [12349]/[12350]. It seems I was also careless with [12294], as it is not working the way I intended. I'll fix that now by direct commit since the changes are simple, and then attach a more complex patch for review, related to the unit tests.

[Ryan J Ollos]

(In [12354]) Refs #10584:

• Corrected an error in [12294]: attachment's description was being shown twice, next to the attachment and as a comment.
• Other minor changes to HTML template to align it with the plain text template.

[Ryan J Ollos]

Patch against r12354 of the trunk.

[Ryan J Ollos]

I've attached t10584-r12354-1.patch​ for review, which adds a unit test for the behavior added in this ticket. Assuming the changes I've made here are correct and necessary, then I'd also suggest moving AnnouncerTemplateProvider to announcer.api, or another "more central" location.

[Steffen Hoffmann]

review results: rather a rework than just some nit-picks

[Steffen Hoffmann]

Sorry for the major changes. I was actually about to commit this, when I realized, that I should have just reviewed. But I won't like to loose these changes.

Btw, did you forget about a file? I get No such file or directory: 'attachment_notification.html' for test_add_attachment_html_notification.

[Ryan J Ollos]

Replying to hasienda:

Sorry for the major changes. I was actually about to commit this, when I realized, that I should have just reviewed. But I won't like to loose these changes.

It looks like the major change to my original patch is the creation of the superclass FormatterTestCase. Looks good. Anything else I overlooked?

Btw, did you forget about a file? I get No such file or directory: 'attachment_notification.html' for test_add_attachment_html_notification.

Yes, I failed to capture the new file in my patch. I'll upload that now, so that you can push all the changes directly, if you like.

[Ryan J Ollos]

New file that should have been included with t10584-r12354-1.patch.

[Steffen Hoffmann]

No, that's your ticket, so cross-test my mods and commit it yourself, please.

I'm heavily over-time tonight and already have to postpone another review-request of yours. Furthermore it's mostly our work, so would be rather unfair IMHO, if I took it without reason.

[Steffen Hoffmann]

Just one more thought: Please don't forget about a changelog entry, because the patch seems rather exhaustive and shouldn't require more action here. Thanks for taking care.

[Ryan J Ollos]

I captured the change that is the main subject of this ticket in [12294]. Did you have in mind other entries for the changelog associated with this ticket, or just not notice that entry had already been made?

[Ryan J Ollos]

Replying to hasienda:

I'm heavily over-time tonight and already have to postpone another review-request of yours.

No worries on that ... I have plenty to keep me busy and in no rush to push every change. I still have many changesets to review myself, just to get fully up to speed on your development work from the past week or so.

[Ryan J Ollos]

Ticket post-commit hook didn't seem to work this time.

(In [12359]) Refs #10584:

• Added test case for HTML-formatted attachment notification.
• TicketFormatter and WikiFormatter now inherit from AnnouncementTemplateProvider, so that the templates are served when other template-serving components are not enabled.
• Wrapped strings in formatters.py for translation.

[Ryan J Ollos]

And with that change, barring any other findings, work on this ticket is complete ...

Replying to rjollos:

... then I'd also suggest moving AnnouncerTemplateProvider to announcer.api, or another "more central" location.

... thought I'd still keep in mind that we may want to make this change.

It would be nice if we had an implemented or in review state for tickets that are thought to be fully implemented, but will be left open until the next release. I hesitate to implement this site-wide however, since it might be confusing to other users and maintainers.

[Steffen Hoffmann]

Replying to rjollos:

I captured the change that is the main subject of this ticket in [12294]. Did you have in mind other entries for the changelog associated with this ticket, or just not notice that entry had already been made?

Right, I did not notice. Your changeset has it in, perfect. Nothing else.

[Steffen Hoffmann]

Replying to rjollos:

Replying to rjollos:

... then I'd also suggest moving AnnouncerTemplateProvider to announcer.api, or another "more central" location.

... thought I'd still keep in mind that we may want to make this change.

Lately I tend to put the template provider into that location, where primary/most extensive use happens, often web_ui.py. One import less, especially, if there's no use for it in api.py. OTOH, it's indisputable part of the plugin API, so anyone would start to look for it in api.py, that's the overall concept. Good thing, the class is abstract, so we may move it at any time, because enabling the component is not an issue (no change to trac.ini's [component] section, right?).

It would be nice if we had an implemented or in review state for tickets that are thought to be fully implemented, but will be left open until the next release. I hesitate to implement this site-wide however, since it might be confusing to other users and maintainers.

Yes, I was thinking this for all of AcctMgr, Tags and Announcer lately. If we made it the last action, why now? Theoretically we could introduce another optional field similar to what version (meaning "implemented in version"), i.e. "Plugin Version:" but this might be confusing too. Keyword review is the least intrusive I can think of, and will start for this plugin, if you agree.

For the other aforementioned plugins I've already prepared release notes, so I have no need for this before next stable release. I would rather adopt this convention afterwards there.

[Ryan J Ollos]

Replying to hasienda:

Good thing, the class is abstract, so we may move it at any time, because enabling the component is not an issue (no change to trac.ini's [component] section, right?).

Yeah, it looks that way to me. So should we make the change against this ticket, or would you like to make the change against another ticket? Should I take the initiative, or do you already have plans to do it yourself?

Yes, I was thinking this for all of AcctMgr, Tags and Announcer lately. If we made it the last action, why now? Theoretically we could introduce another optional field similar to what version (meaning "implemented in version"), i.e. "Plugin Version:" but this might be confusing too. Keyword review is the least intrusive I can think of, and will start for this plugin, if you agree.

Yeah, that seems like a good way to go, so that we can run quick queries and see which tickets are ready and which need work. To me, it is primarily an issue when there are so many tickets, such as announcer currently having 100+. Once we get out release 1.0 and kill half or more of the tickets, it will be less of an issue.

[Ryan J Ollos]

(In [12402]) Refs #10584: In [12359], the full path to attachment_notification.html was not being used.

[Ryan J Ollos]

Please upgrade to Trac 1.2, which has integrated the core of AnnouncerPlugin. Please raise the issue on the ​trac:MailingList if you encounter the issue with Trac 1.2.