162060 - decouple votestoconfirm and unconfirmed

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[Gervase Markham [:gerv]]

One of our most FAQs is "how do I turn on unconfirmed"? The current method (set
votestoconfirm to non-zero) is not very intuitive. Also, confirming by popular
vote has only ever been done on a handful of bugs on b.m.o.

We should remove this feature, and replace it with a simple "enable UNCONFIRMED"
checkbox for each product.

Gerv

Comment 1

[Dave Miller [:justdave]]

The only reason it never gets used on b.m.o is because nobody knows about it. 
The UI needs to change so that if a bug is in the UNCONFIRMED state it states
right next to the status "This bug will be confirmed if X people vote for it" in
small print.  Then I bet it starts working because people outside of the
developers will know it's there.

I'll veto removing this because I use it at InTrec.  On the other hand, the edit
product page for the admin needs an easier way to enable UNCONFIRMED.  I bet
most people could figure it out if there were just an instruction on the page
stating "to enable UNCONFIRMED, set the 'votes to confirm' to a non-zero value".
 Or we could just add a checkbox for it, and if the box is checked, complain if
votestoconfirm is zero.

Comment 2

[Gervase Markham [:gerv]]

> I'll veto removing this because I use it at InTrec. 

In what way? I didn't know your Bugzilla was public...

>  On the other hand, the edit
> product page for the admin needs an easier way to enable UNCONFIRMED.  I bet
> most people could figure it out if there were just an instruction on the page
> stating "to enable UNCONFIRMED, set the 'votes to confirm' to a non-zero 
> value". Or we could just add a checkbox for it, and if the box is checked, 
> complain if votestoconfirm is zero.

The instruction is bad because it indicates clearly that the UI is bad. A
control which needs a 13-word manual is insufficiently understandable.

Complaining is bad because we are beating the user round the head for something
they couldn't possibly expected to know, based on the fact that Bugzilla doesn't
work the way anyone would expect.

A forced link between the UNCONFIRMED feature and the voting feature is bogus,
whichever way you look at it. But, if you are determined to keep it, we could
decouple it by adding the "enable UNCONFIRMED" checkbox but making
votestoconfirm=0 mean "infinity" - i.e. if UNCONFIRMED is on but votestoconfirm
is 0 (the default), you can't confirm using votes. Or we could make "" the
default, meaning infinity.

Gerv

Comment 3

[Dave Miller [:justdave]]

OK, I could deal with separating them.  Make you turn on UNCONFIRMED explicitly,
and if votestoconfirm is 0 then you can't confirm by voting. And if UNCONFIRMED
is disabled and votestoconfirm is nonzero it just gets ignored.  This not only
sounds more intuitive, but sounds like it might be easier to program around, too
(since you just check whether UNCOFIRMED is enabled in most places and the only
place you'd have to deal with votestoconfirm is when you're actually voting).

Comment 4

[Matthew Tuck [:CodeMachine]]

[ ] Bugs start unconfirmed
    ... and will get confirmed if they get [   ] votes.

Comment 5

[Greg Horner]

*** Bug 173474 has been marked as a duplicate of this bug. ***

Comment 6

[chaduv]

I'd like to try my hand at decoupling unconfirmed and votestoconfirm, if no one 
else is working on it. Will have something to view by tomorrow.

Comment 7

[chaduv]

Revise summary to reflect comments.

Comment 8

[chaduv]

Accepting. Sorry for the spam.

Comment 9

[chaduv]

Attachment: First cut

votestoconfirm only referenced now in checksetup, sanitycheck, editproducts,
and CGI.pl (where it is actually used :).

Comment 10

[Jouni Heikniemi]

Comment on attachment 133382 [details] [diff] [review]
First cut

The patch is bitrotten for editproducts.cgi, so I couldn't really test it
deeply, but here are some comments.

