From 8fe27800fcec6accdee1f5cf46ac39e39f613761 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 29 Apr 2014 15:01:50 -0700 Subject: [PATCH] Don't show document types in search for uninstalled applications Summary: Fixes T4917. Currently, if a user doesn't have access to, e.g., Phriction, they still get a checkbox in the search results to search for Wiki Documents. Those results will be filtered anyway, so this is confusing at best. Instead, bind PHID types to applications. This is a relatively tailored fix; some areas for potential future work: - Go through every PHID type and bind them all to applications. Vaguely nice to have, but doesn't get us anything for now. - If no searchable application is installed, we don't show you an error state. This isn't currently possible ("People" is always installed) but in the interest of generality we could throw an exception or something at least. - The elasticserach thing could probably constrain types to visible types, but we don't have a viewer there easily right now. Test Plan: Uninstalled Phriction, saw the checkbox vanish. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T4917 Differential Revision: https://secure.phabricator.com/D8904 --- .../phid/DifferentialPHIDTypeRevision.php | 4 ++ .../maniphest/phid/ManiphestPHIDTypeTask.php | 4 ++ .../phid/PhabricatorPeoplePHIDTypeUser.php | 4 ++ .../phid/type/PhabricatorPHIDType.php | 67 +++++++++++++++++++ .../pholio/phid/PholioPHIDTypeMock.php | 4 ++ .../phid/PhrictionPHIDTypeDocument.php | 4 ++ .../ponder/phid/PonderPHIDTypeQuestion.php | 4 ++ .../PhabricatorProjectPHIDTypeProject.php | 4 ++ .../PhabricatorRepositoryPHIDTypeCommit.php | 4 ++ ...abricatorSearchApplicationSearchEngine.php | 15 +++-- 10 files changed, 108 insertions(+), 6 deletions(-) diff --git a/src/applications/differential/phid/DifferentialPHIDTypeRevision.php b/src/applications/differential/phid/DifferentialPHIDTypeRevision.php index 9a815e150d..50a5f42cc4 100644 --- a/src/applications/differential/phid/DifferentialPHIDTypeRevision.php +++ b/src/applications/differential/phid/DifferentialPHIDTypeRevision.php @@ -12,6 +12,10 @@ final class DifferentialPHIDTypeRevision extends PhabricatorPHIDType { return pht('Revision'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorApplicationDifferential'; + } + public function newObject() { return new DifferentialRevision(); } diff --git a/src/applications/maniphest/phid/ManiphestPHIDTypeTask.php b/src/applications/maniphest/phid/ManiphestPHIDTypeTask.php index 7e1145fbd1..732ac82260 100644 --- a/src/applications/maniphest/phid/ManiphestPHIDTypeTask.php +++ b/src/applications/maniphest/phid/ManiphestPHIDTypeTask.php @@ -12,6 +12,10 @@ final class ManiphestPHIDTypeTask extends PhabricatorPHIDType { return pht('Task'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorApplicationManiphest'; + } + public function newObject() { return new ManiphestTask(); } diff --git a/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php b/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php index 9afc7c7fa4..49ddf01ed4 100644 --- a/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php +++ b/src/applications/people/phid/PhabricatorPeoplePHIDTypeUser.php @@ -12,6 +12,10 @@ final class PhabricatorPeoplePHIDTypeUser extends PhabricatorPHIDType { return pht('User'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorApplicationPeople'; + } + public function getTypeIcon() { return 'policy-all'; } diff --git a/src/applications/phid/type/PhabricatorPHIDType.php b/src/applications/phid/type/PhabricatorPHIDType.php index 4e901e2358..cbb7d0237a 100644 --- a/src/applications/phid/type/PhabricatorPHIDType.php +++ b/src/applications/phid/type/PhabricatorPHIDType.php @@ -13,6 +13,20 @@ abstract class PhabricatorPHIDType { return null; } + + /** + * Get the class name for the application this type belongs to. + * + * @return string|null Class name of the corresponding application, or null + * if the type is not bound to an application. + */ + public function getPHIDTypeApplicationClass() { + // TODO: Some day this should probably be abstract, but for now it only + // affects global search and there's no real burning need to go classify + // every PHID type. + return null; + } + /** * Build a @{class:PhabricatorPolicyAwareQuery} to load objects of this type * by PHID. @@ -103,6 +117,15 @@ abstract class PhabricatorPHIDType { throw new Exception("Not implemented!"); } + + /** + * Get all known PHID types. + * + * To get PHID types a given user has access to, see + * @{method:getAllInstalledTypes}. + * + * @return dict Map of type constants to types. + */ public static function getAllTypes() { static $types; if ($types === null) { @@ -133,4 +156,48 @@ abstract class PhabricatorPHIDType { return $types; } + + /** + * Get all PHID types of applications installed for a given viewer. + * + * @param PhabricatorUser Viewing user. + * @return dict Map of constants to installed + * types. + */ + public static function getAllInstalledTypes(PhabricatorUser $viewer) { + $all_types = self::getAllTypes(); + + $installed_types = array(); + + $app_classes = array(); + foreach ($all_types as $key => $type) { + $app_class = $type->getPHIDTypeApplicationClass(); + + if ($app_class === null) { + // If the PHID type isn't bound to an application, include it as + // installed. + $installed_types[$key] = $type; + continue; + } + + // Otherwise, we need to check if this application is installed before + // including the PHID type. + $app_classes[$app_class][$key] = $type; + } + + if ($app_classes) { + $apps = id(new PhabricatorApplicationQuery()) + ->setViewer($viewer) + ->withInstalled(true) + ->withClasses(array_keys($app_classes)) + ->execute(); + + foreach ($apps as $app_class => $app) { + $installed_types += $app_classes[$app_class]; + } + } + + return $installed_types; + } + } diff --git a/src/applications/pholio/phid/PholioPHIDTypeMock.php b/src/applications/pholio/phid/PholioPHIDTypeMock.php index abfd36d2db..d0567db1cd 100644 --- a/src/applications/pholio/phid/PholioPHIDTypeMock.php +++ b/src/applications/pholio/phid/PholioPHIDTypeMock.php @@ -12,6 +12,10 @@ final class PholioPHIDTypeMock extends PhabricatorPHIDType { return pht('Mock'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorApplicationPholio'; + } + public function newObject() { return new PholioMock(); } diff --git a/src/applications/phriction/phid/PhrictionPHIDTypeDocument.php b/src/applications/phriction/phid/PhrictionPHIDTypeDocument.php index b992b0c157..9202455acc 100644 --- a/src/applications/phriction/phid/PhrictionPHIDTypeDocument.php +++ b/src/applications/phriction/phid/PhrictionPHIDTypeDocument.php @@ -12,6 +12,10 @@ final class PhrictionPHIDTypeDocument extends PhabricatorPHIDType { return pht('Wiki Document'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorApplicationPhriction'; + } + public function newObject() { return new PhrictionDocument(); } diff --git a/src/applications/ponder/phid/PonderPHIDTypeQuestion.php b/src/applications/ponder/phid/PonderPHIDTypeQuestion.php index 2f42e91371..10000df7e0 100644 --- a/src/applications/ponder/phid/PonderPHIDTypeQuestion.php +++ b/src/applications/ponder/phid/PonderPHIDTypeQuestion.php @@ -12,6 +12,10 @@ final class PonderPHIDTypeQuestion extends PhabricatorPHIDType { return pht('Question'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorApplicationPonder'; + } + public function newObject() { return new PonderQuestion(); } diff --git a/src/applications/project/phid/PhabricatorProjectPHIDTypeProject.php b/src/applications/project/phid/PhabricatorProjectPHIDTypeProject.php index 6c95313766..41a86f1d9b 100644 --- a/src/applications/project/phid/PhabricatorProjectPHIDTypeProject.php +++ b/src/applications/project/phid/PhabricatorProjectPHIDTypeProject.php @@ -12,6 +12,10 @@ final class PhabricatorProjectPHIDTypeProject extends PhabricatorPHIDType { return pht('Project'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorApplicationProject'; + } + public function getTypeIcon() { return 'policy-project'; } diff --git a/src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php b/src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php index 881315d256..62075cabfb 100644 --- a/src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php +++ b/src/applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php @@ -12,6 +12,10 @@ final class PhabricatorRepositoryPHIDTypeCommit extends PhabricatorPHIDType { return pht('Commit'); } + public function getPHIDTypeApplicationClass() { + return 'PhabricatorApplicationDiffusion'; + } + public function newObject() { return new PhabricatorRepositoryCommit(); } diff --git a/src/applications/search/query/PhabricatorSearchApplicationSearchEngine.php b/src/applications/search/query/PhabricatorSearchApplicationSearchEngine.php index d68558e6a0..53461d019a 100644 --- a/src/applications/search/query/PhabricatorSearchApplicationSearchEngine.php +++ b/src/applications/search/query/PhabricatorSearchApplicationSearchEngine.php @@ -96,7 +96,7 @@ final class PhabricatorSearchApplicationSearchEngine $type_values = $saved->getParameter('types', array()); $type_values = array_fuse($type_values); - $types = self::getIndexableDocumentTypes(); + $types = self::getIndexableDocumentTypes($this->requireViewer()); $types_control = id(new AphrontFormCheckboxControl()) ->setLabel(pht('Document Types')); @@ -190,18 +190,21 @@ final class PhabricatorSearchApplicationSearchEngine return parent::buildSavedQueryFromBuiltin($query_key); } - public static function getIndexableDocumentTypes() { + public static function getIndexableDocumentTypes( + PhabricatorUser $viewer = null) { + // TODO: This is inelegant and not very efficient, but gets us reasonable // results. It would be nice to do this more elegantly. - // TODO: We should hide types associated with applications the user can - // not access. There's no reasonable way to do this right now. - $indexers = id(new PhutilSymbolLoader()) ->setAncestorClass('PhabricatorSearchDocumentIndexer') ->loadObjects(); - $types = PhabricatorPHIDType::getAllTypes(); + if ($viewer) { + $types = PhabricatorPHIDType::getAllInstalledTypes($viewer); + } else { + $types = PhabricatorPHIDType::getAllTypes(); + } $results = array(); foreach ($types as $type) {