Distinguish between "bad record format" and "bad record value" when validating Trigger rules
ClosedPublic
Actions

Authored by epriestley on Apr 13 2019, 4:33 PM.

Details

Reviewers

amckinley

Maniphest Tasks

T13269: Improve initial implementations of Workboard triggers and groups

Commits

rP870b01f2d070: Distinguish between "bad record format" and "bad record value" when validating…

Summary

Depends on D20416. Ref T13269. See D20329.

If you try to save an "Assign to" rule with no assignee, we currently replace the control with an "InvalidRule" control that isn't editable. We'd prefer to give you an empty field back and let you pick a different value.

Differentiate between "bad record format" (i.e., we can't really do anything with this) and "bad record value" (i.e., everything is structurally fine, you just typed the wrong thing). In the latter case, we still build a properly typed rule for the UI, we just refuse to update storage until you fix the problem.

Test Plan

First, hit the original issue and got a nicer UI with a more consistent control width (note full-width error):

Screen Shot 2019-04-13 at 9.08.36 AM.png (1×1 px, 202 KB)

Then, applied the rest of the patch and got a normal "fix the issue" form state instead of a dead-end:

Screen Shot 2019-04-13 at 9.29.11 AM.png (952×1 px, 188 KB)

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Apr 13 2019, 4:33 PM

Harbormaster completed remote builds in B22581: Diff 48711.Apr 13 2019, 4:35 PM

epriestley requested review of this revision.Apr 13 2019, 4:35 PM

amckinley accepted this revision.Apr 17 2019, 6:18 PM

amckinley added inline comments.

src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php
38	Searching for `"none()"` in the tokenizer doesn't give you any results, FWIW. You have to search for "No Owner". I guess this error is only likely to ever be returned to hypothetical Conduit callers who would be passing the literal string `"none()"` around, though. (Or should they be passing `null`?)
51	I am pretty sure that we've always called `setViewer()` by now, but I'm not positive. If it worked when you tried to hit this error, that works for me.
src/applications/project/xaction/trigger/PhabricatorProjectTriggerRulesetTransaction.php
53	No-op `continue`.

This revision is now accepted and ready to land.Apr 17 2019, 6:18 PM

epriestley added inline comments.Apr 17 2019, 7:29 PM

src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php
38	Hmm, it does for me? I think either "none()" or "null" work via Conduit.

I think either "none()" or "null" work via Conduit.

Looks like only null works for the actual maniphest.edit transaction, since the actual "Assigned to: " field in Maniphest doesn't support "none()".

This is sort of vaguely inconsistent, but we need to distinguish between "No Query Constraint" and "Find Tasks With No Owner" in query contexts, so "no value" and "none()" need to be different values.

Arguably, this field (in triggers) should interpret "you didn't type anything" to mean "no owner", but in the past there was confusion in the bulk editor about how to unassign tasks when the flow had that behavior. I think that was changed, but now it has changed back with the most recent bulk edit update.

I guess I'm mostly pretty "¯\_(ツ)_/¯" about this until actual users hit it. Some of this behavior could probably be slightly more clear/consistent than it currently is, but none of it seems particularly bad or unintuitive or misleading or weird.

Closed by commit rP870b01f2d070: Distinguish between "bad record format" and "bad record value" when validating… (authored by epriestley). · Explain WhyApr 17 2019, 7:41 PM

This revision was automatically updated to reflect the committed changes.

amckinley added inline comments.Apr 17 2019, 7:45 PM

src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php
38	Huh, no longer reproduces for me. I definitely had to backspace twice to change "none" to "no" to get the tokenizer to work before, but it's fine now.