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.
Created attachment 295483 [details] initial patch for function Bugzilla::Bug::set_all($args)
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
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.
modified process_bug.cgi to user set_all + done modifications to set_all + testing
> 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'; }
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.
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.
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 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
> 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 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
> 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
(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
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
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'], delete_cc => ['non']} 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 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 %]
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
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 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', 'bpeck', 'mjenner', 'bugbot.org', 'dmalcolm' ] } ] } }; 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
(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
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
Created attachment 297565 [details] Bugzilla::WebService::Bug::update() new patch with a little fix.
Comment on attachment 297565 [details] Bugzilla::WebService::Bug::update() Looks good.
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.
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.