Page MenuHomePhabricator

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

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

Details

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

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

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

epriestley created this revision.Sat, Apr 13, 4:33 PM
epriestley requested review of this revision.Sat, Apr 13, 4:35 PM
amckinley accepted this revision.Wed, Apr 17, 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.Wed, Apr 17, 6:18 PM
epriestley added inline comments.Wed, Apr 17, 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.

This revision was automatically updated to reflect the committed changes.
amckinley added inline comments.Wed, Apr 17, 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.