442205 - show attachment size in bugmail

Status: NEW

(Bugzilla :: Email Notifications, enhancement, P2)

Description

[Karsten Düsterloh]

When attachments are added on a bug, you will get bugmail about who added which new attachment like this:

--- Comment #1 from Serge Gautherie <[e-mail]>  2008-06-26 22:19:35 PDT ---
Created an attachment (id=327068)
 --> (https://bugzilla.mozilla.org/attachment.cgi?id=327068)
(Av1-SM) <msgHdrViewSMIMEOverlay.js>

It'd be extremely helpful if the *size* of that new attachment would be shown also.

(For example, review requests for very small patches could be priorized without browsing Bugzilla.)

Comment 1

[Nitish Bezzala]

Attachment: Showing the size of the attachment in the email

Don't know how to format to show KB instead of bytes etc.

Comment 2

[A. Shimono [:himorin]]

Might be better to define new (template) parameter and use FILTER unitconvert in .tmpl file, instead.
# And it might be nice to localizers :D

Comment 3

[Frédéric Buclin]

Comment on attachment 355014 [details] [diff] [review]
Showing the size of the attachment in the email

>-                  $attachment->description . "\n";
>+                  $attachment->description . " (" . $attachment->datasize . " bytes)\n";
>+

I don't want more hardcoded strings in .cgi scripts, for the reason himorin mentioned. Also, I don't want this information to be added into comments; the attachment tables already has all the required information. The right place to fix is Bugzilla::BugMail::sendMail(), see http://mxr.mozilla.org/mozilla/source/webtools/bugzilla/Bugzilla/BugMail.pm#590. Also, in addition to the size of the attachment, I also want to see the MIME type of the attachment to be displayed.

Comment 4

[Nitish Bezzala]

Attachment: Adds Size and content-type to attachment email

Will have to wait for 466968 for templatisation of diffs, to enable localisation, and to use unitconvert.

Comment 5

[Frédéric Buclin]

Comment on attachment 355099 [details] [diff] [review]
Adds Size and content-type to attachment email

>+	# If an attachment was created, add its size and mime-type.
>+	my ($attachid) = ($newcomments =~ m/Created an attachment \(id=(\d+)\)/ );

No, fix the existing regexp to get the attachment ID and put your code in it. This way, we have a single "if this is a new attachment" block.


>+	if ( $attachid ) {
>+		my $attachment = new Bugzilla::Attachment($attachid);

Note that the attachment may not exist, or may not be accessible to the user getting the email. You are introducing security leak here. You have to do these checks before displaying the size and MIME type, and do not display them if the attachment is not accessible to the user.



>+	   	$difftext .= "Content-type: " . $attachment->contenttype;

I think MIME type is clearer than Content-type. Also, use get_text() to get the translated strings.

Comment 6

[Frédéric Buclin]

Oh, and we should make the regexp a bit more clever. The "Created an new..." string should be at the very beginning of the bug comment, else someone could build a comment of the form "Created attachment 2 [details] [diff] [review], created attachment 4 [details] [diff] [review], ..." to collect all the data and confuse Bugzilla.

Comment 7

[Nitish Bezzala]

Attachment: used the same if, checked permissions, not changed the regexp

If I change the regexp, it won't be able to handle multiple attachments, like it says in the comments. But I don't know how and where to test multiple attachments to comments. 
If we don't handle multiple attachments, the best thing to do is to remove the /g.
Otherwise, I can try to pass a list of attachments, rather than getting them from the comments.

Comment 8

[Frédéric Buclin]

Comment on attachment 356165 [details] [diff] [review]
used the same if, checked permissions, not changed the regexp

(In reply to comment #7)
> If I change the regexp, it won't be able to handle multiple attachments, like
> it says in the comments.

I don't think we care about multiple attachments, as you cannot create more than one attachment at once.

Also, your security check is incomplete (and partially wrong): you only check for the private bit, but if the attachment is in another bug you cannot see, and the attachment is not private, you will still get the data. Also, we don't care if the user cannot edit the attachment. If he can view it, he should be allowed to get the data, so validate_can_edit() is not the right method to call. And to avoid tons of calls to the DB, which happens with your patch as you recreate the attachment object for each user getting bugmail (+ all the security checks), some check should be done earlier in the code, once for all users.

I put random ideas here, but BugMail is a critical part in the bug edition. We cannot afford to slow it for such a minor feature which would be triggered everytime someone attach a new file. Maybe a good fix is not trivial; I don't know yet.

Comment 10

[(not currently active) Ted Mielczarek]

From my dupe:
"Something like the summary line from "diffstat" would be great:
 2 files changed, 92 insertions(+), 2 deletions(-)

but any similar output would be really helpful. That way I'd be able to tell if a patch is trivial and review it right away."

for patch attachments, getting diffstat-like output would be ideal.

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

:dkl, is this bug still an issue in the current code base?  could we also get a mentor assigned for this bug?

Comment 12

[David Lawrence [:dkl]]

(In reply to Joel Maher (:jmaher) from comment #11)
> :dkl, is this bug still an issue in the current code base?  could we also
> get a mentor assigned for this bug?

I still think this would be nice to have. Emails basically include the same comment text that is stored in the DB table so we would need to include the attachment size in the comment text when the attachment is actually added, not at the time email is generated.

Adding myself as mentor if someone wants to take this on.

dkl

Comment 13

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #12)
> Emails basically include the same
> comment text that is stored in the DB table so we would need to include the
> attachment size in the comment text when the attachment is actually added,
> not at the time email is generated.

I don't think this is needed. If you follow this path, the same approach would be used to implement bug 182535 or bug 907291.

Comment 14

[Mukhtar Ali]

Can u assign this to me and let me know any info that will be helpful.

Comment 15

[Andre Klapper]

Any info that will be helpful is in this very bug report.

Comment 16

[Mukhtar Ali]

used the same if, checked permissions, not changed the regexp. Everything should work fine.

Comment 17

[Frédéric Buclin]

Please do not abuse the needinfo flag.

Comment 18

[Levi]

use File::stat
$attachmentsize = stat($attachment) -> size;

Comment 19

[Firefox Bug Husbandry Bot]

Removing good-first-bug keyword because team does not have bandwidth to mentor at the moment.

Comment 20

[Christa]

I can work on this