785346 - Received bugmail for watched component dependency change, even though deselected in email prefs

Status: NEW

(Bugzilla :: Email Notifications, defect)

Description

[Ed Morley [:emorley]]

I have the 'The dependency tree changes' row for the 'component' column in email prefs unticked.

One of the components I am watching is Webtools::Tinderboxpushlog 
I am not watching mozilla.org::server operations: projects

However, I received an email for a Webtools::Tinderboxpushlog bug whose dependant bug (in server operations: projects) had just been closed. The bug in the component that I am watching hasn't changed for two years (https://bugzilla.mozilla.org/show_activity.cgi?id=573144).

{
Delivered-To: bmo@<snip>
Received: by 10.49.121.193 with SMTP id lm1csp277370qeb;
        Thu, 23 Aug 2012 18:50:45 -0700 (PDT)
Received: by 10.66.89.234 with SMTP id br10mr7293163pab.25.1345773044896;
        Thu, 23 Aug 2012 18:50:44 -0700 (PDT)
Return-Path: <[e-mail]>
<snip>
From: [e-mail]
To: bmo@<snip>
Subject: [Bug 573144] TBPL destroys my umlaut
Date: Fri, 24 Aug 2012 01:50:42 +0000
X-Bugzilla-Reason: None
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: Component-Watcher
X-Bugzilla-Classification: Server Software
X-Bugzilla-ID: 573144
X-Bugzilla-Product: Webtools
X-Bugzilla-Component: Tinderboxpushlog
X-Bugzilla-Keywords: 
X-Bugzilla-Severity: normal
X-Bugzilla-Who: shyam@<snip>
X-Bugzilla-Status: RESOLVED
X-Bugzilla-Priority: --
X-Bugzilla-Assigned-To: [e-mail]
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Changed-Fields: bug_status resolution
Message-ID: <[e-mail]/>
In-Reply-To: <[e-mail]/>
References: <[e-mail]/>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: https://bugzilla.mozilla.org/
Auto-Submitted: auto-generated
MIME-Version: 1.0

https://bugzilla.mozilla.org/show_bug.cgi?id=573144

Bug 573144 depends on bug 452718, which changed state.

Bug 452718 Summary: hgweb pages sometimes don't grok non-ascii characters (output uses incorrect character encoding/charset in some cases)
https://bugzilla.mozilla.org/show_bug.cgi?id=452718

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

Referenced Bugs:

https://bugzilla.mozilla.org/show_bug.cgi?id=452718
[Bug 452718] hgweb pages sometimes don't grok non-ascii characters (output
uses incorrect character encoding/charset in some cases)

-- 
Configure bugmail: https://bugzilla.mozilla.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are watching the component for the bug.
}

Comment 1

[:glob ✱]

i suspect this is most likely a result of the html bugmail backport than the component watching extension.  dkl said he'll investigate this.

Comment 2

[:glob ✱]

bug 788934 is probably related.

Comment 3

[David Lawrence [:dkl]]

Byron. 

  I have looked at the diffs, and all through the code and cannot see how this behavior was not present before. The email preference for "Dependency tree changes" normally means "when a dependency is added or removed from a bug report" but has special meaning on dependency type emails in the code. If a user has "dependency tree changes" deselected for all, but has "The bug is resolved or reopened" and/or "The priority, status, severity, or milestone changes" selected, they should still get the extra dependency emails. An email is sent if one or more events match. And in the code "Dependency tree changes" is being added back as an event in Bugzilla::User::wants_bug_mail if the email is a dependency email (dep_only). This happened in 4.0 and works the same in 4.2. The only way this would work as this bug is being reported is if we ignored all other events and only pass in the event "Dependency tree changes" to Bugzilla::User::wants_mail.  

  FWIW the issue in 788934 does not seem related and was simply a mistake on my part for using bug.id instead of change.blocker.id. Also it doesn't seem component watch related as you mentioned as the extension only adds the relationship to the recipients hash and doesn't dictate anything about whether the user will receive the email or not.

Any ideas?
dkl

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to fix emailing of dependency changes despite preferences (v1)

Ok, I played around with this some more and I am not sure why I didn't notice before but I figured out why this is happening. This is an upstream issue and the default behavior changed from 4.0 to 4.2.

In 4.0, for emails about dependent bug changes the dependency @diffs array was empty as $dependencyText was generated separately from the normal source bug changes @diffs array. This $dependencyText value was passed along with the empty @diffs array to $user->wants_bug_mail() and it used those to determine if EVT_DEPEND_BLOCK was added to the events list before determining if the user wanted the email based on their relationship to the dependent bug. So for dependent emails, only the EVT_DEPEND_BLOCK event existed and so the user's email preferences would allow them to disallow receiving those type of emails if they wanted.

In 4.2, for dependency emails, a hard coded @diffs array is created containing the new status and resolution values and passed to $user->wants_bug_mail along with the value $dep_mail. The method looks at the values in @diffs and determines that status has changed and adds EVT_OPEN_CLOSED and EVT_PROJ_MANAGEMENT. Further down it looks at $dep_mail and adds EVT_DEPEND_BLOCK. So the user receives email if one or more of those are true for the email settings, not just for dependency emails which is probably true for most people.

Not it is arguable if 4.0 had the behavior wrong all along because the "Dependency tree changes" email preference is really for emails where the blocks/dependson list is changed and not necessarily to turn on/off dependency email notifications. And with 4.2 that was fixed to be more correct. But with the 4.2 change, it leaves users without a way to turn off the dependency emails.

