467859 - add X-Bugzilla-Flags to mail headers

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[Noura Elhawary]

Attachment: v1 for adding flag changes to mail headers

Hi,

It would be good if we can have the flag changes that occured in a bug in the mail headers. attached is a patch to enable this.

Noura

Comment 1

[Max Kanat-Alexander]

Comment on attachment 351292 [details] [diff] [review]
v1 for adding flag changes to mail headers

This is LpSolit's area.

Comment 2

[Frédéric Buclin]

Comment on attachment 351292 [details] [diff] [review]
v1 for adding flag changes to mail headers

>Index: Bugzilla/BugMail.pm

>+    for my $f (@$bug_flags){
>+        if (defined $f->type) {
>+            push(@changed_flags, $f->name . $f->status);
>+        }
>+    }


$f->type cannot be undefined. Also, why do you say "changed_flags" when you list all flags?

Comment 3

[Noura Elhawary]

Attachment: v2 for adding flags to mail headers

Thanks for the review Frederic, here is another modified patch.

Noura

Comment 4

[Frédéric Buclin]

Comment on attachment 351850 [details] [diff] [review]
v2 for adding flags to mail headers

>+    my $bug_flags = Bugzilla::Flag->match({ 'bug_id'      => $id,
>+                                            'target_type' => 'bug' });
>+
>+    my @flags;
>+    foreach my $f (@$bug_flags){
>+        push(@flags, $f->name . $f->status);
>+    }

This code must not be in sendMail(), else it would be called again and again for each user getting bugmail. It must go in the first half of Send(), where other %values fields are populated.


>+        flags => join(", ", @flags),

Don't include the comma. I think blocking3.2+ approval+ is nicer than blocking3.2+, approval+



>Index: template/en/default/email/newchangedmail.txt.tmpl

>+X-Bugzilla-Flags: [% flags %]

You should enclose this in [% Bugzilla.has_flags %] [% END %] or [% IF flags %] [% END %]. Maybe the latter is better.

Comment 5

[Frédéric Buclin]

too late for 3.4.

Comment 6

[Matt Selsky [:selsky]]

Attachment: v3

I updated Noura's patch as per the comments.

Comment 7

[Frédéric Buclin]

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

Since this patch was written in 2008, the code about flags has evolved a lot and you can now simply call $bug->flags which will return an arrayref of flag objects. As the backend code already caches bug flags, you don't need to cache the result in BugMail.pm again, and you simply need to generate the desired output in the template itself (it will be regenerated once per recipient, but that's better than passing a flag variable; the perf impact should be minimal).

Comment 8

[Matt Selsky [:selsky]]

Attachment: v4

Ah.  That's much easier.

Comment 9

[Frédéric Buclin]

Comment on attachment 637124 [details] [diff] [review]
v4

r=LpSolit

Comment 10

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified template/en/default/email/bugmail-header.txt.tmpl
Committed revision 8281.

Comment 11

[Frédéric Buclin]

Added to relnotes for 4.4.