From e5906f4e127eb9bba62397f1228fa5d01f52ce22 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 08:21:16 -0700 Subject: [PATCH 01/21] In Differential standalone views, disable some keyboard shortcuts which don't work Summary: Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files. We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI. Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled. Test Plan: - Viewed a revision in normal and standalone views. - No changes in normal view, and all keys still work ("N", "P", etc). - In standalone view, "?" no longer shows nonfunctional key commands. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19571 --- resources/celerity/map.php | 32 +++++++++---------- .../DifferentialChangesetViewController.php | 1 + .../view/DifferentialChangesetListView.php | 11 +++++++ .../js/application/diff/DiffChangesetList.js | 29 +++++++++++------ .../differential/behavior-populate.js | 3 +- 5 files changed, 50 insertions(+), 26 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 9c0d0eda0a..12d47cd5f9 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => 'f515619b', 'core.pkg.js' => '2058ec09', 'differential.pkg.css' => '06dc617c', - 'differential.pkg.js' => 'ef19e026', + 'differential.pkg.js' => '11a08e85', 'diffusion.pkg.css' => 'a2d17c7d', 'diffusion.pkg.js' => '6134c5a1', 'maniphest.pkg.css' => '4845691a', @@ -373,12 +373,12 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', 'rsrc/js/application/diff/DiffChangeset.js' => 'b49b59d6', - 'rsrc/js/application/diff/DiffChangesetList.js' => 'f0ffe8c3', + 'rsrc/js/application/diff/DiffChangesetList.js' => '7b95a80a', 'rsrc/js/application/diff/DiffInline.js' => 'e83d28f3', 'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832', 'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07', 'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1', - 'rsrc/js/application/differential/behavior-populate.js' => '419998ab', + 'rsrc/js/application/differential/behavior-populate.js' => 'f0eb6708', 'rsrc/js/application/differential/behavior-user-select.js' => 'a8d8459d', 'rsrc/js/application/diffusion/DiffusionLocateFileSource.js' => '00676f00', 'rsrc/js/application/diffusion/behavior-audit-preview.js' => 'd835b03a', @@ -594,7 +594,7 @@ return array( 'javelin-behavior-diff-preview-link' => '051c7832', 'javelin-behavior-differential-diff-radios' => 'e1ff79b1', 'javelin-behavior-differential-feedback-preview' => '51c5ad07', - 'javelin-behavior-differential-populate' => '419998ab', + 'javelin-behavior-differential-populate' => 'f0eb6708', 'javelin-behavior-differential-user-select' => 'a8d8459d', 'javelin-behavior-diffusion-commit-branches' => 'bdaf4d04', 'javelin-behavior-diffusion-commit-graph' => '75b83cbb', @@ -752,7 +752,7 @@ return array( 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', 'phabricator-diff-changeset' => 'b49b59d6', - 'phabricator-diff-changeset-list' => 'f0ffe8c3', + 'phabricator-diff-changeset-list' => '7b95a80a', 'phabricator-diff-inline' => 'e83d28f3', 'phabricator-drag-and-drop-file-upload' => '58dea2fa', 'phabricator-draggable-list' => 'bea6e7f4', @@ -1133,14 +1133,6 @@ return array( 'javelin-workflow', 'phabricator-draggable-list', ), - '419998ab' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-stratcom', - 'phabricator-tooltip', - 'phabricator-diff-changeset-list', - 'phabricator-diff-changeset', - ), '4250a34e' => array( 'javelin-behavior', 'javelin-dom', @@ -1508,6 +1500,10 @@ return array( 'owners-path-editor', 'javelin-behavior', ), + '7b95a80a' => array( + 'javelin-install', + 'phuix-button-view', + ), '7cbe244b' => array( 'javelin-install', 'javelin-util', @@ -2117,9 +2113,13 @@ return array( 'javelin-workflow', 'javelin-json', ), - 'f0ffe8c3' => array( - 'javelin-install', - 'phuix-button-view', + 'f0eb6708' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-stratcom', + 'phabricator-tooltip', + 'phabricator-diff-changeset-list', + 'phabricator-diff-changeset', ), 'f1ff5494' => array( 'phui-button-css', diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php index 9d02202f9f..c41f951394 100644 --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -276,6 +276,7 @@ final class DifferentialChangesetViewController extends DifferentialController { ->setDiff($diff) ->setTitle(pht('Standalone View')) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setIsStandalone(true) ->setParser($parser); if ($revision_id) { diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 59f73b5465..2ed963bae3 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -10,6 +10,7 @@ final class DifferentialChangesetListView extends AphrontView { private $whitespace; private $background; private $header; + private $isStandalone; private $standaloneURI; private $leftRawFileURI; @@ -124,6 +125,15 @@ final class DifferentialChangesetListView extends AphrontView { return $this; } + public function setIsStandalone($is_standalone) { + $this->isStandalone = $is_standalone; + return $this; + } + + public function getIsStandalone() { + return $this->isStandalone; + } + public function setBackground($background) { $this->background = $background; return $this; @@ -219,6 +229,7 @@ final class DifferentialChangesetListView extends AphrontView { 'changesetViewIDs' => $ids, 'inlineURI' => $this->inlineURI, 'inlineListURI' => $this->inlineListURI, + 'isStandalone' => $this->getIsStandalone(), 'pht' => array( 'Open in Editor' => pht('Open in Editor'), 'Show All Context' => pht('Show All Context'), diff --git a/webroot/rsrc/js/application/diff/DiffChangesetList.js b/webroot/rsrc/js/application/diff/DiffChangesetList.js index 9e87fb0f31..66c80cb9cf 100644 --- a/webroot/rsrc/js/application/diff/DiffChangesetList.js +++ b/webroot/rsrc/js/application/diff/DiffChangesetList.js @@ -89,7 +89,8 @@ JX.install('DiffChangesetList', { properties: { translations: null, inlineURI: null, - inlineListURI: null + inlineListURI: null, + isStandalone: false }, members: { @@ -149,6 +150,12 @@ JX.install('DiffChangesetList', { this._initialized = true; var pht = this.getTranslations(); + // We may be viewing the normal "/D123" view (with all the changesets) + // or the standalone view (with just one changeset). In the standalone + // view, some options (like jumping to next or previous file) do not + // make sense and do not function. + var standalone = this.getIsStandalone(); + var label; label = pht('Jump to next change.'); @@ -157,11 +164,13 @@ JX.install('DiffChangesetList', { label = pht('Jump to previous change.'); this._installJumpKey('k', label, -1); - label = pht('Jump to next file.'); - this._installJumpKey('J', label, 1, 'file'); + if (!standalone) { + label = pht('Jump to next file.'); + this._installJumpKey('J', label, 1, 'file'); - label = pht('Jump to previous file.'); - this._installJumpKey('K', label, -1, 'file'); + label = pht('Jump to previous file.'); + this._installJumpKey('K', label, -1, 'file'); + } label = pht('Jump to next inline comment.'); this._installJumpKey('n', label, 1, 'comment'); @@ -176,11 +185,13 @@ JX.install('DiffChangesetList', { 'Jump to previous inline comment, including collapsed comments.'); this._installJumpKey('P', label, -1, 'comment', true); - label = pht('Hide or show the current file.'); - this._installKey('h', label, this._onkeytogglefile); + if (!standalone) { + label = pht('Hide or show the current file.'); + this._installKey('h', label, this._onkeytogglefile); - label = pht('Jump to the table of contents.'); - this._installKey('t', label, this._ontoc); + label = pht('Jump to the table of contents.'); + this._installKey('t', label, this._ontoc); + } label = pht('Reply to selected inline comment or change.'); this._installKey('r', label, JX.bind(this, this._onkeyreply, false)); diff --git a/webroot/rsrc/js/application/differential/behavior-populate.js b/webroot/rsrc/js/application/differential/behavior-populate.js index 0a4a7912e4..5b415fd131 100644 --- a/webroot/rsrc/js/application/differential/behavior-populate.js +++ b/webroot/rsrc/js/application/differential/behavior-populate.js @@ -61,7 +61,8 @@ JX.behavior('differential-populate', function(config, statics) { var changeset_list = new JX.DiffChangesetList() .setTranslations(JX.phtize(config.pht)) .setInlineURI(config.inlineURI) - .setInlineListURI(config.inlineListURI); + .setInlineListURI(config.inlineListURI) + .setIsStandalone(config.isStandalone); // Install and activate the current page. var page_id = JX.Quicksand.getCurrentPageID(); From b86dae6214ffccc0fec4bd29ea9b054f9eaa3ac3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 09:25:41 -0700 Subject: [PATCH 02/21] Fix an issue with error handling when no mailers are available Summary: Ref T13164. See PHI785. See D19546. I think I didn't test the updated error messaging here entirely properly, since I have some tasks in queue which error out here ("Missing argument 1 to newMailers(...)"). This is an error condition already, but we want to get through this call so we can raise a tailored message. Test Plan: Tasks which errored out here now succeed. This condition is only reachable if you misconfigure things in the first place. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19572 --- src/applications/metamta/storage/PhabricatorMetaMTAMail.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index d757868483..e7ebf6dc83 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -621,7 +621,7 @@ final class PhabricatorMetaMTAMail public function sendWithMailers(array $mailers) { if (!$mailers) { - $any_mailers = self::newMailers(); + $any_mailers = self::newMailers(array()); // NOTE: We can end up here with some custom list of "$mailers", like // from a unit test. In that case, this message could be misleading. We From fb3ae72e367f5ec57c706593d67975c4630463c6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 09:31:13 -0700 Subject: [PATCH 03/21] When cancelling addition of an Almanac interface, return to the Device page Summary: Fixes T13184. In Almanac, interfaces are always added to devices. However, if you "Add New Interface" and then "Cancel", you go to the nonexistent `/interface/` page. Instead, return to the device page. Test Plan: From a device page, clicked "Add Interface" and then "Cancel". Ended up back where I was. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13184 Differential Revision: https://secure.phabricator.com/D19573 --- src/applications/almanac/editor/AlmanacInterfaceEditEngine.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/applications/almanac/editor/AlmanacInterfaceEditEngine.php b/src/applications/almanac/editor/AlmanacInterfaceEditEngine.php index 2c68ed0374..30c371d6f9 100644 --- a/src/applications/almanac/editor/AlmanacInterfaceEditEngine.php +++ b/src/applications/almanac/editor/AlmanacInterfaceEditEngine.php @@ -127,6 +127,9 @@ final class AlmanacInterfaceEditEngine } protected function getObjectCreateCancelURI($object) { + if ($this->getDevice()) { + return $this->getDevice()->getURI(); + } return '/almanac/interface/'; } From 92a29f72c14c3f4329c337df0706849eb75e06e5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 09:49:22 -0700 Subject: [PATCH 04/21] Make the Drydock repository operation page slightly richer Summary: Ref T13164. See PHI788. The issue requests a "created" timestamp. Also add filtering for repository, state, and author. Test Plan: Used all filters. {F5795085} Reviewers: amckinley Reviewed By: amckinley Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19574 --- .../query/DrydockRepositoryOperationQuery.php | 13 +++++++ ...DrydockRepositoryOperationSearchEngine.php | 34 +++++++++++++++++++ .../storage/DrydockRepositoryOperation.php | 17 ++++++---- 3 files changed, 57 insertions(+), 7 deletions(-) diff --git a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php index 570e68e825..fb1f6583a8 100644 --- a/src/applications/drydock/query/DrydockRepositoryOperationQuery.php +++ b/src/applications/drydock/query/DrydockRepositoryOperationQuery.php @@ -9,6 +9,7 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { private $operationStates; private $operationTypes; private $isDismissed; + private $authorPHIDs; public function withIDs(array $ids) { $this->ids = $ids; @@ -45,6 +46,11 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { return $this; } + public function withAuthorPHIDs(array $phids) { + $this->authorPHIDs = $phids; + return $this; + } + public function newResultObject() { return new DrydockRepositoryOperation(); } @@ -165,6 +171,13 @@ final class DrydockRepositoryOperationQuery extends DrydockQuery { (int)$this->isDismissed); } + if ($this->authorPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'authorPHID IN (%Ls)', + $this->authorPHIDs); + } + return $where; } diff --git a/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php b/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php index 1de32be28f..eeb85329bf 100644 --- a/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php +++ b/src/applications/drydock/query/DrydockRepositoryOperationSearchEngine.php @@ -18,11 +18,41 @@ final class DrydockRepositoryOperationSearchEngine protected function buildQueryFromParameters(array $map) { $query = $this->newQuery(); + if ($map['repositoryPHIDs']) { + $query->withRepositoryPHIDs($map['repositoryPHIDs']); + } + + if ($map['authorPHIDs']) { + $query->withAuthorPHIDs($map['authorPHIDs']); + } + + if ($map['states']) { + $query->withOperationStates($map['states']); + } + return $query; } protected function buildCustomSearchFields() { return array( + id(new PhabricatorSearchDatasourceField()) + ->setLabel(pht('Repositories')) + ->setKey('repositoryPHIDs') + ->setAliases(array('repository', 'repositories', 'repositoryPHID')) + ->setDatasource(new DiffusionRepositoryFunctionDatasource()), + + // NOTE: Repository operations aren't necessarily created by a real + // user, but for now they normally are. Just use a user typeahead until + // more use cases arise. + id(new PhabricatorUsersSearchField()) + ->setLabel(pht('Authors')) + ->setKey('authorPHIDs') + ->setAliases(array('author', 'authors', 'authorPHID')), + id(new PhabricatorSearchCheckboxesField()) + ->setLabel(pht('States')) + ->setKey('states') + ->setAliases(array('state')) + ->setOptions(DrydockRepositoryOperation::getOperationStateNameMap()), ); } @@ -72,6 +102,10 @@ final class DrydockRepositoryOperationSearchEngine $item->setStatusIcon($icon, $name); + + $created = phabricator_datetime($operation->getDateCreated(), $viewer); + $item->addIcon(null, $created); + $item->addByline( array( pht('Via:'), diff --git a/src/applications/drydock/storage/DrydockRepositoryOperation.php b/src/applications/drydock/storage/DrydockRepositoryOperation.php index 190b5b1310..4e25280c3f 100644 --- a/src/applications/drydock/storage/DrydockRepositoryOperation.php +++ b/src/applications/drydock/storage/DrydockRepositoryOperation.php @@ -99,6 +99,15 @@ final class DrydockRepositoryOperation extends DrydockDAO return $this; } + public static function getOperationStateNameMap() { + return array( + self::STATE_WAIT => pht('Waiting'), + self::STATE_WORK => pht('Working'), + self::STATE_DONE => pht('Done'), + self::STATE_FAIL => pht('Failed'), + ); + } + public static function getOperationStateIcon($state) { $map = array( self::STATE_WAIT => 'fa-clock-o', @@ -111,13 +120,7 @@ final class DrydockRepositoryOperation extends DrydockDAO } public static function getOperationStateName($state) { - $map = array( - self::STATE_WAIT => pht('Waiting'), - self::STATE_WORK => pht('Working'), - self::STATE_DONE => pht('Done'), - self::STATE_FAIL => pht('Failed'), - ); - + $map = self::getOperationStateNameMap(); return idx($map, $state, pht('', $state)); } From 9a15129b402fd7a15e81df3b5fe6fdf4a68d1ae0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 11:35:02 -0700 Subject: [PATCH 05/21] Remove 750ms timeout on owners path validation Summary: Ref T13164. See PHI748. Path validation has a 750ms timeout which blames to rP5038ab850c, in 2011. Production path validation is sometimes taking more than 750ms, particularly on the initial page load where we may validate many paths simultaneously. I have no idea why we have this timeout, and it isn't consistent with how we perform other AJAX requests. Just remove it. Test Plan: - Reproduced issue in production, saw all validation calls failing at 750ms. Actual underlying calls succeed, they just take more than 750ms to resolve. - Loaded path validator locally, got green checkmark. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19575 --- resources/celerity/map.php | 20 +++++++++---------- .../js/application/herald/PathTypeahead.js | 2 -- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 12d47cd5f9..2b81560e43 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -393,7 +393,7 @@ return array( 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '549459b8', 'rsrc/js/application/herald/HeraldRuleEditor.js' => 'dca75c0e', - 'rsrc/js/application/herald/PathTypeahead.js' => '662e9cea', + 'rsrc/js/application/herald/PathTypeahead.js' => '6d8c7912', 'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3', 'rsrc/js/application/maniphest/behavior-batch-selector.js' => 'ad54037e', 'rsrc/js/application/maniphest/behavior-line-chart.js' => 'e4232876', @@ -739,7 +739,7 @@ return array( 'owners-path-editor' => 'c96502cf', 'owners-path-editor-css' => '9c136c29', 'paste-css' => '9fcc9773', - 'path-typeahead' => '662e9cea', + 'path-typeahead' => '6d8c7912', 'people-picture-menu-item-css' => 'a06f7f34', 'people-profile-css' => '4df76faf', 'phabricator-action-list-view-css' => '0bcd9a45', @@ -1353,14 +1353,6 @@ return array( 'javelin-workflow', 'javelin-dom', ), - '662e9cea' => array( - 'javelin-install', - 'javelin-typeahead', - 'javelin-dom', - 'javelin-request', - 'javelin-typeahead-ondemand-source', - 'javelin-util', - ), 66888767 => array( 'javelin-behavior', 'javelin-stratcom', @@ -1436,6 +1428,14 @@ return array( 'javelin-typeahead', 'javelin-uri', ), + '6d8c7912' => array( + 'javelin-install', + 'javelin-typeahead', + 'javelin-dom', + 'javelin-request', + 'javelin-typeahead-ondemand-source', + 'javelin-util', + ), '70baed2f' => array( 'javelin-install', 'javelin-dom', diff --git a/webroot/rsrc/js/application/herald/PathTypeahead.js b/webroot/rsrc/js/application/herald/PathTypeahead.js index 0ce9823d13..eed6e62b62 100644 --- a/webroot/rsrc/js/application/herald/PathTypeahead.js +++ b/webroot/rsrc/js/application/herald/PathTypeahead.js @@ -208,8 +208,6 @@ JX.install('PathTypeahead', { this._validationInflight = validation_request; JX.DOM.setContent(error_display, JX.$H(this._icons.test)); - - validation_request.setTimeout(750); validation_request.send(); } } From 3b05e920e0094af372416839f36e34fc9af2458a Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Mon, 13 Aug 2018 14:21:56 -0700 Subject: [PATCH 06/21] Start changing DiffusionCommitController to use identities Summary: Depends on D19491. Test Plan: Viewed some commits where the identity was mapped to a user and another that wasn't; saw the header render either a link to the user or the identity object. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19492 --- .../controller/DiffusionCommitController.php | 32 +++------ .../storage/PhabricatorRepositoryCommit.php | 71 +++++++++++++++++++ .../storage/PhabricatorRepositoryIdentity.php | 8 +++ 3 files changed, 87 insertions(+), 24 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php index 67ffb73357..a6743b6557 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -46,6 +46,7 @@ final class DiffusionCommitController extends DiffusionController { ->needCommitData(true) ->needAuditRequests(true) ->setLimit(100) + ->needIdentities(true) ->execute(); $multiple_results = count($commits) > 1; @@ -504,15 +505,13 @@ final class DiffusionCommitController extends DiffusionController { $phids = $edge_query->getDestinationPHIDs(array($commit_phid)); - if ($data->getCommitDetail('authorPHID')) { - $phids[] = $data->getCommitDetail('authorPHID'); - } + if ($data->getCommitDetail('reviewerPHID')) { $phids[] = $data->getCommitDetail('reviewerPHID'); } - if ($data->getCommitDetail('committerPHID')) { - $phids[] = $data->getCommitDetail('committerPHID'); - } + + $phids[] = $commit->getCommitterDisplayPHID(); + $phids[] = $commit->getAuthorDisplayPHID(); // NOTE: We should never normally have more than a single push log, but // it can occur naturally if a commit is pushed, then the branch it was @@ -573,24 +572,11 @@ final class DiffusionCommitController extends DiffusionController { } } - $author_phid = $data->getCommitDetail('authorPHID'); - $author_name = $data->getAuthorName(); $author_epoch = $data->getCommitDetail('authorEpoch'); $committed_info = id(new PHUIStatusItemView()) - ->setNote(phabricator_datetime($commit->getEpoch(), $viewer)); - - $committer_phid = $data->getCommitDetail('committerPHID'); - $committer_name = $data->getCommitDetail('committer'); - if ($committer_phid) { - $committed_info->setTarget($handles[$committer_phid]->renderLink()); - } else if (strlen($committer_name)) { - $committed_info->setTarget($committer_name); - } else if ($author_phid) { - $committed_info->setTarget($handles[$author_phid]->renderLink()); - } else if (strlen($author_name)) { - $committed_info->setTarget($author_name); - } + ->setNote(phabricator_datetime($commit->getEpoch(), $viewer)) + ->setTarget($commit->renderAnyCommitter($viewer, $handles)); $committed_list = new PHUIStatusListView(); $committed_list->addItem($committed_info); @@ -716,7 +702,7 @@ final class DiffusionCommitController extends DiffusionController { return null; } - $author_phid = $data->getCommitDetail('authorPHID'); + $author_phid = $commit->getAuthorDisplayPHID(); $author_name = $data->getAuthorName(); $author_epoch = $data->getCommitDetail('authorEpoch'); $date = null; @@ -748,10 +734,8 @@ final class DiffusionCommitController extends DiffusionController { ->setImage($image_uri) ->setImageHref($image_href) ->setContent($content); - } - private function buildComments(PhabricatorRepositoryCommit $commit) { $timeline = $this->buildTransactionTimeline( $commit, diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index cf88c0eb54..8ad182ee7b 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -439,6 +439,77 @@ final class PhabricatorRepositoryCommit return $repository->formatCommitName($identifier, $local = true); } + /** + * Make a strong effort to find a way to render this commit's committer. + * This currently attempts to use @{PhabricatorRepositoryIdentity}, and + * falls back to examining the commit detail information. After we force + * the migration to using identities, update this method to remove the + * fallback. See T12164 for details. + */ + public function renderAnyCommitter(PhabricatorUser $viewer, array $handles) { + $committer = $this->renderCommitter($viewer, $handles); + if ($committer) { + return $committer; + } + + return $this->renderAuthor($viewer, $handles); + } + + public function renderCommitter(PhabricatorUser $viewer, array $handles) { + $committer_phid = $this->getCommitterDisplayPHID(); + if ($committer_phid) { + return $handles[$committer_phid]->renderLink(); + } + + $data = $this->getCommitData(); + $committer_name = $data->getCommitDetail('committer'); + if (strlen($committer_name)) { + return $committer_name; + } + + return null; + } + + public function renderAuthor(PhabricatorUser $viewer, array $handles) { + $author_phid = $this->getAuthorDisplayPHID(); + if ($author_phid) { + return $handles[$author_phid]->renderLink(); + } + + $data = $this->getCommitData(); + $author_name = $data->getAuthorName(); + if (strlen($author_name)) { + return $author_name; + } + + return null; + } + + public function hasCommitterIdentity() { + return ($this->getCommitterIdentity() !== null); + } + + public function hasAuthorIdentity() { + return ($this->getAuthorIdentity() !== null); + } + + public function getCommitterDisplayPHID() { + if ($this->hasCommitterIdentity()) { + return $this->getCommitterIdentity()->getIdentityDisplayPHID(); + } + + $data = $this->getCommitData(); + return $data->getCommitDetail('committerPHID'); + } + + public function getAuthorDisplayPHID() { + if ($this->hasAuthorIdentity()) { + return $this->getAuthorIdentity()->getIdentityDisplayPHID(); + } + + $data = $this->getCommitData(); + return $data->getCommitDetail('authorPHID'); + } /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php index bf421692a0..f801e06b89 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryIdentity.php +++ b/src/applications/repository/storage/PhabricatorRepositoryIdentity.php @@ -78,6 +78,14 @@ final class PhabricatorRepositoryIdentity return ($this->currentEffectiveUserPHID != null); } + public function getIdentityDisplayPHID() { + if ($this->hasEffectiveUser()) { + return $this->getCurrentEffectiveUserPHID(); + } else { + return $this->getPHID(); + } + } + public function save() { if ($this->manuallySetUserPHID) { $this->currentEffectiveUserPHID = $this->manuallySetUserPHID; From cc1def6ceaab41483103919e94bd227da01f5cdc Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Mon, 13 Aug 2018 15:37:04 -0700 Subject: [PATCH 07/21] Remove some array typehints for passing around Summary: See discussion at https://secure.phabricator.com/D19492#241996 Test Plan: Refreshed a few Diffusion tabs; nothing broke. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19578 --- .../repository/storage/PhabricatorRepositoryCommit.php | 6 +++--- .../subscriptions/view/SubscriptionListDialogBuilder.php | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 8ad182ee7b..bf3b796a76 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -446,7 +446,7 @@ final class PhabricatorRepositoryCommit * the migration to using identities, update this method to remove the * fallback. See T12164 for details. */ - public function renderAnyCommitter(PhabricatorUser $viewer, array $handles) { + public function renderAnyCommitter(PhabricatorUser $viewer, $handles) { $committer = $this->renderCommitter($viewer, $handles); if ($committer) { return $committer; @@ -455,7 +455,7 @@ final class PhabricatorRepositoryCommit return $this->renderAuthor($viewer, $handles); } - public function renderCommitter(PhabricatorUser $viewer, array $handles) { + public function renderCommitter(PhabricatorUser $viewer, $handles) { $committer_phid = $this->getCommitterDisplayPHID(); if ($committer_phid) { return $handles[$committer_phid]->renderLink(); @@ -470,7 +470,7 @@ final class PhabricatorRepositoryCommit return null; } - public function renderAuthor(PhabricatorUser $viewer, array $handles) { + public function renderAuthor(PhabricatorUser $viewer, $handles) { $author_phid = $this->getAuthorDisplayPHID(); if ($author_phid) { return $handles[$author_phid]->renderLink(); diff --git a/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php b/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php index 7d1b8d799c..da157848ea 100644 --- a/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php +++ b/src/applications/subscriptions/view/SubscriptionListDialogBuilder.php @@ -58,7 +58,7 @@ final class SubscriptionListDialogBuilder extends Phobject { ->addCancelButton($object_handle->getURI(), pht('Close')); } - private function buildBody(PhabricatorUser $viewer, array $handles) { + private function buildBody(PhabricatorUser $viewer, $handles) { $list = id(new PHUIObjectItemListView()) ->setUser($viewer); From ba25586016b2b6731860ab1c682ddeba65eed0bb Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Thu, 16 Aug 2018 06:39:07 +1000 Subject: [PATCH 08/21] Improve symbol generation scripts Summary: Currently the symbol generation scripts fail if passed a list containing no files because `explode("\n", $input)` returns `array("")` rather than `array()`. This means that a generic Harbormaster Build Plan with a step which executes `find . -type f -name '*.php' | ./scripts/generate_php_symbols.php` won't work because it fails in repositories that don't contain any PHP code. Test Plan: Ran `echo | generate_php_symbols` and saw no output instead of an exception. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19588 --- scripts/symbols/generate_ctags_symbols.php | 4 ++++ scripts/symbols/generate_php_symbols.php | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/scripts/symbols/generate_ctags_symbols.php b/scripts/symbols/generate_ctags_symbols.php index 8d77bfc478..e93b0c5cbc 100755 --- a/scripts/symbols/generate_ctags_symbols.php +++ b/scripts/symbols/generate_ctags_symbols.php @@ -39,6 +39,10 @@ $data = array(); $futures = array(); foreach (explode("\n", trim($input)) as $file) { + if (!strlen($file)) { + continue; + } + $file = Filesystem::readablePath($file); $futures[$file] = ctags_get_parser_future($file); } diff --git a/scripts/symbols/generate_php_symbols.php b/scripts/symbols/generate_php_symbols.php index db8412764e..af87d580d8 100755 --- a/scripts/symbols/generate_php_symbols.php +++ b/scripts/symbols/generate_php_symbols.php @@ -27,6 +27,10 @@ $data = array(); $futures = array(); foreach (explode("\n", trim($input)) as $file) { + if (!strlen($file)) { + continue; + } + $file = Filesystem::readablePath($file); $data[$file] = Filesystem::readFile($file); $futures[$file] = PhutilXHPASTBinary::getParserFuture($data[$file]); From 39d415e90eb3111c04cadee78045048bf3c4ec03 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 14:05:47 -0700 Subject: [PATCH 09/21] Move users to modular transactions Summary: Ref T13164. See PHI642. I'd like to provide a third-generation `user.edit` API endpoint and make `user.enable` and `user.disable` obsolete before meddling with policy details, even if it isn't full-fledged yet. Users do already have a transactions table and a Transaction-based editor, but it's only used for editing title, real name, etc. All of these are custom fields, so their support comes in automatically through CustomField extension code. Realign it for modular transactions so new code will be fully modern. There are no actual standalone transaction types yet so this diff is pretty thin. Test Plan: - Grepped for `UserProfileEditor`. - Edited a user's title/real name/icon. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19576 --- src/__phutil_library_map__.php | 8 +++++--- .../controller/PhabricatorPeopleProfileEditController.php | 5 ++--- ...ileEditor.php => PhabricatorUserTransactionEditor.php} | 5 ++--- src/applications/people/storage/PhabricatorUser.php | 2 +- .../people/storage/PhabricatorUserTransaction.php | 6 +++++- .../people/xaction/PhabricatorUserTransactionType.php | 4 ++++ 6 files changed, 19 insertions(+), 11 deletions(-) rename src/applications/people/editor/{PhabricatorUserProfileEditor.php => PhabricatorUserTransactionEditor.php} (73%) create mode 100644 src/applications/people/xaction/PhabricatorUserTransactionType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index bd4cfef6c2..47ea4e8bf1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4583,7 +4583,6 @@ phutil_register_library_map(array( 'PhabricatorUserPreferencesTransaction' => 'applications/settings/storage/PhabricatorUserPreferencesTransaction.php', 'PhabricatorUserPreferencesTransactionQuery' => 'applications/settings/query/PhabricatorUserPreferencesTransactionQuery.php', 'PhabricatorUserProfile' => 'applications/people/storage/PhabricatorUserProfile.php', - 'PhabricatorUserProfileEditor' => 'applications/people/editor/PhabricatorUserProfileEditor.php', 'PhabricatorUserProfileImageCacheType' => 'applications/people/cache/PhabricatorUserProfileImageCacheType.php', 'PhabricatorUserRealNameField' => 'applications/people/customfield/PhabricatorUserRealNameField.php', 'PhabricatorUserRolesField' => 'applications/people/customfield/PhabricatorUserRolesField.php', @@ -4593,6 +4592,8 @@ phutil_register_library_map(array( 'PhabricatorUserTestCase' => 'applications/people/storage/__tests__/PhabricatorUserTestCase.php', 'PhabricatorUserTitleField' => 'applications/people/customfield/PhabricatorUserTitleField.php', 'PhabricatorUserTransaction' => 'applications/people/storage/PhabricatorUserTransaction.php', + 'PhabricatorUserTransactionEditor' => 'applications/people/editor/PhabricatorUserTransactionEditor.php', + 'PhabricatorUserTransactionType' => 'applications/people/xaction/PhabricatorUserTransactionType.php', 'PhabricatorUsersEditField' => 'applications/transactions/editfield/PhabricatorUsersEditField.php', 'PhabricatorUsersPolicyRule' => 'applications/people/policyrule/PhabricatorUsersPolicyRule.php', 'PhabricatorUsersSearchField' => 'applications/people/searchfield/PhabricatorUsersSearchField.php', @@ -10575,7 +10576,6 @@ phutil_register_library_map(array( 'PhabricatorUserPreferencesTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorUserPreferencesTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorUserProfile' => 'PhabricatorUserDAO', - 'PhabricatorUserProfileEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorUserProfileImageCacheType' => 'PhabricatorUserCacheType', 'PhabricatorUserRealNameField' => 'PhabricatorUserCustomField', 'PhabricatorUserRolesField' => 'PhabricatorUserCustomField', @@ -10584,7 +10584,9 @@ phutil_register_library_map(array( 'PhabricatorUserStatusField' => 'PhabricatorUserCustomField', 'PhabricatorUserTestCase' => 'PhabricatorTestCase', 'PhabricatorUserTitleField' => 'PhabricatorUserCustomField', - 'PhabricatorUserTransaction' => 'PhabricatorApplicationTransaction', + 'PhabricatorUserTransaction' => 'PhabricatorModularTransaction', + 'PhabricatorUserTransactionEditor' => 'PhabricatorApplicationTransactionEditor', + 'PhabricatorUserTransactionType' => 'PhabricatorModularTransactionType', 'PhabricatorUsersEditField' => 'PhabricatorTokenizerEditField', 'PhabricatorUsersPolicyRule' => 'PhabricatorPolicyRule', 'PhabricatorUsersSearchField' => 'PhabricatorSearchTokenizerField', diff --git a/src/applications/people/controller/PhabricatorPeopleProfileEditController.php b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php index 3eda8a968b..4876f4495d 100644 --- a/src/applications/people/controller/PhabricatorPeopleProfileEditController.php +++ b/src/applications/people/controller/PhabricatorPeopleProfileEditController.php @@ -38,10 +38,9 @@ final class PhabricatorPeopleProfileEditController new PhabricatorUserTransaction(), $request); - $editor = id(new PhabricatorUserProfileEditor()) + $editor = id(new PhabricatorUserTransactionEditor()) ->setActor($viewer) - ->setContentSource( - PhabricatorContentSource::newFromRequest($request)) + ->setContentSourceFromRequest($request) ->setContinueOnNoEffect(true); try { diff --git a/src/applications/people/editor/PhabricatorUserProfileEditor.php b/src/applications/people/editor/PhabricatorUserTransactionEditor.php similarity index 73% rename from src/applications/people/editor/PhabricatorUserProfileEditor.php rename to src/applications/people/editor/PhabricatorUserTransactionEditor.php index 9ec02ba2af..c0dfa941af 100644 --- a/src/applications/people/editor/PhabricatorUserProfileEditor.php +++ b/src/applications/people/editor/PhabricatorUserTransactionEditor.php @@ -1,6 +1,6 @@ Date: Mon, 13 Aug 2018 14:18:37 -0700 Subject: [PATCH 10/21] Add a modern "user.edit" API method for users Summary: Depends on D19576. Ref T13164. See PHI642. This adds an EditEngine for users and a `user.edit` modern API method. For now, all it supports is editing real name, blurb, title, and icon (same as "Edit Profile" from the UI). Test Plan: - Edited my stuff via the new API method. - Tried to edit another user, got rejected by policies. - Tried to create a user, got rejected by policies. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19577 --- src/__phutil_library_map__.php | 4 ++ .../conduit/UserEditConduitAPIMethod.php | 20 ++++++ .../customfield/PhabricatorUserBlurbField.php | 21 ++++++ .../customfield/PhabricatorUserIconField.php | 21 ++++++ .../PhabricatorUserRealNameField.php | 21 ++++++ .../customfield/PhabricatorUserTitleField.php | 21 ++++++ .../editor/PhabricatorUserEditEngine.php | 70 +++++++++++++++++++ 7 files changed, 178 insertions(+) create mode 100644 src/applications/people/conduit/UserEditConduitAPIMethod.php create mode 100644 src/applications/people/editor/PhabricatorUserEditEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 47ea4e8bf1..628f4958b5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4562,6 +4562,7 @@ phutil_register_library_map(array( 'PhabricatorUserCustomFieldNumericIndex' => 'applications/people/storage/PhabricatorUserCustomFieldNumericIndex.php', 'PhabricatorUserCustomFieldStringIndex' => 'applications/people/storage/PhabricatorUserCustomFieldStringIndex.php', 'PhabricatorUserDAO' => 'applications/people/storage/PhabricatorUserDAO.php', + 'PhabricatorUserEditEngine' => 'applications/people/editor/PhabricatorUserEditEngine.php', 'PhabricatorUserEditor' => 'applications/people/editor/PhabricatorUserEditor.php', 'PhabricatorUserEditorTestCase' => 'applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php', 'PhabricatorUserEmail' => 'applications/people/storage/PhabricatorUserEmail.php', @@ -5233,6 +5234,7 @@ phutil_register_library_map(array( 'TransactionSearchConduitAPIMethod' => 'applications/transactions/conduit/TransactionSearchConduitAPIMethod.php', 'UserConduitAPIMethod' => 'applications/people/conduit/UserConduitAPIMethod.php', 'UserDisableConduitAPIMethod' => 'applications/people/conduit/UserDisableConduitAPIMethod.php', + 'UserEditConduitAPIMethod' => 'applications/people/conduit/UserEditConduitAPIMethod.php', 'UserEnableConduitAPIMethod' => 'applications/people/conduit/UserEnableConduitAPIMethod.php', 'UserFindConduitAPIMethod' => 'applications/people/conduit/UserFindConduitAPIMethod.php', 'UserQueryConduitAPIMethod' => 'applications/people/conduit/UserQueryConduitAPIMethod.php', @@ -10547,6 +10549,7 @@ phutil_register_library_map(array( 'PhabricatorUserCustomFieldNumericIndex' => 'PhabricatorCustomFieldNumericIndexStorage', 'PhabricatorUserCustomFieldStringIndex' => 'PhabricatorCustomFieldStringIndexStorage', 'PhabricatorUserDAO' => 'PhabricatorLiskDAO', + 'PhabricatorUserEditEngine' => 'PhabricatorEditEngine', 'PhabricatorUserEditor' => 'PhabricatorEditor', 'PhabricatorUserEditorTestCase' => 'PhabricatorTestCase', 'PhabricatorUserEmail' => 'PhabricatorUserDAO', @@ -11388,6 +11391,7 @@ phutil_register_library_map(array( 'TransactionSearchConduitAPIMethod' => 'ConduitAPIMethod', 'UserConduitAPIMethod' => 'ConduitAPIMethod', 'UserDisableConduitAPIMethod' => 'UserConduitAPIMethod', + 'UserEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod', 'UserEnableConduitAPIMethod' => 'UserConduitAPIMethod', 'UserFindConduitAPIMethod' => 'UserConduitAPIMethod', 'UserQueryConduitAPIMethod' => 'UserConduitAPIMethod', diff --git a/src/applications/people/conduit/UserEditConduitAPIMethod.php b/src/applications/people/conduit/UserEditConduitAPIMethod.php new file mode 100644 index 0000000000..edc4204c3b --- /dev/null +++ b/src/applications/people/conduit/UserEditConduitAPIMethod.php @@ -0,0 +1,20 @@ +getModernFieldKey(); + } + public function getFieldName() { return pht('Blurb'); } @@ -50,6 +58,11 @@ final class PhabricatorUserBlurbField $this->value = $request->getStr($this->getFieldKey()); } + public function setValueFromStorage($value) { + $this->value = $value; + return $this; + } + public function renderEditControl(array $handles) { return id(new PhabricatorRemarkupControl()) ->setUser($this->getViewer()) @@ -85,4 +98,12 @@ final class PhabricatorUserBlurbField return 'block'; } + public function shouldAppearInConduitTransactions() { + return true; + } + + protected function newConduitEditParameterType() { + return new ConduitStringParameterType(); + } + } diff --git a/src/applications/people/customfield/PhabricatorUserIconField.php b/src/applications/people/customfield/PhabricatorUserIconField.php index a3f43cb7bf..0b25a9dd81 100644 --- a/src/applications/people/customfield/PhabricatorUserIconField.php +++ b/src/applications/people/customfield/PhabricatorUserIconField.php @@ -9,6 +9,14 @@ final class PhabricatorUserIconField return 'user:icon'; } + public function getModernFieldKey() { + return 'icon'; + } + + public function getFieldKeyForConduit() { + return $this->getModernFieldKey(); + } + public function getFieldName() { return pht('Icon'); } @@ -50,6 +58,11 @@ final class PhabricatorUserIconField $this->value = $request->getStr($this->getFieldKey()); } + public function setValueFromStorage($value) { + $this->value = $value; + return $this; + } + public function renderEditControl(array $handles) { return id(new PHUIFormIconSetControl()) ->setName($this->getFieldKey()) @@ -58,4 +71,12 @@ final class PhabricatorUserIconField ->setIconSet(new PhabricatorPeopleIconSet()); } + public function shouldAppearInConduitTransactions() { + return true; + } + + protected function newConduitEditParameterType() { + return new ConduitStringParameterType(); + } + } diff --git a/src/applications/people/customfield/PhabricatorUserRealNameField.php b/src/applications/people/customfield/PhabricatorUserRealNameField.php index 90e490fc3b..e7e610fb07 100644 --- a/src/applications/people/customfield/PhabricatorUserRealNameField.php +++ b/src/applications/people/customfield/PhabricatorUserRealNameField.php @@ -9,6 +9,14 @@ final class PhabricatorUserRealNameField return 'user:realname'; } + public function getModernFieldKey() { + return 'realName'; + } + + public function getFieldKeyForConduit() { + return $this->getModernFieldKey(); + } + public function getFieldName() { return pht('Real Name'); } @@ -53,6 +61,11 @@ final class PhabricatorUserRealNameField $this->value = $request->getStr($this->getFieldKey()); } + public function setValueFromStorage($value) { + $this->value = $value; + return $this; + } + public function renderEditControl(array $handles) { return id(new AphrontFormTextControl()) ->setName($this->getFieldKey()) @@ -65,4 +78,12 @@ final class PhabricatorUserRealNameField return PhabricatorEnv::getEnvConfig('account.editable'); } + public function shouldAppearInConduitTransactions() { + return true; + } + + protected function newConduitEditParameterType() { + return new ConduitStringParameterType(); + } + } diff --git a/src/applications/people/customfield/PhabricatorUserTitleField.php b/src/applications/people/customfield/PhabricatorUserTitleField.php index af6793e76e..5ceb774fa4 100644 --- a/src/applications/people/customfield/PhabricatorUserTitleField.php +++ b/src/applications/people/customfield/PhabricatorUserTitleField.php @@ -9,6 +9,14 @@ final class PhabricatorUserTitleField return 'user:title'; } + public function getModernFieldKey() { + return 'title'; + } + + public function getFieldKeyForConduit() { + return $this->getModernFieldKey(); + } + public function getFieldName() { return pht('Title'); } @@ -50,6 +58,11 @@ final class PhabricatorUserTitleField $this->value = $request->getStr($this->getFieldKey()); } + public function setValueFromStorage($value) { + $this->value = $value; + return $this; + } + public function renderEditControl(array $handles) { return id(new AphrontFormTextControl()) ->setName($this->getFieldKey()) @@ -57,4 +70,12 @@ final class PhabricatorUserTitleField ->setLabel($this->getFieldName()); } + public function shouldAppearInConduitTransactions() { + return true; + } + + protected function newConduitEditParameterType() { + return new ConduitStringParameterType(); + } + } diff --git a/src/applications/people/editor/PhabricatorUserEditEngine.php b/src/applications/people/editor/PhabricatorUserEditEngine.php new file mode 100644 index 0000000000..b978eb2618 --- /dev/null +++ b/src/applications/people/editor/PhabricatorUserEditEngine.php @@ -0,0 +1,70 @@ +getUsername()); + } + + protected function getObjectEditShortText($object) { + return $object->getMonogram(); + } + + protected function getObjectCreateShortText() { + return pht('Create User'); + } + + protected function getObjectName() { + return pht('User'); + } + + protected function getObjectViewURI($object) { + return $object->getURI(); + } + + protected function getCreateNewObjectPolicy() { + // At least for now, forbid creating new users via EditEngine. This is + // primarily enforcing that "user.edit" can not create users via the API. + return PhabricatorPolicies::POLICY_NOONE; + } + + protected function buildCustomEditFields($object) { + return array(); + } + +} From f9673a72a87d61edd69be864491518e83d4645f7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 13 Aug 2018 15:32:24 -0700 Subject: [PATCH 11/21] Allow "user.edit" to enable or disable users Summary: Depends on D19577. Ref T13164. See PHI642. This adds modern transaction-oriented enable/disable support. Currently, this also doesn't let you disable normal users even when you're an administrator. I'll refine the policy model later in this change series, since that's also the goal here (let users set "Can Disable Users" to some more broad set of users than "Administrators"). This also leaves us with two different edit pathways: the old UserEditor one and the new UserTransactionEditor one. The next couple diffs will redefine the other pathways in terms of this pathway. Test Plan: - Enabled/disabled a bot. - Tried to disable another non-bot user. This isn't allowed yet, since even as an administrator you don't have CAN_EDIT on them and currently need it: right now, there's no way for a particular set of transactions to say they can move forward with reduced permissions. - Tried to enable/disable myself. This isn't allowed since you can't enable/disable yourself. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19579 --- src/__phutil_library_map__.php | 2 + .../editor/PhabricatorUserEditEngine.php | 13 +++- .../PhabricatorUserDisableTransaction.php | 72 +++++++++++++++++++ .../PhabricatorUserTransactionType.php | 11 ++- 4 files changed, 96 insertions(+), 2 deletions(-) create mode 100644 src/applications/people/xaction/PhabricatorUserDisableTransaction.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 628f4958b5..c8b84d19fe 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4562,6 +4562,7 @@ phutil_register_library_map(array( 'PhabricatorUserCustomFieldNumericIndex' => 'applications/people/storage/PhabricatorUserCustomFieldNumericIndex.php', 'PhabricatorUserCustomFieldStringIndex' => 'applications/people/storage/PhabricatorUserCustomFieldStringIndex.php', 'PhabricatorUserDAO' => 'applications/people/storage/PhabricatorUserDAO.php', + 'PhabricatorUserDisableTransaction' => 'applications/people/xaction/PhabricatorUserDisableTransaction.php', 'PhabricatorUserEditEngine' => 'applications/people/editor/PhabricatorUserEditEngine.php', 'PhabricatorUserEditor' => 'applications/people/editor/PhabricatorUserEditor.php', 'PhabricatorUserEditorTestCase' => 'applications/people/editor/__tests__/PhabricatorUserEditorTestCase.php', @@ -10549,6 +10550,7 @@ phutil_register_library_map(array( 'PhabricatorUserCustomFieldNumericIndex' => 'PhabricatorCustomFieldNumericIndexStorage', 'PhabricatorUserCustomFieldStringIndex' => 'PhabricatorCustomFieldStringIndexStorage', 'PhabricatorUserDAO' => 'PhabricatorLiskDAO', + 'PhabricatorUserDisableTransaction' => 'PhabricatorUserTransactionType', 'PhabricatorUserEditEngine' => 'PhabricatorEditEngine', 'PhabricatorUserEditor' => 'PhabricatorEditor', 'PhabricatorUserEditorTestCase' => 'PhabricatorTestCase', diff --git a/src/applications/people/editor/PhabricatorUserEditEngine.php b/src/applications/people/editor/PhabricatorUserEditEngine.php index b978eb2618..c547426b12 100644 --- a/src/applications/people/editor/PhabricatorUserEditEngine.php +++ b/src/applications/people/editor/PhabricatorUserEditEngine.php @@ -64,7 +64,18 @@ final class PhabricatorUserEditEngine } protected function buildCustomEditFields($object) { - return array(); + return array( + id(new PhabricatorBoolEditField()) + ->setKey('disabled') + ->setOptions(pht('Active'), pht('Disabled')) + ->setLabel(pht('Disabled')) + ->setDescription(pht('Disable the user.')) + ->setTransactionType(PhabricatorUserDisableTransaction::TRANSACTIONTYPE) + ->setIsConduitOnly(true) + ->setConduitDescription(pht('Disable or enable the user.')) + ->setConduitTypeDescription(pht('True to disable the user.')) + ->setValue($object->getIsDisabled()), + ); } } diff --git a/src/applications/people/xaction/PhabricatorUserDisableTransaction.php b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php new file mode 100644 index 0000000000..0ea6ef36c1 --- /dev/null +++ b/src/applications/people/xaction/PhabricatorUserDisableTransaction.php @@ -0,0 +1,72 @@ +getIsDisabled(); + } + + public function generateNewValue($object, $value) { + return (bool)$value; + } + + public function applyInternalEffects($object, $value) { + $object->setIsDisabled((int)$value); + + $this->newUserLog(PhabricatorUserLog::ACTION_DISABLE) + ->setOldValue((bool)$object->getIsDisabled()) + ->setNewValue((bool)$value) + ->save(); + } + + public function getTitle() { + $new = $this->getNewValue(); + if ($new) { + return pht( + '%s disabled this user.', + $this->renderAuthor()); + } else { + return pht( + '%s enabled this user.', + $this->renderAuthor()); + } + } + + public function getTitleForFeed() { + $new = $this->getNewValue(); + if ($new) { + return pht( + '%s disabled %s.', + $this->renderAuthor(), + $this->renderObject()); + } else { + return pht( + '%s enabled %s.', + $this->renderAuthor(), + $this->renderObject()); + } + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + foreach ($xactions as $xaction) { + $is_disabled = (bool)$object->getIsDisabled(); + + if ((bool)$xaction->getNewValue() === $is_disabled) { + continue; + } + + if ($this->getActingAsPHID() === $object->getPHID()) { + $errors[] = $this->newInvalidError( + pht('You can not enable or disable your own account.')); + } + } + + return $errors; + } + +} diff --git a/src/applications/people/xaction/PhabricatorUserTransactionType.php b/src/applications/people/xaction/PhabricatorUserTransactionType.php index 89392fd039..dcd45d480e 100644 --- a/src/applications/people/xaction/PhabricatorUserTransactionType.php +++ b/src/applications/people/xaction/PhabricatorUserTransactionType.php @@ -1,4 +1,13 @@ getActor(), + $this->getObject()->getPHID(), + $action); + } + +} From 296bf046a8122c451ba0bf1a93733dccccaeb390 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 10:44:52 -0700 Subject: [PATCH 12/21] Remove deprecated Maniphest "Can Edit " capabilities Summary: Depends on D19579. Fixes T10003. These have been deprecated with a setup warning about their impending removal for about two and a half years. Ref T13164. See PHI642. My overall goal here is to simplify how we handle transactions which have special policy behaviors. In particular, I'm hoping to replace `ApplicationTransactionEditor->requireCapabilities()` with a new, more clear policy check. A problem with `requireCapabilities()` is that it doesn't actually enforce any policies in almost all cases: the default is "nothing", not CAN_EDIT. So it ends up looking like it's the right place to specialize policy checks, but it usually isn't. For "Disable", I need to be able to weaken the check selectively (you can disable users if you have the permission, even if you can't edit them otherwise). We have a handful of other edits which work like this (notably, leaving and joining projects) but they're very rare. Test Plan: Grepped for all removed classes. Edited a Maniphest task. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164, T10003 Differential Revision: https://secure.phabricator.com/D19581 --- src/__phutil_library_map__.php | 10 --- .../PhabricatorExtraConfigSetupCheck.php | 65 ------------------- .../PhabricatorManiphestApplication.php | 5 -- .../ManiphestEditAssignCapability.php | 15 ----- .../ManiphestEditPoliciesCapability.php | 16 ----- .../ManiphestEditPriorityCapability.php | 16 ----- .../ManiphestEditProjectsCapability.php | 16 ----- .../ManiphestEditStatusCapability.php | 15 ----- .../editor/ManiphestTransactionEditor.php | 45 ------------- .../query/ManiphestTaskSearchEngine.php | 6 +- 10 files changed, 1 insertion(+), 208 deletions(-) delete mode 100644 src/applications/maniphest/capability/ManiphestEditAssignCapability.php delete mode 100644 src/applications/maniphest/capability/ManiphestEditPoliciesCapability.php delete mode 100644 src/applications/maniphest/capability/ManiphestEditPriorityCapability.php delete mode 100644 src/applications/maniphest/capability/ManiphestEditProjectsCapability.php delete mode 100644 src/applications/maniphest/capability/ManiphestEditStatusCapability.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index c8b84d19fe..af6fa96ba3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1646,13 +1646,8 @@ phutil_register_library_map(array( 'ManiphestDAO' => 'applications/maniphest/storage/ManiphestDAO.php', 'ManiphestDefaultEditCapability' => 'applications/maniphest/capability/ManiphestDefaultEditCapability.php', 'ManiphestDefaultViewCapability' => 'applications/maniphest/capability/ManiphestDefaultViewCapability.php', - 'ManiphestEditAssignCapability' => 'applications/maniphest/capability/ManiphestEditAssignCapability.php', 'ManiphestEditConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestEditConduitAPIMethod.php', 'ManiphestEditEngine' => 'applications/maniphest/editor/ManiphestEditEngine.php', - 'ManiphestEditPoliciesCapability' => 'applications/maniphest/capability/ManiphestEditPoliciesCapability.php', - 'ManiphestEditPriorityCapability' => 'applications/maniphest/capability/ManiphestEditPriorityCapability.php', - 'ManiphestEditProjectsCapability' => 'applications/maniphest/capability/ManiphestEditProjectsCapability.php', - 'ManiphestEditStatusCapability' => 'applications/maniphest/capability/ManiphestEditStatusCapability.php', 'ManiphestEmailCommand' => 'applications/maniphest/command/ManiphestEmailCommand.php', 'ManiphestGetTaskTransactionsConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestGetTaskTransactionsConduitAPIMethod.php', 'ManiphestHovercardEngineExtension' => 'applications/maniphest/engineextension/ManiphestHovercardEngineExtension.php', @@ -7152,13 +7147,8 @@ phutil_register_library_map(array( 'ManiphestDAO' => 'PhabricatorLiskDAO', 'ManiphestDefaultEditCapability' => 'PhabricatorPolicyCapability', 'ManiphestDefaultViewCapability' => 'PhabricatorPolicyCapability', - 'ManiphestEditAssignCapability' => 'PhabricatorPolicyCapability', 'ManiphestEditConduitAPIMethod' => 'PhabricatorEditEngineAPIMethod', 'ManiphestEditEngine' => 'PhabricatorEditEngine', - 'ManiphestEditPoliciesCapability' => 'PhabricatorPolicyCapability', - 'ManiphestEditPriorityCapability' => 'PhabricatorPolicyCapability', - 'ManiphestEditProjectsCapability' => 'PhabricatorPolicyCapability', - 'ManiphestEditStatusCapability' => 'PhabricatorPolicyCapability', 'ManiphestEmailCommand' => 'MetaMTAEmailTransactionCommand', 'ManiphestGetTaskTransactionsConduitAPIMethod' => 'ManiphestConduitAPIMethod', 'ManiphestHovercardEngineExtension' => 'PhabricatorHovercardEngineExtension', diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index 84fd5bedf2..7df50a4524 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -361,69 +361,4 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { return $ancient_config; } - private function executeManiphestFieldChecks() { - $maniphest_appclass = 'PhabricatorManiphestApplication'; - if (!PhabricatorApplication::isClassInstalled($maniphest_appclass)) { - return; - } - - $capabilities = array( - ManiphestEditAssignCapability::CAPABILITY, - ManiphestEditPoliciesCapability::CAPABILITY, - ManiphestEditPriorityCapability::CAPABILITY, - ManiphestEditProjectsCapability::CAPABILITY, - ManiphestEditStatusCapability::CAPABILITY, - ); - - // Check for any of these capabilities set to anything other than - // "All Users". - - $any_set = false; - $app = new PhabricatorManiphestApplication(); - foreach ($capabilities as $capability) { - $setting = $app->getPolicy($capability); - if ($setting != PhabricatorPolicies::POLICY_USER) { - $any_set = true; - break; - } - } - - if (!$any_set) { - return; - } - - $issue_summary = pht( - 'Maniphest is currently configured with deprecated policy settings '. - 'which will be removed in a future version of Phabricator.'); - - - $message = pht( - 'Some policy settings in Maniphest are now deprecated and will be '. - 'removed in a future version of Phabricator. You are currently using '. - 'at least one of these settings.'. - "\n\n". - 'The deprecated settings are "Can Assign Tasks", '. - '"Can Edit Task Policies", "Can Prioritize Tasks", '. - '"Can Edit Task Projects", and "Can Edit Task Status". You can '. - 'find these settings in Applications, or follow the link below.'. - "\n\n". - 'You can find discussion of this change (including rationale and '. - 'recommendations on how to configure similar features) in the upstream, '. - 'at the link below.'. - "\n\n". - 'To resolve this issue, set all of these policies to "All Users" after '. - 'making any necessary form customization changes.'); - - $more_href = 'https://secure.phabricator.com/T10003'; - $edit_href = '/applications/view/PhabricatorManiphestApplication/'; - - $issue = $this->newIssue('maniphest.T10003-per-field-policies') - ->setShortName(pht('Deprecated Policies')) - ->setName(pht('Deprecated Maniphest Field Policies')) - ->setSummary($issue_summary) - ->setMessage($message) - ->addLink($more_href, pht('Learn More: Upstream Discussion')) - ->addLink($edit_href, pht('Edit These Settings')); - } - } diff --git a/src/applications/maniphest/application/PhabricatorManiphestApplication.php b/src/applications/maniphest/application/PhabricatorManiphestApplication.php index e57b9b545e..1ed4096660 100644 --- a/src/applications/maniphest/application/PhabricatorManiphestApplication.php +++ b/src/applications/maniphest/application/PhabricatorManiphestApplication.php @@ -85,11 +85,6 @@ final class PhabricatorManiphestApplication extends PhabricatorApplication { 'template' => ManiphestTaskPHIDType::TYPECONST, 'capability' => PhabricatorPolicyCapability::CAN_EDIT, ), - ManiphestEditStatusCapability::CAPABILITY => array(), - ManiphestEditAssignCapability::CAPABILITY => array(), - ManiphestEditPoliciesCapability::CAPABILITY => array(), - ManiphestEditPriorityCapability::CAPABILITY => array(), - ManiphestEditProjectsCapability::CAPABILITY => array(), ManiphestBulkEditCapability::CAPABILITY => array(), ); } diff --git a/src/applications/maniphest/capability/ManiphestEditAssignCapability.php b/src/applications/maniphest/capability/ManiphestEditAssignCapability.php deleted file mode 100644 index 5da3adf55e..0000000000 --- a/src/applications/maniphest/capability/ManiphestEditAssignCapability.php +++ /dev/null @@ -1,15 +0,0 @@ -setTask($object); } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - parent::requireCapabilities($object, $xaction); - - $app_capability_map = array( - ManiphestTaskPriorityTransaction::TRANSACTIONTYPE => - ManiphestEditPriorityCapability::CAPABILITY, - ManiphestTaskStatusTransaction::TRANSACTIONTYPE => - ManiphestEditStatusCapability::CAPABILITY, - ManiphestTaskOwnerTransaction::TRANSACTIONTYPE => - ManiphestEditAssignCapability::CAPABILITY, - PhabricatorTransactions::TYPE_EDIT_POLICY => - ManiphestEditPoliciesCapability::CAPABILITY, - PhabricatorTransactions::TYPE_VIEW_POLICY => - ManiphestEditPoliciesCapability::CAPABILITY, - ); - - - $transaction_type = $xaction->getTransactionType(); - - $app_capability = null; - if ($transaction_type == PhabricatorTransactions::TYPE_EDGE) { - switch ($xaction->getMetadataValue('edge:type')) { - case PhabricatorProjectObjectHasProjectEdgeType::EDGECONST: - $app_capability = ManiphestEditProjectsCapability::CAPABILITY; - break; - } - } else { - $app_capability = idx($app_capability_map, $transaction_type); - } - - if ($app_capability) { - $app = id(new PhabricatorApplicationQuery()) - ->setViewer($this->getActor()) - ->withClasses(array('PhabricatorManiphestApplication')) - ->executeOne(); - PhabricatorPolicyFilter::requireCapability( - $this->getActor(), - $app, - $app_capability); - } - } - protected function adjustObjectForPolicyChecks( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php index 68f87a6f6b..437362fa41 100644 --- a/src/applications/maniphest/query/ManiphestTaskSearchEngine.php +++ b/src/applications/maniphest/query/ManiphestTaskSearchEngine.php @@ -369,11 +369,7 @@ final class ManiphestTaskSearchEngine $can_edit_priority = false; $can_bulk_edit = false; } else { - $can_edit_priority = PhabricatorPolicyFilter::hasCapability( - $viewer, - $this->getApplication(), - ManiphestEditPriorityCapability::CAPABILITY); - + $can_edit_priority = true; $can_bulk_edit = PhabricatorPolicyFilter::hasCapability( $viewer, $this->getApplication(), From a39852ae1b4a131354a606753495f052ea836e74 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 13:19:53 -0700 Subject: [PATCH 13/21] Remove pointless requireCapabilities() method from PhabricatorProjectColumnTransactionEditor Summary: Depends on D19581. Ref T13164. This method has no effect: - You must always have CAN_EDIT to reach an Editor in the first place. - Per previous change, I'm going to restructure this so transactions explicitly check CAN_EDIT by default anyway. Test Plan: Tried to edit and hide a project column as a user without permission, hit global permission checks long before reaching this method. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19582 --- ...abricatorProjectColumnTransactionEditor.php | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/src/applications/project/editor/PhabricatorProjectColumnTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectColumnTransactionEditor.php index 19f059b8b5..d494767085 100644 --- a/src/applications/project/editor/PhabricatorProjectColumnTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectColumnTransactionEditor.php @@ -137,22 +137,4 @@ final class PhabricatorProjectColumnTransactionEditor return $errors; } - - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case PhabricatorProjectColumnTransaction::TYPE_NAME: - case PhabricatorProjectColumnTransaction::TYPE_STATUS: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - return; - } - - return parent::requireCapabilities($object, $xaction); - } - } From 24d4445845489cb3fa6c9f1e7ab3dfd8dbe40255 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 13:28:37 -0700 Subject: [PATCH 14/21] Remove pointless requireCapabilities() method from PhabricatorRepositoryEditor Summary: Depends on D19582. Ref T13164. It's not possible to reach the editor without passing through a CAN_EDIT check, and it shouldn't be necessarily to manually specify that edits require CAN_EDIT by default. Test Plan: Grepped for `RepositoryEditor`, verified that all callsites pass through a CAN_EDIT check. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19583 --- .../editor/PhabricatorRepositoryEditor.php | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index 768121de5a..882c1a11fa 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -236,40 +236,6 @@ final class PhabricatorRepositoryEditor } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case PhabricatorRepositoryTransaction::TYPE_ACTIVATE: - case PhabricatorRepositoryTransaction::TYPE_NAME: - case PhabricatorRepositoryTransaction::TYPE_DESCRIPTION: - case PhabricatorRepositoryTransaction::TYPE_ENCODING: - case PhabricatorRepositoryTransaction::TYPE_DEFAULT_BRANCH: - case PhabricatorRepositoryTransaction::TYPE_TRACK_ONLY: - case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE_ONLY: - case PhabricatorRepositoryTransaction::TYPE_UUID: - case PhabricatorRepositoryTransaction::TYPE_SVN_SUBPATH: - case PhabricatorRepositoryTransaction::TYPE_VCS: - case PhabricatorRepositoryTransaction::TYPE_NOTIFY: - case PhabricatorRepositoryTransaction::TYPE_AUTOCLOSE: - case PhabricatorRepositoryTransaction::TYPE_PUSH_POLICY: - case PhabricatorRepositoryTransaction::TYPE_DANGEROUS: - case PhabricatorRepositoryTransaction::TYPE_ENORMOUS: - case PhabricatorRepositoryTransaction::TYPE_SLUG: - case PhabricatorRepositoryTransaction::TYPE_SERVICE: - case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_SOURCES: - case PhabricatorRepositoryTransaction::TYPE_SYMBOLS_LANGUAGE: - case PhabricatorRepositoryTransaction::TYPE_STAGING_URI: - case PhabricatorRepositoryTransaction::TYPE_AUTOMATION_BLUEPRINTS: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); - break; - } - } - protected function validateTransaction( PhabricatorLiskDAO $object, $type, From 3b92da22f4333d5bfa050d0da4397648147cee09 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 14:29:19 -0700 Subject: [PATCH 15/21] Move the hierarchical edit policy check in Phriction from requireCapabilities() to validateTransactions() Summary: Depends on D19583. Ref T13164. This continues the work of getting rid of `requireCapabilities()`. This check is valid, but can be a `validateTransactions()` check instead. This is generally more consistent with how other applications work (e.g., creating subprojects). The UI for this isn't terribly great: you get a policy error //after// you try to create the object. But that's how it worked before, so this isn't any worse than it was. The actual policy exception is (very) slightly more clear now (raised against the right object). Test Plan: - Created a child as a user with permission to do so to make sure I didn't break that. - Set edit permission on `a/` to just me, tried to create `a/b/` as another user, got a policy exception since they can't edit the parent. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19584 --- .../editor/PhrictionTransactionEditor.php | 52 ------------------- .../PhrictionDocumentTitleTransaction.php | 23 ++++++++ 2 files changed, 23 insertions(+), 52 deletions(-) diff --git a/src/applications/phriction/editor/PhrictionTransactionEditor.php b/src/applications/phriction/editor/PhrictionTransactionEditor.php index e6bf46c150..b9f2c0e2ca 100644 --- a/src/applications/phriction/editor/PhrictionTransactionEditor.php +++ b/src/applications/phriction/editor/PhrictionTransactionEditor.php @@ -516,58 +516,6 @@ final class PhrictionTransactionEditor } return $error; } - protected function requireCapabilities( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - /* - * New objects have a special case. If a user can't see - * x/y - * then definitely don't let them make some - * x/y/z - * We need to load the direct parent to handle this case. - */ - if ($this->getIsNewObject()) { - $actor = $this->requireActor(); - $parent_doc = null; - $ancestral_slugs = PhabricatorSlug::getAncestry($object->getSlug()); - // No ancestral slugs is "/"; the first person gets to play with "/". - if ($ancestral_slugs) { - $parent = end($ancestral_slugs); - $parent_doc = id(new PhrictionDocumentQuery()) - ->setViewer($actor) - ->withSlugs(array($parent)) - ->executeOne(); - // If the $actor can't see the $parent_doc then they can't create - // the child $object; throw a policy exception. - if (!$parent_doc) { - id(new PhabricatorPolicyFilter()) - ->setViewer($actor) - ->raisePolicyExceptions(true) - ->rejectObject( - $object, - $object->getEditPolicy(), - PhabricatorPolicyCapability::CAN_EDIT); - } - - // If the $actor can't edit the $parent_doc then they can't create - // the child $object; throw a policy exception. - if (!PhabricatorPolicyFilter::hasCapability( - $actor, - $parent_doc, - PhabricatorPolicyCapability::CAN_EDIT)) { - id(new PhabricatorPolicyFilter()) - ->setViewer($actor) - ->raisePolicyExceptions(true) - ->rejectObject( - $object, - $object->getEditPolicy(), - PhabricatorPolicyCapability::CAN_EDIT); - } - } - } - return parent::requireCapabilities($object, $xaction); - } protected function supportsSearch() { return true; diff --git a/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php b/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php index 4f1ba850a7..5c97d1bec8 100644 --- a/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php +++ b/src/applications/phriction/xaction/PhrictionDocumentTitleTransaction.php @@ -91,6 +91,29 @@ final class PhrictionDocumentTitleTransaction pht('Documents must have a title.')); } + if ($this->isNewObject()) { + // No ancestral slugs is "/". No ancestry checks apply when creating the + // root document. + $ancestral_slugs = PhabricatorSlug::getAncestry($object->getSlug()); + if ($ancestral_slugs) { + // You must be able to view and edit the parent document to create a new + // child. + $parent_document = id(new PhrictionDocumentQuery()) + ->setViewer($this->getActor()) + ->withSlugs(array(last($ancestral_slugs))) + ->requireCapabilities( + array( + PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, + )) + ->executeOne(); + if (!$parent_document) { + $errors[] = $this->newInvalidError( + pht('You can not create a document which does not have a parent.')); + } + } + } + return $errors; } From 7e29ec2e2a66087528027b7e80cc548ec30f6fff Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 14 Aug 2018 14:46:59 -0700 Subject: [PATCH 16/21] Move the "Can Lock Projects" check from requireCapabilities() to transaction validation Summary: Depends on D19584. Ref T13164. This check is an //extra// check: you need EDIT //and// this capability. Thus, we can do it in validation without issues. Test Plan: - This code isn't reachable today: all methods of applying this transaction do a separate check for "Can Lock" upfront. - Commented out the "Can Lock" check in the LockController, tried to lock as a user without permission. Was rejected with a policy exception. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19585 --- .../editor/PhabricatorProjectTransactionEditor.php | 6 ------ .../xaction/PhabricatorProjectLockTransaction.php | 8 ++++++++ .../storage/PhabricatorModularTransactionType.php | 10 ++++++++++ 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php index 2a90d6ce29..ef0cc65c8e 100644 --- a/src/applications/project/editor/PhabricatorProjectTransactionEditor.php +++ b/src/applications/project/editor/PhabricatorProjectTransactionEditor.php @@ -120,12 +120,6 @@ final class PhabricatorProjectTransactionEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case PhabricatorProjectLockTransaction::TRANSACTIONTYPE: - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - newv($this->getEditorApplicationClass(), array()), - ProjectCanLockProjectsCapability::CAPABILITY); - return; case PhabricatorTransactions::TYPE_EDGE: switch ($xaction->getMetadataValue('edge:type')) { case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST: diff --git a/src/applications/project/xaction/PhabricatorProjectLockTransaction.php b/src/applications/project/xaction/PhabricatorProjectLockTransaction.php index 42551dfb2c..c54c9284ce 100644 --- a/src/applications/project/xaction/PhabricatorProjectLockTransaction.php +++ b/src/applications/project/xaction/PhabricatorProjectLockTransaction.php @@ -53,4 +53,12 @@ final class PhabricatorProjectLockTransaction } } + public function validateTransactions($object, array $xactions) { + if ($xactions) { + $this->requireApplicationCapability( + ProjectCanLockProjectsCapability::CAPABILITY); + } + return array(); + } + } diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index eaf7d029ce..b0714aeccf 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -356,4 +356,14 @@ abstract class PhabricatorModularTransactionType return array(); } + protected function requireApplicationCapability($capability) { + $application_class = $this->getEditor()->getEditorApplicationClass(); + $application = newv($application_class, array()); + + PhabricatorPolicyFilter::requireCapability( + $this->getActor(), + $application, + $capability); + } + } From 0ccf1410e057cede3e2c2a552cb78981d8ac8179 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 16 Aug 2018 10:07:28 -0700 Subject: [PATCH 17/21] Give PhabricatorAuthPassword a formal CAN_EDIT policy Summary: Depends on D19585. Ref T13164. This is a precursor for D19586, which causes Editors to start doing more explicit CAN_EDIT checks. Passwords have an Editor, but don't actually define a CAN_EDIT capability. Define one (you can edit a password if you can edit the object the password is associated with). (Today, this object is always a User -- this table just unified VCS passwords and Account passwords so they can be handled more consistently.) Test Plan: - With D19586, ran unit tests and got a pass. - Edited my own password. - Tried to edit another user's password and wasn't permitted to. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19592 --- src/applications/auth/storage/PhabricatorAuthPassword.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/applications/auth/storage/PhabricatorAuthPassword.php b/src/applications/auth/storage/PhabricatorAuthPassword.php index 5343e622fd..3bcb95693e 100644 --- a/src/applications/auth/storage/PhabricatorAuthPassword.php +++ b/src/applications/auth/storage/PhabricatorAuthPassword.php @@ -178,6 +178,7 @@ final class PhabricatorAuthPassword public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, + PhabricatorPolicyCapability::CAN_EDIT, ); } @@ -195,7 +196,7 @@ final class PhabricatorAuthPassword public function getExtendedPolicy($capability, PhabricatorUser $viewer) { return array( - array($this->getObject(), PhabricatorPolicyCapability::CAN_VIEW), + array($this->getObject(), $capability), ); } From a48e6897a4180ec074f4581c677448b2ac400812 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 16 Aug 2018 11:07:37 -0700 Subject: [PATCH 18/21] Remove obsolete setup check call to Maniphest "Can Edit " field checks Summary: Ref T13164. Missed this in D19581. Test Plan: - Forced setup checks to re-run by visiting {nav Config > Setup Issues} explicitly. - Before patch: fatal on call to nonexistent method. - After patch: setup issues. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19593 --- .../config/check/PhabricatorExtraConfigSetupCheck.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index 7df50a4524..4bf51602ba 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -84,8 +84,6 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { $issue->addPhabricatorConfig($key); } } - - $this->executeManiphestFieldChecks(); } /** From 438edde03150984b267347b0374998149f2c45e9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 16 Aug 2018 13:48:42 -0700 Subject: [PATCH 19/21] Add some missing aural button labels for accessibility Summary: Ref T13164. See PHI823. (See that issue for some more details and discussion.) Add aural labels to various buttons which were missing reasonable aural labels. The "Search" button (magnifying glass in the global search input) had an entire menu thing inside it. I moved that one level up and it doesn't look like it broke anything (?). All the other changes are pretty straightforward. Test Plan: {F5806497} {F5806498} - Will follow up on the issue to make sure things are in better shape for the reporting user. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19594 --- ...abricatorFavoritesMainMenuBarExtension.php | 3 +- .../PeopleMainMenuBarExtension.php | 3 +- ...catorApplicationTransactionCommentView.php | 2 +- .../view/PHUIDiffInlineCommentDetailView.php | 15 ++++++--- .../menu/PhabricatorMainMenuSearchView.php | 32 +++++++++++-------- .../page/menu/PhabricatorMainMenuView.php | 3 +- src/view/phui/PHUIButtonView.php | 25 +++++++++++++-- src/view/phui/PHUIHeadThingView.php | 2 +- src/view/phui/PHUITimelineEventView.php | 2 +- 9 files changed, 60 insertions(+), 27 deletions(-) diff --git a/src/applications/favorites/engineextension/PhabricatorFavoritesMainMenuBarExtension.php b/src/applications/favorites/engineextension/PhabricatorFavoritesMainMenuBarExtension.php index 9afb836b94..7b5d6d0720 100644 --- a/src/applications/favorites/engineextension/PhabricatorFavoritesMainMenuBarExtension.php +++ b/src/applications/favorites/engineextension/PhabricatorFavoritesMainMenuBarExtension.php @@ -30,7 +30,8 @@ final class PhabricatorFavoritesMainMenuBarExtension ->addClass('phabricator-core-user-menu') ->setNoCSS(true) ->setDropdown(true) - ->setDropdownMenu($dropdown); + ->setDropdownMenu($dropdown) + ->setAuralLabel(pht('Favorites Menu')); return array( $favorites_menu, diff --git a/src/applications/people/engineextension/PeopleMainMenuBarExtension.php b/src/applications/people/engineextension/PeopleMainMenuBarExtension.php index 152fb2becf..e19cedd305 100644 --- a/src/applications/people/engineextension/PeopleMainMenuBarExtension.php +++ b/src/applications/people/engineextension/PeopleMainMenuBarExtension.php @@ -43,7 +43,8 @@ final class PeopleMainMenuBarExtension ->setIcon($profile_image) ->addClass('phabricator-core-user-menu') ->setHasCaret(true) - ->setNoCSS(true); + ->setNoCSS(true) + ->setAuralLabel(pht('Account Menu')); return array( $user_menu, diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php index a5dcfcb4ae..c0d46e21a5 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php @@ -230,7 +230,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { 'div', array( 'style' => 'background-image: url('.$image_uri.')', - 'class' => 'phui-comment-image', + 'class' => 'phui-comment-image visual-only', )); $wedge = phutil_tag( 'div', diff --git a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php index 27a489a705..a32197059a 100644 --- a/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php +++ b/src/infrastructure/diff/view/PHUIDiffInlineCommentDetailView.php @@ -233,7 +233,8 @@ final class PHUIDiffInlineCommentDetailView ->setIcon('fa-reply') ->setTooltip(pht('Reply')) ->addSigil('differential-inline-reply') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Reply')); } if ($this->editable && !$this->preview) { @@ -242,14 +243,16 @@ final class PHUIDiffInlineCommentDetailView ->setIcon('fa-pencil') ->setTooltip(pht('Edit')) ->addSigil('differential-inline-edit') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Edit')); $action_buttons[] = id(new PHUIButtonView()) ->setTag('a') ->setIcon('fa-trash-o') ->setTooltip(pht('Delete')) ->addSigil('differential-inline-delete') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Delete')); } else if ($this->preview) { $links[] = javelin_tag( @@ -268,7 +271,8 @@ final class PHUIDiffInlineCommentDetailView ->setTooltip(pht('Delete')) ->setIcon('fa-trash-o') ->addSigil('differential-inline-delete') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Delete')); } if (!$this->preview && $this->canHide()) { @@ -277,7 +281,8 @@ final class PHUIDiffInlineCommentDetailView ->setTooltip(pht('Collapse')) ->setIcon('fa-times') ->addSigil('hide-inline') - ->setMustCapture(true); + ->setMustCapture(true) + ->setAuralLabel(pht('Collapse')); } $done_button = null; diff --git a/src/view/page/menu/PhabricatorMainMenuSearchView.php b/src/view/page/menu/PhabricatorMainMenuSearchView.php index 0b2ca8ab10..15319a357e 100644 --- a/src/view/page/menu/PhabricatorMainMenuSearchView.php +++ b/src/view/page/menu/PhabricatorMainMenuSearchView.php @@ -94,21 +94,24 @@ final class PhabricatorMainMenuSearchView extends AphrontView { 'action' => '/search/', 'method' => 'POST', ), - phutil_tag_div('phabricator-main-menu-search-container', array( - $input, - phutil_tag( - 'button', - array( - 'id' => $button_id, - 'class' => 'phui-icon-view phui-font-fa fa-search', + phutil_tag( + 'div', + array( + 'class' => 'phabricator-main-menu-search-container', + ), + array( + $input, + phutil_tag( + 'button', + array( + 'id' => $button_id, + 'class' => 'phui-icon-view phui-font-fa fa-search', ), - array( - $selector, - $search_text, - )), - $primary_input, - $target, - ))); + $search_text), + $selector, + $primary_input, + $target, + ))); return $form; } @@ -207,6 +210,7 @@ final class PhabricatorMainMenuSearchView extends AphrontView { id(new PHUIIconView()) ->addSigil('global-search-dropdown-icon') ->setIcon($current_icon)) + ->setAuralLabel(pht('Configure Global Search')) ->setDropdown(true); $input = javelin_tag( diff --git a/src/view/page/menu/PhabricatorMainMenuView.php b/src/view/page/menu/PhabricatorMainMenuView.php index 583952b0a9..67badce911 100644 --- a/src/view/page/menu/PhabricatorMainMenuView.php +++ b/src/view/page/menu/PhabricatorMainMenuView.php @@ -232,7 +232,8 @@ final class PhabricatorMainMenuView extends AphrontView { ->addClass('phabricator-core-user-menu') ->addClass('phabricator-core-user-mobile-menu') ->setNoCSS(true) - ->setDropdownMenu($dropdown); + ->setDropdownMenu($dropdown) + ->setAuralLabel(pht('Page Menu')); } private function renderApplicationMenu() { diff --git a/src/view/phui/PHUIButtonView.php b/src/view/phui/PHUIButtonView.php index 91c336eaaf..1fb2206428 100644 --- a/src/view/phui/PHUIButtonView.php +++ b/src/view/phui/PHUIButtonView.php @@ -31,6 +31,7 @@ final class PHUIButtonView extends AphrontTagView { private $noCSS; private $hasCaret; private $buttonType = self::BUTTONTYPE_DEFAULT; + private $auralLabel; public function setName($name) { $this->name = $name; @@ -123,6 +124,15 @@ final class PHUIButtonView extends AphrontTagView { return $this->buttonType; } + public function setAuralLabel($aural_label) { + $this->auralLabel = $aural_label; + return $this; + } + + public function getAuralLabel() { + return $this->auralLabel; + } + public function setIcon($icon, $first = true) { if (!($icon instanceof PHUIIconView)) { $icon = id(new PHUIIconView()) @@ -265,10 +275,21 @@ final class PHUIButtonView extends AphrontTagView { $caret = phutil_tag('span', array('class' => 'caret'), ''); } + $aural = null; + if ($this->auralLabel !== null) { + $aural = phutil_tag( + 'span', + array( + 'class' => 'aural-only', + ), + $this->auralLabel); + } + + if ($this->iconFirst == true) { - return array($icon, $text, $caret); + return array($aural, $icon, $text, $caret); } else { - return array($text, $icon, $caret); + return array($aural, $text, $icon, $caret); } } } diff --git a/src/view/phui/PHUIHeadThingView.php b/src/view/phui/PHUIHeadThingView.php index dd788399d4..ab2feee984 100644 --- a/src/view/phui/PHUIHeadThingView.php +++ b/src/view/phui/PHUIHeadThingView.php @@ -55,7 +55,7 @@ final class PHUIHeadThingView extends AphrontTagView { $image = phutil_tag( 'a', array( - 'class' => 'phui-head-thing-image', + 'class' => 'phui-head-thing-image visual-only', 'style' => 'background-image: url('.$this->image.');', 'href' => $this->imageHref, )); diff --git a/src/view/phui/PHUITimelineEventView.php b/src/view/phui/PHUITimelineEventView.php index e64b9b06b2..e44332c1b5 100644 --- a/src/view/phui/PHUITimelineEventView.php +++ b/src/view/phui/PHUITimelineEventView.php @@ -404,7 +404,7 @@ final class PHUITimelineEventView extends AphrontView { ($this->userHandle->getURI()) ? 'a' : 'div', array( 'style' => 'background-image: url('.$image_uri.')', - 'class' => 'phui-timeline-image', + 'class' => 'phui-timeline-image visual-only', 'href' => $this->userHandle->getURI(), ), ''); From 5c4c593af32578a406857d2433c1be2a74403128 Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Fri, 17 Aug 2018 12:23:59 -0700 Subject: [PATCH 20/21] Update DiffusionLastModifiedController to use identities Summary: Ref T12164. Updates another controller to use identities. Test Plan: Pretty ad-hoc, but loaded the main pages of several different repos with and without repo identities. I'm not totally convinced the `author` from this data structure is actually being used: ``` $return = array( 'commit' => $modified, 'date' => $date, 'author' => $author, 'details' => $details, ); ``` Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T12164 Differential Revision: https://secure.phabricator.com/D19580 --- .../controller/DiffusionBlameController.php | 8 ++-- .../DiffusionLastModifiedController.php | 47 ++++++------------- .../PhabricatorRepositoryIdentityPHIDType.php | 1 + .../storage/PhabricatorRepositoryCommit.php | 4 +- 4 files changed, 21 insertions(+), 39 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBlameController.php b/src/applications/diffusion/controller/DiffusionBlameController.php index 85ab393ef6..42cabbc0d9 100644 --- a/src/applications/diffusion/controller/DiffusionBlameController.php +++ b/src/applications/diffusion/controller/DiffusionBlameController.php @@ -24,6 +24,7 @@ final class DiffusionBlameController extends DiffusionController { ->setViewer($viewer) ->withRepository($repository) ->withIdentifiers($identifiers) + ->needIdentities(true) ->execute(); $commits = mpull($commits, null, 'getCommitIdentifier'); } else { @@ -68,10 +69,7 @@ final class DiffusionBlameController extends DiffusionController { $handle_phids = array(); foreach ($commits as $commit) { - $author_phid = $commit->getAuthorPHID(); - if ($author_phid) { - $handle_phids[] = $author_phid; - } + $handle_phids[] = $commit->getAuthorDisplayPHID(); } foreach ($revisions as $revision) { @@ -117,7 +115,7 @@ final class DiffusionBlameController extends DiffusionController { $author_phid = null; if ($commit) { - $author_phid = $commit->getAuthorPHID(); + $author_phid = $commit->getAuthorDisplayPHID(); } if (!$author_phid && $revision) { diff --git a/src/applications/diffusion/controller/DiffusionLastModifiedController.php b/src/applications/diffusion/controller/DiffusionLastModifiedController.php index 945f8a58b5..1a31d3a2ba 100644 --- a/src/applications/diffusion/controller/DiffusionLastModifiedController.php +++ b/src/applications/diffusion/controller/DiffusionLastModifiedController.php @@ -35,6 +35,7 @@ final class DiffusionLastModifiedController extends DiffusionController { ->withRepository($drequest->getRepository()) ->withIdentifiers(array_values($modified_map)) ->needCommitData(true) + ->needIdentities(true) ->execute(); $commit_map = mpull($commit_map, null, 'getCommitIdentifier'); } else { @@ -54,9 +55,8 @@ final class DiffusionLastModifiedController extends DiffusionController { $phids = array(); foreach ($commits as $commit) { - $data = $commit->getCommitData(); - $phids[] = $data->getCommitDetail('authorPHID'); - $phids[] = $data->getCommitDetail('committerPHID'); + $phids[] = $commit->getCommitterDisplayPHID(); + $phids[] = $commit->getAuthorDisplayPHID(); } $phids = array_filter($phids); $handles = $this->loadViewerHandles($phids); @@ -110,38 +110,21 @@ final class DiffusionLastModifiedController extends DiffusionController { $date = ''; } - $data = $commit->getCommitData(); - if ($data) { - $author_phid = $data->getCommitDetail('authorPHID'); - if ($author_phid && isset($handles[$author_phid])) { - $author = $handles[$author_phid]->renderLink(); - } else { - $author = DiffusionView::renderName($data->getAuthorName()); - } + $author = $commit->renderAuthor($viewer, $handles); + $committer = $commit->renderCommitter($viewer, $handles); - $committer = $data->getCommitDetail('committer'); - if ($committer) { - $committer_phid = $data->getCommitDetail('committerPHID'); - if ($committer_phid && isset($handles[$committer_phid])) { - $committer = $handles[$committer_phid]->renderLink(); - } else { - $committer = DiffusionView::renderName($committer); - } - if ($author != $committer) { - $author = hsprintf('%s/%s', $author, $committer); - } - } - - $details = DiffusionView::linkDetail( - $drequest->getRepository(), - $commit->getCommitIdentifier(), - $data->getSummary()); - $details = AphrontTableView::renderSingleDisplayLine($details); - } else { - $author = ''; - $details = ''; + if ($author != $committer) { + $author = hsprintf('%s/%s', $author, $committer); } + $data = $commit->getCommitData(); + $details = DiffusionView::linkDetail( + $drequest->getRepository(), + $commit->getCommitIdentifier(), + $data->getSummary()); + $details = AphrontTableView::renderSingleDisplayLine($details); + + $return = array( 'commit' => $modified, 'date' => $date, diff --git a/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php b/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php index 5bb4d5b907..5d9b06e0a3 100644 --- a/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php +++ b/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php @@ -39,6 +39,7 @@ final class PhabricatorRepositoryIdentityPHIDType $handle->setObjectName(pht('Identity %d', $id)); $handle->setName($name); $handle->setURI($identity->getURI()); + $handle->setIcon('fa-user'); } } diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index bf3b796a76..4cb9def346 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -464,7 +464,7 @@ final class PhabricatorRepositoryCommit $data = $this->getCommitData(); $committer_name = $data->getCommitDetail('committer'); if (strlen($committer_name)) { - return $committer_name; + return DiffusionView::renderName($committer_name); } return null; @@ -479,7 +479,7 @@ final class PhabricatorRepositoryCommit $data = $this->getCommitData(); $author_name = $data->getAuthorName(); if (strlen($author_name)) { - return $author_name; + return DiffusionView::renderName($author_name); } return null; From 75a5dd8d8cc0ba8cdf274ab509284ab4f7c0577c Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 17 Aug 2018 10:53:28 -0700 Subject: [PATCH 21/21] Add more accessibility labels for screen readers Summary: Depends on D19594. See PHI823. Ref T13164. - Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers". - Add a label for the floating header display options menu in Differential. - Add `role="button"` to `PHUIButtonView` objects that we render with an `` tag. Test Plan: Viewed a revision with `?__aural__=true`: - Saw "Remove Action: ..." label. - Saw "Display Options" label. - Used inspector to verify that some `` now have ``. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19595 --- resources/celerity/map.php | 48 +++++++++---------- .../view/DifferentialChangesetListView.php | 1 + ...catorApplicationTransactionCommentView.php | 6 ++- src/view/phui/PHUIButtonView.php | 8 ++++ .../js/application/diff/DiffChangesetList.js | 7 +-- .../transactions/behavior-comment-actions.js | 5 +- webroot/rsrc/js/phuix/PHUIXButtonView.js | 25 ++++++++++ 7 files changed, 71 insertions(+), 29 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 2b81560e43..c60c2f7e85 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -12,7 +12,7 @@ return array( 'core.pkg.css' => 'f515619b', 'core.pkg.js' => '2058ec09', 'differential.pkg.css' => '06dc617c', - 'differential.pkg.js' => '11a08e85', + 'differential.pkg.js' => 'c1cfa143', 'diffusion.pkg.css' => 'a2d17c7d', 'diffusion.pkg.js' => '6134c5a1', 'maniphest.pkg.css' => '4845691a', @@ -373,7 +373,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375', 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63', 'rsrc/js/application/diff/DiffChangeset.js' => 'b49b59d6', - 'rsrc/js/application/diff/DiffChangesetList.js' => '7b95a80a', + 'rsrc/js/application/diff/DiffChangesetList.js' => 'e0b984b5', 'rsrc/js/application/diff/DiffInline.js' => 'e83d28f3', 'rsrc/js/application/diff/behavior-preview-link.js' => '051c7832', 'rsrc/js/application/differential/behavior-comment-preview.js' => '51c5ad07', @@ -423,7 +423,7 @@ return array( 'rsrc/js/application/search/behavior-reorder-profile-menu-items.js' => 'e2e0a072', 'rsrc/js/application/search/behavior-reorder-queries.js' => 'e9581f08', 'rsrc/js/application/slowvote/behavior-slowvote-embed.js' => '887ad43f', - 'rsrc/js/application/transactions/behavior-comment-actions.js' => '9a6dd75c', + 'rsrc/js/application/transactions/behavior-comment-actions.js' => '54110499', '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' => '8f29b364', @@ -506,7 +506,7 @@ return array( 'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8', 'rsrc/js/phuix/PHUIXActionView.js' => '8d4a8c72', 'rsrc/js/phuix/PHUIXAutocomplete.js' => 'df1bbd34', - 'rsrc/js/phuix/PHUIXButtonView.js' => '8a91e1ac', + 'rsrc/js/phuix/PHUIXButtonView.js' => '85ac9772', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => '04b2ae03', 'rsrc/js/phuix/PHUIXExample.js' => '68af71ca', 'rsrc/js/phuix/PHUIXFormControl.js' => '210a16c1', @@ -575,7 +575,7 @@ return array( 'javelin-behavior-bulk-job-reload' => 'edf8a145', 'javelin-behavior-calendar-month-view' => 'fe33e256', 'javelin-behavior-choose-control' => '327a00d1', - 'javelin-behavior-comment-actions' => '9a6dd75c', + 'javelin-behavior-comment-actions' => '54110499', 'javelin-behavior-config-reorder-fields' => 'b6993408', 'javelin-behavior-conpherence-menu' => '4047cd35', 'javelin-behavior-conpherence-participant-pane' => 'd057e45a', @@ -752,7 +752,7 @@ return array( 'phabricator-darkmessage' => 'c48cccdd', 'phabricator-dashboard-css' => 'fe5b1869', 'phabricator-diff-changeset' => 'b49b59d6', - 'phabricator-diff-changeset-list' => '7b95a80a', + 'phabricator-diff-changeset-list' => 'e0b984b5', 'phabricator-diff-inline' => 'e83d28f3', 'phabricator-drag-and-drop-file-upload' => '58dea2fa', 'phabricator-draggable-list' => 'bea6e7f4', @@ -858,7 +858,7 @@ return array( 'phuix-action-list-view' => 'b5c256b8', 'phuix-action-view' => '8d4a8c72', 'phuix-autocomplete' => 'df1bbd34', - 'phuix-button-view' => '8a91e1ac', + 'phuix-button-view' => '85ac9772', 'phuix-dropdown-menu' => '04b2ae03', 'phuix-form-control-view' => '210a16c1', 'phuix-icon-view' => 'bff6884b', @@ -1251,6 +1251,15 @@ return array( 'javelin-vector', 'javelin-typeahead-static-source', ), + 54110499 => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-workflow', + 'javelin-dom', + 'phuix-form-control-view', + 'phuix-icon-view', + 'javelin-behavior-phabricator-gesture', + ), '549459b8' => array( 'javelin-behavior', ), @@ -1500,10 +1509,6 @@ return array( 'owners-path-editor', 'javelin-behavior', ), - '7b95a80a' => array( - 'javelin-install', - 'phuix-button-view', - ), '7cbe244b' => array( 'javelin-install', 'javelin-util', @@ -1533,6 +1538,10 @@ return array( 'javelin-dom', 'javelin-stratcom', ), + '85ac9772' => array( + 'javelin-install', + 'javelin-dom', + ), '85ee8ce6' => array( 'aphront-dialog-view-css', ), @@ -1560,10 +1569,6 @@ return array( 'javelin-install', 'javelin-dom', ), - '8a91e1ac' => array( - 'javelin-install', - 'javelin-dom', - ), '8ce821c5' => array( 'phabricator-notification', 'javelin-stratcom', @@ -1640,15 +1645,6 @@ return array( 'javelin-mask', 'phabricator-drag-and-drop-file-upload', ), - '9a6dd75c' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-workflow', - 'javelin-dom', - 'phuix-form-control-view', - 'phuix-icon-view', - 'javelin-behavior-phabricator-gesture', - ), '9a860428' => array( 'javelin-behavior', 'javelin-dom', @@ -2021,6 +2017,10 @@ return array( 'phuix-icon-view', 'phabricator-prefab', ), + 'e0b984b5' => array( + 'javelin-install', + 'phuix-button-view', + ), 'e1d25dfb' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/applications/differential/view/DifferentialChangesetListView.php b/src/applications/differential/view/DifferentialChangesetListView.php index 2ed963bae3..14de553e59 100644 --- a/src/applications/differential/view/DifferentialChangesetListView.php +++ b/src/applications/differential/view/DifferentialChangesetListView.php @@ -309,6 +309,7 @@ final class DifferentialChangesetListView extends AphrontView { 'Show All Inlines' => pht('Show All Inlines'), 'List Inline Comments' => pht('List Inline Comments'), + 'Display Options' => pht('Display Options'), 'Hide or show all inline comments.' => pht('Hide or show all inline comments.'), diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php index c0d46e21a5..7469004e1b 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php @@ -319,14 +319,18 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { foreach ($comment_actions as $key => $comment_action) { $key = $comment_action->getKey(); + $label = $comment_action->getLabel(); + + $action_map[$key] = array( 'key' => $key, - 'label' => $comment_action->getLabel(), + 'label' => $label, 'type' => $comment_action->getPHUIXControlType(), 'spec' => $comment_action->getPHUIXControlSpecification(), 'initialValue' => $comment_action->getInitialValue(), 'groupKey' => $comment_action->getGroupKey(), 'conflictKey' => $comment_action->getConflictKey(), + 'auralLabel' => pht('Remove Action: %s', $label), ); $type_map[$key] = $comment_action; diff --git a/src/view/phui/PHUIButtonView.php b/src/view/phui/PHUIButtonView.php index 1fb2206428..bad24122bc 100644 --- a/src/view/phui/PHUIButtonView.php +++ b/src/view/phui/PHUIButtonView.php @@ -233,6 +233,13 @@ final class PHUIButtonView extends AphrontTagView { $classes = array(); } + // See PHI823. If we aren't rendering a "