777047 - WebService module to get, update, create and remove a product version

Status: NEW

(Bugzilla :: WebService, enhancement)

Description

[David Lawrence [:dkl]]

Create new WebService module allowing to manipulate versions for a product via the WebService API.

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to add Version.pm for WebServices (v1)

Comment 2

[Koosha KM]

David, are you sure you have targeted your reviewed request correctly?!

Comment 3

[Koosha KM]

s/reviewed/review/

Comment 4

[David Lawrence [:dkl]]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #2)
> David, are you sure you have targeted your reviewed request correctly?!

I figured you were working on similar code recently and this would be something you would be appropriate to review. I can also add LpSolit to the review list and see who has the most time to look at it.

dkl

Comment 5

[Koosha KM]

Comment on attachment 645794 [details] [diff] [review]
Patch to add Version.pm for WebServices (v1)

>=== added file 'Bugzilla/WebService/Version.pm'
>--- Bugzilla/WebService/Version.pm	1970-01-01 00:00:00 +0000
>+++ Bugzilla/WebService/Version.pm	2012-07-25 16:28:30 +0000


>+sub get {
>+    my ($self, $params) = validate(@_, 'names', 'ids');
>+
>+    Bugzilla->switch_to_shadow_db();

Bugzilla->switch_to_shadow_db() should be moved to _get_version_objects().

>+sub create {
>+    my ($self, $params) = validate(@_);
>+
>+    my $user = Bugzilla->login(LOGIN_REQUIRED);
>+    
>+    $user->in_group('editcomponents') 
>+        || scalar @{ $user->get_products_by_permission('editcomponents') } 
>+        || ThrowUserError('auth_failure', { group  => 'editcomponents',
>+                                            action => 'edit',
>+                                            object => 'versions' });
>+
>+    $params->{product} 
>+        || ThrowCodeError('param_required', { function => 'Version.create',
>+                                              param    => 'product' });
>+
>+    my $product_obj = $user->check_can_admin_product($params->{product});
>+    
>+    my $version = Bugzilla::Version->create({
>+        value   => $params->{name}, 
>+        product => $product_obj, 
>+    });
>+

I know the current code will throw an error if $params->{name} is not specified, but I think it would be nicer to throw it earlier to be consistent with other 'param_required' errors as well as save some little time before further processing (in case of the lack of $param->{name}).



>+sub update {
>+    my ($self, $params) = validate(@_, 'ids', 'names');
>+
.
.
.
>+    my @return_versions;
>+    foreach my $version (@versions) { 
>+        $version->set_all($params->{updates});

Throw error if $params->{updates} is not passed.


>+        my $return_data = $self->_version_to_hash($params, $version);

I think returning unnecessary info in update() is useless. At least, it makes some inconsistencies.

>+sub remove {
.
.
.
>+
>+    foreach my $version (@versions) {
>+        $version->remove_from_db;
>+    }
>+

I suggest writing this loop this way:

Bugzilla->dbh->bz_start_transaction();
   foreach my $version (@versions) {
       $version->remove_from_db;
    }
Bugzilla->dbh->bz_commit_transaction();

so that if some of the versions specified couldn't get deleted for some reasons, like when 'version_has_bugs' error is triggered, we avoid partial deletions.


>+sub _version_to_hash {
>+    my ($self, $params, $version) = @_;
>+    return filter($params, {
>+        id           => $self->type('int', $version->id),
>+        name         => $self->type('string', $version->name),
>+        product_id   => $self->type('int', $version->product->id),
>+        product_name => $self->type('string', $version->product->name), 
>+        is_active    => $self->type('boolean', $version->is_active), 
>+        sort_key     => $self->type('int', 0), 

Why sort_key is always zero? If it doesn't exist why do you return it? And, if it does exist why don't you return its actual value? I cannot find it in Version.pm.


>+sub _get_version_objects {
>+    my ($function, $params) = @_;
>+
>+    my %version_objects;
>+    if (defined $params->{ids}) {
>+        foreach my $id (@{ $params->{ids} }) {
>+            # Return early if the supplied id is not an integer
>+            ThrowCodeError('param_invalid', 
>+                    { function => $function, 
>+                      param    => 'ids' }) unless $id =~ /^\d+$/;
>+             
>+            $version_objects{$id} ||= Bugzilla::Version->new($id);
>+
>+            ThrowUserError('version_unknown_id', { vers_id => $id })
>+                unless $version_objects{$id};
>+        }

There is no 'version_unknown_id' error. Did you define it and forget to include it in your patch?!


>+    }
>+    elsif (defined $params->{names}) {

s/elsif/if/

>+    else {

s/else/if/ accordingly. if (!defined $pararms->{ids} and !defined $params->{names})

>+        ThrowCodeError('params_required', { function => $function,
>+                                            params => ['ids', 'names'] });
>+    }
>+    
>+    return values %version_objects;

Don't you think that returning an array ref would be more efficient that copying all values, especially when elements are objects and not simple scalars.

>+=head2 Get Product Versions

Inconsistent with POD of Product module!


>+=item C<names> (array) or (hash) - An array of hashes or a single hash, each hash 
>+has product name and version name.

single hash doesn't work: 

JSON::RPC::ReturnObject=HASH(0xab77334)Not an ARRAY reference at Bugzilla/WebService/Version.pm line 146.

Also "An array of hashes or a single hash" is a little ambiguous. I know you mean a _single_ hash, but it could be interpreted as an array of a single hash. Probably it would be better: A single hash or an array of hashes.

In addition: "has product name and version name" -> mention the key names.


>+The return value will be a hash containing one item, C<versions>, 
>+that is an array of hashes. Each hash describes a version,
>+and has the following items:
>+
>+=over
>+
>+=item id

Inconsistency with Product module. s/id/C<id>/


>+C<int> The unique integer ID that Bugzilla uses to identify this version. 
>+Even if the name of the version changes, this ID will stay the same.
>+
>+=item name

Same here.

>+
>+C<string> The name of the version.
>+
>+=item product_id

Same here.

>+
>+C<int> The id of the product that the version belongs to.
>+
>+=item product_name

Same here. And so on...


>+=head2 Create Product Version

Inconsistent with Product.



>+C<boolean> A true or false value signifying whether removals were successful.

1 if successful. Error on failure.

Comment 6

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #5)

> Bugzilla->switch_to_shadow_db() should be moved to _get_version_objects().

No, dkl is right. get() is a read-only method and so can take its data from the slave DB. But when editing the DB, you must be on the master DB, and _get_version_objects() is called in this case, in which case it must not shift to the slave DB (they could be out of sync).

Comment 7

[Koosha KM]

(In reply to Frédéric Buclin from comment #6)
> (In reply to Koosha Khajeh Moogahi [:koosha] from comment #5)
> 
> > Bugzilla->switch_to_shadow_db() should be moved to _get_version_objects().
> 
> No, dkl is right. get() is a read-only method and so can take its data from
> the slave DB. But when editing the DB, you must be on the master DB, and
> _get_version_objects() is called in this case, in which case it must not
> shift to the slave DB (they could be out of sync).

OK. I thought there will be a switch back when the enclosing function returns (i.e. when _get_version_objects() returns).

Comment 8

[David Lawrence [:dkl]]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #7)
> OK. I thought there will be a switch back when the enclosing function
> returns (i.e. when _get_version_objects() returns).

Unfortunately not. You have to manually switch back. which is why I proposed only using it in get().

dkl

Comment 9

[Koosha KM]

139 $version_objects{$id} ||= Bugzilla::Version->new($id);
140 	
141 ThrowUserError('version_unknown_id', { vers_id => $id })
142 	unless $version_objects{$id};

It would be better if we could call Bugzilla::Version->check({ id => $id }) instead of calling new(), checking the validity of the returned object, and return a newly-defined error in case of a mismatch.

Thus, marking this bug dependent on bug 779799.

Comment 10

[David Lawrence [:dkl]]

Attachment: Patch to add Version.pm for WebServices (v2)

New patch with fixes.

dkl

Comment 12

[Koosha KM]

Comment on attachment 685057 [details] [diff] [review]
Patch to add Version.pm for WebServices (v2)

LpSolit: Could you please review this as well? :-)

Comment 13

[Frédéric Buclin]

Comment on attachment 685057 [details] [diff] [review]
Patch to add Version.pm for WebServices (v2)

r- from me for the same reasons as in bug 777499 and bug 419568. For maintainability, it's important that all similar WS methods are written the same way (i.e. all create() methods should be written using the same model, ditto for all update() methods, etc...). Else it's going to be a pain if everyone does something different in each WS module.

Comment 14

[Frédéric Buclin]

Too late for 4.4. We already released 4.4rc1, and 4.4rc2 is just around the corner. We don't have a plan for a 3rd release candidate, so this bug is postponed to 5.0.

Comment 15

[Fabio Porcedda]

I'm also interested to be able to add a version to a product trough the REST API.

There is any chance to have this feature in a bugzilla release?