>-    if ($votes >= $votestoconfirm && $status eq $::unconfirmedstate) {
>+    if ($votestoconfirm > 0 && $votes >= $votestoconfirm
>+                            && $status eq $::unconfirmedstate) {

Nit: Unless there is a specific reason, move the && operator to the previous
line and align $status with the first $votestoconfirm on the upper line.

>+    useunconfirmed tinyint not null,

Maybe default 0?

In editproducts:

>+    my $useunconfirmed = $::FORM{useunconfirmed} || 0;
...
>     SendSQL("INSERT INTO products ( " .
>             "name, description, milestoneurl, disallownew, votesperuser, " .
>-            "maxvotesperbug, votestoconfirm, defaultmilestone" .
>+            "maxvotesperbug, votestoconfirm, defaultmilestone, useunconfirmed" .
>             " ) VALUES ( " .
>             SqlQuote($product) . "," .
>             SqlQuote($description) . "," .
>             SqlQuote($milestoneurl) . "," .
>             $disallownew . "," .
>             "$votesperuser, $maxvotesperbug, $votestoconfirm, " .
>-            SqlQuote($defaultmilestone) . ")");
>+            SqlQuote($defaultmilestone) . ", $useunconfirmed )");

Wouldn't this cause an sql injection vulnerability?

>+@status = "NEW" if (UserInGroup("editbugs") || UserInGroup("canconfirm"));
>+
>+SendSQL("SELECT useunconfirmed FROM products WHERE name = " .
>+        SqlQuote($product));
>+push(@status, $unconfirmedstate) if (FetchOneColumn());
>+
>+push (@status, "NEW") if (!scalar(@status));

This is confusing, although it probably works. How about first checking for and
possibly adding unconfirmed, then pushing new if !scalar(@status) ||
UIG("editbugs") || UIG("canconfirm")? This would group the NEW stuff into one
place.

How about getting rid of the deprecated SendSQL while you're at it?

Comment 11

[Ulrich Windl]

I also got puzzled by the mismatch between documentation and program in 2.16.4:
It's non-obvious that using "0 votes" to move a bug out from UNCONFIRMED state
will disable the UNCONFIRMED state. I thought someone will have to change state
manually from UNCONFIRMED to NEW after confirming the bug. But UNCONFIRMED is
not available at all. This does not match the help texts, nor does it match the
documentation.

Comment 12

[Dave Miller [:justdave]]

All 2.18 bugs that haven't been touched in over 60 days and aren't flagged as
blockers are getting pushed out to 2.20

Comment 13

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 14

[Frédéric Buclin]

*** Bug 312280 has been marked as a duplicate of this bug. ***

Comment 15

[Frédéric Buclin]

*** Bug 325179 has been marked as a duplicate of this bug. ***

Comment 16

[Antonio Batovanja]

Another thing I'm missing is to be able to create an UNCONFIRMED bug while having canconfirm permission. 
A parameter for that option would be great.

Comment 17

[Gervase Markham [:gerv]]

Antonio: as far as I know, you can: just visit enter_bug.cgi and change the dropdown labelled "Initial State".

Gerv

Comment 18

[Antonio Batovanja]

(In reply to comment #17)
Gerv, the "Initial state" field is not a dropdown when you have "canconfirm" permission It's set fix to NEW.

Comment 19

[Reed Loden [:reed]]

(In reply to comment #18)
> Gerv, the "Initial state" field is not a dropdown when you have "canconfirm"
> permission It's set fix to NEW.

No, you can still select other options from that drop down. Available options are: NEW, UNCONFIRMED, ASSIGNED.

Comment 20

[Gervase Markham [:gerv]]

I certainly get the three options Reed talks about.

Antonio: are you seeing this problem on bugzilla.mozilla.org? If so, which account are you using? (I can "impersonate" you and go and look for myself.)

Here's the code that populates the list:
http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/enter_bug.cgi#488

If you are only getting NEW, then a) the user concerned must not have either canconfirm or editbugs, and b) the number of votestoconfirm for the product must be 0 (i.e. you aren't using UNCONFIRMED in that product).

Gerv

Comment 21

[Antonio Batovanja]

Thank you both for your answers.
In my enter_bug.cgi (release 2.22 - revision 1.126 tagged Bugzilla_Stable) it looks like this:
if ($votestoconfirm) {
    if (UserInGroup("editbugs") || UserInGroup("canconfirm")) {
        push(@status, "NEW");
    }
    push(@status, 'UNCONFIRMED');
} else {
    push(@status, "NEW");
}

Since $votestoconfirm is 0, there's no dropdown.
I'm glad to see the change in HEAD, looking forward to the next stable release :)

Toni

Comment 22

[Max Kanat-Alexander]

Attachment: v1

This adds a boolean option to products that specifies whether or not the UNCONFIRMED status is allowed in this product. Ideally we'd have a more generic system than this for fields to control other fields (and we do have such a system, but it's just not ready for this), but we need this as a stopgap so that I can work on bug 372979.

I had to make it so that Bugzilla always allows transitions from a status to itself, because when you disable UNCO in a product now, bugs will remain UNCO even though it's not a valid status.

Comment 23

[Frédéric Buclin]

Comment on attachment 356365 [details] [diff] [review]
v1

I also want to look at it.

Comment 24

[Frédéric Buclin]

Comment on attachment 356365 [details] [diff] [review]
v1

>Index: Bugzilla/Product.pm

>+sub allows_unconfirmed { return $_[0]->{'allows_unconfirmed'}; }

Nit: it would be great if you could mention it in the POD. Also, why allows_ instead of allow_? This looks strange to me.



>Index: template/en/default/admin/products/updated.html.tmpl

>+    status. Note that any <a href="...">bugs 

"bugs" must be [% terms.bugs %] (t/009bugwords........NOK 187/263).


The reasons I deny review (besides the runtest error above) are:

1) If allows_unconfirmed = 1 and votes_to_confirm = 0, you can file bugs as UNCONFIRMED, and sanitycheck.cgi then complains that you have bugs with enough votes to be confirmed, but which aren't. That's problematic.

