From f0dc06529033062a6c96425cb834c67b40773e7c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Feb 2021 17:12:37 -0800 Subject: [PATCH 01/17] Lift bulk tests for "many users against one object" capabilities into "PolicyFilterSet" Summary: Ref T13602. Currently, the policy framework can not execute "test if many users can see one object" particluarly efficiently. This test must be executed more broadly to implement the changes in T13602. To avoid making this any worse than it already is, lift this block into a wrapper class that has a bulk queue + fetch API and could eventually be optimized. Test Plan: Viewed a task with an `@mention` of a user without permission to see it in the summary, saw it rendered in a disabled style. Maniphest Tasks: T13602 Differential Revision: https://secure.phabricator.com/D21546 --- src/__phutil_library_map__.php | 2 + .../markup/PhabricatorMentionRemarkupRule.php | 35 ++++-- .../filter/PhabricatorPolicyFilterSet.php | 105 ++++++++++++++++++ 3 files changed, 131 insertions(+), 11 deletions(-) create mode 100644 src/applications/policy/filter/PhabricatorPolicyFilterSet.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8af718e70b..57e9a22a57 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4270,6 +4270,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyExplainController' => 'applications/policy/controller/PhabricatorPolicyExplainController.php', 'PhabricatorPolicyFavoritesSetting' => 'applications/settings/setting/PhabricatorPolicyFavoritesSetting.php', 'PhabricatorPolicyFilter' => 'applications/policy/filter/PhabricatorPolicyFilter.php', + 'PhabricatorPolicyFilterSet' => 'applications/policy/filter/PhabricatorPolicyFilterSet.php', 'PhabricatorPolicyInterface' => 'applications/policy/interface/PhabricatorPolicyInterface.php', 'PhabricatorPolicyManagementShowWorkflow' => 'applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php', 'PhabricatorPolicyManagementUnlockWorkflow' => 'applications/policy/management/PhabricatorPolicyManagementUnlockWorkflow.php', @@ -10920,6 +10921,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyExplainController' => 'PhabricatorPolicyController', 'PhabricatorPolicyFavoritesSetting' => 'PhabricatorInternalSetting', 'PhabricatorPolicyFilter' => 'Phobject', + 'PhabricatorPolicyFilterSet' => 'Phobject', 'PhabricatorPolicyInterface' => 'PhabricatorPHIDInterface', 'PhabricatorPolicyManagementShowWorkflow' => 'PhabricatorPolicyManagementWorkflow', 'PhabricatorPolicyManagementUnlockWorkflow' => 'PhabricatorPolicyManagementWorkflow', diff --git a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php index 60d4a8168f..6cf3b4b24b 100644 --- a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php +++ b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php @@ -87,24 +87,37 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { $engine->setTextMetadata($mentioned_key, $mentioned); $context_object = $engine->getConfig('contextObject'); + $policy_object = null; + if ($context_object) { + if ($context_object instanceof PhabricatorPolicyInterface) { + $policy_object = $context_object; + } + } + + if ($policy_object) { + $policy_set = new PhabricatorPolicyFilterSet(); + foreach ($actual_users as $user) { + $policy_set->addCapability( + $user, + $policy_object, + PhabricatorPolicyCapability::CAN_VIEW); + } + } + foreach ($metadata as $username => $tokens) { $exists = isset($actual_users[$username]); - $user_has_no_permission = false; + $user_can_not_view = false; if ($exists) { $user = $actual_users[$username]; Javelin::initBehavior('phui-hovercards'); // Check if the user has view access to the object she was mentioned in - if ($context_object - && $context_object instanceof PhabricatorPolicyInterface) { - if (!PhabricatorPolicyFilter::hasCapability( + if ($policy_object) { + $user_can_not_view = !$policy_set->hasCapability( $user, - $context_object, - PhabricatorPolicyCapability::CAN_VIEW)) { - // User mentioned has no permission to this object - $user_has_no_permission = true; - } + $policy_object, + PhabricatorPolicyCapability::CAN_VIEW); } $user_href = '/p/'.$user->getUserName().'/'; @@ -112,7 +125,7 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { if ($engine->isHTMLMailMode()) { $user_href = PhabricatorEnv::getProductionURI($user_href); - if ($user_has_no_permission) { + if ($user_can_not_view) { $colors = ' border-color: #92969D; color: #92969D; @@ -146,7 +159,7 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { ->setName('@'.$user->getUserName()) ->setHref($user_href); - if ($user_has_no_permission) { + if ($user_can_not_view) { $tag->addClass('phabricator-remarkup-mention-nopermission'); } diff --git a/src/applications/policy/filter/PhabricatorPolicyFilterSet.php b/src/applications/policy/filter/PhabricatorPolicyFilterSet.php new file mode 100644 index 0000000000..c1e8156f09 --- /dev/null +++ b/src/applications/policy/filter/PhabricatorPolicyFilterSet.php @@ -0,0 +1,105 @@ +getUserKey($user); + $this->users[$user_key] = $user; + + $object_key = $this->getObjectKey($object); + $this->objects[$object_key] = $object; + + if (!isset($this->capabilities[$capability][$user_key][$object_key])) { + $this->capabilities[$capability][$user_key][$object_key] = true; + $this->queue[$capability][$user_key][$object_key] = true; + } + + return $this; + } + + public function hasCapability( + PhabricatorUser $user, + PhabricatorPolicyInterface $object, + $capability) { + + $user_key = $this->getUserKey($user); + $this->users[$user_key] = $user; + + $object_key = $this->getObjectKey($object); + $this->objects[$object_key] = $object; + + if (!isset($this->capabilities[$capability][$user_key][$object_key])) { + throw new Exception( + pht( + 'Capability "%s" for user "%s" on object "%s" is being resolved, '. + 'but was never queued with "addCapability()".', + $capability, + $user_key, + $object_key)); + } + + if (!isset($this->results[$capability][$user_key][$object_key])) { + $this->resolveCapabilities(); + } + + return $this->results[$capability][$user_key][$object_key]; + } + + private function getUserKey(PhabricatorUser $user) { + return $user->getCacheFragment(); + } + + private function getObjectKey(PhabricatorPolicyInterface $object) { + $object_phid = $object->getPHID(); + + if (!$object_phid) { + throw new Exception( + pht( + 'Unable to perform capability tests on an object (of class "%s") '. + 'with no PHID.', + get_class($object))); + } + + return $object_phid; + } + + private function resolveCapabilities() { + + // This class is primarily used to test if a list of users (like + // subscribers) can see a single object. It is not structured in a way + // that makes this particularly efficient, and performance would probably + // be improved if filtering supported this use case more narrowly. + + foreach ($this->queue as $capability => $user_map) { + foreach ($user_map as $user_key => $object_map) { + $user = $this->users[$user_key]; + $objects = array_select_keys($this->objects, array_keys($object_map)); + + $filter = id(new PhabricatorPolicyFilter()) + ->setViewer($user) + ->requireCapabilities(array($capability)); + $results = $filter->apply($objects); + + foreach ($object_map as $object_key => $object) { + $has_capability = (bool)isset($results[$object_key]); + $this->results[$capability][$user_key][$object_key] = $has_capability; + } + } + } + + $this->queue = array(); + } + +} From a4cb2bb7724757a8f9dd4d68fde3bda2fd6c7895 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 5 Feb 2021 07:33:19 -0800 Subject: [PATCH 02/17] When a subscriber can't see an object, clearly show that they're missing the permission in the curtain UI Summary: Ref T13602. When a subscriber can't see an object, it's currently hard to figure it out. Show this status clearly in the curtain UI. Test Plan: {F8382865} Maniphest Tasks: T13602 Differential Revision: https://secure.phabricator.com/D21547 --- resources/celerity/map.php | 6 +- .../phid/PhabricatorObjectHandle.php | 63 +++++++++++++++++++ .../filter/PhabricatorPolicyFilterSet.php | 62 ++++++++++++++++++ ...abricatorSubscriptionsCurtainExtension.php | 15 ++++- src/view/phui/PHUICurtainObjectRefView.php | 29 +++++++++ .../css/phui/phui-curtain-object-ref-view.css | 13 ++++ 6 files changed, 184 insertions(+), 4 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 0ae7849bfb..155ace9d18 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '0e3cf785', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '937616c0', + 'core.pkg.css' => '970b3ceb', 'core.pkg.js' => 'adc34883', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', @@ -151,7 +151,7 @@ return array( 'rsrc/css/phui/phui-comment-form.css' => '68a2d99a', 'rsrc/css/phui/phui-comment-panel.css' => 'ec4e31c0', 'rsrc/css/phui/phui-crumbs-view.css' => '614f43cf', - 'rsrc/css/phui/phui-curtain-object-ref-view.css' => '12404744', + 'rsrc/css/phui/phui-curtain-object-ref-view.css' => '5f752bdb', 'rsrc/css/phui/phui-curtain-view.css' => '68c5efb6', 'rsrc/css/phui/phui-document-pro.css' => 'b9613a10', 'rsrc/css/phui/phui-document-summary.css' => 'b068eed1', @@ -845,7 +845,7 @@ return array( 'phui-comment-form-css' => '68a2d99a', 'phui-comment-panel-css' => 'ec4e31c0', 'phui-crumbs-view-css' => '614f43cf', - 'phui-curtain-object-ref-view-css' => '12404744', + 'phui-curtain-object-ref-view-css' => '5f752bdb', 'phui-curtain-view-css' => '68c5efb6', 'phui-document-summary-view-css' => 'b068eed1', 'phui-document-view-css' => '52b748a5', diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php index 86f0f848c0..974c5faf39 100644 --- a/src/applications/phid/PhabricatorObjectHandle.php +++ b/src/applications/phid/PhabricatorObjectHandle.php @@ -32,6 +32,7 @@ final class PhabricatorObjectHandle private $tokenIcon; private $commandLineObjectName; private $mailStampName; + private $capabilities = array(); public function setIcon($icon) { $this->icon = $icon; @@ -388,6 +389,68 @@ final class PhabricatorObjectHandle return idx($types, $this->getType()); } + public function hasCapabilities() { + return ($this->getType() === PhabricatorPeopleUserPHIDType::TYPECONST); + } + + public function attachCapability( + PhabricatorPolicyInterface $object, + $capability, + $has_capability) { + + if (!$this->hasCapabilities()) { + throw new Exception( + pht( + 'Attempting to attach capability ("%s") for object ("%s") to '. + 'handle, but this handle (of type "%s") can not have '. + 'capabilities.', + $capability, + get_class($object), + $this->getType())); + } + + $object_key = $this->getObjectCapabilityKey($object); + $this->capabilities[$object_key][$capability] = $has_capability; + + return $this; + } + + public function hasViewCapability(PhabricatorPolicyInterface $object) { + return $this->hasCapability($object, PhabricatorPolicyCapability::CAN_VIEW); + } + + private function hasCapability( + PhabricatorPolicyInterface $object, + $capability) { + + $object_key = $this->getObjectCapabilityKey($object); + + if (!isset($this->capabilities[$object_key][$capability])) { + throw new Exception( + pht( + 'Attempting to test capability "%s" for handle of type "%s", but '. + 'this capability has not been attached.', + $capability, + $this->getType())); + } + + return $this->capabilities[$object_key][$capability]; + } + + private function getObjectCapabilityKey(PhabricatorPolicyInterface $object) { + $object_phid = $object->getPHID(); + + if (!$object_phid) { + throw new Exception( + pht( + 'Object (of class "%s") has no PHID, so handles can not interact '. + 'with capabilities for it.', + get_class($object))); + } + + return $object_phid; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/policy/filter/PhabricatorPolicyFilterSet.php b/src/applications/policy/filter/PhabricatorPolicyFilterSet.php index c1e8156f09..1834504dc4 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilterSet.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilterSet.php @@ -102,4 +102,66 @@ final class PhabricatorPolicyFilterSet $this->queue = array(); } + public static function loadHandleViewCapabilities( + $viewer, + $handles, + array $objects) { + + $capabilities = array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + + assert_instances_of($objects, 'PhabricatorPolicyInterface'); + + if (!$objects) { + return; + } + + $viewer_map = array(); + foreach ($handles as $handle_key => $handle) { + if (!$handle->hasCapabilities()) { + continue; + } + $viewer_map[$handle->getPHID()] = $handle_key; + } + + if (!$viewer_map) { + return; + } + + $users = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withPHIDs(array_keys($viewer_map)) + ->execute(); + $users = mpull($users, null, 'getPHID'); + + $filter_set = new self(); + + foreach ($users as $user_phid => $user) { + foreach ($objects as $object) { + foreach ($capabilities as $capability) { + $filter_set->addCapability($user, $object, $capability); + } + } + } + + foreach ($users as $user_phid => $user) { + $handle_key = $viewer_map[$user_phid]; + $handle = $handles[$handle_key]; + foreach ($objects as $object) { + foreach ($capabilities as $capability) { + $has_capability = $filter_set->hasCapability( + $user, + $object, + $capability); + + $handle->attachCapability( + $object, + $capability, + $has_capability); + } + } + } + } + } diff --git a/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsCurtainExtension.php b/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsCurtainExtension.php index a6df97b095..1ada983ad3 100644 --- a/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsCurtainExtension.php +++ b/src/applications/subscriptions/engineextension/PhabricatorSubscriptionsCurtainExtension.php @@ -62,17 +62,30 @@ final class PhabricatorSubscriptionsCurtainExtension $handles = $viewer->loadHandles($visible_phids); } + PhabricatorPolicyFilterSet::loadHandleViewCapabilities( + $viewer, + $handles, + array($object)); + $ref_list = id(new PHUICurtainObjectRefListView()) ->setViewer($viewer) ->setEmptyMessage(pht('None')); foreach ($visible_phids as $phid) { + $handle = $handles[$phid]; + $ref = $ref_list->newObjectRefView() - ->setHandle($handles[$phid]); + ->setHandle($handle); if ($phid === $viewer_phid) { $ref->setHighlighted(true); } + + if ($handle->hasCapabilities()) { + if (!$handle->hasViewCapability($object)) { + $ref->setExiled(true); + } + } } if ($show_all) { diff --git a/src/view/phui/PHUICurtainObjectRefView.php b/src/view/phui/PHUICurtainObjectRefView.php index f9f383a2ec..aa07d7fc4f 100644 --- a/src/view/phui/PHUICurtainObjectRefView.php +++ b/src/view/phui/PHUICurtainObjectRefView.php @@ -6,6 +6,7 @@ final class PHUICurtainObjectRefView private $handle; private $epoch; private $highlighted; + private $exiled; public function setHandle(PhabricatorObjectHandle $handle) { $this->handle = $handle; @@ -22,6 +23,11 @@ final class PHUICurtainObjectRefView return $this; } + public function setExiled($is_exiled) { + $this->exiled = $is_exiled; + return $this; + } + protected function getTagAttributes() { $classes = array(); $classes[] = 'phui-curtain-object-ref-view'; @@ -29,6 +35,11 @@ final class PHUICurtainObjectRefView if ($this->highlighted) { $classes[] = 'phui-curtain-object-ref-view-highlighted'; } + + if ($this->exiled) { + $classes[] = 'phui-curtain-object-ref-view-exiled'; + } + $classes = implode(' ', $classes); return array( @@ -60,6 +71,24 @@ final class PHUICurtainObjectRefView $more_rows[] = phutil_tag('tr', array(), $epoch_cells); } + if ($this->exiled) { + $exiled_view = array( + id(new PHUIIconView())->setIcon('fa-eye-slash red'), + ' ', + pht('No View Permission'), + ); + + $exiled_cells = array(); + $exiled_cells[] = phutil_tag( + 'td', + array( + 'class' => 'phui-curtain-object-ref-view-exiled-cell', + ), + $exiled_view); + + $more_rows[] = phutil_tag('tr', array(), $exiled_cells); + } + $header_cells = array(); $image_view = $this->newImage(); diff --git a/webroot/rsrc/css/phui/phui-curtain-object-ref-view.css b/webroot/rsrc/css/phui/phui-curtain-object-ref-view.css index 3becc1ca84..8ae14be04f 100644 --- a/webroot/rsrc/css/phui/phui-curtain-object-ref-view.css +++ b/webroot/rsrc/css/phui/phui-curtain-object-ref-view.css @@ -12,6 +12,10 @@ border-radius: 3px; } +.phui-curtain-object-ref-view + .phui-curtain-object-ref-view { + margin-top: 1px; +} + .phui-curtain-object-ref-view-image-cell { min-width: 32px; padding-bottom: 24px; @@ -82,3 +86,12 @@ .phui-curtain-object-ref-view-highlighted { background: {$bluebackground}; } + +.phui-curtain-object-ref-view-exiled { + background: {$lightred}; + opacity: 0.75; +} + +.phui-curtain-object-ref-view-exiled-cell { + color: {$red}; +} From 58bbd6ee8838b9a4fabbd91c12295c44f51b9dbc Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 5 Feb 2021 08:13:35 -0800 Subject: [PATCH 03/17] Propagate the "ContextObject" to Remarkup rendering in timelines Summary: Ref T13602. Currently, timeline comment rendering does not (by default) propagate the context object to the rendering layer. This means that `@mentions` of users who can't see the object aren't rendered properly (currently: they show up as blue, but should show up as grey). Pass the context down the stack and into the remarkup engine. Test Plan: {F8382905} Maniphest Tasks: T13602 Differential Revision: https://secure.phabricator.com/D21548 --- .../engine/PhabricatorTimelineEngine.php | 1 + .../PhabricatorApplicationTransactionView.php | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/applications/transactions/engine/PhabricatorTimelineEngine.php b/src/applications/transactions/engine/PhabricatorTimelineEngine.php index c6c6cd44a4..2fa0b1451d 100644 --- a/src/applications/transactions/engine/PhabricatorTimelineEngine.php +++ b/src/applications/transactions/engine/PhabricatorTimelineEngine.php @@ -84,6 +84,7 @@ abstract class PhabricatorTimelineEngine return $view ->setViewer($viewer) + ->setObject($object) ->setObjectPHID($object->getPHID()) ->setTransactions($xactions); } diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php index 209b6baf64..ee2020f890 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionView.php @@ -9,6 +9,7 @@ class PhabricatorApplicationTransactionView extends AphrontView { private $engine; private $showEditActions = true; private $isPreview; + private $object; private $objectPHID; private $shouldTerminate = false; private $quoteTargetID; @@ -41,6 +42,16 @@ class PhabricatorApplicationTransactionView extends AphrontView { return $this->quoteTargetID; } + public function setObject( + PhabricatorApplicationTransactionInterface $object) { + $this->object = $object; + return $this; + } + + private function getObject() { + return $this->object; + } + public function setObjectPHID($object_phid) { $this->objectPHID = $object_phid; return $this; @@ -238,6 +249,12 @@ class PhabricatorApplicationTransactionView extends AphrontView { $engine = id(new PhabricatorMarkupEngine()) ->setViewer($this->getViewer()); + + $object = $this->getObject(); + if ($object) { + $engine->setContextObject($object); + } + foreach ($this->transactions as $xaction) { if (!$xaction->hasComment()) { continue; From 2aac3156f791f05a0c82653aac75379b113e48ad Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 13 Feb 2021 11:23:54 -0800 Subject: [PATCH 04/17] Restructure Hovercards to support more context information Summary: Ref T13602. Currently, Hovercards are functions only of the object they represent (and the viewer, etc). Recent changes to how users who can't see an object are rendered motivate making them a function of both the object they represent //and// the context in which they are being viewed. In particular, this enables a hovecard for a user to explain "This user can't see the thing you're lookign at right now.", so visual "exiled" markers can have a path forward toward discovery. Test Plan: - This change isn't expected to affect any behavior. - Viewed hovercards, moused over/out, resized windows, viewed standalone cards, viewed debug cards, saw no behavioral changes. Maniphest Tasks: T13602 Differential Revision: https://secure.phabricator.com/D21553 --- resources/celerity/map.php | 50 ++-- resources/celerity/packages.php | 1 + src/aphront/AphrontRequest.php | 37 +++ .../PhabricatorSearchHovercardController.php | 40 +++- .../PhabricatorSystemDebugUIEventListener.php | 2 +- webroot/rsrc/js/core/Hovercard.js | 167 +------------ webroot/rsrc/js/core/HovercardList.js | 226 ++++++++++++++++++ webroot/rsrc/js/core/behavior-hovercard.js | 68 ++---- 8 files changed, 349 insertions(+), 242 deletions(-) create mode 100644 webroot/rsrc/js/core/HovercardList.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 155ace9d18..06469b117a 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => '0e3cf785', 'conpherence.pkg.js' => '020aebcf', 'core.pkg.css' => '970b3ceb', - 'core.pkg.js' => 'adc34883', + 'core.pkg.js' => '2fe70e3d', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', 'differential.pkg.js' => '5080baf4', @@ -460,7 +460,8 @@ return array( 'rsrc/js/core/DraggableList.js' => '0169e425', 'rsrc/js/core/Favicon.js' => '7930776a', 'rsrc/js/core/FileUpload.js' => 'ab85e184', - 'rsrc/js/core/Hovercard.js' => '074f0783', + 'rsrc/js/core/Hovercard.js' => 'd9d29a5f', + 'rsrc/js/core/HovercardList.js' => '10a5f4bf', 'rsrc/js/core/KeyboardShortcut.js' => '1a844c06', 'rsrc/js/core/KeyboardShortcutManager.js' => '81debc48', 'rsrc/js/core/MultirowRowManager.js' => '5b54c823', @@ -485,7 +486,7 @@ return array( 'rsrc/js/core/behavior-global-drag-and-drop.js' => '1cab0e9a', 'rsrc/js/core/behavior-high-security-warning.js' => 'dae2d55b', 'rsrc/js/core/behavior-history-install.js' => '6a1583a8', - 'rsrc/js/core/behavior-hovercard.js' => '6c379000', + 'rsrc/js/core/behavior-hovercard.js' => '3f446c72', 'rsrc/js/core/behavior-keyboard-pager.js' => '1325b731', 'rsrc/js/core/behavior-keyboard-shortcuts.js' => '42c44e8b', 'rsrc/js/core/behavior-lightbox-attachments.js' => 'c7e748bf', @@ -670,7 +671,7 @@ return array( 'javelin-behavior-pholio-mock-view' => '5aa1544e', 'javelin-behavior-phui-dropdown-menu' => '5cf0501a', 'javelin-behavior-phui-file-upload' => 'e150bd50', - 'javelin-behavior-phui-hovercards' => '6c379000', + 'javelin-behavior-phui-hovercards' => '3f446c72', 'javelin-behavior-phui-selectable-list' => 'b26a41e4', 'javelin-behavior-phui-submenu' => 'b5e9bff9', 'javelin-behavior-phui-tab-group' => '242aa08b', @@ -858,7 +859,8 @@ return array( 'phui-formation-view-css' => 'd2dec8ed', 'phui-head-thing-view-css' => 'd7f293df', 'phui-header-view-css' => '36c86a58', - 'phui-hovercard' => '074f0783', + 'phui-hovercard' => 'd9d29a5f', + 'phui-hovercard-list' => '10a5f4bf', 'phui-hovercard-view-css' => '6ca90fa0', 'phui-icon-set-selector-css' => '7aa5f3ec', 'phui-icon-view-css' => '4cbc684a', @@ -986,13 +988,6 @@ return array( 'javelin-uri', 'phabricator-notification', ), - '074f0783' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-vector', - 'javelin-request', - 'javelin-uri', - ), '0889b835' => array( 'javelin-install', 'javelin-event', @@ -1030,6 +1025,14 @@ return array( 'javelin-workflow', 'phuix-icon-view', ), + '10a5f4bf' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-vector', + 'javelin-request', + 'javelin-uri', + 'phui-hovercard', + ), '111bfd2d' => array( 'javelin-install', ), @@ -1266,6 +1269,14 @@ return array( 'phabricator-drag-and-drop-file-upload', 'phabricator-draggable-list', ), + '3f446c72' => array( + 'javelin-behavior', + 'javelin-behavior-device', + 'javelin-stratcom', + 'javelin-vector', + 'phui-hovercard', + 'phui-hovercard-list', + ), '407ee861' => array( 'javelin-behavior', 'javelin-uri', @@ -1557,13 +1568,6 @@ return array( 'javelin-workflow', 'javelin-magical-init', ), - '6c379000' => array( - 'javelin-behavior', - 'javelin-behavior-device', - 'javelin-stratcom', - 'javelin-vector', - 'phui-hovercard', - ), '6cfa0008' => array( 'javelin-dom', 'javelin-dynval', @@ -2132,6 +2136,13 @@ return array( 'javelin-util', 'phabricator-shaped-request', ), + 'd9d29a5f' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-vector', + 'javelin-request', + 'javelin-uri', + ), 'da15d3dc' => array( 'phui-oi-list-view-css', ), @@ -2367,6 +2378,7 @@ return array( 'javelin-behavior-global-drag-and-drop', 'javelin-behavior-phabricator-reveal-content', 'phui-hovercard', + 'phui-hovercard-list', 'javelin-behavior-phui-hovercards', 'javelin-color', 'javelin-fx', diff --git a/resources/celerity/packages.php b/resources/celerity/packages.php index a5dabb6014..47e13932de 100644 --- a/resources/celerity/packages.php +++ b/resources/celerity/packages.php @@ -60,6 +60,7 @@ return array( 'javelin-behavior-global-drag-and-drop', 'javelin-behavior-phabricator-reveal-content', 'phui-hovercard', + 'phui-hovercard-list', 'javelin-behavior-phui-hovercards', 'javelin-color', 'javelin-fx', diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php index d8310386c2..a3fa05cca2 100644 --- a/src/aphront/AphrontRequest.php +++ b/src/aphront/AphrontRequest.php @@ -224,6 +224,43 @@ final class AphrontRequest extends Phobject { } + /** + * @task data + */ + public function getJSONMap($name, $default = array()) { + if (!isset($this->requestData[$name])) { + return $default; + } + + $raw_data = phutil_string_cast($this->requestData[$name]); + $raw_data = trim($raw_data); + if (!strlen($raw_data)) { + return $default; + } + + if ($raw_data[0] !== '{') { + throw new Exception( + pht( + 'Request parameter "%s" is not formatted properly. Expected a '. + 'JSON object, but value does not start with "{".', + $name)); + } + + try { + $json_object = phutil_json_decode($raw_data); + } catch (PhutilJSONParserException $ex) { + throw new Exception( + pht( + 'Request parameter "%s" is not formatted properly. Expected a '. + 'JSON object, but encountered a syntax error: %s.', + $name, + $ex->getMessage())); + } + + return $json_object; + } + + /** * @task data */ diff --git a/src/applications/search/controller/PhabricatorSearchHovercardController.php b/src/applications/search/controller/PhabricatorSearchHovercardController.php index ca83d1896d..0eaae1a138 100644 --- a/src/applications/search/controller/PhabricatorSearchHovercardController.php +++ b/src/applications/search/controller/PhabricatorSearchHovercardController.php @@ -9,7 +9,8 @@ final class PhabricatorSearchHovercardController public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); - $phids = $request->getArr('phids'); + + $cards = $request->getJSONMap('cards'); // If object names are provided, look them up and pretend they were // passed as additional PHIDs. This is primarily useful for debugging, @@ -23,18 +24,29 @@ final class PhabricatorSearchHovercardController ->execute(); foreach ($named_objects as $object) { - $phids[] = $object->getPHID(); + $cards[] = array( + 'objectPHID' => $object->getPHID(), + ); } } + $object_phids = array(); + $handle_phids = array(); + foreach ($cards as $card) { + $object_phid = idx($card, 'objectPHID'); + + $handle_phids[] = $object_phid; + $object_phids[] = $object_phid; + } + $handles = id(new PhabricatorHandleQuery()) ->setViewer($viewer) - ->withPHIDs($phids) + ->withPHIDs($handle_phids) ->execute(); $objects = id(new PhabricatorObjectQuery()) ->setViewer($viewer) - ->withPHIDs($phids) + ->withPHIDs($object_phids) ->execute(); $objects = mpull($objects, null, 'getPHID'); @@ -67,10 +79,12 @@ final class PhabricatorSearchHovercardController array_select_keys($objects, $extension_phids)); } - $cards = array(); - foreach ($phids as $phid) { - $handle = $handles[$phid]; - $object = idx($objects, $phid); + $results = array(); + foreach ($cards as $card_key => $card) { + $object_phid = $card['objectPHID']; + + $handle = $handles[$object_phid]; + $object = idx($objects, $object_phid); $hovercard = id(new PHUIHovercardView()) ->setUser($viewer) @@ -90,18 +104,18 @@ final class PhabricatorSearchHovercardController } } - $cards[$phid] = $hovercard; + $results[$card_key] = $hovercard; } if ($request->isAjax()) { return id(new AphrontAjaxResponse())->setContent( array( - 'cards' => $cards, + 'cards' => $results, )); } - foreach ($cards as $key => $hovercard) { - $cards[$key] = phutil_tag('div', + foreach ($results as $key => $hovercard) { + $results[$key] = phutil_tag('div', array( 'class' => 'ml', ), @@ -109,7 +123,7 @@ final class PhabricatorSearchHovercardController } return $this->newPage() - ->appendChild($cards) + ->appendChild($results) ->setShowFooter(false); } diff --git a/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php b/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php index 18b94323b6..5bfb6843c4 100644 --- a/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php +++ b/src/applications/system/events/PhabricatorSystemDebugUIEventListener.php @@ -42,7 +42,7 @@ final class PhabricatorSystemDebugUIEventListener $submenu[] = id(new PhabricatorActionView()) ->setIcon('fa-address-card-o') ->setName(pht('View Hovercard')) - ->setHref(urisprintf('/search/hovercard/?phids[]=%s', $phid)); + ->setHref(urisprintf('/search/hovercard/?names=%s', $phid)); $developer_action = id(new PhabricatorActionView()) ->setName(pht('Advanced/Developer...')) diff --git a/webroot/rsrc/js/core/Hovercard.js b/webroot/rsrc/js/core/Hovercard.js index 65e1455252..63fc8eab7f 100644 --- a/webroot/rsrc/js/core/Hovercard.js +++ b/webroot/rsrc/js/core/Hovercard.js @@ -10,163 +10,18 @@ JX.install('Hovercard', { - statics : { - _node : null, - _activeRoot : null, - _visiblePHID : null, - _alignment: null, + properties: { + hovercardKey: null, + objectPHID: null, + isLoading: false, + isLoaded: false, + content: null + }, - fetchUrl : '/search/hovercard/', - - /** - * Hovercard storage. {"PHID-XXXX-YYYY":"<...>", ...} - */ - _cards : {}, - - getAnchor : function() { - return this._activeRoot; - }, - - getCard : function() { - var self = JX.Hovercard; - return self._node; - }, - - getAlignment: function() { - var self = JX.Hovercard; - return self._alignment; - }, - - show : function(root, phid) { - var self = JX.Hovercard; - - if (root === this._activeRoot) { - return; - } - - self.hide(); - - self._visiblePHID = phid; - self._activeRoot = root; - - if (!(phid in self._cards)) { - self._load([phid]); - } else { - self._drawCard(phid); - } - }, - - _drawCard : function(phid) { - var self = JX.Hovercard; - // card is loading... - if (self._cards[phid] === true) { - return; - } - // Not the current requested card - if (phid != self._visiblePHID) { - return; - } - // Not loaded - if (!(phid in self._cards)) { - return; - } - - var root = self._activeRoot; - var node = JX.$N('div', - { className: 'jx-hovercard-container' }, - JX.$H(self._cards[phid])); - - self._node = node; - - // Append the card to the document, but offscreen, so we can measure it. - node.style.left = '-10000px'; - document.body.appendChild(node); - - // Retrieve size from child (wrapper), since node gives wrong dimensions? - var child = node.firstChild; - var p = JX.$V(root); - var d = JX.Vector.getDim(root); - var n = JX.Vector.getDim(child); - var v = JX.Vector.getViewport(); - var s = JX.Vector.getScroll(); - - // Move the tip so it's nicely aligned. - var margin = 20; - - - // Try to align the card directly above the link, with left borders - // touching. - var x = p.x; - - // If this would push us off the right side of the viewport, push things - // back to the left. - if ((x + n.x + margin) > (s.x + v.x)) { - x = (s.x + v.x) - n.x - margin; - } - - // Try to put the card above the link. - var y = p.y - n.y - margin; - self._alignment = 'north'; - - // If the card is near the top of the window, show it beneath the - // link we're hovering over instead. - if ((y - margin) < s.y) { - y = p.y + d.y + margin; - self._alignment = 'south'; - } - - node.style.left = x + 'px'; - node.style.top = y + 'px'; - }, - - hide : function() { - var self = JX.Hovercard; - self._visiblePHID = null; - self._activeRoot = null; - if (self._node) { - JX.DOM.remove(self._node); - self._node = null; - } - }, - - /** - * Pass it an array of phids to load them into storage - * - * @param list phids - */ - _load : function(phids) { - var self = JX.Hovercard; - var uri = JX.$U(self.fetchUrl); - - var send = false; - for (var ii = 0; ii < phids.length; ii++) { - var phid = phids[ii]; - if (phid in self._cards) { - continue; - } - self._cards[phid] = true; // means "loading" - uri.setQueryParam('phids['+ii+']', phids[ii]); - send = true; - } - - if (!send) { - // already loaded / loading everything! - return; - } - - new JX.Request(uri, function(r) { - for (var phid in r.cards) { - self._cards[phid] = r.cards[phid]; - - // Don't draw if the user is faster than the browser - // Only draw if the user is still requesting the original card - if (self.getCard() && phid != self._visiblePHID) { - continue; - } - - self._drawCard(phid); - } - }).send(); + members: { + newContentNode: function() { + return JX.$H(this.getContent()); } } + }); diff --git a/webroot/rsrc/js/core/HovercardList.js b/webroot/rsrc/js/core/HovercardList.js new file mode 100644 index 0000000000..91a575d68d --- /dev/null +++ b/webroot/rsrc/js/core/HovercardList.js @@ -0,0 +1,226 @@ +/** + * @requires javelin-install + * javelin-dom + * javelin-vector + * javelin-request + * javelin-uri + * phui-hovercard + * @provides phui-hovercard-list + * @javelin + */ + +JX.install('HovercardList', { + + construct: function() { + this._cards = {}; + this._drawRequest = {}; + }, + + members: { + _cardNode: null, + _rootNode: null, + _cards: null, + _drawRequest: null, + _visibleCard: null, + + _fetchURI : '/search/hovercard/', + + getCard: function(spec) { + var hovercard_key = this._newHovercardKey(spec); + + if (!(hovercard_key in this._cards)) { + var card = new JX.Hovercard() + .setHovercardKey(hovercard_key) + .setObjectPHID(spec.hoverPHID); + + this._cards[hovercard_key] = card; + } + + return this._cards[hovercard_key]; + }, + + drawCard: function(card, node) { + this._drawRequest = { + card: card, + node: node + }; + + if (card.getIsLoaded()) { + return this._paintCard(card); + } + + if (card.getIsLoading()) { + return; + } + + var hovercard_key = card.getHovercardKey(); + + var request = {}; + request[hovercard_key] = this._newCardRequest(card); + request = JX.JSON.stringify(request); + + var uri = JX.$U(this._fetchURI) + .setQueryParam('cards', request); + + var onresponse = JX.bind(this, function(r) { + var card = this._cards[hovercard_key]; + + this._fillCard(card, r.cards[hovercard_key]); + this._paintCard(card); + }); + + card.setIsLoading(true); + + new JX.Request(uri, onresponse) + .send(); + }, + + _newHovercardKey: function(spec) { + return 'phid=' + spec.hoverPHID; + }, + + _newCardRequest: function(card) { + return { + objectPHID: card.getObjectPHID() + }; + }, + + _getCardNode: function() { + if (!this._cardNode) { + var attributes = { + className: 'jx-hovercard-container' + }; + + this._cardNode = JX.$N('div', attributes); + } + + return this._cardNode; + }, + + _fillCard: function(card, response) { + card.setContent(response); + card.setIsLoaded(true); + }, + + _paintCard: function(card) { + var request = this._drawRequest; + + if (request.card !== card) { + // This paint request is no longer the most recent paint request. + return; + } + + this.hideCard(); + + this._rootNode = request.node; + var root = this._rootNode; + var node = this._getCardNode(); + + JX.DOM.setContent(node, card.newContentNode()); + + // Append the card to the document, but offscreen, so we can measure it. + node.style.left = '-10000px'; + document.body.appendChild(node); + + // Retrieve size from child (wrapper), since node gives wrong dimensions? + var child = node.firstChild; + + var p = JX.$V(root); + var d = JX.Vector.getDim(root); + var n = JX.Vector.getDim(child); + var v = JX.Vector.getViewport(); + var s = JX.Vector.getScroll(); + + // Move the tip so it's nicely aligned. + var margin = 20; + + // Try to align the card directly above the link, with left borders + // touching. + var x = p.x; + + // If this would push us off the right side of the viewport, push things + // back to the left. + if ((x + n.x + margin) > (s.x + v.x)) { + x = (s.x + v.x) - n.x - margin; + } + + // Try to put the card above the link. + var y = p.y - n.y - margin; + + var alignment = 'north'; + + // If the card is near the top of the window, show it beneath the + // link we're hovering over instead. + if ((y - margin) < s.y) { + y = p.y + d.y + margin; + alignment = 'south'; + } + + this._alignment = alignment; + node.style.left = x + 'px'; + node.style.top = y + 'px'; + + this._visibleCard = card; + }, + + hideCard: function() { + var node = this._getCardNode(); + JX.DOM.remove(node); + + this._rootNode = null; + this._alignment = null; + this._visibleCard = null; + }, + + onMouseMove: function(e) { + if (!this._visibleCard) { + return; + } + + var root = this._rootNode; + var node = this._getCardNode(); + var alignment = this._alignment; + + var mouse = JX.$V(e); + var node_pos = JX.$V(node); + var node_dim = JX.Vector.getDim(node); + var root_pos = JX.$V(root); + var root_dim = JX.Vector.getDim(root); + + var margin = 20; + + if (alignment === 'south') { + // Cursor is below the node. + if (mouse.y > node_pos.y + node_dim.y + margin) { + this.hideCard(); + } + + // Cursor is above the root. + if (mouse.y < root_pos.y - margin) { + this.hideCard(); + } + } else { + // Cursor is above the node. + if (mouse.y < node_pos.y - margin) { + this.hideCard(); + } + + // Cursor is below the root. + if (mouse.y > root_pos.y + root_dim.y + margin) { + this.hideCard(); + } + } + + // Cursor is too far to the left. + if (mouse.x < Math.min(root_pos.x, node_pos.x) - margin) { + this.hideCard(); + } + + // Cursor is too far to the right. + if (mouse.x > + Math.max(root_pos.x + root_dim.x, node_pos.x + node_dim.x) + margin) { + this.hideCard(); + } + } + } +}); diff --git a/webroot/rsrc/js/core/behavior-hovercard.js b/webroot/rsrc/js/core/behavior-hovercard.js index 8a1c610752..d5caccf650 100644 --- a/webroot/rsrc/js/core/behavior-hovercard.js +++ b/webroot/rsrc/js/core/behavior-hovercard.js @@ -5,10 +5,18 @@ * javelin-stratcom * javelin-vector * phui-hovercard + * phui-hovercard-list * @javelin */ -JX.behavior('phui-hovercards', function() { +JX.behavior('phui-hovercards', function(config, statics) { + if (statics.hovercardList) { + return; + } + + var cards = new JX.HovercardList(); + statics.hovercardList = cards; + // We listen for mousemove instead of mouseover to handle the case when user // scrolls with keyboard. We don't want to display hovercard if node gets @@ -23,65 +31,19 @@ JX.behavior('phui-hovercards', function() { return; } + var node = e.getNode('hovercard'); var data = e.getNodeData('hovercard'); - JX.Hovercard.show( - e.getNode('hovercard'), - data.hoverPHID); + var card = cards.getCard(data); + + cards.drawCard(card, node); }); JX.Stratcom.listen( 'mousemove', null, function (e) { - if (!JX.Hovercard.getCard()) { - return; - } - - var root = JX.Hovercard.getAnchor(); - var node = JX.Hovercard.getCard(); - var align = JX.Hovercard.getAlignment(); - - var mouse = JX.$V(e); - var node_pos = JX.$V(node); - var node_dim = JX.Vector.getDim(node); - var root_pos = JX.$V(root); - var root_dim = JX.Vector.getDim(root); - - var margin = 20; - - if (align == 'south') { - // Cursor is below the node. - if (mouse.y > node_pos.y + node_dim.y + margin) { - JX.Hovercard.hide(); - } - - // Cursor is above the root. - if (mouse.y < root_pos.y - margin) { - JX.Hovercard.hide(); - } - } else { - // Cursor is above the node. - if (mouse.y < node_pos.y - margin) { - JX.Hovercard.hide(); - } - - // Cursor is below the root. - if (mouse.y > root_pos.y + root_dim.y + margin) { - JX.Hovercard.hide(); - } - } - - // Cursor is too far to the left. - if (mouse.x < Math.min(root_pos.x, node_pos.x) - margin) { - JX.Hovercard.hide(); - } - - // Cursor is too far to the right. - if (mouse.x > - Math.max(root_pos.x + root_dim.x, node_pos.x + node_dim.x) + margin) { - JX.Hovercard.hide(); - } + cards.onMouseMove(e); }); // When we leave the page, hide any visible hovercards. If we don't do this, @@ -91,7 +53,7 @@ JX.behavior('phui-hovercards', function() { ['unload', 'onresize'], null, function() { - JX.Hovercard.hide(); + cards.hideCard(); }); }); From 90903282c706db337d655cc8ad2fef42be50e258 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 13 Feb 2021 12:51:06 -0800 Subject: [PATCH 05/17] Render user hovercards with context information about their ability to see the context object Summary: Ref T13602. When rendering a user hovercard, pass the object on which the reference appears. If the user can't see the object, make it clear on the hovecard. Restyle the "nopermission" markup in mentions to make it more obvious what the style means: instead of grey text, use red with an explicit icon. Test Plan: {F8430398} Maniphest Tasks: T13602 Differential Revision: https://secure.phabricator.com/D21554 --- resources/celerity/map.php | 74 +++++++++---------- .../ManiphestTaskDetailController.php | 8 +- .../PeopleHovercardEngineExtension.php | 6 +- .../markup/PhabricatorMentionRemarkupRule.php | 8 +- .../people/view/PhabricatorUserCardView.php | 32 +++++++- .../phid/PhabricatorObjectHandle.php | 4 +- .../PhabricatorSearchHovercardController.php | 56 +++++++++++--- .../graph/ManiphestTaskGraph.php | 4 +- src/view/phui/PHUIHovercardView.php | 10 +++ src/view/phui/PHUITagView.php | 35 ++++++++- .../application/project/project-card-view.css | 11 +++ webroot/rsrc/css/core/remarkup.css | 5 -- webroot/rsrc/css/phui/phui-tag-view.css | 12 +++ webroot/rsrc/js/core/Hovercard.js | 1 + webroot/rsrc/js/core/HovercardList.js | 13 +++- webroot/rsrc/js/core/behavior-hovercard.js | 2 +- 16 files changed, 212 insertions(+), 69 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 06469b117a..830445c0b2 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,8 +9,8 @@ return array( 'names' => array( 'conpherence.pkg.css' => '0e3cf785', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '970b3ceb', - 'core.pkg.js' => '2fe70e3d', + 'core.pkg.css' => '7cb6808c', + 'core.pkg.js' => '079198f6', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', 'differential.pkg.js' => '5080baf4', @@ -101,7 +101,7 @@ return array( 'rsrc/css/application/policy/policy-transaction-detail.css' => 'c02b8384', 'rsrc/css/application/policy/policy.css' => 'ceb56a08', 'rsrc/css/application/ponder/ponder-view.css' => '05a09d0a', - 'rsrc/css/application/project/project-card-view.css' => '4e7371cd', + 'rsrc/css/application/project/project-card-view.css' => 'a9f2c2dd', 'rsrc/css/application/project/project-triggers.css' => 'cd9c8bb9', 'rsrc/css/application/project/project-view.css' => '567858b3', 'rsrc/css/application/releeph/releeph-core.css' => 'f81ff2db', @@ -114,7 +114,7 @@ return array( 'rsrc/css/application/tokens/tokens.css' => 'ce5a50bd', 'rsrc/css/application/uiexample/example.css' => 'b4795059', 'rsrc/css/core/core.css' => 'b3ebd90d', - 'rsrc/css/core/remarkup.css' => '24d48a73', + 'rsrc/css/core/remarkup.css' => '5baa3bd9', 'rsrc/css/core/syntax.css' => '548567f6', 'rsrc/css/core/z-index.css' => 'ac3bfcd4', 'rsrc/css/diviner/diviner-shared.css' => '4bd263b0', @@ -181,7 +181,7 @@ return array( 'rsrc/css/phui/phui-segment-bar-view.css' => '5166b370', 'rsrc/css/phui/phui-spacing.css' => 'b05cadc3', 'rsrc/css/phui/phui-status.css' => 'e5ff8be0', - 'rsrc/css/phui/phui-tag-view.css' => '8519160a', + 'rsrc/css/phui/phui-tag-view.css' => 'fb811341', 'rsrc/css/phui/phui-timeline-view.css' => '2d32d7a9', 'rsrc/css/phui/phui-two-column-view.css' => 'f96d319f', 'rsrc/css/phui/workboards/phui-workboard-color.css' => 'e86de308', @@ -460,8 +460,8 @@ return array( 'rsrc/js/core/DraggableList.js' => '0169e425', 'rsrc/js/core/Favicon.js' => '7930776a', 'rsrc/js/core/FileUpload.js' => 'ab85e184', - 'rsrc/js/core/Hovercard.js' => 'd9d29a5f', - 'rsrc/js/core/HovercardList.js' => '10a5f4bf', + 'rsrc/js/core/Hovercard.js' => '6199f752', + 'rsrc/js/core/HovercardList.js' => 'de4b4919', 'rsrc/js/core/KeyboardShortcut.js' => '1a844c06', 'rsrc/js/core/KeyboardShortcutManager.js' => '81debc48', 'rsrc/js/core/MultirowRowManager.js' => '5b54c823', @@ -486,7 +486,7 @@ return array( 'rsrc/js/core/behavior-global-drag-and-drop.js' => '1cab0e9a', 'rsrc/js/core/behavior-high-security-warning.js' => 'dae2d55b', 'rsrc/js/core/behavior-history-install.js' => '6a1583a8', - 'rsrc/js/core/behavior-hovercard.js' => '3f446c72', + 'rsrc/js/core/behavior-hovercard.js' => '183738e6', 'rsrc/js/core/behavior-keyboard-pager.js' => '1325b731', 'rsrc/js/core/behavior-keyboard-shortcuts.js' => '42c44e8b', 'rsrc/js/core/behavior-lightbox-attachments.js' => 'c7e748bf', @@ -671,7 +671,7 @@ return array( 'javelin-behavior-pholio-mock-view' => '5aa1544e', 'javelin-behavior-phui-dropdown-menu' => '5cf0501a', 'javelin-behavior-phui-file-upload' => 'e150bd50', - 'javelin-behavior-phui-hovercards' => '3f446c72', + 'javelin-behavior-phui-hovercards' => '183738e6', 'javelin-behavior-phui-selectable-list' => 'b26a41e4', 'javelin-behavior-phui-submenu' => 'b5e9bff9', 'javelin-behavior-phui-tab-group' => '242aa08b', @@ -807,7 +807,7 @@ return array( 'phabricator-object-selector-css' => 'ee77366f', 'phabricator-phtize' => '2f1db1ed', 'phabricator-prefab' => '5793d835', - 'phabricator-remarkup-css' => '24d48a73', + 'phabricator-remarkup-css' => '5baa3bd9', 'phabricator-search-results-css' => '9ea70ace', 'phabricator-shaped-request' => '995f5102', 'phabricator-slowvote-css' => '1694baed', @@ -859,8 +859,8 @@ return array( 'phui-formation-view-css' => 'd2dec8ed', 'phui-head-thing-view-css' => 'd7f293df', 'phui-header-view-css' => '36c86a58', - 'phui-hovercard' => 'd9d29a5f', - 'phui-hovercard-list' => '10a5f4bf', + 'phui-hovercard' => '6199f752', + 'phui-hovercard-list' => 'de4b4919', 'phui-hovercard-view-css' => '6ca90fa0', 'phui-icon-set-selector-css' => '7aa5f3ec', 'phui-icon-view-css' => '4cbc684a', @@ -886,7 +886,7 @@ return array( 'phui-segment-bar-view-css' => '5166b370', 'phui-spacing-css' => 'b05cadc3', 'phui-status-list-view-css' => 'e5ff8be0', - 'phui-tag-view-css' => '8519160a', + 'phui-tag-view-css' => 'fb811341', 'phui-theme-css' => '35883b37', 'phui-timeline-view-css' => '2d32d7a9', 'phui-two-column-view-css' => 'f96d319f', @@ -908,7 +908,7 @@ return array( 'policy-edit-css' => '8794e2ed', 'policy-transaction-detail-css' => 'c02b8384', 'ponder-view-css' => '05a09d0a', - 'project-card-view-css' => '4e7371cd', + 'project-card-view-css' => 'a9f2c2dd', 'project-triggers-css' => 'cd9c8bb9', 'project-view-css' => '567858b3', 'releeph-core' => 'f81ff2db', @@ -1025,14 +1025,6 @@ return array( 'javelin-workflow', 'phuix-icon-view', ), - '10a5f4bf' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-vector', - 'javelin-request', - 'javelin-uri', - 'phui-hovercard', - ), '111bfd2d' => array( 'javelin-install', ), @@ -1047,6 +1039,14 @@ return array( 'javelin-stratcom', 'javelin-util', ), + '183738e6' => array( + 'javelin-behavior', + 'javelin-behavior-device', + 'javelin-stratcom', + 'javelin-vector', + 'phui-hovercard', + 'phui-hovercard-list', + ), '1a844c06' => array( 'javelin-install', 'javelin-util', @@ -1269,14 +1269,6 @@ return array( 'phabricator-drag-and-drop-file-upload', 'phabricator-draggable-list', ), - '3f446c72' => array( - 'javelin-behavior', - 'javelin-behavior-device', - 'javelin-stratcom', - 'javelin-vector', - 'phui-hovercard', - 'phui-hovercard-list', - ), '407ee861' => array( 'javelin-behavior', 'javelin-uri', @@ -1528,6 +1520,13 @@ return array( '60cd9241' => array( 'javelin-behavior', ), + '6199f752' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-vector', + 'javelin-request', + 'javelin-uri', + ), '6337cf26' => array( 'javelin-behavior', 'javelin-dom', @@ -2136,13 +2135,6 @@ return array( 'javelin-util', 'phabricator-shaped-request', ), - 'd9d29a5f' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-vector', - 'javelin-request', - 'javelin-uri', - ), 'da15d3dc' => array( 'phui-oi-list-view-css', ), @@ -2155,6 +2147,14 @@ return array( 'javelin-uri', 'phabricator-notification', ), + 'de4b4919' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-vector', + 'javelin-request', + 'javelin-uri', + 'phui-hovercard', + ), 'e150bd50' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/applications/maniphest/controller/ManiphestTaskDetailController.php b/src/applications/maniphest/controller/ManiphestTaskDetailController.php index e542cb017a..8916ad26cf 100644 --- a/src/applications/maniphest/controller/ManiphestTaskDetailController.php +++ b/src/applications/maniphest/controller/ManiphestTaskDetailController.php @@ -638,7 +638,9 @@ final class ManiphestTaskDetailController extends ManiphestController { 'href' => $commit->getURI(), 'sigil' => 'hovercard', 'meta' => array( - 'hoverPHID' => $commit->getPHID(), + 'hovercardSpec' => array( + 'objectPHID' => $commit->getPHID(), + ), ), ), $commit->getSummary()); @@ -705,7 +707,9 @@ final class ManiphestTaskDetailController extends ManiphestController { 'href' => $revision->getURI(), 'sigil' => 'hovercard', 'meta' => array( - 'hoverPHID' => $revision->getPHID(), + 'hovercardSpec' => array( + 'objectPHID' => $revision->getPHID(), + ), ), ), $revision->getTitle()); diff --git a/src/applications/people/engineextension/PeopleHovercardEngineExtension.php b/src/applications/people/engineextension/PeopleHovercardEngineExtension.php index 507715d21a..083a5f0ed1 100644 --- a/src/applications/people/engineextension/PeopleHovercardEngineExtension.php +++ b/src/applications/people/engineextension/PeopleHovercardEngineExtension.php @@ -47,12 +47,14 @@ final class PeopleHovercardEngineExtension return; } + $is_exiled = $hovercard->getIsExiled(); + $user_card = id(new PhabricatorUserCardView()) ->setProfile($user) - ->setViewer($viewer); + ->setViewer($viewer) + ->setIsExiled($is_exiled); $hovercard->appendChild($user_card); - } } diff --git a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php index 6cf3b4b24b..35b15a3486 100644 --- a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php +++ b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php @@ -110,7 +110,6 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { if ($exists) { $user = $actual_users[$username]; - Javelin::initBehavior('phui-hovercards'); // Check if the user has view access to the object she was mentioned in if ($policy_object) { @@ -159,8 +158,13 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { ->setName('@'.$user->getUserName()) ->setHref($user_href); + if ($context_object) { + $tag->setContextObject($context_object); + } + if ($user_can_not_view) { - $tag->addClass('phabricator-remarkup-mention-nopermission'); + $tag->setIcon('fa-eye-slash red'); + $tag->setIsExiled(true); } if ($user->getIsDisabled()) { diff --git a/src/applications/people/view/PhabricatorUserCardView.php b/src/applications/people/view/PhabricatorUserCardView.php index 54d0a41204..83cf3c78f6 100644 --- a/src/applications/people/view/PhabricatorUserCardView.php +++ b/src/applications/people/view/PhabricatorUserCardView.php @@ -5,6 +5,7 @@ final class PhabricatorUserCardView extends AphrontTagView { private $profile; private $viewer; private $tag; + private $isExiled; public function setProfile(PhabricatorUser $profile) { $this->profile = $profile; @@ -42,6 +43,15 @@ final class PhabricatorUserCardView extends AphrontTagView { ); } + public function setIsExiled($is_exiled) { + $this->isExiled = $is_exiled; + return $this; + } + + public function getIsExiled() { + return $this->isExiled; + } + protected function getTagContent() { $user = $this->profile; @@ -108,6 +118,15 @@ final class PhabricatorUserCardView extends AphrontTagView { } } + if ($this->getIsExiled()) { + $body[] = $this->addItem( + 'fa-eye-slash red', + pht('This user can not see this object.'), + array( + 'project-card-item-exiled', + )); + } + $classes[] = 'project-card-image'; $image = phutil_tag( 'img', @@ -160,17 +179,26 @@ final class PhabricatorUserCardView extends AphrontTagView { return $card; } - private function addItem($icon, $value) { + private function addItem($icon, $value, $classes = array()) { + $classes[] = 'project-card-item'; + $icon = id(new PHUIIconView()) ->addClass('project-card-item-icon') ->setIcon($icon); + $text = phutil_tag( 'span', array( 'class' => 'project-card-item-text', ), $value); - return phutil_tag_div('project-card-item', array($icon, $text)); + + return phutil_tag( + 'div', + array( + 'class' => implode(' ', $classes), + ), + array($icon, $text)); } } diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php index 974c5faf39..7c7aafa199 100644 --- a/src/applications/phid/PhabricatorObjectHandle.php +++ b/src/applications/phid/PhabricatorObjectHandle.php @@ -306,7 +306,9 @@ final class PhabricatorObjectHandle $attributes = array( 'sigil' => 'hovercard', 'meta' => array( - 'hoverPHID' => $this->getPHID(), + 'hovercardSpec' => array( + 'objectPHID' => $this->getPHID(), + ), ), ); diff --git a/src/applications/search/controller/PhabricatorSearchHovercardController.php b/src/applications/search/controller/PhabricatorSearchHovercardController.php index 0eaae1a138..3c9d6b6a54 100644 --- a/src/applications/search/controller/PhabricatorSearchHovercardController.php +++ b/src/applications/search/controller/PhabricatorSearchHovercardController.php @@ -32,11 +32,19 @@ final class PhabricatorSearchHovercardController $object_phids = array(); $handle_phids = array(); + $context_phids = array(); foreach ($cards as $card) { $object_phid = idx($card, 'objectPHID'); $handle_phids[] = $object_phid; $object_phids[] = $object_phid; + + $context_phid = idx($card, 'contextPHID'); + + if ($context_phid) { + $object_phids[] = $context_phid; + $context_phids[] = $context_phid; + } } $handles = id(new PhabricatorHandleQuery()) @@ -50,11 +58,20 @@ final class PhabricatorSearchHovercardController ->execute(); $objects = mpull($objects, null, 'getPHID'); + $context_objects = array_select_keys($objects, $context_phids); + + if ($context_objects) { + PhabricatorPolicyFilterSet::loadHandleViewCapabilities( + $viewer, + $handles, + $context_objects); + } + $extensions = PhabricatorHovercardEngineExtension::getAllEnabledExtensions(); $extension_maps = array(); - foreach ($extensions as $key => $extension) { + foreach ($extensions as $extension_key => $extension) { $extension->setViewer($viewer); $extension_phids = array(); @@ -64,18 +81,18 @@ final class PhabricatorSearchHovercardController } } - $extension_maps[$key] = $extension_phids; + $extension_maps[$extension_key] = $extension_phids; } $extension_data = array(); - foreach ($extensions as $key => $extension) { - $extension_phids = $extension_maps[$key]; + foreach ($extensions as $extension_key => $extension) { + $extension_phids = $extension_maps[$extension_key]; if (!$extension_phids) { - unset($extensions[$key]); + unset($extensions[$extension_key]); continue; } - $extension_data[$key] = $extension->willRenderHovercards( + $extension_data[$extension_key] = $extension->willRenderHovercards( array_select_keys($objects, $extension_phids)); } @@ -86,20 +103,35 @@ final class PhabricatorSearchHovercardController $handle = $handles[$object_phid]; $object = idx($objects, $object_phid); + $context_phid = idx($card, 'contextPHID'); + if ($context_phid) { + $context_object = idx($context_objects, $context_phid); + } else { + $context_object = null; + } + $hovercard = id(new PHUIHovercardView()) ->setUser($viewer) ->setObjectHandle($handle); + if ($context_object) { + if ($handle->hasCapabilities()) { + if (!$handle->hasViewCapability($context_object)) { + $hovercard->setIsExiled(true); + } + } + } + if ($object) { $hovercard->setObject($object); - foreach ($extension_maps as $key => $extension_phids) { - if (isset($extension_phids[$phid])) { - $extensions[$key]->renderHovercard( + foreach ($extension_maps as $extension_key => $extension_phids) { + if (isset($extension_phids[$object_phid])) { + $extensions[$extension_key]->renderHovercard( $hovercard, $handle, $object, - $extension_data[$key]); + $extension_data[$extension_key]); } } } @@ -114,8 +146,8 @@ final class PhabricatorSearchHovercardController )); } - foreach ($results as $key => $hovercard) { - $results[$key] = phutil_tag('div', + foreach ($results as $result_key => $hovercard) { + $results[$result_key] = phutil_tag('div', array( 'class' => 'ml', ), diff --git a/src/infrastructure/graph/ManiphestTaskGraph.php b/src/infrastructure/graph/ManiphestTaskGraph.php index 5aef7b102a..c237fd2c35 100644 --- a/src/infrastructure/graph/ManiphestTaskGraph.php +++ b/src/infrastructure/graph/ManiphestTaskGraph.php @@ -69,7 +69,9 @@ final class ManiphestTaskGraph 'href' => $object->getURI(), 'sigil' => 'hovercard', 'meta' => array( - 'hoverPHID' => $object->getPHID(), + 'hovercardSpec' => array( + 'objectPHID' => $object->getPHID(), + ), ), ), $object->getTitle()); diff --git a/src/view/phui/PHUIHovercardView.php b/src/view/phui/PHUIHovercardView.php index f6cfc45a35..6b85364ceb 100644 --- a/src/view/phui/PHUIHovercardView.php +++ b/src/view/phui/PHUIHovercardView.php @@ -18,6 +18,7 @@ final class PHUIHovercardView extends AphrontTagView { private $fields = array(); private $actions = array(); private $badges = array(); + private $isExiled; public function setObjectHandle(PhabricatorObjectHandle $handle) { $this->handle = $handle; @@ -43,6 +44,15 @@ final class PHUIHovercardView extends AphrontTagView { return $this; } + public function setIsExiled($is_exiled) { + $this->isExiled = $is_exiled; + return $this; + } + + public function getIsExiled() { + return $this->isExiled; + } + public function addField($label, $value) { $this->fields[] = array( 'label' => $label, diff --git a/src/view/phui/PHUITagView.php b/src/view/phui/PHUITagView.php index b77ec5d70e..cd6321a852 100644 --- a/src/view/phui/PHUITagView.php +++ b/src/view/phui/PHUITagView.php @@ -44,6 +44,8 @@ final class PHUITagView extends AphrontTagView { private $shade; private $slimShady; private $border; + private $contextObject; + private $isExiled; public function setType($type) { $this->type = $type; @@ -127,6 +129,24 @@ final class PHUITagView extends AphrontTagView { return strlen($this->href) ? 'a' : 'span'; } + public function setContextObject($context_object) { + $this->contextObject = $context_object; + return $this; + } + + public function getContextObject() { + return $this->contextObject; + } + + public function setIsExiled($is_exiled) { + $this->isExiled = $is_exiled; + return $this; + } + + public function getIsExiled() { + return $this->isExiled; + } + protected function getTagAttributes() { require_celerity_resource('phui-tag-view-css'); @@ -155,6 +175,10 @@ final class PHUITagView extends AphrontTagView { $classes[] = 'phui-tag-'.$this->border; } + if ($this->getIsExiled()) { + $classes[] = 'phui-tag-exiled'; + } + $attributes = array( 'href' => $this->href, 'class' => $classes, @@ -170,10 +194,19 @@ final class PHUITagView extends AphrontTagView { if ($this->phid) { Javelin::initBehavior('phui-hovercards'); + $hovercard_spec = array( + 'objectPHID' => $this->phid, + ); + + $context_object = $this->getContextObject(); + if ($context_object) { + $hovercard_spec['contextPHID'] = $context_object->getPHID(); + } + $attributes += array( 'sigil' => 'hovercard', 'meta' => array( - 'hoverPHID' => $this->phid, + 'hovercardSpec' => $hovercard_spec, ), ); } diff --git a/webroot/rsrc/css/application/project/project-card-view.css b/webroot/rsrc/css/application/project/project-card-view.css index cce4789ef7..f8ab045254 100644 --- a/webroot/rsrc/css/application/project/project-card-view.css +++ b/webroot/rsrc/css/application/project/project-card-view.css @@ -72,6 +72,17 @@ color: {$greytext}; } +.project-card-view .project-card-item-exiled { + background-color: {$lightredbackground}; + border-radius: 4px; + padding: 2px 8px; + margin: 2px 0; +} + +.project-card-view .project-card-item-exiled .project-card-item-text { + color: {$red}; +} + .project-card-view .project-card-item-icon { width: 20px; } diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css index bb698f3ac1..38e03290d7 100644 --- a/webroot/rsrc/css/core/remarkup.css +++ b/webroot/rsrc/css/core/remarkup.css @@ -291,11 +291,6 @@ video.phabricator-media { color: {$greytext}; } -.phabricator-remarkup-mention-nopermission .phui-tag-core { - background: {$lightgreybackground}; - color: {$lightgreytext}; -} - .phabricator-remarkup .remarkup-note { margin: 16px 0; padding: 12px; diff --git a/webroot/rsrc/css/phui/phui-tag-view.css b/webroot/rsrc/css/phui/phui-tag-view.css index d0b2afce1d..cf4a4fd5a4 100644 --- a/webroot/rsrc/css/phui/phui-tag-view.css +++ b/webroot/rsrc/css/phui/phui-tag-view.css @@ -531,3 +531,15 @@ a.phui-tag-view:hover.phui-tag-disabled .phui-tag-core { color: {$blacktext}; border-color: {$blacktext}; } + +.phui-tag-exiled .phui-tag-core { + border-color: {$lightredborder}; + color: {$red}; + background: {$lightredbackground}; +} + + +a.phui-tag-view.phui-tag-exiled:hover + .phui-tag-core.phui-tag-color-person { + border-color: {$red}; +} diff --git a/webroot/rsrc/js/core/Hovercard.js b/webroot/rsrc/js/core/Hovercard.js index 63fc8eab7f..ba3c174bd1 100644 --- a/webroot/rsrc/js/core/Hovercard.js +++ b/webroot/rsrc/js/core/Hovercard.js @@ -13,6 +13,7 @@ JX.install('Hovercard', { properties: { hovercardKey: null, objectPHID: null, + contextPHID: null, isLoading: false, isLoaded: false, content: null diff --git a/webroot/rsrc/js/core/HovercardList.js b/webroot/rsrc/js/core/HovercardList.js index 91a575d68d..645a9d00e4 100644 --- a/webroot/rsrc/js/core/HovercardList.js +++ b/webroot/rsrc/js/core/HovercardList.js @@ -31,7 +31,8 @@ JX.install('HovercardList', { if (!(hovercard_key in this._cards)) { var card = new JX.Hovercard() .setHovercardKey(hovercard_key) - .setObjectPHID(spec.hoverPHID); + .setObjectPHID(spec.objectPHID) + .setContextPHID(spec.contextPHID || null); this._cards[hovercard_key] = card; } @@ -76,12 +77,18 @@ JX.install('HovercardList', { }, _newHovercardKey: function(spec) { - return 'phid=' + spec.hoverPHID; + var parts = [ + spec.objectPHID, + spec.contextPHID + ]; + + return parts.join('/'); }, _newCardRequest: function(card) { return { - objectPHID: card.getObjectPHID() + objectPHID: card.getObjectPHID(), + contextPHID: card.getContextPHID() }; }, diff --git a/webroot/rsrc/js/core/behavior-hovercard.js b/webroot/rsrc/js/core/behavior-hovercard.js index d5caccf650..d0b2058166 100644 --- a/webroot/rsrc/js/core/behavior-hovercard.js +++ b/webroot/rsrc/js/core/behavior-hovercard.js @@ -32,7 +32,7 @@ JX.behavior('phui-hovercards', function(config, statics) { } var node = e.getNode('hovercard'); - var data = e.getNodeData('hovercard'); + var data = e.getNodeData('hovercard').hovercardSpec; var card = cards.getCard(data); From 2f33dedc8b6286212ff8fa195702c51725e70b5b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 13 Feb 2021 13:09:36 -0800 Subject: [PATCH 06/17] When a reviewer can't see a revision, show it clearly in the reviewer list Summary: Ref T13602. Similar to subscriber and mention treatments, make it clear when a user doesn't have view permission. Test Plan: {F8430595} Maniphest Tasks: T13602 Differential Revision: https://secure.phabricator.com/D21555 --- resources/celerity/map.php | 6 ++--- .../view/DifferentialReviewersView.php | 23 +++++++++++++++++-- .../phid/PhabricatorObjectHandle.php | 14 +++++++---- src/view/phui/PHUIStatusItemView.php | 10 ++++++++ webroot/rsrc/css/phui/phui-status.css | 8 ++++++- 5 files changed, 51 insertions(+), 10 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 830445c0b2..487da6a02e 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '0e3cf785', 'conpherence.pkg.js' => '020aebcf', - 'core.pkg.css' => '7cb6808c', + 'core.pkg.css' => '0ae696de', 'core.pkg.js' => '079198f6', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', @@ -180,7 +180,7 @@ return array( 'rsrc/css/phui/phui-remarkup-preview.css' => '91767007', 'rsrc/css/phui/phui-segment-bar-view.css' => '5166b370', 'rsrc/css/phui/phui-spacing.css' => 'b05cadc3', - 'rsrc/css/phui/phui-status.css' => 'e5ff8be0', + 'rsrc/css/phui/phui-status.css' => '293b5dad', 'rsrc/css/phui/phui-tag-view.css' => 'fb811341', 'rsrc/css/phui/phui-timeline-view.css' => '2d32d7a9', 'rsrc/css/phui/phui-two-column-view.css' => 'f96d319f', @@ -885,7 +885,7 @@ return array( 'phui-remarkup-preview-css' => '91767007', 'phui-segment-bar-view-css' => '5166b370', 'phui-spacing-css' => 'b05cadc3', - 'phui-status-list-view-css' => 'e5ff8be0', + 'phui-status-list-view-css' => '293b5dad', 'phui-tag-view-css' => 'fb811341', 'phui-theme-css' => '35883b37', 'phui-timeline-view-css' => '2d32d7a9', diff --git a/src/applications/differential/view/DifferentialReviewersView.php b/src/applications/differential/view/DifferentialReviewersView.php index f88669e539..ad6bf1462b 100644 --- a/src/applications/differential/view/DifferentialReviewersView.php +++ b/src/applications/differential/view/DifferentialReviewersView.php @@ -26,6 +26,8 @@ final class DifferentialReviewersView extends AphrontView { public function render() { $viewer = $this->getUser(); $reviewers = $this->reviewers; + $diff = $this->diff; + $handles = $this->handles; $view = new PHUIStatusListView(); @@ -40,10 +42,15 @@ final class DifferentialReviewersView extends AphrontView { } } + PhabricatorPolicyFilterSet::loadHandleViewCapabilities( + $viewer, + $handles, + array($diff)); + $reviewers = $head + $tail; foreach ($reviewers as $reviewer) { $phid = $reviewer->getReviewerPHID(); - $handle = $this->handles[$phid]; + $handle = $handles[$phid]; $action_phid = $reviewer->getLastActionDiffPHID(); $is_current_action = $this->isCurrent($action_phid); @@ -154,7 +161,10 @@ final class DifferentialReviewersView extends AphrontView { } $item->setIcon($icon, $color, $label); - $item->setTarget($handle->renderHovercardLink()); + $item->setTarget( + $handle->renderHovercardLink( + null, + $diff->getPHID())); if ($reviewer->isPackage()) { if (!$reviewer->getChangesets()) { @@ -162,6 +172,15 @@ final class DifferentialReviewersView extends AphrontView { } } + if ($handle->hasCapabilities()) { + if (!$handle->hasViewCapability($diff)) { + $item + ->setIcon('fa-eye-slash', 'red') + ->setNote(pht('No View Permission')) + ->setIsExiled(true); + } + } + $view->addItem($item); } diff --git a/src/applications/phid/PhabricatorObjectHandle.php b/src/applications/phid/PhabricatorObjectHandle.php index 7c7aafa199..966c1a7db9 100644 --- a/src/applications/phid/PhabricatorObjectHandle.php +++ b/src/applications/phid/PhabricatorObjectHandle.php @@ -300,15 +300,21 @@ final class PhabricatorObjectHandle return $this->renderLinkWithAttributes($name, array()); } - public function renderHovercardLink($name = null) { + public function renderHovercardLink($name = null, $context_phid = null) { Javelin::initBehavior('phui-hovercards'); + $hovercard_spec = array( + 'objectPHID' => $this->getPHID(), + ); + + if ($context_phid) { + $hovercard_spec['contextPHID'] = $context_phid; + } + $attributes = array( 'sigil' => 'hovercard', 'meta' => array( - 'hovercardSpec' => array( - 'objectPHID' => $this->getPHID(), - ), + 'hovercardSpec' => $hovercard_spec, ), ); diff --git a/src/view/phui/PHUIStatusItemView.php b/src/view/phui/PHUIStatusItemView.php index 1e859f5fd2..6b375e1166 100644 --- a/src/view/phui/PHUIStatusItemView.php +++ b/src/view/phui/PHUIStatusItemView.php @@ -8,6 +8,7 @@ final class PHUIStatusItemView extends AphrontTagView { private $target; private $note; private $highlighted; + private $isExiled; const ICON_ACCEPT = 'fa-check-circle'; const ICON_REJECT = 'fa-times-circle'; @@ -46,6 +47,11 @@ final class PHUIStatusItemView extends AphrontTagView { return $this; } + public function setIsExiled($is_exiled) { + $this->isExiled = $is_exiled; + return $this; + } + protected function canAppendChild() { return false; } @@ -60,6 +66,10 @@ final class PHUIStatusItemView extends AphrontTagView { $classes[] = 'phui-status-item-highlighted'; } + if ($this->isExiled) { + $classes[] = 'phui-status-item-exiled'; + } + return array( 'class' => $classes, ); diff --git a/webroot/rsrc/css/phui/phui-status.css b/webroot/rsrc/css/phui/phui-status.css index dd22e6f337..7775614739 100644 --- a/webroot/rsrc/css/phui/phui-status.css +++ b/webroot/rsrc/css/phui/phui-status.css @@ -29,10 +29,16 @@ border-radius: 3px; } +.phui-status-item-exiled td { + background-color: {$lightredbackground}; + border-radius: 3px; +} + .phui-status-list-view td a { color: {$darkbluetext}; } -.phui-status-item-highlighted td.phui-status-item-note { +.phui-status-item-highlighted td.phui-status-item-note, +.phui-status-item-exiled td.phui-status-item-note { background-color: transparent; } From 42c26821ef9227938c1796a5b5be2494eebad426 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 13 Feb 2021 13:19:04 -0800 Subject: [PATCH 07/17] When a revision has only human reviewers but none can view it, show a warning banner Summary: Ref T13602. Warn when a reivison has at least one human reviewer, no non-human reviewers, and no human reviewers can view it. Test Plan: {F8430683} Maniphest Tasks: T13602 Differential Revision: https://secure.phabricator.com/D21556 --- .../DifferentialReviewersField.php | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/customfield/DifferentialReviewersField.php b/src/applications/differential/customfield/DifferentialReviewersField.php index 31b175b09d..91410cc5a3 100644 --- a/src/applications/differential/customfield/DifferentialReviewersField.php +++ b/src/applications/differential/customfield/DifferentialReviewersField.php @@ -72,22 +72,40 @@ final class DifferentialReviewersField return array(); } + $viewer = $this->getViewer(); + + PhabricatorPolicyFilterSet::loadHandleViewCapabilities( + $viewer, + $handles, + array($revision)); + $all_resigned = true; $all_disabled = true; $any_reviewers = false; + $all_exiled = true; foreach ($this->getValue() as $reviewer) { $reviewer_phid = $reviewer->getReviewerPHID(); + $handle = $handles[$reviewer_phid]; $any_reviewers = true; - if (!$handles[$reviewer_phid]->isDisabled()) { + if (!$handle->isDisabled()) { $all_disabled = false; } if (!$reviewer->isResigned()) { $all_resigned = false; } + + if (!$handle->hasCapabilities()) { + $all_exiled = false; + } else { + if ($handle->hasViewCapability($revision)) { + $all_exiled = false; + } + } + } $warnings = array(); @@ -101,6 +119,10 @@ final class DifferentialReviewersField } else if ($all_resigned) { $warnings[] = pht( 'This revision needs review, but all reviewers have resigned.'); + } else if ($all_exiled) { + $warnings[] = pht( + 'This revision needs review, but no reviewers have permission '. + 'to view it.'); } return $warnings; From ec5476a01f5fd7dc559fce0a129c8cf73dd690da Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Feb 2021 10:04:47 -0800 Subject: [PATCH 08/17] Add a PHID to Changesets Summary: Ref T13605. Changesets currently have no PHID, which limits their ability to use standard API infrastructure. Give them a PHID, since there's no reason they don't have one other than their age. Test Plan: - Ran migrations, saw PHIDs populated. - Created new changesets, saw PHIDs. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13605 Differential Revision: https://secure.phabricator.com/D21557 --- .../20210215.changeset.01.phid.sql | 2 + .../20210215.changeset.02.phid-populate.php | 26 ++++++++++++ src/__phutil_library_map__.php | 2 + .../phid/DifferentialChangesetPHIDType.php | 41 +++++++++++++++++++ .../storage/DifferentialChangeset.php | 5 +++ 5 files changed, 76 insertions(+) create mode 100644 resources/sql/autopatches/20210215.changeset.01.phid.sql create mode 100644 resources/sql/autopatches/20210215.changeset.02.phid-populate.php create mode 100644 src/applications/differential/phid/DifferentialChangesetPHIDType.php diff --git a/resources/sql/autopatches/20210215.changeset.01.phid.sql b/resources/sql/autopatches/20210215.changeset.01.phid.sql new file mode 100644 index 0000000000..25cab7805a --- /dev/null +++ b/resources/sql/autopatches/20210215.changeset.01.phid.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_changeset + ADD phid VARBINARY(64) NOT NULL; diff --git a/resources/sql/autopatches/20210215.changeset.02.phid-populate.php b/resources/sql/autopatches/20210215.changeset.02.phid-populate.php new file mode 100644 index 0000000000..da64650006 --- /dev/null +++ b/resources/sql/autopatches/20210215.changeset.02.phid-populate.php @@ -0,0 +1,26 @@ +establishConnection('w'); +$table_name = $changeset_table->getTableName(); + +$iterator = new LiskRawMigrationIterator($conn, $table_name); +foreach ($iterator as $changeset_row) { + $phid = $changeset_row['phid']; + + if (strlen($phid)) { + continue; + } + + $phid = PhabricatorPHID::generateNewPHID($phid_type); + + queryfx( + $conn, + 'UPDATE %T SET phid = %s WHERE id = %d', + $table_name, + $phid, + $changeset_row['id']); +} diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 57e9a22a57..847001dd46 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -472,6 +472,7 @@ phutil_register_library_map(array( 'DifferentialChangesetOneUpMailRenderer' => 'applications/differential/render/DifferentialChangesetOneUpMailRenderer.php', 'DifferentialChangesetOneUpRenderer' => 'applications/differential/render/DifferentialChangesetOneUpRenderer.php', 'DifferentialChangesetOneUpTestRenderer' => 'applications/differential/render/DifferentialChangesetOneUpTestRenderer.php', + 'DifferentialChangesetPHIDType' => 'applications/differential/phid/DifferentialChangesetPHIDType.php', 'DifferentialChangesetParser' => 'applications/differential/parser/DifferentialChangesetParser.php', 'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php', 'DifferentialChangesetQuery' => 'applications/differential/query/DifferentialChangesetQuery.php', @@ -6541,6 +6542,7 @@ phutil_register_library_map(array( 'DifferentialChangesetOneUpMailRenderer' => 'DifferentialChangesetRenderer', 'DifferentialChangesetOneUpRenderer' => 'DifferentialChangesetHTMLRenderer', 'DifferentialChangesetOneUpTestRenderer' => 'DifferentialChangesetTestRenderer', + 'DifferentialChangesetPHIDType' => 'PhabricatorPHIDType', 'DifferentialChangesetParser' => 'Phobject', 'DifferentialChangesetParserTestCase' => 'PhabricatorTestCase', 'DifferentialChangesetQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', diff --git a/src/applications/differential/phid/DifferentialChangesetPHIDType.php b/src/applications/differential/phid/DifferentialChangesetPHIDType.php new file mode 100644 index 0000000000..c558c0a6a2 --- /dev/null +++ b/src/applications/differential/phid/DifferentialChangesetPHIDType.php @@ -0,0 +1,41 @@ +withPHIDs($phids); + } + + public function loadHandles( + PhabricatorHandleQuery $query, + array $handles, + array $objects) { + + foreach ($handles as $phid => $handle) { + $changeset = $objects[$phid]; + + $id = $changeset->getID(); + + $handle->setName(pht('Changeset %d', $id)); + } + } + +} diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index d7d66ae97d..add76cd7bf 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -45,6 +45,7 @@ final class DifferentialChangeset protected function getConfiguration() { return array( + self::CONFIG_AUX_PHID => true, self::CONFIG_SERIALIZATION => array( 'metadata' => self::SERIALIZATION_JSON, 'oldProperties' => self::SERIALIZATION_JSON, @@ -75,6 +76,10 @@ final class DifferentialChangeset ) + parent::getConfiguration(); } + public function getPHIDType() { + return DifferentialChangesetPHIDType::TYPECONST; + } + public function getAffectedLineCount() { return $this->getAddLines() + $this->getDelLines(); } From 9feb7343e662c12e794960905cf3f734cb59c251 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 15 Feb 2021 10:02:27 -0800 Subject: [PATCH 09/17] Provide a "differential.changeset.search" Conduit API method Summary: Ref T13605. Support selecting a diff's changesets (to get a list of affected file paths) via the API. Test Plan: Called API with no arguments, diffPHIDs, PHIDs, IDs. Got sensible output. Maniphest Tasks: T13605 Differential Revision: https://secure.phabricator.com/D21558 --- src/__phutil_library_map__.php | 3 ++ ...rentialChangesetSearchConduitAPIMethod.php | 18 +++++++ .../query/DifferentialChangesetQuery.php | 38 +++++++++++++++ .../DifferentialChangesetSearchEngine.php | 14 +++++- .../storage/DifferentialChangeset.php | 47 ++++++++++++++++++- 5 files changed, 118 insertions(+), 2 deletions(-) create mode 100644 src/applications/differential/conduit/DifferentialChangesetSearchConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 847001dd46..79d318b76d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -477,6 +477,7 @@ phutil_register_library_map(array( 'DifferentialChangesetParserTestCase' => 'applications/differential/parser/__tests__/DifferentialChangesetParserTestCase.php', 'DifferentialChangesetQuery' => 'applications/differential/query/DifferentialChangesetQuery.php', 'DifferentialChangesetRenderer' => 'applications/differential/render/DifferentialChangesetRenderer.php', + 'DifferentialChangesetSearchConduitAPIMethod' => 'applications/differential/conduit/DifferentialChangesetSearchConduitAPIMethod.php', 'DifferentialChangesetSearchEngine' => 'applications/differential/query/DifferentialChangesetSearchEngine.php', 'DifferentialChangesetTestRenderer' => 'applications/differential/render/DifferentialChangesetTestRenderer.php', 'DifferentialChangesetTwoUpRenderer' => 'applications/differential/render/DifferentialChangesetTwoUpRenderer.php', @@ -6533,6 +6534,7 @@ phutil_register_library_map(array( 'DifferentialDAO', 'PhabricatorPolicyInterface', 'PhabricatorDestructibleInterface', + 'PhabricatorConduitResultInterface', ), 'DifferentialChangesetDetailView' => 'AphrontView', 'DifferentialChangesetEngine' => 'Phobject', @@ -6547,6 +6549,7 @@ phutil_register_library_map(array( 'DifferentialChangesetParserTestCase' => 'PhabricatorTestCase', 'DifferentialChangesetQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialChangesetRenderer' => 'Phobject', + 'DifferentialChangesetSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', 'DifferentialChangesetSearchEngine' => 'PhabricatorApplicationSearchEngine', 'DifferentialChangesetTestRenderer' => 'DifferentialChangesetRenderer', 'DifferentialChangesetTwoUpRenderer' => 'DifferentialChangesetHTMLRenderer', diff --git a/src/applications/differential/conduit/DifferentialChangesetSearchConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialChangesetSearchConduitAPIMethod.php new file mode 100644 index 0000000000..3ee0f6ccdc --- /dev/null +++ b/src/applications/differential/conduit/DifferentialChangesetSearchConduitAPIMethod.php @@ -0,0 +1,18 @@ +phids = $phids; + return $this; + } + public function withDiffs(array $diffs) { assert_instances_of($diffs, 'DifferentialDiff'); $this->diffs = $diffs; return $this; } + public function withDiffPHIDs(array $phids) { + $this->diffPHIDs = $phids; + return $this; + } + public function needAttachToDiffs($attach) { $this->needAttachToDiffs = $attach; return $this; @@ -134,6 +147,31 @@ final class DifferentialChangesetQuery $this->ids); } + if ($this->phids !== null) { + $where[] = qsprintf( + $conn, + 'phid IN (%Ls)', + $this->phids); + } + + if ($this->diffPHIDs !== null) { + $diff_ids = queryfx_all( + $conn, + 'SELECT id FROM %R WHERE phid IN (%Ls)', + new DifferentialDiff(), + $this->diffPHIDs); + $diff_ids = ipull($diff_ids, 'id', null); + + if (!$diff_ids) { + throw new PhabricatorEmptyQueryException(); + } + + $where[] = qsprintf( + $conn, + 'diffID IN (%Ld)', + $diff_ids); + } + return $where; } diff --git a/src/applications/differential/query/DifferentialChangesetSearchEngine.php b/src/applications/differential/query/DifferentialChangesetSearchEngine.php index 3fe8957971..b6279ec339 100644 --- a/src/applications/differential/query/DifferentialChangesetSearchEngine.php +++ b/src/applications/differential/query/DifferentialChangesetSearchEngine.php @@ -38,11 +38,23 @@ final class DifferentialChangesetSearchEngine protected function buildQueryFromParameters(array $map) { $query = $this->newQuery(); + + if ($map['diffPHIDs']) { + $query->withDiffPHIDs($map['diffPHIDs']); + } + return $query; } protected function buildCustomSearchFields() { - return array(); + return array( + id(new PhabricatorPHIDsSearchField()) + ->setLabel(pht('Diffs')) + ->setKey('diffPHIDs') + ->setAliases(array('diff', 'diffs', 'diffPHID')) + ->setDescription( + pht('Find changesets attached to a particular diff.')), + ); } protected function getURI($path) { diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index add76cd7bf..770a49e411 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -4,7 +4,8 @@ final class DifferentialChangeset extends DifferentialDAO implements PhabricatorPolicyInterface, - PhabricatorDestructibleInterface { + PhabricatorDestructibleInterface, + PhabricatorConduitResultInterface { protected $diffID; protected $oldFile; @@ -735,5 +736,49 @@ final class DifferentialChangeset $this->saveTransaction(); } +/* -( PhabricatorConduitResultInterface )---------------------------------- */ + + public function getFieldSpecificationsForConduit() { + return array( + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('diffPHID') + ->setType('phid') + ->setDescription(pht('The diff the changeset is attached to.')), + ); + } + + public function getFieldValuesForConduit() { + $diff = $this->getDiff(); + + $repository = null; + if ($diff) { + $revision = $diff->getRevision(); + if ($revision) { + $repository = $revision->getRepository(); + } + } + + $absolute_path = $this->getAbsoluteRepositoryPath($repository, $diff); + if (strlen($absolute_path)) { + $absolute_path = base64_encode($absolute_path); + } else { + $absolute_path = null; + } + + $display_path = $this->getDisplayFilename(); + + return array( + 'diffPHID' => $diff->getPHID(), + 'path' => array( + 'displayPath' => $display_path, + 'absolutePath.base64' => $absolute_path, + ), + ); + } + + public function getConduitSearchAttachments() { + return array(); + } + } From 5d6dddc5eb610c89064c7a8160952c74a737832e Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Feb 2021 11:59:18 -0800 Subject: [PATCH 10/17] Add more constraints to "harbormaster.target.search" Summary: Ref T13607. Add some time-oriented constraints to this API method to support compiling build statistics. Test Plan: - Called "harbormaster.target.search" with all new constraints. - Viewed documentation in API console. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13607 Differential Revision: https://secure.phabricator.com/D21559 --- .../query/HarbormasterBuildTargetQuery.php | 80 +++++++++++++++++++ .../HarbormasterBuildTargetSearchEngine.php | 58 ++++++++++++++ .../storage/build/HarbormasterBuildTarget.php | 9 +++ 3 files changed, 147 insertions(+) diff --git a/src/applications/harbormaster/query/HarbormasterBuildTargetQuery.php b/src/applications/harbormaster/query/HarbormasterBuildTargetQuery.php index 6872be835b..54b9c4ee16 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildTargetQuery.php +++ b/src/applications/harbormaster/query/HarbormasterBuildTargetQuery.php @@ -7,6 +7,14 @@ final class HarbormasterBuildTargetQuery private $phids; private $buildPHIDs; private $buildGenerations; + private $dateCreatedMin; + private $dateCreatedMax; + private $dateStartedMin; + private $dateStartedMax; + private $dateCompletedMin; + private $dateCompletedMax; + private $statuses; + private $needBuildSteps; public function withIDs(array $ids) { @@ -29,6 +37,29 @@ final class HarbormasterBuildTargetQuery return $this; } + public function withDateCreatedBetween($min, $max) { + $this->dateCreatedMin = $min; + $this->dateCreatedMax = $max; + return $this; + } + + public function withDateStartedBetween($min, $max) { + $this->dateStartedMin = $min; + $this->dateStartedMax = $max; + return $this; + } + + public function withDateCompletedBetween($min, $max) { + $this->dateCompletedMin = $min; + $this->dateCompletedMax = $max; + return $this; + } + + public function withTargetStatuses(array $statuses) { + $this->statuses = $statuses; + return $this; + } + public function needBuildSteps($need_build_steps) { $this->needBuildSteps = $need_build_steps; return $this; @@ -73,6 +104,55 @@ final class HarbormasterBuildTargetQuery $this->buildGenerations); } + if ($this->dateCreatedMin !== null) { + $where[] = qsprintf( + $conn, + 'dateCreated >= %d', + $this->dateCreatedMin); + } + + if ($this->dateCreatedMax !== null) { + $where[] = qsprintf( + $conn, + 'dateCreated <= %d', + $this->dateCreatedMax); + } + + if ($this->dateStartedMin !== null) { + $where[] = qsprintf( + $conn, + 'dateStarted >= %d', + $this->dateStartedMin); + } + + if ($this->dateStartedMax !== null) { + $where[] = qsprintf( + $conn, + 'dateStarted <= %d', + $this->dateStartedMax); + } + + if ($this->dateCompletedMin !== null) { + $where[] = qsprintf( + $conn, + 'dateCompleted >= %d', + $this->dateCompletedMin); + } + + if ($this->dateCompletedMax !== null) { + $where[] = qsprintf( + $conn, + 'dateCompleted <= %d', + $this->dateCompletedMax); + } + + if ($this->statuses !== null) { + $where[] = qsprintf( + $conn, + 'targetStatus IN (%Ls)', + $this->statuses); + } + return $where; } diff --git a/src/applications/harbormaster/query/HarbormasterBuildTargetSearchEngine.php b/src/applications/harbormaster/query/HarbormasterBuildTargetSearchEngine.php index b224808322..db7d6f7c87 100644 --- a/src/applications/harbormaster/query/HarbormasterBuildTargetSearchEngine.php +++ b/src/applications/harbormaster/query/HarbormasterBuildTargetSearchEngine.php @@ -24,6 +24,42 @@ final class HarbormasterBuildTargetSearchEngine ->setDescription( pht('Search for targets of a given build.')) ->setDatasource(new HarbormasterBuildPlanDatasource()), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Created After')) + ->setKey('createdStart') + ->setDescription( + pht('Search for targets created on or after a particular date.')), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Created Before')) + ->setKey('createdEnd') + ->setDescription( + pht('Search for targets created on or before a particular date.')), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Started After')) + ->setKey('startedStart') + ->setDescription( + pht('Search for targets started on or after a particular date.')), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Started Before')) + ->setKey('startedEnd') + ->setDescription( + pht('Search for targets started on or before a particular date.')), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Completed After')) + ->setKey('completedStart') + ->setDescription( + pht('Search for targets completed on or after a particular date.')), + id(new PhabricatorSearchDateField()) + ->setLabel(pht('Completed Before')) + ->setKey('completedEnd') + ->setDescription( + pht('Search for targets completed on or before a particular date.')), + id(new PhabricatorSearchStringListField()) + ->setLabel(pht('Statuses')) + ->setKey('statuses') + ->setAliases(array('status')) + ->setDescription( + pht('Search for targets with given statuses.')), ); } @@ -34,6 +70,28 @@ final class HarbormasterBuildTargetSearchEngine $query->withBuildPHIDs($map['buildPHIDs']); } + if ($map['createdStart'] !== null || $map['createdEnd'] !== null) { + $query->withDateCreatedBetween( + $map['createdStart'], + $map['createdEnd']); + } + + if ($map['startedStart'] !== null || $map['startedEnd'] !== null) { + $query->withDateStartedBetween( + $map['startedStart'], + $map['startedEnd']); + } + + if ($map['completedStart'] !== null || $map['completedEnd'] !== null) { + $query->withDateCompletedBetween( + $map['completedStart'], + $map['completedEnd']); + } + + if ($map['statuses']) { + $query->withTargetStatuses($map['statuses']); + } + return $query; } diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php index 30b1bd79e4..fa838288c1 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php @@ -119,6 +119,15 @@ final class HarbormasterBuildTarget 'key_build' => array( 'columns' => array('buildPHID', 'buildStepPHID'), ), + 'key_started' => array( + 'columns' => array('dateStarted'), + ), + 'key_completed' => array( + 'columns' => array('dateCompleted'), + ), + 'key_created' => array( + 'columns' => array('dateCreated'), + ), ), ) + parent::getConfiguration(); } From 4f647fb6be2b05573f4ffcd62e5e7efa6d78d93b Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 19 Nov 2020 12:56:58 -0800 Subject: [PATCH 11/17] When updating a Ferret search index document, reuse existing rows where possible Summary: Ref T13587. Currently, when a document is reindexed by Ferret, the old document is completely discarded and a new version is inserted to replace it. This approach is simple to implement, but can lead to exhaustion of the ngram AUTO_INCREMENT id column in reasonable circumstances. Conceptually, this approach "should" be fine and this exhaustion is an awkard implementation detail. However, since it's easy to be less wasteful when performing document updates and all the other approaches are awkward or leaky in other ways that are probably worse, use a more complex implementation to avoid executing unnecessary INSERT statements. Test Plan: - Created and indexed a new document, searched for it. - Updated a document, indexed it with `bin/search index ... --force --trace`, saw only modifications updated in the index. - Searched for newly added terms (got hits) and removed terms (no longer got hits) to verify add/delete index behavior. Maniphest Tasks: T13587 Differential Revision: https://secure.phabricator.com/D21495 --- ...abricatorFerretFulltextEngineExtension.php | 348 +++++++++++++----- 1 file changed, 261 insertions(+), 87 deletions(-) diff --git a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php index 4c3382641d..e97fab2c53 100644 --- a/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php +++ b/src/applications/search/engineextension/PhabricatorFerretFulltextEngineExtension.php @@ -134,123 +134,297 @@ final class PhabricatorFerretFulltextEngineExtension $ngram_engine = new PhabricatorSearchNgramEngine(); $ngrams = $ngram_engine->getTermNgramsFromString($ngrams_source); + $conn = $object->establishConnection('w'); + + if ($ngrams) { + $common = queryfx_all( + $conn, + 'SELECT ngram FROM %T WHERE ngram IN (%Ls)', + $engine->getCommonNgramsTableName(), + $ngrams); + $common = ipull($common, 'ngram', 'ngram'); + + foreach ($ngrams as $key => $ngram) { + if (isset($common[$ngram])) { + unset($ngrams[$key]); + continue; + } + + // NOTE: MySQL discards trailing whitespace in CHAR(X) columns. + $trimmed_ngram = rtrim($ngram, ' '); + if (isset($common[$trimmed_ngram])) { + unset($ngrams[$key]); + continue; + } + } + } + $object->openTransaction(); try { - $conn = $object->establishConnection('w'); - $this->deleteOldDocument($engine, $object, $document); + // See T13587. If this document already exists in the index, we try to + // update the existing rows to avoid leaving the ngrams table heavily + // fragmented. - queryfx( + $old_document = queryfx_one( $conn, - 'INSERT INTO %T (objectPHID, isClosed, epochCreated, epochModified, - authorPHID, ownerPHID) VALUES (%s, %d, %d, %d, %ns, %ns)', + 'SELECT id FROM %T WHERE objectPHID = %s', $engine->getDocumentTableName(), - $object->getPHID(), - $is_closed, - $document->getDocumentCreated(), - $document->getDocumentModified(), - $author_phid, - $owner_phid); + $object->getPHID()); + if ($old_document) { + $old_document_id = (int)$old_document['id']; + } else { + $old_document_id = null; + } - $document_id = $conn->getInsertID(); - foreach ($ferret_fields as $ferret_field) { + if ($old_document_id === null) { queryfx( $conn, - 'INSERT INTO %T (documentID, fieldKey, rawCorpus, termCorpus, - normalCorpus) VALUES (%d, %s, %s, %s, %s)', - $engine->getFieldTableName(), - $document_id, - $ferret_field['fieldKey'], - $ferret_field['rawCorpus'], - $ferret_field['termCorpus'], - $ferret_field['normalCorpus']); - } + 'INSERT INTO %T (objectPHID, isClosed, epochCreated, epochModified, + authorPHID, ownerPHID) VALUES (%s, %d, %d, %d, %ns, %ns)', + $engine->getDocumentTableName(), + $object->getPHID(), + $is_closed, + $document->getDocumentCreated(), + $document->getDocumentModified(), + $author_phid, + $owner_phid); + $document_id = $conn->getInsertID(); - if ($ngrams) { - $common = queryfx_all( + $is_new = true; + } else { + $document_id = $old_document_id; + queryfx( $conn, - 'SELECT ngram FROM %T WHERE ngram IN (%Ls)', - $engine->getCommonNgramsTableName(), - $ngrams); - $common = ipull($common, 'ngram', 'ngram'); + 'UPDATE %T + SET + isClosed = %d, + epochCreated = %d, + epochModified = %d, + authorPHID = %ns, + ownerPHID = %ns + WHERE id = %d', + $engine->getDocumentTableName(), + $is_closed, + $document->getDocumentCreated(), + $document->getDocumentModified(), + $author_phid, + $owner_phid, + $document_id); - foreach ($ngrams as $key => $ngram) { - if (isset($common[$ngram])) { - unset($ngrams[$key]); - continue; - } - - // NOTE: MySQL discards trailing whitespace in CHAR(X) columns. - $trim_ngram = rtrim($ngram, ' '); - if (isset($common[$ngram])) { - unset($ngrams[$key]); - continue; - } - } + $is_new = false; } - if ($ngrams) { - $sql = array(); - foreach ($ngrams as $ngram) { - $sql[] = qsprintf( - $conn, - '(%d, %s)', - $document_id, - $ngram); - } + $this->updateStoredFields( + $conn, + $is_new, + $document_id, + $engine, + $ferret_fields); + + $this->updateStoredNgrams( + $conn, + $is_new, + $document_id, + $engine, + $ngrams); - foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { - queryfx( - $conn, - 'INSERT INTO %T (documentID, ngram) VALUES %LQ', - $engine->getNgramsTableName(), - $chunk); - } - } } catch (Exception $ex) { $object->killTransaction(); throw $ex; + } catch (Throwable $ex) { + $object->killTransaction(); + throw $ex; } $object->saveTransaction(); } - - private function deleteOldDocument( + private function updateStoredFields( + AphrontDatabaseConnection $conn, + $is_new, + $document_id, PhabricatorFerretEngine $engine, - $object, - PhabricatorSearchAbstractDocument $document) { + $new_fields) { - $conn = $object->establishConnection('w'); - - $old_document = queryfx_one( - $conn, - 'SELECT * FROM %T WHERE objectPHID = %s', - $engine->getDocumentTableName(), - $object->getPHID()); - if (!$old_document) { - return; + if (!$is_new) { + $old_fields = queryfx_all( + $conn, + 'SELECT * FROM %T WHERE documentID = %d', + $engine->getFieldTableName(), + $document_id); + } else { + $old_fields = array(); } - $old_id = $old_document['id']; + $old_fields = ipull($old_fields, null, 'fieldKey'); + $new_fields = ipull($new_fields, null, 'fieldKey'); - queryfx( - $conn, - 'DELETE FROM %T WHERE id = %d', - $engine->getDocumentTableName(), - $old_id); + $delete_rows = array(); + $insert_rows = array(); + $update_rows = array(); - queryfx( - $conn, - 'DELETE FROM %T WHERE documentID = %d', - $engine->getFieldTableName(), - $old_id); + foreach ($old_fields as $field_key => $old_field) { + if (!isset($new_fields[$field_key])) { + $delete_rows[] = $old_field; + } + } - queryfx( - $conn, - 'DELETE FROM %T WHERE documentID = %d', - $engine->getNgramsTableName(), - $old_id); + $compare_keys = array( + 'rawCorpus', + 'termCorpus', + 'normalCorpus', + ); + + foreach ($new_fields as $field_key => $new_field) { + if (!isset($old_fields[$field_key])) { + $insert_rows[] = $new_field; + continue; + } + + $old_field = $old_fields[$field_key]; + + $same_row = true; + foreach ($compare_keys as $compare_key) { + if ($old_field[$compare_key] !== $new_field[$compare_key]) { + $same_row = false; + break; + } + } + + if ($same_row) { + continue; + } + + $new_field['id'] = $old_field['id']; + $update_rows[] = $new_field; + } + + if ($delete_rows) { + queryfx( + $conn, + 'DELETE FROM %T WHERE id IN (%Ld)', + $engine->getFieldTableName(), + ipull($delete_rows, 'id')); + } + + foreach ($update_rows as $update_row) { + queryfx( + $conn, + 'UPDATE %T + SET + rawCorpus = %s, + termCorpus = %s, + normalCorpus = %s + WHERE id = %d', + $engine->getFieldTableName(), + $update_row['rawCorpus'], + $update_row['termCorpus'], + $update_row['normalCorpus'], + $update_row['id']); + } + + foreach ($insert_rows as $insert_row) { + queryfx( + $conn, + 'INSERT INTO %T (documentID, fieldKey, rawCorpus, termCorpus, + normalCorpus) VALUES (%d, %s, %s, %s, %s)', + $engine->getFieldTableName(), + $document_id, + $insert_row['fieldKey'], + $insert_row['rawCorpus'], + $insert_row['termCorpus'], + $insert_row['normalCorpus']); + } + } + + private function updateStoredNgrams( + AphrontDatabaseConnection $conn, + $is_new, + $document_id, + PhabricatorFerretEngine $engine, + $new_ngrams) { + + if ($is_new) { + $old_ngrams = array(); + } else { + $old_ngrams = queryfx_all( + $conn, + 'SELECT id, ngram FROM %T WHERE documentID = %d', + $engine->getNgramsTableName(), + $document_id); + } + + $old_ngrams = ipull($old_ngrams, 'id', 'ngram'); + $new_ngrams = array_fuse($new_ngrams); + + $delete_ids = array(); + $insert_ngrams = array(); + + // NOTE: MySQL discards trailing whitespace in CHAR(X) columns. + + foreach ($old_ngrams as $ngram => $id) { + if (isset($new_ngrams[$ngram])) { + continue; + } + + $untrimmed_ngram = $ngram.' '; + if (isset($new_ngrams[$untrimmed_ngram])) { + continue; + } + + $delete_ids[] = $id; + } + + foreach ($new_ngrams as $ngram) { + if (isset($old_ngrams[$ngram])) { + continue; + } + + $trimmed_ngram = rtrim($ngram, ' '); + if (isset($old_ngrams[$trimmed_ngram])) { + continue; + } + + $insert_ngrams[] = $ngram; + } + + if ($delete_ids) { + $sql = array(); + foreach ($delete_ids as $id) { + $sql[] = qsprintf( + $conn, + '%d', + $id); + } + + foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { + queryfx( + $conn, + 'DELETE FROM %T WHERE id IN (%LQ)', + $engine->getNgramsTableName(), + $chunk); + } + } + + if ($insert_ngrams) { + $sql = array(); + foreach ($insert_ngrams as $ngram) { + $sql[] = qsprintf( + $conn, + '(%d, %s)', + $document_id, + $ngram); + } + + foreach (PhabricatorLiskDAO::chunkSQL($sql) as $chunk) { + queryfx( + $conn, + 'INSERT INTO %T (documentID, ngram) VALUES %LQ', + $engine->getNgramsTableName(), + $chunk); + } + } } public function newFerretSearchFunctions() { From 6703fec3e27d72bd3fae0f567284c09941876289 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Feb 2021 15:29:17 -0800 Subject: [PATCH 12/17] When documents are indexed, record the indexer version (versus the object version) and index epoch Summary: Ref T13587. D21495 has significant changes to the ngram indexer, which might possibly contain bugs. Make it easier to reindex a subset of documents (based on the date when the index was built, and/or the software version which generated the index). This is in addition to the existing versioning, which is focused on object versions. Test Plan: Ran `bin/search index` with various old and new arguments. Spot-checked the `IndexVersion` table. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13587 Differential Revision: https://secure.phabricator.com/D21560 --- .../autopatches/20210216.index.01.version.sql | 2 + .../autopatches/20210216.index.02.epoch.sql | 2 + .../search/index/PhabricatorIndexEngine.php | 29 +- ...abricatorSearchManagementIndexWorkflow.php | 325 +++++++++++++----- .../storage/PhabricatorSearchIndexVersion.php | 8 + 5 files changed, 280 insertions(+), 86 deletions(-) create mode 100644 resources/sql/autopatches/20210216.index.01.version.sql create mode 100644 resources/sql/autopatches/20210216.index.02.epoch.sql diff --git a/resources/sql/autopatches/20210216.index.01.version.sql b/resources/sql/autopatches/20210216.index.01.version.sql new file mode 100644 index 0000000000..d24162891a --- /dev/null +++ b/resources/sql/autopatches/20210216.index.01.version.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_search.search_indexversion + ADD indexVersion BINARY(12) NOT NULL; diff --git a/resources/sql/autopatches/20210216.index.02.epoch.sql b/resources/sql/autopatches/20210216.index.02.epoch.sql new file mode 100644 index 0000000000..4e96ded075 --- /dev/null +++ b/resources/sql/autopatches/20210216.index.02.epoch.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_search.search_indexversion + ADD indexEpoch INT UNSIGNED NOT NULL; diff --git a/src/applications/search/index/PhabricatorIndexEngine.php b/src/applications/search/index/PhabricatorIndexEngine.php index 1e1781f169..2517bf994f 100644 --- a/src/applications/search/index/PhabricatorIndexEngine.php +++ b/src/applications/search/index/PhabricatorIndexEngine.php @@ -109,8 +109,10 @@ final class PhabricatorIndexEngine extends Phobject { $rows = queryfx_all( $conn_r, - 'SELECT * FROM %T WHERE objectPHID = %s AND extensionKey IN (%Ls)', - $table->getTableName(), + 'SELECT version, extensionKey + FROM %R + WHERE objectPHID = %s AND extensionKey IN (%Ls)', + $table, $object_phid, $extension_keys); @@ -128,22 +130,35 @@ final class PhabricatorIndexEngine extends Phobject { $table = new PhabricatorSearchIndexVersion(); $conn_w = $table->establishConnection('w'); + $now = PhabricatorTime::getNow(); + + // See T13587. For now, this is just a marker to make it easy to reindex + // documents if some version of the indexing code is later discovered to + // be questionable. + $index_version = '2021-02-16-A'; + $sql = array(); foreach ($versions as $key => $version) { $sql[] = qsprintf( $conn_w, - '(%s, %s, %s)', + '(%s, %s, %s, %s, %d)', $object_phid, $key, - $version); + $version, + $index_version, + $now); } queryfx( $conn_w, - 'INSERT INTO %T (objectPHID, extensionKey, version) + 'INSERT INTO %R (objectPHID, extensionKey, version, + indexVersion, indexEpoch) VALUES %LQ - ON DUPLICATE KEY UPDATE version = VALUES(version)', - $table->getTableName(), + ON DUPLICATE KEY UPDATE + version = VALUES(version), + indexVersion = VALUES(indexVersion), + indexEpoch = VALUES(indexEpoch)', + $table, $sql); } diff --git a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php index 984eeae5fb..b60a3d75f0 100644 --- a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php @@ -8,9 +8,13 @@ final class PhabricatorSearchManagementIndexWorkflow ->setName('index') ->setSynopsis(pht('Build or rebuild search indexes.')) ->setExamples( - "**index** D123\n". - "**index** --type task\n". - "**index** --all") + implode( + "\n", + array( + '**index** D123', + '**index** --all', + '**index** [--type __task__] [--version __version__] ...', + ))) ->setArguments( array( array( @@ -20,6 +24,7 @@ final class PhabricatorSearchManagementIndexWorkflow array( 'name' => 'type', 'param' => 'type', + 'repeat' => true, 'help' => pht( 'Object types to reindex, like "task", "commit" or "revision".'), ), @@ -37,6 +42,28 @@ final class PhabricatorSearchManagementIndexWorkflow 'Force a complete rebuild of the entire index instead of an '. 'incremental update.'), ), + array( + 'name' => 'version', + 'param' => 'version', + 'repeat' => true, + 'help' => pht( + 'Reindex objects previously indexed with a particular '. + 'version of the indexer.'), + ), + array( + 'name' => 'min-index-date', + 'param' => 'date', + 'help' => pht( + 'Reindex objects previously indexed on or after a '. + 'given date.'), + ), + array( + 'name' => 'max-index-date', + 'param' => 'date', + 'help' => pht( + 'Reindex objects previously indexed on or before a '. + 'given date.'), + ), array( 'name' => 'objects', 'wildcard' => true, @@ -47,37 +74,46 @@ final class PhabricatorSearchManagementIndexWorkflow public function execute(PhutilArgumentParser $args) { $this->validateClusterSearchConfig(); - $console = PhutilConsole::getConsole(); - $is_all = $args->getArg('all'); - $is_type = $args->getArg('type'); $is_force = $args->getArg('force'); - $obj_names = $args->getArg('objects'); + $object_types = $args->getArg('type'); + $index_versions = $args->getArg('version'); - if ($obj_names && ($is_all || $is_type)) { + $min_epoch = $args->getArg('min-index-date'); + if ($min_epoch !== null) { + $min_epoch = $this->parseTimeArgument($min_epoch); + } + + $max_epoch = $args->getArg('max-index-date'); + if ($max_epoch !== null) { + $max_epoch = $this->parseTimeArgument($max_epoch); + } + + $object_names = $args->getArg('objects'); + + $any_constraints = + ($object_names) || + ($object_types) || + ($index_versions) || + ($min_epoch) || + ($max_epoch); + + if ($is_all && $any_constraints) { throw new PhutilArgumentUsageException( pht( - "You can not name objects to index alongside the '%s' or '%s' flags.", - '--all', - '--type')); - } else if (!$obj_names && !($is_all || $is_type)) { + 'You can not use query constraint flags (like "--version", '. + '"--type", or a list of specific objects) with "--all".')); + } + + if (!$is_all && !$any_constraints) { throw new PhutilArgumentUsageException( pht( - "Provide one of '%s', '%s' or a list of object names.", - '--all', - '--type')); + 'Provide a list of objects to index (like "D123"), or a set of '. + 'query constraint flags (like "--type"), or "--all" to index '. + 'all objects.')); } - if ($obj_names) { - $phids = $this->loadPHIDsByNames($obj_names); - } else { - $phids = $this->loadPHIDsByTypes($is_type); - } - - if (!$phids) { - throw new PhutilArgumentUsageException(pht('Nothing to index!')); - } if ($args->getArg('background')) { $is_background = true; @@ -87,21 +123,80 @@ final class PhabricatorSearchManagementIndexWorkflow } if (!$is_background) { - echo tsprintf( - "** %s ** %s\n", + $this->logInfo( pht('NOTE'), pht( - 'Run this workflow with "%s" to queue tasks for the daemon workers.', - '--background')); + 'Run this workflow with "--background" to queue tasks for the '. + 'daemon workers.')); } - $groups = phid_group_by_type($phids); - foreach ($groups as $group_type => $group) { - $console->writeOut( - "%s\n", - pht('Indexing %d object(s) of type %s.', count($group), $group_type)); + $this->logInfo( + pht('SELECT'), + pht('Selecting objects to index...')); + + $object_phids = null; + if ($object_names) { + $object_phids = $this->loadPHIDsByNames($object_names); + $object_phids = array_fuse($object_phids); } + $type_phids = null; + if ($is_all || $object_types) { + $object_map = $this->getIndexableObjectsByTypes($object_types); + $type_phids = array(); + foreach ($object_map as $object) { + $iterator = new LiskMigrationIterator($object); + foreach ($iterator as $o) { + $type_phids[] = $o->getPHID(); + } + } + $type_phids = array_fuse($type_phids); + } + + $index_phids = null; + if ($index_versions || $min_epoch || $max_epoch) { + $index_phids = $this->loadPHIDsByIndexConstraints( + $index_versions, + $min_epoch, + $max_epoch); + $index_phids = array_fuse($index_phids); + } + + $working_set = null; + $filter_sets = array( + $object_phids, + $type_phids, + $index_phids, + ); + + foreach ($filter_sets as $filter_set) { + if ($filter_set === null) { + continue; + } + + if ($working_set === null) { + $working_set = $filter_set; + continue; + } + + $working_set = array_intersect_key($working_set, $filter_set); + } + + $phids = array_keys($working_set); + + if (!$phids) { + $this->logWarn( + pht('NO OBJECTS'), + pht('No objects selected to index.')); + return 0; + } + + $this->logInfo( + pht('INDEXING'), + pht( + 'Indexing %s object(s).', + phutil_count($phids))); + $bar = id(new PhutilConsoleProgressBar()) ->setTotal(count($phids)); @@ -166,8 +261,7 @@ final class PhabricatorSearchManagementIndexWorkflow if ($track_skips) { if ($count_updated) { - echo tsprintf( - "** %s ** %s\n", + $this->logOkay( pht('DONE'), pht( 'Updated search indexes for %s document(s).', @@ -175,29 +269,25 @@ final class PhabricatorSearchManagementIndexWorkflow } if ($count_skipped) { - echo tsprintf( - "** %s ** %s\n", + $this->logWarn( pht('SKIP'), pht( 'Skipped %s documents(s) which have not updated since they were '. 'last indexed.', new PhutilNumber($count_skipped))); - echo tsprintf( - "** %s ** %s\n", + $this->logInfo( pht('NOTE'), pht( 'Use "--force" to force the index to update these documents.')); } } else if ($is_background) { - echo tsprintf( - "** %s ** %s\n", + $this->logOkay( pht('DONE'), pht( 'Queued %s document(s) for background indexing.', new PhutilNumber(count($phids)))); } else { - echo tsprintf( - "** %s ** %s\n", + $this->logOkay( pht('DONE'), pht( 'Forced search index updates for %s document(s).', @@ -224,62 +314,100 @@ final class PhabricatorSearchManagementIndexWorkflow return mpull($objects, 'getPHID'); } - private function loadPHIDsByTypes($type) { + private function getIndexableObjectsByTypes(array $types) { $objects = id(new PhutilClassMapQuery()) ->setAncestorClass('PhabricatorIndexableInterface') ->execute(); - $normalized_type = phutil_utf8_strtolower($type); + $type_map = array(); + $normal_map = array(); + foreach ($types as $type) { + $normalized_type = phutil_utf8_strtolower($type); + $type_map[$type] = $normalized_type; - $matches = array(); + if (isset($normal_map[$normalized_type])) { + $old_type = $normal_map[$normalized_type]; + throw new PhutilArgumentUsageException( + pht( + 'Type specification "%s" duplicates type specification "%s". '. + 'Specify each type only once.', + $type, + $old_type)); + } + + $normal_map[$normalized_type] = $type; + } + + $object_matches = array(); + + $matches_map = array(); + $exact_map = array(); foreach ($objects as $object) { $object_class = get_class($object); + + if (!$types) { + $object_matches[$object_class] = $object; + continue; + } + $normalized_class = phutil_utf8_strtolower($object_class); - if ($normalized_class === $normalized_type) { - $matches = array($object_class => $object); - break; + // If a specified type is exactly the name of this class, match it. + if (isset($normal_map[$normalized_class])) { + $object_matches[$object_class] = $object; + $matching_type = $normal_map[$normalized_class]; + $matches_map[$matching_type] = array($object_class); + $exact_map[$matching_type] = true; + continue; } - if (!strlen($type) || - strpos($normalized_class, $normalized_type) !== false) { - $matches[$object_class] = $object; + foreach ($type_map as $type => $normalized_type) { + // If we already have an exact match for this type, don't match it + // as a substring. An indexable "MothObject" should be selectable + // exactly without also selecting "MammothObject". + if (isset($exact_map[$type])) { + continue; + } + // If the selector isn't a substring of the class name, continue. + if (strpos($normalized_class, $normalized_type) === false) { + continue; + } + + $matches_map[$type][] = $object_class; + $object_matches[$object_class] = $object; } } - if (!$matches) { - $all_types = array(); - foreach ($objects as $object) { - $all_types[] = get_class($object); + $all_types = array(); + foreach ($objects as $object) { + $all_types[] = get_class($object); + } + sort($all_types); + $type_list = implode(', ', $all_types); + + foreach ($type_map as $type => $normalized_type) { + $matches = idx($matches_map, $type); + if (!$matches) { + throw new PhutilArgumentUsageException( + pht( + 'Type "%s" matches no indexable objects. '. + 'Supported types are: %s.', + $type, + $type_list)); } - sort($all_types); - throw new PhutilArgumentUsageException( - pht( - 'Type "%s" matches no indexable objects. Supported types are: %s.', - $type, - implode(', ', $all_types))); - } - - if ((count($matches) > 1) && strlen($type)) { - throw new PhutilArgumentUsageException( - pht( - 'Type "%s" matches multiple indexable objects. Use a more '. - 'specific string. Matching object types are: %s.', - $type, - implode(', ', array_keys($matches)))); - } - - $phids = array(); - foreach ($matches as $match) { - $iterator = new LiskMigrationIterator($match); - foreach ($iterator as $object) { - $phids[] = $object->getPHID(); + if (count($matches) > 1) { + throw new PhutilArgumentUsageException( + pht( + 'Type "%s" matches multiple indexable objects. Use a more '. + 'specific string. Matching objects are: %s.', + $type, + implode(', ', $matches))); } } - return $phids; + return $object_matches; } private function loadIndexVersions($phid) { @@ -294,4 +422,43 @@ final class PhabricatorSearchManagementIndexWorkflow $phid); } + private function loadPHIDsByIndexConstraints( + array $index_versions, + $min_date, + $max_date) { + + $table = new PhabricatorSearchIndexVersion(); + $conn = $table->establishConnection('r'); + + $where = array(); + if ($index_versions) { + $where[] = qsprintf( + $conn, + 'indexVersion IN (%Ls)', + $index_versions); + } + + if ($min_date !== null) { + $where[] = qsprintf( + $conn, + 'indexEpoch >= %d', + $min_date); + } + + if ($max_date !== null) { + $where[] = qsprintf( + $conn, + 'indexEpoch <= %d', + $max_date); + } + + $rows = queryfx_all( + $conn, + 'SELECT DISTINCT objectPHID FROM %R WHERE %LA', + $table, + $where); + + return ipull($rows, 'objectPHID'); + } + } diff --git a/src/applications/search/storage/PhabricatorSearchIndexVersion.php b/src/applications/search/storage/PhabricatorSearchIndexVersion.php index 702b1ea4d6..c6c8be0447 100644 --- a/src/applications/search/storage/PhabricatorSearchIndexVersion.php +++ b/src/applications/search/storage/PhabricatorSearchIndexVersion.php @@ -6,6 +6,8 @@ final class PhabricatorSearchIndexVersion protected $objectPHID; protected $extensionKey; protected $version; + protected $indexVersion; + protected $indexEpoch; protected function getConfiguration() { return array( @@ -13,12 +15,18 @@ final class PhabricatorSearchIndexVersion self::CONFIG_COLUMN_SCHEMA => array( 'extensionKey' => 'text64', 'version' => 'text128', + 'indexVersion' => 'bytes12', + 'indexEpoch' => 'epoch', ), self::CONFIG_KEY_SCHEMA => array( 'key_object' => array( 'columns' => array('objectPHID', 'extensionKey'), 'unique' => true, ), + + // NOTE: "bin/search index" may query this table by "indexVersion" or + // "indexEpoch", but this is rare and scanning the table seems fine. + ), ) + parent::getConfiguration(); } From bd4d9d88f2df1203a0b6259c701f1306865a44ef Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 17 Feb 2021 13:08:14 -0800 Subject: [PATCH 13/17] Limit remarkup URI protocol length to 32 characters to avoid expensive regex behavior Summary: Ref T13608. When searching for bare URIs in remarkup text, don't look for URIs with a protocol string longer than 32 characters. This avoids a case where the regexp engine may be tricked into executing at `O(N^2)` or some similar complexity. Test Plan: - Applied remarkup to "AAAA..." (512KB). - Before: 64 seconds to process. - After: <10ms to process. - Ran unit tests. Maniphest Tasks: T13608 Differential Revision: https://secure.phabricator.com/D21562 --- .../PhutilRemarkupHyperlinkRule.php | 40 ++++++++++++++++--- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php b/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php index a926ea44c1..77168c97e3 100644 --- a/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php +++ b/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php @@ -9,18 +9,47 @@ final class PhutilRemarkupHyperlinkRule extends PhutilRemarkupRule { } public function apply($text) { + static $angle_pattern; + static $curly_pattern; + static $bare_pattern; + + if ($angle_pattern === null) { + // See T13608. Limit protocol matches to 32 characters to improve the + // performance of the "://" pattern, which can take a very long + // time to match against long inputs if the maximum length of a protocol + // sequence is unrestricted. + + $protocol_fragment = '\w{3,32}'; + $uri_fragment = '[^\s'.PhutilRemarkupBlockStorage::MAGIC_BYTE.']+'; + + $angle_pattern = sprintf( + '(<(%s://%s?)>)', + $protocol_fragment, + $uri_fragment); + + $curly_pattern = sprintf( + '({(%s://%s?)})', + $protocol_fragment, + $uri_fragment); + + $bare_pattern = sprintf( + '(%s://%s)', + $protocol_fragment, + $uri_fragment); + } + // Hyperlinks with explicit "<>" around them get linked exactly, without // the "<>". Angle brackets are basically special and mean "this is a URL // with weird characters". This is assumed to be reasonable because they - // don't appear in normal text or normal URLs. + // don't appear in most normal text or most normal URLs. $text = preg_replace_callback( - '@<(\w{3,}://[^\s'.PhutilRemarkupBlockStorage::MAGIC_BYTE.']+?)>@', + $angle_pattern, array($this, 'markupHyperlinkAngle'), $text); // We match "{uri}", but do not link it by default. $text = preg_replace_callback( - '@{(\w{3,}://[^\s'.PhutilRemarkupBlockStorage::MAGIC_BYTE.']+?)}@', + $curly_pattern, array($this, 'markupHyperlinkCurly'), $text); @@ -31,8 +60,9 @@ final class PhutilRemarkupHyperlinkRule extends PhutilRemarkupRule { // NOTE: We're explicitly avoiding capturing stored blocks, so text like // `http://www.example.com/[[x | y]]` doesn't get aggressively captured. + $text = preg_replace_callback( - '@(\w{3,}://[^\s'.PhutilRemarkupBlockStorage::MAGIC_BYTE.']+)@', + $bare_pattern, array($this, 'markupHyperlinkUngreedy'), $text); @@ -110,7 +140,7 @@ final class PhutilRemarkupHyperlinkRule extends PhutilRemarkupRule { } protected function markupHyperlinkUngreedy($matches) { - $match = $matches[1]; + $match = $matches[0]; $tail = null; $trailing = null; if (preg_match('/[;,.:!?]+$/', $match, $trailing)) { From 8cfd22c5fe88cf81ce5b2c188b207538aab04f7a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 18 Feb 2021 11:55:20 -0800 Subject: [PATCH 14/17] Add a negative lookbehind to the Remarkup "bare URI" regular expression pattern Summary: Ref T13608. Building on D21562, further anchor this pattern by adding a negative lookbehind. Test Plan: Ran unit tests. Maniphest Tasks: T13608 Differential Revision: https://secure.phabricator.com/D21568 --- .../markuprule/PhutilRemarkupHyperlinkRule.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php b/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php index 77168c97e3..560aa180c3 100644 --- a/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php +++ b/src/infrastructure/markup/markuprule/PhutilRemarkupHyperlinkRule.php @@ -14,10 +14,13 @@ final class PhutilRemarkupHyperlinkRule extends PhutilRemarkupRule { static $bare_pattern; if ($angle_pattern === null) { - // See T13608. Limit protocol matches to 32 characters to improve the - // performance of the "://" pattern, which can take a very long - // time to match against long inputs if the maximum length of a protocol - // sequence is unrestricted. + // See T13608. A previous version of this code matched bare URIs + // starting with "\w{3,}", which can take a very long time to match + // against long inputs. + // + // Use a protocol length limit in all patterns for general sanity, + // and a negative lookbehind in the bare pattern to avoid explosive + // complexity during expression evaluation. $protocol_fragment = '\w{3,32}'; $uri_fragment = '[^\s'.PhutilRemarkupBlockStorage::MAGIC_BYTE.']+'; @@ -33,7 +36,7 @@ final class PhutilRemarkupHyperlinkRule extends PhutilRemarkupRule { $uri_fragment); $bare_pattern = sprintf( - '(%s://%s)', + '((? Date: Thu, 18 Feb 2021 13:34:07 -0800 Subject: [PATCH 15/17] Add more useful PHIDs to Harbormaster build variables Summary: Ref T13609. Add the Object PHID (object being built), Container PHID (container of the object being built), Build PHID, and Buildable PHID to Harbormaster build variables. Test Plan: {F8448191} {F8448192} Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13609 Differential Revision: https://secure.phabricator.com/D21569 --- .../storage/build/HarbormasterBuild.php | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuild.php b/src/applications/harbormaster/storage/build/HarbormasterBuild.php index 8243d2a577..7af5af092f 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuild.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuild.php @@ -128,6 +128,11 @@ final class HarbormasterBuild extends HarbormasterDAO 'step.timestamp' => null, 'build.id' => null, 'initiator.phid' => null, + + 'buildable.phid' => null, + 'buildable.object.phid' => null, + 'buildable.container.phid' => null, + 'build.phid' => null, ); foreach ($this->getBuildParameters() as $key => $value) { @@ -145,6 +150,11 @@ final class HarbormasterBuild extends HarbormasterDAO $results['build.id'] = $this->getID(); $results['initiator.phid'] = $this->getInitiatorPHID(); + $results['buildable.phid'] = $buildable->getPHID(); + $results['buildable.object.phid'] = $object->getPHID(); + $results['buildable.container.phid'] = $buildable->getContainerPHID(); + $results['build.phid'] = $this->getPHID(); + return $results; } @@ -161,6 +171,16 @@ final class HarbormasterBuild extends HarbormasterDAO 'initiator.phid' => pht( 'The PHID of the user or Object that initiated the build, '. 'if applicable.'), + 'buildable.phid' => pht( + 'The object PHID of the Harbormaster Buildable being built.'), + 'buildable.object.phid' => pht( + 'The object PHID of the object (usually a diff or commit) '. + 'being built.'), + 'buildable.container.phid' => pht( + 'The object PHID of the container (usually a revision or repository) '. + 'for the object being built.'), + 'build.phid' => pht( + 'The object PHID of the Harbormaster Build being built.'), ); foreach ($objects as $object) { From b3976acc40a61c412bac91193e9da6c624a03de8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 19 Feb 2021 07:44:28 -0800 Subject: [PATCH 16/17] Improve performance of "phabricator:20210215.changeset.02.phid-populate.php" Summary: Ref T13613. Improve the performance of this migration by using a temporary table and an "UPDATE x JOIN y ..." pattern. Test Plan: - Ran on `secure`, got exit after a few seconds since the migration is idempotent and changesets already had PHIDs. - Ran on `secure` with the `continue;` commented out, got valid new PHIDs in 53s (from 153s). - Tried a larger page size (16K), didn't see any improvement. - From "--trace", client PHID generation seems to be the limiting factor. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13613 Differential Revision: https://secure.phabricator.com/D21570 --- .../20210215.changeset.02.phid-populate.php | 61 ++++++++++++++++--- 1 file changed, 53 insertions(+), 8 deletions(-) diff --git a/resources/sql/autopatches/20210215.changeset.02.phid-populate.php b/resources/sql/autopatches/20210215.changeset.02.phid-populate.php index da64650006..2f35a18172 100644 --- a/resources/sql/autopatches/20210215.changeset.02.phid-populate.php +++ b/resources/sql/autopatches/20210215.changeset.02.phid-populate.php @@ -7,20 +7,65 @@ $changeset_table = new DifferentialChangeset(); $conn = $changeset_table->establishConnection('w'); $table_name = $changeset_table->getTableName(); -$iterator = new LiskRawMigrationIterator($conn, $table_name); -foreach ($iterator as $changeset_row) { - $phid = $changeset_row['phid']; +$chunk_size = 4096; - if (strlen($phid)) { +$temporary_table = 'tmp_20210215_changeset_id_map'; + +queryfx( + $conn, + 'CREATE TEMPORARY TABLE %T ( + changeset_id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + changeset_phid VARBINARY(64) NOT NULL)', + $temporary_table); + +$table_iterator = id(new LiskRawMigrationIterator($conn, $table_name)) + ->setPageSize($chunk_size); + +$chunk_iterator = new PhutilChunkedIterator($table_iterator, $chunk_size); +foreach ($chunk_iterator as $chunk) { + + $map = array(); + foreach ($chunk as $changeset_row) { + $phid = $changeset_row['phid']; + + if (strlen($phid)) { + continue; + } + + $phid = PhabricatorPHID::generateNewPHID($phid_type); + $id = $changeset_row['id']; + + $map[(int)$id] = $phid; + } + + if (!$map) { continue; } - $phid = PhabricatorPHID::generateNewPHID($phid_type); + $sql = array(); + foreach ($map as $changeset_id => $changeset_phid) { + $sql[] = qsprintf( + $conn, + '(%d, %s)', + $changeset_id, + $changeset_phid); + } queryfx( $conn, - 'UPDATE %T SET phid = %s WHERE id = %d', + 'TRUNCATE TABLE %T', + $temporary_table); + + queryfx( + $conn, + 'INSERT INTO %T (changeset_id, changeset_phid) VALUES %LQ', + $temporary_table, + $sql); + + queryfx( + $conn, + 'UPDATE %T c JOIN %T x ON c.id = x.changeset_id + SET c.phid = x.changeset_phid', $table_name, - $phid, - $changeset_row['id']); + $temporary_table); } From be0bb68f653696943ad8dbf2a431fc37016e20fb Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 19 Feb 2021 10:24:14 -0800 Subject: [PATCH 17/17] Remove Facebook OAuth dependency on "security_settings" property Summary: Ref T13615. This property was removed from the Facebook API at some point, perhaps November 2020. Stop relying no it. Test Plan: Created a local Facebook OAuth app, registered a new account locally. Maniphest Tasks: T13615 Differential Revision: https://secure.phabricator.com/D21571 --- .../adapter/PhutilFacebookAuthAdapter.php | 23 ---- .../PhabricatorFacebookAuthProvider.php | 112 ++++-------------- 2 files changed, 26 insertions(+), 109 deletions(-) diff --git a/src/applications/auth/adapter/PhutilFacebookAuthAdapter.php b/src/applications/auth/adapter/PhutilFacebookAuthAdapter.php index ab569a14a0..9d087bbd37 100644 --- a/src/applications/auth/adapter/PhutilFacebookAuthAdapter.php +++ b/src/applications/auth/adapter/PhutilFacebookAuthAdapter.php @@ -5,13 +5,6 @@ */ final class PhutilFacebookAuthAdapter extends PhutilOAuthAuthAdapter { - private $requireSecureBrowsing; - - public function setRequireSecureBrowsing($require_secure_browsing) { - $this->requireSecureBrowsing = $require_secure_browsing; - return $this; - } - public function getAdapterType() { return 'facebook'; } @@ -61,10 +54,6 @@ final class PhutilFacebookAuthAdapter extends PhutilOAuthAuthAdapter { return $this->getOAuthAccountData('name'); } - public function getAccountSecuritySettings() { - return $this->getOAuthAccountData('security_settings'); - } - protected function getAuthenticateBaseURI() { return 'https://www.facebook.com/dialog/oauth'; } @@ -79,7 +68,6 @@ final class PhutilFacebookAuthAdapter extends PhutilOAuthAuthAdapter { 'name', 'email', 'link', - 'security_settings', 'picture', ); @@ -97,17 +85,6 @@ final class PhutilFacebookAuthAdapter extends PhutilOAuthAuthAdapter { $ex); } - if ($this->requireSecureBrowsing) { - if (empty($data['security_settings']['secure_browsing']['enabled'])) { - throw new Exception( - pht( - 'This Phabricator install requires you to enable Secure Browsing '. - 'on your Facebook account in order to use it to log in to '. - 'Phabricator. For more information, see %s', - 'https://www.facebook.com/help/156201551113407/')); - } - } - return $data; } diff --git a/src/applications/auth/provider/PhabricatorFacebookAuthProvider.php b/src/applications/auth/provider/PhabricatorFacebookAuthProvider.php index 67840727e8..d7c57b5ddc 100644 --- a/src/applications/auth/provider/PhabricatorFacebookAuthProvider.php +++ b/src/applications/auth/provider/PhabricatorFacebookAuthProvider.php @@ -3,14 +3,35 @@ final class PhabricatorFacebookAuthProvider extends PhabricatorOAuth2AuthProvider { - const KEY_REQUIRE_SECURE = 'oauth:facebook:require-secure'; - public function getProviderName() { return pht('Facebook'); } protected function getProviderConfigurationHelp() { $uri = PhabricatorEnv::getProductionURI($this->getLoginURI()); + + $domain = id(new PhutilURI($uri))->getDomain(); + + $table = array( + 'Client OAuth Login' => pht('No'), + 'Web OAuth Login' => pht('Yes'), + 'Enforce HTTPS' => pht('Yes'), + 'Force Web OAuth Reauthentication' => pht('Yes (Optional)'), + 'Embedded Browser OAuth Login' => pht('No'), + 'Use Strict Mode for Redirect URIs' => pht('Yes'), + 'Login from Devices' => pht('No'), + 'Valid OAuth Redirect URIs' => '`'.(string)$uri.'`', + 'App Domains' => '`'.$domain.'`', + ); + + $rows = array(); + foreach ($table as $k => $v) { + $rows[] = sprintf('| %s | %s |', $k, $v); + $rows[] = sprintf('|----| |'); + } + $rows = implode("\n", $rows); + + return pht( 'To configure Facebook OAuth, create a new Facebook Application here:'. "\n\n". @@ -18,29 +39,15 @@ final class PhabricatorFacebookAuthProvider "\n\n". 'You should use these settings in your application:'. "\n\n". - " - **Site URL**: Set this to `%s`\n". - " - **Valid OAuth redirect URIs**: You should also set this to `%s`\n". - " - **Client OAuth Login**: Set this to **OFF**.\n". - " - **Embedded browser OAuth Login**: Set this to **OFF**.\n". + "%s\n". "\n\n". - "Some of these settings may be in the **Advanced** tab.\n\n". "After creating your new application, copy the **App ID** and ". "**App Secret** to the fields above.", - (string)$uri, - (string)$uri); - } - - public function getDefaultProviderConfig() { - return parent::getDefaultProviderConfig() - ->setProperty(self::KEY_REQUIRE_SECURE, 1); + $rows); } protected function newOAuthAdapter() { - $require_secure = $this->getProviderConfig()->getProperty( - self::KEY_REQUIRE_SECURE); - - return id(new PhutilFacebookAuthAdapter()) - ->setRequireSecureBrowsing($require_secure); + return new PhutilFacebookAuthAdapter(); } protected function getLoginIcon() { @@ -55,71 +62,4 @@ final class PhabricatorFacebookAuthProvider ); } - public function readFormValuesFromProvider() { - $require_secure = $this->getProviderConfig()->getProperty( - self::KEY_REQUIRE_SECURE); - - return parent::readFormValuesFromProvider() + array( - self::KEY_REQUIRE_SECURE => $require_secure, - ); - } - - public function readFormValuesFromRequest(AphrontRequest $request) { - return parent::readFormValuesFromRequest($request) + array( - self::KEY_REQUIRE_SECURE => $request->getBool(self::KEY_REQUIRE_SECURE), - ); - } - - public function extendEditForm( - AphrontRequest $request, - AphrontFormView $form, - array $values, - array $issues) { - - parent::extendEditForm($request, $form, $values, $issues); - - $key_require = self::KEY_REQUIRE_SECURE; - $v_require = idx($values, $key_require); - - $form - ->appendChild( - id(new AphrontFormCheckboxControl()) - ->addCheckbox( - $key_require, - $v_require, - pht( - "%s ". - "Require users to enable 'secure browsing' on Facebook in order ". - "to use Facebook to authenticate with Phabricator. This ". - "improves security by preventing an attacker from capturing ". - "an insecure Facebook session and escalating it into a ". - "Phabricator session. Enabling it is recommended.", - phutil_tag('strong', array(), pht('Require Secure Browsing:'))))); - } - - public function renderConfigPropertyTransactionTitle( - PhabricatorAuthProviderConfigTransaction $xaction) { - - $author_phid = $xaction->getAuthorPHID(); - $old = $xaction->getOldValue(); - $new = $xaction->getNewValue(); - $key = $xaction->getMetadataValue( - PhabricatorAuthProviderConfigTransaction::PROPERTY_KEY); - - switch ($key) { - case self::KEY_REQUIRE_SECURE: - if ($new) { - return pht( - '%s turned "Require Secure Browsing" on.', - $xaction->renderHandleLink($author_phid)); - } else { - return pht( - '%s turned "Require Secure Browsing" off.', - $xaction->renderHandleLink($author_phid)); - } - } - - return parent::renderConfigPropertyTransactionTitle($xaction); - } - }