1052202 - Web Service module to update and delete components

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement, P3)

Description

[Simon Green]

Following on from bug 419568 ( Web Service module to create a component), we should allow a user to update and delete components via API calls too.

Comment 1

[Simon Green]

Should have a patch for this on Monday or Tuesday. I've finished writing the code, just need to write the POD.

Comment 2

[Simon Green]

Attachment: bug1052202-v1.patch

Comment 3

[David Lawrence [:dkl]]

Comment on attachment 8478231 [details] [diff] [review]
bug1052202-v1.patch

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

I feel that Component.delete should not just go ahead and delete bugs associated with a component is 'allowbugdeletion' param is enabled.
We should first throw an error in that case and then accept a 'delete_bugs' => 1 param or similar to confirm that we want the bugs
to be deleted. This is how editcomponent.cgi behaves when 'allowbugdeletion' is on.

Also noticed that we do not return the default cc list for components of Product.get. Will file a different bug for that.

::: Bugzilla/WebService/Component.pm
@@ +26,5 @@
>      is_open            => 'isactive',
>  };
>  
> +use constant MAPPED_FIELDS => {
> +    is_open            => 'isactive',

nit: remove extra whitespace

Also needs to be is_active as update fails with the error:

"The requested method 'Bugzilla::Component::set_isactive' was not found."

@@ +32,5 @@
> +
> +use constant MAPPED_RETURNS => {
> +    initialowner     => 'default_assignee',
> +    initialqacontact => 'default_qa_contact',
> +    initial_cc       => 'default_cc',

s/initial_cc/cc_list/

@@ +76,5 @@
> +        foreach my $name_hash (@{$params->{names}}) {
> +            my $product = Bugzilla::Product->check({ name => $name_hash->{product} });
> +            $user->can_access_product($product)
> +                || ThrowUserError('product_access_denied',
> +                                  { name => $name_hash->{product} });

Just replace the above with $user->check_can_admin_product instead. The second call below should not matter much as the information is cached.

@@ +95,5 @@
> +
> +        # Can the user see and admin this product?
> +        my $product = $component->product;
> +        $user->can_access_product($product)
> +          || ThrowUserError('product_access_denied', { id => $product->id });

$user->check_can_admin_product below calls internally $user->can_see_product which should be sufficient. We do the same simple check in editcomponents.cgi. So I don't think the can_access_product is needed here.

@@ +106,5 @@
> +}
> +
> +sub update {
> +    my ($self, $params) = @_;
> +

nit: remove extra new line

@@ +137,5 @@
> +            }
> +        }
> +    }
> +
> +

nit: remove extra new line

@@ +213,5 @@
> +                                            object => "components" });
> +
> +    defined($params->{names}) || defined($params->{ids})
> +        || ThrowCodeError('params_required',
> +               { function => 'Component.update', params => ['ids', 'names'] });

s/Component.update/Component.delete/

@@ +217,5 @@
> +               { function => 'Component.update', params => ['ids', 'names'] });
> +
> +    my $component_objects = _component_params_to_objects($params);
> +
> +    $dbh->bz_start_transaction();

No need for bz_start_transaction and bz_commit_transaction around this as remove_from_db has it's own and we are not doing any other db writes outside of remove_from_db.

@@ +227,5 @@
> +
> +    my @result;
> +    foreach my $component (@$component_objects) {
> +        my %hash = (
> +            id      => $component->id,

nit: remove extra whitespace

Comment 4

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #3)
> Comment on attachment 8478231 [details] [diff] [review]
> bug1052202-v1.patch
> 
> Review of attachment 8478231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I feel that Component.delete should not just go ahead and delete bugs
> associated with a component is 'allowbugdeletion' param is enabled.
> We should first throw an error in that case and then accept a 'delete_bugs'
> => 1 param or similar to confirm that we want the bugs
> to be deleted. This is how editcomponent.cgi behaves when 'allowbugdeletion'
> is on.

This is ineffective. It just means that when you call Component.delete, you pass delete_bugs the first time. I would hope that users that have the editcomponent privilege for a product and use an API call have some sort of clue stick. You could return a token on the first call, and then provide it in the second call, but is that really going to stop stupidity when API calls generally are scripted rather than UI driven?

> ::: Bugzilla/WebService/Component.pm
> @@ +217,5 @@
> > +               { function => 'Component.update', params => ['ids', 'names'] });
> > +
> > +    my $component_objects = _component_params_to_objects($params);
> > +
> > +    $dbh->bz_start_transaction();
> 
> No need for bz_start_transaction and bz_commit_transaction around this as
> remove_from_db has it's own and we are not doing any other db writes outside
> of remove_from_db.

I disagree. If one removal succeeds, but one doesn't, you'll only be doing half the job before throwing an error. This is likely to happen if allowbugdeletion is off, the first component is empty, but the second one isn't.

Comment 5

[Simon Green]

Attachment: bug1052202-v2.patch

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8494274 [details] [diff] [review]
bug1052202-v2.patch

Sorry for the delay in reviewing this. I am moving some of my older reviews to a different reviewer who may be able to look at this sooner.

dkl

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Per dkl's comment #3 -- I don't think adding confirmation to the API makes sense. I'll probably r+ this as-is, but I may supply a doc patch later that requests consumers of this API to perform their own confirmation (if such a thing makes sense.)

Comment 8

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8494274 [details] [diff] [review]
bug1052202-v2.patch

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

r=dylan

Comment 9

[Simon Green]

To ssh://[e-mail]/bugzilla/bugzilla.git
   9d1dc1f..5db53c9  master -> master