655229 - Add components, versions and milestones to Product.get

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[:glob ✱]

Attachment: patch v1

The Product.get webservice call should return components, versions and milestones.

The attached patch does that, as well as adding support for passing in a product name as well as id, and making it honour include_fields and exclude_fields.

The new fields are returned by default, but can be excluded if required.

Comment 1

[David Lawrence [:dkl]]

Comment on attachment 530591 [details] [diff] [review]
patch v1

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

::: Bugzilla/WebService/Product.pm
@@ +84,5 @@
>      # Now create a result entry for each.
>      my @products = 
> +        map {
> +            my $field_data = {
> +                internals   => $_,

I would like to see internals go away and just return all information as normal key/values similar to what was done with Bug.get.

@@ +127,1 @@
>          } @requested_accessible;

Lets move this all out to a _get_product_attributes, _get_component_attributes, _get_version_attributes and _get_milestone_attributes.

@@ +234,3 @@
>  
>  A hash containing one item, C<products>, that is an array of
>  hashes. Each hash describes a product, and has the following items:

Need to add to the POD doc information about the include_fields, exclude_fields, etc. that can now be accepted.

From the Bugzilla/WebService/Bug.pm POD:

In addition to the parameters below, this method also accepts the
standard L<include_fields|Bugzilla::WebService/include_fields> and
L<exclude_fields|Bugzilla::WebService/exclude_fields> arguments.

Comment 2

[Frédéric Buclin]

Looks like a dupe of bug 385282 or bug 424921.

Comment 3

[Max Kanat-Alexander]

I might like to see Product.components implemented first, I think. Any thoughts on the value of having separate functions vs one?

Comment 4

[:glob ✱]

(In reply to comment #1)
> I would like to see internals go away and just return all information as
> normal key/values similar to what was done with Bug.get.

i agree, do you know why the internals information was exposed in the first place?

(In reply to comment #2)
> Looks like a dupe of bug 385282 or bug 424921.

it's different from bug 385282 as you can't use a component as a starting point.  it's a super-set of bug 424921, so once this bug has been fixed, that bug can be closed too.

(In reply to comment #3)
> I might like to see Product.components implemented first, I think. Any
> thoughts on the value of having separate functions vs one?

i wanted to avoid having to make multiple xhr calls to retrieve all information for a product (especially given how expensive each call is in some environments).

the use case i have is allowing someone to select a product, and using xhr to retrieve all the information required to fill an enter_bug form (components, versions and milestones).  there's a significant time difference between a single call and four on my dev env.

Comment 5

[:glob ✱]

(In reply to comment #1)
> I would like to see internals go away and just return all information as
> normal key/values similar to what was done with Bug.get.

max indicated this would be better to do in a separate bug, raised as bug 655652.

Comment 6

[:glob ✱]

Attachment: patch v2

Comment 7

[Max Kanat-Alexander]

(In reply to comment #4)
> i wanted to avoid having to make multiple xhr calls to retrieve all
> information for a product (especially given how expensive each call is in
> some environments).

  FWIw, we can solve this in general by implementing system.multicall for XML-RPC and batch mode for JSON-RPC. (This should be pretty easy if we switch to RPC::Any.)

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 530987 [details] [diff] [review]
patch v2

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

Looks good and works as expected. Please add History paragraph in the POD detailing the additions. r=dkl

Comment 9

[Max Kanat-Alexander]

Comment on attachment 530987 [details] [diff] [review]
patch v2

  The code looks great, but on API review there are a few things that need to be fixed before I can approve this:

>=== modified file 'Bugzilla/WebService/Product.pm' (properties changed: -x to +x)
>+        my %names = map { lc $_ => 1 } @{$params->{names}};

  Nit: Precedence is unclear there, it should probably be lc($_) for clarity's sake.

>+    my @products =
>+        map {
>+            $self->_get_product_attributes($params, $_)
>         } @requested_accessible;

  Nit: To be consistent with other places, the formatting here should be something like:

  my @products = map { $self->_product_to_hash($params, $_) }
                     @requested_accessible;

>+sub _get_product_attributes {

  And as noted above, for consistency with Bugzilla::WebService::Bug, this should be named _product_to_hash.

>+sub _get_component_attributes {
>+    my ($self, $component) = @_;
>+    return {
>+        name =>
>+            $self->type('string', $component->name),

  "id" is also needed. (name can change, id cannot--clients may need to uniquely identify objects.)

>+        default_assignee_email =>
>+            $self->type('string' , $component->default_assignee->email),

  Should be default_assigned_to (consistency with Bugzilla::Bug).

>+        default_qa_email =>
>+            $self->type('string' , $component->default_qa_contact->email),

  Should be default_qa_contact.

>+sub _get_version_attributes {
>+    my ($self, $version) = @_;
>+    return {

  Missing "id".

>+sub _get_milestone_attributes {
>+    my ($self, $milestone) = @_;
>+    return {

  Needs id.

>+        name =>
>+            $self->type('string', $milestone->name),
>+        sort_key =>
>+            $self->type('int', $milestone->sortkey),

  This is currently "sortkey" in Bug.fields. However, I prefer it with the underscore. Could you patch Bug.fields to return it as sort_key (and also alias sortkey to that with a note that it will go away in a future version?).

>+=item C<components>
>+
>+C<array> An array of hashes, where each hash describes a component, and has the
>+following items: C<name>, C<description>, C<default_qa_email>, C<default_assignee_email>,
>+and C<id_active>.

  Typo: is_active.

  Also, this needs a sub-item that describes what each of those means and what their type is. (The others don't, it should be clear enough.)

>+=item C<versions>
>+
>+C<array> An array of hashes, where each hash describes a version, and has the
>+following items: C<name> and C<id_active>.

  Typo: is_active.

>+=item C<milestones>
>+
>+C<array> An array of hashes, where each hash describes a milestone, and has the
>+following items: C<name>, C<sort_key> and C<id_active>.

  Typo: is_active.

>+=item C<internals>
>+
>+An internal representation of the product.

  This needs to say UNSTABLE right below =item C<internals>.

Comment 10

[:glob ✱]

(In reply to comment #9)
> >+        sort_key =>
> >+            $self->type('int', $milestone->sortkey),
> 
>   This is currently "sortkey" in Bug.fields. However, I prefer it with the
> underscore. Could you patch Bug.fields to return it as sort_key (and also
> alias sortkey to that with a note that it will go away in a future version?).

i don't see how we can alias returned fields; perhaps it's simpler to change sort_key to sortkey?

i also see Bug.fields is returning sortkey for versions and components (but always 0); should i do the same here?

Comment 11

[:glob ✱]

(In reply to comment #9)
> >+        default_assignee_email =>
> >+            $self->type('string' , $component->default_assignee->email),
> >+        default_qa_email =>
> >+            $self->type('string' , $component->default_qa_contact->email),

i just realised these should be returning ->login not ->email, i'll fix that too.

Comment 12

[Max Kanat-Alexander]

(In reply to comment #10)
> i don't see how we can alias returned fields; perhaps it's simpler to change
> sort_key to sortkey?

  You just send two versions of the fields. We're doing this in a few places already.

> i also see Bug.fields is returning sortkey for versions and components (but
> always 0); should i do the same here?

  Hmmmm, good question. Yeah, consistency is best.

Comment 13

[:glob ✱]

Attachment: patch v3

Comment 14

[Max Kanat-Alexander]

Comment on attachment 531863 [details] [diff] [review]
patch v3

>+    if (defined $params->{names}) {
>+        # Create a hash with the names the user wants
>+        my %names = map { lc($_) => 1 } @{$params->{names}};
>+        
>+        # Return the intersection of this, by grepping the names from 
>+        # accessible products.
>+        push(@requested_accessible,
>+            grep { $names{lc $_->name} } @$accessible_products);
>+    }

  I just realized there's one other problem here--if you specify both an id and a name for the same product, you'll get back duplicates, and shouldn't. (All the other functions attempt to avoid that case, I believe.)

>+        sort_key =>
>+            0,

  Add a comment explaining why you do this, for the product.

>+=item C<name>
>+
>+C<string> The internal name of the product.  This is a unique identifier for
> the product.

  It's actually not internal (this note applies to all the C<name> items).

>+=item C<default_assigned_to>
>+
>+C<string> The login name of the user to whom new bugs will be assigned to.

  "to whom new bugs will be assigned by default."

>+=item C<default_qa_contact>
>+
>+C<string> The login name of the user who will be set as the QA Contact for
>+new bugs.

  "by default."

>+=item C<sort_key>
>+
>+C<int> Values, when displayed in a list, are sorted first by this integer
>+and then secondly by their name.

  s/Values/Components/

>+=item C<is_active>
>+
>+C<boolean> A boolean indicating if the component is active.

  Explain what active means.


  Since we're returning sort_key for products, you should probably document that, too.

Comment 15

[:glob ✱]

Attachment: patch v4

this revision addresses most of your review points, except:

> Since we're returning sort_key for products

we're not returning sort_key for products, here or on Bug.fields.

Comment 16

[Max Kanat-Alexander]

Comment on attachment 532567 [details] [diff] [review]
patch v4

Looks good, except also mention that you added C<names> as an input parameter, in the History (you can do this on checkin).

Comment 17

[:glob ✱]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Product.pm
Committed revision 7817.