Ref T12671. This modernized Maniphest transactions to modular transactions.
Details
- Reviewers
epriestley - Maniphest Tasks
- T12671: Update Maniphest for modular transactions
- Commits
- rPd6a620be4577: Update Maniphest for modular transactions
- Create Task
- Edit Task
- Raise Priority
- Change Status
- Merge as a duplicate
- Create Subtask
- Claim Task
- Assign Project
- Move on Workboard
- Set a cover image
- Assign story points
- Change story points
- Generate lots via lipsum
- Bulk edit tasks
- Leave comments
- Award Token
I'm sure I'm missing something.
Diff Detail
- Repository
- rP Phabricator
- Branch
- maniphest-xaction (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 16945 Build 22645: Run Core Tests Build 22644: arc lint + arc unit
Event Timeline
I think this should (at least mostly) work on its own -- at least in Differential, we were able to do the conversion piecemeal without moving every transactiontype over at once. Maybe you could do something similar here? Like do all the simple title/description stuff and then we can do the workboard things separately? Or are you hitting more complicated issues?
src/applications/maniphest/storage/ManiphestTransaction.php | ||
---|---|---|
43 | At least in theory, it should be possible to get rid of this for converted transactions -- the new renderHandle() stuff doesn't require (or use) preloading anymore. | |
src/applications/maniphest/xaction/ManiphestTaskAttachTransaction.php | ||
7–8 | We could probably just throw this away now, I think we haven't generated these in 3+ years. |
I'm stuck at "Capability Not Supported" when just creating a random task. Haven't been able to track down which transaction is causing it.
I added this:
diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index b5aa399521..6412ac6817 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -404,6 +404,7 @@ abstract class PhabricatorApplicationTransactionEditor case PhabricatorTransactions::TYPE_COMMENT: return null; default: + var_dump($type); return $this->getCustomTransactionOldValue($object, $xaction); } }
...and got core:columns dumped. I think the issue is that ManiphestTransactionEditor-> getCustomTransactionOldValue() was removed completely, but a couple of the transaction types haven't converted yet. You could either convert them (maybe tricky) or put it back with just return null for those couple of types (maybe easier).
I do think this all reasonably works, but I haven't tested email commands, herald... will keep testing.
I'm not especially confident that I didn't miss anything, but I didn't catch anything that looks wrong.
src/applications/maniphest/editor/ManiphestTransactionEditor.php | ||
---|---|---|
492–493 | Does this still get called with ModularTransactions? I think it might not. (Easiest way to test is probably put a throw here and then set a task's parent and see if it blows up.) | |
src/applications/maniphest/xaction/ManiphestTaskCoverImageTransaction.php | ||
89 | This should be isViewableImage(), isViewableInBrowser() includes things like text files and possibly PDFs. | |
src/applications/maniphest/xaction/ManiphestTaskPointsTransaction.php | ||
23 | I believe shouldHideForFeed() fires earlier and prevents more work (e.g., stops publication of the story completely). Returning null from title rendering hides it at display time, but we still do all the work to publish it. The feed story title also falls back to getTitle(), I think, so this event effectively has a story title. |
Not sure how to test the parent transaction validation stuff. I set a bunch of parents, subtasks, changed lots of orders....
The easiest way to test if the code is running is to add throw new Exception("Yep, this code is running.") above the validation and see if it gets hit.
To test that the code actually works, the easiest thing is probably to call maniphest.edit with a bad parent (like "cat"), probably something like:
$ echo '{ "transactions": [ { "type": "parent", "value": "cat" }, { "type": "title", "value": "test" } ] }' | arc call-conduit maniphest.edit --conduit-uri http://local.phacility.com/ {"error":"ERR-CONDUIT-CORE","errorMessage":"ERR-CONDUIT-CORE: Validation errors:\n - Parent task identifier \"cat\" does not identify a visible task.","response":null}
Host returned HTTP/200, but invalid JSON data in response to a Conduit method call. {>} (PhutilJSONParserException)
chad@phac-dev:/var/www/html/dev/phabricator$ echo '{ "transactions": [ { "type": "parent", "value": "PHID-TASK-lwjxr2vkjzmgrhk3ojej" }, { "type": "title", "value": "test" } ] }' | arc call-conduit maniphest.edit --conduit-uri http://local.phacility.com/ Exception Host returned HTTP/200, but invalid JSON data in response to a Conduit method call. Parse error on line 1 at column 0: Expected one of: 'STRING', 'NUMBER', 'NULL', 'TRUE', 'FALSE', '{', '[' (Run with `--trace` for a full exception trace.)
Check for junk in your error log, or make the call from the web UI? I can apply + poke if that doesn't reveal anything.
EXCEPTION: (Error) Call to a member function getIsCreateTransaction() on null
I see this a lot where I want to validate something on creation, but seems like it not supported. Should I move this into a validateAllTransactions check up in Editor?
src/applications/maniphest/xaction/ManiphestTaskParentTransaction.php | ||
---|---|---|
51 | Presumably, it's arising from here? $this->isCreateTransaction() isn't valid in this context. You need to validate $xactions -- a possibly-empty list of transactions, not $this. The $this object is intentionally not associated with any specific transaction. This is a little odd (most of the TransactionType methods are about $this) but I think the alternative is needing to create a bunch of TransactionTypeValidatingThing classes or something like that. You can test if you're acting during creation by doing either of these:
Upshot: use isNewObject() instead. |
{"error":"ERR-CONDUIT-CORE","errorMessage":"ERR-CONDUIT-CORE: Validation errors:\n - Parent task identifier \"cat\" does not identify a visible task.","response":null}
src/applications/maniphest/xaction/ManiphestTaskParentTransaction.php | ||
---|---|---|
51 | What is the reason behind disallowing updating the parent of a task? I wrote a Go script that pushes tasks into phabricator from another source, and tasks may change parents - this verification blocks the task's parents from being updated. Also surprised to see that the API limits the number of parents to 1. Is this intentional or just something that never got implemented? |