71550 - Watching is broken if watched user has set "Do not email me bugs that I change" (which is the default)

Status: VERIFIED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Andreas Franke (gone)]

Let A watch B.
Let B set "Do not email me bugs that I change" pref.
Let B make a change.

Actual Result:
Both A and B are _not_ notified.

Expected Results:
B is not notified, but A is.

Bugzilla version:
bugzilla.mozilla.org as of now.

Additional comments:
This broke with the upgrade. It was working before.

Comment 1

[Matthew Tuck [:CodeMachine]]

Sounds like a 2.12 blocker to me.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

FWIW, the current behavior is what I would have expected to be the case all
along.  However, it certainly seems like the old behavior is more useful.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

OK, I see what's going on here.  The code that checks for watchers is inside
NewProcessOnePerson().  However, much of the filtering happens in
filterEmailGroup(), and NewProcessOnePerson() never gets called for users that
are filtered out there.  A possible fix is to move the watcher code from
NewProcessOnePerson() to filterEmailGroup().  This would make a watcher see all
potential notifications of a watchee subject to the watcher's emailflags (rather
than the watchee's emailflags, which is currently what happens).  

It's conceivable that this proposed fix might break when oldemailtech is
involved, but it might not.

Comment 4

[Dave Miller [:justdave]]

actually, thinking about the fix you just proposed, I think that would make 
watching work with oldemailtech (which it currently doesn't).

Comment 5

[Andreas Franke (gone)]

Hixie: I know from irc that you have an opinion about the side effects of the
proposed solution. Can you comment?

Of course, the main point here is that the "Do not email me bugs that I change"
pref should not affect the decision whether mail is sent to someone watching
you. 

As a long-term solution, I propose the following behaviour:
- ExcludeSelfChanges should only effect mails sent to the person making the
change; it should not have any effect for watchers. This should be independent
of all the other mail-or-not-mail computations, so it should happen at the end
of all the computations, i.e. after the computation for watchers has been done:
immediately before the mail is sent, the person making the change should be
excluded if the flag is set.
- For everyone you watch, you should be able to specify whether you want to be
notified according to 
a) the watched person's email flags, 
b) your own email flags, 
c) the intersection of these email flags (i.e. notify me only if my flags allow
it _and_ the watched person's email flags allow it), or
d) the union of these email flags (i.e. notify me if my flags _or_ the watched
person's flags allow it).

I'm not sure whether b) and d) are necessary, or if they make sense at all, but
a) and c) seems to make sense, at least to me.

Of course, it would be possible to create another set of email flags for use
with watchers (and still offer at least a) and c) for each person you watch), or
even have a complete set of email flags for each one you watch (in this case the
choice between a) and c) would be unnecessary; c) would be always used, and to
achieve a) you would simply select "send mail always", which would be the
default.)

Comment 6

[Dave Miller [:justdave]]

*** Bug 71674 has been marked as a duplicate of this bug. ***

Comment 7

[Daniel Veditz [:dveditz]]

bug 71674 was closed as a duplicate of this, but the situation there was

Let A watch B
Let *A* set "Do not email me bugs that I change"
Let A change a bug that B is CC'd on

Actual result:
email is sent to A

Expected result
A should not receive mail for a bug which he changed, regardless of whether B 
was going to get mail about it.

I couldn't decipher whether this case was covered in the discussion so I'm 
adding it here. If this is not the same thing please reopen bug 71674

Comment 8

[Dave Miller [:justdave]]

*** Bug 71831 has been marked as a duplicate of this bug. ***

Comment 9

[Jacob Steenhagen]

Attachment: Patch to fix watching

Comment 10

[Jacob Steenhagen]

This patch moves the watching code from NewProcessOnePerson() to
filterEmailGroup().  Putting it inside this filtering area makes it possible to
inherit ownership.

IE, if the you are watching person A and person A is the owner of a bug.  When
that bug changes, the email getting sent to you will be filtered according
*your* "owner" settings.

Marking for 2.12 as Matty thinks it's a show-stopper and there is a patch.

Comment 11

[Hixie (not reading bugmail)]

I want to get *all* mail. Regardless of whether someone else wants to get mail
or not. Why should I (as person B) care if person A (whom I am watching) wants 
to get mail or not?

