276600 - checking votes in editproducts.cgi is broken (regression due to bug 271474)

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

The following lines from attachment 168213 [details] [diff] [review] in bug 271474 prevent
editproducts.cgi to correctly check for bugs having enough votes to get confirmed:

+        while (MoreSQLData()) {
+            CheckIfVotedConfirmed(FetchOneColumn(), $::userid);
         }

(04:06:59) LpSolit: CheckIfvotedConfirmed() must not be called while using
MoreSQLData()
(04:07:21) LpSolit: only the first bug is checked, not the others
(04:07:31) justdave: that means CheckIfVoteConfirmed needs a
PushGlobalSQLState() at the begining and PopGlobalSQLState() at the end
(04:07:39) justdave: or rewrite it to use DBI and store its own handle
(04:08:08) LpSolit: justdave, what is the best solution?
(04:08:32) justdave: at this point, I'd say rewrite it to use DBI, so it doesn't
overwrite the global statement handle

This patch has landed into the 2.18 branch... bad! ;)

Comment 1

[Frédéric Buclin]

As a regression, we should have it fixed in 2.18 and 2.20 before we release.

Comment 2

[Nick Barnes]

Mea culpa.  Will re-patch.

Comment 3

[Nick Barnes]

Attachment: patch to trunk or 2.18 tip

Patch changes the relevant code to collect all the bug IDs in a list before any
calls to CheckIfVotedConfirmed.  That's the way this code used to work, before
the 271474 patch broke it while fixing other bugs.

Comment 4

[Nick Barnes]

Comment on attachment 169994 [details] [diff] [review]
patch to trunk or 2.18 tip

Also tested this patch against the trunk.

Comment 5

[Nick Barnes]

Incidentally, when testing this against the trunk I kept hitting an unrelated
bug when voting for bugs.  Something about bugs_activity not being locked.

Comment 6

[Frédéric Buclin]

(In reply to comment #5)
> Incidentally, when testing this against the trunk I kept hitting an unrelated
> bug when voting for bugs.  Something about bugs_activity not being locked.

I discovered this yesterday. I am going to fix that with bug 276605.

Comment 7

[Frédéric Buclin]

Comment on attachment 169994 [details] [diff] [review]
patch to trunk or 2.18 tip

>+        # collect all affected bugs before calling CheckIfVotedConfirmed
>+        # (because the call destroys the SQL state).

I'm not sure a comment is necessary here.


>+        foreach my $bugid (@list) {
>             # The user id below is used for activity log purposes
>-            CheckIfVotedConfirmed(FetchOneColumn(), Bugzilla->user->id);
>+            CheckIfVotedConfirmed($bugid, Bugzilla->user->id);

The whole part about votes use $id to identify bugs. Could you please replace
$bugid by $id for consistency?

Else, it OK for me.

Comment 8

[Nick Barnes]

(In reply to comment #7)
> (From update of attachment 169994 [details] [diff] [review] [edit])
> >+        # collect all affected bugs before calling CheckIfVotedConfirmed
> >+        # (because the call destroys the SQL state).
> 
> I'm not sure a comment is necessary here.

When developing the original patch for 271474 I got rid of @list and put the
CheckIfVotedConfirmed call inside the MoreSQLData loop, because it made the code
shorter and sweeter, and because I didn't realise that it would break in this
way.  A comment like this might help avoid such a mistake in future.

> >+        foreach my $bugid (@list) {
> >             # The user id below is used for activity log purposes
> >-            CheckIfVotedConfirmed(FetchOneColumn(), Bugzilla->user->id);
> >+            CheckIfVotedConfirmed($bugid, Bugzilla->user->id);
> 
> The whole part about votes use $id to identify bugs. Could you please replace
> $bugid by $id for consistency?

Sure, I can rework it.  I was wanting to avoid confusion with the user ID.

> Else, it OK for me.

Great.  I'll make and test and post a modified patch some time this evening.

Comment 9

[Dave Miller [:justdave]]

I was discussing this with someone on IRC the other day...

The best way to solve this is to change CheckIfVotedConfirmed to use DBI calls
directly instead of the old deprecated stuff.  If we need to leave the
deprecated stuff for now, then PushGlobalSQLState and PopGlobalSQLState need to
be used inside CheckIfVotedConfirmed

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
> I was discussing this with someone on IRC the other day...

It was with me, see comment 0. ;)


> The best way to solve this is to change CheckIfVotedConfirmed to use DBI calls
> directly instead of the old deprecated stuff.  If we need to leave the
> deprecated stuff for now, then PushGlobalSQLState and PopGlobalSQLState need to
> be used inside CheckIfVotedConfirmed


I agree. Nick, use this way to fix the problem (you only have to add two lines
in CheckIfVotedConfirmed() in CGI.pl). This way, you don't need to alter
editproducts.cgi anymore and this will prevent other users hacking the code to
do the same error you did.

I can review your patch if you look for a reviewer. ;)

Comment 11

[Vlad Dascalu]

Attachment: v2

Comment 12

[Max Kanat-Alexander]

Comment on attachment 170030 [details] [diff] [review]
v2

This looks "obviously correct" to me, without even testing it. Frederic, you
can also test it, if you want.

We definitely should re-work the code to get rid of the SendSQL calls, though
-- is there a bug for making that change in all of BZ?

Comment 13

[Gervase Markham [:gerv]]

Comment on attachment 170030 [details] [diff] [review]
v2

r=gerv. I can't think of any circumstances where wrapping an SQL call in a Push
and Pop like this could break anything...

Gerv

Comment 14

[Shane H. W. Travis]

2.18
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.211.2.6; previous revision: 1.211.2.5
done

Tip:
Checking in CGI.pl;
/cvsroot/mozilla/webtools/bugzilla/CGI.pl,v  <--  CGI.pl
new revision: 1.220; previous revision: 1.219
done

Comment 15

[Frédéric Buclin]

vladd wrote the patch.. reassigning to him. ;)