From e2c75d5dc214fcccab172c7af9ad5f31492eddd9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 16 Jan 2012 16:54:05 -0800 Subject: [PATCH] Improve Differential handling of disabled users Summary: We currently allow you to assign code review to disabled users, but should not. Test Plan: - Created revisions with no reviewers and only disabled reviewers, was appropriately warned. - Looked at a disabled user handle link, was clearly informed. - Tried to create a new revision with a disabled reviewer, was rebuffed. Reviewers: btrahan, jungejason Reviewed By: btrahan CC: aran Differential Revision: https://secure.phabricator.com/D1429 --- src/__celerity_resource_map__.php | 126 +++++++++--------- src/aphront/response/base/AphrontResponse.php | 12 +- .../DifferentialRevisionViewController.php | 23 ++++ .../base/DifferentialFieldSpecification.php | 10 +- .../PhabricatorPeopleProfileController.php | 8 ++ .../phid/handle/PhabricatorObjectHandle.php | 9 +- .../application/base/standard-page-view.css | 5 + 7 files changed, 118 insertions(+), 75 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index cfbd94d450..ed0e099e03 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -330,6 +330,17 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/js/javelin/lib/behavior.js', ), + 0 => + array( + 'uri' => '/res/b6096fdd/rsrc/js/javelin/lib/__tests__/URI.js', + 'type' => 'js', + 'requires' => + array( + 0 => 'javelin-uri', + 1 => 'javelin-php-serializer', + ), + 'disk' => '/rsrc/js/javelin/lib/__tests__/URI.js', + ), 'javelin-behavior-aphront-basic-tokenizer' => array( 'uri' => '/res/9be30797/rsrc/js/application/core/behavior-tokenizer.js', @@ -1427,6 +1438,18 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/css/application/objectselector/object-selector.css', ), + 'phabricator-prefab' => + array( + 'uri' => '/res/5784a112/rsrc/js/application/core/Prefab.js', + 'type' => 'js', + 'requires' => + array( + 0 => 'javelin-install', + 1 => 'javelin-util', + 2 => 'javelin-dom', + ), + 'disk' => '/rsrc/js/application/core/Prefab.js', + ), 'phabricator-profile-css' => array( 'uri' => '/res/9869d10b/rsrc/css/application/profile/profile-view.css', @@ -1445,29 +1468,6 @@ celerity_register_resource_map(array( ), 'disk' => '/rsrc/css/application/profile/profile-header-view.css', ), - 0 => - array( - 'uri' => '/res/b6096fdd/rsrc/js/javelin/lib/__tests__/URI.js', - 'type' => 'js', - 'requires' => - array( - 0 => 'javelin-uri', - 1 => 'javelin-php-serializer', - ), - 'disk' => '/rsrc/js/javelin/lib/__tests__/URI.js', - ), - 'phabricator-prefab' => - array( - 'uri' => '/res/5784a112/rsrc/js/application/core/Prefab.js', - 'type' => 'js', - 'requires' => - array( - 0 => 'javelin-install', - 1 => 'javelin-util', - 2 => 'javelin-dom', - ), - 'disk' => '/rsrc/js/application/core/Prefab.js', - ), 'phabricator-remarkup-css' => array( 'uri' => '/res/39f358b8/rsrc/css/core/remarkup.css', @@ -1509,7 +1509,7 @@ celerity_register_resource_map(array( ), 'phabricator-standard-page-view' => array( - 'uri' => '/res/827b93f8/rsrc/css/application/base/standard-page-view.css', + 'uri' => '/res/ec5acaed/rsrc/css/application/base/standard-page-view.css', 'type' => 'css', 'requires' => array( @@ -1784,30 +1784,6 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/b164acea/javelin.pkg.js', 'type' => 'js', ), - 'bb4d65a4' => - array( - 'name' => 'core.pkg.css', - 'symbols' => - array( - 0 => 'phabricator-core-css', - 1 => 'phabricator-core-buttons-css', - 2 => 'phabricator-standard-page-view', - 3 => 'aphront-dialog-view-css', - 4 => 'aphront-form-view-css', - 5 => 'aphront-panel-view-css', - 6 => 'aphront-side-nav-view-css', - 7 => 'aphront-table-view-css', - 8 => 'aphront-crumbs-view-css', - 9 => 'aphront-tokenizer-control-css', - 10 => 'aphront-typeahead-control-css', - 11 => 'aphront-list-filter-view-css', - 12 => 'phabricator-directory-css', - 13 => 'phabricator-remarkup-css', - 14 => 'syntax-highlighting-css', - ), - 'uri' => '/res/pkg/bb4d65a4/core.pkg.css', - 'type' => 'css', - ), 'fdcba95b' => array( 'name' => 'differential.pkg.css', @@ -1829,19 +1805,43 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/fdcba95b/differential.pkg.css', 'type' => 'css', ), + 16378540 => + array( + 'name' => 'core.pkg.css', + 'symbols' => + array( + 0 => 'phabricator-core-css', + 1 => 'phabricator-core-buttons-css', + 2 => 'phabricator-standard-page-view', + 3 => 'aphront-dialog-view-css', + 4 => 'aphront-form-view-css', + 5 => 'aphront-panel-view-css', + 6 => 'aphront-side-nav-view-css', + 7 => 'aphront-table-view-css', + 8 => 'aphront-crumbs-view-css', + 9 => 'aphront-tokenizer-control-css', + 10 => 'aphront-typeahead-control-css', + 11 => 'aphront-list-filter-view-css', + 12 => 'phabricator-directory-css', + 13 => 'phabricator-remarkup-css', + 14 => 'syntax-highlighting-css', + ), + 'uri' => '/res/pkg/16378540/core.pkg.css', + 'type' => 'css', + ), ), 'reverse' => array( - 'aphront-crumbs-view-css' => 'bb4d65a4', - 'aphront-dialog-view-css' => 'bb4d65a4', - 'aphront-form-view-css' => 'bb4d65a4', + 'aphront-crumbs-view-css' => '16378540', + 'aphront-dialog-view-css' => '16378540', + 'aphront-form-view-css' => '16378540', 'aphront-headsup-action-list-view-css' => 'fdcba95b', - 'aphront-list-filter-view-css' => 'bb4d65a4', - 'aphront-panel-view-css' => 'bb4d65a4', - 'aphront-side-nav-view-css' => 'bb4d65a4', - 'aphront-table-view-css' => 'bb4d65a4', - 'aphront-tokenizer-control-css' => 'bb4d65a4', - 'aphront-typeahead-control-css' => 'bb4d65a4', + 'aphront-list-filter-view-css' => '16378540', + 'aphront-panel-view-css' => '16378540', + 'aphront-side-nav-view-css' => '16378540', + 'aphront-table-view-css' => '16378540', + 'aphront-tokenizer-control-css' => '16378540', + 'aphront-typeahead-control-css' => '16378540', 'differential-changeset-view-css' => 'fdcba95b', 'differential-core-view-css' => 'fdcba95b', 'differential-inline-comment-editor' => 'a6562582', @@ -1890,16 +1890,16 @@ celerity_register_resource_map(array( 'javelin-vector' => 'b164acea', 'javelin-workflow' => '46547a92', 'phabricator-content-source-view-css' => 'fdcba95b', - 'phabricator-core-buttons-css' => 'bb4d65a4', - 'phabricator-core-css' => 'bb4d65a4', - 'phabricator-directory-css' => 'bb4d65a4', + 'phabricator-core-buttons-css' => '16378540', + 'phabricator-core-css' => '16378540', + 'phabricator-directory-css' => '16378540', 'phabricator-drag-and-drop-file-upload' => 'a6562582', 'phabricator-keyboard-shortcut' => '46547a92', 'phabricator-keyboard-shortcut-manager' => '46547a92', 'phabricator-object-selector-css' => 'fdcba95b', - 'phabricator-remarkup-css' => 'bb4d65a4', + 'phabricator-remarkup-css' => '16378540', 'phabricator-shaped-request' => 'a6562582', - 'phabricator-standard-page-view' => 'bb4d65a4', - 'syntax-highlighting-css' => 'bb4d65a4', + 'phabricator-standard-page-view' => '16378540', + 'syntax-highlighting-css' => '16378540', ), )); diff --git a/src/aphront/response/base/AphrontResponse.php b/src/aphront/response/base/AphrontResponse.php index 2cb1e3afaa..c4b4ce8b31 100644 --- a/src/aphront/response/base/AphrontResponse.php +++ b/src/aphront/response/base/AphrontResponse.php @@ -91,13 +91,11 @@ abstract class AphrontResponse { $this->formatEpochTimestampForHTTPHeader($this->lastModified)); } - $headers[] = array( - // IE has a feature where it may override an explicit Content-Type - // declaration by inferring a content type. This can be a security risk - // and we always explicitly transmit the correct Content-Type header, so - // prevent IE from using inferred content types. - array('X-Content-Type-Options', 'nosniff'), - ); + // IE has a feature where it may override an explicit Content-Type + // declaration by inferring a content type. This can be a security risk + // and we always explicitly transmit the correct Content-Type header, so + // prevent IE from using inferred content types. + $headers[] = array('X-Content-Type-Options', 'nosniff'); return $headers; } diff --git a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php index 2131740807..b30e819598 100644 --- a/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php +++ b/src/applications/differential/controller/revisionview/DifferentialRevisionViewController.php @@ -134,6 +134,28 @@ class DifferentialRevisionViewController extends DifferentialController { $aux_field->setHandles(array_select_keys($handles, $aux_phids[$key])); } + $reviewer_warning = null; + $has_live_reviewer = false; + foreach ($revision->getReviewers() as $reviewer) { + if (!$handles[$reviewer]->isDisabled()) { + $has_live_reviewer = true; + } + } + if (!$has_live_reviewer) { + $reviewer_warning = new AphrontErrorView(); + $reviewer_warning->setSeverity(AphrontErrorView::SEVERITY_WARNING); + $reviewer_warning->setTitle('No Active Reviewers'); + if ($revision->getReviewers()) { + $reviewer_warning->appendChild( + '

