mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-26 00:32:42 +01:00
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
This commit is contained in:
parent
c0ac5be8a8
commit
dec1413684
5 changed files with 202 additions and 14 deletions
|
@ -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',
|
||||
|
|
|
@ -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'))
|
||||
|
|
|
@ -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;
|
||||
|
|
|
@ -0,0 +1,155 @@
|
|||
<?php
|
||||
|
||||
final class DifferentialRevisionAuthorTransaction
|
||||
extends DifferentialRevisionTransactionType {
|
||||
|
||||
const TRANSACTIONTYPE = 'differential.revision.author';
|
||||
const EDITKEY = 'author';
|
||||
|
||||
public function generateOldValue($object) {
|
||||
return $object->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(),
|
||||
);
|
||||
}
|
||||
|
||||
}
|
|
@ -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;
|
||||
|
|
Loading…
Reference in a new issue