Bug 1883428 - Improve the needinfo email situation

Bugzilla/Constants.pm

@@ -89,6 +89,7 @@ use Memoize;
 
   RELATIONSHIPS
   REL_ASSIGNEE REL_QA REL_REPORTER REL_CC REL_GLOBAL_WATCHER
+  REL_FLAG_REQUESTEE REL_FLAG_REQUESTER REL_FLAG_TYPE_CC
   REL_ANY
 
   POS_EVENTS
@@ -355,14 +356,21 @@ use constant REL_CC       => 3;
 # REL 4 was REL_VOTER, before it was moved ino an extension.
 use constant REL_GLOBAL_WATCHER => 5;
 
+# Bug 1883428: recipients added because of a flag change, independently of
+# any other role they may hold on the bug.
+use constant REL_FLAG_REQUESTEE => 6;    # A flag was requested of them
+use constant REL_FLAG_REQUESTER => 7;    # Their flag request was granted/denied
+use constant REL_FLAG_TYPE_CC   => 8;    # On the flag type's admin-configured cc_list
+
 # We need these strings for the X-Bugzilla-Reasons header
 # Note: this hash uses "," rather than "=>" to avoid auto-quoting of the LHS.
 # This should be accessed through Bugzilla::BugMail::relationships() instead
 # of being accessed directly.
 use constant RELATIONSHIPS => {
-  REL_ASSIGNEE,       "AssignedTo", REL_REPORTER, "Reporter",
-  REL_QA,             "QAcontact",  REL_CC,       "CC",
-  REL_GLOBAL_WATCHER, "GlobalWatcher"
+  REL_ASSIGNEE,       "AssignedTo",       REL_REPORTER,       "Reporter",
+  REL_QA,              "QAcontact",       REL_CC,             "CC",
+  REL_GLOBAL_WATCHER,  "GlobalWatcher",   REL_FLAG_REQUESTEE, "FlagRequestee",
+  REL_FLAG_REQUESTER,  "FlagRequester",   REL_FLAG_TYPE_CC,   "FlagTypeCC"
 };
 
 # Used for global events like EVT_FLAG_REQUESTED

Bugzilla/Flag.pm

@@ -48,7 +48,6 @@ use Bugzilla::Hook;
 use Bugzilla::User;
 use Bugzilla::Util;
 use Bugzilla::Error;
-use Bugzilla::Mailer;
 use Bugzilla::Constants;
 use Bugzilla::Field;
 
@@ -69,8 +68,6 @@ use constant AUDIT_REMOVES => 0;
 
 use constant SKIP_REQUESTEE_ON_ERROR => 1;
 