2) If you have bugs with the UNCONFIRMED status, and you then set allows_unconfirmed to 0, you can resolve bugs as RESOLVED (as expected); but when you want to reopen the bug, UNCONFIRMED is listed despite it's no longer a valid bug status, and trying to select it (e.g. reporters with no privs) will throw an error:

 "You are not allowed to change the bug status from RESOLVED to UNCONFIRMED."


Everything else looks good to me.

Comment 25

[Max Kanat-Alexander]

I suspect the last problem you mentioned will be handled bug bug 512606.

Comment 26

[Max Kanat-Alexander]

Attachment: v2

Okay, here's a patch with everything else fixed other than the RESOLVED -> UNCONFIRMED transition (because I think that other patch will just fix that problem outright).

Comment 27

[Frédéric Buclin]

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

>Index: Bugzilla/Bug.pm

>@@ -1101,8 +1101,8 @@
>         @valid_statuses = @{Bugzilla::Status->can_change_to()};
>     }
> 
>-    if (!$product->votes_to_confirm) {
>-        # UNCONFIRMED becomes an invalid status if votes_to_confirm is 0,
>+    if (!$product->allows_unconfirmed) {
>+        # UNCONFIRMED becomes an invalid status if allows_unconfirmed is false,
>         # even if you are in editbugs.
>         @valid_statuses = grep {$_->name ne 'UNCONFIRMED'} @valid_statuses;
>     }

This part is bitrotten as I asked you to move this block in the previous else block.


>+      [%+ get_status('UNCONFIRMED') FILTER html %] status in this 

get_status() no longer exists. Several places are affected.

Comment 28

[Frédéric Buclin]

Oh, also, the UI when editing a product is confusing IMO. You first ask the number of votes to go out of UNCONFIRMED, and then you ask if UNCONFIRMED should be used or not. IMO, both should be merged in a more logical way, e.g. by following the idea from comment 4.

Comment 29

[Frédéric Buclin]

... and the UI should also mention that 0 votes to confirm means that you can never confirm bugs this way.

Comment 30

[Max Kanat-Alexander]

Attachment: v3

I fixed the bitrot, and the get_status stuff, and the new UI was a good idea, so I did that too. And in fact, there's no reason to show the voting parameters now when usevotes is off (since they do absolutely nothing now when voting is off) so they're now hidden totally in that case.

Comment 31

[Frédéric Buclin]

Comment on attachment 417385 [details] [diff] [review]
v3

>Index: Bugzilla/Bug.pm

>+    # We skip this check if we are changing from a status to itself.

But show_bug.cgi doesn't display UNCONFIRMED in the bug status select box when the bug was originally marked as UNCONFIRMED and "use UNCONFIRMED" has been turned off meanwhile. This is annoying, because editing a bug force you to change its bug status, and 1) maybe you want to keep its status alone, and 2) powerless users will get an error that they are not allowed to confirm bugs.



>Index: template/en/default/admin/products/edit-common.html.tmpl

>+[% IF Param('usevotes') %]
>+    <td><input size="5" maxlength="5" name="votesperuser" 
>+               value="[% product.votesperuser FILTER html %]">

While you are on it, please add an ID to the text field. This makes the QA team's life much easier to write Selenium scripts.


>+    <td><input size="5" maxlength="5" name="maxvotesperbug" 
>+               value="[% product.maxvotesperbug FILTER html %]">

Same here.

Also, with usevotes = off, you can no longer edit products. You get the error:
  '' is an invalid value for the  Votes Per User  field, which should contain a non-negative number.

editproducts.cgi shouldn't call $product->set_votes_*() when usevotes = off. I didn't check if this is also a problem on product creation.

Comment 32

[Frédéric Buclin]

2 days before the hard freeze...

Comment 33

[Max Kanat-Alexander]

Attachment: v4

Okay, this fixes all the issues you mentioned! :-)

