From c5ba75ee9ed20afe502f84d82c8843ab4a7bd393 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 21 Feb 2014 11:54:32 -0800 Subject: [PATCH] Implement a "Reviewers" CustomField Summary: Ref T3886: - Adds a "Reviewers" field as a modern CustomField. Ref T418: - Allows CustomFields to emit transaction metadata. Test Plan: {F116254} Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T418, T3886 Differential Revision: https://secure.phabricator.com/D8291 --- src/__phutil_library_map__.php | 2 + .../DifferentialReviewersField.php | 84 +++++++++++++++++++ .../editor/DifferentialTransactionEditor.php | 7 +- .../storage/DifferentialReviewer.php | 7 ++ .../storage/DifferentialRevision.php | 1 + ...habricatorApplicationTransactionEditor.php | 47 +++++++++-- .../field/PhabricatorCustomField.php | 11 +++ .../field/PhabricatorCustomFieldList.php | 24 ++++-- 8 files changed, 165 insertions(+), 18 deletions(-) create mode 100644 src/applications/differential/customfield/DifferentialReviewersField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c3c521a57f..37968fbff3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -441,6 +441,7 @@ phutil_register_library_map(array( 'DifferentialReviewedByFieldSpecification' => 'applications/differential/field/specification/DifferentialReviewedByFieldSpecification.php', 'DifferentialReviewer' => 'applications/differential/storage/DifferentialReviewer.php', 'DifferentialReviewerStatus' => 'applications/differential/constants/DifferentialReviewerStatus.php', + 'DifferentialReviewersField' => 'applications/differential/customfield/DifferentialReviewersField.php', 'DifferentialReviewersFieldSpecification' => 'applications/differential/field/specification/DifferentialReviewersFieldSpecification.php', 'DifferentialReviewersView' => 'applications/differential/view/DifferentialReviewersView.php', 'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php', @@ -2979,6 +2980,7 @@ phutil_register_library_map(array( 'DifferentialRevertPlanFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialReviewRequestMail' => 'DifferentialMail', 'DifferentialReviewedByFieldSpecification' => 'DifferentialFieldSpecification', + 'DifferentialReviewersField' => 'DifferentialCoreCustomField', 'DifferentialReviewersFieldSpecification' => 'DifferentialFieldSpecification', 'DifferentialReviewersView' => 'AphrontView', 'DifferentialRevision' => diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php new file mode 100644 index 0000000000..98fa5dcb74 --- /dev/null +++ b/src/applications/differential/customfield/DifferentialReviewersField.php @@ -0,0 +1,84 @@ +getReviewerStatus(); + } + + public function getNewValueForApplicationTransactions() { + $specs = array(); + foreach ($this->getValue() as $reviewer) { + $specs[$reviewer->getReviewerPHID()] = array( + 'data' => $reviewer->getEdgeData(), + ); + } + + return array('=' => $specs); + } + + public function readValueFromRequest(AphrontRequest $request) { + // Compute a new set of reviewer objects. For reviewers who haven't been + // added or removed, retain their existing status. Also, respect the new + // order. + + $old_status = $this->getValue(); + $old_status = mpull($old_status, null, 'getReviewerPHID'); + + $new_phids = $request->getArr($this->getFieldKey()); + $new_phids = array_fuse($new_phids); + + $new_status = array(); + foreach ($new_phids as $new_phid) { + if (empty($old_status[$new_phid])) { + $new_status[$new_phid] = new DifferentialReviewer( + $new_phid, + array( + 'status' => DifferentialReviewerStatus::STATUS_ADDED, + )); + } else { + $new_status[$new_phid] = $old_status[$new_phid]; + } + } + + $this->setValue($new_status); + } + + public function getRequiredHandlePHIDsForEdit() { + return mpull($this->getValue(), 'getReviewerPHID'); + } + + public function renderEditControl(array $handles) { + return id(new AphrontFormTokenizerControl()) + ->setName($this->getFieldKey()) + ->setDatasource('/typeahead/common/usersorprojects/') + ->setValue($handles) + ->setError($this->getFieldError()) + ->setLabel($this->getFieldName()); + } + + public function getApplicationTransactionType() { + return PhabricatorTransactions::TYPE_EDGE; + } + + public function getApplicationTransactionMetadata() { + return array( + 'edge:type' => PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER, + ); + } + +} diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index afbe9614a4..6838ef0293 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -6,11 +6,11 @@ final class DifferentialTransactionEditor public function getTransactionTypes() { $types = parent::getTransactionTypes(); + $types[] = PhabricatorTransactions::TYPE_EDGE; $types[] = PhabricatorTransactions::TYPE_VIEW_POLICY; $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; /* - $types[] = PhabricatorTransactions::TYPE_EDGE; $types[] = DifferentialTransaction::TYPE_INLINE; $types[] = DifferentialTransaction::TYPE_UPDATE; @@ -59,6 +59,9 @@ final class DifferentialTransactionEditor $object->setEditPolicy($xaction->getNewValue()); return; case PhabricatorTransactions::TYPE_SUBSCRIBERS: + case PhabricatorTransactions::TYPE_EDGE: + // TODO: When removing reviewers, we may be able to move the revision + // to "Accepted". return; } @@ -74,6 +77,7 @@ final class DifferentialTransactionEditor case PhabricatorTransactions::TYPE_EDIT_POLICY: return; case PhabricatorTransactions::TYPE_SUBSCRIBERS: + case PhabricatorTransactions::TYPE_EDGE: return; } @@ -93,7 +97,6 @@ final class DifferentialTransactionEditor return $errors; } - protected function requireCapabilities( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index 62ae27d8f8..dd15ce4004 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -46,4 +46,11 @@ final class DifferentialReviewer { return $this->authority[$viewer_phid]; } + public function getEdgeData() { + return array( + 'status' => $this->status, + 'diffID' => $this->diffID, + ); + } + } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index c70ce306d0..efe8f44ae4 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -469,6 +469,7 @@ final class DifferentialRevision extends DifferentialDAO new DifferentialTitleField(), new DifferentialSummaryField(), new DifferentialTestPlanField(), + new DifferentialReviewersField(), new DifferentialSubscribersField(), new DifferentialRepositoryField(), new DifferentialViewPolicyField(), diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 6217763d07..35d6f1e96c 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -990,21 +990,24 @@ abstract class PhabricatorApplicationTransactionEditor } $result[$dst_phid] = $this->normalizeEdgeTransactionValue( $xaction, - $edge); + $edge, + $dst_phid); } if ($new_set !== null) { foreach ($new_set as $dst_phid => $edge) { $result[$dst_phid] = $this->normalizeEdgeTransactionValue( $xaction, - $edge); + $edge, + $dst_phid); } } foreach ($new_add as $dst_phid => $edge) { $result[$dst_phid] = $this->normalizeEdgeTransactionValue( $xaction, - $edge); + $edge, + $dst_phid); } foreach ($new_rem as $dst_phid => $edge) { @@ -1032,18 +1035,44 @@ abstract class PhabricatorApplicationTransactionEditor } } - protected function normalizeEdgeTransactionValue( + private function normalizeEdgeTransactionValue( PhabricatorApplicationTransaction $xaction, - $edge) { + $edge, + $dst_phid) { if (!is_array($edge)) { - $edge = array( - 'dst' => $edge, - ); + if ($edge != $dst_phid) { + throw new Exception( + pht( + 'Transaction edge data must either be the edge PHID or an edge '. + 'specification dictionary.')); + } + $edge = array(); + } else { + foreach ($edge as $key => $value) { + switch ($key) { + case 'src': + case 'dst': + case 'type': + case 'data': + case 'dateCreated': + case 'dateModified': + case 'seq': + case 'dataID': + break; + default: + throw new Exception( + pht( + 'Transaction edge specification contains unexpected key '. + '"%s".', + $key)); + } + } } - $edge_type = $xaction->getMetadataValue('edge:type'); + $edge['dst'] = $dst_phid; + $edge_type = $xaction->getMetadataValue('edge:type'); if (empty($edge['type'])) { $edge['type'] = $edge_type; } else { diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index 1167c49ab8..56f48e889f 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -744,6 +744,17 @@ abstract class PhabricatorCustomField { } + /** + * @task appxaction + */ + public function getApplicationTransactionMetadata() { + if ($this->proxy) { + return $this->proxy->getApplicationTransactionMetadata(); + } + return array(); + } + + /** * @task appxaction */ diff --git a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php index 5b184aca5c..d5623771dd 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php @@ -199,22 +199,32 @@ final class PhabricatorCustomFieldList extends Phobject { continue; } - $old_value = $field->getOldValueForApplicationTransactions(); - - $field->readValueFromRequest($request); $transaction_type = $field->getApplicationTransactionType(); - $xaction = id(clone $template) - ->setTransactionType($transaction_type) - ->setMetadataValue('customfield:key', $field->getFieldKey()) - ->setNewValue($field->getNewValueForApplicationTransactions()); + ->setTransactionType($transaction_type); if ($transaction_type == PhabricatorTransactions::TYPE_CUSTOMFIELD) { // For TYPE_CUSTOMFIELD transactions only, we provide the old value // as an input. + $old_value = $field->getOldValueForApplicationTransactions(); $xaction->setOldValue($old_value); } + $field->readValueFromRequest($request); + + $xaction + ->setNewValue($field->getNewValueForApplicationTransactions()); + + if ($transaction_type == PhabricatorTransactions::TYPE_CUSTOMFIELD) { + // For TYPE_CUSTOMFIELD transactions, add the field key in metadata. + $xaction->setMetadataValue('customfield:key', $field->getFieldKey()); + } + + $metadata = $field->getApplicationTransactionMetadata(); + foreach ($metadata as $key => $value) { + $xaction->setMetadataValue($key, $value); + } + $xactions[] = $xaction; }