506538 - WebService function to list flag_types (Flag.get)

Status: RESOLVED DUPLICATE of bug 756048

(Bugzilla :: WebService, enhancement, P3)

Description

[Frank Becker]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_7; de-de) AppleWebKit/530.19.2 (KHTML, like Gecko) Version/4.0.2 Safari/530.19
Build Identifier: 3.4rc1

in bug#506530 we return only a list ids from the flag_types. We need an function that can return all needed fileds id, name, description, target_type, is_requestable, is_requesteeble, is_multiplicable, can_set_flag, can_request_flag.


Reproducible: Always

Comment 1

[Frank Becker]

Attachment: patch, V1

Comment 2

[Frank Becker]

Comment on attachment 390715 [details] [diff] [review]
patch, V1

This is now included in bug# 424921

Comment 3

[Frank Becker]

Please reopen this!

Now I need that function.

Comment 4

[Max Kanat-Alexander]

I don't think you need a whole module just for this, but then again I'm not sure where else it would go.

Comment 5

[Frank Becker]

Attachment: patch, V2

Comment 6

[Frank Becker]

(In reply to comment #4)
> I don't think you need a whole module just for this, but then again I'm not
> sure where else it would go.

What method do you think can be use by other clients.
Should I implement an create or update method?
Should we move FlagType.get to Bugzilla.flagtypes ?

Comment 7

[David Lawrence [:dkl]]

(In reply to comment #6)
> (In reply to comment #4)
> > I don't think you need a whole module just for this, but then again I'm not
> > sure where else it would go.
> 
> What method do you think can be use by other clients.
> Should I implement an create or update method?
> Should we move FlagType.get to Bugzilla.flagtypes ?

I think having a FlagType.get could be useful personally. I also think we need a Bug.flag_types as well which could be different sets of data even though they seem similar. 

FlagType.get should return info only on the particular flag. Something missing from your current patch though that would be useful to know is which product/component combinations the flag is enabled for. Just returning the basic flag info doesn't tell the client where the flag is visible. You may need to return both a included and excludes list, such as for example:

includes => [ 'Any', 'Any' ], excludes => [ 'Bugzilla', 'Bugzilla General' ]

The Bug.flag_types would just return the actual flag types visible for the given bug id. It should not include the includes/excludes part from above though as this is irrelevant to the given bug since it is visible already.

Please make the suggested changes to FlagType.get and I will review.

Dave

Comment 8

[Max Kanat-Alexander]

I think we might just want to call it Flag, also.

Comment 9

[Frédéric Buclin]

Comment on attachment 405114 [details] [diff] [review]
patch, V2

dkl's comment 7 means r-, so pushing this patch out of our radar.

I agree that we should use Flag.get instead of FlagType.get. This way, we can handle both flag types, and flags themselves, if we need to. Some useful methods which come to mind:

- Bug.get() should also include data about bug flags (in another bug).
- Bug.attachments should also include data about attachment flags (done in bug 714343).

I don't think we want Bug.flag_types, because it may be redundant with the not yet existing Flag.get_types method. Flag.get_types could take either a list of product/component IDs, or a list of bug IDs, or a list of flag type IDs, and return the appropriate data (this bug).

Comment 10

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #9) 
> - Bug.get() should also include data about bug flags (in another bug).
> - Bug.attachments should also include data about attachment flags (done in
> bug 714343).

Actually, bug 714343 combine Bug.get and Bug.attachments to both return 
the respective flags with the bug/attachment data.

> I don't think we want Bug.flag_types, because it may be redundant with the
> not yet existing Flag.get_types method. Flag.get_types could take either a
> list of product/component IDs, or a list of bug IDs, or a list of flag type
> IDs, and return the appropriate data (this bug).

The Flag.get_types would return the possible flags for a given product/component combination or by bug id if desired. These would be all flags possible to be set but not show set flags.. Where as Bug.get and Bug.attachments  return the flags that are currently set so with both of these methods we would pretty much be covered.

dkl

Comment 11

[Frank Becker]

Attachment: patch V3

This patch has the implementation for Flag.get_types (see comment#10)

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 612469 [details] [diff] [review]
patch V3

Review of attachment 612469 [details] [diff] [review]:
-----------------------------------------------------------------

Will do review of POD once we have the other issues resolved.

dkl

::: Bugzilla/WebService/Flag.pm
@@ +31,5 @@
> +sub get_types {
> +    my ($self, $params) = validate(@_, 'type_ids', 'product_ids');
> +    my @flagtypes;
> +    my $flagtypes_temp;
> +    my @flagtypes_temp1;

@flagtypes_temp1 doesn't seem to be used anywhere.

@@ +60,5 @@
> +            }
> +        }
> +        @$flagtypes_temp = sort { $a->sortkey <=> $b->sortkey || $a->name cmp $b->name }
> +                           values %visible_flagtypes;
> +    }

All this needs to be written in such a way that the flag types are filtered based on
whether the user has permission to see the products that the flags are enable for. If
a user uses type_ids instead of product_ids, they could enter any arbitrary list of ids
and get flag types possibly for products they cannot see. Normally this is handled by the 
surrounding code that calls Bugzilla::FlagType->match, etc. Also the inclusions/exclusions
would include the private product names.

@@ +70,5 @@
> +    push(@flagtypes, $self->_get_flag_attr($params, $_)) foreach @$flagtypes_temp;
> +    return { flagtypes => \@flagtypes };
> +}
> +
> +sub _get_flag_attr {

Change to _flag_to_hash to be consistent with other WebService code.

@@ +87,5 @@
> +        can_request_flag  => $self->type('boolean', $canrequestflag),
> +        inclusion         => $flagtype->inclusions,
> +        inclusion_hash    => $flagtype->inclusions_as_hash,
> +        exclusion         => $flagtype->exclusions,
> +        exclusion_hash    => $flagtype->exclusions_as_hash,

We really just want to return a key called 'inclusions' and a key called 'exclusions'. The client doesn't need to have to worry about the difference between inclusions and inclusions_to_hash. Also for $flagtype->inclusions/exclusions, the format is suited for use internally and is not easily parseable by the client. So for example lets return a hashes such as:

inclusions => [
    { 
        product      => 'Product1',
        product_id   => 45,
        component    => 'Component1',
        component_id => 22
    },
    {
        product      => 'Product2',
        product_id   => 46,
        component    => 'Any',
        component_id => 0,
    },
    ...
]

And similar for exclusions. 

Also, to create the inclusions/exclusions data structure, create another method called _clusions_to_hash() 
that takes the $flagtype->inclusions, changes it to the data structure above, and also calls the proper $self->type 
for each string and integer value.

Comment 13

[Frank Becker]

Attachment: patch V4

Comment 14

[David Lawrence [:dkl]]

Comment on attachment 619402 [details] [diff] [review]
patch V4

Review of attachment 619402 [details] [diff] [review]:
-----------------------------------------------------------------

t/005whitespace.t .... 1/1410 
#   Failed test 'Bugzilla/WebService/Flag.pm contains tabs --WARNING'
#   at t/005whitespace.t line 34

::: Bugzilla/WebService/Flag.pm
@@ +11,5 @@
> +# rights and limitations under the License.
> +#
> +# The Original Code is the Bugzilla Bug Tracking System.
> +#
> +# Contributor(s): Frank Becker <[e-mail]>

for the license use

# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.
#
# This Source Code Form is "Incompatible With Secondary Licenses", as
# defined by the Mozilla Public License, v. 2.0.

and no longer include the contributors list as this can viewed in BZR instead.

@@ +44,5 @@
> +        my @product_accessible = @$accessible_products;
> +        my %product_flagtypes;
> +        foreach my $product (@product_accessible) {
> +            foreach my $target ('bug', 'attachment') {
> +	            my $prod_flagtypes = $product->flag_types->{$target};

nit: remove extra indention

@@ +51,5 @@
> +        }
> +        my @requested_accessible;
> +        push(@requested_accessible,
> +            grep { $type_ids{$_->id} } values(%product_flagtypes));
> +        $visible_flagtypes{$_->id} ||= $_ foreach @requested_accessible;

There is alot of extra moving of values around here that I think we can optimize earlier up the code. For example how about this:

# Only flagtypes that are in the users accessible products,
# can be allowed to be returned
foreach my $product (@{ Bugzilla->user->get_accessible_products }) {
    foreach my $target ('bug', 'attachment') {
        foreach my $type (@{ $product->flag_types->{$target} }) {
            $visible_flagtypes{$type->id} = $type if $type_ids{$type->id};
        }
    }
}

@@ +73,5 @@
> +            foreach my $target ('bug', 'attachment') {
> +	            my $prod_flagtypes = $product->flag_types->{$target};
> +	    	    $visible_flagtypes{$_->id} ||= $_ foreach @$prod_flagtypes;
> +            }
> +        }

Similar here:

foreach my $product (@{ Bugzilla->user->get_accessible_products }) {
    next if !$product_ids{$product->id};
    foreach my $target ('bug', 'attachment') {
        foreach my $type (@{ $product->flag_types->{$target} }) {
            $visible_flagtypes{$type->id} = $type;
        }
    }
}

@@ +85,5 @@
> +            foreach my $target ('bug', 'attachment') {
> +	            my $prod_flagtypes = $product->flag_types->{$target};
> +	    	    $visible_flagtypes{$_->id} ||= $_ foreach @$prod_flagtypes;
> +            }
> +        }

I think we need to put this loop in a private method as we have done this now 3 times.

@@ +127,5 @@
> +                       component    => $self->type('string' , $comp),
> +                       product_id   => $self->type('int'    , $prod_id),
> +                       component_id => $self->type('int'    , $comp_id),
> +                      }
> +            );

