mirror of
https://we.phorge.it/source/phorge.git
synced 2024-11-22 23:02:42 +01:00
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
This commit is contained in:
parent
3d35d6d3f9
commit
8913552970
4 changed files with 60 additions and 11 deletions
|
@ -29,6 +29,14 @@ final class DifferentialRevisionRequiredActionResultBucket
|
|||
}
|
||||
$phids = array_fuse($phids);
|
||||
|
||||
// Before continuing, throw away any revisions which responsible users
|
||||
// have explicitly resigned from.
|
||||
|
||||
// The goal is to allow users to resign from revisions they don't want to
|
||||
// review to get these revisions off their dashboard, even if there are
|
||||
// other project or package reviewers which they have authority over.
|
||||
$this->filterResigned($phids);
|
||||
|
||||
$groups = array();
|
||||
|
||||
$groups[] = $this->newGroup()
|
||||
|
@ -229,4 +237,25 @@ final class DifferentialRevisionRequiredActionResultBucket
|
|||
return $results;
|
||||
}
|
||||
|
||||
private function filterResigned(array $phids) {
|
||||
$resigned = array(
|
||||
DifferentialReviewerStatus::STATUS_RESIGNED,
|
||||
);
|
||||
$resigned = array_fuse($resigned);
|
||||
|
||||
$objects = $this->getRevisionsNotAuthored($this->objects, $phids);
|
||||
|
||||
$results = array();
|
||||
foreach ($objects as $key => $object) {
|
||||
if (!$this->hasReviewersWithStatus($object, $phids, $resigned)) {
|
||||
continue;
|
||||
}
|
||||
|
||||
$results[$key] = $object;
|
||||
unset($this->objects[$key]);
|
||||
}
|
||||
|
||||
return $results;
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -42,4 +42,9 @@ final class DifferentialReviewer
|
|||
return $this->assertAttachedKey($this->authority, $cache_fragment);
|
||||
}
|
||||
|
||||
public function isResigned() {
|
||||
$status_resigned = DifferentialReviewerStatus::STATUS_RESIGNED;
|
||||
return ($this->getReviewerStatus() == $status_resigned);
|
||||
}
|
||||
|
||||
}
|
||||
|
|
|
@ -25,9 +25,23 @@ final class DifferentialReviewersView extends AphrontView {
|
|||
|
||||
public function render() {
|
||||
$viewer = $this->getUser();
|
||||
$reviewers = $this->reviewers;
|
||||
|
||||
$view = new PHUIStatusListView();
|
||||
foreach ($this->reviewers as $reviewer) {
|
||||
|
||||
// Move resigned reviewers to the bottom.
|
||||
$head = array();
|
||||
$tail = array();
|
||||
foreach ($reviewers as $key => $reviewer) {
|
||||
if ($reviewer->isResigned()) {
|
||||
$tail[$key] = $reviewer;
|
||||
} else {
|
||||
$head[$key] = $reviewer;
|
||||
}
|
||||
}
|
||||
|
||||
$reviewers = $head + $tail;
|
||||
foreach ($reviewers as $reviewer) {
|
||||
$phid = $reviewer->getReviewerPHID();
|
||||
$handle = $this->handles[$phid];
|
||||
|
||||
|
@ -98,6 +112,13 @@ final class DifferentialReviewersView extends AphrontView {
|
|||
pht('Blocking Review'));
|
||||
break;
|
||||
|
||||
case DifferentialReviewerStatus::STATUS_RESIGNED:
|
||||
$item->setIcon(
|
||||
'fa-times',
|
||||
'grey',
|
||||
pht('Resigned'));
|
||||
break;
|
||||
|
||||
default:
|
||||
$item->setIcon(
|
||||
PHUIStatusItemView::ICON_QUESTION,
|
||||
|
|
|
@ -178,16 +178,10 @@ abstract class DifferentialRevisionReviewTransaction
|
|||
$reviewer->setLastActionDiffPHID($diff_phid);
|
||||
}
|
||||
|
||||
if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) {
|
||||
if ($reviewer->getID()) {
|
||||
$reviewer->delete();
|
||||
}
|
||||
} else {
|
||||
try {
|
||||
$reviewer->save();
|
||||
} catch (AphrontDuplicateKeyQueryException $ex) {
|
||||
// At least for now, just ignore it if we lost a race.
|
||||
}
|
||||
try {
|
||||
$reviewer->save();
|
||||
} catch (AphrontDuplicateKeyQueryException $ex) {
|
||||
// At least for now, just ignore it if we lost a race.
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Reference in a new issue