Merge remote-tracking branch 'bmo/mojo'

11 files changed, 944 insertions, 248 deletions

diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index 7d6b4e0ed..f2a440bb1 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -16,6 +16,9 @@ use List::MoreUtils qw(any uniq);
 use Moo;
 use Scalar::Util qw(blessed);
 use Try::Tiny;
+use Type::Params qw( compile );
+use Type::Utils;
+use Types::Standard qw( :types );
 
 use Bugzilla::Constants;
 use Bugzilla::Error;
@@ -24,16 +27,15 @@ use Bugzilla::Logging;
 use Bugzilla::Mailer;
 use Bugzilla::Search;
 use Bugzilla::Util qw(diff_arrays format_time with_writable_database with_readonly_database);
-
+use Bugzilla::Types qw(:types);
+use Bugzilla::Extension::PhabBugz::Types qw(:types);
 use Bugzilla::Extension::PhabBugz::Constants;
 use Bugzilla::Extension::PhabBugz::Policy;
 use Bugzilla::Extension::PhabBugz::Revision;
 use Bugzilla::Extension::PhabBugz::User;
 use Bugzilla::Extension::PhabBugz::Util qw(
-    add_security_sync_comments
     create_revision_attachment
     get_bug_role_phids
-    get_security_sync_groups
     is_attachment_phab_revision
     request
     set_phab_user
@@ -41,6 +43,8 @@ use Bugzilla::Extension::PhabBugz::Util qw(
 
 has 'is_daemon' => ( is => 'rw', default => 0 );
 
+my $Invocant = class_type { class => __PACKAGE__ };
+
 sub start {
     my ($self) = @_;
 
@@ -50,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($_);
@@ -66,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($_);
@@ -82,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($_);
@@ -145,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');
     }
 
@@ -193,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
@@ -347,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
@@ -372,17 +381,39 @@ 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);
 
-    # Pre setup before making changes
-    my $old_user = set_phab_user();
-    my $bug = Bugzilla::Bug->new({ id => $revision->bug_id, cache => 1 });
+    # change to the phabricator user, which returns a guard that restores the previous user.
+    my $restore_prev_user = set_phab_user();
+    my $bug = $revision->bug;
+
+    # Check to make sure bug id is valid and author can see it
+    if ($bug->{error}
+        ||!$revision->author->bugzilla_user->can_see_bug($revision->bug_id))
+    {
+        if ($story_text =~ /\s+created\s+D\d+/) {
+            INFO('Invalid bug ID or author does not have access to the bug. ' .
+                 'Waiting til next revision update to notify author.');
+            return;
+        }
+
+        INFO('Invalid bug ID or author does not have access to the bug');
+        my $phab_error_message = "";
+        Bugzilla->template->process('revision/comments.html.tmpl',
+                                    { message => 'invalid_bug_id' },
+                                    \$phab_error_message);
+        $revision->add_comment($phab_error_message);
+        $revision->update();
+        return;
+    }
 
     # REVISION SECURITY POLICY
 
@@ -393,48 +424,38 @@ sub process_revision_change {
     }
     # else bug is private.
     else {
-        my @set_groups = get_security_sync_groups($bug);
-
-        # If bug privacy groups do not have any matching synchronized groups,
-        # then leave revision private and it will have be dealt with manually.
-        if (!@set_groups) {
-            INFO('No matching groups. Adding comments to bug and revision');
-            add_security_sync_comments([$revision], $bug);
-        }
-        # Otherwise, we create a new custom policy containing the project
+        # Here we create a new custom policy containing the project
         # groups that are mapped to bugzilla groups.
-        else {
-            my $set_project_names = [ map { "bmo-" . $_ } @set_groups ];
-
-            # If current policy projects matches what we want to set, then
-            # we leave the current policy alone.
-            my $current_policy;
-            if ($revision->view_policy =~ /^PHID-PLCY/) {
-                INFO("Loading current policy: " . $revision->view_policy);
-                $current_policy
-                    = Bugzilla::Extension::PhabBugz::Policy->new_from_query({ phids => [ $revision->view_policy ]});
-                my $current_project_names = [ map { $_->name } @{ $current_policy->rule_projects } ];
-                INFO("Current policy projects: " . join(", ", @$current_project_names));
-                my ($added, $removed) = diff_arrays($current_project_names, $set_project_names);
-                if (@$added || @$removed) {
-                    INFO('Project groups do not match. Need new custom policy');
-                    $current_policy = undef;
-                }
-                else {
-                    INFO('Project groups match. Leaving current policy as-is');
-                }
+        my $set_project_names = [ map { "bmo-" . $_->name } @{ $bug->groups_in } ];
+
+        # If current policy projects matches what we want to set, then
+        # we leave the current policy alone.
+        my $current_policy;
+        if ($revision->view_policy =~ /^PHID-PLCY/) {
+            INFO("Loading current policy: " . $revision->view_policy);
+            $current_policy
+                = Bugzilla::Extension::PhabBugz::Policy->new_from_query({ phids => [ $revision->view_policy ]});
+            my $current_project_names = [ map { $_->name } @{ $current_policy->rule_projects } ];
+            INFO("Current policy projects: " . join(", ", @$current_project_names));
+            my ($added, $removed) = diff_arrays($current_project_names, $set_project_names);
+            if (@$added || @$removed) {
+                INFO('Project groups do not match. Need new custom policy');
+                $current_policy = undef;
             }
-
-            if (!$current_policy) {
-                INFO("Creating new custom policy: " . join(", ", @$set_project_names));
-                $revision->make_private($set_project_names);
+            else {
+                INFO('Project groups match. Leaving current policy as-is');
             }
+        }
 
-            # Subscriber list of the private revision should always match
-            # the bug roles such as assignee, qa contact, and cc members.
-            my $subscribers = get_bug_role_phids($bug);
-            $revision->set_subscribers($subscribers);
+        if (!$current_policy) {
+            INFO("Creating new custom policy: " . join(", ", @$set_project_names));
+            $revision->make_private($set_project_names);
         }
+
+        # Subscriber list of the private revision should always match
+        # the bug roles such as assignee, qa contact, and cc members.
+        my $subscribers = get_bug_role_phids($bug);
+        $revision->set_subscribers($subscribers);
     }
 
     my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()");
@@ -482,31 +503,15 @@ sub process_revision_change {
 
     # REVIEWER STATUSES
 
-    my (@accepted_phids, @denied_phids, @accepted_user_ids, @denied_user_ids);
-    foreach my $reviewer (@{ $revision->reviewers }) {
-        push(@accepted_phids, $reviewer->phid) if $reviewer->{phab_review_status} eq 'accepted';
-        push(@denied_phids, $reviewer->phid) if $reviewer->{phab_review_status} eq 'rejected';
-    }
-
-    if ( @accepted_phids ) {
-        my $phab_users = Bugzilla::Extension::PhabBugz::User->match(
-          {
-            phids => \@accepted_phids
-          }
-        );
-        @accepted_user_ids = map { $_->bugzilla_user->id } grep { defined $_->bugzilla_user } @$phab_users;
-    }
-
-    if ( @denied_phids ) {
-        my $phab_users = Bugzilla::Extension::PhabBugz::User->match(
-          {
-            phids => \@denied_phids
-          }
-        );
-        @denied_user_ids = map { $_->bugzilla_user->id } grep { defined $_->bugzilla_user } @$phab_users;
+    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 %reviewers_hash =  map { $_->name => 1 } @{ $revision->reviewers };
+    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);
@@ -534,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};
@@ -542,37 +549,55 @@ sub process_revision_change {
             push(@new_flags, { type_id => $flag_type->id, setter => $user, status => '+' });
         }
 
-        # Also add comment to for attachment update showing the user's name
-        # that changed the revision.
-        my $comment;
+        # Process each flag change by updating the flag and adding a comment
         foreach my $flag_data (@new_flags) {
-            $comment .= $flag_data->{setter}->name . " has approved the revision.\n";
+            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) {
-            $comment .= $flag_data->{setter}->name . " has requested changes to the revision.\n";
+            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) {
-            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";
+            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
+                }
+            );
         }
-
-        if ($comment) {
-            $comment .= "\n" . Bugzilla->params->{phabricator_base_uri} . "D" . $revision->id;
-            INFO("Flag comment: $comment");
-            # Add transaction_id as anchor if one present
-            # $comment .= "#" . $params->{transaction_id} if $params->{transaction_id};
-            $bug->add_comment($comment, {
-                isprivate  => $attachment->isprivate,
-                type       => CMT_ATTACHMENT_UPDATED,
-                extra_data => $attachment->id
-            });
-        }
-
-        $attachment->set_flags([ @denied_flags, @removed_flags ], \@new_flags);
-        $attachment->update($timestamp);
     }
 
     # FINISH UP
@@ -583,16 +608,15 @@ 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 });
     }
 
