From 46a7c61c80811d8bccb9b688a1092da4efca90ae Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 20 Jun 2013 14:13:53 -0700 Subject: [PATCH] Improve errors associated with adding new login providers Summary: Ref T1536. - When users try to add a one-of provider which already exists, give them a better error (a dialog explaining what's up with reasonable choices). - Disable such providers and label why they're disabled on the "new provider" screen. Test Plan: {F47012} {F47013} Reviewers: chad, btrahan Reviewed By: chad CC: aran Maniphest Tasks: T1536 Differential Revision: https://secure.phabricator.com/D6256 --- src/__celerity_resource_map__.php | 90 +++++++++---------- .../config/PhabricatorAuthEditController.php | 18 +++- .../config/PhabricatorAuthNewController.php | 21 ++++- src/view/AphrontDialogView.php | 9 +- .../control/AphrontFormRadioButtonControl.php | 12 ++- webroot/rsrc/css/aphront/form-view.css | 7 ++ 6 files changed, 106 insertions(+), 51 deletions(-) diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 3834f9ec8e..ac60fd92fb 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -814,7 +814,7 @@ celerity_register_resource_map(array( ), 'aphront-form-view-css' => array( - 'uri' => '/res/d541ef47/rsrc/css/aphront/form-view.css', + 'uri' => '/res/068dd7ed/rsrc/css/aphront/form-view.css', 'type' => 'css', 'requires' => array( @@ -4057,7 +4057,7 @@ celerity_register_resource_map(array( ), array( 'packages' => array( - 'c85c10a8' => + 'f0f013fd' => array( 'name' => 'core.pkg.css', 'symbols' => @@ -4105,7 +4105,7 @@ celerity_register_resource_map(array( 40 => 'phabricator-property-list-view-css', 41 => 'phabricator-tag-view-css', ), - 'uri' => '/res/pkg/c85c10a8/core.pkg.css', + 'uri' => '/res/pkg/f0f013fd/core.pkg.css', 'type' => 'css', ), 'f2ad0683' => @@ -4299,16 +4299,16 @@ celerity_register_resource_map(array( 'reverse' => array( 'aphront-attached-file-view-css' => 'adc3c36d', - 'aphront-dialog-view-css' => 'c85c10a8', - 'aphront-error-view-css' => 'c85c10a8', - 'aphront-form-view-css' => 'c85c10a8', - 'aphront-list-filter-view-css' => 'c85c10a8', - 'aphront-pager-view-css' => 'c85c10a8', - 'aphront-panel-view-css' => 'c85c10a8', - 'aphront-table-view-css' => 'c85c10a8', - 'aphront-tokenizer-control-css' => 'c85c10a8', - 'aphront-tooltip-css' => 'c85c10a8', - 'aphront-typeahead-control-css' => 'c85c10a8', + 'aphront-dialog-view-css' => 'f0f013fd', + 'aphront-error-view-css' => 'f0f013fd', + 'aphront-form-view-css' => 'f0f013fd', + 'aphront-list-filter-view-css' => 'f0f013fd', + 'aphront-pager-view-css' => 'f0f013fd', + 'aphront-panel-view-css' => 'f0f013fd', + 'aphront-table-view-css' => 'f0f013fd', + 'aphront-tokenizer-control-css' => 'f0f013fd', + 'aphront-tooltip-css' => 'f0f013fd', + 'aphront-typeahead-control-css' => 'f0f013fd', 'differential-changeset-view-css' => 'dd27a69b', 'differential-core-view-css' => 'dd27a69b', 'differential-inline-comment-editor' => '9488bb69', @@ -4322,7 +4322,7 @@ celerity_register_resource_map(array( 'differential-table-of-contents-css' => 'dd27a69b', 'diffusion-commit-view-css' => 'c8ce2d88', 'diffusion-icons-css' => 'c8ce2d88', - 'global-drag-and-drop-css' => 'c85c10a8', + 'global-drag-and-drop-css' => 'f0f013fd', 'inline-comment-summary-css' => 'dd27a69b', 'javelin-aphlict' => 'f2ad0683', 'javelin-behavior' => 'a9f14d76', @@ -4396,55 +4396,55 @@ celerity_register_resource_map(array( 'javelin-util' => 'a9f14d76', 'javelin-vector' => 'a9f14d76', 'javelin-workflow' => 'a9f14d76', - 'lightbox-attachment-css' => 'c85c10a8', + 'lightbox-attachment-css' => 'f0f013fd', 'maniphest-task-summary-css' => 'adc3c36d', 'maniphest-transaction-detail-css' => 'adc3c36d', - 'phabricator-action-list-view-css' => 'c85c10a8', - 'phabricator-application-launch-view-css' => 'c85c10a8', + 'phabricator-action-list-view-css' => 'f0f013fd', + 'phabricator-application-launch-view-css' => 'f0f013fd', 'phabricator-busy' => 'f2ad0683', 'phabricator-content-source-view-css' => 'dd27a69b', - 'phabricator-core-css' => 'c85c10a8', - 'phabricator-crumbs-view-css' => 'c85c10a8', + 'phabricator-core-css' => 'f0f013fd', + 'phabricator-crumbs-view-css' => 'f0f013fd', 'phabricator-drag-and-drop-file-upload' => '9488bb69', 'phabricator-dropdown-menu' => 'f2ad0683', 'phabricator-file-upload' => 'f2ad0683', - 'phabricator-filetree-view-css' => 'c85c10a8', - 'phabricator-flag-css' => 'c85c10a8', - 'phabricator-form-view-css' => 'c85c10a8', - 'phabricator-header-view-css' => 'c85c10a8', + 'phabricator-filetree-view-css' => 'f0f013fd', + 'phabricator-flag-css' => 'f0f013fd', + 'phabricator-form-view-css' => 'f0f013fd', + 'phabricator-header-view-css' => 'f0f013fd', 'phabricator-hovercard' => 'f2ad0683', - 'phabricator-jump-nav' => 'c85c10a8', + 'phabricator-jump-nav' => 'f0f013fd', 'phabricator-keyboard-shortcut' => 'f2ad0683', 'phabricator-keyboard-shortcut-manager' => 'f2ad0683', - 'phabricator-main-menu-view' => 'c85c10a8', + 'phabricator-main-menu-view' => 'f0f013fd', 'phabricator-menu-item' => 'f2ad0683', - 'phabricator-nav-view-css' => 'c85c10a8', + 'phabricator-nav-view-css' => 'f0f013fd', 'phabricator-notification' => 'f2ad0683', - 'phabricator-notification-css' => 'c85c10a8', - 'phabricator-notification-menu-css' => 'c85c10a8', - 'phabricator-object-item-list-view-css' => 'c85c10a8', + 'phabricator-notification-css' => 'f0f013fd', + 'phabricator-notification-menu-css' => 'f0f013fd', + 'phabricator-object-item-list-view-css' => 'f0f013fd', 'phabricator-object-selector-css' => 'dd27a69b', 'phabricator-phtize' => 'f2ad0683', 'phabricator-prefab' => 'f2ad0683', 'phabricator-project-tag-css' => 'adc3c36d', - 'phabricator-property-list-view-css' => 'c85c10a8', - 'phabricator-remarkup-css' => 'c85c10a8', + 'phabricator-property-list-view-css' => 'f0f013fd', + 'phabricator-remarkup-css' => 'f0f013fd', 'phabricator-shaped-request' => '9488bb69', - 'phabricator-side-menu-view-css' => 'c85c10a8', - 'phabricator-standard-page-view' => 'c85c10a8', - 'phabricator-tag-view-css' => 'c85c10a8', + 'phabricator-side-menu-view-css' => 'f0f013fd', + 'phabricator-standard-page-view' => 'f0f013fd', + 'phabricator-tag-view-css' => 'f0f013fd', 'phabricator-textareautils' => 'f2ad0683', 'phabricator-tooltip' => 'f2ad0683', - 'phabricator-transaction-view-css' => 'c85c10a8', - 'phabricator-zindex-css' => 'c85c10a8', - 'phui-button-css' => 'c85c10a8', - 'phui-form-css' => 'c85c10a8', - 'phui-icon-view-css' => 'c85c10a8', - 'phui-spacing-css' => 'c85c10a8', - 'sprite-apps-large-css' => 'c85c10a8', - 'sprite-gradient-css' => 'c85c10a8', - 'sprite-icons-css' => 'c85c10a8', - 'sprite-menu-css' => 'c85c10a8', - 'syntax-highlighting-css' => 'c85c10a8', + 'phabricator-transaction-view-css' => 'f0f013fd', + 'phabricator-zindex-css' => 'f0f013fd', + 'phui-button-css' => 'f0f013fd', + 'phui-form-css' => 'f0f013fd', + 'phui-icon-view-css' => 'f0f013fd', + 'phui-spacing-css' => 'f0f013fd', + 'sprite-apps-large-css' => 'f0f013fd', + 'sprite-gradient-css' => 'f0f013fd', + 'sprite-icons-css' => 'f0f013fd', + 'sprite-menu-css' => 'f0f013fd', + 'syntax-highlighting-css' => 'f0f013fd', ), )); diff --git a/src/applications/auth/controller/config/PhabricatorAuthEditController.php b/src/applications/auth/controller/config/PhabricatorAuthEditController.php index 225be6c695..5c5d0328da 100644 --- a/src/applications/auth/controller/config/PhabricatorAuthEditController.php +++ b/src/applications/auth/controller/config/PhabricatorAuthEditController.php @@ -56,8 +56,22 @@ final class PhabricatorAuthEditController ->execute(); if ($configs) { - // TODO: We could link to the other config's edit interface here. - throw new Exception("This provider is already configured!"); + $id = head($configs)->getID(); + $dialog = id(new AphrontDialogView()) + ->setUser($viewer) + ->setMethod('GET') + ->setSubmitURI($this->getApplicationURI('config/edit/'.$id.'/')) + ->setTitle(pht('Provider Already Configured')) + ->appendChild( + pht( + 'This provider ("%s") already exists, and you can not add more '. + 'than one instance of it. You can edit the existing provider, '. + 'or you can choose a different provider.', + $provider->getProviderName())) + ->addCancelButton($this->getApplicationURI('config/new/')) + ->addSubmitButton(pht('Edit Existing Provider')); + + return id(new AphrontDialogResponse())->setDialog($dialog); } $config = $provider->getDefaultProviderConfig(); diff --git a/src/applications/auth/controller/config/PhabricatorAuthNewController.php b/src/applications/auth/controller/config/PhabricatorAuthNewController.php index 4d28838d80..15c98bdaea 100644 --- a/src/applications/auth/controller/config/PhabricatorAuthNewController.php +++ b/src/applications/auth/controller/config/PhabricatorAuthNewController.php @@ -46,12 +46,31 @@ final class PhabricatorAuthNewController ->setName('provider') ->setError($e_provider); + $configured = PhabricatorAuthProvider::getAllProviders(); + $configured_classes = array(); + foreach ($configured as $configured_provider) { + $configured_classes[get_class($configured_provider)] = true; + } + + // Sort providers by login order, and move disabled providers to the + // bottom. $providers = msort($providers, 'getLoginOrder'); + $providers = array_diff_key($providers, $configured_classes) + $providers; + foreach ($providers as $provider) { + if (isset($configured_classes[get_class($provider)])) { + $disabled = true; + $description = pht('This provider is already configured.'); + } else { + $disabled = false; + $description = $provider->getDescriptionForCreate(); + } $options->addButton( get_class($provider), $provider->getNameForCreate(), - $provider->getDescriptionForCreate()); + $description, + $disabled ? 'disabled' : null, + $disabled); } $form = id(new AphrontFormView()) diff --git a/src/view/AphrontDialogView.php b/src/view/AphrontDialogView.php index 4fa384a205..6c3d4372d5 100644 --- a/src/view/AphrontDialogView.php +++ b/src/view/AphrontDialogView.php @@ -14,6 +14,13 @@ final class AphrontDialogView extends AphrontView { private $headerColor = PhabricatorActionHeaderView::HEADER_DARK_GREY; private $footers = array(); private $isStandalone; + private $method = 'POST'; + + + public function setMethod($method) { + $this->method = $method; + return $this; + } public function setIsStandalone($is_standalone) { $this->isStandalone = $is_standalone; @@ -159,7 +166,7 @@ final class AphrontDialogView extends AphrontView { $form_attributes = array( 'action' => $this->submitURI, - 'method' => 'post', + 'method' => $this->method, 'id' => $this->formID, ); diff --git a/src/view/form/control/AphrontFormRadioButtonControl.php b/src/view/form/control/AphrontFormRadioButtonControl.php index 1fca18e9d5..b69879fc46 100644 --- a/src/view/form/control/AphrontFormRadioButtonControl.php +++ b/src/view/form/control/AphrontFormRadioButtonControl.php @@ -4,12 +4,18 @@ final class AphrontFormRadioButtonControl extends AphrontFormControl { private $buttons = array(); - public function addButton($value, $label, $caption, $class = null) { + public function addButton( + $value, + $label, + $caption, + $class = null, + $disabled = false) { $this->buttons[] = array( 'value' => $value, 'label' => $label, 'caption' => $caption, 'class' => $class, + 'disabled' => $disabled, ); return $this; } @@ -32,7 +38,9 @@ final class AphrontFormRadioButtonControl extends AphrontFormControl { 'checked' => ($button['value'] == $this->getValue()) ? 'checked' : null, - 'disabled' => $this->getDisabled() ? 'disabled' : null, + 'disabled' => ($this->getDisabled() || $button['disabled']) + ? 'disabled' + : null, )); $label = phutil_tag( 'label', diff --git a/webroot/rsrc/css/aphront/form-view.css b/webroot/rsrc/css/aphront/form-view.css index b4f1ebe832..cf4837f85c 100644 --- a/webroot/rsrc/css/aphront/form-view.css +++ b/webroot/rsrc/css/aphront/form-view.css @@ -190,6 +190,7 @@ table.aphront-form-control-radio-layout th { color: #222222; } + table.aphront-form-control-checkbox-layout th { padding-top: 2px; padding-left: 8px; @@ -204,6 +205,12 @@ table.aphront-form-control-checkbox-layout th { width: auto; } + +.aphront-form-control-radio-layout label.disabled, +.aphront-form-control-checkbox-layout label.disabled { + color: #777777; +} + .aphront-form-radio-caption { margin-top: 4px; font-size: 12px;