Bug 494478

Created attachment 338459 [details]
new xmlrpc function Flag.get()

Noura,

This patch includes another function Flag.update() which may need to change later.  Could you please just review the Flag.get() function first?


Thanks,
Tony

Comment on attachment 338459 [details]
new xmlrpc function Flag.get()

Hey Tony,

Patch looks good to me, tested it and seems to be working fine for Flag.get, my only suggestion is to make the function accept multiple bug ids and return flag info for them in an array I would say, so use should be able to call the function as the following:

$call->rpc('Flag.get', {ids => [1,2]});

and then you would loop through those ids and push the hashes you create in an array that you can return at the end.

Thanks,
Noura

Comment on attachment 338459 [details]
new xmlrpc function Flag.get()

>Index: Bugzilla/WebService/Flag.pm
>+# function to get flag information
>+# $call = $rpc->call('Flag.get', id)
>+sub get {
>+    my ($self, $id) = @_;
>+    my $user = Bugzilla->login(LOGIN_REQUIRED);
>+
>+    # REDHAT EXTENSION BEGIN 406301
>+    $logger->debug("Starting: " . Bugzilla->user->login);
>+    # REDHAT EXTENSION END 406301
>+    
>+    $id || ThrowCodeError('param_required', { param => 'id' });

As Noura mentioned, we need to be able to take single and multiple ids.

>+
>+    ValidateBugID($id);
>+    my $bug = new Bugzilla::Bug($id);
>+
>+    my (%item, %flag) = ();
>+    # get attachment's information
>+    $item{attachments} = $bug->attachments;
>+    if (not $item{attachments}) {
>+        $flag{attachments} = [];
>+    } else {
>+        foreach my $attach (@{$item{attachments}}) {
>+           my %attach_attr = ();
>+           $attach_attr{attach_id} = $attach->{id};
>+           $attach_attr{description} = $attach->{description};
>+           push (@{$flag{attachments}}, \%attach_attr);
>+        }
>+    }
>+
>+    # get flagtype's information
>+    $item{flag_types}  = $bug->flag_types;
>+    if (not $item{flag_types}) {
>+        $flag{flag_types} = [];
>+    } else {
>+        foreach my $flag_type (@{$item{flag_types}}) {
>+            my %flagtype_attr = ();
>+            $flagtype_attr{type_id} = $flag_type->{id};
>+            $flagtype_attr{name} = $flag_type->{name};
>+            $flagtype_attr{description} = $flag_type->{description};
>+            $flagtype_attr{flags} = $flag_type->{flags};
>+            push (@{$flag{flag_type}}, \%flagtype_attr);
>+        }
>+    }
>+
>+    return \%flag;
>+}

Nit-picks:

1. Why use the %item hash instead of just accessing the $bug attributes
directly here and above for attachments?
2. %flagtype_attr would be more readable if you just declare it with the
proper values initially.
3. Also $flag_type{flags} should be filtered similar to the flag type.
4. Use 'id' instead of 'type_id' for the flag type.

For example:

    # get flagtype's information
    $flag{flag_types} = [];
    foreach my $flag_type (@{$bug->flag_types}) {
        my %flagtype_attr = (
            id               => $flag_type->{id},
            name             => $flag_type->{name},
            description      => $flag_type->{description},
            is_requestable   => $flag_type->{is_requestable},
            is_multiplicable => $flag_type->{is_multiplicable},
            is_requesteeble  => $flag_type->{is_requesteeble},
            is_active        => $flag_type->{is_active},
            flags            => [],
        );

        foreach my $flag (@{$flag_type->{flags}}) {
            my %flag_attr = (
                id           => $flag->{id},
                status       => $flag->{status},
                setter_id    => $flag->{setter_id},
                requestee_id => $flag->{requestee_id},
            );

            if (Bugzilla->user->id) {
                setter_email    => user_id_to_login($flag->{setter_id}),
                requestee_email => user_id_to_login($flag->{requestee_id}),
            }

            push(@{$flagtype_attr{flags}}, \%flag_attr);
        }

        push (@{$flag{flag_type}}, \%flagtype_attr);
    }

And similar structure for the attachments section above. You will need to include Bugzilla::User at the top of the
Flag.pm file to import user_id_to_login().

What do you think?

Dave

Created attachment 338656 [details]
Flag.get() function

