512648 - Bugzilla::Bug should centrally control available statuses in enter_bug.cgi

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Max Kanat-Alexander]

Right now there's pretty much duplicate code in enter_bug.cgi and Bugzilla::Bug::_check_bug_status to determine which statuses are valid for this user when filing a bug. It should be centralized in Bugzilla::Bug so that people can make changes to it in just one place and have it be effective everywhere.

Comment 1

[Frédéric Buclin]

We no longer accept new features for Bugzilla 3.6. Retargetting to 3.8.

Comment 2

[Robert Webb]

Attachment: Proposed refactoring patch v1

This is a first go at refactoring the default bug status code. Unfortunately I had to make some choices that I thought were rather hacky. Because enter_bug requires both a list of available new bug statuses in addition to a default bug status, I had to make two methods to get these.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 560635 [details] [diff] [review]
Proposed refactoring patch v1

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

Overall, I like this approach. I don't feel too much like it's a hack, at all, actually.  :-)

::: Bugzilla/Bug.pm
@@ +3222,5 @@
>      return $self->{classification};
>  }
>  
> +sub default_bug_status {
> +    my ($class) = shift;

You don't need the ( ) around $class there.

@@ +3223,5 @@
>  }
>  
> +sub default_bug_status {
> +    my ($class) = shift;
> +    my @statuses = @_;

Hmm, could you just call new_bug_statuses inside of this method? I think that would be cleaner.

@@ +3226,5 @@
> +    my ($class) = shift;
> +    my @statuses = @_;
> +
> +    my $status;
> +    if (scalar @statuses == 1) {

parens: scalar(@statuses) == 1

(for clarity)

@@ +3354,5 @@
>      return \@comments;
>  }
>  
> +sub new_bug_statuses {
> +    my ($class, $product, $user) = @_;

Instead of passing $user, you should use Bugzilla->user.

@@ +3361,5 @@
> +    my @statuses = @{ Bugzilla::Bug->statuses_available($product) };
> +
> +    # If the user has no privs...
> +    unless ($user->in_group('editbugs', $product->id) ||
> +            $user->in_group('canconfirm', $product->id)) {

The open-curly-brace gets aligned with the "u" in "unless" for this.

@@ +3486,5 @@
> +    
> +        # If this bug has an inactive status set, it should still be in the list.
> +        if (!grep($_->name eq $invocant->status->name, @available)) {
> +            unshift(@available, $invocant->status);
> +        }

Could you break the above bits out into a separate helper function, just for readability's sake?

Comment 4

[Robert Webb]

Attachment: Proposed refactoring patch v2

Made the requested changes.

Comment 5

[Frédéric Buclin]

Comment on attachment 561313 [details] [diff] [review]
Proposed refactoring patch v2

>=== modified file 'Bugzilla/Bug.pm'

>+    unless ($user->in_group('editbugs', $product->id) ||
>+            $user->in_group('canconfirm', $product->id))

Please put || at the beginning of the next line.


>+        $invocant->{'statuses_available'} = $available;
>+        return $invocant->{'statuses_available'};

Nit: you could simply write: return $available.


>+sub _refine_avialable_statuses {

Typo in the name: avialable -> available


>+    my $invocant = shift;

No reason to rename $self to $invocant. This method is only called for existing bugs.


Otherwise looks good.

Comment 6

[Robert Webb]

Attachment: Proposed refactoring patch v3

Requested changes have been made.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 561338 [details] [diff] [review]
Proposed refactoring patch v3

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

::: Bugzilla/Bug.pm
@@ +1283,4 @@
>      else {
>          $product = $params->{product};
>          $comment = $params->{comment};
> +        @valid_statuses = @{Bugzilla::Bug->statuses_available($product)};

Nit: Space after { and before }.

@@ +3339,4 @@
>      return \@comments;
>  }
>  
> +sub new_bug_status {

Ah, I'd rather have this called default_bug_status, that makes more sense.

@@ +3340,5 @@
>  }
>  
> +sub new_bug_status {
> +    my $class = shift;
> +    my @statuses = @_;

What about using new_bug_statuses here instead of passing in @statuses?

Comment 8

[Robert Webb]

Attachment: Proposed refactoring patch v4

Comment 9

[Robert Webb]

Attachment: Proposed refactoring patch v4

Comment 10

[Robert Webb]

Attachment: Proposed refactoring patch v4

Comment 11

[Max Kanat-Alexander]

Comment on attachment 561348 [details] [diff] [review]
Proposed refactoring patch v4

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

Looks awesome. :-) Thank you!

::: Bugzilla/Bug.pm
@@ +3466,5 @@
> +    if (ref $invocant) {
> +      return [] if $invocant->{'error'};
> +
> +      return $invocant->{'statuses_available'}
> +        if defined $invocant->{'statuses_available'};

Nit: Four spaces for the indent, there.

Comment 12

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified enter_bug.cgi
modified Bugzilla/Bug.pm
Committed revision 7968.