From 292f8fc612bd2d070110eb22877454937bfd7899 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 26 Oct 2019 12:06:45 -0700 Subject: [PATCH 01/15] Fix an issue where added or removed source files could incorrectly select a DocumentEngine Summary: Ref T13425. The changes in D20865 could incorrectly lead to selection of a DocumentEngine that can not generate document diffs if a file was added or removed (for example, when a source file is added). Move the engine pruning code to be shared -- we should always discard engines which can't generate a diff, even if we don't have both documents. Test Plan: Viewed an added source file, no more document ref error arising from document engine selection. Maniphest Tasks: T13425 Differential Revision: https://secure.phabricator.com/D20866 --- .../parser/DifferentialChangesetParser.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php index 571405f64b..92d5f23c1a 100644 --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -1743,12 +1743,6 @@ final class DifferentialChangesetParser extends Phobject { if ($new_engines !== null && $old_engines !== null) { $shared_engines = array_intersect_key($new_engines, $old_engines); $default_engine = head_key($new_engines); - - foreach ($shared_engines as $key => $shared_engine) { - if (!$shared_engine->canDiffDocuments($old_ref, $new_ref)) { - unset($shared_engines[$key]); - } - } } else if ($new_engines !== null) { $shared_engines = $new_engines; $default_engine = head_key($shared_engines); @@ -1759,6 +1753,12 @@ final class DifferentialChangesetParser extends Phobject { return null; } + foreach ($shared_engines as $key => $shared_engine) { + if (!$shared_engine->canDiffDocuments($old_ref, $new_ref)) { + unset($shared_engines[$key]); + } + } + $engine_key = $this->getDocumentEngineKey(); if (strlen($engine_key)) { if (isset($shared_engines[$engine_key])) { From 5d8457a07ee35612a55d744aece7665061ae6f4c Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 28 Oct 2019 14:22:07 -0700 Subject: [PATCH 02/15] In the repository URI index, store Phabricator's own URIs as tokens Summary: Fixes T13435. If you move Phabricator or copy data from one environment to another, the repository URI index currently still references the old URI, since it writes the URI as a plain string. This may make "arc which" and similar workflows have difficulty identifying repositories. Instead, store the "phabricator.base-uri" domain and the "diffusion.ssh-host" domain as tokens, so lookups continue to work correctly even after these values change. Test Plan: - Added unit tests to cover the normalization. - Ran migration, ran daemons, inspected `repository_uriindex` table, saw a mixture of sensible tokens (for local domains) and static domains (like "github.com"). - Ran this thing: ``` $ echo '{"remoteURIs": ["ssh://git@local.phacility.com/diffusion/P"]}' | ./bin/conduit call --method repository.query --trace --input - Reading input from stdin... >>> [2] (+0) repository.query() >>> [3] (+3) local_repository <<< [3] (+3) 555 us >>> [4] (+5) SELECT `r`.* FROM `repository` `r` LEFT JOIN `local_repository`.`repository_uriindex` uri ON r.phid = uri.repositoryPHID WHERE (uri.repositoryURI IN ('/diffusion/P')) GROUP BY `r`.phid ORDER BY `r`.`id` DESC LIMIT 101 <<< [4] (+5) 596 us <<< [2] (+6) 6,108 us { "result": [ { "id": "1", "name": "Phabricator", "phid": "PHID-REPO-2psrynlauicce7d3q7g2", "callsign": "P", "monogram": "rP", "vcs": "git", "uri": "http://local.phacility.com/source/phabricator/", "remoteURI": "https://github.com/phacility/phabricator.git", "description": "asdf", "isActive": true, "isHosted": false, "isImporting": false, "encoding": "UTF-8", "staging": { "supported": true, "prefix": "phabricator", "uri": null } } ] } ``` Note the `WHERE` clause in the query normalizes the URI into "", and the lookup succeeds. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13435 Differential Revision: https://secure.phabricator.com/D20872 --- .../20191028.uriindex.01.rebuild.php | 4 +++ .../PhabricatorRepositoryURINormalizer.php | 29 ++++++++++++++++-- ...ricatorRepositoryURINormalizerTestCase.php | 30 +++++++++++++++++++ 3 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 resources/sql/autopatches/20191028.uriindex.01.rebuild.php diff --git a/resources/sql/autopatches/20191028.uriindex.01.rebuild.php b/resources/sql/autopatches/20191028.uriindex.01.rebuild.php new file mode 100644 index 0000000000..c9bd3d97ca --- /dev/null +++ b/resources/sql/autopatches/20191028.uriindex.01.rebuild.php @@ -0,0 +1,4 @@ +getDomain(); if (!strlen($domain)) { - $domain = ''; + return ''; } - return phutil_utf8_strtolower($domain); + $domain = phutil_utf8_strtolower($domain); + + // See T13435. If the domain for a repository URI is same as the install + // base URI, store it as a "" token instead of the actual domain + // so that the index does not fall out of date if the install moves. + + $base_uri = PhabricatorEnv::getURI('/'); + $base_uri = new PhutilURI($base_uri); + $base_domain = $base_uri->getDomain(); + $base_domain = phutil_utf8_strtolower($base_domain); + if ($domain === $base_domain) { + return ''; + } + + // Likewise, store a token for the "SSH Host" domain so it can be changed + // without requiring an index rebuild. + + $ssh_host = PhabricatorEnv::getEnvConfig('diffusion.ssh-host'); + if (strlen($ssh_host)) { + $ssh_host = phutil_utf8_strtolower($ssh_host); + if ($domain === $ssh_host) { + return ''; + } + } + + return $domain; } diff --git a/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php b/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php index 81dd735562..8ab54a23a4 100644 --- a/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php +++ b/src/applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php @@ -31,6 +31,36 @@ final class PhabricatorRepositoryURINormalizerTestCase } } + public function testDomainURINormalizer() { + $base_domain = 'base.phabricator.example.com'; + $ssh_domain = 'ssh.phabricator.example.com'; + + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig('phabricator.base-uri', 'http://'.$base_domain); + $env->overrideEnvConfig('diffusion.ssh-host', $ssh_domain); + + $cases = array( + '/' => '', + '/path/to/local/repo.git' => '', + 'ssh://user@domain.com/path.git' => 'domain.com', + 'ssh://user@DOMAIN.COM/path.git' => 'domain.com', + 'http://'.$base_domain.'/diffusion/X/' => '', + 'ssh://'.$ssh_domain.'/diffusion/X/' => '', + 'git@'.$ssh_domain.':bananas.git' => '', + ); + + $type_git = PhabricatorRepositoryURINormalizer::TYPE_GIT; + + foreach ($cases as $input => $expect) { + $normal = new PhabricatorRepositoryURINormalizer($type_git, $input); + + $this->assertEqual( + $expect, + $normal->getNormalizedDomain(), + pht('Normalized domain for "%s".', $input)); + } + } + public function testSVNURINormalizer() { $cases = array( 'file:///path/to/repo' => 'path/to/repo', From 02f85f03bda713f5d47e2114ef843da46945b608 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 13 Sep 2019 07:47:51 -0700 Subject: [PATCH 03/15] Remove the "ssh-auth-key" script Summary: Ref T13436. Historically, this script could be used with a forked copy of "sshd" to do lower-cost per-key auth. Relatively modern "sshd" supports "%f" to "AuthorizedKeysCommand", which effectively moots this. Users have never been instructed to use this script for anything, and we moved away from this specific patch to "sshd" some time ago. Test Plan: Grepped for "ssh-auth-key", no hits. Maniphest Tasks: T13436 Differential Revision: https://secure.phabricator.com/D20873 --- bin/ssh-auth-key | 1 - scripts/ssh/ssh-auth-key.php | 42 ------------------------------------ 2 files changed, 43 deletions(-) delete mode 120000 bin/ssh-auth-key delete mode 100755 scripts/ssh/ssh-auth-key.php diff --git a/bin/ssh-auth-key b/bin/ssh-auth-key deleted file mode 120000 index 7dff83c316..0000000000 --- a/bin/ssh-auth-key +++ /dev/null @@ -1 +0,0 @@ -../scripts/ssh/ssh-auth-key.php \ No newline at end of file diff --git a/scripts/ssh/ssh-auth-key.php b/scripts/ssh/ssh-auth-key.php deleted file mode 100755 index 0c23a20edf..0000000000 --- a/scripts/ssh/ssh-auth-key.php +++ /dev/null @@ -1,42 +0,0 @@ -#!/usr/bin/env php -setViewer(PhabricatorUser::getOmnipotentUser()) - ->withKeys(array($public_key)) - ->withIsActive(true) - ->executeOne(); -if (!$key) { - exit(1); -} - -$object = $key->getObject(); -if (!($object instanceof PhabricatorUser)) { - exit(1); -} - -$bin = $root.'/bin/ssh-exec'; -$cmd = csprintf('%s --phabricator-ssh-user %s', $bin, $object->getUsername()); -// This is additional escaping for the SSH 'command="..."' string. -$cmd = addcslashes($cmd, '"\\'); - -$options = array( - 'command="'.$cmd.'"', - 'no-port-forwarding', - 'no-X11-forwarding', - 'no-agent-forwarding', - 'no-pty', -); - -echo implode(',', $options); -exit(0); From 24f771c1bc079caa3c7e75b63c5023000c332095 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 13 Sep 2019 08:25:22 -0700 Subject: [PATCH 04/15] Add an optional "--sshd-key" argument to "bin/ssh-auth" for reading "%k" from modern sshd Summary: Depends on D20873. Ref T13436. Allow callers to configure "bin/ssh-auth --sshd-key %k" as an "AuthorizedKeysCommand"; if they do, and we recognize the key, emit just that key in the output. Test Plan: - Used `git pull` locally, still worked fine. - Instrumented things, saw the public key lookup actually work and emit a single key. - Ran without "--sshd-key", got a full key list as before. Maniphest Tasks: T13436 Differential Revision: https://secure.phabricator.com/D20874 --- scripts/ssh/ssh-auth.php | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/scripts/ssh/ssh-auth.php b/scripts/ssh/ssh-auth.php index 3c4f3f2b33..2a27329d4a 100755 --- a/scripts/ssh/ssh-auth.php +++ b/scripts/ssh/ssh-auth.php @@ -4,6 +4,24 @@ $root = dirname(dirname(dirname(__FILE__))); require_once $root.'/scripts/init/init-script.php'; +// TODO: For now, this is using "parseParital()", not "parse()". This allows +// the script to accept (and ignore) additional arguments. This preserves +// backward compatibility until installs have time to migrate to the new +// syntax. + +$args = id(new PhutilArgumentParser($argv)) + ->parsePartial( + array( + array( + 'name' => 'sshd-key', + 'param' => 'k', + 'help' => pht( + 'Accepts the "%%k" parameter from "AuthorizedKeysCommand".'), + ), + )); + +$sshd_key = $args->getArg('sshd-key'); + // NOTE: We are caching a datastructure rather than the flat key file because // the path on disk to "ssh-exec" is arbitrarily mutable at runtime. See T12397. @@ -85,6 +103,22 @@ if ($authstruct === null) { $cache->setKey($authstruct_key, $authstruct_raw, $ttl); } +// If we've received an "--sshd-key" argument and it matches some known key, +// only emit that key. (For now, if the key doesn't match, we'll fall back to +// emitting all keys.) +if ($sshd_key !== null) { + $matches = array(); + foreach ($authstruct['keys'] as $key => $key_struct) { + if (phutil_hashes_are_identical($key_struct['key'], $sshd_key)) { + $matches[$key] = $key_struct; + } + } + + if ($matches) { + $authstruct['keys'] = $matches; + } +} + $bin = $root.'/bin/ssh-exec'; $instance = PhabricatorEnv::getEnvConfig('cluster.instance'); From 4a53fc339e323b4a415996db5c57940f2530c5f3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 28 Oct 2019 18:28:11 -0700 Subject: [PATCH 05/15] Don't use "phutil_hashes_are_identical()" to compare public keys Summary: Ref T13436. There's no real security value to doing this comparison, it just wards off evil "security researchers" who get upset if you ever compare two strings with a non-constant-time algorithm. In practice, SSH public keys are pretty long, pretty public, and have pretty similar lengths. This leads to a relatively large amount of work to do constant-time comparisons on them (we frequently can't abort early after identifying differing string length). Test Plan: Ran `bin/ssh-auth --sshd-key ...` on `secure` with ~1K keys, saw runtime drop by ~50% (~400ms to ~200ms) with `===`. Maniphest Tasks: T13436 Differential Revision: https://secure.phabricator.com/D20875 --- scripts/ssh/ssh-auth.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/ssh/ssh-auth.php b/scripts/ssh/ssh-auth.php index 2a27329d4a..19b1cc46b4 100755 --- a/scripts/ssh/ssh-auth.php +++ b/scripts/ssh/ssh-auth.php @@ -109,7 +109,7 @@ if ($authstruct === null) { if ($sshd_key !== null) { $matches = array(); foreach ($authstruct['keys'] as $key => $key_struct) { - if (phutil_hashes_are_identical($key_struct['key'], $sshd_key)) { + if ($key_struct['key'] === $sshd_key) { $matches[$key] = $key_struct; } } From e1da1d86d68021f7e69191e72f27e4293d2a0fe4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 29 Oct 2019 09:37:22 -0700 Subject: [PATCH 06/15] Trim and URI encode symbol names before building URIs from them Summary: Fixes T13437. This URI construction was just missing URI encoding. Also, trim the symbol because my test case ended up catching "#define\n" as symbol text. Test Plan: - Configured a repository to have PHP symbols. - Touched a ".php" file with "#define" in it. - Diffed the change. - Command-clicked "#define" in the UI, in Safari/MacOS, to jump to the definition. - Before: taken to a nonsense page where "#define" became an anchor. - After: taken to symbol search for "#define". Maniphest Tasks: T13437 Differential Revision: https://secure.phabricator.com/D20876 --- resources/celerity/map.php | 18 +++++++++--------- .../repository/repository-crossreference.js | 11 ++++++++++- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0e53737aaf..19d74a3f52 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => '686ae87c', 'core.pkg.js' => '6e5c894f', 'differential.pkg.css' => '607c84be', - 'differential.pkg.js' => 'a0212a0b', + 'differential.pkg.js' => '1b97518d', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -428,7 +428,7 @@ return array( 'rsrc/js/application/releeph/releeph-preview-branch.js' => '75184d68', 'rsrc/js/application/releeph/releeph-request-state-change.js' => '9f081f05', 'rsrc/js/application/releeph/releeph-request-typeahead.js' => 'aa3a100c', - 'rsrc/js/application/repository/repository-crossreference.js' => 'c15122b4', + 'rsrc/js/application/repository/repository-crossreference.js' => '1c95ea63', 'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e5bdb730', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'b86f297f', 'rsrc/js/application/transactions/behavior-comment-actions.js' => '4dffaeb2', @@ -682,7 +682,7 @@ return array( 'javelin-behavior-reorder-applications' => 'aa371860', 'javelin-behavior-reorder-columns' => '8ac32fd9', 'javelin-behavior-reorder-profile-menu-items' => 'e5bdb730', - 'javelin-behavior-repository-crossreference' => 'c15122b4', + 'javelin-behavior-repository-crossreference' => '1c95ea63', 'javelin-behavior-scrollbar' => '92388bae', 'javelin-behavior-search-reorder-queries' => 'b86f297f', 'javelin-behavior-select-content' => 'e8240b50', @@ -1034,6 +1034,12 @@ return array( 'javelin-install', 'javelin-util', ), + '1c95ea63' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-uri', + ), '1cab0e9a' => array( 'javelin-behavior', 'javelin-dom', @@ -1977,12 +1983,6 @@ return array( 'c03f2fb4' => array( 'javelin-install', ), - 'c15122b4' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-uri', - ), 'c2c500a7' => array( 'javelin-install', 'javelin-dom', diff --git a/webroot/rsrc/js/application/repository/repository-crossreference.js b/webroot/rsrc/js/application/repository/repository-crossreference.js index d6ff2a06aa..ba522d5b47 100644 --- a/webroot/rsrc/js/application/repository/repository-crossreference.js +++ b/webroot/rsrc/js/application/repository/repository-crossreference.js @@ -152,7 +152,16 @@ JX.behavior('repository-crossreference', function(config, statics) { query.char = char; } - var uri = JX.$U('/diffusion/symbol/' + symbol + '/'); + var uri_symbol = symbol; + + // In some cases, lexers may include whitespace in symbol tags. Trim it, + // since symbols with semantic whitespace aren't supported. + uri_symbol = uri_symbol.trim(); + + // See T13437. Symbols like "#define" need to be encoded. + uri_symbol = encodeURIComponent(uri_symbol); + + var uri = JX.$U('/diffusion/symbol/' + uri_symbol + '/'); uri.addQueryParams(query); window.open(uri.toString()); From 114166dd3261326a46e17651aef7d1a2744e7056 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 29 Oct 2019 13:13:59 -0700 Subject: [PATCH 07/15] Roughly implement "harbormaster.artifact.search" Summary: Ref T13438. This is a sort of minimal plausible implementation. Test Plan: Used "harbormaster.artifact.search" to query information about artifacts. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13438 Differential Revision: https://secure.phabricator.com/D20878 --- src/__phutil_library_map__.php | 5 + ...ormasterArtifactSearchConduitAPIMethod.php | 18 ++++ .../HarbormasterArtifactSearchEngine.php | 93 +++++++++++++++++++ .../build/HarbormasterBuildArtifact.php | 48 +++++++++- 4 files changed, 159 insertions(+), 5 deletions(-) create mode 100644 src/applications/harbormaster/conduit/HarbormasterArtifactSearchConduitAPIMethod.php create mode 100644 src/applications/harbormaster/query/HarbormasterArtifactSearchEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 9d51d92760..83954637ce 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1338,6 +1338,8 @@ phutil_register_library_map(array( 'HarbormasterArcLintBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcLintBuildStepImplementation.php', 'HarbormasterArcUnitBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterArcUnitBuildStepImplementation.php', 'HarbormasterArtifact' => 'applications/harbormaster/artifact/HarbormasterArtifact.php', + 'HarbormasterArtifactSearchConduitAPIMethod' => 'applications/harbormaster/conduit/HarbormasterArtifactSearchConduitAPIMethod.php', + 'HarbormasterArtifactSearchEngine' => 'applications/harbormaster/query/HarbormasterArtifactSearchEngine.php', 'HarbormasterAutotargetsTestCase' => 'applications/harbormaster/__tests__/HarbormasterAutotargetsTestCase.php', 'HarbormasterBuild' => 'applications/harbormaster/storage/build/HarbormasterBuild.php', 'HarbormasterBuildAbortedException' => 'applications/harbormaster/exception/HarbormasterBuildAbortedException.php', @@ -7369,6 +7371,8 @@ phutil_register_library_map(array( 'HarbormasterArcLintBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterArcUnitBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterArtifact' => 'Phobject', + 'HarbormasterArtifactSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', + 'HarbormasterArtifactSearchEngine' => 'PhabricatorApplicationSearchEngine', 'HarbormasterAutotargetsTestCase' => 'PhabricatorTestCase', 'HarbormasterBuild' => array( 'HarbormasterDAO', @@ -7384,6 +7388,7 @@ phutil_register_library_map(array( 'HarbormasterDAO', 'PhabricatorPolicyInterface', 'PhabricatorDestructibleInterface', + 'PhabricatorConduitResultInterface', ), 'HarbormasterBuildArtifactPHIDType' => 'PhabricatorPHIDType', 'HarbormasterBuildArtifactQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', diff --git a/src/applications/harbormaster/conduit/HarbormasterArtifactSearchConduitAPIMethod.php b/src/applications/harbormaster/conduit/HarbormasterArtifactSearchConduitAPIMethod.php new file mode 100644 index 0000000000..63cba16af4 --- /dev/null +++ b/src/applications/harbormaster/conduit/HarbormasterArtifactSearchConduitAPIMethod.php @@ -0,0 +1,18 @@ +setLabel(pht('Targets')) + ->setKey('buildTargetPHIDs') + ->setAliases( + array( + 'buildTargetPHID', + 'buildTargets', + 'buildTarget', + 'targetPHIDs', + 'targetPHID', + 'targets', + 'target', + )) + ->setDescription( + pht('Search for artifacts attached to particular build targets.')), + ); + } + + protected function buildQueryFromParameters(array $map) { + $query = $this->newQuery(); + + if ($map['buildTargetPHIDs']) { + $query->withBuildTargetPHIDs($map['buildTargetPHIDs']); + } + + return $query; + } + + protected function getURI($path) { + return '/harbormaster/artifact/'.$path; + } + + protected function getBuiltinQueryNames() { + return array( + 'all' => pht('All Artifacts'), + ); + } + + public function buildSavedQueryFromBuiltin($query_key) { + $query = $this->newSavedQuery(); + $query->setQueryKey($query_key); + + switch ($query_key) { + case 'all': + return $query; + } + + return parent::buildSavedQueryFromBuiltin($query_key); + } + + protected function renderResultList( + array $artifacts, + PhabricatorSavedQuery $query, + array $handles) { + assert_instances_of($artifacts, 'HarbormasterBuildArtifact'); + + $viewer = $this->requireViewer(); + + $list = new PHUIObjectItemListView(); + foreach ($artifacts as $artifact) { + $id = $artifact->getID(); + + $item = id(new PHUIObjectItemView()) + ->setObjectName(pht('Artifact %d', $id)); + + $list->addItem($item); + } + + return id(new PhabricatorApplicationSearchResultView()) + ->setObjectList($list) + ->setNoDataString(pht('No artifacts found.')); + } + +} diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php index 7cd8d60b6a..8b4972c154 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildArtifact.php @@ -4,7 +4,8 @@ final class HarbormasterBuildArtifact extends HarbormasterDAO implements PhabricatorPolicyInterface, - PhabricatorDestructibleInterface { + PhabricatorDestructibleInterface, + PhabricatorConduitResultInterface { protected $buildTargetPHID; protected $artifactType; @@ -18,6 +19,7 @@ final class HarbormasterBuildArtifact public static function initializeNewBuildArtifact( HarbormasterBuildTarget $build_target) { + return id(new HarbormasterBuildArtifact()) ->attachBuildTarget($build_target) ->setBuildTargetPHID($build_target->getPHID()); @@ -53,9 +55,8 @@ final class HarbormasterBuildArtifact ) + parent::getConfiguration(); } - public function generatePHID() { - return PhabricatorPHID::generateNewPHID( - HarbormasterBuildArtifactPHIDType::TYPECONST); + public function getPHIDType() { + return HarbormasterBuildArtifactPHIDType::TYPECONST; } public function attachBuildTarget(HarbormasterBuildTarget $build_target) { @@ -147,7 +148,8 @@ final class HarbormasterBuildArtifact } public function describeAutomaticCapability($capability) { - return pht('Users must be able to see a buildable to see its artifacts.'); + return pht( + 'Users must be able to see a build target to see its artifacts.'); } @@ -165,4 +167,40 @@ final class HarbormasterBuildArtifact $this->saveTransaction(); } + +/* -( PhabricatorConduitResultInterface )---------------------------------- */ + + public function getFieldSpecificationsForConduit() { + return array( + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('buildTargetPHID') + ->setType('phid') + ->setDescription(pht('The build target this artifact is attached to.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('artifactType') + ->setType('string') + ->setDescription(pht('The artifact type.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('artifactKey') + ->setType('string') + ->setDescription(pht('The artifact key.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('isReleased') + ->setType('bool') + ->setDescription(pht('True if this artifact has been released.')), + ); + } + + public function getFieldValuesForConduit() { + return array( + 'buildTargetPHID' => $this->getBuildTargetPHID(), + 'artifactType' => $this->getArtifactType(), + 'artifactKey' => $this->getArtifactKey(), + 'isReleased' => (bool)$this->getIsReleased(), + ); + } + + public function getConduitSearchAttachments() { + return array(); + } } From 9d8cdce8e1f2f34b4588969256a3e2c4aef9fafb Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 29 Oct 2019 13:43:57 -0700 Subject: [PATCH 08/15] Make the top-level burndown chart in "Maniphest > Reports" show open tasks, not total tasks Summary: Ref T13279. See PHI1491. Currently, the top-level "Burnup Rate" chart in Maniphest shows total created tasks above the X-axis, without adjusting for closures. This is unintended and not very useful. The filtered-by-project charts show the right value (cumulative open tasks, i.e. open minus close). Change the value to aggregate creation events and status change events. Test Plan: Viewed top-level chart, saw the value no longer monotonically increasing. Maniphest Tasks: T13279 Differential Revision: https://secure.phabricator.com/D20879 --- .../project/chart/PhabricatorProjectBurndownChartEngine.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php index 1296f2eec8..16760d515f 100644 --- a/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php +++ b/src/applications/project/chart/PhabricatorProjectBurndownChartEngine.php @@ -53,7 +53,11 @@ final class PhabricatorProjectBurndownChartEngine $open_function = $this->newFunction( array( 'accumulate', - array('fact', 'tasks.open-count.create'), + array( + 'sum', + array('fact', 'tasks.open-count.create'), + array('fact', 'tasks.open-count.status'), + ), )); $closed_function = $this->newFunction( From bcf15abcd33c77f1fc4d63b4c157b0fd3ccd1b05 Mon Sep 17 00:00:00 2001 From: Arturas Moskvinas Date: Mon, 21 Oct 2019 15:44:46 +0300 Subject: [PATCH 09/15] Return empty data if fact dimension is missing, not yet available Summary: On fresh installation which doesn't have yet any task closed you will not be able to open charts because of error below: Fixes: ``` [Mon Oct 21 15:42:41 2019] [2019-10-21 15:42:41] EXCEPTION: (TypeError) Argument 1 passed to head_key() must be of the type array, null given, called in ..phabricator/src/applications/fact/chart/PhabricatorFactChartFunction.php on line 86 at [/src/utils/utils.php:832] [Mon Oct 21 15:42:41 2019] #0 phlog(TypeError) called at [/src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php:27] [Mon Oct 21 15:42:41 2019] #1 PhabricatorAjaxRequestExceptionHandler::handleRequestThrowable(AphrontRequest, TypeError) called at [/src/aphront/configuration/AphrontApplicationConfiguration.php:797] [Mon Oct 21 15:42:41 2019] #2 AphrontApplicationConfiguration::handleThrowable(TypeError) called at [/src/aphront/configuration/AphrontApplicationConfiguration.php:345] [Mon Oct 21 15:42:41 2019] #3 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [/src/aphront/configuration/AphrontApplicationConfiguration.php:214] [Mon Oct 21 15:42:41 2019] #4 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [/webroot/index.php:35 ``` To fix issue - lets return empty data set instead Test Plan: 1) Create fresh phabricator installation 2) Create fresh project 3) Try viewing charts Reviewers: epriestley, Pawka, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, yelirekim Differential Revision: https://secure.phabricator.com/D20861 --- src/applications/fact/chart/PhabricatorFactChartFunction.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/fact/chart/PhabricatorFactChartFunction.php b/src/applications/fact/chart/PhabricatorFactChartFunction.php index 0e940d644d..1713282116 100644 --- a/src/applications/fact/chart/PhabricatorFactChartFunction.php +++ b/src/applications/fact/chart/PhabricatorFactChartFunction.php @@ -29,6 +29,7 @@ final class PhabricatorFactChartFunction $key_id = id(new PhabricatorFactKeyDimension()) ->newDimensionID($fact->getKey()); if (!$key_id) { + $this->map = array(); return; } From 97bed350857966458c39caac3c7b9a5e5f6eb2d6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Oct 2019 08:50:31 -0700 Subject: [PATCH 10/15] Show repository information (and use repository identities) in commit hovercards Summary: Ref T12164. Ref T13439. Commit hovercards don't currently show the repository. Although this is sometimes obvious from context, it isn't at other times and it's clearly useful/important. Also, use identities to render author/committer information and show committer if the committer differs from the author. Test Plan: {F6989595} Maniphest Tasks: T13439, T12164 Differential Revision: https://secure.phabricator.com/D20881 --- .../DiffusionHovercardEngineExtension.php | 43 +++++++++++++++---- 1 file changed, 34 insertions(+), 9 deletions(-) diff --git a/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php b/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php index 3ce95cb3c3..cd0b3cceab 100644 --- a/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php +++ b/src/applications/diffusion/engineextension/DiffusionHovercardEngineExtension.php @@ -26,20 +26,45 @@ final class DiffusionHovercardEngineExtension $viewer = $this->getViewer(); - $author_phid = $commit->getAuthorPHID(); - if ($author_phid) { - $author = $viewer->renderHandle($author_phid); - } else { - $commit_data = $commit->loadCommitData(); - $author = phutil_tag('em', array(), $commit_data->getAuthorName()); + $commit = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->needIdentities(true) + ->needCommitData(true) + ->withPHIDs(array($commit->getPHID())) + ->executeOne(); + if (!$commit) { + return; } + $author_phid = $commit->getAuthorDisplayPHID(); + $committer_phid = $commit->getCommitterDisplayPHID(); + $repository_phid = $commit->getRepository()->getPHID(); + + $phids = array(); + $phids[] = $author_phid; + $phids[] = $committer_phid; + $phids[] = $repository_phid; + + $handles = $viewer->loadHandles($phids); + $hovercard->setTitle($handle->getName()); $hovercard->setDetail($commit->getSummary()); - $hovercard->addField(pht('Author'), $author); - $hovercard->addField(pht('Date'), - phabricator_date($commit->getEpoch(), $viewer)); + $repository = $handles[$repository_phid]->renderLink(); + $hovercard->addField(pht('Repository'), $repository); + + $author = $handles[$author_phid]->renderLink(); + if ($author_phid) { + $hovercard->addField(pht('Author'), $author); + } + + if ($committer_phid && ($committer_phid !== $author_phid)) { + $committer = $handles[$committer_phid]->renderLink(); + $hovercard->addField(pht('Committer'), $committer); + } + + $date = phabricator_date($commit->getEpoch(), $viewer); + $hovercard->addField(pht('Date'), $date); if (!$commit->isAuditStatusNoAudit()) { $status = $commit->getAuditStatusObject(); From b0d9f89c953573a47cf2912765be07962b91d82d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Oct 2019 10:23:21 -0700 Subject: [PATCH 11/15] Remove "State Icons" from handles Summary: Ref T13440. This feature is used in only one interface which I'm about to rewrite, so throw it away. Test Plan: Grepped for all affected symbols, didn't find any hits anywhere. Maniphest Tasks: T13440 Differential Revision: https://secure.phabricator.com/D20882 --- .../phid/DifferentialRevisionPHIDType.php | 9 ---- .../ManiphestTaskDetailController.php | 6 +-- .../phid/PhabricatorObjectHandle.php | 53 ------------------- .../phid/view/PHUIHandleListView.php | 15 ------ src/applications/phid/view/PHUIHandleView.php | 15 ------ .../PhabricatorRepositoryCommitPHIDType.php | 10 ---- 6 files changed, 2 insertions(+), 106 deletions(-) diff --git a/src/applications/differential/phid/DifferentialRevisionPHIDType.php b/src/applications/differential/phid/DifferentialRevisionPHIDType.php index a117690d66..a7d3c9f4a7 100644 --- a/src/applications/differential/phid/DifferentialRevisionPHIDType.php +++ b/src/applications/differential/phid/DifferentialRevisionPHIDType.php @@ -44,15 +44,6 @@ final class DifferentialRevisionPHIDType extends PhabricatorPHIDType { if ($revision->isClosed()) { $handle->setStatus(PhabricatorObjectHandle::STATUS_CLOSED); } - - $icon = $revision->getStatusIcon(); - $color = $revision->getStatusIconColor(); - $name = $revision->getStatusDisplayName(); - - $handle - ->setStateIcon($icon) - ->setStateColor($color) - ->setStateName($name); } } diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index c5dba7d3b5..00b884d610 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -413,8 +413,7 @@ final class ManiphestTaskDetailController extends ManiphestController { foreach ($commit_phids as $phid) { $revisions_commits[$phid] = $handles->renderHandle($phid) - ->setShowHovercard(true) - ->setShowStateIcon(true); + ->setShowHovercard(true); $revision_phid = key($drev_edges[$phid][$commit_drev]); $revision_handle = $handles->getHandleIfExists($revision_phid); if ($revision_handle) { @@ -435,8 +434,7 @@ final class ManiphestTaskDetailController extends ManiphestController { $edge_handles = $viewer->loadHandles(array_keys($edges[$edge_type])); - $edge_list = $edge_handles->renderList() - ->setShowStateIcons(true); + $edge_list = $edge_handles->renderList(); $view->addProperty($edge_name, $edge_list); } diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php index ba93dbcead..86f0f848c0 100644 --- a/src/applications/phid/PhabricatorObjectHandle.php +++ b/src/applications/phid/PhabricatorObjectHandle.php @@ -33,10 +33,6 @@ final class PhabricatorObjectHandle private $commandLineObjectName; private $mailStampName; - private $stateIcon; - private $stateColor; - private $stateName; - public function setIcon($icon) { $this->icon = $icon; return $this; @@ -299,55 +295,6 @@ final class PhabricatorObjectHandle return $this->complete; } - public function setStateIcon($state_icon) { - $this->stateIcon = $state_icon; - return $this; - } - - public function getStateIcon() { - return $this->stateIcon; - } - - public function setStateColor($state_color) { - $this->stateColor = $state_color; - return $this; - } - - public function getStateColor() { - return $this->stateColor; - } - - public function setStateName($state_name) { - $this->stateName = $state_name; - return $this; - } - - public function getStateName() { - return $this->stateName; - } - - public function renderStateIcon() { - $icon = $this->getStateIcon(); - if ($icon === null) { - $icon = 'fa-question-circle-o'; - } - - $color = $this->getStateColor(); - - $name = $this->getStateName(); - if ($name === null) { - $name = pht('Unknown'); - } - - return id(new PHUIIconView()) - ->setIcon($icon, $color) - ->addSigil('has-tooltip') - ->setMetadata( - array( - 'tip' => $name, - )); - } - public function renderLink($name = null) { return $this->renderLinkWithAttributes($name, array()); } diff --git a/src/applications/phid/view/PHUIHandleListView.php b/src/applications/phid/view/PHUIHandleListView.php index 24104fe76d..c5b2f19784 100644 --- a/src/applications/phid/view/PHUIHandleListView.php +++ b/src/applications/phid/view/PHUIHandleListView.php @@ -13,7 +13,6 @@ final class PHUIHandleListView private $handleList; private $asInline; private $asText; - private $showStateIcons; private $glyphLimit; public function setHandleList(PhabricatorHandleList $list) { @@ -39,15 +38,6 @@ final class PHUIHandleListView return $this->asText; } - public function setShowStateIcons($show_state_icons) { - $this->showStateIcons = $show_state_icons; - return $this; - } - - public function getShowStateIcons() { - return $this->showStateIcons; - } - public function setGlyphLimit($glyph_limit) { $this->glyphLimit = $glyph_limit; return $this; @@ -70,7 +60,6 @@ final class PHUIHandleListView protected function getTagContent() { $list = $this->handleList; - $show_state_icons = $this->getShowStateIcons(); $glyph_limit = $this->getGlyphLimit(); $items = array(); @@ -79,10 +68,6 @@ final class PHUIHandleListView ->setShowHovercard(true) ->setAsText($this->getAsText()); - if ($show_state_icons) { - $view->setShowStateIcon(true); - } - if ($glyph_limit) { $view->setGlyphLimit($glyph_limit); } diff --git a/src/applications/phid/view/PHUIHandleView.php b/src/applications/phid/view/PHUIHandleView.php index fe3c62a9ac..6cdf84f391 100644 --- a/src/applications/phid/view/PHUIHandleView.php +++ b/src/applications/phid/view/PHUIHandleView.php @@ -17,7 +17,6 @@ final class PHUIHandleView private $asText; private $useShortName; private $showHovercard; - private $showStateIcon; private $glyphLimit; public function setHandleList(PhabricatorHandleList $list) { @@ -50,15 +49,6 @@ final class PHUIHandleView return $this; } - public function setShowStateIcon($show_state_icon) { - $this->showStateIcon = $show_state_icon; - return $this; - } - - public function getShowStateIcon() { - return $this->showStateIcon; - } - public function setGlyphLimit($glyph_limit) { $this->glyphLimit = $glyph_limit; return $this; @@ -104,11 +94,6 @@ final class PHUIHandleView $link = $handle->renderLink($name); } - if ($this->showStateIcon) { - $icon = $handle->renderStateIcon(); - $link = array($icon, ' ', $link); - } - return $link; } diff --git a/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php b/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php index c37bdc04f9..df84f2dcfd 100644 --- a/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php +++ b/src/applications/repository/phid/PhabricatorRepositoryCommitPHIDType.php @@ -81,16 +81,6 @@ final class PhabricatorRepositoryCommitPHIDType extends PhabricatorPHIDType { $handle->setFullName($full_name); $handle->setURI($commit->getURI()); $handle->setTimestamp($commit->getEpoch()); - - $status = $commit->getAuditStatusObject(); - $icon = $status->getIcon(); - $color = $status->getColor(); - $name = $status->getName(); - - $handle - ->setStateIcon($icon) - ->setStateColor($color) - ->setStateName($name); } } From 7bdfe5b46adaab3b87f43485deb659baab2e58b6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Oct 2019 10:23:27 -0700 Subject: [PATCH 12/15] Show commits and revisions on tasks in a tabular view instead of handle lists Summary: Depends on D20882. Ref T13440. Instead of lists of "Differential Revisions" and "Commits", show all changes related to the task in a tabular view. Test Plan: {F6989816} Maniphest Tasks: T13440 Differential Revision: https://secure.phabricator.com/D20883 --- .../ManiphestTaskDetailController.php | 323 +++++++++++++++--- 1 file changed, 273 insertions(+), 50 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 00b884d610..496b4f8cb2 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -179,11 +179,14 @@ final class ManiphestTaskDetailController extends ManiphestController { ->addTabGroup($tab_group); } + $changes_view = $this->newChangesView($task, $edges); + $view = id(new PHUITwoColumnView()) ->setHeader($header) ->setCurtain($curtain) ->setMainColumn( array( + $changes_view, $tab_view, $timeline, $comment_view, @@ -395,56 +398,6 @@ final class ManiphestTaskDetailController extends ManiphestController { $source)); } - $edge_types = array( - ManiphestTaskHasRevisionEdgeType::EDGECONST - => pht('Differential Revisions'), - ); - - $revisions_commits = array(); - - $commit_phids = array_keys( - $edges[ManiphestTaskHasCommitEdgeType::EDGECONST]); - if ($commit_phids) { - $commit_drev = DiffusionCommitHasRevisionEdgeType::EDGECONST; - $drev_edges = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs($commit_phids) - ->withEdgeTypes(array($commit_drev)) - ->execute(); - - foreach ($commit_phids as $phid) { - $revisions_commits[$phid] = $handles->renderHandle($phid) - ->setShowHovercard(true); - $revision_phid = key($drev_edges[$phid][$commit_drev]); - $revision_handle = $handles->getHandleIfExists($revision_phid); - if ($revision_handle) { - $task_drev = ManiphestTaskHasRevisionEdgeType::EDGECONST; - unset($edges[$task_drev][$revision_phid]); - $revisions_commits[$phid] = hsprintf( - '%s / %s', - $revision_handle->renderHovercardLink($revision_handle->getName()), - $revisions_commits[$phid]); - } - } - } - - foreach ($edge_types as $edge_type => $edge_name) { - if (!$edges[$edge_type]) { - continue; - } - - $edge_handles = $viewer->loadHandles(array_keys($edges[$edge_type])); - - $edge_list = $edge_handles->renderList(); - - $view->addProperty($edge_name, $edge_list); - } - - if ($revisions_commits) { - $view->addProperty( - pht('Commits'), - phutil_implode_html(phutil_tag('br'), $revisions_commits)); - } - $field_list->appendFieldsToPropertyList( $task, $viewer, @@ -594,5 +547,275 @@ final class ManiphestTaskDetailController extends ManiphestController { return $handles->newSublist($phids); } + private function newChangesView(ManiphestTask $task, array $edges) { + $viewer = $this->getViewer(); + + $revision_type = ManiphestTaskHasRevisionEdgeType::EDGECONST; + $commit_type = ManiphestTaskHasCommitEdgeType::EDGECONST; + + $revision_phids = idx($edges, $revision_type, array()); + $revision_phids = array_keys($revision_phids); + $revision_phids = array_fuse($revision_phids); + + $commit_phids = idx($edges, $commit_type, array()); + $commit_phids = array_keys($commit_phids); + $commit_phids = array_fuse($commit_phids); + + if (!$revision_phids && !$commit_phids) { + return null; + } + + if ($commit_phids) { + $link_type = DiffusionCommitHasRevisionEdgeType::EDGECONST; + $link_query = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs($commit_phids) + ->withEdgeTypes(array($link_type)); + $link_query->execute(); + + $commits = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withPHIDs($commit_phids) + ->execute(); + $commits = mpull($commits, null, 'getPHID'); + } else { + $commits = array(); + } + + if ($revision_phids) { + $revisions = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withPHIDs($revision_phids) + ->execute(); + $revisions = mpull($revisions, null, 'getPHID'); + } else { + $revisions = array(); + } + + $handle_phids = array(); + $any_linked = false; + + $tail = array(); + foreach ($commit_phids as $commit_phid) { + $handle_phids[] = $commit_phid; + + $link_phids = $link_query->getDestinationPHIDs(array($commit_phid)); + foreach ($link_phids as $link_phid) { + $handle_phids[] = $link_phid; + unset($revision_phids[$link_phid]); + $any_linked = true; + } + + $commit = idx($commits, $commit_phid); + if ($commit) { + $repository_phid = $commit->getRepository()->getPHID(); + $handle_phids[] = $repository_phid; + } else { + $repository_phid = null; + } + + $status_view = null; + if ($commit) { + $status = $commit->getAuditStatusObject(); + if (!$status->isNoAudit()) { + $status_view = id(new PHUITagView()) + ->setType(PHUITagView::TYPE_SHADE) + ->setIcon($status->getIcon()) + ->setColor($status->getColor()) + ->setName($status->getName()); + + } + } + + $object_link = null; + if ($commit) { + $commit_monogram = $commit->getDisplayName(); + $commit_monogram = phutil_tag( + 'span', + array( + 'class' => 'object-name', + ), + $commit_monogram); + + $commit_link = javelin_tag( + 'a', + array( + 'href' => $commit->getURI(), + 'sigil' => 'hovercard', + 'meta' => array( + 'hoverPHID' => $commit->getPHID(), + ), + ), + $commit->getSummary()); + + $object_link = array( + $commit_monogram, + ' ', + $commit_link, + ); + } + + $tail[] = array( + 'objectPHID' => $commit_phid, + 'objectLink' => $object_link, + 'repositoryPHID' => $repository_phid, + 'revisionPHIDs' => $link_phids, + 'status' => $status_view, + ); + } + + $head = array(); + foreach ($revision_phids as $revision_phid) { + $handle_phids[] = $revision_phid; + + $revision = idx($revisions, $revision_phid); + if ($revision) { + $repository_phid = $revision->getRepositoryPHID(); + $handle_phids[] = $repository_phid; + } else { + $repository_phid = null; + } + + if ($revision) { + $icon = $revision->getStatusIcon(); + $color = $revision->getStatusIconColor(); + $name = $revision->getStatusDisplayName(); + + $status_view = id(new PHUITagView()) + ->setType(PHUITagView::TYPE_SHADE) + ->setIcon($icon) + ->setColor($color) + ->setName($name); + } else { + $status_view = null; + } + + $object_link = null; + if ($revision) { + $revision_monogram = $revision->getMonogram(); + $revision_monogram = phutil_tag( + 'span', + array( + 'class' => 'object-name', + ), + $revision_monogram); + + $revision_link = javelin_tag( + 'a', + array( + 'href' => $revision->getURI(), + 'sigil' => 'hovercard', + 'meta' => array( + 'hoverPHID' => $revision->getPHID(), + ), + ), + $revision->getTitle()); + + $object_link = array( + $revision_monogram, + ' ', + $revision_link, + ); + } + + $head[] = array( + 'objectPHID' => $revision_phid, + 'objectLink' => $object_link, + 'repositoryPHID' => $repository_phid, + 'revisionPHIDs' => array(), + 'status' => $status_view, + ); + } + + $objects = array_merge($head, $tail); + $handles = $viewer->loadHandles($handle_phids); + + $rows = array(); + foreach ($objects as $object) { + $object_phid = $object['objectPHID']; + $handle = $handles[$object_phid]; + + $object_link = $object['objectLink']; + if ($object_link === null) { + $object_link = $handle->renderLink(); + } + + $object_icon = id(new PHUIIconView()) + ->setIcon($handle->getIcon()); + + $repository_link = null; + $repository_phid = $object['repositoryPHID']; + if ($repository_phid) { + $repository_link = $handles[$repository_phid]->renderLink(); + } + + $status_view = $object['status']; + + $revision_tags = array(); + foreach ($object['revisionPHIDs'] as $link_phid) { + $revision_handle = $handles[$link_phid]; + + $revision_name = $revision_handle->getName(); + $revision_tags[] = $revision_handle + ->renderHovercardLink($revision_name); + } + $revision_tags = phutil_implode_html( + phutil_tag('br'), + $revision_tags); + + $rows[] = array( + $object_icon, + $status_view, + $repository_link, + $revision_tags, + $object_link, + ); + } + + $changes_table = id(new AphrontTableView($rows)) + ->setNoDataString(pht('This task has no related commits or revisions.')) + ->setHeaders( + array( + null, + null, + pht('Repository'), + null, + pht('Revision/Commit'), + )) + ->setColumnClasses( + array( + 'center', + null, + null, + null, + 'wide pri object-link', + )) + ->setColumnVisibility( + array( + true, + true, + true, + $any_linked, + true, + )) + ->setDeviceVisibility( + array( + false, + true, + false, + false, + true, + )); + + $changes_header = id(new PHUIHeaderView()) + ->setHeader(pht('Revisions and Commits')); + + $changes_view = id(new PHUIObjectBoxView()) + ->setHeader($changes_header) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setTable($changes_table); + + return $changes_view; + } + } From c48f300eb16954ef7f254be55f8a3df777061422 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Oct 2019 11:59:19 -0700 Subject: [PATCH 13/15] Add support for rendering section dividers in tables; use section dividers for changes on tasks Summary: Depends on D20883. Ref T13440. In most cases, all changes belong to the same repository, which makes the "Repository" column redundant and visually noisy. Show repository information in a section header. Test Plan: {F6989932} Maniphest Tasks: T13440 Differential Revision: https://secure.phabricator.com/D20884 --- resources/celerity/map.php | 6 +-- .../ManiphestTaskDetailController.php | 52 +++++++++++++------ src/view/control/AphrontTableView.php | 27 +++++++++- webroot/rsrc/css/aphront/table-view.css | 6 +++ 4 files changed, 71 insertions(+), 20 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 19d74a3f52..8793ab347c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '686ae87c', + 'core.pkg.css' => '9a391b14', 'core.pkg.js' => '6e5c894f', 'differential.pkg.css' => '607c84be', 'differential.pkg.js' => '1b97518d', @@ -30,7 +30,7 @@ return array( 'rsrc/css/aphront/notification.css' => '30240bd2', 'rsrc/css/aphront/panel-view.css' => '46923d46', 'rsrc/css/aphront/phabricator-nav-view.css' => 'f8a0c1bf', - 'rsrc/css/aphront/table-view.css' => '5f13a9e4', + 'rsrc/css/aphront/table-view.css' => '061e45eb', 'rsrc/css/aphront/tokenizer.css' => 'b52d0668', 'rsrc/css/aphront/tooltip.css' => 'e3f2412f', 'rsrc/css/aphront/typeahead-browse.css' => 'b7ed02d2', @@ -535,7 +535,7 @@ return array( 'aphront-list-filter-view-css' => 'feb64255', 'aphront-multi-column-view-css' => 'fbc00ba3', 'aphront-panel-view-css' => '46923d46', - 'aphront-table-view-css' => '5f13a9e4', + 'aphront-table-view-css' => '061e45eb', 'aphront-tokenizer-control-css' => 'b52d0668', 'aphront-tooltip-css' => 'e3f2412f', 'aphront-typeahead-control-css' => '8779483d', diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index 496b4f8cb2..f2a15e5605 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -594,7 +594,8 @@ final class ManiphestTaskDetailController extends ManiphestController { $handle_phids = array(); $any_linked = false; - $tail = array(); + $idx = 0; + $objects = array(); foreach ($commit_phids as $commit_phid) { $handle_phids[] = $commit_phid; @@ -654,16 +655,20 @@ final class ManiphestTaskDetailController extends ManiphestController { ); } - $tail[] = array( + $objects[] = array( 'objectPHID' => $commit_phid, 'objectLink' => $object_link, 'repositoryPHID' => $repository_phid, 'revisionPHIDs' => $link_phids, 'status' => $status_view, + 'order' => id(new PhutilSortVector()) + ->addInt($repository_phid ? 1 : 0) + ->addString((string)$repository_phid) + ->addInt(1) + ->addInt($idx++), ); } - $head = array(); foreach ($revision_phids as $revision_phid) { $handle_phids[] = $revision_phid; @@ -717,20 +722,44 @@ final class ManiphestTaskDetailController extends ManiphestController { ); } - $head[] = array( + $objects[] = array( 'objectPHID' => $revision_phid, 'objectLink' => $object_link, 'repositoryPHID' => $repository_phid, 'revisionPHIDs' => array(), 'status' => $status_view, + 'order' => id(new PhutilSortVector()) + ->addInt($repository_phid ? 1 : 0) + ->addString((string)$repository_phid) + ->addInt(0) + ->addInt($idx++), ); } - $objects = array_merge($head, $tail); $handles = $viewer->loadHandles($handle_phids); + $order = ipull($objects, 'order'); + $order = msortv($order, 'getSelf'); + $objects = array_select_keys($objects, array_keys($order)); + + $last_repository = false; $rows = array(); + $rowd = array(); foreach ($objects as $object) { + $repository_phid = $object['repositoryPHID']; + if ($repository_phid !== $last_repository) { + $repository_link = null; + if ($repository_phid) { + $repository_link = $handles[$repository_phid]->renderLink(); + $rows[] = array( + $repository_link, + ); + $rowd[] = true; + } + + $last_repository = $repository_phid; + } + $object_phid = $object['objectPHID']; $handle = $handles[$object_phid]; @@ -742,12 +771,6 @@ final class ManiphestTaskDetailController extends ManiphestController { $object_icon = id(new PHUIIconView()) ->setIcon($handle->getIcon()); - $repository_link = null; - $repository_phid = $object['repositoryPHID']; - if ($repository_phid) { - $repository_link = $handles[$repository_phid]->renderLink(); - } - $status_view = $object['status']; $revision_tags = array(); @@ -762,10 +785,10 @@ final class ManiphestTaskDetailController extends ManiphestController { phutil_tag('br'), $revision_tags); + $rowd[] = false; $rows[] = array( $object_icon, $status_view, - $repository_link, $revision_tags, $object_link, ); @@ -773,11 +796,11 @@ final class ManiphestTaskDetailController extends ManiphestController { $changes_table = id(new AphrontTableView($rows)) ->setNoDataString(pht('This task has no related commits or revisions.')) + ->setRowDividers($rowd) ->setHeaders( array( null, null, - pht('Repository'), null, pht('Revision/Commit'), )) @@ -786,12 +809,10 @@ final class ManiphestTaskDetailController extends ManiphestController { 'center', null, null, - null, 'wide pri object-link', )) ->setColumnVisibility( array( - true, true, true, $any_linked, @@ -802,7 +823,6 @@ final class ManiphestTaskDetailController extends ManiphestController { false, true, false, - false, true, )); diff --git a/src/view/control/AphrontTableView.php b/src/view/control/AphrontTableView.php index cae9dabec2..dff64169c2 100644 --- a/src/view/control/AphrontTableView.php +++ b/src/view/control/AphrontTableView.php @@ -24,6 +24,8 @@ final class AphrontTableView extends AphrontView { protected $sortValues = array(); private $deviceReadyTable; + private $rowDividers = array(); + public function __construct(array $data) { $this->data = $data; } @@ -53,6 +55,11 @@ final class AphrontTableView extends AphrontView { return $this; } + public function setRowDividers(array $dividers) { + $this->rowDividers = $dividers; + return $this; + } + public function setNoDataString($no_data_string) { $this->noDataString = $no_data_string; return $this; @@ -258,10 +265,15 @@ final class AphrontTableView extends AphrontView { } } + $dividers = $this->rowDividers; + $data = $this->data; if ($data) { $row_num = 0; + $row_idx = 0; foreach ($data as $row) { + $is_divider = !empty($dividers[$row_num]); + $row_size = count($row); while (count($row) > count($col_classes)) { $col_classes[] = null; @@ -289,6 +301,18 @@ final class AphrontTableView extends AphrontView { $class = trim($class.' '.$this->cellClasses[$row_num][$col_num]); } + if ($is_divider) { + $tr[] = phutil_tag( + 'td', + array( + 'class' => 'row-divider', + 'colspan' => count($headers), + ), + $value); + $row_idx = -1; + break; + } + $tr[] = phutil_tag( 'td', array( @@ -299,7 +323,7 @@ final class AphrontTableView extends AphrontView { } $class = idx($this->rowClasses, $row_num); - if ($this->zebraStripes && ($row_num % 2)) { + if ($this->zebraStripes && ($row_idx % 2)) { if ($class !== null) { $class = 'alt alt-'.$class; } else { @@ -309,6 +333,7 @@ final class AphrontTableView extends AphrontView { $table[] = phutil_tag('tr', array('class' => $class), $tr); ++$row_num; + ++$row_idx; } } else { $colspan = max(count(array_filter($visibility)), 1); diff --git a/webroot/rsrc/css/aphront/table-view.css b/webroot/rsrc/css/aphront/table-view.css index 3736ffe841..c58e2ab2fc 100644 --- a/webroot/rsrc/css/aphront/table-view.css +++ b/webroot/rsrc/css/aphront/table-view.css @@ -55,6 +55,12 @@ background-color: {$lightbluebackground}; } +.aphront-table-view td.row-divider { + background-color: {$bluebackground}; + font-weight: bold; + padding: 8px 12px; +} + .aphront-table-view th { border-bottom: 1px solid {$thinblueborder}; } From e46e383bf25f42233302e1af7896f0971ea472aa Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Oct 2019 12:26:23 -0700 Subject: [PATCH 14/15] Clean up "Revisions/Commits" table in Maniphest slightly Summary: Ref T13440. Give the table more obvious visual structure and get rid of the largely useless header columns. Test Plan: Viewed table, saw a slightly cleaner result. Maniphest Tasks: T13440 Differential Revision: https://secure.phabricator.com/D20885 --- resources/celerity/map.php | 6 ++--- .../ManiphestTaskDetailController.php | 22 ++++++++----------- src/view/control/AphrontTableView.php | 2 +- webroot/rsrc/css/aphront/table-view.css | 4 ++++ 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 8793ab347c..8547f8a0a3 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '3c8a0668', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '9a391b14', + 'core.pkg.css' => '77de226f', 'core.pkg.js' => '6e5c894f', 'differential.pkg.css' => '607c84be', 'differential.pkg.js' => '1b97518d', @@ -30,7 +30,7 @@ return array( 'rsrc/css/aphront/notification.css' => '30240bd2', 'rsrc/css/aphront/panel-view.css' => '46923d46', 'rsrc/css/aphront/phabricator-nav-view.css' => 'f8a0c1bf', - 'rsrc/css/aphront/table-view.css' => '061e45eb', + 'rsrc/css/aphront/table-view.css' => '0bb61df1', 'rsrc/css/aphront/tokenizer.css' => 'b52d0668', 'rsrc/css/aphront/tooltip.css' => 'e3f2412f', 'rsrc/css/aphront/typeahead-browse.css' => 'b7ed02d2', @@ -535,7 +535,7 @@ return array( 'aphront-list-filter-view-css' => 'feb64255', 'aphront-multi-column-view-css' => 'fbc00ba3', 'aphront-panel-view-css' => '46923d46', - 'aphront-table-view-css' => '061e45eb', + 'aphront-table-view-css' => '0bb61df1', 'aphront-tokenizer-control-css' => 'b52d0668', 'aphront-tooltip-css' => 'e3f2412f', 'aphront-typeahead-control-css' => '8779483d', diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index f2a15e5605..b6985268db 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -593,6 +593,7 @@ final class ManiphestTaskDetailController extends ManiphestController { $handle_phids = array(); $any_linked = false; + $any_status = false; $idx = 0; $objects = array(); @@ -623,7 +624,6 @@ final class ManiphestTaskDetailController extends ManiphestController { ->setIcon($status->getIcon()) ->setColor($status->getColor()) ->setName($status->getName()); - } } @@ -750,9 +750,9 @@ final class ManiphestTaskDetailController extends ManiphestController { if ($repository_phid !== $last_repository) { $repository_link = null; if ($repository_phid) { - $repository_link = $handles[$repository_phid]->renderLink(); + $repository_handle = $handles[$repository_phid]; $rows[] = array( - $repository_link, + $repository_handle->renderLink(), ); $rowd[] = true; } @@ -772,6 +772,9 @@ final class ManiphestTaskDetailController extends ManiphestController { ->setIcon($handle->getIcon()); $status_view = $object['status']; + if ($status_view) { + $any_status = true; + } $revision_tags = array(); foreach ($object['revisionPHIDs'] as $link_phid) { @@ -797,16 +800,9 @@ final class ManiphestTaskDetailController extends ManiphestController { $changes_table = id(new AphrontTableView($rows)) ->setNoDataString(pht('This task has no related commits or revisions.')) ->setRowDividers($rowd) - ->setHeaders( - array( - null, - null, - null, - pht('Revision/Commit'), - )) ->setColumnClasses( array( - 'center', + 'indent center', null, null, 'wide pri object-link', @@ -814,14 +810,14 @@ final class ManiphestTaskDetailController extends ManiphestController { ->setColumnVisibility( array( true, - true, + $any_status, $any_linked, true, )) ->setDeviceVisibility( array( false, - true, + $any_status, false, true, )); diff --git a/src/view/control/AphrontTableView.php b/src/view/control/AphrontTableView.php index dff64169c2..a3c0a49be4 100644 --- a/src/view/control/AphrontTableView.php +++ b/src/view/control/AphrontTableView.php @@ -306,7 +306,7 @@ final class AphrontTableView extends AphrontView { 'td', array( 'class' => 'row-divider', - 'colspan' => count($headers), + 'colspan' => count($visibility), ), $value); $row_idx = -1; diff --git a/webroot/rsrc/css/aphront/table-view.css b/webroot/rsrc/css/aphront/table-view.css index c58e2ab2fc..e92f499634 100644 --- a/webroot/rsrc/css/aphront/table-view.css +++ b/webroot/rsrc/css/aphront/table-view.css @@ -61,6 +61,10 @@ padding: 8px 12px; } +.aphront-table-view td.indent { + padding-left: 24px; +} + .aphront-table-view th { border-bottom: 1px solid {$thinblueborder}; } From be2b8f4bcb62deb953049debacfbc7dca2e6edef Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 31 Oct 2019 12:43:40 -0700 Subject: [PATCH 15/15] Support querying projects by "Root Projects" in the UI, and "min/max depth" in the API Summary: Fixes T13441. Internally, projects can be queried by depth, but this is not exposed in the UI. Add a "Is root project?" contraint in the UI, and "minDepth" / "maxDepth" constraints to the API. Test Plan: - Used the UI to query root projects, got only root projects back. - Used "project.search" in the API to query combinations of root projects and projects at particular depths, got matching results. Maniphest Tasks: T13441 Differential Revision: https://secure.phabricator.com/D20886 --- src/__phutil_library_map__.php | 2 + .../query/PhabricatorProjectSearchEngine.php | 65 +++++++++++++++++++ .../field/PhabricatorSearchIntField.php | 22 +++++++ 3 files changed, 89 insertions(+) create mode 100644 src/applications/search/field/PhabricatorSearchIntField.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 83954637ce..2db41f0a69 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4652,6 +4652,7 @@ phutil_register_library_map(array( 'PhabricatorSearchHovercardController' => 'applications/search/controller/PhabricatorSearchHovercardController.php', 'PhabricatorSearchIndexVersion' => 'applications/search/storage/PhabricatorSearchIndexVersion.php', 'PhabricatorSearchIndexVersionDestructionEngineExtension' => 'applications/search/engineextension/PhabricatorSearchIndexVersionDestructionEngineExtension.php', + 'PhabricatorSearchIntField' => 'applications/search/field/PhabricatorSearchIntField.php', 'PhabricatorSearchManagementIndexWorkflow' => 'applications/search/management/PhabricatorSearchManagementIndexWorkflow.php', 'PhabricatorSearchManagementInitWorkflow' => 'applications/search/management/PhabricatorSearchManagementInitWorkflow.php', 'PhabricatorSearchManagementNgramsWorkflow' => 'applications/search/management/PhabricatorSearchManagementNgramsWorkflow.php', @@ -11273,6 +11274,7 @@ phutil_register_library_map(array( 'PhabricatorSearchHovercardController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchIndexVersion' => 'PhabricatorSearchDAO', 'PhabricatorSearchIndexVersionDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension', + 'PhabricatorSearchIntField' => 'PhabricatorSearchField', 'PhabricatorSearchManagementIndexWorkflow' => 'PhabricatorSearchManagementWorkflow', 'PhabricatorSearchManagementInitWorkflow' => 'PhabricatorSearchManagementWorkflow', 'PhabricatorSearchManagementNgramsWorkflow' => 'PhabricatorSearchManagementWorkflow', diff --git a/src/applications/project/query/PhabricatorProjectSearchEngine.php b/src/applications/project/query/PhabricatorProjectSearchEngine.php index 88a35676cc..cb179c995f 100644 --- a/src/applications/project/query/PhabricatorProjectSearchEngine.php +++ b/src/applications/project/query/PhabricatorProjectSearchEngine.php @@ -65,6 +65,35 @@ final class PhabricatorProjectSearchEngine pht( 'Pass true to find only milestones, or false to omit '. 'milestones.')), + id(new PhabricatorSearchThreeStateField()) + ->setLabel(pht('Root Projects')) + ->setKey('isRoot') + ->setOptions( + pht('(Show All)'), + pht('Show Only Root Projects'), + pht('Hide Root Projects')) + ->setDescription( + pht( + 'Pass true to find only root projects, or false to omit '. + 'root projects.')), + id(new PhabricatorSearchIntField()) + ->setLabel(pht('Minimum Depth')) + ->setKey('minDepth') + ->setIsHidden(true) + ->setDescription( + pht( + 'Find projects with a given minimum depth. Root projects '. + 'have depth 0, their immediate children have depth 1, and '. + 'so on.')), + id(new PhabricatorSearchIntField()) + ->setLabel(pht('Maximum Depth')) + ->setKey('maxDepth') + ->setIsHidden(true) + ->setDescription( + pht( + 'Find projects with a given maximum depth. Root projects '. + 'have depth 0, their immediate children have depth 1, and '. + 'so on.')), id(new PhabricatorSearchDatasourceField()) ->setLabel(pht('Subtypes')) ->setKey('subtypes') @@ -137,6 +166,42 @@ final class PhabricatorProjectSearchEngine $query->withIsMilestone($map['isMilestone']); } + $min_depth = $map['minDepth']; + $max_depth = $map['maxDepth']; + + if ($min_depth !== null || $max_depth !== null) { + if ($min_depth !== null && $max_depth !== null) { + if ($min_depth > $max_depth) { + throw new Exception( + pht( + 'Search constraint "minDepth" must be no larger than '. + 'search constraint "maxDepth".')); + } + } + } + + if ($map['isRoot'] !== null) { + if ($map['isRoot']) { + if ($max_depth === null) { + $max_depth = 0; + } else { + $max_depth = min(0, $max_depth); + } + + $query->withDepthBetween(null, 0); + } else { + if ($min_depth === null) { + $min_depth = 1; + } else { + $min_depth = max($min_depth, 1); + } + } + } + + if ($min_depth !== null || $max_depth !== null) { + $query->withDepthBetween($min_depth, $max_depth); + } + if ($map['parentPHIDs']) { $query->withParentProjectPHIDs($map['parentPHIDs']); } diff --git a/src/applications/search/field/PhabricatorSearchIntField.php b/src/applications/search/field/PhabricatorSearchIntField.php new file mode 100644 index 0000000000..70af934470 --- /dev/null +++ b/src/applications/search/field/PhabricatorSearchIntField.php @@ -0,0 +1,22 @@ +getInt($key); + } + + protected function newControl() { + return new AphrontFormTextControl(); + } + + protected function newConduitParameterType() { + return new ConduitIntParameterType(); + } + +}