647980 - Implementation of Product.update webservice

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Julien Heyman]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0
Build Identifier: 4.0

Implementation of Product.update webservice

Reproducible: Always

Comment 1

[Julien Heyman]

Attachment: Implement the Product.update webservice v1

Hi!

The first version of my patch.

This patch can be apply after the patch in https://bugzilla.mozilla.org/show_bug.cgi?id=469193

Thanks to review it when you can.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 524168 [details] [diff] [review]
Implement the Product.update webservice v1

This needs to be able to update multiple products. So it should take both an "ids" and a "names" argument, like other functions in the WebService do.

Also, don't call individual set_ functions, use set_all. (See editproducts.cgi.)

For the Errors section, you can just say:

The same as L</create>.

Comment 3

[Max Kanat-Alexander]

Oh, and thank you for the patch and thank you for working on this, by the way! :-)

Comment 4

[Julien Heyman]

Attachment: Implement the Product.update webservice v2

New version ;-)

Thanks for review.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 524634 [details] [diff] [review]
Implement the Product.update webservice v2

>=== modified file 'Bugzilla/WebService/Product.pm'
>+    my @product_objects;
>+    @product_objects = map { Bugzilla::Product->check($_) } @{ $params->{names} } if $params->{names};
>+
>+    my $products_by_ids;
>+    $products_by_ids = Bugzilla::Product->new_from_list($params->{ids}) if $params->{ids};

  You should probably be doing a map of Bugzilla::Product->check({ id => $_ }) so that we throw an error if somebody passes an invalid id.

>+    # start filtering to remove duplicate products ids
>+    my %unique_products; 
>+    map { $unique_products{$_->id} = $_ } @product_objects;
>+    @product_objects = values %unique_products;
>+    foreach my $obj (@$products_by_ids){
>+        push (@product_objects, $obj) if !$unique_products{$obj->id};
>+    }

  The same comment here that I made for User.update.

