Bug 2085141 - Make it more obvious when @mention is going to needinfo someone

Bugzilla/API/V1/BugComment.pm

@@ -271,7 +271,7 @@ sub render_comment {
   # Extensions can add params and return values. When id is given, the Needinfo
   # extension accepts include_mentions and mention_is_private and returns
   # mentions: the users that saving the comment on that bug would CC and
-  # needinfo, shown by the comment preview.
+  # needinfo. (The comment editor's live warning uses POST /needinfo/mentions.)
   my $result = {html => $html};
   Bugzilla::Hook::process('webservice_bug_render_comment',
     {webservice => $self, bug => $bug, params => $params, result => $result});

extensions/BugModal/web/attachments_overlay.js

@@ -716,6 +716,8 @@ window.addEventListener('DOMContentLoaded', () => {
   const updateComment = () => {
     if ($commentPane) {
       $form.comment.value = '';
+      // Let the text editor know, e.g. to clear the @mention warning
+      $form.comment.dispatchEvent(new InputEvent('input'));
       $form.needinfo.checked = false;
       $form.needinfo_role.value = 'other';
       $form.needinfo_from.value = '';

extensions/BugModal/web/comments.js

@@ -1124,6 +1124,8 @@ Bugzilla.BugModal.InstantUpdater = class InstantUpdater {
     this.$commentTextArea.readOnly = false;
     this.$commentTextArea.hidden = false;
     this.$commentTextArea.value = '';
+    // Let listeners know, e.g. to clear the @mention warning and the saved comment draft
+    this.$commentTextArea.dispatchEvent(new InputEvent('input'));
 
     // Switch to preview mode to show the comment box
     document.querySelector('#add-comment [data-command="edit"]').click();

extensions/Needinfo/Extension.pm

@@ -305,11 +305,60 @@ sub _users_for_mentions {
 
 # Returns the users a set of new comments ({thetext, isprivate} hashes) on
 # $bug would CC and needinfo, applying the safeguards described above
-# _process_mentions. Shared by saving and the comment preview so the preview
-# can't disagree with what actually happens. $old_bug is only passed when
-# saving, to account for groups added in the same update.
+# _process_mentions. Shared by saving, the comment preview and the live
+# mention warning so they can't disagree with what actually happens. $old_bug
+# is only passed when saving, to account for groups added in the same update.
 sub _mentioned_users {
   my ($bug, $comments, $old_bug) = @_;
+
+  # can_see_bug reads the groups stored in the database, which don't yet
+  # include groups added by this update, so check those separately. Only
+  # looked up once a mention has passed the cheaper checks.
+  my $added_groups;
+  my $can_see = sub {
+    my ($mentioned) = @_;
+    return 0 unless $mentioned->can_see_bug($bug->id);
+    $added_groups //= _added_groups($bug, $old_bug);
+    return all { $mentioned->in_group($_) } @$added_groups;
+  };
+
+  return _filter_mentioned_users($bug->product_id, $comments, $can_see);
+}
+
+# Like _mentioned_users, for a bug that is still being filed in $product with
+# the bug groups named in $group_names. Used by the live mention warning on
+# the bug entry form, where there is no bug to check visibility against yet:
+# the mentioned user has to be able to see the product and be in every group
+# the new bug will be in (the selected ones plus the product's mandatory ones).
+sub _mentioned_users_for_new_bug {
+  my ($product, $group_names, $comments) = @_;
+
+  my %valid = map { $_->name => 1 } @{$product->groups_valid};
+  my %groups = map { $_ => 1 } (grep { $valid{$_} } @{$group_names || []}),
+    (map { $_->name } @{$product->groups_mandatory});
+
+  my $can_see = sub {
+    my ($mentioned) = @_;
+    return $mentioned->can_see_product($product->name)
+      && all { $mentioned->in_group($_) } keys %groups;
+  };
+
+  return _filter_mentioned_users($product->id, $comments, $can_see);
+}
+
+sub _added_groups {
+  my ($bug, $old_bug) = @_;
+  return [] unless $old_bug;
+  my %old_groups = map { $_->id => 1 } @{$old_bug->groups_in};
+  return [map { $_->name } grep { !$old_groups{$_->id} } @{$bug->groups_in}];
+}
+
+# The checks shared by _mentioned_users and _mentioned_users_for_new_bug.
+# $can_see is called with each mentioned user and returns whether they can see
+# the bug. With strict_isolation on, users who can't edit the product are
+# skipped, since add_cc would reject them.
+sub _filter_mentioned_users {
+  my ($product_id, $comments, $can_see) = @_;
   return () unless Bugzilla->params->{needinfo_mentions_enabled};
 
   # Lowercased nick => true if mentioned in at least one public comment.
@@ -328,23 +377,15 @@ sub _mentioned_users {
 
   # Checked after parsing so updates without mentions skip the group lookup.
   my $user = Bugzilla->user;
-  return () unless $user->in_group('editbugs', $bug->product_id);
-
-  # can_see_bug reads the groups stored in the database, which don't yet
-  # include groups added by this update, so check those separately.
-  my @added_groups;
-  if ($old_bug) {
-    my %old_groups = map { $_->id => 1 } @{$old_bug->groups_in};
-    @added_groups
-      = map { $_->name } grep { !$old_groups{$_->id} } @{$bug->groups_in};
-  }
+  return () unless $user->in_group('editbugs', $product_id);
+  my $strict_isolation = Bugzilla->params->{strict_isolation};
 
   return grep {
     my $mentioned = $_;
          $mentioned->id != $user->id
       && ($public{lc $mentioned->nick} || $mentioned->is_insider)
-      && $mentioned->can_see_bug($bug->id)
-      && all { $mentioned->in_group($_) } @added_groups
+      && (!$strict_isolation || $mentioned->can_edit_product($product_id))
+      && $can_see->($mentioned)
   } _users_for_mentions(@nicks);
 }
 
@@ -374,14 +415,12 @@ sub _process_mentions {
     = _mentioned_users($bug, $bug->{added_comments} || [], $old_bug);
   return unless @mentioned;
 
-  my ($type) = grep { $_->name eq 'needinfo' } @{$bug->flag_types};
-  undef $type
-    unless $type && $bug->check_can_change_field('flagtypes.name', 0, 1)->{allowed};
+  my $type = _needinfo_type($bug);
 
-  # A failure for one user (e.g. add_cc's strict_isolation check) skips that
-  # user rather than rolling back the whole update. Throw*Error only dies
-  # (instead of printing an error page and exiting) in ERROR_MODE_DIE, so the
-  # eval can catch it.
+  # A failure for one user (e.g. a validation error from add_cc or set_flags)
+  # skips that user rather than rolling back the whole update. Throw*Error
+  # only dies (instead of printing an error page and exiting) in
+  # ERROR_MODE_DIE, so the eval can catch it.
   local Bugzilla->request_cache->{error_mode} = ERROR_MODE_DIE;
   local $@;
   foreach my $mentioned (@mentioned) {
@@ -397,38 +436,140 @@ sub _is_mass_edit {
   return grep {/^id_[1-9]/} Bugzilla->cgi->param;
 }
 
+# The needinfo flag type the current user can set on $bug, if any.
+sub _needinfo_type {
+  my ($bug) = @_;
+  my ($type) = grep { $_->name eq 'needinfo' } @{$bug->flag_types};
+  return undef
+    unless $type
+    && $bug->check_can_change_field('flagtypes.name', 0, 1)->{allowed};
+  return $type;
+}
+
+# What _can_needinfo checks a mention against: how many needinfo flags the
+# bug has, and the ids of users with a pending needinfo request. Built from
+# the flags in $type->{flags} (none for a bug being filed).
+sub _needinfo_state {
+  my ($type) = @_;
+  my @flags = $type ? @{$type->{flags} || []} : ();
+  return {
+    count   => scalar @flags,
+    pending => {
+      map  { $_->requestee_id => 1 }
+      grep { $_->status eq '?' && $_->requestee_id } @flags
+    },
+  };
+}
+
+# Whether a mention of $mentioned would request needinfo, given the needinfo
+# $type from _needinfo_type and its $state from _needinfo_state. Mirrors the
+# checks Bugzilla::Flag makes when the request is saved, so that a request
+# which would be rejected is predicted (and handled) as CC-only.
+sub _can_needinfo {
+  my ($type, $mentioned, $state) = @_;
+  return 0 if !$type || $mentioned->needinfo_blocked;
+  return 0
+    unless $type->is_active && $type->is_requestable && $type->is_requesteeble;
+  return 0 unless Bugzilla->user->can_change_flag($type, 'X', '?');
+  return 0 unless $mentioned->can_set_flag($type);
+
+  # A non-multiplicable type allows only one needinfo flag per bug.
+  return 0 if !$type->is_multiplicable && $state->{count};
+  return 0 if $state->{pending}{$mentioned->id};
+
+  return 1;
+}
+
 sub _cc_and_needinfo {
   my ($bug, $mentioned, $type) = @_;
   $bug->add_cc($mentioned);
 
-  return if !$type || $mentioned->needinfo_blocked;
-
-  # A non-multiplicable type allows only one needinfo flag per bug.
-  return if !$type->is_multiplicable && @{$type->{flags}};
-  return if grep {
-    $_->status eq '?' && ($_->requestee_id // 0) == $mentioned->id
-  } @{$type->{flags}};
+  return unless _can_needinfo($type, $mentioned, _needinfo_state($type));
 
   # One call per user so $type->{flags} reflects each flag as it is added.
   $bug->set_flags([],
     [{type_id => $type->id, status => '?', requestee => $mentioned->login}]);
 }
 
+# Describes what saving a new comment would do with its @mentions, for the
+# live mention warning in the comment editor and the comment preview. $comment
+# is a {thetext, isprivate} hash. $target is either {bug => $bug} for a comment
+# on an existing bug, or {product => $product, component => $component,
+# groups => [group names]} for the initial comment of a bug being filed.
+# Returns an array ref of {nick, name, needinfo} hashes, one per user that
+# would be CC'd; needinfo is true if they would also get a needinfo request.
+sub mention_effects {
+  my ($comment, $target) = @_;
+
+  # Saving a new bug doesn't process the mentions in its initial comment yet
+  # (bug 2087012), so predict nothing rather than promise notifications that
+  # won't be sent. Once it does, return new_bug_mention_effects here.
+  my $bug = $target->{bug} or return [];
+
+  my @mentioned = _mentioned_users($bug, [$comment]);
+  return [] unless @mentioned;
+  return _describe_mentions(_needinfo_type($bug), \@mentioned);
+}
+
+# mention_effects for the initial comment of a bug being filed, with $target
+# as described there (component and groups are optional). Not used until
+# saving a new bug processes its mentions (bug 2087012).
+sub new_bug_mention_effects {
+  my ($comment, $target) = @_;
+  my $product = $target->{product};
+  my @mentioned
+    = _mentioned_users_for_new_bug($product, $target->{groups}, [$comment]);
+  return [] unless @mentioned;
+
+  # A new bug has no flags yet, and the needinfo type is checked against the
+  # product (and component) the bug is being filed in.
+  my $component = $target->{component};
+  my ($type) = @{
+    Bugzilla::FlagType::match({
+      name         => 'needinfo',
+      target_type  => 'bug',
+      is_active    => 1,
+      product_id   => $product->id,
+      component_id => $component ? $component->id : undef,
+    })
+  };
+  return _describe_mentions($type, \@mentioned);
+}
+
+# The {nick, name, needinfo} hashes for mention_effects. Saving requests
+# needinfo one user at a time, so each predicted request is counted before
+# the next user is checked (e.g. only the first user mentioned gets a
+# non-multiplicable needinfo). The bug's actual flags are left alone.
+sub _describe_mentions {
+  my ($type, $mentioned) = @_;
+  my $state = _needinfo_state($type);
+
+  return [
+    map {
+      my $needinfo = _can_needinfo($type, $_, $state);
+      if ($needinfo) {
+        $state->{count}++;
+        $state->{pending}{$_->id} = 1;
+      }
+      +{nick => $_->nick, name => $_->name, needinfo => $needinfo};
+    } @$mentioned
+  ];
+}
+
 # Lets the comment preview list who saving the comment would CC and needinfo,
-# so a stray @word is caught before the comment is submitted. The comment
-# editor sends include_mentions (and mention_is_private) along with the bug id
-# only for new comments on an existing bug, the only case _process_mentions
-# handles.
+# so a stray @word is caught before the comment is submitted. Clients send
+# include_mentions (and mention_is_private) along with the bug id only for
+# new comments on an existing bug. The comment editor itself now uses the
+# lighter POST /rest/needinfo/mentions endpoint (lib/API/V1/Mentions.pm).
 sub webservice_bug_render_comment {
   my ($self, $args) = @_;
   my ($bug, $params) = @$args{qw(bug params)};
   return unless $bug && $params->{include_mentions};
 
-  my @mentioned = _mentioned_users($bug,
-    [{thetext => $params->{text}, isprivate => $params->{mention_is_private}}]);
-
-  $args->{result}->{mentions}
-    = [map { +{nick => $_->nick, name => $_->name} } @mentioned];
+  $args->{result}->{mentions} = mention_effects(
+    {thetext => $params->{text}, isprivate => $params->{mention_is_private}},
+    {bug     => $bug}
+  );
 }
 
 sub _check_requestee {

extensions/Needinfo/lib/API/V1/Mentions.pm

@@ -0,0 +1,101 @@
+# 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::Extension::Needinfo::API::V1::Mentions;
+
+use 5.10.1;
+use Mojo::Base qw( Mojolicious::Controller );
+
+use Bugzilla::Bug;
+use Bugzilla::Component;
+use Bugzilla::Constants;
+use Bugzilla::API::V1::Util qw(coerce_booleans merge_request_params);
+
+sub setup_routes {
+  my ($class, $r) = @_;
+  $r->post('/needinfo/mentions')->to('Needinfo::API::V1::Mentions#mentions');
+}
+
+# POST /needinfo/mentions
+#
+# Lists the users that the @mentions in a new comment would CC and needinfo,
+# without rendering the comment. The comment editor calls this as the user
+# types, to warn before saving. Params:
+#   text        the comment text (required)
+#   id          id or alias of the bug the comment is added to, or
+#   product     name of the product a new bug is being filed in, with optional
+#   component   component name and
+#   groups      bug group names for the new bug
+#   is_private  whether the comment is private
+# Returns {mentions => [{nick, name, needinfo}]}; see
+# Bugzilla::Extension::Needinfo::mention_effects. For a new bug the list is
+# empty until saving a new bug processes its mentions (bug 2087012).
+sub mentions {
+  my ($self) = @_;
+  Bugzilla->usage_mode(USAGE_MODE_MOJO_REST);
+
+  my $user = $self->bugzilla->login;
+  $user->id || return $self->user_error('login_required');
+
+  my ($params, $error) = merge_request_params($self, ['groups']);
+  return $self->user_error($error) if $error;
+
+  if ($error = coerce_booleans($params, 'is_private')) {
+    return $self->user_error('invalid_params', {type_error => $error});
+  }
+
+  if (!defined $params->{text}) {
+    return $self->code_error('params_required',
+      {function => 'Needinfo.mentions', params => ['text']});
+  }
+  if (!$params->{id} && !$params->{product}) {
+    return $self->code_error('params_required',
+      {function => 'Needinfo.mentions', params => ['id', 'product']});
+  }
+
+  # Single values, so they can't reach the object loaders as conditions.
+  foreach my $name (qw(text id product component)) {
+    next unless ref $params->{$name};
+    return $self->user_error('invalid_params',
+      {type_error => "$name must be a single value"});
+  }
+  my $groups = $params->{groups} // [];
+  $groups = [$groups] unless ref $groups;
+  if (ref $groups ne 'ARRAY' || grep {ref} @$groups) {
+    return $self->user_error('invalid_params',
+      {type_error => 'groups must be a list of group names'});
+  }
+
+  my $comment = {thetext => $params->{text}, isprivate => $params->{is_private}};
+
+  # Nothing to look up without an @, the common case while typing.
+  return $self->render(json => {mentions => []}) unless $params->{text} =~ /@/;
+
+  Bugzilla->switch_to_shadow_db();
+
+  my $target;
+  if ($params->{id}) {
+    $target = {bug => Bugzilla::Bug->check($params->{id})};
+  }
+  else {
+    my $product = $user->can_enter_product($params->{product}, THROW_ERROR);
+    my $component
+      = defined $params->{component} && $params->{component} ne ''
+      ? Bugzilla::Component->new({product => $product, name => $params->{component}
+      })
+      : undef;
+    $target = {product => $product, component => $component, groups => $groups,};
+  }
+
+  return $self->render(
+    json => {
+      mentions => Bugzilla::Extension::Needinfo::mention_effects($comment, $target)
+    }
+  );
+}
+
+1;