277370 - Ability to specify an email address to which notification about all bugs should go

Status: RESOLVED FIXED

(Bugzilla :: Email Notifications, enhancement)

Description

[Rajat Mishra]

In Bugzilla user can not specify a mail id to which notification about all 
logged problems will go.

Comment 1

[Marc Schumann [:Wurblzap]]

Bug 38922 does that. In a more detailed way, though (component-wise), so I don't
mark this as a duplicate of it.

Comment 2

[Frédéric Buclin]

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

Comment 3

[guillomovitch]

Attachment: Patch against CVS head

The following patch add this features.

It should be noted that then adress given there is not a user account, but a
real email-adress. As a consequence, there will be no verification such as "is
this user allowed to watch this bug", etc...

Comment 4

[Vlad Dascalu]

Comment on attachment 177608 [details] [diff] [review]
Patch against CVS head

Your patch contains TABs. Bugzilla uses spaces instead. Commiting your patch in
the current form will break the tinderbox tests (you can run runtests.pl from
the Bugzilla directory to make sure that the testing suite will pass).

Comment 5

[guillomovitch]

Attachment: Patch against CVS head with spaces instead of tabs

Ooops, sorry.

Comment 6

[Frédéric Buclin]

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

Comment 7

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 8

[Frédéric Buclin]

Comment on attachment 177647 [details] [diff] [review]
Patch against CVS head with spaces instead of tabs

BugMail.pm has changed a lot. Please update your patch and ask [e-mail]
for review.

Comment 9

[guillomovitch]

Attachment: Patch against current CVS head

Here is an updated patch

Comment 10

[Gervase Markham [:gerv]]

Comment on attachment 180873 [details] [diff] [review]
Patch against current CVS head

I'm afraid I don't think this is the right thing to do at all :-(

We shouldn't be doing end-runs around Bugzilla's user model like this. And,
given that this feature could be implemented with a simple 

push(@ccs, Param('allemail'))

in the right place, the current code implementation is over-complex.

guillomovitch: thanks for your patch, and I hope you find it very useful, but
it's not right for the Bugzilla codebase as it is.

Gerv

Comment 11

[guillomovitch]

(In reply to comment #10)
> (From update of attachment 180873 [details] [diff] [review] [edit])
> I'm afraid I don't think this is the right thing to do at all :-(
> 
> We shouldn't be doing end-runs around Bugzilla's user model like this.
I also realized it later when rewriting the patch that using a disabled user
account would probably better.

> And,
> given that this feature could be implemented with a simple 
> 
> push(@ccs, Param('allemail'))
> 
> in the right place, the current code implementation is over-complex.
Agreed.

> guillomovitch: thanks for your patch, and I hope you find it very useful, but
> it's not right for the Bugzilla codebase as it is.
OK, I'll provide a clear rewrite then.

Comment 12

[guillomovitch]

Attachment: Cleaner reimplementation

This reimplementation uses a bugzilla user.

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 184436 [details] [diff] [review]
Cleaner reimplementation

I apologise; I didn't intend my short comment to be interpreted as a spec!

>+   name => 'globalwatcher',

"globalwatchers". It should be a list. For bonus points, have a validation
function which checks they are all existing Bugzilla users.

>+   desc => 'The email address of a person who should receive all notification' .
>+           ' mails about opended bugs.',

"A comma-separated list of users who should receive a copy of every
notification mail the system sends."

>Index: Bugzilla/BugMail.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v
>retrieving revision 1.39
>diff -a -u -r1.39 BugMail.pm
>--- Bugzilla/BugMail.pm	4 Apr 2005 21:09:17 -0000	1.39
>+++ Bugzilla/BugMail.pm	24 May 2005 21:55:33 -0000
>@@ -49,7 +49,8 @@
>                  REL_REPORTER          , "Reporter",
>                  REL_QA                , "QAcontact",
>                  REL_CC                , "CC",
>-                 REL_VOTER             , "Voter");
>+                 REL_VOTER             , "Voter",
>+                 REL_WATCHER           , "GlobalWhatcher");

REL_GLOBAL_WATCHER. We may acquire other sorts of watcher later.

Also, "GlobalWatcher".

>+    # Global watcher
>+    my $watcher = Param("globalwatcher");

Split on commas and push each entry.

>+            This allows to define a specific user that will
>+            receive notification each time a new bug in entered, or when
>+            an existing bug changes, according to the normal groupset
>+            permissions. It may be useful for sending notifications to a
>+            mailing-list, for instance.

"This allows you to define a list of users who will receive a copy
of all bug change emails sent by the system (according to the normal groupset
permissions)."

Gerv

Comment 14

[guillomovitch]

Attachment: Corrected patch, with additional checking function

Comment 15

[Gervase Markham [:gerv]]

Comment on attachment 184754 [details] [diff] [review]
Corrected patch, with additional checking function

There's two things wrong here. The first is that you haven't updated all
references to REL_WATCHER. The second is that the docs part of the patch looks
wrong - as if you've modified a different para instead.

Gerv

Comment 16

[guillomovitch]

Attachment: fixed patch

Comment 17

[Max Kanat-Alexander]

Comment on attachment 184823 [details] [diff] [review]
fixed patch

>+                 REL_GLOBAL_WATCHER    , "GlobalWhatcher");

  Nit: Spelling ('GlobalWatcher')

  Everything else looks pretty good to me, but I'll let Gerv do the final r+; I
