Page MenuHomePhabricator
Files F17910789

D8340.diff
No OneTemporary
Actions

D8340.diff
View Options

Index: src/applications/differential/constants/DifferentialReviewerStatus.php
===================================================================
--- src/applications/differential/constants/DifferentialReviewerStatus.php
+++ src/applications/differential/constants/DifferentialReviewerStatus.php
@@ -8,4 +8,30 @@
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);
+ }
+
}
Index: src/applications/differential/editor/DifferentialTransactionEditor.php
===================================================================
--- src/applications/differential/editor/DifferentialTransactionEditor.php
+++ src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -190,17 +190,46 @@
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,
+ );
+ }
+ }
+ }
- case DifferentialTransaction::TYPE_ACTION:
+ if ($edits) {
+ $results[] = id(new DifferentialTransaction())
+ ->setTransactionType($type_edge)
+ ->setMetadataValue('edge:type', $edge_reviewer)
+ ->setIgnoreOnNoEffect(true)
+ ->setNewValue(array('+' => $edits));
+ }
+ break;
- $actor = $this->getActor();
- $actor_phid = $actor->getPHID();
- $type_edge = PhabricatorTransactions::TYPE_EDGE;
- $edge_reviewer = PhabricatorEdgeConfig::TYPE_DREV_HAS_REVIEWER;
+ case DifferentialTransaction::TYPE_ACTION:
$action_type = $xaction->getNewValue();
switch ($action_type) {
@@ -312,6 +341,35 @@
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) {
Index: src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
===================================================================
--- src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -953,7 +953,50 @@
$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 @@
return $u;
}
+ protected function mergeEdgeData($type, array $u, array $v) {
+ return $v + $u;
+ }
+
protected function getPHIDTransactionNewValue(
PhabricatorApplicationTransaction $xaction) {

File Metadata

Mime Type
text/plain
Expires
Wed, Jul 30, 3:06 PM (5 d, 12 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
8666376
Default Alt Text
D8340.diff (7 KB)

Event Timeline

Phabricator · Built by Phacility