Page MenuHomePhabricator
Files F14408068

D8339.diff
No OneTemporary
Actions

D8339.diff
View Options

Index: src/applications/conpherence/editor/ConpherenceEditor.php
===================================================================
--- src/applications/conpherence/editor/ConpherenceEditor.php
+++ src/applications/conpherence/editor/ConpherenceEditor.php
@@ -304,6 +304,8 @@
}
$participant->save();
}
+
+ return $xactions;
}
protected function mergeTransactions(
Index: src/applications/differential/editor/DifferentialTransactionEditor.php
===================================================================
--- src/applications/differential/editor/DifferentialTransactionEditor.php
+++ src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -13,6 +13,7 @@
$types[] = DifferentialTransaction::TYPE_ACTION;
$types[] = DifferentialTransaction::TYPE_INLINE;
+ $types[] = DifferentialTransaction::TYPE_STATUS;
/*
@@ -145,7 +146,6 @@
case DifferentialTransaction::TYPE_INLINE:
return;
case PhabricatorTransactions::TYPE_EDGE:
- // TODO: Update review status.
return;
case DifferentialTransaction::TYPE_ACTION:
$status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
@@ -166,15 +166,12 @@
return;
case DifferentialAction::ACTION_RECLAIM:
$object->setStatus($status_review);
- // TODO: Update review status?
return;
case DifferentialAction::ACTION_REOPEN:
$object->setStatus($status_review);
- // TODO: Update review status?
return;
case DifferentialAction::ACTION_REQUEST:
$object->setStatus($status_review);
- // TODO: Update review status?
return;
case DifferentialAction::ACTION_CLOSE:
$object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED);
@@ -315,6 +312,98 @@
return parent::applyCustomExternalTransaction($object, $xaction);
}
+ protected function applyFinalEffects(
+ PhabricatorLiskDAO $object,
+ array $xactions) {
+
+ $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED;
+ $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
+ $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
+
+ $old_status = $object->getStatus();
+ switch ($old_status) {
+ case $status_accepted:
+ case $status_revision:
+ case $status_review:
+ // Load the most up-to-date version of the revision and its reviewers,
+ // so we don't need to try to deduce the state of reviewers by examining
+ // all the changes made by the transactions.
+ $new_revision = id(new DifferentialRevisionQuery())
+ ->setViewer($this->getActor())
+ ->needReviewerStatus(true)
+ ->withIDs(array($object->getID()))
+ ->executeOne();
+ if (!$new_revision) {
+ throw new Exception(
+ pht('Failed to load revision from transaction finalization.'));
+ }
+
+ // Try to move a revision to "accepted". We look for:
+ //
+ // - at least one accepting reviewer who is a user; and
+ // - no rejects; and
+ // - no blocking reviewers.
+
+ $has_accepting_user = false;
+ $has_rejecting_reviewer = false;
+ $has_blocking_reviewer = false;
+ foreach ($new_revision->getReviewerStatus() as $reviewer) {
+ $reviewer_status = $reviewer->getStatus();
+ switch ($reviewer_status) {
+ case DifferentialReviewerStatus::STATUS_REJECTED:
+ $has_rejecting_reviewer = true;
+ break;
+ case DifferentialReviewerStatus::STATUS_BLOCKING:
+ $has_blocking_reviewer = true;
+ break;
+ case DifferentialReviewerStatus::STATUS_ACCEPTED:
+ if ($reviewer->isUser()) {
+ $has_accepting_user = true;
+ }
+ break;
+ }
+ }
+
+ $new_status = null;
+ if ($has_accepting_user &&
+ !$has_rejecting_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;
+ }
+
+ if ($new_status !== null && $new_status != $old_status) {
+ $xaction = id(new DifferentialTransaction())
+ ->setTransactionType(DifferentialTransaction::TYPE_STATUS)
+ ->setOldValue($old_status)
+ ->setNewValue($new_status);
+
+ $xaction = $this->populateTransaction($object, $xaction)->save();
+
+ $xactions[] = $xaction;
+
+ $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;
+ }
+
+ return $xactions;
+ }
+
+
protected function validateTransaction(
PhabricatorLiskDAO $object,
$type,
Index: src/applications/differential/storage/DifferentialTransaction.php
===================================================================
--- src/applications/differential/storage/DifferentialTransaction.php
+++ src/applications/differential/storage/DifferentialTransaction.php
@@ -5,6 +5,7 @@
const TYPE_INLINE = 'differential:inline';
const TYPE_UPDATE = 'differential:update';
const TYPE_ACTION = 'differential:action';
+ const TYPE_STATUS = 'differential:status';
public function getApplicationName() {
return 'differential';
@@ -18,6 +19,28 @@
return new DifferentialTransactionComment();
}
+ public function shouldHide() {
+ switch ($this->getTransactionType()) {
+ case PhabricatorTransactions::TYPE_EDGE:
+ $old = $this->getOldValue();
+ $new = $this->getNewValue();
+ $add = array_diff_key($new, $old);
+ $rem = array_diff_key($old, $new);
+
+ // Hide metadata-only edge transactions. These correspond to users
+ // accepting or rejecting revisions, but the change is always explicit
+ // because of the TYPE_ACTION transaction. Rendering these transactions
+ // just creates clutter.
+
+ if (!$add && !$rem) {
+ return true;
+ }
+ break;
+ }
+
+ return false;
+ }
+
public function getTitle() {
$author_phid = $this->getAuthorPHID();
$author_handle = $this->renderHandleLink($author_phid);
@@ -45,6 +68,18 @@
}
case self::TYPE_ACTION:
return DifferentialAction::getBasicStoryText($new, $author_handle);
+ case self::TYPE_STATUS:
+ switch ($this->getNewValue()) {
+ case ArcanistDifferentialRevisionStatus::ACCEPTED:
+ return pht(
+ 'This revision is now accepted and ready to land.');
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+ return pht(
+ 'This revision now requires changes to proceed.');
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+ return pht(
+ 'This revision now requires review to proceed.');
+ }
}
return parent::getTitle();
@@ -56,6 +91,16 @@
return 'comment';
case self::TYPE_UPDATE:
return 'refresh';
+ case self::TYPE_STATUS:
+ switch ($this->getNewValue()) {
+ case ArcanistDifferentialRevisionStatus::ACCEPTED:
+ return 'enable';
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+ return 'delete';
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+ return 'refresh';
+ }
+ break;
case self::TYPE_ACTION:
switch ($this->getNewValue()) {
case DifferentialAction::ACTION_CLOSE:
@@ -82,10 +127,41 @@
return parent::getIcon();
}
+ public function shouldDisplayGroupWith(array $group) {
+
+ // Never group status changes with other types of actions, they're indirect
+ // and don't make sense when combined with direct actions.
+
+ $type_status = self::TYPE_STATUS;
+
+ if ($this->getTransactionType() == $type_status) {
+ return false;
+ }
+
+ foreach ($group as $xaction) {
+ if ($xaction->getTransactionType() == $type_status) {
+ return false;
+ }
+ }
+
+ return parent::shouldDisplayGroupWith($group);
+ }
+
+
public function getColor() {
switch ($this->getTransactionType()) {
case self::TYPE_UPDATE:
return PhabricatorTransactions::COLOR_SKY;
+ case self::TYPE_STATUS:
+ switch ($this->getNewValue()) {
+ case ArcanistDifferentialRevisionStatus::ACCEPTED:
+ return PhabricatorTransactions::COLOR_GREEN;
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
+ return PhabricatorTransactions::COLOR_RED;
+ case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+ return PhabricatorTransactions::COLOR_ORANGE;
+ }
+ break;
case self::TYPE_ACTION:
switch ($this->getNewValue()) {
case DifferentialAction::ACTION_CLOSE:
Index: src/applications/legalpad/editor/LegalpadDocumentEditor.php
===================================================================
--- src/applications/legalpad/editor/LegalpadDocumentEditor.php
+++ src/applications/legalpad/editor/LegalpadDocumentEditor.php
@@ -104,6 +104,8 @@
$object->save();
}
+
+ return $xactions;
}
protected function mergeTransactions(
Index: src/applications/paste/editor/PhabricatorPasteEditor.php
===================================================================
--- src/applications/paste/editor/PhabricatorPasteEditor.php
+++ src/applications/paste/editor/PhabricatorPasteEditor.php
@@ -113,6 +113,8 @@
$this->getActor(),
$object->getPHID());
}
+
+ return $xactions;
}
Index: src/applications/pholio/editor/PholioMockEditor.php
===================================================================
--- src/applications/pholio/editor/PholioMockEditor.php
+++ src/applications/pholio/editor/PholioMockEditor.php
@@ -275,6 +275,8 @@
$image->setMockID($object->getID());
$image->save();
}
+
+ return $xactions;
}
protected function mergeTransactions(
Index: src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
===================================================================
--- src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
+++ src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
@@ -377,9 +377,36 @@
"implementation!");
}
+ /**
+ * Fill in a transaction's common values, like author and content source.
+ */
+ protected function populateTransaction(
+ PhabricatorLiskDAO $object,
+ PhabricatorApplicationTransaction $xaction) {
+
+ $actor = $this->getActor();
+
+ // TODO: This needs to be more sophisticated once we have meta-policies.
+ $xaction->setViewPolicy(PhabricatorPolicies::POLICY_PUBLIC);
+ $xaction->setEditPolicy($actor->getPHID());
+
+ $xaction->setAuthorPHID($actor->getPHID());
+ $xaction->setContentSource($this->getContentSource());
+ $xaction->attachViewer($actor);
+ $xaction->attachObject($object);
+
+ if ($object->getPHID()) {
+ $xaction->setObjectPHID($object->getPHID());
+ }
+
+ return $xaction;
+ }
+
+
protected function applyFinalEffects(
PhabricatorLiskDAO $object,
array $xactions) {
+ return $xactions;
}
public function setContentSource(PhabricatorContentSource $content_source) {
@@ -421,14 +448,7 @@
$xactions = $this->combineTransactions($xactions);
foreach ($xactions as $xaction) {
- // TODO: This needs to be more sophisticated once we have meta-policies.
- $xaction->setViewPolicy(PhabricatorPolicies::POLICY_PUBLIC);
- $xaction->setEditPolicy($actor->getPHID());
-
- $xaction->setAuthorPHID($actor->getPHID());
- $xaction->setContentSource($this->getContentSource());
- $xaction->attachViewer($this->getActor());
- $xaction->attachObject($object);
+ $xaction = $this->populateTransaction($object, $xaction);
}
$is_preview = $this->getIsPreview();
@@ -557,7 +577,7 @@
$this->applyHeraldRules($object, $xactions);
}
- $this->applyFinalEffects($object, $xactions);
+ $xactions = $this->applyFinalEffects($object, $xactions);
if ($read_locking) {
$object->endReadLocking();

File Metadata

Mime Type
text/plain
Expires
Wed, Dec 25, 4:04 AM (7 h, 16 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6922669
Default Alt Text
D8339.diff (12 KB)

Event Timeline

Phabricator · Built by Phacility