Add a new xmlrpc call (updateFlagRequestee()) to handle the requestee fild of requestable flag.
Created attachment 331527 [details] added a new xmlrpc call - updateFlagRequestee Noura, Thanks for your review. I have modified my code and please review it again. Cheers, Tony
Comment on attachment 331527 [details] added a new xmlrpc call - updateFlagRequestee Hi Dave, can you please review this. It's needed as part of the account closure procedure. I just want to make sure that this code should be separate from Bug.update.
Created attachment 331660 [details] new xmlrpc function - updateFlagRequestee
+ # Map the flag names to flag id's for processing by validate() You copied this comment from updateFlags but we are not doing that action here. + my $flag_name; + my $new_requestee; + while ( my ($name, $new_value) = each ( %$data ) ) { + if ( $name eq 'nomail' ) { next; } + $flag_name = $name; + $new_requestee = $new_value; + } + # Return if user does not exist + my $uid = Bugzilla::User::login_to_id($new_requestee); You should code this in a way where multiple requestees could be set. The way you have it only one can be set at a time. + my $found_one = 0; + my $cgi = Bugzilla->cgi; + foreach my $type (@{$flag_types}) { + foreach my $flag (@{$type->{flags}}) { + $cgi->param(-name=>"flag-" . $flag->id, -value=>$flag->status); + if ($type->{'is_requesteeble'} && $flag->requestee && ( $type->name ne $flag_name )) { + $cgi->param(-name=>"requestee-" . $flag->id, -value=>$flag->requestee->login); + } This is not needed. If the flag is not the one you want then you do not need to set anything. The code in Bugzilla::Flag will leave it alone. You simply just put at the beginning of the @{$flag_types} foreach loop. next if $type->name ne $flag_name; + if ($type->{'is_requesteeble'} && $flag->requestee && ( $type->name nq $flag_name )) { + $cgi->param(-name=>"requestee-" . $flag->id, -value=>$flag->requestee->login); + } Looks like you have the same code repeated here. Also the second repeated code has $type->name nq $flag_name which is incorrect. + if ($type->{'is_requesteeble'} && $flag->requestee && ( $type->name eq $flag_name )) { + $cgi->param(-name=>"requestee-" . $flag->id, -value=>$new_requestee); + $found_one = 1; + } You need to also make sure the status is already set to '?' as a flag cannot have a requestee for any other state than '?'. Also we need to think whether we want the state to be automatically set to '?' if we are setting a requestee. + } + if (!@{$type->{flags}} && $type->is_active) { + $cgi->param(-name=>"flag_type-" . $type->id, -value=>'X'); + } + } If the flag type currently has no flags, would we want to set the new requestee then and new status to '?' therefore creating a new flag? Probably. Dave
Just wanted to also comment that this functionality should eventually be merged into bugzilla.updateFlags but since this feature is needed soon then that can wait. Tony, once this feature is committed and pushed live, can you create a new bug stating that we need to merge bugzilla.updateFlags and bugzilla.updateFlagRequestees Thanks Dave
(In reply to comment #5) > Just wanted to also comment that this functionality should eventually be merged > into bugzilla.updateFlags but since this feature is needed soon then that can > wait. Dave, good point. 1) Are we under such high pressure to get a fix out? I don't see the rush. 2) Can we not take the time to integrate this into updateFlags now? 3) And is updateFlags the only place for it? How will this work with the new upstream API? Should it be in Bug.update? Will upstream accept a patch?
(In reply to comment #6) > (In reply to comment #5) > > Just wanted to also comment that this functionality should eventually be merged > > into bugzilla.updateFlags but since this feature is needed soon then that can > > wait. > > Dave, good point. > > 1) Are we under such high pressure to get a fix out? I don't see the rush. > Dunno. was going by your previous comment that it is needed as part of the account closure procedure so I assumed it was needed sooner. If it can wait then yes I would prefer to do it right this time around. > 2) Can we not take the time to integrate this into updateFlags now? Yes > 3) And is updateFlags the only place for it? How will this work with the new > upstream API? Should it be in Bug.update? Will upstream accept a patch? For now updateFlags is the right place. The flags code in Bugzilla::Flags and Bugzilla::FlagTypes is still dependent on CGI.pm params and is requires special things to make it work with XMLRPC and other places. There is already a bug being worked on upstream that is to separate the flags code from CGI.pm. Once that is done making it work with Bug.update is trivial. Dave
Comment on attachment 331660 [details] new xmlrpc function - updateFlagRequestee Dave has done the review for this patch. Noura
Hi Dave, Thank you for the review. I have updated my patch according to your review. Thanks, Tony
Created attachment 332005 [details] bugzilla.updateFlagrequestees function 1. changed code to allow multiple requestees to be set. 2. Changed code to set the status of a requesteeble flag to '?' when updating this flag's requestee.
Comment on attachment 332005 [details] bugzilla.updateFlagrequestees function >Index: extensions/compat_xmlrpc/code/webservice.pl >+# REDHAT EXTENSION START 485013 >+# Updated requesteeble flag's requestee for a bug report >+# $data = { >+# "<flag name>" => "new_requestee", Nit: "<flag_name>" => "<Bugzilla Account>", >+ # check if the new requestee is a valid user >+ my %data_hash = %$data; >+ while ( my ( $key, $new_requestee ) = each ( %data_hash ) ) { >+ if ( $key eq 'nomail' ) { next; } Nit: next if $key eq 'nomail'; >+ # Return if user does not exist >+ my $uid = Bugzilla::User::login_to_id($new_requestee); >+ if (!$uid) { >+ ThrowUserError('invalid_username', { name => $new_requestee }); >+ } Nit: $uid || ThrowUserError('invalid_username', { name => $new_requestee }); >+ } >+ >+ # The types of flags that can be set on this bug. >+ my $flag_types = $bug->flag_types; >+ >+ my $found_one = 0; >+ my $cgi = Bugzilla->cgi; >+ foreach my $type (@{$flag_types}) { >+ foreach my $flag (@{$type->{flags}}) { >+ $cgi->param(-name=>"flag-" . $flag->id, -value=>$flag->status); >+ my $flag_name = $type->name; >+ if ($type->{'is_requesteeble'} && $flag->requestee && ( !$data_hash{$flag_name} )) { >+ $cgi->param(-name=>"requestee-" . $flag->id, -value=>$flag->requestee->login); >+ } This block is not necessary. For flags not specified in the %data_hash, we just leave them alone. >+ if ($type->{'is_requesteeble'} && $data_hash{$flag_name}) { >+ $cgi->param(-name=>"requestee-" . $flag->id, -value=>$data_hash{$flag_name} ); >+ $cgi->param(-name=>"flag-" . $flag->id, -value=>'?'); >+ $found_one = 1; >+ } Nit-pick: Please use $type->is_requesteeble and not access the object value directly. >+ } >+ if (!@{$type->{flags}} && $type->is_active) { >+ $cgi->param(-name=>"flag_type-" . $type->id, -value=>'X'); >+ } This block is unnecessary since it the flag_type does not have any flags set already, we do not need to clear them with 'X'. You do need to however add code here that if no flags for this flag_type are defined but the flag name is specified in the %data_hash, you need to create a new flag with status set to '?' and the requestee set to the new requestee's Bugzilla account. if (!@{$type->{flags}} && $type->is_activei && $data_hash{$type->name}) { $cgi->param(-name => "flag_type-" . $type->id, -value=>'?'); $cgi->param(-name => "requestee_type-" . $type->id, -value => $data_hash{$type->name}); } Another nit pick is to make sure your tabs are set to 4 spaces properly and that you indent your code. I am seeing places after applying the patch where certain lines are not lining up properly. Thanks Dave
Also for extra points, please create a short POD doc section for this new method above the subroutine itself. Dave
> >+ if ($type->{'is_requesteeble'} && $flag->requestee && ( !$data_hash{$flag_name} )) { > >+ $cgi->param(-name=>"requestee-" . $flag->id, -value=>$flag->requestee->login); > >+ } > > This block is not necessary. For flags not specified in the %data_hash, we just > leave them alone. > I have tested the code without this block. It seems that if any flag which is not specified in %data_hash has existing requestee, the requestee information of that flag will be removed after calling this function. For example, I created a test bug (#473793) on bz-web2 and set two flag "needinfo" and "needinfo2" and set requestee for both of them as 'tfu'. When I called updateFlagrequestees() to update needinfo flag's requestee to 'nelhawar' and the requestee of flag needinfo was changed to 'nelhawar', but the requestee of flag needinof2 was removed as well.
Created attachment 332180 [details] bugzilla.updateFlagrequestees function Modified code according to dkl's previous review.
Comment on attachment 332180 [details] bugzilla.updateFlagrequestees function >Index: extensions/compat_xmlrpc/code/webservice.pl >+ my $found_one = 0; >+ my $cgi = Bugzilla->cgi; >+ foreach my $type (@{$flag_types}) { >+ foreach my $flag (@{$type->{flags}}) { >+ $cgi->param(-name=>"flag-" . $flag->id, -value=>$flag->status); >+ my $flag_name = $type->name; >+ if ($type->is_requesteeble && $flag->requestee && ( !$data_hash{$flag_name} )) { >+ $cgi->param(-name=>"requestee-" . $flag->id, -value=>$flag->requestee->login); >+ } I was not able to recreate the problem you were stating. For eaxample: https://bz-web2-test.devel.redhat.com/show_bug.cgi?id=966 I set needinfo2?(dklawren) using the web UI. Then I used commented out the following lines in your code: #$cgi->param(-name=>"flag-" . $flag->id, -value=>$flag->status); my $flag_name = $type->name; #if ($type->is_requesteeble && $flag->requestee && ( !$data_hash{$flag_name} )) { # $cgi->param(-name=>"requestee-" . $flag->id, -value=>$flag->requestee->login); #} Then I used the bugzilla.updateFlagRequestees method to set needinfo?(dkl). It properly set needinfo?(dkl) but did not touch the previous needinfo2?(dklawren) and left it the way it was. So it seems to be working fine for me by leaving out that part. What exact test case were you using that was giving you the failure? >+ if ($type->is_requesteeble && $data_hash{$flag_name}) { >+ $cgi->param(-name=>"requestee-" . $flag->id, -value=>$data_hash{$flag_name} ); >+ $cgi->param(-name=>"flag-" . $flag->id, -value=>'?'); >+ $found_one = 1; >+ } >+ } >+ if (!@{$type->{flags}} && $type->is_active && $type->is_requesteeble && $data_hash{$type->name}) { >+ $cgi->param(-name => "flag_type-" . $type->id, -value=>'?'); >+ $cgi->param(-name => "requestee_type-" . $type->id, -value => $data_hash{$type->name}); >+ $found_one = 1; >+ } >+ } Nit-pick: Just to save typing you could do in the flag_types loop: foreach my $type (@{$flag_types}) { my $flag_name = $type->name; foreach my $flag (@{$type->{flags}}) { and then use $flag_name everywhere instead of just in the flags loop. >@@ -3463,6 +3554,7 @@ > updateDepends > isPrivate > getBugModified >+updateFlagRequestee No need to add this line here if you have added POD documentation above your function. This list was originally to show other methods that are available but are not yet documented. Dave
(In reply to comment #15) > (From update of attachment 332180 [details]) > >Index: extensions/compat_xmlrpc/code/webservice.pl > >+ my $found_one = 0; > >+ my $cgi = Bugzilla->cgi; > >+ foreach my $type (@{$flag_types}) { > >+ foreach my $flag (@{$type->{flags}}) { > >+ $cgi->param(-name=>"flag-" . $flag->id, -value=>$flag->status); > >+ my $flag_name = $type->name; > >+ if ($type->is_requesteeble && $flag->requestee && ( !$data_hash{$flag_name} )) { > >+ $cgi->param(-name=>"requestee-" . $flag->id, -value=>$flag->requestee->login); > >+ } > > I was not able to recreate the problem you were stating. For eaxample: > > https://bz-web2-test.devel.redhat.com/show_bug.cgi?id=966 > > I set needinfo2?(dklawren) using the web UI. > > Then I used commented out the following lines in your code: > > #$cgi->param(-name=>"flag-" . $flag->id, -value=>$flag->status); > my $flag_name = $type->name; > #if ($type->is_requesteeble && $flag->requestee && ( > !$data_hash{$flag_name} )) { > # $cgi->param(-name=>"requestee-" . $flag->id, > -value=>$flag->requestee->login); > #} > > Then I used the bugzilla.updateFlagRequestees method to set > needinfo?(dkl). > > It properly set needinfo?(dkl) but did not touch the previous > needinfo2?(dklawren) > and left it the way it was. > > So it seems to be working fine for me by leaving out that part. > > What exact test case were you using that was giving you the failure? > Dave, My bad. I forgot to comment out $cgi->param(-name=>"flag-" . $flag->id, -value=>$flag->status) when I did the test. Now everything seems to work fine. Thanks, Tony
Created attachment 332329 [details] bugzilla.updateFlagRequestees function
Comment on attachment 332329 [details] bugzilla.updateFlagRequestees function Looks good Tony.
(In reply to comment #18) > (From update of attachment 332329 [details]) > Looks good Tony. Dave, Thanks for the review. I close this ticket. Tony
I thought we were going to integrate this into updateFlags? see comments 6 & 7
(In reply to comment #20) > I thought we were going to integrate this into updateFlags? see comments 6 & 7 Filed bug 486898 Dave