Opened 4 years ago

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

Reported by: Owned by: rjollos rjollos normal AnnouncerPlugin normal HTML formatting hasienda

### Description

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

### comment:1 Changed 4 years ago by rjollos

• Priority changed from normal to high
• Status changed from new to assigned

### comment:2 follow-up: ↓ 3 Changed 4 years ago by 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. 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.

### Changed 4 years ago by rjollos

Patch against r12265 of the AnnouncerPlugin trunk.

### comment:3 in reply to: ↑ 2 ; follow-up: ↓ 5 Changed 4 years ago by hasienda

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.

### comment:4 Changed 4 years ago by rjollos

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

### comment:5 in reply to: ↑ 3 ; follow-up: ↓ 6 Changed 4 years ago by rjollos

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.

### comment:6 in reply to: ↑ 5 Changed 4 years ago by 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?
======================================================================
----------------------------------------------------------------------
Traceback (most recent call last):
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)
raise TemplateNotFound(filename, search_path)


### comment:7 Changed 4 years ago by 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.

### comment:8 Changed 4 years ago by rjollos

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

### comment:9 follow-up: ↓ 10 Changed 4 years ago by hasienda

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.

### comment:10 in reply to: ↑ 9 Changed 4 years ago by rjollos

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())


### comment:11 Changed 4 years ago by hasienda

(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.

### comment:12 Changed 4 years ago by rjollos

(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.

### comment:13 Changed 4 years ago by rjollos

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

### comment:14 Changed 4 years ago by rjollos

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

### comment:15 Changed 4 years ago by rjollos

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

### comment:16 Changed 4 years ago by rjollos

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.

### comment:17 Changed 4 years ago by rjollos

(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.

### Changed 4 years ago by rjollos

Patch against r12354 of the trunk.

### comment:18 follow-up: ↓ 26 Changed 4 years ago by rjollos

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.

### Changed 4 years ago by hasienda

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

### comment:19 follow-up: ↓ 20 Changed 4 years ago by 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.

### comment:20 in reply to: ↑ 19 Changed 4 years ago by rjollos

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?

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.

### Changed 4 years ago by rjollos

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

### comment:21 follow-up: ↓ 24 Changed 4 years ago by hasienda

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.

### comment:22 Changed 4 years ago by hasienda

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.

### comment:23 follow-up: ↓ 27 Changed 4 years ago by 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?

### comment:24 in reply to: ↑ 21 Changed 4 years ago by rjollos

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.

### comment:25 Changed 4 years ago by rjollos

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

(In [12359]) Refs #10584:

• 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.

### comment:26 in reply to: ↑ 18 ; follow-up: ↓ 28 Changed 4 years ago by rjollos

• Priority changed from high to normal

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

... 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.

### comment:27 in reply to: ↑ 23 Changed 4 years ago by hasienda

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.

### comment:28 in reply to: ↑ 26 ; follow-up: ↓ 29 Changed 4 years ago by hasienda

... 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.

### comment:29 in reply to: ↑ 28 Changed 4 years ago by rjollos

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.

### comment:30 Changed 4 years ago by rjollos

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

### comment:31 Changed 3 years ago by rjollos

• Status changed from assigned to new