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();