From de6349dd67afe3a190607753aa3b3229878a6e6e Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Mon, 27 Jun 2016 20:29:47 +0000 Subject: [PATCH] Revision substate CLOSED_FROM_ACCEPTED Summary: Ref T9838. Add a Properties field to Revision, and update a `wasAcceptedBeforeClose` when closing a revision. Test Plan: A quick run through the obvious steps (Close with commit/manually, with or w/o accept) and calling `differential.query` shows the `wasAcceptedBeforeClose` property was setup correctly. Pushing closed + accepted passes the relevant herald, which was my immediate issue; Pushing un-accepted is blocked. Test the "commit" rule (Different from "pre-commit") by hacking the DB and running the "has accepted revision" rule in a test-console. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, epriestley Maniphest Tasks: T9838 Differential Revision: https://secure.phabricator.com/D15085 --- .../20160201.revision.properties.1.sql | 2 ++ .../20160201.revision.properties.2.sql | 2 ++ .../DifferentialQueryConduitAPIMethod.php | 1 + .../editor/DifferentialTransactionEditor.php | 6 +++++ .../storage/DifferentialRevision.php | 17 ++++++++++++ ...usionCommitRevisionAcceptedHeraldField.php | 27 ++++++++++++++++--- ...mmitContentRevisionAcceptedHeraldField.php | 15 ++++++++--- 7 files changed, 63 insertions(+), 7 deletions(-) create mode 100644 resources/sql/autopatches/20160201.revision.properties.1.sql create mode 100644 resources/sql/autopatches/20160201.revision.properties.2.sql diff --git a/resources/sql/autopatches/20160201.revision.properties.1.sql b/resources/sql/autopatches/20160201.revision.properties.1.sql new file mode 100644 index 0000000000..2ab60b2ce9 --- /dev/null +++ b/resources/sql/autopatches/20160201.revision.properties.1.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_revision +ADD properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20160201.revision.properties.2.sql b/resources/sql/autopatches/20160201.revision.properties.2.sql new file mode 100644 index 0000000000..41d3234abe --- /dev/null +++ b/resources/sql/autopatches/20160201.revision.properties.2.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_differential.differential_revision +SET properties = '{}' WHERE properties = ''; diff --git a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php index 2bf43d295e..1d4bcc2a8d 100644 --- a/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialQueryConduitAPIMethod.php @@ -206,6 +206,7 @@ final class DifferentialQueryConduitAPIMethod 'statusName' => ArcanistDifferentialRevisionStatus::getNameForRevisionStatus( $revision->getStatus()), + 'properties' => $revision->getProperties(), 'branch' => $diff->getBranch(), 'summary' => $revision->getSummary(), 'testPlan' => $revision->getTestPlan(), diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index a87c12c926..bf2df0acee 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -182,6 +182,7 @@ final class DifferentialTransactionEditor $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION; $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED; $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED; + $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_INLINE: @@ -233,7 +234,12 @@ final class DifferentialTransactionEditor $object->setStatus($status_review); return; case DifferentialAction::ACTION_CLOSE: + $old_status = $object->getStatus(); $object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED); + $was_accepted = ($old_status == $status_accepted); + $object->setProperty( + DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED, + $was_accepted); return; case DifferentialAction::ACTION_CLAIM: $object->setAuthorPHID($this->getActingAsPHID()); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index c2915d92c6..e0c8effada 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -35,6 +35,7 @@ final class DifferentialRevision extends DifferentialDAO protected $repositoryPHID; protected $viewPolicy = PhabricatorPolicies::POLICY_USER; protected $editPolicy = PhabricatorPolicies::POLICY_USER; + protected $properties = array(); private $relationships = self::ATTACHABLE; private $commits = self::ATTACHABLE; @@ -53,6 +54,8 @@ final class DifferentialRevision extends DifferentialDAO const RELATION_REVIEWER = 'revw'; const RELATION_SUBSCRIBED = 'subd'; + const PROPERTY_CLOSED_FROM_ACCEPTED = 'wasAcceptedBeforeClose'; + public static function initializeNewRevision(PhabricatorUser $actor) { $app = id(new PhabricatorApplicationQuery()) ->setViewer($actor) @@ -76,6 +79,7 @@ final class DifferentialRevision extends DifferentialDAO self::CONFIG_SERIALIZATION => array( 'attached' => self::SERIALIZATION_JSON, 'unsubscribed' => self::SERIALIZATION_JSON, + 'properties' => self::SERIALIZATION_JSON, ), self::CONFIG_COLUMN_SCHEMA => array( 'title' => 'text255', @@ -114,6 +118,19 @@ final class DifferentialRevision extends DifferentialDAO ) + parent::getConfiguration(); } + public function setProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + + public function getProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + public function hasRevisionProperty($key) { + return array_key_exists($key, $this->properties); + } + public function getMonogram() { $id = $this->getID(); return "D{$id}"; diff --git a/src/applications/diffusion/herald/DiffusionCommitRevisionAcceptedHeraldField.php b/src/applications/diffusion/herald/DiffusionCommitRevisionAcceptedHeraldField.php index 10d9c29c92..810b58f2a1 100644 --- a/src/applications/diffusion/herald/DiffusionCommitRevisionAcceptedHeraldField.php +++ b/src/applications/diffusion/herald/DiffusionCommitRevisionAcceptedHeraldField.php @@ -19,10 +19,31 @@ final class DiffusionCommitRevisionAcceptedHeraldField return null; } + $status = $revision->getStatus(); + + switch ($status) { + case ArcanistDifferentialRevisionStatus::ACCEPTED: + return $revision->getPHID(); + case ArcanistDifferentialRevisionStatus::CLOSED: + if ($revision->hasRevisionProperty( + DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED)) { + + if ($revision->getProperty( + DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED)) { + return $revision->getPHID(); + } else { + return null; + } + } else { + // continue on to old-style precommitRevisionStatus + break; + } + default: + return null; + } + $data = $object->getCommitData(); - $status = $data->getCommitDetail( - 'precommitRevisionStatus', - $revision->getStatus()); + $status = $data->getCommitDetail('precommitRevisionStatus'); switch ($status) { case ArcanistDifferentialRevisionStatus::ACCEPTED: diff --git a/src/applications/diffusion/herald/DiffusionPreCommitContentRevisionAcceptedHeraldField.php b/src/applications/diffusion/herald/DiffusionPreCommitContentRevisionAcceptedHeraldField.php index 864cc043af..e19878e627 100644 --- a/src/applications/diffusion/herald/DiffusionPreCommitContentRevisionAcceptedHeraldField.php +++ b/src/applications/diffusion/herald/DiffusionPreCommitContentRevisionAcceptedHeraldField.php @@ -20,12 +20,19 @@ final class DiffusionPreCommitContentRevisionAcceptedHeraldField return null; } - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; - if ($revision->getStatus() != $status_accepted) { - return null; + switch ($revision->getStatus()) { + case ArcanistDifferentialRevisionStatus::ACCEPTED: + return $revision->getPHID(); + case ArcanistDifferentialRevisionStatus::CLOSED: + if ($revision->getProperty( + DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED)) { + + return $revision->getPHID(); + } + break; } - return $revision->getPHID(); + return null; } protected function getHeraldFieldStandardType() {