Modify

Opened 12 years ago

Closed 12 years ago

#165 closed enhancement (fixed)

This message has 0 attachment(s)

Reported by: dereks@… Owned by: bas
Priority: minor Milestone: Release 1.1.0
Component: email2trac Version: trunk
Keywords: Cc: hju@…

Description

Every ticket I create via email, using email2trac, gets a comment that says:

Comments: (by derek) This message has 0 attachment(s)

If there are zero attachments, I'd prefer it if there was no comment at all. I don't need to document that something doesn't exist.

Also, this "feature" breaks the AnnouncementPlugin? (which allows Trac to send pretty HTML-formatted notifications). If I enable that plugin, I get two HTML-formatted notifications: one for the new ticket (similar to Trac's built-in notifications), but then a second, redundant one to tell me there is a new comment (which just says: "This message has 0 attachment(s)" ).

Thanks for working on email2trac, so far it's looking pretty awesome...

Attachments (1)

no_zero_attachment_comment.patch (446 bytes) - added by dereks@… 12 years ago.
Patch to fix this bug

Download all attachments as: .zip

Change History (12)

Changed 12 years ago by dereks@…

Patch to fix this bug

comment:1 Changed 12 years ago by dereks@…

So I took a quick look and saw how easy it would be to fix this... patch is attached.

It's a trivial 3-line change, so here it is for quick review:

--- email2trac-1.0	2009-12-09 20:36:15.000000000 -0800
+++ email2trac	2009-12-09 20:40:11.000000000 -0800
@@ -1451,9 +1451,10 @@
 			fd.close()
 			os.unlink(path)
 
-		# Return how many attachments
+		# Return how many attachments, if more than zero
 		#
-		status = 'This message has %d attachment(s)\n%s' %(count, status)
+		status = None
+		if count > 0: status = 'This message has %d attachment(s)\n%s' %(count, status)
 		return status

I've tested this here. It Works For Me(tm).

By the way, if you google for "This message has 0 attachment(s)" you can find a bunch of Trac sites that have email2trac installed. :)

comment:2 Changed 12 years ago by bas

  • Milestone set to Release 1.1.0
  • Status changed from new to assigned
  • Type changed from defect to enhancement

Thanks i will fix it when i am back from holidays

comment:7 follow-up: Changed 12 years ago by dereks@…

I just found a minor bug with my patch.

The last line in the diff should read:

{{{
if count > 0: status = 'This message has %d attachment(s)\n' %(count,
)
}}}

The second %s and the string placement "status" has been removed.
This
bug causes the word "None" to appear in the comment if there are
actually attachments.

If you need me to make another patch let me know... I figure you'll
just
edit this by hand.


On 2009-12-10 11:56, email2trac wrote:

comment:8 Changed 12 years ago by dereks@…

I've been using email2trac with this patch and I think that the comment is unnecessary, even if there are more than zero attachments.

Trac already shows the attachments. Having the comment there is just noise, in my opinion.

So I propose that these three lines be replace with just:

return None

comment:9 Changed 12 years ago by bas

  • Version changed from 1.0.0 to trunk

I go for the last option. I will skip this comment and it will not be displayed anymore in the changelog of an ticket

comment:6 Changed 12 years ago by bas

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [292]) Do not show the message, 'This message has X attachment(s)' in the changelog of a ticket, closes #165

comment:7 follow-up: Changed 12 years ago by hju@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

with this change you have totally removed all information about the attachments from the ticket. OK, this is an option...

But there are also some usefull informations I would miss (i.e. if there ia a single attachment which exceeds the max_size limit and is not added) which are calculated and stored in status during the for part in message_parts: loop in attachments().

I'm more with dereks solution - with a little change (checking if status is set before)

	def attachments(self, message_parts, update=False):
...

		# Return how many attachments if more than zero
		#
		if status or count > 0:
			status = 'This message has %d attachment(s)\n%s' %(count, status)

		return status

perhaps it is possible you consider this?

comment:8 in reply to: ↑ 7 Changed 12 years ago by dereks@…

  • Priority changed from major to minor

Replying to hju@…:

But there are also some usefull informations I would miss (i.e. if there ia a single attachment which exceeds the max_size limit and is not added)

I agree with hju that email attachments should not be silently discarded, leaving the user wondering what happened to it. If there was an email attachment, but it could not be added to the ticket, then it would be useful to know why.

I'm more with dereks solution - with a little change (checking if status is set before)

if status or count > 0:

status = 'This message has %d attachment(s)\n%s' %(count, status)

I disagree with this particular code patch. I had it like this for a while and I discovered that it's very annoying to be looking at a ticket, seeing two attachments, and then having a comment right there that says "This message has 2 attachment(s)". It's just noise on the screen.

I would suggest that any successful attachments simply be added to the ticket (with no extraneous comment). If there is an error processing an attachment, then the user should be told why. But that could be as a ticket comment, or a simple email reply that tells the user the attachments could not be processed, or just a log entry (for the administrator only).

comment:9 Changed 12 years ago by bas

  • Cc hju@… added

I will go for the option if there is an error with an attachment i will log it in the ticket

comment:10 Changed 12 years ago by hju@…

I had it like this for a while and I discovered that it's very annoying to be looking at a ticket, seeing two attachments, and then having a comment right there that says "This message has 2 attachment(s)". It's just noise on the screen.

I would suggest that any successful attachments simply be added to the ticket (with no extraneous comment). If there is an error processing an attachment, then the user should be told why.

Your're right. This will be better!
Never thought about it but it is really redundant (and useless) info.

comment:11 Changed 12 years ago by bas

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [319]) When there a problems with saving attachments. Show it as comment in the ticket, closes #165

Add Comment

Modify Ticket

Change Properties
Action
as closed The owner will remain bas.
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.