From 0b15624c37e86684315ccc5842d424b65e1b9ac5 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 11 Mar 2014 13:47:30 -0700
Subject: [PATCH] Fix two minor Differential issues

Summary: The JIRA field is currently always enabled. This isn't correct; it
should be disabled if there's no JIRA provider.

We also use the old set of reviewers to compute mail delivery. Instead, reload
the correct set of reviewers before moving on after finalizing transactions.
---
 .../DifferentialJIRAIssuesField.php           |  4 +++
 .../editor/DifferentialTransactionEditor.php  | 34 +++++++++++--------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/src/applications/differential/customfield/DifferentialJIRAIssuesField.php b/src/applications/differential/customfield/DifferentialJIRAIssuesField.php
index 1f529ba950..dd63675d9b 100644
--- a/src/applications/differential/customfield/DifferentialJIRAIssuesField.php
+++ b/src/applications/differential/customfield/DifferentialJIRAIssuesField.php
@@ -13,6 +13,10 @@ final class DifferentialJIRAIssuesField
     return 'jira.issues';
   }
 
+  public function isFieldEnabled() {
+    return (bool)PhabricatorAuthProviderOAuth1JIRA::getJIRAProvider();
+  }
+
   public function canDisableField() {
     return false;
   }
diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index 0f72444f71..eda8b04e74 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -504,6 +504,25 @@ final class DifferentialTransactionEditor
     PhabricatorLiskDAO $object,
     array $xactions) {
 
+    // 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. Then, update the reviewers
+    // on the object to make sure we're acting on the current reviewer set
+    // (and, for example, sending mail to the right people).
+
+    $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.'));
+    }
+
+    $object->attachReviewerStatus($new_revision->getReviewerStatus());
+
+
     foreach ($xactions as $xaction) {
       switch ($xaction->getTransactionType()) {
         case DifferentialTransaction::TYPE_UPDATE:
@@ -527,19 +546,6 @@ final class DifferentialTransactionEditor
       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
@@ -551,7 +557,7 @@ final class DifferentialTransactionEditor
         $has_rejecting_reviewer = false;
         $has_rejecting_older_reviewer = false;
         $has_blocking_reviewer = false;
-        foreach ($new_revision->getReviewerStatus() as $reviewer) {
+        foreach ($object->getReviewerStatus() as $reviewer) {
           $reviewer_status = $reviewer->getStatus();
           switch ($reviewer_status) {
             case DifferentialReviewerStatus::STATUS_REJECTED: