914986 - Create Bug.update_attachment to update attachments via RPC/REST

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Simon Green]

Attachment: v1 patch

This patch adds Bug.update_attachment RPC call to update attachment information (content type, summary, filename, is patch, is private and is obsolete) via RPC. These are the same things that can be updated from the UI.

Flag updates will be handled by bug 756048

  -- simon

Comment 1

[Simon Green]

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

The change in Bugzilla/Attachment.pm mirrors what we do with Bugzilla::Bug->update (and fixes the indentation)

Comment 2

[David Lawrence [:dkl]]

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

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

Comments: 

1. Would be a plus to also be able to add a comment at the same time but I realize there is Bug.add_comment which could be used right after although the timestamps would be different.
2. If you decide to use Bugzilla::Object::set_all you will need to add the following to update_attachment() or it will error in REST. We can address a better behavior for this in a separate bug.

    # These interfere with Bugzilla::Object::set_all so remove
    delete $params->{login};
    delete $params->{password};
    delete $params->{token};

::: Bugzilla/WebService/Bug.pm
@@ +744,5 @@
> +    my $ids = delete $params->{ids};
> +    defined $ids || ThrowCodeError('param_required', { param => 'ids' });
> +
> +    # We can't update flags, and summary is really description
> +    delete $params->{flags};

We will be able to do so when bug 756048 lands but not yet.

@@ +747,5 @@
> +    # We can't update flags, and summary is really description
> +    delete $params->{flags};
> +    if (exists $params->{summary}) {
> +        $params->{description} = delete $params->{summary};
> +    }

nit: newline

@@ +770,5 @@
> +    }
> +
> +    # Do the actual update and get information to return to user
> +    my @result;
> +    $dbh->bz_start_transaction();

nit:

   $dbh->bz_start_transaction();

   # Do the actual update and get information to return to user
   my @result;

@@ +790,5 @@
> +            # stays consistent for things like Deadline that can become
> +            # empty.
> +            $hash{changes}->{$field} = {
> +                removed => $self->type('string', $change->[0] // ''),
> +                added   => $self->type('string', $change->[1] // '') 

whitespace

@@ +3032,5 @@
> +=item B<REST>
> +
> +To update attachment metadata on a current attachment:
> +
> +POST /bug/attachment/<attach_id>/update

PUT /bug/attachment/<attach_id>

@@ +3034,5 @@
> +To update attachment metadata on a current attachment:
> +
> +POST /bug/attachment/<attach_id>/update
> +
> +The params to include in the POST body, as well as the returned

s/POST/PUT/

@@ +3044,5 @@
> +=over
> +
> +=item C<ids>
> +
> +B<Required> C<array> An array of ints -- the ids of the attachments you want

nit: s/ints/integers/

@@ +3055,5 @@
> +
> +=item C<summary>
> +
> +B<Required> C<string> A short string describing the
> +attachment.

nit: You should clarify that this is not a required parameter for the update_attachment() request, but that the summary cannot be empty. May be confusing.

@@ +3117,5 @@
> +=back
> +
> +Here's an example of what a return value might look like:
> +
> + { 

whitespace

::: Bugzilla/WebService/Server/REST/Resources/Bug.pm
@@ +97,5 @@
>              }
>          },
> +        qr{^/bug/attachment/([^/]+)/update$}, {
> +            POST => {
> +                method => 'update_attachments',

update_attachment

@@ +99,5 @@
> +        qr{^/bug/attachment/([^/]+)/update$}, {
> +            POST => {
> +                method => 'update_attachments',
> +                params => sub {
> +                    return { attachment_ids => [ $_[0] ] };

return  { ids => $_[0] };

@@ +102,5 @@
> +                params => sub {
> +                    return { attachment_ids => [ $_[0] ] };
> +                }
> +            }
> +        },

This would be PUT operation, not a POST and also you would not need 'update' in the path. 

So you would just add it to the current attachment resource:

        qr{^/bug/attachment/([^/]+)$}, {
            GET => {
                method => 'attachments',
                params => sub {
                    return { attachment_ids => [ $_[0] ] };
                }
            },
            PUT => {
                method => 'update_attachment',
		    params => sub {
		    return { attachment_ids => [ $_[0] ] };
		}
            }
        },

Comment 3

[Simon Green]

Attachment: v2 patch

Comment 4

[David Lawrence [:dkl]]

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

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

Fix the REST resource on checkin. After that all works well. r=dkl

::: Bugzilla/WebService/Server/REST/Resources/Bug.pm
@@ +97,5 @@
> +            },
> +            PUT => {
> +                method => 'update_attachment',
> +                params => sub {
> +                    return { attachment_ids => [ $_[0] ] };

return { ids => $_[0] };

Did you test REST? :)

Comment 5

[Simon Green]

modified Bugzilla/Attachment.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/WebService/Server/REST/Resources/Bug.pm
Committed revision 8735.

Comment 6

[Frédéric Buclin]

Added to relnotes for 5.0rc1.

Comment 7

[Frédéric Buclin]

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

>=== modified file 'Bugzilla/WebService/Bug.pm'

>+=item B<Returns>
>+
>+A C<hash> with a single field, "attachment".

s/attachment/attachments/ (plural)!

To ssh://[e-mail]/bugzilla/bugzilla.git
   ad6cd7c..4a900ca  master -> master

To ssh://[e-mail]/bugzilla/bugzilla.git
   b67de8c..154e70f  5.0 -> 5.0