From 3635a11f848f094bcb3a5c2a8de3d6f6080d1952 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 22 May 2020 15:27:52 -0700 Subject: [PATCH 1/3] When cancelling an edit of an inline with content, don't hide the inline Summary: See PHI1753. This condition got rewritten for suggested edits and accidentally inverted. Test Plan: - Create a comment, type text, save draft, edit comment, cancel. - Before: comment hides itself. - After: comment properly cancels into pre-edit draft state. Differential Revision: https://secure.phabricator.com/D21286 --- resources/celerity/map.php | 12 ++++++------ webroot/rsrc/js/application/diff/DiffInline.js | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 1d98650a7e..df11037696 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -13,7 +13,7 @@ return array( 'core.pkg.js' => '845355f4', 'dark-console.pkg.js' => '187792c2', 'differential.pkg.css' => '5c459f92', - 'differential.pkg.js' => '2b4a7014', + 'differential.pkg.js' => '218fda21', 'diffusion.pkg.css' => '42c75c37', 'diffusion.pkg.js' => 'a98c0bf7', 'maniphest.pkg.css' => '35995d6d', @@ -381,7 +381,7 @@ return array( 'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => '0116d3e8', 'rsrc/js/application/diff/DiffChangeset.js' => '39dcf2c3', 'rsrc/js/application/diff/DiffChangesetList.js' => 'cc2c5de5', - 'rsrc/js/application/diff/DiffInline.js' => '008b6a15', + 'rsrc/js/application/diff/DiffInline.js' => '511a1315', 'rsrc/js/application/diff/DiffPathView.js' => '8207abf9', 'rsrc/js/application/diff/DiffTreeView.js' => '5d83623b', 'rsrc/js/application/differential/behavior-diff-radios.js' => '925fe8cd', @@ -778,7 +778,7 @@ return array( 'phabricator-dashboard-css' => '5a205b9d', 'phabricator-diff-changeset' => '39dcf2c3', 'phabricator-diff-changeset-list' => 'cc2c5de5', - 'phabricator-diff-inline' => '008b6a15', + 'phabricator-diff-inline' => '511a1315', 'phabricator-diff-path-view' => '8207abf9', 'phabricator-diff-tree-view' => '5d83623b', 'phabricator-drag-and-drop-file-upload' => '4370900d', @@ -919,9 +919,6 @@ return array( 'unhandled-exception-css' => '9ecfc00d', ), 'requires' => array( - '008b6a15' => array( - 'javelin-dom', - ), '0116d3e8' => array( 'javelin-behavior', 'javelin-dom', @@ -1397,6 +1394,9 @@ return array( 'javelin-stratcom', 'javelin-dom', ), + '511a1315' => array( + 'javelin-dom', + ), '5202e831' => array( 'javelin-install', 'javelin-dom', diff --git a/webroot/rsrc/js/application/diff/DiffInline.js b/webroot/rsrc/js/application/diff/DiffInline.js index 1e700855f1..705d95e70a 100644 --- a/webroot/rsrc/js/application/diff/DiffInline.js +++ b/webroot/rsrc/js/application/diff/DiffInline.js @@ -863,7 +863,7 @@ JX.install('DiffInline', { // If this was an empty box and we typed some text and then hit cancel, // don't show the empty concrete inline. - if (!this._isVoidContentState(this._originalState)) { + if (this._isVoidContentState(this._originalState)) { this.setInvisible(true); } else { this.setInvisible(false); From a529efa5b8554167f8a2c16f1423837ae47cc596 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 23 May 2020 08:13:41 -0700 Subject: [PATCH 2/3] Fix an issue where inline comments with only edit suggestions are considered empty Summary: Ref T13513. An inline is not considered empty if it has a suggestion, but some of the shared transaction code doesn't test for this properly. Update the shared transaction code to be aware that application comments may have more complex emptiness rules. Test Plan: - Posted an inline with only an edit suggestion, comment went through. - Tried to post a normal empty comment, got an appropriate warning. Maniphest Tasks: T13513 Differential Revision: https://secure.phabricator.com/D21287 --- .../storage/DifferentialTransactionComment.php | 12 ++++++++++++ .../storage/PhabricatorApplicationTransaction.php | 9 +++------ .../PhabricatorApplicationTransactionComment.php | 12 ++++++++++++ 3 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/applications/differential/storage/DifferentialTransactionComment.php b/src/applications/differential/storage/DifferentialTransactionComment.php index 491d89a968..873aecdd41 100644 --- a/src/applications/differential/storage/DifferentialTransactionComment.php +++ b/src/applications/differential/storage/DifferentialTransactionComment.php @@ -144,4 +144,16 @@ final class DifferentialTransactionComment return $this; } + + public function isEmptyComment() { + if (!parent::isEmptyComment()) { + return false; + } + + return $this->newInlineCommentObject() + ->getContentState() + ->isEmptyContentState(); + } + + } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index b815354d11..95ef1de6c3 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -127,15 +127,12 @@ abstract class PhabricatorApplicationTransaction } public function hasComment() { - if (!$this->getComment()) { + $comment = $this->getComment(); + if (!$comment) { return false; } - $content = $this->getComment()->getContent(); - - // If the content is empty or consists of only whitespace, don't count - // this as comment. - if (!strlen(trim($content))) { + if ($comment->isEmptyComment()) { return false; } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php index 896b45556a..00cad18d2d 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php @@ -107,6 +107,18 @@ abstract class PhabricatorApplicationTransactionComment $this->getTransactionPHID()); } + public function isEmptyComment() { + $content = $this->getContent(); + + // The comment is empty if there's no content, or if the content only has + // whitespace. + if (!strlen(trim($content))) { + return true; + } + + return false; + } + /* -( PhabricatorMarkupInterface )----------------------------------------- */ From f686a0b8275eb0dd63ff3717b835a32d5e6ed0b7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 26 May 2020 07:02:31 -0700 Subject: [PATCH 3/3] In Phortune accounts, prevent self-removal more narrowly Summary: Currently, Phortune attempts to prevent users from removing themselves as account managers. It does this by checking that the new list includes them. Usually this is sufficient, because you can't normally edit an account unless you're already a manager. However, we get the wrong result (incorrect rejection of the edit) if the actor is omnipotent and the acting user was not already a member. It's okay to edit an account into a state which doesn't include you if you have permission to edit the account and aren't already a manager. Specifically, this supports more formal tooling around staff modifications to billing accounts, where the actor has staff-omnipotence and the acting user is a staff member and only used for purposes of leaving a useful audit trail. Test Plan: Elsewhere, ran staff tooling to modify accounts and was able to act as "alice" to add "bailey", even though "alice" was not herself a manager. Differential Revision: https://secure.phabricator.com/D21288 --- src/applications/phortune/editor/PhortuneAccountEditor.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/phortune/editor/PhortuneAccountEditor.php b/src/applications/phortune/editor/PhortuneAccountEditor.php index 50a71c476f..8344bc2b6e 100644 --- a/src/applications/phortune/editor/PhortuneAccountEditor.php +++ b/src/applications/phortune/editor/PhortuneAccountEditor.php @@ -63,7 +63,7 @@ final class PhortuneAccountEditor } $actor_phid = $this->getActingAsPHID(); - if (!isset($new[$actor_phid])) { + if (isset($old[$actor_phid]) && !isset($new[$actor_phid])) { $error = new PhabricatorApplicationTransactionValidationError( $type, pht('Invalid'),