Page MenuHomePhabricator

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

Authored by epriestley on Apr 13 2019, 4:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Mar 27, 1:57 AM
Unknown Object (File)
Wed, Mar 20, 8:33 PM
Unknown Object (File)
Feb 24 2024, 5:09 PM
Unknown Object (File)
Feb 3 2024, 10:07 PM
Unknown Object (File)
Jan 25 2024, 2:03 AM
Unknown Object (File)
Jan 1 2024, 9:44 PM
Unknown Object (File)
Dec 22 2023, 2:02 AM
Unknown Object (File)
Dec 10 2023, 3:26 PM
Subscribers
None

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

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
Branch
trigger2
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 22581
Build 30934: Run Core Tests
Build 30933: arc lint + arc unit

Event Timeline

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
src/applications/project/trigger/PhabricatorProjectTriggerManiphestOwnerRule.php
38

Hmm, it does for me?

Screen Shot 2019-04-17 at 12.27.12 PM.png (262×1 px, 29 KB)

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.
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.