Comment 34

[Frédéric Buclin]

Comment on attachment 418122 [details] [diff] [review]
v4

>Index: Bugzilla/Bug.pm

>+    # If this bug has an inactive status set, it should still be in the list.
>+    if (!grep($_ eq $self->status->name, @available)) {

@available contains objects, so you want $_->name instead of $_ alone.



>Index: template/en/default/admin/products/edit-common.html.tmpl

>+        <input size="3" maxlength="5" name="votestoconfirm" id="votestoconfirm"
>+               value="[% product.votes_to_confirm FILTER html %]">
>+         votes. (Setting this to 0 disables auto-confirming [% terms.bugs %]
>+         by vote.)

Setting votestoconfirm to 0 confirms all bugs despite what the comment says. Bugzilla::Bug::CheckIfVotedConfirmed() must be fixed:

  if (!$bug->everconfirmed && $bug->votes >= $bug->product_obj->votes_to_confirm)

It must first check that votes_to_confirm is not zero.


Not exactly related to this bug, but sanitycheck.cgi complains when a bug which has enough votes is set back to UNCONFIRMED, when the status workflow allows this transition. And every time a new user votes for the bug, the bug comes back to NEW, even if a power user wants to keep it UNCONFIRMED. That's pretty confusing to the average (and even advanced) user, IMO. Should we forbird to set bugs back to UNCONFIRMED when there are enough votes on them?

Comment 35

[Max Kanat-Alexander]

(In reply to comment #34)

  Thanks for catching all that stuff there. :-) I'll attach a new patch soon.

> Not exactly related to this bug, but sanitycheck.cgi complains when a bug which
> has enough votes is set back to UNCONFIRMED, when the status workflow allows
> this transition. And every time a new user votes for the bug, the bug comes
> back to NEW, even if a power user wants to keep it UNCONFIRMED. That's pretty
> confusing to the average (and even advanced) user, IMO. Should we forbird to
> set bugs back to UNCONFIRMED when there are enough votes on them?

  Oh, auugh. That will confuse the heck out of users who are trying to re-unconfirm bugs and can't do it for only *some* of them. No, I think we should let people set them back to UNCONFIRMED if they really want to. Voting just auto-confirms them and they're *most likely* to stay that way, I suppose. Probably the solution is to make it so that bugs are only confirmed if they go from below the number of votes required to equal or greater than the number of votes required, so that the confirmation only happens once.

  Also, we should remove the voting system from core and make it into an extension. :-)

Comment 36

[Max Kanat-Alexander]

Attachment: v5

Okay, this fixes the errors found.

Comment 37

[Frédéric Buclin]

Comment on attachment 418265 [details] [diff] [review]
v5

I have no more ideas about what else to test. :) r=LpSolit

Comment 38

[Max Kanat-Alexander]

Great, thank you! :-)

Checking in attachment.cgi;
/cvsroot/mozilla/webtools/bugzilla/attachment.cgi,v  <--  attachment.cgi
new revision: 1.166; previous revision: 1.165
done
Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.154; previous revision: 1.153
done
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.176; previous revision: 1.175
done
Checking in importxml.pl;
/cvsroot/mozilla/webtools/bugzilla/importxml.pl,v  <--  importxml.pl
new revision: 1.95; previous revision: 1.94
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.148; previous revision: 1.147
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.307; previous revision: 1.306
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.44; previous revision: 1.43
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.128; previous revision: 1.127
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.81; previous revision: 1.80
done
Checking in template/en/default/admin/products/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/admin/products/edit-common.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/edit-common.html.tmpl,v  <--  edit-common.html.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/admin/products/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.15; previous revision: 1.14
done
Checking in template/en/default/admin/products/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/updated.html.tmpl,v  <--  updated.html.tmpl
new revision: 1.11; previous revision: 1.10
done
Checking in template/en/default/attachment/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/attachment/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.46; previous revision: 1.45
done

Comment 39

[Max Kanat-Alexander]

Added to the release notes in bug 547466.