2017-03-13 19:31:57 +01:00
|
|
|
<?php
|
|
|
|
|
|
|
|
final class DifferentialReviewer
|
|
|
|
extends DifferentialDAO {
|
|
|
|
|
|
|
|
protected $revisionPHID;
|
|
|
|
protected $reviewerPHID;
|
|
|
|
protected $reviewerStatus;
|
2017-03-20 17:54:48 +01:00
|
|
|
protected $lastActionDiffPHID;
|
|
|
|
protected $lastCommentDiffPHID;
|
2017-03-22 19:42:52 +01:00
|
|
|
protected $lastActorPHID;
|
2017-03-28 14:23:42 +02:00
|
|
|
protected $voidedPHID;
|
2017-03-20 17:54:48 +01:00
|
|
|
|
2017-03-20 20:35:30 +01:00
|
|
|
private $authority = array();
|
|
|
|
|
2017-03-13 19:31:57 +01:00
|
|
|
protected function getConfiguration() {
|
|
|
|
return array(
|
|
|
|
self::CONFIG_COLUMN_SCHEMA => array(
|
|
|
|
'reviewerStatus' => 'text64',
|
2017-03-20 17:54:48 +01:00
|
|
|
'lastActionDiffPHID' => 'phid?',
|
|
|
|
'lastCommentDiffPHID' => 'phid?',
|
2017-03-22 19:42:52 +01:00
|
|
|
'lastActorPHID' => 'phid?',
|
2017-03-28 14:23:42 +02:00
|
|
|
'voidedPHID' => 'phid?',
|
2017-03-13 19:31:57 +01:00
|
|
|
),
|
|
|
|
self::CONFIG_KEY_SCHEMA => array(
|
|
|
|
'key_revision' => array(
|
|
|
|
'columns' => array('revisionPHID', 'reviewerPHID'),
|
|
|
|
'unique' => true,
|
|
|
|
),
|
2017-03-22 14:50:51 +01:00
|
|
|
'key_reviewer' => array(
|
|
|
|
'columns' => array('reviewerPHID', 'revisionPHID'),
|
|
|
|
),
|
2017-03-13 19:31:57 +01:00
|
|
|
),
|
|
|
|
) + parent::getConfiguration();
|
|
|
|
}
|
|
|
|
|
2017-03-20 20:35:30 +01:00
|
|
|
public function isUser() {
|
|
|
|
$user_type = PhabricatorPeopleUserPHIDType::TYPECONST;
|
|
|
|
return (phid_get_type($this->getReviewerPHID()) == $user_type);
|
|
|
|
}
|
|
|
|
|
|
|
|
public function attachAuthority(PhabricatorUser $user, $has_authority) {
|
|
|
|
$this->authority[$user->getCacheFragment()] = $has_authority;
|
|
|
|
return $this;
|
|
|
|
}
|
|
|
|
|
|
|
|
public function hasAuthority(PhabricatorUser $viewer) {
|
|
|
|
$cache_fragment = $viewer->getCacheFragment();
|
|
|
|
return $this->assertAttachedKey($this->authority, $cache_fragment);
|
|
|
|
}
|
|
|
|
|
Store "resigned" as an explicit reviewer state
Summary:
Fixes T11050. Today, when a user resigns, we just delete the record of them ever being a reviewer.
However, this means you have no way to say "I don't care about this and don't want to see it on my dashboard" if you are a member of any project or package reviewers.
Instead, store "resigned" as a distinct state from "not a reviewer", and treat it a little differently in the UI:
- On the bucketing screen, discard revisions any responsible user has resigned from.
- On the main `/Dxxx` page, show these users as resigned explicitly (we could just hide them, too, but I think this is good to start with).
- In the query, don't treat a "resigned" state as a real "reviewer" (this change happened earlier, in D17517).
- When resigning, write a "resigned" state instead of deleting the row.
- When editing a list of reviewers, I'm still treating this reviewer as a reviewer and not special casing it. I think that's sufficiently clear but we could tailor this behavior later.
Test Plan:
- Resigned from a revision.
- Saw "Resigned" in reviewers list.
- Saw revision disappear from my dashboard.
- Edited revision, saw user still appear as an editable reviewer. Saved revision, saw no weird side effects.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11050
Differential Revision: https://secure.phabricator.com/D17531
2017-03-22 14:38:35 +01:00
|
|
|
public function isResigned() {
|
|
|
|
$status_resigned = DifferentialReviewerStatus::STATUS_RESIGNED;
|
|
|
|
return ($this->getReviewerStatus() == $status_resigned);
|
|
|
|
}
|
|
|
|
|
2017-03-28 15:22:19 +02:00
|
|
|
public function isRejected($diff_phid) {
|
|
|
|
$status_rejected = DifferentialReviewerStatus::STATUS_REJECTED;
|
|
|
|
|
|
|
|
if ($this->getReviewerStatus() != $status_rejected) {
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($this->getVoidedPHID()) {
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($this->isCurrentAction($diff_phid)) {
|
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
|
|
|
|
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 17:28:36 +01:00
|
|
|
public function isAccepted($diff_phid) {
|
|
|
|
$status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED;
|
|
|
|
|
|
|
|
if ($this->getReviewerStatus() != $status_accepted) {
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
2017-03-28 14:23:42 +02:00
|
|
|
// If this accept has been voided (for example, but a reviewer using
|
|
|
|
// "Request Review"), don't count it as a real "Accept" even if it is
|
|
|
|
// against the current diff PHID.
|
|
|
|
if ($this->getVoidedPHID()) {
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
2017-03-28 15:22:19 +02:00
|
|
|
if ($this->isCurrentAction($diff_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 17:28:36 +01:00
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
2017-03-28 15:22:19 +02:00
|
|
|
$sticky_key = 'differential.sticky-accept';
|
|
|
|
$is_sticky = PhabricatorEnv::getEnvConfig($sticky_key);
|
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 17:28:36 +01:00
|
|
|
|
2017-03-28 15:22:19 +02:00
|
|
|
if ($is_sticky) {
|
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 17:28:36 +01:00
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
2017-03-28 15:22:19 +02:00
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
|
|
|
private function isCurrentAction($diff_phid) {
|
|
|
|
if (!$diff_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 17:28:36 +01:00
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
2017-03-28 15:22:19 +02:00
|
|
|
$action_phid = $this->getLastActionDiffPHID();
|
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 17:28:36 +01:00
|
|
|
|
2017-03-28 15:22:19 +02:00
|
|
|
if (!$action_phid) {
|
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
|
|
|
if ($action_phid == $diff_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 17:28:36 +01:00
|
|
|
return true;
|
|
|
|
}
|
|
|
|
|
|
|
|
return false;
|
|
|
|
}
|
|
|
|
|
2017-03-13 19:31:57 +01:00
|
|
|
}
|