Revert "Bug 1440086 - Refactor PhabBugz extension code to use new User.pm module for better type checking"

This reverts commit 739676cf4b122cdec12981c2bc3a79c3f54aa7e4.

7 files changed, 176 insertions, 107 deletions

diff --git a/extensions/PhabBugz/Extension.pm b/extensions/PhabBugz/Extension.pm
index c857c60ab..ee96901a2 100644
--- a/extensions/PhabBugz/Extension.pm
+++ b/extensions/PhabBugz/Extension.pm
@@ -18,6 +18,11 @@ use Bugzilla::Extension::PhabBugz::Feed;
 
 our $VERSION = '0.01';
 
+BEGIN {
+    *Bugzilla::User::phab_phid = sub { return $_[0]->{phab_phid}; };
+    *Bugzilla::User::phab_review_status = sub { return $_[0]->{phab_review_status}; };
+}
+
 sub config_add_panels {
     my ($self, $args) = @_;
     my $modules = $args->{panel_modules};
@@ -80,7 +85,7 @@ sub install_filesystem {
     my $files = $args->{'files'};
 
     my $extensionsdir = bz_locations()->{'extensionsdir'};
-    my $scriptname = $extensionsdir . "/PhabBugz/bin/phabbugz_feed.pl";
+    my $scriptname = $extensionsdir . "/PhabBugz/bin/phabbugzd.pl";
 
     $files->{$scriptname} = {
         perms => Bugzilla::Install::Filesystem::WS_EXECUTE
diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm
index 648844055..821ec6527 100644
--- a/extensions/PhabBugz/lib/Feed.pm
+++ b/extensions/PhabBugz/lib/Feed.pm
@@ -31,6 +31,7 @@ use Bugzilla::Extension::PhabBugz::Util qw(
     add_security_sync_comments
     create_revision_attachment
     get_bug_role_phids
+    get_phab_bmo_ids
     get_project_phid
     get_security_sync_groups
     is_attachment_phab_revision
@@ -145,14 +146,10 @@ sub feed_query {
         }
 
         # Skip changes done by phab-bot user
-        my $phab_user = 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) {
+        my $phab_users = get_phab_bmo_ids({ phids => [$author_phid] });
+        if (@$phab_users) {
+            my $user = Bugzilla::User->new({ id => $phab_users->[0]->{id}, cache => 1 });
+            if ($user->login eq PHAB_AUTOMATION_USER) {
                 INFO("SKIPPING: Change made by phabricator user");
                 $self->save_last_id($story_id, 'feed');
                 next;
@@ -259,10 +256,11 @@ sub group_query {
             );
         }
 
-        INFO("Setting group members for " . $project->name);
-        my @group_members = get_group_members($group);
-        $project->set_members( \@group_members );
-        $project->update();
+        if ( my @group_members = get_group_members($group) ) {
+            INFO("Setting group members for " . $project->name);
+            $project->set_members( \@group_members );
+            $project->update();
+        }
     }
 }
 
@@ -416,28 +414,15 @@ sub process_revision_change {
     my (@accepted_phids, @denied_phids, @accepted_user_ids, @denied_user_ids);
     unless ($revision->status eq 'changes-planned' || $revision->status eq 'needs-review') {
         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';
+            push(@accepted_phids, $reviewer->phab_phid) if $reviewer->phab_review_status eq 'accepted';
+            push(@denied_phids, $reviewer->phab_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 } @$phab_users;
-    }
-
-    if ( @denied_phids ) {
-        my $phab_users = Bugzilla::Extension::PhabBugz::User->match(
-          {
-            phids => \@denied_phids
-          }
-        );
-        @denied_user_ids = map { $_->bugzilla_user->id } @$phab_users;
-    }
+    my $phab_users = get_phab_bmo_ids({ phids => \@accepted_phids });
+    @accepted_user_ids = map { $_->{id} } @$phab_users;
+    $phab_users = get_phab_bmo_ids({ phids => \@denied_phids });
+    @denied_user_ids = map { $_->{id} } @$phab_users;
 
     my %reviewers_hash =  map { $_->name => 1 } @{ $revision->reviewers };
 
@@ -727,28 +712,24 @@ sub save_last_id {
 
 sub get_group_members {
     my ($group) = @_;
-
     my $group_obj =
       ref $group ? $group : Bugzilla::Group->check( { name => $group, cache => 1 } );
     my $members_all = $group_obj->members_complete();
-
-    my @userids;
+    my %users;
     foreach my $name ( keys %$members_all ) {
         foreach my $user ( @{ $members_all->{$name} } ) {
-            push @userids, $user->id;
+            $users{ $user->id } = $user;
         }
     }
 
-    return if !@userids;
-    
     # Look up the phab ids for these users
-    my $phab_users = Bugzilla::Extension::PhabBugz::User->match(
-      {
-        ids => \@userids
-      }
-    );
+    my $phab_users = get_phab_bmo_ids( { ids => [ keys %users ] } );
+    foreach my $phab_user ( @{$phab_users} ) {
+        $users{ $phab_user->{id} }->{phab_phid} = $phab_user->{phid};
+    }
 
-    return map { $_->phid } @$phab_users;
+    # We only need users who have accounts in phabricator
+    return grep { $_->phab_phid } values %users;
 }
 
 1;
diff --git a/extensions/PhabBugz/lib/Project.pm b/extensions/PhabBugz/lib/Project.pm
index c52e1a661..cbf1bdcaf 100644
--- a/extensions/PhabBugz/lib/Project.pm
+++ b/extensions/PhabBugz/lib/Project.pm
@@ -9,14 +9,15 @@ package Bugzilla::Extension::PhabBugz::Project;
 
 use 5.10.1;
 use Moo;
-use Scalar::Util qw(blessed);
 use Types::Standard -all;
 use Type::Utils;
 
 use Bugzilla::Error;
 use Bugzilla::Util qw(trim);
-use Bugzilla::Extension::PhabBugz::User;
-use Bugzilla::Extension::PhabBugz::Util qw(request);
+use Bugzilla::Extension::PhabBugz::Util qw(
+  request
+  get_phab_bmo_ids
+);
 
 #########################
 #    Initialization     #
@@ -280,20 +281,20 @@ sub set_description {
 sub add_member {
     my ( $self, $member ) = @_;
     $self->{add_members} ||= [];
-    my $member_phid = blessed $member ? $member->phid : $member;
+    my $member_phid = blessed $member ? $member->phab_phid : $member;
     push( @{ $self->{add_members} }, $member_phid );
 }
 
 sub remove_member {
     my ( $self, $member ) = @_;
     $self->{remove_members} ||= [];
-    my $member_phid = blessed $member ? $member->phid : $member;
+    my $member_phid = blessed $member ? $member->phab_phid : $member;
     push( @{ $self->{remove_members} }, $member_phid );
 }
 
 sub set_members {
     my ( $self, $members ) = @_;
-    $self->{set_members} = [ map { blessed $_ ? $_->phid : $_ } @$members ];
+    $self->{set_members} = [ map { $_->phab_phid } @$members ];
 }
 
 sub set_policy {
@@ -317,13 +318,16 @@ sub _build_members {
 
     return [] if !@phids;
 
-    my $users = Bugzilla::Extension::PhabBugz::User->match(
-      {
-        phids => \@phids
-      }
-    );
+    my $users = get_phab_bmo_ids( { phids => \@phids } );
 
-    return [ map { $_->bugzilla_user } @$users ];
+    my @members;
+    foreach my $user (@$users) {
+        my $member = Bugzilla::User->new( { id => $user->{id}, cache => 1 } );
+        $member->{phab_phid} = $user->{phid};
+        push( @members, $member );
+    }
+
+    return \@members;
 }
 
 1;
diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm
index 950779f0d..98c3196c2 100644
--- a/extensions/PhabBugz/lib/Revision.pm
+++ b/extensions/PhabBugz/lib/Revision.pm
@@ -17,8 +17,10 @@ use Type::Utils;
 use Bugzilla::Bug;
 use Bugzilla::Error;
 use Bugzilla::Util qw(trim);
-use Bugzilla::Extension::PhabBugz::User;
-use Bugzilla::Extension::PhabBugz::Util qw(request);
+use Bugzilla::Extension::PhabBugz::Util qw(
+  get_phab_bmo_ids
+  request
+);
 
 #########################
 #    Initialization     #
@@ -282,13 +284,12 @@ sub _build_bug {
 sub _build_author {
     my ($self) = @_;
     return $self->{author} if $self->{author};
-    my $phab_user = Bugzilla::Extension::PhabBugz::User->new_from_query(
-      {
-        phids => [ $self->author_phid ]
-      }
-    );
-    if ($phab_user) {
-        return $self->{author} = $phab_user->bugzilla_user;
+    my $users = get_phab_bmo_ids( { phids => [ $self->author_phid ] } );
+    if (@$users) {
+        $self->{author} =
+          new Bugzilla::User( { id => $users->[0]->{id}, cache => 1 } );
+        $self->{author}->{phab_phid} = $self->author_phid;
+        return $self->{author};
     }
 }
 
@@ -305,22 +306,22 @@ sub _build_reviewers {
 
     return [] unless @phids;
 
-    my $users = Bugzilla::Extension::PhabBugz::User->match(
-      {
-        phids => \@phids
-      }
-    );
+    my $users = get_phab_bmo_ids( { phids => \@phids } );
 
+    my @reviewers;
     foreach my $user (@$users) {
+        my $reviewer = Bugzilla::User->new( { id => $user->{id}, cache => 1 } );
+        $reviewer->{phab_phid} = $user->{phid};
         foreach my $reviewer_data ( @{ $self->reviewers_raw } ) {
-            if ( $reviewer_data->{reviewerPHID} eq $user->phid ) {
-                $user->{phab_review_status} = $reviewer_data->{status};
+            if ( $reviewer_data->{reviewerPHID} eq $user->{phid} ) {
+                $reviewer->{phab_review_status} = $reviewer_data->{status};
                 last;
             }
         }
+        push @reviewers, $reviewer;
     }
 
-    return $self->{reviewers} = $users;
+    return \@reviewers;
 }
 
 sub _build_subscribers {
@@ -334,13 +335,19 @@ sub _build_subscribers {
         push @phids, $phid;
     }
 
-    my $users = Bugzilla::Extension::PhabBugz::User->math(
-      {
-        phids => \@phids
-      }
-    );
+    my $users = get_phab_bmo_ids( { phids => \@phids } );
+
+    return [] unless @phids;
+
+    my @subscribers;
+    foreach my $user (@$users) {
+        my $subscriber =
+          Bugzilla::User->new( { id => $user->{id}, cache => 1 } );
+        $subscriber->{phab_phid} = $user->{phid};
+        push @subscribers, $subscriber;
+    }
 
-    return $self->{subscribers} = $users;
+    return \@subscribers;
 }
 
 #########################
@@ -357,27 +364,27 @@ sub add_comment {
 sub add_reviewer {
     my ( $self, $reviewer ) = @_;
     $self->{add_reviewers} ||= [];
-    my $reviewer_phid = blessed $reviewer ? $reviewer->phid : $reviewer;
+    my $reviewer_phid = blessed $reviewer ? $reviewer->phab_phid : $reviewer;
     push @{ $self->{add_reviewers} }, $reviewer_phid;
 }
 
 sub remove_reviewer {
     my ( $self, $reviewer ) = @_;
     $self->{remove_reviewers} ||= [];
-    my $reviewer_phid = blessed $reviewer ? $reviewer->phid : $reviewer;
+    my $reviewer_phid = blessed $reviewer ? $reviewer->phab_phid : $reviewer;
     push @{ $self->{remove_reviewers} }, $reviewer_phid;
 }
 
 sub set_reviewers {
     my ( $self, $reviewers ) = @_;
-    $self->{set_reviewers} = [ map { $_->phid } @$reviewers ];
+    $self->{set_reviewers} = [ map { $_->phab_phid } @$reviewers ];
 }
 
 sub add_subscriber {
     my ( $self, $subscriber ) = @_;
     $self->{add_subscribers} ||= [];
     my $subscriber_phid =
-      blessed $subscriber ? $subscriber->phid : $subscriber;
+      blessed $subscriber ? $subscriber->phab_phid : $subscriber;
     push @{ $self->{add_subscribers} }, $subscriber_phid;
 }
 
@@ -385,7 +392,7 @@ sub remove_subscriber {
     my ( $self, $subscriber ) = @_;
     $self->{remove_subscribers} ||= [];
     my $subscriber_phid =
-      blessed $subscriber ? $subscriber->phid : $subscriber;
+      blessed $subscriber ? $subscriber->phab_phid : $subscriber;
     push @{ $self->{remove_subscribers} }, $subscriber_phid;
 }
 
diff --git a/extensions/PhabBugz/lib/User.pm b/extensions/PhabBugz/lib/User.pm
index 9d4e9eef4..3b2d87e60 100644
--- a/extensions/PhabBugz/lib/User.pm
+++ b/extensions/PhabBugz/lib/User.pm
@@ -116,14 +116,14 @@ sub match {
     my ( $class, $params ) = @_;
 
     # BMO id search takes precedence if bugzilla_ids is used.
-    my $bugzilla_ids = delete $params->{ids};
+    my $bugzilla_ids = delete $params->{bugzilla_ids};
     if ($bugzilla_ids) {
         my $bugzilla_data =
           $class->get_phab_bugzilla_ids( { ids => $bugzilla_ids } );
         $params->{phids} = [ map { $_->{phid} } @$bugzilla_data ];
     }
 
-    return [] if !@{ $params->{phids} };
+    return [] if !$params->{phids};
 
     # Look for BMO external user id in external-accounts attachment
     my $data = {
@@ -177,7 +177,6 @@ sub get_phab_bugzilla_ids {
 
         # Store new values in memcache for later retrieval
         foreach my $user ( @{ $result->{result} } ) {
-            next if !$user->{phid};
             $memcache->set(
                 {
                     key   => "phab_user_bugzilla_id_" . $user->{id},
diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm
index 916455e24..42da79c39 100644
--- a/extensions/PhabBugz/lib/Util.pm
+++ b/extensions/PhabBugz/lib/Util.pm
@@ -34,7 +34,10 @@ our @EXPORT_OK = qw(
     edit_revision_policy
     get_attachment_revisions
     get_bug_role_phids
+    get_members_by_bmo_id
+    get_members_by_phid
     get_needs_review
+    get_phab_bmo_ids
     get_project_phid
     get_revisions_by_ids
     get_revisions_by_phids
@@ -131,13 +134,7 @@ sub get_bug_role_phids {
     push(@bug_users, $bug->qa_contact) if $bug->qa_contact;
     push(@bug_users, @{ $bug->cc_users }) if @{ $bug->cc_users };
 
-    my $phab_users = Bugzilla::Extension::PhabBugz::User->match(
-      {
-        ids => [ map { $_->id } @bug_users ]
-      }
-    );
-
-    return [ map { $_->phid } @$phab_users ];
+    return get_members_by_bmo_id(\@bug_users);
 }
 
 sub create_private_revision_policy {
@@ -357,6 +354,84 @@ sub set_project_members {
     return $result->{result}{object}{phid};
 }
 
+sub get_members_by_bmo_id {
+    my $users = shift;
+
+    my $result = get_phab_bmo_ids({ ids => [ map { $_->id } @$users ] });
+
+    my @phab_ids;
+    foreach my $user (@$result) {
+        push(@phab_ids, $user->{phid})
+          if ($user->{phid} && $user->{phid} =~ /^PHID-USER/);
+    }
+
+    return \@phab_ids;
+}
+
+sub get_members_by_phid {
+    my $phids = shift;
+
+    my $result = get_phab_bmo_ids({ phids => $phids });
+
+    my @bmo_ids;
+    foreach my $user (@$result) {
+        push(@bmo_ids, $user->{id})
+          if ($user->{phid} && $user->{phid} =~ /^PHID-USER/);
+    }
+
+    return \@bmo_ids;
+}
+
+sub get_phab_bmo_ids {
+    my ($params) = @_;
+    my $memcache = Bugzilla->memcached;
+
+    # Try to find the values in memcache first
+    my @results;
+    if ($params->{ids}) {
+        my @bmo_ids = @{ $params->{ids} };
+        for (my $i = 0; $i < @bmo_ids; $i++) {
+            my $phid = $memcache->get({ key => "phab_user_bmo_id_" . $bmo_ids[$i] });
+            if ($phid) {
+                push(@results, {
+                    id   => $bmo_ids[$i],
+                    phid => $phid
+                });
+                splice(@bmo_ids, $i, 1);
+            }
+        }
+        $params->{ids} = \@bmo_ids;
+    }
+
+    if ($params->{phids}) {
+        my @phids = @{ $params->{phids} };
+        for (my $i = 0; $i < @phids; $i++) {
+            my $bmo_id = $memcache->get({ key => "phab_user_phid_" . $phids[$i] });
+            if ($bmo_id) {
+                push(@results, {
+                    id   => $bmo_id,
+                    phid => $phids[$i]
+                });
+                splice(@phids, $i, 1);
+            }
+        }
+        $params->{phids} = \@phids;
+    }
+
+    my $result = request('bugzilla.account.search', $params);
+
+    # Store new values in memcache for later retrieval
+    foreach my $user (@{ $result->{result} }) {
+        $memcache->set({ key   => "phab_user_bmo_id_" . $user->{id},
+                         value => $user->{phid} });
+        $memcache->set({ key   => "phab_user_phid_" . $user->{phid},
+                         value => $user->{id} });
+        push(@results, $user);
+    }
+
+    return \@results;
+}
+
 sub is_attachment_phab_revision {
     my ($attachment) = @_;
     return ($attachment->contenttype eq PHAB_CONTENT_TYPE
@@ -484,13 +559,9 @@ sub get_needs_review {
     $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 $ids = get_members_by_bmo_id([$user]);
+    return [] unless @$ids;
+    my $phid_user = $ids->[0];
 
     my $diffs = request(
         'differential.revision.search',
@@ -499,7 +570,7 @@ sub get_needs_review {
                 reviewers => 1,
             },
             constraints => {
-                reviewerPHIDs => [$phab_user->phid],
+                reviewerPHIDs => [$phid_user],
                 statuses      => [qw( needs-review )],
             },
             order       => 'newest',
@@ -513,7 +584,7 @@ sub get_needs_review {
     foreach my $diff (@{ $diffs->{result}{data} }) {
         my $attachments = delete $diff->{attachments};
         my $reviewers   = $attachments->{reviewers}{reviewers};
-        my $review      = first { $_->{reviewerPHID} eq $phab_user->phid } @$reviewers;
+        my $review      = first { $_->{reviewerPHID} eq $phid_user } @$reviewers;
         $diff->{fields}{review_status} = $review->{status};
         push @result, $diff;
     }
diff --git a/extensions/PhabBugz/lib/WebService.pm b/extensions/PhabBugz/lib/WebService.pm
index f018ba702..32cea1b51 100644
--- a/extensions/PhabBugz/lib/WebService.pm
+++ b/extensions/PhabBugz/lib/WebService.pm
@@ -30,6 +30,7 @@ use Bugzilla::Extension::PhabBugz::Util qw(
     create_revision_attachment
     edit_revision_policy
     get_bug_role_phids
+    get_phab_bmo_ids
     get_needs_review
     get_project_phid
     get_revisions_by_ids
@@ -306,7 +307,8 @@ sub needs_review {
 
     my $reviews = get_needs_review();
 
-    my $authors = Bugzilla::Extension::PhabBugz::User->match({
+    # map author phids to bugzilla users
+    my $author_id_map = get_phab_bmo_ids({
         phids => [
             uniq
             grep { defined }
@@ -314,9 +316,9 @@ sub needs_review {
             @$reviews
         ]
     });
-
-    my %author_phab_to_id = map { $_->phid => $_->bugzilla_user->id } @$authors;
-    my %author_id_to_user = map { $_->bugzilla_user->id => $_->bugzilla_user } @$authors;
+    my %author_phab_to_id = map { $_->{phid} => $_->{id} } @$author_id_map;
+    my $author_users      = Bugzilla::User->new_from_list([ map { $_->{id} } @$author_id_map ]);
+    my %author_id_to_user = map { $_->id => $_ } @$author_users;
 
     # bug data
     my $visible_bugs = $user->visible_bugs([