Right now there's a lot of logic in knob.html.tmpl to control what statuses are available to change to, for the current bug. Instead, I think we should have this in an accessor like $bug->available_statuses, so that we can do all the actual user-related control there (and take anything user-related out of Bugzilla::Status if it's there--just have it return literal information about the workflow).

This implements bug.choices.bug_status, which gives us control of statuses when editing individual bugs--that was the most complex bit, so I'm posting this patch for now, and then I'll do another patch on top of this that fixes things for enter_bug.cgi too. I actually tested this fairly extensively. So I know it works properly for reporters without canconfirm, people with no privileges at all on a bug, and people with editbugs.

Oh, and of course, this requires both blocking patches to be applied first.

Oh, by the way, I also did a bit of cleanup. I removed Bugzilla::Status->can_change_from because it's dead code and we don't need it anywhere. And I removed the "$field eq 'canconfirm' bit in check_can_change_field."

Comment on attachment 396679 [details] [diff] [review] v1 bitrotten by some of your own patches and mine (you should commit bug 512623 before fixing conflicts).

Okay, this fixes the bitrot.

Updated to apply against the latest patch on the dependent bug.

Comment on attachment 403907 [details] [diff] [review] v2.1 >=== modified file 'Bugzilla/Bug.pm' > if (ref $invocant) { >+ @valid_statuses = @{$invocant->_statuses_available}; _statuses_available() already excludes UNCONFIRMED if the product has votestoconfirm = 0, so the check right after this IF block should only happen on bug creation, i.e. should be moved into the ELSE block. >-sub isunconfirmed { You must remove this subroutine from _validate_attribute(). >+sub _statuses_available { Why is this method private? Why not naming it statuses_available()? >=== modified file 'template/en/default/bug/knob.html.tmpl' >+ value = bug.status.name Nit: you could pass bug.bug_status, so that you don't need to build a Bugzilla::Status object to get its name. Now two problems with this patch: - If a bug is RESOLVED (with everconfirmed = 1) and as a powerless reporter you want to reopen it, both UNCONFIRMED and REOPENED are available in the select field, but selecting UNCONFIRMED throws "You tried to change the Ever confirmed field from 1 to 0, but only the assignee of the bug, or a user with the required permissions may change that field." - As both UNCONFIRMED and REOPENED are available for closed bugs, power users have no idea which one to select when they want to reopen a bug. They would most of the time override everconfirmed by accident (having to look at the bug history to know its previous state is not an acceptable workaround). Otherwise looks great and is a nice cleanup!

(In reply to comment #7) > - If a bug is RESOLVED (with everconfirmed = 1) and as a powerless reporter you > want to reopen it, both UNCONFIRMED and REOPENED are available in the select > field, but selecting UNCONFIRMED throws "You tried to change the Ever confirmed > field from 1 to 0, but only the assignee of the bug, or a user with the > required permissions may change that field." Okay, I'll look into that. > - As both UNCONFIRMED and REOPENED are available for closed bugs, power users > have no idea which one to select when they want to reopen a bug. They would > most of the time override everconfirmed by accident (having to look at the bug > history to know its previous state is not an acceptable workaround). Yeah, that's something that people will have to handle with their workflow, and that I handled with my new proposed workflow (which has no REOPENED).

Okay, I fixed it by breaking out the code to check if everconfirmed is changing into a separate subroutine, which makes it much easier to code and read. I also changed canconfirm to be absolute, even if you have editbugs.

(In reply to comment #8) > Yeah, that's something that people will have to handle with their workflow, > and that I handled with my new proposed workflow (which has no REOPENED). Your new workflow won't help. Even with REOPENED going away, a power user still has to decide between UNCONFIRMED and NEW.

(In reply to comment #10) > Your new workflow won't help. Even with REOPENED going away, a power user still > has to decide between UNCONFIRMED and NEW. No, he has to decide between UNCONFIRMED and CONFIRMED, a much more reasonable choice.

(In reply to comment #11) > No, he has to decide between UNCONFIRMED and CONFIRMED, a much more > reasonable choice. This makes everconfirmed totally irrelevant to me. It exists for this exact purpose, i.e. to decide which one to display between UNCONFIRMED and REOPENED.

(In reply to comment #12) > This makes everconfirmed totally irrelevant to me. It exists for this exact > purpose, i.e. to decide which one to display between UNCONFIRMED and REOPENED. Okay. So?

Also, if you look at the code, everconfirmed is indeed quite still relevant. It's used in this very path, in fact.

(In reply to comment #14) > It's used in this very path, in fact. *patch :-)

(In reply to comment #13) > Okay. So? So everconfirmed should either be killed entierly, or it should be taken into account to display *either* UNCONFIRMED or REOPENED.

Comment on attachment 408685 [details] [diff] [review] v3 >Index: Bugzilla/Bug.pm > if (ref $invocant) { >- @valid_statuses = @{$invocant->status->can_change_to}; >+ @valid_statuses = @{$invocant->statuses_available}; Please fix my first comment from my previous review. No need to look for UNCONFIRMED twice. >- # Allow anyone with (product-specific) "editbugs" privs to change anything. >- if ($user->in_group('editbugs', $self->{'product_id'})) { >- return 1; >- } As discussed on AIM, this change should be part of another bug. >Index: template/en/default/bug/knob.html.tmpl >+ value = bug.status.name What's the point to call an object when all you want is its name? bug.bug_status already has this information. >+ <noscript><br>resolved&nbsp;as&nbsp;</noscript> It should only be displayed if you can edit the resolution or there is already a resolution set on the bug. Else a powerless user sees "NEW as resolved", which is meaningless.

Okay, this addresses all your comments.

Comment on attachment 414941 [details] [diff] [review] v4 >Index: Bugzilla/Bug.pm >+sub _changes_everconfirmed { >+ return 1 if $new eq 'UNCONFIRMED' Nit: missing semicolon at the end of the line. r=LpSolit, despite I think it would be good to know if a resolved bug was confirmed or not, to know what to choose when reopening it.

(In reply to comment #19) > r=LpSolit, despite I think it would be good to know if a resolved bug was > confirmed or not, to know what to choose when reopening it. Yeah, that's fine, but if we want that, we should implement it in the Status Workflow system.

Checking in Bugzilla/Bug.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v <-- Bug.pm new revision: 1.304; previous revision: 1.303 done Checking in Bugzilla/Status.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Status.pm,v <-- Status.pm new revision: 1.13; previous revision: 1.12 done Checking in template/en/default/bug/knob.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/knob.html.tmpl,v <-- knob.html.tmpl new revision: 1.45; previous revision: 1.44 done

Added to the release notes in bug 547466.