Bugmail filtering allows through bugmail that should be blocked when combined with the standard email prefs

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: BugmailFilter
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: emorley, Assigned: glob)

Tracking

Production

Details

Attachments

(2 attachments, 1 obsolete attachment)

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?
(Reporter)

Updated

3 years ago
Summary: Bugmail filtering allows through bugmail that would be blocked → Bugmail filtering allows through bugmail that should be blocked when combined with the standard email prefs
(Assignee)

Comment 1

3 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.
Assignee: nobody → glob
Component: Extensions: Other → Extensions: BugmailFilter
No longer depends on: 990980
(Reporter)

Comment 2

3 years ago
Sorry yes that was a typo - s/checkout/checkbox/
(Reporter)

Comment 3

3 years ago
Created attachment 8453781 [details]
email-prefs-screenshot
(Assignee)

Comment 4

3 years ago
Created attachment 8455412 [details] [diff] [review]
1036872_1.patch

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.
Attachment #8455412 - Flags: review?(dkl)
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?
(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
(Assignee)

Comment 7

3 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).
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1079190
(Reporter)

Comment 9

3 years ago
dkl, ping for review? :-)
Created attachment 8646643 [details] [diff] [review]
1036872_2.patch

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
Attachment #8455412 - Attachment is obsolete: true
Attachment #8455412 - Flags: review?(dkl)
Attachment #8646643 - Flags: review+
(Assignee)

Comment 11

2 years ago
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   397050b..56c61ae  master -> master
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.