Closed Bug 1007605 Opened 10 years ago Closed 9 years ago

Make "FIXED" non-fixed, by changing noresolveonopenblockers to define what the fixed resolution is

Categories

(Bugzilla :: Administration, task)

task
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 6.0

People

(Reporter: gerv, Assigned: gerv)

References

Details

(Keywords: relnote)

Attachments

(2 files, 1 obsolete file)

At the moment, it's not permitted to delete or alter the name of the FIXED resolution (ChoiceInterface.pm around line 88):

sub is_static {
...
    if ($self->field->name eq 'resolution') {
        return grep($_ eq $self->name, ('', 'FIXED', 'DUPLICATE'))
               ? 1 : 0;
    }
...
}

Grepping the code, the only reason for this I can find is that the FIXED resolution is hard-coded in the implementation of the "noresolveonopenblockers" param. 

We should change "noresolveonopenblockers" so that it's no longer a boolean, but instead a choice of resolutions, such that if you set it, it's not possible to set a bug to that chosen resolution without resolving all the open blockers. That way, we can un-hard-code "FIXED", and allow implementations to delete it or change its name.

Gerv
Severity: normal → enhancement
OS: Linux → All
Hardware: x86 → All
Attached patch Patch v.1 (obsolete) — Splinter Review
There are a few knock-on effects, but it's all a fairly straightforward param upgrade. I think the added flexibility of being able to rename or change all resolutions apart from DUPLICATE is very helpful.

Gerv
Assignee: administration → gerv
Status: NEW → ASSIGNED
Attachment #8531211 - Flags: review?(glob)
Comment on attachment 8531211 [details] [diff] [review]
Patch v.1

Review of attachment 8531211 [details] [diff] [review]:
-----------------------------------------------------------------

this generally looks good, just a few minor things to address.

::: Bugzilla/Config.pm
@@ +213,5 @@
>      }
>  
> +    if (exists $param->{'noresolveonopenblockers'}) {
> +        $new_params{'resolution_forbidden_with_open_blockers'} =
> +                            $param->{'noresolveonopenblockers'} ? 'FIXED' : "";

nit: no need to wrap this line

::: Bugzilla/Config/BugChange.pm
@@ +30,5 @@
>        # If no closed state was found, use the default list above.
>        @closed_bug_statuses = @current_closed_states if scalar(@current_closed_states);
>    };
>  
> +  my $resolution_field = new Bugzilla::Field({ name => 'resolution' });

please don't use old style perl constructors, and always use the cache where possible:
Bugzilla::Field->new({ name => 'resolution, cache => 1 })

@@ +83,2 @@
>    } );
> +  

remove trailing whitespace

::: Bugzilla/Config/Common.pm
@@ +178,5 @@
>  }
>  
> +sub check_resolution {
> +    my $resolution = shift;
> +    my $resolution_field = new Bugzilla::Field({ name => 'resolution' });

same point here regarding ->new and caching

@@ +181,5 @@
> +    my $resolution = shift;
> +    my $resolution_field = new Bugzilla::Field({ name => 'resolution' });
> +    # The empty resolution is included - it represents "no value"
> +    my @resolutions = map {$_->name} @{ $resolution_field->legal_values };
> +   

remove trailing whitespace

::: Bugzilla/Field/ChoiceInterface.pm
@@ +92,1 @@
>                 ? 1 : 0;

this would be better as
return $self->name eq '' || $self->name eq 'DUPLICATE'

::: docs/en/rst/administration.rst
@@ -210,5 @@
>         database users than having a developer mark a bug "fixed" without
>         any comment as to what the fix was (or even that it was truly
>         fixed!)
>  
> -noresolveonopenblockers

you've removed documentation for this param, but didn't add docs for the param while replaces it.

::: docs/en/rst/api/core/v1/bugzilla.rst
@@ -202,5 @@
>            "maintainer" : "admin@example.com",
>            "maxattachmentsize" : "1000",
>            "maxlocalattachment" : "0",
>            "musthavemilestoneonaccept" : "0",
> -          "noresolveonopenblockers" : "0",

the new parameter is returned, so it should be added to the example response

@@ -240,5 @@
>  * maintainer
>  * maxattachmentsize
>  * maxlocalattachment
>  * musthavemilestoneonaccept
> -* noresolveonopenblockers

similarly resolution_forbidden_with_open_blockers should be added here

::: template/en/default/admin/params/bugchange.html.tmpl
@@ +36,5 @@
>  
>    commentonduplicate => "If this option is on, the user needs to enter a short comment " _
>                          "if the $terms.bug is marked as duplicate.",
>  
> +  resolution_forbidden_with_open_blockers => "Don\'t allow $terms.bugs to be resolved with " _

nit: no need to escape the ' in Don't
Attachment #8531211 - Flags: review?(glob) → review-
Attached patch Patch v.2Splinter Review
Review comments addressed.

Gerv
Attachment #8531211 - Attachment is obsolete: true
Attachment #8580120 - Flags: review?(glob)
Comment on attachment 8580120 [details] [diff] [review]
Patch v.2

Review of attachment 8580120 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob
Attachment #8580120 - Flags: review?(glob) → review+
Flags: approval+
Target Milestone: --- → Bugzilla 6.0
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla
   2c82105..79ec299  master -> master

Gerv
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 8580120 [details] [diff] [review]
Patch v.2

>+++ b/Bugzilla/Config/BugChange.pm

>+  my $resolution_field = Bugzilla::Field->new({ name => 'resolution', cache => 1 });
>+  # The empty resolution is included - it represents "no value"
>+  my @resolutions = map {$_->name} @{ $resolution_field->legal_values };


checksetup.pl now crashes when installing Bugzilla for the first time, because the fielddefs table hasn't been populated yet, and so you get:

  Can't call method "legal_values" on an undefined value at Bugzilla/Config/BugChange.pm line 37.
Attached patch fix bustageSplinter Review
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   a29f798..52ff6aa  master -> master


Unrelated to the crash, but related to the original patch: the description of the new parameter could be clearer and should specify that leaving this parameter empty disables the feature.
Attachment #8587966 - Flags: review+
Sorry :-( 

Filed bug 1151396 for improvements to parameter text.

Gerv
Blocks: 1151396
Keywords: relnote
Blocks: 1169767
I updated the documentation now that FIXED is no longer a hardcoded resolution. (I also fixed a link pointing to landfill, so that the user can choose which Bugzilla version he wants to play with. Unrelated to this bug, I know. ;))

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   f798c04..c688265  master -> master
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: