Bug 567846: Modify set_status, set_resolution, and set_dup_id to use

VALIDATOR_DEPENDENCIES, so that they don't need custom code in set_all.

5 files changed, 44 insertions, 30 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 8705d3805..378e219ff 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -192,11 +192,13 @@ sub VALIDATOR_DEPENDENCIES {
 
     my %deps = (
         assigned_to      => ['component'],
-        bug_status       => ['product', 'comment'],
+        bug_status       => ['product', 'comment', 'target_milestone'],
         cc               => ['component'],
         component        => ['product'],
+        dup_id           => ['bug_status', 'resolution'],
         groups           => ['product'],
         keywords         => ['product'],
+        resolution       => ['bug_status'],
         qa_contact       => ['component'],
         target_milestone => ['product'],
         version          => ['product'],
@@ -1971,7 +1973,6 @@ sub set_all {
     my %normal_set_all;
     foreach my $name (keys %$params) {
         # These are handled separately below.
-        next if grep($_ eq $name, qw(status resolution dup_id));
         if ($self->can("set_$name")) {
             $normal_set_all{$name} = $params->{$name};
         }
@@ -2010,22 +2011,6 @@ sub set_all {
     {
         ThrowUserError('dupe_not_allowed');
     }
-
-    # Seting the status, resolution, and dupe_of has to be done
-    # down here, because the validity of status changes depends on
-    # other fields, such as Target Milestone.
-    if (exists $params->{'status'}) {
-        $self->set_status($params->{'status'},
-            { resolution => $params->{'resolution'},
-              dupe_of    => $params->{'dup_id'} });
-    }
-    elsif (exists $params->{'resolution'}) {
-       $self->set_resolution($params->{'resolution'},
-           { dupe_of => $params->{'dup_id'} });
-    }
-    elsif (exists $params->{'dup_id'}) {
-        $self->set_dup_id($params->{'dup_id'});
-    }
 }
 
 # Helper for set_all that helps with fields that have an "add/remove"
@@ -2120,6 +2105,21 @@ sub set_dup_id {
     $self->set('dup_id', $dup_id);
     my $new = $self->dup_id;
     return if $old == $new;
+
+    # Make sure that we have the DUPLICATE resolution. This is needed
+    # if somebody calls set_dup_id without calling set_bug_status or
+    # set_resolution.
+    if ($self->resolution ne 'DUPLICATE') {
+        # Even if the current status is VERIFIED, we change it back to
+        # RESOLVED (or whatever the duplicate_or_move_bug_status is) here,
+        # because that's the same thing the UI does when you click on the
+        # "Mark as Duplicate" link. If people really want to retain their
+        # current status, they can use set_bug_status and set the DUPLICATE
+        # resolution before getting here.
+        $self->set_bug_status(
+            Bugzilla->params->{'duplicate_or_move_bug_status'},
+            { resolution => 'DUPLICATE' });
+    }
     
     # Update the other bug.
     my $dupe_of = new Bugzilla::Bug($self->dup_id);
@@ -2343,13 +2343,17 @@ sub set_resolution {
     # of another, theoretically. Note that this code block will also run
     # when going between different closed states.
     if ($self->resolution eq 'DUPLICATE') {
-        if ($params->{dupe_of}) {
-            $self->set_dup_id($params->{dupe_of});
+        if (my $dup_id = $params->{dup_id}) {
+            $self->set_dup_id($dup_id);
         }
         elsif (!$self->dup_id) {
             ThrowUserError('dupe_id_required');
         }
     }
+
+    # This method has handled dup_id, so set_all doesn't have to worry
+    # about it now.
+    delete $params->{dup_id};
 }
 sub clear_resolution {
     my $self = shift;
@@ -2360,7 +2364,7 @@ sub clear_resolution {
     $self->_clear_dup_id; 
 }
 sub set_severity       { $_[0]->set('bug_severity',  $_[1]); }
-sub set_status {
+sub set_bug_status {
     my ($self, $status, $params) = @_;
     my $old_status = $self->status;
     $self->set('bug_status', $status);
@@ -2368,11 +2372,15 @@ sub set_status {
     delete $self->{'statuses_available'};
     delete $self->{'choices'};
     my $new_status = $self->status;
-    
+   
     if ($new_status->is_open) {
         # Check for the everconfirmed transition
         $self->_set_everconfirmed($new_status->name eq 'UNCONFIRMED' ? 0 : 1);
         $self->clear_resolution();
+        # Calling clear_resolution handled the "resolution" and "dup_id"
+        # setting, so set_all doesn't have to worry about them.
+        delete $params->{resolution};
+        delete $params->{dup_id};
     }
     else {
         # We do this here so that we can make sure closed statuses have
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index ba5b82f9f..67517c405 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -316,13 +316,20 @@ sub set {
 sub set_all {
     my ($self, $params) = @_;
 
-    my @sorted_names = $self->_sort_by_dep(keys %$params);
+    # Don't let setters modify the values in $params for the caller.
+    my %field_values = %$params;
+
+    my @sorted_names = $self->_sort_by_dep(keys %field_values);
     foreach my $key (@sorted_names) {
+        # It's possible for one set_ method to delete a key from $params
+        # for another set method, so if that's happened, we don't call the
+        # other set method.
+        next if !exists $field_values{$key};
         my $method = "set_$key";
-        $self->$method($params->{$key});
+        $self->$method($field_values{$key}, \%field_values);
     }
-    Bugzilla::Hook::process('object_end_of_set_all', { object => $self,
-                                                       params => $params });
+    Bugzilla::Hook::process('object_end_of_set_all', 
+                            { object => $self, params => \%field_values });
 }
 
 sub update {
diff --git a/attachment.cgi b/attachment.cgi
index b650a3522..cdfcc6bf7 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -515,7 +515,7 @@ sub insert {
           && ($bug_status->name ne 'UNCONFIRMED' 
               || $bug->product_obj->allows_unconfirmed))
       {
-          $bug->set_status($bug_status->name);
+          $bug->set_bug_status($bug_status->name);
           $bug->clear_resolution();
       }
       # Make sure the person we are taking the bug from gets mail.
diff --git a/extensions/Voting/Extension.pm b/extensions/Voting/Extension.pm
index 97e061313..24ac4fdb5 100644
--- a/extensions/Voting/Extension.pm
+++ b/extensions/Voting/Extension.pm
@@ -844,7 +844,7 @@ sub _confirm_if_vote_confirmed {
             }
             ThrowCodeError('voting_no_open_bug_status') unless $new_status;
 
-            # We cannot call $bug->set_status() here, because a user without
+            # We cannot call $bug->set_bug_status() here, because a user without
             # canconfirm privs should still be able to confirm a bug by
             # popular vote. We already know the new status is valid, so it's safe.
             $bug->{bug_status} = $new_status;
diff --git a/process_bug.cgi b/process_bug.cgi
index e71c7ef4d..bb4a9f653 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -262,7 +262,6 @@ my %field_translation = (
     set_default_qa_contact => 'reset_qa_contact',
     keywordaction => 'keywords_action',
     confirm_product_change => 'product_change_confirmed',
-    bug_status => 'status',
 );
 
 my %set_all_fields = ( other_bugs => \@bug_objects );
@@ -409,7 +408,7 @@ if ($move_action eq Bugzilla->params->{'move-button-text'}) {
 
     my $new_status = Bugzilla->params->{'duplicate_or_move_bug_status'};
     foreach my $bug (@bug_objects) {
-        $bug->set_status($new_status, {resolution => 'MOVED', moving => 1});
+        $bug->set_bug_status($new_status, {resolution => 'MOVED', moving => 1});
     }
     $_->update() foreach @bug_objects;
     $dbh->bz_commit_transaction();