summaryrefslogtreecommitdiffstats
path: root/extensions/Voting
diff options
context:
space:
mode:
authorFrédéric Buclin <LpSolit@gmail.com>2013-09-28 00:37:03 +0200
committerFrédéric Buclin <LpSolit@gmail.com>2013-09-28 00:37:03 +0200
commit48bc6d3981dfedf1183a76f2ccc17d8889edaa56 (patch)
tree0c938bcb0ac1b62ea4db658c727fa5d0b6d51145 /extensions/Voting
parent5d0e6171ebce1410aedb378aa64e65abbcf916eb (diff)
downloadbugzilla-48bc6d3981dfedf1183a76f2ccc17d8889edaa56.tar.gz
bugzilla-48bc6d3981dfedf1183a76f2ccc17d8889edaa56.tar.xz
Bug 851267: Bugzilla times out when a user has several thousands of votes
r=dkl a=justdave
Diffstat (limited to 'extensions/Voting')
-rw-r--r--extensions/Voting/Extension.pm162
-rw-r--r--extensions/Voting/template/en/default/pages/voting/user.html.tmpl19
2 files changed, 89 insertions, 92 deletions
diff --git a/extensions/Voting/Extension.pm b/extensions/Voting/Extension.pm
index 387d44ee0..aa93ec235 100644
--- a/extensions/Voting/Extension.pm
+++ b/extensions/Voting/Extension.pm
@@ -21,7 +21,7 @@ use Bugzilla::User;
use Bugzilla::Util qw(detaint_natural);
use Bugzilla::Token;
-use List::Util qw(min);
+use List::Util qw(min sum);
use constant VERSION => BUGZILLA_VERSION;
use constant DEFAULT_VOTES_PER_BUG => 1;
@@ -187,7 +187,7 @@ sub bug_end_of_update {
# If some votes have been removed, RemoveVotes() returns
# a list of messages to send to voters.
@msgs = _remove_votes($bug->id, 0, 'votes_bug_moved');
- _confirm_if_vote_confirmed($bug->id);
+ _confirm_if_vote_confirmed($bug);
foreach my $msg (@msgs) {
MessageToMTA($msg);
@@ -391,7 +391,7 @@ sub _page_user {
# If a bug_id is given, and we're editing, we'll add it to the votes list.
my $bug_id = $input->{bug_id};
- my $bug = Bugzilla::Bug->check($bug_id) if $bug_id;
+ my $bug = Bugzilla::Bug->check({ id => $bug_id, cache => 1 }) if $bug_id;
my $who_id = $input->{user_id} || $user->id;
# Logged-out users must specify a user_id.
@@ -420,53 +420,38 @@ sub _page_user {
foreach my $product (@{ $user->get_selectable_products }) {
next unless ($product->{votesperuser} > 0);
- my @bugs;
- my @bug_ids;
- my $total = 0;
- my $onevoteonly = 0;
-
my $vote_list =
- $dbh->selectall_arrayref('SELECT votes.bug_id, votes.vote_count,
- bugs.short_desc
- FROM votes
- INNER JOIN bugs
- ON votes.bug_id = bugs.bug_id
- WHERE votes.who = ?
- AND bugs.product_id = ?
- ORDER BY votes.bug_id',
- undef, ($who->id, $product->id));
-
- $user->visible_bugs([map { $_->[0] } @$vote_list]);
- foreach (@$vote_list) {
- my ($id, $count, $summary) = @$_;
- $total += $count;
-
- # Next if user can't see this bug. So, the totals will be correct
- # and they can see there are votes 'missing', but not on what bug
- # they are. This seems a reasonable compromise; the alternative is
- # to lie in the totals.
- next if !$user->can_see_bug($id);
-
- push (@bugs, { id => $id,
- summary => $summary,
- count => $count });
- push (@bug_ids, $id);
- push (@all_bug_ids, $id);
- }
+ $dbh->selectall_arrayref('SELECT votes.bug_id, votes.vote_count
+ FROM votes
+ INNER JOIN bugs
+ ON votes.bug_id = bugs.bug_id
+ WHERE votes.who = ?
+ AND bugs.product_id = ?',
+ undef, ($who->id, $product->id));
+
+ my %votes = map { $_->[0] => $_->[1] } @$vote_list;
+ my @bug_ids = sort keys %votes;
+ # Exclude bugs that the user can no longer see.
+ @bug_ids = @{ $user->visible_bugs(\@bug_ids) };
+ next unless scalar @bug_ids;
+
+ push(@all_bug_ids, @bug_ids);
+ my @bugs = @{ Bugzilla::Bug->new_from_list(\@bug_ids) };
+ $_->{count} = $votes{$_->id} foreach @bugs;
+ # We include votes from bugs that the user can no longer see.
+ my $total = sum(values %votes) || 0;
+ my $onevoteonly = 0;
$onevoteonly = 1 if (min($product->{votesperuser},
$product->{maxvotesperbug}) == 1);
- # Only add the product for display if there are any bugs in it.
- if ($#bugs > -1) {
- push (@products, { name => $product->name,
- bugs => \@bugs,
- bug_ids => \@bug_ids,
- onevoteonly => $onevoteonly,
- total => $total,
- maxvotes => $product->{votesperuser},
- maxperbug => $product->{maxvotesperbug} });
- }
+ push(@products, { name => $product->name,
+ bugs => \@bugs,
+ bug_ids => \@bug_ids,
+ onevoteonly => $onevoteonly,
+ total => $total,
+ maxvotes => $product->{votesperuser},
+ maxperbug => $product->{maxvotesperbug} });
}
if ($canedit && $bug) {
@@ -500,6 +485,7 @@ sub _update_votes {
# IDs and the field values are the number of votes.
my @buglist = grep {/^\d+$/} keys %$input;
+ my (%bugs, %votes);
# If no bugs are in the buglist, let's make sure the user gets notified
# that their votes will get nuked if they continue.
@@ -515,20 +501,23 @@ sub _update_votes {
exit;
}
}
+ else {
+ $user->visible_bugs(\@buglist);
+ my $bugs_obj = Bugzilla::Bug->new_from_list(\@buglist);
+ $bugs{$_->id} = $_ foreach @$bugs_obj;
+ }
- # Call check() on each bug ID to make sure it is a positive
- # integer representing an existing bug that the user is authorized
- # to access, and make sure the number of votes submitted is also
- # a non-negative integer (a series of digits not preceded by a
- # minus sign).
- my (%votes, @bugs);
+ # Call check_is_visible() on each bug to make sure it is an existing bug
+ # that the user is authorized to access, and make sure the number of votes
+ # submitted is also an integer.
foreach my $id (@buglist) {
- my $bug = Bugzilla::Bug->check($id);
- push(@bugs, $bug);
- $id = $bug->id;
- $votes{$id} = $input->{$id};
- detaint_natural($votes{$id})
- || ThrowUserError("voting_must_be_nonnegative");
+ my $bug = $bugs{$id}
+ or ThrowUserError('bug_id_does_not_exist', { bug_id => $id });
+ $bug->check_is_visible;
+ $id = $bug->id;
+ $votes{$id} = $input->{$id};
+ detaint_natural($votes{$id})
+ || ThrowUserError("voting_must_be_nonnegative");
}
my $token = $cgi->param('token');
@@ -541,10 +530,10 @@ sub _update_votes {
# If the user is voting for bugs, make sure they aren't overstuffing
# the ballot box.
- if (scalar @bugs) {
+ if (scalar @buglist) {
my (%prodcount, %products);
- foreach my $bug (@bugs) {
- my $bug_id = $bug->id;
+ foreach my $bug_id (keys %bugs) {
+ my $bug = $bugs{$bug_id};
my $prod = $bug->product;
$products{$prod} ||= $bug->product_obj;
$prodcount{$prod} ||= 0;
@@ -568,56 +557,65 @@ sub _update_votes {
}
}
- # Update the user's votes in the database. If the user did not submit
- # any votes, they may be using a form with checkboxes to remove all their
- # votes (checkboxes are not submitted along with other form data when
- # they are not checked, and Bugzilla uses them to represent single votes
- # for products that only allow one vote per bug). In that case, we still
- # need to clear the user's votes from the database.
- my %affected;
+ # Update the user's votes in the database.
$dbh->bz_start_transaction();
- # Take note of, and delete the user's old votes from the database.
- my $bug_list = $dbh->selectcol_arrayref('SELECT bug_id FROM votes
+ my $old_list = $dbh->selectall_arrayref('SELECT bug_id, vote_count FROM votes
WHERE who = ?', undef, $who);
- foreach my $id (@$bug_list) {
- $affected{$id} = 1;
- }
- $dbh->do('DELETE FROM votes WHERE who = ?', undef, $who);
+ my %old_votes = map { $_->[0] => $_->[1] } @$old_list;
my $sth_insertVotes = $dbh->prepare('INSERT INTO votes (who, bug_id, vote_count)
VALUES (?, ?, ?)');
+ my $sth_updateVotes = $dbh->prepare('UPDATE votes SET vote_count = ?
+ WHERE bug_id = ? AND who = ?');
- # Insert the new values in their place
- foreach my $id (@buglist) {
- if ($votes{$id} > 0) {
+ my %affected = map { $_ => 1 } (@buglist, keys %old_votes);
+ my @deleted_votes;
+
+ foreach my $id (keys %affected) {
+ if (!$votes{$id}) {
+ push(@deleted_votes, $id);
+ next;
+ }
+ if ($votes{$id} == ($old_votes{$id} || 0)) {
+ delete $affected{$id};
+ next;
+ }
+ # We use 'defined' in case 0 was accidentally stored in the DB.
+ if (defined $old_votes{$id}) {
+ $sth_updateVotes->execute($votes{$id}, $id, $who);
+ }
+ else {
$sth_insertVotes->execute($who, $id, $votes{$id});
}
- $affected{$id} = 1;
+ }
+
+ if (@deleted_votes) {
+ $dbh->do('DELETE FROM votes WHERE who = ? AND ' .
+ $dbh->sql_in('bug_id', \@deleted_votes), undef, $who);
}
# Update the cached values in the bugs table
- print $cgi->header();
my @updated_bugs = ();
my $sth_getVotes = $dbh->prepare("SELECT SUM(vote_count) FROM votes
WHERE bug_id = ?");
- my $sth_updateVotes = $dbh->prepare("UPDATE bugs SET votes = ?
- WHERE bug_id = ?");
+ $sth_updateVotes = $dbh->prepare('UPDATE bugs SET votes = ? WHERE bug_id = ?');
foreach my $id (keys %affected) {
$sth_getVotes->execute($id);
my $v = $sth_getVotes->fetchrow_array || 0;
$sth_updateVotes->execute($v, $id);
- my $confirmed = _confirm_if_vote_confirmed($id);
+ my $confirmed = _confirm_if_vote_confirmed($bugs{$id} || $id);
push (@updated_bugs, $id) if $confirmed;
}
$dbh->bz_commit_transaction();
+ print $cgi->header() if scalar @updated_bugs;
$vars->{'type'} = "votes";
$vars->{'title_tag'} = 'change_votes';
foreach my $bug_id (@updated_bugs) {
@@ -828,7 +826,7 @@ sub _remove_votes {
# confirm a bug has been reduced, check if the bug is now confirmed.
sub _confirm_if_vote_confirmed {
my $id = shift;
- my $bug = new Bugzilla::Bug($id);
+ my $bug = ref $id ? $id : new Bugzilla::Bug({ id => $id, cache => 1 });
my $ret = 0;
if (!$bug->everconfirmed
diff --git a/extensions/Voting/template/en/default/pages/voting/user.html.tmpl b/extensions/Voting/template/en/default/pages/voting/user.html.tmpl
index b1cf53a85..b08b4c878 100644
--- a/extensions/Voting/template/en/default/pages/voting/user.html.tmpl
+++ b/extensions/Voting/template/en/default/pages/voting/user.html.tmpl
@@ -85,8 +85,7 @@
</tr>
[% FOREACH bug = product.bugs %]
- <tr [% IF bug.id == this_bug.id && canedit %]
- class="bz_bug_being_voted_on" [% END %]>
+ <tr [% IF bug.id == this_bug.id && canedit %] class="bz_bug_being_voted_on"[% END %]>
<td>
[% IF bug.id == this_bug.id && canedit %]
[% IF product.onevoteonly %]
@@ -96,25 +95,25 @@
[% END %]
[%- END %]
</td>
- <td align="right"><a name="vote_[% bug.id FILTER html %]">
+ <td align="right"><a name="vote_[% bug.id FILTER none %]">
[% IF canedit %]
[% IF product.onevoteonly %]
- <input type="checkbox" name="[% bug.id FILTER html %]" value="1"
- [% " checked" IF bug.count %] id="bug_[% bug.id FILTER html %]">
+ <input type="checkbox" name="[% bug.id FILTER none %]" value="1"
+ [% " checked" IF bug.count %] id="bug_[% bug.id FILTER none %]">
[% ELSE %]
- <input name="[% bug.id FILTER html %]" value="[% bug.count FILTER html %]"
- size="2" id="bug_[% bug.id FILTER html %]">
+ <input name="[% bug.id FILTER none %]" value="[% bug.count FILTER html %]"
+ size="2" id="bug_[% bug.id FILTER none %]">
[% END %]
[% ELSE %]
[% bug.count FILTER html %]
[% END %]
</a></td>
<td align="center">
- [% bug.id FILTER bug_link(bug.id) FILTER none %]
+ [% PROCESS bug/link.html.tmpl bug = bug, link_text = bug.id %]
</td>
<td>
- [% bug.summary FILTER html %]
- (<a href="page.cgi?id=voting/bug.html&amp;bug_id=[% bug.id FILTER uri %]">Show Votes</a>)
+ [% bug.short_desc FILTER html %]
+ (<a href="page.cgi?id=voting/bug.html&amp;bug_id=[% bug.id FILTER none %]">Show Votes</a>)
</td>
</tr>
[% END %]