1016273 - Bugzilla whine emails missing change to From header made by bug 1010751

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[Ed Morley [:emorley]]

I'm presuming this needs updating:
https://github.com/mozilla/webtools-bmo-bugzilla/blob/47bfbf7c402135e67ccc9832331b38a789d94731/whine.pl#L100

Unrelated, but I also spotted:
https://github.com/mozilla/webtools-bmo-bugzilla/blob/47bfbf7c402135e67ccc9832331b38a789d94731/extensions/ContributorEngagement/lib/Constants.pm#L19

Which appears to be used without prefixing:
https://github.com/mozilla/webtools-bmo-bugzilla/blob/47bfbf7c402135e67ccc9832331b38a789d94731/extensions/ContributorEngagement/Extension.pm#L112

Many thanks :-)

Comment 1

[David Lawrence [:dkl]]

Attachment: 1016273_1.patch

Comment 2

[:glob ✱]

Comment on attachment 8429418 [details] [diff] [review]
1016273_1.patch

Review of attachment 8429418 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob, with the following to be fixed on commit:

all of these are missing quotes around the BugzillaTitle term.
it needs to be:  "Bugzilla@Mozilla" <...>

Comment 3

[David Lawrence [:dkl]]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   24a16ae..775b7cc  master -> master

Comment 4

[David Lawrence [:dkl]]

Closing.

Comment 5

[:glob ✱]

this causes whine.pl to throw warnings:

Use of uninitialized value $lang in hash element at Bugzilla/Util.pm line 753.
Use of uninitialized value $lang in hash element at Bugzilla/Util.pm line 766.

Comment 6

[:glob ✱]

i was poking around the code...  i think i made a mistake with my original patch.

i think it could be better to revert this patch and bug 1010751, and instead change the mailfrom parameter's value to:  "Bugzilla@Mozilla" <[e-mail]>

as far i can tell mailfrom is always inserted unfiltered/unmodified into an email, but that probably needs more checking if we take that path.

dkl, are you able to double-check if that would work?

Comment 7

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #6) 
> i think it could be better to revert this patch and bug 1010751, and instead
> change the mailfrom parameter's value to:  "Bugzilla@Mozilla"
> <[e-mail]>

That is an alternative way to do it yes. I liked the idea of an admin not having
to worry about From: formatting and it would just put the two values together
automatically (email address and site title). But I suppose an admin may want to
have the site title possible be different than what is in the email From: header
so would want to designate the value manually.
 
> as far i can tell mailfrom is always inserted unfiltered/unmodified into an
> email, but that probably needs more checking if we take that path.

It is. As for filtering we could leave it unfiltered as it is an admin param and
we have always assumed that the admin would know what they are doing.

> dkl, are you able to double-check if that would work?

1. Updated the data/params to:
   'mailfrom' => '"Bugzilla@Mozilla" <[e-mail]>',

2. And performed the below git commands to roll back related commits:

$ git revert -n 1af952ceaa1c06a10e0eeb6f139e5a5652002e98
$ git revert -n 3f35dd081156d231710553099f01f40481aa187a
$ git revert -n 6c7add0ece060f9a945c7b18b4d03546a2220043
$ git status
# On branch master
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	modified:   email_in.pl
#	modified:   extensions/RequestNagger/template/en/default/email/request_nagging-requestee-header.txt.tmpl
#	modified:   extensions/RequestNagger/template/en/default/email/request_nagging-watching-header.txt.tmpl
#	modified:   extensions/SecureMail/template/en/default/account/email/securemail-test.txt.tmpl
#	modified:   extensions/Voting/template/en/default/voting/votes-removed.txt.tmpl
#	modified:   importxml.pl
#	modified:   template/en/default/account/cancel-token.txt.tmpl
#	modified:   template/en/default/account/email/change-new.txt.tmpl
#	modified:   template/en/default/account/email/change-old.txt.tmpl
#	modified:   template/en/default/account/email/request-new.txt.tmpl
#	modified:   template/en/default/account/password/forgotten-password.txt.tmpl
#	modified:   template/en/default/email/bugmail-header.txt.tmpl
#	modified:   template/en/default/email/lockout.txt.tmpl
#	modified:   template/en/default/email/sanitycheck.txt.tmpl
#	modified:   template/en/default/email/sudo.txt.tmpl
#	modified:   template/en/default/email/whine.txt.tmpl
#	modified:   template/en/default/request/email.txt.tmpl
#	modified:   whine.pl

3. All of the different email tests I performed properly filled in the From: header:

From: "Bugzilla@Mozilla" <[e-mail]>

So I would say this would be fine to roll these changes back and instead update the params value if you concur.

dkl

Comment 8

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #7)
> But I suppose an admin may want to have the site title possible be
> different than what is in the email From: header so would want to
> designate the value manually.

the site title is a bmo customisation, so there's no need to worry about any admins aside from us.

> So I would say this would be fine to roll these changes back and instead
> update the params value if you concur.

let's do that :)

Comment 9

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #8)
> let's do that :)

Committed. Make sure to update mailfrom param right after so people do not notice any difference.

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   ac9980a..685776f  master -> master

dkl

Comment 10

[:glob ✱]

param changed to: "Bugzilla@Mozilla" <[e-mail]>