Page MenuHomePhabricator

Update Maniphest for modular transactions
ClosedPublic

Authored by chad on May 8 2017, 5:28 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Apr 21, 7:30 PM
Unknown Object (File)
Sat, Apr 20, 1:50 AM
Unknown Object (File)
Fri, Apr 19, 6:27 PM
Unknown Object (File)
Thu, Apr 11, 8:36 AM
Unknown Object (File)
Thu, Apr 11, 2:43 AM
Unknown Object (File)
Sun, Apr 7, 5:25 PM
Unknown Object (File)
Fri, Apr 5, 6:52 PM
Unknown Object (File)
Thu, Apr 4, 12:06 AM

Details

Summary

Ref T12671. This modernized Maniphest transactions to modular transactions.

Test Plan
  • 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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

chad planned changes to this revision.May 8 2017, 5:29 PM
  • punt a little further
chad planned changes to this revision.May 8 2017, 6:20 PM

I think this and projects will need to be landed sequentially.

  • about as far as I can go

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).

chad edited the test plan for this revision. (Show Details)
  • missed a transaction here
chad retitled this revision from [WIP] Update Maniphest for modular transactions to Update Maniphest for modular transactions.May 11 2017, 10:17 PM
  • use renderValue
  • fix "Points" always rendering a transaction.

I do think this all reasonably works, but I haven't tested email commands, herald... will keep testing.

hazelyang added inline comments.
src/applications/maniphest/command/ManiphestPriorityEmailCommand.php
72

1

src/applications/maniphest/command/ManiphestStatusEmailCommand.php
71

2

src/applications/maniphest/conduit/ManiphestConduitAPIMethod.php
67

3

I'm being lazy about looking at this until after the release cut. Zzz

Don't even trip. You got this.

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.

This revision is now accepted and ready to land.May 15 2017, 1:59 PM

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.

derp, forgot to check error log

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
52

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:

  • Call $this->isNewObject() (probably what you want).
  • Call $xaction->isCreateTransaction() on each transaction (sort of equivalent, but not really ideal).

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}

This revision was automatically updated to reflect the committed changes.
danieldanciu added inline comments.
src/applications/maniphest/xaction/ManiphestTaskParentTransaction.php
52

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?