From c457d23a1d30f81ff96f1fa0c0c90550a1d49b53 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 24 Nov 2018 06:41:34 -0800 Subject: [PATCH] Tailor the "no reviewers on this revision" warnings to handle the case where all reviewers have resigned Summary: Ref T13216. See PHI985. We currently use a banner to warn you when a revision has no reviewers or only disabled users, but since the changes to track "Resign" more explicilty we'll no longer warn you if everyone has resigned. (Previously, they'd no longer be reviewers, so you'd end up with the "no reviewers are assigned" warning if everyone resigned.) This can still interact slightly oddly with some states (e.g., only a package or project reviewer) but I'd like to wait for T731 to tighten those cases up, and they're more advanced/unusual. Test Plan: {F6026832} {F6026833} {F6026834} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13216 Differential Revision: https://secure.phabricator.com/D19834 --- .../DifferentialReviewersField.php | 25 +++++++++++++++---- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php index f835854e2f..31b175b09d 100644 --- a/src/applications/differential/customfield/DifferentialReviewersField.php +++ b/src/applications/differential/customfield/DifferentialReviewersField.php @@ -72,20 +72,35 @@ final class DifferentialReviewersField return array(); } + $all_resigned = true; + $all_disabled = true; + $any_reviewers = false; + foreach ($this->getValue() as $reviewer) { - if (!$handles[$reviewer->getReviewerPHID()]->isDisabled()) { - return array(); + $reviewer_phid = $reviewer->getReviewerPHID(); + + $any_reviewers = true; + + if (!$handles[$reviewer_phid]->isDisabled()) { + $all_disabled = false; + } + + if (!$reviewer->isResigned()) { + $all_resigned = false; } } $warnings = array(); - if ($this->getValue()) { + if (!$any_reviewers) { + $warnings[] = pht( + 'This revision needs review, but there are no reviewers specified.'); + } else if ($all_disabled) { $warnings[] = pht( 'This revision needs review, but all specified reviewers are '. 'disabled or inactive.'); - } else { + } else if ($all_resigned) { $warnings[] = pht( - 'This revision needs review, but there are no reviewers specified.'); + 'This revision needs review, but all reviewers have resigned.'); } return $warnings;