Attachment 296081 Details for Bug 433744 – patch for Bugzilla::Bug::set_all($args) + process_bug.cgi

[patch] patch for Bugzilla::Bug::set_all($args) + process_bug.cgi

patch.diff (text/plain), 21.05 KB, created by Noura El hawary on 2008-02-27 16:03:31 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Noura El hawary

Created: 2008-02-27 16:03:31 UTC

Size: 21.05 KB

patch

obsolete

>Index: process_bug.cgi
>===================================================================
>RCS file: /cvs/qa/rh_bugzilla_3/process_bug.cgi,v
>retrieving revision 1.14
>diff -u -r1.14 process_bug.cgi
>--- process_bug.cgi	13 Feb 2008 16:44:32 -0000	1.14
>+++ process_bug.cgi	27 Feb 2008 15:54:47 -0000
>@@ -241,185 +241,180 @@
>     }
> }
> 
>-# For security purposes, and because lots of other checks depend on it,
>-# we set the product first before anything else.
>-my $product_change; # Used only for strict_isolation checks, right now.
>-if (should_set('product')) {
>-    foreach my $b (@bug_objects) {
>-        my $changed = $b->set_product(scalar $cgi->param('product'),
>-            { component        => scalar $cgi->param('component'),
>-              version          => scalar $cgi->param('version'),
>-              target_milestone => scalar $cgi->param('target_milestone'),
>-              change_confirmed => scalar $cgi->param('confirm_product_change'),
>-              other_bugs => \@bug_objects,
>-            });
>-        $product_change ||= $changed;
>-    }
>-}
>-        
>-# strict_isolation checks mean that we should set the groups
>-# immediately after changing the product.
>+# Noura adds this
> foreach my $b (@bug_objects) {
>+
>+    my $args;
>+
>+    if (should_set('product')) {
>+        $args->{product} = scalar $cgi->param('product');
>+        $args->{component} = scalar $cgi->param('component');
>+        $args->{version} = scalar $cgi->param('version');
>+        $args->{target_milestone} = scalar $cgi->param('target_milestone');
>+        $args->{confirm_product_change} =  scalar $cgi->param('confirm_product_change');        
>+        $args->{bug_objects} = \@bug_objects;
>+    }
>+
>     foreach my $group (@{$b->product_obj->groups_valid}) {
>         my $gid = $group->id;
>         if (should_set("bit-$gid", 1)) {
>             # Check ! first to avoid having to check defined below.
>             if (!$cgi->param("bit-$gid")) {
>-                $b->remove_group($gid);
>+                push (@{$args->{remove_group}}, $gid);
>             }
>             # "== 1" is important because mass-change uses -1 to mean
>             # "don't change this restriction"
>             elsif ($cgi->param("bit-$gid") == 1) {
>-                $b->add_group($gid);
>+                push (@{$args->{add_group}}, $gid);
>             }
>         }
>     }
>-}
> 
>-if ($cgi->param('id') && (defined $cgi->param('dependson')
>+    if ($cgi->param('id') && (defined $cgi->param('dependson')
>                           || defined $cgi->param('blocked')) )
>-{
>-    $first_bug->set_dependencies(scalar $cgi->param('dependson'),
>-                                 scalar $cgi->param('blocked'));
>-}
>-# Right now, you can't modify dependencies on a mass change.
>-else {
>-    $cgi->delete('dependson');
>-    $cgi->delete('blocked');
>-}
>-
>-my $any_keyword_changes;
>-if (defined $cgi->param('keywords')) {
>-    foreach my $b (@bug_objects) {
>-        my $return =
>-            $b->modify_keywords(scalar $cgi->param('keywords'),
>-                                scalar $cgi->param('keywordaction'));
>-        $any_keyword_changes ||= $return;
>+    {
>+        $args->{dependson} = scalar $cgi->param('dependson');
>+        $args->{blocked} = scalar $cgi->param('blocked');
>+    }
>+    # Right now, you can't modify dependencies on a mass change.
>+    else {
>+        $cgi->delete('dependson');
>+        $cgi->delete('blocked');
>     }
>-}
> 
>-# Component, target_milestone, and version are in here just in case
>-# the 'product' field wasn't defined in the CGI. It doesn't hurt to set
>-# them twice.
>-# REDHAT EXTENSION START 406151
>-# Added devel_whiteboard, qa_whiteboard, internal_whiteboard and fixed_in
>-my @set_fields = qw(op_sys rep_platform priority bug_severity
>-                    component target_milestone version
>-                    bug_file_loc status_whiteboard devel_whiteboard
>-                    qa_whiteboard internal_whiteboard fixed_in short_desc
>-                    deadline remaining_time estimated_time);
>-# REDHAT EXTENSION END 406151
>-push(@set_fields, 'assigned_to') if !$cgi->param('set_default_assignee');
>-push(@set_fields, 'qa_contact')  if !$cgi->param('set_default_qa_contact');
>-my @custom_fields = Bugzilla->get_fields({custom => 1, obsolete => 0});
>-
>-my %methods = (
>-    bug_severity => 'set_severity',
>-    rep_platform => 'set_platform',
>-    short_desc   => 'set_summary',
>-    bug_file_loc => 'set_url',
>-);
>-foreach my $b (@bug_objects) {
>-    if (should_set('comment') || $cgi->param('work_time')) {
>-        # Add a comment as needed to each bug. This is done early because
>-        # there are lots of things that want to check if we added a comment.
>-        $b->add_comment(scalar($cgi->param('comment')),
>-            { isprivate => scalar $cgi->param('commentprivacy'),
>-              work_time => scalar $cgi->param('work_time') });
>+    if (defined $cgi->param('keywords')) {
>+        my $keyword_arg_name = scalar $cgi->param('keywordaction') . "_keywords";
>+        $args->{$keyword_arg_name}  = scalar $cgi->param('keywords');
>     }
>+ 
>+    my @custom_fields = Bugzilla->get_fields({custom => 1, obsolete => 0});
>+ 
>+    if (should_set('comment') || $cgi->param('work_time')) {
>+        $args->{comment} = scalar $cgi->param('comment');
>+        $args->{work_time} = $cgi->param('work_time');
>+        $args->{commentprivacy} = scalar $cgi->param('commentprivacy');
>+    }
>+
>+    # Component, target_milestone, and version are in here just in case
>+    # the 'product' field wasn't defined in the CGI. It doesn't hurt to set
>+    # them twice.
>+    # REDHAT EXTENSION START 406151
>+    # Added devel_whiteboard, qa_whiteboard, internal_whiteboard and fixed_in
>+    my @set_fields = qw(op_sys rep_platform priority bug_severity
>+                        component target_milestone version
>+                        bug_file_loc status_whiteboard devel_whiteboard
>+                        qa_whiteboard internal_whiteboard fixed_in short_desc
>+                        deadline remaining_time estimated_time);
>+    # REDHAT EXTENSION END 406151
>+
>     foreach my $field_name (@set_fields) {
>         if (should_set($field_name)) {
>-            my $method = $methods{$field_name};
>-            $method ||= "set_" . $field_name;
>-            $b->$method($cgi->param($field_name));
>+            $args->{$field_name} = $cgi->param($field_name);
>         }
>     }
>-    $b->reset_assigned_to if $cgi->param('set_default_assignee');
>-    $b->reset_qa_contact  if $cgi->param('set_default_qa_contact');
> 
>-    # And set custom fields.
>+    $args->{set_default_assignee} = $cgi->param('set_default_assignee');
>+    $args->{set_default_qa_contact} = $cgi->param('set_default_qa_contact');
>+
>     foreach my $field (@custom_fields) {
>         my $fname = $field->name;
>         if (should_set($fname, 1)) {
>-            $b->set_custom_field($field, [$cgi->param($fname)]);
>+            $args->{$fname} = $cgi->param($fname);
>         }
>-    }
>-}
> 
>-# Certain changes can only happen on individual bugs, never on mass-changes.
>-if (defined $cgi->param('id')) {
>-    # Since aliases are unique (like bug numbers), they can only be changed
>-    # for one bug at a time.
>-    if (Bugzilla->params->{"usebugaliases"} && defined $cgi->param('alias')) {
>-        $first_bug->set_alias($cgi->param('alias'));
>-    }
>-
>-    # reporter_accessible and cclist_accessible--these are only set if
>-    # the user can change them and they appear on the page.
>-    if (should_set('cclist_accessible', 1)) {
>-        $first_bug->set_cclist_accessible($cgi->param('cclist_accessible'))
>-    }
>-    if (should_set('reporter_accessible', 1)) {
>-        $first_bug->set_reporter_accessible($cgi->param('reporter_accessible'))
>     }
>     
>-    # You can only mark/unmark comments as private on single bugs. If
>-    # you're not in the insider group, this code won't do anything.
>-    foreach my $field (grep(/^defined_isprivate/, $cgi->param())) {
>-        $field =~ /(\d+)$/;
>-        my $comment_id = $1;
>-        $first_bug->set_comment_is_private($comment_id,
>-                                           $cgi->param("isprivate_$comment_id"));
>-    }
>-}
>+    # Certain changes can only happen on individual bugs, never on mass-changes.
>+    if (defined $cgi->param('id')) {
>+        if (Bugzilla->params->{"usebugaliases"} && defined $cgi->param('alias')) {
>+            $args->{alias} = $cgi->param('alias');
>+        }
> 
>-# We need to check the addresses involved in a CC change before we touch 
>-# any bugs. What we'll do here is formulate the CC data into two arrays of
>-# users involved in this CC change.  Then those arrays can be used later 
>-# on for the actual change.
>-my (@cc_add, @cc_remove);
>-if (defined $cgi->param('newcc')
>-    || defined $cgi->param('addselfcc')
>-    || defined $cgi->param('removecc')
>-    || defined $cgi->param('masscc')) {
>-        
>-    # If masscc is defined, then we came from buglist and need to either add or
>-    # remove cc's... otherwise, we came from bugform and may need to do both.
>-    my ($cc_add, $cc_remove) = "";
>-    if (defined $cgi->param('masscc')) {
>-        if ($cgi->param('ccaction') eq 'add') {
>-            $cc_add = join(' ',$cgi->param('masscc'));
>-        } elsif ($cgi->param('ccaction') eq 'remove') {
>-            $cc_remove = join(' ',$cgi->param('masscc'));
>-        }
>-    } else {
>-        $cc_add = join(' ',$cgi->param('newcc'));
>-        # We came from bug_form which uses a select box to determine what cc's
>-        # need to be removed...
>-        if (defined $cgi->param('removecc') && $cgi->param('cc')) {
>-            $cc_remove = join (",", $cgi->param('cc'));
>+        # reporter_accessible and cclist_accessible--these are only set if
>+        # the user can change them and they appear on the page.
>+        if (should_set('cclist_accessible', 1)) {
>+            $args->{cclist_accessible} = $cgi->param('cclist_accessible');
>+        }
>+        if (should_set('reporter_accessible', 1)) {
>+            $args->{reporter_accessible} = $cgi->param('reporter_accessible');
>         }
>+
>+        # You can only mark/unmark comments as private on single bugs. If
>+        # you're not in the insider group, this code won't do anything.
>+        foreach my $cgi_field (grep(/^defined_isprivate/, $cgi->param())) {
>+            $cgi_field =~ /(\d+)$/;
>+            $args->{comment_id} = $1;
>+            $args->{comment_privacy} = $cgi->param("isprivate_$args->{comment_id}");
>+        }       
>+    }   
>+
>+    # We need to check the addresses involved in a CC change before we touch 
>+    # any bugs. What we'll do here is formulate the CC data into two arrays of
>+    # users involved in this CC change.  Then those arrays can be used later 
>+    # on for the actual change.
>+    if (defined $cgi->param('newcc')
>+        || defined $cgi->param('addselfcc')
>+        || defined $cgi->param('removecc')
>+        || defined $cgi->param('masscc')) {
>+
>+        # If masscc is defined, then we came from buglist and need to either add or
>+        # remove cc's... otherwise, we came from bugform and may need to do both.
>+        my ($cc_add, $cc_remove) = "";
>+        if (defined $cgi->param('masscc')) {
>+            if ($cgi->param('ccaction') eq 'add') {
>+                $cc_add = join(' ',$cgi->param('masscc'));
>+            } elsif ($cgi->param('ccaction') eq 'remove') {
>+                $cc_remove = join(' ',$cgi->param('masscc'));
>+            }
>+        } else {
>+            $cc_add = join(' ',$cgi->param('newcc'));
>+            # We came from bug_form which uses a select box to determine what cc's
>+            # need to be removed...
>+            if (defined $cgi->param('removecc') && $cgi->param('cc')) {
>+                $cc_remove = join (",", $cgi->param('cc'));
>+            }
>+        }
>+
>+        push(@{$args->{add_cc}}, split(/[\s,]+/, $cc_add)) if $cc_add;
>+        push(@{$args->{add_cc}}, Bugzilla->user) if $cgi->param('addselfcc');
>+
>+        push(@{$args->{remove_cc}}, split(/[\s,]+/, $cc_remove)) if $cc_remove;
>     }
> 
>-    push(@cc_add, split(/[\s,]+/, $cc_add)) if $cc_add;
>-    push(@cc_add, Bugzilla->user) if $cgi->param('addselfcc');
>+    # You cannot mark bugs as duplicates when changing several bugs at once
>+    # (because currently there is no way to check for duplicate loops in that
>+    # situation).
>+    if (!$cgi->param('id') && $cgi->param('dup_id')) {
>+        ThrowUserError('dupe_not_allowed');
>+    }
> 
>-    push(@cc_remove, split(/[\s,]+/, $cc_remove)) if $cc_remove;
>-}
>+    # Set the status, resolution, and dupe_of (if needed). This has to be done
>+    # down here, because the validity of status changes depends on other fields,
>+    # such as Target Milestone.
>+   
>+    if (should_set('knob')) {
>+        # First, get the correct resolution <select>, in case there is more
>+        # than one open -> closed transition allowed. Allow to fallback to
>+        # 'resolution' (useful when called from email_in.pl).
>+        my $knob = $cgi->param('knob');
>+        my $status = new Bugzilla::Status({name => $knob});
>+        my $resolution;
>+        if ($status) {
>+            $resolution = $cgi->param('resolution_knob_' . $status->id)
>+                          || $cgi->param('resolution');
>+        }
>+        else {
>+            $resolution = $cgi->param('resolution_knob_change_resolution');
>+        }
>+        $args->{knob} = $knob;
>+        $args->{resolution} = $resolution;
>+        $args->{dup_id} = scalar $cgi->param('dup_id');
> 
>-foreach my $b (@bug_objects) {
>-    $b->remove_cc($_) foreach @cc_remove;
>-    $b->add_cc($_) foreach @cc_add;
>-    # Theoretically you could move a product without ever specifying
>-    # a new assignee or qa_contact, or adding/removing any CCs. So,
>-    # we have to check that the current assignee, qa, and CCs are still
>-    # valid if we've switched products, under strict_isolation. We can only
>-    # do that here. There ought to be some better way to do this,
>-    # architecturally, but I haven't come up with it.
>-    if ($product_change) {
>-        $b->_check_strict_isolation();
>     }
>+   
>+    $b->set_all($args); 
> }
>+    
> 
> my $move_action = $cgi->param('action') || '';
> if ($move_action eq Bugzilla->params->{'move-button-text'}) {
>@@ -494,38 +489,6 @@
>     exit;
> }
> 
>-
>-# You cannot mark bugs as duplicates when changing several bugs at once
>-# (because currently there is no way to check for duplicate loops in that
>-# situation).
>-if (!$cgi->param('id') && $cgi->param('dup_id')) {
>-    ThrowUserError('dupe_not_allowed');
>-}
>-
>-# Set the status, resolution, and dupe_of (if needed). This has to be done
>-# down here, because the validity of status changes depends on other fields,
>-# such as Target Milestone.
>-foreach my $b (@bug_objects) {
>-    if (should_set('knob')) {
>-        # First, get the correct resolution <select>, in case there is more
>-        # than one open -> closed transition allowed. Allow to fallback to
>-        # 'resolution' (useful when called from email_in.pl).
>-        my $knob = $cgi->param('knob');
>-        my $status = new Bugzilla::Status({name => $knob});
>-        my $resolution;
>-        if ($status) {
>-            $resolution = $cgi->param('resolution_knob_' . $status->id)
>-                          || $cgi->param('resolution');
>-        }
>-        else {
>-            $resolution = $cgi->param('resolution_knob_change_resolution');
>-        }
>-        
>-        # Translate the knob values into new status and resolution values.
>-        $b->process_knob($knob, $resolution, scalar $cgi->param('dup_id'));
>-    }
>-}
>-
> ##############################
> # Do Actual Database Updates #
> ##############################
>Index: Bugzilla/Bug.pm
>===================================================================
>RCS file: /cvs/qa/rh_bugzilla_3/Bugzilla/Bug.pm,v
>retrieving revision 1.23
>diff -u -r1.23 Bug.pm
>--- Bugzilla/Bug.pm	21 Feb 2008 16:23:57 -0000	1.23
>+++ Bugzilla/Bug.pm	27 Feb 2008 15:54:59 -0000
>@@ -554,6 +554,131 @@
>     return $params;
> }
> 
>+sub set_all {
>+    my ($self, $args) = @_;
>+
>+    # For security purposes, and because lots of other checks depend on it,
>+    # we set the product first before anything else.
>+    my $product_change; # Used only for strict_isolation checks, right now.
>+    if ($args->{product}) {
>+        my $changed = $self->set_product($args->{product},
>+                                        { component => $args->{component},
>+                                          version   => $args->{version},
>+                                          target_milestone => $args->{target_milestone},
>+                                          change_confirmed => $args->{confirm_product_change},
>+                                          other_bugs => $args->{bug_objects},
>+                                        });
>+        $product_change ||= $changed;                                                                                   
>+    }
>+
>+    # add/remove groups
>+    $self->remove_group($_) foreach @{$args->{remove_group}};
>+    $self->add_group($_) foreach @{$args->{add_group}};
>+   
>+    # set dependson and blocked
>+    if ($args->{dependson} || $args->{blocked}) {
>+        $self->set_dependencies($args->{dependson}, $args->{blocked});
>+    }
>+
>+    # set keywords , the hash key for the keywords can either be 
>+    # add_keywords, delete_keywords, or makeexact_keywords
>+    # get the action from the hash key name
>+    foreach my $key_name (keys %$args){
>+        if ($key_name =~ /([a-z]*)_keywords$/){
>+            my $keyword_action = $1;
>+            $self->modify_keywords($args->{$key_name}, $keyword_action);
>+        }
>+    }
>+
>+    # Component, target_milestone, and version are in here just in case
>+    # the 'product' field wasn't defined in the CGI. It doesn't hurt to set
>+    # them twice.
>+    # REDHAT EXTENSION START 406151
>+    # Added devel_whiteboard, qa_whiteboard, internal_whiteboard and fixed_in
>+    my @set_fields = qw(op_sys rep_platform priority bug_severity
>+                        component target_milestone version
>+                        bug_file_loc status_whiteboard devel_whiteboard
>+                        qa_whiteboard internal_whiteboard fixed_in short_desc
>+                        deadline remaining_time estimated_time);
>+    # REDHAT EXTENSION END 406151
>+
>+    push(@set_fields, 'assigned_to') if !$args->{set_default_assignee};
>+    push(@set_fields, 'qa_contact')  if !$args->{set_default_qa_contact};
>+
>+    my @custom_fields = Bugzilla->get_fields({custom => 1, obsolete => 0});
>+
>+    my %methods = (
>+        bug_severity => 'set_severity',
>+        rep_platform => 'set_platform',
>+        short_desc   => 'set_summary',
>+        bug_file_loc => 'set_url',
>+    );
>+
>+    # adding comment to a bug
>+    if ($args->{comment} || $args->{work_time}) {
>+        # Add a comment as needed to the bug. This is done early because
>+        # there are lots of things that want to check if we added a comment.
>+        $self->add_comment($args->{comment},
>+            { isprivate => $args->{commentprivacy},
>+              work_time => $args->{work_time} });
>+    }
>+    foreach my $field_name (@set_fields) {
>+        if ($args->{$field_name}) {
>+            my $method = $methods{$field_name};
>+            $method ||= "set_" . $field_name;
>+            $self->$method($args->{$field_name});
>+        }
>+    }
>+    $self->reset_assigned_to if $args->{set_default_assignee};
>+    $self->reset_qa_contact  if $args->{set_default_qa_contact};
>+
>+    # And set custom fields.
>+    foreach my $field (@custom_fields) {
>+        my $fname = $field->name;
>+        if ($args->{$fname}) {
>+            $self->set_custom_field($field, [$args->{$fname}]);
>+        }
>+    }
>+
>+    $self->set_alias($args->{alias}) if $args->{alias};
>+
>+    # set reporter_accessible and cclist_accessible
>+    $self->set_cclist_accessible($args->{cclist_accessible}) if $args->{cclist_accessible};
>+    $self->set_reporter_accessible($args->{reporter_accessible}) if $args->{reporter_accessible};
>+
>+    # mark an existing comment as private by its id. note the user
>+    # must be in the insidergroup or his change will be ignored
>+    if ($args->{comment_id}){
>+        $args->{comment_privacy}  ||= 0; 
>+        $self->set_comment_is_private($args->{comment_id}, $args->{comment_privacy});
>+    }
>+
>+    # add/remove cc  
>+    $self->remove_cc($_) foreach @{$args->{remove_cc}};
>+    $self->add_cc($_) foreach @{$args->{add_cc}};
>+
>+    if ($product_change) {
>+        # Theoretically you could move a product without ever specifying
>+        # a new assignee or qa_contact, or adding/removing any CCs. So,
>+        # we have to check that the current assignee, qa, and CCs are still
>+        # valid if we've switched products, under strict_isolation. We can only
>+        # do that here. There ought to be some better way to do this,
>+        # architecturally, but I haven't come up with it.
>+        $self->_check_strict_isolation();
>+    }
>+    
>+    # Set the status, resolution, and dupe_of (if needed). This has to be done
>+    # down here, because the validity of status changes depends on other fields,
>+    # such as Target Milestone.
>+    # the hash key for
>+    # NOTE: not sure if knob here is the right thing to do as knob can 
>+    #       be the actual bug status or the action to be performed on 
>+    #       the bug status and resolution  
>+    $args->{knob} ||= 'none';
>+    $self->process_knob($args->{knob}, $args->{resolution}, $args->{dup_id});
>+    
>+}
>+
> sub update {
>     my $self = shift;
>

Actions: View | Diff

Attachments on bug 433744: 295483 | 296081 | 296620 | 296768 | 297132 | 297564 | 297565