Merge remote-tracking branch 'bmo/master'

4 files changed, 60 insertions, 306 deletions

diff --git a/extensions/PhabBugz/lib/Constants.pm b/extensions/PhabBugz/lib/Constants.pm
index 1692f8fb9..19987de25 100644
--- a/extensions/PhabBugz/lib/Constants.pm
+++ b/extensions/PhabBugz/lib/Constants.pm
@@ -19,6 +19,7 @@ our @EXPORT = qw(
     PHAB_FEED_POLL_SECONDS
     PHAB_USER_POLL_SECONDS
     PHAB_GROUP_POLL_SECONDS
+    PHAB_TIMEOUT
 );
 
 use constant PHAB_ATTACHMENT_PATTERN => qr/^phabricator-D(\d+)/;
@@ -27,5 +28,6 @@ use constant PHAB_CONTENT_TYPE       => 'text/x-phabricator-request';
 use constant PHAB_FEED_POLL_SECONDS  => $ENV{PHAB_FEED_POLL} // 5;
 use constant PHAB_USER_POLL_SECONDS  => $ENV{PHAB_USER_POLL} // 60;
 use constant PHAB_GROUP_POLL_SECONDS => $ENV{PHAB_GROUP_POLL} // 300;
+use constant PHAB_TIMEOUT            => $ENV{PHAB_TIMEOUT} // 60;
 
 1;
diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index f2a440bb1..71e6aa827 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -11,10 +11,10 @@ use 5.10.1;
 
 use IO::Async::Timer::Periodic;
 use IO::Async::Loop;
+use IO::Async::Signal;
 use List::Util qw(first);
 use List::MoreUtils qw(any uniq);
 use Moo;
-use Scalar::Util qw(blessed);
 use Try::Tiny;
 use Type::Params qw( compile );
 use Type::Utils;
