Ed Morley [:emorley] Reporter
	Description • 10 years ago

My Bugmail filtering prefs look like:
	Any 	Any 	Any 	            Mentoring 	        Include
	Any 	Any 	Attachment is obsolete 	Not Assignee 	Exclude
	Any 	Any 	Iteration 	    Not Assignee 	Exclude
	Any 	Any 	QA Whiteboard 	    Not Assignee 	Exclude
	Any 	Any 	Flags 	            Not Assignee 	Exclude
	Any 	Any 	OS/Version 	    Not Assignee 	Exclude
	Any 	Any 	QAContact 	    Not Assignee 	Exclude
	Any 	Any 	B2G Flags 	    Not Assignee 	Exclude
	Any 	Any 	Blocking Flags 	    Not Assignee 	Exclude
	Any 	Any 	Project Flags 	    Not Assignee 	Exclude
	Any 	Any 	Tracking Flags 	    Not Assignee 	Exclude
	Any 	Any 	Version 	    Not Assignee 	Exclude

And on the standard email preferences page, I have every checkout unchecked in the "The CC field changes" row.

However I just received the following:

Subject: [Bug 1035101] H264 HTML5 videos appears black/blank on Youtube and
 others in 2014-07-07 nightly
Date: Thu, 10 Jul 2014 09:54:18 +0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Classification: Components
X-Bugzilla-ID: 1035101
X-Bugzilla-Product: Core
X-Bugzilla-Component: Graphics: Layers
X-Bugzilla-Version: 33 Branch
X-Bugzilla-Keywords: regression
X-Bugzilla-Severity: normal
X-Bugzilla-Who: bkerensa@<snip>.com
X-Bugzilla-Status: RESOLVED
X-Bugzilla-Resolution: FIXED
X-Bugzilla-Priority: --
X-Bugzilla-Assigned-To: nical.bugzilla@gmail.com
X-Bugzilla-Target-Milestone: mozilla33
X-Bugzilla-Flags: 
X-Bugzilla-OS: Windows 7
X-Bugzilla-Changed-Fields: CC tracking-firefox33
X-Bugzilla-Changed-Field-Names: cc cf_tracking_firefox33
X-Bugzilla-URL: https://bugzilla.mozilla.org/
{
Benjamin Kerensa [:bkerensa] <bkerensa@<snip>.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bkerensa@<snip>.com
 tracking-firefox33|?                           |-

-- 
Configure bugmail: https://bugzilla.mozilla.org/userprefs.cgi?tab=email

-------------------------------
Product/Component: Core :: Graphics: Layers


------- Tracking Flags: -------
tracking-firefox33:-
status-firefox33:affected

------- You are receiving this mail because: -------
You are on the CC list for the bug.
}

I'm guessing this occurred because:
* Standard email filter saw CC+tracking change, and let the bugmail through, since I've asked for tracking flags changes (via "Any field not mentioned above changes")
* Bugmail filtering applies after that - and sees not just the tracking change let through, but also the CC change. So it filters out the tracking flag change, leaving the CC change - so still thinks it warrants an email.

(I also had another bugmail that got through - that had tracking+target milestone changes - similarly, the target milestone changes should be blocked by the mail email prefs, and the tracking flags by bugmail filtering.)

Note I think that it *is* useful to show filtered changes iff we happen to be sending a bugmail anyway (eg: if someone changes a tracking flag and makes a comment at the same time, I'd like to see the tracking change) - so I understand it makes sense to pass through unwanted fields from one stage of filtering to the next. 

I guess the options we have are:
1) Decide this is expected, and add some documentation to the bugmail filtering page.
2) Make the bugmail filtering page take into account the standard email prefs too (for bonus points, they could be shown in the table as greyed out items inherited from the main email prefs... though this would be spammy and really just mean we should do #3 below).
3) Combine the emails prefs and bugmail filtering pages, since they overlap quite a bit.

I imagine #3 would require a lot of upstream work, but would make the most sense longer term perhaps? With #1/#2 for the short term?

	:glob ✱ Assignee
	Comment 1 • 10 years ago

(In reply to Ed Morley [:edmorley UTC+0] from comment #0)
> And on the standard email preferences page, I have every checkout unchecked
> in the "The CC field changes" row.

do you mean you've unchecked every checkbox in the "CC field changes" row?


this looks like a bug; the filters should be applied after the email-preferences are taken into account.  i'll investigate.

	Ed Morley [:emorley] Reporter
	Comment 2 • 10 years ago

Sorry yes that was a typo - s/checkout/checkbox/

	:glob ✱ Assignee
	Comment 4 • 10 years ago

this patch makes bugmail filtering interact correctly with email preferences.

it works by checking if each individual field would have been excluded by email preferences, and if so it adds a Filter object to the user's filter list.

	David Lawrence [:dkl]
	Comment 5 • 10 years ago

Comment on attachment 8455412 [details] [diff] [review]
1036872_1.patch

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

::: extensions/BugmailFilter/Extension.pm
@@ +176,5 @@
> +    my $depth = 0;
> +    for (my $stack = 1; my $sub = (caller($stack))[3]; $stack++) {
> +        $depth++ if $sub eq 'Bugzilla::User::wants_bug_mail';
> +    }
> +    return if $depth > 1;

Could you just set a request_cache value such as bugmail_filter_done => 1 which may be simpler?

	David Lawrence [:dkl]
	Comment 6 • 10 years ago

(In reply to David Lawrence [:dkl] from comment #5)
> Could you just set a request_cache value such as bugmail_filter_done => 1
> which may be simpler?

Course I meant per user id :)

dkl

	:glob ✱ Assignee
	Comment 7 • 10 years ago

(In reply to David Lawrence [:dkl] from comment #5)
> Could you just set a request_cache value such as bugmail_filter_done => 1
> which may be simpler?

that would be possible, however i prefer to avoid using the request_cache to track state unless reaps substantially simpler code (which wouldn't be the case here; checking the caller stack is already pretty simple).

	Ed Morley [:emorley] Reporter
	Comment 9 • 10 years ago

dkl, ping for review? :-)

	David Lawrence [:dkl]
	Comment 10 • 9 years ago

Sorry for taking so long on this one :( And of course there was some bit-rot along the way because of the time.

I have attached a v2 of the patch that has the bit-rot fixed and it passes review and testing for me.

Please look over my changes and if ok feel free to push out.

r=dkl

	:glob ✱ Assignee
	Comment 11 • 9 years ago

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   397050b..56c61ae  master -> master