139309 - Include real name (not just email address) in bugmail comment and diff headers

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement, P4)

Description

[John Levon]

A minor tic, but bugmail only contains email addresses,
whereas bugzilla only contains the user name as visible
(with email as the link target). More often than not I
find myself getting lost trying to associate a bugmail
with the comment on the actual HTML bug report.

Expected behaviour: add something in bugmail to indicate
both email addres + username

(Couldn't find a dupe ...)

Comment 1

[Matthew Tuck [:CodeMachine]]

The reporter is referring to real names as "usernames" here.

We need comment numbers here too.  Hopefully this will be easy once email
templatisation is done.

Comment 2

[Joel Peshkin]

These unloved bugs have been sitting untouched since June 2002 or longer.  If
nobody does anything else to them, they certainly won't make 2.18
Retargetting to 2.20.  If you really plan to push them right now, you might pull
them back in.

Comment 3

[Karsten Düsterloh]

Attachment: add changer's realname to bugmail

Perl isn't 'my' language and I don't have a Bugzilla installation to test this against. But given the other code in User.pm and BugMail.pm, I feel this could do: change the bugmail line 
   <joe_user@somewhere> changed:
into 
   Joe "L" User <joe_user@somewhere> changed:

Comment 4

[Karsten Düsterloh]

Attachment: additionally add realname to comment header

Better version which changes
  joe_user@somewhere changed:
into
  Joe User <joe_user@somewhere> changed:
and 
  ------- Comment #3 from joe_user@somewhere  2005-12-27 12:47 PST
into
  ------- Comment #3 from Joe User <joe_user@somewhere>  2005-12-27 12:47 PST

Comment 5

[timeless]

Comment on attachment 206949 [details] [diff] [review]
additionally add realname to comment header

looks fine to me, but i'm not going to be able to testit. so i'm bouncing the request :).

Comment 6

[Frédéric Buclin]

Comment on attachment 206949 [details] [diff] [review]
additionally add realname to comment header

>-            $diffheader = "\n$who" . Param('emailsuffix') . " changed:\n\n";
>+            $diffheader = "\n$whoname <$who>" . Param('emailsuffix') . " changed:\n\n";

First of all, Param('emailsuffix') only makes sense when added together with the login name, not the real name (Frederic [e-mail] would be rather ridiculous). Moreover, the real name is not unique and is not mandatory and so you could get: " changed:" with no name at all.

I'm fine if you want to include the real name in emails, but the login name should remain.

Comment 7

[Karsten Düsterloh]

Attachment: v3: fixed name display

Bah, bug 180296 et al. stroke again, I didn't get any notice. :-/

Anyway, I think this patch finally does what I promised in comment #4...

Comment 8

[Frédéric Buclin]

Comment on attachment 216155 [details] [diff] [review]
v3: fixed name display

>-            $result .= "\n\n------- Comment #$count from $who" .
>-                       Param('emailsuffix'). "  " . format_time($when) .
>+            $result .= "\n\n------- Comment #$count from $whoname <$who" .
>+                       Param('emailsuffix'). ">  " . format_time($when) .
>                        " -------\n";

We would get exactly the same result using $user->identity, but it probably doesn't make sense to build a user object to use this method only. So r=LpSolit

Nit: the number of hyphens should be reduced to 3 only, else this line will really be too long. This can be fixed on checkin, if desired.

Comment 9

[Dave Miller [:justdave]]

some things to think about...

a) this is going to break the bots that report attachments being added on IRC channels.

b) some people have empty realnames, and this patch doesn't account for that.

Don't we have user.identity which already combines all of this?  Could use that and that would cover for b).

As for a), when do we get XML bugmail? :)

Comment 10

[Dave Miller [:justdave]]

a) is kinda of a wishlist or "would be nice if" thing, but b) needs to be addressed I think (or someone tell me why it doesn't).

Comment 11

[Karsten Düsterloh]

> a) is kinda of a wishlist or "would be nice if" thing,

I wouldn't know what to do about it anyway - I actually didn't intend to tinker with the mozbot code I know even less than Bugzilla's...

> but b) needs to be addressed I think (or someone tell me why it doesn't).

My main problem is still that I can't test my Bugzilla patches, so I'm like soldering in a dark room, blindfolded. I could try to "build a user object" and use $user->identity, but actually I don't see the point: if the real name is empty, the output looks like before, just with <> around the still existing email address, which is not an uncommon notation or some such...

Comment 12

[Frédéric Buclin]

(In reply to comment #10)
> b) needs to be
> addressed I think (or someone tell me why it doesn't).
> 

$user->identity returns:

        $self->{identity} = 
          $self->{name} ? "$self->{name} <$self->{login}>" : $self->{login};

In both cases, I see no problem with empty names. b) is not an issue.

Comment 13

[Karsten Düsterloh]

Attachment: updated to trunk, v3b

Updated to trunk, carrying over r+.
And even could test now that it works as intended (see <http://landfill.bugzilla.org/Mnyro139309/>).

Comment 14

[Dave Miller [:justdave]]

ok, looks good I guess.  It does indeed break the bots though.

Adding an attachment to http://landfill.bugzilla.org/Mnyro139309/show_bug.cgi?id=2222 produces bugmail with a comment header looking like this:

------- Comment #3 from Dave Miller <[e-mail]>  2006-05-21 19:26 PST -------

And feeding said email to ssdbot produces a comment on IRC that looks like this:

22:29:45 <@ssdbot> Dave added attachment 483 [details] [diff] [review] to bug 2222.

compared to the current comments that look like this:

15:16:05 <@ssdbot> [e-mail] added attachment 222795 [details] [diff] [review] to bug 329377.

Guess the bot authors will have to deal with it.  Of course, we have an X-Bugzilla-Who field that the bots can extract the "who did it" stuff from now.  I guess the backward-compatible way is to look for that header, and use it if present, and if not present, fall back on the current regexp out of the body.

Comment 15

[Frédéric Buclin]

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.73; previous revision: 1.72
done