Page MenuHomePhabricator
Files F14092826

D18398.diff
No OneTemporary
Actions

D18398.diff
View Options

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -257,7 +257,6 @@
$status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
- $edge_reviewer = DifferentialRevisionHasReviewerEdgeType::EDGECONST;
$edge_ref_task = DifferentialRevisionHasTaskEdgeType::EDGECONST;
$is_sticky_accept = PhabricatorEnv::getEnvConfig(
@@ -297,48 +296,6 @@
$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 @@
}
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 @@
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 @@
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) {
diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
--- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
@@ -103,7 +103,7 @@
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;
}
}
diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
--- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
@@ -210,34 +210,6 @@
$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 @@
}
$table = new DifferentialReviewer();
+ $src_phid = $revision->getPHID();
$reviewers = $table->loadAllWhere(
'revisionPHID = %s AND reviewerPHID IN (%Ls)',
@@ -256,7 +229,7 @@
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())
diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php
--- a/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionReviewersTransaction.php
@@ -106,43 +106,9 @@
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();

File Metadata

Mime Type
text/plain
Expires
Tue, Nov 26, 8:08 AM (20 h, 25 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6788652
Default Alt Text
D18398.diff (10 KB)

Event Timeline

Phabricator · Built by Phacility