Bug 457357 - Add additional param to Bugzilla::BugMail::Send() to temporarily not send email for the current change
Add additional param to Bugzilla::BugMail::Send() to temporarily not send ema...
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: Email Notifications (Show other bugs)
3.2
All Linux
medium Severity medium (vote)
: ---
: ---
Assigned To: Noura El hawary
: FutureFeature
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-31 00:34 EDT by David Lawrence
Modified: 2013-06-24 00:10 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Enhancement
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-12-05 14:50:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
v1 for adding nomail param to BugMail.pm to force suppressing email notifications (2.90 KB, patch)
2008-12-03 23:40 EST, Noura El hawary
dkl: review-
Details | Diff
v2 for adding nomail flag to webservice functions (11.53 KB, patch)
2008-12-04 23:57 EST, Noura El hawary
dkl: review+
Details | Diff

  None (edit)
Description David Lawrence 2008-07-31 00:34:18 EDT
When we run xmlrpc scripts to perform mass changes, it is sometimes desirable to
make the change but to not actually send the email notification if it is a small
change. 

A new param should be added to Bugzilla::BugMail::Send() to disable email being
sent but to perform all other functions such as updating lastdiffed proper, etc.

For example around line 515 we could add $nomail to the if condition that would
either call sendMail or skip it.

            # Make sure the user isn't in the nomail list, and the insider and 
            # dep checks passed.
            if ($user->email_enabled &&
                $insider_ok &&
                $dep_ok &&
                !$nomail)
            {
                # OK, OK, if we must. Email the user.
                $sent_mail = sendMail($user,
                                      \@headerlist,
                                      \%rels_which_want,
                                      \%values,
                                      \%defmailhead,
                                      \%fielddescription,
                                      \@diffparts,
                                      $comments{$lang},
                                      $anyprivate,
                                      ! $start,
                                      $id,
                                      exists $watching{$user_id} ?
                                             $watching{$user_id} : undef);
            }
Comment 1 Noura El hawary 2008-12-03 23:40:54 EST
Created attachment 325637 [details]
v1 for adding nomail param to BugMail.pm to force suppressing email notifications

Hi Dave,

The attached patch applies the nomail param to BugMail Send function to prevent sending mail in a more of a global way. basically i added the nomail argument to forced hash the gets passed to Send function, wasn't sure if i should make it an argument on its own or incorporate it into the forced hash, then i though that is what hashes are for anyways ;),, what do you think?

Noura
Comment 2 David Lawrence 2008-12-04 13:43:16 EST
Comment on attachment 325637 [details]
v1 for adding nomail param to BugMail.pm to force suppressing email notifications

>Index: Bugzilla/BugMail.pm
>===================================================================
>RCS file: /cvs/qa/rh_bugzilla_3/Bugzilla/BugMail.pm,v
>retrieving revision 1.24
>diff -p -u -r1.24 BugMail.pm
>--- Bugzilla/BugMail.pm	10 Nov 2008 07:35:02 -0000	1.24
>+++ Bugzilla/BugMail.pm	4 Dec 2008 04:26:17 -0000
>@@ -116,7 +116,8 @@ sub three_columns {
> # args: bug_id, and an optional hash ref which may have keys for:
> # changer, owner, qa, reporter, cc
> # Optional hash contains values of people which will be forced to those
>-# roles when the email is sent.
>+# roles when the email is sent,  also contains boolean value nomail
>+# to indicate weather email should be forced to be suppressed or not.
> # All the names are email addresses, not userids
> # values are scalars, except for cc, which is a list
> # This hash usually comes from the "mailrecipients" var in a template call.
>@@ -524,9 +525,11 @@ sub Send {
> 
>             # Make sure the user isn't in the nomail list, and the insider and 
>             # dep checks passed.

Nit-pick: Add text here to also describe the nomail check.

>+            $forced->{nomail} ||= 0;
>             if ($user->email_enabled &&
>                 $insider_ok &&
>-                $dep_ok)
>+                $dep_ok &&
>+                !$forced->{nomail})
>             {
>                 # OK, OK, if we must. Email the user.
>                 $sent_mail = sendMail($user, 
>Index: Bugzilla/WebService/Bug.pm
>@@ -337,7 +337,8 @@ sub add_comment {
>     $bug->update();
>     
>     # Send mail.
>-    Bugzilla::BugMail::Send($bug->bug_id, { changer => Bugzilla->user->login });
>+    $params->{nomail} ||= 0;
>+    Bugzilla::BugMail::Send($bug->bug_id, { changer => Bugzilla->user->login, nomail => $params->{nomail} });
>     return undef;
> }

Should we be returning more than undef here? Not sure why upstream us doing this here and returning more meaningful data
in other places.

I would instead do:

      my $mailresults = Bugzilla::BugMail::Send($bug->bug_id, { changer => Bugzilla->user->login, 
                                                                nomail  => $params->{nomail} });
      return { id => $bug->bug_id, $mail_results };

or even:
    
      return [ $bug->bug_id, $mail_results ];

whichever is fine. But at least the caller would know which bug the comment was successfully added to especially if doing
system.multicall() with multiple Bug.add_comment() calls. You can leave this alone if you want and we can address it later
in a different bug upstream or change it now. Up to you.

>@@ -883,10 +884,8 @@ sub update {
>         $changes{$bug->bug_id} = [$bug_changes];
> 
>         $params->{nomail} ||= 0;
>-        if ( not $params->{nomail} ) {
>-            my $mail_results = Bugzilla::BugMail::Send($bug->bug_id, { changer => Bugzilla->user->login });
>-            push (@{$changes{$bug->bug_id}}, $mail_results);
>-        }
>+        my $mail_results = Bugzilla::BugMail::Send($bug->bug_id, { changer => Bugzilla->user->login, nomail => $params->{nomail} });
>+        push (@{$changes{$bug->bug_id}}, $mail_results);
>         $dbh->bz_commit_transaction();
>     }
> 
>@@ -1330,6 +1329,9 @@ it is assumed to be public.
> on the bug. If you are not in the time tracking group, this value will
> be ignored.
> 
>+=item C<nomail> (boolean) B<Optional> - A boolean value to indicate if mail
>+with the new comment should be sent to the relevant users. If set to 1 then
>+mail will be suppressed, and if ignored or set to 0 then mail will be sent.
> 
> =back
> 

Patch looks good otherwise. I would also update all of the other calls in extensions/compat_xmlrpc/code/webservice.pl
to use the new format as well and add a new patch which I will review+.

Dave
Comment 3 Noura El hawary 2008-12-04 23:57:42 EST
Created attachment 325797 [details]
v2 for adding nomail flag to webservice functions 

Thanks for the review Dave, I agree with you on that we should always return something from add_comment function, so i have change that and did other modifications as you have suggested.

Noura
Comment 4 David Lawrence 2008-12-05 14:50:39 EST
Comment on attachment 325797 [details]
v2 for adding nomail flag to webservice functions 

Looks good Noura. I went ahead and committed to CVS so I could push it out with this update.

Dave

Note You need to log in before you can comment on or make changes to this bug.