From d50a6b449af491efea79fb31ee818b48d949f6bb Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Fri, 7 Aug 2026 15:58:58 -0400 Subject: [PATCH 1/2] Bug 2061831 - Add additional information to bugzilla comments in REST API if comment edited before --- Bugzilla/WebService/Bug.pm | 67 +++++++++++++ docs/en/rst/api/core/v1/comment.rst | 85 ++++++++++------- qa/t/rest_bug_comment_edits.t | 141 ++++++++++++++++++++++++++++ 3 files changed, 259 insertions(+), 34 deletions(-) create mode 100644 qa/t/rest_bug_comment_edits.t diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm index 902a0bb71e..d1630c6d57 100644 --- a/Bugzilla/WebService/Bug.pm +++ b/Bugzilla/WebService/Bug.pm @@ -356,6 +356,7 @@ sub comments { my $comments = $bug->comments({order => 'oldest_to_newest', after => $params->{new_since} }); + $self->_preload_comment_edit_info($comments); my @result; foreach my $comment (@$comments) { next if $comment->is_private && !$user->is_insider; @@ -381,6 +382,8 @@ sub comments { my %got_bug_ids = map { $_->bug_id => 1 } @$comment_data; Bugzilla::Bug->check($_) foreach (keys %got_bug_ids); + $self->_preload_comment_edit_info($comment_data); + foreach my $comment (@$comment_data) { if ($comment->is_private && !$user->is_insider) { ThrowUserError('comment_is_private', {id => $comment->id}); @@ -411,6 +414,43 @@ sub render_comment { return {html => $html}; } +# Helper for Bug.comments and Bug.get. Fetches the revision metadata (number of +# edits and the timestamp of the most recent edit) for a set of comments using a +# single query, and stashes it on each comment object for _translate_comment. +# +# The data is only exposed to users who are allowed to edit other people's +# comments; everyone else gets no edit_count/last_change_time keys at all. +# Revisions hidden by an edit-comments admin are only counted for admins. +sub _preload_comment_edit_info { + my ($self, $comments) = @_; + my $user = Bugzilla->user; + + return unless @$comments; + + # can_edit_comments is injected by the EditComments extension; without it + # there is no longdescs_activity table to query. + return unless $user->can('can_edit_comments') && $user->can_edit_comments; + + my $dbh = Bugzilla->dbh; + my @ids = map { $_->id } @$comments; + + # Admins can see hidden revisions, so they are counted for them only. + my $hidden_clause = $user->is_edit_comments_admin ? '' : 'AND is_hidden = 0'; + + my $rows = $dbh->selectall_hashref( + 'SELECT comment_id, COUNT(*) AS edit_count, + MAX(change_when) AS last_change_time + FROM longdescs_activity + WHERE ' . $dbh->sql_in('comment_id', \@ids) . " $hidden_clause + GROUP BY comment_id", 'comment_id' + ); + + foreach my $comment (@$comments) { + $comment->{edit_info} = $rows->{$comment->id} + || {edit_count => 0, last_change_time => undef}; + } +} + # Helper for Bug.comments sub _translate_comment { my ($self, $comment, $filters, $types, $prefix) = @_; @@ -430,6 +470,13 @@ sub _translate_comment { count => $self->type('int', $comment->count), }; + # Only set by _preload_comment_edit_info when the user may edit others' comments + if (my $edit_info = $comment->{edit_info}) { + $comment_hash->{edit_count} = $self->type('int', $edit_info->{edit_count}); + $comment_hash->{last_change_time} + = $self->type('dateTime', $edit_info->{last_change_time}); + } + if (Bugzilla->params->{use_comment_reactions}) { $comment_hash->{reactions} = {}; @@ -1600,6 +1647,7 @@ sub _bug_to_hash { my @result; my $comments = $bug->comments({order => 'oldest_to_newest', after => $params->{new_since}}); + $self->_preload_comment_edit_info($comments); foreach my $comment (@$comments) { next if $comment->is_private && !$user->is_insider; push(@result, @@ -2782,6 +2830,23 @@ may be deprecated and removed in a future release. C True if this comment is private (only visible to a certain group called the "insidergroup"), False otherwise. +=item edit_count + +C The number of times this comment has been edited. C<0> if the comment +has never been edited. + +This key is only present for users who are allowed to edit other people's +comments. Revisions that have been hidden by an edit-comments admin are only +counted for members of the edit-comments admins group. + +=item last_change_time + +C The time (in Bugzilla's timezone) of the most recent edit to this +comment, or null if the comment has never been edited. + +This key is only present for users who are allowed to edit other people's +comments, and follows the same rules as C for hidden revisions. + =back =item B @@ -2824,6 +2889,8 @@ C. =item C was added in Bugzilla B<6.0>. +=item C and C were added in Bugzilla B<6.0>. + =back diff --git a/docs/en/rst/api/core/v1/comment.rst b/docs/en/rst/api/core/v1/comment.rst index 915c3124fe..e7709f2572 100644 --- a/docs/en/rst/api/core/v1/comment.rst +++ b/docs/en/rst/api/core/v1/comment.rst @@ -82,40 +82,57 @@ and the value is the comment. (The format of comments is described below.) A "comment" as described above is a object that contains the following items: -============= ======== ======================================================== -name type description -============= ======== ======================================================== -id int The globally unique ID for the comment. -bug_id int The ID of the bug that this comment is on. -attachment_id int If the comment was made on an attachment, this will be - the ID of that attachment. Otherwise it will be null. -count int The number of the comment local to the bug. The - Description is 0, comments start with 1. -text string The body of the comment, including any special text - (such as "this bug was marked as a duplicate of..."). -raw_text string The body of the comment without any special additional - text. -creator string The login name of the comment's author. -time datetime The time (in Bugzilla's timezone) that the comment was - added. -creation_time datetime This is exactly same as the ``time`` key. Use this - field instead of ``time`` for consistency with other - methods including :ref:`rest_single_bug` and - :ref:`rest_attachments`. - - For compatibility, ``time`` is still usable. However, - please note that ``time`` may be deprecated and removed - in a future release. - -is_private boolean ``true`` if this comment is private (only visible to a - certain group called the "insidergroup"), ``false`` - otherwise. -is_markdown boolean ``true`` if this comment is markdown. ``false`` if this - comment is plaintext. -reactions object An object containing reacted emoji names and - corresponding counts. To retrieve reacted users, use - :ref:`rest_get_comment_reactions`. -============= ======== ======================================================== +================ ======== ===================================================== +name type description +================ ======== ===================================================== +id int The globally unique ID for the comment. +bug_id int The ID of the bug that this comment is on. +attachment_id int If the comment was made on an attachment, this will + be the ID of that attachment. Otherwise it will be + null. +count int The number of the comment local to the bug. The + Description is 0, comments start with 1. +text string The body of the comment, including any special text + (such as "this bug was marked as a duplicate of..."). +raw_text string The body of the comment without any special + additional text. +creator string The login name of the comment's author. +time datetime The time (in Bugzilla's timezone) that the comment + was added. +creation_time datetime This is exactly same as the ``time`` key. Use this + field instead of ``time`` for consistency with other + methods including :ref:`rest_single_bug` and + :ref:`rest_attachments`. + + For compatibility, ``time`` is still usable. + However, please note that ``time`` may be deprecated + and removed in a future release. + +is_private boolean ``true`` if this comment is private (only visible to + a certain group called the "insidergroup"), + ``false`` otherwise. +is_markdown boolean ``true`` if this comment is markdown. ``false`` if + this comment is plaintext. +edit_count int The number of times this comment has been edited. + ``0`` if the comment has never been edited. + + Only present for users who are allowed to edit other + people's comments. Revisions hidden by an + edit-comments admin are only counted for members of + the edit-comments admins group. + +last_change_time datetime The time (in Bugzilla's timezone) of the most recent + edit to this comment, or null if the comment has + never been edited. + + Only present for users who are allowed to edit other + people's comments, and follows the same rules as + ``edit_count`` for hidden revisions. + +reactions object An object containing reacted emoji names and + corresponding counts. To retrieve reacted users, use + :ref:`rest_get_comment_reactions`. +================ ======== ===================================================== **Errors** diff --git a/qa/t/rest_bug_comment_edits.t b/qa/t/rest_bug_comment_edits.t new file mode 100644 index 0000000000..9779c66f05 --- /dev/null +++ b/qa/t/rest_bug_comment_edits.t @@ -0,0 +1,141 @@ +#!/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 edit_count and last_change_time comment fields # +# GET /rest/bug//comment # +# GET /rest/bug/comment/ # +############################################################## + +use 5.10.1; +use strict; +use warnings; +use lib qw(lib ../../lib ../../local/lib/perl5); + +use Bugzilla; +use List::Util qw(first); +use QA::Util qw(get_config); +use QA::REST::Util qw(api_headers); + +use Test::Mojo; +use Test::More; + +# REST returns dates in ISO-8601 (with a trailing Z). +use constant DATETIME_REGEX => qr/^\d{4}-\d\d-\d\dT\d\d:\d\d:\d\dZ?$/; + +my $config = get_config(); +my $url = Bugzilla->localconfig->urlbase; + +# edit_comments_group defaults to 'editbugs' and edit_comments_admins_group +# defaults to 'admin', so these three users cover all three visibility cases. +my $admin_key = $config->{admin_user_api_key}; +my $editbugs_key = $config->{editbugs_user_api_key}; +my $unprivileged_key = $config->{unprivileged_user_api_key}; + +my $t = Test::Mojo->new(); +$t->ua->max_redirects(1); + +################################### +# Create the comments to be edited # +################################### + +sub add_comment { + my ($text) = @_; + $t->post_ok($url + . 'rest/bug/public_bug/comment' => {'X-Bugzilla-API-Key' => $editbugs_key} => + json => {comment => $text})->status_is(201); + return $t->tx->res->json->{id}; +} + +my $edited_id = add_comment('comment that will be edited'); +my $unedited_id = add_comment('comment that will never be edited'); + +# Two visible edits by the comment's own author... +foreach my $revision (1, 2) { + $t->put_ok($url + . "rest/editcomments/comment/$edited_id" => + {'X-Bugzilla-API-Key' => $editbugs_key} => json => + {new_comment => "edited text, revision $revision"})->status_is(200); +} + +# ...and one edit whose revision is hidden, which only an edit-comments admin +# may do. +$t->put_ok($url + . "rest/editcomments/comment/$edited_id" => + {'X-Bugzilla-API-Key' => $admin_key} => json => + {new_comment => 'edited text, hidden revision', is_hidden => 1}) + ->status_is(200); + +############################### +# GET /rest/bug/comment/ # +############################### + +sub get_comment { + my ($comment_id, $api_key) = @_; + $t->get_ok($url . "rest/bug/comment/$comment_id" => api_headers($api_key)) + ->status_is(200); + return $t->tx->res->json->{comments}{$comment_id}; +} + +foreach my $case ( + {name => 'logged-out user', key => undef}, + {name => 'unprivileged user', key => $unprivileged_key}, + ) +{ + my $comment = get_comment($edited_id, $case->{key}); + ok(!exists $comment->{edit_count}, "$case->{name} does not get edit_count"); + ok( + !exists $comment->{last_change_time}, + "$case->{name} does not get last_change_time" + ); +} + +# A user who may edit others' comments sees the counts, but hidden revisions +# are not counted for them. +my $comment = get_comment($edited_id, $editbugs_key); +is($comment->{edit_count}, 2, 'editbugs user sees only the visible edits'); +like($comment->{last_change_time}, + DATETIME_REGEX, 'editbugs user gets a well-formed last_change_time'); + +# An edit-comments admin also sees the hidden revision. +$comment = get_comment($edited_id, $admin_key); +is($comment->{edit_count}, 3, 'admin user also sees the hidden edit'); +like($comment->{last_change_time}, + DATETIME_REGEX, 'admin user gets a well-formed last_change_time'); + +# A comment that was never edited reports zero and a null timestamp. +$comment = get_comment($unedited_id, $editbugs_key); +is($comment->{edit_count}, 0, 'unedited comment has an edit_count of 0'); +ok(exists $comment->{last_change_time}, + 'unedited comment still has a last_change_time key'); +is($comment->{last_change_time}, + undef, 'unedited comment has a null last_change_time'); + +################################# +# GET /rest/bug//comment # +################################# + +# The bug-level route uses a separate code path, so check it too. +$t->get_ok($url . 'rest/bug/public_bug/comment' => api_headers($editbugs_key)) + ->status_is(200); +my $bug_comments = (values %{$t->tx->res->json->{bugs}})[0]{comments}; +my $found = first { $_->{id} == $edited_id } @$bug_comments; +ok($found, 'found the edited comment via the bug-level route'); +is($found->{edit_count}, 2, 'bug-level route reports the same edit_count'); +like($found->{last_change_time}, + DATETIME_REGEX, 'bug-level route reports last_change_time'); + +$t->get_ok( + $url . 'rest/bug/public_bug/comment' => api_headers($unprivileged_key)) + ->status_is(200); +$bug_comments = (values %{$t->tx->res->json->{bugs}})[0]{comments}; +$found = first { $_->{id} == $edited_id } @$bug_comments; +ok(!exists $found->{edit_count}, + 'bug-level route omits edit_count for unprivileged users'); + +done_testing(); From 3371a9a4370d67a2ee4b89a4c405ce6952f64890 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sat, 8 Aug 2026 14:59:42 -0400 Subject: [PATCH 2/2] Copilot review fixes --- Bugzilla/WebService/Bug.pm | 46 ++++++++++++++++++++++++++++------- qa/t/rest_bug_comment_edits.t | 37 ++++++++++++++++++++++++++-- 2 files changed, 72 insertions(+), 11 deletions(-) diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm index d1630c6d57..0d3ba14a1c 100644 --- a/Bugzilla/WebService/Bug.pm +++ b/Bugzilla/WebService/Bug.pm @@ -341,6 +341,7 @@ sub comments { } my %bugs; + my @bug_comments; foreach my $bug_id (@$bug_ids) { my $bug; @@ -356,7 +357,15 @@ sub comments { my $comments = $bug->comments({order => 'oldest_to_newest', after => $params->{new_since} }); - $self->_preload_comment_edit_info($comments); + push(@bug_comments, [$bug, $comments]); + } + + # Preload every requested bug's comments in one query, otherwise a bulk + # request would run one aggregation query per bug. + $self->_preload_comment_edit_info([map { @{$_->[1]} } @bug_comments]); + + foreach my $bug_comment (@bug_comments) { + my ($bug, $comments) = @$bug_comment; my @result; foreach my $comment (@$comments) { next if $comment->is_private && !$user->is_insider; @@ -414,25 +423,30 @@ sub render_comment { return {html => $html}; } -# Helper for Bug.comments and Bug.get. Fetches the revision metadata (number of -# edits and the timestamp of the most recent edit) for a set of comments using a -# single query, and stashes it on each comment object for _translate_comment. +# Helper for every method that returns comments. Fetches the revision metadata +# (number of edits and the timestamp of the most recent edit) for a set of +# comments using a single query, and stashes it on each comment object for +# _translate_comment. # # The data is only exposed to users who are allowed to edit other people's # comments; everyone else gets no edit_count/last_change_time keys at all. # Revisions hidden by an edit-comments admin are only counted for admins. +# +# Comments that were already preloaded are skipped, so callers that batch every +# comment of a request up front turn the later per-bug calls into no-ops. sub _preload_comment_edit_info { my ($self, $comments) = @_; my $user = Bugzilla->user; - return unless @$comments; - # can_edit_comments is injected by the EditComments extension; without it # there is no longdescs_activity table to query. return unless $user->can('can_edit_comments') && $user->can_edit_comments; + my @todo = grep { !exists $_->{edit_info} } @$comments; + return unless @todo; + my $dbh = Bugzilla->dbh; - my @ids = map { $_->id } @$comments; + my @ids = map { $_->id } @todo; # Admins can see hidden revisions, so they are counted for them only. my $hidden_clause = $user->is_edit_comments_admin ? '' : 'AND is_hidden = 0'; @@ -445,12 +459,22 @@ sub _preload_comment_edit_info { GROUP BY comment_id", 'comment_id' ); - foreach my $comment (@$comments) { + foreach my $comment (@todo) { $comment->{edit_info} = $rows->{$comment->id} || {edit_count => 0, last_change_time => undef}; } } +# Batch-preloads the comment edit info for every bug about to be passed through +# _bug_to_hash(), so that a multi-bug request runs one aggregation query rather +# than one per bug. +sub _preload_bugs_comment_edit_info { + my ($self, $bugs, $params) = @_; + + return unless filter_wants $params, 'comments', ['extra']; + $self->_preload_comment_edit_info([map { @{$_->comments} } @$bugs]); +} + # Helper for Bug.comments sub _translate_comment { my ($self, $comment, $filters, $types, $prefix) = @_; @@ -528,9 +552,11 @@ sub get { $bug = Bugzilla::Bug->check($bug_id); } push(@bugs, $bug); - push(@hashes, $self->_bug_to_hash($bug, $params)); } + $self->_preload_bugs_comment_edit_info(\@bugs, $params); + @hashes = map { $self->_bug_to_hash($_, $params) } @bugs; + # Set the ETag before inserting the update tokens # since the tokens will always be unique even if # the data has not changed. @@ -759,6 +785,7 @@ sub search { my %bug_objects = map { $_->id => $_ } @{Bugzilla::Bug->new_from_list(\@bug_ids)}; my @bugs = map { $bug_objects{$_} } @bug_ids; + $self->_preload_bugs_comment_edit_info(\@bugs, $params); @bugs = map { $self->_bug_to_hash($_, $params) } @bugs; return {bugs => \@bugs}; @@ -812,6 +839,7 @@ sub possible_duplicates { @$possible_dupes = grep { $_->id != $params->{id} } @$possible_dupes; } + $self->_preload_bugs_comment_edit_info($possible_dupes, $params); my @hashes = map { $self->_bug_to_hash($_, $params) } @$possible_dupes; $self->_add_update_tokens($params, $possible_dupes, \@hashes); return {bugs => \@hashes}; diff --git a/qa/t/rest_bug_comment_edits.t b/qa/t/rest_bug_comment_edits.t index 9779c66f05..77b49a3c4b 100644 --- a/qa/t/rest_bug_comment_edits.t +++ b/qa/t/rest_bug_comment_edits.t @@ -6,11 +6,12 @@ # This Source Code Form is "Incompatible With Secondary Licenses", as # defined by the Mozilla Public License, v. 2.0. -############################################################## +############################################################### # Test for the edit_count and last_change_time comment fields # # GET /rest/bug//comment # # GET /rest/bug/comment/ # -############################################################## +# GET /rest/bug/?include_fields=comments # +############################################################### use 5.10.1; use strict; @@ -138,4 +139,36 @@ $found = first { $_->{id} == $edited_id } @$bug_comments; ok(!exists $found->{edit_count}, 'bug-level route omits edit_count for unprivileged users'); +############################################## +# GET /rest/bug/?include_fields=comments # +############################################## + +# Bug.get renders comments through _bug_to_hash(), which is a third code path. +sub get_bug_comments { + my ($api_key) = @_; + $t->get_ok( + $url . 'rest/bug/public_bug?include_fields=comments' => api_headers($api_key)) + ->status_is(200); + return $t->tx->res->json->{bugs}[0]{comments}; +} + +$bug_comments = get_bug_comments($editbugs_key); +$found = first { $_->{id} == $edited_id } @$bug_comments; +ok($found, 'found the edited comment via Bug.get'); +is($found->{edit_count}, 2, 'Bug.get reports the same edit_count'); +like($found->{last_change_time}, + DATETIME_REGEX, 'Bug.get reports last_change_time'); + +$found = first { $_->{id} == $unedited_id } @$bug_comments; +is($found->{edit_count}, 0, 'Bug.get reports an edit_count of 0 when unedited'); +is($found->{last_change_time}, + undef, 'Bug.get reports a null last_change_time when unedited'); + +$bug_comments = get_bug_comments($unprivileged_key); +$found = first { $_->{id} == $edited_id } @$bug_comments; +ok(!exists $found->{edit_count}, + 'Bug.get omits edit_count for unprivileged users'); +ok(!exists $found->{last_change_time}, + 'Bug.get omits last_change_time for unprivileged users'); + done_testing();