290513 - Move CheckIfVotedConfirmed out of CGI.pl

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

Together with RemoveVotes, this function should go into Bug.pm. I need to
rewrite a large part of this function to fix bug 34172, so doing the move at the
same time sounds appropriate.

Comment 1

[Max Kanat-Alexander]

OK. Re-writing a large part of a function sounds like something for Bugzilla
2.22 at this point, though.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> OK. Re-writing a large part of a function sounds like something for Bugzilla
> 2.22 at this point, though.

Don't worry, the change is straightforward and safe. ;)

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

The main change in CheckIfVotedConfirmed() is that we now check if
everconfirmed == 0 instead of bug_status = "UNCONFIRMED". This patch also fixes
the voting system which gerv has broken due to a missing 'email_setting READ'
in a bz_lock() call in votes.cgi.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 180899 [details] [diff] [review]
patch, v1

>+    my ($votes, $status, $votestoconfirm, $everconfirmed, $timestamp) =
>+        $dbh->selectrow_array("SELECT bugs.votes, bugs.bug_status, " .
>+                              "products.votestoconfirm, bugs.everconfirmed, NOW() " .
>+                              "FROM bugs INNER JOIN products " .
>+                              "ON products.id = bugs.product_id " .
>+                              "WHERE bugs.bug_id = ?", undef, $id);

  Sorry to be really nitpicky, but the previous spacing of this SQL statement
was much easier to read. (I read a *lot* of SQL in Bugzilla. :-D)

  Something like this is what I *ideally* prefer:

    SELECT a.column, b.column2
	   c.column3
      FROM a INNER JOIN b
		     ON a.column = b.baz
	     INNER JOIN c ON b.foo = c.bar
     WHERE c.qux = ?
	   AND f.mmm = ?
  ORDER BY c.column3

  Note that I said "ideally" though. In particular, the "ON" part doesn't have
to be super-indented like that if there's not enough space. :-)

>+    my $sql_timestamp = $dbh->quote($timestamp);

  This is old cruft; I think you should feel free to remove it while you're in
here anyway. Also, it's actually a bug to pass something in to a placeholder
that's *already* quoted. (This is the r-)

>+        if ($status eq 'UNCONFIRMED') {
>+            my $fieldid = &::GetFieldID("bug_status");
>+            $dbh->do("UPDATE bugs SET bug_status = 'NEW', everconfirmed = 1, " .
>+                     "delta_ts = ? WHERE bug_id = ?",
>+                     undef, ($sql_timestamp, $id));
>+            $dbh->do("INSERT INTO bugs_activity " .
>+                     "(bug_id, who, bug_when, fieldid, removed, added) " .
>+                     "VALUES (?, ?, ?, ?, ?, ?)",
>+                     undef, ($id, $who, $sql_timestamp, $fieldid, 'UNCONFIRMED', 'NEW'));
>+        }
>+        else {
>+            $dbh->do("UPDATE bugs SET everconfirmed = 1 " .
>+                     "WHERE bug_id = ?", undef, $id);
>+        }

  You could just move this outside of the else, since the activity entry is
already outside of the else. It's up to you, though.

>+        my $template = $::template;
>+        my $vars = $::vars;
>+
>+        $vars->{'type'} = "votes";
>+        $vars->{'id'} = $id;
>+        $vars->{'mailrecipients'} = { 'changer' => $who };
>+
>+        $template->process("bug/process/results.html.tmpl", $vars)
>+          || ThrowTemplateError($template->error());

  Ohhh... this is really bad to have inside here. Can we somehow move this out
of this function?

Comment 5

[Frédéric Buclin]

Attachment: patch, v2

The template is really part of this function. I don't see how I could move it
outside. Some other .pm files such as Flag.pm, Token.pm and User.pm also use
templates inside some of their functions.

Comment 6

[Max Kanat-Alexander]

(In reply to comment #5)
> The template is really part of this function. I don't see how I could move it
> outside. Some other .pm files such as Flag.pm, Token.pm and User.pm also use
> templates inside some of their functions.

  Yes, but they really shouldn't display them to the user unless absolutely
necessary. It's not a good idea to have direct output in libraries, except for
errors.

  But anyhow, that's part of a different patch than this, most certainly.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 180931 [details] [diff] [review]
patch, v2

OK, this all *looks* right to me, but I also need somebody to actually *test*
it. :-)

Comment 8

[Teemu Mannermaa (:wicked)]

Comment on attachment 180931 [details] [diff] [review]
patch, v2

This indeed seems to fix both the problem described in bug 34172 as well as
email_setting locking problem.

Unfortunately this also adds a duplicate page header to editproducts.cgi's
output. Happens when atleast one bug is confirmed due to changing
votestoconfirm to a lower value and CheckIfVotedConfirmed() sub shows the bug
changed template.

I tested and this header duplication does not happen on an unpatched tip
install.

Comment 9

[Frédéric Buclin]

Attachment: patch, v3

replace $vars = {} by $vars = $::vars, which contains the information not to
redisplay the header: $::vars->{'header_done'} = 1.

Comment 10

[Teemu Mannermaa (:wicked)]

Comment on attachment 181214 [details] [diff] [review]
patch, v3

Tested. This works correctly. Little things sometime make a world of
difference.. :)

Comment 11

[Frédéric Buclin]

Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.241; previous revision: 1.240
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.29; previous revision: 1.28
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.75; previous revision: 1.74
done