Workboards - add transactions for column changes
ClosedPublic
Actions

Authored by btrahan on Feb 27 2014, 9:56 PM.

Details

Reviewers

epriestley

Maniphest Tasks

T4422: Changing Workboards should generate a transaction

Commits

Restricted Diffusion Commit
rPd1e64e64ffff: Workboards - add transactions for column changes

Summary

adds ManiphestTransaction::TYPE_PROJECT_COLUMN and makes it work. Had to clean up the Javascript ever so slightly as it was sending up the string "null" when it should just omit the data in these cases. Ref T4422.

NOTE: this is overall a bit buggy - e.g. move a task Foo from column A to top of column B; refresh; task Foo is at bottom of column B and should be at top of column B - but I plan on additional diff or three to clean this up.

Test Plan

dragged tasks around columns. clicked on those tasks and saw some nice transactions.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

...I had to take javelinsymbols out of the lint path after failing to compile it for awhile =(

One inline; it would be slightly cleaner to get the oldValue and newValue in the same format, rather than shoving old information in the newValue.

src/applications/project/controller/PhabricatorProjectMoveController.php
74–77	It would be cleaner to have both the old and new values have `columnPHIDs` and `projectPHID`, maybe?

This revision is now accepted and ready to land.Feb 27 2014, 10:08 PM

I am maybe missing what you mean...?

src/applications/project/controller/PhabricatorProjectMoveController.php
74–77	I originally had a good ole "setOldValue" here that made much more sense to me but application transactions seems designed to not let you setOldValue...? EXCEPTION: (Exception) You can not apply transactions which already have oldValue! at [/Users/btrahan/Dropbox/code/phalanx/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:759] e.g. ManiphestTransaction::TYPE_EDGE has an exception that is noted in the code as bunk crud. The alternative I see is to have getCustomTransactionOldValue in the ManiphestEditor issue a query to get the edge phids, but since that won't be done transactionally I figured it would be better to have the data query in the controller. getCustomTransactionOldValue issuing a query generally seemed like poor form to me, but I am noob. I also feel like this might be complex / hard to get right since a task can be in many projects (and thus many columns) though there is a 1 task : 1 project column relationship.

The alternative I see is to have getCustomTransactionOldValue in the ManiphestEditor issue a query to get the edge phids

This is the approach I was considering, but you're right that it's pretty messy.

How about we make the setOldValue() restriction overridable? So we do something like:

Add some method like shouldGenerateOldValue(), which defaults to true, to Transaction.
Override it for the special cases (CUSTOMFIELD, and something else IIRC?).
Override it for this.
If it returns false, don't throw the exception (instead, throw if the old value isn't set, I guess?)
If it returns false, don't call setOldValue/getOldValue/etc stuff.

The only intent of the current behavior is so you don't accidentally setOldValue(), expect it to do something, and have that overridden, and so that the 95% of transactions which can easily figure out the right old value don't have to duplicate any code. There's no real reason we can't put more transactions in the "prefill" set, though.

(There's also a small argument for transctional stuff, but we could wrap all of the edit in a read lock or something if we really needed to, and I think this is completely irrlevant in practice.)