From 64509dcca7be680ce3c85c8d0269bc515907ab3f Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 16 Dec 2016 04:45:55 -0800 Subject: [PATCH] Drive CLI-based revision edits through "differential.revision.edit" API + EditEngine Summary: Ref T11114. This creates `differential.revision.edit` (a modern, v3 API method) and redefines the existing methods in terms of it. Both `differential.createrevision` and `differential.updaterevision` are now internally implemented by building a `differential.revision.edit` API call and then executing it. I //think// this covers everything except custom fields, which need some tweaking to work with EditEngine. I'll clean that up in the next change. Test Plan: - Created, updated, and edited revisions via `arc`. - Called APIs manually via test console. - Stored custom fields ("Blame Rev", "Revert Plan") aren't exposed yet. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11114 Differential Revision: https://secure.phabricator.com/D17067 --- src/__phutil_library_map__.php | 2 + .../conduit/DifferentialConduitAPIMethod.php | 88 ++++++------------- ...erentialCreateRevisionConduitAPIMethod.php | 8 +- ...entialGetCommitMessageConduitAPIMethod.php | 12 ++- ...fferentialRevisionEditConduitAPIMethod.php | 18 ++++ .../editor/DifferentialRevisionEditEngine.php | 39 ++++---- .../DifferentialCommitMessageCustomField.php | 5 ++ .../field/DifferentialCommitMessageField.php | 7 ++ ...ifferentialConflictsCommitMessageField.php | 4 + ...DifferentialGitSVNIDCommitMessageField.php | 4 + ...fferentialReviewedByCommitMessageField.php | 4 + ...ifferentialReviewersCommitMessageField.php | 24 +++++ ...fferentialRevisionIDCommitMessageField.php | 4 + ...ferentialSubscribersCommitMessageField.php | 9 ++ .../DifferentialSummaryCommitMessageField.php | 9 ++ .../DifferentialTagsCommitMessageField.php | 8 ++ .../DifferentialTasksCommitMessageField.php | 4 + ...DifferentialTestPlanCommitMessageField.php | 9 ++ .../DifferentialTitleCommitMessageField.php | 9 ++ ...fferentialRevisionReviewersTransaction.php | 13 ++- ...DifferentialRevisionSummaryTransaction.php | 1 + ...ifferentialRevisionTestPlanTransaction.php | 1 + .../DifferentialRevisionTitleTransaction.php | 1 + ...PhabricatorProjectsEditEngineExtension.php | 10 ++- ...icatorSubscriptionsEditEngineExtension.php | 10 ++- .../PhabricatorCommentEditEngineExtension.php | 3 +- .../PhabricatorTypeaheadDatasource.php | 23 ++++- 27 files changed, 231 insertions(+), 98 deletions(-) create mode 100644 src/applications/differential/conduit/DifferentialRevisionEditConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 798aa49cbe..c89c4733c5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -527,6 +527,7 @@ phutil_register_library_map(array( 'DifferentialRevisionControlSystem' => 'applications/differential/constants/DifferentialRevisionControlSystem.php', 'DifferentialRevisionDependedOnByRevisionEdgeType' => 'applications/differential/edge/DifferentialRevisionDependedOnByRevisionEdgeType.php', 'DifferentialRevisionDependsOnRevisionEdgeType' => 'applications/differential/edge/DifferentialRevisionDependsOnRevisionEdgeType.php', + 'DifferentialRevisionEditConduitAPIMethod' => 'applications/differential/conduit/DifferentialRevisionEditConduitAPIMethod.php', 'DifferentialRevisionEditEngine' => 'applications/differential/editor/DifferentialRevisionEditEngine.php', 'DifferentialRevisionEditProController' => 'applications/differential/controller/DifferentialRevisionEditProController.php', 'DifferentialRevisionFulltextEngine' => 'applications/differential/search/DifferentialRevisionFulltextEngine.php', @@ -5203,6 +5204,7 @@ phutil_register_library_map(array( 'DifferentialRevisionControlSystem' => 'Phobject', 'DifferentialRevisionDependedOnByRevisionEdgeType' => 'PhabricatorEdgeType', 'DifferentialRevisionDependsOnRevisionEdgeType' => 'PhabricatorEdgeType', + 'DifferentialRevisionEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod', 'DifferentialRevisionEditEngine' => 'PhabricatorEditEngine', 'DifferentialRevisionEditProController' => 'DifferentialController', 'DifferentialRevisionFulltextEngine' => 'PhabricatorFulltextEngine', diff --git a/src/applications/differential/conduit/DifferentialConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialConduitAPIMethod.php index 111e0f04b4..34843b5609 100644 --- a/src/applications/differential/conduit/DifferentialConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialConduitAPIMethod.php @@ -53,21 +53,16 @@ abstract class DifferentialConduitAPIMethod extends ConduitAPIMethod { $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'); + // We're going to build the body of a "differential.revision.edit" API + // request, then just call that code directly. $xactions = array(); + $xactions[] = array( + 'type' => DifferentialRevisionEditEngine::KEY_UPDATE, + 'value' => $diff->getPHID(), + ); - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) - ->setNewValue($diff->getPHID()); - + $field_map = DifferentialCommitMessageField::newEnabledFields($viewer); $values = $request->getValue('fields', array()); foreach ($values as $key => $value) { $field = idx($field_map, $key); @@ -79,53 +74,20 @@ abstract class DifferentialConduitAPIMethod extends ConduitAPIMethod { continue; } - $role = PhabricatorCustomField::ROLE_APPLICATIONTRANSACTIONS; - if (!$field->shouldEnableForRole($role)) { - continue; - } - - // 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); + $value = $field->readFieldValueFromConduit($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()); + foreach ($field->getFieldTransactions($value) as $xaction) { + $xactions[] = $xaction; } - - $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. See some discussion in - // T7899. + // This is a little awkward, and should move elsewhere or be removed. It + // largely exists for legacy reasons. See some discussion in T7899. $first_line = head(phutil_split_lines($message, false)); $first_line = id(new PhutilUTF8StringTruncator()) @@ -136,20 +98,24 @@ abstract class DifferentialConduitAPIMethod extends ConduitAPIMethod { $diff->setDescription($first_line); $diff->save(); - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) - ->attachComment( - id(new DifferentialTransactionComment()) - ->setContent($message)); + $xactions[] = array( + 'type' => PhabricatorCommentEditEngineExtension::EDITKEY, + 'value' => $message, + ); } - $editor = id(new DifferentialTransactionEditor()) - ->setActor($viewer) - ->setContentSource($request->newContentSource()) - ->setContinueOnNoEffect(true) - ->setContinueOnMissingFields(true); + $method = 'differential.revision.edit'; + $params = array( + 'transactions' => $xactions, + ); - $editor->applyTransactions($revision, $xactions); + if ($revision->getID()) { + $params['objectIdentifier'] = $revision->getID(); + } + + return id(new ConduitCall($method, $params, $strict = true)) + ->setUser($viewer) + ->execute(); } protected function loadCustomFieldsForRevisions( diff --git a/src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php index 95b4309c70..371d57cd9c 100644 --- a/src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialCreateRevisionConduitAPIMethod.php @@ -45,16 +45,18 @@ final class DifferentialCreateRevisionConduitAPIMethod $revision = DifferentialRevision::initializeNewRevision($viewer); $revision->attachReviewerStatus(array()); - $this->applyFieldEdit( + $result = $this->applyFieldEdit( $request, $revision, $diff, $request->getValue('fields', array()), $message = null); + $revision_id = $result['object']['id']; + return array( - 'revisionid' => $revision->getID(), - 'uri' => PhabricatorEnv::getURI('/D'.$revision->getID()), + 'revisionid' => $revision_id, + 'uri' => PhabricatorEnv::getURI('/D'.$revision_id), ); } diff --git a/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php index 57dd419790..2c5c936679 100644 --- a/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialGetCommitMessageConduitAPIMethod.php @@ -49,9 +49,15 @@ final class DifferentialGetCommitMessageConduitAPIMethod $revision = DifferentialRevision::initializeNewRevision($viewer); } + // There are three modes here: "edit", "create", and "read" (which has + // no value for the "edit" parameter). + + // In edit or create mode, we hide read-only fields. In create mode, we + // show "Field:" templates for some fields even if they are empty. $edit_mode = $request->getValue('edit'); + + $is_any_edit = (bool)strlen($edit_mode); $is_create = ($edit_mode == 'create'); - $is_edit = ($edit_mode && !$is_create); $field_list = DifferentialCommitMessageField::newEnabledFields($viewer); @@ -62,7 +68,7 @@ final class DifferentialGetCommitMessageConduitAPIMethod // If we're editing the message, remove fields like "Conflicts" and // "git-svn-id" which should not be presented to the user for editing. - if ($is_edit) { + if ($is_any_edit) { foreach ($field_list as $field_key => $field) { if (!$field->isFieldEditable()) { unset($field_list[$field_key]); @@ -96,7 +102,7 @@ final class DifferentialGetCommitMessageConduitAPIMethod $wire_value = $value_map[$field_key]; $value = $field->renderFieldValue($wire_value); - $is_template = ($is_edit && $field->isTemplateField()); + $is_template = ($is_create && $field->isTemplateField()); if (!is_string($value) && !is_null($value)) { throw new Exception( diff --git a/src/applications/differential/conduit/DifferentialRevisionEditConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialRevisionEditConduitAPIMethod.php new file mode 100644 index 0000000000..cb7c7e2e93 --- /dev/null +++ b/src/applications/differential/conduit/DifferentialRevisionEditConduitAPIMethod.php @@ -0,0 +1,18 @@ +setKey('update') - ->setLabel(pht('Update Diff')) - ->setDescription(pht('New diff to create or update the revision with.')) - ->setConduitDescription(pht('Create or update a revision with a diff.')) - ->setConduitTypeDescription(pht('PHID of the diff.')) - ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) - ->setHandleParameterType(new AphrontPHIDListHTTPParameterType()) - ->setSingleValue($diff_phid) - ->setIsReorderable(false) - ->setIsDefaultable(false) - ->setIsInvisible(true) - ->setIsLockable(false); - } + $fields[] = id(new PhabricatorHandlesEditField()) + ->setKey(self::KEY_UPDATE) + ->setLabel(pht('Update Diff')) + ->setDescription(pht('New diff to create or update the revision with.')) + ->setConduitDescription(pht('Create or update a revision with a diff.')) + ->setConduitTypeDescription(pht('PHID of the diff.')) + ->setTransactionType(DifferentialTransaction::TYPE_UPDATE) + ->setHandleParameterType(new AphrontPHIDListHTTPParameterType()) + ->setSingleValue($diff_phid) + ->setIsConduitOnly(!$diff) + ->setIsReorderable(false) + ->setIsDefaultable(false) + ->setIsInvisible(true) + ->setIsLockable(false); if ($is_update) { $fields[] = id(new PhabricatorInstructionsEditField()) @@ -134,7 +135,7 @@ final class DifferentialRevisionEditEngine } $fields[] = id(new PhabricatorTextEditField()) - ->setKey('title') + ->setKey(DifferentialRevisionTitleTransaction::EDITKEY) ->setLabel(pht('Title')) ->setIsRequired(true) ->setTransactionType( @@ -145,7 +146,7 @@ final class DifferentialRevisionEditEngine ->setValue($object->getTitle()); $fields[] = id(new PhabricatorRemarkupEditField()) - ->setKey('summary') + ->setKey(DifferentialRevisionSummaryTransaction::EDITKEY) ->setLabel(pht('Summary')) ->setTransactionType( DifferentialRevisionSummaryTransaction::TRANSACTIONTYPE) @@ -156,7 +157,7 @@ final class DifferentialRevisionEditEngine if ($plan_enabled) { $fields[] = id(new PhabricatorRemarkupEditField()) - ->setKey('testPlan') + ->setKey(DifferentialRevisionTestPlanTransaction::EDITKEY) ->setLabel(pht('Test Plan')) ->setIsRequired($plan_required) ->setTransactionType( @@ -169,7 +170,7 @@ final class DifferentialRevisionEditEngine } $fields[] = id(new PhabricatorDatasourceEditField()) - ->setKey('reviewerPHIDs') + ->setKey(DifferentialRevisionReviewersTransaction::EDITKEY) ->setLabel(pht('Reviewers')) ->setDatasource(new DifferentialReviewerDatasource()) ->setUseEdgeTransactions(true) diff --git a/src/applications/differential/field/DifferentialCommitMessageCustomField.php b/src/applications/differential/field/DifferentialCommitMessageCustomField.php index 8e7e81739e..b53b8d3147 100644 --- a/src/applications/differential/field/DifferentialCommitMessageCustomField.php +++ b/src/applications/differential/field/DifferentialCommitMessageCustomField.php @@ -60,4 +60,9 @@ abstract class DifferentialCommitMessageCustomField return $idx; } + public function getFieldTransactions($value) { + // TODO: Implement this! + return array(); + } + } diff --git a/src/applications/differential/field/DifferentialCommitMessageField.php b/src/applications/differential/field/DifferentialCommitMessageField.php index 81ffd0fd2e..6e3a0dbbd9 100644 --- a/src/applications/differential/field/DifferentialCommitMessageField.php +++ b/src/applications/differential/field/DifferentialCommitMessageField.php @@ -67,6 +67,13 @@ abstract class DifferentialCommitMessageField return $value; } + public function getFieldTransactions($value) { + if (!$this->isFieldEditable()) { + return array(); + } + throw new PhutilMethodNotImplementedException(); + } + final public function getCommitMessageFieldKey() { return $this->getPhobjectClassConstant('FIELDKEY', 64); } diff --git a/src/applications/differential/field/DifferentialConflictsCommitMessageField.php b/src/applications/differential/field/DifferentialConflictsCommitMessageField.php index 360b799b34..5a4c281157 100644 --- a/src/applications/differential/field/DifferentialConflictsCommitMessageField.php +++ b/src/applications/differential/field/DifferentialConflictsCommitMessageField.php @@ -17,4 +17,8 @@ final class DifferentialConflictsCommitMessageField return false; } + public function isTemplateField() { + return false; + } + } diff --git a/src/applications/differential/field/DifferentialGitSVNIDCommitMessageField.php b/src/applications/differential/field/DifferentialGitSVNIDCommitMessageField.php index 786f05a2c3..c4b84afc12 100644 --- a/src/applications/differential/field/DifferentialGitSVNIDCommitMessageField.php +++ b/src/applications/differential/field/DifferentialGitSVNIDCommitMessageField.php @@ -17,4 +17,8 @@ final class DifferentialGitSVNIDCommitMessageField return false; } + public function isTemplateField() { + return false; + } + } diff --git a/src/applications/differential/field/DifferentialReviewedByCommitMessageField.php b/src/applications/differential/field/DifferentialReviewedByCommitMessageField.php index 9c8bf5fb40..523697b1e7 100644 --- a/src/applications/differential/field/DifferentialReviewedByCommitMessageField.php +++ b/src/applications/differential/field/DifferentialReviewedByCommitMessageField.php @@ -27,6 +27,10 @@ final class DifferentialReviewedByCommitMessageField return false; } + public function isTemplateField() { + return false; + } + public function readFieldValueFromObject(DifferentialRevision $revision) { if (!$revision->getPHID()) { return array(); diff --git a/src/applications/differential/field/DifferentialReviewersCommitMessageField.php b/src/applications/differential/field/DifferentialReviewersCommitMessageField.php index fd55d8f06d..0f110b6e3a 100644 --- a/src/applications/differential/field/DifferentialReviewersCommitMessageField.php +++ b/src/applications/differential/field/DifferentialReviewersCommitMessageField.php @@ -77,6 +77,30 @@ final class DifferentialReviewersCommitMessageField return $this->renderHandleList($phid_list, $suffix_map); } + public function getFieldTransactions($value) { + $value = $this->inflateReviewers($value); + + $reviewer_list = array(); + foreach ($value as $reviewer) { + $phid = $reviewer['phid']; + if (isset($reviewer['suffixes']['!'])) { + $reviewer_list[] = 'blocking('.$phid.')'; + } else { + $reviewer_list[] = $phid; + } + } + + $xaction_key = DifferentialRevisionReviewersTransaction::EDITKEY; + $xaction_type = "{$xaction_key}.set"; + + return array( + array( + 'type' => $xaction_type, + 'value' => $reviewer_list, + ), + ); + } + private function flattenReviewers(array $values) { // NOTE: For now, `arc` relies on this field returning only scalars, so we // need to reduce the results into scalars. See T10981. diff --git a/src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php b/src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php index c440a5d4c8..1c4a2c9d60 100644 --- a/src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php +++ b/src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php @@ -76,4 +76,8 @@ final class DifferentialRevisionIDCommitMessageField return PhabricatorEnv::getProductionURI('/D'.$value); } + public function getFieldTransactions($value) { + return array(); + } + } diff --git a/src/applications/differential/field/DifferentialSubscribersCommitMessageField.php b/src/applications/differential/field/DifferentialSubscribersCommitMessageField.php index 713dc9d3ed..12d36e4381 100644 --- a/src/applications/differential/field/DifferentialSubscribersCommitMessageField.php +++ b/src/applications/differential/field/DifferentialSubscribersCommitMessageField.php @@ -48,4 +48,13 @@ final class DifferentialSubscribersCommitMessageField return $this->renderHandleList($value); } + public function getFieldTransactions($value) { + return array( + array( + 'type' => PhabricatorSubscriptionsEditEngineExtension::EDITKEY_SET, + 'value' => $value, + ), + ); + } + } diff --git a/src/applications/differential/field/DifferentialSummaryCommitMessageField.php b/src/applications/differential/field/DifferentialSummaryCommitMessageField.php index 2689205fbe..6b57942470 100644 --- a/src/applications/differential/field/DifferentialSummaryCommitMessageField.php +++ b/src/applications/differential/field/DifferentialSummaryCommitMessageField.php @@ -17,4 +17,13 @@ final class DifferentialSummaryCommitMessageField return $revision->getSummary(); } + public function getFieldTransactions($value) { + return array( + array( + 'type' => DifferentialRevisionSummaryTransaction::EDITKEY, + 'value' => $value, + ), + ); + } + } diff --git a/src/applications/differential/field/DifferentialTagsCommitMessageField.php b/src/applications/differential/field/DifferentialTagsCommitMessageField.php index bd03ba639e..fc6267f1ce 100644 --- a/src/applications/differential/field/DifferentialTagsCommitMessageField.php +++ b/src/applications/differential/field/DifferentialTagsCommitMessageField.php @@ -54,5 +54,13 @@ final class DifferentialTagsCommitMessageField return $this->renderHandleList($value); } + public function getFieldTransactions($value) { + return array( + array( + 'type' => PhabricatorProjectsEditEngineExtension::EDITKEY_SET, + 'value' => $value, + ), + ); + } } diff --git a/src/applications/differential/field/DifferentialTasksCommitMessageField.php b/src/applications/differential/field/DifferentialTasksCommitMessageField.php index 7e0224fbbc..69101f5b95 100644 --- a/src/applications/differential/field/DifferentialTasksCommitMessageField.php +++ b/src/applications/differential/field/DifferentialTasksCommitMessageField.php @@ -54,4 +54,8 @@ final class DifferentialTasksCommitMessageField return $this->renderHandleList($value); } + public function getFieldTransactions($value) { + // TODO: Implement this! + return array(); + } } diff --git a/src/applications/differential/field/DifferentialTestPlanCommitMessageField.php b/src/applications/differential/field/DifferentialTestPlanCommitMessageField.php index e915d5932d..a477a9036e 100644 --- a/src/applications/differential/field/DifferentialTestPlanCommitMessageField.php +++ b/src/applications/differential/field/DifferentialTestPlanCommitMessageField.php @@ -41,4 +41,13 @@ final class DifferentialTestPlanCommitMessageField return $revision->getTestPlan(); } + public function getFieldTransactions($value) { + return array( + array( + 'type' => DifferentialRevisionTestPlanTransaction::EDITKEY, + 'value' => $value, + ), + ); + } + } diff --git a/src/applications/differential/field/DifferentialTitleCommitMessageField.php b/src/applications/differential/field/DifferentialTitleCommitMessageField.php index cb481900a5..6b39796f3a 100644 --- a/src/applications/differential/field/DifferentialTitleCommitMessageField.php +++ b/src/applications/differential/field/DifferentialTitleCommitMessageField.php @@ -47,4 +47,13 @@ final class DifferentialTitleCommitMessageField return $value; } + public function getFieldTransactions($value) { + return array( + array( + 'type' => DifferentialRevisionTitleTransaction::EDITKEY, + 'value' => $value, + ), + ); + } + } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php index 6c67f48706..6ecf738162 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php @@ -4,6 +4,7 @@ final class DifferentialRevisionReviewersTransaction extends DifferentialRevisionTransactionType { const TRANSACTIONTYPE = 'differential.revision.reviewers'; + const EDITKEY = 'reviewers'; public function generateOldValue($object) { $reviewers = $object->getReviewerStatus(); @@ -18,6 +19,7 @@ final class DifferentialRevisionReviewersTransaction ->setViewer($actor); $reviewers = $this->generateOldValue($object); + $old_reviewers = $reviewers; // First, remove any reviewers we're getting rid of. $rem = idx($value, '-', array()); @@ -63,7 +65,7 @@ final class DifferentialRevisionReviewersTransaction $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; foreach ($add_map as $phid => $new_status) { - $old_status = idx($reviewers, $phid); + $old_status = idx($old_reviewers, $phid); // If we have an old status and this didn't make the reviewer blocking // or nonblocking, just retain the old status. This makes sure we don't @@ -76,6 +78,7 @@ final class DifferentialRevisionReviewersTransaction $is_unblock = (!$now_blocking && $was_blocking); if (!$is_block && !$is_unblock) { + $reviewers[$phid] = $old_status; continue; } } @@ -86,6 +89,14 @@ final class DifferentialRevisionReviewersTransaction return $reviewers; } + public function getTransactionHasEffect($object, $old, $new) { + // At least for now, we ignore transactions which ONLY reorder reviewers + // without making any actual changes. + ksort($old); + ksort($new); + return ($old !== $new); + } + public function applyExternalEffects($object, $value) { $src_phid = $object->getPHID(); diff --git a/src/applications/differential/xaction/DifferentialRevisionSummaryTransaction.php b/src/applications/differential/xaction/DifferentialRevisionSummaryTransaction.php index 0ad6ba0971..a4f1dcafa5 100644 --- a/src/applications/differential/xaction/DifferentialRevisionSummaryTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionSummaryTransaction.php @@ -4,6 +4,7 @@ final class DifferentialRevisionSummaryTransaction extends DifferentialRevisionTransactionType { const TRANSACTIONTYPE = 'differential.revision.summary'; + const EDITKEY = 'summary'; public function generateOldValue($object) { return $object->getSummary(); diff --git a/src/applications/differential/xaction/DifferentialRevisionTestPlanTransaction.php b/src/applications/differential/xaction/DifferentialRevisionTestPlanTransaction.php index 20aef744e6..bf2beab3d8 100644 --- a/src/applications/differential/xaction/DifferentialRevisionTestPlanTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionTestPlanTransaction.php @@ -4,6 +4,7 @@ final class DifferentialRevisionTestPlanTransaction extends DifferentialRevisionTransactionType { const TRANSACTIONTYPE = 'differential.revision.testplan'; + const EDITKEY = 'testPlan'; public function generateOldValue($object) { return $object->getTestPlan(); diff --git a/src/applications/differential/xaction/DifferentialRevisionTitleTransaction.php b/src/applications/differential/xaction/DifferentialRevisionTitleTransaction.php index d5f529f47b..9b763c53ca 100644 --- a/src/applications/differential/xaction/DifferentialRevisionTitleTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionTitleTransaction.php @@ -4,6 +4,7 @@ final class DifferentialRevisionTitleTransaction extends DifferentialRevisionTransactionType { const TRANSACTIONTYPE = 'differential.revision.title'; + const EDITKEY = 'title'; public function generateOldValue($object) { return $object->getTitle(); diff --git a/src/applications/project/engineextension/PhabricatorProjectsEditEngineExtension.php b/src/applications/project/engineextension/PhabricatorProjectsEditEngineExtension.php index ce9fbf332c..5daacd315c 100644 --- a/src/applications/project/engineextension/PhabricatorProjectsEditEngineExtension.php +++ b/src/applications/project/engineextension/PhabricatorProjectsEditEngineExtension.php @@ -5,6 +5,10 @@ final class PhabricatorProjectsEditEngineExtension const EXTENSIONKEY = 'projects.projects'; + const EDITKEY_ADD = 'projects.add'; + const EDITKEY_SET = 'projects.set'; + const EDITKEY_REMOVE = 'projects.remove'; + public function getExtensionPriority() { return 500; } @@ -58,14 +62,14 @@ final class PhabricatorProjectsEditEngineExtension $projects_field->setViewer($engine->getViewer()); - $edit_add = $projects_field->getConduitEditType('projects.add') + $edit_add = $projects_field->getConduitEditType(self::EDITKEY_ADD) ->setConduitDescription(pht('Add project tags.')); - $edit_set = $projects_field->getConduitEditType('projects.set') + $edit_set = $projects_field->getConduitEditType(self::EDITKEY_SET) ->setConduitDescription( pht('Set project tags, overwriting current value.')); - $edit_rem = $projects_field->getConduitEditType('projects.remove') + $edit_rem = $projects_field->getConduitEditType(self::EDITKEY_REMOVE) ->setConduitDescription(pht('Remove project tags.')); return array( diff --git a/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php b/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php index a0a6cbf7be..ac0d3896cc 100644 --- a/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php +++ b/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsEditEngineExtension.php @@ -5,6 +5,10 @@ final class PhabricatorSubscriptionsEditEngineExtension const EXTENSIONKEY = 'subscriptions.subscribers'; + const EDITKEY_ADD = 'subscribers.add'; + const EDITKEY_SET = 'subscribers.set'; + const EDITKEY_REMOVE = 'subscribers.remove'; + public function getExtensionPriority() { return 750; } @@ -52,14 +56,14 @@ final class PhabricatorSubscriptionsEditEngineExtension $subscribers_field->setViewer($engine->getViewer()); - $edit_add = $subscribers_field->getConduitEditType('subscribers.add') + $edit_add = $subscribers_field->getConduitEditType(self::EDITKEY_ADD) ->setConduitDescription(pht('Add subscribers.')); - $edit_set = $subscribers_field->getConduitEditType('subscribers.set') + $edit_set = $subscribers_field->getConduitEditType(self::EDITKEY_SET) ->setConduitDescription( pht('Set subscribers, overwriting current value.')); - $edit_rem = $subscribers_field->getConduitEditType('subscribers.remove') + $edit_rem = $subscribers_field->getConduitEditType(self::EDITKEY_REMOVE) ->setConduitDescription(pht('Remove subscribers.')); return array( diff --git a/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php b/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php index a0a303bdc7..7eb2aee037 100644 --- a/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php +++ b/src/applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php @@ -4,6 +4,7 @@ final class PhabricatorCommentEditEngineExtension extends PhabricatorEditEngineExtension { const EXTENSIONKEY = 'transactions.comment'; + const EDITKEY = 'comment'; public function getExtensionPriority() { return 9000; @@ -39,7 +40,7 @@ final class PhabricatorCommentEditEngineExtension $comment_type = PhabricatorTransactions::TYPE_COMMENT; $comment_field = id(new PhabricatorCommentEditField()) - ->setKey('comment') + ->setKey(self::EDITKEY) ->setLabel(pht('Comments')) ->setAliases(array('comments')) ->setIsHidden(true) diff --git a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php index 2c5556cb4a..6956561490 100644 --- a/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php +++ b/src/applications/typeahead/datasource/PhabricatorTypeaheadDatasource.php @@ -396,13 +396,16 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject { if (!self::isFunctionToken($token)) { $results[] = $token; } else { - $evaluate[] = $token; + // Put a placeholder in the result list so that we retain token order + // when possible. We'll overwrite this below. + $results[] = null; + $evaluate[last_key($results)] = $token; } } $results = $this->evaluateValues($results); - foreach ($evaluate as $function) { + foreach ($evaluate as $result_key => $function) { $function = self::parseFunction($function); if (!$function) { throw new PhabricatorTypeaheadInvalidTokenException(); @@ -411,11 +414,23 @@ abstract class PhabricatorTypeaheadDatasource extends Phobject { $name = $function['name']; $argv = $function['argv']; - foreach ($this->evaluateFunction($name, array($argv)) as $phid) { - $results[] = $phid; + $evaluated_tokens = $this->evaluateFunction($name, array($argv)); + if (!$evaluated_tokens) { + unset($results[$result_key]); + } else { + $is_first = true; + foreach ($evaluated_tokens as $phid) { + if ($is_first) { + $results[$result_key] = $phid; + $is_first = false; + } else { + $results[] = $phid; + } + } } } + $results = array_values($results); $results = $this->didEvaluateTokens($results); return $results;