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

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

@@ -16,6 +16,39 @@
 </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' %]
+          [% IF event.type.name == 'needinfo' %]
+            [% INCLUDE global/user.html.tmpl user = to_user, who = event.setter %]
+            needs more information from you to work on this [% terms.bug %].
+          [% ELSE %]
+            [% 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 %].
+          [% END %]
+          [% 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 %].
+        [% 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

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

@@ -11,6 +11,24 @@
 
 [% isnew = bug.lastdiffed ? 0 : 1 %]
 
+[% FOREACH event = flag_events %]
+[% IF event.action == 'requested' %]
+[% IF event.type.name == 'needinfo' %]
+[%+ event.setter.identity %] needs more information from you to work on this [% terms.bug %].
+[% ELSE %]
+[%+ event.setter.identity %] has requested [% event.type.name %][% " for attachment ${event.attachment_id}" IF event.attachment_id %] from you on this [% terms.bug %].
+[% END %]
+[% IF event.attachment && event.attachment.external_redirect %]
+[% external = event.attachment.external_redirect %]
+[%+ external.title _ ": " _ event.attachment.data %]
+[% END %]
+[% ELSIF event.action == 'answered' %]
+[% status_word = event.status == '+' ? 'granted' : event.status == '-' ? 'denied' : 'cleared' %]
+Your request for [% event.type.name %][% " on attachment ${event.attachment_id}" IF event.attachment_id %] has been [% status_word %] by [% event.setter.identity %].
+[% END %]
+[%+ Hook.process('flag_event', 'email/bugmail.txt.tmpl') %]
+
+[% END %]
 [% IF !to_user.in_group('editbugs') %]
 Do not reply to this email. You can add comments to this [% terms.bug %] at
 [% END %]

extensions/Needinfo/template/en/default/hook/email/bugmail-flag_event.html.tmpl

@@ -0,0 +1,18 @@
+[%# 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.
+  #%]
+
+[% RETURN UNLESS event.action == 'requested' && event.type.name == 'needinfo' %]
+<br>
+[% IF event.requestee_id == bug.reporter.id %]
+Since you reported this [% terms.bug %], the person asking needs your input to understand it.
+Responding to the question will enable developers to take further action on this [% terms.bug %].
+[% ELSE %]
+Please respond as soon as possible so developers may take action on this [% terms.bug %].
+[% END %]
+If you have questions about responding to needinfo requests, please see the
+<a href="https://wiki.mozilla.org/BMO/UserGuide#Needinfo_Flag">needinfo user guide</a>.

extensions/Needinfo/template/en/default/hook/email/bugmail-flag_event.txt.tmpl

@@ -0,0 +1,16 @@
+[%# 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.
+  #%]
+
+[% RETURN UNLESS event.action == 'requested' && event.type.name == 'needinfo' %]
+[% IF event.requestee_id == bug.reporter.id %]
+Since you reported this [% terms.bug %], the person asking needs your input to understand it. Responding to the question will enable developers to take further action on this [% terms.bug %].
+[% ELSE %]
+Please respond as soon as possible so developers may take action on this [% terms.bug %].
+[% END %]
+If you have questions about responding to needinfo requests, please see
+https://wiki.mozilla.org/BMO/UserGuide#Needinfo_Flag.

extensions/Splinter/template/en/default/hook/email/bugmail-flag_event.html.tmpl

@@ -0,0 +1,14 @@
+[%# 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.
+  #%]
+
+[% USE Bugzilla %]
+[% RETURN UNLESS event.action == 'requested'
+      && (event.type.name == 'review' || event.type.name == 'feedback')
+      && event.attachment && event.attachment.can_review %]
+<br>
+<a href="[% Bugzilla.splinter_review_url(bug.id, event.attachment_id, 1) FILTER html %]">Review</a>