1226028 - API for batching MozReview requests

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Extensions: MozReview Integration, defect)

Description

[Mark Côté [:mcote]]

Right now, when developers push commits to MozReview's review repo, because of restrictions in BMO's API there is an API call for each commit, which creates or updates an attachment, sets flags, and adds a comment.  This results in one comment and one email per commit.  This is annoying and not supportive of our intention to have developers split up their work.

We need an API that can perform multiple create_attachment() and update_attachment() calls with only a single comment and bugmail update.  It should also take a tag argument, or perhaps just automatically add one to the comment, e.g. "mozreview".

Comment 1

[:glob ✱]

(In reply to Mark Côté [:mcote] from comment #0)
> We need an API that can perform multiple create_attachment() and
> update_attachment() calls with only a single comment and bugmail update.  It
> should also take a tag argument, or perhaps just automatically add one to
> the comment, e.g. "mozreview".

the comment tag should be provided by mozreview, not baked into the api call.

also, i think just "review" as a comment tag makes more sense.  filed as bug 1226080 because comment tagging is possible with the current api.

Comment 2

[:glob ✱]

the best we can do without major changes to bugzilla is still generate one comment per commit, however we'd be able to fold all changes into a single email by setting them to use the same timestamp.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

If you can collapse all those comments into one compact block in the frontend, sure. I think this issue is annoying not only because of emails, but also the long boring information listed in bug page.

Comment 4

[:glob ✱]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> If you can collapse all those comments into one compact block in the
> frontend, sure. I think this issue is annoying not only because of emails,
> but also the long boring information listed in bug page.

collapsing into a single block is bug 1226488, and out of scope here.

Comment 5

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

There is a difficulty with the current implementation of this.

$bug->add_comment() does not immediately add a comment -- that happens
in $bug->update. Comment tags are set on the bug object (with $bug->set_all({comment_tags => '...'}).

This means it is currently not possible to set different tags on different attachments created in the new add_attachments() and update_attachments() methods. Working around this is more complicated that I had assumed. :(

To better explain: if you are adding 3 attachments, and you want to tag them all with 'foo, bar, baz'
that is possible, but you can't tag the 1st attachment with 'foo', the second with 'bar', and the third with 'baz'.

Is that a requirement for this to be used?

Comment 6

[Mark Côté [:mcote]]

A single tag for all comments is all we need, for now at least.

Comment 7

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

Attachment: 1226028_1.patch

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8701665 [details] [diff] [review]
1226028_1.patch

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

Mostly nits at the moment. Will finish functional testing on 2nd revision.

dkl

::: extensions/MozReview/Extension.pm
@@ +110,2 @@
>  
> +    return unless $app_id;

nit: return unless ($mozreview_app_id && $app_id); # or similar

Also seems like you could just do this before everything else.

::: extensions/MozReview/lib/WebService.pm
@@ +39,5 @@
> +    # BMO: Don't allow updating of bugs if disabled
> +    if (Bugzilla->params->{disable_bug_updates}) {
> +        ThrowErrorPage('bug/process/updates-disabled.html.tmpl',
> +            'Bug updates are currently disabled.');
> +    }

Seems you would want this for update_attachments as well.

@@ +59,5 @@
> +    my $timestamp = $dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)');
> +
> +    my $flags        = delete $params->{flags};
> +    my $comment_tags = delete $params->{comment_tags};
> +    my $attachments  = delete $params->{attachments};

nit: These 3 lines are not necessary as you are not passing $params directly to set_all

@@ +98,5 @@
> +    my %attachments = map { $_->id => $self->_attachment_to_hash($_, $params) }
> +                          @created;
> +
> +    return { attachments => \%attachments };
> +}

add empty line

@@ +115,5 @@
> +        my $attachment = Bugzilla::Attachment->new($id)
> +          or ThrowUserError("invalid_attach_id", { attach_id => $id });
> +
> +        my $bug = $attachment->bug;
> +        $attachment->_check_bug;

Nit: can be shortened to 

my $bug = $attachment->_check_bug;

@@ +118,5 @@
> +        my $bug = $attachment->bug;
> +        $attachment->_check_bug;
> +
> +        push @attachments, $attachment;
> +        $bugs{$bug->id} = $bug;

Move these to the bottom of the foreach loop for consistency

@@ +131,5 @@
> +            ThrowUserError('auth_failure',
> +                           { group  => Bugzilla->params->{comment_taggers_group},
> +                             action => 'update',
> +                             object => 'comment_tags' })
> +              unless $user->can_tag_comments;

I find it odd that we don't need to do this also for add_attachments?

@@ +173,5 @@
> +    foreach my $attachment (@attachments) {
> +        my $changes = $attachment->update();
> +        my $comment_text = $comments{$attachment->id};
> +
> +        if ($comment_text = trim($comment_text)) {

nit:

if (my $comment_text = trim($comments{$attachment->id})) {

@@ +218,5 @@
> +
> +sub rest_resources {
> +    return [
> +        qr{^/mozreview/attachments$}, {
> +            POST => {

POST should be for add_attachments and PUT for update_attachments

@@ +222,5 @@
> +            POST => {
> +                method => 'update_attachments',
> +                params => sub {
> +                    return { };
> +                },

params => can be omitted when returning nothing.

@@ +226,5 @@
> +                },
> +            },
> +        },
> +        qr{^/mozreview/(\d+)/attachments$}, {
> +            PUT => {

s/PUT/POST/

Comment 9

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

Attachment: 1226028_2.patch

All changes should be in this one. Let me know if the it applies cleanly with git apply.

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 8704352 [details] [diff] [review]
1226028_2.patch

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

Just a few things to fix, otherwise works fine. Quick r+ on the next go round.

::: extensions/MozReview/Extension.pm
@@ +108,1 @@
>  

I feel like we need to block MozReview.* specific calls if there is no mozreview_app_id or app_id set.

if ($is_mozreview_method
    && (!$mozreview_app_id || !$app_id || $mozreview_app_id ne $app_id))
{
    ThrowUserError('forbidden_method', { method => $full_method });
}

::: extensions/MozReview/lib/WebService.pm
@@ +29,5 @@
> +    add_attachments
> +    update_attachments
> +);
> +
> +BEGIN { *_attachment_to_hash = \&Bugzilla::WebService::Bug::_attachment_to_hash }

Also need _flag_to_hash

BEGIN {
    *_attachment_to_hash = \&Bugzilla::WebService::Bug::_attachment_to_hash;
    *_flag_to_hash       = \&Bugzilla::WebService::Bug::_flag_to_hash;
}

@@ +124,5 @@
> +    my (%bugs, @attachments, %comments);
> +
> +    # set flags and params.
> +    foreach my $attachment_update (@{$attachment_updates}) {
> +        my $id = delete $attachment_update->{attachment_id};

$id || ThrowCodeError('param_required', { param => 'attachment_id' });

Otherwise you get an internal error about Bugzilla::Attachment::new

@@ +146,5 @@
> +            $bug->set_all({ comment_tags => $params->{comment_tags} });
> +        }
> +
> +        my ($update_flags, $new_flags) = $flags
> +            ? extract_flags($flags, $attachment->bug, $attachment)

nit: s/$attachment->bug/$bug/

Comment 11

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

Attachment: 1226028_3.patch

Ready for review, finally! Single update or modify method.

Comment 12

[David Lawrence [:dkl]]

Comment on attachment 8710738 [details] [diff] [review]
1226028_3.patch

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

Otherwise testing looked good. All can be fixed on commit as far as I am concerned. r=dkl

::: extensions/MozReview/lib/WebService.pm
@@ +47,5 @@
> +      unless defined $params->{attachments};
> +
> +    my $bug = Bugzilla::Bug->check($params->{bug_id});
> +
> +    ThrowUserError("product_edit_denied", {product => $bug->product})

nit: space after { and before }.

@@ +75,5 @@
> +        my $comment       = delete $attachment->{comment};
> +        my $attachment_obj;
> +
> +        if ($attachment_id) {
> +            $attachment_obj = Bugzilla::Attachment->check({id => $attachment_id});

nit: space after { and before }.

@@ +82,5 @@
> +
> +            $attachment = translate($attachment, Bugzilla::WebService::Bug::ATTACHMENT_MAPPED_SETTERS);
> +
> +            my ($update_flags, $new_flags) = $flags
> +              ? extract_flags($flags, $attachment_obj->bug, $attachment)

extract_flags($flags, $attachment_obj->bug, $attachment_obj)

@@ +109,5 @@
> +            }
> +            else {
> +                ThrowUserError('illegal_attachment_edit', { attach_id => $attachment_obj->id });
> +            }
> +            

whitepsace

@@ +163,5 @@
> +                                extra_data => $attachment_obj->id });
> +
> +        }
> +    }
> +    

more whitespace

@@ +170,5 @@
> +    $dbh->bz_commit_transaction();
> +    $bug->send_changes();
> +
> +    my %attachments_created = map { $_->id => $self->_attachment_to_hash($_, $params) } @created;
> +    my %attachments_modified = map { $_->{id} => $_ } @modified;

my %attachments_modified = map { $_->{id}->value => $_ } @modified;

otherwise you get:

'attachments_modified' => {
                                    'XMLRPC::Data=HASH(0x70ede20)' => {
                                                                      'changes' => {},
                                                                      'id' => '8591314',
                                                                      'last_change_time' => '20160122T17:43:24'
                                                                    }
                                  }

Comment 13

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

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   203cb22..1912ffe  master -> master