365735 - Add classification, alias, assigned to name, reporter login and reporter name fields to BugMail

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[Albert Ting]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061204 Firefox/2.0.0.1
Build Identifier: 

I'd like to request there be more fields/values for the BugMail template (classification, reporter, reporter name, owner, and ownername).

Reproducible: Always

Comment 1

[Albert Ting]

Attachment: v1

Comment 2

[Max Kanat-Alexander]

Comment on attachment 250246 [details] [diff] [review]
v1

"Owner" already exists as "Assigned-To".

Also, if you're going to add these fields, you should add X-Bugzilla headers for them to the template.

Also, the way you do reportername is incorrect--it should be "new Bugzilla::User({ name => $values{reporter}})" to instantiate the object. Also, this object should already be available through the bug object, but maybe we don't have a bug object available at this point?

Comment 3

[Max Kanat-Alexander]

Also, we have to decide if we want to just keep adding headers to bugmail, or if there's some reason to limit it.

Comment 4

[Albert Ting]

Do we need to keep adding headers?  I can see perhaps classification and reporter, but not much else.  My main interest is to add more info in the email message itself.   

Comment 5

[Albert Ting]

Attachment: v2

Updated patch to include X-Bugzilla header for classification.  But I did not add the other ones, as I don't think they're necessary.  Also cleaned up the Bugzilla::User calls.

Comment 6

[Max Kanat-Alexander]

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

I think Wicked is a better reviewer for this.

Comment 7

[Teemu Mannermaa (:wicked)]

In the future we should look into generic settings for these. Something like that the one suggested in bug 86201. Or perhaps give email templates objects that they can use to retrieve all info they ever want to add into emails.

Comment 8

[Teemu Mannermaa (:wicked)]

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

These additions seem to work as expected.

Alias can be blank if it's not defined but that's the way some other fields work too so it's fine.

Also, I'm not entirely sure we should add classification field when they are disabled. Looks like they will always be Unclassified so maybe that's not too bad.

Comment 9

[Max Kanat-Alexander]

Just somebody make sure before you check this in that "alias" isn't actually *undefined* if usebugaliases is off.

Comment 10

[Frédéric Buclin]

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

>+        assignedtoname => new Bugzilla::User({ name => $values{'assigned_to'}})->name,

IIRC, this syntax is incorrect. You must write Bugzilla::User->new()->name.

Comment 11

[Teemu Mannermaa (:wicked)]

(In reply to comment #10)
> >+        assignedtoname => new Bugzilla::User({ name => $values{'assigned_to'}})->name,
> IIRC, this syntax is incorrect. You must write Bugzilla::User->new()->name.

It works in my tests and that's what mkanat suggested in comment #2. So, what is it, mkanat?

Comment 12

[Max Kanat-Alexander]

It's ambiguous, but it will work. LpSolit's syntax is more reliable as far as compatibility with future versions of Perl (or future modifications of that line of code). It should work either way, but feel free to make LpSolit's change on checkin.

Comment 13

[Frédéric Buclin]

(In reply to comment #9)
> Just somebody make sure before you check this in that "alias" isn't actually
> *undefined* if usebugaliases is off.

Why? If we don't use aliases, I expect it to be blank. Or do you mean blank vs undefined?

Comment 14

[Max Kanat-Alexander]

(In reply to comment #13)
> Why? If we don't use aliases, I expect it to be blank. Or do you mean blank vs
> undefined?

  Right, I mean undef.

Comment 15

[Frédéric Buclin]

In this case, I will wait for the reviewer to check this before committing the patch.

Comment 16

[Teemu Mannermaa (:wicked)]

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

I just tested this patch to work with both LpSolit's change to object construction and with disabled aliases and classifications. Even when these fields are disabled, their current value is given to the email template. This works because even though aliases or classifications are disabled in params they are considered editable fields by the Bug object. Specifically, any field of bugs table is editable except custom fields that have been obsoleted.

So my r+ still stands.

Comment 17

[Frédéric Buclin]

Isn't it a problem? If aliases and classifications are turned off, I would expect to see "" instead of their real value. After all, they are unusable and trying to use them in Bugzilla will fail, especially the alias. We are not very consistent.

Comment 18

[Frédéric Buclin]

Clearing the approval flag till my question in comment 17 is addressed and till the patch is updated per my comment 10.

Comment 19

[Albert Ting]

Attachment: v3

Updated patch to use the updated construction method and conditionals.

Comment 20

[Frédéric Buclin]

Re-request approval when v3 is reviewed.

Comment 21

[Teemu Mannermaa (:wicked)]

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

>--- Bugzilla/BugMail.pm	2004-05-26 00:21:43.000000000 -0700
>+        assignedtoname => new Bugzilla::User->new({ name => $values{'assigned_to'}})->name,

Remove the first new before checkin.

>+        reportername => new Bugzilla::User->new({ name => $values{'reporter'}})->name,

Same here.

Comment 22

[Frédéric Buclin]

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

>+    my $classification = new Bugzilla::Classification($product->classification_id);

Where do you |use Bugzilla::Classification|? The only reason BugMail.pm doesn't crash is because Bugzilla::User uses it, which is a dangerous assumption.

I will attach an updated patch which also fixes the incorrect usage of 'new'.

Comment 23

[Frédéric Buclin]

Attachment: patch, v3.1

Add use Bugzilla::Classification.

Comment 24

[Frédéric Buclin]

Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.109; previous revision: 1.108
done
Checking in template/en/default/email/newchangedmail.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/newchangedmail.txt.tmpl,v  <--  newchangedmail.txt.tmpl
new revision: 1.10; previous revision: 1.9
done