Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow batch-commenting on test results overview #5509

Merged
merged 3 commits into from Mar 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 37 additions & 10 deletions assets/javascripts/audit_log.js
Expand Up @@ -49,6 +49,35 @@ function getURLForType(type, event_data) {
}
}

function undoComments(undoButton) {
const ids = undoButton.dataset.ids.split(',');
if (!window.confirm(`Do you really want to delete the ${ids.length} comment(s)?`)) {
return;
}
undoButton.style.display = 'none';
$.ajax({
url: urlWithBase('/api/v1/comments'),
method: 'DELETE',
data: ids.map(id => `id=${id}`).join('&'),
success: () => addFlash('info', 'The coments have been deleted.'),
error: (jqXHR, textStatus, errorThrown) => {
undoButton.style.display = 'inline';
addFlash('danger', 'The comments could not be deleted: ' + getXhrError(jqXHR, textStatus, errorThrown));
}
});
}

function getElementForEventType(type, eventData) {
if (type !== 'comments_create') {
return '';
}
const ids = (eventData?.created || [])
.map(data => parseInt(data?.id))
.filter(Number.isInteger)
.join(',');
return `<br><button class="btn btn-danger undo-event" style="float: right" data-ids="${ids}" onclick="undoComments(this)">Undo</button>`;
}

