Bug 512606: Make statuses currently available for the user to change this bug to be controlled by Bugzilla::Bug, not the template

Patch by Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=LpSolit

3 files changed, 91 insertions, 145 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 83e95aecd..2c3a11e8c 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -1110,7 +1110,7 @@ sub _check_bug_status {
     my $old_status; # Note that this is undef for new bugs.
 
     if (ref $invocant) {
-        @valid_statuses = @{$invocant->status->can_change_to};
+        @valid_statuses = @{$invocant->statuses_available};
         $product = $invocant->product_obj;
         $old_status = $invocant->status;
         my $comments = $invocant->{added_comments} || [];
@@ -1118,12 +1118,11 @@ sub _check_bug_status {
     }
     else {
         @valid_statuses = @{Bugzilla::Status->can_change_to()};
-    }
-
-    if (!$product->votes_to_confirm) {
-        # UNCONFIRMED becomes an invalid status if votes_to_confirm is 0,
-        # even if you are in editbugs.
-        @valid_statuses = grep {$_->name ne 'UNCONFIRMED'} @valid_statuses;
+        if (!$product->votes_to_confirm) {
+            # UNCONFIRMED becomes an invalid status if votes_to_confirm is 0,
+            # even if you are in editbugs.
+            @valid_statuses = grep {$_->name ne 'UNCONFIRMED'} @valid_statuses;
+        }
     }
 
     # Check permissions for users filing new bugs.
@@ -2167,6 +2166,8 @@ sub set_status {
     my $old_status = $self->status;
     $self->set('bug_status', $status);
     delete $self->{'status'};
+    delete $self->{'statuses_available'};
+    delete $self->{'choices'};
     my $new_status = $self->status;
     
     if ($new_status->is_open) {
@@ -2680,11 +2681,6 @@ sub isopened {
     return is_open_state($self->{bug_status}) ? 1 : 0;
 }
 
-sub isunconfirmed {
-    my $self = shift;
-    return ($self->bug_status eq 'UNCONFIRMED') ? 1 : 0;
-}
-
 sub keywords {
     my ($self) = @_;
     return join(', ', (map { $_->name } @{$self->keyword_objects}));
@@ -2805,6 +2801,31 @@ sub status {
     return $self->{'status'};
 }
 
+sub statuses_available {
+    my $self = shift;
+    return [] if $self->{'error'};
+    return $self->{'statuses_available'}
+        if defined $self->{'statuses_available'};
+
+    my @statuses = @{ $self->status->can_change_to };
+
+    # UNCONFIRMED is only a valid status if it is enabled in this product.
+    if (!$self->product_obj->votes_to_confirm) {
+        @statuses = grep { $_->name ne 'UNCONFIRMED' } @statuses;
+    }
+
+    my @available;
+    foreach my $status (@statuses) {
+        # Make sure this is a legal status transition
+        next if !$self->check_can_change_field(
+                     'bug_status', $self->status->name, $status->name);
+        push(@available, $status);
+    }
+
+    $self->{'statuses_available'} = \@available;
+    return $self->{'statuses_available'};
+}
+
 sub show_attachment_flags {
     my ($self) = @_;
     return $self->{'show_attachment_flags'} 
@@ -2958,9 +2979,10 @@ sub choices {
     }
 
     my %choices = (
-        product   => \@products,
-        component => $self->product_obj->components,
-        version   => $self->product_obj->versions,
+        bug_status => $self->statuses_available,
+        product    => \@products,
+        component  => $self->product_obj->components,
+        version    => $self->product_obj->versions,
         target_milestone => $self->product_obj->milestones,
     );
 
@@ -3451,12 +3473,7 @@ sub check_can_change_field {
     }
 
     # *Only* users with (product-specific) "canconfirm" privs can confirm bugs.
-    if ($field eq 'canconfirm'
-        || ($field eq 'everconfirmed' && $newvalue)
-        || ($field eq 'bug_status'
-            && $oldvalue eq 'UNCONFIRMED'
-            && is_open_state($newvalue)))
-    {
+    if ($self->_changes_everconfirmed($field, $oldvalue, $newvalue)) {
         $$PrivilegesRequired = 3;
         return $user->in_group('canconfirm', $self->{'product_id'});
     }
@@ -3532,6 +3549,24 @@ sub check_can_change_field {
     return 0;
 }
 
+# A helper for check_can_change_field
+sub _changes_everconfirmed {
+    my ($self, $field, $old, $new) = @_;
+    return 1 if $field eq 'everconfirmed';
+    if ($field eq 'bug_status') {
+        if ($self->everconfirmed) {
+            # Moving a confirmed bug to UNCONFIRMED will change everconfirmed.
+            return 1 if $new eq 'UNCONFIRMED';
+        }
+        else {
+            # Moving an unconfirmed bug to an open state that isn't 
+            # UNCONFIRMED will confirm the bug.
+            return 1 if (is_open_state($new) and $new ne 'UNCONFIRMED');
+        }
+    }
+    return 0;
+}
+
 #
 # Field Validation
 #
@@ -3626,8 +3661,7 @@ sub _validate_attribute {
     my @valid_attributes = (
         # Miscellaneous properties and methods.
         qw(error groups product_id component_id
-           comments milestoneurl attachments
-           isopened isunconfirmed
+           comments milestoneurl attachments isopened
            flag_types num_attachment_flag_types
            show_attachment_flags any_flags_requesteeble),
 
diff --git a/Bugzilla/Status.pm b/Bugzilla/Status.pm
index 4720dc129..4d1281e7e 100644
--- a/Bugzilla/Status.pm
+++ b/Bugzilla/Status.pm
@@ -176,28 +176,6 @@ sub can_change_to {
     return $self->{'can_change_to'};
 }
 
-sub can_change_from {
-    my $self = shift;
-    my $dbh = Bugzilla->dbh;
-
-    if (!defined $self->{'can_change_from'}) {
-        my $old_status_ids = $dbh->selectcol_arrayref('SELECT old_status
-                                                         FROM status_workflow
-                                                   INNER JOIN bug_status
-                                                           ON id = old_status
-                                                        WHERE isactive = 1
-                                                          AND new_status = ?
-                                                          AND old_status IS NOT NULL',
-                                                        undef, $self->id);
-
-        # Allow the bug status to remain unchanged.
-        push(@$old_status_ids, $self->id);
-        $self->{'can_change_from'} = Bugzilla::Status->new_from_list($old_status_ids);
-    }
-
-    return $self->{'can_change_from'};
-}
-
 sub comment_required_on_change_from {
     my ($self, $old_status) = @_;
     my ($cond, $values) = $self->_status_condition($old_status);
@@ -300,17 +278,6 @@ below.
 
  Returns:     A list of Bugzilla::Status objects.
 
-=item C<can_change_from>
-
- Description: Returns the list of active statuses a bug can be changed from
-              given the new bug status. If the bug status is available on
-              bug creation, this method doesn't return this information.
-              You have to call C<can_change_to> instead.
-
- Params:      none.
-
- Returns:     A list of Bugzilla::Status objects.
-
 =item C<comment_required_on_change_from>
 
 =over
diff --git a/template/en/default/bug/knob.html.tmpl b/template/en/default/bug/knob.html.tmpl
index 10d58e51b..ac14e6dc0 100644
--- a/template/en/default/bug/knob.html.tmpl
+++ b/template/en/default/bug/knob.html.tmpl
@@ -23,71 +23,33 @@
 
 [% PROCESS global/variables.none.tmpl %]
 <div id="status">
-  [% initial_action_shown = 0 %]
-  [% show_resolution = 0 %]
-  [% bug_status_select_displayed = 0 %]
+  [% PROCESS bug/field.html.tmpl
+      no_tds = 1
+      field  = bug_fields.bug_status
+      value  = bug.bug_status
+      override_legal_values = bug.choices.bug_status
+      editable = bug.choices.bug_status.size > 1
+  %]
 
-  [% closed_status_array = [] %]
-  [%# These actions are based on the current custom workflow. %]
-  [% FOREACH bug_status = bug.status.can_change_to %]
-    [% NEXT IF bug.isunconfirmed && bug_status.is_open && !bug.user.canconfirm %]
-    [% NEXT IF bug.isopened && !bug.isunconfirmed && bug_status.is_open && !bug.user.canedit %]
-    [% NEXT IF (!bug_status.is_open || !bug.isopened) && !bug.user.canedit && !bug.user.isreporter %]
-    [%# Special hack to only display UNCO or REOP when reopening, but not both;
-      # for compatibility with older versions. %]
-    [% NEXT IF !bug.isopened && (bug.everconfirmed && bug_status.name == "UNCONFIRMED"
-                                 || !bug.everconfirmed && bug_status.name == "REOPENED") %]
-    [% IF NOT bug_status_select_displayed %]
-      <select name="bug_status" id="bug_status">
-      [% bug_status_select_displayed = 1 %]
-    [% END %]
-    [% PROCESS initial_action %]
-    [% NEXT IF bug_status.name == bug.bug_status %]
-    <option value="[% bug_status.name FILTER html %]">
-      [% display_value("bug_status", bug_status.name) FILTER html %]
-    </option>
-    [% IF  !bug_status.is_open  %]
-      [% show_resolution = 1 %]
-      [% filtered_status = bug_status.name FILTER js %]
-      [% closed_status_array.push( filtered_status ) %]
-    [% END %]
+  [% IF bug.resolution 
+        OR bug.check_can_change_field('resolution', bug.resolution, 1)
+  %]
+    <noscript><br>resolved&nbsp;as&nbsp;</noscript>
   [% END %]
 
-  [%# These actions are special and are independent of the workflow. %]
-  [% IF bug.user.canedit || bug.user.isreporter %]
-    [% IF NOT bug_status_select_displayed %]
-      <select name="bug_status" id="bug_status">
-      [% bug_status_select_displayed = 1 %] 
-    [% END %]
-    [% IF bug.isopened %]
-      [% IF bug.resolution %]
-        [% PROCESS initial_action %]
-      [% END %]
-    [% ELSIF bug.resolution != "MOVED" || bug.user.canmove  %]
-        [% PROCESS initial_action %]
-        [% show_resolution = 1 %]
-    [% END %]
-  [% END %]  
-  [% IF bug_status_select_displayed %]
-    </select>
-  [% ELSE %]
-      [% display_value("bug_status", bug.bug_status) FILTER html %]
-      [% IF bug.resolution %]
-        [%+ display_value("resolution", bug.resolution) FILTER html %]
-        [% IF bug.dup_id %]
-          <span id="duplicate_display">of 
-          [% "${terms.bug} ${bug.dup_id}" FILTER bug_link(bug.dup_id) FILTER none %]</span>
-        [% END %]
-      [% END %]
-  [% END %]
-  [% IF bug.user.canedit || bug.user.isreporter %]  
-    [% IF show_resolution %]
-      <noscript><br>resolved&nbsp;as&nbsp;</noscript>
-      <span id="resolution_settings">[% PROCESS select_resolution %]</span>
-    [% END %]
+  <span id="resolution_settings">
+  [% PROCESS bug/field.html.tmpl
+      no_tds = 1
+      field  = bug_fields.resolution
+      value  = bug.resolution
+      override_legal_values = bug.choices.resolution
+      editable = bug.check_can_change_field('resolution', bug.resolution, 1)
+  %]
+  </span>
+
+  [% IF bug.check_can_change_field('dup_id', 0, 1) %]
     <noscript><br> duplicate</noscript>
-    
-    <span id="duplicate_settings">of 
+    <span id="duplicate_settings">of
       <span id="dup_id_container" class="bz_default_hidden">
         [% "${terms.bug} ${bug.dup_id}" FILTER bug_link(bug.dup_id) FILTER none %]
         (<a href="#" id="dup_id_edit_action">edit</a>)
@@ -101,11 +63,20 @@
     <div id="dup_id_discoverable" class="bz_default_hidden">
       <a href="#" id="dup_id_discoverable_action">Mark as Duplicate</a>
     </div>
+  [% ELSIF bug.dup_id %]
+    <noscript><br> duplicate</noscript>
+    <span id="duplicate_display">of 
+      [% "${terms.bug} ${bug.dup_id}" FILTER bug_link(bug.dup_id) FILTER none %]</span>
   [% END %]
 </div>
+
 <script type="text/javascript">
-  var close_status_array = new Array("[% closed_status_array.join('", "') FILTER replace(',$', '')
-                                                                FILTER none %]");
+  var close_status_array = [
+    [% FOREACH status = bug.choices.bug_status %]
+      [% NEXT IF status.is_open %]
+      '[% status.name FILTER js %]'[% ',' UNLESS loop.last %]
+    [% END %]
+  ];
   YAHOO.util.Dom.removeClass('dup_id_discoverable', 'bz_default_hidden');
   hideEditableField( "dup_id_container", "dup_id", 'dup_id_edit_action',
                      'dup_id', '[% bug.dup_id FILTER js %]' )
@@ -127,29 +98,3 @@
   [% INCLUDE "bug/field-events.js.tmpl" field = select_fields.bug_status %]
   [% INCLUDE "bug/field-events.js.tmpl" field = select_fields.resolution %]
 </script>
-
-[%# Common actions %]
-
-[% BLOCK initial_action %]
-  [% IF !initial_action_shown %]
-    <option selected value="[% bug.bug_status FILTER html %]">
-      [% display_value("bug_status", bug.bug_status) FILTER html %]
-    </option>
-    [% IF !bug.isopened  %] 
-      [% show_resolution = 1 %]
-      [% filtered_status = bug.bug_status FILTER js %]
-      [% closed_status_array.push(filtered_status) %]
-    [% END %]
-    [% initial_action_shown = 1 %]
-  [% END %]
-[% END %]
-
-[% BLOCK select_resolution %]
-  <select name="resolution" id="resolution">
-    [% FOREACH r = bug.choices.resolution %]
-      <option value="[% r.name FILTER html %]"
-      [% ' selected="selected"' IF r.name == bug.resolution %]>
-        [% display_value("resolution", r.name) FILTER html %]</option>
-    [% END %]
-  </select>
-[% END %]