haven't tested this code or anything.

Comment 18

[guillomovitch]

So?

Comment 19

[Olav Vitters]

Comment on attachment 184823 [details] [diff] [review]
fixed patch

>+    # Global watcher
>+    my $watchers = Param("globalwatchers");
>+    if ($watchers) {
>+        foreach my $watcher (split(/[\s,]+/, $watchers));
>+            push(@{$recipients{$watcher}, REL_GLOBAL_WATCHER);
>+        }
>+    }
>+

Maybe I'm missing something, but shouldn't this push a userid?

Comment 20

[guillomovitch]

>Maybe I'm missing something, but shouldn't this push a userid?
As far as I'm understanding the code, userid are the keys of %recipients hash,
and values are the list of reason why they receive mail.

Comment 21

[guillomovitch]

Attachment: Corrected patch

>Nit: Spelling ('GlobalWatcher')
Fixed.

Comment 22

[Olav Vitters]

(In reply to comment #20)
> >Maybe I'm missing something, but shouldn't this push a userid?
> As far as I'm understanding the code, userid are the keys of %recipients hash,
> and values are the list of reason why they receive mail.

Sorry, didn't explain properly. I believe Param("globalwatchers") contains email
addresses. In check_globalwatchers you also split on "," and check with
login_to_id if the email addresses are valid.

Now later in BugMail.pm, you again split on ",", and have:
  push(@{$recipients{$watcher}, REL_GLOBAL_WATCHER);
Wouldn't $watcher be an email address instead of an userid?

mkanat said this was basically ok, so reassigning + confirming.

Comment 23

[guillomovitch]

Attachment: Corrected patch

>Wouldn't $watcher be an email address instead of an userid?
You're perfectly right.

Morevoer, I had a typo in my patch I just corrected.

Comment 24

[Zain Memon]

Attachment 190159 [details] [diff] is broken. Patch goes through clean, checksetup.pl complains
that the param user_verify_class is not found at Config.pm on line 151. 

mkanat suggests the problem is that you can't use Bugzilla::User in defparams.pl. 

Comment 25

[Teemu Mannermaa (:wicked)]

Comment on attachment 190152 [details] [diff] [review]
Corrected patch

This patch looks to be obsoleted by a newer patch. Clearing review request and
marking as such.

Comment 26

[Teemu Mannermaa (:wicked)]

Comment on attachment 190159 [details] [diff] [review]
Corrected patch

Checksetup.pl worked for me but runtests.pl fails (probably because of what
Zain said):

Failed Test	Stat Wstat Total Fail  Failed  List of Failed
-------------------------------------------------------------------------------
t/001compile.t	   4  1024    99    4	4.04%  4-5 66 90
t/007util.t	 255 65280    14   14 100.00%  1-14
t/009bugwords.t  255 65280    ??   ??	    %  ??

These seem to be because of the error:
Couldn't parse defparams.pl: "trick_taint" is not exported by the
Bugzilla::Util module

(This was not a full review.)

Comment 27

[guillomovitch]

Attachment: Corrected patch

The problems seems to comes from circular dependencies between perl modules. As
a solution, I removed the loading of Bugzilla::Config from Bugzilla::Utils,
which seems to be a compilation of utility function without any need from
configuration value.

Comment 28

[Frédéric Buclin]

Comment on attachment 196245 [details] [diff] [review]
Corrected patch

>Index: Bugzilla/Util.pm

>-use Bugzilla::Config;
> use Bugzilla::Constants;

Util.pm uses Config::Param(). You cannot remove this dependency.

Comment 29

[guillomovitch]

Attachment: Updated patch

Parameter checking can't be done as other parameters from Bugzilla::Config::Common, as it introduces a dependency loop. As a workaround, I implemented the check in editparams.cgi

Comment 30

[Vlad Dascalu]

Comment on attachment 202029 [details] [diff] [review]
Updated patch

You are not validating the email addresses here:

+        foreach (
+            map { login_to_id($_) }
+            split(/[,\s]+/, $watchers)
+        ) {
+            push(@{$recipients{$_}}, REL_GLOBAL_WATCHER);
+        }

More explicitely, if the global watcher changes his email address, map (login_to_id($_)) could return 0, and you would end up pushing the zero value in the array. (in the code pasted above)

But even more important, when you review again the params, and say change another parameter to another valid value, the globalwatchers parameter could fail the validation, even if you haven't touched it. This would be similar to data corruption, since you're not keeping the parameter valid all the time (you're just checking for validation when you submit editparams, but validation status could change without changing the param value).

Comment 31

[timeless]

why aren't you storing numbers in params instead of email addresses?

then when a user's account changes, you don't care.

that said, is this really a param or a permission?

Comment 32

[guillomovitch]

(In reply to comment #30)
> More explicitely, if the global watcher changes his email address, map
> (login_to_id($_)) could return 0, and you would end up pushing the zero value
> in the array. (in the code pasted above)
I wasn't aware you could change your login :/ I could eventually filter out null values.

> But even more important, when you review again the params, and say change
> another parameter to another valid value, the globalwatchers parameter could
> fail the validation, even if you haven't touched it. This would be similar to
> data corruption, since you're not keeping the parameter valid all the time
> (you're just checking for validation when you submit editparams, but validation
> status could change without changing the param value).
OK, it seems validation of the parameter is not possible at all:
- I can't validate this parameters as all other parameters, because of internal Bugzilla limitations (no way to use Bugzilla::User from Bugzilla::Config::Common)
- Param validation from editparams.cgi is not enough

Should I submit this patch without any parameter checking, as it is the only problem ?

Comment 33

[guillomovitch]

(In reply to comment #31)
> why aren't you storing numbers in params instead of email addresses?
Because I prefer to have user-editable parameters, and asking bugzilla admin to enter a list of user ids would be really unfriendly. Unless there is some way to store numerical id, and present logins instead.

> then when a user's account changes, you don't care.
> 
> that said, is this really a param or a permission?
I initially submitted it as a parameter. Making it a permission for invidual user would be a different implementation. Morevoer, reviewing each existing user before sending any mail to check which are global watcher would be quite costly.

Comment 34

[guillomovitch]

Attachment: Updated patch

This updated patch filters out erroneous values resulting from email adress unvalid anymore.

I agree than turning this from a param into a user permission may solve the issue, but:
- mover is already implemented this way AFAIK
- current Bugzilla::Group implementation lacks a useful ->get_members() function

Comment 35

[Frédéric Buclin]

Comment on attachment 225450 [details] [diff] [review]
Updated patch

>Index: editparams.cgi

>+            } elsif ($name eq 'globalwatchers') {
>+                # can't check this as others, as Bugzilla::Config::Common
>+                # can not use Bugzilla::User
>+                foreach my $watcher (split(/[,\s]+/, $value)) {
>+                    ThrowUserError('invalid_parameter', { name => $name, err => "no such user $watcher" }) unless login_to_id($watcher);
>+                }
>             }

I don't like this check being done in editparams.cgi. It should go in Config::Common (yes, I'm aware of the dependency loops we are introducing). As Guillaume said in his previous comment, movers aren't validated. Maybe should we duplicate the login_to_id() code in Config::Common for now? And replace it by login_to_id() when we have fixed this dependency loop problem?

Comment 36

[Frédéric Buclin]

Comment on attachment 225450 [details] [diff] [review]
Updated patch

>Index: editparams.cgi

>+                foreach my $watcher (split(/[,\s]+/, $value)) {
>+                    ThrowUserError('invalid_parameter', { name => $name, err => "no such user $watcher" }) unless login_to_id($watcher);
>+                }

This line is too long. You should write:

    foreach my $watcher (split(/[,\s]+/, $value)) {
        next if login_to_id($watcher);
        ThrowUserError('invalid_parameter', { name => $name,
                                              err => "no such user $watcher" });
    }



>Index: Bugzilla/BugMail.pm

>+    # Global watcher
>+    my $watchers = Param("globalwatchers");
>+    if ($watchers) {
>+        foreach (
>+            grep { $_ }                # filter error values, as 
>+            map { login_to_id($_) }    # user may change login
>+            split(/[,\s]+/, $watchers)

Nit: This is a bit hard to read, see my comment below.


>+            push(@{$recipients{$_}}, REL_GLOBAL_WATCHER);

This is wrong (from what bkor told me on IRC). You should use the same notation as for other fields, e.g.:

    # Global watcher
    my @watchers = split(/[,\s]+/, Bugzilla->params->{'globalwatchers'});
    foreach (@watchers) {
        my $watcher_id = login_to_id($_);
        $recipients{$watcher_id}->{+REL_GLOBAL_WATCHER} = BIT_DIRECT if $watcher_id;
    }

Anyway, this is not enough to get notifications. I tested your patch, and users in 'globalwatchers' do not get any notification, probably because you didn't fix User::wants_bug_mail().

Comment 37

[Frédéric Buclin]

Guillaume, any chance to get an updated patch? We freeze for 3.0 in two weeks.

Comment 38

[guillomovitch]

Attachment: Corrected patch

This is a corrected (but untested patch).

I've 'fixed' User::want_mail by just bypassing database query when relationship is  GLOBAL_WATCHER. However, this is completly orthogonal to the new email preference system. I'm also not sure about the line 1275 in User::want_bug_mail, which is marked as temporarily: if the plan is to remove it, then an event will have to be generated for global watchers too, so as to avoid immediate return in User::want_mail.

Comment 39

[Olav Vitters]

Comment on attachment 242349 [details] [diff] [review]
Corrected patch

>Index: Bugzilla/BugMail.pm

>@@ -349,6 +350,14 @@
>     # Assignee
>     $recipients{$_}->{+REL_ASSIGNEE} = BIT_DIRECT foreach (@assignees);
> 
>+    # Global watcher
>+    my @watchers = split(/[,\s]+/, Bugzilla->params->{'globalwatchers'});
>+    foreach (@watchers) {
>+        my $watcher_id = login_to_id($_);
>+        next unless $watcher_id;
>+        $recipients{$watcher_id}->{+REL_GLOBAL_WATCHER} = BIT_DIRECT;
>+    }

We talked about this on #mozwebtools. Basically the global watcher should behave like a global watcher. Meaning you should *not* get a copy of every bugreport if you watch someone who is a global watcher. Exactly like when you currently watch someone. You get their email and not the persons they are watching as well.

Practially this means that above code should be moved below the following block:
  if (Bugzilla->params->{"supportwatchers"}) {

(located closely down to above code)

Code passes my (limited) testing, just change this, ask me for review and after that it should be r+.

Comment 40

[guillomovitch]

Attachment: Corrected patch

Here it is...

However, the more I think about it, the more I think it would be better to reimplement purely with mail preferences, avoiding the need for a global watcher parameter, and the need to check this parameter.

Comment 41

[Frédéric Buclin]

I think that's something we should really take for 3.0.

Comment 42

[Frédéric Buclin]

Comment on attachment 244006 [details] [diff] [review]
Corrected patch

vladd is away these days...

Comment 43

[Olav Vitters]

Comment on attachment 244006 [details] [diff] [review]
Corrected patch

>Index: Bugzilla/BugMail.pm
>     if (Bugzilla->params->{"supportwatchers"}) {
>+        # Global watcher
>+        my @watchers = split(/[,\s]+/, Bugzilla->params->{'globalwatchers'});
>+        foreach (@watchers) {
>+            my $watcher_id = login_to_id($_);
>+            next unless $watcher_id;
>+            $recipients{$watcher_id}->{+REL_GLOBAL_WATCHER} = BIT_DIRECT;
>+        }
>+

I meant move this code below the entire block; meaning after the '}' that closes the supportwatchers if statement.

Above can be fixed on checkin.

Passes my tests. r=bkor

Comment 44

[Olav Vitters]

Includes checkin fix.

Checking in editparams.cgi;
/cvsroot/mozilla/webtools/bugzilla/editparams.cgi,v  <--  editparams.cgi
new revision: 1.42; previous revision: 1.41
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.96; previous revision: 1.95
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.61; previous revision: 1.60
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.142; previous revision: 1.141
done
Checking in Bugzilla/Config/MTA.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/MTA.pm,v  <--  MTA.pm
new revision: 1.13; previous revision: 1.12
done
Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.62; previous revision: 1.61
done
Checking in template/en/default/admin/params/mta.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/params/mta.html.tmpl,v  <--  mta.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/email/newchangedmail.txt.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/email/newchangedmail.txt.tmpl,v  <--  newchangedmail.txt.tmpl
new revision: 1.5; previous revision: 1.4
done


(In reply to comment #40)
> However, the more I think about it, the more I think it would be better to
> reimplement purely with mail preferences, avoiding the need for a global
> watcher parameter, and the need to check this parameter.

I already thought about that. One day Bugzilla should support watching bugs using inclusion/exclusion lists (like flags has). Then people can just watch any product + any component and receive everything. At that time, this can go.

Comment 45

[Max Kanat-Alexander]

Added to the relnotes currently attached to bug 349423.