Remove the actor itself from reviewer list in commandeering

Summary: When the commandeerer was a reviewer, after the commandeering, he stayed as a reviewer. He can no longer amend the diff. He has to go 'Edit Revision' to remove himself/herself. The fix is to remove it automatically.

Test Plan:
comamndeereded a revision and the behavior is correct now:
- I was removed from the revision list
- the comment transaction shows one more entry that I removed myself as a reviewer

Reviewers: epriestley, vrana

Reviewed By: vrana

CC: nh, vrana, aran, Korvin

Maniphest Tasks: T1225

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

src/applications/differential/editor/DifferentialCommentEditor.php

@@ -26,6 +26,7 @@ final class DifferentialCommentEditor {
   protected $message;
   protected $changedByCommit;
   protected $addedReviewers = array();
+  protected $removedReviewers = array();
   private $addedCCs = array();
 
   private $parentMessageID;
@@ -76,6 +77,15 @@ final class DifferentialCommentEditor {
     return $this->addedReviewers;
   }
 
+  public function setRemovedReviewers(array $removeded_reviewers) {
+    $this->removedReviewers = $removeded_reviewers;
+    return $this;
+  }
+
+  public function getRemovedReviewers() {
+    return $this->removedReviewers;
+  }
+
   public function setAddedCCs($added_ccs) {
     $this->addedCCs = $added_ccs;
     return $this;
@@ -234,7 +244,7 @@ final class DifferentialCommentEditor {
               "Unexpected revision state '{$revision_status}'!");
         }
 
-        $added_reviewers = $this->addReviewers();
+        list($added_reviewers, $ignored) = $this->alterReviewers();
         if ($added_reviewers) {
           $key = DifferentialComment::METADATA_ADDED_REVIEWERS;
           $metadata[$key] = $added_reviewers;
@@ -360,7 +370,7 @@ final class DifferentialCommentEditor {
         break;
 
       case DifferentialAction::ACTION_ADDREVIEWERS:
-        $added_reviewers = $this->addReviewers();
+        list($added_reviewers, $ignored) = $this->alterReviewers();
 
         if ($added_reviewers) {
           $key = DifferentialComment::METADATA_ADDED_REVIEWERS;
@@ -430,17 +440,23 @@ final class DifferentialCommentEditor {
         }
 
         $this->setAddedReviewers(array($revision->getAuthorPHID()));
+        $this->setRemovedReviewers(array($actor_phid));
 
         // NOTE: Set the new author PHID before calling addReviewers(), since it
         // doesn't permit the author to become a reviewer.
         $revision->setAuthorPHID($actor_phid);
 
-        $added_reviewers = $this->addReviewers();
+        list($added_reviewers, $removed_reviewers) = $this->alterReviewers();
         if ($added_reviewers) {
           $key = DifferentialComment::METADATA_ADDED_REVIEWERS;
           $metadata[$key] = $added_reviewers;
         }
 
+        if ($removed_reviewers) {
+          $key = DifferentialComment::METADATA_REMOVED_REVIEWERS;
+          $metadata[$key] = $removed_reviewers;
+        }
+
         break;
       default:
         throw new Exception('Unsupported action.');
@@ -613,32 +629,41 @@ final class DifferentialCommentEditor {
     return $ccs;
   }
 
-  private function addReviewers() {
+  private function alterReviewers() {
     $revision = $this->revision;
     $added_reviewers = $this->getAddedReviewers();
+    $removed_reviewers = $this->getRemovedReviewers();
     $reviewer_phids = $revision->getReviewers();
 
+    $reviewer_phids_map = array_fill_keys($reviewer_phids, true);
     foreach ($added_reviewers as $k => $user_phid) {
       if ($user_phid == $revision->getAuthorPHID()) {
         unset($added_reviewers[$k]);
       }
-      if (!empty($reviewer_phids[$user_phid])) {
+      if (isset($reviewer_phids_map[$user_phid])) {
         unset($added_reviewers[$k]);
       }
     }
 
+    foreach ($removed_reviewers as $k => $user_phid) {
+      if (!isset($reviewer_phids_map[$user_phid])) {
+        unset($removed_reviewers[$k]);
+      }
+    }
+
     $added_reviewers = array_unique($added_reviewers);
+    $removed_reviewers = array_unique($removed_reviewers);
 
     if ($added_reviewers) {
       DifferentialRevisionEditor::alterReviewers(
         $revision,
         $reviewer_phids,
-        $rem = array(),
+        $removed_reviewers,
         $added_reviewers,
         $this->actorPHID);
     }
 
-    return $added_reviewers;
+    return array($added_reviewers, $removed_reviewers);
   }
 
 }

src/applications/differential/storage/DifferentialComment.php

@@ -19,6 +19,7 @@
 final class DifferentialComment extends DifferentialDAO {
 
   const METADATA_ADDED_REVIEWERS   = 'added-reviewers';
+  const METADATA_REMOVED_REVIEWERS = 'removed-reviewers';
   const METADATA_ADDED_CCS         = 'added-ccs';
   const METADATA_DIFF_ID           = 'diff-id';
 

src/applications/differential/view/DifferentialRevisionCommentView.php

@@ -136,6 +136,10 @@ final class DifferentialRevisionCommentView extends AphrontView {
       $metadata,
       DifferentialComment::METADATA_ADDED_REVIEWERS,
       array());
+    $removed_reviewers = idx(
+      $metadata,
+      DifferentialComment::METADATA_REMOVED_REVIEWERS,
+      array());
     $added_ccs = idx(
       $metadata,
       DifferentialComment::METADATA_ADDED_CCS,
@@ -180,6 +184,11 @@ final class DifferentialRevisionCommentView extends AphrontView {
         $this->renderHandleList($added_reviewers).".";
     }
 
+    if ($removed_reviewers) {
+      $actions[] = "{$author_link} removed reviewers: ".
+        $this->renderHandleList($removed_reviewers).".";
+    }
+
     if ($added_ccs) {
       $actions[] = "{$author_link} added CCs: ".
         $this->renderHandleList($added_ccs).".";