revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-21 13:00:56 +01:00
Code Issues Releases Wiki Activity

Fix a case where "Accept + Comment" would ignore the "Accept"

Browse source 
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
This commit is contained in:
epriestley 2017-01-05 11:02:55 -08:00
parent 68374aa264
commit 1f2306999b
1 changed files with 15 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

17
src/applications/differential/editor/DifferentialTransactionEditor.php
View file

@ -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;