nit:

push(@result, { product      => $self->type('string' , $prod),
                component    => $self->type('string' , $comp),
                product_id   => $self->type('int' , $prod_id),
                component_id => $self->type('int' , $comp_id) });

@@ +167,5 @@
> +=over
> +
> +=item B<Description>
> +
> +Returns a list of information about the flagtype passed to it.

s/flagtype/flag types/

@@ +184,5 @@
> +=over
> +
> +=item C<type_ids>
> +
> +An array of flagtyp ids.

s/flagtyp/flag type/

@@ +188,5 @@
> +An array of flagtyp ids.
> +
> +=item C<product_ids>
> +
> +an array of product ids.

An

@@ +195,5 @@
> +
> +=item B<Returns> 
> +
> +A hash containing one item, C<flagtypes>, that is an array of
> +hashes. Each hash describes a flagtypes, and has the following items:

hashes. Each hash describes a specific flag type, and has the following items:

@@ +201,5 @@
> +=over
> +
> +=item C<id>
> +
> +The ID of the flagtype (Type = integer).

C<int> The ID of the flag type

Same for the other return keys such as C<string> and C<boolean>

@@ +231,5 @@
> +This boolean attribute is true if multiple flags of this type can be set on the same bug.
> +
> +=item C<can_set_flag> 
> +
> +This boolean attribute is true if the user is in the grant group (this meens that

s/meens/means/

@@ +241,5 @@
> + are requestable, this user can request them).
> +
> +=item C<inclusion>
> +
> +An array of hashes. Each hash describes a inclusion.

