Page MenuHomePhabricator
Files F15451014

D18413.id.diff
No OneTemporary
Actions

D18413.id.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
@@ -125,23 +125,16 @@
PhabricatorLiskDAO $object,
PhabricatorApplicationTransaction $xaction) {
- $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
- $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
- $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
- $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED;
- $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
-
switch ($xaction->getTransactionType()) {
case DifferentialTransaction::TYPE_INLINE:
return;
case DifferentialTransaction::TYPE_UPDATE:
if (!$this->getIsCloseByCommit()) {
- switch ($object->getStatus()) {
- case $status_revision:
- case $status_plan:
- case $status_abandoned:
- $object->setStatus($status_review);
- break;
+ if ($object->isNeedsRevision() ||
+ $object->isChangePlanned() ||
+ $object->isAbandoned()) {
+ $object->setModernRevisionStatus(
+ DifferentialRevisionStatus::NEEDS_REVIEW);
}
}
@@ -196,8 +189,6 @@
$actor_phid = $this->getActingAsPHID();
$type_edge = PhabricatorTransactions::TYPE_EDGE;
- $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED;
-
$edge_ref_task = DifferentialRevisionHasTaskEdgeType::EDGECONST;
$is_sticky_accept = PhabricatorEnv::getEnvConfig(
@@ -220,7 +211,7 @@
case DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE:
$downgrade_rejects = true;
if ((!$is_sticky_accept) ||
- ($object->getStatus() != $status_plan)) {
+ (!$object->isChangePlanned())) {
// If the old state isn't "changes planned", downgrade the accepts.
// This exception allows an accepted revision to go through
// "Plan Changes" -> "Request Review" and return to "accepted" if
@@ -462,102 +453,114 @@
}
}
- $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
- $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
- $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
-
- $is_sticky_accept = PhabricatorEnv::getEnvConfig(
- 'differential.sticky-accept');
+ $xactions = $this->updateReviewStatus($object, $xactions);
+ $this->markReviewerComments($object, $xactions);
- $old_status = $object->getStatus();
- $active_diff = $object->getActiveDiff();
- switch ($old_status) {
- case $status_accepted:
- case $status_revision:
- case $status_review:
- // Try to move a revision to "accepted". We look for:
- //
- // - at least one accepting reviewer who is a user; and
- // - no rejects; and
- // - no rejects of older diffs; and
- // - no blocking reviewers.
-
- $has_accepting_user = false;
- $has_rejecting_reviewer = false;
- $has_rejecting_older_reviewer = false;
- $has_blocking_reviewer = false;
- foreach ($object->getReviewers() as $reviewer) {
- $reviewer_status = $reviewer->getReviewerStatus();
- switch ($reviewer_status) {
- case DifferentialReviewerStatus::STATUS_REJECTED:
- $active_phid = $active_diff->getPHID();
- if ($reviewer->isRejected($active_phid)) {
- $has_rejecting_reviewer = true;
- } else {
- $has_rejecting_older_reviewer = true;
- }
- break;
- case DifferentialReviewerStatus::STATUS_REJECTED_OLDER:
- $has_rejecting_older_reviewer = true;
- break;
- case DifferentialReviewerStatus::STATUS_BLOCKING:
- $has_blocking_reviewer = true;
- break;
- case DifferentialReviewerStatus::STATUS_ACCEPTED:
- if ($reviewer->isUser()) {
- $active_phid = $active_diff->getPHID();
- if ($reviewer->isAccepted($active_phid)) {
- $has_accepting_user = true;
- }
- }
- break;
- }
- }
+ return $xactions;
+ }
- $new_status = null;
- if ($has_accepting_user &&
- !$has_rejecting_reviewer &&
- !$has_rejecting_older_reviewer &&
- !$has_blocking_reviewer) {
- $new_status = $status_accepted;
- } else if ($has_rejecting_reviewer) {
- // This isn't accepted, and there's at least one rejecting reviewer,
- // so the revision needs changes. This usually happens after a
- // "reject".
- $new_status = $status_revision;
- } else if ($old_status == $status_accepted) {
- // This revision was accepted, but it no longer satisfies the
- // conditions for acceptance. This usually happens after an accepting
- // reviewer resigns or is removed.
- $new_status = $status_review;
- }
+ private function updateReviewStatus(
+ DifferentialRevision $revision,
+ array $xactions) {
- if ($new_status !== null && ($new_status != $old_status)) {
- $xaction = id(new DifferentialTransaction())
- ->setTransactionType(
- DifferentialRevisionStatusTransaction::TRANSACTIONTYPE)
- ->setOldValue($old_status)
- ->setNewValue($new_status);
+ $was_accepted = $revision->isAccepted();
+ $was_revision = $revision->isNeedsRevision();
+ $was_review = $revision->isNeedsReview();
+ if (!$was_accepted && !$was_revision && !$was_review) {
+ // Revisions can't transition out of other statuses (like closed or
+ // abandoned) as a side effect of reviewer status changes.
+ return $xactions;
+ }
+
+ // Try to move a revision to "accepted". We look for:
+ //
+ // - at least one accepting reviewer who is a user; and
+ // - no rejects; and
+ // - no rejects of older diffs; and
+ // - no blocking reviewers.
+
+ $has_accepting_user = false;
+ $has_rejecting_reviewer = false;
+ $has_rejecting_older_reviewer = false;
+ $has_blocking_reviewer = false;
+
+ $active_diff = $revision->getActiveDiff();
+ foreach ($revision->getReviewers() as $reviewer) {
+ $reviewer_status = $reviewer->getReviewerStatus();
+ switch ($reviewer_status) {
+ case DifferentialReviewerStatus::STATUS_REJECTED:
+ $active_phid = $active_diff->getPHID();
+ if ($reviewer->isRejected($active_phid)) {
+ $has_rejecting_reviewer = true;
+ } else {
+ $has_rejecting_older_reviewer = true;
+ }
+ break;
+ case DifferentialReviewerStatus::STATUS_REJECTED_OLDER:
+ $has_rejecting_older_reviewer = true;
+ break;
+ case DifferentialReviewerStatus::STATUS_BLOCKING:
+ $has_blocking_reviewer = true;
+ break;
+ case DifferentialReviewerStatus::STATUS_ACCEPTED:
+ if ($reviewer->isUser()) {
+ $active_phid = $active_diff->getPHID();
+ if ($reviewer->isAccepted($active_phid)) {
+ $has_accepting_user = true;
+ }
+ }
+ break;
+ }
+ }
- $xaction = $this->populateTransaction($object, $xaction)->save();
+ $new_status = null;
+ if ($has_accepting_user &&
+ !$has_rejecting_reviewer &&
+ !$has_rejecting_older_reviewer &&
+ !$has_blocking_reviewer) {
+ $new_status = DifferentialRevisionStatus::ACCEPTED;
+ } else if ($has_rejecting_reviewer) {
+ // This isn't accepted, and there's at least one rejecting reviewer,
+ // so the revision needs changes. This usually happens after a
+ // "reject".
+ $new_status = DifferentialRevisionStatus::NEEDS_REVISION;
+ } else if ($was_accepted) {
+ // This revision was accepted, but it no longer satisfies the
+ // conditions for acceptance. This usually happens after an accepting
+ // reviewer resigns or is removed.
+ $new_status = DifferentialRevisionStatus::NEEDS_REVIEW;
+ }
- $xactions[] = $xaction;
+ if ($new_status === null) {
+ return $xactions;
+ }
- $object->setStatus($new_status)->save();
- }
- break;
- default:
- // Revisions can't transition out of other statuses (like closed or
- // abandoned) as a side effect of reviewer status changes.
- break;
+ $old_legacy_status = $revision->getStatus();
+ $revision->setModernRevisionStatus($new_status);
+ $new_legacy_status = $revision->getStatus();
+ if ($new_legacy_status == $old_legacy_status) {
+ return $xactions;
}
+ $xaction = id(new DifferentialTransaction())
+ ->setTransactionType(
+ DifferentialRevisionStatusTransaction::TRANSACTIONTYPE)
+ ->setOldValue($old_legacy_status)
+ ->setNewValue($new_legacy_status);
- $this->markReviewerComments($object, $xactions);
+ $xaction = $this->populateTransaction($revision, $xaction)
+ ->save();
+ $xactions[] = $xaction;
+
+ // Save the status adjustment we made earlier.
+ // TODO: This can be a little cleaner and more obvious once storage
+ // migrates.
+ $revision->save();
return $xactions;
}
+
protected function validateTransaction(
PhabricatorLiskDAO $object,
$type,
diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php
--- a/src/applications/differential/storage/DifferentialRevision.php
+++ b/src/applications/differential/storage/DifferentialRevision.php
@@ -612,6 +612,21 @@
return $this;
}
+ public function setModernRevisionStatus($status) {
+ $status_object = DifferentialRevisionStatus::newForStatus($status);
+
+ if ($status_object->getKey() != $status) {
+ throw new Exception(
+ pht(
+ 'Trying to set revision to invalid status "%s".',
+ $status));
+ }
+
+ $legacy_status = $status_object->getLegacyKey();
+
+ return $this->setStatus($legacy_status);
+ }
+
public function isClosed() {
return $this->getStatusObject()->isClosedStatus();
}
@@ -628,6 +643,10 @@
return $this->getStatusObject()->isNeedsReview();
}
+ public function isNeedsRevision() {
+ return $this->getStatusObject()->isNeedsRevision();
+ }
+
public function isChangePlanned() {
return $this->getStatusObject()->isChangePlanned();
}

File Metadata

Mime Type
text/plain
Expires
Sat, Mar 29, 7:40 PM (3 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7649349
Default Alt Text
D18413.id.diff (10 KB)

Event Timeline

Phabricator · Built by Phacility