993223 - Notify Review Board when a bug is made confidential

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, enhancement, P1)

Description

[Mark Côté [:mcote]]

As described in bug 993222, we need to delete the associated review from Review Board if a bug is made confidential.  We need a Bugzilla extension to notify Review Board (probably through a custom web API in the rbbz Review Board extension) when a bug, or a ReviewBoard-URL attachment, is flipped to nonpublic.

Note that the API will probably return the diff, so that it isn't lost, which should be attached to the bug.

Comment 1

[Mark Côté [:mcote]]

Update: we *won't* save the current review diff when we delete the review from Review Board, at least not for now.  So this extension is now very simple: when a bug becomes nonpublic, if there's an attachment that contains a Review Board review (e.g. https://reviewboard.allizom.org/r/*/), we need to call a custom API (to be defined in bug 993222) on Review Board that will delete the review, and then mark the attachment as obsolete.

Comment 2

[Mark Côté [:mcote]]

Of course, we'll need authentication so not just anyone can delete a review request (to use the proper terminology).  This gets a little complicated, since the person who marks the bug as confidential may not be the person who opened the review.  For now, I think we'll have to use a Review Board admin account, so the Bugzilla extension will have to store these credentials somewhere.

Comment 3

[Mark Côté [:mcote]]

Aha, seems that there's already support for deleting review requests, at least if you have admin credentials.  You just send a DELETE HTTP method to the appropriate URL.  In the case here, the attachment will be a file containing a URL in the form https://reviewboard.allizom.org/r/<id>/; to delete it, you send a DELETE call to https://reviewboard.allizom.org/api/review-requests/<id>/.

There is a bit of weirdness if the "Depends On" field is set (or if another review request has its Depends On set to the request being deleted), but nothing major.  I'm going to leave the blocking bug here open for now until I am certain this is the way forward for now, but I'm pretty sure it is okay for a minimum viable product.

Comment 4

[:glob ✱]

i don't want updates to bugzilla being blocked by calling out to external systems, which means this code needs to run asynchronously.

that means this needs to be implemented as a connector within the push extension.

dylan: have a look at https://github.com/mozilla/webtools-bmo-bugzilla/tree/master/extensions/Push/lib/Connector

there's comments in Base.pm describing the methods which need to be implemented.


i'll ping you tomorrow to give you a walk through of the push ext.

Comment 5

[:glob ✱]

3am pseduo-code:

sub should_send {
  return
    $message->routing_key =~ /\.modify:is_private$/
    && (any attachments with a content-type of 'text/x-review-board-request')
}

sub send {
  perform additional expensive "should send" checks if required
  eval {
    call reviewboard api
  };
  if ($@) {
    return (PUSH_RESULT_TRANSIENT, clean_error($@));
  }
  return PUSH_RESULT_OK;
}

Comment 6

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

Attachment: bug-993223-v1.patch

Hello! I'm asking for feedback on this for two reasons:

1) I'm pretty sure the way I get a list of attachments is bad. I note that $bug->attachments will return nothing unless Bugzilla->user is an insider, which the default null-user is not. Should my connector take as a configuration parameter ($connect->config->{user}) a bugzilla user that is an insider? If I do that, I will need to inject this push-connector user into the request cache. Does that sound right?

2) I've gone over and over my should_send() logic, and I would appreciate another set of eyes on it to make sure it is sane. From my limited testing I think it's okay.

Also, nits: Some of the debugging log messages need to be removed (like the one that dumps yaml), those will be removed before I ask for review.

Comment 7

[:glob ✱]

Comment on attachment 8422314 [details] [diff] [review]
bug-993223-v1.patch

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

most of this looks sane.  here's my feedback from just reading through the code...

::: extensions/Push/lib/Connector/ReviewBoard.pm
@@ +20,5 @@
> +use Bugzilla::Attachment;
> +use Bugzilla::Extension::Push::ReviewBoard::Client;
> +
> +use JSON 'decode_json';
> +use Try::Tiny;

there's no need to add a dependency for this; use standard eval {} please.

even if one of the modules we use right now depends on try::tiny, that isn't guaranteed to always be the case.

