From 52ac242eb3b2b0a0efd6c8db5a865ec0735f8e35 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 13 May 2016 06:26:54 -0700 Subject: [PATCH] Implement "Auto Review" in packages with a "Subscribe" option Summary: Ref T10939. Ref T8887. This moves toward letting packages automatically become reviewers or blocking reviewers of owned code. This change adds an "Auto Review" option to packages. Because adding reviewers/blocking reviewers is a little tricky, it doesn't actually have these options yet -- just a "subscribe" option. I'll do the reviewer work in the next update. Test Plan: Created a revision in a package with "Auto Review: Subscribe to Changes". The package got subscribed. {F1311677} {F1311678} {F1311679} Reviewers: chad Reviewed By: chad Maniphest Tasks: T8887, T10939 Differential Revision: https://secure.phabricator.com/D15915 --- .../20160513.owners.01.autoreview.sql | 2 + .../20160513.owners.02.autoreviewnone.sql | 2 + .../DifferentialProjectReviewersField.php | 2 +- .../editor/DifferentialTransactionEditor.php | 73 +++++++++++++++++++ .../PhabricatorOwnersDetailController.php | 6 ++ .../PhabricatorOwnersPackageEditEngine.php | 15 ++++ ...bricatorOwnersPackageTransactionEditor.php | 28 +++++++ .../storage/PhabricatorOwnersPackage.php | 26 +++++++ .../PhabricatorOwnersPackageTransaction.php | 13 ++++ ...habricatorApplicationTransactionEditor.php | 24 ++++-- 10 files changed, 182 insertions(+), 9 deletions(-) create mode 100644 resources/sql/autopatches/20160513.owners.01.autoreview.sql create mode 100644 resources/sql/autopatches/20160513.owners.02.autoreviewnone.sql diff --git a/resources/sql/autopatches/20160513.owners.01.autoreview.sql b/resources/sql/autopatches/20160513.owners.01.autoreview.sql new file mode 100644 index 0000000000..8b3d6e5819 --- /dev/null +++ b/resources/sql/autopatches/20160513.owners.01.autoreview.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_owners.owners_package + ADD autoReview VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20160513.owners.02.autoreviewnone.sql b/resources/sql/autopatches/20160513.owners.02.autoreviewnone.sql new file mode 100644 index 0000000000..d5c8a184e5 --- /dev/null +++ b/resources/sql/autopatches/20160513.owners.02.autoreviewnone.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_owners.owners_package + SET autoReview = 'none' WHERE autoReview = ''; diff --git a/src/applications/differential/customfield/DifferentialProjectReviewersField.php b/src/applications/differential/customfield/DifferentialProjectReviewersField.php index 5412aa81b2..c900f72659 100644 --- a/src/applications/differential/customfield/DifferentialProjectReviewersField.php +++ b/src/applications/differential/customfield/DifferentialProjectReviewersField.php @@ -8,7 +8,7 @@ final class DifferentialProjectReviewersField } public function getFieldName() { - return pht('Coalition Reviewers'); + return pht('Group Reviewers'); } public function getFieldDescription() { diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 3855edeac9..126d48970d 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -7,6 +7,7 @@ final class DifferentialTransactionEditor private $isCloseByCommit; private $repositoryPHIDOverride = false; private $didExpandInlineState = false; + private $affectedPaths; public function getEditorApplicationClass() { return 'PhabricatorDifferentialApplication'; @@ -1481,6 +1482,75 @@ final class DifferentialTransactionEditor return parent::shouldApplyHeraldRules($object, $xactions); } + protected function didApplyHeraldRules( + PhabricatorLiskDAO $object, + HeraldAdapter $adapter, + HeraldTranscript $transcript) { + + $repository = $object->getRepository(); + if (!$repository) { + return array(); + } + + if (!$this->affectedPaths) { + return array(); + } + + $packages = PhabricatorOwnersPackage::loadAffectedPackages( + $repository, + $this->affectedPaths); + + foreach ($packages as $key => $package) { + if ($package->isArchived()) { + unset($packages[$key]); + } + } + + if (!$packages) { + return array(); + } + + $auto_subscribe = array(); + $auto_review = array(); + $auto_block = array(); + + foreach ($packages as $package) { + switch ($package->getAutoReview()) { + case PhabricatorOwnersPackage::AUTOREVIEW_SUBSCRIBE: + $auto_subscribe[] = $package; + break; + case PhabricatorOwnersPackage::AUTOREVIEW_REVIEW: + $auto_review[] = $package; + break; + case PhabricatorOwnersPackage::AUTOREVIEW_BLOCK: + $auto_block[] = $package; + break; + case PhabricatorOwnersPackage::AUTOREVIEW_NONE: + default: + break; + } + } + + $owners_phid = id(new PhabricatorOwnersApplication()) + ->getPHID(); + + $xactions = array(); + if ($auto_subscribe) { + + $xactions[] = $object->getApplicationTransactionTemplate() + ->setAuthorPHID($owners_phid) + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue( + array( + '+' => mpull($auto_subscribe, 'getPHID'), + )); + } + + // TODO: Implement autoreview and autoblock, but these are more invovled. + + return $xactions; + } + protected function buildHeraldAdapter( PhabricatorLiskDAO $object, array $xactions) { @@ -1557,6 +1627,9 @@ final class DifferentialTransactionEditor } $all_paths = array_keys($all_paths); + // Save the affected paths; we'll use them later to query Owners. + $this->affectedPaths = $all_paths; + $path_ids = PhabricatorRepositoryCommitChangeParserWorker::lookupOrCreatePaths( $all_paths); diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php index 8329e9f931..a697fc6e74 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -184,6 +184,12 @@ final class PhabricatorOwnersDetailController } $view->addProperty(pht('Owners'), $owner_list); + $auto = $package->getAutoReview(); + $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap(); + $spec = idx($autoreview_map, $auto, array()); + $name = idx($spec, 'name', $auto); + $view->addProperty(pht('Auto Review'), $name); + if ($package->getAuditingEnabled()) { $auditing = pht('Enabled'); } else { diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php index 7f705ec13d..9c3c3c471c 100644 --- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php +++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php @@ -84,6 +84,9 @@ applying a transaction of this type. EOTEXT ); + $autoreview_map = PhabricatorOwnersPackage::getAutoreviewOptionsMap(); + $autoreview_map = ipull($autoreview_map, 'name'); + return array( id(new PhabricatorTextEditField()) ->setKey('name') @@ -100,6 +103,18 @@ EOTEXT ->setDatasource(new PhabricatorProjectOrUserDatasource()) ->setIsCopyable(true) ->setValue($object->getOwnerPHIDs()), + id(new PhabricatorSelectEditField()) + ->setKey('autoReview') + ->setLabel(pht('Auto Review')) + ->setDescription( + pht( + 'Automatically trigger reviews for commits affecting files in '. + 'this package.')) + ->setTransactionType( + PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW) + ->setIsCopyable(true) + ->setValue($object->getAutoReview()) + ->setOptions($autoreview_map), id(new PhabricatorSelectEditField()) ->setKey('auditing') ->setLabel(pht('Auditing')) diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php b/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php index f9267a0ba9..597c1a10b5 100644 --- a/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php +++ b/src/applications/owners/editor/PhabricatorOwnersPackageTransactionEditor.php @@ -20,6 +20,7 @@ final class PhabricatorOwnersPackageTransactionEditor $types[] = PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION; $types[] = PhabricatorOwnersPackageTransaction::TYPE_PATHS; $types[] = PhabricatorOwnersPackageTransaction::TYPE_STATUS; + $types[] = PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; @@ -47,6 +48,8 @@ final class PhabricatorOwnersPackageTransactionEditor return mpull($paths, 'getRef'); case PhabricatorOwnersPackageTransaction::TYPE_STATUS: return $object->getStatus(); + case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW: + return $object->getAutoReview(); } } @@ -58,6 +61,7 @@ final class PhabricatorOwnersPackageTransactionEditor case PhabricatorOwnersPackageTransaction::TYPE_NAME: case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION: case PhabricatorOwnersPackageTransaction::TYPE_STATUS: + case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW: return $xaction->getNewValue(); case PhabricatorOwnersPackageTransaction::TYPE_PATHS: $new = $xaction->getNewValue(); @@ -113,6 +117,9 @@ final class PhabricatorOwnersPackageTransactionEditor case PhabricatorOwnersPackageTransaction::TYPE_STATUS: $object->setStatus($xaction->getNewValue()); return; + case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW: + $object->setAutoReview($xaction->getNewValue()); + return; } return parent::applyCustomInternalTransaction($object, $xaction); @@ -127,6 +134,7 @@ final class PhabricatorOwnersPackageTransactionEditor case PhabricatorOwnersPackageTransaction::TYPE_DESCRIPTION: case PhabricatorOwnersPackageTransaction::TYPE_AUDITING: case PhabricatorOwnersPackageTransaction::TYPE_STATUS: + case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW: return; case PhabricatorOwnersPackageTransaction::TYPE_OWNERS: $old = $xaction->getOldValue(); @@ -220,6 +228,26 @@ final class PhabricatorOwnersPackageTransactionEditor } } + break; + case PhabricatorOwnersPackageTransaction::TYPE_AUTOREVIEW: + $map = PhabricatorOwnersPackage::getAutoreviewOptionsMap(); + foreach ($xactions as $xaction) { + $new = $xaction->getNewValue(); + + if (empty($map[$new])) { + $valid = array_keys($map); + + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Invalid'), + pht( + 'Autoreview setting "%s" is not valid. '. + 'Valid settings are: %s.', + $new, + implode(', ', $valid)), + $xaction); + } + } break; case PhabricatorOwnersPackageTransaction::TYPE_PATHS: if (!$xactions) { diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index 14eb618c30..3f24db8508 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -14,6 +14,7 @@ final class PhabricatorOwnersPackage protected $name; protected $originalName; protected $auditingEnabled; + protected $autoReview; protected $description; protected $primaryOwnerPHID; protected $mailKey; @@ -28,6 +29,11 @@ final class PhabricatorOwnersPackage const STATUS_ACTIVE = 'active'; const STATUS_ARCHIVED = 'archived'; + const AUTOREVIEW_NONE = 'none'; + const AUTOREVIEW_SUBSCRIBE = 'subscribe'; + const AUTOREVIEW_REVIEW = 'review'; + const AUTOREVIEW_BLOCK = 'block'; + public static function initializeNewPackage(PhabricatorUser $actor) { $app = id(new PhabricatorApplicationQuery()) ->setViewer($actor) @@ -41,6 +47,7 @@ final class PhabricatorOwnersPackage return id(new PhabricatorOwnersPackage()) ->setAuditingEnabled(0) + ->setAutoReview(self::AUTOREVIEW_NONE) ->setViewPolicy($view_policy) ->setEditPolicy($edit_policy) ->attachPaths(array()) @@ -56,6 +63,24 @@ final class PhabricatorOwnersPackage ); } + public static function getAutoreviewOptionsMap() { + return array( + self::AUTOREVIEW_NONE => array( + 'name' => pht('No Autoreview'), + ), + self::AUTOREVIEW_SUBSCRIBE => array( + 'name' => pht('Subscribe to Changes'), + ), + // TODO: Implement these. + // self::AUTOREVIEW_REVIEW => array( + // 'name' => pht('Review Changes'), + // ), + // self::AUTOREVIEW_BLOCK => array( + // 'name' => pht('Review Changes (Blocking)'), + // ), + ); + } + protected function getConfiguration() { return array( // This information is better available from the history table. @@ -69,6 +94,7 @@ final class PhabricatorOwnersPackage 'auditingEnabled' => 'bool', 'mailKey' => 'bytes20', 'status' => 'text32', + 'autoReview' => 'text32', ), ) + parent::getConfiguration(); } diff --git a/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php b/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php index f59544b1f0..2d51c0aa30 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackageTransaction.php @@ -10,6 +10,7 @@ final class PhabricatorOwnersPackageTransaction const TYPE_DESCRIPTION = 'owners.description'; const TYPE_PATHS = 'owners.paths'; const TYPE_STATUS = 'owners.status'; + const TYPE_AUTOREVIEW = 'owners.autoreview'; public function getApplicationName() { return 'owners'; @@ -143,6 +144,18 @@ final class PhabricatorOwnersPackageTransaction '%s archived this package.', $this->renderHandleLink($author_phid)); } + case self::TYPE_AUTOREVIEW: + $map = PhabricatorOwnersPackage::getAutoreviewOptionsMap(); + $map = ipull($map, 'name'); + + $old = idx($map, $old, $old); + $new = idx($map, $new, $new); + + return pht( + '%s adjusted autoreview from "%s" to "%s".', + $this->renderHandleLink($author_phid), + $old, + $new); } return parent::getTitle(); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 3f51ca7296..de52515c01 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -703,7 +703,13 @@ abstract class PhabricatorApplicationTransactionEditor $xaction->setEditPolicy($this->getActingAsPHID()); } - $xaction->setAuthorPHID($this->getActingAsPHID()); + // If the transaction already has an explicit author PHID, allow it to + // stand. This is used by applications like Owners that hook into the + // post-apply change pipeline. + if (!$xaction->getAuthorPHID()) { + $xaction->setAuthorPHID($this->getActingAsPHID()); + } + $xaction->setContentSource($this->getContentSource()); $xaction->attachViewer($actor); $xaction->attachObject($object); @@ -957,6 +963,12 @@ abstract class PhabricatorApplicationTransactionEditor if ($herald_xactions) { $xscript_id = $this->getHeraldTranscript()->getID(); foreach ($herald_xactions as $herald_xaction) { + // Don't set a transcript ID if this is a transaction from another + // application or source, like Owners. + if ($herald_xaction->getAuthorPHID()) { + continue; + } + $herald_xaction->setMetadataValue('herald:transcriptID', $xscript_id); } @@ -1217,6 +1229,7 @@ abstract class PhabricatorApplicationTransactionEditor $xaction, pht('You can not apply transactions which already have IDs/PHIDs!')); } + if ($xaction->getObjectPHID()) { throw new PhabricatorApplicationTransactionStructureException( $xaction, @@ -1224,13 +1237,7 @@ abstract class PhabricatorApplicationTransactionEditor 'You can not apply transactions which already have %s!', 'objectPHIDs')); } - if ($xaction->getAuthorPHID()) { - throw new PhabricatorApplicationTransactionStructureException( - $xaction, - pht( - 'You can not apply transactions which already have %s!', - 'authorPHIDs')); - } + if ($xaction->getCommentPHID()) { throw new PhabricatorApplicationTransactionStructureException( $xaction, @@ -1238,6 +1245,7 @@ abstract class PhabricatorApplicationTransactionEditor 'You can not apply transactions which already have %s!', 'commentPHIDs')); } + if ($xaction->getCommentVersion() !== 0) { throw new PhabricatorApplicationTransactionStructureException( $xaction,