From 1f2306999bb599bcedc0c4bfff2f2b3aade58630 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 5 Jan 2017 11:02:55 -0800 Subject: [PATCH] Fix a case where "Accept + Comment" would ignore the "Accept" Summary: Ref T11114. When you comment, we try to upgrade your review status to "commented". This can conflict with upgrading it to "accepted" or "rejected", or removing it entirely. For now, just avoid making this update. After T10967, I expect "you commented" to be orthogonal to accepted/rejected so it should stop conflicting on its own. Test Plan: - As an "added" reviewer, accepted a revision with a comment in the same transaction. - Before patch: accept didn't stick. - After patch: accept sticks. This may be somewhat magical/order-dependent but I was able to reproduce it locally. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11114 Differential Revision: https://secure.phabricator.com/D17146 --- .../editor/DifferentialTransactionEditor.php | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 2381150188..eedcd6b7bb 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -7,6 +7,7 @@ final class DifferentialTransactionEditor private $isCloseByCommit; private $repositoryPHIDOverride = false; private $didExpandInlineState = false; + private $hasReviewTransaction = false; private $affectedPaths; public function getEditorApplicationClass() { @@ -261,13 +262,20 @@ final class DifferentialTransactionEditor PhabricatorLiskDAO $object, array $xactions) { - // If we have an "Inline State" transaction already, the caller built it - // for us so we don't need to expand it again. foreach ($xactions as $xaction) { switch ($xaction->getTransactionType()) { case PhabricatorTransactions::TYPE_INLINESTATE: + // If we have an "Inline State" transaction already, the caller + // built it for us so we don't need to expand it again. $this->didExpandInlineState = true; break; + case DifferentialRevisionAcceptTransaction::TRANSACTIONTYPE: + case DifferentialRevisionRejectTransaction::TRANSACTIONTYPE: + case DifferentialRevisionResignTransaction::TRANSACTIONTYPE: + // If we have a review transaction, we'll skip marking the user + // as "Commented" later. This should get cleaner after T10967. + $this->hasReviewTransaction = true; + break; } } @@ -426,6 +434,11 @@ final class DifferentialTransactionEditor // "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; -- 2.51.2