424921 - WebService function to list product components

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Noura Elhawary]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.9) Gecko/20070209 Fedora/1.5.0.9-3.fc6 Firefox/1.5.0.9 pango-text
Build Identifier: 

Need a WebService function to enable listing a product's components.

Reproducible: Always

Comment 1

[Max Kanat-Alexander]

We don't need a whole new function, just a new argument to Component.get.

Comment 2

[Noura Elhawary]

Attachment: WebService function Component.get_by_product

New webservice function for new WebService module Component.pm called Component.get_by_product()  to list components of given products. you can pass to it either product ids or names or both:
$call = $rpc->call('Component.get_by_product', {ids => [1,2], names => 'TestProduct']});

it returns a hash containing one item  'components_by_products' that points to a hash of product names each pointing to an array of hashes with each hash containing a component info. 

I didn't include the POD yet, Please review when you can and if it looks good then I will start writing the pod for it.

Noura

Comment 3

[Noura Elhawary]

(In reply to comment #1)
> We don't need a whole new function, just a new argument to Component.get.
> 

Hi Max, I created this function as per your suggestion in:
https://bugzilla.mozilla.org/show_bug.cgi?id=385282#c25

I think having new function maybe a better solution specially that the output format is abit different to Component.get , and it might cause confusion if we include it to Component.get?

Noura

Comment 4

[Frédéric Buclin]

Comment on attachment 311511 [details] [diff] [review]
WebService function Component.get_by_product

>+package Bugzilla::WebService::Component;

I don't understand why you put this method here. IMO, getting components of products is a method of products (Product.get_components), not of components. So my vote to put the method in the existing WebService/Product.pm file.


>+    foreach my $product (@accessible_products) {
>+        my @components = $product->components;
>+        foreach my $item (@components){
>+            foreach my $c (@$item){
>+                my $component_obj = new Bugzilla::Component($c->{id});

I don't understand what you do here. $product->components already returns a list of component objects, which is what you want.

Comment 6

[Frank Becker]

Attachment: Patch, V1

Patch from bug#505549

Comment 7

[David Lawrence [:dkl]]

Comment on attachment 390026 [details] [diff] [review]
Patch, V1

>+sub get_components {
>+    my ($self, $params) = validate(@_, 'ids');
>+
>+    # Only products that are in the users accessible products,
>+    # can be allowed to be returned
>+    my $accessible_products = Bugzilla->user->get_accessible_products;
>+
>+    # Create a hash with the ids the user wants
>+    my %ids = map { $_ => 1 } @{$params->{ids}};
>+
>+    # Return the intersection of this, by grepping the ids from
>+    # accessible products.
>+    my @requested_accessible = grep { $ids{$_->id} } @$accessible_products;
>+    my @componentlist;

Nit: Use underscore in the var name (@component_list)

>+    foreach my $product (@requested_accessible) {
>+      my $components = $product->components;
>+      push(@componentlist, map {{
>+                                  internals   => $_,
>+                                  id          => $self->type('int', $_->id),
>+                                  name        => $self->type('string', $_->name),
>+                                  description => $self->type('string', $_->description),
>+                                }
>+                           }@$components);
>+    }

Please use 4 space indentation.

>+    return { Components => \@componentlist };
>+}

The returned data structure should be similar to the following:

      return { 
          products => {
              '<product_name>' => {
                  components => \@components_for_product
              },
              ...
          }
      };

This way if the caller passes in multiple product id's the component lists are all kept separate. Otherwise you have all of the components in one list as coded in your patch. 

All else looks good. One thing to consider is the ability to pass in a { names => [] } param also instead of just 'ids' in case the caller does not know the product id.
I understand that Product.get does not take names either so not a show stopper but it also should take names eventually. See User.get for example of what I mean.

Dave

Comment 8

[Frank Becker]

Attachment: patch, V2

(In reply to comment #7)
...

Hope that I have fixed all things.

Comment 9

[David Lawrence [:dkl]]

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

>+sub get_components {
>+    my ($self, $params) = validate(@_, 'ids', 'names');
>+
>+    # Only products that are in the users accessible products,
>+    # can be allowed to be returned
>+    my $accessible_products = Bugzilla->user->get_accessible_products;
>+
>+    my @requested_accessible;
>+    if ($params->{names}) {
>+        # Create a hash with the ids the user wants
>+        my %names = map { $_ => 1 } @{$params->{names}};
>+
>+        # Return the intersection of this, by grepping the ids from
>+        # accessible products.
>+        @requested_accessible = grep { $names{$_->name} } @$accessible_products;
>+    }
>+    else {
>+        # Create a hash with the ids the user wants
>+        my %ids = map { $_ => 1 } @{$params->{ids}};
>+
>+        # Return the intersection of this, by grepping the ids from
>+        # accessible products.
>+        @requested_accessible = grep { $ids{$_->id} } @$accessible_products;
>+    }

From my understanding of the Web Services roadmap we definitely want the final query list to be 
an intersection of products by id and products by name, not exclusive to one or the other. So we
need to be able to take id's, name's, or both. So for example something like this:

      my %ids = map { $_ => 1 } @{$params->{ids}};
      my %names = map { $_ => 1 } @{$params->{names}};
      my @requested_accessible = grep { $ids{$_->id} || $names{$_->name} } @$accessible_products;


for this we want to be able to take ei
>+    my @result_list;
>+    foreach my $product (@requested_accessible) {
>+        my $components = $product->components;
>+        my @component_list =
>+             map {{
>+                 internals   => $_,
>+                 id          => $self->type('int', $_->id),
>+                 name        => $self->type('string', $_->name),
>+                 description => $self->type('string', $_->description),
>+                 }
>+             }@$components;
>+
>+        push(@result_list, {$product->name => \@component_list});
>+    }
>+    return { products => \@result_list };
>+}
>+
>+
> 1;
> 
> __END__
>@@ -194,5 +237,40 @@
> 
> =back
> 
>+=item C<get_components>
>+
>+B<EXPERIMENTAL>
>+
>+=over
>+
>+=item B<Description>
>+
>+Returns a list of information about the components that are accessible thrue the
>+products passed to it.
>+

Nit: s/thrue/through/

The rest looks much better to me.

Dave

Comment 10

[Frank Becker]

Attachment: patch, V3

next version

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 390865 [details] [diff] [review]
patch, V3

>+=item B<Params>
>+
>+A hash containing the following items C<ids>, C<names>. You can passing either product
>+ids or product names or both together.
>+

Looks good. Please add a sentence stating that ids and names need to be in a list format when checking in.

review+
Dave

Comment 12

[Max Kanat-Alexander]

Comment on attachment 390865 [details] [diff] [review]
patch, V3

>+sub get_components {


  Don't prefix method names with "get". So this should just be "components".

>+    my @requested_accessible;
>+    # Create a hash with the names the user wants
>+    my %names = map { $_ => 1 } @{$params->{names}};
>+    # Create a hash with the ids the user wants
>+    my %ids = map { $_ => 1 } @{$params->{ids}};

  You didn't validate the existence of those parameters, so if either of them are missing, this will just die.

>+
>+    # Return the intersection of this, by grepping the ids/names from
>+    # accessible products.
>+    @requested_accessible = grep {$ids{$_->id} || $names{$_->name}} @$accessible_products;

  By default, if somebody requests an inaccessible product or a product that doesn't exist, we should be throwing an error.

  In a later bug, we can add a "permissive" argument that works much like the Bug.get permissive argument.

>+    foreach my $product (@requested_accessible) {
>+        my $components = $product->components;
>+        my @component_list =

>+             map {{
>+                 internals   => $_,

  Do not pass internals. We want to full spec the API for components, not pass around unnecessary items.

>+                 id          => $self->type('int', $_->id),
>+                 name        => $self->type('string', $_->name),
>+                 description => $self->type('string', $_->description),

  You should include the default assignee, the default qa contact, and the default CC.

>+=item B<Params>
>+
>+A hash containing the following items C<ids>, C<names>. You can passing either product
>+ids or product names or both together.

  s/passing/pass/

>+=item B<Returns>

  What this returns seems very inconsistent with our other methods.

  The return value should look like this (this would be more consistent with the rest of our API):

  products => {
    product1 => {
      id => $product_id,
      components => [..list of hashes..]
    },
    product2 => {
      id => $product_id2,
      components => [..list of hashes..]
    }
  }

  Otherwise there's just no way to tell what product matches to what if you asked for things by id.


  You left out the Errors and History sections of the POD.

Comment 13

[Frank Becker]

Attachment: patch, V4

Here is the next version. Hope that I fixed all things.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 394879 [details] [diff] [review]
patch, V4

Make sure to request review from somebody specific--otherwise your review is likely to never happen.

Comment 15

[Frank Becker]

Attachment: patch, V5

some code from bug#506530 is included, so we do not have the same code twice.

Comment 16

[Frank Becker]

Attachment: patch, V6

Now I remove the id's from the components and return the flag_types and not only the id's of the flag_types.

Comment 17

[David Lawrence [:dkl]]

Comment on attachment 399555 [details] [diff] [review]
patch, V6

>Index: Bugzilla/WebService/Product.pm
>
>+=item C<components>
>+
>+B<EXPERIMENTAL>
>+
>+=over
>+
>+=item B<Description>
>+
>+Returns a list of information about the components that are accessible through the
>+products passed to it.
>+
>+=item B<Params>
>+
>+You can pass either product
>+ids or product names or both together.
>+

Nit: You can pass either product ids, product names, or both together.

>+=over
>+
>+=item C<ids>
>+
>+An array of product ids.
>+
>+=item C<names>
>+
>+An array of product ids.
>+

An array of product names.

>+=item C<permissive> B<UNSTABLE>
>+
>+C<boolean> Normally, if you request any inaccessible or invalid product,
>+Product.components will throw an error. If this parameter is True, instead of throwing an
>+error we return an array of hashes with a C<id> od C<name>, C<faultString> and C<faultCode> 

s/od/or/

>+for each product that fails, and return normal information for the other products that 
>+were accessible.
>+
>+=item C<with_flagtypes> B<UNSTABLE>
>+
>+C<boolean> Normally, if you request you did not get the information of the flagtypes.
>+If this parameter is True, information of the flagtypes are returned.

C<boolean> Normally, when you make a request you do not get information about flagtypes.

>+
>+=back 
>+
>+=item B<Returns>
>+
>+Two items are returned:
>+
>+=over
>+
>+=item B<products>
>+

=item C<products>

>+An array of hashes of the valid products.  Each hash contains the following items:
>+

This doesn't match the format that Max suggested to be returned. The format to return would be

A hash containing product names, each pointing to a hash containing the following items:

>+=over
>+
>+=item id
>+
>+C<int> The numeric product_id of this product.
>+
>+=item components
>+
>+An array of hashes with the components of the product.  Each hash contains the following items:
>+
>+=over
>+

Missing:

=item id

C<int> The unique integer ID of the component.

>+=item name
>+
>+C<string> The name of this component.
>+
>+=item description
>+
>+C<string> The description of this component.
>+
>+=item initialowner
>+
>+C<int> The unique integer ID that Bugzilla uses as the initial user.
>+
>+=item initialqacontact
>+
>+C<int> The unique integer ID that Bugzilla uses as the default qa contact.
>+
>+=item default_assignee_email
>+
>+C<string> The email of the initialowner.
>+
>+=item default_qa_email
>+
>+C<string> The email of the initialqacontact.
>+
>+=item flag_type_bug
>+
>+An array with the ids of the flagtypes of this component from type bug.
>+
>+=item flag_type_attachment
>+
>+An array with the ids of the flagtypes of this component from type attachment.
>+
>+=back
>+
>+=back
>+
>+=item C<faults> B<UNSTABLE>
>+
>+An array of hashes that contains invalid product ids / names with error messages
>+returned for them. Each hash contains the following items:
>+C<id> or C<name> and C<faultString>, C<faultCode>
>+
>+=over
>+
>+=item id
>+
>+C<int> The numeric product_id of this product.
>+
>+=item name
>+
>+C<string> The name of this product.
>+
>+
>+=item faultString 
>+
>+c<string> This will only be returned for invalid bugs if the C<permissive>
>+argument was set when calling Bug.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 Bug.get, and it is the error code for the 
>+invalid bug error.


The above text needs to be suited to match Product.components, not Bug.get.

>+=back
>+
> =back
> 
>+=back
>+
>+=item B<Errors>
>+
>+=item B<History>
>+
>+Added in Bugzilla B<3.6>.
>+
>+=back
>+
>+

Comment 18

[Frank Becker]

Attachment: patch, V7

changes requested in comment#17 are now included

Comment 19

[Frank Becker]

Attachment: patch, V8

I add the id of flagtypes in the hash.

This is needed for posting new flags using the http post method.

Comment 20

[David Lawrence [:dkl]]

Comment on attachment 403100 [details]
patch, V8


>Index: Bugzilla/WebService/Product.pm
>+
>+sub components {
>+    my ($self, $params) = validate(@_, 'ids', 'names');
>+ 
>+    # test that at least one parameter is there
>+    if (!(defined $params->{ids} || defined $params->{names})) {
>+        ThrowCodeError('params_required',
>+                       { function => 'Product.components',
>+                         params   => ['ids', 'names'] });
>+    }
>+
>+    # Only products that are in the users accessible products,
>+    # can be allowed to be returned
>+    my $accessible_products = Bugzilla->user->get_accessible_products;

As I look at this patch, I have a few more suggestions before I can personally review+ it:

1. The way you are loading product information to me is not efficient. For example in the beginning
you are loading all accessible products into $accessible_products which is a reference to a list of Product objects most of which you probably do not need since the user will normally just request a small amount.

Then in two foreach loops, you call eval {$product_loop = Bugzilla::Product->check({id => $product_id}); }
which is again loading the product from the database when you already have the data in $accessible_products.

So you can either 1) continue to use $accessible_products which is already loaded and loop through each passed in id or product name, grepping to see if $accessible_products contains the match. Or 2) do not use $accessible_products at all and just loop through the product ids and names passed in calling Bugzilla::Product->check on each on, adding the object to a list of product objects matched.

The negative of the 1st solution is that you are still loading more products than you need to so the second solution suits better I think.

One way you could do it maybe is this:

    my @ids = @{$params->{ids}};
    my @names = @{$params->{names}};

    my $obj_by_ids = Bugzilla::Product->new_from_list(\@ids) if $params->{ids};

    # to get the product objects from the passed product name
    my @product_objs = map { Bugzilla::Product->new({ name => $_ }) } @names if $params->{names};

    my %unique_products = map { $_->id => $_ } @product_objs;
    @product_objs = values %unique_products;

    foreach my $obj (@$obj_by_ids){
        push (@product_objs, $obj) if !$unique_products{$obj->id};
    }   
    
    my %product_hash;
    foreach my $prod_obj (@product_objs) {
        if ($params->{permissive}) {
            eval { Bugzilla->user->can_enter_product($prod_obj->name, THROW_ERROR); };
            if ($@) {
                push(@faults, {id => $product_id,
                               faultString => $@->faultstring,
                               faultCode => $@->faultcode,
                              }
                    );
                undef $@;
                next;
            }
        }
        else {
            Bugzilla->user->can_enter_product($prod_obj->name, THROW_ERROR);
        }

        my $components = $product->components;
        my @component_list;
        foreach my $component (@$components){
        ....


2. You have quite a bit of duplicate code in the flag attributes and component attributes section. You could create small subroutines to return for example the flag attributes instead of using the same code for both bug and attachment type flags. For example:

# internal function to get flag's information
sub _get_flag_attr {
    my $flagtype = shift;
    my $cansetflag     = Bugzilla->user->can_set_flag($flagtype);
    my $canrequestflag = Bugzilla->user->can_request_flag($flagtype);
    return { 
        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),
        is_requestable    => $self->type('int'   , $flagtype->is_requestable),
        is_requesteeble   => $self->type('int'   , $flagtype->is_requesteeble),
        is_multiplicable  => $self->type('int'   , $flagtype->is_multiplicable),
        can_set_flag      => $self->type('int'   , $cansetflag),
        can_request_flag  => $self->type('int'   , $canrequestflag),
    };
}
 
The in the Product.components() method just do:

                my $flagtypes        = $component->flag_types;
                my @flagt_bug;
                my @flagt_attachment;

                foreach my $flagtype (@{$flagtypes->{'bug'}}) {
                    push(@flagt_bug, _get_flag_attr($flag_type);
                }
                foreach my $flagtype (@{$flagtypes->{'attachment'}}) {
                    push(@flagt_bug, _get_flag_attr($flag_type);
                }

You could then do similar for components:

sub _get_component_attr {
    my ($component, $flagt_bug, $flagt_attachment) = @_;
    return {
        name                   => $self->type('string', $component->name),
        description            => $self->type('string', $component->description),
        initialowner           => $self->type('int',    $component->{'initialowner'}),
        initialqacontact       => $self->type('int',    $component->{'initialqacontact'}),
        default_assignee_email => $self->type('string', $component->default_assignee->email),
        default_qa_email       => $self->type('string', $component->default_qa_contact->email),
        flag_type_bug          => $flagt_bug,
        flag_type_attachment   => $flagt_attachment,
    };
}

Let me know if anything doesn't make sense.

Dave

Comment 21

[Frank Becker]

(In reply to comment #20)
> (From update of attachment 403100 [details])
> 
> 1. The way you are loading product information to me is not efficient. For
> example in the beginning
> you are loading all accessible products into $accessible_products which is a
> reference to a list of Product objects most of which you probably do not need
> since the user will normally just request a small amount.

I plan to use this from mylyn as an replacement of config.cgi so we want all components.
We use Product.get_accessible_products and call  Product.components with the result.
> 
> Then in two foreach loops, you call eval {$product_loop =
> Bugzilla::Product->check({id => $product_id}); }
> which is again loading the product from the database when you already have the
> data in $accessible_products.

Sorry I did not noticed that.
...

> The negative of the 1st solution is that you are still loading more products
> than you need to so the second solution suits better I think.
> 

OK I update my patch!

> 
> 
> 2. You have quite a bit of duplicate code in the flag attributes and component
> attributes section. You could create small subroutines to return for example
> the flag attributes instead of using the same code for both bug and attachment
> type flags. For example:
> 
...
> 
> Let me know if anything doesn't make sense.

OK I do this. But _get_flag_attr would be better change to only return the id and we implement Bugzilla.flagtypes to get the  other fields.

Thougths?


Frank

Comment 22

[David Lawrence [:dkl]]

(In reply to comment #21)
> I plan to use this from mylyn as an replacement of config.cgi so we want all
> components.
> We use Product.get_accessible_products and call  Product.components with the
> result.

Ah I see. But still better to just loop through the name/id list and load only the products requested even if it is all accessible products.

> OK I do this. But _get_flag_attr would be better change to only return the id
> and we implement Bugzilla.flagtypes to get the  other fields.

I would at a minimum return the id and flag type name to be more descriptive to the end user. And then if the user wants more data about the flag then they can call another xmlrpc call to get it as you suggest. I would rather though a method called Flag.get be used instead of Bugzilla.flagtypes.

Dave

Comment 23

[Frank Becker]

Attachment: patch, V9

Comment 24

[David Lawrence [:dkl]]

Comment on attachment 403881 [details] [diff] [review]
patch, V9

>Index: Bugzilla/WebService/Product.pm
>+sub components {
>+    my ($self, $params) = validate(@_, 'ids', 'names');
>+ 
>+    # test that at least one parameter is there
>+    if (!(defined $params->{ids} || defined $params->{names})) {
>+        ThrowCodeError('params_required',
>+                       { function => 'Product.components',
>+                         params   => ['ids', 'names'] });
>+    }
>+    my @ids = @{$params->{ids}} if $params->{ids};
>+
>+    my $obj_by_ids = Bugzilla::Product->new_from_list(\@ids) if $params->{ids};
>+
>+    # to get the product objects from the passed product name
>+    my @product_objs;
>+    my @faults;
>+    if ($params->{names}) {
>+        my $product_names = $params->{names};
>+        foreach my $product_name (@$product_names) {
>+            my $product_loop;
>+            if ($params->{permissive}) {
>+                eval {$product_loop = Bugzilla::Product->check({name => $product_name}); };
>+                if ($@) {
>+                    push(@faults, {name => $product_name,
>+                                   faultString => $@->faultstring,
>+                                   faultCode => $@->faultcode,
>+                                  }
>+                        );
>+                    undef $@;
>+                    next;
>+                }
>+            }
>+            else {
>+                $product_loop = Bugzilla::Product->check({name => $product_name});
>+            }
>+            push(@product_objs,$product_loop);
>+        }
>+    }

You are not permission checking any of the products. Bugzilla::Product->new_from_list() and Bugzilla::Product->check()
do not call Bugzilla::User->can_enter_product($product_obj) internally so you still need to do that yourself and wrap that in eval() when $params->{permissive} is set.

Also the $params->{permissive} is not even considered for any product ids being passed, only with product names.

Please see my code example in comment #7

>+    my $len_objs = @product_objs;
>+    my %unique_products = map { $_->id => $_ } @product_objs if $len_objs > 0;

Nit: Shorter to just write 
my %unique_products = map { $_->id => $_ } @product_objs;

>+    foreach my $obj (@$obj_by_ids){
>+        push (@product_objs, $obj) if !exists $unique_products{$obj->id};
>+    }   
>+    my %product_hash;
>+    my @faults;

You are re-declaring @faults here which will wipe out any faults added earlier in the previous @faults.

>+    foreach my $prod_obj (@product_objs) {
>+        my $components = $prod_obj->components;
...

I like the new subroutines _get_flag_attr and _get_component_attr. Those look fine so far.

Dave

Comment 25

[Max Kanat-Alexander]

can_enter isn't the issue--it's can_access--I don't know if we have that.

Comment 26

[David Lawrence [:dkl]]

(In reply to comment #25)
> can_enter isn't the issue--it's can_access--I don't know if we have that.

Sorry. There is in fact $user->can_access_product which is a intersection of $user->can_enter_product and $user->can_select_product so that would work.

Dave

Comment 27

[David Lawrence [:dkl]]

(In reply to comment #26)
> (In reply to comment #25)
> > can_enter isn't the issue--it's can_access--I don't know if we have that.
> 
> Sorry. There is in fact $user->can_access_product which is a intersection of
> $user->can_enter_product and $user->can_select_product so that would work.
> 
> Dave

Sorry, did not mean intersection. I meant the union of the two accessible methods.

Dave

Comment 28

[Frank Becker]

Attachment: patch, V10

Thinks from comment#24 are now included.
New is that the information if the user can enter bugs to a product.

Comment 29

[:glob ✱]

this has been implemented by the work on bug 655229.

Comment 30

[Frédéric Buclin]

(In reply to comment #29)
> this has been implemented by the work on bug 655229.

In that case, please mark bug 655229 as blocking this bug and put [fixed by blocker] in the status whiteboard, retarget this bug to the branch the feature has been implemented (I guess 4.2?), reassign the bug to the one who fixed bug 655229 and remove the now useless request for review. :)