From 60bd095be5ed415de6fa2fc690fbbfeca2d356a6 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 18 Oct 2017 23:49:16 -0400 Subject: [PATCH 01/13] WIP: Bug 1409957 - Create polling daemon to query Phabricator for recent transcations and update bug data according to revision changes --- extensions/PhabBugz/Extension.pm | 61 +++++++++++++ extensions/PhabBugz/bin/phabbugz_feed.pl | 50 +++++++++++ extensions/PhabBugz/lib/Constants.pm | 2 + extensions/PhabBugz/lib/Daemon.pm | 95 +++++++++++++++++++++ extensions/PhabBugz/lib/Feed.pm | 104 +++++++++++++++++++++++ extensions/PhabBugz/lib/Logger.pm | 46 ++++++++++ extensions/PhabBugz/lib/Util.pm | 31 +++++++ 7 files changed, 389 insertions(+) create mode 100755 extensions/PhabBugz/bin/phabbugz_feed.pl create mode 100644 extensions/PhabBugz/lib/Daemon.pm create mode 100644 extensions/PhabBugz/lib/Feed.pm create mode 100644 extensions/PhabBugz/lib/Logger.pm diff --git a/extensions/PhabBugz/Extension.pm b/extensions/PhabBugz/Extension.pm index 68090aa10e..039ff33a91 100644 --- a/extensions/PhabBugz/Extension.pm +++ b/extensions/PhabBugz/Extension.pm @@ -12,8 +12,26 @@ use strict; use warnings; use parent qw(Bugzilla::Extension); +use Bugzilla::Constants; +use Bugzilla::Extension::PhabBugz::Feed; +use Bugzilla::Extension::PhabBugz::Logger; + our $VERSION = '0.01'; +BEGIN { + *Bugzilla::phabbugz_ext = \&_get_instance; +} + +sub _get_instance { + my $cache = Bugzilla->request_cache; + if (!$cache->{'phabbugz.instance'}) { + my $instance = Bugzilla::Extension::PhabBugz::Feed->new(); + $cache->{'phabbugz.instance'} = $instance; + $instance->logger(Bugzilla::Extension::PhabBugz::Logger->new()); + } + return $cache->{'phabbugz.instance'}; +} + sub config_add_panels { my ($self, $args) = @_; my $modules = $args->{panel_modules}; @@ -40,4 +58,47 @@ sub webservice { $args->{dispatch}->{PhabBugz} = "Bugzilla::Extension::PhabBugz::WebService"; } +# +# installation/config hooks +# + +sub db_schema_abstract_schema { + my ($self, $args) = @_; + $args->{'schema'}->{'phabbugz'} = { + FIELDS => [ + id => { + TYPE => 'MEDIUMSERIAL', + NOTNULL => 1, + PRIMARYKEY => 1, + }, + name => { + TYPE => 'VARCHAR(64)', + NOTNULL => 1, + }, + value => { + TYPE => 'MEDIUMTEXT', + NOTNULL => 1 + } + ], + INDEXES => [ + phabbugz_idx => { + FIELDS => ['name'], + TYPE => 'UNIQUE', + }, + ], + }; +} + +sub install_filesystem { + my ($self, $args) = @_; + my $files = $args->{'files'}; + + my $extensionsdir = bz_locations()->{'extensionsdir'}; + my $scriptname = $extensionsdir . "/PhabBugz/bin/phabbugzd.pl"; + + $files->{$scriptname} = { + perms => Bugzilla::Install::Filesystem::WS_EXECUTE + }; +} + __PACKAGE__->NAME; diff --git a/extensions/PhabBugz/bin/phabbugz_feed.pl b/extensions/PhabBugz/bin/phabbugz_feed.pl new file mode 100755 index 0000000000..7e11885f8e --- /dev/null +++ b/extensions/PhabBugz/bin/phabbugz_feed.pl @@ -0,0 +1,50 @@ +#!/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 { + use Bugzilla; + Bugzilla->extensions; +} + +use Bugzilla::Extension::PhabBugz::Daemon; +Bugzilla::Extension::PhabBugz::Daemon->start(); + +=head1 NAME + +phabbugzd.pl - Query Phabricator for interesting changes and update bugs related to revisions. + +=head1 SYNOPSIS + + phabbugzd.pl [OPTIONS] COMMAND + + OPTIONS: + -f Run in the foreground (don't detach) + -d Output a lot of debugging information + -p file Specify the file where phabbugzd.pl should store its current + process id. Defaults to F. + -n name What should this process call itself in the system log? + Defaults to the full path you used to invoke the script. + + COMMANDS: + start Starts a new phabbugzd daemon if there isn't one running already + stop Stops a running phabbugzd daemon + restart Stops a running phabbugzd if one is running, and then + starts a new one. + check Report the current status of the daemon. + install On some *nix systems, this automatically installs and + configures phabbugzd.pl as a system service so that it will + start every time the machine boots. + uninstall Removes the system service for phabbugzd.pl. + help Display this usage info \ No newline at end of file diff --git a/extensions/PhabBugz/lib/Constants.pm b/extensions/PhabBugz/lib/Constants.pm index f7485e8c4f..754130f0b1 100644 --- a/extensions/PhabBugz/lib/Constants.pm +++ b/extensions/PhabBugz/lib/Constants.pm @@ -16,10 +16,12 @@ our @EXPORT = qw( PHAB_AUTOMATION_USER PHAB_ATTACHMENT_PATTERN PHAB_CONTENT_TYPE + PHAB_POLL_SECONDS ); use constant PHAB_ATTACHMENT_PATTERN => qr/^phabricator-D(\d+)/; use constant PHAB_AUTOMATION_USER => 'phab-bot@bmo.tld'; use constant PHAB_CONTENT_TYPE => 'text/x-phabricator-request'; +use constant PHAB_POLL_SECONDS => 5; 1; diff --git a/extensions/PhabBugz/lib/Daemon.pm b/extensions/PhabBugz/lib/Daemon.pm new file mode 100644 index 0000000000..bacc39e8e6 --- /dev/null +++ b/extensions/PhabBugz/lib/Daemon.pm @@ -0,0 +1,95 @@ +# 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::Daemon; + +use 5.10.1; +use strict; +use warnings; + +use Bugzilla::Constants; +use Carp qw(confess); +use Daemon::Generic; +use File::Basename; +use Pod::Usage; + +sub start { + newdaemon(); +} + +# +# daemon::generic config +# + +sub gd_preconfig { + my $self = shift; + my $pidfile = $self->{gd_args}{pidfile}; + if (!$pidfile) { + $pidfile = bz_locations()->{datadir} . '/' . $self->{gd_progname} . ".pid"; + } + return (pidfile => $pidfile); +} + +sub gd_getopt { + my $self = shift; + $self->SUPER::gd_getopt(); + if ($self->{gd_args}{progname}) { + $self->{gd_progname} = $self->{gd_args}{progname}; + } else { + $self->{gd_progname} = basename($0); + } + $self->{_original_zero} = $0; + $0 = $self->{gd_progname}; +} + +sub gd_postconfig { + my $self = shift; + $0 = delete $self->{_original_zero}; +} + +sub gd_more_opt { + my $self = shift; + return ( + 'pidfile=s' => \$self->{gd_args}{pidfile}, + 'n=s' => \$self->{gd_args}{progname}, + ); +} + +sub gd_usage { + pod2usage({ -verbose => 0, -exitval => 'NOEXIT' }); + return 0; +}; + +sub gd_redirect_output { + my $self = shift; + + my $filename = bz_locations()->{datadir} . '/' . $self->{gd_progname} . ".log"; + open(STDERR, ">>", $filename) or (print "could not open stderr: $!" && exit(1)); + close(STDOUT); + open(STDOUT, ">&", STDERR) or die "redirect STDOUT -> STDERR: $!"; + $SIG{HUP} = sub { + close(STDERR); + open(STDERR, ">>", $filename) or (print "could not open stderr: $!" && exit(1)); + }; +} + +sub gd_setup_signals { + my $self = shift; + $self->SUPER::gd_setup_signals(); + $SIG{TERM} = sub { $self->gd_quit_event(); } +} + +sub gd_run { + my $self = shift; + $::SIG{__DIE__} = \&Carp::confess if $self->{debug}; + my $phabbugz = Bugzilla->phabbugz_ext; + $phabbugz->is_daemon(1); + $phabbugz->logger->{debug} = $self->{debug}; + $phabbugz->start(); +} + +1; diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm new file mode 100644 index 0000000000..fc201e19c1 --- /dev/null +++ b/extensions/PhabBugz/lib/Feed.pm @@ -0,0 +1,104 @@ +# 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::Feed; + +use 5.10.1; +use strict; +use warnings; + +use Bugzilla::Extension::PhabBugz::Constants; +use Bugzilla::Extension::PhabBugz::Util; + +sub new { + my ($class) = @_; + my $self = {}; + bless($self, $class); + $self->{is_daemon} = 0; + return $self; +} + +sub is_daemon { + my ($self, $value) = @_; + if (defined $value) { + $self->{is_daemon} = $value ? 1 : 0; + } + return $self->{is_daemon}; +} + +sub logger { + my ($self, $value) = @_; + $self->{logger} = $value if $value; + return $self->{logger}; +} + +sub start { + my ($self) = @_; + while(1) { + if ($self->_dbh_check()) { + $self->feed_query(); + } + sleep(PHAB_POLL_SECONDS); + } +} + +sub feed_query { + my ($self) = @_; + my $dbh = Bugzilla->dbh; + + $self->logger->info("FEED: Polling"); + + my $last_ts = $dbh->selectrow_array(" + SELECT value FROM phabbugz WHERE name = 'feed_last_ts'"); + + # Check for new transctions (stories) + my $transactions = get_feed_transactions($last_ts+1); + if (!$transactions) { + $self->logger->info("FEED: No new transactions"); + return; + } + + # Process each story + foreach my $story (keys %$transactions) { + $self->logger->info("STORY: $story"); + my $story_data = $transactions->{$story}; + my $object_phid = $story_data->{objectPHID}; + $self->logger->info("OBJECT: $object_phid"); + if ($object_phid !~ /^PHID-DREV/) { + $self->logger->info("SKIP: Not a revision change"); + next; + } + my ($revision) = get_revisions_by_phids([$object_phid]); + $self->logger->info("REVSION: " . $revision->{'id'} . ": " . + $revision->{'fields'}->{'title'} . " " . + $revision->{'fields'}->{'bugzilla.bug-id'} . " " . + $story_data->{'text'}); + + # Find the highest epoch for storage in feed_last_ts + if ($story_data->{epoch} > $last_ts) { + $last_ts = $story_data->{epoch}; + } + } + + $self->logger->debug("LAST_TS: $last_ts"); + $dbh->do("REPLACE INTO phabbugz (name, value) VALUES ('feed_last_ts', ?)", + undef, $last_ts); +} + +sub _dbh_check { + my ($self) = @_; + eval { + Bugzilla->dbh->selectrow_array("SELECT 1 FROM phabbugz"); + }; + if ($@) { + return 0; + } else { + return 1; + } +} + +1; diff --git a/extensions/PhabBugz/lib/Logger.pm b/extensions/PhabBugz/lib/Logger.pm new file mode 100644 index 0000000000..9ddacd8f7d --- /dev/null +++ b/extensions/PhabBugz/lib/Logger.pm @@ -0,0 +1,46 @@ +# 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::Logger; + +use 5.10.1; +use strict; +use warnings; + +use Bugzilla::Extension::PhabBugz::Constants; + +sub new { + my ($class) = @_; + my $self = {}; + bless($self, $class); + return $self; +} + +sub info { shift->_log_it('INFO', @_) } +sub error { shift->_log_it('ERROR', @_) } +sub debug { shift->_log_it('DEBUG', @_) } + +sub debugging { + my ($self) = @_; + return $self->{debug}; +} + +sub _log_it { + require Apache2::Log; + my ($self, $method, $message) = @_; + return if $method eq 'DEBUG' && !$self->debugging; + chomp $message; + if ($ENV{MOD_PERL}) { + Apache2::ServerRec::warn("Push $method: $message"); + } elsif ($ENV{SCRIPT_FILENAME}) { + print STDERR "Push $method: $message\n"; + } else { + print STDERR '[' . localtime(time) ."] $method: $message\n"; + } +} + +1; diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index 95b2b15982..cc2b1722bf 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -33,9 +33,11 @@ our @EXPORT = qw( edit_revision_policy get_attachment_revisions get_bug_role_phids + get_feed_transactions get_members_by_bmo_id get_project_phid get_revisions_by_ids + get_revisions_by_phids get_security_sync_groups intersect is_attachment_phab_revision @@ -64,6 +66,24 @@ sub get_revisions_by_ids { return @{$result->{result}{data}}; } +sub get_revisions_by_phids { + my ($phids) = @_; + + my $data = { + queryKey => 'all', + constraints => { + phids => $phids + } + }; + + my $result = request('differential.revision.search', $data); + + ThrowUserError('invalid_phabricator_revision_id') + unless (exists $result->{result}{data} && @{ $result->{result}{data} }); + + return @{$result->{result}{data}}; +} + sub create_revision_attachment { my ( $bug, $revision_id, $revision_title ) = @_; @@ -457,4 +477,15 @@ sub add_security_sync_comments { Bugzilla->set_user($old_user); } +sub get_feed_transactions { + my ($epoch) = @_; + my $data = { view => 'text' }; + $data->{epochStart} = $epoch if $epoch; + my $result = request('feed.query_epoch', $data); + # Stupid conduit. If the feed results are empty it returns + # an empty list ([]). If there is data it returns it in a + # hash ({}) so we have adjust to be consistent. + return ref $result->{result} eq 'HASH' ? $result->{result} : {}; +} + 1; From e969e034646a97750d13e66210f50c842ede4b8c Mon Sep 17 00:00:00 2001 From: Dylan William Hardison Date: Thu, 19 Oct 2017 15:52:56 -0400 Subject: [PATCH 02/13] Remove some boilerplate using Moo --- extensions/PhabBugz/lib/Feed.pm | 37 ++++++------------------------- extensions/PhabBugz/lib/Logger.pm | 24 ++++++-------------- 2 files changed, 14 insertions(+), 47 deletions(-) diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index fc201e19c1..03bf183bfa 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -8,37 +8,17 @@ package Bugzilla::Extension::PhabBugz::Feed; use 5.10.1; -use strict; -use warnings; +use Moo; use Bugzilla::Extension::PhabBugz::Constants; use Bugzilla::Extension::PhabBugz::Util; -sub new { - my ($class) = @_; - my $self = {}; - bless($self, $class); - $self->{is_daemon} = 0; - return $self; -} - -sub is_daemon { - my ($self, $value) = @_; - if (defined $value) { - $self->{is_daemon} = $value ? 1 : 0; - } - return $self->{is_daemon}; -} - -sub logger { - my ($self, $value) = @_; - $self->{logger} = $value if $value; - return $self->{logger}; -} +has 'is_daemon' => (is => 'rw', default => 0); +has 'logger' => (is => 'rw'); sub start { my ($self) = @_; - while(1) { + while (1) { if ($self->_dbh_check()) { $self->feed_query(); } @@ -91,14 +71,11 @@ sub feed_query { sub _dbh_check { my ($self) = @_; - eval { + + my $ok = eval { Bugzilla->dbh->selectrow_array("SELECT 1 FROM phabbugz"); }; - if ($@) { - return 0; - } else { - return 1; - } + return defined $ok; } 1; diff --git a/extensions/PhabBugz/lib/Logger.pm b/extensions/PhabBugz/lib/Logger.pm index 9ddacd8f7d..ecff0b4776 100644 --- a/extensions/PhabBugz/lib/Logger.pm +++ b/extensions/PhabBugz/lib/Logger.pm @@ -8,33 +8,23 @@ package Bugzilla::Extension::PhabBugz::Logger; use 5.10.1; -use strict; -use warnings; +use Moo; use Bugzilla::Extension::PhabBugz::Constants; -sub new { - my ($class) = @_; - my $self = {}; - bless($self, $class); - return $self; -} - -sub info { shift->_log_it('INFO', @_) } -sub error { shift->_log_it('ERROR', @_) } -sub debug { shift->_log_it('DEBUG', @_) } +has 'debugging' => ( is => 'ro' ); -sub debugging { - my ($self) = @_; - return $self->{debug}; -} +sub info { $_[0]->_log_it('INFO', @_) } +sub error { $_[0]->_log_it('ERROR', @_) } +sub debug { $_[0]->_log_it('DEBUG', @_) } sub _log_it { - require Apache2::Log; my ($self, $method, $message) = @_; + return if $method eq 'DEBUG' && !$self->debugging; chomp $message; if ($ENV{MOD_PERL}) { + require Apache2::Log; Apache2::ServerRec::warn("Push $method: $message"); } elsif ($ENV{SCRIPT_FILENAME}) { print STDERR "Push $method: $message\n"; From d2662bcc888dcb7b17efd22995c6e007fd78cd58 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 18 Oct 2017 23:49:16 -0400 Subject: [PATCH 03/13] WIP: Bug 1409957 - Create polling daemon to query Phabricator for recent transcations and update bug data according to revision changes --- extensions/PhabBugz/Extension.pm | 61 +++++++++++++ extensions/PhabBugz/bin/phabbugz_feed.pl | 50 +++++++++++ extensions/PhabBugz/lib/Constants.pm | 2 + extensions/PhabBugz/lib/Daemon.pm | 95 +++++++++++++++++++++ extensions/PhabBugz/lib/Feed.pm | 104 +++++++++++++++++++++++ extensions/PhabBugz/lib/Logger.pm | 46 ++++++++++ extensions/PhabBugz/lib/Util.pm | 31 +++++++ 7 files changed, 389 insertions(+) create mode 100755 extensions/PhabBugz/bin/phabbugz_feed.pl create mode 100644 extensions/PhabBugz/lib/Daemon.pm create mode 100644 extensions/PhabBugz/lib/Feed.pm create mode 100644 extensions/PhabBugz/lib/Logger.pm diff --git a/extensions/PhabBugz/Extension.pm b/extensions/PhabBugz/Extension.pm index 68090aa10e..039ff33a91 100644 --- a/extensions/PhabBugz/Extension.pm +++ b/extensions/PhabBugz/Extension.pm @@ -12,8 +12,26 @@ use strict; use warnings; use parent qw(Bugzilla::Extension); +use Bugzilla::Constants; +use Bugzilla::Extension::PhabBugz::Feed; +use Bugzilla::Extension::PhabBugz::Logger; + our $VERSION = '0.01'; +BEGIN { + *Bugzilla::phabbugz_ext = \&_get_instance; +} + +sub _get_instance { + my $cache = Bugzilla->request_cache; + if (!$cache->{'phabbugz.instance'}) { + my $instance = Bugzilla::Extension::PhabBugz::Feed->new(); + $cache->{'phabbugz.instance'} = $instance; + $instance->logger(Bugzilla::Extension::PhabBugz::Logger->new()); + } + return $cache->{'phabbugz.instance'}; +} + sub config_add_panels { my ($self, $args) = @_; my $modules = $args->{panel_modules}; @@ -40,4 +58,47 @@ sub webservice { $args->{dispatch}->{PhabBugz} = "Bugzilla::Extension::PhabBugz::WebService"; } +# +# installation/config hooks +# + +sub db_schema_abstract_schema { + my ($self, $args) = @_; + $args->{'schema'}->{'phabbugz'} = { + FIELDS => [ + id => { + TYPE => 'MEDIUMSERIAL', + NOTNULL => 1, + PRIMARYKEY => 1, + }, + name => { + TYPE => 'VARCHAR(64)', + NOTNULL => 1, + }, + value => { + TYPE => 'MEDIUMTEXT', + NOTNULL => 1 + } + ], + INDEXES => [ + phabbugz_idx => { + FIELDS => ['name'], + TYPE => 'UNIQUE', + }, + ], + }; +} + +sub install_filesystem { + my ($self, $args) = @_; + my $files = $args->{'files'}; + + my $extensionsdir = bz_locations()->{'extensionsdir'}; + my $scriptname = $extensionsdir . "/PhabBugz/bin/phabbugzd.pl"; + + $files->{$scriptname} = { + perms => Bugzilla::Install::Filesystem::WS_EXECUTE + }; +} + __PACKAGE__->NAME; diff --git a/extensions/PhabBugz/bin/phabbugz_feed.pl b/extensions/PhabBugz/bin/phabbugz_feed.pl new file mode 100755 index 0000000000..7e11885f8e --- /dev/null +++ b/extensions/PhabBugz/bin/phabbugz_feed.pl @@ -0,0 +1,50 @@ +#!/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 { + use Bugzilla; + Bugzilla->extensions; +} + +use Bugzilla::Extension::PhabBugz::Daemon; +Bugzilla::Extension::PhabBugz::Daemon->start(); + +=head1 NAME + +phabbugzd.pl - Query Phabricator for interesting changes and update bugs related to revisions. + +=head1 SYNOPSIS + + phabbugzd.pl [OPTIONS] COMMAND + + OPTIONS: + -f Run in the foreground (don't detach) + -d Output a lot of debugging information + -p file Specify the file where phabbugzd.pl should store its current + process id. Defaults to F. + -n name What should this process call itself in the system log? + Defaults to the full path you used to invoke the script. + + COMMANDS: + start Starts a new phabbugzd daemon if there isn't one running already + stop Stops a running phabbugzd daemon + restart Stops a running phabbugzd if one is running, and then + starts a new one. + check Report the current status of the daemon. + install On some *nix systems, this automatically installs and + configures phabbugzd.pl as a system service so that it will + start every time the machine boots. + uninstall Removes the system service for phabbugzd.pl. + help Display this usage info \ No newline at end of file diff --git a/extensions/PhabBugz/lib/Constants.pm b/extensions/PhabBugz/lib/Constants.pm index f7485e8c4f..754130f0b1 100644 --- a/extensions/PhabBugz/lib/Constants.pm +++ b/extensions/PhabBugz/lib/Constants.pm @@ -16,10 +16,12 @@ our @EXPORT = qw( PHAB_AUTOMATION_USER PHAB_ATTACHMENT_PATTERN PHAB_CONTENT_TYPE + PHAB_POLL_SECONDS ); use constant PHAB_ATTACHMENT_PATTERN => qr/^phabricator-D(\d+)/; use constant PHAB_AUTOMATION_USER => 'phab-bot@bmo.tld'; use constant PHAB_CONTENT_TYPE => 'text/x-phabricator-request'; +use constant PHAB_POLL_SECONDS => 5; 1; diff --git a/extensions/PhabBugz/lib/Daemon.pm b/extensions/PhabBugz/lib/Daemon.pm new file mode 100644 index 0000000000..bacc39e8e6 --- /dev/null +++ b/extensions/PhabBugz/lib/Daemon.pm @@ -0,0 +1,95 @@ +# 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::Daemon; + +use 5.10.1; +use strict; +use warnings; + +use Bugzilla::Constants; +use Carp qw(confess); +use Daemon::Generic; +use File::Basename; +use Pod::Usage; + +sub start { + newdaemon(); +} + +# +# daemon::generic config +# + +sub gd_preconfig { + my $self = shift; + my $pidfile = $self->{gd_args}{pidfile}; + if (!$pidfile) { + $pidfile = bz_locations()->{datadir} . '/' . $self->{gd_progname} . ".pid"; + } + return (pidfile => $pidfile); +} + +sub gd_getopt { + my $self = shift; + $self->SUPER::gd_getopt(); + if ($self->{gd_args}{progname}) { + $self->{gd_progname} = $self->{gd_args}{progname}; + } else { + $self->{gd_progname} = basename($0); + } + $self->{_original_zero} = $0; + $0 = $self->{gd_progname}; +} + +sub gd_postconfig { + my $self = shift; + $0 = delete $self->{_original_zero}; +} + +sub gd_more_opt { + my $self = shift; + return ( + 'pidfile=s' => \$self->{gd_args}{pidfile}, + 'n=s' => \$self->{gd_args}{progname}, + ); +} + +sub gd_usage { + pod2usage({ -verbose => 0, -exitval => 'NOEXIT' }); + return 0; +}; + +sub gd_redirect_output { + my $self = shift; + + my $filename = bz_locations()->{datadir} . '/' . $self->{gd_progname} . ".log"; + open(STDERR, ">>", $filename) or (print "could not open stderr: $!" && exit(1)); + close(STDOUT); + open(STDOUT, ">&", STDERR) or die "redirect STDOUT -> STDERR: $!"; + $SIG{HUP} = sub { + close(STDERR); + open(STDERR, ">>", $filename) or (print "could not open stderr: $!" && exit(1)); + }; +} + +sub gd_setup_signals { + my $self = shift; + $self->SUPER::gd_setup_signals(); + $SIG{TERM} = sub { $self->gd_quit_event(); } +} + +sub gd_run { + my $self = shift; + $::SIG{__DIE__} = \&Carp::confess if $self->{debug}; + my $phabbugz = Bugzilla->phabbugz_ext; + $phabbugz->is_daemon(1); + $phabbugz->logger->{debug} = $self->{debug}; + $phabbugz->start(); +} + +1; diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm new file mode 100644 index 0000000000..fc201e19c1 --- /dev/null +++ b/extensions/PhabBugz/lib/Feed.pm @@ -0,0 +1,104 @@ +# 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::Feed; + +use 5.10.1; +use strict; +use warnings; + +use Bugzilla::Extension::PhabBugz::Constants; +use Bugzilla::Extension::PhabBugz::Util; + +sub new { + my ($class) = @_; + my $self = {}; + bless($self, $class); + $self->{is_daemon} = 0; + return $self; +} + +sub is_daemon { + my ($self, $value) = @_; + if (defined $value) { + $self->{is_daemon} = $value ? 1 : 0; + } + return $self->{is_daemon}; +} + +sub logger { + my ($self, $value) = @_; + $self->{logger} = $value if $value; + return $self->{logger}; +} + +sub start { + my ($self) = @_; + while(1) { + if ($self->_dbh_check()) { + $self->feed_query(); + } + sleep(PHAB_POLL_SECONDS); + } +} + +sub feed_query { + my ($self) = @_; + my $dbh = Bugzilla->dbh; + + $self->logger->info("FEED: Polling"); + + my $last_ts = $dbh->selectrow_array(" + SELECT value FROM phabbugz WHERE name = 'feed_last_ts'"); + + # Check for new transctions (stories) + my $transactions = get_feed_transactions($last_ts+1); + if (!$transactions) { + $self->logger->info("FEED: No new transactions"); + return; + } + + # Process each story + foreach my $story (keys %$transactions) { + $self->logger->info("STORY: $story"); + my $story_data = $transactions->{$story}; + my $object_phid = $story_data->{objectPHID}; + $self->logger->info("OBJECT: $object_phid"); + if ($object_phid !~ /^PHID-DREV/) { + $self->logger->info("SKIP: Not a revision change"); + next; + } + my ($revision) = get_revisions_by_phids([$object_phid]); + $self->logger->info("REVSION: " . $revision->{'id'} . ": " . + $revision->{'fields'}->{'title'} . " " . + $revision->{'fields'}->{'bugzilla.bug-id'} . " " . + $story_data->{'text'}); + + # Find the highest epoch for storage in feed_last_ts + if ($story_data->{epoch} > $last_ts) { + $last_ts = $story_data->{epoch}; + } + } + + $self->logger->debug("LAST_TS: $last_ts"); + $dbh->do("REPLACE INTO phabbugz (name, value) VALUES ('feed_last_ts', ?)", + undef, $last_ts); +} + +sub _dbh_check { + my ($self) = @_; + eval { + Bugzilla->dbh->selectrow_array("SELECT 1 FROM phabbugz"); + }; + if ($@) { + return 0; + } else { + return 1; + } +} + +1; diff --git a/extensions/PhabBugz/lib/Logger.pm b/extensions/PhabBugz/lib/Logger.pm new file mode 100644 index 0000000000..9ddacd8f7d --- /dev/null +++ b/extensions/PhabBugz/lib/Logger.pm @@ -0,0 +1,46 @@ +# 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::Logger; + +use 5.10.1; +use strict; +use warnings; + +use Bugzilla::Extension::PhabBugz::Constants; + +sub new { + my ($class) = @_; + my $self = {}; + bless($self, $class); + return $self; +} + +sub info { shift->_log_it('INFO', @_) } +sub error { shift->_log_it('ERROR', @_) } +sub debug { shift->_log_it('DEBUG', @_) } + +sub debugging { + my ($self) = @_; + return $self->{debug}; +} + +sub _log_it { + require Apache2::Log; + my ($self, $method, $message) = @_; + return if $method eq 'DEBUG' && !$self->debugging; + chomp $message; + if ($ENV{MOD_PERL}) { + Apache2::ServerRec::warn("Push $method: $message"); + } elsif ($ENV{SCRIPT_FILENAME}) { + print STDERR "Push $method: $message\n"; + } else { + print STDERR '[' . localtime(time) ."] $method: $message\n"; + } +} + +1; diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index 95b2b15982..cc2b1722bf 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -33,9 +33,11 @@ our @EXPORT = qw( edit_revision_policy get_attachment_revisions get_bug_role_phids + get_feed_transactions get_members_by_bmo_id get_project_phid get_revisions_by_ids + get_revisions_by_phids get_security_sync_groups intersect is_attachment_phab_revision @@ -64,6 +66,24 @@ sub get_revisions_by_ids { return @{$result->{result}{data}}; } +sub get_revisions_by_phids { + my ($phids) = @_; + + my $data = { + queryKey => 'all', + constraints => { + phids => $phids + } + }; + + my $result = request('differential.revision.search', $data); + + ThrowUserError('invalid_phabricator_revision_id') + unless (exists $result->{result}{data} && @{ $result->{result}{data} }); + + return @{$result->{result}{data}}; +} + sub create_revision_attachment { my ( $bug, $revision_id, $revision_title ) = @_; @@ -457,4 +477,15 @@ sub add_security_sync_comments { Bugzilla->set_user($old_user); } +sub get_feed_transactions { + my ($epoch) = @_; + my $data = { view => 'text' }; + $data->{epochStart} = $epoch if $epoch; + my $result = request('feed.query_epoch', $data); + # Stupid conduit. If the feed results are empty it returns + # an empty list ([]). If there is data it returns it in a + # hash ({}) so we have adjust to be consistent. + return ref $result->{result} eq 'HASH' ? $result->{result} : {}; +} + 1; From 71d177dec0fb3a5e6bd45e8f714f8ea6b72a5ccd Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Tue, 31 Oct 2017 15:25:41 -0400 Subject: [PATCH 04/13] Cleanups and refactoring from last commit. New Revision.pm class that will eventually handle all loading and updating of a revision How to test: 1. You need to be running a mozilla/phabext image that contains the latest phabricator-extensions code that has the feed.query_epoch Conduit API call. 2. cd /path/to/mozilla-conduit/bmo-extensions 3. docker-compose up -d --build 4. docker exec -it bmoextensions_bmo.test_1 su - bugzilla 5. cd /var/www/html/bmo 6. perl extensions/PhabBugz/bin/phabbugz_feed.pl -f -d start 7. In Phabricator, create a new revision with the bug id of an existing bug in the bugzilla database. 8. BMO should update the revision with either public or private policies depending on the bugs permissions. --- extensions/PhabBugz/Extension.pm | 14 +- extensions/PhabBugz/bin/phabbugz_feed.pl | 22 +- extensions/PhabBugz/lib/Daemon.pm | 13 +- extensions/PhabBugz/lib/Feed.pm | 158 +++++++++----- extensions/PhabBugz/lib/Logger.pm | 23 +- extensions/PhabBugz/lib/Revision.pm | 266 +++++++++++++++++++++++ extensions/PhabBugz/lib/Util.pm | 31 +-- 7 files changed, 417 insertions(+), 110 deletions(-) create mode 100644 extensions/PhabBugz/lib/Revision.pm diff --git a/extensions/PhabBugz/Extension.pm b/extensions/PhabBugz/Extension.pm index 039ff33a91..b8ecf001dc 100644 --- a/extensions/PhabBugz/Extension.pm +++ b/extensions/PhabBugz/Extension.pm @@ -10,6 +10,7 @@ package Bugzilla::Extension::PhabBugz; use 5.10.1; use strict; use warnings; + use parent qw(Bugzilla::Extension); use Bugzilla::Constants; @@ -19,17 +20,8 @@ use Bugzilla::Extension::PhabBugz::Logger; our $VERSION = '0.01'; BEGIN { - *Bugzilla::phabbugz_ext = \&_get_instance; -} - -sub _get_instance { - my $cache = Bugzilla->request_cache; - if (!$cache->{'phabbugz.instance'}) { - my $instance = Bugzilla::Extension::PhabBugz::Feed->new(); - $cache->{'phabbugz.instance'} = $instance; - $instance->logger(Bugzilla::Extension::PhabBugz::Logger->new()); - } - return $cache->{'phabbugz.instance'}; + *Bugzilla::User::phab_phid = sub { return $_[0]->{phab_phid}; }; + *Bugzilla::User::phab_review_status = sub { return $_[0]->{phab_review_status}; }; } sub config_add_panels { diff --git a/extensions/PhabBugz/bin/phabbugz_feed.pl b/extensions/PhabBugz/bin/phabbugz_feed.pl index 7e11885f8e..9db491bd07 100755 --- a/extensions/PhabBugz/bin/phabbugz_feed.pl +++ b/extensions/PhabBugz/bin/phabbugz_feed.pl @@ -7,9 +7,9 @@ # This Source Code Form is "Incompatible With Secondary Licenses", as # defined by the Mozilla Public License, v. 2.0. +use 5.10.1; use strict; use warnings; -use 5.10.1; use lib qw(. lib local/lib/perl5); @@ -23,28 +23,28 @@ BEGIN =head1 NAME -phabbugzd.pl - Query Phabricator for interesting changes and update bugs related to revisions. +phabbugz_feed.pl - Query Phabricator for interesting changes and update bugs related to revisions. =head1 SYNOPSIS - phabbugzd.pl [OPTIONS] COMMAND + phabbugz_feed.pl [OPTIONS] COMMAND OPTIONS: -f Run in the foreground (don't detach) -d Output a lot of debugging information - -p file Specify the file where phabbugzd.pl should store its current - process id. Defaults to F. + -p file Specify the file where phabbugz_feed.pl should store its current + process id. Defaults to F. -n name What should this process call itself in the system log? Defaults to the full path you used to invoke the script. COMMANDS: - start Starts a new phabbugzd daemon if there isn't one running already - stop Stops a running phabbugzd daemon - restart Stops a running phabbugzd if one is running, and then + start Starts a new phabbugz_feed daemon if there isn't one running already + stop Stops a running phabbugz_feed daemon + restart Stops a running phabbugz_feed if one is running, and then starts a new one. check Report the current status of the daemon. install On some *nix systems, this automatically installs and - configures phabbugzd.pl as a system service so that it will + configures phabbugz_feed.pl as a system service so that it will start every time the machine boots. - uninstall Removes the system service for phabbugzd.pl. - help Display this usage info \ No newline at end of file + uninstall Removes the system service for phabbugz_feed.pl. + help Display this usage info diff --git a/extensions/PhabBugz/lib/Daemon.pm b/extensions/PhabBugz/lib/Daemon.pm index bacc39e8e6..c8b4f73af9 100644 --- a/extensions/PhabBugz/lib/Daemon.pm +++ b/extensions/PhabBugz/lib/Daemon.pm @@ -12,9 +12,13 @@ use strict; use warnings; use Bugzilla::Constants; +use Bugzilla::Extension::PhabBugz::Feed; +use Bugzilla::Extension::PhabBugz::Logger; + use Carp qw(confess); use Daemon::Generic; use File::Basename; +use File::Spec; use Pod::Usage; sub start { @@ -29,7 +33,7 @@ sub gd_preconfig { my $self = shift; my $pidfile = $self->{gd_args}{pidfile}; if (!$pidfile) { - $pidfile = bz_locations()->{datadir} . '/' . $self->{gd_progname} . ".pid"; + $pidfile = File::Spec->catfile(bz_locations()->{datadir}, $self->{gd_progname} . ".pid"); } return (pidfile => $pidfile); } @@ -67,7 +71,7 @@ sub gd_usage { sub gd_redirect_output { my $self = shift; - my $filename = bz_locations()->{datadir} . '/' . $self->{gd_progname} . ".log"; + my $filename = File::Spec->catfile(bz_locations()->{datadir}, $self->{gd_progname} . ".log"); open(STDERR, ">>", $filename) or (print "could not open stderr: $!" && exit(1)); close(STDOUT); open(STDOUT, ">&", STDERR) or die "redirect STDOUT -> STDERR: $!"; @@ -86,9 +90,10 @@ sub gd_setup_signals { sub gd_run { my $self = shift; $::SIG{__DIE__} = \&Carp::confess if $self->{debug}; - my $phabbugz = Bugzilla->phabbugz_ext; + my $phabbugz = Bugzilla::Extension::PhabBugz::Feed->new(); $phabbugz->is_daemon(1); - $phabbugz->logger->{debug} = $self->{debug}; + $phabbugz->logger( + Bugzilla::Extension::PhabBugz::Logger->new(debugging => $self->{debug})); $phabbugz->start(); } diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index fc201e19c1..321ecfd9fb 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -8,41 +8,34 @@ package Bugzilla::Extension::PhabBugz::Feed; use 5.10.1; -use strict; -use warnings; -use Bugzilla::Extension::PhabBugz::Constants; -use Bugzilla::Extension::PhabBugz::Util; - -sub new { - my ($class) = @_; - my $self = {}; - bless($self, $class); - $self->{is_daemon} = 0; - return $self; -} - -sub is_daemon { - my ($self, $value) = @_; - if (defined $value) { - $self->{is_daemon} = $value ? 1 : 0; - } - return $self->{is_daemon}; -} +use Moo; -sub logger { - my ($self, $value) = @_; - $self->{logger} = $value if $value; - return $self->{logger}; -} +use Bugzilla::Extension::PhabBugz::Constants; +use Bugzilla::Extension::PhabBugz::Revision; +use Bugzilla::Extension::PhabBugz::Util qw( + add_security_sync_comments + create_revision_attachment + create_private_revision_policy + edit_revision_policy + get_bug_role_phids + get_members_by_phid + make_revision_public + request + get_security_sync_groups +); + +has 'is_daemon' => ( is => 'rw', default => 0 ); +has 'logger' => ( is => 'rw' ); sub start { my ($self) = @_; - while(1) { - if ($self->_dbh_check()) { + while (1) { + if (Bugzilla->params->{phabricator_enabled}) { $self->feed_query(); } sleep(PHAB_POLL_SECONDS); + Bugzilla->_cleanup(); } } @@ -50,13 +43,21 @@ sub feed_query { my ($self) = @_; my $dbh = Bugzilla->dbh; - $self->logger->info("FEED: Polling"); + # Ensure Phabricator syncing is enabled + if (!Bugzilla->params->{phabricator_enabled}) { + $self->logger->info("PHABRICATOR SYNC DISABLED"); + return; + } + + $self->logger->info("FEED: Fetching new transactions"); my $last_ts = $dbh->selectrow_array(" SELECT value FROM phabbugz WHERE name = 'feed_last_ts'"); + $last_ts ||= 0; + $self->logger->debug("LAST_TS: $last_ts"); # Check for new transctions (stories) - my $transactions = get_feed_transactions($last_ts+1); + my $transactions = $self->feed_transactions($last_ts); if (!$transactions) { $self->logger->info("FEED: No new transactions"); return; @@ -64,41 +65,88 @@ sub feed_query { # Process each story foreach my $story (keys %$transactions) { - $self->logger->info("STORY: $story"); + my $skip = 0; + + $self->logger->debug("STORY: $story"); my $story_data = $transactions->{$story}; my $object_phid = $story_data->{objectPHID}; - $self->logger->info("OBJECT: $object_phid"); + $self->logger->debug("OBJECT: $object_phid"); + + # Only interested in changes to revisions for now. if ($object_phid !~ /^PHID-DREV/) { - $self->logger->info("SKIP: Not a revision change"); - next; + $self->logger->debug("SKIP: Not a revision change"); + $skip = 1; } - my ($revision) = get_revisions_by_phids([$object_phid]); - $self->logger->info("REVSION: " . $revision->{'id'} . ": " . - $revision->{'fields'}->{'title'} . " " . - $revision->{'fields'}->{'bugzilla.bug-id'} . " " . - $story_data->{'text'}); - - # Find the highest epoch for storage in feed_last_ts - if ($story_data->{epoch} > $last_ts) { - $last_ts = $story_data->{epoch}; + + # Skip changes done by phab-bot user + my $userids = get_members_by_phid([$story_data->{authorPHID}]); + if (@$userids) { + my $user = Bugzilla::User->new({ id => $userids->[0], cache => 1 }); + $skip = 1 if $user->login eq PHAB_AUTOMATION_USER; } - } - $self->logger->debug("LAST_TS: $last_ts"); - $dbh->do("REPLACE INTO phabbugz (name, value) VALUES ('feed_last_ts', ?)", - undef, $last_ts); + if (!$skip) { + my $revision = Bugzilla::Extension::PhabBugz::Revision->new({ phids => [$object_phid] }); + $self->process_revision_change($revision, $story_data->{text}); + } + + # Store the largest last epoch so we can start from there in the next session + $self->logger->debug("UPDATING LAST_TS: $last_ts"); + $dbh->do("REPLACE INTO phabbugz (name, value) VALUES ('feed_last_ts', ?)", + undef, $story_data->{epoch}+1); + } } -sub _dbh_check { - my ($self) = @_; - eval { - Bugzilla->dbh->selectrow_array("SELECT 1 FROM phabbugz"); - }; - if ($@) { - return 0; - } else { - return 1; +sub process_revision_change { + my ($self, $revision, $story_text) = @_; + + Bugzilla->set_user(Bugzilla::User->new({ name => PHAB_AUTOMATION_USER })); + + my $revision_id = $revision->id; + my $revision_phid = $revision->phid; + my $revision_title = $revision->title || 'Unknown Description'; + my $bug_id = $revision->bug_id; + + $self->logger->info("REVISION CHANGE FOUND: D$revision_id: $revision_title | bug: $bug_id | $story_text"); + + my $bug = Bugzilla::Bug->new($bug_id); + + # If bug is public then remove privacy policy + my $result; + if (!@{ $bug->groups_in }) { + $result = make_revision_public($revision_id); } + # 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) { + add_security_sync_comments([$revision], $bug); + } + + my $policy_phid = create_private_revision_policy($bug, \@set_groups); + my $subscribers = get_bug_role_phids($bug); + $result = edit_revision_policy($revision_phid, $policy_phid, $subscribers); + } + + my $attachment = create_revision_attachment($bug, $revision_id, $revision_title); + + Bugzilla::BugMail::Send($bug_id, { changer => Bugzilla->user }); + + $self->logger->info("SUCCESS"); +} + +sub feed_transactions { + my ($self, $epoch) = @_; + my $data = { view => 'text' }; + $data->{epochStart} = $epoch if $epoch; + my $result = request('feed.query_epoch', $data); + # Stupid conduit. If the feed results are empty it returns + # an empty list ([]). If there is data it returns it in a + # hash ({}) so we have adjust to be consistent. + return ref $result->{result} eq 'HASH' ? $result->{result} : {}; } 1; diff --git a/extensions/PhabBugz/lib/Logger.pm b/extensions/PhabBugz/lib/Logger.pm index 9ddacd8f7d..3127b66db8 100644 --- a/extensions/PhabBugz/lib/Logger.pm +++ b/extensions/PhabBugz/lib/Logger.pm @@ -8,36 +8,27 @@ package Bugzilla::Extension::PhabBugz::Logger; use 5.10.1; -use strict; -use warnings; + +use Moo; use Bugzilla::Extension::PhabBugz::Constants; -sub new { - my ($class) = @_; - my $self = {}; - bless($self, $class); - return $self; -} +has 'debugging' => ( is => 'ro' ); sub info { shift->_log_it('INFO', @_) } sub error { shift->_log_it('ERROR', @_) } sub debug { shift->_log_it('DEBUG', @_) } -sub debugging { - my ($self) = @_; - return $self->{debug}; -} - sub _log_it { - require Apache2::Log; my ($self, $method, $message) = @_; + return if $method eq 'DEBUG' && !$self->debugging; chomp $message; if ($ENV{MOD_PERL}) { - Apache2::ServerRec::warn("Push $method: $message"); + require Apache2::Log; + Apache2::ServerRec::warn("FEED $method: $message"); } elsif ($ENV{SCRIPT_FILENAME}) { - print STDERR "Push $method: $message\n"; + print STDERR "FEED $method: $message\n"; } else { print STDERR '[' . localtime(time) ."] $method: $message\n"; } diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm new file mode 100644 index 0000000000..464a4a720f --- /dev/null +++ b/extensions/PhabBugz/lib/Revision.pm @@ -0,0 +1,266 @@ +# 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::Revision; + +use 5.10.1; + +use Moo; + +use Bugzilla::Bug; +use Bugzilla::Error; +use Bugzilla::Util qw(trim); +use Bugzilla::Extension::PhabBugz::Util qw( + request + get_members_by_phid +); + +use parent qw(Bugzilla::Object); + +######################### +# Initialization # +######################### + +# This is an external object so we do not want any auditing. +use constant AUDIT_CREATES => 0; +use constant AUDIT_UPDATES => 0; + +sub new { + my ($class, $params) = @_; + my $self = $params ? _load($params) : {}; + bless($self, $class); + return $self; +} + +sub _load { + my ($params) = @_; + + my $data = { + queryKey => 'all', + attachments => { + projects => 1, + reviewers => 1, + subscribers => 1 + } + }; + + if ($params->{ids}) { + $data->{constraints} = { + ids => $params->{ids} + }; + } + elsif ($params->{phids}) { + $data->{constraints} = { + phids => $params->{phids} + }; + } + else { + ThrowUserError('invalid_phabricator_revision_id'); + } + + my $result = request('differential.revision.search', $data); + + ThrowUserError('invalid_phabricator_revision_id') + unless (exists $result->{result}{data} && @{ $result->{result}{data} }); + + return $result->{result}->{data}->[0]; +} + +# { +# "data": [ +# { +# "id": 25, +# "type": "DREV", +# "phid": "PHID-DREV-uozm3ggfp7e7uoqegmc3", +# "fields": { +# "title": "Added .arcconfig", +# "authorPHID": "PHID-USER-4wigy3sh5fc5t74vapwm", +# "dateCreated": 1507666113, +# "dateModified": 1508514027, +# "policy": { +# "view": "public", +# "edit": "admin" +# }, +# "bugzilla.bug-id": "1154784" +# }, +# "attachments": { +# "reviewers": { +# "reviewers": [ +# { +# "reviewerPHID": "PHID-USER-2gjdpu7thmpjxxnp7tjq", +# "status": "added", +# "isBlocking": false, +# "actorPHID": null +# }, +# { +# "reviewerPHID": "PHID-USER-o5dnet6dp4dkxkg5b3ox", +# "status": "rejected", +# "isBlocking": false, +# "actorPHID": "PHID-USER-o5dnet6dp4dkxkg5b3ox" +# } +# ] +# }, +# "subscribers": { +# "subscriberPHIDs": [], +# "subscriberCount": 0, +# "viewerIsSubscribed": true +# }, +# "projects": { +# "projectPHIDs": [] +# } +# } +# } +# ], +# "maps": {}, +# "query": { +# "queryKey": null +# }, +# "cursor": { +# "limit": 100, +# "after": null, +# "before": null, +# "order": null +# } +# } + +######################### +# Modification # +######################### + +sub update { + my ($self) = @_; + + if ($self->{added_comments}) { + foreach my $comment (@{ $self->{added_comments} }) { + my $data = { + transactions => [ + { + type => 'comment', + value => $comment + } + ], + objectIdentifier => $self->phid + }; + my $result = request('differential.revision.edit', $data); + } + } + + if ($self->{set_subscribers}) { + my $data = { + transactions => [ + { + type => 'subscribers.set', + value => $self->{set_subscribers} + } + ], + objectIdentifier => $self->phid + }; + + my $result = request('differential.revision.edit', $data); + } +} + +######################### +# Accessors # +######################### + +sub id { return $_[0]->{id}; } +sub phid { return $_[0]->{phid}; } +sub title { return $_[0]->{fields}->{title}; } +sub creation_ts { return $_[0]->{fields}->{dateCreated}; } +sub modification_ts { return $_[0]->{fields}->{dateModified}; } +sub author_phid { return $_[0]->{fields}->{authorPHID}; } +sub bug_id { return $_[0]->{fields}->{'bugzilla.bug-id'}; } + +sub view_policy { return $_[0]->{fields}->{policy}->{view}; } +sub edit_policy { return $_[0]->{fields}->{policy}->{edit}; } + +sub reviewers_raw { return $_[0]->{atachments}->{reviewers}->{reviewers}; } +sub subscribers_raw { return $_[0]->{attachments}->{subscribers}; } +sub projects_raw { return $_[0]->{attachments}->{projects}; } +sub subscriber_count { return $_[0]->{attachments}->{subscribers}->{subscriberCount}; } + +sub bug { + my $self = shift; + my $bug = $self->{bug} ||= new Bugzilla::Bug($self->bug_id); + weaken($self->{bug}) unless isweak($self->{bug}); + return $bug; +} + +sub author { + my $self = shift; + return $self->{author} if $self->{author}; + my $userids = get_members_by_phid([$self->author_phid]); + $self->{'author'} = new Bugzilla::User({ id => $userids->[0], cache => 1 }); + return $self->{'author'}; +} + +sub reviewers { + my ($self) = @_; + return $self->{reviewers} if $self->{reviewers}; + + my @phids; + foreach my $reviewer (@{ $self->reviewers_raw }) { + push(@phids, $reviewer->{reviewerPHID}); + } + + my $userids = get_members_by_phid(\@phids); + + my @reviewers; + my $i = 0; + foreach my $userid (@$userids) { + my $reviewer = Bugzilla::User->new({ id => $userid, cache => 1}); + $reviewer->{phab_review_status} = $self->reviewers_raw->[$i]->{status}; + $reviewer->{phab_phid} = $self->reviewers_raw->[$i]->{reviewerPHID}; + push(@reviewers, $reviewer); + $i++; + } + + return \@reviewers; +} + +sub subscribers { + my ($self) = @_; + return $self->{subscribers} if $self->{subscribers}; + + my @phids; + foreach my $phid (@{ $self->subscribers_raw->{subscriberPHIDs} }) { + push(@phids, $phid); + } + + my $userids = get_members_by_phid(\@phids); + + my @subscribers; + my $i = 0; + foreach my $userid (@$userids) { + my $subscriber = Bugzilla::User->new({ id => $userid, cache => 1}); + $subscriber->{phab_phid} = $self->subscribers_raw->{subscriberPHIDs}->[$i]; + push(@subscribers, $subscriber); + $i++; + } + + return \@subscribers; + +} + +######################### +# Mutators # +######################### + +sub add_comment { + my ($self, $comment) = @_; + $comment = trim($comment); + $self->{added_comments} ||= []; + push(@{ $self->{added_comments} }, $comment); +} + +sub set_subscribers { + my ($self, $subscribers) = @_; + $self->{set_subscribers} = $subscribers; +} + +1; \ No newline at end of file diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index cc2b1722bf..bc31117dac 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -33,8 +33,8 @@ our @EXPORT = qw( edit_revision_policy get_attachment_revisions get_bug_role_phids - get_feed_transactions get_members_by_bmo_id + get_members_by_phid get_project_phid get_revisions_by_ids get_revisions_by_phids @@ -81,7 +81,7 @@ sub get_revisions_by_phids { ThrowUserError('invalid_phabricator_revision_id') unless (exists $result->{result}{data} && @{ $result->{result}{data} }); - return @{$result->{result}{data}}; + return $result->{result}{data}; } sub create_revision_attachment { @@ -358,6 +358,22 @@ sub get_members_by_bmo_id { return \@phab_ids; } +sub get_members_by_phid { + my $phids = shift; + + my $data = { phids => $phids }; + + my $result = request('bugzilla.account.search', $data); + + my @bmo_ids; + foreach my $user (@{ $result->{result} }) { + push(@bmo_ids, $user->{id}) + if ($user->{phid} && $user->{phid} =~ /^PHID-USER/); + } + + return \@bmo_ids; +} + sub is_attachment_phab_revision { my ($attachment, $include_obsolete) = @_; return ($attachment->contenttype eq PHAB_CONTENT_TYPE @@ -477,15 +493,4 @@ sub add_security_sync_comments { Bugzilla->set_user($old_user); } -sub get_feed_transactions { - my ($epoch) = @_; - my $data = { view => 'text' }; - $data->{epochStart} = $epoch if $epoch; - my $result = request('feed.query_epoch', $data); - # Stupid conduit. If the feed results are empty it returns - # an empty list ([]). If there is data it returns it in a - # hash ({}) so we have adjust to be consistent. - return ref $result->{result} eq 'HASH' ? $result->{result} : {}; -} - 1; From 67ac4de9827b2e06ba1dd0d70d9ced8aa79cc5b9 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 2 Nov 2017 13:34:56 -0400 Subject: [PATCH 05/13] Fixed merge issue with PhabBugz/lib/Util.pm --- extensions/PhabBugz/lib/Util.pm | 22 ---------------------- 1 file changed, 22 deletions(-) diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index c24c7c7672..bc31117dac 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -66,7 +66,6 @@ sub get_revisions_by_ids { return @{$result->{result}{data}}; } -<<<<<<< HEAD sub get_revisions_by_phids { my ($phids) = @_; @@ -85,27 +84,6 @@ sub get_revisions_by_phids { return $result->{result}{data}; } -||||||| merged common ancestors -======= -sub get_revisions_by_phids { - my ($phids) = @_; - - my $data = { - queryKey => 'all', - constraints => { - phids => $phids - } - }; - - my $result = request('differential.revision.search', $data); - - ThrowUserError('invalid_phabricator_revision_id') - unless (exists $result->{result}{data} && @{ $result->{result}{data} }); - - return @{$result->{result}{data}}; -} - ->>>>>>> e969e034646a97750d13e66210f50c842ede4b8c sub create_revision_attachment { my ( $bug, $revision_id, $revision_title ) = @_; From c6ba40301af4a84628dccfb10c31aa5951fdf94b Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Fri, 3 Nov 2017 14:23:58 -0400 Subject: [PATCH 06/13] - Added Project.pm for wrapping phabricator projects. Can be used for loading existing projects and creating new ones. - More cleanup and bug fixes. --- extensions/PhabBugz/lib/Feed.pm | 44 +++-- extensions/PhabBugz/lib/Project.pm | 283 ++++++++++++++++++++++++++++ extensions/PhabBugz/lib/Revision.pm | 128 +++++++------ extensions/PhabBugz/lib/Util.pm | 55 +++--- 4 files changed, 407 insertions(+), 103 deletions(-) create mode 100644 extensions/PhabBugz/lib/Project.pm diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index 321ecfd9fb..86f937463c 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -15,14 +15,15 @@ use Bugzilla::Extension::PhabBugz::Constants; use Bugzilla::Extension::PhabBugz::Revision; use Bugzilla::Extension::PhabBugz::Util qw( add_security_sync_comments - create_revision_attachment create_private_revision_policy + create_revision_attachment edit_revision_policy get_bug_role_phids get_members_by_phid + get_security_sync_groups make_revision_public request - get_security_sync_groups + set_phab_user ); has 'is_daemon' => ( is => 'rw', default => 0 ); @@ -80,6 +81,7 @@ sub feed_query { # Skip changes done by phab-bot user my $userids = get_members_by_phid([$story_data->{authorPHID}]); + if (@$userids) { my $user = Bugzilla::User->new({ id => $userids->[0], cache => 1 }); $skip = 1 if $user->login eq PHAB_AUTOMATION_USER; @@ -89,6 +91,9 @@ sub feed_query { my $revision = Bugzilla::Extension::PhabBugz::Revision->new({ phids => [$object_phid] }); $self->process_revision_change($revision, $story_data->{text}); } + else { + $self->logger->info('SKIPPING'); + } # Store the largest last epoch so we can start from there in the next session $self->logger->debug("UPDATING LAST_TS: $last_ts"); @@ -100,21 +105,23 @@ sub feed_query { sub process_revision_change { my ($self, $revision, $story_text) = @_; - Bugzilla->set_user(Bugzilla::User->new({ name => PHAB_AUTOMATION_USER })); - - my $revision_id = $revision->id; - my $revision_phid = $revision->phid; - my $revision_title = $revision->title || 'Unknown Description'; - my $bug_id = $revision->bug_id; + my $old_user = set_phab_user(); - $self->logger->info("REVISION CHANGE FOUND: D$revision_id: $revision_title | bug: $bug_id | $story_text"); + my $log_message = sprintf( + "REVISION CHANGE FOUND: D%d: %s | bug: %d | %s", + $revision->id, + $revision->title, + $revision->bug_id, + $story_text); + $self->logger->info($log_message); - my $bug = Bugzilla::Bug->new($bug_id); + my $bug = Bugzilla::Bug->new($revision->bug_id); # If bug is public then remove privacy policy my $result; if (!@{ $bug->groups_in }) { - $result = make_revision_public($revision_id); + $revision->set_policy('view', 'public'); + $revision->set_policy('edit', 'users'); } # else bug is private else { @@ -123,17 +130,24 @@ sub process_revision_change { # 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) { - add_security_sync_comments([$revision], $bug); + add_security_sync_comments($revision, $bug); } my $policy_phid = create_private_revision_policy($bug, \@set_groups); my $subscribers = get_bug_role_phids($bug); - $result = edit_revision_policy($revision_phid, $policy_phid, $subscribers); + + $revision->set_policy('view', $policy_phid); + $revision->set_policy('edit', $policy_phid); + $revision->set_subscribers($subscribers); } - my $attachment = create_revision_attachment($bug, $revision_id, $revision_title); + $revision->update(); + + my $attachment = create_revision_attachment($bug, $revision->id, $revision->title); + + Bugzilla::BugMail::Send($revision->bug_id, { changer => Bugzilla->user }); - Bugzilla::BugMail::Send($bug_id, { changer => Bugzilla->user }); + Bugzilla->set_user($old_user); $self->logger->info("SUCCESS"); } diff --git a/extensions/PhabBugz/lib/Project.pm b/extensions/PhabBugz/lib/Project.pm new file mode 100644 index 0000000000..c9aedec00e --- /dev/null +++ b/extensions/PhabBugz/lib/Project.pm @@ -0,0 +1,283 @@ +# 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::Project; + +use 5.10.1; + +use Moo; + +use Bugzilla::Error; +use Bugzilla::Util qw(trim); +use Bugzilla::Extension::PhabBugz::Util qw( + request + get_phab_bmo_ids +); + +######################### +# Initialization # +######################### + +sub new { + my ($class, $params) = @_; + my $self = $params ? _load($params) : {}; + bless($self, $class); + return $self; +} + +sub _load { + my ($params) = @_; + + my $data = { + queryKey => 'all', + attachments => { + projects => 1, + reviewers => 1, + subscribers => 1 + } + }; + + if ($params->{ids}) { + $data->{constraints} = { + ids => $params->{ids} + }; + } + elsif ($params->{phids}) { + $data->{constraints} = { + phids => $params->{phids} + }; + } + else { + return {}; + } + + my $result = request('project.search', $data); + if (exists $result->{result}{data} && @{ $result->{result}{data} }) { + return $result->{result}->{data}->[0]; + } + + return {}; +} + +# { +# "data": [ +# { +# "id": 1, +# "type": "PROJ", +# "phid": "PHID-PROJ-pfssn7lndryddv7hbx4i", +# "fields": { +# "name": "bmo-core-security", +# "slug": "bmo-core-security", +# "milestone": null, +# "depth": 0, +# "parent": null, +# "icon": { +# "key": "group", +# "name": "Group", +# "icon": "fa-users" +# }, +# "color": { +# "key": "red", +# "name": "Red" +# }, +# "dateCreated": 1500403964, +# "dateModified": 1505248862, +# "policy": { +# "view": "admin", +# "edit": "admin", +# "join": "admin" +# }, +# "description": "BMO Security Group for core-security" +# }, +# "attachments": { +# "members": { +# "members": [ +# { +# "phid": "PHID-USER-23ia7vewbjgcqahewncu" +# }, +# { +# "phid": "PHID-USER-uif2miph2poiehjeqn5q" +# } +# ] +# }, +# "ancestors": { +# "ancestors": [] +# }, +# "watchers": { +# "watchers": [] +# } +# } +# } +# ], +# "maps": { +# "slugMap": {} +# }, +# "query": { +# "queryKey": null +# }, +# "cursor": { +# "limit": 100, +# "after": null, +# "before": null, +# "order": null +# } +# } + +######################### +# Modification # +######################### + +sub create { + my ($class, $params) = @_; + + my $name = trim($params->{name}); + $name || ThrowCodeError('param_required', { param => 'name' }); + + my $description = $params->{description} || 'Need description'; + my $view_policy = $params->{view_policy} || 'admin'; + my $edit_policy = $params->{edit_policy} || 'admin'; + my $join_policy = $params->{join_policy} || 'admin'; + + my $data = { + transactions => [ + { type => 'name', value => $name }, + { type => 'description', value => $description }, + { type => 'edit', value => $edit_policy }, + { type => 'join', value => $join_policy }, + { type => 'view', value => $view_policy }, + { type => 'icon', value => 'group' }, + { type => 'color', value => 'red' } + ] + }; + + my $result = request('project.edit', $data); + return $class->new({ phids => $result->{result}{object}{phid} }); +} + +sub update { + my ($self) = @_; + + my $data = { + objectIdentifier => $self->phid, + transactions => [] + }; + + if ($self->{set_name}) { + push(@{ $data->{transactions} }, { + type => 'name', + value => $self->{set_name} + }); + } + + if ($self->{set_description}) { + push(@{ $data->{transactions} }, { + type => 'description', + value => $self->{set_description} + }); + } + + if ($self->{added_members}) { + push(@{ $data->{transactions} }, { + type => 'members.add', + value => $self->{added_members} + }); + } + + if ($self->{removed_members}) { + push(@{ $data->{transactions} }, { + type => 'members.remove', + value => $self->{removed_members} + }); + } + + if ($self->{set_policy}) { + foreach my $name ("view", "edit") { + next unless $self->{set_policy}->{$name}; + push(@{ $data->{transactions} }, { + type => $name, + value => $self->{set_policy}->{$name} + }); + } + } + + request('differential.project.edit', $data); +} + +######################### +# Accessors # +######################### + +sub id { return $_[0]->{id}; } +sub phid { return $_[0]->{phid}; } +sub type { return $_[0]->{type}; } +sub name { return $_[0]->{fields}->{name}; } +sub description { return $_[0]->{fields}->{description}; } +sub creation_ts { return $_[0]->{fields}->{dateCreated}; } +sub modification_ts { return $_[0]->{fields}->{dateModified}; } + +sub view_policy { return $_[0]->{fields}->{policy}->{view}; } +sub edit_policy { return $_[0]->{fields}->{policy}->{edit}; } +sub join_policy { return $_[0]->{fields}->{policy}->{join}; } + +sub members_raw { return $_[0]->{atachments}->{members}->{members}; } + +sub members { + my ($self) = @_; + return $self->{members} if $self->{members}; + + my @phids; + foreach my $member (@{ $self->members_raw }) { + push(@phids, $member->{phid}); + } + + my $users = get_phab_bmo_ids({ 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; +} + +######################### +# Mutators # +######################### + +sub set_name { + my ($self, $name) = @_; + $name = trim($name); + $self->{set_name} = $name; +} + +sub set_description { + my ($self, $description) = @_; + $description = trim($description); + $self->{set_description} = $description; +} + +sub add_member { + my ($self, $member) = @_; + $self->{added_members} ||= []; + push(@{ $self->{added_members} }, $member->phab_phid); +} + +sub remove_member { + my ($self, $member) = @_; + $self->{removed_members} ||= []; + push(@{ $self->{removed_members} }, $member->phab_phid); +} + +sub set_policy { + my ($self, $name, $policy) = @_; + $self->{set_policy} ||= {}; + $self->{set_policy}->{$name} = $policy; +} + +1; \ No newline at end of file diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm index 464a4a720f..f2b7b35a41 100644 --- a/extensions/PhabBugz/lib/Revision.pm +++ b/extensions/PhabBugz/lib/Revision.pm @@ -1,4 +1,4 @@ -# This Source Code Form is subject to the terms of the Mozilla Public +# This Source Code Form is hasject 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/. # @@ -15,20 +15,14 @@ use Bugzilla::Bug; use Bugzilla::Error; use Bugzilla::Util qw(trim); use Bugzilla::Extension::PhabBugz::Util qw( + get_phab_bmo_ids request - get_members_by_phid ); -use parent qw(Bugzilla::Object); - ######################### # Initialization # ######################### -# This is an external object so we do not want any auditing. -use constant AUDIT_CREATES => 0; -use constant AUDIT_UPDATES => 0; - sub new { my ($class, $params) = @_; my $self = $params ? _load($params) : {}; @@ -59,15 +53,15 @@ sub _load { }; } else { - ThrowUserError('invalid_phabricator_revision_id'); + return {}; } my $result = request('differential.revision.search', $data); + if (exists $result->{result}{data} && @{ $result->{result}{data} }) { + return $result->{result}->{data}->[0]; + } - ThrowUserError('invalid_phabricator_revision_id') - unless (exists $result->{result}{data} && @{ $result->{result}{data} }); - - return $result->{result}->{data}->[0]; + return {}; } # { @@ -134,65 +128,69 @@ sub _load { sub update { my ($self) = @_; + my $data = { + objectIdentifier => $self->phid, + transactions => [] + }; + if ($self->{added_comments}) { foreach my $comment (@{ $self->{added_comments} }) { - my $data = { - transactions => [ - { - type => 'comment', - value => $comment - } - ], - objectIdentifier => $self->phid - }; - my $result = request('differential.revision.edit', $data); + push(@{ $data->{transactions} }, { + type => 'comment', + value => $comment + }); } } if ($self->{set_subscribers}) { - my $data = { - transactions => [ - { - type => 'subscribers.set', - value => $self->{set_subscribers} - } - ], - objectIdentifier => $self->phid - }; + push(@{ $data->{transactions} }, { + type => 'subscribers.set', + value => $self->{set_subscribers} + }); + } - my $result = request('differential.revision.edit', $data); + if ($self->{set_policy}) { + foreach my $name ("view", "edit") { + next unless $self->{set_policy}->{$name}; + push(@{ $data->{transactions} }, { + type => $name, + value => $self->{set_policy}->{$name} + }); + } } + + request('differential.revision.edit', $data); } ######################### # Accessors # ######################### -sub id { return $_[0]->{id}; } -sub phid { return $_[0]->{phid}; } -sub title { return $_[0]->{fields}->{title}; } -sub creation_ts { return $_[0]->{fields}->{dateCreated}; } -sub modification_ts { return $_[0]->{fields}->{dateModified}; } -sub author_phid { return $_[0]->{fields}->{authorPHID}; } -sub bug_id { return $_[0]->{fields}->{'bugzilla.bug-id'}; } +sub id { $_[0]->{id}; } +sub phid { $_[0]->{phid}; } +sub title { $_[0]->{fields}->{title}; } +sub creation_ts { $_[0]->{fields}->{dateCreated}; } +sub modification_ts { $_[0]->{fields}->{dateModified}; } +sub author_phid { $_[0]->{fields}->{authorPHID}; } +sub bug_id { $_[0]->{fields}->{'bugzilla.bug-id'}; } -sub view_policy { return $_[0]->{fields}->{policy}->{view}; } -sub edit_policy { return $_[0]->{fields}->{policy}->{edit}; } +sub view_policy { $_[0]->{fields}->{policy}->{view}; } +sub edit_policy { $_[0]->{fields}->{policy}->{edit}; } -sub reviewers_raw { return $_[0]->{atachments}->{reviewers}->{reviewers}; } -sub subscribers_raw { return $_[0]->{attachments}->{subscribers}; } -sub projects_raw { return $_[0]->{attachments}->{projects}; } -sub subscriber_count { return $_[0]->{attachments}->{subscribers}->{subscriberCount}; } +sub reviewers_raw { $_[0]->{atachments}->{reviewers}->{reviewers}; } +sub subscribers_raw { $_[0]->{attachments}->{subscribers}; } +sub projects_raw { $_[0]->{attachments}->{projects}; } +sub subscriber_count { $_[0]->{attachments}->{subscribers}->{subscriberCount}; } sub bug { - my $self = shift; + my ($self) = @_; my $bug = $self->{bug} ||= new Bugzilla::Bug($self->bug_id); weaken($self->{bug}) unless isweak($self->{bug}); return $bug; } sub author { - my $self = shift; + my ($self) = @_; return $self->{author} if $self->{author}; my $userids = get_members_by_phid([$self->author_phid]); $self->{'author'} = new Bugzilla::User({ id => $userids->[0], cache => 1 }); @@ -208,16 +206,19 @@ sub reviewers { push(@phids, $reviewer->{reviewerPHID}); } - my $userids = get_members_by_phid(\@phids); + my $users = get_phab_bmo_ids({ phids => \@phids }); my @reviewers; - my $i = 0; - foreach my $userid (@$userids) { - my $reviewer = Bugzilla::User->new({ id => $userid, cache => 1}); - $reviewer->{phab_review_status} = $self->reviewers_raw->[$i]->{status}; - $reviewer->{phab_phid} = $self->reviewers_raw->[$i]->{reviewerPHID}; + foreach my $user (@$users) { + my $reviewer = Bugzilla::User->new({ id => $user->{id}, cache => 1}); + $reviewer->{phab_phid} = $user->{phid}; + foreach my $reviewer_data ($self->reviews_raw) { + if ($reviewer_data->{reviewerPHID} eq $user->{phid}) { + $reviewer->{phab_review_status} = $reviewer_data->{status}; + last; + } + } push(@reviewers, $reviewer); - $i++; } return \@reviewers; @@ -232,19 +233,16 @@ sub subscribers { push(@phids, $phid); } - my $userids = get_members_by_phid(\@phids); + my $users = get_phab_bmo_ids({ phids => \@phids }); my @subscribers; - my $i = 0; - foreach my $userid (@$userids) { - my $subscriber = Bugzilla::User->new({ id => $userid, cache => 1}); - $subscriber->{phab_phid} = $self->subscribers_raw->{subscriberPHIDs}->[$i]; + foreach my $user (@$users) { + my $subscriber = Bugzilla::User->new({ id => $user->{id}, cache => 1}); + $subscriber->{phab_phid} = $user->{phid}; push(@subscribers, $subscriber); - $i++; } return \@subscribers; - } ######################### @@ -263,4 +261,10 @@ sub set_subscribers { $self->{set_subscribers} = $subscribers; } +sub set_policy { + my ($self, $name, $policy) = @_; + $self->{set_policy} ||= {}; + $self->{set_policy}->{$name} = $policy; +} + 1; \ No newline at end of file diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index c24c7c7672..02cf5e245b 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -35,6 +35,7 @@ our @EXPORT = qw( get_bug_role_phids get_members_by_bmo_id get_members_by_phid + get_phab_bmo_ids get_project_phid get_revisions_by_ids get_revisions_by_phids @@ -44,6 +45,7 @@ our @EXPORT = qw( make_revision_private make_revision_public request + set_phab_user set_project_members set_revision_subscribers ); @@ -66,7 +68,6 @@ sub get_revisions_by_ids { return @{$result->{result}{data}}; } -<<<<<<< HEAD sub get_revisions_by_phids { my ($phids) = @_; @@ -85,27 +86,6 @@ sub get_revisions_by_phids { return $result->{result}{data}; } -||||||| merged common ancestors -======= -sub get_revisions_by_phids { - my ($phids) = @_; - - my $data = { - queryKey => 'all', - constraints => { - phids => $phids - } - }; - - my $result = request('differential.revision.search', $data); - - ThrowUserError('invalid_phabricator_revision_id') - unless (exists $result->{result}{data} && @{ $result->{result}{data} }); - - return @{$result->{result}{data}}; -} - ->>>>>>> e969e034646a97750d13e66210f50c842ede4b8c sub create_revision_attachment { my ( $bug, $revision_id, $revision_title ) = @_; @@ -126,7 +106,7 @@ sub create_revision_attachment { Bugzilla->switch_to_main_db if $is_shadow_db; my $old_user = Bugzilla->user; - _set_phab_user(); + set_phab_user(); my $dbh = Bugzilla->dbh; $dbh->bz_start_transaction; @@ -396,6 +376,28 @@ sub get_members_by_phid { return \@bmo_ids; } +sub get_phab_bmo_ids { + my ($self, $params) = @_; + + my $data = { + queryKey => 'all' + }; + + if ($params->{ids}) { + $data->{constraints} = { + ids => $params->{ids} + }; + } + elsif ($params->{phids}) { + $data->{constraints} = { + phids => $params->{phids} + }; + } + + my $result = request('bugzilla.account.search', $data); + return $result->{result}->{data}; +} + sub is_attachment_phab_revision { my ($attachment, $include_obsolete) = @_; return ($attachment->contenttype eq PHAB_CONTENT_TYPE @@ -482,10 +484,12 @@ sub get_security_sync_groups { return @set_groups; } -sub _set_phab_user { +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 { @@ -504,8 +508,7 @@ sub add_security_sync_comments { : 'One revision was' ) . ' made private due to unknown Bugzilla groups.'; - my $old_user = Bugzilla->user; - _set_phab_user(); + my $old_user = set_phab_user(); $bug->add_comment( $bmo_error_message, { isprivate => 0 } ); From 5c7ff1146270d2c8bd4500c8178a4a2c57d97ec5 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Mon, 6 Nov 2017 15:56:38 -0500 Subject: [PATCH 07/13] - Support for obsoleting attachments - Refactored where phab_user is set and where we wrap in a DB transaction - More debugging output that only displays when using -d - Some bug fixes --- extensions/PhabBugz/lib/Feed.pm | 84 +++++++++++++++++--- extensions/PhabBugz/lib/Revision.pm | 3 +- extensions/PhabBugz/lib/Util.pm | 30 ++----- extensions/PhabBugz/lib/WebService.pm | 2 +- extensions/Push/lib/Connector/Phabricator.pm | 4 +- 5 files changed, 84 insertions(+), 39 deletions(-) diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index 86f937463c..08f5488e1d 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -21,6 +21,7 @@ use Bugzilla::Extension::PhabBugz::Util qw( get_bug_role_phids get_members_by_phid get_security_sync_groups + is_attachment_phab_revision make_revision_public request set_phab_user @@ -59,7 +60,7 @@ sub feed_query { # Check for new transctions (stories) my $transactions = $self->feed_transactions($last_ts); - if (!$transactions) { + if (!%$transactions) { $self->logger->info("FEED: No new transactions"); return; } @@ -67,11 +68,17 @@ sub feed_query { # Process each story foreach my $story (keys %$transactions) { my $skip = 0; - - $self->logger->debug("STORY: $story"); - my $story_data = $transactions->{$story}; + my $story_data = $transactions->{$story}; + my $author_phid = $story_data->{authorPHID}; my $object_phid = $story_data->{objectPHID}; - $self->logger->debug("OBJECT: $object_phid"); + my $story_text = $story_data->{text}; + my $story_epoch = $story_data->{epoch}; + + $self->logger->debug("STORY PHID: $story"); + $self->logger->debug("STORY_EPOCH: $story_epoch"); + $self->logger->debug("AUTHOR PHID: $author_phid"); + $self->logger->debug("OBJECT PHID: $object_phid"); + $self->logger->debug("STORY TEXT: $story_text"); # Only interested in changes to revisions for now. if ($object_phid !~ /^PHID-DREV/) { @@ -80,7 +87,7 @@ sub feed_query { } # Skip changes done by phab-bot user - my $userids = get_members_by_phid([$story_data->{authorPHID}]); + my $userids = get_members_by_phid([$author_phid]); if (@$userids) { my $user = Bugzilla::User->new({ id => $userids->[0], cache => 1 }); @@ -89,24 +96,34 @@ sub feed_query { if (!$skip) { my $revision = Bugzilla::Extension::PhabBugz::Revision->new({ phids => [$object_phid] }); - $self->process_revision_change($revision, $story_data->{text}); + $self->process_revision_change($revision, $story_text); } else { $self->logger->info('SKIPPING'); } - # Store the largest last epoch so we can start from there in the next session - $self->logger->debug("UPDATING LAST_TS: $last_ts"); + # Store the largest last epoch + 1 so we can start from there in the next session + $story_epoch++; + $self->logger->debug("UPDATING LAST_TS: $story_epoch"); $dbh->do("REPLACE INTO phabbugz (name, value) VALUES ('feed_last_ts', ?)", - undef, $story_data->{epoch}+1); + undef, $story_epoch); } } sub process_revision_change { my ($self, $revision, $story_text) = @_; + # Pre setup before making changes my $old_user = set_phab_user(); + my $is_shadow_db = Bugzilla->is_shadow_db; + Bugzilla->switch_to_main_db if $is_shadow_db; + + my $dbh = Bugzilla->dbh; + $dbh->bz_start_transaction; + + my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); + my $log_message = sprintf( "REVISION CHANGE FOUND: D%d: %s | bug: %d | %s", $revision->id, @@ -117,6 +134,8 @@ sub process_revision_change { my $bug = Bugzilla::Bug->new($revision->bug_id); + # REVISION SECURITY POLICY + # If bug is public then remove privacy policy my $result; if (!@{ $bug->groups_in }) { @@ -130,7 +149,7 @@ sub process_revision_change { # 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) { - add_security_sync_comments($revision, $bug); + add_security_sync_comments([$revision], $bug); } my $policy_phid = create_private_revision_policy($bug, \@set_groups); @@ -141,12 +160,51 @@ sub process_revision_change { $revision->set_subscribers($subscribers); } - $revision->update(); + my $attachment = create_revision_attachment($bug, $revision->id, $revision->title, $timestamp); + + # ATTACHMENT OBSOLETES + + # fixup attachments on current bug + my @attachments = + grep { is_attachment_phab_revision($_) } @{ $bug->attachments() }; + + foreach my $attachment (@attachments) { + my ($attach_revision_id) = ($attachment->filename =~ PHAB_ATTACHMENT_PATTERN); + next if $attach_revision_id != $revision->id; - my $attachment = create_revision_attachment($bug, $revision->id, $revision->title); + my $make_obsolete = $revision->status eq 'abandoned' ? 1 : 0; + $attachment->set_is_obsolete($make_obsolete); + + if ($revision->id == $attach_revision_id + && $revision->title ne $attachment->description) { + $attachment->set_description($revision->title); + } + + $attachment->update($timestamp); + last; + } + + # fixup attachments with same revision id but on different bugs + my $other_attachments = Bugzilla::Attachment->match({ + mimetype => PHAB_CONTENT_TYPE, + filename => 'phabricator-D' . $revision->id . '-url.txt', + WHERE => { 'bug_id != ? AND NOT isobsolete' => $bug->id } + }); + foreach my $attachment (@$other_attachments) { + $attachment->set_is_obsolete(1); + $attachment->update($timestamp); + } + + # FINISH UP + + $bug->update($timestamp); + $revision->update(); Bugzilla::BugMail::Send($revision->bug_id, { changer => Bugzilla->user }); + $dbh->bz_commit_transaction; + Bugzilla->switch_to_shadow_db if $is_shadow_db; + Bugzilla->set_user($old_user); $self->logger->info("SUCCESS"); diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm index f2b7b35a41..ad49b0e6fd 100644 --- a/extensions/PhabBugz/lib/Revision.pm +++ b/extensions/PhabBugz/lib/Revision.pm @@ -1,4 +1,4 @@ -# This Source Code Form is hasject to the terms of the Mozilla Public + # This Source Code Form is hasject 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/. # @@ -169,6 +169,7 @@ sub update { sub id { $_[0]->{id}; } sub phid { $_[0]->{phid}; } sub title { $_[0]->{fields}->{title}; } +sub status { $_[0]->{fields}->{status}->{value}; } sub creation_ts { $_[0]->{fields}->{dateCreated}; } sub modification_ts { $_[0]->{fields}->{dateModified}; } sub author_phid { $_[0]->{fields}->{authorPHID}; } diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index 02cf5e245b..0f9410e630 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -87,7 +87,7 @@ sub get_revisions_by_phids { } sub create_revision_attachment { - my ( $bug, $revision_id, $revision_title ) = @_; + my ( $bug, $revision_id, $revision_title, $timestamp ) = @_; my $phab_base_uri = Bugzilla->params->{phabricator_base_uri}; ThrowUserError('invalid_phabricator_uri') unless $phab_base_uri; @@ -102,16 +102,10 @@ sub create_revision_attachment { return $review_attachment if defined $review_attachment; # No attachment is present, so we can now create new one - my $is_shadow_db = Bugzilla->is_shadow_db; - Bugzilla->switch_to_main_db if $is_shadow_db; - my $old_user = Bugzilla->user; - set_phab_user(); - - my $dbh = Bugzilla->dbh; - $dbh->bz_start_transaction; - - my ($timestamp) = $dbh->selectrow_array("SELECT NOW()"); + if (!$timestamp) { + ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); + } my $attachment = Bugzilla::Attachment->create( { @@ -126,13 +120,9 @@ sub create_revision_attachment { } ); - $bug->update($timestamp); - $attachment->update($timestamp); - - $dbh->bz_commit_transaction; - Bugzilla->switch_to_shadow_db if $is_shadow_db; - - Bugzilla->set_user($old_user); + # Insert a comment about the new attachment into the database. + $bug->add_comment('', { type => CMT_ATTACHMENT_CREATED, + extra_data => $attachment->id }); return $attachment; } @@ -399,9 +389,8 @@ sub get_phab_bmo_ids { } sub is_attachment_phab_revision { - my ($attachment, $include_obsolete) = @_; + my ($attachment) = @_; return ($attachment->contenttype eq PHAB_CONTENT_TYPE - && ($include_obsolete || !$attachment->isobsolete) && $attachment->attacher->login eq PHAB_AUTOMATION_USER) ? 1 : 0; } @@ -512,9 +501,6 @@ sub add_security_sync_comments { $bug->add_comment( $bmo_error_message, { isprivate => 0 } ); - my $bug_changes = $bug->update(); - $bug->send_changes($bug_changes); - Bugzilla->set_user($old_user); } diff --git a/extensions/PhabBugz/lib/WebService.pm b/extensions/PhabBugz/lib/WebService.pm index 7380778804..b552e56565 100644 --- a/extensions/PhabBugz/lib/WebService.pm +++ b/extensions/PhabBugz/lib/WebService.pm @@ -268,7 +268,7 @@ sub obsolete_attachments { my $bug = Bugzilla::Bug->check($bug_id); my @attachments = - grep { is_attachment_phab_revision($_, 1) } @{ $bug->attachments() }; + grep { is_attachment_phab_revision($_) } @{ $bug->attachments() }; return { result => [] } if !@attachments; diff --git a/extensions/Push/lib/Connector/Phabricator.pm b/extensions/Push/lib/Connector/Phabricator.pm index 4f0a57793c..988403727b 100644 --- a/extensions/Push/lib/Connector/Phabricator.pm +++ b/extensions/Push/lib/Connector/Phabricator.pm @@ -22,8 +22,8 @@ use Bugzilla::Extension::PhabBugz::Constants; use Bugzilla::Extension::PhabBugz::Util qw( add_comment_to_revision create_private_revision_policy edit_revision_policy get_attachment_revisions get_bug_role_phids - get_revisions_by_ids intersect is_attachment_phab_revision - make_revision_public make_revision_private set_revision_subscribers + get_revisions_by_ids intersect make_revision_public + make_revision_private set_revision_subscribers get_security_sync_groups add_security_sync_comments); use Bugzilla::Extension::Push::Constants; use Bugzilla::Extension::Push::Util qw(is_public); From 11334e2d62b947de1f07e041a5b905548ed60006 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Mon, 6 Nov 2017 16:54:58 -0500 Subject: [PATCH 08/13] - Use get_phab_bmo_ids instead of get_members_by_phid --- extensions/PhabBugz/lib/Feed.pm | 9 ++++----- extensions/PhabBugz/lib/Util.pm | 24 ++++-------------------- 2 files changed, 8 insertions(+), 25 deletions(-) diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index 08f5488e1d..f89ecb4c02 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -19,7 +19,7 @@ use Bugzilla::Extension::PhabBugz::Util qw( create_revision_attachment edit_revision_policy get_bug_role_phids - get_members_by_phid + get_phab_bmo_ids get_security_sync_groups is_attachment_phab_revision make_revision_public @@ -87,10 +87,9 @@ sub feed_query { } # Skip changes done by phab-bot user - my $userids = get_members_by_phid([$author_phid]); - - if (@$userids) { - my $user = Bugzilla::User->new({ id => $userids->[0], cache => 1 }); + 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 }); $skip = 1 if $user->login eq PHAB_AUTOMATION_USER; } diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index 0f9410e630..2f7f25e62b 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -355,7 +355,7 @@ sub get_members_by_phid { my $data = { phids => $phids }; - my $result = request('bugzilla.account.search', $data); + my $result = request('bugzilla.account.search', $data); my @bmo_ids; foreach my $user (@{ $result->{result} }) { @@ -367,25 +367,9 @@ sub get_members_by_phid { } sub get_phab_bmo_ids { - my ($self, $params) = @_; - - my $data = { - queryKey => 'all' - }; - - if ($params->{ids}) { - $data->{constraints} = { - ids => $params->{ids} - }; - } - elsif ($params->{phids}) { - $data->{constraints} = { - phids => $params->{phids} - }; - } - - my $result = request('bugzilla.account.search', $data); - return $result->{result}->{data}; + my ($params) = @_; + my $result = request('bugzilla.account.search', $params); + return $result->{result}; } sub is_attachment_phab_revision { From 62af28918eeb9af71be0ef08b926af3f088132cd Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 8 Nov 2017 13:12:33 -0500 Subject: [PATCH 09/13] - Fixed some typos in Revision.pm - Fixes for Feed.pm to prevent looping - Added code to update review flag statuses when revision is accepted/rejected. --- extensions/PhabBugz/lib/Feed.pm | 121 +++++++++++++++++++++++----- extensions/PhabBugz/lib/Revision.pm | 13 ++- 2 files changed, 111 insertions(+), 23 deletions(-) diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index f89ecb4c02..cec593f0c8 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -9,8 +9,11 @@ package Bugzilla::Extension::PhabBugz::Feed; use 5.10.1; +use List::Util qw(first); +use List::MoreUtils qw(any); use Moo; +use Bugzilla::Constants; use Bugzilla::Extension::PhabBugz::Constants; use Bugzilla::Extension::PhabBugz::Revision; use Bugzilla::Extension::PhabBugz::Util qw( @@ -56,7 +59,7 @@ sub feed_query { my $last_ts = $dbh->selectrow_array(" SELECT value FROM phabbugz WHERE name = 'feed_last_ts'"); $last_ts ||= 0; - $self->logger->debug("LAST_TS: $last_ts"); + $self->logger->debug("QUERY LAST_TS: $last_ts"); # Check for new transctions (stories) my $transactions = $self->feed_transactions($last_ts); @@ -88,7 +91,7 @@ sub feed_query { # Skip changes done by phab-bot user my $phab_users = get_phab_bmo_ids({ phids => [$author_phid] }); - if (@$phab_users) { + if (!$skip && @$phab_users) { my $user = Bugzilla::User->new({ id => $phab_users->[0]->{id}, cache => 1 }); $skip = 1 if $user->login eq PHAB_AUTOMATION_USER; } @@ -135,28 +138,33 @@ sub process_revision_change { # REVISION SECURITY POLICY - # If bug is public then remove privacy policy - my $result; - if (!@{ $bug->groups_in }) { - $revision->set_policy('view', 'public'); - $revision->set_policy('edit', 'users'); - } - # 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) { - add_security_sync_comments([$revision], $bug); + # Do not set policy if a custom policy has already been set + # This keeps from setting new custom policy everytime a change + # is made. + unless ($revision->view_policy =~ /^PHID-PLCY/) { + + # If bug is public then remove privacy policy + if (!@{ $bug->groups_in }) { + $revision->set_policy('view', 'public'); + $revision->set_policy('edit', 'users'); } + # 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) { + add_security_sync_comments([$revision], $bug); + } - my $policy_phid = create_private_revision_policy($bug, \@set_groups); - my $subscribers = get_bug_role_phids($bug); + my $policy_phid = create_private_revision_policy($bug, \@set_groups); + my $subscribers = get_bug_role_phids($bug); - $revision->set_policy('view', $policy_phid); - $revision->set_policy('edit', $policy_phid); - $revision->set_subscribers($subscribers); + $revision->set_policy('view', $policy_phid); + $revision->set_policy('edit', $policy_phid); + $revision->set_subscribers($subscribers); + } } my $attachment = create_revision_attachment($bug, $revision->id, $revision->title, $timestamp); @@ -194,6 +202,77 @@ sub process_revision_change { $attachment->update($timestamp); } + # REVIEWER STATUSES + + 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'; + } + + 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; + + foreach my $attachment (@attachments) { + my ($attach_revision_id) = ($attachment->filename =~ PHAB_ATTACHMENT_PATTERN); + next if $revision->id != $attach_revision_id; + + # Clear old flags if no longer accepted + my (@denied_flags, @new_flags, @removed_flags, %accepted_done, $flag_type); + foreach my $flag (@{ $attachment->flags }) { + next if $flag->type->name ne 'review'; + $flag_type = $flag->type; + if (any { $flag->setter->id == $_ } @denied_user_ids) { + push(@denied_flags, { id => $flag->id, setter => $flag->setter, status => 'X' }); + } + if (any { $flag->setter->id == $_ } @accepted_user_ids) { + $accepted_done{$flag->setter->id}++; + } + if ($flag->status eq '+' + && !any { $flag->setter->id == $_ } (@accepted_user_ids, @denied_user_ids)) { + push(@removed_flags, { id => $flag->id, setter => $flag->setter, status => 'X' }); + } + } + + $flag_type ||= first { $_->name eq 'review' } @{ $attachment->flag_types }; + + # Create new flags + foreach my $user_id (@accepted_user_ids) { + next if $accepted_done{$user_id}; + my $user = Bugzilla::User->check({ id => $user_id, cache => 1 }); + 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; + foreach my $flag_data (@new_flags) { + $comment .= $flag_data->{setter}->name . " has approved the revision.\n"; + } + foreach my $flag_data (@denied_flags) { + $comment .= $flag_data->{setter}->name . " has requested changes to the revision.\n"; + } + foreach my $flag_data (@removed_flags) { + $comment .= $flag_data->{setter}->name . " has been removed from the revision.\n"; + } + + if ($comment) { + $comment .= "\n" . Bugzilla->params->{phabricator_base_uri} . "D" . $revision->id; + # 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 $bug->update($timestamp); diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm index ad49b0e6fd..27dfa1ead6 100644 --- a/extensions/PhabBugz/lib/Revision.pm +++ b/extensions/PhabBugz/lib/Revision.pm @@ -42,6 +42,11 @@ sub _load { } }; + # If proper ids and phids constraints were + # provided, we return an empty data structure + # instead of failing outright. This allows for + # silently checking for the existence of a + # revision. if ($params->{ids}) { $data->{constraints} = { ids => $params->{ids} @@ -178,7 +183,7 @@ sub bug_id { $_[0]->{fields}->{'bugzilla.bug-id'}; } sub view_policy { $_[0]->{fields}->{policy}->{view}; } sub edit_policy { $_[0]->{fields}->{policy}->{edit}; } -sub reviewers_raw { $_[0]->{atachments}->{reviewers}->{reviewers}; } +sub reviewers_raw { $_[0]->{attachments}->{reviewers}->{reviewers}; } sub subscribers_raw { $_[0]->{attachments}->{subscribers}; } sub projects_raw { $_[0]->{attachments}->{projects}; } sub subscriber_count { $_[0]->{attachments}->{subscribers}->{subscriberCount}; } @@ -207,13 +212,15 @@ sub reviewers { push(@phids, $reviewer->{reviewerPHID}); } + return [] if !@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->reviews_raw) { + foreach my $reviewer_data (@{ $self->reviewers_raw }) { if ($reviewer_data->{reviewerPHID} eq $user->{phid}) { $reviewer->{phab_review_status} = $reviewer_data->{status}; last; @@ -236,6 +243,8 @@ sub subscribers { my $users = get_phab_bmo_ids({ phids => \@phids }); + return [] if !@phids; + my @subscribers; foreach my $user (@$users) { my $subscriber = Bugzilla::User->new({ id => $user->{id}, cache => 1}); From 81c2de1f39ba3a9f55eac7f5f864310d20d53d24 Mon Sep 17 00:00:00 2001 From: Dylan William Hardison Date: Thu, 9 Nov 2017 15:01:36 -0500 Subject: [PATCH 10/13] catch exceptions that bubble up to main loop (#269) --- extensions/PhabBugz/lib/Feed.pm | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index cec593f0c8..14c6c07a33 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -36,11 +36,15 @@ has 'logger' => ( is => 'rw' ); sub start { my ($self) = @_; while (1) { - if (Bugzilla->params->{phabricator_enabled}) { - $self->feed_query(); - } + my $ok = eval { + if (Bugzilla->params->{phabricator_enabled}) { + $self->feed_query(); + Bugzilla->_cleanup(); + } + 1; + }; + $self->logger->error( $@ // "unknown exception" ) unless $ok; sleep(PHAB_POLL_SECONDS); - Bugzilla->_cleanup(); } } From ea49d299356fafc1832e8e554749be793d324049 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 9 Nov 2017 17:12:45 -0500 Subject: [PATCH 11/13] - Fixed some of the review comments by Dylan such as transaction ordering and better error checking for API calls. - Update update_project_members.pl to use Project.pm properly. TODO: - Use memcache inside get_phab_bmo_id() calls as the values are unlikely to change - Update the Phabricator.pm Push connector to use Revision.pm --- extensions/PhabBugz/Extension.pm | 4 +- .../PhabBugz/bin/update_project_members.pl | 41 +++--- extensions/PhabBugz/lib/Feed.pm | 27 +++- extensions/PhabBugz/lib/Project.pm | 77 ++++++----- extensions/PhabBugz/lib/Revision.pm | 121 +++++++++++++----- 5 files changed, 181 insertions(+), 89 deletions(-) diff --git a/extensions/PhabBugz/Extension.pm b/extensions/PhabBugz/Extension.pm index b8ecf001dc..b3ad44819d 100644 --- a/extensions/PhabBugz/Extension.pm +++ b/extensions/PhabBugz/Extension.pm @@ -59,12 +59,12 @@ sub db_schema_abstract_schema { $args->{'schema'}->{'phabbugz'} = { FIELDS => [ id => { - TYPE => 'MEDIUMSERIAL', + TYPE => 'INTSERIAL', NOTNULL => 1, PRIMARYKEY => 1, }, name => { - TYPE => 'VARCHAR(64)', + TYPE => 'VARCHAR(255)', NOTNULL => 1, }, value => { diff --git a/extensions/PhabBugz/bin/update_project_members.pl b/extensions/PhabBugz/bin/update_project_members.pl index bdc054e1a8..06cc556264 100755 --- a/extensions/PhabBugz/bin/update_project_members.pl +++ b/extensions/PhabBugz/bin/update_project_members.pl @@ -20,11 +20,9 @@ use Bugzilla::Error; use Bugzilla::Group; +use Bugzilla::Extension::PhabBugz::Project; use Bugzilla::Extension::PhabBugz::Util qw( - create_project - get_members_by_bmo_id - get_project_phid - set_project_members + get_phab_bmo_ids ); Bugzilla->usage_mode(USAGE_MODE_CMDLINE); @@ -55,23 +53,22 @@ my $sync_groups = Bugzilla::Group->match({ name => [ split('[,\s]+', $phab_sync_groups) ] }); foreach my $group (@$sync_groups) { - my @users = get_group_members($group); - # Create group project if one does not yet exist my $phab_project_name = 'bmo-' . $group->name; - my $project_phid = get_project_phid($phab_project_name); - if (!$project_phid) { - $project_phid = create_project($phab_project_name, 'BMO Security Group for ' . $group->name); + my $project = Bugzilla::Extension::PhabBugz::Project->new({ + name => $phab_project_name + }); + if (!$project->id) { + $project = Bugzilla::Extension::PhabBugz::Project->create({ + name => $phab_project_name, + description => 'BMO Security Group for ' . $group->name + }); } - # Get the internal user ids for the bugzilla group members - my $phab_user_ids = []; - if (@users) { - $phab_user_ids = get_members_by_bmo_id(\@users); - } + my @group_members = get_group_members($group); - # Set the project members to the exact list - set_project_members($project_phid, $phab_user_ids); + $project->set_members(\@group_members); + $project->update(); } sub get_group_members { @@ -84,5 +81,13 @@ sub get_group_members { $users{$user->id} = $user; } } - return values %users; -} + + # 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}; + } + + # We only need users who have accounts in phabricator + return grep { $_->phab_phid } values %users; +} \ No newline at end of file diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index 14c6c07a33..3d337e7484 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -67,21 +67,21 @@ sub feed_query { # Check for new transctions (stories) my $transactions = $self->feed_transactions($last_ts); - if (!%$transactions) { + if (!@$transactions) { $self->logger->info("FEED: No new transactions"); return; } # Process each story - foreach my $story (keys %$transactions) { + foreach my $story_data (@$transactions) { my $skip = 0; - my $story_data = $transactions->{$story}; + my $story_phid = $story_data->{storyPHID}; my $author_phid = $story_data->{authorPHID}; my $object_phid = $story_data->{objectPHID}; my $story_text = $story_data->{text}; my $story_epoch = $story_data->{epoch}; - $self->logger->debug("STORY PHID: $story"); + $self->logger->debug("STORY PHID: $story_phid"); $self->logger->debug("STORY_EPOCH: $story_epoch"); $self->logger->debug("AUTHOR PHID: $author_phid"); $self->logger->debug("OBJECT PHID: $object_phid"); @@ -138,7 +138,7 @@ sub process_revision_change { $story_text); $self->logger->info($log_message); - my $bug = Bugzilla::Bug->new($revision->bug_id); + my $bug = Bugzilla::Bug->new({ id => $revision->bug_id, cache => 1 }); # REVISION SECURITY POLICY @@ -297,10 +297,23 @@ sub feed_transactions { my $data = { view => 'text' }; $data->{epochStart} = $epoch if $epoch; my $result = request('feed.query_epoch', $data); - # Stupid conduit. If the feed results are empty it returns + + # Stupid Conduit. If the feed results are empty it returns # an empty list ([]). If there is data it returns it in a # hash ({}) so we have adjust to be consistent. - return ref $result->{result} eq 'HASH' ? $result->{result} : {}; + my $stories = ref $result->{result} eq 'HASH' ? $result->{result} : {}; + + # PHP array retain key order but Perl does not. So we will + # loop over the data and place the stories into a list instead + # of a hash. We will then sort the list by ascending epoch. + my @story_list; + foreach my $story_phid (keys %$stories) { + my $story_data = $stories->{$story_phid}; + $story_data->{storyPHID} = $story_phid; + push(@story_list, $story_data); + } + + return [ sort { $a->{epoch} <=> $b->{epoch} } @story_list ]; } 1; diff --git a/extensions/PhabBugz/lib/Project.pm b/extensions/PhabBugz/lib/Project.pm index c9aedec00e..63ba652809 100644 --- a/extensions/PhabBugz/lib/Project.pm +++ b/extensions/PhabBugz/lib/Project.pm @@ -8,8 +8,8 @@ package Bugzilla::Extension::PhabBugz::Project; use 5.10.1; - -use Moo; +use strict; +use warnings; use Bugzilla::Error; use Bugzilla::Util qw(trim); @@ -38,29 +38,16 @@ sub _load { projects => 1, reviewers => 1, subscribers => 1 - } + }, + constraints => $params }; - if ($params->{ids}) { - $data->{constraints} = { - ids => $params->{ids} - }; - } - elsif ($params->{phids}) { - $data->{constraints} = { - phids => $params->{phids} - }; - } - else { - return {}; - } - my $result = request('project.search', $data); if (exists $result->{result}{data} && @{ $result->{result}{data} }) { return $result->{result}->{data}->[0]; } - return {}; + return $result; } # { @@ -155,6 +142,12 @@ sub create { }; my $result = request('project.edit', $data); + + if ($result->{error_code}) { + ThrowCodeError('phabricator_api_error', + { code => $result->{error_code}, reason => $result->{error_info} }); + } + return $class->new({ phids => $result->{result}{object}{phid} }); } @@ -180,18 +173,26 @@ sub update { }); } - if ($self->{added_members}) { + if ($self->{set_members}) { push(@{ $data->{transactions} }, { - type => 'members.add', - value => $self->{added_members} + type => 'members.set', + value => $self->{set_members} }); } + else { + if ($self->{add_members}) { + push(@{ $data->{transactions} }, { + type => 'members.add', + value => $self->{add_members} + }); + } - if ($self->{removed_members}) { - push(@{ $data->{transactions} }, { - type => 'members.remove', - value => $self->{removed_members} - }); + if ($self->{remove_members}) { + push(@{ $data->{transactions} }, { + type => 'members.remove', + value => $self->{remove_members} + }); + } } if ($self->{set_policy}) { @@ -204,7 +205,14 @@ sub update { } } - request('differential.project.edit', $data); + my $result = request('project.edit', $data); + + if ($result->{error_code}) { + ThrowCodeError('phabricator_api_error', + { code => $result->{error_code}, reason => $result->{error_info} }); + } + + return $result; } ######################### @@ -264,14 +272,21 @@ sub set_description { sub add_member { my ($self, $member) = @_; - $self->{added_members} ||= []; - push(@{ $self->{added_members} }, $member->phab_phid); + $self->{add_members} ||= []; + my $member_phid = blessed $member ? $member->phab_phid : $member; + push(@{ $self->{add_members} }, $member_phid); } sub remove_member { my ($self, $member) = @_; - $self->{removed_members} ||= []; - push(@{ $self->{removed_members} }, $member->phab_phid); + $self->{remove_members} ||= []; + 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 { $_->phab_phid } @$members ]; } sub set_policy { diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm index 27dfa1ead6..1128ff6c23 100644 --- a/extensions/PhabBugz/lib/Revision.pm +++ b/extensions/PhabBugz/lib/Revision.pm @@ -8,8 +8,8 @@ package Bugzilla::Extension::PhabBugz::Revision; use 5.10.1; - -use Moo; +use strict; +use warnings; use Bugzilla::Bug; use Bugzilla::Error; @@ -39,34 +39,16 @@ sub _load { projects => 1, reviewers => 1, subscribers => 1 - } + }, + constraints => $params }; - # If proper ids and phids constraints were - # provided, we return an empty data structure - # instead of failing outright. This allows for - # silently checking for the existence of a - # revision. - if ($params->{ids}) { - $data->{constraints} = { - ids => $params->{ids} - }; - } - elsif ($params->{phids}) { - $data->{constraints} = { - phids => $params->{phids} - }; - } - else { - return {}; - } - my $result = request('differential.revision.search', $data); if (exists $result->{result}{data} && @{ $result->{result}{data} }) { return $result->{result}->{data}->[0]; } - return {}; + return $result; } # { @@ -154,6 +136,41 @@ sub update { }); } + if ($self->{add_subscribers}) { + push(@{ $data->{transactions} }, { + type => 'subscribers.add', + value => $self->{add_subscribers} + }); + } + + if ($self->{remove_subscribers}) { + push(@{ $data->{transactions} }, { + type => 'subscribers.remove', + value => $self->{remove_subscribers} + }); + } + + if ($self->{set_reviewers}) { + push(@{ $data->{transactions} }, { + type => 'reviewers.set', + value => $self->{set_reviewers} + }); + } + + if ($self->{add_reviewers}) { + push(@{ $data->{transactions} }, { + type => 'reviewers.add', + value => $self->{add_reviewers} + }); + } + + if ($self->{remove_reviewers}) { + push(@{ $data->{transactions} }, { + type => 'reviewers.remove', + value => $self->{remove_reviewers} + }); + } + if ($self->{set_policy}) { foreach my $name ("view", "edit") { next unless $self->{set_policy}->{$name}; @@ -164,7 +181,14 @@ sub update { } } - request('differential.revision.edit', $data); + my $result = request('differential.revision.edit', $data); + + if ($result->{error_code}) { + ThrowCodeError('phabricator_api_error', + { code => $result->{error_code}, reason => $result->{error_info} }); + } + + return $result; } ######################### @@ -183,24 +207,26 @@ sub bug_id { $_[0]->{fields}->{'bugzilla.bug-id'}; } sub view_policy { $_[0]->{fields}->{policy}->{view}; } sub edit_policy { $_[0]->{fields}->{policy}->{edit}; } -sub reviewers_raw { $_[0]->{attachments}->{reviewers}->{reviewers}; } +sub reviewers_raw { $_[0]->{attachments}->{reviewers}->{reviewers}; } sub subscribers_raw { $_[0]->{attachments}->{subscribers}; } sub projects_raw { $_[0]->{attachments}->{projects}; } sub subscriber_count { $_[0]->{attachments}->{subscribers}->{subscriberCount}; } sub bug { my ($self) = @_; - my $bug = $self->{bug} ||= new Bugzilla::Bug($self->bug_id); - weaken($self->{bug}) unless isweak($self->{bug}); - return $bug; + return $self->{bug} ||= Bugzilla::Bug->new({ id => $self->bug_id, cache => 1 }); } sub author { my ($self) = @_; return $self->{author} if $self->{author}; - my $userids = get_members_by_phid([$self->author_phid]); - $self->{'author'} = new Bugzilla::User({ id => $userids->[0], cache => 1 }); - return $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}; + } + return undef; } sub reviewers { @@ -266,6 +292,39 @@ sub add_comment { push(@{ $self->{added_comments} }, $comment); } +sub add_reviewer { + my ($self, $reviewer) = @_; + $self->{add_reviewers} ||= []; + 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->phab_phid : $reviewer; + push(@{ $self->{remove_reviewers} }, $reviewer_phid); +} + +sub set_reviewers { + my ($self, $reviewers) = @_; + $self->{set_reviewers} = [ map { $_->phab_phid } @$reviewers ]; +} + +sub add_subscriber { + my ($self, $subscriber) = @_; + $self->{add_subscribers} ||= []; + my $subscriber_phid = blessed $subscriber ? $subscriber->phab_phid : $subscriber; + push(@{ $self->{add_subscribers} }, $subscriber_phid); +} + +sub remove_subscriber { + my ($self, $subscriber) = @_; + $self->{remove_subscribers} ||= []; + my $subscriber_phid = blessed $subscriber ? $subscriber->phab_phid : $subscriber; + push(@{ $self->{remove_subscribers} }, $subscriber_phid); +} + sub set_subscribers { my ($self, $subscribers) = @_; $self->{set_subscribers} = $subscribers; From ce0a21923c034b4766372977e93ab7491dab36e3 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Thu, 16 Nov 2017 17:07:36 -0500 Subject: [PATCH 12/13] - Changed to use feed.query_id instead of feed.query_epoch. - Store last ID in the BMO table instead of epoch and use ID for retrieving newest stories. - Updates to different functons in Util.pm to use newer methods for getting data from phabricator. - Moved API error checking into Util::request --- extensions/PhabBugz/lib/Feed.pm | 37 ++++++------ extensions/PhabBugz/lib/Project.pm | 14 +---- extensions/PhabBugz/lib/Revision.pm | 7 +-- extensions/PhabBugz/lib/Util.pm | 93 +++++++++++++++++++---------- 4 files changed, 84 insertions(+), 67 deletions(-) diff --git a/extensions/PhabBugz/lib/Feed.pm b/extensions/PhabBugz/lib/Feed.pm index 3d337e7484..d178f249b1 100644 --- a/extensions/PhabBugz/lib/Feed.pm +++ b/extensions/PhabBugz/lib/Feed.pm @@ -60,13 +60,13 @@ sub feed_query { $self->logger->info("FEED: Fetching new transactions"); - my $last_ts = $dbh->selectrow_array(" - SELECT value FROM phabbugz WHERE name = 'feed_last_ts'"); - $last_ts ||= 0; - $self->logger->debug("QUERY LAST_TS: $last_ts"); + my $last_id = $dbh->selectrow_array(" + SELECT value FROM phabbugz WHERE name = 'feed_last_id'"); + $last_id ||= 0; + $self->logger->debug("QUERY LAST_ID: $last_id"); # Check for new transctions (stories) - my $transactions = $self->feed_transactions($last_ts); + my $transactions = $self->feed_transactions($last_id); if (!@$transactions) { $self->logger->info("FEED: No new transactions"); return; @@ -75,14 +75,14 @@ sub feed_query { # Process each story foreach my $story_data (@$transactions) { my $skip = 0; + my $story_id = $story_data->{id}; my $story_phid = $story_data->{storyPHID}; my $author_phid = $story_data->{authorPHID}; my $object_phid = $story_data->{objectPHID}; my $story_text = $story_data->{text}; - my $story_epoch = $story_data->{epoch}; + $self->logger->debug("STORY ID: $story_id"); $self->logger->debug("STORY PHID: $story_phid"); - $self->logger->debug("STORY_EPOCH: $story_epoch"); $self->logger->debug("AUTHOR PHID: $author_phid"); $self->logger->debug("OBJECT PHID: $object_phid"); $self->logger->debug("STORY TEXT: $story_text"); @@ -108,11 +108,10 @@ sub feed_query { $self->logger->info('SKIPPING'); } - # Store the largest last epoch + 1 so we can start from there in the next session - $story_epoch++; - $self->logger->debug("UPDATING LAST_TS: $story_epoch"); - $dbh->do("REPLACE INTO phabbugz (name, value) VALUES ('feed_last_ts', ?)", - undef, $story_epoch); + # Store the largest last key so we can start from there in the next session + $self->logger->debug("UPDATING FEED_LAST_ID: $story_id"); + $dbh->do("REPLACE INTO phabbugz (name, value) VALUES ('feed_last_id', ?)", + undef, $story_id); } } @@ -293,19 +292,21 @@ sub process_revision_change { } sub feed_transactions { - my ($self, $epoch) = @_; + my ($self, $after) = @_; my $data = { view => 'text' }; - $data->{epochStart} = $epoch if $epoch; - my $result = request('feed.query_epoch', $data); + $data->{after} = $after if $after; + my $result = request('feed.query_id', $data); # Stupid Conduit. If the feed results are empty it returns # an empty list ([]). If there is data it returns it in a # hash ({}) so we have adjust to be consistent. - my $stories = ref $result->{result} eq 'HASH' ? $result->{result} : {}; + my $stories = ref $result->{result}{data} eq 'HASH' + ? $result->{result}{data} + : {}; # PHP array retain key order but Perl does not. So we will # loop over the data and place the stories into a list instead - # of a hash. We will then sort the list by ascending epoch. + # of a hash. We will then sort the list by id. my @story_list; foreach my $story_phid (keys %$stories) { my $story_data = $stories->{$story_phid}; @@ -313,7 +314,7 @@ sub feed_transactions { push(@story_list, $story_data); } - return [ sort { $a->{epoch} <=> $b->{epoch} } @story_list ]; + return [ sort { $a->{id} <=> $b->{id} } @story_list ]; } 1; diff --git a/extensions/PhabBugz/lib/Project.pm b/extensions/PhabBugz/lib/Project.pm index 63ba652809..3ad9558ff1 100644 --- a/extensions/PhabBugz/lib/Project.pm +++ b/extensions/PhabBugz/lib/Project.pm @@ -143,11 +143,6 @@ sub create { my $result = request('project.edit', $data); - if ($result->{error_code}) { - ThrowCodeError('phabricator_api_error', - { code => $result->{error_code}, reason => $result->{error_info} }); - } - return $class->new({ phids => $result->{result}{object}{phid} }); } @@ -207,11 +202,6 @@ sub update { my $result = request('project.edit', $data); - if ($result->{error_code}) { - ThrowCodeError('phabricator_api_error', - { code => $result->{error_code}, reason => $result->{error_info} }); - } - return $result; } @@ -231,7 +221,7 @@ sub view_policy { return $_[0]->{fields}->{policy}->{view}; } sub edit_policy { return $_[0]->{fields}->{policy}->{edit}; } sub join_policy { return $_[0]->{fields}->{policy}->{join}; } -sub members_raw { return $_[0]->{atachments}->{members}->{members}; } +sub members_raw { return $_[0]->{attachments}->{members}->{members}; } sub members { my ($self) = @_; @@ -242,6 +232,8 @@ sub members { push(@phids, $member->{phid}); } + return [] if !@phids; + my $users = get_phab_bmo_ids({ phids => \@phids }); my @members; diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm index 1128ff6c23..5b614a95aa 100644 --- a/extensions/PhabBugz/lib/Revision.pm +++ b/extensions/PhabBugz/lib/Revision.pm @@ -1,4 +1,4 @@ - # This Source Code Form is hasject to the terms of the Mozilla Public +# This Source Code Form is hasject 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/. # @@ -183,11 +183,6 @@ sub update { my $result = request('differential.revision.edit', $data); - if ($result->{error_code}) { - ThrowCodeError('phabricator_api_error', - { code => $result->{error_code}, reason => $result->{error_info} }); - } - return $result; } diff --git a/extensions/PhabBugz/lib/Util.pm b/extensions/PhabBugz/lib/Util.pm index 2f7f25e62b..a00e205516 100644 --- a/extensions/PhabBugz/lib/Util.pm +++ b/extensions/PhabBugz/lib/Util.pm @@ -52,30 +52,20 @@ our @EXPORT = qw( sub get_revisions_by_ids { my ($ids) = @_; - - my $data = { - queryKey => 'all', - constraints => { - ids => $ids - } - }; - - my $result = request('differential.revision.search', $data); - - ThrowUserError('invalid_phabricator_revision_id') - unless (exists $result->{result}{data} && @{ $result->{result}{data} }); - - return @{$result->{result}{data}}; + return _get_revisions({ ids => $ids }); } sub get_revisions_by_phids { my ($phids) = @_; + return _get_revisions({ phids => $phids }); +} + +sub _get_revisions { + my ($constraints) = @_; my $data = { - queryKey => 'all', - constraints => { - phids => $phids - } + queryKey => 'all', + constraints => $constraints }; my $result = request('differential.revision.search', $data); @@ -334,12 +324,7 @@ sub set_project_members { sub get_members_by_bmo_id { my $users = shift; - my $data = { - accountids => [ map { $_->id } @$users ] - }; - - my $result = request('bmoexternalaccount.search', $data); - return [] if (!$result->{result}); + my $result = get_phab_bmo_ids({ ids => [ map { $_->id } @$users ] }); my @phab_ids; foreach my $user (@{ $result->{result} }) { @@ -353,9 +338,7 @@ sub get_members_by_bmo_id { sub get_members_by_phid { my $phids = shift; - my $data = { phids => $phids }; - - my $result = request('bugzilla.account.search', $data); + my $result = get_phab_bmo_ids({ phids => $phids }); my @bmo_ids; foreach my $user (@{ $result->{result} }) { @@ -368,8 +351,52 @@ sub get_members_by_phid { 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); - return $result->{result}; + + # 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 { @@ -433,10 +460,12 @@ sub request { my $result; my $result_ok = eval { $result = decode_json( $response->content); 1 }; - if ( !$result_ok ) { - ThrowCodeError( - 'phabricator_api_error', - { reason => 'JSON decode failure' } ); + if (!$result_ok || $result->{error_code}) { + ThrowCodeError('phabricator_api_error', + { reason => 'JSON decode failure' }) if !$result_ok; + ThrowCodeError('phabricator_api_error', + { code => $result->{error_code}, + reason => $result->{error_info} }) if $result->{error_code}; } return $result; From 7ff004a1b4704e33a3842bc58df2f9ba9a587fbf Mon Sep 17 00:00:00 2001 From: Dylan William Hardison Date: Wed, 22 Nov 2017 10:16:39 -0500 Subject: [PATCH 13/13] add some validation / type checking --- extensions/PhabBugz/lib/Revision.pm | 44 +++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) diff --git a/extensions/PhabBugz/lib/Revision.pm b/extensions/PhabBugz/lib/Revision.pm index 5b614a95aa..29d665009f 100644 --- a/extensions/PhabBugz/lib/Revision.pm +++ b/extensions/PhabBugz/lib/Revision.pm @@ -19,15 +19,53 @@ use Bugzilla::Extension::PhabBugz::Util qw( request ); +use Types::Standard -all; + +my $SearchResult = Dict[ + id => Int, + type => Str, + phid => Str, + fields => Dict[ + title => Str, + authorPHID => Str, + dateCreated => Int, + dateModified => Int, + policy => Dict[ view => Str, edit => Str ], + "bugzilla.bug-id" => Int, + ], + attachments => Dict[ + reviewers => Dict[ + reviewers => ArrayRef[ + Dict[ + reviewerPHID => Str, + status => Str, + isBlocking => Bool, + actorPHID => Maybe[Str], + ], + ], + ], + subscribers => Dict[ + subscriberPHIDs => ArrayRef[Str], + subscriberCount => Int, + viewerIsSubscribed => Bool, + ], + projects => Dict[ projectPHIDs => ArrayRef[Str] ], + ], +]; + +my $NewParams = Dict[ phids => ArrayRef[Str] ]; + ######################### # Initialization # ######################### sub new { my ($class, $params) = @_; - my $self = $params ? _load($params) : {}; - bless($self, $class); - return $self; + $NewParams->assert_valid($params); + my $self = _load($params); + $SearchResult->assert_valid($self); + + return bless($self, $class); } sub _load {