137261 - X-Bugzilla-Who: an email header specifying the bugzilla account that made these changes

Status: VERIFIED FIXED

(Bugzilla :: Email Notifications, enhancement, P3)

Description

[Daniel Brooks [:db48x]]

I'd like this header so that I can mark bugmail read if I was the one who made
the change, since I probably still remeber what it was that I did.

Comment 1

[J. Paul Reed [:preed]]

bug 126151 has another way of doing this, and is thus related.

If I were cooler, I'd make that bug depend on this one... or something like that.

Comment 2

[m_mozilla]

i'd like it as a convenient way to killfile some folks who spam bugs (voting for
bug)

Comment 3

[John Morrison]

I was just chatting with Myk about such a feature (I called it X-Bugzilla-Actor)
and he thought it would be a Good Thing, and please file a bug. But, I did 
a quick query and found this bug.

I would find this immensely useful, for the "what did I say" and the "/ignore"
reasons noted above, and also to let me "give priority" to people that I'm 
actively working with. (And I think the X-... header is a better fix than 
changing the 'From', since changing that header will break many existing client
filter which key of the bugzilla-daemon@.. 

Comment 4

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 5

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 6

[Olav Vitters]

*** Bug 285368 has been marked as a duplicate of this bug. ***

Comment 7

[Olav Vitters]

Bug 285368 had a patch to implement this (but it needed work).

Comment 8

[Albert Ting]

Attachment: v1 patch

Seems like all one needs are some changer (and changername) substitution variables.  You can then modify your newchangedmail parameter to add in the X-Bugzilla-Who attribute (and/or change the From line)

Comment 9

[Frédéric Buclin]

> Seems like all one needs are some changer (and changername) substitution
> variables.

What happens if some emails have not been sent for a while and are sent concatenated? The last changer is attributed to X-Bugzilla-Who. Is that the desired effect?

Comment 10

[Albert Ting]

(In reply to comment #9)
> > Seems like all one needs are some changer (and changername) substitution
> > variables.
> 
> What happens if some emails have not been sent for a while and are sent
> concatenated? The last changer is attributed to X-Bugzilla-Who. Is that the
> desired effect?

It's may not be ideal, but I don't think I would want multiple X-Bugzilla-Who values.

Comment 11

[Frédéric Buclin]

(In reply to comment #10)
> It's may not be ideal, but I don't think I would want multiple X-Bugzilla-Who
> values.

I agree. :)

Comment 12

[Frédéric Buclin]

Comment on attachment 203471 [details] [diff] [review]
v1 patch

>@@ -621,4 +621,6 @@
>     $substs{"reasonsbody"} = $reasonsbody;
>     $substs{"space"} = " ";
>+    $substs{"changer"} = $values{'changer'};
>+    $substs{"changername"} = Bugzilla::User->new(&::DBNameToIdAndCheck($values{'changer'}))->name();
>     if ($isnew) {
>         $substs{'threadingmarker'} = "Message-ID: <bug-$id-" . 

Not sure we want 'changername'. First, the real name is not unique, and second it requires to build a user object. You should only keep 'changer'.

Another reason to deny review is that X-Bugzilla-Who should be added by default into the template, IMO. It's easier for an admin to remove it than to add it. And I'm pretty sure many people would be happy to be able to filter incoming email based on this header.

Comment 13

[Albert Ting]

(In reply to comment #12)
> Not sure we want 'changername'. First, the real name is not unique, and second
> it requires to build a user object. You should only keep 'changer'.

What's the harm of having both?  The Who field could then have 

  %changername% <%changer%>

Besides, we are already using the real name in our bugzilla comment headers, etc.  I don't understand why we have to do things differently here.

> Another reason to deny review is that X-Bugzilla-Who should be added by default
> into the template, IMO. It's easier for an admin to remove it than to add it.
> And I'm pretty sure many people would be happy to be able to filter incoming
> email based on this header.

That sounds reasonable

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
> What's the harm of having both?  The Who field could then have 
> 
>   %changername% <%changer%>
> 
> Besides, we are already using the real name in our bugzilla comment headers,
> etc.  I don't understand why we have to do things differently here.

Because you cannot add any filter based on the real name as you can freely change it. OK, you can also change your email address, but this is much less likely; and people often change their real name to indicate when they will be on vacation, for instance: Foo Bar (away from ... to...).

And depending on your email client, it's probably easier to write:
X-Bugzilla-Who is [e-mail]
than
X-Bugzilla-Who end with <[e-mail]>

Comment 15

[Albert Ting]

> Because you cannot add any filter based on the real name as you can freely
> change it. 

I can understand the arguments raised.  However, I feel this is too focused on the specific use of this param.  That is, I see no reason why we shouldn't include it, even if it is not useful for X-Who.  

In my situation, I'm using changername in the body text to highlight the poster real name.  My coworkers find that useful, rather than seeing just an email address (which coincidentally changed significantly at my company).

Comment 16

[Frédéric Buclin]

(In reply to comment #15)
> In my situation, I'm using changername in the body text to highlight the poster
> real name.  My coworkers find that useful, rather than seeing just an email
> address (which coincidentally changed significantly at my company).

OK, so what you say is that 'changername' is not used in the X-Bugzilla-Who header, which is the topic of this bug. You seem to agree that 'changername' should not go into this header, right? In this case 'changername' should not be part of this bug, IMO.

Comment 17

[Albert Ting]

Attachment: v2

Discussed this further offline with Frederic.

Frederic writes:
> Here my deal: we keep both variables, but only put changer in the header.
> 
> And note that in order to spare one SQL call, you should use 
> Bugzilla::User->new_from_login() to directly get a user object using its 
> login name.

Updated patch included

Comment 18

[Frédéric Buclin]

Comment on attachment 211340 [details] [diff] [review]
v2

r=LpSolit

Comment 19

[Frédéric Buclin]

Albert. By looking at the patch more closely, I realized that BugMail::sendMail() is called once per addressee, and so 'changername' is "recalculated" by far too many times, which could be a severe performance issue. You should move Bugzilla::User->new_from_login($values{'changer'})->name earlier in the code, together with $values{'changer'} = $forced->{'changer'}. This way you only have to write $substs{"changername"} = $values{"changername"} in sendMail().

Please fix that.

Comment 20

[Albert Ting]

Attachment: v3

Updated patch, per comment #9

Comment 21

[Frédéric Buclin]

Comment on attachment 211406 [details] [diff] [review]
v3

>--- Bugzilla/BugMail.pm~	2005-03-08 10:27:27.000000000 -0800
>+++ Bugzilla/BugMail.pm	2005-03-08 16:29:52.000000000 -0800

>+    $values{'changer'} = $forced->{'changer'};
>+    $values{'changername'} = Bugzilla::User->new_from_login($values{'changer'})->name;

Nit: my $changer = $forced->{'changer'}; has been defined a few lines above these two and is used only once. We could use it here to make the code a little bit cleaner:

    $values{'changer'} = $changer;
    $values{'changername'} = Bugzilla::User->new_from_login($changer)->name;

Either attach a new patch with this nit fixed and carry forward my r+, or I will fix it on checkin. But I would prefer an updated patch. ;)

r=LpSolit

Comment 22

[Albert Ting]

Attachment: v4

Updated patch.

Comment 23

[Vlad Dascalu]

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.63; previous revision: 1.62
done
Checking in Bugzilla/Config/MTA.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/MTA.pm,v  <--  MTA.pm
new revision: 1.4; previous revision: 1.3
done

Comment 24

[Frédéric Buclin]

*** Bug 126151 has been marked as a duplicate of this bug. ***

Comment 25

[Max Kanat-Alexander]

Added to the release notes as part of bug 349423.

Comment 26

[Ben Bucksch (:BenB)]

I would like - no, I *need to* - to have "Mike Shaver" in my From column in Thunderbird, to efficiently process my bugspam.
That's bug 126151, but there's resistance.

For a poor man's fix, I could maybe configure my postfix to Change the From header to the X-Bugzilla-Who header content. But the lack of the realname makes this mostly useless. I don't want to see saw email addresses, but real names. So, the lack of changername in X-Bugzilla-Who makes it impossible to use this bug as workaround for 126151, as was proposed in that bug.

Please add changername. re comment 14: Filters can run on substrings. *All* my filters for persons run on substrings.