-our $disable_flagmail = 0;
-
 sub DB_COLUMNS {
   my $dbh = Bugzilla->dbh;
   return qw(
@@ -580,20 +577,15 @@ sub update_flags {
       $new_flag->{id}                = $flag->id;
       $new_flag->{creation_date}     = format_time($timestamp, '%Y-%m-%d %H:%i:%s');
       $new_flag->{modification_date} = format_time($timestamp, '%Y-%m-%d %H:%i:%s');
-      $class->notify($new_flag, undef, $self, $timestamp);
     }
     else {
-      my $changes = $new_flag->update($timestamp);
-      if (scalar(keys %$changes)) {
-        $class->notify($new_flag, $old_flags{$new_flag->id}, $self, $timestamp);
-      }
+      $new_flag->update($timestamp);
       delete $old_flags{$new_flag->id};
     }
   }
 
   # These flags have been deleted.
   foreach my $old_flag (values %old_flags) {
-    $class->notify(undef, $old_flag, $self, $timestamp);
 
     # BMO - provide a hook which passes the timestamp,
     # because that isn't passed to remove_from_db().
@@ -714,7 +706,6 @@ sub force_retarget {
     else {
       # Track deleted attachment flags.
       push(@removed, $class->snapshot([$flag])) if $flag->attach_id;
-      $class->notify(undef, $flag, $bug || $flag->bug);
 
       # BMO - provide a hook which passes the timestamp,
       # because that isn't passed to remove_from_db().
@@ -1066,151 +1057,6 @@ sub extract_flags_from_cgi {
   return (\@flags, \@new_flags);
 }
 
-=pod
-
-=over
-
-=item C<notify($flag, $old_flag, $object, $timestamp)>
-
-Sends an email notification about a flag being created, fulfilled
-or deleted.
-
-=back
-
-=cut
-
-sub notify {
-  my ($class, $flag, $old_flag, $obj, $timestamp) = @_;
-
-  if ($disable_flagmail) {
-    return;
-  }
-
-  my ($bug, $attachment);
-  if (blessed($obj) && $obj->isa('Bugzilla::Attachment')) {
-    $attachment = $obj;
-    $bug        = $attachment->bug;
-  }
-  elsif (blessed($obj) && $obj->isa('Bugzilla::Bug')) {
-    $bug = $obj;
-  }
-  else {
-    # Not a good time to throw an error.
-    return;
-  }
-
-  my $addressee;
-
-  # If the flag is set to '?', maybe the requestee wants a notification.
-  if ( $flag
-    && $flag->requestee_id
-    && (!$old_flag || ($old_flag->requestee_id || 0) != $flag->requestee_id))
-  {
-    if ($flag->requestee->wants_mail([EVT_FLAG_REQUESTED])) {
-      $addressee = $flag->requestee;
-    }
-  }
-  elsif ($old_flag
-    && $old_flag->status eq '?'
-    && (!$flag || $flag->status ne '?'))
-  {
-    if ($old_flag->setter->wants_mail([EVT_REQUESTED_FLAG])) {
-      $addressee = $old_flag->setter;
-    }
-  }
-
-  my $cc_list = $flag ? $flag->type->cc_list : $old_flag->type->cc_list;
-  $cc_list //= '';
-
-  # Is there someone to notify?
-  return unless ($addressee || $cc_list);
-
-  # The email client will display the Date: header in the desired timezone,
-  # so we can always use UTC here.
-  $timestamp ||= Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
-  $timestamp = format_time($timestamp, '%a, %d %b %Y %T %z', 'UTC');
-
-  # If the target bug is restricted to one or more groups, then we need
-  # to make sure we don't send email about it to unauthorized users
-  # on the request type's CC: list, so we have to trawl the list for users
-  # not in those groups or email addresses that don't have an account.
-  my @bug_in_groups = grep { $_->{'ison'} || $_->{'mandatory'} } @{$bug->groups};
-  my $attachment_is_private = $attachment ? $attachment->isprivate : undef;
-
-  my %recipients;
-  foreach my $cc (split(/[, ]+/, $cc_list)) {
-    my $ccuser = new Bugzilla::User({name => $cc, cache => 1});
-    next
-      if (scalar(@bug_in_groups)
-      && (!$ccuser || !$ccuser->can_see_bug($bug->bug_id)));
-    next if $attachment_is_private && (!$ccuser || !$ccuser->is_insider);
-
-    # Prevent duplicated entries due to case sensitivity.
-    $cc = $ccuser ? $ccuser->email : $cc;
-    $recipients{$cc} = $ccuser;
-  }
-
-  # Only notify if the addressee is allowed to receive the email
-  # and can see the bug (prevents short_desc leaking via Subject/body).
-  if (
-       $addressee
-    && $addressee->email_enabled
-    && ( (!scalar(@bug_in_groups) || $addressee->can_see_bug($bug->bug_id))
-      && (!$attachment_is_private || $addressee->is_insider))
-    )
-  {
-    $recipients{$addressee->email} = $addressee;
-  }
-
-  return unless keys %recipients;
-
-  # Process and send notification for each recipient.
-  # If there are users in the CC list who don't have an account,
-  # use the default language for email notifications.
-  my $default_lang;
-  if (grep { !$_ } values %recipients) {
-    $default_lang = Bugzilla::User->new()->setting('lang');
-  }
-
-  # Get comments on the bug
-  my $all_comments = $bug->comments({after => $bug->lastdiffed});
-  @$all_comments = grep { $_->type || $_->body =~ /\S/ } @$all_comments;
-
-  # Get public only comments
-  my $public_comments = [grep { !$_->is_private } @$all_comments];
-
-  foreach my $to (keys %recipients) {
-
-    # Add threadingmarker to allow flag notification emails to be the
-    # threaded similar to normal bug change emails.
-    my $thread_user_id = $recipients{$to} ? $recipients{$to}->id : 0;
-
-    # We only want to show private comments to users in the is_insider group
-    my $comments = $recipients{$to}
-      && $recipients{$to}->is_insider ? $all_comments : $public_comments;
-
-    my $vars = {
-      flag            => $flag,
-      old_flag        => $old_flag,
-      to              => $to,
-      date            => $timestamp,
-      bug             => $bug,
-      attachment      => $attachment,
-      threadingmarker => build_thread_marker($bug->id, $thread_user_id),
-      new_comments    => $comments,
-    };
-
-    my $lang = $recipients{$to} ? $recipients{$to}->setting('lang') : $default_lang;
-
-    my $template = Bugzilla->template_inner($lang);
-    my $message;
-    $template->process("request/email.txt.tmpl", $vars, \$message)
-      || ThrowTemplateError($template->error());
-
-    MessageToMTA($message);
-  }
-}
-
 # This is an internal function used by $bug->flag_types
 # and $attachment->flag_types to collect data about available
 # flag types and existing flags set on them. You should never

Bugzilla/FlagType.pm

@@ -155,6 +155,10 @@ sub update {
 
   # Clear existing flags for bugs/attachments in categories no longer on
   # the list of inclusions or that have been added to the list of exclusions.
+  # force_retarget() doesn't touch the bug row, so nothing else sends bugmail
+  # for the flags it clears here; remember the affected bugs and send it once
+  # the transaction has committed (bug 1883428).
+  my %retargeted_bug_ids;
   my $flag_ids = $dbh->selectcol_arrayref(
     'SELECT DISTINCT flags.id
                                                FROM flags
@@ -170,6 +174,7 @@ sub update {
                                                 AND i.type_id IS NULL', undef,
     $self->id
   );
+  $retargeted_bug_ids{$_} = 1 foreach @{_bug_ids_for_flags($flag_ids)};
   Bugzilla::Flag->force_retarget($flag_ids);
 
   $flag_ids = $dbh->selectcol_arrayref(
@@ -186,6 +191,7 @@ sub update {
                                                   OR e.component_id IS NULL)',
     undef, $self->id
   );
+  $retargeted_bug_ids{$_} = 1 foreach @{_bug_ids_for_flags($flag_ids)};
   Bugzilla::Flag->force_retarget($flag_ids);
 
   # Silently remove requestees from flags which are no longer
@@ -209,9 +215,32 @@ sub update {
     {type => $self, changed => $changes});
 
   $dbh->bz_commit_transaction();
+
+  # An inclusion/exclusion edit can touch many bugs, so queue the bugmail
+  # rather than run a full Send() per bug inside this request.
+  my @bug_ids = sort { $a <=> $b } keys %retargeted_bug_ids;
+  if (@bug_ids && Bugzilla->get_param_with_override('use_mailer_queue')) {
+    Bugzilla->job_queue->insert('bug_mail_send',
+      {bug_ids => \@bug_ids, changer_id => Bugzilla->user->id});
+  }
+  elsif (@bug_ids) {
+
+    # Bugzilla::BugMail uses this module, so load it lazily.
+    require Bugzilla::BugMail;
+    Bugzilla::BugMail::Send($_, {changer => Bugzilla->user}) foreach @bug_ids;
+  }
+
   return $changes;
 }
 
+sub _bug_ids_for_flags {
+  my ($flag_ids) = @_;
+  return [] if !@$flag_ids;
+  my $dbh = Bugzilla->dbh;
+  return $dbh->selectcol_arrayref(
+    'SELECT DISTINCT bug_id FROM flags WHERE ' . $dbh->sql_in('id', $flag_ids));
+}
+
 ###############################
 ####      Accessors      ######
 ###############################

Bugzilla/Job/BugMailSend.pm

@@ -0,0 +1,34 @@
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+
+package Bugzilla::Job::BugMailSend;
+
+use 5.10.1;
+use strict;
+use warnings;
+
+use Bugzilla::Bug;
+use Bugzilla::BugMail;
+use Bugzilla::User;
+use parent qw(Bugzilla::Job::Mailer);
+
+# Runs BugMail::Send for a list of bugs, for callers that touch too many
+# bugs to do it inside the web request (e.g. a flag type inclusion/exclusion
+# edit clearing flags across many bugs). A retry after a partial failure
+# doesn't re-mail bugs already done: Send advances lastdiffed, so their
+# window is empty the second time.
+sub process_job {
+  my ($class, $arg) = @_;
+  my $changer = Bugzilla::User->new($arg->{changer_id});
+  Bugzilla->set_user($changer);
+
+  # new_from_list skips bugs deleted since the job was queued.
+  Bugzilla::BugMail::Send($_->id, {changer => $changer})
+    foreach @{Bugzilla::Bug->new_from_list($arg->{bug_ids})};
+}
+
+1;

Bugzilla/JobQueue.pm

@@ -28,9 +28,10 @@ use Carp qw(longmess);
 # This maps job names for Bugzilla::JobQueue to the appropriate modules.
 # If you add new types of jobs, you should add a mapping here.
 use constant JOB_MAP => {
-  send_mail => 'Bugzilla::Job::Mailer',
-  bug_mail  => 'Bugzilla::Job::BugMail',
-  run_task  => 'Bugzilla::Job::RunTask',
+  send_mail     => 'Bugzilla::Job::Mailer',
+  bug_mail      => 'Bugzilla::Job::BugMail',
+  bug_mail_send => 'Bugzilla::Job::BugMailSend',
+  run_task      => 'Bugzilla::Job::RunTask',
 };
 
 # Without a driver cache TheSchwartz opens a new database connection

Bugzilla/User.pm

@@ -2680,6 +2680,14 @@ sub create {
   require Bugzilla::BugMail;
   my %relationships = Bugzilla::BugMail::relationships();
   foreach my $rel (keys %relationships) {
+
+    # Flag relationships bypass wants_bug_mail() entirely (bug 1883428),
+    # so email_setting rows for them are never consulted.
+    next
+      if $rel == REL_FLAG_REQUESTEE
+      || $rel == REL_FLAG_REQUESTER
+      || $rel == REL_FLAG_TYPE_CC;
+
     foreach my $event (POS_EVENTS, NEG_EVENTS) {
 
       # These "exceptions" define the default email preferences.

extensions/BMO/template/en/default/email/bugmail.html.tmpl

@@ -16,6 +16,37 @@
 </head>
 <body style="font-family: sans-serif">
 
+  [% IF flag_events.size %]
+    <div id="flag_events">
+      <ul>
+      [% FOREACH event = flag_events %]
+        <li>
+        [% IF event.action == 'requested' %]
+          [% INCLUDE global/user.html.tmpl user = to_user, who = event.setter %]
+          has requested [% event.type.name FILTER html %][% " for attachment ${event.attachment_id}" FILTER none IF event.attachment_id %]
+          from you on this [% terms.bug %].
+          [% IF event.attachment && event.attachment.external_redirect %]
+            [% external = event.attachment.external_redirect %]
+            <br>
+            <a href="[% event.attachment.data FILTER html %]">[% external.title FILTER html %]</a>
+          [% END %]
+        [% ELSIF event.action == 'answered' %]
+          [% status_word = event.status == '+' ? 'granted' : event.status == '-' ? 'denied' : 'cleared' %]
+          Your request for [% event.type.name FILTER html %][% " on attachment ${event.attachment_id}" FILTER none IF event.attachment_id %]
+          has been [% status_word FILTER html %] by
+          [% INCLUDE global/user.html.tmpl user = to_user, who = event.setter %].
+        [% ELSE %]
+          [% INCLUDE global/user.html.tmpl user = to_user, who = event.setter %]
+          set [% event.type.name FILTER html %][% event.status FILTER html IF event.status != 'X' %][% " for attachment ${event.attachment_id}" FILTER none IF event.attachment_id %][% " (cleared)" IF event.status == 'X' %].
+        [% END %]
+        [% Hook.process('flag_event', 'email/bugmail.html.tmpl') %]
+        </li>
+      [% END %]
+      </ul>
+    </div>
+    <hr style="border: 1px dashed #969696">
+  [% END %]
+
   [% IF !to_user.in_group('editbugs') %]
     <div id="noreply" style="font-size: 90%; color: #666666">
       Do not reply to this email. You can add comments to this [% terms.bug %] at