All specified reviewers are disabled. You may want to add '. + 'some new reviewers.

'); + } else { + $reviewer_warning->appendChild( + '

This revision has no specified reviewers. You may want to '. + 'add some.

'); + } + } + $request_uri = $request->getRequestURI(); $limit = 100; @@ -272,6 +294,7 @@ class DifferentialRevisionViewController extends DifferentialController { $page_pane = id(new DifferentialPrimaryPaneView()) ->setLineWidthFromChangesets($changesets) ->setID($pane_id) + ->appendChild($reviewer_warning) ->appendChild( $revision_detail->render(). $comment_view->render(). diff --git a/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php index 064f1af6ce..6e26e58244 100644 --- a/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php +++ b/src/applications/differential/field/specification/base/DifferentialFieldSpecification.php @@ -1,7 +1,7 @@ loadAllWhere( - '(username IN (%Ls)) OR (email IN (%Ls))', + '((username IN (%Ls)) OR (email IN (%Ls))) + AND isDisabled = 0 + AND isSystemAgent = 0', $value, $value); + $user_map = mpull($users, 'getPHID', 'getUsername'); foreach ($user_map as $username => $phid) { // Usernames may have uppercase letters in them. Put both names in the @@ -620,7 +623,8 @@ abstract class DifferentialFieldSpecification { ? "users and mailing lists" : "users"; throw new DifferentialFieldParseException( - "Commit message references nonexistent {$what}: {$invalid}."); + "Commit message references disabled or nonexistent {$what}: ". + "{$invalid}."); } return array_unique($results); diff --git a/src/applications/people/controller/profile/PhabricatorPeopleProfileController.php b/src/applications/people/controller/profile/PhabricatorPeopleProfileController.php index b3abeb5e40..0e7ae2f0fd 100644 --- a/src/applications/people/controller/profile/PhabricatorPeopleProfileController.php +++ b/src/applications/people/controller/profile/PhabricatorPeopleProfileController.php @@ -137,6 +137,14 @@ class PhabricatorPeopleProfileController extends PhabricatorPeopleController { $nav->addFilter(null, 'Edit Profile...', '/settings/page/profile/'); } + if ($viewer->getIsAdmin()) { + $nav->addSpacer(); + $nav->addFilter( + null, + 'Administrate User...', + '/people/edit/'.$user->getID().'/'); + } + return $this->buildStandardPageResponse( $header, array( diff --git a/src/applications/phid/handle/PhabricatorObjectHandle.php b/src/applications/phid/handle/PhabricatorObjectHandle.php index 56e826099b..6ef910501a 100644 --- a/src/applications/phid/handle/PhabricatorObjectHandle.php +++ b/src/applications/phid/handle/PhabricatorObjectHandle.php @@ -1,7 +1,7 @@ status != PhabricatorObjectHandleStatus::STATUS_OPEN) { - $class = 'handle-status-'.$this->status; + $class .= ' handle-status-'.$this->status; + } + + if ($this->disabled) { + $class .= ' handle-disabled'; } return phutil_render_tag( diff --git a/webroot/rsrc/css/application/base/standard-page-view.css b/webroot/rsrc/css/application/base/standard-page-view.css index ca0ff4338a..b74f921a9d 100644 --- a/webroot/rsrc/css/application/base/standard-page-view.css +++ b/webroot/rsrc/css/application/base/standard-page-view.css @@ -172,6 +172,11 @@ td.phabricator-login-details { text-decoration: line-through; } +a.handle-disabled { + background: #999999; + color: #cccccc; +} + .PhabricatorMonospaced { font-family: "Menlo", "Consolas", "Monaco", monospace; font-size: 10px;