Bug 1482145 - PhabBot changes are showing up as from the wrong user

4 files changed, 252 insertions, 31 deletions

diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index 2be96153e..f2a440bb1 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -54,8 +54,10 @@ sub start {
         interval       => PHAB_FEED_POLL_SECONDS,
         reschedule     => 'drift',
         on_tick        => sub {
-            try{
-                $self->feed_query();
+            try {
+                with_writable_database {
+                    $self->feed_query();
+                };
             }
             catch {
                 FATAL($_);
@@ -70,8 +72,10 @@ sub start {
         interval       => PHAB_USER_POLL_SECONDS,
         reschedule     => 'drift',
         on_tick        => sub {
-            try{
-                $self->user_query();
+            try {
+                with_writable_database {
+                    $self->user_query();
+                };
             }
             catch {
                 FATAL($_);
@@ -86,8 +90,10 @@ sub start {
         interval       => PHAB_GROUP_POLL_SECONDS,
         reschedule     => 'drift',
         on_tick        => sub {
-            try{
-                $self->group_query();
+            try {
+                with_writable_database {
+                    $self->group_query();
+                };
             }
             catch {
                 FATAL($_);
@@ -149,23 +155,30 @@ sub feed_query {
         }
 
         # Skip changes done by phab-bot user
-        my $phab_user = Bugzilla::Extension::PhabBugz::User->new_from_query(
-          {
-            phids => [ $author_phid ]
-          }
+        # If changer does not exist in bugzilla database
+        # we use the phab-bot account as the changer
+        my $author = Bugzilla::Extension::PhabBugz::User->new_from_query(
+          { phids => [ $author_phid  ] }
         );
 
-        if ($phab_user && $phab_user->bugzilla_id) {
-            if ($phab_user->bugzilla_user->login eq PHAB_AUTOMATION_USER) {
+        if ($author && $author->bugzilla_id) {
+            if ($author->bugzilla_user->login eq PHAB_AUTOMATION_USER) {
                 INFO("SKIPPING: Change made by phabricator user");
                 $self->save_last_id($story_id, 'feed');
                 next;
             }
         }
-
-        with_writable_database {
-            $self->process_revision_change($object_phid, $story_text);
-        };
+        else {
+            my $phab_user = Bugzilla::User->new( { name => PHAB_AUTOMATION_USER } );
+            $author = Bugzilla::Extension::PhabBugz::User->new_from_query(
+                {
+                    ids => [ $phab_user->id ]
+                }
+            );
+        }
+        # Load the revision from Phabricator
+        my $revision = Bugzilla::Extension::PhabBugz::Revision->new_from_query({ phids => [ $object_phid ] });
+        $self->process_revision_change($revision, $author, $story_text);
         $self->save_last_id($story_id, 'feed');
     }
 
@@ -197,9 +210,7 @@ sub feed_query {
             }
         );
 
-        with_writable_database {
-            $self->process_revision_change($revision, " created D" . $revision->id);
-        };
+        $self->process_revision_change( $revision, $revision->author, " created D" . $revision->id );
 
         # Set the build target to a passing status to
         # allow the revision to exit draft state
@@ -351,16 +362,10 @@ sub group_query {
 }
 
 sub process_revision_change {
-    my ($self, $revision_phid, $story_text) = @_;
-
-    # Load the revision from Phabricator
-    my $revision =
-        blessed $revision_phid
-        ? $revision_phid
-        : Bugzilla::Extension::PhabBugz::Revision->new_from_query({ phids => [ $revision_phid ] });
+    state $check = compile($Invocant, Revision, LinkedPhabUser, Str);
+    my ($self, $revision, $changer, $story_text) = $check->(@_);
 
     # NO BUG ID
-
     if (!$revision->bug_id) {
         if ($story_text =~ /\s+created\s+D\d+/) {
             # If new revision and bug id was omitted, make revision public
@@ -378,10 +383,11 @@ sub process_revision_change {
 
 
     my $log_message = sprintf(
-        "REVISION CHANGE FOUND: D%d: %s | bug: %d | %s",
+        "REVISION CHANGE FOUND: D%d: %s | bug: %d | %s | %s",
         $revision->id,
         $revision->title,
         $revision->bug_id,
+        $changer->name,
         $story_text);
     INFO($log_message);
 
@@ -533,6 +539,8 @@ sub process_revision_change {
 
         $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};
@@ -600,7 +608,7 @@ sub process_revision_change {
     # Email changes for this revisions bug and also for any other
     # bugs that previously had these revision attachments
     foreach my $bug_id ($revision->bug_id, keys %other_bugs) {
-        Bugzilla::BugMail::Send($bug_id, { changer => $rev_attachment->attacher });
+        Bugzilla::BugMail::Send($bug_id, { changer => $changer->bugzilla_user });
     }
 
     INFO('SUCCESS: Revision D' . $revision->id . ' processed');
diff --git a/extensions/PhabBugz/lib/Types.pm b/extensions/PhabBugz/lib/Types.pm
index 44987bfee..493e97fbc 100644
--- a/extensions/PhabBugz/lib/Types.pm
+++ b/extensions/PhabBugz/lib/Types.pm
@@ -13,13 +13,16 @@ use warnings;
 
 use Type::Library
     -base,
-    -declare => qw( Revision PhabUser Policy Project );
+    -declare => qw( Revision LinkedPhabUser PhabUser Policy Project );
 use Type::Utils -all;
-use Types::Standard -types;
+use Types::Standard -all;
 
 class_type Revision, { class => 'Bugzilla::Extension::PhabBugz::Revision' };
 class_type Policy, { class => 'Bugzilla::Extension::PhabBugz::Policy' };
 class_type Project, { class => 'Bugzilla::Extension::PhabBugz::Project' };
 class_type PhabUser, { class => 'Bugzilla::Extension::PhabBugz::User' };
+declare LinkedPhabUser,
+    as PhabUser,
+    where { is_Int($_->bugzilla_id) };
 
 1;
diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
index 9c79c1855..a7ae98744 100644
--- a/extensions/PhabBugz/lib/Util.pm
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -85,6 +85,7 @@ sub create_revision_attachment {
     $bug->add_comment($revision->summary, { type       => CMT_ATTACHMENT_CREATED,
                                             extra_data => $attachment->id });
 
+    delete $bug->{attachments};
 
     return $attachment;
 }
diff --git a/extensions/PhabBugz/t/review-flags.t b/extensions/PhabBugz/t/review-flags.t
new file mode 100644
index 000000000..610c46dca
--- /dev/null
+++ b/extensions/PhabBugz/t/review-flags.t
@@ -0,0 +1,209 @@
+#!/usr/bin/perl
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+#
+# This Source Code Form is "Incompatible With Secondary Licenses", as
+# defined by the Mozilla Public License, v. 2.0.
+use strict;
+use warnings;
+use 5.10.1;
+use lib qw( . lib local/lib/perl5 );
+BEGIN { $ENV{LOG4PERL_CONFIG_FILE} = 'log4perl-t.conf' }
+use Test2::V0;
+
+our @EMAILS;
+
+BEGIN {
+    require Bugzilla::Mailer;
+    no warnings 'redefine';
+    *Bugzilla::Mailer::MessageToMTA = sub {
+        push @EMAILS, [@_];
+    };
+}
+use Bugzilla::Test::MockDB;
+use Bugzilla::Test::MockParams;
+use Bugzilla::Test::Util qw(create_user);
+use Test2::Tools::Mock;
+use Try::Tiny;
+use JSON::MaybeXS;
+use Bugzilla::Constants;
+use URI;
+use File::Basename;
+use Digest::SHA qw(sha1_hex);
+use Data::Dumper;
+
+use ok 'Bugzilla::Extension::PhabBugz::Feed';
+use ok 'Bugzilla::Extension::PhabBugz::Constants', 'PHAB_AUTOMATION_USER';
+use ok 'Bugzilla::Config', 'SetParam';
+can_ok('Bugzilla::Extension::PhabBugz::Feed', qw( group_query feed_query user_query ));
+
+SetParam(phabricator_base_uri => 'http://fake.phabricator.tld/');
+SetParam(mailfrom => 'bugzilla-daemon');
+Bugzilla->error_mode(ERROR_MODE_TEST);
+my $nobody = create_user('nobody@mozilla.org', '*');
+my $phab_bot = create_user(PHAB_AUTOMATION_USER, '*');
+
+# Steve Rogers is the revision author
+my $steve = create_user('steverogers@avengers.org', '*', realname => 'Steve Rogers :steve');
+
+# Bucky Barns is the reviewer
+my $bucky = create_user('bucky@avengers.org', '*', realname => 'Bucky Barns :bucky');
+
+my $firefox = Bugzilla::Product->create(
+    {
+        name => 'Firefox',
+        description => 'Fake firefox product',
+        version => 'Unspecified',
+    },
+);
+
+my $general = Bugzilla::Component->create(
+    {
+        product =>$firefox,
+        name => 'General',
+        description => 'The most general description',
+        initialowner => { id => $nobody->id },
+    }
+);
+
+Bugzilla->set_user($steve);
+my $bug = Bugzilla::Bug->create(
+    {
+        short_desc   => 'test bug',
+        product      => $firefox,
+        component    => $general->name,
+        bug_severity => 'normal',
+        op_sys       => 'Unspecified',
+        rep_platform => 'Unspecified',
+        version      => 'Unspecified',
+        comment      => 'first post',
+        priority     => 'P1',
+    }
+);
+
+my $recipients = { changer => $steve };
+Bugzilla::BugMail::Send($bug->bug_id, $recipients);
+@EMAILS = ();
+
+my $revision = Bugzilla::Extension::PhabBugz::Revision->new(
+    {
+        id           => 1,
+        phid         => 'PHID-DREV-uozm3ggfp7e7uoqegmc3',
+        type         => 'DREV',
+        fields => {
+            title        => "title",
+            summary      => "the summary of the revision",
+            status       => { value => "not sure" },
+            dateCreated  => time() - (60 * 60),
+            dateModified => time() - (60 * 5),
+            authorPHID   => 'authorPHID',
+            policy       => {
+                view => 'policy.view',
+                edit => 'policy.edit',
+            },
+            'bugzilla.bug-id' => $bug->id,
+        },
+        attachments => {
+            projects => { projectPHIDs => [] },
+            reviewers => {
+                reviewers => [ ],
+            },
+            subscribers => {
+                subscriberPHIDs => [],
+                subscriberCount => 1,
+                viewerIsSubscribed => 1,
+            }
+        },
+        reviews => [
+            {
+                user => new_phab_user($bucky),
+                status => 'accepted',
+            }
+        ]
+    }
+);
+my $PhabRevisionMock = mock 'Bugzilla::Extension::PhabBugz::Revision' => (
+    override => [
+        make_public => sub { },
+        update => sub { },
+    ]
+);
+my $PhabUserMock = mock 'Bugzilla::Extension::PhabBugz::User' => (
+    override => [
+        match => sub {
+            my ($class, $query) = @_;
+            if ($query && $query->{phids} && $query->{phids}[0]) {
+                my $phid = $query->{phids}[0];
+                if ($phid eq 'authorPHID') {
+                    return [ new_phab_user($steve, $phid) ];
+                }
+            }
+        },
+    ]
+);
+
+
+my $feed    = Bugzilla::Extension::PhabBugz::Feed->new;
+my $changer = new_phab_user($bucky);
+@EMAILS = ();
+$feed->process_revision_change(
+    $revision, $changer, "story text"
+);
+
+# The first comment, and the comment made when the attachment is attached
+# are made by Steve.
+# The review comment is made by Bucky.
+
+my $sth = Bugzilla->dbh->prepare("select profiles.login_name, thetext from longdescs join profiles on who = userid");
+$sth->execute;
+while (my $row = $sth->fetchrow_hashref) {
+    if ($row->{thetext} =~ /first post/i) {
+        is($row->{login_name}, $steve->login, 'first post author');
+    }
+    elsif ($row->{thetext} =~ /the summary of the revision/i) {
+        is($row->{login_name}, $steve->login, 'the first attachment comment');
+    }
+    elsif ($row->{thetext} =~ /has approved the revision/i) {
+        is($row->{login_name}, $bucky->login);
+    }
+}
+
+diag Dumper(\@EMAILS);
+
+done_testing;
+
+sub new_phab_user {
+    my ($bug_user, $phid) = @_;
+
+    return Bugzilla::Extension::PhabBugz::User->new(
+        {
+            id => $bug_user->id * 1000,
+            type => "USER",
+            phid => $phid // "PHID-USER-" . ( $bug_user->id * 1000 ),
+            fields => {
+                username     => $bug_user->nick,
+                realName     => $bug_user->name,
+                dateCreated  => time() - 60 * 60 * 24,
+                dateModified => time(),
+                roles        => [],
+                policy       => {
+                    view => 'view',
+                    edit => 'edit',
+                },
+            },
+            attachments => {
+                'external-accounts' => {
+                    'external-accounts' => [
+                        {
+                            type => 'bmo',
+                            id   => $bug_user->id,
+                        }
+                    ]
+                }
+            }
+        }
+    );
+
+
+}
\ No newline at end of file