function loadAuditLogTable() {
$('#audit_log_table').DataTable({
lengthMenu: [20, 40, 100],
Expand Down Expand Up @@ -109,19 +138,17 @@ function loadAuditLogTable() {
width: '70%',
render: function (data, type, row) {
if (type === 'display' && data) {
var parsed_data;
let parsedData;
let typeSpecificElement = '';
try {
parsed_data = JSON.stringify(JSON.parse(data), null, 2);
const eventData = JSON.parse(data);
parsedData = JSON.stringify(eventData, null, 2);
typeSpecificElement = getElementForEventType(row.event, eventData);
} catch (e) {
parsed_data = data;
parsedData = data;
}
return (
'<span class="audit_event_data" title="' +
htmlEscape(parsed_data) +
'">' +
htmlEscape(parsed_data) +
'</span>'
);
const escapedData = htmlEscape(parsedData);
return `${typeSpecificElement}<span class="audit_event_data" title="${escapedData}">${escapedData}</span>`;
} else {
return data;
}
Expand Down
33 changes: 33 additions & 0 deletions assets/javascripts/overview.js
Expand Up @@ -281,3 +281,36 @@ function changeClassOfDependencyJob(array, className, add) {
add ? classList.add(className) : classList.remove(className);
}
}

function showBatchCommentingDialog() {
$('#batch-commenting-modal').modal();
}

function addBatchComment(form) {
const text = form.elements.text.value;
if (!text.length) {
return window.alert("The comment text mustn't be empty.");
}
const progressIndication = document.getElementById('batch-commenting-progress-indication');
const controls = document.getElementById('batch-commenting-controls');
progressIndication.style.display = 'flex';
controls.style.display = 'none';
const done = () => {
progressIndication.style.display = 'none';
controls.style.display = 'inline';
$('#batch-commenting-modal').modal('hide');
};
$.ajax({
url: form.action,
method: 'POST',
data: $(form).serialize(),
success: response => {
done();
addFlash('info', 'The comments have been created.');
},
error: (jqXHR, textStatus, errorThrown) => {
done();
addFlash('danger', 'The comments could not be added: ' + getXhrError(jqXHR, textStatus, errorThrown));
}
});
}
33 changes: 14 additions & 19 deletions assets/stylesheets/comments.scss
@@ -1,3 +1,17 @@
.trigger-edit-button, .remove-edit-button {
float: right;
margin-top: -2px;
margin-left: 4px;
border-radius: 50rem;
width: 2.2rem;
height: 2.2rem;
opacity: 0.6;

&:hover {
opacity: 1;
}
}

.comment-body {
.comment-editing-control {
display: none;
Expand All @@ -11,25 +25,6 @@
color: #000;
}

.comment-buttons {
margin-top: -10px;
margin-bottom: 15px;
}

Comment on lines -14 to -18
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this class doesn't seem to be use anywhere and the comment buttons still look good without it. So the removal is intentional.

.trigger-edit-button, .remove-edit-button {
float: right;
margin-top: -2px;
margin-left: 4px;
border-radius: 50rem;
width: 2.2rem;
height: 2.2rem;
opacity: 0.6;

&:hover {
opacity: 1;
}
}

.comment-anchor {
color: #444;
cursor: pointer;
Expand Down
2 changes: 2 additions & 0 deletions lib/OpenQA/WebAPI.pm
Expand Up @@ -501,6 +501,8 @@ sub startup ($self) {
->name('apiv1_put_parent_group_comment')->to('comment#update');
$api_ra->delete('/parent_groups/<parent_group_id:num>/comments/<comment_id:num>')
->name('apiv1_delete_parent_group_comment')->to('comment#delete');
$api_ru->post('/comments')->name('apiv1_post_comments')->to('comment#create_many');
okurz marked this conversation as resolved.
Show resolved Hide resolved
foursixnine marked this conversation as resolved.
Show resolved Hide resolved
$api_ra->delete('/comments')->name('apiv1_delete_comments')->to('comment#delete_many');

$api_ra->delete('/user/<id:num>')->name('apiv1_delete_user')->to('user#delete');

Expand Down
83 changes: 83 additions & 0 deletions lib/OpenQA/WebAPI/Controller/API/V1/Comment.pm
Expand Up @@ -147,6 +147,60 @@ sub create ($self) {

=over 4

=item create_many()

Adds new comments to the specified jobs. Returns 200 if all comments have been
created or 400 if not all comments could be created. Returns a JSON object with
the created and failed comment IDs or an error message in case a fatal error
occurred.

All comments will have the same text which is passed via the mandatory C<text>
parameter.

At this point only job comments are supported. The job IDs are specified by
passing one or more C<job_id> parameters.

=back

=cut

sub create_many ($self) {
my $validation = $self->validation;
$validation->required('text')->like(qr/^(?!\s*$).+/);
$validation->required('job_id')->num(0, undef);
return $self->reply->validation_error({format => 'json'}) if $validation->has_error;

my $text = $validation->param('text');
my $job_ids = $validation->every_param('job_id');
my $schema = $self->schema;
my $comments = $schema->resultset('Comments');
my (@created, @failed);
for my $job_id (@$job_ids) {
my $txn_guard = $schema->txn_scope_guard;
eval {
my $comment = $comments->create(
{
job_id => $job_id,
text => href_to_bugref($text),
user_id => $self->current_user->id
});
$comment->handle_special_contents($self);
$txn_guard->commit;
push @created, $comment->event_data;
};
push @failed, {job_id => $job_id} if $@;
}

# create a single event containing all relevant IDs for this action
my %res = (created => \@created, failed => \@failed);
$self->emit_event('openqa_comments_create', \%res);

$res{error} = 'Not all comments could be created.' if @failed;
$self->render(json => \%res, status => (@failed ? 400 : 200));
}

=over 4

=item update()

Updates an existing comment specified by job/group id and comment id. An update text argument
Expand Down Expand Up @@ -207,4 +261,33 @@ sub delete ($self) {
$self->render(json => {id => $res->id});
}

=over 4

=item delete_many()

Deletes multiple comments by their IDs which are specified by passing one or
more C<id> parameters. Returns a JSON object with the number of deleted comments
or an error message in case a fatal error occurred. Returns a 200 code when all
comments have been deleted and a 400 code if not all comments could be deleted.

=back

=cut

sub delete_many ($self) {
my $validation = $self->validation;
$validation->required('id')->num(0, undef);
return $self->reply->validation_error({format => 'json'}) if $validation->has_error;

my $ids = $validation->every_param('id');
my $comments = $self->schema->resultset('Comments');
my $deleted_rows = $comments->search({id => {-in => $ids}, text => {-not_like => '%label:force_result:%'}})->delete;
my $ok = $deleted_rows && $deleted_rows == scalar(@$ids);

my %res = (ids => $ids, deleted => int($deleted_rows));
$self->emit_event('openqa_comments_delete', \%res) if $deleted_rows;
$res{error} = 'Not all comments could be deleted.' unless $ok;
$self->render(json => \%res, status => ($ok ? 200 : 400));
}

1;
8 changes: 6 additions & 2 deletions lib/OpenQA/WebAPI/Controller/Test.pm
Expand Up @@ -624,6 +624,7 @@ sub _calculate_preferred_machines {
sub _prepare_job_results ($self, $all_jobs, $limit) {
my %archs;
my %results;
my @job_ids;
my $aggregated = {
none => 0,
passed => 0,
Expand Down Expand Up @@ -715,11 +716,14 @@ sub _prepare_job_results ($self, $all_jobs, $limit) {
$results{$distri}{$version}{$flavor}{$test} //= {};
$results{$distri}{$version}{$flavor}{$test}{$arch} = $result;

# populate job IDs for batch-commenting
push @job_ids, $id;

# add description
my $description = $settings_by_job_id{$id}->{JOB_DESCRIPTION} // $descriptions{$test_suite_names{$id}};
$results{$distri}{$version}{$flavor}{$test}{description} //= $description;
}
return ($limit_exceeded, \%archs, \%results, $aggregated);
return ($limit_exceeded, \%archs, \%results, \@job_ids, $aggregated);
}

sub _prepare_groupids ($self) {
Expand Down Expand Up @@ -805,7 +809,7 @@ sub overview {
my @jobs = $self->schema->resultset('Jobs')->complex_query(%$search_args)->latest_jobs($until);

my $limit = $config->{misc_limits}->{tests_overview_max_jobs};
(my $limit_exceeded, $stash{archs}, $stash{results}, $stash{aggregated})
(my $limit_exceeded, $stash{archs}, $stash{results}, $stash{job_ids}, $stash{aggregated})
= $self->_prepare_job_results(\@jobs, $limit);

# determine distri/version from job results if not explicitly specified via search args
Expand Down
2 changes: 1 addition & 1 deletion lib/OpenQA/WebAPI/Plugin/AuditLog.pm
Expand Up @@ -14,7 +14,7 @@ my @job_events = qw(job_create job_delete job_cancel job_restart jobs_restart jo
my @jobgroup_events = qw(jobgroup_create jobgroup_update jobgroup_delete jobgroup_connect);
my @jobtemplate_events = qw(jobtemplate_create jobtemplate_delete);
my @user_events = qw(user_update user_login user_deleted);
my @comment_events = qw(comment_create comment_update comment_delete);
my @comment_events = qw(comment_create comments_create comment_update comment_delete);
my @asset_events = qw(asset_register asset_delete);
my @iso_events = qw(iso_create iso_delete iso_cancel);
my @worker_events = qw(command_enqueue worker_register worker_delete);
Expand Down
38 changes: 36 additions & 2 deletions t/api/09-comments.t
Expand Up @@ -17,6 +17,7 @@ use Mojo::IOLoop;

OpenQA::Test::Case->new->init_data(fixtures_glob => '01-jobs.pl 03-users.pl');
my $t = client(Test::Mojo->new('OpenQA::WebAPI'));
my $comments = $t->app->schema->resultset('Comments');

# create a parent group
$t->app->schema->resultset('JobGroupParents')->create({id => 1, name => 'Test parent', sort_order => 0});
Expand Down Expand Up @@ -156,6 +157,18 @@ subtest 'parent group comments' => sub {
test_comments(parent_groups => 1);
};

subtest 'create many comments' => sub {
$t->post_ok('/api/v1/comments?job_id=42&job_id=foo')->status_is(400);
$t->json_is('/error' => 'Erroneous parameters (job_id invalid, text missing)');
$t->post_ok('/api/v1/comments?job_id=42&job_id=80000&text=batch-comment')->status_is(400);
$t->json_is('/error' => 'Not all comments could be created.', 'error returned');
$t->json_is('/failed' => [{job_id => 42}], 'failed IDs returned');
$t->json_is('/created' => [{job_id => 80000, id => 5}], 'created comment returned');
ok my $comment = $comments->find(5), 'comment created' or return;
is $comment->text, 'batch-comment', 'comment has expected text';
$t->post_ok('/api/v1/comments?job_id=80000&text=batch-comment-2')->status_is(200);
};

subtest 'server-side limit has precedence over user-specified limit' => sub {
$t->app->schema->txn_begin;

Expand Down Expand Up @@ -205,6 +218,24 @@ subtest 'admin can delete comments' => sub {
};
};

$t->app->schema->txn_begin;

subtest 'delete many comments' => sub {
$t->delete_ok('/api/v1/comments?id=42&id=foo')->status_is(400);
$t->json_is('/error' => 'Erroneous parameters (id invalid)');
$t->delete_ok('/api/v1/comments?id=5')->status_is(200, '200 returned if all comments deleted');
$t->json_is('/deleted' => 1, 'one comment deleted');
ok !$comments->find(5), 'comment is gone';
$comments->create({id => 5, user_id => 1, text => 'foo label:force_result:softfailed'});
$t->delete_ok('/api/v1/comments?id=5')->status_is(400, '400 returned if some comments preserved');
$t->json_is('/deleted' => 0, 'no comments deleted');
ok $comments->find(5), 'comment with "force_result"-label not deleted';
$t->delete_ok('/api/v1/comments?id=98765&id=1&id=2')->status_is(400, '400 returned if some comments did not exist');
$t->json_is('/deleted' => 2, 'two out of three comments deleted');
};

$t->app->schema->txn_rollback;

subtest 'can not edit comment by other user' => sub {
$t->put_ok("/api/v1/jobs/99981/comments/1" => form => {text => $edited_test_message . $another_test_message})
->status_is(403, 'editing comments by other users is forbidden');
Expand All @@ -217,10 +248,10 @@ subtest 'can update job result with special label comment' => sub {
my $jobs = $schema->resultset('Jobs');
my $events = $schema->resultset('AuditEvents');
is $jobs->find($job_id)->result, 'failed', 'job initially is failed';
is $events->all, 9, 'only 9 events initially';
is $events->all, 11, '11 events emitted so far';
test_create_comment('jobs', $job_id, 'label:force_result:softfailed:simon_says');
is $jobs->find($job_id)->result, 'softfailed', 'job is updated to softfailed';
is $events->all, 11, 'events for result update emitted';
is $events->all, 13, 'events for result update emitted';
ok $events->find({event => 'job_update_result'}), 'job_update_result event found';
my $route = "/api/v1/jobs/$job_id/comments";
my $comments = $schema->resultset('Comments');
Expand Down Expand Up @@ -270,6 +301,9 @@ subtest 'unauthorized users can only read' => sub {
$t->app($app);
test_get_comment(jobs => 99981, 1, $edited_test_message);
test_get_comment(groups => 1001, 2, $edited_test_message);
$t->post_ok('/api/v1/comments?job_id=80000&text=batch-comment')->status_is(403);
$t->delete_ok('/api/v1/comments?id=1')->status_is(403);
ok $comments->find(1), 'comment still exists';
};

done_testing();