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

Bug 1508201 - Allow bulk edits to happen in the background, asynchronously #973

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Bugzilla/Install.pm
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,10 @@ use constant SYSTEM_GROUPS => (
name => 'bz_can_disable_mfa',
description => 'Can disable MFA when editing users',
},
{
name => 'bz_can_async_bulk_edit',
description => 'Can asynchronously bulk edit bugs',
},
);

use constant DEFAULT_CLASSIFICATION =>
Expand Down
14 changes: 14 additions & 0 deletions Bugzilla/Object/Lazy.pm
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
# 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::Object::Lazy;
use 5.10.1;
use Moo;

has 'id' => (is => 'ro', required => 1);

1;
6 changes: 6 additions & 0 deletions Bugzilla/Task.pm
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,10 @@ sub _build_name {
return decamelize($class);
}

sub proper_name {
my ($self) = @_;

return join(' ', map { ucfirst } split(/_/, $self->name));
}

1;
104 changes: 104 additions & 0 deletions Bugzilla/Task/BulkEdit.pm
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
# 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::Task::BulkEdit;
use 5.10.1;
use Moo;

use Bugzilla::Error;
use DateTime::Duration;
use List::Util qw(any);
use Try::Tiny;
use Type::Utils qw(duck_type);
use Types::Standard -types;

with 'Bugzilla::Task';

has 'ids' => (is => 'ro', isa => ArrayRef [Int], required => 1);
has 'set_all' => (is => 'ro', isa => HashRef, required => 1);
has 'ids_with_ts' => (is => 'lazy', isa => ArrayRef [Tuple [Int, Str]]);

sub subject {
my ($self) = @_;
my @ids = @{$self->ids};

if (@ids > 100) {
return "Bulk Edit " . scalar(@ids) . " bugs";
}
else {
return "Bulk Edit " . join(", ", @ids);
}
}

sub _build_estimated_duration {
my ($self) = @_;

return DateTime::Duration->new(seconds => 0 + @{$self->ids});
}

sub prepare {
my ($self) = @_;

# pickup timestamps
$self->ids_with_ts;
}

sub _build_ids_with_ts {
my ($self) = @_;
my $dbh = Bugzilla->dbh;

return [] if @{$self->ids} == 0;
return $dbh->selectall_arrayref(
"SELECT bug_id, delta_ts FROM bugs WHERE @{[$dbh->sql_in('bug_id', $self->ids)]}"
);
}

sub run {
my ($self) = @_;

return {async_bulk_edit => 1, all_sent_changes => [map { $self->edit_bug(@$_) } @{$self->ids_with_ts}]};
}

