From a1d7038494ee5fb327b21b48c7d1e5fb75b39e72 Mon Sep 17 00:00:00 2001 From: dklawren Date: Fri, 11 May 2018 13:10:09 -0400 Subject: Bug 1440086 - Refactor PhabBugz extension code to use new User.pm module for better type checking --- extensions/PhabBugz/Extension.pm | 7 +-- extensions/PhabBugz/lib/Feed.pm | 67 ++++++++++++++-------- extensions/PhabBugz/lib/Project.pm | 28 ++++----- extensions/PhabBugz/lib/Revision.pm | 63 +++++++++------------ extensions/PhabBugz/lib/User.pm | 5 +- extensions/PhabBugz/lib/Util.pm | 103 ++++++---------------------------- extensions/PhabBugz/lib/WebService.pm | 10 ++-- 7 files changed, 107 insertions(+), 176 deletions(-) diff --git a/extensions/PhabBugz/Extension.pm b/extensions/PhabBugz/Extension.pm index ee96901a2..c857c60ab 100644 --- a/extensions/PhabBugz/Extension.pm +++ b/extensions/PhabBugz/Extension.pm @@ -18,11 +18,6 @@ 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}; @@ -85,7 +80,7 @@ sub install_filesystem { my $files = $args->{'files'}; my $extensionsdir = bz_locations()->{'extensionsdir'}; - my $scriptname = $extensionsdir . "/PhabBugz/bin/phabbugzd.pl"; + my $scriptname = $extensionsdir . "/PhabBugz/bin/phabbugz_feed.pl"; $files->{$scriptname} = { perms => Bugzilla::Install::Filesystem::WS_EXECUTE diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index a0012cc10..a7bb75148 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -31,7 +31,6 @@ 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 @@ -146,10 +145,14 @@ sub feed_query { } # Skip changes done by phab-bot 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) { + 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) { INFO("SKIPPING: Change made by phabricator user"); $self->save_last_id($story_id, 'feed'); next; @@ -256,11 +259,10 @@ sub group_query { ); } - if ( my @group_members = get_group_members($group) ) { - INFO("Setting group members for " . $project->name); - $project->set_members( \@group_members ); - $project->update(); - } + INFO("Setting group members for " . $project->name); + my @group_members = get_group_members($group); + $project->set_members( \@group_members ); + $project->update(); } } @@ -413,14 +415,27 @@ sub process_revision_change { my (@accepted_phids, @denied_phids, @accepted_user_ids, @denied_user_ids); foreach my $reviewer (@{ $revision->reviewers }) { - 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'; + push(@accepted_phids, $reviewer->phid) if $reviewer->{phab_review_status} eq 'accepted'; + push(@denied_phids, $reviewer->phid) if $reviewer->{phab_review_status} eq 'rejected'; } - 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; + 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 %reviewers_hash = map { $_->name => 1 } @{ $revision->reviewers }; @@ -710,24 +725,28 @@ 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 %users; + + my @userids; foreach my $name ( keys %$members_all ) { foreach my $user ( @{ $members_all->{$name} } ) { - $users{ $user->id } = $user; + push @userids, $user->id; } } + return if !@userids; + # Look up the phab ids for these users - 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}; - } + my $phab_users = Bugzilla::Extension::PhabBugz::User->match( + { + ids => \@userids + } + ); - # We only need users who have accounts in phabricator - return grep { $_->phab_phid } values %users; + return map { $_->phid } @$phab_users; } 1; diff --git a/extensions/PhabBugz/lib/Project.pm b/extensions/PhabBugz/lib/Project.pm index cbf1bdcaf..c52e1a661 100644 --- a/extensions/PhabBugz/lib/Project.pm +++ b/extensions/PhabBugz/lib/Project.pm @@ -9,15 +9,14 @@ 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::Util qw( - request - get_phab_bmo_ids -); +use Bugzilla::Extension::PhabBugz::User; +use Bugzilla::Extension::PhabBugz::Util qw(request); ######################### # Initialization # @@ -281,20 +280,20 @@ sub set_description { sub add_member { my ( $self, $member ) = @_; $self->{add_members} ||= []; - my $member_phid = blessed $member ? $member->phab_phid : $member; + my $member_phid = blessed $member ? $member->phid : $member; push( @{ $self->{add_members} }, $member_phid ); } sub remove_member { my ( $self, $member ) = @_; $self->{remove_members} ||= []; - my $member_phid = blessed $member ? $member->phab_phid : $member; + my $member_phid = blessed $member ? $member->phid : $member; push( @{ $self->{remove_members} }, $member_phid ); } sub set_members { my ( $self, $members ) = @_; - $self->{set_members} = [ map { $_->phab_phid } @$members ]; + $self->{set_members} = [ map { blessed $_ ? $_->phid : $_ } @$members ]; } sub set_policy { @@ -318,16 +317,13 @@ sub _build_members { return [] if !@phids; - my $users = get_phab_bmo_ids( { phids => \@phids } ); + my $users = Bugzilla::Extension::PhabBugz::User->match( + { + phids => \@phids + } + ); - 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; + return [ map { $_->bugzilla_user } @$users ]; } 1; diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm index 98c3196c2..d87ca8bd2 100644 --- a/extensions/PhabBugz/lib/Revision.pm +++ b/extensions/PhabBugz/lib/Revision.pm @@ -17,10 +17,8 @@ use Type::Utils; use Bugzilla::Bug; use Bugzilla::Error; use Bugzilla::Util qw(trim); -use Bugzilla::Extension::PhabBugz::Util qw( - get_phab_bmo_ids - request -); +use Bugzilla::Extension::PhabBugz::User; +use Bugzilla::Extension::PhabBugz::Util qw(request); ######################### # Initialization # @@ -284,12 +282,13 @@ sub _build_bug { sub _build_author { my ($self) = @_; return $self->{author} if $self->{author}; - 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}; + my $phab_user = Bugzilla::Extension::PhabBugz::User->new_from_query( + { + phids => [ $self->author_phid ] + } + ); + if ($phab_user) { + return $self->{author} = $phab_user->bugzilla_user; } } @@ -306,22 +305,22 @@ sub _build_reviewers { return [] unless @phids; - my $users = get_phab_bmo_ids( { phids => \@phids } ); + my $users = Bugzilla::Extension::PhabBugz::User->match( + { + 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} ) { - $reviewer->{phab_review_status} = $reviewer_data->{status}; + if ( $reviewer_data->{reviewerPHID} eq $user->phid ) { + $user->{phab_review_status} = $reviewer_data->{status}; last; } } - push @reviewers, $reviewer; } - return \@reviewers; + return $self->{reviewers} = $users; } sub _build_subscribers { @@ -335,19 +334,13 @@ sub _build_subscribers { push @phids, $phid; } - 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; - } + my $users = Bugzilla::Extension::PhabBugz::User->match( + { + phids => \@phids + } + ); - return \@subscribers; + return $self->{subscribers} = $users; } ######################### @@ -364,27 +357,27 @@ sub add_comment { sub add_reviewer { my ( $self, $reviewer ) = @_; $self->{add_reviewers} ||= []; - my $reviewer_phid = blessed $reviewer ? $reviewer->phab_phid : $reviewer; + my $reviewer_phid = blessed $reviewer ? $reviewer->phid : $reviewer; push @{ $self->{add_reviewers} }, $reviewer_phid; } sub remove_reviewer { my ( $self, $reviewer ) = @_; $self->{remove_reviewers} ||= []; - my $reviewer_phid = blessed $reviewer ? $reviewer->phab_phid : $reviewer; + my $reviewer_phid = blessed $reviewer ? $reviewer->phid : $reviewer; push @{ $self->{remove_reviewers} }, $reviewer_phid; } sub set_reviewers { my ( $self, $reviewers ) = @_; - $self->{set_reviewers} = [ map { $_->phab_phid } @$reviewers ]; + $self->{set_reviewers} = [ map { $_->phid } @$reviewers ]; } sub add_subscriber { my ( $self, $subscriber ) = @_; $self->{add_subscribers} ||= []; my $subscriber_phid = - blessed $subscriber ? $subscriber->phab_phid : $subscriber; + blessed $subscriber ? $subscriber->phid : $subscriber; push @{ $self->{add_subscribers} }, $subscriber_phid; } @@ -392,7 +385,7 @@ sub remove_subscriber { my ( $self, $subscriber ) = @_; $self->{remove_subscribers} ||= []; my $subscriber_phid = - blessed $subscriber ? $subscriber->phab_phid : $subscriber; + blessed $subscriber ? $subscriber->phid : $subscriber; push @{ $self->{remove_subscribers} }, $subscriber_phid; } diff --git a/extensions/PhabBugz/lib/User.pm b/extensions/PhabBugz/lib/User.pm index 3b2d87e60..9d4e9eef4 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->{bugzilla_ids}; + my $bugzilla_ids = delete $params->{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,6 +177,7 @@ 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 42da79c39..916455e24 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -34,10 +34,7 @@ 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 @@ -134,7 +131,13 @@ 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 }; - return get_members_by_bmo_id(\@bug_users); + my $phab_users = Bugzilla::Extension::PhabBugz::User->match( + { + ids => [ map { $_->id } @bug_users ] + } + ); + + return [ map { $_->phid } @$phab_users ]; } sub create_private_revision_policy { @@ -354,84 +357,6 @@ 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 @@ -559,9 +484,13 @@ sub get_needs_review { $user //= Bugzilla->user; return unless $user->id; - my $ids = get_members_by_bmo_id([$user]); - return [] unless @$ids; - my $phid_user = $ids->[0]; + my $phab_user = Bugzilla::Extension::PhabBugz::User->new_from_query( + { + ids => [ $user->id ] + } + ); + + return [] unless $phab_user; my $diffs = request( 'differential.revision.search', @@ -570,7 +499,7 @@ sub get_needs_review { reviewers => 1, }, constraints => { - reviewerPHIDs => [$phid_user], + reviewerPHIDs => [$phab_user->phid], statuses => [qw( needs-review )], }, order => 'newest', @@ -584,7 +513,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 $phid_user } @$reviewers; + my $review = first { $_->{reviewerPHID} eq $phab_user->phid } @$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 32cea1b51..f018ba702 100644 --- a/extensions/PhabBugz/lib/WebService.pm +++ b/extensions/PhabBugz/lib/WebService.pm @@ -30,7 +30,6 @@ 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 @@ -307,8 +306,7 @@ sub needs_review { my $reviews = get_needs_review(); - # map author phids to bugzilla users - my $author_id_map = get_phab_bmo_ids({ + my $authors = Bugzilla::Extension::PhabBugz::User->match({ phids => [ uniq grep { defined } @@ -316,9 +314,9 @@ sub needs_review { @$reviews ] }); - 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; + + 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([ -- cgit v1.2.3-24-g4f1b