Noura, Dave,

Thanks for the review.  I have changed the Flag.get() according to your review, please take a look.


Thanks,
Tony

Comment on attachment 338656 [details]
Flag.get() function

looks good to me Tony, tested it and it works well.

Thanks,
Noura

Comment on attachment 338656 [details]
Flag.get() function

># -*- Mode: perl; indent-tabs-mode: nil -*-
>#
># The contents of this file are subject to the Mozilla Public
># License Version 1.1 (the "License"); you may not use this file
># except in compliance with the License. You may obtain a copy of
># the License at http://www.mozilla.org/MPL/
>#
># Software distributed under the License is distributed on an "AS
># IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
># implied. See the License for the specific language governing
># rights and limitations under the License.
>#
># The Initial Developer of the Original Code is Netscape Communications
># Corporation. Portions created by Netscape are
># Copyright (C) 1998 Netscape Communications Corporation. All
># Rights Reserved.
>#
># The Original Code is the Bugzilla Bug Tracking System.
>#
># Contributor(s): Tony Fu <tfu>
>#                 
>
>package Bugzilla::WebService::Flag;
>
>use strict;
>use base qw(Bugzilla::WebService);
>use Bugzilla::User;
>use Bugzilla::Bug;
>use Bugzilla::Flag;
>use Bugzilla::Constants;
>use Bugzilla::Error;
>
># REDHAT EXTENSION BEGIN 406301
>use Bugzilla;
>my $logger = Bugzilla->logger;
># REDHAT EXTENSION END 406301
>
>
># function to get flag information
># $call = $rpc->call('Flag.get', {ids=>[bug_id, ...])
>sub get {
>    my ($self, $params) = @_;
>    my $ids = $params->{ids};
>    my $user = Bugzilla->login(LOGIN_REQUIRED);
>
>    # REDHAT EXTENSION BEGIN 406301
>    $logger->debug("Starting: " . Bugzilla->user->login);
>    # REDHAT EXTENSION END 406301
>    

Need to also work with single scalar for 'ids'

     my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{ids});

>    defined $ids || ThrowCodeError('param_required', { param => 'ids' });
>
>    my @flags;
>    foreach my $bug_id (@$ids) {
>        ValidateBugID($bug_id);
>        my $bug = new Bugzilla::Bug($bug_id);
>
>        my %flag = ();
>        # get attachment's information
>        foreach my $attach (@{$bug->attachments}) {
>            my %attach_attr = (
>                attach_id   => $attach->{id},
>                description => $attach->{description},
>                bug_id      => $bug_id,
>                flags       => []
>            );
>            if (Bugzilla->user->id) {
>                $attach_attr{attacher_email} = user_id_to_login($attach->{attacher_id});
>            }
>            foreach my $flag (@{$attach->{flags}}) {
>                my $flag_attr = _get_flag_attr($flag);
>                push(@{$attach_attr{flags}}, $flag_attr);
>           }
>           push (@{$flag{attachments}}, \%attach_attr);
>        }
>
>        # get flagtype's information
>        foreach my $flag_type (@{$bug->flag_types}) {
>            my %flagtype_attr = (
>                id               => $flag_type->{id},
>                name             => $flag_type->{name},
>                description      => $flag_type->{description},
>                is_requestable   => $flag_type->{is_requestable},
>                is_multiplicable => $flag_type->{is_multiplicable},
>                is_requesteeble  => $flag_type->{is_requesteeble},
>                is_active        => $flag_type->{is_active},
>                bug_id           => $bug_id,
>                flags  => [],
>            );
>
>            foreach my $flag (@{$flag_type->{flags}}) {
>                my $flag_attr = _get_flag_attr($flag);
>                push(@{$flagtype_attr{flags}}, $flag_attr);
>            }
>
>            push (@{$flag{flag_type}}, \%flagtype_attr);

Nit: Better to call this 'bug' rather than 'flag_type'. This way we have the two distinctions,
'attachments' and 'bug' flag types.

>        }
>
>        push(@flags, \%flag);
>    }
>
>    return \@flags;
>}

You need to include the bug id in your return data structure so that the user knows which bug
the flag data belongs to.

So the return struct would look like:

$VAR1 = [
          {
            'bug_id' => <bug_id>,
            'attachments' => [ ... ],
            'bug => [ ... ],
          },
          ...
        ];

Or maybe it should be:

$VAR1 = [
          {
            <bug_id> => {
              'attachments' => [ ... ],
              'bug => [ ... ],
            },
          },
          ...
        ];
        
Noura what do you think? The rest looks good so far Tony. Make sure the docs are up to date as well.

Dave

(In reply to comment #6) 
> You need to include the bug id in your return data structure so that the user
> knows which bug
> the flag data belongs to.
> 
> So the return struct would look like:
> 
> $VAR1 = [
>           {
>             'bug_id' => <bug_id>,
>             'attachments' => [ ... ],
>             'bug => [ ... ],
>           },
>           ...
>         ];
> 
> Or maybe it should be:
> 
> $VAR1 = [
>           {
>             <bug_id> => {
>               'attachments' => [ ... ],
>               'bug => [ ... ],
>             },
>           },
>           ...
>         ];
> 
> Noura what do you think? The rest looks good so far Tony. Make sure the docs
> are up to date as well.

After more thought and looking through the upstream code I think we need to stick to the normal convention that the upstream is trying to adhere to.

So I propose the returned data structure should look like:

$VAR1 = {
          bugs => {
                    <bug_id1> => {
                                   'attachments' => [ ... ],
                                   'bug'         => [ ... ],
                                 },
                    <bug_id2> => {
                                   'attachments' => [ ... ],
                                   'bug'         => [ ... ],
                                 },
                    ...,
                  }
         };

Later we would need to add a 'faults' key to contain any errors that are encountered if the 'permissive' param is set to true.

The reason for all of this is that since we are putting this in the 3.x API and
not the compat_xmlrpc extension, we need to make sure we stick with the upstream
format when possible.

Dave

Noura and Dave,

Thanks for the review.  I have modified the code to return the data structure proposed by Dave.  Also I added the pod doc about Flag.get().


Thanks,
Tony

Created attachment 338869 [details]
Flag.get() function in Bugzilla/WebService/Flag.pm

1. returned the data structure proposed by dkl
2. add Flag.get() information into POD

Comment on attachment 338869 [details]
Flag.get() function in Bugzilla/WebService/Flag.pm

># function to get flag information
># $call = $rpc->call('Flag.get', {ids=>[bug_id, ...])
>sub get {
>    my ($self, $params) = @_;
>    my $ids = $params->{ids};
>    my $user = Bugzilla->login(LOGIN_REQUIRED);
>
>    # REDHAT EXTENSION BEGIN 406301
>    $logger->debug("Starting: " . Bugzilla->user->login);
>    # REDHAT EXTENSION END 406301
>    
>    defined $ids || ThrowCodeError('param_required', { param => 'ids' });

Still needs to also work with single scalar for 'ids'

     my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{ids});


>=head1 NAME
>
>Bugzilla::WebService::Flag - The Flag API
>
>=head1 DESCRIPTION
>
>This part of the Bugzilla API allows you to update flags on a bug or an attachment.
>
>=head1 METHODS
>
>See L<Bugzilla::WebService> for a description of what B<STABLE>, B<UNSTABLE>,
>and B<EXPERIMENTAL> mean, and for more information about error codes.
>
>=over
>
>=item C<get> B<UNSTABLE>
>
>=over
>
>=item B<Description>
>
>Get information of all flags (including attachment flags) associated with a specific bug
>

Get information on all flags (including attachment flags) associated with a specific set of bugs.
Also allows updating of flag information for a specific set of bugs.

>=item B<Params>
>
>=over
>
>=item B<ids> - C<int>.  An array of bug ids.
>
>=item B<permissive> - C<Boolean>.
>
>Normally, if you pass invalid bug ids, Flag.get will throw an error.  If this parameter is True, instead of >throwing an error, we return an array of hashes with a "id", "faultString" and "faultCode" for each invalid >bug id, and return normal flags information for the other bugs
>
>=back
>
>=item B<Returns>
>
>Two items are returned:
>
>=over
>
>=item B<bugs>
>
>A hashes that contains flag information associated with bugs.  The hash key is bug id and the value is a hash >that contains the following two items
>

An array of hashes that contain flag information associated with a specific set of bugs. The hash key is the bug id and the value is a hash that contains the following two items

>=over
>
>=item B<attachment> 
>
>A hash array including the information about all attachments' flags in a specific bug.  And each hash >includes the following items:
>

An array of hashes including the information about all attachments' flags for a specific bug. And each hash includes the following items:

>=over
>
>=item B<flags> - a hash array including the information of flags associated with an attachment.  And each hash has 'status', 'requestee_email', 'id', and 'setter_email' keys and their coresponding values.
>
>=item B<attacher_email> - the person who set this attachment.
>
>=item B<description> - the description of this attachment.
>
>=item B<attach_id> - the attach_id of this attachment.
>
>=back
>
>=item B<bug>
>
>A hash array including the information about all normal flags (not attachment's flag) in a specific bug.  And each hash includes the following items:
>

An array of hashes including the information about all normal bug flags (not attachment flags) for a specific bug. Each hash includes the following items:

>=over
>
>=item B<flags> - a hash array including the information of flags associated with a flagtype.  And each hash has 'status', 'requestee_email', 'id', and 'setter_email' keys and their coresponding values.
>
>=item B<name> - the flagtype name.
>
>=item B<description> - the description of this flagtype.
>
>=item B<is_requestable> - 1 if this flagtype is requestable and 0 if this flagtype is not 
>
>=item B<is_multiplicable> - 1 if this flagtype is multiplicable and 0 if this flagtype is not
>
>=item B<is_requesteeble> - 1 if this flagtype is requesteeble and 0 if this flagtype is not 
>
>=item B<is_active> - 1 if this flagtype is active and 0 if this flagtype is not 
>
>=item B<id> -  this flagtype's id
>
>=back
>
>=back
>
>=item B<faults>
>
>An array of hashes that contains invalid bug ids with error messages
>returned for them. Each hash contains the following items:
>
>=over
>
>=item id
>
>C<int> The numeric bug_id of this bug.
>
>=item faultString 
>
>C<string> This will only be returned for invalid bugs if the C<permissive>
>argument was set when calling Flag.get, and it is an error indicating that 
>the bug id was invalid.
>
>=item faultCode
>
>C<int> This will only be returned for invalid bugs if the C<permissive>
>argument was set when calling Flag.get, and it is the error code for the 
>invalid bug error.
>
>=back
>
>=back
>


The built in test suite found the following POD errors:

*** WARNING: line containing nothing but whitespace in paragraph at line 451 in file Bugzilla/WebService/Flag.pm
*** ERROR: =over on line 303 without closing =back at line EOF in file Bugzilla/WebService/Flag.pm
not ok 140 - Bugzilla/WebService/Flag.pm has incorrect POD syntax --ERROR
#   Failed test 'Bugzilla/WebService/Flag.pm has incorrect POD syntax --ERROR'
#   at t/011pod.t line 56.

Please run the test suite each time you submit a patch for review to catch common errors. I usually do:

$ perl runtests.pl --noxmlrpc --noselenium

Also please remove the Flag.update code from this patch as we are going to review that separately from this bug report.

Dave

Created attachment 339415 [details]
Flag.get() function in Bugzilla/WebService/Flag.pm

changes:

1. handle multiple bug ids
2. pod doc

Comment on attachment 339415 [details]
Flag.get() function in Bugzilla/WebService/Flag.pm

>Index: template/en/default/global/user-error.html.tmpl
>===================================================================
>--- template/en/default/global/user-error.html.tmpl	(revision 1198)
>+++ template/en/default/global/user-error.html.tmpl	(working copy)
>@@ -545,6 +545,15 @@
>     You can't ask more than one person at a time for
>     <em>[% type.name FILTER html %]</em>.
> 
>+  [% ELSIF error ==  "update_flags_invalid_flag_name" %]
>+    [% title = "Invalid or No Flag Name Provided" %]
>+    No data given for change or flag name invalid.
>+
>+  [% ELSIF error == "update_flagrequestees_invalid_flag_name" %]
>+    [% title = "Invalid or No Flag Name Provided" %]
>+    Flag name invalid or the provided flag is not 
>+    requesteeble flag.
>+
>   [% ELSIF error == "flag_requestee_needs_privs" %]
>     [% title = "Flag Requestee Needs Privileges" %]
>     [% requestee.identity FILTER html %] does not have permission to set the

Looks good Tony. Please leave these errors out though in your final check in as I think they go with the Flag.update() method which will be addressed in a different bug.

Good work
Dave