diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php index 5138733c41..ad96a22b88 100644 --- a/src/applications/differential/customfield/DifferentialReviewersField.php +++ b/src/applications/differential/customfield/DifferentialReviewersField.php @@ -7,10 +7,6 @@ final class DifferentialReviewersField return 'differential:reviewers'; } - public function getFieldKeyForConduit() { - return 'reviewerPHIDs'; - } - public function getFieldName() { return pht('Reviewers'); } @@ -24,108 +20,6 @@ final class DifferentialReviewersField return $revision->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) { - $datasource = id(new DifferentialBlockingReviewerDatasource()) - ->setViewer($request->getViewer()); - - $new_phids = $request->getArr($this->getFieldKey()); - $new_phids = $datasource->evaluateTokens($new_phids); - - $reviewers = array(); - foreach ($new_phids as $spec) { - if (!is_array($spec)) { - $reviewers[$spec] = DifferentialReviewerStatus::STATUS_ADDED; - } else { - $reviewers[$spec['phid']] = $spec['type']; - } - } - - $this->updateReviewers($this->getValue(), $reviewers); - } - - private function updateReviewers(array $old_reviewers, array $new_map) { - // Compute a new set of reviewer objects. We're going to respect the new - // reviewer order, add or remove any new or missing reviewers, and respect - // any blocking or unblocking changes. For reviewers who were there before - // and are still there, we're going to keep the old value because it - // may be something like "Accept", "Reject", etc. - - $old_map = mpull($old_reviewers, 'getStatus', 'getReviewerPHID'); - $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; - - $new_reviewers = array(); - foreach ($new_map as $phid => $new) { - $old = idx($old_map, $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 - // throw away rejects, accepts, etc. - if ($old) { - $is_block = ($old !== $status_blocking && $new === $status_blocking); - $is_unblock = ($old === $status_blocking && $new !== $status_blocking); - if (!$is_block && !$is_unblock) { - $new_reviewers[$phid] = $old; - continue; - } - } - - $new_reviewers[$phid] = $new; - } - - foreach ($new_reviewers as $phid => $status) { - $new_reviewers[$phid] = new DifferentialReviewerProxy( - $phid, - array( - 'status' => $status, - )); - } - - $this->setValue($new_reviewers); - } - - public function renderEditControl(array $handles) { - $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; - - $value = array(); - foreach ($this->getValue() as $reviewer) { - $phid = $reviewer->getReviewerPHID(); - if ($reviewer->getStatus() == $status_blocking) { - $value[] = 'blocking('.$phid.')'; - } else { - $value[] = $phid; - } - } - - return id(new AphrontFormTokenizerControl()) - ->setUser($this->getViewer()) - ->setName($this->getFieldKey()) - ->setDatasource(new DifferentialReviewerDatasource()) - ->setValue($value) - ->setError($this->getFieldError()) - ->setLabel($this->getFieldName()); - } - - public function getApplicationTransactionType() { - return PhabricatorTransactions::TYPE_EDGE; - } - - public function getApplicationTransactionMetadata() { - return array( - 'edge:type' => DifferentialRevisionHasReviewerEdgeType::EDGECONST, - ); - } - public function shouldAppearInPropertyView() { return true; } @@ -164,99 +58,6 @@ final class DifferentialReviewersField return $reviewers; } - public function shouldAppearInCommitMessage() { - return true; - } - - public function shouldAppearInCommitMessageTemplate() { - return true; - } - - public function getCommitMessageLabels() { - return array( - 'Reviewer', - 'Reviewers', - ); - } - - public function parseValueFromCommitMessage($value) { - $results = $this->parseObjectList( - $value, - array( - PhabricatorPeopleUserPHIDType::TYPECONST, - PhabricatorProjectProjectPHIDType::TYPECONST, - PhabricatorOwnersPackagePHIDType::TYPECONST, - ), - false, - array('!')); - - return $this->flattenReviewers($results); - } - - public function getRequiredHandlePHIDsForCommitMessage() { - return mpull($this->getValue(), 'getReviewerPHID'); - } - - public function readValueFromCommitMessage($value) { - $value = $this->inflateReviewers($value); - - $reviewers = array(); - foreach ($value as $spec) { - $phid = $spec['phid']; - - $is_blocking = isset($spec['suffixes']['!']); - if ($is_blocking) { - $status = DifferentialReviewerStatus::STATUS_BLOCKING; - } else { - $status = DifferentialReviewerStatus::STATUS_ADDED; - } - - $reviewers[$phid] = $status; - } - - $this->updateReviewers( - $this->getObject()->getReviewerStatus(), - $reviewers); - - return $this; - } - - public function renderCommitMessageValue(array $handles) { - $suffixes = array(); - - $status_blocking = DifferentialReviewerStatus::STATUS_BLOCKING; - - foreach ($this->getValue() as $reviewer) { - if ($reviewer->getStatus() == $status_blocking) { - $phid = $reviewer->getReviewerPHID(); - $suffixes[$phid] = '!'; - } - } - - return $this->renderObjectList($handles, $suffixes); - } - - public function validateCommitMessageValue($value) { - if (!$value) { - return; - } - - $author_phid = $this->getObject()->getAuthorPHID(); - - $config_self_accept_key = 'differential.allow-self-accept'; - $allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key); - - $value = $this->inflateReviewers($value); - foreach ($value as $spec) { - $phid = $spec['phid']; - - if (($phid == $author_phid) && !$allow_self_accept) { - throw new DifferentialFieldValidationException( - pht('The author of a revision can not be a reviewer.')); - } - } - } - public function getRequiredHandlePHIDsForRevisionHeaderWarnings() { return mpull($this->getValue(), 'getReviewerPHID'); } @@ -288,44 +89,4 @@ final class DifferentialReviewersField return $warnings; } - public function getProTips() { - return array( - pht( - 'You can mark a reviewer as blocking by adding an exclamation '. - 'mark ("!") after their name.'), - ); - } - - 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. - $result = array(); - - foreach ($values as $value) { - $result[] = $value['phid'].implode('', array_keys($value['suffixes'])); - } - - return $result; - } - - private function inflateReviewers(array $values) { - $result = array(); - - foreach ($values as $value) { - if (substr($value, -1) == '!') { - $value = substr($value, 0, -1); - $suffixes = array('!' => '!'); - } else { - $suffixes = array(); - } - - $result[] = array( - 'phid' => $value, - 'suffixes' => $suffixes, - ); - } - - return $result; - } - } diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index fdf4121e1f..0bdb0d2151 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -27,6 +27,9 @@ final class DifferentialTransaction // without doing a migration. At some point, we should do a migration and // throw this away. + // NOTE: Old reviewer edits are raw edge transactions. They could be + // migrated to modular transactions when the rest of this migrates. + $xaction_type = $this->getTransactionType(); if ($xaction_type == PhabricatorTransactions::TYPE_CUSTOMFIELD) { switch ($this->getMetadataValue('customfield:key')) { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index cc583d34c2..8157166bb4 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -960,9 +960,18 @@ abstract class PhabricatorApplicationTransaction if ($field) { return $field->getApplicationTransactionTitle($this); } else { - return pht( - '%s edited a custom field.', - $this->renderHandleLink($author_phid)); + $developer_mode = 'phabricator.developer-mode'; + $is_developer = PhabricatorEnv::getEnvConfig($developer_mode); + if ($is_developer) { + return pht( + '%s edited a custom field (with key "%s").', + $this->renderHandleLink($author_phid), + $this->getMetadata('customfield:key')); + } else { + return pht( + '%s edited a custom field.', + $this->renderHandleLink($author_phid)); + } } case PhabricatorTransactions::TYPE_TOKEN: