2016-12-29 10:35:47 -08:00
|
|
|
<?php
|
|
|
|
|
|
|
|
final class DifferentialRevisionAcceptTransaction
|
2016-12-29 13:14:09 -08:00
|
|
|
extends DifferentialRevisionReviewTransaction {
|
2016-12-29 10:35:47 -08:00
|
|
|
|
|
|
|
const TRANSACTIONTYPE = 'differential.revision.accept';
|
|
|
|
const ACTIONKEY = 'accept';
|
|
|
|
|
|
|
|
protected function getRevisionActionLabel() {
|
|
|
|
return pht("Accept Revision \xE2\x9C\x94");
|
|
|
|
}
|
|
|
|
|
|
|
|
protected function getRevisionActionDescription() {
|
|
|
|
return pht('These changes will be approved.');
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getIcon() {
|
|
|
|
return 'fa-check-circle-o';
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getColor() {
|
|
|
|
return 'green';
|
|
|
|
}
|
|
|
|
|
Order actions sensibly within Differential revision comment action groups
Summary:
Ref T11114. See D17114 for some discussion.
For review actions: accept, reject, resign.
For revision actions, order is basically least-severe to most-severe action pairs: plan changes, request review, close, reopen, abandon, reclaim, commandeer.
Test Plan: Viewed revisions as an author and a reviewer, saw sensible action order within action groups.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17115
2016-12-29 13:36:36 -08:00
|
|
|
protected function getRevisionActionOrder() {
|
|
|
|
return 500;
|
|
|
|
}
|
|
|
|
|
2017-01-12 07:40:48 -08:00
|
|
|
public function getActionName() {
|
|
|
|
return pht('Accepted');
|
|
|
|
}
|
|
|
|
|
2017-01-02 11:36:02 -08:00
|
|
|
public function getCommandKeyword() {
|
|
|
|
$accept_key = 'differential.enable-email-accept';
|
|
|
|
$allow_email_accept = PhabricatorEnv::getEnvConfig($accept_key);
|
|
|
|
if (!$allow_email_accept) {
|
|
|
|
return null;
|
|
|
|
}
|
|
|
|
|
|
|
|
return 'accept';
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getCommandAliases() {
|
|
|
|
return array();
|
|
|
|
}
|
|
|
|
|
|
|
|
public function getCommandSummary() {
|
|
|
|
return pht('Accept a revision.');
|
|
|
|
}
|
|
|
|
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
protected function getActionOptions(
|
|
|
|
PhabricatorUser $viewer,
|
|
|
|
DifferentialRevision $revision) {
|
|
|
|
|
|
|
|
$reviewers = $revision->getReviewers();
|
|
|
|
|
|
|
|
$options = array();
|
|
|
|
$value = array();
|
|
|
|
|
|
|
|
// Put the viewer's user reviewer first, if it exists, so that "Accept as
|
|
|
|
// yourself" is always at the top.
|
|
|
|
$head = array();
|
|
|
|
$tail = array();
|
|
|
|
foreach ($reviewers as $key => $reviewer) {
|
|
|
|
if ($reviewer->isUser()) {
|
|
|
|
$head[$key] = $reviewer;
|
|
|
|
} else {
|
|
|
|
$tail[$key] = $reviewer;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
$reviewers = $head + $tail;
|
|
|
|
|
|
|
|
$diff_phid = $this->getActiveDiffPHID($revision);
|
|
|
|
$reviewer_phids = array();
|
|
|
|
|
|
|
|
// If the viewer isn't a reviewer, add them to the list of options first.
|
|
|
|
// This happens when you navigate to some revision you aren't involved in:
|
|
|
|
// you can accept and become a reviewer.
|
|
|
|
|
|
|
|
$viewer_phid = $viewer->getPHID();
|
|
|
|
if ($viewer_phid) {
|
|
|
|
if (!isset($reviewers[$viewer_phid])) {
|
|
|
|
$reviewer_phids[$viewer_phid] = $viewer_phid;
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2017-03-28 09:30:58 -07:00
|
|
|
$default_unchecked = array();
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
foreach ($reviewers as $reviewer) {
|
2017-03-28 09:30:58 -07:00
|
|
|
$reviewer_phid = $reviewer->getReviewerPHID();
|
|
|
|
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
if (!$reviewer->hasAuthority($viewer)) {
|
|
|
|
// If the viewer doesn't have authority to act on behalf of a reviewer,
|
2017-03-28 09:30:58 -07:00
|
|
|
// we check if they can accept by force.
|
|
|
|
if ($revision->canReviewerForceAccept($viewer, $reviewer)) {
|
|
|
|
$default_unchecked[$reviewer_phid] = true;
|
|
|
|
} else {
|
|
|
|
continue;
|
|
|
|
}
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
}
|
|
|
|
|
|
|
|
if ($reviewer->isAccepted($diff_phid)) {
|
|
|
|
// If a reviewer is already in a full "accepted" state, don't
|
|
|
|
// include that reviewer as an option.
|
|
|
|
continue;
|
|
|
|
}
|
|
|
|
|
|
|
|
$reviewer_phids[$reviewer_phid] = $reviewer_phid;
|
|
|
|
}
|
|
|
|
|
|
|
|
$handles = $viewer->loadHandles($reviewer_phids);
|
|
|
|
|
2017-03-28 09:30:58 -07:00
|
|
|
$head = array();
|
|
|
|
$tail = array();
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
foreach ($reviewer_phids as $reviewer_phid) {
|
2017-03-28 09:30:58 -07:00
|
|
|
$is_force = isset($default_unchecked[$reviewer_phid]);
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
|
2017-03-28 09:30:58 -07:00
|
|
|
if ($is_force) {
|
|
|
|
$tail[] = $reviewer_phid;
|
|
|
|
|
|
|
|
$options[$reviewer_phid] = pht(
|
|
|
|
'Force accept as %s',
|
|
|
|
$viewer->renderHandle($reviewer_phid));
|
|
|
|
} else {
|
|
|
|
$head[] = $reviewer_phid;
|
|
|
|
$value[] = $reviewer_phid;
|
|
|
|
|
|
|
|
$options[$reviewer_phid] = pht(
|
|
|
|
'Accept as %s',
|
|
|
|
$viewer->renderHandle($reviewer_phid));
|
|
|
|
}
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
}
|
|
|
|
|
2017-03-28 09:30:58 -07:00
|
|
|
// Reorder reviewers so "force accept" reviewers come at the end.
|
|
|
|
$options =
|
|
|
|
array_select_keys($options, $head) +
|
|
|
|
array_select_keys($options, $tail);
|
|
|
|
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
return array($options, $value);
|
|
|
|
}
|
|
|
|
|
2016-12-29 10:35:47 -08:00
|
|
|
public function generateOldValue($object) {
|
|
|
|
$actor = $this->getActor();
|
2017-01-18 12:56:49 -08:00
|
|
|
return $this->isViewerFullyAccepted($object, $actor);
|
2016-12-29 10:35:47 -08:00
|
|
|
}
|
|
|
|
|
|
|
|
public function applyExternalEffects($object, $value) {
|
|
|
|
$status = DifferentialReviewerStatus::STATUS_ACCEPTED;
|
|
|
|
$actor = $this->getActor();
|
|
|
|
$this->applyReviewerEffect($object, $actor, $value, $status);
|
|
|
|
}
|
|
|
|
|
|
|
|
protected function validateAction($object, PhabricatorUser $viewer) {
|
|
|
|
if ($object->isClosed()) {
|
|
|
|
throw new Exception(
|
|
|
|
pht(
|
|
|
|
'You can not accept this revision because it has already been '.
|
|
|
|
'closed. Only open revisions can be accepted.'));
|
|
|
|
}
|
|
|
|
|
|
|
|
$config_key = 'differential.allow-self-accept';
|
|
|
|
if (!PhabricatorEnv::getEnvConfig($config_key)) {
|
|
|
|
if ($this->isViewerRevisionAuthor($object, $viewer)) {
|
|
|
|
throw new Exception(
|
|
|
|
pht(
|
|
|
|
'You can not accept this revision because you are the revision '.
|
|
|
|
'author. You can only accept revisions you do not own. You can '.
|
|
|
|
'change this behavior by adjusting the "%s" setting in Config.',
|
|
|
|
$config_key));
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2017-01-18 12:56:49 -08:00
|
|
|
if ($this->isViewerFullyAccepted($object, $viewer)) {
|
2016-12-29 10:35:47 -08:00
|
|
|
throw new Exception(
|
|
|
|
pht(
|
|
|
|
'You can not accept this revision because you have already '.
|
|
|
|
'accepted it.'));
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
protected function validateOptionValue($object, $actor, array $value) {
|
|
|
|
if (!$value) {
|
|
|
|
throw new Exception(
|
|
|
|
pht(
|
|
|
|
'When accepting a revision, you must accept on behalf of at '.
|
|
|
|
'least one reviewer.'));
|
|
|
|
}
|
|
|
|
|
|
|
|
list($options) = $this->getActionOptions($actor, $object);
|
|
|
|
foreach ($value as $phid) {
|
|
|
|
if (!isset($options[$phid])) {
|
|
|
|
throw new Exception(
|
|
|
|
pht(
|
|
|
|
'Reviewer "%s" is not a valid reviewer which you have authority '.
|
|
|
|
'to accept on behalf of.',
|
|
|
|
$phid));
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2016-12-29 10:35:47 -08:00
|
|
|
public function getTitle() {
|
When accepting revisions, allow users to accept on behalf of a subset of reviewers
Summary:
Ref T12271. Currenty, when you "Accept" a revision, you always accept it for all reviewers you have authority over.
There are some situations where communication can be more clear if users can accept as only themselves, or for only some packages, etc. T12271 discusses some of these use cases in more depth.
Instead of making "Accept" a blanket action, default it to doing what it does now but let the user uncheck reviewers.
In cases where project/package reviewers aren't in use, this doesn't change anything.
For now, "reject" still acts the old way (reject everything). We could make that use checkboxes too, but I'm not sure there's as much of a use case for it, and I generally want users who are blocking stuff to have more direct accountability in a product sense.
Test Plan:
- Accepted normally.
- Accepted a subset.
- Tried to accept none.
- Tried to accept bogus reviewers.
- Accepted with myself not a reviewer
- Accepted with only one reviewer (just got normal "this will be accepted" text).
{F4251255}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12271
Differential Revision: https://secure.phabricator.com/D17533
2017-03-22 09:28:36 -07:00
|
|
|
$new = $this->getNewValue();
|
|
|
|
if (is_array($new) && $new) {
|
|
|
|
return pht(
|
|
|
|
'%s accepted this revision as %s reviewer(s): %s.',
|
|
|
|
$this->renderAuthor(),
|
|
|
|
phutil_count($new),
|
|
|
|
$this->renderHandleList($new));
|
|
|
|
} else {
|
|
|
|
return pht(
|
|
|
|
'%s accepted this revision.',
|
|
|
|
$this->renderAuthor());
|
|
|
|
}
|
2016-12-29 10:35:47 -08:00
|
|
|
}
|
|
|
|
|
|
|
|
public function getTitleForFeed() {
|
|
|
|
return pht(
|
|
|
|
'%s accepted %s.',
|
|
|
|
$this->renderAuthor(),
|
|
|
|
$this->renderObject());
|
|
|
|
}
|
|
|
|
|
|
|
|
}
|