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

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Creating/Changing Bugs
--
enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)

Tracking

Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

(Whiteboard: [relnote comment 8])

Attachments

(1 attachment, 4 obsolete attachments)

v4
13.14 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Updated

9 years ago
Depends on: 512618
(Assignee)

Updated

9 years ago
Depends on: 512623
(Assignee)

Comment 1

9 years ago
Created attachment 396679 [details] [diff] [review]
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.
Assignee: create-and-change → mkanat
Status: NEW → ASSIGNED
Attachment #396679 - Flags: review?(LpSolit)
(Assignee)

Comment 2

9 years ago
Oh, and of course, this requires both blocking patches to be applied first.
(Assignee)

Updated

9 years ago
Blocks: 512648
(Assignee)

Comment 3

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

Updated

9 years ago
Blocks: 162060

Comment 4

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

Comment 5

9 years ago
Created attachment 403901 [details] [diff] [review]
v2

Okay, this fixes the bitrot.
Attachment #396679 - Attachment is obsolete: true
Attachment #403901 - Flags: review?(LpSolit)
(Assignee)

Comment 6

9 years ago
Created attachment 403907 [details] [diff] [review]
v2.1

Updated to apply against the latest patch on the dependent bug.
Attachment #403901 - Attachment is obsolete: true
Attachment #403907 - Flags: review?(LpSolit)
Attachment #403901 - Flags: review?(LpSolit)

Comment 7

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

Comment 8

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

Comment 9

9 years ago
Created attachment 408685 [details] [diff] [review]
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.
Attachment #408685 - Flags: review?(LpSolit)

Comment 10

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

Comment 11

9 years ago
(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

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

Comment 13

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

Comment 14

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

Comment 15

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

  *patch :-)

Comment 16

9 years ago
(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

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

Updated

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

Comment 18

9 years ago
Created attachment 414941 [details] [diff] [review]
v4

Okay, this addresses all your comments.
Attachment #403907 - Attachment is obsolete: true
Attachment #408685 - Attachment is obsolete: true
Attachment #414941 - Flags: review?(LpSolit)
(Assignee)

Updated

9 years ago
Whiteboard: [needs new patch asap]

Comment 19

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

Updated

9 years ago
Flags: approval+
(Assignee)

Comment 20

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

Comment 21

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: relnote
Resolution: --- → FIXED
Whiteboard: [relnote comment 8]
(Assignee)

Comment 22

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