Bug 486898 - Integrate updateFlagRequestees() webservice into updateFlags()
Integrate updateFlagRequestees() webservice into updateFlags()
Status: CLOSED CURRENTRELEASE
Product: Bugzilla
Classification: Community
Component: WebService (Show other bugs)
3.2
All Linux
medium Severity low (vote)
: ---
: ---
Assigned To: Tony Fu
: FutureFeature
Depends On:
Blocks: 489343
  Show dependency treegraph
 
Reported: 2009-02-22 21:13 EST by David Lawrence
Modified: 2013-06-24 00:07 EDT (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Enhancement
Doc Text:
Story Points: ---
Clone Of: 485013
: 489343 (view as bug list)
Environment:
Last Closed: 2009-07-21 01:57:20 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)
Add updating flag's requestee function into updateFlag() (6.69 KB, patch)
2009-03-08 22:42 EDT, Tony Fu
dkl: review-
Details | Diff
Add updating flag's requestee function into updateFlag() (7.87 KB, patch)
2009-03-09 02:47 EDT, Tony Fu
dkl: review+
Details | Diff

  None (edit)
Description David Lawrence 2009-02-22 21:13:22 EST
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

--- Additional comment from kbaker@redhat.com on 2009-02-12 12:35:25 EDT ---

(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?

--- Additional comment from dkl@redhat.com on 2009-02-12 13:49:13 EDT ---

(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 1 Tony Fu 2009-03-08 22:42:06 EDT
Created attachment 334460 [details]
Add updating flag's requestee function into updateFlag()

Dave,

I integrated the function porvided by updateFlagRequestees() into updateFlag().  Please review my patch.


Thanks,
Tony
Comment 2 David Lawrence 2009-03-09 00:16:08 EDT
Comment on attachment 334460 [details]
Add updating flag's requestee function into updateFlag()

>Index: extensions/compat_xmlrpc/code/webservice.pl
>-    my $cgi = Bugzilla->cgi;
>     my %flags_id_map = ();
>     my %flagtypes_id_map = ();
>+    my %requesteeble_flags = ();
>+    my %flagtypes_requestee = ();
>     foreach my $type (@{$flag_types}) {
>         foreach my $flag (@{$type->{flags}}) {
>             $flags_id_map{$type->name} = $flag->id;
>-            $cgi->param(-name=>"flag-" . $flag->id, -value=>$flag->status);
>-            if ($type->{'is_requesteeble'} && $flag->requestee) {
>-                $cgi->param(-name=>"requestee-" . $flag->id, -value=>$flag->requestee->login);
>+            if ($type->{'is_requesteeble'}) {
>+                $requesteeble_flags{$type->name} = 1;
>             }
>         }
>         if (!@{$type->{flags}} && $type->is_active) {
>             $flagtypes_id_map{$type->name} = $type->id;
>-            $cgi->param(-name=>"flag_type-" . $type->id, -value=>'X');
>         }
>+        if (!@{$type->{flags}} && $type->is_active && $type->{'is_requesteeble'} ) {
>+            $flagtypes_id_map{$type->name} = $type->id;
>+            $flagtypes_requestee{$type->name} = 1;
>+        }

$flagtypes_id_map{$type->name} = $type->id is already set previously so no need to set again. Also you are also done two very similar conditionals here and would be cleaner to combine them together. For example:

        if (!@{$type->{flags}} && $type->is_active) {
            $flagtypes_id_map{$type->name} = $type->id;
            if ($type->{'is_requesteeble'} ) {
                $flagtypes_requestee{$type->name} = 1;
            }
        }

Also it may be less confusing to call %flagtypes_requestee instead %requesteeable_flagtypes similar to %requesteeble_flags.

>     }
>-    
>     my $found_one = 0;
>+
>+    my $cgi = Bugzilla->cgi;
>     foreach my $key (keys %{$data}) {
>+        # XXX-tfu find out if the key is for setting a flag's requestee
>+	if ( $key =~ /^(.*)_requestee$/ ) {

nit: Please fix indenting here.

>+            my $flag = $1;
>+            my $requestee = $data->{$key};
>+            # check if the new requestee is a valid user
>+            my $uid = Bugzilla::User::login_to_id($requestee);
>+            $uid || ThrowUserError('invalid_username', { name => $requestee });
>+            my $flag_id = $flags_id_map{$flag};
>+            # check if the flag is requesteeble flag
>+            if ($requesteeble_flags{$flag}) {
>+                $cgi->param(-name=>"requestee-" . $flag_id, -value=>$requestee );
>+                $cgi->param(-name=>"flag-" . $flag_id, -value=>'?');

Just use $flags_id_map{$flag} in setting of the $cgi->params() instead of $flag_id to be consistent to how you to $flagtypes_id_map{$flag} below.

>+                $found_one = 1;
>+                next;
>+	    }

nit: indenting here also.

>+            if ($flagtypes_requestee{$flag}) {
>+                $cgi->param(-name => "flag_type-" . $flagtypes_id_map{$flag}, -value=>'?');
>+                $cgi->param(-name => "requestee_type-" . $flagtypes_id_map{$flag}, -value => $requestee);
>+                $found_one = 1;
>+                next;
>+            }
>+            ThrowUserError('update_flagrequestees_invalid_flag_name');
>+        }
>         if ($flags_id_map{$key}) {
>             $cgi->param(-name=>"flag-$flags_id_map{$key}", -value=>$data->{$key});
>             $found_one = 1;

Also please add a check and throw an error if the user is trying to set a flag's requestee and also setting the flag's status to something *other* then ? in the same call. For example the following data should be invalid:

 $data = {
      "<flag name>" => "+",
      "<flag name_requestee>" => "Bugzilla Account",
 }

Dave
Comment 3 Tony Fu 2009-03-09 02:47:05 EDT
Dave,

Thanks for the review.  I have updated my code according to your review, please see new attachment.


Thanks,
Tony
Comment 4 Tony Fu 2009-03-09 02:47:44 EDT
Created attachment 334467 [details]
Add updating flag's requestee function into updateFlag()
Comment 5 David Lawrence 2009-03-09 11:53:08 EDT
Comment on attachment 334467 [details]
Add updating flag's requestee function into updateFlag()

Tony, the patch looks good and works as expected.

But...I have thought about it some more and I think we are getting ourselves stuck where there are things you can do with the web UI that we cannot do with this XMLRPC method. For example the web UI allows for setting multiple flags of the same name if the admin has the system configured that way. With this XMLRPC method that cannot be done as we only look for the flag name and not the flag id in the params data. And other functions as well. Let's go ahead and check this in as it gives us:

1) Gives us exact same behaviour of the old updateFlags() method.
2) It gives the new ability to set the requestee of a flag using the same method instead of a separate method.

I think we should create a new method in the 3.2 XMLRPC API called Flags.update() that we will take some of the code from the compat xmlrpc and
design it in such a way that we can implement all of the same features of the web UI. And if people implement new tools that need to use these other
flag features, we will make them use the Flags.update method instead.

Thanks
Dave
Comment 6 David Lawrence 2009-03-13 16:49:57 EDT
Bug 489343 filed for the new Flag.update() method.

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