From dec14136844d7f2dd7dd1e6b77d7d3b2ed43df98 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 3 Mar 2021 14:48:58 -0800 Subject: [PATCH] Provide a more general "Author" transaction for Differential Summary: Ref T13628. Currently, Differential has a "Commandeer" action, but no way to edit the author otherwise. This is largely archaic: there is no reason to prevent editing the author, and this makes it difficult to undo mistakes if you commandeer by accident. Instead, provide a normal "Author" field and a "Foist Upon" action, similar to the "Owner" and "Claim/Assign" fields in Maniphest. Test Plan: - Applied author transactions as the old author ("foisted"), the new author ("commandeerd"), and an arbitrary third party ("changed author"). - Tried to unassign author, etc. - Viewed stories in feed and transaction timeline. - Saw sensible automatic reviewer changes. - Used existing "Commandeer" action, which is unchanged. - Called "transaction.search" and saw reasonable transaction values. Maniphest Tasks: T13628 Differential Revision: https://secure.phabricator.com/D21591 --- src/__phutil_library_map__.php | 2 + .../editor/DifferentialRevisionEditEngine.php | 17 ++ .../editor/DifferentialTransactionEditor.php | 39 +++-- .../DifferentialRevisionAuthorTransaction.php | 155 ++++++++++++++++++ .../xaction/ManiphestTaskOwnerTransaction.php | 3 +- 5 files changed, 202 insertions(+), 14 deletions(-) create mode 100644 src/applications/differential/xaction/DifferentialRevisionAuthorTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7dee9ba3f0..5a1b71bdf5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -616,6 +616,7 @@ phutil_register_library_map(array( 'DifferentialRevisionAuthorHeraldField' => 'applications/differential/herald/DifferentialRevisionAuthorHeraldField.php', 'DifferentialRevisionAuthorPackagesHeraldField' => 'applications/differential/herald/DifferentialRevisionAuthorPackagesHeraldField.php', 'DifferentialRevisionAuthorProjectsHeraldField' => 'applications/differential/herald/DifferentialRevisionAuthorProjectsHeraldField.php', + 'DifferentialRevisionAuthorTransaction' => 'applications/differential/xaction/DifferentialRevisionAuthorTransaction.php', 'DifferentialRevisionBuildableTransaction' => 'applications/differential/xaction/DifferentialRevisionBuildableTransaction.php', 'DifferentialRevisionCloseDetailsController' => 'applications/differential/controller/DifferentialRevisionCloseDetailsController.php', 'DifferentialRevisionCloseTransaction' => 'applications/differential/xaction/DifferentialRevisionCloseTransaction.php', @@ -6727,6 +6728,7 @@ phutil_register_library_map(array( 'DifferentialRevisionAuthorHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionAuthorPackagesHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionAuthorProjectsHeraldField' => 'DifferentialRevisionHeraldField', + 'DifferentialRevisionAuthorTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionBuildableTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionCloseDetailsController' => 'DifferentialController', 'DifferentialRevisionCloseTransaction' => 'DifferentialRevisionActionTransaction', diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php index cc90e36a61..2da1734f34 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -176,6 +176,23 @@ final class DifferentialRevisionEditEngine ->setConduitTypeDescription(pht('New revision title.')) ->setValue($object->getTitle()); + $author_field = id(new PhabricatorDatasourceEditField()) + ->setKey(DifferentialRevisionAuthorTransaction::EDITKEY) + ->setLabel(pht('Author')) + ->setDatasource(new PhabricatorPeopleDatasource()) + ->setTransactionType( + DifferentialRevisionAuthorTransaction::TRANSACTIONTYPE) + ->setDescription(pht('Foist this revision upon someone else.')) + ->setConduitDescription(pht('Foist this revision upon another user.')) + ->setConduitTypeDescription(pht('New author.')) + ->setSingleValue($object->getAuthorPHID()); + + if ($viewer->getPHID() === $object->getAuthorPHID()) { + $author_field->setCommentActionLabel(pht('Foist Upon')); + } + + $fields[] = $author_field; + $fields[] = id(new PhabricatorRemarkupEditField()) ->setKey(DifferentialRevisionSummaryTransaction::EDITKEY) ->setLabel(pht('Summary')) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 3967344405..964cf38722 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -198,7 +198,7 @@ final class DifferentialTransactionEditor ->setNewValue($want_downgrade); } - $is_commandeer = false; + $new_author_phid = null; switch ($xaction->getTransactionType()) { case DifferentialRevisionUpdateTransaction::TRANSACTIONTYPE: if ($this->getIsCloseByCommit()) { @@ -239,12 +239,22 @@ final class DifferentialTransactionEditor break; case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE: - $is_commandeer = true; + $new_author_phid = $actor_phid; break; + + case DifferentialRevisionAuthorTransaction::TRANSACTIONTYPE: + $new_author_phid = $xaction->getNewValue(); + break; + } - if ($is_commandeer) { - $results[] = $this->newCommandeerReviewerTransaction($object); + if ($new_author_phid) { + $swap_xaction = $this->newSwapReviewersTransaction( + $object, + $new_author_phid); + if ($swap_xaction) { + $results[] = $swap_xaction; + } } if (!$this->didExpandInlineState) { @@ -1472,21 +1482,25 @@ final class DifferentialTransactionEditor return $this; } - private function newCommandeerReviewerTransaction( - DifferentialRevision $revision) { + private function newSwapReviewersTransaction( + DifferentialRevision $revision, + $new_author_phid) { - $actor_phid = $this->getActingAsPHID(); - $owner_phid = $revision->getAuthorPHID(); + $old_author_phid = $revision->getAuthorPHID(); - // If the user is commandeering, add the previous owner as a - // reviewer and remove the actor. + if ($old_author_phid === $new_author_phid) { + return; + } + + // If the revision is changing authorship, add the previous author as a + // reviewer and remove the new author. $edits = array( '-' => array( - $actor_phid, + $new_author_phid, ), '+' => array( - $owner_phid, + $old_author_phid, ), ); @@ -1503,6 +1517,7 @@ final class DifferentialTransactionEditor ->setNewValue($edits); } + public function getActiveDiff($object) { if ($this->getIsNewObject()) { return null; diff --git a/src/applications/differential/xaction/DifferentialRevisionAuthorTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAuthorTransaction.php new file mode 100644 index 0000000000..f86bf8a66c --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionAuthorTransaction.php @@ -0,0 +1,155 @@ +getAuthorPHID(); + } + + public function generateNewValue($object, $value) { + return $value; + } + + public function applyInternalEffects($object, $value) { + $object->setAuthorPHID($value); + } + + public function validateTransactions($object, array $xactions) { + $actor = $this->getActor(); + $errors = array(); + + if (!$xactions) { + return $errors; + } + + foreach ($xactions as $xaction) { + $old = $xaction->generateOldValue($object); + $new = $xaction->getNewValue(); + + if ($old === $new) { + continue; + } + + if (!$new) { + $errors[] = $this->newInvalidError( + pht('Revisions must have an assigned author.'), + $xaction); + continue; + } + + $author_objects = id(new PhabricatorPeopleQuery()) + ->setViewer($actor) + ->withPHIDs(array($new)) + ->execute(); + if (!$author_objects) { + $errors[] = $this->newInvalidError( + pht('Author "%s" is not a valid user.', $new), + $xaction); + continue; + } + } + + return $errors; + } + + public function getIcon() { + $author_phid = $this->getAuthorPHID(); + $old_phid = $this->getOldValue(); + $new_phid = $this->getNewValue(); + + $is_commandeer = ($author_phid === $new_phid); + $is_foist = ($author_phid === $old_phid); + + if ($is_commandeer) { + return 'fa-flag'; + } + + if ($is_foist) { + return 'fa-gift'; + } + + return 'fa-user'; + } + + public function getColor() { + return 'sky'; + } + + public function getTitle() { + $author_phid = $this->getAuthorPHID(); + $old_phid = $this->getOldValue(); + $new_phid = $this->getNewValue(); + + $is_commandeer = ($author_phid === $new_phid); + $is_foist = ($author_phid === $old_phid); + + if ($is_commandeer) { + return pht( + '%s commandeered this revision from %s.', + $this->renderAuthor(), + $this->renderOldHandle()); + } + + if ($is_foist) { + return pht( + '%s foisted this revision upon %s.', + $this->renderAuthor(), + $this->renderNewHandle()); + } + + return pht( + '%s changed the author of this revision from %s to %s.', + $this->renderAuthor(), + $this->renderOldHandle(), + $this->renderNewHandle()); + } + + public function getTitleForFeed() { + $author_phid = $this->getAuthorPHID(); + $old_phid = $this->getOldValue(); + $new_phid = $this->getNewValue(); + + $is_commandeer = ($author_phid === $new_phid); + $is_foist = ($author_phid === $old_phid); + + if ($is_commandeer) { + return pht( + '%s commandeered %s from %s.', + $this->renderAuthor(), + $this->renderObject(), + $this->renderOldHandle()); + } + + if ($is_foist) { + return pht( + '%s foisted %s upon %s.', + $this->renderAuthor(), + $this->renderObject(), + $this->renderNewHandle()); + } + + return pht( + '%s changed the author of %s from %s to %s.', + $this->renderAuthor(), + $this->renderObject(), + $this->renderOldHandle(), + $this->renderNewHandle()); + + } + + public function getTransactionTypeForConduit($xaction) { + return 'author'; + } + + public function getFieldValuesForConduit($object, $data) { + return array( + 'old' => $object->getOldValue(), + 'new' => $object->getNewValue(), + ); + } + +} diff --git a/src/applications/maniphest/xaction/ManiphestTaskOwnerTransaction.php b/src/applications/maniphest/xaction/ManiphestTaskOwnerTransaction.php index 7f58c5dcab..0ebef78976 100644 --- a/src/applications/maniphest/xaction/ManiphestTaskOwnerTransaction.php +++ b/src/applications/maniphest/xaction/ManiphestTaskOwnerTransaction.php @@ -127,8 +127,7 @@ final class ManiphestTaskOwnerTransaction if (!$assignee_list) { $errors[] = $this->newInvalidError( - pht('User "%s" is not a valid user.', - $new)); + pht('User "%s" is not a valid user.', $new)); } } return $errors;