500900 - Confirming bugs requires NEW state to exist

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

CheckIfVotedConfirmed does:

            $dbh->do("UPDATE bugs SET bug_status = 'NEW', everconfirmed = 1, " .
                     "delta_ts = ? WHERE bug_id = ?",
                     undef, ($timestamp, $id));
            $dbh->do("INSERT INTO bugs_activity " .
                     "(bug_id, who, bug_when, fieldid, removed, added) " .
                     "VALUES (?, ?, ?, ?, ?, ?)",
                     undef, ($id, $who, $timestamp, $fieldid, 'UNCONFIRMED', 'NEW'));

UNCONFIRMED isn't allowed to be changed, but NEW can be, so if someone removes/renames the NEW state, you end up with a bug in an invalid state that it can't be moved out of....

Comment 1

[Rolf Leggewie]

We got hit by this at http://bugs.openembedded.org.  Bradley was so kind to open this report.  I can confirm the problem.

Comment 2

[Max Kanat-Alexander]

They should instead go into the confirmed state that sorts the lowest in the list.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

The user ID passed to CheckIfVotedConfirmed() is always the currently logged in user, so this parameter can go away as $bug->update() already does this assumption. I also replaced direct SQL calls by setters, so that security and other validity checks can work correctly.

Comment 4

[Max Kanat-Alexander]

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

>Index: Bugzilla/Bug.pm
>+            $bug->_set_everconfirmed(1);

  I assume this is to catch some other case where the bug is marked in a confirmed status but doesn't have everconfirmed set? That's an anomalous case, and we should let sanitycheck deal with it, and never expect it to happen otherwise.

>+  [% ELSIF error == "no_open_bug_status" %]
>+    [% title = "No Open Bug Status" %]
>+    There is no way to confirm [% terms.abug %] in this installation.

   Mmmm, let's be more specific--say that there is no valid transition from UNCONFIRMED to an open state, meaning that bugs cannot be confirmed.

Comment 5

[Bradley Baetz (:bbaetz)]

Should this just be a param?

Comment 6

[Max Kanat-Alexander]

  No, it's too small of a functionality to deserve a parameter. Maybe when voting becomes a plugin.

Comment 7

[Frédéric Buclin]

(In reply to comment #4)
>   I assume this is to catch some other case where the bug is marked in a
> confirmed status but doesn't have everconfirmed set? That's an anomalous case,
> and we should let sanitycheck deal with it, and never expect it to happen
> otherwise.

No, this is for bugs which are in a closed state. If a bug is e.g. RESOLVED WONTFIX, we only set everconfirmed = 1, but leave the bug as RESOLVED. So this has nothing to do with an anomalous case. Max, was that the reason for your r-? If yes, please reconsider.

Comment 8

[Max Kanat-Alexander]

Ah, okay. Add a comment about that and change the error message and it'll be r+. I think it's OK to call _set_everconfirmed, because we are inside Bugzilla::Bug, after all. Eventually we should make it $bug->confirm_if_vote_confirmed.

Comment 9

[Frédéric Buclin]

Attachment: patch, v2

Comment added + fixed the error message. code-error now calls field-descs.none.tmpl, as user-error does.

Comment 10

[Max Kanat-Alexander]

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

Looks good to me.

Comment 11

[Frédéric Buclin]

Attachment: patch for 3.2, v1

Backport for 3.2. CheckIfVotedConfirmed() is still in editproducts.cgi instead of Product.pm.

Comment 12

[Frédéric Buclin]

tip:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.418; previous revision: 1.417
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.58; previous revision: 1.57
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.281; previous revision: 1.280
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.38; previous revision: 1.37
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.113; previous revision: 1.112
done


3.3.4:

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.417.2.1; previous revision: 1.417
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.57.2.1; previous revision: 1.57
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.276.2.5; previous revision: 1.276.2.4
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.34.2.1; previous revision: 1.34
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.109.2.1; previous revision: 1.109
done


3.2.3:

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.142.2.3; previous revision: 1.142.2.2
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.410.2.3; previous revision: 1.410.2.2
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.55.2.1; previous revision: 1.55
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.241.2.19; previous revision: 1.241.2.18
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.105.2.1; previous revision: 1.105
done