This patch looks at $dep_mail in wants_bug_mail() and passes only the EVT_DEPEND_BLOCK to determine if the user wants the email regardless of the other values passed in @diffs.

Comments?

dkl

Comment 5

[Reed Loden [:reed]]

Comment on attachment 659295 [details] [diff] [review]
Patch to fix emailing of dependency changes despite preferences (v1)

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

drive-by comments...

::: Bugzilla/BugMail.pm
@@ +118,4 @@
>      }
>      if ($params->{dep_only}) {
>          push(@diffs, { field_name => 'bug_status',
> +                       field_desc => 'Status', 

This and the below addition seem wrong... Shouldn't the code use the localized version of field_desc, as it may not always be English.

::: Bugzilla/User.pm
@@ +1562,5 @@
>          $events{+EVT_DEPEND_BLOCK} = 1;
>      }
> +    else {
> +        # Make a list of the events which have happened during this bug change,
> +        # from the point of view of this user.    

You added a lot of whitespace. Can you remove that, please?

Comment 6

[Frédéric Buclin]

Comment on attachment 659295 [details] [diff] [review]
Patch to fix emailing of dependency changes despite preferences (v1)

As I rewrote a large part of BugMail.pm for 4.2 (bug 466968), I want to review this patch too.

Comment 7

[David Lawrence [:dkl]]

(In reply to Reed Loden [:reed] from comment #5)
> Comment on attachment 659295 [details] [diff] [review]
> Patch to fix emailing of dependency changes despite preferences (v1)
> 
> Review of attachment 659295 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> drive-by comments...
> 
> ::: Bugzilla/BugMail.pm
> @@ +118,4 @@
> >      }
> >      if ($params->{dep_only}) {
> >          push(@diffs, { field_name => 'bug_status',
> > +                       field_desc => 'Status', 
> 
> This and the below addition seem wrong... Shouldn't the code use the
> localized version of field_desc, as it may not always be English.

Sorry. Originally I did this against the BMO tree which needed this change since we need 'field_desc' for the email headers. Will create a trunk patch without this included.
 
> ::: Bugzilla/User.pm
> @@ +1562,5 @@
> >          $events{+EVT_DEPEND_BLOCK} = 1;
> >      }
> > +    else {
> > +        # Make a list of the events which have happened during this bug change,
> > +        # from the point of view of this user.    
> 
> You added a lot of whitespace. Can you remove that, please?

New patch coming up.

dkl

Comment 8

[David Lawrence [:dkl]]

Attachment: Patch to fix emailing of dependency changes despite preferences (v2)

Comment 9

[David Lawrence [:dkl]]

The more that I think about this, it is confusing about the current email preferences in that it states "Dependency tree changes". This makes me think that the preference should be for when blockers are add/removed from a bug, then email the user.

We should have a separate email preference and related event constant specifically for enabling/disabling receiving of dependency changes. And let the current preference do what it is supposed to do by it's description. 

I can work on a patch if everyone feels this would make more sense.

dkl

Comment 10

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #9)
> that the preference should be for when blockers are add/removed from a bug,

Right, that's what it's supposed to do.


> We should have a separate email preference and related event constant
> specifically for enabling/disabling receiving of dependency changes. And let
> the current preference do what it is supposed to do by it's description. 

This would be better than hacking the way the current email pref works, yes.

Comment 11

[Frédéric Buclin]

Comment on attachment 659315 [details] [diff] [review]
Patch to fix emailing of dependency changes despite preferences (v2)

Per my previous comment.

Comment 12

[Frédéric Buclin]

The current behavior is currently not a bug, and we are not going to add a new email pref to 4.2. The fix can go in 4.4 if it's done before 4.4rc1.

Comment 14

[Ed Morley [:emorley]]

(In reply to Frédéric Buclin from comment #12)
> The current behavior is currently not a bug, and we are not going to add a
> new email pref to 4.2. The fix can go in 4.4 if it's done before 4.4rc1.

Whilst I agree that the current pref name is unclear (comment 9 / comment 10), that is orthogonal to this bug as filed.

The problem I would like to see resolved is (example scenario):
* Bug A is in component Foo, which is on my component watch list.
* My email prefs for watched components have only the following ticked: "A new bug is created" & "The product or component changes"
* There is another bug, Bug B, which blocks bug A.
* Neither bug A nor bug B is related to me (CCed, voted, reported etc)
* Bug B is marked as fixed.
-> I should not get bugmail about bug A, when this happens - but I currently do.

I've just gone through my bugmail in trash, and this bug accounts for a fair percentage of the noise.

Please can we at least prevent the emails from being sent in this case - and save fixing the pref name (/adding another pref to cover what "The dependency tree changes" was meant to mean originally etc) to a followup?

Comment 15

[Ed Morley [:emorley]]

To add to comment 14:

* Bug B is in component Bar, which is *not* on my watch list.

Comment 16

[Ed Morley [:emorley]]

Tweaking my email prefs after seeing http://hearsum.ca/blog/how-to-not-get-spammed-by-bugzilla/ reminded me about this bug again. Could you take a look at comment 14 and see if that is what the attached patch was attempting to fix? I'm confused by comment 12 - I don't know whether we're all talking about the same thing :-)

Comment 17

[ben]

fyi, this still seems to affect at least BMO - for now I've disabled all emails for the "Voter" relationship, which might help. 

Possibly I also misunderstand the matrix. I voted on https://bugzilla.mozilla.org/show_bug.cgi?id=1378418 and I keep getting emails about the dependencies being resolved despite the only email for voter relationship being "The bug is resolved or reopened" (which I would assume is not the same as dependencies being resolved).