s/a/an/

@@ +247,5 @@
> +=over
> +
> +=item B<product>
> +
> +C<String> The name of an product or __Any__ for all products.

string in lowercase

@@ +251,5 @@
> +C<String> The name of an product or __Any__ for all products.
> +
> +=item B<component>
> +
> +C<String> The name of an component or __Any__ for all components.

same

@@ +265,5 @@
> +=back
> +
> +=item C<exclusion>
> +
> +An array of hashes. Each hash describes a exclusion.

s/a/an/

@@ +269,5 @@
> +An array of hashes. Each hash describes a exclusion.
> +
> +=over
> +
> +=item B<product>

use C<> instead of B<> to be consistent with other returned values.

@@ +296,5 @@
> +=item B<Errors> (none)
> +
> +=item B<History>
> +
> +Added in Bugzilla B<4.4>.

=over

=item Added in Bugzilla B<4.4>.

=back

Comment 15

[Frank Becker]

Attachment: patch V5

reworked based on comment#14

Comment 16

[David Lawrence [:dkl]]

Comment on attachment 643260 [details] [diff] [review]
patch V5

Review of attachment 643260 [details] [diff] [review]:
-----------------------------------------------------------------

Comments below...

::: Bugzilla/WebService/Flag.pm
@@ +36,5 @@
> +         foreach my $product (@{ Bugzilla->user->get_accessible_products }) {
> +             foreach my $target ('bug', 'attachment') {
> +                foreach my $type (@{ $product->flag_types->{$target} }) {
> +                    next if !$type_ids{$type->id};
> +                    $visible_flagtypes{$type->id} = $type if $type_ids{$type->id};

Nit: The second if is redundant. Just write as:

                   $visible_flagtypes{$type->id} = $type;

Actually instead, lets do:

1) change _add_visible_flagtypes to just _visible_flagtypes
2) Instead of copying the return of _visible_flagtypes to %visible_flagtypes, pass in a reference
to %visible_flagtypes where it can be added to instead of overwriting it.

For example:

my %visible_flagtypes;

if (defined $params->{type_ids}) {
    ...
 
    foreach my $type_id (@{ $params->{type_ids} }) {
        foreach my $product (@{ Bugzilla->user->get_accessible_products }) {
            $self->_visible_flagtypes(\%visible_flagtypes, $product, $type_id);
        }
    }
}

@@ +60,5 @@
> +        my @requested_accessible = @$accessible_products;
> +
> +        foreach my $product (@requested_accessible) {
> +            %visible_flagtypes = $self->_add_visible_flagtypes($product, %visible_flagtypes);
> +        }

With my suggested method change outlined above we can also shorten this by writing:

       foreach my $product (@{ Bugzilla->user->get_accessible_products }) {
           $self->_visible_flagtypes(\%visible_flagtypes, $product);
       }

@@ +76,5 @@
> +            foreach my $target ('bug', 'attachment') {
> +                foreach my $type (@{ $product->flag_types->{$target} }) {
> +                    $visible_flagtypes{$type->id} = $type;
> +                }
> +            }

indention is wrong from foreach to here

As outlined above we can change this method to:

sub _visible_flagtypes {
    my ($self, $visible_flagtypes, $product, $type_id) = @_;
    foreach my $target ('bug', 'attachment') {
        foreach my $type (@{ $product->flag_types->{$target} }) {
             next if $type_id && $type_id != $type->id;
             $$visible_flagtypes{$type->id} = $type;
        }
    }
}
}

