From 7a79131bf27980422dfac27b3d753021858f3f15 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 18 Apr 2020 14:46:55 -0700 Subject: [PATCH] Replace old hard-coded URI-based "changes saved" jank with new overgeneralized cookie-based "changes saved" jank Summary: Ref T13515. Settings currently has some highly specialized code for rendering "Changes saved." messages. The "saved" state is communicated across a redirect-after-POST by adding `/saved/` to the end of the URI. This isn't great. It needs a lot of moving pieces, including special accommodations in routing rules. It's user-visible. It has the wrong behavior if you reload the page or navigate directly to the "saved" URI. Try this scheme, which is also pretty sketchy but seems like an upgrade on the balance: - Set a cookie on the redirect which identifies the form we just saved. - On page startup: if this cookie exists, save the value and clear it. - If the current page started with a cookie identifying the form on the page, treat the page as a "saved" page. This supports passing a small amount of state across the redirect-after-POST flow, and when you reload the page it doesn't keep the message around. Applications don't need to coordinate it, either. Seems somewhat cleaner? Test Plan: In Firefox, Safari, and Chrome: saved settings, saw a "Saved changes" banner without any URI junk. Reloaded page, saw banner vanish properly. Maniphest Tasks: T13515 Differential Revision: https://secure.phabricator.com/D21144 --- src/__phutil_library_map__.php | 2 + src/aphront/AphrontRequest.php | 15 ++++++ .../AphrontApplicationConfiguration.php | 2 + .../PhabricatorAuthApplication.php | 2 +- .../auth/constants/PhabricatorCookies.php | 7 +++ .../PhabricatorSettingsApplication.php | 2 +- .../editor/PhabricatorSettingsEditEngine.php | 16 +++++-- ...abricatorEmailPreferencesSettingsPanel.php | 1 - .../PhabricatorPasswordSettingsPanel.php | 1 - .../editengine/PhabricatorEditEngine.php | 46 ++++++++++++++++-- .../PhabricatorEditEnginePageState.php | 47 +++++++++++++++++++ src/view/phui/PHUIObjectBoxView.php | 15 ------ 12 files changed, 128 insertions(+), 28 deletions(-) create mode 100644 src/applications/transactions/editengine/PhabricatorEditEnginePageState.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 91e599dda7..3e80fbe7ba 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3251,6 +3251,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineMFAInterface' => 'applications/transactions/editengine/PhabricatorEditEngineMFAInterface.php', 'PhabricatorEditEngineNameTransaction' => 'applications/transactions/xaction/PhabricatorEditEngineNameTransaction.php', 'PhabricatorEditEngineOrderTransaction' => 'applications/transactions/xaction/PhabricatorEditEngineOrderTransaction.php', + 'PhabricatorEditEnginePageState' => 'applications/transactions/editengine/PhabricatorEditEnginePageState.php', 'PhabricatorEditEnginePointsCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEnginePointsCommentAction.php', 'PhabricatorEditEnginePreambleTransaction' => 'applications/transactions/xaction/PhabricatorEditEnginePreambleTransaction.php', 'PhabricatorEditEngineProfileMenuItem' => 'applications/search/menuitem/PhabricatorEditEngineProfileMenuItem.php', @@ -9714,6 +9715,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineMFAEngine' => 'Phobject', 'PhabricatorEditEngineNameTransaction' => 'PhabricatorEditEngineTransactionType', 'PhabricatorEditEngineOrderTransaction' => 'PhabricatorEditEngineTransactionType', + 'PhabricatorEditEnginePageState' => 'Phobject', 'PhabricatorEditEnginePointsCommentAction' => 'PhabricatorEditEngineCommentAction', 'PhabricatorEditEnginePreambleTransaction' => 'PhabricatorEditEngineTransactionType', 'PhabricatorEditEngineProfileMenuItem' => 'PhabricatorProfileMenuItem', diff --git a/src/aphront/AphrontRequest.php b/src/aphront/AphrontRequest.php index 469e6ba766..d8310386c2 100644 --- a/src/aphront/AphrontRequest.php +++ b/src/aphront/AphrontRequest.php @@ -30,6 +30,7 @@ final class AphrontRequest extends Phobject { private $controller; private $uriData = array(); private $cookiePrefix; + private $submitKey; public function __construct($host, $path) { $this->host = $host; @@ -914,5 +915,19 @@ final class AphrontRequest extends Phobject { return $future; } + public function updateEphemeralCookies() { + $submit_cookie = PhabricatorCookies::COOKIE_SUBMIT; + + $submit_key = $this->getCookie($submit_cookie); + if (strlen($submit_key)) { + $this->clearCookie($submit_cookie); + $this->submitKey = $submit_key; + } + + } + + public function getSubmitKey() { + return $this->submitKey; + } } diff --git a/src/aphront/configuration/AphrontApplicationConfiguration.php b/src/aphront/configuration/AphrontApplicationConfiguration.php index bb4fbefa41..c0e66f3251 100644 --- a/src/aphront/configuration/AphrontApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontApplicationConfiguration.php @@ -27,6 +27,8 @@ final class AphrontApplicationConfiguration $request->setApplicationConfiguration($this); $request->setCookiePrefix($cookie_prefix); + $request->updateEphemeralCookies(); + return $request; } diff --git a/src/applications/auth/application/PhabricatorAuthApplication.php b/src/applications/auth/application/PhabricatorAuthApplication.php index 6d4a70b735..3e020a208b 100644 --- a/src/applications/auth/application/PhabricatorAuthApplication.php +++ b/src/applications/auth/application/PhabricatorAuthApplication.php @@ -73,7 +73,7 @@ final class PhabricatorAuthApplication extends PhabricatorApplication { 'session/downgrade/' => 'PhabricatorAuthDowngradeSessionController', 'enroll/' => array( - '(?:(?P[^/]+)/)?(?:(?Psaved)/)?' + '(?:(?P[^/]+)/)?' => 'PhabricatorAuthNeedsMultiFactorController', ), 'sshkey/' => array( diff --git a/src/applications/auth/constants/PhabricatorCookies.php b/src/applications/auth/constants/PhabricatorCookies.php index 855537b0d4..2573bf3ec6 100644 --- a/src/applications/auth/constants/PhabricatorCookies.php +++ b/src/applications/auth/constants/PhabricatorCookies.php @@ -63,6 +63,13 @@ final class PhabricatorCookies extends Phobject { const COOKIE_INVITE = 'invite'; + /** + * Stores a workflow completion across a redirect-after-POST following a + * form submission. This can be used to show "Changes Saved" messages. + */ + const COOKIE_SUBMIT = 'phfrm'; + + /* -( Client ID Cookie )--------------------------------------------------- */ diff --git a/src/applications/settings/application/PhabricatorSettingsApplication.php b/src/applications/settings/application/PhabricatorSettingsApplication.php index e5e0034e1a..879130677e 100644 --- a/src/applications/settings/application/PhabricatorSettingsApplication.php +++ b/src/applications/settings/application/PhabricatorSettingsApplication.php @@ -23,7 +23,7 @@ final class PhabricatorSettingsApplication extends PhabricatorApplication { } public function getRoutes() { - $panel_pattern = '(?:page/(?P[^/]+)/(?:(?Psaved)/)?)?'; + $panel_pattern = '(?:page/(?P[^/]+)/)?'; return array( '/settings/' => array( diff --git a/src/applications/settings/editor/PhabricatorSettingsEditEngine.php b/src/applications/settings/editor/PhabricatorSettingsEditEngine.php index 34a6132d80..218b5b82b0 100644 --- a/src/applications/settings/editor/PhabricatorSettingsEditEngine.php +++ b/src/applications/settings/editor/PhabricatorSettingsEditEngine.php @@ -128,10 +128,6 @@ final class PhabricatorSettingsEditEngine return PhabricatorPolicies::POLICY_ADMIN; } - public function getEffectiveObjectEditDoneURI($object) { - return parent::getEffectiveObjectViewURI($object).'saved/'; - } - public function getEffectiveObjectEditCancelURI($object) { if (!$object->getUser()) { return '/settings/'; @@ -253,4 +249,16 @@ final class PhabricatorSettingsEditEngine return parent::getValidationExceptionShortMessage($ex, $field); } + protected function newEditFormHeadContent( + PhabricatorEditEnginePageState $state) { + + if ($state->getIsSave()) { + return id(new PHUIInfoView()) + ->setSeverity(PHUIInfoView::SEVERITY_NOTICE) + ->appendChild(pht('Changes saved.')); + } + + return null; + } + } diff --git a/src/applications/settings/panel/PhabricatorEmailPreferencesSettingsPanel.php b/src/applications/settings/panel/PhabricatorEmailPreferencesSettingsPanel.php index defee73393..50d64072f5 100644 --- a/src/applications/settings/panel/PhabricatorEmailPreferencesSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorEmailPreferencesSettingsPanel.php @@ -138,7 +138,6 @@ final class PhabricatorEmailPreferencesSettingsPanel $form_box = id(new PHUIObjectBoxView()) ->setHeaderText(pht('Email Preferences')) - ->setFormSaved($request->getStr('saved')) ->setFormErrors($errors) ->setBackground(PHUIObjectBoxView::WHITE_CONFIG) ->setForm($form); diff --git a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php index 77f32f977d..9980d1d355 100644 --- a/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php +++ b/src/applications/settings/panel/PhabricatorPasswordSettingsPanel.php @@ -206,7 +206,6 @@ final class PhabricatorPasswordSettingsPanel extends PhabricatorSettingsPanel { $algo_box = $this->newBox(pht('Password Algorithms'), $properties); $form_box = id(new PHUIObjectBoxView()) ->setHeaderText(pht('Change Password')) - ->setFormSaved($request->getStr('saved')) ->setFormErrors($errors) ->setBackground(PHUIObjectBoxView::WHITE_CONFIG) ->setForm($form); diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index 14143c733a..89f23845b7 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1036,9 +1036,13 @@ abstract class PhabricatorEditEngine $fields = $this->buildEditFields($object); $template = $object->getApplicationTransactionTemplate(); + $page_state = new PhabricatorEditEnginePageState(); + if ($this->getIsCreate()) { $cancel_uri = $this->getObjectCreateCancelURI($object); $submit_button = $this->getObjectCreateButtonText($object); + + $page_state->setIsCreate(true); } else { $cancel_uri = $this->getEffectiveObjectEditCancelURI($object); $submit_button = $this->getObjectEditButtonText($object); @@ -1069,6 +1073,8 @@ abstract class PhabricatorEditEngine $validation_exception = null; if ($request->isFormOrHisecPost() && $request->getBool('editEngine')) { + $page_state->setIsSubmit(true); + $submit_fields = $fields; foreach ($submit_fields as $key => $field) { @@ -1154,6 +1160,8 @@ abstract class PhabricatorEditEngine $field->setControlError($message); } + + $page_state->setIsError(true); } } else { if ($this->getIsCreate()) { @@ -1252,6 +1260,17 @@ abstract class PhabricatorEditEngine $box_header->addActionLink($action_button); } + $request_submit_key = $request->getSubmitKey(); + $engine_submit_key = $this->getEditEngineSubmitKey(); + + if ($request_submit_key === $engine_submit_key) { + $page_state->setIsSubmit(true); + $page_state->setIsSave(true); + } + + $head = $this->newEditFormHeadContent($page_state); + $tail = $this->newEditFormTailContent($page_state); + $box = id(new PHUIObjectBoxView()) ->setUser($viewer) ->setHeader($box_header) @@ -1259,14 +1278,11 @@ abstract class PhabricatorEditEngine ->setBackground(PHUIObjectBoxView::WHITE_CONFIG) ->appendChild($form); - // This is fairly questionable, but in use by Settings. - if ($request->getURIData('formSaved')) { - $box->setFormSaved(true); - } - $content = array( + $head, $box, $previews, + $tail, ); $view = new PHUITwoColumnView(); @@ -1291,14 +1307,34 @@ abstract class PhabricatorEditEngine return $page; } + protected function newEditFormHeadContent( + PhabricatorEditEnginePageState $state) { + return null; + } + + protected function newEditFormTailContent( + PhabricatorEditEnginePageState $state) { + return null; + } + protected function newEditResponse( AphrontRequest $request, $object, array $xactions) { + + $submit_cookie = PhabricatorCookies::COOKIE_SUBMIT; + $submit_key = $this->getEditEngineSubmitKey(); + + $request->setTemporaryCookie($submit_cookie, $submit_key); + return id(new AphrontRedirectResponse()) ->setURI($this->getEffectiveObjectEditDoneURI($object)); } + private function getEditEngineSubmitKey() { + return 'edit-engine/'.$this->getEngineKey(); + } + private function buildEditForm($object, array $fields) { $viewer = $this->getViewer(); $controller = $this->getController(); diff --git a/src/applications/transactions/editengine/PhabricatorEditEnginePageState.php b/src/applications/transactions/editengine/PhabricatorEditEnginePageState.php new file mode 100644 index 0000000000..7c0da74aec --- /dev/null +++ b/src/applications/transactions/editengine/PhabricatorEditEnginePageState.php @@ -0,0 +1,47 @@ +isCreate = $is_create; + return $this; + } + + public function getIsCreate() { + return $this->isCreate; + } + + public function setIsSubmit($is_submit) { + $this->isSubmit = $is_submit; + return $this; + } + + public function getIsSubmit() { + return $this->isSubmit; + } + + public function setIsError($is_error) { + $this->isError = $is_error; + return $this; + } + + public function getIsError() { + return $this->isError; + } + + public function setIsSave($is_save) { + $this->isSave = $is_save; + return $this; + } + + public function getIsSave() { + return $this->isSave; + } + +} diff --git a/src/view/phui/PHUIObjectBoxView.php b/src/view/phui/PHUIObjectBoxView.php index 39c6d4f5b0..4d42187479 100644 --- a/src/view/phui/PHUIObjectBoxView.php +++ b/src/view/phui/PHUIObjectBoxView.php @@ -7,7 +7,6 @@ final class PHUIObjectBoxView extends AphrontTagView { private $background; private $tabGroups = array(); private $formErrors = null; - private $formSaved = false; private $infoView; private $form; private $validationException; @@ -75,19 +74,6 @@ final class PHUIObjectBoxView extends AphrontTagView { return $this; } - public function setFormSaved($saved, $text = null) { - if (!$text) { - $text = pht('Changes saved.'); - } - if ($saved) { - $save = id(new PHUIInfoView()) - ->setSeverity(PHUIInfoView::SEVERITY_NOTICE) - ->appendChild($text); - $this->formSaved = $save; - } - return $this; - } - public function addTabGroup(PHUITabGroupView $view) { $this->tabGroups[] = $view; return $this; @@ -324,7 +310,6 @@ final class PHUIObjectBoxView extends AphrontTagView { $header, $this->infoView, $this->formErrors, - $this->formSaved, $exception_errors, $this->form, $this->tabGroups,