decouple votestoconfirm and unconfirmed

RESOLVED FIXED in Bugzilla 3.6

Status

()

enhancement
P1
normal
RESOLVED FIXED
17 years ago
7 years ago

People

(Reporter: gerv, Assigned: mkanat)

Tracking

2.17
Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 5 obsolete attachments)

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
Blocks: 156174
Target Milestone: --- → Bugzilla 2.18
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.
> 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
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).
[ ] Bugs start unconfirmed
    ... and will get confirmed if they get [   ] votes.

Comment 5

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

Comment 6

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

16 years ago
Revise summary to reflect comments.
Assignee: justdave → caduvall
Summary: votestoconfirm must die → decouple votestoconfirm and unconfirmed

Comment 8

16 years ago
Accepting. Sorry for the spam.
Status: NEW → ASSIGNED

Comment 9

16 years ago
Posted patch First cut (obsolete) — Splinter Review
votestoconfirm only referenced now in checksetup, sanitycheck, editproducts,
and CGI.pl (where it is actually used :).

Updated

16 years ago
Attachment #133382 - Flags: review?(justdave)

Comment 10

16 years ago
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?
Attachment #133382 - Flags: review?(justdave) → review-

Comment 11

16 years ago
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.
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
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
(Assignee)

Comment 13

14 years ago
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 nobody@bugzilla.org 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.
Target Milestone: Bugzilla 2.20 → ---

Comment 14

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

Comment 15

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

Updated

13 years ago
QA Contact: mattyt-bugzilla → default-qa

Comment 16

12 years ago
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.
Antonio: as far as I know, you can: just visit enter_bug.cgi and change the dropdown labelled "Initial State".

Gerv

Comment 18

12 years ago
(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.
(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.
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

12 years ago
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
(Assignee)

Updated

12 years ago
(Assignee)

Updated

10 years ago
Assignee: caduvall → mkanat
Severity: normal → enhancement
Priority: -- → P1
Target Milestone: --- → Bugzilla 4.0
(Assignee)

Comment 22

10 years ago
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #133382 - Attachment is obsolete: true
Attachment #356365 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #356365 - Flags: review? → review?(bugzilla)

Comment 23

10 years ago
Comment on attachment 356365 [details] [diff] [review]
v1

I also want to look at it.
Attachment #356365 - Flags: review?(LpSolit)

Updated

10 years ago
Attachment #356365 - Flags: review?(bugzilla)
Attachment #356365 - Flags: review?(LpSolit)
Attachment #356365 - Flags: review-

Comment 24

10 years ago
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.
(Assignee)

Comment 25

10 years ago
I suspect the last problem you mentioned will be handled bug bug 512606.
Depends on: 512606
(Assignee)

Comment 26

10 years ago
Posted patch v2 (obsolete) — Splinter Review
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).
Attachment #356365 - Attachment is obsolete: true
Attachment #398464 - Flags: review?(LpSolit)
(Assignee)

Updated

10 years ago
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6

Updated

10 years ago
Blocks: 486292

Updated

10 years ago
Attachment #398464 - Flags: review?(LpSolit) → review-

Comment 27

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

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

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

Updated

10 years ago
Whiteboard: [needs new patch asap]
(Assignee)

Updated

10 years ago
Whiteboard: [needs new patch asap]
(Assignee)

Comment 30

10 years ago
Posted patch v3 (obsolete) — Splinter Review
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.
Attachment #398464 - Attachment is obsolete: true
Attachment #417385 - Flags: review?(LpSolit)

Comment 31

10 years ago
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.
Attachment #417385 - Flags: review?(LpSolit) → review-

Comment 32

10 years ago
2 days before the hard freeze...
Whiteboard: [needs new patch asap]
(Assignee)

Comment 33

10 years ago
Posted patch v4 (obsolete) — Splinter Review
Okay, this fixes all the issues you mentioned! :-)
Attachment #417385 - Attachment is obsolete: true
Attachment #418122 - Flags: review?(LpSolit)
(Assignee)

Updated

10 years ago
Whiteboard: [needs new patch asap]

Comment 34

10 years ago
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?
Attachment #418122 - Flags: review?(LpSolit) → review-
(Assignee)

Comment 35

10 years ago
(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. :-)
(Assignee)

Comment 36

10 years ago
Posted patch v5Splinter Review
Okay, this fixes the errors found.
Attachment #418122 - Attachment is obsolete: true
Attachment #418265 - Flags: review?(LpSolit)

Comment 37

10 years ago
Comment on attachment 418265 [details] [diff] [review]
v5

I have no more ideas about what else to test. :) r=LpSolit
Attachment #418265 - Flags: review?(LpSolit) → review+

Updated

10 years ago
Flags: approval+
(Assignee)

Comment 38

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

Updated

10 years ago
Keywords: relnote
(Assignee)

Comment 39

9 years ago
Added to the release notes in bug 547466.
Keywords: relnote
Duplicate of this bug: 461548
You need to log in before you can comment on or make changes to this bug.