@@ +81,5 @@
> +
> +    return %visible_flagtypes; 
> +}
> +
> +sub _flag_to_hash {

Rename to _flagtype_to_hash as I predict we will we will need a separate _flag_to_hash in the future if Flag.pm 
supports getting set flags for a given bug id or attachment.

@@ +125,5 @@
> +
> +=head1 NAME
> +
> +Bugzilla::Webservice::Flag - The API for getting the
> +details of Flag and FlagTypes.

s/Flag/Flags/

@@ +131,5 @@
> +=head1 DESCRIPTION
> +
> +This part of the Bugzilla API allows you to get al the details of the
> +FlagType. 
> +

This part of the Bugzilla API allows retrieval of the details about Flags and FlagTypes.

@@ +151,5 @@
> +=over
> +
> +=item B<Description>
> +
> +Returns a list of information about the flag types passed to it.

Returns a detailed list of flagtypes requested that are accessible to the user.

@@ +158,5 @@
> +
> +You can pass either flagtype ids or product ids..
> +
> +B<Note>: If neither C<type_ids> nor C<product_ids> is specified, then all 
> +flagtypes will be returned.

accessible flagtypes will be returned.

@@ +197,5 @@
> +C<string> The description of the flag type.
> +
> +=item C<target_type>
> +
> +C<string> The type of the flag type. Leagal Values are C<bug> or 

Legal values

@@ +207,5 @@
> +
> +=item C<is_requesteeble>
> +
> +C<boolean> This attribute is true if users can ask specific other users to set flags 
> +of this type as opposed to just asking the wind.

s/other//

@@ +225,5 @@
> + are requestable, this user can request them).
> +
> +=item C<inclusion>
> +
> +An array of hashes. Each hash describes an inclusion.

Need to describe what inclusion means, such as that this flag is only visible for bugs in particular combinations of Product/Component, etc.

@@ +249,5 @@
> +=back
> +
> +=item C<exclusion>
> +
> +An array of hashes. Each hash describes an exclusion.

Same as inclusion where we need to describe what exclusion means.

@@ +274,5 @@
> +
> +=back 
> +
> +Note, that if the user tries to access a flagtype that not
> +exists, that is silently ignored, and you get no error. 

Note, that if a user tries to access a flagtype that does not exist, it is silently ignored and no error is returned.

@@ +279,5 @@
> +
> +=item B<Errors> (none)
> +
> +=item B<History>
> +

=item Added in Bugzilla B<4.4>.

Comment 17

[Frank Becker]

Attachment: patch V6

Comment 18

[David Lawrence [:dkl]]

Comment on attachment 673273 [details] [diff] [review]
patch V6