@@ +40,5 @@
> +sub options {
> +    return (
> +        {
> +            name     => 'base_uri',
> +            label    => 'Virtual Host',

'virtual host' isn't a great label.  how about "review board url" ?

@@ +74,5 @@
> +
> +sub should_send {
> +    my ($self, $message) = @_;
> +
> +    my $logger  = Bugzilla->push_ext->logger;

you're not using the logger here

@@ +76,5 @@
> +    my ($self, $message) = @_;
> +
> +    my $logger  = Bugzilla->push_ext->logger;
> +    my $payload = $message->payload_decoded();
> +    my $target  = $payload->{event}->{target};

don't initialise $payload or $target unless you actually need to (ie. move these lines inside of the if block)

@@ +78,5 @@
> +    my $logger  = Bugzilla->push_ext->logger;
> +    my $payload = $message->payload_decoded();
> +    my $target  = $payload->{event}->{target};
> +
> +    if ($message->routing_key =~ /^(?:attachment|bug)\.modify:is_private$/) {

this won't work is more than just the is_private field is changed (eg. changing an attachment's description as well as making it private).

@@ +97,5 @@
> +    my ($self, $message) = @_;
> +    my $logger = Bugzilla->push_ext->logger;
> +    my $config = $self->config;
> +
> +    return PUSH_RESULT_IGNORED unless $self->should_send($message);

this line isn't required; the daemon calls should_send() before send()

@@ +106,5 @@
> +        my $event   = $payload->{event};
> +        my $target  = $event->{target};
> +        if ($target eq 'attachment') {
> +            use YAML;
> +            $logger->debug(Dump($payload->{$target}));

remove debugging

@@ +126,5 @@
> +}
> +
> +sub _process_attachment {
> +    my ($self, $attachment) = @_;
> +    my $content    = $attachment->data;

you should check the attachment's content-type before getting its data

@@ +167,5 @@
> +}
> +
> +sub _process_bug {
> +    my ($self, $bug) = @_;
> +    my $attachments = Bugzilla::Attachment->match({bug_id => $bug->{id}});

yeah, this is a bit funky.
note: don't access an object hash's keys directly; use $bug->id not $bug->{id}

try:
Bugzilla->set_user(Bugzilla::User->super_user);
...
Bugzilla->logout;

@@ +168,5 @@
> +
> +sub _process_bug {
> +    my ($self, $bug) = @_;
> +    my $attachments = Bugzilla::Attachment->match({bug_id => $bug->{id}});
> +    $self->_process_attachment($_) foreach @$attachments;

please use named variables in loops

foreach my $attachment (@$attachments) { .. }

::: extensions/Push/lib/ReviewBoard/Client.pm
@@ +8,5 @@
> +package Bugzilla::Extension::Push::ReviewBoard::Client;
> +use strict;
> +use warnings;
> +
> +use Params::Validate qw(:all);

there's no need to add this dependency either.
all the params passed to the constructor here have already been validated by push.

@@ +67,5 @@
> +
> +    return
> +      Bugzilla::Extension::Push::ReviewBoard::ReviewRequest->new(
> +                                                                client => $self,
> +                                                                @_);

that's some really weird formattting

::: extensions/Push/lib/ReviewBoard/Resource.pm
@@ +9,5 @@
> +use strict;
> +use warnings;
> +use 5.10.1;
> +
> +use Params::Validate qw(:all);

same deal with new dependencies here too :)

@@ +35,5 @@
> +sub uri {
> +    my ($self, @path) = @_;
> +
> +    my $uri  = URI->new( $self->client->base_uri );
> +    $uri->path( join('/', $self->path, @path) );

nit: remove spaces after ( and before )

Comment 8

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

Attachment: bug-993223-v2.patch

I have removed Params::Validate, although I think we should depend on it and use it in some cases. Try::Tiny is also removed, and I left a comment in case Push is ported to upstream (the value of $@ is not reliable on perls older than 5.14).

Comment 9

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

Attachment: bug-993223-v2.1.patch

Caught an error in the patch after I *though* I had tested it. Actually, I am somewhat nervous about this code given the difficulty in testing it. :-/

Comment 10

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #8)
> I have removed Params::Validate, although I think we should depend on it and
> use it in some cases. Try::Tiny is also removed, and I left a comment in
> case Push is ported to upstream (the value of $@ is not reliable on perls
> older than 5.14).

push will never be ported upstream; this comment isn't necessary.

Comment 11

[:glob ✱]

Comment on attachment 8423545 [details] [diff] [review]
bug-993223-v2.1.patch

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

::: extensions/Push/lib/Connector/ReviewBoard.pm
@@ +164,5 @@
> +    foreach my $attachment (@{ $bug->attachments }) {
> +        next if $attachment->content_type ne RB_CONTENT_TYPE;
> +        $self->_process_attachment($attachment);
> +    }
> +    Bugzilla->logout;

i think it would be safer to:

bugzilla->set_user(..)
$bug = ..
@attachments = $bug->attachments
bugzilla->logout

foreach $attachment (@attachments)
  ...

Comment 12

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

Attachment: bug-993223-v2.2.patch

270 lines of testing later, the code paths of the connector have been walked. Ready for review. My test case is really ugly, and is not included in the review. I'll put a link in IRC so someone can gawk at it.

Comment 13

[:glob ✱]

Comment on attachment 8423695 [details] [diff] [review]
bug-993223-v2.2.patch

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

overall this looks great.

unfortunately the first thing i tested failed.

i marked an attachment as private in bugzilla.  the resulted in:

[Fri May 16 15:55:58 2014] DEBUG: Does Not Exist: Review Request 8 does not exist

this isn't true, review 8 exists and is returned in the list from https://reviewboard.allizom.org/api/review-requests/

the url used is https://reviewboard.allizom.org/api/review-requests/8 which if you hit it with your browser says it doesn't exist.
however it you add a trailing / .. https://reviewboard.allizom.org/api/review-requests/8/ it works.

(time passes)

- $uri->path(join('/', $self->path, @path));
+ $uri->path(join('/', $self->path, @path) . '/');

fixes it..

[Fri May 16 16:16:40 2014] DEBUG: polling
[Fri May 16 16:16:40 2014] DEBUG: pushing to ReviewBoard
[Fri May 16 16:16:40 2014] DEBUG: immediate send
[Fri May 16 16:16:43 2014] ERROR: Permission Denied: ReviewBoard Push Connector may be misconfigured
[Fri May 16 16:16:43 2014] INFO: ReviewBoard: Message #4: TRANSIENT-ERROR You don't have permission for this

and after upgrading my rb account to be a super user:

[Fri May 16 16:22:45 2014] DEBUG: polling
[Fri May 16 16:22:45 2014] DEBUG: processing backlog for ReviewBoard
[Fri May 16 16:22:49 2014] DEBUG: Deleted review request 80
[Fri May 16 16:22:49 2014] INFO: ReviewBoard: Message #4: OK



setting a bug as private throws the following error results in the connector failing with:

[Fri May 16 16:25:17 2014] INFO: ReviewBoard: Message #5: TRANSIENT-ERROR Can't locate object method "content_type" via package "Bugzilla::Attachment"

obviously it should be $attachment->mimetype

::: extensions/Push/lib/Connector/ReviewBoard.pm
@@ +14,5 @@
> +
> +use Bugzilla::Constants;
> +use Bugzilla::Extension::Push::Constants;
> +use Bugzilla::Extension::Push::Util;
> +use Bugzilla::Util qw(generate_random_password);

generate_random_password isn't used here

@@ +103,5 @@
> +        my $payload = $message->payload_decoded();
> +        my $target  = $payload->{event}->{target};
> +        my $method  = $self->can("_process_$target");
> +
> +        $self->$method( $payload->{$target} );

there's probably no point calling $self->can without checking the return value.

you probably want this instead:

if (my $method = $self->can(..)) {
  $self->$method(..)
}

nit: remove space after ( and before )

@@ +106,5 @@
> +
> +        $self->$method( $payload->{$target} );
> +    };
> +    # This form of exceptional handling is unreliable on any perl before 5.14.
> +    # Reference: http://wp.me/pBBoS-4c

as per comment 10, please remove this comment

@@ +135,5 @@
> +        }
> +        elsif ($status == 404) {
> +            # API error 100 - Does Not Exist
> +            $logger->debug( "Does Not Exist: "
> +                           . "Review Request $id does not exist");

in bugzilla we put the . at the end of the proceeding line, and the space after the ( needs to be removed:

$logger->debug("Does Not Exist: " .
               "Review Request $id does not exist");

@@ +178,5 @@
> +    foreach my $attachment (@{ $bug->attachments }) {
> +        next if $attachment->content_type ne RB_CONTENT_TYPE;
> +        $self->_process_attachment($attachment);
> +    }
> +    Bugzilla->logout;

as per comment 11, i'd prefer for the attachment array to be built, then logout, then processed.

::: extensions/Push/lib/ReviewBoard/Client.pm
@@ +4,5 @@
> +#
> +# This Source Code Form is "Incompatible With Secondary Licenses", as
> +# defined by the Mozilla Public License, v. 2.0.
> +
> +package Bugzilla::Extension::Push::ReviewBoard::Client;

as these packages are only for the reviewboard connector, please move them to
Bugzilla::Extension::Push::Connector::ReviewBoard::Client

(everything connector related should live under the Connector directory)

@@ +20,5 @@
> +
> +sub new {
> +    my ($class, %params) = @_;
> +
> +    # croak "->new() is a class method" if blessed($class);

remove this comment; there's no point keeping commented out code in the tree

Comment 14

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

Attachment: bug-993223-v3.patch

glob> Were you able to just test this locally or did you use dev? What settings did you use to make it talk to reviewboard?

All suggested changes applied.

Comment 15

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #14)
> Created attachment 8424612 [details] [diff] [review]
> bug-993223-v3.patch
> 
> glob> Were you able to just test this locally or did you use dev?

i do all my testing locally.

> What settings did you use to make it talk to reviewboard?

i used https://reviewboard.allizom.org with my bmo credentials.  i didn't test the proxy settings (didn't see the point until the other more critical issues were addressed first).

Comment 16

[Mark Côté [:mcote]]

You should be using reviewboard-dev.allizom.org for testing, since (a) it is running the latest Review Board and rbbz versions, (b) it's tied to bugzilla-dev and (c) it's meant for testing and hence you can mess around on it with the danger of stomping on someone's real work.

Comment 17

[:glob ✱]

Comment on attachment 8424612 [details] [diff] [review]
bug-993223-v3.patch

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

Bareword "PUSH_RESULTIGNORED" not allowed while "strict subs" in use at ./extensions/Push/lib/Connector/ReviewBoard.pm line 109

[Tue May 20 13:38:25 2014] INFO: ReviewBoard: Message #5: TRANSIENT-ERROR Can't locate object method "mimetype" via package "Bugzilla::Attachment"

so the column name is mimetype, but the method to call is $attachment->contenttype().  sorry about leading you astray in my prior review, but this should have been picked up when you tested your changes.

::: extensions/Push/t/client.t
@@ +1,1 @@
> +#!/usr/bin/perl -T

this test needs to be clearly identifiable as related to the reviewboard connector.

please rename to extensions/Push/t/connector/ReviewBoard.t

Comment 18

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

Attachment: bug-993223-v4.patch

(In reply to Byron Jones ‹:glob› from comment #17)
> Comment on attachment 8424612 [details] [diff] [review]
> bug-993223-v3.patch
> 
> Review of attachment 8424612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bareword "PUSH_RESULTIGNORED" not allowed while "strict subs" in use at
> ./extensions/Push/lib/Connector/ReviewBoard.pm line 109
> 
> [Tue May 20 13:38:25 2014] INFO: ReviewBoard: Message #5: TRANSIENT-ERROR
> Can't locate object method "mimetype" via package "Bugzilla::Attachment"
> 
> so the column name is mimetype, but the method to call is
> $attachment->contenttype().  sorry about leading you astray in my prior
> review, but this should have been picked up when you tested your changes.

On the road the being able to test... Can you flip my admin bit on reviewboard-dev?

[Tue May 20 05:02:44 2014] ERROR: Permission Denied: ReviewBoard Push Connector may be misconfigured
[Tue May 20 05:02:44 2014] INFO: ReviewBoard: Message #58: TRANSIENT-ERROR You don't have permission for this
[Tue May 20 05:02:44 2014] DEBUG: setting next attempt for ReviewBoard to 2014-05-20 09:02:49 (attempt 1)


> ::: extensions/Push/t/client.t
> @@ +1,1 @@
> > +#!/usr/bin/perl -T
> 
> this test needs to be clearly identifiable as related to the reviewboard
> connector.
> 
> please rename to extensions/Push/t/connector/ReviewBoard.t
It was a pretty worthless test, as it didn't even catch the above error.

I rewrote the unit test (using that name) and it detected the mimetype vs. contenttype error (which is also fixed, now).

Not requesting review until I get the admin bit set on rb-dev. :-)

Comment 19

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

Comment on attachment 8425314 [details] [diff] [review]
bug-993223-v4.patch

Both the unit test and my manual testing work. Still really weird that the RB API requires terminating slashes on all resources...

Comment 20

[:glob ✱]

Comment on attachment 8425314 [details] [diff] [review]
bug-993223-v4.patch

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

r=glob \o/

::: extensions/Push/lib/Connector/ReviewBoard.pm
@@ +160,5 @@
> +                }
> +            }
> +        }
> +        else {
> +            $logger->error("Cannot find link: ReviewBoard Push Connecor may be misconfigured");

s/Connecor/Connector/

Comment 21

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

To [e-mail]:webtools/bmo/bugzilla.git
   3d93c59..e1aea96  master -> master

Comment 22

[Mark Côté [:mcote]]

NB The other side (prohibiting RB from publishing review requests for nonpublic bugs) was already pushed a couple weeks ago (https://github.com/mozilla/rbbz/commit/5573294073af632c281f9dffea429e6599e8bea6).

Comment 23

[Mark Côté [:mcote]]

Er wrong bug, meant to write that into the depends-on bug.