Bug 433744 - Implement Bugzilla::Bug set_all, which takes parameters from a hash and does all updates in the right order
Implement Bugzilla::Bug set_all, which takes parameters from a hash and does ...
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: Bugzilla General (Show other bugs)
3.2
All Linux
high Severity high (vote)
: ---
: ---
Assigned To: Noura El hawary
:
Depends On:
Blocks: 433745
  Show dependency treegraph
 
Reported: 2008-02-21 00:07 EST by Noura El hawary
Modified: 2013-06-24 00:15 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-15 08:38:25 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
initial patch for function Bugzilla::Bug::set_all($args) (5.52 KB, patch)
2008-02-21 00:07 EST, Noura El hawary
no flags Details | Diff
patch for Bugzilla::Bug::set_all($args) + process_bug.cgi (21.05 KB, patch)
2008-02-27 11:03 EST, Noura El hawary
no flags Details | Diff
Bugzilla::Bug::set_all($args) + Bugzilla::WebService::Bug::update() + process_bug.cgi (31.32 KB, patch)
2008-03-03 10:36 EST, Noura El hawary
no flags Details | Diff
Bugzilla::Bug::set_all($args) + Bugzilla::WebService::Bug::update() + process_bug.cgi + new user errors (33.80 KB, patch)
2008-03-04 12:23 EST, Noura El hawary
dkl: review-
Details | Diff
New WebService function Bug.update + POD (41.28 KB, patch)
2008-03-06 21:32 EST, Noura El hawary
dkl: review-
nelhawar: review? (kbaker)
Details | Diff
Bugzlla::WebService::Bug::update() (36.30 KB, patch)
2008-03-11 02:09 EDT, Noura El hawary
no flags Details | Diff
Bugzilla::WebService::Bug::update() (36.32 KB, patch)
2008-03-11 02:13 EDT, Noura El hawary
dkl: review+
Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Mozilla Foundation 418342 None None None Never

  None (edit)
Description Noura El hawary 2008-02-21 00:07:39 EST
Right now if we want to re-use the Bugzilla::Bug update()-based code anywhere
else (like in the WebService or the email interface) we would have to basically
copy all of process_bug.cgi.

Instead, we should have a method called set_all that just takes a hash of
parameters and does all of the "setting" in the right order.
Comment 1 Noura El hawary 2008-02-21 00:07:40 EST
Created attachment 295483 [details]
initial patch for function Bugzilla::Bug::set_all($args)
Comment 2 Noura El hawary 2008-02-21 00:09:29 EST
spent alot of time understanding and analyzing the upstream code for
process_bug.cgi and Bugzilla/Bug.pm , and wrote initial code for the function
set_all , submitted patch upstream and will see what they think.

Noura
Comment 3 Noura El hawary 2008-02-27 11:03:31 EST
Created attachment 296081 [details]
patch for Bugzilla::Bug::set_all($args) + process_bug.cgi

Hi Dave,

here is a patch for new Bug object method set_all() and I modified
process_bug.cgi to use it. I tested it using single and mass bug changes and
seems to be working fine, I haven't tested moving bugs though. also with the
status and resolution change , not sure if passing $args->{knob} is the right
thing to do ,,I guess from process_bug.cgi is fine but from WebService
Bug.update() it will be confusing to the user as from the function
process_knob() the args->{knob} can either be the action to perform which is
one of the following:
duplicate, move, clearresolution and change_resolution 
or it can be the new Status like NEW, ASSIGNED etc.

Please review when you can. and let me know what you think, once we agree on
set_all then I cab write Bug.update for WebService.
Comment 4 Noura El hawary 2008-02-27 11:23:25 EST
modified process_bug.cgi to user set_all + done modifications to set_all + testing
Comment 5 David Lawrence 2008-02-27 13:07:19 EST
> also with the
> status and resolution change , not sure if passing $args->{knob} is the right
> thing to do ,,I guess from process_bug.cgi is fine but from WebService
> Bug.update() it will be confusing to the user as from the function
> process_knob() the args->{knob} can either be the action to perform which is
> one of the following:
> duplicate, move, clearresolution and change_resolution 
> or it can be the new Status like NEW, ASSIGNED etc.

