Fix a looping workflow when trying to submit a partially-effectless transaction group

@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge

phorge phabricator

fork

Summary:
Ref T13289. If you do this:

- Subscribe to a task (so we don't generate a subscribe side-effect later).
- Prepare a transaction group: sign with MFA, change projects (don't make any changes), add a comment.
- Submit the transaction group.

...you'll get prompted "Some actions don't have any effect (the non-change to projects), apply remaining effects?".

If you confirm, you get MFA'd, but the MFA flow loses the "continue" confirmation, so you get trapped in a workflow loop of confirming and MFA'ing.

Instead, retain the "continue" bit through the MFA.

Also, don't show "You can't sign an empty transaction group" if there's a comment.

See also T13295, since the amount of magic here can probably be reduced. There's likely little reason for "continue" or "hisec" to be magic nowadays.

Test Plan:
- Went through the workflow above.
- Before: looping workflow.
- After: "Continue" carries through the MFA gate.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13289

Differential Revision: https://secure.phabricator.com/D20552

epriestley 7 years ago ce6fc5be 719dd6d3

+9 -2

3 changed files

expand all

src

aphront

AphrontRequest.php

handler

PhabricatorHighSecurityRequestExceptionHandler.php

applications

transactions

editor

PhabricatorApplicationTransactionEditor.php

+1 -1

src/aphront/AphrontRequest.php

··· 663 663 } 664 664 665 665 public function isContinueRequest() { 666 - return $this->isFormPost() && $this->getStr('__continue__'); 666 + return $this->isFormOrHisecPost() && $this->getStr('__continue__'); 667 667 } 668 668 669 669 public function isPreviewRequest() {

src/aphront/handler/PhabricatorHighSecurityRequestExceptionHandler.php

··· 120 120 $dialog->addHiddenInput($key, $value); 121 121 } 122 122 123 + // See T13289. If the user hit a "some transactions have no effect" dialog 124 + // and elected to continue, we want to pass that flag through the MFA 125 + // dialog even though it is not normally a passthrough request parameter. 126 + if ($request->isContinueRequest()) { 127 + $dialog->addHiddenInput(AphrontRequest::TYPE_CONTINUE, 1); 128 + } 129 + 123 130 return $dialog; 124 131 } 125 132

+1 -1

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

··· 2535 2535 // If none of the transactions have an effect, the meta-transactions also 2536 2536 // have no effect. Add them to the "no effect" list so we get a full set 2537 2537 // of errors for everything. 2538 - if (!$any_effect) { 2538 + if (!$any_effect && !$has_comment) { 2539 2539 $no_effect += $meta_xactions; 2540 2540 } 2541 2541

Configure Feed

Configure Feed