diff --git a/src/applications/differential/constants/DifferentialReviewerStatus.php b/src/applications/differential/constants/DifferentialReviewerStatus.php index a892762914..725286ecf1 100644 --- a/src/applications/differential/constants/DifferentialReviewerStatus.php +++ b/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); + } + } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index b6c5bdc902..d3fb3637fc 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/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 - // edge metadata to STATUS_COMMENTED. + 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. + + $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) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index cfd3ffd6d4..adc83a2819 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/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) {