>+    foreach my $product (@product_objects) {
>+        $product->set_all(\%values);
>+        $changes{$product->id} = $product->update();

  Does that return the right field names, or will they need to be mapped?

>+=head1 Product Creation and Update

  "and Modification" instead of "and Update". (When in doubt, look at the POD for Bugzilla::WebService::Bug.)

  I can do any other POD fixes myself on checkin once this patch is ready.

Comment 6

[Julien Heyman]

Attachment: Implement the Product.update webservice v3

Hi,

it's a new version.

Please review when you can.

Thanks.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 526248 [details] [diff] [review]
Implement the Product.update webservice v3

Review of attachment 526248 [details] [diff] [review]:

Looks pretty good! I didn't really look over the POD; if there are any issues, I'll just fix them on checkin, for the POD. A few tiny notes (these aren't really a big r-, but I figure they're all things you might want to fix yourself instead of having me fix them on checkin):

::: Bugzilla/WebService/Product.pm
@@ +37,5 @@
+    has_unconfirmed => 'allows_unconfirmed',
+    is_open => 'is_active'
+};
+
+use constant MAPPED_RETURNS => {

Since this is a perfect reverse mapping, you don't need two constants, you can just reverse the hash:

  my %reverse_map = reverse %{ MAPPED_FIELDS() };

@@ +126,5 @@
+    defined($params->{names}) || defined($params->{ids})
+        || ThrowCodeError('params_required', 
+               { function => 'Product.update', params => ['ids', 'names'] });
+
+        || ThrowUserError("auth_failure", { group  => "editcomponents",

I think this line is longer than 80 characters.

@@ +128,5 @@
+               { function => 'Product.update', params => ['ids', 'names'] });
+
+    my @product_objects = map { Bugzilla::Product->check($_) } @{ $params->{names} } if $params->{names};
+
+                                            action => "edit",

This one too.

@@ +139,5 @@
+    while ( my ($key,$value) = each(%$params)) {
+        my $mapped_field = MAPPED_FIELDS->{$key} || $key;
+        $values{ $mapped_field } = $value;
+    }
+

Could use a comma here explaining the deletes.

@@ +146,5 @@
+    my %changes;
+    foreach my $product (@product_objects) {
+        $product->set_all(\%values);
+        my $returned_changes = $product->update();
+    my @products_by_ids = map { Bugzilla::Product->check({ id => $_ }) } @{ $params->{ids} } if $params->{ids};

Same note about making a subroutine for the mapping as I mentioned for User.update.

Comment 8

[Max Kanat-Alexander]

Looks like the Splinter review tool is creating comments with my review notes in the wrong place, click on the "Review" link to see where I actually put everything.

Comment 9

[Julien Heyman]

Hi. 

I'll give you a new patch the next week with fix.

Thanks.

Comment 10

[Julien Heyman]

Attachment: Implement the Product.update webservice v4

Hi, 

it's a new version.

Please review when you can.

Thanks.

Comment 11

[Julien Heyman]

(In reply to comment #7)
> Comment on attachment 526248 [details] [diff] [review] [review]
> Implement the Product.update webservice v3
> 
> Review of attachment 526248 [details] [diff] [review] [review]:
> 
> Looks pretty good! I didn't really look over the POD; if there are any
> issues, I'll just fix them on checkin, for the POD. A few tiny notes (these
> aren't really a big r-, but I figure they're all things you might want to
> fix yourself instead of having me fix them on checkin):
> 
> ::: Bugzilla/WebService/Product.pm
> @@ +37,5 @@
> +    has_unconfirmed => 'allows_unconfirmed',
> +    is_open => 'is_active'
> +};
> +
> +use constant MAPPED_RETURNS => {
> 
> Since this is a perfect reverse mapping, you don't need two constants, you
> can just reverse the hash:
> 
>   my %reverse_map = reverse %{ MAPPED_FIELDS() };
> 
> @@ +126,5 @@
> +    defined($params->{names}) || defined($params->{ids})
> +        || ThrowCodeError('params_required', 
> +               { function => 'Product.update', params => ['ids', 'names']
> });
> +
> +        || ThrowUserError("auth_failure", { group  => "editcomponents",
> 
> I think this line is longer than 80 characters.
> 
> @@ +128,5 @@
> +               { function => 'Product.update', params => ['ids', 'names']
> });
> +
> +    my @product_objects = map { Bugzilla::Product->check($_) } @{
> $params->{names} } if $params->{names};
> +
> +                                            action => "edit",
> 
> This one too.
> 
> @@ +139,5 @@
> +    while ( my ($key,$value) = each(%$params)) {
> +        my $mapped_field = MAPPED_FIELDS->{$key} || $key;
> +        $values{ $mapped_field } = $value;
> +    }
> +
> 
> Could use a comma here explaining the deletes.
> 
> @@ +146,5 @@
> +    my %changes;
> +    foreach my $product (@product_objects) {
> +        $product->set_all(\%values);
> +        my $returned_changes = $product->update();
> +    my @products_by_ids = map { Bugzilla::Product->check({ id => $_ }) } @{
> $params->{ids} } if $params->{ids};
> 
> Same note about making a subroutine for the mapping as I mentioned for
> User.update.


Hi, it's not a perfect reverse mapping.

Comment 12

[Julien Heyman]

Comment on attachment 526248 [details] [diff] [review]
Implement the Product.update webservice v3

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

::: Bugzilla/WebService/Product.pm
@@ +37,5 @@
> +    has_unconfirmed => 'allows_unconfirmed',
> +    is_open => 'is_active'
> +};
> +
> +use constant MAPPED_RETURNS => {

No, it's not a perfect reverse mapping.

Comment 13

[Max Kanat-Alexander]

Comment on attachment 532962 [details] [diff] [review]
Implement the Product.update webservice v4

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

::: Bugzilla/WebService/Product.pm
@@ +135,5 @@
> +
> +    push(@product_objects, @products_by_ids);
> +    my %seen;
> +    @product_objects = grep { !$seen{$_->id}++ } @product_objects;
> +

The code block above, we seem to be doing over and over. Could you perhaps put a function somewhere that would just do all of the above for us? (Perhaps Bugzilla::WebService::Util would be the right place.)

It would look something like:

my @objects = _params_to_objects($params, 'Bugzilla::Product');

@@ +137,5 @@
> +    my %seen;
> +    @product_objects = grep { !$seen{$_->id}++ } @product_objects;
> +
> +    my %values;
> +    %values = _translate($params,MAPPED_FIELDS);

That does not need to be on two separate lines, and _translate should be called as a method, not as a function. (So like $self->_translate.)

Also, you need a space after the comma, there.

@@ +147,5 @@
> +
> +    my %changes;
> +    foreach my $product (@product_objects) {
> +        $product->set_all(\%values);
> +        my $returned_changes = $product->update();

Update must be called in a separate loop, after set_all is called. Also, all of these parts should be wrapped in a transaction.

Generally, see my latest comments on the bug for Group.update, they all apply here as well.

@@ +148,5 @@
> +    my %changes;
> +    foreach my $product (@product_objects) {
> +        $product->set_all(\%values);
> +        my $returned_changes = $product->update();
> +        $changes{$product->id} = {_translate($returned_changes,MAPPED_RETURNS)};

%changes needs to be returned in a format like Bug.update returns it, not in this format.

@@ +406,5 @@
> +B<Required> C<array> Contain names of products to update.
> +
> +=item C<name>
> +
> +C<string> A new name for products.

It can only be set on one product, so plural isn't necessary.

Comment 14

[Julien Heyman]

Attachment: Implement the Product.update webservice v5

Hi,

it's a new version with your comments.

Please review when you can.

Thanks.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 536090 [details] [diff] [review]
Implement the Product.update webservice v5

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

Thanks for the revision! :-) Some comments:

::: Bugzilla/WebService/Product.pm
@@ +128,5 @@
> +    defined($params->{names}) || defined($params->{ids})
> +        || ThrowCodeError('params_required', 
> +               { function => 'Product.update', params => ['ids', 'names'] });
> +
> +    my $product_objects = params_to_objects($params,'Bugzilla::Product');

Nit: Space after the comma.

@@ +141,5 @@
> +        $product->set_all(\%values);
> +    }
> +
> +    my %changes;
> +    $dbh->bz_start_transaction();

It would probably be best to start the transaction before the set_all loop.

@@ +144,5 @@
> +    my %changes;
> +    $dbh->bz_start_transaction();
> +    foreach my $product (@$product_objects) {
> +        my $returned_changes = $product->update();
> +        $changes{$product->id} = {translate($returned_changes, MAPPED_RETURNS)};    

Nit: Spaces like: 

{ translate ... }

(Although that won't be necessary anyway if we change translate() to returning a hashref.)

@@ +158,5 @@
> +
> +        foreach my $field (keys %{ $changes{$product->id} }) {
> +            my $change = $changes{$product->id}->{$field};
> +            # We normalize undef to an empty string, so that the API
> +            # stays consistent for things like Deadline that can become

Deadline doesn't apply here, just remove the words "like Deadline".

@@ +162,5 @@
> +            # stays consistent for things like Deadline that can become
> +            # empty.
> +            $change->[0] = '' if !defined $change->[0];
> +            $change->[1] = '' if !defined $change->[1];
> +            $hash{changes}{$field} = {

Don't you also need to fix this so that it uses the API names for fields instead of the internal names?

@@ +164,5 @@
> +            $change->[0] = '' if !defined $change->[0];
> +            $change->[1] = '' if !defined $change->[1];
> +            $hash{changes}{$field} = {
> +                removed => $change->[0],
> +                added   => $change->[1] 

These need to be explicitly typed as strings.

::: Bugzilla/WebService/Util.pm
@@ +110,4 @@
>      return ($self, $params);
>  }
>  
> +sub translate {

Nice! :-)

@@ +111,5 @@
>  }
>  
> +sub translate {
> +    my $params = shift;
> +    my $mapped = shift;

No need to do shift twice there. Just do: my ($params, $mapped) = @_;

@@ +117,5 @@
> +    while ( my ($key,$value) = each (%$params)) {
> +        my $new_field = $mapped->{$key} || $key;
> +        $changes{$new_field} = $value;
> +    }
> +    return %changes;

How about we have translate return a hashref instead of a hash?

@@ +120,5 @@
> +    }
> +    return %changes;
> +}
> +
> +sub params_to_objects {

This is great! :-)

@@ +122,5 @@
> +}
> +
> +sub params_to_objects {
> +    my $params = shift;
> +    my $object = shift;

Same note there about shift. Also, the second parameter here should be called "$class", not "$object".

Comment 16

[Julien Heyman]

Attachment: Implement the Product.update webservice v6

Hi,

it's a new version with your comments.

Please review when you can.

Thanks.

Comment 17

[Max Kanat-Alexander]

Comment on attachment 539453 [details] [diff] [review]
Implement the Product.update webservice v6

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

This looks good, although a few nits and one question to answer before checkin. Also note that I'm going to fix up the POD on checkin myself.

::: Bugzilla/WebService/Product.pm
@@ +136,5 @@
> +    Bugzilla->login(LOGIN_REQUIRED);
> +    Bugzilla->user->in_group('editcomponents')
> +        || ThrowUserError("auth_failure", { group  => "editcomponents",
> +                                            action => "edit",
> +                                            object => "products"});

Nit: Space before } if there's a space after {.

@@ +166,5 @@
> +    my @result;
> +    foreach my $product (@$product_objects) {
> +        my %hash = (
> +            id               => $product->id,
> +            changes          => {},

Nit: Too much spacing here.

@@ +174,5 @@
> +            my $change = $changes{$product->id}->{$field};
> +            # We normalize undef to an empty string, so that the API
> +            # stays consistent for things that can become empty.
> +            #$change->[0] = '' if !defined $change->[0];
> +            #$change->[1] = '' if !defined $change->[1];

Why are you commenting-out those lines?

::: Bugzilla/WebService/Util.pm
@@ +112,5 @@
>  
> +sub translate {
> +    my ($params, $mapped) = @_;
> +    my %changes;
> +    while ( my ($key,$value) = each (%$params)) {

Nit: Either no space after the opening ( or a space before the closing ).

Comment 18

[Julien Heyman]

Comment on attachment 539453 [details] [diff] [review]
Implement the Product.update webservice v6

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

I send you a new patch

::: Bugzilla/WebService/Product.pm
@@ +174,5 @@
> +            my $change = $changes{$product->id}->{$field};
> +            # We normalize undef to an empty string, so that the API
> +            # stays consistent for things that can become empty.
> +            #$change->[0] = '' if !defined $change->[0];
> +            #$change->[1] = '' if !defined $change->[1];

It's an error. I would like try without this lines... and, I will delete it.

Comment 19

[Julien Heyman]

Attachment: Implement the Product.update webservice v7

Hi,

it's a new version with your comments.

Please review when you can.

Thanks.

Comment 20

[Max Kanat-Alexander]

Comment on attachment 539743 [details] [diff] [review]
Implement the Product.update webservice v7

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

This looks good on a basic overview. I will also test it before checkin. (The only thing I want to be sure of is that everything in "changes" translates properly, and that the return values look right.)

Comment 21

[Max Kanat-Alexander]

Okay, all checked in! Thank you so much for your work on this, Julien! :-)

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

Comment 22

[Frédéric Buclin]

Added to relnotes for 4.4.