diff --git a/src/applications/differential/conduit/ConduitAPI_differential_Method.php b/src/applications/differential/conduit/ConduitAPI_differential_Method.php --- a/src/applications/differential/conduit/ConduitAPI_differential_Method.php +++ b/src/applications/differential/conduit/ConduitAPI_differential_Method.php @@ -43,5 +43,109 @@ ); } + protected function applyFieldEdit( + ConduitAPIRequest $request, + DifferentialRevision $revision, + DifferentialDiff $diff, + array $fields, + $message) { + + $viewer = $request->getUser(); + + $field_list = PhabricatorCustomField::getObjectFields( + $revision, + DifferentialCustomField::ROLE_COMMITMESSAGEEDIT); + + $field_list + ->setViewer($viewer) + ->readFieldsFromStorage($revision); + $field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit'); + + $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); + } } diff --git a/src/applications/differential/conduit/ConduitAPI_differential_createrevision_Method.php b/src/applications/differential/conduit/ConduitAPI_differential_createrevision_Method.php --- a/src/applications/differential/conduit/ConduitAPI_differential_createrevision_Method.php +++ b/src/applications/differential/conduit/ConduitAPI_differential_createrevision_Method.php @@ -1,13 +1,10 @@ getValue('fields'); + $viewer = $request->getUser(); $diff = id(new DifferentialDiffQuery()) - ->setViewer($request->getUser()) + ->setViewer($viewer) ->withIDs(array($request->getValue('diffid'))) ->executeOne(); if (!$diff) { throw new ConduitException('ERR_BAD_DIFF'); } - $revision = DifferentialRevisionEditor::newRevisionFromConduitWithDiff( - $fields, + $revision = DifferentialRevision::initializeNewRevision($viewer); + $revision->attachReviewerStatus(array()); + + $this->applyFieldEdit( + $request, + $revision, $diff, - $request->getUser()); + $request->getValue('fields', array()), + $message = null); return array( 'revisionid' => $revision->getID(), diff --git a/src/applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php b/src/applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php --- a/src/applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php +++ b/src/applications/differential/conduit/ConduitAPI_differential_updaterevision_Method.php @@ -1,7 +1,7 @@ applyFieldEdit( + $request, $revision, - DifferentialCustomField::ROLE_COMMITMESSAGEEDIT); - - $field_list - ->setViewer($viewer) - ->readFieldsFromStorage($revision); - $field_map = mpull($field_list->getFields(), null, 'getFieldKeyForConduit'); - - $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); + $diff, + $request->getValue('fields', array()), + $request->getValue('message')); return array( 'revisionid' => $revision->getID(), diff --git a/src/applications/differential/customfield/DifferentialSummaryField.php b/src/applications/differential/customfield/DifferentialSummaryField.php --- a/src/applications/differential/customfield/DifferentialSummaryField.php +++ b/src/applications/differential/customfield/DifferentialSummaryField.php @@ -21,6 +21,9 @@ protected function readValueFromRevision( DifferentialRevision $revision) { + if (!$revision->getID()) { + return null; + } return $revision->getSummary(); } @@ -82,6 +85,11 @@ $xaction->getNewValue()); } + public function shouldHideInApplicationTransactions( + PhabricatorApplicationTransaction $xaction) { + return ($xaction->getOldValue() === null); + } + public function shouldAppearInGlobalSearch() { return true; } diff --git a/src/applications/differential/customfield/DifferentialTestPlanField.php b/src/applications/differential/customfield/DifferentialTestPlanField.php --- a/src/applications/differential/customfield/DifferentialTestPlanField.php +++ b/src/applications/differential/customfield/DifferentialTestPlanField.php @@ -21,6 +21,9 @@ protected function readValueFromRevision( DifferentialRevision $revision) { + if (!$revision->getID()) { + return null; + } return $revision->getTestPlan(); } @@ -96,6 +99,10 @@ $xaction->getNewValue()); } + public function shouldHideInApplicationTransactions( + PhabricatorApplicationTransaction $xaction) { + return ($xaction->getOldValue() === null); + } public function shouldAppearInGlobalSearch() { return true; diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -20,10 +20,23 @@ } public function shouldHide() { + $old = $this->getOldValue(); + $new = $this->getNewValue(); + switch ($this->getTransactionType()) { + case self::TYPE_UPDATE: + // Older versions of this transaction have an ID for the new value, + // and/or do not record the old value. Only hide the transaction if + // the new value is a PHID, indicating that this is a newer style + // transaction. + if ($old === null) { + if (phid_get_type($new) == DifferentialPHIDTypeDiff::TYPECONST) { + return true; + } + } + break; + case PhabricatorTransactions::TYPE_EDGE: - $old = $this->getOldValue(); - $new = $this->getNewValue(); $add = array_diff_key($new, $old); $rem = array_diff_key($old, $new); @@ -38,7 +51,7 @@ break; } - return false; + return parent::shouldHide(); } public function shouldHideForMail(array $xactions) { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -356,6 +356,11 @@ return false; } break; + case PhabricatorTransactions::TYPE_CUSTOMFIELD: + $field = $this->getTransactionCustomField(); + if ($field) { + return $field->shouldHideInApplicationTransactions($this); + } } return false; diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -991,6 +991,14 @@ return array(); } + public function shouldHideInApplicationTransactions( + PhabricatorApplicationTransaction $xaction) { + if ($this->proxy) { + return $this->proxy->shouldHideInApplicationTransactions($xaction); + } + return false; + } + /* -( Edit View )---------------------------------------------------------- */