Add a "Needs Verification" state to Audit

Summary:
Fixes T2393. This allows authors to explicitly say "I think I fixed everything, please accept my commit now thank you".

Also improves behavior of "re-accept" and "re-reject" after new auditors you have authority over get added.

Test Plan:
  - Kicked a commit back and forth between an author and auditor by alternately using "Request Verification" and "Raise Concern".
  - Verified it showed up properly in bucketing for both users.
  - Accepted, added a project, accepted again (works now; didn't before).
  - Audited on behalf of projects / packages.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2393

Differential Revision: https://secure.phabricator.com/D17252

src/__phutil_library_map__.php

@@ -674,6 +674,7 @@ phutil_register_library_map(array(
     'DiffusionCommitStateTransaction' => 'applications/diffusion/xaction/DiffusionCommitStateTransaction.php',
     'DiffusionCommitTagsController' => 'applications/diffusion/controller/DiffusionCommitTagsController.php',
     'DiffusionCommitTransactionType' => 'applications/diffusion/xaction/DiffusionCommitTransactionType.php',
+    'DiffusionCommitVerifyTransaction' => 'applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php',
     'DiffusionCompareController' => 'applications/diffusion/controller/DiffusionCompareController.php',
     'DiffusionConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionConduitAPIMethod.php',
     'DiffusionController' => 'applications/diffusion/controller/DiffusionController.php',
@@ -5382,6 +5383,7 @@ phutil_register_library_map(array(
     'DiffusionCommitStateTransaction' => 'DiffusionCommitTransactionType',
     'DiffusionCommitTagsController' => 'DiffusionController',
     'DiffusionCommitTransactionType' => 'PhabricatorModularTransactionType',
+    'DiffusionCommitVerifyTransaction' => 'DiffusionCommitAuditTransaction',
     'DiffusionCompareController' => 'DiffusionController',
     'DiffusionConduitAPIMethod' => 'ConduitAPIMethod',
     'DiffusionController' => 'PhabricatorController',

src/applications/audit/constants/PhabricatorAuditCommitStatusConstants.php

@@ -7,12 +7,14 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
   const CONCERN_RAISED      = 2;
   const PARTIALLY_AUDITED   = 3;
   const FULLY_AUDITED       = 4;
+  const NEEDS_VERIFICATION = 5;
 
   public static function getStatusNameMap() {
     $map = array(
       self::NONE                => pht('No Audits'),
       self::NEEDS_AUDIT         => pht('Audit Required'),
       self::CONCERN_RAISED      => pht('Concern Raised'),
+      self::NEEDS_VERIFICATION => pht('Needs Verification'),
       self::PARTIALLY_AUDITED   => pht('Partially Audited'),
       self::FULLY_AUDITED       => pht('Audited'),
     );
@@ -28,6 +30,7 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
     return array(
       self::CONCERN_RAISED,
       self::NEEDS_AUDIT,
+      self::NEEDS_VERIFICATION,
       self::PARTIALLY_AUDITED,
     );
   }
@@ -49,6 +52,9 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
       case self::NONE:
         $color = 'bluegrey';
         break;
+      case self::NEEDS_VERIFICATION:
+        $color = 'indigo';
+        break;
       default:
         $color = null;
         break;
@@ -56,7 +62,7 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
     return $color;
   }
 
-    public static function getStatusIcon($code) {
+  public static function getStatusIcon($code) {
     switch ($code) {
       case self::CONCERN_RAISED:
         $icon = 'fa-times-circle';
@@ -73,6 +79,9 @@ final class PhabricatorAuditCommitStatusConstants extends Phobject {
       case self::NONE:
         $icon = 'fa-check';
         break;
+      case self::NEEDS_VERIFICATION:
+        $icon = 'fa-refresh';
+        break;
       default:
         $icon = null;
         break;

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -11,6 +11,7 @@ final class PhabricatorAuditEditor
   private $auditorPHIDs = array();
 
   private $didExpandInlineState = false;
+  private $oldAuditStatus = null;
 
   public function addAuditReason($phid, $reason) {
     if (!isset($this->auditReasonMap[$phid])) {
@@ -78,6 +79,8 @@ final class PhabricatorAuditEditor
       }
     }
 
+    $this->oldAuditStatus = $object->getAuditStatus();
+
     $object->loadAndAttachAuditAuthority(
       $this->getActor(),
       $this->getActingAsPHID());
@@ -269,7 +272,7 @@ final class PhabricatorAuditEditor
       }
     }
 
-    $old_status = $object->getAuditStatus();
+    $old_status = $this->oldAuditStatus;
 
     $requests = $object->getAudits();
     $object->updateAuditStatus($requests);

src/applications/diffusion/query/DiffusionCommitRequiredActionResultBucket.php

@@ -33,6 +33,11 @@ final class DiffusionCommitRequiredActionResultBucket
       ->setNoDataString(pht('None of your commits have active concerns.'))
       ->setObjects($this->filterConcernRaised($phids));
 
+    $groups[] = $this->newGroup()
+      ->setName(pht('Needs Verification'))
+      ->setNoDataString(pht('No commits are awaiting your verification.'))
+      ->setObjects($this->filterNeedsVerification($phids));
+
     $groups[] = $this->newGroup()
       ->setName(pht('Ready to Audit'))
       ->setNoDataString(pht('No commits are waiting for you to audit them.'))
@@ -82,6 +87,36 @@ final class DiffusionCommitRequiredActionResultBucket
     return $results;
   }
 
+  private function filterNeedsVerification(array $phids) {
+    $results = array();
+    $objects = $this->objects;
+
+    $status_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION;
+    $has_concern = array(
+      PhabricatorAuditStatusConstants::CONCERNED,
+    );
+    $has_concern = array_fuse($has_concern);
+
+    foreach ($objects as $key => $object) {
+      if (isset($phids[$object->getAuthorPHID()])) {
+        continue;
+      }
+
+      if ($object->getAuditStatus() != $status_verify) {
+        continue;
+      }
+
+      if (!$this->hasAuditorsWithStatus($object, $phids, $has_concern)) {
+        continue;
+      }
+
+      $results[$key] = $object;
+      unset($this->objects[$key]);
+    }
+
+    return $results;
+  }
+
   private function filterShouldAudit(array $phids) {
     $results = array();
     $objects = $this->objects;
@@ -132,6 +167,7 @@ final class DiffusionCommitRequiredActionResultBucket
 
     $status_waiting = array(
       PhabricatorAuditCommitStatusConstants::NEEDS_AUDIT,
+      PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION,
       PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED,
     );
     $status_waiting = array_fuse($status_waiting);

src/applications/diffusion/xaction/DiffusionCommitAcceptTransaction.php

@@ -30,11 +30,6 @@ final class DiffusionCommitAcceptTransaction
     return pht('Accepted');
   }
 
-  public function generateOldValue($object) {
-    $actor = $this->getActor();
-    return $this->isViewerAcceptingAuditor($object, $actor);
-  }
-
   public function applyExternalEffects($object, $value) {
     $status = PhabricatorAuditStatusConstants::ACCEPTED;
     $actor = $this->getActor();
@@ -54,7 +49,7 @@ final class DiffusionCommitAcceptTransaction
       }
     }
 
-    if ($this->isViewerAcceptingAuditor($object, $viewer)) {
+    if ($this->isViewerFullyAccepted($object, $viewer)) {
       throw new Exception(
         pht(
           'You can not accept this commit because you have already '.

src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php

@@ -7,6 +7,10 @@ abstract class DiffusionCommitAuditTransaction
     return DiffusionCommitEditEngine::ACTIONGROUP_AUDIT;
   }
 
+  public function generateOldValue($object) {
+    return false;
+  }
+
   protected function isViewerAnyAuditor(
     PhabricatorRepositoryCommit $commit,
     PhabricatorUser $viewer) {
@@ -18,22 +22,23 @@ abstract class DiffusionCommitAuditTransaction
     PhabricatorUser $viewer) {
 
     // This omits various inactive states like "Resigned" and "Not Required".
+    $active = array(
+      PhabricatorAuditStatusConstants::AUDIT_REQUIRED,
+      PhabricatorAuditStatusConstants::CONCERNED,
+      PhabricatorAuditStatusConstants::ACCEPTED,
+      PhabricatorAuditStatusConstants::AUDIT_REQUESTED,
+    );
+    $active = array_fuse($active);
 
-    return $this->isViewerAuditStatusAmong(
-      $commit,
-      $viewer,
-      array(
-        PhabricatorAuditStatusConstants::AUDIT_REQUIRED,
-        PhabricatorAuditStatusConstants::CONCERNED,
-        PhabricatorAuditStatusConstants::ACCEPTED,
-        PhabricatorAuditStatusConstants::AUDIT_REQUESTED,
-      ));
+    $viewer_status = $this->getViewerAuditStatus($commit, $viewer);
+
+    return isset($active[$viewer_status]);
   }
 
-  protected function isViewerAcceptingAuditor(
+  protected function isViewerFullyAccepted(
     PhabricatorRepositoryCommit $commit,
     PhabricatorUser $viewer) {
-    return $this->isViewerAuditStatusAmong(
+    return $this->isViewerAuditStatusFullyAmong(
       $commit,
       $viewer,
       array(
@@ -41,10 +46,10 @@ abstract class DiffusionCommitAuditTransaction
       ));
   }
 
-  protected function isViewerRejectingAuditor(
+  protected function isViewerFullyRejected(
     PhabricatorRepositoryCommit $commit,
     PhabricatorUser $viewer) {
-    return $this->isViewerAuditStatusAmong(
+    return $this->isViewerAuditStatusFullyAmong(
       $commit,
       $viewer,
       array(
@@ -71,7 +76,7 @@ abstract class DiffusionCommitAuditTransaction
     return null;
   }
 
-  protected function isViewerAuditStatusAmong(
+  protected function isViewerAuditStatusFullyAmong(
     PhabricatorRepositoryCommit $commit,
     PhabricatorUser $viewer,
     array $status_list) {
@@ -82,7 +87,20 @@ abstract class DiffusionCommitAuditTransaction
     }
 
     $status_map = array_fuse($status_list);
-    return isset($status_map[$status]);
+    foreach ($commit->getAudits() as $audit) {
+      if (!$commit->hasAuditAuthority($viewer, $audit)) {
+        continue;
+      }
+
+      $status = $audit->getAuditStatus();
+      if (isset($status_map[$status])) {
+        continue;
+      }
+
+      return false;
+    }
+
+    return true;
   }
 
   protected function applyAuditorEffect(

src/applications/diffusion/xaction/DiffusionCommitConcernTransaction.php

@@ -30,9 +30,11 @@ final class DiffusionCommitConcernTransaction
     return pht('Raised Concern');
   }
 
-  public function generateOldValue($object) {
-    $actor = $this->getActor();
-    return $this->isViewerRejectingAuditor($object, $actor);
+  public function applyInternalEffects($object, $value) {
+    // NOTE: We force the commit directly into "Concern Raised" so that we
+    // override a possible "Needs Verification" state.
+    $object->setAuditStatus(
+      PhabricatorAuditCommitStatusConstants::CONCERN_RAISED);
   }
 
   public function applyExternalEffects($object, $value) {
@@ -50,11 +52,17 @@ final class DiffusionCommitConcernTransaction
           'you did not author.'));
     }
 
-    if ($this->isViewerRejectingAuditor($object, $viewer)) {
-      throw new Exception(
-        pht(
-          'You can not raise a concern with this commit because you have '.
-          'already raised a concern with it.'));
+    // Even if you've already raised a concern, you can raise again as long
+    // as the author requsted you verify.
+    $state_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION;
+
+    if ($this->isViewerFullyRejected($object, $viewer)) {
+      if ($object->getAuditStatus() != $state_verify) {
+        throw new Exception(
+          pht(
+            'You can not raise a concern with this commit because you have '.
+            'already raised a concern with it.'));
+      }
     }
   }
 

src/applications/diffusion/xaction/DiffusionCommitStateTransaction.php

@@ -31,6 +31,8 @@ final class DiffusionCommitStateTransaction
         return pht('This commit now requires audit.');
       case PhabricatorAuditCommitStatusConstants::CONCERN_RAISED:
         return pht('This commit now has outstanding concerns.');
+      case PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION:
+        return pht('This commit now requires verification by auditors.');
       case PhabricatorAuditCommitStatusConstants::FULLY_AUDITED:
         return pht('All concerns with this commit have now been addressed.');
     }
@@ -54,6 +56,10 @@ final class DiffusionCommitStateTransaction
         return pht(
           '%s now has outstanding concerns.',
           $this->renderObject());
+      case PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION:
+        return pht(
+          '%s now requires verification by auditors.',
+          $this->renderObject());
       case PhabricatorAuditCommitStatusConstants::FULLY_AUDITED:
         return pht(
           'All concerns with %s have now been addressed.',

src/applications/diffusion/xaction/DiffusionCommitVerifyTransaction.php

@@ -0,0 +1,73 @@
+<?php
+
+final class DiffusionCommitVerifyTransaction
+  extends DiffusionCommitAuditTransaction {
+
+  const TRANSACTIONTYPE = 'diffusion.commit.verify';
+  const ACTIONKEY = 'verify';
+
+  protected function getCommitActionLabel() {
+    return pht('Request Verification');
+  }
+
+  protected function getCommitActionDescription() {
+    return pht(
+      'Auditors will be asked to verify that concerns have been addressed.');
+  }
+
+  protected function getCommitActionGroupKey() {
+    return DiffusionCommitEditEngine::ACTIONGROUP_COMMIT;
+  }
+
+  public function getIcon() {
+    return 'fa-refresh';
+  }
+
+  public function getColor() {
+    return 'indigo';
+  }
+
+  protected function getCommitActionOrder() {
+    return 600;
+  }
+
+  public function getActionName() {
+    return pht('Requested Verification');
+  }
+
+  public function applyInternalEffects($object, $value) {
+    $object->setAuditStatus(
+      PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION);
+  }
+
+  protected function validateAction($object, PhabricatorUser $viewer) {
+    if (!$this->isViewerCommitAuthor($object, $viewer)) {
+      throw new Exception(
+        pht(
+          'You can not request verification of this commit because you '.
+          'are not the author.'));
+    }
+
+    $status = $object->getAuditStatus();
+    if ($status != PhabricatorAuditCommitStatusConstants::CONCERN_RAISED) {
+      throw new Exception(
+        pht(
+          'You can not request verification of this commit because no '.
+          'auditors have raised conerns with it.'));
+    }
+  }
+
+  public function getTitle() {
+    return pht(
+      '%s requested verification of this commit.',
+      $this->renderAuthor());
+  }
+
+  public function getTitleForFeed() {
+    return pht(
+      '%s requested verification of %s.',
+      $this->renderAuthor(),
+      $this->renderObject());
+  }
+
+}

src/applications/repository/storage/PhabricatorRepositoryCommit.php

@@ -327,8 +327,17 @@ final class PhabricatorRepositoryCommit
       }
     }
 
+    $current_status = $this->getAuditStatus();
+    $status_verify = PhabricatorAuditCommitStatusConstants::NEEDS_VERIFICATION;
+
     if ($any_concern) {
-      $status = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED;
+      if ($current_status == $status_verify) {
+        // If the change is in "Needs Verification", we keep it there as
+        // long as any auditors still have concerns.
+        $status = $status_verify;
+      } else {
+        $status = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED;
+      }
     } else if ($any_accept) {
       if ($any_need) {
         $status = PhabricatorAuditCommitStatusConstants::PARTIALLY_AUDITED;