From 6bada7db4ceb83158d60b0ff1ff79554833f909b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 6 Nov 2019 07:48:26 -0800 Subject: [PATCH 01/13] Change the Herald "do not include [any of]" condition label to "include none of" Summary: Fixes T13445. Make the meaning of this condition more clear, since the current wording is ambiguous between "any of" and "all of". Test Plan: Edited a Herald rule with a PHID list field ("Project tags"), saw text label say "include none of". Maniphest Tasks: T13445 Differential Revision: https://secure.phabricator.com/D20889 --- src/applications/herald/adapter/HeraldAdapter.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 2832c6d9f4..70f6e3d5cb 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -399,7 +399,7 @@ abstract class HeraldAdapter extends Phobject { self::CONDITION_IS_NOT_ANY => pht('is not any of'), self::CONDITION_INCLUDE_ALL => pht('include all of'), self::CONDITION_INCLUDE_ANY => pht('include any of'), - self::CONDITION_INCLUDE_NONE => pht('do not include'), + self::CONDITION_INCLUDE_NONE => pht('include none of'), self::CONDITION_IS_ME => pht('is myself'), self::CONDITION_IS_NOT_ME => pht('is not myself'), self::CONDITION_REGEXP => pht('matches regexp'), From f5f2a0bc5696372945465b421d0026a9dd8b7a54 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 6 Nov 2019 08:05:41 -0800 Subject: [PATCH 02/13] Allow username changes which modify letter case to go through as valid Summary: Fixes T13446. Currently, the validation logic here rejects a rename like "alice" to "ALICE" (which changes only letter case) but this is a permissible rename. Allow collisions that collide with the same user to permit this rename. Also, fix an issue where an empty rename was treated improperly. Test Plan: - Renamed "alice" to "ALICE". - Before: username collision error. - After: clean rename. - Renamed "alice" to "orange" (an existing user). Got an error. - Renamed "alice" to "", "!@#$", etc (invalid usernames). Got sensible errors. Maniphest Tasks: T13446 Differential Revision: https://secure.phabricator.com/D20890 --- .../PhabricatorUserUsernameTransaction.php | 21 +++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php index 338b296335..e30a131cff 100644 --- a/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php +++ b/src/applications/people/xaction/PhabricatorUserUsernameTransaction.php @@ -71,21 +71,30 @@ final class PhabricatorUserUsernameTransaction } if (!strlen($new)) { - $errors[] = $this->newRequiredError( - pht('New username is required.'), $xaction); + $errors[] = $this->newInvalidError( + pht('New username is required.'), + $xaction); } else if (!PhabricatorUser::validateUsername($new)) { $errors[] = $this->newInvalidError( - PhabricatorUser::describeValidUsername(), $xaction); + PhabricatorUser::describeValidUsername(), + $xaction); } $user = id(new PhabricatorPeopleQuery()) ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withUsernames(array($new)) ->executeOne(); - if ($user) { - $errors[] = $this->newInvalidError( - pht('Another user already has that username.'), $xaction); + // See T13446. We may be changing the letter case of a username, which + // is a perfectly fine edit. + $is_self = ($user->getPHID() === $object->getPHID()); + if (!$is_self) { + $errors[] = $this->newInvalidError( + pht( + 'Another user already has the username "%s".', + $new), + $xaction); + } } } From 9dbde24dda723959d43c79ea5069eb91db9024b8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Nov 2019 15:26:12 -0800 Subject: [PATCH 03/13] Manually prune Git working copy refs instead of using "--prune", to improve "Fetch Refs" behavior Summary: Ref T13448. When "Fetch Refs" is configured: - We switch to a narrow mode when running "ls-remote" against the local working copy. This excludes surplus refs, so we'll incorrectly detect that the local and remote working copies are identical in cases where the local working copy really has surplus refs. - We rely on "--prune" to remove surplus local refs, but it only prunes refs matching the refspecs we pass "git fetch". Since these refspecs are very narrow under "Fetch Only", the pruning behavior is also very narrow. Instead: - When listing local refs, always list everything. If we have too much stuff locally, we want to get rid of it. - When we identify surplus local refs, explicitly delete them instead of relying on "--prune". We can just do this in all cases so we don't have separate "--prune" and "manual" cases. Test Plan: - Created a new repository, observed from a GitHub repository, with many tags/refs/branches. Pulled it. - Observed lots of refs in `git for-each-ref`. - Changed "Fetch Refs" to "refs/heads/master". - Ran `bin/repository pull X --trace --verbose`. On deciding to do something: - Before: since "master" did not change, the pull declined to act. - After: the pull detected surplus refs and deleted them. Since the states then matched, it declined further action. On pruning: - Before: if the pull was forced to act, it ran "fetch --prune" with a narrow refspec, which did not prune the working copy. - After: saw working copy pruned explicitly with "update-ref -d" commands. Also, set "Fetch Refs" back to the default (empty) and pulled, saw everything pull. Maniphest Tasks: T13448 Differential Revision: https://secure.phabricator.com/D20892 --- .../PhabricatorRepositoryPullEngine.php | 84 +++++++++++++++---- 1 file changed, 69 insertions(+), 15 deletions(-) diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index ea70f380aa..a7302341f4 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -353,13 +353,56 @@ final class PhabricatorRepositoryPullEngine // Load the refs we're planning to fetch from the remote repository. $remote_refs = $this->loadGitRemoteRefs( $repository, - $repository->getRemoteURIEnvelope()); + $repository->getRemoteURIEnvelope(), + $is_local = false); // Load the refs we're planning to fetch from the local repository, by // using the local working copy path as the "remote" repository URI. $local_refs = $this->loadGitRemoteRefs( $repository, - new PhutilOpaqueEnvelope($path)); + new PhutilOpaqueEnvelope($path), + $is_local = true); + + // See T13448. The "git fetch --prune ..." flag only prunes local refs + // matching the refspecs we pass it. If "Fetch Refs" is configured, we'll + // pass it a very narrow list of refspecs, and it won't prune older refs + // that aren't currently subject to fetching. + + // Since we want to prune everything that isn't (a) on the fetch list and + // (b) in the remote, handle pruning of any surplus leftover refs ourselves + // before we fetch anything. + + // (We don't have to do this if "Fetch Refs" isn't set up, since "--prune" + // will work in that case, but it's a little simpler to always go down the + // same code path.) + + $surplus_refs = array(); + foreach ($local_refs as $local_ref => $local_hash) { + $remote_hash = idx($remote_refs, $local_ref); + if ($remote_hash === null) { + $surplus_refs[] = $local_ref; + } + } + + if ($surplus_refs) { + $this->log( + pht( + 'Found %s surplus local ref(s) to delete.', + phutil_count($surplus_refs))); + foreach ($surplus_refs as $surplus_ref) { + $this->log( + pht( + 'Deleting surplus local ref "%s" ("%s").', + $surplus_ref, + $local_refs[$surplus_ref])); + + $repository->execLocalCommand( + 'update-ref -d %R --', + $surplus_ref); + + unset($local_refs[$surplus_ref]); + } + } if ($remote_refs === $local_refs) { $this->log( @@ -378,7 +421,7 @@ final class PhabricatorRepositoryPullEngine // checked out. See T13280. $future = $repository->getRemoteCommandFuture( - 'fetch --prune --update-head-ok -- %P %Ls', + 'fetch --update-head-ok -- %P %Ls', $repository->getRemoteURIEnvelope(), $fetch_rules); @@ -474,21 +517,32 @@ final class PhabricatorRepositoryPullEngine private function loadGitRemoteRefs( PhabricatorRepository $repository, - PhutilOpaqueEnvelope $remote_envelope) { + PhutilOpaqueEnvelope $remote_envelope, + $is_local) { - $ref_rules = $this->getGitRefRules($repository); + // See T13448. When listing local remotes, we want to list everything, + // not just refs we expect to fetch. This allows us to detect that we have + // undesirable refs (which have been deleted in the remote, but are still + // present locally) so we can update our state to reflect the correct + // remote state. - // NOTE: "git ls-remote" does not support "--" until circa January 2016. - // See T12416. None of the flags to "ls-remote" appear dangerous, but - // refuse to list any refs beginning with "-" just in case. + if ($is_local) { + $ref_rules = array(); + } else { + $ref_rules = $this->getGitRefRules($repository); - foreach ($ref_rules as $ref_rule) { - if (preg_match('/^-/', $ref_rule)) { - throw new Exception( - pht( - 'Refusing to list potentially dangerous ref ("%s") beginning '. - 'with "-".', - $ref_rule)); + // NOTE: "git ls-remote" does not support "--" until circa January 2016. + // See T12416. None of the flags to "ls-remote" appear dangerous, but + // refuse to list any refs beginning with "-" just in case. + + foreach ($ref_rules as $ref_rule) { + if (preg_match('/^-/', $ref_rule)) { + throw new Exception( + pht( + 'Refusing to list potentially dangerous ref ("%s") beginning '. + 'with "-".', + $ref_rule)); + } } } From 8dd57a1ed221f592579dd925f22979047d289e78 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Nov 2019 15:41:50 -0800 Subject: [PATCH 04/13] When fetching Git repositories, pass "--no-tags" to make explicit "Fetch Refs" operate more narrowly Summary: Ref T13448. The default behavior of "git fetch '+refs/heads/master:refs/heads/master'" is to follow and fetch associated tags. We don't want this when we pass a narrow refspec because of "Fetch Refs" configuration. Tell Git that we only want the refs we explicitly passed. Note that this doesn't prevent us from fetching tags (if the refspec specifies them), it just stops us from fetching extra tags that aren't part of the refspec. Test Plan: - Ran "bin/repository pull X --trace --verbose" in a repository with a "Fetch Refs" configuration, saw only "master" get fetched (previously: "master" and reachable tags). - Ran "git fetch --no-tags '+refs/*:refs/*'", saw tags fetched normally ("--no-tags" does not disable fetching tags). Maniphest Tasks: T13448 Differential Revision: https://secure.phabricator.com/D20893 --- .../repository/engine/PhabricatorRepositoryPullEngine.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index a7302341f4..c228932ac8 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -421,7 +421,7 @@ final class PhabricatorRepositoryPullEngine // checked out. See T13280. $future = $repository->getRemoteCommandFuture( - 'fetch --update-head-ok -- %P %Ls', + 'fetch --no-tags --update-head-ok -- %P %Ls', $repository->getRemoteURIEnvelope(), $fetch_rules); From 1b40f7e5408b7865677f16cb20dcec87a6cd8545 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Nov 2019 16:04:53 -0800 Subject: [PATCH 05/13] Always initialize Git repositories with "git init", never with "git clone" Summary: Fixes T13448. We currently "git clone" to initialize repositories, but this will fetch too many refs if "Fetch Refs" is configured. In modern Phabricator, there's no apparent reason to "git clone"; we can just "git init" instead. This workflow naturally falls through to an update, where we'll do a "git fetch" and pull in exactly the refs we want. Test Plan: - Configured an observed repository with "Fetch Refs". - Destroyed the working copy. - Ran "bin/repository pull X --trace --verbose". - Before: saw "git clone" pull in the world. - After: saw "git init" create a bare empty working copy, then "git fetch" fill it surgically. Both flows end up in the same place, this one is just simpler and does less work. Maniphest Tasks: T13448 Differential Revision: https://secure.phabricator.com/D20894 --- .../PhabricatorRepositoryPullEngine.php | 54 +++++++++---------- 1 file changed, 25 insertions(+), 29 deletions(-) diff --git a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php index c228932ac8..2b008b630b 100644 --- a/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php +++ b/src/applications/repository/engine/PhabricatorRepositoryPullEngine.php @@ -257,16 +257,15 @@ final class PhabricatorRepositoryPullEngine $path = rtrim($repository->getLocalPath(), '/'); - if ($repository->isHosted()) { - $repository->execxRemoteCommand( - 'init --bare -- %s', - $path); - } else { - $repository->execxRemoteCommand( - 'clone --bare -- %P %s', - $repository->getRemoteURIEnvelope(), - $path); - } + // See T13448. In all cases, we create repositories by using "git init" + // to build a bare, empty working copy. If we try to use "git clone" + // instead, we'll pull in too many refs if "Fetch Refs" is also + // configured. There's no apparent way to make "git clone" behave narrowly + // and no apparent reason to bother. + + $repository->execxRemoteCommand( + 'init --bare -- %s', + $path); } @@ -290,29 +289,27 @@ final class PhabricatorRepositoryPullEngine $files = Filesystem::listDirectory($path, $include_hidden = true); if (!$files) { $message = pht( - "Expected to find a git repository at '%s', but there ". - "is an empty directory there. Remove the directory: the daemon ". - "will run '%s' for you.", - $path, - 'git clone'); + 'Expected to find a Git repository at "%s", but there is an '. + 'empty directory there. Remove the directory. A daemon will '. + 'construct the working copy for you.', + $path); } else { $message = pht( - "Expected to find a git repository at '%s', but there is ". - "a non-repository directory (with other stuff in it) there. Move ". - "or remove this directory (or reconfigure the repository to use a ". - "different directory), and then either clone a repository ". - "yourself or let the daemon do it.", + 'Expected to find a Git repository at "%s", but there is '. + 'a non-repository directory (with other stuff in it) there. '. + 'Move or remove this directory. A daemon will construct '. + 'the working copy for you.', $path); } } else if (is_file($path)) { $message = pht( - "Expected to find a git repository at '%s', but there is a ". - "file there instead. Remove it and let the daemon clone a ". - "repository for you.", + 'Expected to find a Git repository at "%s", but there is a '. + 'file there instead. Move or remove this file. A daemon will '. + 'construct the working copy for you.', $path); } else { $message = pht( - "Expected to find a git repository at '%s', but did not.", + 'Expected to find a git repository at "%s", but did not.', $path); } } else { @@ -327,11 +324,10 @@ final class PhabricatorRepositoryPullEngine } else if (!Filesystem::pathsAreEquivalent($repo_path, $path)) { $err = true; $message = pht( - "Expected to find repo at '%s', but the actual git repository root ". - "for this directory is '%s'. Something is misconfigured. ". - "The repository's 'Local Path' should be set to some place where ". - "the daemon can check out a working copy, ". - "and should not be inside another git repository.", + 'Expected to find a Git repository at "%s", but the actual Git '. + 'repository root for this directory is "%s". Something is '. + 'misconfigured. This directory should be writable by the daemons '. + 'and not inside another Git repository.', $path, $repo_path); } From 502ca4767e4b4ce5d06d83e2889c3696e2df22ef Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Nov 2019 16:24:49 -0800 Subject: [PATCH 06/13] When "Fetch Refs" are configured for a repository, highlight the "Branches" menu item in the Diffusion Management UI Summary: Ref T13448. Minor UI issue: setting a "Fetch Refs" value does not highlight the associated menu item, but should. Test Plan: Set only "Fetch Refs", now saw menu item highlighted. Maniphest Tasks: T13448 Differential Revision: https://secure.phabricator.com/D20895 --- .../management/DiffusionRepositoryBranchesManagementPanel.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php index 43b4d31252..e24bae7da4 100644 --- a/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php +++ b/src/applications/diffusion/management/DiffusionRepositoryBranchesManagementPanel.php @@ -23,6 +23,7 @@ final class DiffusionRepositoryBranchesManagementPanel $has_any = $repository->getDetail('default-branch') || + $repository->getFetchRules() || $repository->getTrackOnlyRules() || $repository->getPermanentRefRules(); From d4491ddc225e8f02015b6f475d842d58e9b55c50 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 Nov 2019 07:33:16 -0800 Subject: [PATCH 07/13] Fix an issue with 1up diff block rendering for added or removed blocks Summary: Ref T13425. When a change adds or removes a block (vs adding or removing a document, or changing a block), we currently try to render `null` as cell content in the unified view. Make this check broader to catch both "no opposite document" and "no opposite cell". Test Plan: {F7008772} Subscribers: artms Maniphest Tasks: T13425 Differential Revision: https://secure.phabricator.com/D20897 --- .../render/DifferentialChangesetOneUpRenderer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php index 19c939274d..b3e6520fd9 100644 --- a/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php +++ b/src/applications/differential/render/DifferentialChangesetOneUpRenderer.php @@ -371,7 +371,7 @@ final class DifferentialChangesetOneUpRenderer $cell_classes = $block_diff->getNewClasses(); } } else if ($row_type === 'old') { - if (!$old_ref) { + if (!$old_ref || !$old) { continue; } @@ -384,7 +384,7 @@ final class DifferentialChangesetOneUpRenderer $new_key = null; } else if ($row_type === 'new') { - if (!$new_ref) { + if (!$new_ref || !$new) { continue; } From 338b4cb2e70950c1cd4795d0dbb79eca9567f18d Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 Nov 2019 08:17:45 -0800 Subject: [PATCH 08/13] Prevent workboard cards from being grabbed by the "Txxx" object name text Summary: Fixes T13452. We currently give users mixed signals about the interaction mode of this text: the cursor says "text" but the behavior is "grab". Make the behavior "text" to align with the cursor. An alternate variation of this change is to remove the cursor, but this is preferable if it doesn't cause problems, since copying the task ID is at least somewhat useful. Test Plan: In Safari, Firefox, and Chrome: selected and copied object names from workboard cards; and dragged workboard cards by other parts of their UI. Maniphest Tasks: T13452 Differential Revision: https://secure.phabricator.com/D20898 --- resources/celerity/map.php | 22 +++++++++++----------- src/view/phui/PHUIObjectItemView.php | 3 ++- webroot/rsrc/js/core/DraggableList.js | 9 +++++++++ 3 files changed, 22 insertions(+), 12 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 8547f8a0a3..ffe4024ed7 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '77de226f', - 'core.pkg.js' => '6e5c894f', + 'core.pkg.js' => '705aec2c', 'differential.pkg.css' => '607c84be', 'differential.pkg.js' => '1b97518d', 'diffusion.pkg.css' => '42c75c37', @@ -448,7 +448,7 @@ return array( 'rsrc/js/application/uiexample/notification-example.js' => '29819b75', 'rsrc/js/core/Busy.js' => '5202e831', 'rsrc/js/core/DragAndDropFileUpload.js' => '4370900d', - 'rsrc/js/core/DraggableList.js' => 'c9ad6f70', + 'rsrc/js/core/DraggableList.js' => '0169e425', 'rsrc/js/core/Favicon.js' => '7930776a', 'rsrc/js/core/FileUpload.js' => 'ab85e184', 'rsrc/js/core/Hovercard.js' => '074f0783', @@ -777,7 +777,7 @@ return array( 'phabricator-diff-changeset-list' => '0f5c016d', 'phabricator-diff-inline' => 'a4a14a94', 'phabricator-drag-and-drop-file-upload' => '4370900d', - 'phabricator-draggable-list' => 'c9ad6f70', + 'phabricator-draggable-list' => '0169e425', 'phabricator-fatal-config-template-css' => '20babf50', 'phabricator-favicon' => '7930776a', 'phabricator-feed-css' => 'd8b6e3f8', @@ -920,6 +920,14 @@ return array( 'javelin-uri', 'phabricator-notification', ), + '0169e425' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + 'javelin-vector', + 'javelin-magical-init', + ), '022516b4' => array( 'javelin-install', 'javelin-util', @@ -2032,14 +2040,6 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut-manager', ), - 'c9ad6f70' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - 'javelin-vector', - 'javelin-magical-init', - ), 'cf32921f' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/view/phui/PHUIObjectItemView.php b/src/view/phui/PHUIObjectItemView.php index 05747c7ce6..b5ad5a7fd6 100644 --- a/src/view/phui/PHUIObjectItemView.php +++ b/src/view/phui/PHUIObjectItemView.php @@ -379,10 +379,11 @@ final class PHUIObjectItemView extends AphrontTagView { if ($this->objectName) { $header_name[] = array( - phutil_tag( + javelin_tag( 'span', array( 'class' => 'phui-oi-objname', + 'sigil' => 'ungrabbable', ), $this->objectName), ' ', diff --git a/webroot/rsrc/js/core/DraggableList.js b/webroot/rsrc/js/core/DraggableList.js index 5f19b7061d..8930f43f94 100644 --- a/webroot/rsrc/js/core/DraggableList.js +++ b/webroot/rsrc/js/core/DraggableList.js @@ -181,6 +181,15 @@ JX.install('DraggableList', { return; } + // See T13452. If this is an ungrabble part of the item, don't start a + // drag. We use this to allow users to select text on cards. + var target = e.getTarget(); + if (target) { + if (JX.Stratcom.hasSigil(target, 'ungrabbable')) { + return; + } + } + if (JX.Stratcom.pass()) { // Let other handlers deal with this event before we do. return; From 2223d6b914678b402e2049bf321492a7ca8e9d59 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 Nov 2019 08:46:37 -0800 Subject: [PATCH 09/13] Update Asana Auth adapter for "gid" API changes Summary: Ref T13453. The Asana API has changed, replacing all "id" fields with "gid", including the "users/me" API call result. Test Plan: Linked an Asana account. Before: error about missing 'id'. After: clean link. Maniphest Tasks: T13453 Differential Revision: https://secure.phabricator.com/D20899 --- src/applications/auth/adapter/PhutilAsanaAuthAdapter.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/applications/auth/adapter/PhutilAsanaAuthAdapter.php b/src/applications/auth/adapter/PhutilAsanaAuthAdapter.php index 5d9a9ec478..5fe343671e 100644 --- a/src/applications/auth/adapter/PhutilAsanaAuthAdapter.php +++ b/src/applications/auth/adapter/PhutilAsanaAuthAdapter.php @@ -14,7 +14,9 @@ final class PhutilAsanaAuthAdapter extends PhutilOAuthAuthAdapter { } public function getAccountID() { - return $this->getOAuthAccountData('id'); + // See T13453. The Asana API has changed to string IDs and now returns a + // "gid" field (previously, it returned an "id" field). + return $this->getOAuthAccountData('gid'); } public function getAccountEmail() { From cd60a8aa563bfc733a0bccde451bc3974a688418 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 Nov 2019 08:57:35 -0800 Subject: [PATCH 10/13] Update various Asana odds-and-ends for "gid" API changes Summary: Ref T13453. Some of the Asana integrations also need API updates. Depends on D20899. Test Plan: - Viewed "asana.workspace-id" in Config, got a sensible GID list. - Created a revision, saw the associated Asana task get assigned. - Pasted an Asana link I could view into a revision description, saw it Doorkeeper in the metadata. Maniphest Tasks: T13453 Differential Revision: https://secure.phabricator.com/D20900 --- .../doorkeeper/bridge/DoorkeeperBridgeAsana.php | 7 +++++-- .../doorkeeper/option/PhabricatorAsanaConfigOptions.php | 5 ++++- .../doorkeeper/worker/DoorkeeperAsanaFeedWorker.php | 8 ++++---- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php index 05ee786337..59c3e4026d 100644 --- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php +++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php @@ -123,8 +123,11 @@ final class DoorkeeperBridgeAsana extends DoorkeeperBridge { } public function fillObjectFromData(DoorkeeperExternalObject $obj, $result) { - $id = $result['id']; - $uri = "https://app.asana.com/0/{$id}/{$id}"; + $gid = $result['gid']; + $uri = urisprintf( + 'https://app.asana.com/0/%s/%s', + $gid, + $gid); $obj->setObjectURI($uri); } diff --git a/src/applications/doorkeeper/option/PhabricatorAsanaConfigOptions.php b/src/applications/doorkeeper/option/PhabricatorAsanaConfigOptions.php index 1771a6615e..3a9c9abac5 100644 --- a/src/applications/doorkeeper/option/PhabricatorAsanaConfigOptions.php +++ b/src/applications/doorkeeper/option/PhabricatorAsanaConfigOptions.php @@ -102,7 +102,10 @@ final class PhabricatorAsanaConfigOptions pht('Workspace Name')); $out[] = '| ------------ | -------------- |'; foreach ($workspaces as $workspace) { - $out[] = sprintf('| `%s` | `%s` |', $workspace['id'], $workspace['name']); + $out[] = sprintf( + '| `%s` | `%s` |', + $workspace['gid'], + $workspace['name']); } $out = implode("\n", $out); diff --git a/src/applications/doorkeeper/worker/DoorkeeperAsanaFeedWorker.php b/src/applications/doorkeeper/worker/DoorkeeperAsanaFeedWorker.php index 1d293956b3..00b75b7a56 100644 --- a/src/applications/doorkeeper/worker/DoorkeeperAsanaFeedWorker.php +++ b/src/applications/doorkeeper/worker/DoorkeeperAsanaFeedWorker.php @@ -358,7 +358,7 @@ final class DoorkeeperAsanaFeedWorker extends DoorkeeperFeedWorker { 'POST', $subtask_data + array( 'assignee' => $phid_aid_map[$user_phid], - 'completed' => $is_completed, + 'completed' => (int)$is_completed, 'parent' => $parent_ref->getObjectID(), )); @@ -393,7 +393,7 @@ final class DoorkeeperAsanaFeedWorker extends DoorkeeperFeedWorker { 'PUT', $subtask_data + array( 'assignee' => $phid_aid_map[$user_phid], - 'completed' => $is_completed, + 'completed' => (int)$is_completed, )); } @@ -484,7 +484,7 @@ final class DoorkeeperAsanaFeedWorker extends DoorkeeperFeedWorker { return array( 'name' => $title, 'notes' => $notes, - 'completed' => $is_completed, + 'completed' => (int)$is_completed, ); } @@ -632,7 +632,7 @@ final class DoorkeeperAsanaFeedWorker extends DoorkeeperFeedWorker { ->setApplicationType(DoorkeeperBridgeAsana::APPTYPE_ASANA) ->setApplicationDomain(DoorkeeperBridgeAsana::APPDOMAIN_ASANA) ->setObjectType($type) - ->setObjectID($result['id']) + ->setObjectID($result['gid']) ->setIsVisible(true); $xobj = $ref->newExternalObject(); From b83b3224bb756a2c2e29727df01b1c419f2c832b Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 1 Nov 2019 10:13:13 -0700 Subject: [PATCH 11/13] Add an "Advanced/Developer..." action item for viewing object handle details and hovercards Summary: Ref T13442. Ref T13157. There's a secret URI to look at an object's hovercard in a standalone view, but it's hard to remember and impossible to discover. In developer mode, add an action to "View Hovercard". Also add "View Handle", which primarily shows the object PHID. Test Plan: Viewed some objects, saw "Advanced/Developer...". Used "View Hovercard" to view hovercards and "View Handle" to view handles. Maniphest Tasks: T13442, T13157 Differential Revision: https://secure.phabricator.com/D20887 --- src/__phutil_library_map__.php | 4 + .../PhabricatorSearchApplication.php | 2 + .../PhabricatorSearchHandleController.php | 89 +++++++++++++++++++ .../PhabricatorSystemApplication.php | 6 ++ .../PhabricatorSystemDebugUIEventListener.php | 58 ++++++++++++ src/view/layout/PhabricatorActionListView.php | 8 ++ 6 files changed, 167 insertions(+) create mode 100644 src/applications/search/controller/PhabricatorSearchHandleController.php create mode 100644 src/applications/system/events/PhabricatorSystemDebugUIEventListener.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2db41f0a69..bb6aeb20a1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4648,6 +4648,7 @@ phutil_register_library_map(array( 'PhabricatorSearchEngineExtensionModule' => 'applications/search/engineextension/PhabricatorSearchEngineExtensionModule.php', 'PhabricatorSearchFerretNgramGarbageCollector' => 'applications/search/garbagecollector/PhabricatorSearchFerretNgramGarbageCollector.php', 'PhabricatorSearchField' => 'applications/search/field/PhabricatorSearchField.php', + 'PhabricatorSearchHandleController' => 'applications/search/controller/PhabricatorSearchHandleController.php', 'PhabricatorSearchHost' => 'infrastructure/cluster/search/PhabricatorSearchHost.php', 'PhabricatorSearchHovercardController' => 'applications/search/controller/PhabricatorSearchHovercardController.php', 'PhabricatorSearchIndexVersion' => 'applications/search/storage/PhabricatorSearchIndexVersion.php', @@ -4872,6 +4873,7 @@ phutil_register_library_map(array( 'PhabricatorSystemActionRateLimitException' => 'applications/system/exception/PhabricatorSystemActionRateLimitException.php', 'PhabricatorSystemApplication' => 'applications/system/application/PhabricatorSystemApplication.php', 'PhabricatorSystemDAO' => 'applications/system/storage/PhabricatorSystemDAO.php', + 'PhabricatorSystemDebugUIEventListener' => 'applications/system/events/PhabricatorSystemDebugUIEventListener.php', 'PhabricatorSystemDestructionGarbageCollector' => 'applications/system/garbagecollector/PhabricatorSystemDestructionGarbageCollector.php', 'PhabricatorSystemDestructionLog' => 'applications/system/storage/PhabricatorSystemDestructionLog.php', 'PhabricatorSystemObjectController' => 'applications/system/controller/PhabricatorSystemObjectController.php', @@ -11270,6 +11272,7 @@ phutil_register_library_map(array( 'PhabricatorSearchEngineExtensionModule' => 'PhabricatorConfigModule', 'PhabricatorSearchFerretNgramGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorSearchField' => 'Phobject', + 'PhabricatorSearchHandleController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchHost' => 'Phobject', 'PhabricatorSearchHovercardController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchIndexVersion' => 'PhabricatorSearchDAO', @@ -11511,6 +11514,7 @@ phutil_register_library_map(array( 'PhabricatorSystemActionRateLimitException' => 'Exception', 'PhabricatorSystemApplication' => 'PhabricatorApplication', 'PhabricatorSystemDAO' => 'PhabricatorLiskDAO', + 'PhabricatorSystemDebugUIEventListener' => 'PhabricatorEventListener', 'PhabricatorSystemDestructionGarbageCollector' => 'PhabricatorGarbageCollector', 'PhabricatorSystemDestructionLog' => 'PhabricatorSystemDAO', 'PhabricatorSystemObjectController' => 'PhabricatorController', diff --git a/src/applications/search/application/PhabricatorSearchApplication.php b/src/applications/search/application/PhabricatorSearchApplication.php index 3cf5923b9c..7547506258 100644 --- a/src/applications/search/application/PhabricatorSearchApplication.php +++ b/src/applications/search/application/PhabricatorSearchApplication.php @@ -33,6 +33,8 @@ final class PhabricatorSearchApplication extends PhabricatorApplication { 'index/(?P[^/]+)/' => 'PhabricatorSearchIndexController', 'hovercard/' => 'PhabricatorSearchHovercardController', + 'handle/(?P[^/]+)/' + => 'PhabricatorSearchHandleController', 'edit/' => array( 'key/(?P[^/]+)/' => 'PhabricatorSearchEditController', 'id/(?P[^/]+)/' => 'PhabricatorSearchEditController', diff --git a/src/applications/search/controller/PhabricatorSearchHandleController.php b/src/applications/search/controller/PhabricatorSearchHandleController.php new file mode 100644 index 0000000000..751b4e367d --- /dev/null +++ b/src/applications/search/controller/PhabricatorSearchHandleController.php @@ -0,0 +1,89 @@ +getViewer(); + $phid = $request->getURIData('phid'); + + $handles = $viewer->loadHandles(array($phid)); + $handle = $handles[$phid]; + + $cancel_uri = $handle->getURI(); + if (!$cancel_uri) { + $cancel_uri = '/'; + } + + $rows = array(); + + $rows[] = array( + pht('PHID'), + $phid, + ); + + $rows[] = array( + pht('PHID Type'), + phid_get_type($phid), + ); + + $rows[] = array( + pht('URI'), + $handle->getURI(), + ); + + $icon = $handle->getIcon(); + if ($icon !== null) { + $icon = id(new PHUIIconView()) + ->setIcon($handle->getIcon()); + } + + $rows[] = array( + pht('Icon'), + $icon, + ); + + $rows[] = array( + pht('Object Name'), + $handle->getObjectName(), + ); + + $rows[] = array( + pht('Name'), + $handle->getName(), + ); + + $rows[] = array( + pht('Full Name'), + $handle->getFullName(), + ); + + $rows[] = array( + pht('Tag'), + $handle->renderTag(), + ); + + $rows[] = array( + pht('Link'), + $handle->renderLink(), + ); + + $table = id(new AphrontTableView($rows)) + ->setColumnClasses( + array( + 'header', + 'wide', + )); + + return $this->newDialog() + ->setTitle(pht('Handle: %s', $phid)) + ->setWidth(AphrontDialogView::WIDTH_FORM) + ->appendChild($table) + ->addCancelButton($cancel_uri, pht('Done')); + } + +} diff --git a/src/applications/system/application/PhabricatorSystemApplication.php b/src/applications/system/application/PhabricatorSystemApplication.php index b6cc13050f..88f07ae17c 100644 --- a/src/applications/system/application/PhabricatorSystemApplication.php +++ b/src/applications/system/application/PhabricatorSystemApplication.php @@ -14,6 +14,12 @@ final class PhabricatorSystemApplication extends PhabricatorApplication { return true; } + public function getEventListeners() { + return array( + new PhabricatorSystemDebugUIEventListener(), + ); + } + public function getRoutes() { return array( '/status/' => 'PhabricatorStatusController', diff --git a/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php b/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php new file mode 100644 index 0000000000..18b94323b6 --- /dev/null +++ b/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php @@ -0,0 +1,58 @@ +listen(PhabricatorEventType::TYPE_UI_DIDRENDERACTIONS); + } + + public function handleEvent(PhutilEvent $event) { + switch ($event->getType()) { + case PhabricatorEventType::TYPE_UI_DIDRENDERACTIONS: + $this->handleActionEvent($event); + break; + } + } + + private function handleActionEvent($event) { + $viewer = $event->getUser(); + $object = $event->getValue('object'); + + if (!PhabricatorEnv::getEnvConfig('phabricator.developer-mode')) { + return; + } + + if (!$object || !$object->getPHID()) { + // If we have no object, or the object doesn't have a PHID, we can't + // do anything useful. + return; + } + + $phid = $object->getPHID(); + + $submenu = array(); + + $submenu[] = id(new PhabricatorActionView()) + ->setIcon('fa-asterisk') + ->setName(pht('View Handle')) + ->setHref(urisprintf('/search/handle/%s/', $phid)) + ->setWorkflow(true); + + $submenu[] = id(new PhabricatorActionView()) + ->setIcon('fa-address-card-o') + ->setName(pht('View Hovercard')) + ->setHref(urisprintf('/search/hovercard/?phids[]=%s', $phid)); + + $developer_action = id(new PhabricatorActionView()) + ->setName(pht('Advanced/Developer...')) + ->setIcon('fa-magic') + ->setOrder(9001) + ->setSubmenu($submenu); + + $actions = $event->getValue('actions'); + $actions[] = $developer_action; + $event->setValue('actions', $actions); + } + +} diff --git a/src/view/layout/PhabricatorActionListView.php b/src/view/layout/PhabricatorActionListView.php index 134c336735..22e995ab64 100644 --- a/src/view/layout/PhabricatorActionListView.php +++ b/src/view/layout/PhabricatorActionListView.php @@ -52,6 +52,14 @@ final class PhabricatorActionListView extends AphrontTagView { $action->setViewer($viewer); } + $sort = array(); + foreach ($actions as $key => $action) { + $sort[$key] = id(new PhutilSortVector()) + ->addInt($action->getOrder()); + } + $sort = msortv($sort, 'getSelf'); + $actions = array_select_keys($actions, array_keys($sort)); + require_celerity_resource('phabricator-action-list-view-css'); $items = array(); From a3f4cbd7484b591425e83bbfc7642bccb04d0d57 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 Nov 2019 16:53:11 -0800 Subject: [PATCH 12/13] Correct rendering of workboard column move stories when a single transaction performs moves on multiple boards Summary: See . If a single transaction performs column moves on multiple different boards (which is permitted in the API), the rendering logic currently fails. Make it render properly. Test Plan: {F7011464} Differential Revision: https://secure.phabricator.com/D20901 --- .../transactions/storage/PhabricatorApplicationTransaction.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 7e65ac0a09..1ec29557da 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -1133,6 +1133,8 @@ abstract class PhabricatorApplicationTransaction } else { $fragments = array(); foreach ($moves as $move) { + $to_column = $move['columnPHID']; + $board_phid = $move['boardPHID']; $fragments[] = pht( '%s (%s)', $this->renderHandleLink($board_phid), From 72f82abe0723a866856fa871f24a5e40ad9642bc Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 8 Nov 2019 17:08:44 -0800 Subject: [PATCH 13/13] Improve recovery from panel action rendering exceptions, and mark "Changeset" queries as not suitable for panel generation Summary: Fixes T13443. When a panel raises an exception during edit action generation, it currently escapes to top level. Instead, catch it more narrowly. Additionally, mark "ChangesetSearchEngine" as not being a suitable search engine for use in query panels. There's no list view or search URI so it can't generate a sensible panel. Test Plan: - Added a changeset panel to a dashboard. - Before: entire dashboard fataled. - After: panel fataled narrowly, menu fatals narrowly, dashboard no longer permits creation of another Changeset query panel. Maniphest Tasks: T13443 Differential Revision: https://secure.phabricator.com/D20902 --- .../PhabricatorDashboardPanelRenderingEngine.php | 13 ++++++++++--- .../query/DifferentialChangesetSearchEngine.php | 4 ++++ 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php b/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php index 29a710209e..8969151c06 100644 --- a/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php +++ b/src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php @@ -329,9 +329,16 @@ final class PhabricatorDashboardPanelRenderingEngine extends Phobject { $actions = array(); if ($panel) { - $panel_actions = $panel->newHeaderEditActions( - $viewer, - $context_phid); + try { + $panel_actions = $panel->newHeaderEditActions( + $viewer, + $context_phid); + } catch (Exception $ex) { + $error_action = id(new PhabricatorActionView()) + ->setIcon('fa-exclamation-triangle red') + ->setName(pht('')); + $panel_actions[] = $error_action; + } if ($panel_actions) { foreach ($panel_actions as $panel_action) { diff --git a/src/applications/differential/query/DifferentialChangesetSearchEngine.php b/src/applications/differential/query/DifferentialChangesetSearchEngine.php index 0dfec94a53..3fe8957971 100644 --- a/src/applications/differential/query/DifferentialChangesetSearchEngine.php +++ b/src/applications/differential/query/DifferentialChangesetSearchEngine.php @@ -22,6 +22,10 @@ final class DifferentialChangesetSearchEngine return 'PhabricatorDifferentialApplication'; } + public function canUseInPanelContext() { + return false; + } + public function newQuery() { $query = id(new DifferentialChangesetQuery());