Page MenuHomePhabricator

Implement "Resign" action against ApplicationTransactions
ClosedPublic

Authored by epriestley on Feb 25 2014, 2:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 3:24 AM
Unknown Object (File)
Fri, Nov 22, 10:56 PM
Unknown Object (File)
Tue, Nov 19, 8:40 AM
Unknown Object (File)
Fri, Nov 15, 7:55 AM
Unknown Object (File)
Mon, Nov 11, 11:44 AM
Unknown Object (File)
Mon, Nov 11, 8:43 AM
Unknown Object (File)
Thu, Nov 7, 2:00 AM
Unknown Object (File)
Mon, Nov 4, 8:09 AM
Subscribers

Details

Summary

Ref T2222. This introduces two small new concepts:

  • expandTransactions(): allows a transaction to expand into several transactions. For example, "resign" adds a "remove reviewers" transaction.
    • We have some other cases which could use this, but currently hard-code things outside of the Editor.
      • One example is that in Maniphest, closing a task implies claiming it if it is unowned.
  • setIgnoreOnNoEffect(): The whole Editor can be set to continue or stop if any transactions have no effect, but this allows the behavior to be refined at the individual transaction level. This is primarily to make the UX less confusing, so the user gets only a single relevant error instead of one for each expanded transaction.

Otherwise, this is pretty straightforward.

Test Plan

Rigged comment form to use SavePro controller, enabled resign action, then tried to resign from a bunch of stuff.

{F117743}

Diff Detail

Repository
rP Phabricator
Branch
dxpfield1
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/differential/editor/DifferentialTransactionEditor.php:125XHP16TODO Comment
Unit
Tests Passed

Event Timeline

I am probably missing something with my inline.

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
873

no array_mergev or anything? the array of transactions becomes an array of arrays of transactions so i figure eg combineTransactions isn't working since it used the earlier format?

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
868

We iterate over the array of return values here, so $expanded is the individual transactions.

We could array_mergev() this instead but this seemed easier to read when I was writing it. :P

Let me rewrite this slightly for clarity, I was confused by this too in the cold light of morning when I saw your comment.