Mark reviewers as "commented" when they leave a comment

Summary:
Ref T2222. This requires one new trick:

  - When merging edge transactions which both add/update an edge, the Editor gets to control how the edge data is merged.

Specifically, we pick the "strongest" state to keep, so "accept + comment" leaves you with an accept instead of a comment.

Test Plan: Accepted, commented on, and comment + accepted revisions. Added some debugging dumps to verify that the merging was getting hit and working correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

src/applications/differential/constants/DifferentialReviewerStatus.php

@@ -8,4 +8,30 @@ final class DifferentialReviewerStatus {
   const STATUS_REJECTED = 'rejected';
   const STATUS_COMMENTED = 'commented';
 
+  /**
+   * Returns the relative strength of a status, used to pick a winner when a
+   * transaction group makes several status changes to a particular reviewer.
+   *
+   * For example, if you accept a revision and leave a comment, the transactions
+   * will attempt to update you to both "commented" and "accepted". We want
+   * "accepted" to win, because it's the stronger of the two.
+   *
+   * @param   const Reviewer status constant.
+   * @return  int   Relative strength (higher is stronger).
+   */
+  public static function getStatusStrength($constant) {
+    $map = array(
+      self::STATUS_ADDED      => 1,
+
+      self::STATUS_COMMENTED  => 2,
+
+      self::STATUS_BLOCKING   => 3,
+
+      self::STATUS_ACCEPTED   => 4,
+      self::STATUS_REJECTED   => 4,
+    );
+
+    return idx($map, $constant, 0);
+  }
+
 }

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -190,17 +190,46 @@ final class DifferentialTransactionEditor
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
 
+    $actor = $this->getActor();
+    $actor_phid = $actor->getPHID();
+    $type_edge = PhabricatorTransactions::TYPE_EDGE;
+    $edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
+
     $results = parent::expandTransaction($object, $xaction);
     switch ($xaction->getTransactionType()) {
-      // TODO: If the user comments and is a reviewer, we should upgrade their
+      case PhabricatorTransactions::TYPE_COMMENT:
-      // edge metadata to STATUS_COMMENTED.
+        // 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.
+
+        $status_added = DifferentialReviewerStatus::STATUS_ADDED;
+        $status_commented = DifferentialReviewerStatus::STATUS_COMMENTED;
+
+        $data = array(
+          'status' => $status_commented,
+        );
+
+        $edits = array();
+        foreach ($object->getReviewerStatus() as $reviewer) {
+          if ($reviewer->getReviewerPHID() == $actor_phid) {
+            if ($reviewer->getStatus() == $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 DifferentialTransaction::TYPE_ACTION:
-
-        $actor = $this->getActor();
-        $actor_phid = $actor->getPHID();
-        $type_edge = PhabricatorTransactions::TYPE_EDGE;
-        $edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
         $action_type = $xaction->getNewValue();
 
         switch ($action_type) {
@@ -312,6 +341,35 @@ final class DifferentialTransactionEditor
     return parent::applyCustomExternalTransaction($object, $xaction);
   }
 
+  protected function mergeEdgeData($type, array $u, array $v) {
+    $result = parent::mergeEdgeData($type, $u, $v);
+
+    switch ($type) {
+      case PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER:
+        // 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) {

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -953,7 +953,50 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $result = $u->getNewValue();
     foreach ($v->getNewValue() as $key => $value) {
-      $result[$key] = array_merge($value, idx($result, $key, array()));
+      if ($u->getTransactionType() == PhabricatorTransactions::TYPE_EDGE) {
+        if (empty($result[$key])) {
+          $result[$key] = $value;
+        } else {
+          // We're merging two lists of edge adds, sets, or removes. Merge
+          // them by merging individual PHIDs within them.
+          $merged = $result[$key];
+
+          foreach ($value as $dst => $v_spec) {
+            if (empty($merged[$dst])) {
+              $merged[$dst] = $v_spec;
+            } else {
+              // Two transactions are trying to perform the same operation on
+              // the same edge. Normalize the edge data and then merge it. This
+              // allows transactions to specify how data merges execute in a
+              // precise way.
+
+              $u_spec = $merged[$dst];
+
+              if (!is_array($u_spec)) {
+                $u_spec = array('dst' => $u_spec);
+              }
+              if (!is_array($v_spec)) {
+                $v_spec = array('dst' => $v_spec);
+              }
+
+              $ux_data = idx($u_spec, 'data', array());
+              $vx_data = idx($v_spec, 'data', array());
+
+              $merged_data = $this->mergeEdgeData(
+                $u->getMetadataValue('edge:type'),
+                $ux_data,
+                $vx_data);
+
+              $u_spec['data'] = $merged_data;
+              $merged[$dst] = $u_spec;
+            }
+          }
+
+          $result[$key] = $merged;
+        }
+      } else {
+        $result[$key] = array_merge($value, idx($result, $key, array()));
+      }
     }
     $u->setNewValue($result);
 
@@ -966,6 +1009,10 @@ abstract class PhabricatorApplicationTransactionEditor
     return $u;
   }
 
+  protected function mergeEdgeData($type, array $u, array $v) {
+    return $v + $u;
+  }
+
   protected function getPHIDTransactionNewValue(
     PhabricatorApplicationTransaction $xaction) {