Page MenuHomePhabricator

Add a trigger rule to reassign a task
ClosedPublic

Authored by amckinley on Mar 26 2019, 10:35 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 25, 7:39 AM
Unknown Object (File)
Mon, Mar 25, 7:39 AM
Unknown Object (File)
Mon, Mar 25, 7:39 AM
Unknown Object (File)
Mon, Mar 25, 7:39 AM
Unknown Object (File)
Mon, Mar 25, 7:39 AM
Unknown Object (File)
Mon, Mar 25, 7:39 AM
Unknown Object (File)
Sun, Mar 10, 6:38 AM
Unknown Object (File)
Sat, Mar 9, 9:18 AM
Subscribers

Details

Summary

Ref T13269. Workboard triggers can now reassign tasks on column drop. Also sprinkles some setViewer() calls in places that needed them.

This mostly works, but a few issues:

  • To set the owner to unassigned, you must explicitly put the "No Owner" token in the typeahead. Maybe this should just figure out you've put nothing in that field and set it for you?
  • I'm pretty sure this was already broken, but if you change the rule type from a tokenizer to a different type, the default for the field doesn't populate correctly:
    Screen Shot 2019-03-26 at 3.29.49 PM.png (434ร—804 px, 27 KB)

Also adds a new hook for trigger rules: getValueForField($value) which allows you to transform a value stored in the DB into a form suitable for setting on a form control.

Test Plan

Dragged tasks between columns and observed new owners as expected. Didn't try to get fancy to assign tasks to deleted users, users that the viewer can't see, bot users, etc etc. I'm relying on the underlying transaction to hopefully do the right thing.

Diff Detail

Repository
rP Phabricator
Branch
task-owner (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22397
Build 30642: Run Core Tests
Build 30641: arc lint + arc unit

Event Timeline

I'm pretty sure this was already broken, but if you change the rule type from a tokenizer to a different tpye, the default for the field doesn't populate correctly:

That's probably my fault.


Couple of inlines, do they fix any of the other issues?

src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php
47

Does adding an icon to the effect fix the alignment?

(No way to do user profile images for icons yet, but I'll probably add that at some point.)

Does this have to be, like:

$value = head($value);
if ($value === BLAH BLAH FUNCTION) {
  $value = null;
}

Desired behavior is that no preview shows if a drop would have no effect, so:

  • Drag a card assigned to "Alice" over an "Assign to Alice" column: expect no preview effect.
  • Drag an unassigned card over a "Unassign" column: expect no preview effect.

I think they'll both show right now since we'll compare PHID-USER-xyz vs array(PHID-USER-xyz), or null vs none().

80

Debugging leftover? ๐Ÿ˜Ž

93

For consistency, maybe strong tag this, without the colon?

src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php
80

๐Ÿ˜ฌ

src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php
47

Adding an icon fixes the alignment; yay!

No, it doesn't have to be that way. I was just making it work as-is without needing to add callbacks for mangling the value that gets returned by the tokenizer. Now I get how this is supposed to work; I'll hook it up correctly.

Makes preview behavior work correctly. Still doesn't quite do the right thing: the value is getting mangled when the rules come out of the DB, but not on the way in.

Makes preview behavior work correctly. Still doesn't quite do the right thing: the value is getting mangled when the rules come out of the DB, but not on the way in.

The main side effect of this is that the typeahead is getting populated with nonsense still.

Pretty much everything works now, including drop action previews. Only breakage is UI stuff when changing the type of an existing trigger rule.

This was about 100x more complicated than I expected because I went around and around about how to handle getting the viewer context down to the level of the rule objects. It turns out there are quite a few ways to generate trigger and rule objects, and some of them make more sense than others for explicitly passing $viewer around. Settled on manually setting the viewer inside the controllers that need to work with triggers.

I found two existing bugs while writing this:

  • Putting a trigger rule in an invalid state results in showing the validation warning, but renders the edit page as though the edit actually went through, making it not-obvious how to correct the error:
    Screen Shot 2019-04-04 at 4.39.56 PM.png (584ร—1 px, 70 KB)
  • Changing the type of an existing rule correctly changes the type of the corresponding field, but screws up the value. Example changing the type from task status to task owner:
    Screen Shot 2019-04-04 at 4.41.57 PM.png (360ร—688 px, 21 KB)

(I have a plan to fix that second bug and some idea of what to do about the first one.)

src/applications/project/storage/PhabricatorProjectTrigger.php
85 โ†—(On Diff #48620)

Can we make this (PhabricatorUser $viewer) and always pass it, or does that get tricky?

125 โ†—(On Diff #48620)

This doesn't actually take $viewer, I think.

src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php
13

Maybe be more specific, e.g. ...ForEditorField or ...ForEditControl or similar? ...ForField isn't immediately obvious to me.

This revision is now accepted and ready to land.Apr 4 2019, 11:45 PM
amckinley added inline comments.
src/applications/project/storage/PhabricatorProjectTrigger.php
85 โ†—(On Diff #48620)

We can; the biggest side effect is that PhabricatorProjectTrigger->getDropEffects() and PhabricatorProjectTrigger->getSoundEffects() also have to take $viewer. If you're fine with that, I'll do that and remove most (all?) of the setViewer() calls.

I probably like more explicit PhabricatorUser $viewer over = null since I think it makes it a little more obvious how to use the API, but not a real issue either way.

src/applications/project/storage/PhabricatorProjectTrigger.php
85 โ†—(On Diff #48620)

Oh right; and then that means PhabricatorProjectColumn->getDropEffects() also has to take $viewer, and that was the point where I gave up and decided this was creeping into too many callsites.

Haha, just leave it like this for now then and maybe we can clean it up late once more rules exist and we're confident everything is getting everywhere it needs to go.

Change method name, remove extraneous argument.

This revision was automatically updated to reflect the committed changes.