From 90f390ebbd18c1d2c531fc55657561f4d1ab0ead Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 4 Nov 2026 11:36:10 -0500 Subject: [PATCH 1/4] Bug 2085141 - Make it more obvious when @mention is going to needinfo someone Adds a live aria-live notice under the comment box listing who the comment will needinfo, and who will only be CC'd, visible in both Edit and Preview. Backed by POST /rest/needinfo/mentions, which shares the save-time mention rules, so the notice matches what saving does. Replaces the in-preview mention notice. --- Bugzilla/API/V1/BugComment.pm | 2 +- extensions/Needinfo/Extension.pm | 171 ++++++++++++--- extensions/Needinfo/lib/API/V1/Mentions.pm | 100 +++++++++ extensions/Needinfo/t/mentions.t | 143 ++++++++++++- .../hook/global/header-start.html.tmpl | 5 + js/text-editor.js | 197 +++++++++++++++--- qa/t/rest_needinfo_mentions.t | 121 +++++++++++ skins/standard/text-editor.css | 15 +- .../global/text-editor-include.html.tmpl | 3 +- 9 files changed, 690 insertions(+), 67 deletions(-) create mode 100644 extensions/Needinfo/lib/API/V1/Mentions.pm create mode 100644 qa/t/rest_needinfo_mentions.t 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/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index c298d8a5eb..e68190493c 100644 --- a/extensions/Needinfo/Extension.pm +++ b/extensions/Needinfo/Extension.pm @@ -305,11 +305,59 @@ 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. +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 +376,13 @@ 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); 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 + && $can_see->($mentioned) } _users_for_mentions(@nicks); } @@ -374,9 +412,7 @@ 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 @@ -397,38 +433,105 @@ 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; +} + +# Whether a mention of $mentioned would request needinfo, given the needinfo +# $type from _needinfo_type with its current flags in $type->{flags}. +sub _can_needinfo { + my ($type, $mentioned) = @_; + return 0 if !$type || $mentioned->needinfo_blocked; + + # A non-multiplicable type allows only one needinfo flag per bug. + return 0 if !$type->is_multiplicable && @{$type->{flags} || []}; + return 0 + if grep { $_->status eq '?' && ($_->requestee_id // 0) == $mentioned->id } + @{$type->{flags} || []}; + + 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); # 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 +# (component and groups are optional). 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) = @_; + my ($type, @mentioned); + + if (my $bug = $target->{bug}) { + @mentioned = _mentioned_users($bug, [$comment]); + $type = _needinfo_type($bug) if @mentioned; + } + else { + my $product = $target->{product}; + @mentioned + = _mentioned_users_for_new_bug($product, $target->{groups}, [$comment]); + + # A new bug has no flags yet, and the needinfo type is checked against + # the product (and component) the bug is being filed in. + if (@mentioned) { + my $component = $target->{component}; + ($type) = @{ + Bugzilla::FlagType::match({ + name => 'needinfo', + target_type => 'bug', + is_active => 1, + product_id => $product->id, + component_id => $component ? $component->id : undef, + }) + }; + $type->{flags} = [] if $type; + } + } + + return [ + map { + +{ + nick => $_->nick, + name => $_->name, + needinfo => _can_needinfo($type, $_) ? 1 : 0 + } + } @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..4f39fe1de5 --- /dev/null +++ b/extensions/Needinfo/lib/API/V1/Mentions.pm @@ -0,0 +1,100 @@ +# 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. +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..cf502f4859 100644 --- a/extensions/Needinfo/t/mentions.t +++ b/extensions/Needinfo/t/mentions.t @@ -63,7 +63,8 @@ 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 } package FakeBug; sub new { @@ -113,6 +114,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 => $_ } ( @@ -251,7 +262,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 +275,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 +290,133 @@ 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`\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' + ); +} + +# The initial comment of a bug being filed: there is no bug to check +# visibility against, so the product and the new bug's groups are used. +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( + $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( + $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( + $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( + $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( + $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( + $effects->({thetext => '@alice'}, {product => $product}), + [{nick => 'alice', name => 'Alice', needinfo => 0}], + 'new bug: a user blocking needinfo is only CCd' +); +$users{alice}{blocked} = 0; + +$needinfo_types = []; +is( + $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($effects->({thetext => '@alice'}, {product => $product}), + [], 'new bug: nobody when needinfo_mentions_enabled is off'); +$params{needinfo_mentions_enabled} = 1; + $commenter->{editbugs} = 0; +is($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..e38ccd4f5f --- /dev/null +++ b/qa/t/rest_needinfo_mentions.t @@ -0,0 +1,121 @@ +#!/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. +$t->post_ok( + $endpoint => $admin => json => { + text => "\@$nick can you look?", + product => 'TestProduct', + component => 'TestComponent', + groups => [], + } +)->status_is(200)->json_is( + '/mentions' => $expected, + 'new bug: the mentioned user would be needinfod' +); + +$t->post_ok($endpoint => $admin => json => + {text => "\@$nick can you look?", product => 'TestProduct'}) + ->status_is(200) + ->json_is('/mentions' => $expected, '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/', From b1983a3fe962f52c84636784ef67d4eda8d0bd4d Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 4 Nov 2026 14:00:02 -0500 Subject: [PATCH 2/4] Copilot review fixes --- extensions/Needinfo/Extension.pm | 118 +++++++++++++-------- extensions/Needinfo/lib/API/V1/Mentions.pm | 3 +- extensions/Needinfo/t/mentions.t | 92 +++++++++++++--- qa/t/rest_needinfo_mentions.t | 9 +- 4 files changed, 162 insertions(+), 60 deletions(-) diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index e68190493c..e5dcc7d38b 100644 --- a/extensions/Needinfo/Extension.pm +++ b/extensions/Needinfo/Extension.pm @@ -443,17 +443,35 @@ sub _needinfo_type { 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 with its current flags in $type->{flags}. +# $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) = @_; + my ($type, $mentioned, $state) = @_; return 0 if !$type || $mentioned->needinfo_blocked; + return 0 unless $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 && @{$type->{flags} || []}; - return 0 - if grep { $_->status eq '?' && ($_->requestee_id // 0) == $mentioned->id } - @{$type->{flags} || []}; + return 0 if !$type->is_multiplicable && $state->{count}; + return 0 if $state->{pending}{$mentioned->id}; return 1; } @@ -462,7 +480,7 @@ sub _cc_and_needinfo { my ($bug, $mentioned, $type) = @_; $bug->add_cc($mentioned); - return unless _can_needinfo($type, $mentioned); + 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([], @@ -473,48 +491,64 @@ sub _cc_and_needinfo { # 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 -# (component and groups are optional). 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. +# 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) = @_; - my ($type, @mentioned); - if (my $bug = $target->{bug}) { - @mentioned = _mentioned_users($bug, [$comment]); - $type = _needinfo_type($bug) if @mentioned; - } - else { - my $product = $target->{product}; - @mentioned - = _mentioned_users_for_new_bug($product, $target->{groups}, [$comment]); - - # A new bug has no flags yet, and the needinfo type is checked against - # the product (and component) the bug is being filed in. - if (@mentioned) { - my $component = $target->{component}; - ($type) = @{ - Bugzilla::FlagType::match({ - name => 'needinfo', - target_type => 'bug', - is_active => 1, - product_id => $product->id, - component_id => $component ? $component->id : undef, - }) - }; - $type->{flags} = [] if $type; - } - } + # 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 { - +{ - nick => $_->nick, - name => $_->name, - needinfo => _can_needinfo($type, $_) ? 1 : 0 + my $needinfo = _can_needinfo($type, $_, $state); + if ($needinfo) { + $state->{count}++; + $state->{pending}{$_->id} = 1; } - } @mentioned + +{nick => $_->nick, name => $_->name, needinfo => $needinfo}; + } @$mentioned ]; } diff --git a/extensions/Needinfo/lib/API/V1/Mentions.pm b/extensions/Needinfo/lib/API/V1/Mentions.pm index 4f39fe1de5..01224799e7 100644 --- a/extensions/Needinfo/lib/API/V1/Mentions.pm +++ b/extensions/Needinfo/lib/API/V1/Mentions.pm @@ -32,7 +32,8 @@ sub setup_routes { # 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. +# 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); diff --git a/extensions/Needinfo/t/mentions.t b/extensions/Needinfo/t/mentions.t index cf502f4859..365d583dfd 100644 --- a/extensions/Needinfo/t/mentions.t +++ b/extensions/Needinfo/t/mentions.t @@ -64,7 +64,9 @@ 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 can_see_product { $_[0]{sees_product} // 1 } - sub needinfo_blocked { $_[0]{blocked} // 0 } + sub needinfo_blocked { $_[0]{blocked} // 0 } + sub can_set_flag { $_[0]{can_set} // 1 } + sub can_change_flag { $_[0]{can_request} // 1 } package FakeBug; sub new { @@ -102,6 +104,8 @@ 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 } package FakeFlag; sub status { $_[0]{status} } @@ -194,6 +198,20 @@ 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('@boom @alice'); ok(lives { $process->($bug) }, 'a failing CC (e.g. strict_isolation) does not throw'); @@ -319,6 +337,47 @@ is( '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}; + +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'); @@ -332,9 +391,16 @@ is($effects->({thetext => "`\@alice`\n> \@bob"}, {bug => FakeBug->new('')}), ); } -# The initial comment of a bug being filed: there is no bug to check -# visibility against, so the product and the new bug's groups are used. -my $needinfo_types = [bless({multiplicable => 1}, 'FakeType')]; +# 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 => [ @@ -344,7 +410,7 @@ my $mock_flagtype = mock 'Bugzilla::FlagType' => ( my $product = FakeProduct->new(valid => [$security]); is( - $effects->({thetext => '@alice @hidden @me'}, {product => $product}), + $new_bug_effects->({thetext => '@alice @hidden @me'}, {product => $product}), [ {nick => 'alice', name => 'Alice', needinfo => 1}, {nick => 'hidden', name => 'Hidden', needinfo => 1}, @@ -356,14 +422,14 @@ is($type_criteria{component_id}, undef, '... of the product when no component'); $users{alice}{sees_product} = 0; is( - $effects->({thetext => '@alice @bob'}, {product => $product}), + $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( - $effects->( + $new_bug_effects->( {thetext => '@alice @bob'}, {product => $product, groups => ['core-security', 'not-valid-here']} ), @@ -372,7 +438,7 @@ is( ); is( - $effects->( + $new_bug_effects->( {thetext => '@alice @bob'}, {product => FakeProduct->new(mandatory => [$security])} ), @@ -381,7 +447,7 @@ is( ); is( - $effects->( + $new_bug_effects->( {thetext => '@alice'}, {product => $product, component => FakeGroup->new(7, 'General')} ), @@ -392,7 +458,7 @@ is($type_criteria{component_id}, 7, '... uses that component\'s flag types'); $users{alice}{blocked} = 1; is( - $effects->({thetext => '@alice'}, {product => $product}), + $new_bug_effects->({thetext => '@alice'}, {product => $product}), [{nick => 'alice', name => 'Alice', needinfo => 0}], 'new bug: a user blocking needinfo is only CCd' ); @@ -400,19 +466,19 @@ $users{alice}{blocked} = 0; $needinfo_types = []; is( - $effects->({thetext => '@alice'}, {product => $product}), + $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($effects->({thetext => '@alice'}, {product => $product}), +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($effects->({thetext => '@alice'}, {product => $product}), +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'); diff --git a/qa/t/rest_needinfo_mentions.t b/qa/t/rest_needinfo_mentions.t index e38ccd4f5f..3023334d9a 100644 --- a/qa/t/rest_needinfo_mentions.t +++ b/qa/t/rest_needinfo_mentions.t @@ -84,7 +84,8 @@ $t->post_ok($endpoint => $no_privs => json => 'nobody is needinfod by a user without editbugs' ); -# The initial comment of a bug being filed. +# 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?", @@ -93,14 +94,14 @@ $t->post_ok( groups => [], } )->status_is(200)->json_is( - '/mentions' => $expected, - 'new bug: the mentioned user would be needinfod' + '/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' => $expected, 'new bug: the component is optional'); + ->json_is('/mentions' => [], 'new bug: the component is optional'); # Errors. $t->post_ok($endpoint => $admin => json => {text => '@admin'}) From 24772cde675978c3045e607f023805f81b7d80d6 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 4 Nov 2026 14:54:32 -0500 Subject: [PATCH 3/4] fix: Bug 2085141 - Address second round of Copilot review on @mention warning - Treat an inactive needinfo type as CC-only, since core won't add new flags of an inactive type. - With strict_isolation on, skip mentioned users who can't edit the product (add_cc would reject them) in the shared mention filter, so the warning neither lists them nor gives them a non-multiplicable needinfo slot. --- extensions/Needinfo/Extension.pm | 16 ++++++---- extensions/Needinfo/t/mentions.t | 54 ++++++++++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/extensions/Needinfo/Extension.pm b/extensions/Needinfo/Extension.pm index e5dcc7d38b..1e01ea20c2 100644 --- a/extensions/Needinfo/Extension.pm +++ b/extensions/Needinfo/Extension.pm @@ -355,7 +355,8 @@ sub _added_groups { # 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. +# 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}; @@ -377,11 +378,13 @@ sub _filter_mentioned_users { # Checked after parsing so updates without mentions skip the group lookup. my $user = Bugzilla->user; 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) + && (!$strict_isolation || $mentioned->can_edit_product($product_id)) && $can_see->($mentioned) } _users_for_mentions(@nicks); } @@ -414,10 +417,10 @@ sub _process_mentions { 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) { @@ -465,7 +468,8 @@ sub _needinfo_state { sub _can_needinfo { my ($type, $mentioned, $state) = @_; return 0 if !$type || $mentioned->needinfo_blocked; - return 0 unless $type->is_requestable && $type->is_requesteeble; + 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); diff --git a/extensions/Needinfo/t/mentions.t b/extensions/Needinfo/t/mentions.t index 365d583dfd..cdec447b88 100644 --- a/extensions/Needinfo/t/mentions.t +++ b/extensions/Needinfo/t/mentions.t @@ -67,6 +67,7 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); 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 { @@ -106,6 +107,7 @@ is([$extract->("~~~\n\@unclosed\n\@more")], [], 'unclosed fence runs to end'); 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} } @@ -212,9 +214,25 @@ 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'); @@ -370,6 +388,28 @@ is( ); 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)}), @@ -464,6 +504,16 @@ is( ); $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}), From 5da2615fb32e2014d35fda2d2480d9b9d585836f Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 4 Nov 2026 17:16:24 -0500 Subject: [PATCH 4/4] fix: Bug 2085141 - Clear the @mention warning when a comment box is cleared by script Setting a textarea's value doesn't fire an input event, so the live @mention warning stayed visible (and a pending check could still show it) after the attachment overlay cleared its comment on switching attachments, and after an instant update cleared the main comment box. Dispatch an input event in both places, as bug_modal.js already does when it restores a saved comment. --- extensions/BugModal/web/attachments_overlay.js | 2 ++ extensions/BugModal/web/comments.js | 2 ++ 2 files changed, 4 insertions(+) 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();