From 8e680f17a9dc0a115e76843101d47205ec90589f Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 23 Aug 2017 08:57:04 -0700 Subject: [PATCH 01/27] Add Home menu default Dashboard documentation Summary: From Z1336, we don't currently document anywhere how the default dashboard works. I should also update the copy in the UI. Ref T12969 Test Plan: regenerate docs, read carefully Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Maniphest Tasks: T12969 Differential Revision: https://secure.phabricator.com/D18454 --- src/docs/user/userguide/profile_menu.diviner | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/docs/user/userguide/profile_menu.diviner b/src/docs/user/userguide/profile_menu.diviner index 2725e71851..88eb32d3ff 100644 --- a/src/docs/user/userguide/profile_menu.diviner +++ b/src/docs/user/userguide/profile_menu.diviner @@ -153,6 +153,12 @@ to a Project or to a Home menu, that Dashboard will be presented in the context of that menu. This allows customization of different pages of content without having the user leave Home or the Project. +To use a Dashboard to replace the default Home menu, install it as a Global +Menu Item and move it to the topmost item. By default, the first Dashboard +the menu renders will be selected as the default. Users that modify their +personal Home menu, will have their topmost Dashboard be their default, +overriding the Global settings. + Writing New Item Types ====================== From 748725a47d3857da693c905a77b5f37e4fc37e96 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 23 Aug 2017 09:36:47 -0700 Subject: [PATCH 02/27] Don't select disabled menu items as default Summary: Fixes T12969. If you disable "Home" but leave it at the top, we still load it. Test Plan: Disabled "Home". Move Dashboard into first position, see correct home layout. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Maniphest Tasks: T12969 Differential Revision: https://secure.phabricator.com/D18455 --- src/applications/search/engine/PhabricatorProfileMenuEngine.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/search/engine/PhabricatorProfileMenuEngine.php b/src/applications/search/engine/PhabricatorProfileMenuEngine.php index 7b1e213d20..90f056ac4f 100644 --- a/src/applications/search/engine/PhabricatorProfileMenuEngine.php +++ b/src/applications/search/engine/PhabricatorProfileMenuEngine.php @@ -465,7 +465,7 @@ abstract class PhabricatorProfileMenuEngine extends Phobject { $default = null; $first = null; foreach ($items as $item) { - if (!$item->canMakeDefault()) { + if (!$item->canMakeDefault() || $item->isDisabled()) { continue; } From ac91ab1ef9196eee0deabfd70157ccc0d53d666e Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 23 Aug 2017 08:44:04 -0700 Subject: [PATCH 03/27] Update blame view in Diffusion Summary: Ref T12824, adds more information to the blame view, exposes date, commit summary, lighter colors. Test Plan: Review many diffs with and without blame on. {F5111758} {F5111759} Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Maniphest Tasks: T12824 Differential Revision: https://secure.phabricator.com/D18452 --- resources/celerity/map.php | 4 +- .../controller/DiffusionBrowseController.php | 90 +++++++------------ .../diffusion/diffusion-source.css | 61 +++++++++---- 3 files changed, 78 insertions(+), 77 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 87b45c9dc6..22ec7abf53 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -74,7 +74,7 @@ return array( 'rsrc/css/application/diffusion/diffusion-icons.css' => '0c15255e', 'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec', - 'rsrc/css/application/diffusion/diffusion-source.css' => '750add59', + 'rsrc/css/application/diffusion/diffusion-source.css' => '48d222a6', 'rsrc/css/application/diffusion/diffusion.css' => 'ceacf994', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', @@ -574,7 +574,7 @@ return array( 'diffusion-icons-css' => '0c15255e', 'diffusion-readme-css' => '419dd5b6', 'diffusion-repository-css' => 'ee6f20ec', - 'diffusion-source-css' => '750add59', + 'diffusion-source-css' => '48d222a6', 'diviner-shared-css' => '896f1d43', 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 9ab7f194bf..2deb3b8b15 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1116,7 +1116,6 @@ final class DiffusionBrowseController extends DiffusionController { )); } - $skip_text = pht('Skip Past This Commit'); foreach ($display as $line_index => $line) { $row = array(); @@ -1132,12 +1131,14 @@ final class DiffusionBrowseController extends DiffusionController { $revision_link = null; $commit_link = null; $before_link = null; + $commit_date = null; - $style = 'background: '.$line['color'].';'; + $style = 'border-right: 2px solid '.$line['color'].';'; if ($identifier && !$line['duplicate']) { if (isset($commit_links[$identifier])) { - $commit_link = $commit_links[$identifier]; + $commit_link = $commit_links[$identifier]['link']; + $commit_date = $commit_links[$identifier]['date']; } if (isset($revision_map[$identifier])) { @@ -1148,6 +1149,10 @@ final class DiffusionBrowseController extends DiffusionController { } $skip_href = $line_href.'?before='.$identifier.'&view=blame'; + $skip_text = pht('Skip Past This Commit'); + $icon = id(new PHUIIconView()) + ->setIcon('fa-caret-square-o-left'); + $before_link = javelin_tag( 'a', array( @@ -1159,7 +1164,7 @@ final class DiffusionBrowseController extends DiffusionController { 'size' => 300, ), ), - "\xC2\xAB"); + $icon); } if ($show_blame) { @@ -1183,20 +1188,26 @@ final class DiffusionBrowseController extends DiffusionController { 'class' => 'diffusion-rev-link', ), $object_links); + + $row[] = phutil_tag( + 'th', + array( + 'class' => 'diffusion-blame-date', + ), + $commit_date); } $line_link = phutil_tag( 'a', array( 'href' => $line_href, - 'style' => $style, ), $line_number); $row[] = javelin_tag( 'th', array( - 'class' => 'diffusion-line-link', + 'class' => 'diffusion-line-link ', 'sigil' => 'phabricator-source-line', 'style' => $style, ), @@ -1510,33 +1521,6 @@ final class DiffusionBrowseController extends DiffusionController { return head($parents); } - private function renderRevisionTooltip( - DifferentialRevision $revision, - $handles) { - $viewer = $this->getRequest()->getUser(); - - $date = phabricator_date($revision->getDateModified(), $viewer); - $id = $revision->getID(); - $title = $revision->getTitle(); - $header = "D{$id} {$title}"; - - $author = $handles[$revision->getAuthorPHID()]->getName(); - - return "{$header}\n{$date} \xC2\xB7 {$author}"; - } - - private function renderCommitTooltip( - PhabricatorRepositoryCommit $commit, - $author) { - - $viewer = $this->getRequest()->getUser(); - - $date = phabricator_date($commit->getEpoch(), $viewer); - $summary = trim($commit->getSummary()); - - return "{$summary}\n{$date} \xC2\xB7 {$author}"; - } - protected function markupText($text) { $engine = PhabricatorMarkupEngine::newDiffusionMarkupEngine(); $engine->setConfig('viewer', $this->getRequest()->getUser()); @@ -1737,9 +1721,6 @@ final class DiffusionBrowseController extends DiffusionController { ->setViewer($viewer) ->withRepository($repository) ->withIdentifiers($identifiers) - // TODO: We only fetch this to improve author display behavior, but - // shouldn't really need to? - ->needCommitData(true) ->execute(); $commits = mpull($commits, null, 'getCommitIdentifier'); } else { @@ -1751,25 +1732,27 @@ final class DiffusionBrowseController extends DiffusionController { private function renderCommitLinks(array $commits, $handles) { $links = array(); + $viewer = $this->getViewer(); foreach ($commits as $identifier => $commit) { - $tooltip = $this->renderCommitTooltip( - $commit, - $commit->renderAuthorShortName($handles)); + $date = phabricator_date($commit->getEpoch(), $viewer); + $summary = trim($commit->getSummary()); - $commit_link = javelin_tag( + $commit_link = phutil_tag( 'a', array( 'href' => $commit->getURI(), - 'sigil' => 'has-tooltip', - 'meta' => array( - 'tip' => $tooltip, - 'align' => 'E', - 'size' => 600, - ), ), - $commit->getLocalName()); + $summary); - $links[$identifier] = $commit_link; + $commit_date = phutil_tag( + 'a', + array( + 'href' => $commit->getURI(), + ), + $date); + + $links[$identifier]['link'] = $commit_link; + $links[$identifier]['date'] = $commit_date; } return $links; @@ -1780,19 +1763,10 @@ final class DiffusionBrowseController extends DiffusionController { foreach ($revisions as $revision) { $revision_id = $revision->getID(); - - $tooltip = $this->renderRevisionTooltip($revision, $handles); - - $revision_link = javelin_tag( + $revision_link = phutil_tag( 'a', array( 'href' => '/'.$revision->getMonogram(), - 'sigil' => 'has-tooltip', - 'meta' => array( - 'tip' => $tooltip, - 'align' => 'E', - 'size' => 600, - ), ), $revision->getMonogram()); diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index 744f84359a..e4a4f5be8a 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -7,24 +7,22 @@ background: {$page.content}; } -.diffusion-source tr.phabricator-source-highlight { - background: {$sh-yellowbackground}; +.diffusion-source tr.phabricator-source-highlight th, +.diffusion-source tr.phabricator-source-highlight td { + background: {$gentle.highlight}; } .diffusion-source th { text-align: right; vertical-align: top; - background: {$lightgreybackground}; - color: {$bluetext}; + color: {$darkbluetext}; border-right: 1px solid {$thinblueborder}; } .diffusion-source td { vertical-align: top; white-space: pre-wrap; - padding-top: 1px; - padding-bottom: 1px; - padding-left: 8px; + padding: 3px 12px; width: 100%; word-break: break-all; } @@ -34,29 +32,46 @@ } .diffusion-blame-link, -.diffusion-rev-link { +.diffusion-rev-link, +.diffusion-blame-date { white-space: nowrap; } -.diffusion-blame-link { - min-width: 28px; +.diffusion-blame-date { + background: {$lightgreybackground}; + font: {$basefont}; + font-size: {$smallestfontsize}; +} + +.diffusion-blame-link, +.diffusion-line-link { + background: {$lightgreybackground}; } .diffusion-source th.diffusion-rev-link { text-align: left; min-width: 130px; + background: {$lightgreybackground}; + font: {$basefont}; + font-size: {$smallestfontsize}; } -.diffusion-blame-link a, -.diffusion-rev-link a, -.diffusion-line-link a { +.diffusion-source a { color: {$darkbluetext}; } +.diffusion-rev-link a { + max-width: 340px; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; +} + .diffusion-rev-link a, -.diffusion-rev-link span { - margin: 2px 8px 0; - display: inline-block; +.diffusion-rev-link span, +.diffusion-blame-date a { + margin: 3px 8px; + display: block; } .diffusion-rev-link span { @@ -69,7 +84,19 @@ .diffusion-line-link a { /* Give the user a larger click target. */ display: block; - padding: 2px 8px; + padding: 4px 8px 3px; +} + +.diffusion-line-link a { + color: {$lightgreytext}; +} + +.diffusion-blame-link a .phui-icon-view { + color: {$bluetext}; +} + +.diffusion-blame-link a:hover .phui-icon-view { + color: {$sky}; } .diffusion-line-link { From 8c4f5aba3339844e17140222b9c45692b63d67dc Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 21 Aug 2017 18:47:27 -0700 Subject: [PATCH 04/27] Fix Back to HEAD link Summary: I missed an anchor tag here, adds it back Test Plan: View blame, click a previous version of the file, click Back to HEAD link. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18451 --- .../diffusion/controller/DiffusionBrowseController.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 2deb3b8b15..f3c9210c50 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1601,6 +1601,7 @@ final class DiffusionBrowseController extends DiffusionController { $head = null; if ($behind_head) { $head = id(new PHUIButtonView()) + ->setTag('a') ->setText(pht('Back to HEAD')) ->setHref($head_uri) ->setIcon('fa-home') From 63bd1784b08cf77b71adcaa110abe676cf4fc510 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Wed, 23 Aug 2017 14:35:02 -0700 Subject: [PATCH 05/27] Allow more granularity on real-time notifications Summary: Fixes T12792. Expands the Notifications to "web, desktop, both, or none" for real-time notifications in settings. Test Plan: Test with "test notifications" button, and while logged into two accounts with each of the 4 settings. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Maniphest Tasks: T12792 Differential Revision: https://secure.phabricator.com/D18457 --- resources/celerity/map.php | 70 +++++++++---------- src/__phutil_library_map__.php | 8 +-- .../PhabricatorNotificationBuilder.php | 13 ++-- ...icatorNotificationIndividualController.php | 1 + ...PhabricatorNotificationsSettingsPanel.php} | 46 +++++++----- ...PhabricatorDesktopNotificationsSetting.php | 12 ---- .../PhabricatorNotificationsSetting.php | 44 ++++++++++++ .../aphlict/behavior-aphlict-listen.js | 1 + .../behavior-desktop-notifications-control.js | 12 ++-- webroot/rsrc/js/core/Notification.js | 12 ++++ 10 files changed, 140 insertions(+), 79 deletions(-) rename src/applications/settings/panel/{PhabricatorDesktopNotificationsSettingsPanel.php => PhabricatorNotificationsSettingsPanel.php} (83%) delete mode 100644 src/applications/settings/setting/PhabricatorDesktopNotificationsSetting.php create mode 100644 src/applications/settings/setting/PhabricatorNotificationsSetting.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 22ec7abf53..96367a5ef7 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => 'b5b51108', 'core.pkg.css' => 'fe4effd6', - 'core.pkg.js' => '5d80e0db', + 'core.pkg.js' => '396dee49', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', 'differential.pkg.js' => 'b71b8c5d', @@ -375,9 +375,9 @@ return array( 'rsrc/image/texture/table_header_tall.png' => 'd56b434f', 'rsrc/js/application/aphlict/Aphlict.js' => 'e1d4b11a', 'rsrc/js/application/aphlict/behavior-aphlict-dropdown.js' => 'caade6f2', - 'rsrc/js/application/aphlict/behavior-aphlict-listen.js' => '3c547a81', + 'rsrc/js/application/aphlict/behavior-aphlict-listen.js' => 'a14cbdfc', 'rsrc/js/application/aphlict/behavior-aphlict-status.js' => '5e2634b9', - 'rsrc/js/application/aphlict/behavior-desktop-notifications-control.js' => 'd5a2d665', + 'rsrc/js/application/aphlict/behavior-desktop-notifications-control.js' => '27ca6289', 'rsrc/js/application/calendar/behavior-day-view.js' => '4b3c4443', 'rsrc/js/application/calendar/behavior-event-all-day.js' => 'b41537c9', 'rsrc/js/application/calendar/behavior-month-view.js' => 'fe33e256', @@ -468,7 +468,7 @@ return array( 'rsrc/js/core/KeyboardShortcut.js' => '1ae869f2', 'rsrc/js/core/KeyboardShortcutManager.js' => 'c19dd9b9', 'rsrc/js/core/MultirowRowManager.js' => 'b5d57730', - 'rsrc/js/core/Notification.js' => 'ccf1cbf8', + 'rsrc/js/core/Notification.js' => '5c3349b2', 'rsrc/js/core/Prefab.js' => 'c5af80a2', 'rsrc/js/core/ShapedRequest.js' => '7cbe244b', 'rsrc/js/core/TextAreaUtils.js' => '320810c8', @@ -587,7 +587,7 @@ return array( 'javelin-aphlict' => 'e1d4b11a', 'javelin-behavior' => '61cbc29a', 'javelin-behavior-aphlict-dropdown' => 'caade6f2', - 'javelin-behavior-aphlict-listen' => '3c547a81', + 'javelin-behavior-aphlict-listen' => 'a14cbdfc', 'javelin-behavior-aphlict-status' => '5e2634b9', 'javelin-behavior-aphront-basic-tokenizer' => 'b3a4b884', 'javelin-behavior-aphront-drag-and-drop-textarea' => '484a6e22', @@ -612,7 +612,7 @@ return array( 'javelin-behavior-dashboard-query-panel-select' => '453c5375', 'javelin-behavior-dashboard-tab-panel' => 'd4eecc63', 'javelin-behavior-day-view' => '4b3c4443', - 'javelin-behavior-desktop-notifications-control' => 'd5a2d665', + 'javelin-behavior-desktop-notifications-control' => '27ca6289', 'javelin-behavior-detect-timezone' => '4c193c96', 'javelin-behavior-device' => 'bb1dd507', 'javelin-behavior-diff-preview-link' => '051c7832', @@ -791,7 +791,7 @@ return array( 'phabricator-keyboard-shortcut-manager' => 'c19dd9b9', 'phabricator-main-menu-view' => '16053029', 'phabricator-nav-view-css' => 'faf6a6fc', - 'phabricator-notification' => 'ccf1cbf8', + 'phabricator-notification' => '5c3349b2', 'phabricator-notification-css' => '3f6c89c9', 'phabricator-notification-menu-css' => '73fefdfa', 'phabricator-object-selector-css' => '85ee8ce6', @@ -1058,6 +1058,13 @@ return array( 'phabricator-drag-and-drop-file-upload', 'javelin-workboard-board', ), + '27ca6289' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'javelin-uri', + 'phabricator-notification', + ), '2926fff2' => array( 'javelin-behavior', 'javelin-dom', @@ -1115,20 +1122,6 @@ return array( 'javelin-dom', 'javelin-magical-init', ), - '3c547a81' => array( - 'javelin-behavior', - 'javelin-aphlict', - 'javelin-stratcom', - 'javelin-request', - 'javelin-uri', - 'javelin-dom', - 'javelin-json', - 'javelin-router', - 'javelin-util', - 'javelin-leader', - 'javelin-sound', - 'phabricator-notification', - ), '3cb0b2fc' => array( 'javelin-behavior', 'javelin-dom', @@ -1338,6 +1331,13 @@ return array( 'javelin-vector', 'javelin-dom', ), + '5c3349b2' => array( + 'javelin-install', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-util', + 'phabricator-notification-css', + ), '5c54cbf3' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1676,6 +1676,20 @@ return array( 'javelin-util', 'phabricator-keyboard-shortcut', ), + 'a14cbdfc' => array( + 'javelin-behavior', + 'javelin-aphlict', + 'javelin-stratcom', + 'javelin-request', + 'javelin-uri', + 'javelin-dom', + 'javelin-json', + 'javelin-router', + 'javelin-util', + 'javelin-leader', + 'javelin-sound', + 'phabricator-notification', + ), 'a37126bd' => array( 'javelin-install', 'javelin-dom', @@ -1951,13 +1965,6 @@ return array( 'cae95e89' => array( 'syntax-default-css', ), - 'ccf1cbf8' => array( - 'javelin-install', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-util', - 'phabricator-notification-css', - ), 'cd2b9b77' => array( 'phui-oi-list-view-css', ), @@ -1996,13 +2003,6 @@ return array( 'javelin-dom', 'javelin-stratcom', ), - 'd5a2d665' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'javelin-uri', - 'phabricator-notification', - ), 'd6a7e717' => array( 'multirow-row-manager', 'javelin-install', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 070f56d5c8..fb94c23eeb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2647,8 +2647,6 @@ phutil_register_library_map(array( 'PhabricatorDebugController' => 'applications/system/controller/PhabricatorDebugController.php', 'PhabricatorDefaultRequestExceptionHandler' => 'aphront/handler/PhabricatorDefaultRequestExceptionHandler.php', 'PhabricatorDefaultSyntaxStyle' => 'infrastructure/syntax/PhabricatorDefaultSyntaxStyle.php', - 'PhabricatorDesktopNotificationsSetting' => 'applications/settings/setting/PhabricatorDesktopNotificationsSetting.php', - 'PhabricatorDesktopNotificationsSettingsPanel' => 'applications/settings/panel/PhabricatorDesktopNotificationsSettingsPanel.php', 'PhabricatorDestructibleCodex' => 'applications/system/codex/PhabricatorDestructibleCodex.php', 'PhabricatorDestructibleCodexInterface' => 'applications/system/interface/PhabricatorDestructibleCodexInterface.php', 'PhabricatorDestructibleInterface' => 'applications/system/interface/PhabricatorDestructibleInterface.php', @@ -3214,6 +3212,8 @@ phutil_register_library_map(array( 'PhabricatorNotificationTestFeedStory' => 'applications/notification/feed/PhabricatorNotificationTestFeedStory.php', 'PhabricatorNotificationUIExample' => 'applications/uiexample/examples/PhabricatorNotificationUIExample.php', 'PhabricatorNotificationsApplication' => 'applications/notification/application/PhabricatorNotificationsApplication.php', + 'PhabricatorNotificationsSetting' => 'applications/settings/setting/PhabricatorNotificationsSetting.php', + 'PhabricatorNotificationsSettingsPanel' => 'applications/settings/panel/PhabricatorNotificationsSettingsPanel.php', 'PhabricatorNuanceApplication' => 'applications/nuance/application/PhabricatorNuanceApplication.php', 'PhabricatorOAuth1AuthProvider' => 'applications/auth/provider/PhabricatorOAuth1AuthProvider.php', 'PhabricatorOAuth1SecretTemporaryTokenType' => 'applications/auth/provider/PhabricatorOAuth1SecretTemporaryTokenType.php', @@ -7952,8 +7952,6 @@ phutil_register_library_map(array( 'PhabricatorDebugController' => 'PhabricatorController', 'PhabricatorDefaultRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorDefaultSyntaxStyle' => 'PhabricatorSyntaxStyle', - 'PhabricatorDesktopNotificationsSetting' => 'PhabricatorInternalSetting', - 'PhabricatorDesktopNotificationsSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorDestructibleCodex' => 'Phobject', 'PhabricatorDestructionEngine' => 'Phobject', 'PhabricatorDestructionEngineExtension' => 'Phobject', @@ -8577,6 +8575,8 @@ phutil_register_library_map(array( 'PhabricatorNotificationTestFeedStory' => 'PhabricatorFeedStory', 'PhabricatorNotificationUIExample' => 'PhabricatorUIExample', 'PhabricatorNotificationsApplication' => 'PhabricatorApplication', + 'PhabricatorNotificationsSetting' => 'PhabricatorInternalSetting', + 'PhabricatorNotificationsSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorNuanceApplication' => 'PhabricatorApplication', 'PhabricatorOAuth1AuthProvider' => 'PhabricatorOAuthAuthProvider', 'PhabricatorOAuth1SecretTemporaryTokenType' => 'PhabricatorAuthTemporaryTokenType', diff --git a/src/applications/notification/builder/PhabricatorNotificationBuilder.php b/src/applications/notification/builder/PhabricatorNotificationBuilder.php index 12e4b57bfb..5691755cc7 100644 --- a/src/applications/notification/builder/PhabricatorNotificationBuilder.php +++ b/src/applications/notification/builder/PhabricatorNotificationBuilder.php @@ -145,13 +145,16 @@ final class PhabricatorNotificationBuilder extends Phobject { $dict = array(); $viewer = $this->user; - $desktop_key = PhabricatorDesktopNotificationsSetting::SETTINGKEY; - $desktop_enabled = $viewer->getUserSetting($desktop_key); + $key = PhabricatorNotificationsSetting::SETTINGKEY; + $setting = $viewer->getUserSetting($key); + $desktop_ready = PhabricatorNotificationsSetting::desktopReady($setting); + $web_ready = PhabricatorNotificationsSetting::webReady($setting); foreach ($stories as $story) { if ($story instanceof PhabricatorApplicationTransactionFeedStory) { $dict[] = array( - 'desktopReady' => $desktop_enabled, + 'desktopReady' => $desktop_ready, + 'webReady' => $web_ready, 'title' => $story->renderText(), 'body' => $story->renderTextBody(), 'href' => $story->getURI(), @@ -159,7 +162,8 @@ final class PhabricatorNotificationBuilder extends Phobject { ); } else if ($story instanceof PhabricatorNotificationTestFeedStory) { $dict[] = array( - 'desktopReady' => $desktop_enabled, + 'desktopReady' => $desktop_ready, + 'webReady' => $web_ready, 'title' => pht('Test Notification'), 'body' => $story->renderText(), 'href' => null, @@ -168,6 +172,7 @@ final class PhabricatorNotificationBuilder extends Phobject { } else { $dict[] = array( 'desktopReady' => false, + 'webReady' => false, 'title' => null, 'body' => null, 'href' => null, diff --git a/src/applications/notification/controller/PhabricatorNotificationIndividualController.php b/src/applications/notification/controller/PhabricatorNotificationIndividualController.php index af3e8bcff0..8ef6286225 100644 --- a/src/applications/notification/controller/PhabricatorNotificationIndividualController.php +++ b/src/applications/notification/controller/PhabricatorNotificationIndividualController.php @@ -42,6 +42,7 @@ final class PhabricatorNotificationIndividualController 'pertinent' => true, 'primaryObjectPHID' => $story->getPrimaryObjectPHID(), 'desktopReady' => $data['desktopReady'], + 'webReady' => $data['webReady'], 'href' => $data['href'], 'icon' => $data['icon'], 'title' => $data['title'], diff --git a/src/applications/settings/panel/PhabricatorDesktopNotificationsSettingsPanel.php b/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php similarity index 83% rename from src/applications/settings/panel/PhabricatorDesktopNotificationsSettingsPanel.php rename to src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php index f94de959cd..d61ece6da5 100644 --- a/src/applications/settings/panel/PhabricatorDesktopNotificationsSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorNotificationsSettingsPanel.php @@ -1,6 +1,6 @@ getViewer(); $preferences = $this->getPreferences(); - $notifications_key = PhabricatorDesktopNotificationsSetting::SETTINGKEY; + $notifications_key = PhabricatorNotificationsSetting::SETTINGKEY; $notifications_value = $preferences->getSettingValue($notifications_key); if ($request->isFormPost()) { @@ -43,7 +43,7 @@ final class PhabricatorDesktopNotificationsSettingsPanel ->setURI($this->getPanelURI('?saved=true')); } - $title = pht('Desktop Notifications'); + $title = pht('Notifications'); $control_id = celerity_generate_unique_node_id(); $status_id = celerity_generate_unique_node_id(); $browser_status_id = celerity_generate_unique_node_id(); @@ -97,19 +97,31 @@ final class PhabricatorDesktopNotificationsSettingsPanel 'id' => $message_id, )); + $saved_box = null; + if ($request->getBool('saved')) { + $saved_box = id(new PHUIInfoView()) + ->setSeverity(PHUIInfoView::SEVERITY_NOTICE) + ->appendChild(pht('Changes saved.')); + } + $status_box = id(new PHUIInfoView()) ->setSeverity(PHUIInfoView::SEVERITY_NOTICE) ->setID($status_id) ->setIsHidden(true) ->appendChild($message_container); + $status_box = id(new PHUIBoxView()) + ->addClass('mll mlr') + ->appendChild($status_box); + $control_config = array( 'controlID' => $control_id, 'statusID' => $status_id, 'messageID' => $message_id, 'browserStatusID' => $browser_status_id, 'defaultMode' => 0, - 'desktopMode' => 1, + 'desktop' => 1, + 'desktopOnly' => 2, 'cancelAsk' => $cancel_ask, 'grantedAsk' => $accept_ask, 'deniedAsk' => $reject_ask, @@ -127,16 +139,12 @@ final class PhabricatorDesktopNotificationsSettingsPanel ->setControlID($control_id) ->setName($notifications_key) ->setValue($notifications_value) - ->setOptions( - array( - 1 => pht('Send Desktop Notifications Too'), - 0 => pht('Send Application Notifications Only'), - )) + ->setOptions(PhabricatorNotificationsSetting::getOptionsMap()) ->setCaption( pht( - 'Should Phabricator send desktop notifications? These are sent '. - 'in addition to the notifications within the Phabricator '. - 'application.')) + 'Phabricator can send real-time notifications to your web browser '. + 'or to your desktop. Select where you\'d want to receive these '. + 'real-time updates.')) ->initBehavior( 'desktop-notifications-control', $control_config)) @@ -154,12 +162,14 @@ final class PhabricatorDesktopNotificationsSettingsPanel $form_box = id(new PHUIObjectBoxView()) ->setHeader( id(new PHUIHeaderView()) - ->setHeader(pht('Desktop Notifications')) + ->setHeader(pht('Notifications')) ->addActionLink($test_button)) - ->setForm($form) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) - ->setInfoView($status_box) - ->setFormSaved($request->getBool('saved')); + ->appendChild(array( + $saved_box, + $status_box, + $form, + )); $browser_status_box = id(new PHUIInfoView()) ->setID($browser_status_id) diff --git a/src/applications/settings/setting/PhabricatorDesktopNotificationsSetting.php b/src/applications/settings/setting/PhabricatorDesktopNotificationsSetting.php deleted file mode 100644 index f590d37325..0000000000 --- a/src/applications/settings/setting/PhabricatorDesktopNotificationsSetting.php +++ /dev/null @@ -1,12 +0,0 @@ - pht('Web Only'), + self::WEB_AND_DESKTOP => pht('Web and Desktop'), + self::DESKTOP_ONLY => pht('Desktop Only'), + self::NONE => pht('No Notifications'), + ); + } + + public static function desktopReady($option) { + switch ($option) { + case self::WEB_AND_DESKTOP: + case self::DESKTOP_ONLY: + return true; + } + return false; + } + + public static function webReady($option) { + switch ($option) { + case self::WEB_AND_DESKTOP: + case self::WEB_ONLY: + return true; + } + return false; + } + +} diff --git a/webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js b/webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js index 333e8daac1..2886aa0372 100644 --- a/webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js +++ b/webroot/rsrc/js/application/aphlict/behavior-aphlict-listen.js @@ -82,6 +82,7 @@ JX.behavior('aphlict-listen', function(config) { new JX.Notification() .setContent(JX.$H(response.content)) .setDesktopReady(response.desktopReady) + .setWebReady(response.webReady) .setKey(response.primaryObjectPHID) .setTitle(response.title) .setBody(response.body) diff --git a/webroot/rsrc/js/application/aphlict/behavior-desktop-notifications-control.js b/webroot/rsrc/js/application/aphlict/behavior-desktop-notifications-control.js index 3a9b028ca3..9be9c510b0 100644 --- a/webroot/rsrc/js/application/aphlict/behavior-desktop-notifications-control.js +++ b/webroot/rsrc/js/application/aphlict/behavior-desktop-notifications-control.js @@ -84,12 +84,12 @@ JX.behavior('desktop-notifications-control', function(config, statics) { return; } var value = e.getTarget().value; - if (value == config.desktopMode) { - window.Notification.requestPermission( - function (permission) { - updateFormStatus(permission); - updateBrowserStatus(permission); - }); + if ((value == config.desktop) || (value == config.desktopOnly)) { + window.Notification.requestPermission( + function (permission) { + updateFormStatus(permission); + updateBrowserStatus(permission); + }); } else { var statusEl = JX.$(config.statusID); JX.DOM.hide(statusEl); diff --git a/webroot/rsrc/js/core/Notification.js b/webroot/rsrc/js/core/Notification.js index 50585419c5..9e59ec1501 100644 --- a/webroot/rsrc/js/core/Notification.js +++ b/webroot/rsrc/js/core/Notification.js @@ -27,6 +27,7 @@ JX.install('Notification', { _hideTimer : null, _duration : 12000, _desktopReady : false, + _webReady : false, _key : null, _title : null, _body : null, @@ -35,6 +36,12 @@ JX.install('Notification', { show : function() { var self = JX.Notification; + + // This person doesn't like any real-time notification + if (!this._desktopReady && !this._webReady) { + return; + } + if (!this._visible) { this._visible = true; @@ -92,6 +99,11 @@ JX.install('Notification', { return this; }, + setWebReady : function(ready) { + this._webReady = ready; + return this; + }, + setTitle : function(title) { this._title = title; return this; From 68df3cebc83468bcf8c0ebf7b77a54a19909c9ce Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 23 Aug 2017 13:11:21 -0700 Subject: [PATCH 06/27] Allow task parents and subtasks to be edited via Conduit API Summary: See PHI39. This adds support for editing parents and subtasks of a task via Conduit. It might be nice to tie this into the `PhabricatorObjectRelationship` stuff eventually, but I think we'd effectively end up in the same place anyway in terms of what the API looks like. Test Plan: {F5116163} Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D18456 --- .../maniphest/editor/ManiphestEditEngine.php | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/applications/maniphest/editor/ManiphestEditEngine.php b/src/applications/maniphest/editor/ManiphestEditEngine.php index 9f5e647ca9..b4ca86a442 100644 --- a/src/applications/maniphest/editor/ManiphestEditEngine.php +++ b/src/applications/maniphest/editor/ManiphestEditEngine.php @@ -251,6 +251,56 @@ EODOCS id(new PHUIRemarkupPreviewPanel()) ->setHeader(pht('Description Preview'))); + $parent_type = ManiphestTaskDependedOnByTaskEdgeType::EDGECONST; + $subtask_type = ManiphestTaskDependsOnTaskEdgeType::EDGECONST; + + $src_phid = $object->getPHID(); + if ($src_phid) { + $edge_query = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs(array($src_phid)) + ->withEdgeTypes( + array( + $parent_type, + $subtask_type, + )); + $edge_query->execute(); + + $parent_phids = $edge_query->getDestinationPHIDs( + array($src_phid), + array($parent_type)); + + $subtask_phids = $edge_query->getDestinationPHIDs( + array($src_phid), + array($subtask_type)); + } else { + $parent_phids = array(); + $subtask_phids = array(); + } + + $fields[] = id(new PhabricatorHandlesEditField()) + ->setKey('parents') + ->setLabel(pht('Parents')) + ->setDescription(pht('Parent tasks.')) + ->setConduitDescription(pht('Change the parents of this task.')) + ->setConduitTypeDescription(pht('List of parent task PHIDs.')) + ->setUseEdgeTransactions(true) + ->setIsConduitOnly(true) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $parent_type) + ->setValue($parent_phids); + + $fields[] = id(new PhabricatorHandlesEditField()) + ->setKey('subtasks') + ->setLabel(pht('Subtasks')) + ->setDescription(pht('Subtasks.')) + ->setConduitDescription(pht('Change the subtasks of this task.')) + ->setConduitTypeDescription(pht('List of subtask PHIDs.')) + ->setUseEdgeTransactions(true) + ->setIsConduitOnly(true) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue('edge:type', $subtask_type) + ->setValue($parent_phids); + return $fields; } From b7843da963e2898edbc6cb3ab4fb25d65b3f09f2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 23 Aug 2017 14:45:16 -0700 Subject: [PATCH 07/27] Don't expand folded timelines just because users went to any anchor whatsoever Summary: Ref T12970. See PHI43. Currently, the "Show Older Comments" link gets auto-clicked if the user visits **any** anchor. This is not correct. Instead, only auto-click it if the user visits a numeric anchor. This fixes the behavior approximately 98% of the time. See T12970 for a followup on the remaining ambiguous cases. Test Plan: - Viewed a revision with some folded transactions and a "Show Older Comments" link. - Clicked a link to a file in the table of contents, with a hash like `#1234abcd`. - Before: Timeline expanded and I ended up somewhere bad. - After: Timeline no longer expanded. - Manually changed hash to `#1234` (purely numeric), saw timeline expand. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12970 Differential Revision: https://secure.phabricator.com/D18458 --- .../transactions/behavior-show-older-transactions.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js b/webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js index ab4bf768d8..28754ee3a2 100644 --- a/webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js +++ b/webroot/rsrc/js/application/transactions/behavior-show-older-transactions.js @@ -17,6 +17,13 @@ JX.behavior('phabricator-show-older-transactions', function(config) { if (!hash) { return false; } + + // If the hash isn't purely numeric, ignore it. Comments always have + // numeric hashes. See PHI43 and T12970. + if (!hash.match(/^\d+$/)) { + return false; + } + var id = 'anchor-'+hash; try { JX.$(id); From 1dd27fc1c73327c0dabbfdb4b089af0bef7edef8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 23 Aug 2017 14:56:23 -0700 Subject: [PATCH 08/27] Celerity map. --- resources/celerity/map.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 96367a5ef7..4dc78958e3 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -10,7 +10,7 @@ return array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => 'b5b51108', 'core.pkg.css' => 'fe4effd6', - 'core.pkg.js' => '396dee49', + 'core.pkg.js' => '6c085267', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', 'differential.pkg.js' => 'b71b8c5d', @@ -452,7 +452,7 @@ return array( 'rsrc/js/application/transactions/behavior-comment-actions.js' => '9a6dd75c', 'rsrc/js/application/transactions/behavior-reorder-configs.js' => 'd7a74243', 'rsrc/js/application/transactions/behavior-reorder-fields.js' => 'b59e1e96', - 'rsrc/js/application/transactions/behavior-show-older-transactions.js' => 'ae95d984', + 'rsrc/js/application/transactions/behavior-show-older-transactions.js' => '8f29b364', 'rsrc/js/application/transactions/behavior-transaction-comment-form.js' => 'b23b49e6', 'rsrc/js/application/transactions/behavior-transaction-list.js' => '1f6794f6', 'rsrc/js/application/typeahead/behavior-typeahead-browse.js' => '635de1ec', @@ -665,7 +665,7 @@ return array( 'javelin-behavior-phabricator-remarkup-assist' => 'acd29eee', 'javelin-behavior-phabricator-reveal-content' => '60821bc7', 'javelin-behavior-phabricator-search-typeahead' => 'd0a99ab4', - 'javelin-behavior-phabricator-show-older-transactions' => 'ae95d984', + 'javelin-behavior-phabricator-show-older-transactions' => '8f29b364', 'javelin-behavior-phabricator-tooltips' => 'c420b0b9', 'javelin-behavior-phabricator-transaction-comment-form' => 'b23b49e6', 'javelin-behavior-phabricator-transaction-list' => '1f6794f6', @@ -1575,6 +1575,12 @@ return array( 'javelin-install', 'phuix-button-view', ), + '8f29b364' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'phabricator-busy', + ), '8ff5e24c' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1765,12 +1771,6 @@ return array( 'phuix-autocomplete', 'javelin-mask', ), - 'ae95d984' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'phabricator-busy', - ), 'b003d4fb' => array( 'javelin-behavior', 'javelin-stratcom', From 0a013341721f8b1fc249047fe6db26062138b562 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 24 Aug 2017 12:00:17 -0700 Subject: [PATCH 09/27] Fix float issue on diffusion blame view Summary: These items should be floated, not display: block. Test Plan: Test blame view with commits AND revisions, check they display inline. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18462 --- resources/celerity/map.php | 4 ++-- webroot/rsrc/css/application/diffusion/diffusion-source.css | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 4dc78958e3..94425e2df7 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -74,7 +74,7 @@ return array( 'rsrc/css/application/diffusion/diffusion-icons.css' => '0c15255e', 'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec', - 'rsrc/css/application/diffusion/diffusion-source.css' => '48d222a6', + 'rsrc/css/application/diffusion/diffusion-source.css' => '5c697665', 'rsrc/css/application/diffusion/diffusion.css' => 'ceacf994', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', @@ -574,7 +574,7 @@ return array( 'diffusion-icons-css' => '0c15255e', 'diffusion-readme-css' => '419dd5b6', 'diffusion-repository-css' => 'ee6f20ec', - 'diffusion-source-css' => '48d222a6', + 'diffusion-source-css' => '5c697665', 'diviner-shared-css' => '896f1d43', 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index e4a4f5be8a..8662683a0c 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -71,7 +71,7 @@ .diffusion-rev-link span, .diffusion-blame-date a { margin: 3px 8px; - display: block; + float: left; } .diffusion-rev-link span { From 66613240fa05237a78144405b8097512a6b16d17 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 24 Aug 2017 11:56:33 -0700 Subject: [PATCH 10/27] Have text-less dropdown buttons look better Summary: Using icons and dropdown buttons without text looks a little wonky, this resets the CSS a bit. Test Plan: Review button with icon and text, just icon, just test, and dropdowns. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18461 --- resources/celerity/map.php | 6 +++--- src/applications/uiexample/examples/PHUIButtonExample.php | 7 ++++++- webroot/rsrc/css/phui/button/phui-button.css | 6 ++++++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 94425e2df7..9bb0828d2f 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => 'b5b51108', - 'core.pkg.css' => 'fe4effd6', + 'core.pkg.css' => 'd254d540', 'core.pkg.js' => '6c085267', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -127,7 +127,7 @@ return array( 'rsrc/css/layout/phabricator-source-code-view.css' => 'aea41829', 'rsrc/css/phui/button/phui-button-bar.css' => 'f1ff5494', 'rsrc/css/phui/button/phui-button-simple.css' => '8e1baf68', - 'rsrc/css/phui/button/phui-button.css' => '340f55c1', + 'rsrc/css/phui/button/phui-button.css' => 'a37aa3a8', 'rsrc/css/phui/calendar/phui-calendar-day.css' => '572b1893', 'rsrc/css/phui/calendar/phui-calendar-list.css' => '576be600', 'rsrc/css/phui/calendar/phui-calendar-month.css' => '21154caf', @@ -824,7 +824,7 @@ return array( 'phui-big-info-view-css' => 'acc3492c', 'phui-box-css' => '745e881d', 'phui-button-bar-css' => 'f1ff5494', - 'phui-button-css' => '340f55c1', + 'phui-button-css' => 'a37aa3a8', 'phui-button-simple-css' => '8e1baf68', 'phui-calendar-css' => 'f1ddf11c', 'phui-calendar-day-css' => '572b1893', diff --git a/src/applications/uiexample/examples/PHUIButtonExample.php b/src/applications/uiexample/examples/PHUIButtonExample.php index c7107250ff..f6e8fb526c 100644 --- a/src/applications/uiexample/examples/PHUIButtonExample.php +++ b/src/applications/uiexample/examples/PHUIButtonExample.php @@ -58,10 +58,12 @@ final class PHUIButtonExample extends PhabricatorUIExample { array( 'text' => pht('Comment'), 'icon' => 'fa-comment', + 'dropdown' => true, ), array( 'text' => pht('Give Token'), 'icon' => 'fa-trophy', + 'dropdown' => true, ), array( 'text' => pht('Reverse Time'), @@ -73,9 +75,11 @@ final class PHUIButtonExample extends PhabricatorUIExample { ), array( 'icon' => 'fa-rocket', + 'dropdown' => true, ), array( 'icon' => 'fa-clipboard', + 'dropdown' => true, ), array( 'icon' => 'fa-upload', @@ -95,7 +99,8 @@ final class PHUIButtonExample extends PhabricatorUIExample { ->setColor(PHUIButtonView::GREY) ->setIcon(idx($spec, 'icon')) ->setText(idx($spec, 'text')) - ->addClass(PHUI::MARGIN_SMALL_RIGHT); + ->addClass(PHUI::MARGIN_SMALL_RIGHT) + ->setDropdown(idx($spec, 'dropdown')); $copy = idx($spec, 'copy'); if ($copy !== null) { diff --git a/webroot/rsrc/css/phui/button/phui-button.css b/webroot/rsrc/css/phui/button/phui-button.css index a36d69d4bc..99fef3707c 100644 --- a/webroot/rsrc/css/phui/button/phui-button.css +++ b/webroot/rsrc/css/phui/button/phui-button.css @@ -271,11 +271,17 @@ a.policy-control .phui-button-text { position: relative; } +.button.has-icon.dropdown .phui-icon-view { + margin-right: 8px; + margin-left: -2px; +} + .button.has-text .phui-icon-view { display: inline-block; position: absolute; top: 7px; left: 12px; + margin: 0; } .button.icon-last .phui-icon-view { From d6e47eef19f4e8840bcb0b8f4d4b39dae45d88d7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Aug 2017 05:58:16 -0700 Subject: [PATCH 11/27] Don't set a default "group by priority" in the task search engine Summary: See PHI42. Currently, `maniphest.search` incorrectly applies this default (group by priority) to all queries via Conduit. The correct behavior is to apply no grouping constraint. I think this is also a reasonable general behavior, and the current code seems to date from D6960 in 2013 and didn't seem particularly carefully considered. This is a minor compatibility break -- saved queries which are more than 4 years old might change their group behavior. I'll note this in the change logs but expect essentially no one to be affected. Test Plan: Ran a `maniphest.search` Conduit call and observed the underlying query. Before this change, it executed `ORDER BY priority, id`. After this change, it correctly executed `ORDER BY id` only. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D18459 --- src/applications/maniphest/query/ManiphestTaskSearchEngine.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index 53efaf0bbc..150ec81def 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -236,8 +236,6 @@ final class ManiphestTaskSearchEngine $group = idx($this->getGroupValues(), $group); if ($group) { $query->setGroupBy($group); - } else { - $query->setGroupBy(head($this->getGroupValues())); } if ($map['ids']) { From ec88917dd71e69e8d635aab5e5b0357869695cc3 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 24 Aug 2017 12:39:00 -0700 Subject: [PATCH 12/27] Remove hover state from labels in action list Summary: This is wierd and I can't think of a use for it? Causing issues on hover states. Test Plan: Action list, menus, dropdowns, etc. Labels shouldn't have hover states. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18463 --- resources/celerity/map.php | 6 +++--- webroot/rsrc/css/phui/phui-action-list.css | 6 ------ 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 9bb0828d2f..eecee2ed5a 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => 'b5b51108', - 'core.pkg.css' => 'd254d540', + 'core.pkg.css' => '06a86de6', 'core.pkg.js' => '6c085267', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -138,7 +138,7 @@ return array( 'rsrc/css/phui/object-item/phui-oi-flush-ui.css' => '9d9685d6', 'rsrc/css/phui/object-item/phui-oi-list-view.css' => 'bf094950', 'rsrc/css/phui/object-item/phui-oi-simple-ui.css' => 'a8beebea', - 'rsrc/css/phui/phui-action-list.css' => '6ee16164', + 'rsrc/css/phui/phui-action-list.css' => 'e7eba156', 'rsrc/css/phui/phui-action-panel.css' => 'b4798122', 'rsrc/css/phui/phui-badge.css' => '22c0cf4f', 'rsrc/css/phui/phui-basic-nav-view.css' => 'a0705f53', @@ -767,7 +767,7 @@ return array( 'path-typeahead' => 'f7fc67ec', 'people-picture-menu-item-css' => 'a06f7f34', 'people-profile-css' => '4df76faf', - 'phabricator-action-list-view-css' => '6ee16164', + 'phabricator-action-list-view-css' => 'e7eba156', 'phabricator-busy' => '59a7976a', 'phabricator-chatlog-css' => 'd295b020', 'phabricator-content-source-view-css' => '4b8b05d4', diff --git a/webroot/rsrc/css/phui/phui-action-list.css b/webroot/rsrc/css/phui/phui-action-list.css index 44b93e61be..a7ad662d33 100644 --- a/webroot/rsrc/css/phui/phui-action-list.css +++ b/webroot/rsrc/css/phui/phui-action-list.css @@ -112,12 +112,6 @@ -webkit-font-smoothing: antialiased; } -.device-desktop li.phabricator-action-view-label:hover - .phabricator-action-view-item { - background-color: {$page.content}; - color: {$bluetext}; -} - .phabricator-action-view + .phabricator-action-view-label { padding-top: 8px; } From 58b889c5b0562ea833336afb42ebea9b231f2980 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 14 Aug 2017 09:42:39 -0700 Subject: [PATCH 13/27] Make the default ApplicationSearch query explicit, not just the first item in the list Summary: Ref T12956. Currently, when you visit `/maniphest/` (or any other ApplicationSearch application) we execute the first query in the list by default. In T12956, I plan to make changes so that personal queries are always first, then global/builtin queries. Without changing the "default query" rule, this will make it harder to have, for example, some custom queries in Differential but still run a global query like "Active" by default. To make this work, you'd have to save a personal copy of the "Active" query, then put it at the top. This feels a bit cumbersome and this rule is kind of implicit and a little weird anyway. To make this work a little better as we make changes here, add an explicit pinning action, like the one we have in Project ProfileMenus. You can now explicitly choose a query to make default. Test Plan: - Browsed without pinning anything, saw normal behavior. - Pinned queries, viewed `/maniphest/`, saw a non-initial query selected by default. - Pinned a query, deleted it, nothing exploded. {F5098484} Reviewers: chad Reviewed By: chad Maniphest Tasks: T12956 Differential Revision: https://secure.phabricator.com/D18422 --- .../20170814.search.01.qconfig.sql | 9 ++ src/__phutil_library_map__.php | 9 ++ .../PhabricatorSearchApplication.php | 2 + ...PhabricatorApplicationSearchController.php | 64 ++++++++++--- .../PhabricatorSearchDefaultController.php | 81 ++++++++++++++++ .../PhabricatorSearchEditController.php | 1 - .../PhabricatorApplicationSearchEngine.php | 28 ++++++ .../PhabricatorNamedQueryConfigQuery.php | 64 +++++++++++++ .../storage/PhabricatorNamedQueryConfig.php | 92 +++++++++++++++++++ 9 files changed, 336 insertions(+), 14 deletions(-) create mode 100644 resources/sql/autopatches/20170814.search.01.qconfig.sql create mode 100644 src/applications/search/controller/PhabricatorSearchDefaultController.php create mode 100644 src/applications/search/query/PhabricatorNamedQueryConfigQuery.php create mode 100644 src/applications/search/storage/PhabricatorNamedQueryConfig.php diff --git a/resources/sql/autopatches/20170814.search.01.qconfig.sql b/resources/sql/autopatches/20170814.search.01.qconfig.sql new file mode 100644 index 0000000000..7914336dc4 --- /dev/null +++ b/resources/sql/autopatches/20170814.search.01.qconfig.sql @@ -0,0 +1,9 @@ +CREATE TABLE {$NAMESPACE}_search.search_namedqueryconfig ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + engineClassName VARCHAR(128) NOT NULL COLLATE {$COLLATE_TEXT}, + scopePHID VARBINARY(64) NOT NULL, + properties LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_scope` (engineClassName, scopePHID) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index fb94c23eeb..b15e4f7ff1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3189,6 +3189,8 @@ phutil_register_library_map(array( 'PhabricatorMySQLSearchHost' => 'infrastructure/cluster/search/PhabricatorMySQLSearchHost.php', 'PhabricatorMySQLSetupCheck' => 'applications/config/check/PhabricatorMySQLSetupCheck.php', 'PhabricatorNamedQuery' => 'applications/search/storage/PhabricatorNamedQuery.php', + 'PhabricatorNamedQueryConfig' => 'applications/search/storage/PhabricatorNamedQueryConfig.php', + 'PhabricatorNamedQueryConfigQuery' => 'applications/search/query/PhabricatorNamedQueryConfigQuery.php', 'PhabricatorNamedQueryQuery' => 'applications/search/query/PhabricatorNamedQueryQuery.php', 'PhabricatorNavigationRemarkupRule' => 'infrastructure/markup/rule/PhabricatorNavigationRemarkupRule.php', 'PhabricatorNeverTriggerClock' => 'infrastructure/daemon/workers/clock/PhabricatorNeverTriggerClock.php', @@ -3908,6 +3910,7 @@ phutil_register_library_map(array( 'PhabricatorSearchDatasourceField' => 'applications/search/field/PhabricatorSearchDatasourceField.php', 'PhabricatorSearchDateControlField' => 'applications/search/field/PhabricatorSearchDateControlField.php', 'PhabricatorSearchDateField' => 'applications/search/field/PhabricatorSearchDateField.php', + 'PhabricatorSearchDefaultController' => 'applications/search/controller/PhabricatorSearchDefaultController.php', 'PhabricatorSearchDeleteController' => 'applications/search/controller/PhabricatorSearchDeleteController.php', 'PhabricatorSearchDocument' => 'applications/search/storage/document/PhabricatorSearchDocument.php', 'PhabricatorSearchDocumentField' => 'applications/search/storage/document/PhabricatorSearchDocumentField.php', @@ -8552,6 +8555,11 @@ phutil_register_library_map(array( 'PhabricatorSearchDAO', 'PhabricatorPolicyInterface', ), + 'PhabricatorNamedQueryConfig' => array( + 'PhabricatorSearchDAO', + 'PhabricatorPolicyInterface', + ), + 'PhabricatorNamedQueryConfigQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorNamedQueryQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorNavigationRemarkupRule' => 'PhutilRemarkupRule', 'PhabricatorNeverTriggerClock' => 'PhabricatorTriggerClock', @@ -9448,6 +9456,7 @@ phutil_register_library_map(array( 'PhabricatorSearchDatasourceField' => 'PhabricatorSearchTokenizerField', 'PhabricatorSearchDateControlField' => 'PhabricatorSearchField', 'PhabricatorSearchDateField' => 'PhabricatorSearchField', + 'PhabricatorSearchDefaultController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchDeleteController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchDocument' => 'PhabricatorSearchDAO', 'PhabricatorSearchDocumentField' => 'PhabricatorSearchDAO', diff --git a/src/applications/search/application/PhabricatorSearchApplication.php b/src/applications/search/application/PhabricatorSearchApplication.php index a2a28c7665..22f4228ba8 100644 --- a/src/applications/search/application/PhabricatorSearchApplication.php +++ b/src/applications/search/application/PhabricatorSearchApplication.php @@ -34,6 +34,8 @@ final class PhabricatorSearchApplication extends PhabricatorApplication { 'hovercard/' => 'PhabricatorSearchHovercardController', 'edit/(?P[^/]+)/' => 'PhabricatorSearchEditController', + 'default/(?P[^/]+)/(?P[^/]+)/' + => 'PhabricatorSearchDefaultController', 'delete/(?P[^/]+)/(?P[^/]+)/' => 'PhabricatorSearchDeleteController', 'order/(?P[^/]+)/' => 'PhabricatorSearchOrderController', diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index dbf964f9e4..0e1dfdb699 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -127,7 +127,7 @@ final class PhabricatorApplicationSearchController if (!$found_query_data) { // Otherwise, there's no query data so just run the user's default // query for this application. - $query_key = head_key($engine->loadEnabledNamedQueries()); + $query_key = $engine->getDefaultQueryKey(); } } @@ -400,6 +400,8 @@ final class PhabricatorApplicationSearchController 'orderURI' => '/search/order/'.get_class($engine).'/', )); + $default_key = $engine->getDefaultQueryKey(); + foreach ($named_queries as $named_query) { $class = get_class($engine); $key = $named_query->getQueryKey(); @@ -410,28 +412,64 @@ final class PhabricatorApplicationSearchController if ($named_query->getIsBuiltin() && $named_query->getIsDisabled()) { $icon = 'fa-plus'; + $disable_name = pht('Enable'); } else { $icon = 'fa-times'; + if ($named_query->getIsBuiltin()) { + $disable_name = pht('Disable'); + } else { + $disable_name = pht('Delete'); + } } $item->addAction( id(new PHUIListItemView()) ->setIcon($icon) ->setHref('/search/delete/'.$key.'/'.$class.'/') + ->setRenderNameAsTooltip(true) + ->setName($disable_name) ->setWorkflow(true)); - if ($named_query->getIsBuiltin()) { - if ($named_query->getIsDisabled()) { - $item->addIcon('fa-times lightgreytext', pht('Disabled')); - $item->setDisabled(true); - } else { - $item->addIcon('fa-lock lightgreytext', pht('Builtin')); - } + $default_disabled = $named_query->getIsDisabled(); + $default_icon = 'fa-thumb-tack'; + + if ($default_key === $key) { + $default_color = 'green'; } else { - $item->addAction( - id(new PHUIListItemView()) - ->setIcon('fa-pencil') - ->setHref('/search/edit/'.$key.'/')); + $default_color = null; + } + + $item->addAction( + id(new PHUIListItemView()) + ->setIcon("{$default_icon} {$default_color}") + ->setHref('/search/default/'.$key.'/'.$class.'/') + ->setRenderNameAsTooltip(true) + ->setName(pht('Make Default')) + ->setWorkflow(true) + ->setDisabled($default_disabled)); + + if ($named_query->getIsBuiltin()) { + $edit_icon = 'fa-lock lightgreytext'; + $edit_disabled = true; + $edit_name = pht('Builtin'); + $edit_href = null; + } else { + $edit_icon = 'fa-pencil'; + $edit_disabled = false; + $edit_name = pht('Edit'); + $edit_href = '/search/edit/'.$key.'/'; + } + + $item->addAction( + id(new PHUIListItemView()) + ->setIcon($edit_icon) + ->setHref($edit_href) + ->setRenderNameAsTooltip(true) + ->setName($edit_name) + ->setDisabled($edit_disabled)); + + if ($named_query->getIsDisabled()) { + $item->setDisabled(true); } $item->setGrippable(true); @@ -610,7 +648,7 @@ final class PhabricatorApplicationSearchController $engine_class = get_class($engine); $query_key = $this->getQueryKey(); if (!$query_key) { - $query_key = head_key($engine->loadEnabledNamedQueries()); + $query_key = $engine->getDefaultQueryKey(); } $can_use = $engine->canUseInPanelContext(); diff --git a/src/applications/search/controller/PhabricatorSearchDefaultController.php b/src/applications/search/controller/PhabricatorSearchDefaultController.php new file mode 100644 index 0000000000..707f487f20 --- /dev/null +++ b/src/applications/search/controller/PhabricatorSearchDefaultController.php @@ -0,0 +1,81 @@ +getViewer(); + $engine_class = $request->getURIData('engine'); + + $base_class = 'PhabricatorApplicationSearchEngine'; + if (!is_subclass_of($engine_class, $base_class)) { + return new Aphront400Response(); + } + + $engine = newv($engine_class, array()); + $engine->setViewer($viewer); + + $key = $request->getURIData('queryKey'); + + $named_query = id(new PhabricatorNamedQueryQuery()) + ->setViewer($viewer) + ->withEngineClassNames(array($engine_class)) + ->withQueryKeys(array($key)) + ->withUserPHIDs(array($viewer->getPHID())) + ->executeOne(); + + if (!$named_query && $engine->isBuiltinQuery($key)) { + $named_query = $engine->getBuiltinQuery($key); + } + + if (!$named_query) { + return new Aphront404Response(); + } + + $return_uri = $engine->getQueryManagementURI(); + + $builtin = null; + if ($engine->isBuiltinQuery($key)) { + $builtin = $engine->getBuiltinQuery($key); + } + + if ($request->isFormPost()) { + $config = id(new PhabricatorNamedQueryConfigQuery()) + ->setViewer($viewer) + ->withEngineClassNames(array($engine_class)) + ->withScopePHIDs(array($viewer->getPHID())) + ->executeOne(); + if (!$config) { + $config = PhabricatorNamedQueryConfig::initializeNewQueryConfig() + ->setEngineClassName($engine_class) + ->setScopePHID($viewer->getPHID()); + } + + $config->setConfigProperty( + PhabricatorNamedQueryConfig::PROPERTY_PINNED, + $key); + + $config->save(); + + return id(new AphrontRedirectResponse())->setURI($return_uri); + } + + if ($named_query->getIsBuiltin()) { + $query_name = $builtin->getQueryName(); + } else { + $query_name = $named_query->getQueryName(); + } + + $title = pht('Set Default Query'); + $body = pht( + 'This query will become your default query in the current application.'); + $button = pht('Set Default Query'); + + return $this->newDialog() + ->setTitle($title) + ->appendChild($body) + ->addCancelButton($return_uri) + ->addSubmitButton($button); + } + +} diff --git a/src/applications/search/controller/PhabricatorSearchEditController.php b/src/applications/search/controller/PhabricatorSearchEditController.php index c7f6c860e9..8b76ea82d4 100644 --- a/src/applications/search/controller/PhabricatorSearchEditController.php +++ b/src/applications/search/controller/PhabricatorSearchEditController.php @@ -10,7 +10,6 @@ final class PhabricatorSearchEditController ->setViewer($viewer) ->withQueryKeys(array($request->getURIData('queryKey'))) ->executeOne(); - if (!$saved_query) { return new Aphront404Response(); } diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index 5b2f7827f9..0f17a1737c 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -511,6 +511,34 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { return $named_queries; } + public function getDefaultQueryKey() { + $viewer = $this->requireViewer(); + + $configs = id(new PhabricatorNamedQueryConfigQuery()) + ->setViewer($viewer) + ->withEngineClassNames(array(get_class($this))) + ->withScopePHIDs( + array( + $viewer->getPHID(), + PhabricatorNamedQueryConfig::SCOPE_GLOBAL, + )) + ->execute(); + $configs = msortv($configs, 'getStrengthSortVector'); + + $key_pinned = PhabricatorNamedQueryConfig::PROPERTY_PINNED; + $map = $this->loadEnabledNamedQueries(); + foreach ($configs as $config) { + $pinned = $config->getConfigProperty($key_pinned); + if (!isset($map[$pinned])) { + continue; + } + + return $pinned; + } + + return head_key($map); + } + protected function setQueryProjects( PhabricatorCursorPagedPolicyAwareQuery $query, PhabricatorSavedQuery $saved) { diff --git a/src/applications/search/query/PhabricatorNamedQueryConfigQuery.php b/src/applications/search/query/PhabricatorNamedQueryConfigQuery.php new file mode 100644 index 0000000000..6c4fad31a3 --- /dev/null +++ b/src/applications/search/query/PhabricatorNamedQueryConfigQuery.php @@ -0,0 +1,64 @@ +ids = $ids; + return $this; + } + + public function withScopePHIDs(array $scope_phids) { + $this->scopePHIDs = $scope_phids; + return $this; + } + + public function withEngineClassNames(array $engine_class_names) { + $this->engineClassNames = $engine_class_names; + return $this; + } + + public function newResultObject() { + return new PhabricatorNamedQueryConfig(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->engineClassNames !== null) { + $where[] = qsprintf( + $conn, + 'engineClassName IN (%Ls)', + $this->engineClassNames); + } + + if ($this->scopePHIDs !== null) { + $where[] = qsprintf( + $conn, + 'scopePHID IN (%Ls)', + $this->scopePHIDs); + } + + return $where; + } + + public function getQueryApplicationClass() { + return 'PhabricatorSearchApplication'; + } + +} diff --git a/src/applications/search/storage/PhabricatorNamedQueryConfig.php b/src/applications/search/storage/PhabricatorNamedQueryConfig.php new file mode 100644 index 0000000000..d5cdfe88d0 --- /dev/null +++ b/src/applications/search/storage/PhabricatorNamedQueryConfig.php @@ -0,0 +1,92 @@ + array( + 'properties' => self::SERIALIZATION_JSON, + ), + self::CONFIG_COLUMN_SCHEMA => array( + 'engineClassName' => 'text128', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_scope' => array( + 'columns' => array('engineClassName', 'scopePHID'), + 'unique' => true, + ), + ), + ) + parent::getConfiguration(); + } + + public static function initializeNewQueryConfig() { + return new self(); + } + + public function isGlobal() { + return ($this->getScopePHID() == self::SCOPE_GLOBAL); + } + + public function getConfigProperty($key, $default = null) { + return idx($this->properties, $key, $default); + } + + public function setConfigProperty($key, $value) { + $this->properties[$key] = $value; + return $this; + } + + public function getStrengthSortVector() { + // Apply personal preferences before global preferences. + if (!$this->isGlobal()) { + $phase = 0; + } else { + $phase = 1; + } + + return id(new PhutilSortVector()) + ->addInt($phase) + ->addInt($this->getID()); + } + + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + return PhabricatorPolicies::POLICY_NOONE; + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + if ($this->isGlobal()) { + return true; + } + + if ($viewer->getPHID() == $this->getScopePHID()) { + return true; + } + + return false; + } + + public function describeAutomaticCapability($capability) { + return null; + } + +} From 47da632a22ead5e29a86ca0e26b1e07085c68b88 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 14 Aug 2017 12:19:06 -0700 Subject: [PATCH 14/27] Separate saved queries in applications into "personal" and "global" queries Summary: Ref T12956. UI changes: - Administrators get a new `[X] Save as global query` option when saving a query. - "Edit Queries..." is split into "Personal" and "Global" sections. For administrators, each section can be edited. For non-admins, only the top section can be edited, but any query can be pinned. A couple notes: - This doesn't support "pin for everyone by default". New users just get the first query from the bottom set. That seems reasonable for now. - Reordering is currently a little buggy (it works if you've reordered before, but not if you're reordering for the first time), but I need to migrate before I can fix / test that properly. So that'll get cleaned up in the next change or two. Test Plan: - As an admin and non-admin, viewed, edited, disabled, saved-as-personal and saved-as-global various queries. {F5098581} {F5098582} Reviewers: chad Reviewed By: chad Maniphest Tasks: T12956 Differential Revision: https://secure.phabricator.com/D18426 --- .../PhabricatorSearchApplication.php | 13 +- ...PhabricatorApplicationSearchController.php | 213 ++++++++++++------ .../PhabricatorSearchDefaultController.php | 6 +- .../PhabricatorSearchDeleteController.php | 51 +++-- .../PhabricatorSearchEditController.php | 56 ++++- .../PhabricatorApplicationSearchEngine.php | 10 +- .../search/storage/PhabricatorNamedQuery.php | 40 +++- 7 files changed, 279 insertions(+), 110 deletions(-) diff --git a/src/applications/search/application/PhabricatorSearchApplication.php b/src/applications/search/application/PhabricatorSearchApplication.php index 22f4228ba8..3cf5923b9c 100644 --- a/src/applications/search/application/PhabricatorSearchApplication.php +++ b/src/applications/search/application/PhabricatorSearchApplication.php @@ -33,11 +33,18 @@ final class PhabricatorSearchApplication extends PhabricatorApplication { 'index/(?P[^/]+)/' => 'PhabricatorSearchIndexController', 'hovercard/' => 'PhabricatorSearchHovercardController', - 'edit/(?P[^/]+)/' => 'PhabricatorSearchEditController', + 'edit/' => array( + 'key/(?P[^/]+)/' => 'PhabricatorSearchEditController', + 'id/(?P[^/]+)/' => 'PhabricatorSearchEditController', + ), 'default/(?P[^/]+)/(?P[^/]+)/' => 'PhabricatorSearchDefaultController', - 'delete/(?P[^/]+)/(?P[^/]+)/' - => 'PhabricatorSearchDeleteController', + 'delete/' => array( + 'key/(?P[^/]+)/(?P[^/]+)/' + => 'PhabricatorSearchDeleteController', + 'id/(?P[^/]+)/' + => 'PhabricatorSearchDeleteController', + ), 'order/(?P[^/]+)/' => 'PhabricatorSearchOrderController', 'rel/(?P[^/]+)/(?P[^/]+)/' => 'PhabricatorSearchRelationshipController', diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index 0e1dfdb699..f781e20380 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -174,7 +174,7 @@ final class PhabricatorApplicationSearchController if ($run_query && !$named_query && $user->isLoggedIn()) { $save_button = id(new PHUIButtonView()) ->setTag('a') - ->setHref('/search/edit/'.$saved_query->getQueryKey().'/') + ->setHref('/search/edit/key/'.$saved_query->getQueryKey().'/') ->setText(pht('Save Query')) ->setIcon('fa-floppy-o'); $submit->addButton($save_button); @@ -377,7 +377,7 @@ final class PhabricatorApplicationSearchController private function processEditRequest() { $parent = $this->getDelegatingController(); $request = $this->getRequest(); - $user = $request->getUser(); + $viewer = $request->getUser(); $engine = $this->getSearchEngine(); $nav = $this->getNavigation(); @@ -387,21 +387,89 @@ final class PhabricatorApplicationSearchController $named_queries = $engine->loadAllNamedQueries(); - $list_id = celerity_generate_unique_node_id(); + $can_global = $viewer->getIsAdmin(); - $list = new PHUIObjectItemListView(); - $list->setUser($user); - $list->setID($list_id); + $groups = array( + 'personal' => array( + 'name' => pht('Personal Saved Queries'), + 'items' => array(), + 'edit' => true, + ), + 'global' => array( + 'name' => pht('Global Saved Queries'), + 'items' => array(), + 'edit' => $can_global, + ), + ); - Javelin::initBehavior( - 'search-reorder-queries', - array( - 'listID' => $list_id, - 'orderURI' => '/search/order/'.get_class($engine).'/', - )); + foreach ($named_queries as $named_query) { + if ($named_query->isGlobal()) { + $group = 'global'; + } else { + $group = 'personal'; + } + + $groups[$group]['items'][] = $named_query; + } $default_key = $engine->getDefaultQueryKey(); + $lists = array(); + foreach ($groups as $group) { + $lists[] = $this->newQueryListView( + $group['name'], + $group['items'], + $default_key, + $group['edit']); + } + + $crumbs = $parent + ->buildApplicationCrumbs() + ->addTextCrumb(pht('Saved Queries'), $engine->getQueryManagementURI()) + ->setBorder(true); + + $nav->selectFilter('query/edit'); + + $header = id(new PHUIHeaderView()) + ->setHeader(pht('Saved Queries')) + ->setProfileHeader(true); + + $view = id(new PHUITwoColumnView()) + ->setHeader($header) + ->setFooter($lists); + + return $this->newPage() + ->setApplicationMenu($this->buildApplicationMenu()) + ->setTitle(pht('Saved Queries')) + ->setCrumbs($crumbs) + ->setNavigation($nav) + ->appendChild($view); + } + + private function newQueryListView( + $list_name, + array $named_queries, + $default_key, + $can_edit) { + + $engine = $this->getSearchEngine(); + $viewer = $this->getViewer(); + + $list = id(new PHUIObjectItemListView()) + ->setViewer($viewer); + + if ($can_edit) { + $list_id = celerity_generate_unique_node_id(); + $list->setID($list_id); + + Javelin::initBehavior( + 'search-reorder-queries', + array( + 'listID' => $list_id, + 'orderURI' => '/search/order/'.get_class($engine).'/', + )); + } + foreach ($named_queries as $named_query) { $class = get_class($engine); $key = $named_query->getQueryKey(); @@ -410,25 +478,43 @@ final class PhabricatorApplicationSearchController ->setHeader($named_query->getQueryName()) ->setHref($engine->getQueryResultsPageURI($key)); - if ($named_query->getIsBuiltin() && $named_query->getIsDisabled()) { - $icon = 'fa-plus'; - $disable_name = pht('Enable'); - } else { - $icon = 'fa-times'; - if ($named_query->getIsBuiltin()) { - $disable_name = pht('Disable'); + if ($named_query->getIsDisabled()) { + if ($can_edit) { + $item->setDisabled(true); } else { - $disable_name = pht('Delete'); + // If an item is disabled and you don't have permission to edit it, + // just skip it. + continue; } } - $item->addAction( - id(new PHUIListItemView()) - ->setIcon($icon) - ->setHref('/search/delete/'.$key.'/'.$class.'/') - ->setRenderNameAsTooltip(true) - ->setName($disable_name) - ->setWorkflow(true)); + if ($can_edit) { + if ($named_query->getIsBuiltin() && $named_query->getIsDisabled()) { + $icon = 'fa-plus'; + $disable_name = pht('Enable'); + } else { + $icon = 'fa-times'; + if ($named_query->getIsBuiltin()) { + $disable_name = pht('Disable'); + } else { + $disable_name = pht('Delete'); + } + } + + if ($named_query->getID()) { + $disable_href = '/search/delete/id/'.$named_query->getID().'/'; + } else { + $disable_href = '/search/delete/key/'.$key.'/'.$class.'/'; + } + + $item->addAction( + id(new PHUIListItemView()) + ->setIcon($icon) + ->setHref($disable_href) + ->setRenderNameAsTooltip(true) + ->setName($disable_name) + ->setWorkflow(true)); + } $default_disabled = $named_query->getIsDisabled(); $default_icon = 'fa-thumb-tack'; @@ -448,31 +534,29 @@ final class PhabricatorApplicationSearchController ->setWorkflow(true) ->setDisabled($default_disabled)); - if ($named_query->getIsBuiltin()) { - $edit_icon = 'fa-lock lightgreytext'; - $edit_disabled = true; - $edit_name = pht('Builtin'); - $edit_href = null; - } else { - $edit_icon = 'fa-pencil'; - $edit_disabled = false; - $edit_name = pht('Edit'); - $edit_href = '/search/edit/'.$key.'/'; + if ($can_edit) { + if ($named_query->getIsBuiltin()) { + $edit_icon = 'fa-lock lightgreytext'; + $edit_disabled = true; + $edit_name = pht('Builtin'); + $edit_href = null; + } else { + $edit_icon = 'fa-pencil'; + $edit_disabled = false; + $edit_name = pht('Edit'); + $edit_href = '/search/edit/id/'.$named_query->getID().'/'; + } + + $item->addAction( + id(new PHUIListItemView()) + ->setIcon($edit_icon) + ->setHref($edit_href) + ->setRenderNameAsTooltip(true) + ->setName($edit_name) + ->setDisabled($edit_disabled)); } - $item->addAction( - id(new PHUIListItemView()) - ->setIcon($edit_icon) - ->setHref($edit_href) - ->setRenderNameAsTooltip(true) - ->setName($edit_name) - ->setDisabled($edit_disabled)); - - if ($named_query->getIsDisabled()) { - $item->setDisabled(true); - } - - $item->setGrippable(true); + $item->setGrippable($can_edit); $item->addSigil('named-query'); $item->setMetadata( array( @@ -484,31 +568,10 @@ final class PhabricatorApplicationSearchController $list->setNoDataString(pht('No saved queries.')); - $crumbs = $parent - ->buildApplicationCrumbs() - ->addTextCrumb(pht('Saved Queries'), $engine->getQueryManagementURI()) - ->setBorder(true); - - $nav->selectFilter('query/edit'); - - $header = id(new PHUIHeaderView()) - ->setHeader(pht('Saved Queries')) - ->setProfileHeader(true); - - $box = id(new PHUIObjectBoxView()) - ->setHeader($header) - ->setObjectList($list) - ->addClass('application-search-results'); - - $nav->addClass('application-search-view'); - require_celerity_resource('application-search-view-css'); - - return $this->newPage() - ->setApplicationMenu($this->buildApplicationMenu()) - ->setTitle(pht('Saved Queries')) - ->setCrumbs($crumbs) - ->setNavigation($nav) - ->appendChild($box); + return id(new PHUIObjectBoxView()) + ->setHeaderText($list_name) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setObjectList($list); } public function buildApplicationMenu() { diff --git a/src/applications/search/controller/PhabricatorSearchDefaultController.php b/src/applications/search/controller/PhabricatorSearchDefaultController.php index 707f487f20..a4f68e503a 100644 --- a/src/applications/search/controller/PhabricatorSearchDefaultController.php +++ b/src/applications/search/controller/PhabricatorSearchDefaultController.php @@ -21,7 +21,11 @@ final class PhabricatorSearchDefaultController ->setViewer($viewer) ->withEngineClassNames(array($engine_class)) ->withQueryKeys(array($key)) - ->withUserPHIDs(array($viewer->getPHID())) + ->withUserPHIDs( + array( + $viewer->getPHID(), + PhabricatorNamedQuery::SCOPE_GLOBAL, + )) ->executeOne(); if (!$named_query && $engine->isBuiltinQuery($key)) { diff --git a/src/applications/search/controller/PhabricatorSearchDeleteController.php b/src/applications/search/controller/PhabricatorSearchDeleteController.php index f72c283519..9cbabd3a2f 100644 --- a/src/applications/search/controller/PhabricatorSearchDeleteController.php +++ b/src/applications/search/controller/PhabricatorSearchDeleteController.php @@ -5,32 +5,45 @@ final class PhabricatorSearchDeleteController public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); - $key = $request->getURIData('queryKey'); - $engine_class = $request->getURIData('engine'); - $base_class = 'PhabricatorApplicationSearchEngine'; - if (!is_subclass_of($engine_class, $base_class)) { - return new Aphront400Response(); - } + $id = $request->getURIData('id'); + if ($id) { + $named_query = id(new PhabricatorNamedQueryQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$named_query) { + return new Aphront404Response(); + } - $engine = newv($engine_class, array()); - $engine->setViewer($viewer); + $engine = newv($named_query->getEngineClassName(), array()); + $engine->setViewer($viewer); - $named_query = id(new PhabricatorNamedQueryQuery()) - ->setViewer($viewer) - ->withEngineClassNames(array($engine_class)) - ->withQueryKeys(array($key)) - ->withUserPHIDs(array($viewer->getPHID())) - ->executeOne(); + $key = $named_query->getQueryKey(); + } else { + $key = $request->getURIData('queryKey'); + $engine_class = $request->getURIData('engine'); + + $base_class = 'PhabricatorApplicationSearchEngine'; + if (!is_subclass_of($engine_class, $base_class)) { + return new Aphront400Response(); + } + + $engine = newv($engine_class, array()); + $engine->setViewer($viewer); + + if (!$engine->isBuiltinQuery($key)) { + return new Aphront404Response(); + } - if (!$named_query && $engine->isBuiltinQuery($key)) { $named_query = $engine->getBuiltinQuery($key); } - if (!$named_query) { - return new Aphront404Response(); - } - $builtin = null; if ($engine->isBuiltinQuery($key)) { $builtin = $engine->getBuiltinQuery($key); diff --git a/src/applications/search/controller/PhabricatorSearchEditController.php b/src/applications/search/controller/PhabricatorSearchEditController.php index 8b76ea82d4..a526091503 100644 --- a/src/applications/search/controller/PhabricatorSearchEditController.php +++ b/src/applications/search/controller/PhabricatorSearchEditController.php @@ -6,9 +6,30 @@ final class PhabricatorSearchEditController public function handleRequest(AphrontRequest $request) { $viewer = $this->getViewer(); + $id = $request->getURIData('id'); + if ($id) { + $named_query = id(new PhabricatorNamedQueryQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$named_query) { + return new Aphront404Response(); + } + + $query_key = $named_query->getQueryKey(); + } else { + $query_key = $request->getURIData('queryKey'); + $named_query = null; + } + $saved_query = id(new PhabricatorSavedQueryQuery()) ->setViewer($viewer) - ->withQueryKeys(array($request->getURIData('queryKey'))) + ->withQueryKeys(array($query_key)) ->executeOne(); if (!$saved_query) { return new Aphront404Response(); @@ -19,11 +40,6 @@ final class PhabricatorSearchEditController $complete_uri = $engine->getQueryManagementURI(); $cancel_uri = $complete_uri; - $named_query = id(new PhabricatorNamedQueryQuery()) - ->setViewer($viewer) - ->withQueryKeys(array($saved_query->getQueryKey())) - ->withUserPHIDs(array($viewer->getPHID())) - ->executeOne(); if (!$named_query) { $named_query = id(new PhabricatorNamedQuery()) ->setUserPHID($viewer->getPHID()) @@ -35,12 +51,27 @@ final class PhabricatorSearchEditController // management interface. $cancel_uri = $engine->getQueryResultsPageURI( $saved_query->getQueryKey()); + + $is_new = true; + } else { + $is_new = false; } + $can_global = ($viewer->getIsAdmin() && $is_new); + + $v_global = false; + $e_name = true; $errors = array(); if ($request->isFormPost()) { + if ($can_global) { + $v_global = $request->getBool('global'); + if ($v_global) { + $named_query->setUserPHID(PhabricatorNamedQuery::SCOPE_GLOBAL); + } + } + $named_query->setQueryName($request->getStr('name')); if (!strlen($named_query->getQueryName())) { $e_name = pht('Required'); @@ -50,6 +81,7 @@ final class PhabricatorSearchEditController } if (!$errors) { + $named_query->save(); return id(new AphrontRedirectResponse())->setURI($complete_uri); } @@ -65,6 +97,18 @@ final class PhabricatorSearchEditController ->setValue($named_query->getQueryName()) ->setError($e_name)); + if ($can_global) { + $form->appendChild( + id(new AphrontFormCheckboxControl()) + ->addCheckbox( + 'global', + '1', + pht( + 'Save this query as a global query, making it visible to '. + 'all users.'), + $v_global)); + } + $form->appendChild( id(new AphrontFormSubmitControl()) ->setValue(pht('Save Query')) diff --git a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php index 0f17a1737c..6122e66e23 100644 --- a/src/applications/search/engine/PhabricatorApplicationSearchEngine.php +++ b/src/applications/search/engine/PhabricatorApplicationSearchEngine.php @@ -474,8 +474,12 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { if ($this->namedQueries === null) { $named_queries = id(new PhabricatorNamedQueryQuery()) ->setViewer($viewer) - ->withUserPHIDs(array($viewer->getPHID())) ->withEngineClassNames(array(get_class($this))) + ->withUserPHIDs( + array( + $viewer->getPHID(), + PhabricatorNamedQuery::SCOPE_GLOBAL, + )) ->execute(); $named_queries = mpull($named_queries, null, 'getQueryKey'); @@ -494,7 +498,7 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { unset($builtin[$key]); } - $named_queries = msort($named_queries, 'getSortKey'); + $named_queries = msortv($named_queries, 'getNamedQuerySortVector'); $this->namedQueries = $named_queries; } @@ -631,7 +635,7 @@ abstract class PhabricatorApplicationSearchEngine extends Phobject { $sequence = 0; foreach ($names as $key => $name) { $queries[$key] = id(new PhabricatorNamedQuery()) - ->setUserPHID($this->requireViewer()->getPHID()) + ->setUserPHID(PhabricatorNamedQuery::SCOPE_GLOBAL) ->setEngineClassName(get_class($this)) ->setQueryName($name) ->setQueryKey($key) diff --git a/src/applications/search/storage/PhabricatorNamedQuery.php b/src/applications/search/storage/PhabricatorNamedQuery.php index ac34a4fa32..44d7a403b1 100644 --- a/src/applications/search/storage/PhabricatorNamedQuery.php +++ b/src/applications/search/storage/PhabricatorNamedQuery.php @@ -12,6 +12,8 @@ final class PhabricatorNamedQuery extends PhabricatorSearchDAO protected $isDisabled = 0; protected $sequence = 0; + const SCOPE_GLOBAL = 'scope.global'; + protected function getConfiguration() { return array( self::CONFIG_COLUMN_SCHEMA => array( @@ -31,8 +33,29 @@ final class PhabricatorNamedQuery extends PhabricatorSearchDAO ) + parent::getConfiguration(); } - public function getSortKey() { - return sprintf('~%010d%010d', $this->sequence, $this->getID()); + public function isGlobal() { + if ($this->getIsBuiltin()) { + return true; + } + + if ($this->getUserPHID() === self::SCOPE_GLOBAL) { + return true; + } + + return false; + } + + public function getNamedQuerySortVector() { + if (!$this->isGlobal()) { + $phase = 0; + } else { + $phase = 1; + } + + return id(new PhutilSortVector()) + ->addInt($phase) + ->addInt($this->sequence) + ->addInt($this->getID()); } /* -( PhabricatorPolicyInterface )----------------------------------------- */ @@ -41,6 +64,7 @@ final class PhabricatorNamedQuery extends PhabricatorSearchDAO public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, ); } @@ -49,9 +73,19 @@ final class PhabricatorNamedQuery extends PhabricatorSearchDAO } public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { - if ($viewer->getPHID() == $this->userPHID) { + if ($viewer->getPHID() == $this->getUserPHID()) { return true; } + + if ($this->isGlobal()) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + return true; + case PhabricatorPolicyCapability::CAN_EDIT: + return $viewer->getIsAdmin(); + } + } + return false; } From bc0963d54b1c92b97090f855f1931e7a5f1b7d0f Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Aug 2017 12:49:47 -0700 Subject: [PATCH 15/27] Remove rows for personal saved builtin queries Summary: Ref T12956. After this change, individual users will no longer be able to modify builtin queries on a user-by-user basis: they will always appear at the bottom of the list, under their personal queries, and can only be managed by administrators. To support this, clean up the old rows which could be hanging around from before: delete any personal saved queries where the saved query is a builtin query. To ease this transition, try to pin the query we're deleting //if// the user had reordered things to put it on top. Test Plan: - Ran the migration, saw no changes in the UI but fewer rows. - Went back to `master`, reordered queries to put a builtin one on top. - Ran the migration. - Saw that builtin one drop to the bottom (since it can't be on top anymore) but be pinned, preserving the behavior of `/maniphest/`. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12956 Differential Revision: https://secure.phabricator.com/D18464 --- .../autopatches/20170824.search.01.saved.php | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 resources/sql/autopatches/20170824.search.01.saved.php diff --git a/resources/sql/autopatches/20170824.search.01.saved.php b/resources/sql/autopatches/20170824.search.01.saved.php new file mode 100644 index 0000000000..ab1485ebd5 --- /dev/null +++ b/resources/sql/autopatches/20170824.search.01.saved.php @@ -0,0 +1,46 @@ +establishConnection('w'); + +$config_table = new PhabricatorNamedQueryConfig(); + +foreach (new LiskMigrationIterator($table) as $named_query) { + + // If this isn't a builtin query, it isn't changing. Leave it alone. + if (!$named_query->getIsBuiltin()) { + continue; + } + + // If the user reordered things but left a builtin query at the top, pin + // the query before we remove the row. + if ($named_query->getSequence() == 1) { + queryfx( + $conn, + 'INSERT IGNORE INTO %T + (engineClassName, scopePHID, properties, dateCreated, dateModified) + VALUES + (%s, %s, %s, %d, %d)', + $config_table->getTableName(), + $named_query->getEngineClassName(), + $named_query->getUserPHID(), + phutil_json_encode( + array( + PhabricatorNamedQueryConfig::PROPERTY_PINNED => + $named_query->getQueryKey(), + )), + PhabricatorTime::getNow(), + PhabricatorTime::getNow()); + } + + $named_query->delete(); +} From ba1925b155152909e1c164817b2e47ca55c91b7c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Aug 2017 13:38:52 -0700 Subject: [PATCH 16/27] Prevent Differential changeset HTML anchors from colliding with comment anchors Summary: Fixes T12970. This is easier than I expected, and appears to occur in only one place. This prevents a change from ever generating with an anchor like `#12345678`, which is ambiguous because it may be a comment anchor. Test Plan: Viewed a revision, saw new `change-xxxyyyzzz` anchors, clicked one, got jumped to the right place. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12970 Differential Revision: https://secure.phabricator.com/D18465 --- src/applications/differential/storage/DifferentialChangeset.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index be83c5e73b..9c718a8765 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -170,7 +170,7 @@ final class DifferentialChangeset extends DifferentialDAO } public function getAnchorName() { - return substr(md5($this->getFilename()), 0, 8); + return 'change-'.PhabricatorHash::digestForIndex($this->getFilename()); } public function getAbsoluteRepositoryPath( From 2722c167d8c9432c15823f6c5dc96722422a4a0c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Aug 2017 14:14:47 -0700 Subject: [PATCH 17/27] Add the skeleton for a "transaction.search" Conduit API method Summary: Ref T5873. See PHI14. This does the basics that are shared across everything (IDs, PHIDs, dates, comments). It doesn't do types (I think I don't necessarily want to expose internal types over the API?) or transaction-specific data. In the next change, I'm going to add ways to let ModularTransactions "opt-in" to providing more data to Conduit. I'll use this to flesh out the actual desired transaction types (comments, presumably inline comments) and likely leave the rest as skeletons for now until use cases arise so we don't create a backward compatibility issue (or a security issue!) by exposing tons of internal stuff as public-facing API. Test Plan: Ran queries, used paging. Retrieved an edited, deleted, and normal comment. {F5120060} Reviewers: chad Reviewed By: chad Maniphest Tasks: T5873 Differential Revision: https://secure.phabricator.com/D18466 --- src/__phutil_library_map__.php | 2 + .../TransactionSearchConduitAPIMethod.php | 152 ++++++++++++++++++ 2 files changed, 154 insertions(+) create mode 100644 src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b15e4f7ff1..f732fdb33b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4884,6 +4884,7 @@ phutil_register_library_map(array( 'TokenGiveConduitAPIMethod' => 'applications/tokens/conduit/TokenGiveConduitAPIMethod.php', 'TokenGivenConduitAPIMethod' => 'applications/tokens/conduit/TokenGivenConduitAPIMethod.php', 'TokenQueryConduitAPIMethod' => 'applications/tokens/conduit/TokenQueryConduitAPIMethod.php', + 'TransactionSearchConduitAPIMethod' => 'applications/transactions/conduit/TransactionSearchConduitAPIMethod.php', 'UserConduitAPIMethod' => 'applications/people/conduit/UserConduitAPIMethod.php', 'UserDisableConduitAPIMethod' => 'applications/people/conduit/UserDisableConduitAPIMethod.php', 'UserEnableConduitAPIMethod' => 'applications/people/conduit/UserEnableConduitAPIMethod.php', @@ -10631,6 +10632,7 @@ phutil_register_library_map(array( 'TokenGiveConduitAPIMethod' => 'TokenConduitAPIMethod', 'TokenGivenConduitAPIMethod' => 'TokenConduitAPIMethod', 'TokenQueryConduitAPIMethod' => 'TokenConduitAPIMethod', + 'TransactionSearchConduitAPIMethod' => 'ConduitAPIMethod', 'UserConduitAPIMethod' => 'ConduitAPIMethod', 'UserDisableConduitAPIMethod' => 'UserConduitAPIMethod', 'UserEnableConduitAPIMethod' => 'UserConduitAPIMethod', diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php new file mode 100644 index 0000000000..0f655199fa --- /dev/null +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -0,0 +1,152 @@ + 'phid|string', + ) + $this->getPagerParamTypes(); + } + + protected function defineReturnType() { + return 'list'; + } + + protected function defineErrorTypes() { + return array(); + } + + protected function execute(ConduitAPIRequest $request) { + $viewer = $request->getUser(); + $pager = $this->newPager($request); + + $object_name = $request->getValue('objectIdentifier', null); + if (!strlen($object_name)) { + throw new Exception( + pht( + 'When calling "transaction.search", you must provide an object to '. + 'retrieve transactions for.')); + } + + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withNames(array($object_name)) + ->executeOne(); + if (!$object) { + throw new Exception( + pht( + 'No object "%s" exists.', + $object_name)); + } + + if (!($object instanceof PhabricatorApplicationTransactionInterface)) { + throw new Exception( + pht( + 'Object "%s" does not implement "%s", so transactions can not '. + 'be loaded for it.')); + } + + $xaction_query = PhabricatorApplicationTransactionQuery::newQueryForObject( + $object); + + $xactions = $xaction_query + ->withObjectPHIDs(array($object->getPHID())) + ->setViewer($viewer) + ->executeWithCursorPager($pager); + + if ($xactions) { + $template = head($xactions)->getApplicationTransactionCommentObject(); + + $query = new PhabricatorApplicationTransactionTemplatedCommentQuery(); + + $comment_map = $query + ->setViewer($viewer) + ->setTemplate($template) + ->withTransactionPHIDs(mpull($xactions, 'getPHID')) + ->execute(); + + $comment_map = msort($comment_map, 'getCommentVersion'); + $comment_map = array_reverse($comment_map); + $comment_map = mgroup($comment_map, 'getTransactionPHID'); + } else { + $comment_map = array(); + } + + $data = array(); + foreach ($xactions as $xaction) { + $comments = idx($comment_map, $xaction->getPHID()); + + $comment_data = array(); + if ($comments) { + $removed = head($comments)->getIsDeleted(); + + foreach ($comments as $comment) { + if ($removed) { + // If the most recent version of the comment has been removed, + // don't show the history. This is for consistency with the web + // UI, which also prevents users from retrieving the content of + // removed comments. + $content = array( + 'raw' => '', + ); + } else { + $content = array( + 'raw' => (string)$comment->getContent(), + ); + } + + $comment_data[] = array( + 'id' => (int)$comment->getID(), + 'phid' => (string)$comment->getPHID(), + 'version' => (int)$comment->getCommentVersion(), + 'authorPHID' => (string)$comment->getAuthorPHID(), + 'dateCreated' => (int)$comment->getDateCreated(), + 'dateModified' => (int)$comment->getDateModified(), + 'removed' => (bool)$comment->getIsDeleted(), + 'content' => $content, + ); + } + } + + $fields = array(); + + if (!$fields) { + $fields = (object)$fields; + } + + $data[] = array( + 'id' => (int)$xaction->getID(), + 'phid' => (string)$xaction->getPHID(), + 'authorPHID' => (string)$xaction->getAuthorPHID(), + 'objectPHID' => (string)$xaction->getObjectPHID(), + 'dateCreated' => (int)$xaction->getDateCreated(), + 'dateModified' => (int)$xaction->getDateModified(), + 'comments' => $comment_data, + 'fields' => $fields, + ); + } + + $results = array( + 'data' => $data, + ); + + return $this->addPagerResults($results, $pager); + } +} From 6c9026c33a45aaec7cab5d805a116e4c3e71cc78 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Aug 2017 14:40:11 -0700 Subject: [PATCH 18/27] Allow ModularTransactions to opt in to providing data to Conduit Summary: Ref T5873. See PHI14. I don't want to just expose internal transaction data to Conduit by default, since it's often: unstable, unusable, sensitive, or some combination of the three. Instead, let ModularTransactions opt in to providing additional data to Conduit, similar to other infrastructure. If a transaction doesn't, the API returns an empty skeleton for it. This is generally fine since most transactions have no real use cases, and I think we can fill them in as we go. This also probably builds toward T5726, which would likely use the same format, and perhaps simply not publish stuff which did not opt in. This doesn't actually cover "comment" or "inline comment", which are presumably what PHI14 is after, since neither is modular. I'll probably just put a hack in place for this until they can modularize since I suspect modularizing them here is difficult. Test Plan: Ran `transaction.search` on a revision, saw some transactions (title and status transactions) populate with values. Reviewers: chad Reviewed By: chad Maniphest Tasks: T5873 Differential Revision: https://secure.phabricator.com/D18467 --- .../DifferentialRevisionStatusTransaction.php | 11 +++++ .../DifferentialRevisionTitleTransaction.php | 11 +++++ .../TransactionSearchConduitAPIMethod.php | 43 +++++++++++++++++++ .../PhabricatorModularTransactionType.php | 12 ++++++ 4 files changed, 77 insertions(+) diff --git a/src/applications/differential/xaction/DifferentialRevisionStatusTransaction.php b/src/applications/differential/xaction/DifferentialRevisionStatusTransaction.php index c08eb9d187..615ce38bcf 100644 --- a/src/applications/differential/xaction/DifferentialRevisionStatusTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionStatusTransaction.php @@ -70,4 +70,15 @@ final class DifferentialRevisionStatusTransaction return DifferentialRevisionStatus::newForStatus($new); } + public function getTransactionTypeForConduit($xaction) { + return 'status'; + } + + public function getFieldValuesForConduit($object, $data) { + return array( + 'old' => $object->getOldValue(), + 'new' => $object->getNewValue(), + ); + } + } diff --git a/src/applications/differential/xaction/DifferentialRevisionTitleTransaction.php b/src/applications/differential/xaction/DifferentialRevisionTitleTransaction.php index 9b763c53ca..812464b26d 100644 --- a/src/applications/differential/xaction/DifferentialRevisionTitleTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionTitleTransaction.php @@ -55,4 +55,15 @@ final class DifferentialRevisionTitleTransaction return $errors; } + public function getTransactionTypeForConduit($xaction) { + return 'title'; + } + + public function getFieldValuesForConduit($object, $data) { + return array( + 'old' => $object->getOldValue(), + 'new' => $object->getNewValue(), + ); + } + } diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php index 0f655199fa..b3065b0d9c 100644 --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -89,6 +89,36 @@ final class TransactionSearchConduitAPIMethod $comment_map = array(); } + $modular_classes = array(); + $modular_objects = array(); + $modular_xactions = array(); + foreach ($xactions as $xaction) { + if (!$xaction instanceof PhabricatorModularTransaction) { + continue; + } + + $modular_template = $xaction->getModularType(); + $modular_class = get_class($modular_template); + if (!isset($modular_objects[$modular_class])) { + try { + $modular_object = newv($modular_class, array()); + $modular_objects[$modular_class] = $modular_object; + } catch (Exception $ex) { + continue; + } + } + + $modular_classes[$xaction->getPHID()] = $modular_class; + $modular_xactions[$modular_class][] = $xaction; + } + + $modular_data_map = array(); + foreach ($modular_objects as $class => $modular_type) { + $modular_data_map[$class] = $modular_type + ->setViewer($viewer) + ->loadTransactionTypeConduitData($modular_xactions[$class]); + } + $data = array(); foreach ($xactions as $xaction) { $comments = idx($comment_map, $xaction->getPHID()); @@ -126,6 +156,18 @@ final class TransactionSearchConduitAPIMethod } $fields = array(); + $type = null; + + if (isset($modular_classes[$xaction->getPHID()])) { + $modular_class = $modular_classes[$xaction->getPHID()]; + $modular_object = $modular_objects[$modular_class]; + $modular_data = $modular_data_map[$modular_class]; + + $type = $modular_object->getTransactionTypeForConduit($xaction); + $fields = $modular_object->getFieldValuesForConduit( + $xaction, + $modular_data); + } if (!$fields) { $fields = (object)$fields; @@ -134,6 +176,7 @@ final class TransactionSearchConduitAPIMethod $data[] = array( 'id' => (int)$xaction->getID(), 'phid' => (string)$xaction->getPHID(), + 'type' => $type, 'authorPHID' => (string)$xaction->getAuthorPHID(), 'objectPHID' => (string)$xaction->getObjectPHID(), 'dateCreated' => (int)$xaction->getDateCreated(), diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index 128b5c7c19..119bfedd32 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -332,4 +332,16 @@ abstract class PhabricatorModularTransactionType return $this->getStorage()->getMetadataValue($key, $default); } + public function loadTransactionTypeConduitData(array $xactions) { + return null; + } + + public function getTransactionTypeForConduit($xaction) { + return null; + } + + public function getFieldValuesForConduit($xaction, $data) { + return array(); + } + } From 9639ec0dfabdf530ef9837f4bf6a4f83b4c2d919 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Aug 2017 15:15:10 -0700 Subject: [PATCH 19/27] Slightly simplify logic for determining if an inline comment has an effect Summary: Minor cleanup, this logic can be simpler. Instead of special-casing inlines as having an effect if the have a comment, just consider any transaction with a comment to have an effect. I'm fairly certain this is always true. Test Plan: Made inlines, tried to submit empty comments. Behavior unchanged. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D18468 --- .../editor/DifferentialTransactionEditor.php | 14 -------------- .../PhabricatorApplicationTransactionEditor.php | 6 ++++-- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index af246feeb0..e916728c86 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -107,20 +107,6 @@ final class DifferentialTransactionEditor return parent::getCustomTransactionNewValue($object, $xaction); } - protected function transactionHasEffect( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - $actor_phid = $this->getActingAsPHID(); - - switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_INLINE: - return $xaction->hasComment(); - } - - return parent::transactionHasEffect($object, $xaction); - } - protected function applyCustomInternalTransaction( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 07f85407fe..21e082b4ed 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -486,8 +486,6 @@ abstract class PhabricatorApplicationTransactionEditor switch ($xaction->getTransactionType()) { case PhabricatorTransactions::TYPE_CREATE: return true; - case PhabricatorTransactions::TYPE_COMMENT: - return $xaction->hasComment(); case PhabricatorTransactions::TYPE_CUSTOMFIELD: $field = $this->getCustomFieldForTransaction($object, $xaction); return $field->getApplicationTransactionHasEffect($xaction); @@ -534,6 +532,10 @@ abstract class PhabricatorApplicationTransactionEditor $xaction->getNewValue()); } + if ($xaction->hasComment()) { + return true; + } + return ($xaction->getOldValue() !== $xaction->getNewValue()); } From fa5bcf5d9478b41cb4bdaa23f22eef1aab4dfc6c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 24 Aug 2017 15:15:16 -0700 Subject: [PATCH 20/27] Provide some more detailed information about inline comments in "transaction.search" Summary: Ref T5873. This provides paths and line numbers for inline comments. This is a touch hacky but I was able to keep it mostly under control. Test Plan: - Made inline comments. - Called API, got path/line information. {F5120157} Reviewers: chad Reviewed By: chad Maniphest Tasks: T5873 Differential Revision: https://secure.phabricator.com/D18469 --- src/__phutil_library_map__.php | 2 + .../DifferentialRevisionInlineTransaction.php | 53 +++++++++++++++++++ .../TransactionSearchConduitAPIMethod.php | 23 +++++++- 3 files changed, 77 insertions(+), 1 deletion(-) create mode 100644 src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f732fdb33b..f0c9271634 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -544,6 +544,7 @@ phutil_register_library_map(array( 'DifferentialRevisionHeraldField' => 'applications/differential/herald/DifferentialRevisionHeraldField.php', 'DifferentialRevisionHeraldFieldGroup' => 'applications/differential/herald/DifferentialRevisionHeraldFieldGroup.php', 'DifferentialRevisionIDCommitMessageField' => 'applications/differential/field/DifferentialRevisionIDCommitMessageField.php', + 'DifferentialRevisionInlineTransaction' => 'applications/differential/xaction/DifferentialRevisionInlineTransaction.php', 'DifferentialRevisionInlinesController' => 'applications/differential/controller/DifferentialRevisionInlinesController.php', 'DifferentialRevisionListController' => 'applications/differential/controller/DifferentialRevisionListController.php', 'DifferentialRevisionListView' => 'applications/differential/view/DifferentialRevisionListView.php', @@ -5547,6 +5548,7 @@ phutil_register_library_map(array( 'DifferentialRevisionHeraldField' => 'HeraldField', 'DifferentialRevisionHeraldFieldGroup' => 'HeraldFieldGroup', 'DifferentialRevisionIDCommitMessageField' => 'DifferentialCommitMessageField', + 'DifferentialRevisionInlineTransaction' => 'PhabricatorModularTransactionType', 'DifferentialRevisionInlinesController' => 'DifferentialController', 'DifferentialRevisionListController' => 'DifferentialController', 'DifferentialRevisionListView' => 'AphrontView', diff --git a/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php b/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php new file mode 100644 index 0000000000..9e566047ec --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php @@ -0,0 +1,53 @@ +getViewer(); + + $changeset_ids = array(); + foreach ($xactions as $xaction) { + $changeset_ids[] = $xaction->getComment()->getChangesetID(); + } + + $changesets = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withIDs($changeset_ids) + ->execute(); + + $changesets = mpull($changesets, null, 'getID'); + + return $changesets; + } + + public function getFieldValuesForConduit($object, $data) { + $comment = $object->getComment(); + + $changeset = $data[$comment->getChangesetID()]; + $diff = $changeset->getDiff(); + + return array( + 'diff' => array( + 'id' => $diff->getID(), + 'phid' => $diff->getPHID(), + ), + 'path' => $changeset->getDisplayFilename(), + 'line' => (int)$comment->getLineNumber(), + 'length' => (int)($comment->getLineLength() + 1), + 'replyToCommentPHID' => $comment->getReplyToCommentPHID(), + ); + } + +} diff --git a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php index b3065b0d9c..43b94874bf 100644 --- a/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php +++ b/src/applications/transactions/conduit/TransactionSearchConduitAPIMethod.php @@ -97,7 +97,18 @@ final class TransactionSearchConduitAPIMethod continue; } - $modular_template = $xaction->getModularType(); + // TODO: Hack things so certain transactions which don't have a modular + // type yet can use a pseudotype until they modularize. Some day, we'll + // modularize everything and remove this. + switch ($xaction->getTransactionType()) { + case DifferentialTransaction::TYPE_INLINE: + $modular_template = new DifferentialRevisionInlineTransaction(); + break; + default: + $modular_template = $xaction->getModularType(); + break; + } + $modular_class = get_class($modular_template); if (!isset($modular_objects[$modular_class])) { try { @@ -173,6 +184,16 @@ final class TransactionSearchConduitAPIMethod $fields = (object)$fields; } + // If we haven't found a modular type, fallback for some simple core + // types. Ideally, we'll modularize everything some day. + if ($type === null) { + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_COMMENT: + $type = 'comment'; + break; + } + } + $data[] = array( 'id' => (int)$xaction->getID(), 'phid' => (string)$xaction->getPHID(), From 336fe5cdc52508d9280f8b1ed046d5086eefdfc4 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 24 Aug 2017 18:52:28 -0700 Subject: [PATCH 21/27] Dont send an email when someone views a Phame post Summary: lulz. :( Test Plan: Load PhamePost, get email. Fix. Reload PhamePost, no email. Reviewers: epriestley, avivey Reviewed By: avivey Spies: Korvin Differential Revision: https://secure.phabricator.com/D18471 --- src/applications/phame/editor/PhamePostEditor.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/applications/phame/editor/PhamePostEditor.php b/src/applications/phame/editor/PhamePostEditor.php index 845538dc12..9dea9692bf 100644 --- a/src/applications/phame/editor/PhamePostEditor.php +++ b/src/applications/phame/editor/PhamePostEditor.php @@ -32,6 +32,12 @@ final class PhamePostEditor if ($object->isDraft() || ($object->isArchived())) { return false; } + foreach ($xactions as $xaction) { + switch ($xaction->getTransactionType()) { + case PhamePostViewsTransaction::TRANSACTIONTYPE: + return false; + } + } return true; } From c49896f7c5a78b1a1cec3a62491899f27221b6d7 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 24 Aug 2017 19:10:28 -0700 Subject: [PATCH 22/27] Some header tag icons are too small Summary: Ref D17991, this rule got more specific with shade tag re-write, so needs updating for headers. Test Plan: Review a differential header, first icon is now 15px instead of 12px Reviewers: epriestley, avivey Reviewed By: avivey Spies: Korvin Differential Revision: https://secure.phabricator.com/D18472 --- resources/celerity/map.php | 6 +++--- webroot/rsrc/css/phui/phui-header-view.css | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index eecee2ed5a..e078912296 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => 'b5b51108', - 'core.pkg.css' => '06a86de6', + 'core.pkg.css' => '5b85ece6', 'core.pkg.js' => '6c085267', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -158,7 +158,7 @@ return array( 'rsrc/css/phui/phui-form-view.css' => 'ae9f8d16', 'rsrc/css/phui/phui-form.css' => '7aaa04e3', 'rsrc/css/phui/phui-head-thing.css' => 'fd311e5f', - 'rsrc/css/phui/phui-header-view.css' => 'e7de7ee2', + 'rsrc/css/phui/phui-header-view.css' => '808b82c7', 'rsrc/css/phui/phui-hovercard.css' => 'f0592bcf', 'rsrc/css/phui/phui-icon-set-selector.css' => '87db8fee', 'rsrc/css/phui/phui-icon.css' => '5c4a5de6', @@ -845,7 +845,7 @@ return array( 'phui-form-css' => '7aaa04e3', 'phui-form-view-css' => 'ae9f8d16', 'phui-head-thing-view-css' => 'fd311e5f', - 'phui-header-view-css' => 'e7de7ee2', + 'phui-header-view-css' => '808b82c7', 'phui-hovercard' => '1bd28176', 'phui-hovercard-view-css' => 'f0592bcf', 'phui-icon-set-selector-css' => '87db8fee', diff --git a/webroot/rsrc/css/phui/phui-header-view.css b/webroot/rsrc/css/phui/phui-header-view.css index 7926e431c1..8aefcee31c 100644 --- a/webroot/rsrc/css/phui/phui-header-view.css +++ b/webroot/rsrc/css/phui/phui-header-view.css @@ -187,8 +187,8 @@ body .phui-header-shell.phui-bleed-header margin-right: 4px; } -.phui-header-subheader .phui-tag-view .phui-icon-view, -.phui-header-subheader .policy-header-callout .phui-icon-view { +.phui-header-subheader .phui-tag-view span.phui-icon-view, +.phui-header-subheader .policy-header-callout span.phui-icon-view { display: inline-block; margin: -2px 4px -2px 0; font-size: 15px; From 12ae08b6b1a1b7c330593e76c32817f7cdbc87dd Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 24 Aug 2017 19:36:33 -0700 Subject: [PATCH 23/27] Move differential revision to its own table column in blame view Summary: There is still some layout issues with revisions, so I've tested it better and moved it to it's own column Test Plan: Fake in some revision data, test various sizes and shapes. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18473 --- resources/celerity/map.php | 4 ++-- .../controller/DiffusionBrowseController.php | 19 +++++++++++-------- .../diffusion/diffusion-source.css | 18 ++++++++++-------- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index e078912296..10305393cc 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -74,7 +74,7 @@ return array( 'rsrc/css/application/diffusion/diffusion-icons.css' => '0c15255e', 'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec', - 'rsrc/css/application/diffusion/diffusion-source.css' => '5c697665', + 'rsrc/css/application/diffusion/diffusion-source.css' => 'cb2bf02e', 'rsrc/css/application/diffusion/diffusion.css' => 'ceacf994', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', @@ -574,7 +574,7 @@ return array( 'diffusion-icons-css' => '0c15255e', 'diffusion-readme-css' => '419dd5b6', 'diffusion-repository-css' => 'ee6f20ec', - 'diffusion-source-css' => '5c697665', + 'diffusion-source-css' => 'cb2bf02e', 'diviner-shared-css' => '896f1d43', 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index f3c9210c50..ac2d08ef1f 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -706,6 +706,7 @@ final class DiffusionBrowseController extends DiffusionController { $buttons[] = id(new PHUIButtonView()) + ->setTag('a') ->setText(pht('Last Change')) ->setColor(PHUIButtonView::GREY) ->setHref( @@ -1175,19 +1176,21 @@ final class DiffusionBrowseController extends DiffusionController { ), $before_link); - $object_links = array(); - $object_links[] = $commit_link; - if ($revision_link) { - $object_links[] = phutil_tag('span', array(), '/'); - $object_links[] = $revision_link; - } - $row[] = phutil_tag( 'th', array( 'class' => 'diffusion-rev-link', ), - $object_links); + $commit_link); + + if ($revision_link) { + $row[] = phutil_tag( + 'th', + array( + 'class' => 'diffusion-blame-revision', + ), + $revision_link); + } $row[] = phutil_tag( 'th', diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index 8662683a0c..90b1fb01fe 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -40,11 +40,12 @@ .diffusion-blame-date { background: {$lightgreybackground}; font: {$basefont}; - font-size: {$smallestfontsize}; + font-size: {$smallerfontsize}; } .diffusion-blame-link, -.diffusion-line-link { +.diffusion-line-link, +.diffusion-blame-revision { background: {$lightgreybackground}; } @@ -53,7 +54,7 @@ min-width: 130px; background: {$lightgreybackground}; font: {$basefont}; - font-size: {$smallestfontsize}; + font-size: {$smallerfontsize}; } .diffusion-source a { @@ -61,17 +62,18 @@ } .diffusion-rev-link a { - max-width: 340px; + max-width: 300px; overflow: hidden; white-space: nowrap; text-overflow: ellipsis; + margin: 3px 8px; + display: block; } -.diffusion-rev-link a, -.diffusion-rev-link span, -.diffusion-blame-date a { +.diffusion-blame-date a, +.diffusion-blame-revision a { + float: right; margin: 3px 8px; - float: left; } .diffusion-rev-link span { From 94cad30ac3f052a711ececf7e370bf5c0071827f Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 24 Aug 2017 19:52:12 -0700 Subject: [PATCH 24/27] Fix bad tables in diffusion blame Summary: My fake data was 100%, and not all tables have full revision history. This leads to a broken table. Instead check if we have //any// revisions at all, then always show the column, with or without a link inside. Test Plan: going on a limb this is the correct fix and test on secure... again ... Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18474 --- resources/celerity/map.php | 4 ++-- .../controller/DiffusionBrowseController.php | 2 +- .../css/application/diffusion/diffusion-source.css | 14 ++++---------- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 10305393cc..99dfc0b6dd 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -74,7 +74,7 @@ return array( 'rsrc/css/application/diffusion/diffusion-icons.css' => '0c15255e', 'rsrc/css/application/diffusion/diffusion-readme.css' => '419dd5b6', 'rsrc/css/application/diffusion/diffusion-repository.css' => 'ee6f20ec', - 'rsrc/css/application/diffusion/diffusion-source.css' => 'cb2bf02e', + 'rsrc/css/application/diffusion/diffusion-source.css' => '47db8a7c', 'rsrc/css/application/diffusion/diffusion.css' => 'ceacf994', 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', @@ -574,7 +574,7 @@ return array( 'diffusion-icons-css' => '0c15255e', 'diffusion-readme-css' => '419dd5b6', 'diffusion-repository-css' => 'ee6f20ec', - 'diffusion-source-css' => 'cb2bf02e', + 'diffusion-source-css' => '47db8a7c', 'diviner-shared-css' => '896f1d43', 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index ac2d08ef1f..b9d63b2f85 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -1183,7 +1183,7 @@ final class DiffusionBrowseController extends DiffusionController { ), $commit_link); - if ($revision_link) { + if ($revision_map) { $row[] = phutil_tag( 'th', array( diff --git a/webroot/rsrc/css/application/diffusion/diffusion-source.css b/webroot/rsrc/css/application/diffusion/diffusion-source.css index 90b1fb01fe..ffbed2589e 100644 --- a/webroot/rsrc/css/application/diffusion/diffusion-source.css +++ b/webroot/rsrc/css/application/diffusion/diffusion-source.css @@ -37,24 +37,18 @@ white-space: nowrap; } -.diffusion-blame-date { +.diffusion-blame-date, +.diffusion-blame-link, +.diffusion-blame-revision, +.diffusion-rev-link { background: {$lightgreybackground}; font: {$basefont}; font-size: {$smallerfontsize}; } -.diffusion-blame-link, -.diffusion-line-link, -.diffusion-blame-revision { - background: {$lightgreybackground}; -} - .diffusion-source th.diffusion-rev-link { text-align: left; min-width: 130px; - background: {$lightgreybackground}; - font: {$basefont}; - font-size: {$smallerfontsize}; } .diffusion-source a { From 213e4ec9b55344609f89aee20bc80af823d39b9b Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 25 Aug 2017 07:23:54 -0700 Subject: [PATCH 25/27] Add a missing (int) cast to diff IDs for new "transaction.search" method Summary: These come out of the database as strings (see T12678), force them to integers for the API. Test Plan: Called `transaction.search`, got integers in JSON instead of strings. Reviewers: chad Reviewed By: chad Differential Revision: https://secure.phabricator.com/D18476 --- .../xaction/DifferentialRevisionInlineTransaction.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php b/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php index 9e566047ec..35d5034033 100644 --- a/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionInlineTransaction.php @@ -40,7 +40,7 @@ final class DifferentialRevisionInlineTransaction return array( 'diff' => array( - 'id' => $diff->getID(), + 'id' => (int)$diff->getID(), 'phid' => $diff->getPHID(), ), 'path' => $changeset->getDisplayFilename(), From 79c6b500491478e81a7e47df3db1b2fa88fb1916 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Fri, 25 Aug 2017 08:30:23 -0700 Subject: [PATCH 26/27] Fix fatal on logged out Phame Post Summary: Just deletes the view code until I have time to better plan this out, or just not ship. Test Plan: Visit Phame post on public logged out page, view count doesnt cause transaction fatal. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18475 --- .../20170825.phame.01.post.views.sql | 2 ++ src/__phutil_library_map__.php | 2 -- .../post/PhamePostViewController.php | 24 ------------------- .../phame/editor/PhamePostEditor.php | 12 ---------- src/applications/phame/storage/PhamePost.php | 5 +--- .../xaction/PhamePostViewsTransaction.php | 18 -------------- 6 files changed, 3 insertions(+), 60 deletions(-) create mode 100644 resources/sql/autopatches/20170825.phame.01.post.views.sql delete mode 100644 src/applications/phame/xaction/PhamePostViewsTransaction.php diff --git a/resources/sql/autopatches/20170825.phame.01.post.views.sql b/resources/sql/autopatches/20170825.phame.01.post.views.sql new file mode 100644 index 0000000000..5cb5c9c7b6 --- /dev/null +++ b/resources/sql/autopatches/20170825.phame.01.post.views.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_phame.phame_post + DROP COLUMN views; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f0c9271634..4575908ef8 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4421,7 +4421,6 @@ phutil_register_library_map(array( 'PhamePostTransactionQuery' => 'applications/phame/query/PhamePostTransactionQuery.php', 'PhamePostTransactionType' => 'applications/phame/xaction/PhamePostTransactionType.php', 'PhamePostViewController' => 'applications/phame/controller/post/PhamePostViewController.php', - 'PhamePostViewsTransaction' => 'applications/phame/xaction/PhamePostViewsTransaction.php', 'PhamePostVisibilityTransaction' => 'applications/phame/xaction/PhamePostVisibilityTransaction.php', 'PhameSchemaSpec' => 'applications/phame/storage/PhameSchemaSpec.php', 'PhameSite' => 'applications/phame/site/PhameSite.php', @@ -10054,7 +10053,6 @@ phutil_register_library_map(array( 'PhamePostTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhamePostTransactionType' => 'PhabricatorModularTransactionType', 'PhamePostViewController' => 'PhameLiveController', - 'PhamePostViewsTransaction' => 'PhamePostTransactionType', 'PhamePostVisibilityTransaction' => 'PhamePostTransactionType', 'PhameSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhameSite' => 'PhabricatorSite', diff --git a/src/applications/phame/controller/post/PhamePostViewController.php b/src/applications/phame/controller/post/PhamePostViewController.php index 7dbaddc17a..63adedb7ae 100644 --- a/src/applications/phame/controller/post/PhamePostViewController.php +++ b/src/applications/phame/controller/post/PhamePostViewController.php @@ -18,25 +18,6 @@ final class PhamePostViewController $is_live = $this->getIsLive(); $is_external = $this->getIsExternal(); - // Register a blog "view" count - // - if (!$post->isDraft() && !$post->isArchived()) { - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - $xactions = array(); - $xactions[] = id(new PhamePostTransaction()) - ->setTransactionType(PhamePostViewsTransaction::TRANSACTIONTYPE) - ->setNewValue(null); - - $editor = id(new PhamePostEditor()) - ->setActor($viewer) - ->setContentSourceFromRequest($request) - ->setContinueOnMissingFields(true) - ->setContinueOnNoEffect(true); - - $editor->applyTransactions($post, $xactions); - unset($unguarded); - } - $header = id(new PHUIHeaderView()) ->addClass('phame-header-bar') ->setUser($viewer); @@ -170,11 +151,6 @@ final class PhamePostViewController ->setUser($viewer) ->setObject($post); - $views = id(new PhutilNumber($post->getViews())); - $properties->addProperty( - pht('Views'), - pht('%s', $views)); - $is_live = $this->getIsLive(); $is_external = $this->getIsExternal(); $next_view = new PhameNextPostView(); diff --git a/src/applications/phame/editor/PhamePostEditor.php b/src/applications/phame/editor/PhamePostEditor.php index 9dea9692bf..488d7a4938 100644 --- a/src/applications/phame/editor/PhamePostEditor.php +++ b/src/applications/phame/editor/PhamePostEditor.php @@ -32,12 +32,6 @@ final class PhamePostEditor if ($object->isDraft() || ($object->isArchived())) { return false; } - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case PhamePostViewsTransaction::TRANSACTIONTYPE: - return false; - } - } return true; } @@ -47,12 +41,6 @@ final class PhamePostEditor if ($object->isDraft() || $object->isArchived()) { return false; } - foreach ($xactions as $xaction) { - switch ($xaction->getTransactionType()) { - case PhamePostViewsTransaction::TRANSACTIONTYPE: - return false; - } - } return true; } diff --git a/src/applications/phame/storage/PhamePost.php b/src/applications/phame/storage/PhamePost.php index c3ec4d4608..a9525e0be7 100644 --- a/src/applications/phame/storage/PhamePost.php +++ b/src/applications/phame/storage/PhamePost.php @@ -22,7 +22,6 @@ final class PhamePost extends PhameDAO protected $phameTitle; protected $body; protected $visibility; - protected $views; protected $configData; protected $datePublished; protected $blogPHID; @@ -41,8 +40,7 @@ final class PhamePost extends PhameDAO ->setBlogPHID($blog->getPHID()) ->attachBlog($blog) ->setDatePublished(PhabricatorTime::getNow()) - ->setVisibility(PhameConstants::VISIBILITY_PUBLISHED) - ->setViews(0); + ->setVisibility(PhameConstants::VISIBILITY_PUBLISHED); return $post; } @@ -130,7 +128,6 @@ final class PhamePost extends PhameDAO 'subtitle' => 'text64', 'phameTitle' => 'sort64?', 'visibility' => 'uint32', - 'views' => 'uint32', 'mailKey' => 'bytes20', 'headerImagePHID' => 'phid?', diff --git a/src/applications/phame/xaction/PhamePostViewsTransaction.php b/src/applications/phame/xaction/PhamePostViewsTransaction.php deleted file mode 100644 index 2260a1d752..0000000000 --- a/src/applications/phame/xaction/PhamePostViewsTransaction.php +++ /dev/null @@ -1,18 +0,0 @@ -getViews(); - } - - public function applyInternalEffects($object, $value) { - $views = $object->getViews(); - $views++; - $object->setViews($views); - } - -} From 750be1c92a66c651844b99dcad5f37d5f259ee11 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Fri, 25 Aug 2017 09:34:51 -0700 Subject: [PATCH 27/27] Minor spacing clean up on search button Summary: Gives the search box a small amount of space, smaller button Test Plan: look closely. Reviewers: epriestley Reviewed By: epriestley Spies: Korvin Differential Revision: https://secure.phabricator.com/D18477 --- resources/celerity/map.php | 12 ++++++------ webroot/rsrc/css/application/base/main-menu-view.css | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 99dfc0b6dd..2433beba80 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => 'e68cf1fa', 'conpherence.pkg.js' => 'b5b51108', - 'core.pkg.css' => '5b85ece6', + 'core.pkg.css' => '291cbd98', 'core.pkg.js' => '6c085267', 'darkconsole.pkg.js' => '1f9a31bc', 'differential.pkg.css' => '45951e9e', @@ -39,7 +39,7 @@ return array( 'rsrc/css/aphront/typeahead.css' => 'a4a21016', 'rsrc/css/application/almanac/almanac.css' => 'dbb9b3af', 'rsrc/css/application/auth/auth.css' => '0877ed6e', - 'rsrc/css/application/base/main-menu-view.css' => '16053029', + 'rsrc/css/application/base/main-menu-view.css' => '1802a242', 'rsrc/css/application/base/notification-menu.css' => '73fefdfa', 'rsrc/css/application/base/phui-theme.css' => '9f261c6b', 'rsrc/css/application/base/standard-page-view.css' => '34ee718b', @@ -789,7 +789,7 @@ return array( 'phabricator-flag-css' => 'bba8f811', 'phabricator-keyboard-shortcut' => '1ae869f2', 'phabricator-keyboard-shortcut-manager' => 'c19dd9b9', - 'phabricator-main-menu-view' => '16053029', + 'phabricator-main-menu-view' => '1802a242', 'phabricator-nav-view-css' => 'faf6a6fc', 'phabricator-notification' => '5c3349b2', 'phabricator-notification-css' => '3f6c89c9', @@ -981,9 +981,6 @@ return array( 'aphront-typeahead-control-css', 'phui-tag-view-css', ), - 16053029 => array( - 'phui-theme-css', - ), '17bb8539' => array( 'javelin-behavior', 'javelin-stratcom', @@ -994,6 +991,9 @@ return array( 'phabricator-darklog', 'phabricator-darkmessage', ), + '1802a242' => array( + 'phui-theme-css', + ), '185bbd53' => array( 'javelin-install', ), diff --git a/webroot/rsrc/css/application/base/main-menu-view.css b/webroot/rsrc/css/application/base/main-menu-view.css index 39ec89c9fc..80adda0502 100644 --- a/webroot/rsrc/css/application/base/main-menu-view.css +++ b/webroot/rsrc/css/application/base/main-menu-view.css @@ -166,7 +166,7 @@ border: none; background-color: {$page.content}; height: 28px; - padding: 3px 28px 3px 52px; + padding: 3px 28px 3px 48px; float: left; width: 280px; } @@ -212,7 +212,7 @@ position: absolute; right: auto; left: 12px; - width: 46px; + width: 40px; background: {$greybackground}; z-index: 1; } @@ -252,7 +252,7 @@ a.phabricator-core-user-menu .caret:before { .phabricator-main-menu-search-dropdown .caret { position: absolute; - right: 18px; + right: 20px; top: 2px; border: none; margin-top: 1px;