Page MenuHomePhabricator

Add a trigger rule to reassign a task
ClosedPublic

Authored by amckinley on Mar 26 2019, 10:35 PM.

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:

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

amckinley created this revision.Mar 26 2019, 10:35 PM
amckinley edited the summary of this revision. (Show Details)Mar 26 2019, 10:35 PM
amckinley requested review of this revision.Mar 26 2019, 10:36 PM

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?

amckinley added inline comments.Mar 26 2019, 10:49 PM
src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php
80

😬

amckinley added inline comments.Mar 26 2019, 10:55 PM
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.

amckinley marked 2 inline comments as done.Mar 26 2019, 10:55 PM
amckinley updated this revision to Diff 48526.Mar 26 2019, 11:06 PM

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.

amckinley updated this revision to Diff 48620.Apr 4 2019, 11:33 PM

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

amckinley edited the summary of this revision. (Show Details)Apr 4 2019, 11:34 PM
amckinley edited the summary of this revision. (Show Details)Apr 4 2019, 11:35 PM

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:
  • 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:
epriestley accepted this revision.Apr 4 2019, 11:45 PM

(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

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

125

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 marked an inline comment as done.Apr 5 2019, 3:56 PM
amckinley added inline comments.
src/applications/project/storage/PhabricatorProjectTrigger.php
85

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.

amckinley added inline comments.Apr 5 2019, 3:58 PM
src/applications/project/storage/PhabricatorProjectTrigger.php
85

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.

amckinley updated this revision to Diff 48621.Apr 5 2019, 4:14 PM

Change method name, remove extraneous argument.

This revision was automatically updated to reflect the committed changes.