We may need to do a mapping table of some sort in
Bugzilla::WebService::Bug::update() to set the knob based on the value of
bug_status, resolution, and/or dup_id that is passed in as params to the
update() call. 

For example:
if ( defined $params->{bug_status} 
     and $old_bug->status->name ne $params->{bug_status} 
     and is_open_state($params->{bug_status}) ) {
    $args->{knob} = $params->{bug_status};
} 
elsif ( defined $params->{bug_status} 
        and $old_bug->status->name ne $params->{bug_status} 
        and not is_open_state($params->{bug_status}  ) {
    $args->{knob} = $params->{bug_status};
    my $status = Bugzilla::Status->new({ name => $params->{bug_status} });
    $args->{'resolution_knob_' . $status->id} = $params->{resolution};
}
elsif ( defined $params->{resolution}
        and $old_bug->resolution->name ne $params->{resolution}
        and not is_open_state($old_bug->status->name) ) {
    $args->{knob} = 'change_resolution';
    $args->{resolution_knob_change_resolution} = $params->{resolution};
elsif ( ) {
    # I am sure there are others that need to be addresses here as well
    # clear resolution, duplicates, etc.
} else {
    $args->{knob} = $params->{knob} || 'none';
}

Comment 6 David Lawrence 2008-02-27 13:21:58 EST
Some of the includes in process_bug.cgi can probably be removed now since
Bugzilla/Bug.pm is doing some of the heavy processing. Such as:

use Bugzilla::Field;
use Bugzilla::Product;
use Bugzilla::Component;

May need to doublecheck those though.
Comment 7 David Lawrence 2008-02-27 15:23:59 EST
Another short cut you could use in some places is the Vars() method for CGI.pm

my $args = {};
my $cgi = Bugzilla->cgi;

%{$args} = $cgi->Vars;

This will create a hash version of the $cgi object instead of doing

$args->{fieldname} = $cgi->param(fieldname);

in a lot of places. 
Comment 8 Noura El hawary 2008-03-03 10:36:34 EST
Created attachment 296620 [details]
Bugzilla::Bug::set_all($args) + Bugzilla::WebService::Bug::update() + process_bug.cgi

Hi ,, This is a patch that includes code moved from process_bug.cgi to
$bug->set_all , and updated process_bug.cgi and New WebService function
Bug.update() that use the set_all() function. I haven't properly tested the
WebService function yet , but i have tested process_bug.cgi and it works good.
Just attaching the code now if you would like to take a look and make any
suggestions.

Thanks,
Noura
Comment 9 David Lawrence 2008-03-03 11:25:43 EST
Comment on attachment 296620 [details]
Bugzilla::Bug::set_all($args) + Bugzilla::WebService::Bug::update() + process_bug.cgi

>Index: template/en/default/global/user-error.html.tmpl
>
>+  [% ELSIF error == "dependencies_not_allowed" %]
>+    [% title = "Cannot mark $terms.Bug as dependson/blocked" %]
>+    You cannot mark [% terms.abug %] as dependson or blocked when
>+    changing several [% terms.bugs %] bugs at ones.

Remove the word bugs after [% terms.bugs %]. It is causing the bugwords test to
fail.

perl t/009bugwords.t:
t/009bugwords..........58/282 
#   Failed test 'template/en/default/global/user-error.html.tmpl contains
invalid bare words (e.g. 'bug') --WARNING'
#   at t/009bugwords.t line 90.
t/009bugwords..........282/282 # Looks like you failed 1 test of 282.
t/009bugwords.......... Dubious, test returned 1 (wstat 256, 0x100)
 Failed 1/282 subtests 

Also you have tabs in Bugzilla/WebService/User.pm

perl t/005no_tabs.t
t/005no_tabs...........1/435 
#   Failed test 'Bugzilla/WebService/User.pm contains tabs --WARNING'
#   at t/005no_tabs.t line 47.
# Looks like you failed 1 test of 435.
t/005no_tabs........... Dubious, test returned 1 (wstat 256, 0x100)
 Failed 1/435 subtests 

Dave
Comment 10 David Lawrence 2008-03-03 11:28:15 EST
> Also you have tabs in Bugzilla/WebService/User.pm
> 
> perl t/005no_tabs.t
> t/005no_tabs...........1/435 
> #   Failed test 'Bugzilla/WebService/User.pm contains tabs --WARNING'
> #   at t/005no_tabs.t line 47.
> # Looks like you failed 1 test of 435.
> t/005no_tabs........... Dubious, test returned 1 (wstat 256, 0x100)
>  Failed 1/435 subtests 

Disregard this comment. This error is from some changes already in CVS that I
though was from your patch when I ran "./runtests.pl". I will fix the tabs and
check in the fix.

Dave

Comment 11 David Lawrence 2008-03-03 14:11:02 EST
Comment on attachment 296620 [details]
Bugzilla::Bug::set_all($args) + Bugzilla::WebService::Bug::update() + process_bug.cgi

>+++ Bugzilla/WebService/Bug.pm	3 Mar 2008 15:31:13 -0000
>+# this is a function that gets bug activity for list of bug ids 
>+# it can be called as the following:

Has text from get_activity() instead of update() above.

>+# $call = $rpc->call( 'Bug.update', {ids => [1,2], update => {bug_status => 'NEW', priority => 'P3'}}
>+sub update {
>+
>+    my ($self, $params) = @_;      
>+    my $user = Bugzilla->login(LOGIN_REQUIRED);
>+
>+    my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{ids});
>+    @ids || ThrowCodeError('param_required', { param => 'ids' });
>+
>+    my $updates = $params->{update} ||  ThrowCodeError('param_required', { param => 'update' });
>+
>+    my @valid_ids;
>+    foreach my $bug_id (@ids) {
>+        ValidateBugID($bug_id);
>+        push(@valid_ids, $bug_id);
>+    }
>+
>+    my @bug_objects;
>+    @bug_objects = @{Bugzilla::Bug->new_from_list(\@valid_ids)};
>+    scalar(@bug_objects) || ThrowUserError("no_bugs_chosen", {action => 'modify'}); 
>+
>+    # For each bug, we have to check if the user can edit the bug the product
>+    # is currently in, before we allow them to change anything.
>+    foreach my $bug (@bug_objects) {
>+        if (!Bugzilla->user->can_edit_product($bug->product_obj->id) ) {
>+            ThrowUserError("product_edit_denied",
>+                          { product => $bug->product });
>+        }
>+    }
>+    
>+    foreach my $bug (@bug_objects) {
>+        my $args;
>+
>+        if ($args->{product}) {
>+            $args->{product} = $updates->{product};
>+            $args->{component} = $updates->{component};
>+            $args->{version} = $updates->{version};
>+            $args->{target_milestone} = $updates->{target_milestone};
>+            $args->{confirm_product_change} =  $updates->{confirm_product_change};
>+            $args->{other_bugs} = \@bug_objects;
>+        }
>+        
>+        foreach my $group (@{$bug->product_obj->groups_valid}) {
>+            my $gid = $group->id;
>+            my $gname = $group->name;
>+            if (!grep {$_ =~ /$gname/ } @{$updates->{add_group}}) {

Changed $updates->{add_group} to $updates->{remove_group}.

>+                push (@{$args->{remove_group}}, $gid);
>+            }
>+            # "== 1" is important because mass-change uses -1 to mean
>+            # "don't change this restriction"

This comment doesnt seem  to be relevant here.

>+            elsif (grep {$_ =~ /$gname/ } @{$updates->{add_group}}) {
>+                push (@{$args->{add_group}}, $gid);
>+            }
>+        }

>+        if ($updates->{comment} || $updates->{work_time}) {
>+            $args->{comment} = $updates->{comment};
>+            $args->{work_time} = $updates->{work_time};
>+            $args->{commentprivacy} = $updates->{private};
>+        }

Nit-Pick: line up the statements values vertically.

>+
>+        # 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.

Comment not relevant for XMLRPC. Now that I think about it we will need to
figure out
how to handle the case where the product *is* changed using Bug.update().
Currently 
when changing product in the web UI it shows an interim screen requesting
choice
of version/component/milestone/etc. Will we just accept the values passed in to
the
XMLRPC call and throw an error if a required piece of information is missing?

Example:
$update = {
     product   => "Red Hat Enterprise Linux 6", # Previous product was Fedora
     component => "anaconda", # New component choice for RHEL 6 since product
change
     version   => "6.1", # Require a new version for RHEL 6 since product
change
     milestone => "beta1", # New milestone value for RHEL6 (default to --- if
none passed?)
};

>+        $args->{set_default_assignee} = $upadtes->{set_default_assignee};
>+        $args->{set_default_qa_contact} = $updates->{set_default_qa_contact};

$updates misspelled.

>+
>+        foreach my $field (@custom_fields) {
>+            my $fname = $field->name;
>+            $args->{$fname} = $updates->{$fname} if $updates->{$fname};
>+        }

Need to check for defined instead of if true/false.

	      $args->{$fname} = $updates->{$fname} if defined
$updates->{$fname};

This way a person would be able to clear out the value for $args->{$fname} by
passing
$updates->{$fname} => "". We should do the same everywhere in Bug.update() so
we can
specify only specific changes to make.

>+    # 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.

Nit-Pick: Move over 4 spaces

>+        if ( defined $updates->{bug_status} && ($bug->status->name ne $updates->{bug_status}) 
>+                                            && (is_open_state($updates->{bug_status})) ) {
>+            $args->{knob} = $updates->{bug_status};
>+        } 
>+        elsif ( defined $updates->{bug_status} && ($bug->status->name ne $updates->{bug_status}) 
>+                                               &&  !(is_open_state($updates->{bug_status}) ) {
>+            $args->{knob} = $updates->{bug_status};
>+            $args->{resolution} = $updates->{resolution};
>+        }

I think the dupe_id code below should be incorporated in this section since
when we duplicate a bug we are closing it with a closed status. 

$updates = {
     bug_status => 'CLOSED',
     resolution => 'DUPLICATE',
     dupe_id	=> '00000',
}

So maybe:

	  elsif ( defined $updates->{bug_status} && ($bug->status->name ne
$updates->{bug_status}) 
						 && 
!(is_open_state($updates->{bug_status}) ) {
	      $args->{knob} = $updates->{bug_status};
	      $args->{resolution} = $updates->{resolution};

	      if ( defined $updates->{dupe_id} ) {
		  $args->{knob} = 'duplicate';
		  $args->{dupe_id} = $updates->{dupe_id};
	      }
	  }

Or do you think it should be a separate action as you have it and we just look
for dupe_id and ignore bug_status and resolution completely?


>+        elsif ( defined $updates->{resolution} && ($bug->resolution->name ne $updates->{resolution})
>+                                               && !(is_open_state($bug->status->name)) ) {
>+            $args->{knob} = 'change_resolution';
>+            $args->{resolution} = $updates->{resolution};
>+        }
>+        elsif ( defined $updates->{dupe_id} ) {
>+            $args->{knob} = 'duplicate';
>+            $args->{dupe_id} = $updates->{dupe_id};
>+        }
>+        else {
>+            $args->{knob} = 'none';
>+        }
>+
>+        $bug->set_all($args);
>+    }
>+
>+    my %changes;    
>+    foreach my $b (@bug_objects){
>+        my $returned_changes = $b->update();
>+        $changes{$b->bug_id} = $returned_changes;
>+        Bugzilla::BugMail::Send($b->bug_id, { changer => Bugzilla->user->login });
>+    }
>+
>+    return { bug_updates => \%changes };   
>+}

Currently in all of our bugzilla.update* methods we return also the list of
email recipients that were mailed and the ones that were excluded. So we should
do:

       foreach my $b (@bug_objects){
	   my $returned_changes = $b->update();
	   my $mail_results = Bugzilla::BugMail::Send($b->bug_id, { changer => 
Bugzilla->user->login });
	   $changes{$b->bug_id} = [$returned_changes, $mail_results];
       }

Make sense?

Dave
Comment 12 Noura El hawary 2008-03-04 12:19:59 EST
> I think the dupe_id code below should be incorporated in this section since
> when we duplicate a bug we are closing it with a closed status. 
> 
> $updates = {
>      bug_status => 'CLOSED',
>      resolution => 'DUPLICATE',
>      dupe_id	=> '00000',
> }
> 
> So maybe:
> 
> 	  elsif ( defined $updates->{bug_status} && ($bug->status->name ne
> $updates->{bug_status}) 
> 						 && 
> !(is_open_state($updates->{bug_status}) ) {
> 	      $args->{knob} = $updates->{bug_status};
> 	      $args->{resolution} = $updates->{resolution};
> 
> 	      if ( defined $updates->{dupe_id} ) {
> 		  $args->{knob} = 'duplicate';
> 		  $args->{dupe_id} = $updates->{dupe_id};
> 	      }
> 	  }
> 
> Or do you think it should be a separate action as you have it and we just look
> for dupe_id and ignore bug_status and resolution completely?
> 

Dave ,, I think for this one it is better that we only pass the dupe_id and then
the status and the resolution will be appropriately changed. just to make it
match with the code in Bug::process_knob().

Noura
Comment 13 David Lawrence 2008-03-04 12:22:52 EST
(In reply to comment #12)
> Dave ,, I think for this one it is better that we only pass the dupe_id and then
> the status and the resolution will be appropriately changed. just to make it
> match with the code in Bug::process_knob().
> 
> Noura

Ok, works for me.

Dave
Comment 14 Noura El hawary 2008-03-04 12:23:37 EST
Created attachment 296768 [details]
Bugzilla::Bug::set_all($args) + Bugzilla::WebService::Bug::update() + process_bug.cgi + new user errors 

Hi,, this is new patch for the function Bug::update() I had to do alot of
changes to the previous patch as while testing I found out that it wasn't as
easy as i expected. I tested this patch and it worked good for me. Please
review and test when you can , when all good i will write the POD for it.

Thanks,
Noura
Comment 15 Noura El hawary 2008-03-04 12:56:43 EST
Just to make it easier for testing as there is no POD yet for the function :
this is a description of the parameters that you can pass to the function ,, 

so basically: 
$call = $rpc->call('Bug.update', {ids => [1,'alias1'], 
                             update => {bug_status => 'P1', priority => 'high'}});

ids : so in the ids hash you can either pass single value or an array of values
,  also it can be alias or bug ids.

update: in the update hash you can pass the following:
1- to change a product:
update => {product=> '', component => '', version => '', target_milestone => ''}

2- to add groups to the bug  :  update => {add_group => ['gname1', 'gname2']}
   to delete groups from the bug: update => {delete_group => ['gname1']}

3- to change dependencies:
   add/delete depends on : update => {add_dependson => [1,2],
                                       delete_dependson => [4]}

   add/delete blocked: update => {add_blocked => [4], 
                                  delete_blocked => [5,7]}

4- keywords update:  update => {add_keyword => ['Security','Test']}
                     update => {delete_keyword => ['Reopened']}
                     update => {makeexact => ['Security']}

5- to add work time to the bug:
                     update => {work_time => 2 , comment => 'worked 2 hours',
                                private => 1}

6- to add a comment same as in #5 just without the work_time with the optional
private comment.

7- then there is the @set_fields in the function code just provide them as they
are in the update hash with the new value.

8- set_default_assignee and set_default_qa_contact are both boolean values just
give it 1 or 0. same as cclist_accessible and reporter_accessible


9- add/remove from cc list: update => {add_cc => ['test@redhat.com'],
                                     delete_cc => ['non@redhat.com']}

10 - for status : bug_status => '', resolution => ''. and to close abug as
duplicate only tou need to provide dupe_id => $id
                      
                             
11- to mark/unmark comment as private you need to provide the actual id for the
comment from the longdescs table as the following:
update => { private_comment => [1,4], non_private_comment => [788]}
it is not user friendly and will have to find a better way later on.

Noura
Comment 16 David Lawrence 2008-03-04 17:33:38 EST
Comment on attachment 296768 [details]
Bugzilla::Bug::set_all($args) + Bugzilla::WebService::Bug::update() + process_bug.cgi + new user errors 

>Index: template/en/default/global/user-error.html.tmpl
>===================================================================
>RCS file: /cvs/qa/rh_bugzilla_3/template/en/default/global/user-error.html.tmpl,v
>+  [% ELSIF error == "product_update_not_allowed" %]
>+    [% title = "Product Update Not Allowed" %]
>+    Missing required information to change a product of a bug.

      Missing required information to change a product of a [% terms.bug %].

>+    You must specify all the following:
>+    component name, version, and target milestone.
>+
>+  [% ELSIF error == "alias_not_allowed" %]
>+    [% title = "Alias Update Not Allowed" %]
>+    You cannot update the alias when changing several
>+    bugs at once.

      [% terms.bugs %] at once.

>+
>   [% ELSIF error == "dependency_loop_multi" %]
>     [% title = "Dependency Loop Detected" %]
>     The following [% terms.bug %](s) would appear on both the "depends on"
>@@ -358,6 +369,11 @@
>     [% title = "Dependency Loop Detected" %]
>     You can't make [% terms.abug %] block itself or depend on itself.
> 
>+  [% ELSIF error == "dependencies_not_allowed" %]
>+    [% title = "Cannot mark $terms.Bug as dependson/blocked" %]
>+    You cannot mark [% terms.abug %] as dependson or blocked when
>+    changing several [% terms.bugs %] at once.
>+
>   [% ELSIF error == "dupe_id_required" %]
>     [% title = "Duplicate $terms.Bug Id Required" %]
>     You must specify [% terms.abug %] id to mark this [% terms.bug %]
Comment 17 David Lawrence 2008-03-04 17:36:35 EST
I have this patch installed on https://bugdev.devel.redhat.com/bugzilla if you
want to use that to test. I am going to test this some more tonight.

Dave
Comment 18 Noura El hawary 2008-03-06 21:32:38 EST
Created attachment 297132 [details]
New WebService function Bug.update + POD

This new patch has:

1- the modifications that Dave has suggested to the user error template
2- modified process_bug.cgi after Dave has added his new custom fields patch.
3- made changes to Bug.update to update those cf_* fields properly.
4- added POD for the Bug.update function.
5- added error codes from POD to WebService/Constants.pm

Please review and let me know what you think.

Thanks,
Noura
Comment 19 David Lawrence 2008-03-10 16:05:56 EDT
Comment on attachment 297132 [details]
New WebService function Bug.update + POD

>+# this is a function that updates a bug:
>+# it can be called as the following:
>+# $call = $rpc->call( 'Bug.update', {ids => [1,2], update => {bug_status => 'NEW', priority => 'P3'}}

Nit-Pick: Should we called the 'update' key 'updates' instead? Since there
could be more than one update similar to how 'ids' is called 'ids'?


>+=item C<update> (hash) B<Required> - A hash containing bug fields
>+and the new values it will be updated to. The hash may contain any of the following:
>+B<NOTE> Some of the following items can only be updated when changing only
>+single bug, those items are: C<alias>, C<add|delete_dependson>,
>+C<add_delete_blocked>, C<dupe_id>, C<cclist_accessible>, C<reporter_accessible>. 

  C<add|delete_blocked>


The rest looks good code wise. When I tried to do a test using a script I
wrote, I encountered some strangeness.

Call to XMLRPC:

$call = $rpc->call("Bug.update", { ids => 172222, update => { bug_status =>
'CLOSED', resolution => 'RAWHIDE', comment => 'Closing this bug' } });

The change occurred properly on the bug report as expected. But the output
looked strange.

The result output to the command line using Data::Dumper:

$VAR1 = {
	  'bug_updates' => {
			   '172222' => [
				       {
					 'bug_status' => [
							 'ASSIGNED',
							 'CLOSED'
						       ],
					 'resolution' => [
							 '',
							 'RAWHIDE'
						       ]
				       },
				       {
					 'keywords' => [
						       [],
						       []
						     ]
				       },
				       {
					 'cc' => []
				       },
				       {
					 'dependencies' => {
							   'dependson' => [
									  [],
									  []
									],
							   'blocked' => [
									[],
									[]
								      ]
							 }
				       },
				       {
					 'excluded' => [],
					 'sent' => [
						   'dkl@redhat.com',
						   'bpeck@redhat.com',
						   'mjenner@redhat.com',
						  
'bugbot@landfill.bugzilla.org',
						   'dmalcolm@redhat.com'
						 ]
				       }
				     ]
			 }
	};

I did not change any keywords or dependencies yet I got the change list back.
Looking at bug 172222 which does have a keyword and dependson set, they were
still there so real change was made for those fields. So the output was
harmless but strange that it was included. 

Thanks
Dave
Comment 20 David Lawrence 2008-03-10 16:21:03 EDT
(In reply to comment #19)
> I did not change any keywords or dependencies yet I got the change list back.
> Looking at bug 172222 which does have a keyword and dependson set, they were
> still there so real change was made for those fields. So the output was
> harmless but strange that it was included. 

Oops, I mean no real change was made to the dependencies or keywords so no harm
done. 

Also with other changes in my testing, I always get the dependencies, cc, and
keywords empty change report back. So it happens every time. Expected behaviour
would be to only see the changes that are made.

Dave

Comment 21 Noura El hawary 2008-03-11 02:09:40 EDT
Created attachment 297564 [details]
Bugzlla::WebService::Bug::update()

Thanks for the review Dave. I have applied the changes per your suggestions.

regarding how you always get in the changes hash "dependencies,keywords,cc"
even if no changes has been made to them, basically in the upstream code they
still have separate functions to update those three fields, so they don't get
updated by calling the function $bug->update, but I think upstream are going to
change that to make $bug->update does all the work, so basically I have to call

$bug->update 
$bug->update_keywords
$bug->update_cc
$bug->update_dependencies

and the problem is that they always return data structures even if no changes
were made, so the data structures will be empty as how you have seen in the
results you got. But as it doesn't look good having them empty like that i have
added some code that is not really very nice , but can do the job for now and
hide those empty data strctures until the upstream have all the updates done in
the $bug->update function.I have done this:

      my $keyword_changes= $bug->update_keywords($timestamp);
	push @{$changes{$bug->bug_id}}, {keywords => $keyword_changes}
	    if (scalar $keyword_changes->[0][0] || scalar
$keyword_changes->[0][1]);

	my ($cc_removed) = $bug->update_cc($timestamp);
	$cc_removed = [map {$_->login} @$cc_removed];
	push  @{$changes{$bug->bug_id}}, {cc => $cc_removed} if scalar
@$cc_removed;

	my ($dep_changes) = $bug->update_dependencies($timestamp);
	push  @{$changes{$bug->bug_id}}, {dependencies => $dep_changes}
	    if (scalar $dep_changes->{dependson}->[0][0] || scalar
$dep_changes->{dependson}->[0][1]
		|| $dep_changes->{blocked}->[0][0] ||
$dep_changes->{blocked}->[0][1]);


attached new patch.

Noura
Comment 22 Noura El hawary 2008-03-11 02:13:04 EDT
Created attachment 297565 [details]
Bugzilla::WebService::Bug::update()

new patch with a little fix.
Comment 23 David Lawrence 2008-03-27 15:01:36 EDT
Comment on attachment 297565 [details]
Bugzilla::WebService::Bug::update()

Looks good.
Comment 24 Noura El hawary 2008-04-10 22:36:50 EDT
started to split the patch for moving code from process_bug.cgi to
$bug->set_all() into smaller bugs as requested by upstream using bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=418342

to track all those smaller bugs.
Comment 25 Noura El hawary 2008-05-15 08:38:25 EDT
This patch is already committed into our cvs . will close this bug, and use the
upstream one to track upstream reviews and changes to it.

Note You need to log in before you can comment on or make changes to this bug.