Actually, I don't want to get *all* mail. Mail where only fields X, Y and Z
change (except for target milestones of 'Future' and cc lists that include me
or dbaron) could be dropped without me feeling any sort of loss.

Does that answer all the questions I was cc'ed to answer? Please ping me again
if not! :-)

Comment 12

[Jacob Steenhagen]

I guess I didn't explain well enough what the patch does.  You (person A)
inherit person B's relationship to the bug, not their mail filtering prefrences.
Your filters are then run as if it were you in that position.

Example for this bug:
I watch Tara.  I'm also on the CC list of this bug.  So the filters for owner
and CC would be run for me when I click commit.  If either of those filters
decide I should get the mail, I'll get it.

Comment 13

[Jacob Steenhagen]

Attachment: Patch v2

Comment 14

[Jacob Steenhagen]

Patch v2 doesn't really change much, it just removes the now obsolete flag for
$checkWatchers in NewProcessOnePerson().  However, I just realized that I forgot
to remove this from calls to NewProcessOnePerson().  Patch v3 will be up in a
minute w/that addition.

Comment 15

[Jacob Steenhagen]

Attachment: Patch v3

Comment 16

[Andreas Franke (gone)]

Jake: if I understand your patch, it implements my alternative b), correct?
If so, I think that's a good solution for now.

Hixie: if I understand your comment, this is exactly what you want. The only
thing that you might want to comment on is why you said "I hope not" when I
described this behaviour to you on irc some days ago :)

Comment 17

[Jacob Steenhagen]

Andreas: Yes, this impliments your b) only.  I personally don't think that a, c,
or d are worthwhile, after all I'm not really concerned with the e-mail flags of
who I'm watching (why should I be concerned if the person I'm watching doesn't
like e-mail so they uncheck all the boxes [except of course "When I'm removed
from Reporter"]?).  Also, adding the prefs and code support to do a, c, and/or d
would really be a lot of extra work.

Comment 18

[Hixie (not reading bugmail)]

afranke: probably just crossed wires, sorry :-)

Comment 19

[Dave Miller [:justdave]]

r= [e-mail]

checked in.

Comment 20

[Jacob Steenhagen]

Oops... I just noticed a slight error in my patch (when I was reading the diff
on bonsai).  Bugfix 1 is forthcoming.

The error is that the code for populating the @watchers array wasn't wrapped in
a while(MoreSQLData()) loop.  This made it so each user could only be watched
once (and I know there's alot of ppl out there w/more than one watcher :).

Comment 21

[Jacob Steenhagen]

Attachment: Bugfix 1

Comment 22

[Dave Miller [:justdave]]

r= [e-mail]

Checked in.

Comment 23

[Jacob Steenhagen]

*Mumble* [bad word]... it's still not entirely fixed.... the good news, I found
the problem!!!!

I didn't Push/Pop the Global SQL state before calling DBID_to_name so it lost
its query results in the while(MoreSQLData()) loop.

Comment 24

[Jacob Steenhagen]

Attachment: Bugfix 2

Comment 25

[Jacob Steenhagen]

Bugfix v2 should be the final patch for this bug.  5 Patches should be enough to
fix a simple problem, right?

Comment 26

[Dave Miller [:justdave]]

Jake: I don't think this patch is the right approach.

DBID_to_name() should push and pop the SQL state internally, because most of the 
newbie programmers looking at it aren't going to remember that it makes an SQL 
call, so it should make it easy on them even inside loops :-)

Comment 27

[Dave Miller [:justdave]]

Attachment: Bugfix 2 v2

Comment 28

[Jacob Steenhagen]

Yes, Bugfix 2 v2 is the more correct way of fixing this (and possible other)
bugs.  I'd give it my "r"  :)

Comment 29

[Dave Miller [:justdave]]

OK, it's checked in.

Comment 30

[Dawn Endico]

b.m.o updated

Comment 31

[Jacob Steenhagen]

V. 
Watching, filtering and "Don't notify me of my own changes" are all working
based on the prefs of the person recieving the mail, not the person being watched.

Comment 32

[Jacob Steenhagen]

*** Bug 71687 has been marked as a duplicate of this bug. ***

Comment 33

[Dave Miller [:justdave]]

Moving closed bugs to Bugzilla product