diff --git a/Bugzilla/API/V1/BugComment.pm b/Bugzilla/API/V1/BugComment.pm index 261ead983d..6498ee7e26 100644 --- a/Bugzilla/API/V1/BugComment.pm +++ b/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}); diff --git a/extensions/BugModal/web/attachments_overlay.js b/extensions/BugModal/web/attachments_overlay.js index 2360b3b7ce..3255e9ac2b 100644 --- a/extensions/BugModal/web/attachments_overlay.js +++ b/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 = ''; diff --git a/extensions/BugModal/web/comments.js b/extensions/BugModal/web/comments.js index dc7f66d6b7..9a807e61d7 100644 --- a/extensions/BugModal/web/comments.js +++ b/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(); diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index c298d8a5eb..1e01ea20c2 100644 --- a/extensions/Needinfo/Extension.pm +++ b/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 { diff --git a/extensions/Needinfo/lib/API/V1/Mentions.pm b/extensions/Needinfo/lib/API/V1/Mentions.pm new file mode 100644 index 0000000000..01224799e7 --- /dev/null +++ b/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; diff --git a/extensions/Needinfo/t/mentions.t b/extensions/Needinfo/t/mentions.t index 0d37d3702d..cdec447b88 100644 --- a/extensions/Needinfo/t/mentions.t +++ b/extensions/Needinfo/t/mentions.t @@ -63,7 +63,11 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); } sub is_insider { $_[0]{insider} } sub can_see_bug { $_[0]{sees_bug} // 1 } - sub needinfo_blocked { 0 } + sub can_see_product { $_[0]{sees_product} // 1 } + sub needinfo_blocked { $_[0]{blocked} // 0 } + sub can_set_flag { $_[0]{can_set} // 1 } + sub can_change_flag { $_[0]{can_request} // 1 } + sub can_edit_product { $_[0]{edits_product} // 1 } package FakeBug; sub new { @@ -101,6 +105,9 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); sub name {'needinfo'} sub id {1} sub is_multiplicable { $_[0]{multiplicable} } + sub is_requestable { $_[0]{requestable} // 1 } + sub is_requesteeble { $_[0]{requesteeble} // 1 } + sub is_active { $_[0]{active} // 1 } package FakeFlag; sub status { $_[0]{status} } @@ -113,6 +120,16 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); sub new { my ($class, $id, $name) = @_; bless {id => $id, name => $name}, $class } sub id { $_[0]{id} } sub name { $_[0]{name} } + + package FakeProduct; + sub new { + my ($class, %args) = @_; + bless {valid => [], mandatory => [], %args}, $class; + } + sub id {1} + sub name {'Fake Product'} + sub groups_valid { $_[0]{valid} } + sub groups_mandatory { $_[0]{mandatory} } } my %users = map { $_->nick => $_ } ( @@ -183,9 +200,39 @@ push @{$bug->{type}{flags}}, bless({status => '?', requestee_id => 2}, 'FakeFlag $process->($bug); is($bug->{needinfo}, [], 'no duplicate needinfo for a pending request'); +$users{bob}{can_set} = 0; +$bug = FakeBug->new('@alice @bob'); +$process->($bug); +is($bug->{cc}, ['alice', 'bob'], 'requestee without flag privs: CCd'); +is($bug->{needinfo}, ['alice@example.com'], '... but not needinfod'); +delete $users{bob}{can_set}; + +$commenter->{can_request} = 0; +$bug = FakeBug->new('@alice'); +$process->($bug); +is($bug->{cc}, ['alice'], 'commenter cannot request needinfo: CCd'); +is($bug->{needinfo}, [], '... but not needinfod'); +delete $commenter->{can_request}; + +$bug = FakeBug->new('@alice', active => 0); +push @{$bug->{type}{flags}}, + bless({status => '?', requestee_id => 9}, 'FakeFlag'); +$process->($bug); +is($bug->{cc}, ['alice'], 'inactive type with existing flags: CCd'); +is($bug->{needinfo}, [], '... but not needinfod'); + +$params{strict_isolation} = 1; +$users{bob}{edits_product} = 0; +$bug = FakeBug->new('@bob @alice', multiplicable => 0); +$process->($bug); +is($bug->{cc}, ['alice'], + 'strict_isolation: users who cannot edit the product are skipped'); +is($bug->{needinfo}, ['alice@example.com'], '... and do not take the needinfo'); +delete $users{bob}{edits_product}; +delete $params{strict_isolation}; + $bug = FakeBug->new('@boom @alice'); -ok(lives { $process->($bug) }, - 'a failing CC (e.g. strict_isolation) does not throw'); +ok(lives { $process->($bug) }, 'a failing CC does not throw'); is($bug->{cc}, ['alice'], '... that user is skipped'); is($bug->{needinfo}, ['alice@example.com'], '... others still processed'); @@ -251,7 +298,7 @@ my $render = sub { is( $render->({text => '@alice @hidden @me', id => 1, include_mentions => 1}) ->{mentions}, - [{nick => 'alice', name => 'Alice'}], + [{nick => 'alice', name => 'Alice', needinfo => 1}], 'preview lists the users a save would notify' ); is($preview_bug->{cc}, [], '... without changing the bug'); @@ -264,7 +311,7 @@ is( include_mentions => 1, mention_is_private => 1 })->{mentions}, - [{nick => 'bob', name => 'Bob'}], + [{nick => 'bob', name => 'Bob', needinfo => 1}], 'preview of a private comment lists only insiders' ); $users{bob}{insider} = 0; @@ -279,7 +326,213 @@ is($render->({text => '@alice', id => 1, include_mentions => 1})->{mentions}, [], 'preview lists nobody when needinfo_mentions_enabled is off'); $params{needinfo_mentions_enabled} = 1; +# The live mention warning (POST /rest/needinfo/mentions) tells needinfo from +# CC-only mentions. +my $effects = \&Bugzilla::Extension::Needinfo::mention_effects; + +$preview_bug = FakeBug->new(''); +push @{$preview_bug->{type}{flags}}, + bless({status => '?', requestee_id => 3}, 'FakeFlag'); +$users{alice}{blocked} = 1; +is( + $effects->({thetext => '@alice @bob @u1 @me'}, {bug => $preview_bug}), + [ + {nick => 'alice', name => 'Alice', needinfo => 0}, + {nick => 'bob', name => 'Bob', needinfo => 0}, + {nick => 'u1', name => 'U1', needinfo => 1}, + ], + 'warning: blocked and already needinfod users are only CCd' +); +$users{alice}{blocked} = 0; +is($preview_bug->{cc}, [], '... without changing the bug'); + +$preview_bug = FakeBug->new('', multiplicable => 0); +push @{$preview_bug->{type}{flags}}, + bless({status => '?', requestee_id => 9}, 'FakeFlag'); +is( + $effects->({thetext => '@alice'}, {bug => $preview_bug}), + [{nick => 'alice', name => 'Alice', needinfo => 0}], + 'warning: non-multiplicable type with a flag only CCs' +); + +is( + $effects->( + {thetext => '@alice @bob'}, + {bug => FakeBug->new('', multiplicable => 0)} + ), + [ + {nick => 'alice', name => 'Alice', needinfo => 1}, + {nick => 'bob', name => 'Bob', needinfo => 0}, + ], + 'warning: non-multiplicable type without a flag only needinfos the first' +); + +$preview_bug = FakeBug->new(''); +$users{bob}{can_set} = 0; +is( + $effects->({thetext => '@alice @bob'}, {bug => $preview_bug}), + [ + {nick => 'alice', name => 'Alice', needinfo => 1}, + {nick => 'bob', name => 'Bob', needinfo => 0}, + ], + 'warning: a requestee without flag privs is only CCd' +); +delete $users{bob}{can_set}; +is($preview_bug->{type}{flags}, [], '... and the bug\'s flags are unchanged'); + +$commenter->{can_request} = 0; +is( + $effects->({thetext => '@alice'}, {bug => FakeBug->new('')}), + [{nick => 'alice', name => 'Alice', needinfo => 0}], + 'warning: no needinfo when the commenter cannot request it' +); +delete $commenter->{can_request}; + +$preview_bug = FakeBug->new('', active => 0); +push @{$preview_bug->{type}{flags}}, + bless({status => '?', requestee_id => 9}, 'FakeFlag'); +is( + $effects->({thetext => '@alice'}, {bug => $preview_bug}), + [{nick => 'alice', name => 'Alice', needinfo => 0}], + 'warning: no needinfo for an inactive type with existing flags' +); + +$params{strict_isolation} = 1; +$users{bob}{edits_product} = 0; +is( + $effects->( + {thetext => '@bob @alice'}, + {bug => FakeBug->new('', multiplicable => 0)} + ), + [{nick => 'alice', name => 'Alice', needinfo => 1}], + 'warning: strict_isolation skips users who cannot edit the product' +); +delete $users{bob}{edits_product}; +delete $params{strict_isolation}; + +foreach my $field (qw(requestable requesteeble)) { + is( + $effects->({thetext => '@alice'}, {bug => FakeBug->new('', $field => 0)}), + [{nick => 'alice', name => 'Alice', needinfo => 0}], + "warning: no needinfo when the type is not $field" + ); +} + +is($effects->({thetext => "`\@alice`\n> \@bob"}, {bug => FakeBug->new('')}), + [], 'warning: mentions in code and quotes are ignored'); + +{ + my $mock_bug_type = mock 'FakeBug' => + (override => [check_can_change_field => sub { {allowed => 0} }]); + is( + $effects->({thetext => '@alice'}, {bug => FakeBug->new('')}), + [{nick => 'alice', name => 'Alice', needinfo => 0}], + 'warning: no needinfo when the user cannot change flags on the bug' + ); +} + +# Saving a new bug doesn't process mentions yet (bug 2087012), so the warning +# on the bug entry form lists nobody. +is($effects->({thetext => '@alice'}, {product => FakeProduct->new}), + [], 'new bug: nobody until saving a new bug processes mentions'); + +# The prediction for the initial comment of a bug being filed, ready for when +# it does: there is no bug to check visibility against, so the product and +# the new bug's groups are used. +my $new_bug_effects = \&Bugzilla::Extension::Needinfo::new_bug_mention_effects; +my $needinfo_types = [bless({multiplicable => 1}, 'FakeType')]; +my %type_criteria; +my $mock_flagtype = mock 'Bugzilla::FlagType' => ( + override => [ + match => sub { %type_criteria = %{$_[0]}; return $needinfo_types }, + ], +); +my $product = FakeProduct->new(valid => [$security]); + +is( + $new_bug_effects->({thetext => '@alice @hidden @me'}, {product => $product}), + [ + {nick => 'alice', name => 'Alice', needinfo => 1}, + {nick => 'hidden', name => 'Hidden', needinfo => 1}, + ], + 'new bug: users who can see the product are needinfod' +); +is($type_criteria{name}, 'needinfo', '... using the needinfo flag type'); +is($type_criteria{component_id}, undef, '... of the product when no component'); + +$users{alice}{sees_product} = 0; +is( + $new_bug_effects->({thetext => '@alice @bob'}, {product => $product}), + [{nick => 'bob', name => 'Bob', needinfo => 1}], + 'new bug: users who cannot see the product are skipped' +); +delete $users{alice}{sees_product}; + +is( + $new_bug_effects->( + {thetext => '@alice @bob'}, + {product => $product, groups => ['core-security', 'not-valid-here']} + ), + [{nick => 'bob', name => 'Bob', needinfo => 1}], + 'new bug: selected groups limit who is mentioned, invalid ones ignored' +); + +is( + $new_bug_effects->( + {thetext => '@alice @bob'}, + {product => FakeProduct->new(mandatory => [$security])} + ), + [{nick => 'bob', name => 'Bob', needinfo => 1}], + 'new bug: mandatory product groups limit who is mentioned' +); + +is( + $new_bug_effects->( + {thetext => '@alice'}, + {product => $product, component => FakeGroup->new(7, 'General')} + ), + [{nick => 'alice', name => 'Alice', needinfo => 1}], + 'new bug with a component' +); +is($type_criteria{component_id}, 7, '... uses that component\'s flag types'); + +$users{alice}{blocked} = 1; +is( + $new_bug_effects->({thetext => '@alice'}, {product => $product}), + [{nick => 'alice', name => 'Alice', needinfo => 0}], + 'new bug: a user blocking needinfo is only CCd' +); +$users{alice}{blocked} = 0; + +$params{strict_isolation} = 1; +$users{bob}{edits_product} = 0; +is( + $new_bug_effects->({thetext => '@bob @alice'}, {product => FakeProduct->new}), + [{nick => 'alice', name => 'Alice', needinfo => 1}], + 'new bug: strict_isolation skips users who cannot edit the product' +); +delete $users{bob}{edits_product}; +delete $params{strict_isolation}; + +$needinfo_types = []; +is( + $new_bug_effects->({thetext => '@alice'}, {product => $product}), + [{nick => 'alice', name => 'Alice', needinfo => 0}], + 'new bug: no needinfo without a needinfo flag type' +); +$mock_flagtype = undef; + +$params{needinfo_mentions_enabled} = 0; +is($new_bug_effects->({thetext => '@alice'}, {product => $product}), + [], 'new bug: nobody when needinfo_mentions_enabled is off'); +$params{needinfo_mentions_enabled} = 1; + $commenter->{editbugs} = 0; +is($new_bug_effects->({thetext => '@alice'}, {product => $product}), + [], 'new bug: nobody for commenters without editbugs'); +is($effects->({thetext => '@alice'}, {bug => FakeBug->new('')}), + [], 'warning: nobody for commenters without editbugs'); + @looked_up = (); $bug = FakeBug->new('@alice'); $process->($bug); diff --git a/extensions/Needinfo/template/en/default/hook/global/header-start.html.tmpl b/extensions/Needinfo/template/en/default/hook/global/header-start.html.tmpl index 0cca4b09a3..909449fa0e 100644 --- a/extensions/Needinfo/template/en/default/hook/global/header-start.html.tmpl +++ b/extensions/Needinfo/template/en/default/hook/global/header-start.html.tmpl @@ -16,3 +16,8 @@ style_urls.push('extensions/Needinfo/web/styles/needinfo.css'); END; %] + +[%# Lets the comment editor warn, as the user types, who the comment's + # @mentions would needinfo. The server still decides per bug. %] +[% js_BUGZILLA.param.needinfo_mentions_enabled + = (user.id && Param('needinfo_mentions_enabled')) ? 1 : 0 %] diff --git a/js/text-editor.js b/js/text-editor.js index 04ff6ad1ea..5fb48a7a16 100644 --- a/js/text-editor.js +++ b/js/text-editor.js @@ -684,25 +684,12 @@ Bugzilla.TextEditor = class TextEditor { this.$preview.innerHTML = `

${this.str.loading.htmlEncode()}

`; try { - const { html, mentions } = await Bugzilla.API.post('bug/comment/render', params); + const { html } = await Bugzilla.API.post('bug/comment/render', params); // Use the HTML without `htmlEncode()` because the API-provided rendered text should be safe // to be injected as is; using `htmlEncode()` rather breaks the text this.$preview.innerHTML = html; - // List who saving the comment will CC and needinfo, so a stray `@word` is noticed before - // the comment is submitted - if (mentions?.length) { - const users = mentions - .map(({ nick, name }) => (name ? `${name} (@${nick})` : `@${nick}`).htmlEncode()) - .join(', '); - - this.$preview.insertAdjacentHTML( - 'afterbegin', - `

${this.str.mention_notice.htmlEncode()} ${users}

`, - ); - } - // Highlight code if possible if (Prism) { Prism.highlightAllUnder(this.$preview); @@ -761,6 +748,8 @@ Bugzilla.CommentEditor = class CommentEditor extends Bugzilla.TextEditor { 'Accel+Enter': { handler: () => this.submitForm() }, }); } + + this.initMentionWarning(); } /** @@ -771,29 +760,189 @@ Bugzilla.CommentEditor = class CommentEditor extends Bugzilla.TextEditor { this.$editTabPanel.appendChild(this.$textarea); } + /** + * The ID of the bug a new comment is being added to, if any. + * @type {string | undefined} + */ + get bugId() { + if (this.$textarea.name !== 'comment') { + return undefined; + } + + return BUGZILLA.bug_id ?? this.$textarea.form?.elements.namedItem('bugid')?.value; + } + /** * Get the params sent to the comment render API when previewing. A new comment on an existing - * bug is rendered against that bug, and also asks for the users its `@nickname` mentions will - * CC and needinfo. + * bug is rendered against that bug. * @returns {Record} Params. */ getPreviewParams() { const params = super.getPreviewParams(); - const form = this.$textarea.form; - const bugId = BUGZILLA.bug_id ?? form?.elements.namedItem('bugid')?.value; + const { bugId } = this; - if (bugId && this.$textarea.name === 'comment') { + if (bugId) { params.id = bugId; - params.include_mentions = true; - params.mention_is_private = !!( - form?.elements.namedItem('comment_is_private')?.checked || - form?.elements.namedItem('isprivate')?.checked - ); } return params; } + /** + * Add a live region below the editor that warns who the comment's `@nickname` mentions will + * needinfo and CC once it's saved, so nobody is needinfo'd by accident, e.g. by pasted text. + * It stays visible on both the Edit and Preview tabs, and updates as the user types or pastes. + */ + initMentionWarning() { + if (!BUGZILLA.param.needinfo_mentions_enabled || this.$textarea.name !== 'comment') { + return; + } + + // The live region stays in the document, even when empty, so screen readers announce updates + /** @type {HTMLElement} */ + this.$mentionWarning = document.createElement('div'); + this.$mentionWarning.className = 'mention-warning'; + this.$mentionWarning.setAttribute('aria-live', 'polite'); + this.$previewTabPanel.insertAdjacentElement('afterend', this.$mentionWarning); + + /** @type {number | undefined} */ + this.mentionTimer = undefined; + /** @type {string | undefined} */ + this.lastMentionParams = undefined; + /** @type {number} */ + this.mentionRequestCount = 0; + /** @type {string} */ + this.mentionWarningHTML = ''; + + const schedule = () => this.scheduleMentionCheck(); + + this.$textarea.addEventListener('input', schedule); + // The comment privacy and, on bug entry, the product, component and groups affect the result + this.$textarea.form?.addEventListener('change', schedule); + this.$textarea.form?.addEventListener('reset', () => window.setTimeout(schedule)); + // The textarea may already have text, e.g. a cloned bug's description + schedule(); + } + + /** + * Get the params sent to the mentions API, or `undefined` if mentions can't trigger a needinfo + * here, e.g. on a mass edit, or the text doesn't contain any mention. + * @returns {Record | undefined} Params. + */ + getMentionParams() { + const text = this.$textarea.value; + + // Same as the check in `Bugzilla.BugModal.InstantUpdater`; the server does the real parsing + if (!/(? + /** @type {HTMLInputElement | null} */ (form.elements.namedItem(name))?.value; + const product = valueOf('product'); + const component = valueOf('component'); + // Group checkboxes, or hidden fields on custom bug forms + const groups = [ + .../** @type {NodeListOf} */ (form.querySelectorAll('[name="groups"]')), + ] + .filter(($input) => $input.type !== 'checkbox' || $input.checked) + .map(($input) => $input.value); + + if (product) { + return { text, product, component, groups, is_private }; + } + } + + return undefined; + } + + /** + * Check the mentions shortly after the user stops typing. + */ + scheduleMentionCheck() { + window.clearTimeout(this.mentionTimer); + this.mentionTimer = window.setTimeout(() => this.checkMentions(), 500); + } + + /** + * Ask the server who the comment's mentions will needinfo and CC, and update the warning. + */ + async checkMentions() { + const params = this.getMentionParams(); + const paramsKey = JSON.stringify(params ?? null); + + if (paramsKey === this.lastMentionParams) { + return; + } + + this.lastMentionParams = paramsKey; + + // Ignore responses to requests that have been superseded while waiting + const requestCount = ++this.mentionRequestCount; + + if (!params) { + this.showMentionWarning([]); + return; + } + + try { + const { mentions } = await Bugzilla.API.post('needinfo/mentions', params); + + if (requestCount === this.mentionRequestCount) { + this.showMentionWarning(mentions ?? []); + } + } catch { + if (requestCount === this.mentionRequestCount) { + // Try again on the next change rather than showing a stale warning + this.lastMentionParams = undefined; + this.showMentionWarning([]); + } + } + } + + /** + * Show who saving the comment will needinfo and CC, or hide the warning if nobody. + * @param {{ nick: string, name: string, needinfo: number | boolean }[]} mentions Users the + * comment mentions, as returned by the API. + */ + showMentionWarning(mentions) { + /** @param {{ nick: string, name: string }} user */ + const label = ({ nick, name }) => (name ? `${name} (@${nick})` : `@${nick}`).htmlEncode(); + const needinfo = mentions.filter((user) => user.needinfo).map(label); + const ccOnly = mentions.filter((user) => !user.needinfo).map(label); + const lines = []; + + if (needinfo.length) { + lines.push(`${this.str.mention_needinfo.htmlEncode()} ${needinfo.join(', ')}`); + } + + if (ccOnly.length) { + lines.push(`${this.str.mention_cc_only.htmlEncode()} ${ccOnly.join(', ')}`); + } + + const html = lines.map((line) => `

${line}

`).join(''); + + // Avoid announcing the same warning again + if (html !== this.mentionWarningHTML) { + this.mentionWarningHTML = html; + this.$mentionWarning.innerHTML = html; + } + } + /** * Submit the bug form if the comment is entered. Trigger a `click` event instead of submitting * the form directly to make sure any event handler associated with the button will be called. diff --git a/qa/t/rest_needinfo_mentions.t b/qa/t/rest_needinfo_mentions.t new file mode 100644 index 0000000000..3023334d9a --- /dev/null +++ b/qa/t/rest_needinfo_mentions.t @@ -0,0 +1,122 @@ +#!/usr/bin/env perl +# 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. + +##################################################### +# Test for the live @mention warning endpoint of # +# the Needinfo extension: # +# POST /rest/needinfo/mentions # +##################################################### + +use 5.10.1; +use strict; +use warnings; +use lib qw(lib ../../lib ../../local/lib/perl5); + +use Bugzilla; +use QA::Util qw(get_config random_string); +use QA::REST::Util qw(api_headers); + +use Test::Mojo; +use Test::More; + +my $config = get_config(); +my $url = Bugzilla->localconfig->urlbase; +my $admin = api_headers($config->{admin_user_api_key}); +my $no_privs = api_headers($config->{unprivileged_user_api_key}); +my $endpoint = $url . 'rest/needinfo/mentions'; + +my $t = Test::Mojo->new(); +$t->ua->max_redirects(1); + +# Mentions are looked up by the nickname in the real name, so create a user +# that has one. +my $nick = 'mention' . lc random_string(8); +$t->post_ok( + $url + . 'rest/user' => $admin => json => { + email => "$nick\@webservice.test", + full_name => "Mention Target [:$nick]", + password => 'UiX1Shuuchid', + } +)->status_is(201); + +my $expected + = [{nick => $nick, name => "Mention Target [:$nick]", needinfo => 1}]; + +# A comment on an existing bug. +$t->post_ok($endpoint => $admin => json => + {text => "\@$nick can you look? cc \@admin", id => 'public_bug'}) + ->status_is(200) + ->json_is( + '/mentions' => $expected, + 'the mentioned user would be needinfod, the commenter is skipped' + ); + +$t->post_ok($endpoint => $admin => json => + {text => "`\@$nick`\n> \@$nick\nfoo\@$nick.test", id => 'public_bug'}) + ->status_is(200) + ->json_is( + '/mentions' => [], + 'mentions in code, quotes and emails are ignored' + ); + +$t->post_ok($endpoint => $admin => json => + {text => '@nobody-has-this-nick', id => 'public_bug'}) + ->status_is(200) + ->json_is('/mentions' => [], 'unknown nicknames are ignored'); + +$t->post_ok( + $endpoint => $admin => json => {text => 'no mentions here', id => 'public_bug'}) + ->status_is(200) + ->json_is('/mentions' => [], 'no mentions without an @'); + +# Only editbugs users trigger mentions. +$t->post_ok($endpoint => $no_privs => json => + {text => "\@$nick can you look?", id => 'public_bug'}) + ->status_is(200) + ->json_is( + '/mentions' => [], + 'nobody is needinfod by a user without editbugs' + ); + +# The initial comment of a bug being filed. Saving a new bug doesn't process +# mentions yet (bug 2087012), so nobody is listed. +$t->post_ok( + $endpoint => $admin => json => { + text => "\@$nick can you look?", + product => 'TestProduct', + component => 'TestComponent', + groups => [], + } +)->status_is(200)->json_is( + '/mentions' => [], + 'new bug: nobody until saving a new bug processes mentions' +); + +$t->post_ok($endpoint => $admin => json => + {text => "\@$nick can you look?", product => 'TestProduct'}) + ->status_is(200) + ->json_is('/mentions' => [], 'new bug: the component is optional'); + +# Errors. +$t->post_ok($endpoint => $admin => json => {text => '@admin'}) + ->status_isnt(200) + ->json_like('/message' => qr/id, product/); +$t->post_ok($endpoint => $admin => json => {id => 'public_bug'}) + ->status_isnt(200) + ->json_like('/message' => qr/text/); +$t->post_ok($endpoint => $admin => json => + {text => '@admin', id => {condition => 'bug_id = ?', values => [1]}}) + ->status_is(400) + ->json_like('/message' => qr/id must be a single value/); +$t->post_ok($endpoint => api_headers(undef) => json => + {text => "\@$nick", id => 'public_bug'}) + ->status_isnt(200) + ->json_like('/message' => qr/You must log in/); + +done_testing(); diff --git a/skins/standard/text-editor.css b/skins/standard/text-editor.css index 7cc543f9ce..2adfe2d1ca 100644 --- a/skins/standard/text-editor.css +++ b/skins/standard/text-editor.css @@ -93,13 +93,20 @@ font-style: italic; } -.text-editor .comment-text .mention-notice { - border-bottom: 1px solid var(--control-border-color); - padding-bottom: 8px; - color: var(--secondary-label-color); +/* Live region: kept in the document while empty so screen readers announce updates */ +.text-editor .mention-warning:not(:empty) { + margin: 4px 0 0; + border-left: 4px solid var(--warning-message-border-color); + border-radius: 4px; + padding: 4px 8px; + background-color: var(--warning-message-background-color); font-size: var(--font-size-small); } +.text-editor .mention-warning p { + margin: 0; +} + .text-editor footer:not([hidden]) { padding: 8px 4px; font-size: var(--font-size-small); diff --git a/template/en/default/global/text-editor-include.html.tmpl b/template/en/default/global/text-editor-include.html.tmpl index b8387edc5a..851a5b3d1c 100644 --- a/template/en/default/global/text-editor-include.html.tmpl +++ b/template/en/default/global/text-editor-include.html.tmpl @@ -29,7 +29,8 @@ js_BUGZILLA.string.TextEditor = { command_bulleted_list => 'Bulleted list', loading => 'Loading…', preview_error => 'Preview could not be loaded. Please try again later.', - mention_notice => 'Saving will CC these users and request needinfo where allowed:', + mention_needinfo => 'This comment will request needinfo from:', + mention_cc_only => 'This comment will CC (without needinfo):', markdown_link => { text => 'Markdown supported', href => 'https://guides.github.com/features/mastering-markdown/',