Make "JIRA Issues" field work better with noncredentialed accounts

Summary:
Currently, users get an error when making any changes to this field if they don't have a linked JIRA account.

Instead:

  - We should only raise an error if they're trying to //add// issues, and only on the new issues. It's always fine to remove issues, and existing issues the author can't see are also fine.
  - When we can't add things because there's no account (vs because there's a permissions error or they don't exist), raise a more tailored exception.

Test Plan:
  - As JIRA and non-JIRA users, made various edits to this field.
  - Got appropriate exceptions, including better tailoring.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: mbishopim3, epriestley

Differential Revision: https://secure.phabricator.com/D8676

src/__phutil_library_map__.php

@@ -622,6 +622,7 @@ phutil_register_library_map(array(
     'DoorkeeperFeedWorkerAsana' => 'applications/doorkeeper/worker/DoorkeeperFeedWorkerAsana.php',
     'DoorkeeperFeedWorkerJIRA' => 'applications/doorkeeper/worker/DoorkeeperFeedWorkerJIRA.php',
     'DoorkeeperImportEngine' => 'applications/doorkeeper/engine/DoorkeeperImportEngine.php',
+    'DoorkeeperMissingLinkException' => 'applications/doorkeeper/exception/DoorkeeperMissingLinkException.php',
     'DoorkeeperObjectRef' => 'applications/doorkeeper/engine/DoorkeeperObjectRef.php',
     'DoorkeeperRemarkupRule' => 'applications/doorkeeper/remarkup/DoorkeeperRemarkupRule.php',
     'DoorkeeperRemarkupRuleAsana' => 'applications/doorkeeper/remarkup/DoorkeeperRemarkupRuleAsana.php',
@@ -3196,6 +3197,7 @@ phutil_register_library_map(array(
     'DoorkeeperFeedWorkerAsana' => 'DoorkeeperFeedWorker',
     'DoorkeeperFeedWorkerJIRA' => 'DoorkeeperFeedWorker',
     'DoorkeeperImportEngine' => 'Phobject',
+    'DoorkeeperMissingLinkException' => 'Exception',
     'DoorkeeperObjectRef' => 'Phobject',
     'DoorkeeperRemarkupRule' => 'PhutilRemarkupRule',
     'DoorkeeperRemarkupRuleAsana' => 'DoorkeeperRemarkupRule',

src/applications/differential/customfield/DifferentialJIRAIssuesField.php

@@ -116,11 +116,11 @@ final class DifferentialJIRAIssuesField
   }
 
   public function getOldValueForApplicationTransactions() {
-    return nonempty($this->getValue(), array());
+    return array_unique(nonempty($this->getValue(), array()));
   }
 
   public function getNewValueForApplicationTransactions() {
-    return nonempty($this->getValue(), array());
+    return array_unique(nonempty($this->getValue(), array()));
   }
 
   public function validateApplicationTransactions(
@@ -137,12 +137,36 @@ final class DifferentialJIRAIssuesField
 
     $transaction = null;
     foreach ($xactions as $xaction) {
-      $value = $xaction->getNewValue();
+      $old = $xaction->getOldValue();
+      $new = $xaction->getNewValue();
 
-      $refs = id(new DoorkeeperImportEngine())
-        ->setViewer($this->getViewer())
-        ->setRefs($this->buildDoorkeeperRefs($value))
-        ->execute();
+      $add = array_diff($new, $old);
+      if (!$add) {
+        continue;
+      }
+
+      // Only check that the actor can see newly added JIRA refs. You're
+      // allowed to remove refs or make no-op changes even if you aren't
+      // linked to JIRA.
+
+      try {
+        $refs = id(new DoorkeeperImportEngine())
+          ->setViewer($this->getViewer())
+          ->setRefs($this->buildDoorkeeperRefs($add))
+          ->setThrowOnMissingLink(true)
+          ->execute();
+      } catch (DoorkeeperMissingLinkException $ex) {
+        $this->error = pht('Not Linked');
+        $errors[] = new PhabricatorApplicationTransactionValidationError(
+          $type,
+          pht('Not Linked'),
+          pht(
+            'You can not add JIRA issues (%s) to this revision because your '.
+            'Phabricator account is not linked to a JIRA account.',
+            implode(', ', $add)),
+          $xaction);
+        continue;
+      }
 
       $bad = array();
       foreach ($refs as $ref) {
@@ -155,7 +179,7 @@ final class DifferentialJIRAIssuesField
         $bad = implode(', ', $bad);
         $this->error = pht('Invalid');
 
-        $error = new PhabricatorApplicationTransactionValidationError(
+        $errors[] = new PhabricatorApplicationTransactionValidationError(
           $type,
           pht('Invalid'),
           pht(
@@ -163,7 +187,6 @@ final class DifferentialJIRAIssuesField
             "you may not have permission to view them: %s",
             $bad),
           $xaction);
-        $errors[] = $error;
       }
     }
 

src/applications/doorkeeper/bridge/DoorkeeperBridge.php

@@ -3,6 +3,12 @@
 abstract class DoorkeeperBridge extends Phobject {
 
   private $viewer;
+  private $throwOnMissingLink;
+
+  public function setThrowOnMissingLink($throw_on_missing_link) {
+    $this->throwOnMissingLink = $throw_on_missing_link;
+    return $this;
+  }
 
   final public function setViewer($viewer) {
     $this->viewer = $viewer;
@@ -21,6 +27,15 @@ abstract class DoorkeeperBridge extends Phobject {
   abstract public function pullRefs(array $refs);
 
   public function fillObjectFromData(DoorkeeperExternalObject $obj, $result) {
+    return;
+  }
+
+  public function didFailOnMissingLink() {
+    if ($this->throwOnMissingLink) {
+      throw new DoorkeeperMissingLinkException();
+    }
+
+    return null;
   }
 
 }

src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php

@@ -40,7 +40,7 @@ final class DoorkeeperBridgeAsana extends DoorkeeperBridge {
       ->execute();
 
     if (!$accounts) {
-      return;
+      return $this->didFailOnMissingLink();
     }
 
     // TODO: If the user has several linked Asana accounts, we just pick the

src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php

@@ -34,7 +34,7 @@ final class DoorkeeperBridgeJIRA extends DoorkeeperBridge {
       ->execute();
 
     if (!$accounts) {
-      return;
+      return $this->didFailOnMissingLink();
     }
 
     // TODO: When we support multiple JIRA instances, we need to disambiguate

src/applications/doorkeeper/engine/DoorkeeperImportEngine.php

@@ -6,6 +6,7 @@ final class DoorkeeperImportEngine extends Phobject {
   private $refs = array();
   private $phids = array();
   private $localOnly;
+  private $throwOnMissingLink;
 
   public function setViewer(PhabricatorUser $viewer) {
     $this->viewer = $viewer;
@@ -36,6 +37,16 @@ final class DoorkeeperImportEngine extends Phobject {
     return $this;
   }
 
+
+  /**
+   * Configure behavior if remote refs can not be retrieved because an
+   * authentication link is missing.
+   */
+  public function setThrowOnMissingLink($throw) {
+    $this->throwOnMissingLink = $throw;
+    return $this;
+  }
+
   public function execute() {
     $refs = $this->getRefs();
     $viewer = $this->getViewer();
@@ -86,6 +97,7 @@ final class DoorkeeperImportEngine extends Phobject {
           unset($bridges[$key]);
         }
         $bridge->setViewer($viewer);
+        $bridge->setThrowOnMissingLink($this->throwOnMissingLink);
       }
 
       $working_set = $refs;

src/applications/doorkeeper/exception/DoorkeeperMissingLinkException.php

@@ -0,0 +1,5 @@
+<?php
+
+final class DoorkeeperMissingLinkException extends Exception {
+
+}