Bug 494478 - Create new webservice method Flags.get for getting flag and attachment attributes for a given bug
Create new webservice method Flags.get for getting flag and attachment attrib...
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-04-07 00:12 EDT by Tony Fu
Modified: 2013-06-24 00:03 EDT (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Enhancement
Doc Text:
Story Points: ---
Clone Of: 489343
Environment:
Last Closed: 2009-05-19 03:51:41 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)
new xmlrpc function Flag.get() (11.81 KB, patch)
2009-04-07 02:41 EDT, Tony Fu
no flags Details | Diff
Flag.get() function (11.27 KB, text/plain)
2009-04-08 02:54 EDT, Tony Fu
nelhawar: review+
dkl: review-
Details
Flag.get() function in Bugzilla/WebService/Flag.pm (14.39 KB, text/plain)
2009-04-09 03:18 EDT, Tony Fu
dkl: review-
Details
Flag.get() function in Bugzilla/WebService/Flag.pm (9.25 KB, patch)
2009-04-13 23:54 EDT, Tony Fu
dkl: review+
Details | Diff

  None (edit)
Description Tony Fu 2009-04-07 00:12:57 EDT
Create a new function Flag.get() to get flag and attachment's information.

--- Additional comment from Noura

Hey Tony,

Regarding getting the flagtypes ids and attach ids of a specific bug you can use the current xmlrpc function in the webservice.pl extension bugzilla.getBug as it returns all details about bug flags and attachments, but maybe it is better to have a specific function to return only flag details including attachment flags then it is a good idea to have a new function Flag.get,, a function that you can pass to bug ids and it should get the bug objects from those ids and filter our  bug and attachment flag types from the returned bug object details.

Noura

--- Additional comment from dkl@redhat.com on 2009-04-06 16:53:04 EDT ---


Tony, Noura. I agree that having a Bugzilla::WebService::Flag::get() function would complement the new update() function. A user would then be able to use that to get all flag information for a bug and any attachments for the bug. The bugzilla.getBug() method will eventually go away in favor of the 3.X API so we need to have this in Bugzilla::WebService::Flag as well.

Tony, please work on that now. File a new bug to track the new xmlrpc method as this is a separate issue (regarding getting flag information) and then make the new bug block this one. Make the returned data structure distinguish between bug and attachment flags.
 
Then when the Flag.get method is done and committed, we can have the new Flag.update method throw an error if the user tries to make a change to a multiplicable flag that already has a previous flag status set and the user doesn't specify the flag type id.

Also the Flag.update method can look for the attach_id as well.

Thanks
Dave
Comment 1 Tony Fu 2009-04-07 02:41:30 EDT
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 2 Noura El hawary 2009-04-07 08:44:45 EDT
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 3 David Lawrence 2009-04-07 17:28:57 EDT
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
Comment 4 Tony Fu 2009-04-08 02:54:53 EDT
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 5 Noura El hawary 2009-04-08 08:29:42 EDT
Comment on attachment 338656 [details]
Flag.get() function

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

Thanks,
Noura
Comment 6 David Lawrence 2009-04-08 13:34:53 EDT
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@redhat.com>
>#                 
>
>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
Comment 7 David Lawrence 2009-04-08 14:49:55 EDT
(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
Comment 8 Tony Fu 2009-04-09 03:13:28 EDT
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
Comment 9 Tony Fu 2009-04-09 03:18:30 EDT
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 10 David Lawrence 2009-04-10 00:54:55 EDT
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
Comment 11 Tony Fu 2009-04-13 23:54:32 EDT
Created attachment 339415 [details]
Flag.get() function in Bugzilla/WebService/Flag.pm

changes:

1. handle multiple bug ids
2. pod doc
Comment 12 David Lawrence 2009-04-14 00:40:06 EDT
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

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