diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 203367c1a3..033c6e6930 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/differential/customfield/DifferentialJIRAIssuesField.php b/src/applications/differential/customfield/DifferentialJIRAIssuesField.php index 8ba27ed4ed..5a0e62164f 100644 --- a/src/applications/differential/customfield/DifferentialJIRAIssuesField.php +++ b/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; } } diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridge.php b/src/applications/doorkeeper/bridge/DoorkeeperBridge.php index 5b23721d00..2061150aba 100644 --- a/src/applications/doorkeeper/bridge/DoorkeeperBridge.php +++ b/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; } } diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php index 0f0783ce26..855498f471 100644 --- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php +++ b/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 diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php index d259e40ebb..5149c962dd 100644 --- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php +++ b/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 diff --git a/src/applications/doorkeeper/engine/DoorkeeperImportEngine.php b/src/applications/doorkeeper/engine/DoorkeeperImportEngine.php index f4cf151a97..bbbef3191b 100644 --- a/src/applications/doorkeeper/engine/DoorkeeperImportEngine.php +++ b/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; diff --git a/src/applications/doorkeeper/exception/DoorkeeperMissingLinkException.php b/src/applications/doorkeeper/exception/DoorkeeperMissingLinkException.php new file mode 100644 index 0000000000..e745007040 --- /dev/null +++ b/src/applications/doorkeeper/exception/DoorkeeperMissingLinkException.php @@ -0,0 +1,5 @@ +