Closed Bug 276600 Opened 17 years ago Closed 17 years ago

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

Categories

(Bugzilla :: Administration, task)

2.18
task
Not set
major

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: LpSolit, Assigned: goobix)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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! ;)
As a regression, we should have it fixed in 2.18 and 2.20 before we release.
Flags: blocking2.20?
Flags: blocking2.18?
Keywords: regression
No longer depends on: 271474
Depends on: 271474
Mea culpa.  Will re-patch.
Status: NEW → ASSIGNED
Attached patch patch to trunk or 2.18 tip (obsolete) — Splinter Review
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.
Attachment #169994 - Flags: review?
Comment on attachment 169994 [details] [diff] [review]
patch to trunk or 2.18 tip

Also tested this patch against the trunk.
Attachment #169994 - Attachment description: patch to 2.18 branch tip → patch to trunk or 2.18 tip
Incidentally, when testing this against the trunk I kept hitting an unrelated
bug when voting for bugs.  Something about bugs_activity not being locked.
(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 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.
Attachment #169994 - Flags: review?
(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.
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
Flags: blocking2.20?
Flags: blocking2.20+
Flags: blocking2.18?
Flags: blocking2.18+
Target Milestone: --- → Bugzilla 2.18
(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. ;)
Attached patch v2Splinter Review
Attachment #170030 - Flags: review?(LpSolit)
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?
Attachment #170030 - Flags: review+
Flags: approval?
Flags: approval2.18?
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
Attachment #170030 - Flags: review?(LpSolit) → review+
Flags: approval?
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
Attachment #169994 - Attachment is obsolete: true
Whiteboard: patch awaiting checkin
Blocks: 277013
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: patch awaiting checkin
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee: justdave → vladd
Status: REOPENED → NEW
vladd wrote the patch.. reassigning to him. ;)
Status: NEW → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.