If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Move CheckIfVotedConfirmed out of CGI.pl

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Bugzilla-General
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Frédéric Buclin, Assigned: Frédéric Buclin)

Tracking

2.19.2
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

6.25 KB, patch
wicked
: review+
Details | Diff | Splinter Review
(Assignee)

Description

13 years ago
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

13 years ago
OK. Re-writing a large part of a function sounds like something for Bugzilla
2.22 at this point, though.
(Assignee)

Comment 2

13 years ago
(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. ;)
Status: NEW → ASSIGNED
(Assignee)

Comment 3

13 years ago
Created attachment 180899 [details] [diff] [review]
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.
Attachment #180899 - Flags: review?(mkanat)
(Assignee)

Updated

13 years ago
Target Milestone: --- → Bugzilla 2.20

Comment 4

13 years ago
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?
Attachment #180899 - Flags: review?(mkanat) → review-
(Assignee)

Comment 5

13 years ago
Created attachment 180931 [details] [diff] [review]
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.
Attachment #180899 - Attachment is obsolete: true
Attachment #180931 - Flags: review?(mkanat)

Comment 6

13 years ago
(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

13 years ago
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. :-)
Attachment #180931 - Flags: review?(wicked)
Attachment #180931 - Flags: review?(mkanat)
Attachment #180931 - Flags: review+
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.
Attachment #180931 - Flags: review?(wicked) → review-
(Assignee)

Comment 9

13 years ago
Created attachment 181214 [details] [diff] [review]
patch, v3

replace $vars = {} by $vars = $::vars, which contains the information not to
redisplay the header: $::vars->{'header_done'} = 1.
Attachment #180931 - Attachment is obsolete: true
Attachment #181214 - Flags: review?(wicked)
Comment on attachment 181214 [details] [diff] [review]
patch, v3

Tested. This works correctly. Little things sometime make a world of
difference.. :)
Attachment #181214 - Flags: review?(wicked) → review+
(Assignee)

Updated

13 years ago
Flags: approval?
Flags: approval? → approval+
(Assignee)

Comment 11

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Assignee)

Updated

13 years ago
Blocks: 87411
You need to log in before you can comment on or make changes to this bug.