122900 - allow user to turn off email notification for Unconfirmed bugs

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect)

Description

[sairuh (rarely reading bugmail)]

not sure if this is the right place, or if this should go to the mozilla.org
product... but in order to cut down on the amound of bugzilla email
notifications, it would be highly useful to be able to voluntarily turn off
email for bugs which are UNCONFIRMED.

how feasible would this be?

Comment 1

[Peter Trudelle]

This would be a *huge* benefit for XPApps/Navigator/XPToolkit.  About 80% of our
bugs are not fixable (wfm,dup,wontfix,etc) , and many of those are unconfirmed.
W e are drowining in worthless bugs, and could fix a lot more of the real
defects if we  weren't getting notifications on bugs we can't even afford to read.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: implements feature

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Note that this patch is a diff against a Bugzilla installation with version six
of Gerv's patch to templatize userprefs.cgi applied (bug 117060, attachment
71284 [details] [diff] [review]).  Removing dependency on bug 98123, which doesn't apply (my mistake)
because this is an email-specific preference, and adding dependency on bug
117060, which looks just about ready to get checked in.

Also, cleaning up various fields and targeting to the 2.16 milestone.  This
patch would prove extremely useful to a lot of frequent users of one of the
largest Bugzilla installations, and it is small and unscary, so let's get it in.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: corrected default to "true" users with existing email preferences

This patch fixes a bug in the previous patch where users with existing email
preferences would have "notify me about unconfirmed bugs" turned off when they
went to their email preferences page.  This default should be true so that
users get the same emails after the patch as before unless they explicitly turn
off these notifications.

Comment 5

[Andreas Franke (gone)]

Comment on attachment 71538 [details] [diff] [review]
patch v1: implements feature

sub filterEmailGroup ($$$) {

-    my ($emailGroup,$refAttributes,$emailList) = @_;
+    my ($emailGroup,$refAttributes,$emailList,$bug_status) = @_;

Are you sure the three $$$ are still correct?

A purely philosophical question: shouldn't we talk about ExcludeUnconfirmed
instead of IncludeUnconfirmed?

The reason for the current naming seems to be ease of implementation
(analogous to ExcludeSelf).

Hm, I think there should be different checkboxes for different roles.

For example, if I'm cc'ed on a bug, I certainly want to get mails about it even
if it's unconfirmed. Same for QAs: QA contacts _should_ get mails about
unconfirmed bugs because they are most likely to reproduce them.

On the other hand, overloaded assignees may want to turn these notifications
off.

I think it should be per role, not a global pref.

Comment 6

[Bradley Baetz (:bbaetz)]

If someone ccs me on an unconfirmed bug to say "what do you think"? I'd like to
get mail. If I'm QA contant for some areas, and asignee for others, hten I'd
want unconfirmed mail for those I'm qa contact on, but not those I'm the
assignee for.


This should definately be role based.

Comment 7

[lchiang]

I think this preference should be written so that the QA Contact CAN override to
be notified on unconfirmed bugs.    It's not just engineers who get overloaded
with bugmail. QA contacts do also (for certain areas, not all).  

I'd like to have it so that the default for QA contacts would be to get bugmail
on unconfirmed bugs.  However, a preference can be set to override this.   If we
do this, I will still ensure that the QA contacts (working at Netscape) look at
unconfirmed bugs, but not have to fill their bugmail with these notifications.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

>Are you sure the three $$$ are still correct?

Nope, in fact I'm sure they are wrong. :-)  Thanks for catching that.

>A purely philosophical question: shouldn't we talk about ExcludeUnconfirmed
>instead of IncludeUnconfirmed?
>
>The reason for the current naming seems to be ease of implementation
>(analogous to ExcludeSelf).

Actually, I chose the current naming scheme because it is an affirmative
statement, which is usually a more intuitive description of a value represented
by a checkbox, and because it is consistent with the majority of other
statements on the page (check the box to get the mail).

Nevertheless there is an ease of implementation issue here as well if this
preference becomes role-based rather than global (which everyone seems to agree
is necessary), since all other role-based preferences are "on" by default, and
the code thus assumes that new preferences are also "on" by default for all
users unless they explicitly turn them off.  This makes it much easier to
implement an affirmative statement than a negative one.

Comment 9

[Andreas Franke (gone)]

I fear that we will need two sections for each role:
- an "include" section that is computed first, and
- an "exclude" section that is computed afterwards.

<screenshot type="ASCII" comment="haven't looked at the new table layout yet">

Notify me of the following changes: 

                        | assignee | qa | cc | reporter |
                        +----------+----+----+----------+
   - include option 1   |          |    |    |          |
                        +----------+----+----+----------+
   - ...                |          |    |    |          |
                        +----------+----+----+----------+
   - include option n   |          |    |    |          |
                        +----------+----+----+----------+

but do not send me any notification (overrides above):

                        +----------+----+----+----------+
   - at all             |          |    |    |          |
                        +----------+----+----+----------+
   - for my own changes |          |    |    |          |
                        +----------+----+----+----------+
   - for UNCONFIRMED    |          |    |    |          |
                        +----------+----+----+----------+

</screenshot>

You get the idea. The "at all" option would be a simple switch to not get any
mail at all, "for my own changes" would be "excludeSelf" per role (you wouldn't
need the global "excludeSelf" any more. [You could even add another option to
exclude "comments-only changes made by people in my blacklist" (bug 117928), but
people may have different opinions about this feature.]

In this model with two sections, the items within each part are OR-ed: First you
check whether any of the "include" options applies, and if it does, you still
check whether any of the "exclude" options applies, and if it does not, then you
send the mail.

Or, if you don't like this "two sections" design, then there should be some
other easily understandable rule about how the items interact. One possibility
would be: "Options further down in the list override earlier options." In this
case, the "exclude UNCONFIRMED" just needs to be fairly at the end of the option
list.

Comment 10

[Andreas Franke (gone)]

BTW, vacation mode (=exclude all) is bug 123971.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: per-role preference

This patch implements a per-role preference for whether or not to receive email
about unconfirmed bugs.  It depends on the email filtering code rewrite in bug
128469.  The larger issue of redesigning the email preferences form has been
broken off and filed as bug 128839.

Comment 12

[Bradley Baetz (:bbaetz)]

This isn't going to make 2.16 if it depends on an email prefs rewrite, is it?

Comment 13

[Myk Melez [:myk] [@mykmelez]]

This bug does *not* depend on an email preferences re-write.  Re-writing the way
email preferences work is a much larger issue than this bug, which is why I
broke it off into a separate bug.

This bug *does*, however, depend on a rewrite of one function in processmail
which I also broke off into a separate bug to make it easier to review.  That
patch could just as easily have been made part of this one.

Comment 14

[Bradley Baetz (:bbaetz)]

Oops. I misread your last comment.

Comment 15

[Dave Miller [:justdave]]

depends on a 2.18 bug
-> 2.18

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: updated for new location of template file

Patch v3 held up surprisingly well in this time of great internal architectural
changes.  Except for the name of the template file, it patched cleanly.  Here's
a version with the name corrected.

Comment 17

[Jouni Heikniemi]

Comment on attachment 84828 [details] [diff] [review]
patch v4: updated for new location of template file

I applied the patches from bug 128469 and this one and tested this out. 

It worked mostly fine, but the biggest problem is that 
I am getting mail about new bugs being filed even after I 
switched off the unconfirmed category. If someone then later
commented on those bugs without changing the status, everything
was fine though.

Some comments based on a quick glance now, not enough for a 
thorough review, though:

>Index: processmail
>+    push (@flags, 'Unconfirmed') if $bug->{'bug_status'} eq $::unconfirmedstate;

Nit: Should be written as if $bug... { push ... } to preserve style, see 
the following lines.

>Index: template/en/default/account/prefs/email.html.tmpl
>===================================================================
>+        description = 'Any field not mentioned above changes' },
>+      { name = 'Unconfirmed',       
>+        description = 'The bug is unconfirmed' },
>+  ] %]