sub edit_bug {
my ($self, $bug_id, $delta_ts) = @_;
my $result;
try {
my $bug = Bugzilla::Bug->check($bug_id);
ThrowUserError('bulk_edit_stale', {bug => $bug, expected_delta_ts => $delta_ts})
unless $bug->delta_ts eq $delta_ts;
ThrowUserError('product_edit_denied', {product => $bug->product})
unless $self->user->can_edit_product($bug->product_obj->id);

my $set_all_fields = $self->set_all;

# Don't blindly ask to remove unchecked groups available in the UI.
# A group can be already unchecked, and the user didn't try to remove it.
# In this case, we don't want remove_group() to complain.
my @remove_groups;
my @unchecked_groups = @{$set_all_fields->{groups}{remove} // []};

foreach my $group (@{$bug->groups_in}) {
push(@remove_groups, $group->name)
if any { $_ eq $group->name } @unchecked_groups;
}

local $set_all_fields->{groups}->{remove} = \@remove_groups;
$bug->set_all($set_all_fields);
my $changes = $bug->update();
$result = $bug->send_changes($changes);
}
catch {
$result = { bug_id => $bug_id, error => $_ };
}
finally {
Bugzilla::Bug->CLEANUP();
};

return $result;
}

1;
67 changes: 55 additions & 12 deletions process_bug.cgi
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ use Bugzilla::Flag;
use Bugzilla::Status;
use Bugzilla::Token;
use Bugzilla::Hook;
use Bugzilla::Task::BulkEdit;

use Scalar::Util qw(blessed);
use List::MoreUtils qw(firstidx);
Expand Down Expand Up @@ -73,23 +74,36 @@ if (Bugzilla->params->{disable_bug_updates}) {

# Create a list of objects for all bugs being modified in this request.
my @bug_objects;
my @bug_ids;

# $async_bulk_edit is whether or not we use the bulk editing code,
# $background is if the background=1 parameter was passed.
my $async_bulk_edit = $cgi->param('async_bulk_edit') // 0;
$cgi->delete('async_bulk_edit') if $async_bulk_edit;

if (defined $cgi->param('id')) {
my $bug = Bugzilla::Bug->check(scalar $cgi->param('id'));
$cgi->param('id', $bug->id);
push(@bug_objects, $bug);
}
else {
foreach my $i ($cgi->param()) {
if ($i =~ /^id_([1-9][0-9]*)/) {
my $id = $1;
push(@bug_objects, Bugzilla::Bug->check($id));
}
@bug_ids = map { /^id_([1-9][0-9]*)/ ? $1 : () } $cgi->param;

# Make sure there are bugs to process.
ThrowUserError("no_bugs_chosen", {action => 'modify'}) unless @bug_ids;
$async_bulk_edit = 1 if @bug_ids > 10;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just noticed this. Seems that even if the user does not check the bulk edit checkbox on buglist.cgi, it will still enable it if @bug_ids > 10 which negates the need for a checkbox altogether.

Maybe this should be instead:

$async_bulk_edit = 0 if @bug_ids < 10;

Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right now, doing > 10 edits is likely to fail, so it's meant to make them more likely to succeed. Eventually bz_async_bulk_edit should be the same as editbugs. Does that make sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds fine. But is still sort of misleading from a user standpoint. Maybe we should add some text or tooltip that states it is automatic if more than 10 bugs?

$async_bulk_edit = 0 unless $user->in_group('bz_can_async_bulk_edit');

if ($async_bulk_edit) {
# Not sure if we need $first_bug at all during a bulk update,
# but it won't hurt.
@bug_objects = (Bugzilla::Bug->check($bug_ids[0]));
}
else {
@bug_objects = map { Bugzilla::Bug->check($_) } @bug_ids;
}
}

# Make sure there are bugs to process.
scalar(@bug_objects) || ThrowUserError("no_bugs_chosen", {action => 'modify'});

my $first_bug = $bug_objects[0]; # Used when we're only updating a single bug.

# Delete any parameter set to 'dontchange'.
Expand Down Expand Up @@ -226,9 +240,12 @@ else {

# For each bug, we have to check if the user can edit the bug the product
# is currently in, before we allow them to change anything.
foreach my $bug (@bug_objects) {
if (!Bugzilla->user->can_edit_product($bug->product_obj->id)) {
ThrowUserError("product_edit_denied", {product => $bug->product});
# For bulk edits, this will be done in the background task.
if (!$async_bulk_edit) {
foreach my $bug (@bug_objects) {
if (!Bugzilla->user->can_edit_product($bug->product_obj->id)) {
ThrowUserError("product_edit_denied", {product => $bug->product});
}
}
}

Expand All @@ -255,7 +272,14 @@ my %field_translation = (
confirm_product_change => 'product_change_confirmed',
);

my %set_all_fields = (other_bugs => \@bug_objects);
my %set_all_fields;
if (not $async_bulk_edit) {
$set_all_fields{other_bugs} = \@bug_objects;
}
else {
require Bugzilla::Object::Lazy;
$set_all_fields{other_bugs} = [ map { Bugzilla::Object::Lazy->new(id => $_) } @bug_ids ];
}
foreach my $field_name (@set_fields) {
if (should_set($field_name, 1)) {
my $param_name = $field_translation{$field_name} || $field_name;
Expand All @@ -278,6 +302,7 @@ if (should_set('comment')) {
is_markdown => Bugzilla->params->{use_markdown} ? 1 : 0,
};
}

if (should_set('see_also')) {
$set_all_fields{'see_also'}->{add} = [split(/[\s,]+/, $cgi->param('see_also'))];
}
Expand All @@ -286,6 +311,7 @@ if (should_set('remove_see_also')) {
}
foreach my $dep_field (qw(dependson blocked)) {
if (should_set($dep_field)) {

if (my $dep_action = $cgi->param("${dep_field}_action")) {
$set_all_fields{$dep_field}->{$dep_action}
= [split(/[\s,]+/, $cgi->param($dep_field))];
Expand Down Expand Up @@ -369,6 +395,23 @@ foreach my $field (keys %$user_match_fields) {

# We are going to alter the list of removed groups, so we keep a copy here.
my @unchecked_groups = @$removed_groups;

if ($async_bulk_edit) {
my $task = Bugzilla::Task::BulkEdit->new(
ids => \@bug_ids,
set_all => \%set_all_fields,
user => Bugzilla->user,
);
Bugzilla->job_queue->run_task($task);
my $name = $task->name;
# Delete the session token used for the mass-change.
delete_token($token) unless $cgi->param('id');
print $cgi->header();
$template->process("task/created.html.tmpl", {task => $task})
or ThrowTemplateError($template->error);
exit;
}

foreach my $b (@bug_objects) {

# Don't blindly ask to remove unchecked groups available in the UI.
Expand Down
71 changes: 71 additions & 0 deletions t/bulk-edit.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
#!/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.
use strict;
use warnings;
use 5.10.1;
use lib qw( . lib local/lib/perl5 );
use Storable qw(freeze);

# this provides a default urlbase.
# Most localconfig options the other Bugzilla::Test::Mock* modules take care for us.
use Bugzilla::Test::MockLocalconfig (urlbase => 'http://bmo-web.vm');

use Bugzilla::Test::MockParams ( antispam_multi_user_limit_age => 0);

# This configures an in-memory sqlite database.
use Bugzilla::Test::MockDB;

# Util provides a few functions more making mock data in the DB.
use Bugzilla::Test::Util qw(create_user create_bug );

use Test2::V0;
use Test2::Tools::Mock;

use ok 'Bugzilla::Task::BulkEdit';

my $user = create_user('[email protected]', '*');
Bugzilla->set_user($user);

my @bug_ids;
foreach (1..100) {
my $bug = create_bug(
short_desc => "a bug",
comment => "this is a bug",
assigned_to => scalar $user->login,
);
push @bug_ids, $bug->id;
}

is(0+@bug_ids, 100, "made 100 bugs");

my $task = Bugzilla::Task::BulkEdit->new(
user => $user,
ids => \@bug_ids,
set_all => {
comment => {
body => "bunnies",
is_private => 0,
},
}
);
$task->prepare;

try_ok {
local $Storable::Deparse = 0;
freeze($task);
} "Can we store the bulk edit task?";

my $results = $task->run;
if (my $edits = $results->{edits}) {
is(0 + @$edits, 100);
}

my $comments = Bugzilla::Bug->check($bug_ids[-1])->comments;
is(0 + @$comments, 2);

done_testing;
10 changes: 8 additions & 2 deletions template/en/default/bug/process/bugmail.html.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@
%]
[% recipient_count = sent_bugmail.sent.size %]

[% IF async_bulk_edit %]
[% show_recipients = 1 %]
[% ELSE %]
<script [% script_nonce FILTER none %]>
function toggleBugmailRecipients(bug_id, show) {
if (show) {
Expand All @@ -56,6 +59,7 @@
});
});
</script>
[% END %]

<dl id="bugmail_summary_[% mailing_bugid FILTER none %]"
[% IF !show_recipients %]style="display:none;"[% END %]>
Expand All @@ -69,8 +73,10 @@
[% ELSE %]
no one
[% END %]
(<a href="#" class="toggleBugmailRecipients" data-mailing-bugid="[% mailing_bugid FILTER html %]"
data-mailing-show="false">hide</a>)
[% IF NOT async_bulk_edit %]
(<a href="#" class="toggleBugmailRecipients" data-mailing-bugid="[% mailing_bugid FILTER html %]"
data-mailing-show="false">hide</a>)
[% END %]
[% ELSE %]
(list of e-mails not available)
[% END %]
Expand Down
9 changes: 7 additions & 2 deletions template/en/default/bug/process/results.html.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@

[% DEFAULT type="bug" %]

[% Link = BLOCK %][% "$terms.Bug $id" FILTER bug_link(id) %][% END %]
[% link = BLOCK %][% "$terms.bug $id" FILTER bug_link(id) %][% END %]
[% IF async_bulk_edit %]
[% Link = BLOCK %][% "$terms.Bug $id" FILTER bug_link(id, full_url => 1) FILTER none %][% END %]
[% link = BLOCK %][% "$terms.bug $id" FILTER bug_link(id, full_url => 1) FILTER none %][% END %]
[% ELSE %]
[% Link = BLOCK %][% "$terms.Bug $id" FILTER bug_link(id) %][% END %]
[% link = BLOCK %][% "$terms.bug $id" FILTER bug_link(id) %][% END %]
[% END %]

[%
title = {
Expand Down
Loading