Remove old reviewer double writes to legacy edge table in Differential

Summary:
Ref T2543. Ref T10967. This isn't precisely related to "draft" status, but while I'm churning this stuff anyway, get rid of the old double writes to clean the code up a bit.

These were added in T10967 to make sure the migration was reversible/recoverable, but we haven't seen any issues with it in several months so I believe they can now be removed safely. Nothing has read this table since ~April.

Test Plan: Took various review actions on revisions (accept, reject, resign, comment, etc). If this change is correct, there should be no visible effect.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10967, T2543

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

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -257,7 +257,6 @@ final class DifferentialTransactionEditor
 
     $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
 
-    $edge_reviewer = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
     $edge_ref_task = DifferentialRevisionHasTaskEdgeType::EDGECONST;
 
     $is_sticky_accept = PhabricatorEnv::getEnvConfig(
@@ -297,48 +296,6 @@ final class DifferentialTransactionEditor
     $old_accept = DifferentialReviewerStatus::STATUS_ACCEPTED_OLDER;
     $old_reject = DifferentialReviewerStatus::STATUS_REJECTED_OLDER;
 
-    if ($downgrade_rejects || $downgrade_accepts) {
-      // When a revision is updated, change all "reject" to "rejected older
-      // revision". This means we won't immediately push the update back into
-      // "needs review", but outstanding rejects will still block it from
-      // moving to "accepted".
-
-      // We also do this for "Request Review", even though the diff is not
-      // updated directly. Essentially, this acts like an update which doesn't
-      // actually change the diff text.
-
-      $edits = array();
-      foreach ($object->getReviewers() as $reviewer) {
-        if ($downgrade_rejects) {
-          if ($reviewer->getReviewerStatus() == $new_reject) {
-            $edits[$reviewer->getReviewerPHID()] = array(
-              'data' => array(
-                'status' => $old_reject,
-              ),
-            );
-          }
-        }
-
-        if ($downgrade_accepts) {
-          if ($reviewer->getReviewerStatus() == $new_accept) {
-            $edits[$reviewer->getReviewerPHID()] = array(
-              'data' => array(
-                'status' => $old_accept,
-              ),
-            );
-          }
-        }
-      }
-
-      if ($edits) {
-        $results[] = id(new DifferentialTransaction())
-          ->setTransactionType($type_edge)
-          ->setMetadataValue('edge:type', $edge_reviewer)
-          ->setIgnoreOnNoEffect(true)
-          ->setNewValue(array('+' => $edits));
-      }
-    }
-
     $downgrade = array();
     if ($downgrade_accepts) {
       $downgrade[] = DifferentialReviewerStatus::STATUS_ACCEPTED;
@@ -397,43 +354,6 @@ final class DifferentialTransactionEditor
         }
         break;
 
-      case PhabricatorTransactions::TYPE_COMMENT:
-        // When a user leaves a comment, upgrade their reviewer status from
-        // "added" to "commented" if they're also a reviewer. We may further
-        // upgrade this based on other actions in the transaction group.
-
-        if ($this->hasReviewTransaction) {
-          // If we're also applying a review transaction, skip this.
-          break;
-        }
-
-        $status_added = DifferentialReviewerStatus::STATUS_ADDED;
-        $status_commented = DifferentialReviewerStatus::STATUS_COMMENTED;
-
-        $data = array(
-          'status' => $status_commented,
-        );
-
-        $edits = array();
-        foreach ($object->getReviewers() as $reviewer) {
-          if ($reviewer->getReviewerPHID() == $actor_phid) {
-            if ($reviewer->getReviewerStatus() == $status_added) {
-              $edits[$actor_phid] = array(
-                'data' => $data,
-              );
-            }
-          }
-        }
-
-        if ($edits) {
-          $results[] = id(new DifferentialTransaction())
-            ->setTransactionType($type_edge)
-            ->setMetadataValue('edge:type', $edge_reviewer)
-            ->setIgnoreOnNoEffect(true)
-            ->setNewValue(array('+' => $edits));
-        }
-        break;
-
       case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE:
         $is_commandeer = true;
         break;
@@ -575,33 +495,6 @@ final class DifferentialTransactionEditor
     return parent::applyBuiltinExternalTransaction($object, $xaction);
   }
 