Review of attachment 673273 [details] [diff] [review]:
-----------------------------------------------------------------

Works otherwise so r=dkl. Please submit new patch with request_group and grant_group added and I will move my r+ forward on verification.

::: Bugzilla/WebService/Flag.pm
@@ +27,5 @@
> +    my %visible_flagtypes;
> +    my $flagtypes_temp;
> +
> +    if (defined $params->{type_ids}) {
> + #       }

Remove on commit

@@ +85,5 @@
> +        is_multiplicable  => $self->type('boolean', $flagtype->is_multiplicable),
> +        can_set_flag      => $self->type('boolean', $cansetflag),
> +        can_request_flag  => $self->type('boolean', $canrequestflag),
> +        inclusion         => $self->_clusion_to_hash($flagtype->inclusions),
> +        exclusion         => $self->_clusion_to_hash($flagtype->exclusions),

We need to add (similar to bug 741722) request_group and grant_group to the returned data and related POD.

Comment 19

[Frédéric Buclin]

Comment on attachment 673273 [details] [diff] [review]
patch V6

>=== added file 'Bugzilla/WebService/Flag.pm'

>+sub get_types {

You must have editcomponents privs to be allowed to access this information. I see no such restrictions.

Also, is there any plan to have information about a specific instance of a flag type, i.e. flags set in a bug or attachment? If not, then this method should be renamed get().

dkl?

Comment 20

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #19)
> Comment on attachment 673273 [details] [diff] [review]
> patch V6
> 
> >=== added file 'Bugzilla/WebService/Flag.pm'
> 
> >+sub get_types {
> 
> You must have editcomponents privs to be allowed to access this information.
> I see no such restrictions.

I assumed this would be OK since we do not yet have the ability to make flags private and people can just go to enter a bug about a specific product and component to see much of the same information and which flags are available. Although we could possibly argue that we should require 'editcomponents' to see the request_group and grant_group info but right now it would only show the id and not the group names.

Obviously if we ever implement a Flag.update_type then I would obviously need to limit that to 'editcomponents'.

> Also, is there any plan to have information about a specific instance of a
> flag type, i.e. flags set in a bug or attachment? If not, then this method
> should be renamed get().

Yes eventually I would like to see a Flag.get for obtaining details about a "set" flag given either flag "ids" or "bug_ids". We can implement that a separate bug of course.

dkl

Comment 21

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #20)
> I assumed this would be OK since we do not yet have the ability to make
> flags private and people can just go to enter a bug about a specific product
> and component to see much of the same information and which flags are
> available.

Except that there is no reason to disclose more information here than what you can get from the UI. Also, if there are sensitive flag names in private products, I don't want a security hole in this WebServices method to disclose them.

Comment 22

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #21) 
> Except that there is no reason to disclose more information here than what
> you can get from the UI. Also, if there are sensitive flag names in private
> products, I don't want a security hole in this WebServices method to
> disclose them.

We already filter out products the user cannot see which also removes the related flag types as well. So flag names/descriptions with sensitive data can be seen already via the UI. What if we do this instead:

sub _flagtype_to_hash {
    my ($self, $params, $flagtype) = @_;
    my $cansetflag     = Bugzilla->user->can_set_flag($flagtype);
    my $canrequestflag = Bugzilla->user->can_request_flag($flagtype);

    my $field_data = {  
        id                => $self->type('int'    , $flagtype->id),
        name              => $self->type('string' , $flagtype->name),
        description       => $self->type('string' , $flagtype->description),
        target_type       => $self->type('string' , $flagtype->target_type),
    };

    if (Bugzilla->user->in_group('editcomponents')) {
        $field_data->{is_requestable}   = $self->type('boolean', $flagtype->is_requestable);
        $field_data->{is_requesteeble}  = $self->type('boolean', $flagtype->is_requesteeble);
        $field_data->{is_multiplicable} = $self->type('boolean', $flagtype->is_multiplicable);
        $field_data->{can_set_flag}     = $self->type('boolean', $cansetflag);
        $field_data->{can_request_flag} = $self->type('boolean', $canrequestflag);
        $field_data->{inclusion}        = $self->_clusion_to_hash($flagtype->inclusions);
        $field_data->{exclusion}        = $self->_clusion_to_hash($flagtype->exclusions); 
    }

    return filter($params, $field_data);
}

If so, we would also need to go back and do the same for Product.get.

dkl