-    Bugzilla->set_user($old_user);
-
     INFO('SUCCESS: Revision D' . $revision->id . ' processed');
 }
 
 sub process_new_user {
-    my ( $self, $user_data ) = @_;
+    state $check = compile($Invocant, HashRef);
+    my ( $self, $user_data ) = $check->(@_);
 
     # Load the user data into a proper object
     my $phab_user = Bugzilla::Extension::PhabBugz::User->new($user_data);
@@ -605,7 +629,7 @@ sub process_new_user {
     my $bug_user  = $phab_user->bugzilla_user;
 
     # Pre setup before querying DB
-    my $old_user = set_phab_user();
+    my $restore_prev_user = set_phab_user();
 
     # CHECK AND WARN FOR POSSIBLE USERNAME SQUATTING
     INFO("Checking for username squatters");
@@ -688,7 +712,7 @@ sub process_new_user {
         # that are connected to revisions
         f11 => 'attachments.filename',
         o11 => 'regexp',
-        v11 => '^phabricator-D[[:digit:]]+-url[[.period.]]txt$',
+        v11 => '^phabricator-D[[:digit:]]+-url.txt$',
     };
 
     my $search = Bugzilla::Search->new( fields => [ 'bug_id' ],
@@ -724,8 +748,6 @@ sub process_new_user {
         }
     }
 
-    Bugzilla->set_user($old_user);
-
     INFO('SUCCESS: User ' . $phab_user->id . ' processed');
 }
 
@@ -793,8 +815,8 @@ sub save_last_id {
 }
 
 sub get_group_members {
-    my ( $self, $group ) = @_;
-
+    state $check = compile( $Invocant, Group | Str );
+    my ( $self, $group ) = $check->(@_);
     my $group_obj =
       ref $group ? $group : Bugzilla::Group->check( { name => $group, cache => 1 } );
 
@@ -817,4 +839,38 @@ 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/Policy.pm b/extensions/PhabBugz/lib/Policy.pm
index a86c83036..415ea20fb 100644
--- a/extensions/PhabBugz/lib/Policy.pm
+++ b/extensions/PhabBugz/lib/Policy.pm
@@ -13,11 +13,13 @@ use Moo;
 use Bugzilla::Error;
 use Bugzilla::Extension::PhabBugz::Util qw(request);
 use Bugzilla::Extension::PhabBugz::Project;
+use Bugzilla::Extension::PhabBugz::Types qw(:types);
 
 use List::Util qw(first);
 
 use Types::Standard -all;
 use Type::Utils;
+use Type::Params qw( compile );
 
 has 'phid'      => ( is => 'ro', isa => Str );
 has 'type'      => ( is => 'ro', isa => Str );
@@ -41,7 +43,7 @@ has 'rules' => (
 
 has 'rule_projects' => (
     is => 'lazy',
-    isa => ArrayRef[Object],
+    isa => ArrayRef[Project],
 );
 
 # {
@@ -79,8 +81,11 @@ has 'rule_projects' => (
 #   }
 # }
 
+my $Invocant = class_type { class => __PACKAGE__ };
+
 sub new_from_query {
-    my ($class, $params) = @_;
+    state $check = compile($Invocant | ClassName, Dict[phids => ArrayRef[Str]]);
+    my ($class, $params) = $check->(@_);
     my $result = request('policy.query', $params);
     if (exists $result->{result}{data} && @{ $result->{result}{data} }) {
         return $class->new($result->{result}->{data}->[0]);
@@ -88,7 +93,8 @@ sub new_from_query {
 }
 
 sub create {
-    my ($class, $projects) = @_;
+    state $check = compile($Invocant | ClassName, ArrayRef[Project]);
+    my ($class, $projects) = $check->(@_);
 
     my $data = {
         objectType => 'DREV',
diff --git a/extensions/PhabBugz/lib/Project.pm b/extensions/PhabBugz/lib/Project.pm
index b93a6eb9e..c18708887 100644
--- a/extensions/PhabBugz/lib/Project.pm
+++ b/extensions/PhabBugz/lib/Project.pm
@@ -12,10 +12,12 @@ use Moo;
 use Scalar::Util qw(blessed);
 use Types::Standard -all;
 use Type::Utils;
+use Type::Params qw( compile );
 
 use Bugzilla::Error;
 use Bugzilla::Util qw(trim);
 use Bugzilla::Extension::PhabBugz::User;
+use Bugzilla::Extension::PhabBugz::Types qw(:types);
 use Bugzilla::Extension::PhabBugz::Util qw(request);
 
 #########################
@@ -33,7 +35,9 @@ has view_policy     => ( is => 'ro', isa => Str );
 has edit_policy     => ( is => 'ro', isa => Str );
 has join_policy     => ( is => 'ro', isa => Str );
 has members_raw     => ( is => 'ro', isa => ArrayRef [ Dict [ phid => Str ] ] );
-has members => ( is => 'lazy', isa => ArrayRef [Object] );
+has members         => ( is => 'lazy', isa => ArrayRef[PhabUser] );
+
+my $Invocant = class_type { class => __PACKAGE__ };
 
 sub new_from_query {
     my ( $class, $params ) = @_;
@@ -142,12 +146,20 @@ sub BUILDARGS {
 #########################
 
 sub create {
-    my ( $class, $params ) = @_;
-
-    my $name = trim( $params->{name} );
-    $name || ThrowCodeError( 'param_required', { param => 'name' } );
+    state $check = compile(
+        $Invocant | ClassName,
+        Dict[
+            name => Str,
+            description => Str,
+            view_policy => Str,
+            edit_policy => Str,
+            join_policy => Str,
+        ]
+    );
+    my ( $class, $params ) = $check->(@_);
 
-    my $description = $params->{description} || 'Need description';
+    my $name        = trim($params->{name});
+    my $description = $params->{description};
     my $view_policy = $params->{view_policy};
     my $edit_policy = $params->{edit_policy};
     my $join_policy = $params->{join_policy};
@@ -324,5 +336,4 @@ sub _build_members {
     );
 }
 
-1;
-
+1;
\ No newline at end of file
diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm
index 4e82fa500..6ad906829 100644
--- a/extensions/PhabBugz/lib/Revision.pm
+++ b/extensions/PhabBugz/lib/Revision.pm
@@ -15,10 +15,12 @@ use Types::Standard -all;
 use Type::Utils;
 
 use Bugzilla::Bug;
+use Bugzilla::Types qw(JSONBool);
 use Bugzilla::Error;
 use Bugzilla::Util qw(trim);
 use Bugzilla::Extension::PhabBugz::Project;
 use Bugzilla::Extension::PhabBugz::User;
+use Bugzilla::Extension::PhabBugz::Types qw(:types);
 use Bugzilla::Extension::PhabBugz::Util qw(request);
 
 #########################
@@ -39,16 +41,16 @@ has edit_policy      => ( is => 'ro',   isa => Str );
 has subscriber_count => ( is => 'ro',   isa => Int );
 has bug              => ( is => 'lazy', isa => Object );
 has author           => ( is => 'lazy', isa => Object );
-has reviewers        => ( is => 'lazy', isa => ArrayRef [Object] );
-has subscribers      => ( is => 'lazy', isa => ArrayRef [Object] );
-has projects         => ( is => 'lazy', isa => ArrayRef [Object] );
+has reviews          => ( is => 'lazy', isa => ArrayRef [ Dict [ user => PhabUser, status => Str ] ] );
+has subscribers      => ( is => 'lazy', isa => ArrayRef [PhabUser] );
+has projects         => ( is => 'lazy', isa => ArrayRef [Project] );
 has reviewers_raw => (
     is  => 'ro',
     isa => ArrayRef [
         Dict [
             reviewerPHID => Str,
             status       => Str,
-            isBlocking   => Bool,
+            isBlocking   => Bool | JSONBool,
             actorPHID    => Maybe [Str],
         ],
     ]
@@ -58,7 +60,7 @@ has subscribers_raw => (
     isa => Dict [
         subscriberPHIDs => ArrayRef [Str],
         subscriberCount => Int,
-        viewerIsSubscribed => Bool,
+        viewerIsSubscribed => Bool | JSONBool,
     ]
 );
 has projects_raw => (
@@ -109,7 +111,7 @@ sub BUILDARGS {
     $params->{bug_id}           = $params->{fields}->{'bugzilla.bug-id'};
     $params->{view_policy}      = $params->{fields}->{policy}->{view};
     $params->{edit_policy}      = $params->{fields}->{policy}->{edit};
-    $params->{reviewers_raw}    = $params->{attachments}->{reviewers}->{reviewers};
+    $params->{reviewers_raw}    = $params->{attachments}->{reviewers}->{reviewers} // [];
     $params->{subscribers_raw}  = $params->{attachments}->{subscribers};
     $params->{projects_raw}     = $params->{attachments}->{projects};
     $params->{subscriber_count} =
@@ -301,35 +303,24 @@ sub _build_author {
     }
 }
 
-sub _build_reviewers {
+sub _build_reviews {
     my ($self) = @_;
 
-    return $self->{reviewers} if $self->{reviewers};
-    return [] unless $self->reviewers_raw;
-
-    my @phids;
-    foreach my $reviewer ( @{ $self->reviewers_raw } ) {
-        push @phids, $reviewer->{reviewerPHID};
-    }
-
-    return [] unless @phids;
-
+    my %by_phid = map { $_->{reviewerPHID} => $_ } @{ $self->reviewers_raw };
     my $users = Bugzilla::Extension::PhabBugz::User->match(
-      {
-        phids => \@phids
-      }
+        {
+            phids => [keys %by_phid]
+        }
     );
 
-    foreach my $user (@$users) {
-        foreach my $reviewer_data ( @{ $self->reviewers_raw } ) {
-            if ( $reviewer_data->{reviewerPHID} eq $user->phid ) {
-                $user->{phab_review_status} = $reviewer_data->{status};
-                last;
+    return [
+        map {
+            {
+                user => $_,
+                status => $by_phid{ $_->phid }{status},
             }
-        }
-    }
-
-    return $self->{reviewers} = $users;
+        } @$users
+    ];
 }
 
 sub _build_subscribers {
@@ -478,8 +469,14 @@ sub make_private {
 sub make_public {
     my ( $self ) = @_;
 
-    $self->set_policy('view', 'public');
-    $self->set_policy('edit', 'users');
+    my $editbugs = Bugzilla::Extension::PhabBugz::Project->new_from_query(
+        {
+            name => 'bmo-editbugs-team'
+        }
+    );
+
+    $self->set_policy( 'view', 'public' );
+    $self->set_policy( 'edit', ( $editbugs ? $editbugs->phid : 'users' ) );
 
     my @current_group_projects = grep { $_->name =~ /^(bmo-.*|secure-revision)$/ } @{ $self->projects };
     foreach my $project (@current_group_projects) {
diff --git a/extensions/PhabBugz/lib/Types.pm b/extensions/PhabBugz/lib/Types.pm
new file mode 100644
index 000000000..493e97fbc
--- /dev/null
+++ b/extensions/PhabBugz/lib/Types.pm
@@ -0,0 +1,28 @@
+# 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.
+
+package Bugzilla::Extension::PhabBugz::Types;
+
+use 5.10.1;
+use strict;
+use warnings;
+
+use Type::Library
+    -base,
+    -declare => qw( Revision LinkedPhabUser PhabUser Policy Project );
+use Type::Utils -all;
+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/User.pm b/extensions/PhabBugz/lib/User.pm
index da573be37..209425bdf 100644
--- a/extensions/PhabBugz/lib/User.pm
+++ b/extensions/PhabBugz/lib/User.pm
@@ -11,12 +11,13 @@ use 5.10.1;
 use Moo;
 
 use Bugzilla::User;
-
+use Bugzilla::Types qw(:types);
 use Bugzilla::Extension::PhabBugz::Util qw(request);
 
 use List::Util qw(first);
 use Types::Standard -all;
 use Type::Utils;
+use Type::Params qw(compile);
 
 #########################
 #    Initialization     #
@@ -33,7 +34,9 @@ has 'roles'           => ( is => 'ro', isa => ArrayRef [Str] );
 has 'view_policy'     => ( is => 'ro', isa => Str );
 has 'edit_policy'     => ( is => 'ro', isa => Str );
 has 'bugzilla_id'     => ( is => 'ro', isa => Maybe [Int] );
-has 'bugzilla_user'   => ( is => 'lazy' );
+has 'bugzilla_user'   => ( is => 'lazy', isa => Maybe [User] );
+
+my $Invocant = class_type { class => __PACKAGE__ };
 
 sub BUILDARGS {
     my ( $class, $params ) = @_;
@@ -113,7 +116,8 @@ sub new_from_query {
 }
 
 sub match {
-    my ( $class, $params ) = @_;
+    state $check = compile( $Invocant | ClassName, Dict[ ids => ArrayRef[Int] ] | Dict[ phids => ArrayRef[Str] ] );
+    my ( $class, $params ) = $check->(@_);
 
     # BMO id search takes precedence if bugzilla_ids is used.
     my $bugzilla_ids = delete $params->{ids};
@@ -158,7 +162,8 @@ sub _build_bugzilla_user {
 }
 
 sub get_phab_bugzilla_ids {
-    my ( $class, $params ) = @_;
+    state $check = compile($Invocant | ClassName, Dict[ids => ArrayRef[Int]]);
+    my ( $class, $params ) = $check->(@_);
 
     my $memcache = Bugzilla->memcached;
 
diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
index 5ad8a5207..a93533e75 100644
--- a/extensions/PhabBugz/lib/Util.pm
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -15,24 +15,27 @@ use Bugzilla::Bug;
 use Bugzilla::Constants;
 use Bugzilla::Error;
 use Bugzilla::User;
+use Bugzilla::Types qw(:types);
 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 base qw(Exporter);
 
 our @EXPORT = qw(
-    add_security_sync_comments
     create_revision_attachment
     get_attachment_revisions
     get_bug_role_phids
     get_needs_review
-    get_security_sync_groups
     intersect
     is_attachment_phab_revision
     request
@@ -40,7 +43,8 @@ our @EXPORT = qw(
 );
 
 sub create_revision_attachment {
-    my ( $bug, $revision, $timestamp, $submitter ) = @_;
+    state $check = compile(Bug, Revision, Str, User);
+    my ( $bug, $revision, $timestamp, $submitter ) = $check->(@_);
 
     my $phab_base_uri = Bugzilla->params->{phabricator_base_uri};
     ThrowUserError('invalid_phabricator_uri') unless $phab_base_uri;
@@ -61,37 +65,27 @@ sub create_revision_attachment {
     }
 
     # If submitter, then switch to that user when creating attachment
-    my ($old_user, $attachment);
-    try {
-        if ($submitter) {
-            $old_user = Bugzilla->user;
-            $submitter->{groups} = [ Bugzilla::Group->get_all ]; # We need to always be able to add attachment
-            Bugzilla->set_user($submitter);
+    local $submitter->{groups} = [ Bugzilla::Group->get_all ]; # We need to always be able to add attachment
+    my $restore_prev_user = Bugzilla->set_user($submitter, scope_guard => 1);
+
+    my $attachment = Bugzilla::Attachment->create(
+        {
+            bug         => $bug,
+            creation_ts => $timestamp,
+            data        => $revision_uri,
+            description => $revision->title,
+            filename    => 'phabricator-D' . $revision->id . '-url.txt',
+            ispatch     => 0,
+            isprivate   => 0,
+            mimetype    => PHAB_CONTENT_TYPE,
         }
+    );
 
-        $attachment = Bugzilla::Attachment->create(
-            {
-                bug         => $bug,
-                creation_ts => $timestamp,
-                data        => $revision_uri,
-                description => $revision->title,
-                filename    => 'phabricator-D' . $revision->id . '-url.txt',
-                ispatch     => 0,
-                isprivate   => 0,
-                mimetype    => PHAB_CONTENT_TYPE,
-            }
-        );
+    # Insert a comment about the new attachment into the database.
+    $bug->add_comment($revision->summary, { type       => CMT_ATTACHMENT_CREATED,
+                                            extra_data => $attachment->id });
 
-        # Insert a comment about the new attachment into the database.
-        $bug->add_comment($revision->summary, { type       => CMT_ATTACHMENT_CREATED,
-                                                extra_data => $attachment->id });
-    }
-    catch {
-        die $_;
-    }
-    finally {
-        Bugzilla->set_user($old_user) if $old_user;
-    };
+    delete $bug->{attachments};
 
     return $attachment;
 }
@@ -103,7 +97,8 @@ sub intersect {
 }
 
 sub get_bug_role_phids {
-    my ($bug) = @_;
+    state $check = compile(Bug);
+    my ($bug) = $check->(@_);
 
     my @bug_users = ( $bug->reporter );
     push(@bug_users, $bug->assigned_to)
@@ -122,12 +117,14 @@ sub get_bug_role_phids {
 }
 
 sub is_attachment_phab_revision {
-    my ($attachment) = @_;
+    state $check = compile(Attachment);
+    my ($attachment) = $check->(@_);
     return $attachment->contenttype eq PHAB_CONTENT_TYPE;
 }
 
 sub get_attachment_revisions {
-    my $bug = shift;
+    state $check = compile(Bug);
+    my ($bug) = $check->(@_);
 
     my @attachments =
       grep { is_attachment_phab_revision($_) } @{ $bug->attachments() };
@@ -156,7 +153,8 @@ sub get_attachment_revisions {
 }
 
 sub request {
-    my ($method, $data) = @_;
+    state $check = compile(Str, HashRef);
+    my ($method, $data) = $check->(@_);
     my $request_cache = Bugzilla->request_cache;
     my $params        = Bugzilla->params;
 
@@ -201,49 +199,11 @@ sub request {
     return $result;
 }
 
-sub get_security_sync_groups {
-    my $bug = shift;
-
-    my $sync_groups = Bugzilla::Group->match( { isactive => 1, isbuggroup => 1 } );
-    my $sync_group_names = [ map { $_->name } @$sync_groups ]; 
-
-    my $bug_groups = $bug->groups_in;
-    my $bug_group_names = [ map { $_->name } @$bug_groups ];
-
-    my @set_groups = intersect($bug_group_names, $sync_group_names);
-
-    return @set_groups;
-}
-
 sub set_phab_user {
-    my $old_user = Bugzilla->user;
     my $user = Bugzilla::User->new( { name => PHAB_AUTOMATION_USER } );
     $user->{groups} = [ Bugzilla::Group->get_all ];
-    Bugzilla->set_user($user);
-    return $old_user;
-}
-
-sub add_security_sync_comments {
-    my ($revisions, $bug) = @_;
-
-    my $phab_error_message = 'Revision is being made private due to unknown Bugzilla groups.';
-
-    foreach my $revision (@$revisions) {
-        $revision->add_comment($phab_error_message);
-    }
-
-    my $num_revisions = scalar @$revisions;
-    my $bmo_error_message =
-    ( $num_revisions > 1
-    ? $num_revisions.' revisions were'
-    : 'One revision was' )
-    . ' made private due to unknown Bugzilla groups.';
-
-    my $old_user = set_phab_user();
-
-    $bug->add_comment( $bmo_error_message, { isprivate => 0 } );
 
-    Bugzilla->set_user($old_user);
+    return Bugzilla->set_user($user, scope_guard => 1);
 }
 
 sub get_needs_review {
diff --git a/extensions/PhabBugz/t/basic.t b/extensions/PhabBugz/t/basic.t
new file mode 100644
index 000000000..9a6723ccb
--- /dev/null
+++ b/extensions/PhabBugz/t/basic.t
@@ -0,0 +1,250 @@
+#!/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 );
+use Bugzilla;
+
+BEGIN { Bugzilla->extensions };
+
+use Test::More;
+use Test2::Tools::Mock;
+use Data::Dumper;
+use JSON::MaybeXS;
+use Carp;
+use Try::Tiny;
+
+use ok 'Bugzilla::Extension::PhabBugz::Feed';
+use ok 'Bugzilla::Extension::PhabBugz::Util', qw( get_attachment_revisions );
+can_ok('Bugzilla::Extension::PhabBugz::Feed', 'group_query');
+
+our @group_members;
+our @project_members;
+
+
+my $User = mock 'Bugzilla::Extension::PhabBugz::User' => (
+    add_constructor => [
+        'fake_new' => 'hash',
+    ],
+    override => [
+        'match' => sub { [ mock() ] },
+    ],
+);
+
+my $Feed = mock 'Bugzilla::Extension::PhabBugz::Feed' => (
+    override => [
+        get_group_members => sub {
+            return [ map { Bugzilla::Extension::PhabBugz::User->fake_new(%$_) } @group_members ];
+        }
+    ]
+);
+
+my $Project = mock 'Bugzilla::Extension::PhabBugz::Project' => (
+    override_constructor => [
+        new_from_query => 'ref_copy',
+    ],
+    override => [
+        'members' => sub {
+            return [ map { Bugzilla::Extension::PhabBugz::User->fake_new(%$_) } @project_members ];
+        }
+    ]
+);
+
+local Bugzilla->params->{phabricator_enabled} = 1;
+local Bugzilla->params->{phabricator_api_key} = 'FAKE-API-KEY';
+local Bugzilla->params->{phabricator_base_uri} = 'http://fake.fabricator.tld';
+
+my $Bugzilla = mock 'Bugzilla' => (
+    override => [
+        'dbh'  => sub { mock() },
+        'user' => sub { Bugzilla::User->new({ name => 'phab-bot@bmo.tld' }) },
+    ],
+);
+
+my $BugzillaGroup = mock 'Bugzilla::Group' => (
+    add_constructor => [
+        'fake_new' => 'hash',
+    ],
+    override => [
+        'match' => sub { [ Bugzilla::Group->fake_new(id => 1, name => 'firefox-security' ) ] },
+    ],
+);
+
+my $BugzillaUser = mock 'Bugzilla::User' => (
+    add_constructor => [
+        'fake_new' => 'hash',
+    ],
+    override => [
+        'new' => sub {
+            my ($class, $hash) = @_;
+            if ($hash->{name} eq 'phab-bot@bmo.tld') {
+                return $class->fake_new( id => 8_675_309, login_name => 'phab-bot@bmo.tld', realname => 'Fake PhabBot' );
+            }
+            else {
+            }
+        },
+        'match' => sub { [ mock() ] },
+    ],
+);
+
+
+my $feed = Bugzilla::Extension::PhabBugz::Feed->new;
+
+# Same members in both
+do {
+    my $UserAgent = mock 'LWP::UserAgent' => (
+        override => [
+            'post' => sub {
+                my ($self, $url, $params) = @_;
+                my $data = decode_json($params->{params});
+                is_deeply($data->{transactions}, [], 'no-op');
+                return mock({is_error => 0, content => '{}'});
+            },
+        ],
+    );
+    local @group_members = (
+        { phid => 'foo' },
+    );
+    local @project_members = (
+        { phid => 'foo' },
+    );
+    $feed->group_query;
+};
+
+# Project has members not in group
+do {
+    my $UserAgent = mock 'LWP::UserAgent' => (
+        override => [
+            'post' => sub {
+                my ($self, $url, $params) = @_;
+                my $data = decode_json($params->{params});
+                my $expected = [ { type => 'members.remove', value => ['foo'] } ];
+                is_deeply($data->{transactions}, $expected, 'remove foo');
+                return mock({is_error => 0, content => '{}'});
+            },
+        ]
+    );
+    local @group_members = ();
+    local @project_members = (
+        { phid => 'foo' },
+    );
+    $feed->group_query;
+};
+
+# Group has members not in project
+do {
+    my $UserAgent = mock 'LWP::UserAgent' => (
+        override => [
+            'post' => sub {
+                my ($self, $url, $params) = @_;
+                my $data = decode_json($params->{params});
+                my $expected = [ { type => 'members.add', value => ['foo'] } ];
+                is_deeply($data->{transactions}, $expected, 'add foo');
+                return mock({is_error => 0, content => '{}'});
+            },
+        ]
+    );
+    local @group_members = (
+        { phid => 'foo' },
+    );
+    local @project_members = (
+    );
+    $feed->group_query;
+};
+
+do {
+    my $Revision  = mock 'Bugzilla::Extension::PhabBugz::Revision' => (
+        override => [
+            'update' => sub { 1 },
+        ],
+    );
+    my $UserAgent = mock 'LWP::UserAgent' => (
+        override => [
+            'post' => sub {
+                my ($self, $url, $params) = @_;
+                if ($url =~ /differential\.revision\.search/) {
+                    my $content = <<JSON;
+{
+    "error_info": null,
+    "error_code": null,
+    "result": {
+        "data": [
+            {
+                "id": 9999,
+                "type": "DREV",
+                "phid": "PHID-DREV-uozm3ggfp7e7uoqegmc3",
+                "fields": {
+                    "title": "Added .arcconfig",
+                    "summary": "Added .arcconfig",
+                    "authorPHID": "PHID-USER-4wigy3sh5fc5t74vapwm",
+                    "dateCreated": 1507666113,
+                    "dateModified": 1508514027,
+                    "policy": {
+                        "view": "public",
+                        "edit": "admin"
+                    },
+                    "bugzilla.bug-id": "23",
+                    "status": {
+                        "value": "needs-review",
+                        "name": "Needs Review",
+                        "closed": false,
+                        "color.ansi": "magenta"
+                    }
+                },
+                "attachments": {
+                    "reviewers": {
+                        "reviewers": []
+                    },
+                    "subscribers": {
+                        "subscriberPHIDs": [],
+                        "subscriberCount": 0,
+                        "viewerIsSubscribed": true
+                    },
+                    "projects": {
+                        "projectPHIDs": []
+                    }
+                }
+            }
+        ]
+    }
+}
+JSON
+                    return mock { is_error => 0, content => $content };
+                }
+                else {
+                    return mock { is_error => 1, message => "bad request" };
+                }
+            },
+        ],
+    );
+    my $Attachment = mock 'Bugzilla::Attachment' => (
+        add_constructor => [ fake_new => 'hash' ],
+    );
+    my $Bug = mock 'Bugzilla::Bug' => (
+        add_constructor => [ fake_new => 'hash' ],
+    );
+    my $bug = Bugzilla::Bug->fake_new(
+        bug_id      => 23,
+        attachments => [
+            Bugzilla::Attachment->fake_new(
+                mimetype => 'text/x-phabricator-request',
+                filename => 'phabricator-D9999-url.txt',
+            ),
+        ]
+    );
+
+    my $revisions = get_attachment_revisions($bug);
+    is(ref($revisions), 'ARRAY', 'it is an array ref');
+    isa_ok($revisions->[0], 'Bugzilla::Extension::PhabBugz::Revision');
+    is($revisions->[0]->bug_id, 23, 'Bugzila ID is 23');
+    ok( try { $revisions->[0]->update }, 'update revision');
+
+};
+
+done_testing;
diff --git a/extensions/PhabBugz/t/feed-daemon-guts.t b/extensions/PhabBugz/t/feed-daemon-guts.t
new file mode 100644
index 000000000..376af18e4
--- /dev/null
+++ b/extensions/PhabBugz/t/feed-daemon-guts.t
@@ -0,0 +1,160 @@
+#!/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 Bugzilla::Test::MockDB;
+use Bugzilla::Test::MockParams;
+use Bugzilla::Test::Util qw(create_user);
+use Test::More;
+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 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 ));
+
+Bugzilla->error_mode(ERROR_MODE_TEST);
+
+my $phab_bot = create_user(PHAB_AUTOMATION_USER, '*');
+
+my $UserAgent = mock 'LWP::UserAgent' => ();
+
+{
+    SetParam('phabricator_enabled', 0);
+    my $feed = Bugzilla::Extension::PhabBugz::Feed->new;
+    my $Feed = mock 'Bugzilla::Extension::PhabBugz::Feed' => (
+        override => [
+            get_last_id => sub { die "get_last_id" },
+        ],
+    );
+
+    foreach my $method (qw( feed_query user_query group_query )) {
+        try {
+            $feed->$method;
+            pass "disabling the phabricator sync: $method";
+        }
+        catch {
+            fail "disabling the phabricator sync: $method";
+        }
+    }
+}
+
+my @bad_response = (
+    ['http error', mock({ is_error => 1, message => 'some http error' }) ],
+    ['invalid json', mock({ is_error => 0, content => '<xml>foo</xml>' })],
+    ['json containing error code', mock({ is_error => 0, content => encode_json({error_code => 1234 }) })],
+);
+
+SetParam(phabricator_enabled => 1);
+SetParam(phabricator_api_key => 'FAKE-API-KEY');
+SetParam(phabricator_base_uri => 'http://fake.fabricator.tld/');
+
+foreach my $bad_response (@bad_response) {
+    my $feed = Bugzilla::Extension::PhabBugz::Feed->new;
+    $UserAgent->override(
+        post => sub {
+            my ( $self, $url, $params ) = @_;
+            return $bad_response->[1];
+        }
+    );
+
+    foreach my $method (qw( feed_query user_query group_query )) {
+        try {
+            # This is a hack to get reasonable exception objects.
+            local $Bugzilla::Template::is_processing = 1;
+            $feed->$method;
+            fail "$method - $bad_response->[0]";
+        }
+        catch {
+            is( $_->type, 'bugzilla.code.phabricator_api_error', "$method - $bad_response->[0]" );
+        };
+    }
+    $UserAgent->reset('post');
+}
+
+my $feed      = Bugzilla::Extension::PhabBugz::Feed->new;
+my $json      = JSON::MaybeXS->new( canonical => 1, pretty => 1 );
+my $dylan     = create_user( 'dylan@mozilla.com', '*', realname => 'Dylan Hardison :dylan' );
+my $evildylan = create_user( 'dylan@gmail.com', '*', realname => 'Evil Dylan :dylan' );
+my $myk       = create_user( 'myk@mozilla.com', '*', realname => 'Myk Melez :myk' );
+
+my $phab_bot_phid = next_phid('PHID-USER');
+
+done_testing;
+
+sub user_search {
+    my (%conf) = @_;
+
+    return {
+        error_info => undef,
+        error_code => undef,
+        result     => {
+            cursor => {
+                after  => $conf{after},
+                order  => undef,
+                limit  => 100,
+                before => undef
+            },
+            query => {
+                queryKey => undef
+            },
+            maps => {},
+            data => [
+                map {
+                    +{
+                        attachments => {
+                            $_->{bmo_id}
+                            ? ( "external-accounts" => {
+                                    "external-accounts" => [
+                                        {
+                                            type => 'bmo',
+                                            id   => $_->{bmo_id},
+                                        }
+                                    ]
+                                }
+                              )
+                            : (),
+                        },
+                        fields => {
+                            roles        => [ "verified", "approved", "activated" ],
+                            realName     => $_->{realname},
+                            dateModified => time,
+                            policy       => {
+                                view => "public",
+                                edit => "no-one"
+                            },
+                            dateCreated => time,
+                            username    => $_->{username},
+                        },
+                        phid => next_phid("PHID-USER"),
+                        type => "USER",
+                        id   => $_->{phab_id},
+                      },
+                } @{ $conf{users} },
+            ]
+        }
+    };
+
+}
+
+sub next_phid {
+    my ($prefix) = @_;
+    state $number = 'a' x 20;
+    return $prefix . '-' .  ($number++);
+}
+
+
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
diff --git a/extensions/PhabBugz/template/en/default/revision/comments.html.tmpl b/extensions/PhabBugz/template/en/default/revision/comments.html.tmpl
new file mode 100644
index 000000000..b18daf376
--- /dev/null
+++ b/extensions/PhabBugz/template/en/default/revision/comments.html.tmpl
@@ -0,0 +1,14 @@
+[%# 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.
+  #%]
+
+[% IF message == "invalid_bug_id" %]
+Revision is being kept private due to invalid [% terms.bug %] ID
+or author does not have access to the [% terms.bug %]. Either remove
+the [% terms.bug %] ID, automatically making the revision public, or
+enter the correct [% terms.bug %] ID for this revision.
+[% END %]
\ No newline at end of file