@@ -48,6 +48,13 @@ my $Invocant = class_type { class => __PACKAGE__ };
 sub start {
     my ($self) = @_;
 
+    my $sig_alarm =  IO::Async::Signal->new(
+        name => 'ALRM',
+        on_receipt => sub {
+            FATAL("Timeout reached");
+            exit;
+        },
+    );
     # Query for new revisions or changes
     my $feed_timer = IO::Async::Timer::Periodic->new(
         first_interval => 0,
@@ -56,13 +63,17 @@ sub start {
         on_tick        => sub {
             try {
                 with_writable_database {
+                    alarm(PHAB_TIMEOUT);
                     $self->feed_query();
                 };
             }
             catch {
                 FATAL($_);
+            }
+            finally {
+                alarm(0);
+                Bugzilla->_cleanup();
             };
-            Bugzilla->_cleanup();
         },
     );
 
@@ -74,13 +85,17 @@ sub start {
         on_tick        => sub {
             try {
                 with_writable_database {
+                    alarm(PHAB_TIMEOUT);
                     $self->user_query();
                 };
             }
             catch {
                 FATAL($_);
+            }
+            finally {
+                alarm(0);
+                Bugzilla->_cleanup();
             };
-            Bugzilla->_cleanup();
         },
     );
 
@@ -92,13 +107,18 @@ sub start {
         on_tick        => sub {
             try {
                 with_writable_database {
+                    alarm(PHAB_TIMEOUT);
                     $self->group_query();
                 };
             }
             catch {
                 FATAL($_);
+            }
+            finally {
+                alarm(0);
+                Bugzilla->_cleanup();
             };
-            Bugzilla->_cleanup();
+
         },
     );
 
@@ -106,6 +126,7 @@ sub start {
     $loop->add($feed_timer);
     $loop->add($user_timer);
     $loop->add($group_timer);
+    $loop->add($sig_alarm);
     $feed_timer->start;
     $user_timer->start;
     $group_timer->start;
@@ -221,6 +242,11 @@ sub feed_query {
 
         $delete_build_target->execute($target->{name}, $target->{value});
      }
+
+    if (Bugzilla->datadog) {
+      my $dd = Bugzilla->datadog();
+      $dd->increment('bugzilla.phabbugz.feed_query_count');
+    }
 }
 
 sub user_query {
@@ -261,6 +287,11 @@ sub user_query {
         };
         $self->save_last_id($user_id, 'user');
     }
+
+    if (Bugzilla->datadog) {
+      my $dd = Bugzilla->datadog();
+      $dd->increment('bugzilla.phabbugz.user_query_count');
+    }
 }
 
 sub group_query {
@@ -359,6 +390,11 @@ sub group_query {
             INFO( "Project " . $project->name . " updated" );
         }
     }
+
+    if (Bugzilla->datadog) {
+      my $dd = Bugzilla->datadog();
+      $dd->increment('bugzilla.phabbugz.group_query_count');
+    }
 }
 
 sub process_revision_change {
@@ -501,105 +537,6 @@ sub process_revision_change {
         $attachment->update($timestamp);
     }
 
-    # REVIEWER STATUSES
-
-    my (@accepted, @denied);
-    foreach my $review (@{ $revision->reviews }) {
-        push @accepted, $review->{user} if $review->{status} eq 'accepted';
-        push @denied,   $review->{user} if $review->{status} eq 'rejected';
-    }
-
-    my @accepted_user_ids = map { $_->bugzilla_user->id } grep { defined $_->bugzilla_user } @accepted;
-    my @denied_user_ids   = map { $_->bugzilla_user->id } grep { defined $_->bugzilla_user } @denied;
-    my %reviewers_hash    = map { $_->{user}->name => 1 } @{ $revision->reviews };
-
-    foreach my $attachment (@attachments) {
-        my ($attach_revision_id) = ($attachment->filename =~ PHAB_ATTACHMENT_PATTERN);
-        next if $revision->id != $attach_revision_id;
-
-        # Clear old accepted review flags if no longer accepted
-        my (@denied_flags, @new_flags, @removed_flags, %accepted_done, $flag_type);
-        foreach my $flag (@{ $attachment->flags }) {
-            next if $flag->type->name ne 'review';
-            $flag_type = $flag->type if $flag->type->is_active;
-            next if $flag->status ne '+';
-            if (any { $flag->setter->id == $_ } @denied_user_ids) {
-                INFO('Denying review flag set by ' . $flag->setter->name);
-                push(@denied_flags, { id => $flag->id, setter => $flag->setter, status => 'X' });
-            }
-            if (any { $flag->setter->id == $_ } @accepted_user_ids) {
-                INFO('Skipping as review+ already set by ' . $flag->setter->name);
-                $accepted_done{$flag->setter->id}++;
-            }
-            if (!any { $flag->setter->id == $_ } (@accepted_user_ids, @denied_user_ids)) {
-                INFO('Clearing review+ flag set by ' . $flag->setter->name);
-                push(@removed_flags, { id => $flag->id, setter => $flag->setter, status => 'X' });
-            }
-        }
-
-        $flag_type ||= first { $_->name eq 'review' && $_->is_active } @{ $attachment->flag_types };
-
-        die "Unable to find review flag!" unless $flag_type;
-
-        # Create new flags
-        foreach my $user_id (@accepted_user_ids) {
-            next if $accepted_done{$user_id};
-            my $user = Bugzilla::User->check({ id => $user_id, cache => 1 });
-            INFO('Setting new review+ flag for ' . $user->name);
-            push(@new_flags, { type_id => $flag_type->id, setter => $user, status => '+' });
-        }
-
-        # Process each flag change by updating the flag and adding a comment
-        foreach my $flag_data (@new_flags) {
-            my $comment = $flag_data->{setter}->name . " has approved the revision.";
-            $self->add_flag_comment(
-                {
-                    bug        => $bug,
-                    attachment => $attachment,
-                    comment    => $comment,
-                    user       => $flag_data->{setter},
-                    old_flags  => [],
-                    new_flags  => [$flag_data],
-                    timestamp  => $timestamp
-                }
-            );
-        }
-        foreach my $flag_data (@denied_flags) {
-            my $comment = $flag_data->{setter}->name . " has requested changes to the revision.\n";
-            $self->add_flag_comment(
-                {
-                    bug        => $bug,
-                    attachment => $attachment,
-                    comment    => $comment,
-                    user       => $flag_data->{setter},
-                    old_flags  => [$flag_data],
-                    new_flags  => [],
-                    timestamp  => $timestamp
-                }
-            );
-        }
-        foreach my $flag_data (@removed_flags) {
-            my $comment;
-            if ( exists $reviewers_hash{ $flag_data->{setter}->name } ) {
-                $comment = "Flag set by " . $flag_data->{setter}->name . " is no longer active.\n";
-            }
-            else {
-                $comment = $flag_data->{setter}->name . " has been removed from the revision.\n";
-            }
-            $self->add_flag_comment(
-                {
-                    bug        => $bug,
-                    attachment => $attachment,
-                    comment    => $comment,
-                    user       => $flag_data->{setter},
-                    old_flags  => [$flag_data],
-                    new_flags  => [],
-                    timestamp  => $timestamp
-                }
-            );
-        }
-    }
-
     # FINISH UP
 
     $bug->update($timestamp);
@@ -736,6 +673,11 @@ sub process_new_user {
         foreach my $attachment (@attachments) {
             my ($revision_id) = ($attachment->filename =~ PHAB_ATTACHMENT_PATTERN);
 
+            if (!$revision_id) {
+                WARN("Skipping " . $attachment->filename . " on bug $bug_id. Filename should be fixed.");
+                next;
+            }
+
             INFO("Processing revision D$revision_id");
 
             my $revision = Bugzilla::Extension::PhabBugz::Revision->new_from_query(
@@ -839,38 +781,4 @@ sub get_group_members {
     );
 }
 
-sub add_flag_comment {
-    state $check = compile(
-        $Invocant,
-        Dict [
-            bug        => Bug,
-            attachment => Attachment,
-            comment    => Str,
-            user       => User,
-            old_flags  => ArrayRef,
-            new_flags  => ArrayRef,
-            timestamp  => Str,
-        ],
-    );
-    my ( $self, $params ) = $check->(@_);
-    my ( $bug, $attachment, $comment, $user, $old_flags, $new_flags, $timestamp )
-        = @$params{qw(bug attachment comment user old_flags new_flags timestamp)};
-
-    # when this function returns, Bugzilla->user will return to its previous value.
-    my $restore_prev_user = Bugzilla->set_user($user, scope_guard => 1);
-
-    INFO("Flag comment: $comment");
-    $bug->add_comment(
-        $comment,
-        {
-            isprivate  => $attachment->isprivate,
-            type       => CMT_ATTACHMENT_UPDATED,
-            extra_data => $attachment->id
-        }
-    );
-
-    $attachment->set_flags( $old_flags, $new_flags );
-    $attachment->update($timestamp);
-}
-
 1;
diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
index a93533e75..32f860413 100644
--- a/extensions/PhabBugz/lib/Util.pm
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -20,14 +20,14 @@ use Bugzilla::Util qw(trim);
 use Bugzilla::Extension::PhabBugz::Constants;
 use Bugzilla::Extension::PhabBugz::Types qw(:types);
 
-use JSON::XS qw(encode_json decode_json);
 use List::Util qw(first);
-use LWP::UserAgent;
 use Taint::Util qw(untaint);
 use Try::Tiny;
 use Type::Params qw( compile );
 use Type::Utils;
 use Types::Standard qw( :types );
+use Mojo::UserAgent;
+use Mojo::JSON qw(encode_json);
 
 use base qw(Exporter);
 
@@ -35,7 +35,6 @@ our @EXPORT = qw(
     create_revision_attachment
     get_attachment_revisions
     get_bug_role_phids
-    get_needs_review
     intersect
     is_attachment_phab_revision
     request
@@ -160,11 +159,10 @@ sub request {
 
     my $ua = $request_cache->{phabricator_ua};
     unless ($ua) {
-        $ua = $request_cache->{phabricator_ua} = LWP::UserAgent->new(timeout => 10);
+        $ua = $request_cache->{phabricator_ua} = Mojo::UserAgent->new;
         if ($params->{proxy_url}) {
-            $ua->proxy('https', $params->{proxy_url});
+            $ua->proxy($params->{proxy_url});
         }
-        $ua->default_header('Content-Type' => 'application/x-www-form-urlencoded');
     }
 
     my $phab_api_key = $params->{phabricator_api_key};
@@ -176,25 +174,16 @@ sub request {
 
     $data->{__conduit__} = { token => $phab_api_key };
 
-    my $response = $ua->post($full_uri, { params => encode_json($data) });
-
+    my $response = $ua->post($full_uri => form => { params => encode_json($data) })->result;
     ThrowCodeError('phabricator_api_error', { reason => $response->message })
       if $response->is_error;
 
-    my $result;
-    my $result_ok = eval {
-        my $content = $response->content;
-        untaint($content);
-        $result = decode_json( $content );
-        1;
-    };
-    if (!$result_ok || $result->{error_code}) {
-        ThrowCodeError('phabricator_api_error',
-            { reason => 'JSON decode failure' }) if !$result_ok;
-        ThrowCodeError('phabricator_api_error',
-            { code   => $result->{error_code},
-              reason => $result->{error_info} }) if $result->{error_code};
-    }
+    my $result = $response->json;
+    ThrowCodeError('phabricator_api_error',
+        { reason => 'JSON decode failure' }) if !defined($result);
+    ThrowCodeError('phabricator_api_error',
+        { code   => $result->{error_code},
+          reason => $result->{error_info} }) if $result->{error_code};
 
     return $result;
 }
@@ -206,48 +195,4 @@ sub set_phab_user {
     return Bugzilla->set_user($user, scope_guard => 1);
 }
 
-sub get_needs_review {
-    my ($user) = @_;
-    $user //= Bugzilla->user;
-    return unless $user->id;
-
-    my $phab_user = Bugzilla::Extension::PhabBugz::User->new_from_query(
-      {
-        ids => [ $user->id ]
-      }
-    );
-
-    return [] unless $phab_user;
-
-    my $diffs = request(
-        'differential.revision.search',
-        {
-            attachments => {
-                reviewers => 1,
-            },
-            constraints => {
-                reviewerPHIDs => [$phab_user->phid],
-                statuses      => ["open()"],
-            },
-            order => 'newest',
-        }
-    );
-    ThrowCodeError('phabricator_api_error', { reason => 'Malformed Response' })
-        unless exists $diffs->{result}{data};
-
-    my @revisions;
-    foreach my $revision ( @{ $diffs->{result}{data} } ) {
-        foreach my $reviewer ( @{ $revision->{attachments}->{reviewers}->{reviewers} } ) {
-            if (   $reviewer->{reviewerPHID} eq $phab_user->phid
-                && $reviewer->{status} =~ /^(?:added|blocking)$/ )
-            {
-                push @revisions, $revision;
-                last;
-            }
-        }
-    }
-
-    return \@revisions;
-}
-
 1;
diff --git a/extensions/PhabBugz/lib/WebService.pm b/extensions/PhabBugz/lib/WebService.pm
index fa9306667..19a758a70 100644
--- a/extensions/PhabBugz/lib/WebService.pm
+++ b/extensions/PhabBugz/lib/WebService.pm
@@ -16,29 +16,23 @@ use base qw(Bugzilla::WebService);
 use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::User;
-use Bugzilla::Util qw(detaint_natural datetime_from time_ago trick_taint);
+use Bugzilla::Util qw(detaint_natural trick_taint);
 use Bugzilla::WebService::Constants;
 
 use Bugzilla::Extension::PhabBugz::Constants;
-use Bugzilla::Extension::PhabBugz::Util qw(
-    get_needs_review
-);
 
-use DateTime ();
-use List::Util qw(first uniq);
+use List::Util qw(first);
 use List::MoreUtils qw(any);
 use MIME::Base64 qw(decode_base64);
 
 use constant READ_ONLY => qw(
     check_user_enter_bug_permission
     check_user_permission_for_bug
-    needs_review
 );
 
 use constant PUBLIC_METHODS => qw(
     check_user_enter_bug_permission
     check_user_permission_for_bug
-    needs_review
     set_build_target
 );
 
@@ -99,95 +93,6 @@ sub check_user_enter_bug_permission {
     };
 }
 
-sub needs_review {
-    my ($self, $params) = @_;
-
-    $self->_check_phabricator();
-
-    my $user = Bugzilla->login(LOGIN_REQUIRED);
-    my $dbh  = Bugzilla->dbh;
-
-    my $reviews = get_needs_review();
-
-    my $authors = Bugzilla::Extension::PhabBugz::User->match({
-        phids => [
-            uniq
-            grep { defined }
-            map { $_->{fields}{authorPHID} }
-            @$reviews
-        ]
-    });
-
-    my %author_phab_to_id = map { $_->phid => $_->bugzilla_user->id } @$authors;
-    my %author_id_to_user = map { $_->bugzilla_user->id => $_->bugzilla_user } @$authors;
-
-    # bug data
-    my $visible_bugs = $user->visible_bugs([
-        uniq
-        grep { $_ }
-        map { $_->{fields}{'bugzilla.bug-id'} }
-        @$reviews
-    ]);
-
-    # get all bug statuses and summaries in a single query to avoid creation of
-    # many bug objects
-    my %bugs;
-    if (@$visible_bugs) {
-        #<<<
-        my $bug_rows =$dbh->selectall_arrayref(
-            'SELECT bug_id, bug_status, short_desc ' .
-            '  FROM bugs ' .
-            ' WHERE bug_id IN (' . join(',', ('?') x @$visible_bugs) . ')',
-            { Slice => {} },
-            @$visible_bugs
-        );
-        #>>>
-        %bugs = map { $_->{bug_id} => $_ } @$bug_rows;
-    }
-
-    # build result
-    my $datetime_now = DateTime->now(time_zone => $user->timezone);
-    my @result;
-    foreach my $review (@$reviews) {
-        my $review_flat = {
-            id     => $review->{id},
-            title  => $review->{fields}{title},
-            url    => Bugzilla->params->{phabricator_base_uri} . 'D' . $review->{id},
-        };
-
-        # show date in user's timezone
-        my $datetime = DateTime->from_epoch(
-            epoch     => $review->{fields}{dateModified},
-            time_zone => 'UTC'
-        );
-        $datetime->set_time_zone($user->timezone);
-        $review_flat->{updated}       = $datetime->strftime('%Y-%m-%d %T %Z');
-        $review_flat->{updated_fancy} = time_ago($datetime, $datetime_now);
-
-        # review requester
-        if (my $author = $author_id_to_user{$author_phab_to_id{ $review->{fields}{authorPHID} }}) {
-            $review_flat->{author_name}  = $author->name;
-            $review_flat->{author_email} = $author->email;
-        }
-        else {
-            $review_flat->{author_name}  = 'anonymous';
-            $review_flat->{author_email} = 'anonymous';
-        }
-
-        # referenced bug
-        if (my $bug_id = $review->{fields}{'bugzilla.bug-id'}) {
-            my $bug = $bugs{$bug_id};
-            $review_flat->{bug_id}      = $bug_id;
-            $review_flat->{bug_status}  = $bug->{bug_status};
-            $review_flat->{bug_summary} = $bug->{short_desc};
-        }
-
-        push @result, $review_flat;
-    }
-
-    return { result => \@result };
-}
-
 sub set_build_target {
     my ( $self, $params ) = @_;
 
@@ -257,12 +162,6 @@ sub rest_resources {
                 },
             },
         },
-        # Review requests
-        qw{^/phabbugz/needs_review$}, {
-            GET => {
-                method => 'needs_review',
-            },
-        }
     ];
 }