-  protected function mergeEdgeData($type, array $u, array $v) {
-    $result = parent::mergeEdgeData($type, $u, $v);
-
-    switch ($type) {
-      case DifferentialRevisionHasReviewerEdgeType::EDGECONST:
-        // When the same reviewer has their status updated by multiple
-        // transactions, we want the strongest status to win. An example of
-        // this is when a user adds a comment and also accepts a revision which
-        // they are a reviewer on. The comment creates a "commented" status,
-        // while the accept creates an "accepted" status. Since accept is
-        // stronger, it should win and persist.
-
-        $u_status = idx($u, 'status');
-        $v_status = idx($v, 'status');
-        $u_str = DifferentialReviewerStatus::getStatusStrength($u_status);
-        $v_str = DifferentialReviewerStatus::getStatusStrength($v_status);
-        if ($u_str > $v_str) {
-          $result['status'] = $u_status;
-        } else {
-          $result['status'] = $v_status;
-        }
-        break;
-    }
-
-    return $result;
-  }
-
   protected function applyFinalEffects(
     PhabricatorLiskDAO $object,
     array $xactions) {
@@ -748,40 +641,6 @@ final class DifferentialTransactionEditor
 
     foreach ($xactions as $xaction) {
       switch ($type) {
-        case PhabricatorTransactions::TYPE_EDGE:
-          switch ($xaction->getMetadataValue('edge:type')) {
-            case DifferentialRevisionHasReviewerEdgeType::EDGECONST:
-
-              // Prevent the author from becoming a reviewer.
-
-              // NOTE: This is pretty gross, but this restriction is unusual.
-              // If we end up with too much more of this, we should try to clean
-              // this up -- maybe by moving validation to after transactions
-              // are adjusted (so we can just examine the final value) or adding
-              // a second phase there?
-
-              $author_phid = $object->getAuthorPHID();
-              $new = $xaction->getNewValue();
-
-              $add = idx($new, '+', array());
-              $eq = idx($new, '=', array());
-              $phids = array_keys($add + $eq);
-
-              foreach ($phids as $phid) {
-                if (($phid == $author_phid) &&
-                    !$allow_self_accept &&
-                    !$xaction->getIsCommandeerSideEffect()) {
-                  $errors[] =
-                    new PhabricatorApplicationTransactionValidationError(
-                      $type,
-                      pht('Invalid'),
-                      pht('The author of a revision can not be a reviewer.'),
-                      $xaction);
-                }
-              }
-              break;
-          }
-          break;
         case DifferentialTransaction::TYPE_UPDATE:
           $diff = $this->loadDiff($xaction->getNewValue());
           if (!$diff) {

src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php

@@ -103,7 +103,7 @@ final class DifferentialRevisionAcceptTransaction
         if ($reviewer->isAccepted($diff_phid)) {
           // If a reviewer is already in a full "accepted" state, don't
           // include that reviewer as an option unless we're listing all
-          // reviwers, including reviewers who have already accepted.
+          // reviewers, including reviewers who have already accepted.
           continue;
         }
       }

src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php

@@ -210,34 +210,6 @@ abstract class DifferentialRevisionReviewTransaction
       $map = array_select_keys($map, $value);
     }
 
-    // Convert reviewer statuses into edge data.
-    foreach ($map as $reviewer_phid => $reviewer_status) {
-      $map[$reviewer_phid] = array(
-        'data' => array(
-          'status' => $reviewer_status,
-        ),
-      );
-    }
-
-    // This is currently double-writing: to the old (edge) store and the new
-    // (reviewer) store. Do the old edge write first.
-
-    $src_phid = $revision->getPHID();
-    $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
-
-    $editor = new PhabricatorEdgeEditor();
-    foreach ($map as $dst_phid => $edge_data) {
-      if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) {
-        // TODO: For now, we just remove these reviewers. In the future, we will
-        // store resignations explicitly.
-        $editor->removeEdge($src_phid, $edge_type, $dst_phid);
-      } else {
-        $editor->addEdge($src_phid, $edge_type, $dst_phid, $edge_data);
-      }
-    }
-
-    $editor->save();
-
     // Now, do the new write.
 
     if ($map) {
@@ -249,6 +221,7 @@ abstract class DifferentialRevisionReviewTransaction
       }
 
       $table = new DifferentialReviewer();
+      $src_phid = $revision->getPHID();
 
       $reviewers = $table->loadAllWhere(
         'revisionPHID = %s AND reviewerPHID IN (%Ls)',
@@ -256,7 +229,7 @@ abstract class DifferentialRevisionReviewTransaction
         array_keys($map));
       $reviewers = mpull($reviewers, null, 'getReviewerPHID');
 
-      foreach ($map as $dst_phid => $edge_data) {
+      foreach (array_keys($map) as $dst_phid) {
         $reviewer = idx($reviewers, $dst_phid);
         if (!$reviewer) {
           $reviewer = id(new DifferentialReviewer())

src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php

@@ -106,43 +106,9 @@ final class DifferentialRevisionReviewersTransaction
   public function applyExternalEffects($object, $value) {
     $src_phid = $object->getPHID();
 
-    // This is currently double-writing: to the old (edge) store and the new
-    // (reviewer) store. Do the old edge write first.
-
     $old = $this->generateOldValue($object);
     $new = $value;
-    $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
-
-    $editor = new PhabricatorEdgeEditor();
-
     $rem = array_diff_key($old, $new);
-    foreach ($rem as $dst_phid => $status) {
-      $editor->removeEdge($src_phid, $edge_type, $dst_phid);
-    }
-
-    foreach ($new as $dst_phid => $status) {
-      $old_status = idx($old, $dst_phid);
-      if ($old_status === $status) {
-        continue;
-      }
-
-      $data = array(
-        'data' => array(
-          'status' => $status,
-
-          // TODO: This seemes like it's buggy before the Modular Transactions
-          // changes. Figure out what's going on here? We don't have a very
-          // clean way to get the active diff ID right now.
-          'diffID' => null,
-        ),
-      );
-
-      $editor->addEdge($src_phid, $edge_type, $dst_phid, $data);
-    }
-
-    $editor->save();
-
-    // Now, do the new write.
 
     $table = new DifferentialReviewer();
     $table_name = $table->getTableName();