Bug 486898

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

Dave,

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


Thanks,
Tony

Created attachment 334467 [details]
Add updating flag's requestee function into updateFlag()

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

Bug 489343 filed for the new Flag.update() method.