512606 - Statuses currently available for the user to change this bug to should be controlled by Bugzilla::Bug, not the template

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

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).

Comment 1

[Max Kanat-Alexander]

Attachment: v1

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.

Comment 2

[Max Kanat-Alexander]

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

Comment 3

[Max Kanat-Alexander]

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 4

[Frédéric Buclin]

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).

Comment 5

[Max Kanat-Alexander]

Attachment: v2

Okay, this fixes the bitrot.

Comment 6

[Max Kanat-Alexander]

Attachment: v2.1

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

Comment 7

[Frédéric Buclin]

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!

Comment 8

[Max Kanat-Alexander]

(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).

Comment 9

[Max Kanat-Alexander]

Attachment: v3

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.

Comment 10

[Frédéric Buclin]

(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.

Comment 11

[Max Kanat-Alexander]

(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.

Comment 12

[Frédéric Buclin]

(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.

Comment 13

[Max Kanat-Alexander]

(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?

Comment 14

[Max Kanat-Alexander]

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

Comment 15

[Max Kanat-Alexander]

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

  *patch :-)

Comment 16

[Frédéric Buclin]

(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 17

[Frédéric Buclin]

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.

Comment 18

[Max Kanat-Alexander]

Attachment: v4

Okay, this addresses all your comments.

Comment 19

[Frédéric Buclin]

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.

Comment 20

[Max Kanat-Alexander]

(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.

Comment 21

[Max Kanat-Alexander]

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

Comment 22

[Max Kanat-Alexander]

Added to the release notes in bug 547466.