This is an interface change. Should we bump the minor version number now?

And that "The bug is unconfirmed" can't stay as it is. There is already
a description "The bug is resolved or verified", and that sounds quite
odd ("Here, I'll go and unconfirm this bug"). 

A simple change would be to use "The bug is not confirmed yet.", but even
with that, I must say that it's hard to understand the relationships 
between the inclusive and exclusive features on the page. 

Marking needs-work based mostly on the problem with new bug notifications
being sent despite my settings.

Comment 18

[Jouni Heikniemi]

>This is an interface change. Should we bump the minor version number now?

No it's not, forget that. I was misreading the patch.

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v5: review fixes

>And that "The bug is unconfirmed" can't stay as it is. There is already
>a description "The bug is resolved or verified", and that sounds quite
>odd ("Here, I'll go and unconfirm this bug"). 

"The bug is in the unconfirmed state"

>I must say that it's hard to understand the relationships 
>between the inclusive and exclusive features on the page. 

This is bug 128839.

>Marking needs-work based mostly on the problem with new bug notifications
>being sent despite my settings. 

Fixed.

Comment 20

[Gervase Markham [:gerv]]

Comment on attachment 86513 [details] [diff] [review]
patch v5: review fixes

r=gerv. Can't see anything immediately wrong with it. 

I suggest, however, that a post is made to the newsgroups saying something
like:

The following areas have had less testing than normal, and people are asked to
be especially vigilant about glitches there:
- email preferences (the right people being included and excluded)
- ...

Gerv

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v6: the same as patch v5 but without the patch in bug 128469

The last patch inadvertently included the patch on bug 122900.	Here's the true
independent patch.

Comment 22

[Myk Melez [:myk] [@mykmelez]]

Jouni reviewed and said the following to me in email while Bugzilla was down:

Okay, thanks. Needs-work for the patch on bug 122900, because it still has
the problem with sending email about just filed unco bugs. A bit of
debugging would say that this is because the owner/assignee/etc. fields
"change" (from empty to something) and the removeme email pref is set. 

An easy solution would probably be to make the exclusion pref check run
before the force hash check (in filterEmailGroups) but this may have
unprecedented side effects. Another alternative could be to disable
the removeme code for bugs just filed, but I'm not sure if this would
practically mean problems to people with some configurations. 

And, you've still got the nit from my previous review:

push (@flags, 'Unconfirmed') if $bug->{'bug_status'} eq 
$::unconfirmedstate;

which should IMO be 

    if ($bug->{'bug_status'} eq $::unconfirmedstate) {
        push (@flags, 'Unconfirmed');
    }                               

to suit the style of the other if...push-conditions in the next 20+ lines. 


As for the patch in bug 128469:

Index: processmail
===================================================================
 sub filterEmailGroup ($$$) {
+    # Given a role (a type of relationship to the bug such as reporter
+    # or assignee), a set of reasons users might or might not want to
+    # receive email about a bug (f.e. changes made to it), and a list
+    # of users in the given role, determine which users would and would
+    # not receive email notification about the bug by comparing the
+    # reasons to the users' email notification preferences.

Ungh... I have severe trouble reading and understanding this. How about:

This sub figures out who should receive email about the bug, based on the
user's role with regard to the bug (assignee, reporter etc.), the changes
that occurred (new comments, attachment added, status changed etc.) and
the user's email preferences.
     
+    # Returns a filtered list of those users who would receive email
+    # about these changes, and adds the names of those users who would
+    # not receive email about them to the global @excludedAddresses list.
     
+    # Make an list of users to filter.

Nit: _a_ list, please.

+    my @users = split(/,/,$users);

Any chance of a different regexp separator or a space to make /,/,
something more readable?

+        my ($prefs) = FetchSQLData();

Why those parenthesis?

+        # Write the user's preferences into a Perl record indexed by 
+        # preference name.  We pass the value "255" to the split function 

Perl "record"... "Hash"?


The logic seems fairly sane to me and r=jouni is close, but the
new-bug-problem with the bug 122900 patch will cause a change in this (I
think?), so I want to test the logic again after the changes.


Jouni

Comment 23

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v7: review fixes

I knew there was a reason why I included the patch for bug 128469 in this
patch.	Here's a version that contains the fix for the problem where users
still receive email about newly created Unconfirmed bugs.  The fix is in the
middle of the code changed by bug 128469, so the patch for that bug is included
in this patch.

This patch also contains fixes for the style and comment nits.

Comment 24

[Jouni Heikniemi]

Comment on attachment 87204 [details] [diff] [review]
patch v7: review fixes

>+        if (grep("Unconfirmed", @$reasons)

Make that /Unconfirmed/ so you won't treat all mail as being unco :-)

After all the work that has been done with these two patches, I'm ready to say
that we should put this into use and fix possible minor bugs if they occur. The
testing results from the other bug (128469) support the conclusion that this
code works fine in most cases. Thus, fix the problem above and you've got
r=jouni.

Comment 25

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v8: review fixes

Comment 26

[Jouni Heikniemi]

Comment on attachment 87615 [details] [diff] [review]
patch v8: review fixes

r=jouni

Comment 27

[Jouni Heikniemi]

Comment on attachment 87615 [details] [diff] [review]
patch v8: review fixes

Drat, drat, drat. Withdrawing my review and marking needs-work.

>+        # If the user prefers to be excluded from receiving mail 
>+        # about this change, filter them from the list.
>+        foreach my $reason (@$reasons) {
>+            my $pref = "email$role$reason";
>+            if (exists($prefs{$pref}) && $prefs{$pref} eq '') {
>+                push(@excludedAddresses, $user);
>+                next USER;

This section has a serious logical problem. If I have disabled cc related mail
and someone goes and changes both cc and status (for example), I'm not getting
mailed if CC is before status in the reasons list. The logic needs inversion:
mail me only if at least one of the reason/role combos is of interest to me.

This patch is checked in on bmo, isn't it? That would certainly explain some
odd mail results I've seen when going through the old patches (where I always
cc myself and add a comment). 

How could I miss this earlier? :-o

Comment 28

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v9: inverts the logic so filters work properly on multiple changes

Indeed, Jouni is right; the logic is inverted.	D'oh.  This is on b.m.o and
would have caused some folks not to get email about certain changes.  Here's
the fix, which I've applied to b.m.o.  Test script coming up.

Comment 29

[Myk Melez [:myk] [@mykmelez]]

Attachment: test script for patch v9

Comment 30

[Jouni Heikniemi]

Comment on attachment 87686 [details] [diff] [review]
patch v9: inverts the logic so filters work properly on multiple changes

Ok, the code looks sane now and it seems to work, too. At least until we
discover another issue with it :-) 

r=jouni

Comment 31

[Jouni Heikniemi]

Comment on attachment 87686 [details] [diff] [review]
patch v9: inverts the logic so filters work properly on multiple changes

All right. Since we've got the new review policy blah blah and afranke says
he's cool with the stuff in bug 128469, let's get this in.

r=jouni for the second time. Sold!

Comment 32

[Myk Melez [:myk] [@mykmelez]]

Checking in processmail;
/cvsroot/mozilla/webtools/bugzilla/processmail,v  <--  processmail
new revision: 1.83; previous revision: 1.82
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.37; previous revision: 1.36
done
Checking in template/en/default/account/prefs/email.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/email.html.tmpl,v
 <--  email.html.tmpl
new revision: 1.4; previous revision: 1.3
done