279318 - Flag notifications are sent even if emails are turned off for the addressee

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, defect)

Description

[Jared Warren]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

an entry in ./data/nomail appears to fail to block flag requests

Reproducible: Always

Steps to Reproduce:
1.  Add your account's email address to ./data/nomail
2.  Open a bug to ensure you don't get mail
3.  Set a flag to 'request' status, and provide your account as the requestee
4.  You'll get mail

Actual Results:  
Got mail

Expected Results:  
Didn't expect to get mail

Comment 1

[Max Kanat-Alexander]

Shane, do you know anything about this?

Comment 2

[Shane H. W. Travis]

Not specifically, no... but I do know that flag mail is wonked in other ways. 
If you say that you never want to see mail from your own changes, you still get 
bugmail from request changes.

It seems to me that the mail algorithm for flag mail doesn't follow the same 
paths as for bugmail. Adding Frederic to the cc: list because he does a lot of 
work on flags, and maybe he can confirm this... and perhaps even come up with a 
fix for it.

Comment 3

[Jared Warren]

Just did another clean install of 2.18 and tested this again to make sure I
hadn't just wierded out.  It does appear to be broken.

Comment 4

[Olav Vitters]

Confirming. Bugzilla/Flag.pm does not use data/nomail.

Comment 5

[Tiago Mello [:timello]]

Attachment: Skips the email notification if the addressee is not allowed to receive it.

Comment 6

[Frédéric Buclin]

Comment on attachment 274024 [details] [diff] [review]
Skips the email notification if the addressee is not allowed to receive it.

>     # There is nobody to notify.
>     return unless ($flag->{'addressee'} || $flag->type->cc_list);
> 
>+    # The addressee is not allowed to receive the notification.
>+    return if $flag->{'addressee'}->email_disabled;

At this point, you aren't sure that $flag->{'addressee'} is defined. Maybe you passed the previous test because $flag->type->cc_list is defined. If it's undefined, you cannot call ->email_disabled() on it.
Moreover, you shouldn't return at this point. Even if the addressee is defined and shouldn't get email notification, you should still notify users on the CC list of this flag type (assuming they pass the following group checks). A probably better approach is to exclude it from the final loop at the end of notify().

Comment 7

[Tiago Mello [:timello]]

Attachment: Does not push the addressee if he is not allowed to receive emails.

Comment 8

[Frédéric Buclin]

Comment on attachment 274621 [details] [diff] [review]
Does not push the addressee if he is not allowed to receive emails.

Works fine. Thanks for the patch. r=LpSolit

Comment 9

[Frédéric Buclin]

Looks safe to take for 3.0.1.

Comment 10

[Frédéric Buclin]

tip:

Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.85; previous revision: 1.84
done

3.0:

Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.83.2.2; previous revision: 1.83.2.1
done