From d8968755e9d11e2a278c1fb3c7b23a49900a8d40 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 8 Mar 2014 10:51:01 -0800 Subject: [PATCH] Use CustomField for `differential.updaterevision` Summary: Ref T2222. Ref T3794. Medium term goal is to remove `DifferentialRevisionEditor`. This removes one of two callsites. Test Plan: Used `arc diff --edit` to repeatedly update a revision, making changes to various fields. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T3794, T2222 Differential Revision: https://secure.phabricator.com/D8451 --- ...API_differential_updaterevision_Method.php | 123 +++++++++++++++--- .../DifferentialManiphestTasksField.php | 35 ++++- ...habricatorApplicationTransactionEditor.php | 1 + 3 files changed, 135 insertions(+), 24 deletions(-) diff --git a/src/applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php b/src/applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php index d7907194b6..b29c3c8392 100644 --- a/src/applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php +++ b/src/applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php @@ -1,13 +1,10 @@ getUser(); + $diff = id(new DifferentialDiffQuery()) - ->setViewer($request->getUser()) + ->setViewer($viewer) ->withIDs(array($request->getValue('diffid'))) ->executeOne(); if (!$diff) { @@ -44,32 +43,116 @@ final class ConduitAPI_differential_updaterevision_Method $revision = id(new DifferentialRevisionQuery()) ->setViewer($request->getUser()) ->withIDs(array($request->getValue('id'))) + ->needReviewerStatus(true) + ->needActiveDiffs(true) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) ->executeOne(); if (!$revision) { throw new ConduitException('ERR_BAD_REVISION'); } - if ($request->getUser()->getPHID() !== $revision->getAuthorPHID()) { - throw new ConduitException('ERR_WRONG_USER'); - } - if ($revision->getStatus() == ArcanistDifferentialRevisionStatus::CLOSED) { throw new ConduitException('ERR_CLOSED'); } - $content_source = PhabricatorContentSource::newForSource( - PhabricatorContentSource::SOURCE_CONDUIT, - array()); + $field_list = PhabricatorCustomField::getObjectFields( + $revision, + DifferentialCustomField::ROLE_COMMITMESSAGEEDIT); - $editor = new DifferentialRevisionEditor( - $revision); - $editor->setActor($request->getUser()); - $editor->setContentSource($content_source); - $fields = $request->getValue('fields'); - $editor->addDiff($diff, $request->getValue('message')); - $editor->copyFieldsFromConduit($fields); + $field_list + ->setViewer($viewer) + ->readFieldsFromStorage($revision); + $field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit'); - $editor->save(); + $xactions = array(); + + $xactions[] = id(new DifferentialTransaction()) + ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) + ->setNewValue($diff->getPHID()); + + $values = $request->getValue('fields', array()); + foreach ($values as $key => $value) { + $field = idx($field_map, $key); + if (!$field) { + // NOTE: We're just ignoring fields we don't know about. This isn't + // ideal, but the way the workflow currently works involves us getting + // several read-only fields, like the revision ID field, which we should + // just skip. + continue; + } + + $role = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS; + if (!$field->shouldEnableForRole($role)) { + throw new Exception( + pht( + 'Request attempts to update field "%s", but that field can not '. + 'perform transactional updates.', + $key)); + } + + // TODO: This is fairly similar to PhabricatorCustomField's + // buildFieldTransactionsFromRequest() method, but that's currently not + // easy to reuse. + + $transaction_type = $field->getApplicationTransactionType(); + $xaction = id(new DifferentialTransaction()) + ->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); + } + + // The transaction itself will be validated so this is somewhat + // redundant, but this validator will sometimes give us a better error + // message or a better reaction to a bad value type. + $field->validateCommitMessageValue($value); + $field->readValueFromCommitMessage($value); + + $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 $meta_key => $meta_value) { + $xaction->setMetadataValue($meta_key, $meta_value); + } + + $xactions[] = $xaction; + } + + $message = $request->getValue('message'); + if (strlen($message)) { + // This is a little awkward, and should maybe move inside the transaction + // editor. It largely exists for legacy reasons. + $first_line = head(phutil_split_lines($message, false)); + $diff->setDescription($first_line); + $diff->save(); + + $xactions[] = id(new DifferentialTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) + ->attachComment( + id(new DifferentialTransactionComment()) + ->setContent($message)); + } + + $editor = id(new DifferentialTransactionEditor()) + ->setActor($viewer) + ->setContentSourceFromConduitRequest($request) + ->setContinueOnNoEffect(true) + ->setContinueOnMissingFields(true); + + $editor->applyTransactions($revision, $xactions); return array( 'revisionid' => $revision->getID(), diff --git a/src/applications/differential/customfield/DifferentialManiphestTasksField.php b/src/applications/differential/customfield/DifferentialManiphestTasksField.php index e01defb619..6e12b46a01 100644 --- a/src/applications/differential/customfield/DifferentialManiphestTasksField.php +++ b/src/applications/differential/customfield/DifferentialManiphestTasksField.php @@ -1,7 +1,7 @@ getFieldName(); } - public function getRequiredHandlePHIDsForPropertyView() { - if (!$this->getObject()->getPHID()) { + public function readValueFromRevision(DifferentialRevision $revision) { + if (!$revision->getPHID()) { return array(); } return PhabricatorEdgeQuery::loadDestinationPHIDs( - $this->getObject()->getPHID(), + $revision->getPHID(), PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK); } + public function getApplicationTransactionType() { + return PhabricatorTransactions::TYPE_EDGE; + } + + public function getApplicationTransactionMetadata() { + return array( + 'edge:type' => PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK, + ); + } + + public function getNewValueForApplicationTransactions() { + $edges = array(); + foreach ($this->getValue() as $phid) { + $edges[$phid] = $phid; + } + + return array('=' => $edges); + } + + public function getRequiredHandlePHIDsForPropertyView() { + return $this->getValue(); + } + public function renderPropertyViewValue(array $handles) { return $this->renderHandleList($handles); } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index a416d5697f..b95cd975ba 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1496,6 +1496,7 @@ abstract class PhabricatorApplicationTransactionEditor $field_list = PhabricatorCustomField::getObjectFields( $object, PhabricatorCustomField::ROLE_EDIT); + $field_list->setViewer($this->getActor()); $role_xactions = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS; foreach ($field_list->getFields() as $field) {