Page MenuHomePhabricator

Herald "block" and "send an email" actions are mutually exclusive, which isn't intuitive
Open, WishlistPublic


See PHI765. If you write a rule like this which has both a "block" action and a "send an email" action:

[ Author ] [ is ] [ Weasel ]
Take actions:
[ Block push with message ] [ No weasels allowed! ]
[ Send an email to ] [ ]

...the "send an email" action will never have any effect. When we block a push, we don't send email about it.

The intent of the "send an email" action is almost certainly "send an email when a push is blocked".

I think this is a generally reasonable thing to want.

In contrast, the intent with this rule:

[ Always ]
Take actions:
[ Send an email to ] [ ] almost certainly "send an email when a push is not blocked".

We could reasonably infer these intents, although we'd have to make some code changes to how "Send me an email" and "Block" work and manage data.

I don't love this because giving Herald rule actions contextual magic seems generally spooky, but I think we can pretty reasonably infer the intent in both cases.

Event Timeline

epriestley triaged this task as Normal priority.Jul 17 2018, 4:56 PM
epriestley created this task.
epriestley lowered the priority of this task from Normal to Wishlist.Jul 17 2018, 5:15 PM

I'm going to look at improving the push logs a bit to show which rule blocked a push to address the actual use case in PHI765, which makes this more of a distance nice-to-have.

The root use case here was a desire for a better sense of how often certain rules were firing, and we improved the push logs instead to address this more directly (D19555, D19556, D19557).

The "block" + "send email" interaction is still unintuitive, but we aren't currently aware of any use cases in the wild where there's interest in a sort of real-time alert that a push was blocked.

A sort of broad issue here is that Herald sometimes knows (or could know, or could guess, or maybe could speculate) that a rule won't do what you expect, but it doesn't tell you.

Here's a clear-cut example, from this task:

[ Block commit with message ][ ... ]
[ Any other action ][ ... ]

The "Block commit with message" action always (or at least "always") stops other actions from evaluating, so Herald could reasonably warn you that the "Any other action" action can "never" have any effect.

(I put "always" and "never" in quotes because they're true of all upstream actions, but it's possible to write a third-party action which does still have an effect.)

Here's a more complicated example:

[ Run build plan ][ X ]
[ Send an email to ][ Y ]

This rule will never do anything during revision creation while the revision is in draft, which isn't obvious: non-update actions will forbid "run build plan", and draft transactions will forbid "send an email to". However, this rule does work if the revision is updated after it is sent for review, which is a plausible (if very unlikely) intended behavior.

Here's another case:

[ Revision ][ is newly created ][ is true ]
Take actions:
[ Send an email to ][ Y ]

This rule essentially never works: "send an email to" is forbidden while the revision is a draft and "is newly created" is never true when a revision is not a draft. The right way to write this rule is "take actions the first time this rule matches: ...". In this case, there's some overlap with T8644.

I don't think we can realistically detect this stuff in the general case: there are too many interactions between conditions, actions, rule settings, other rules via [ Another Herald rule ], etc. We can't know or necessarily guess about the behavior of conditions or actions in general, as they may include third-party actions which cause side effects directly (rather than by emitting transactions).

I'm inclined to say that we should instead let conditions and actions generate documentation (similar to modern Datasources and Conduit API methods) and let them show some kind of generic warning hint icon in the rule editing UI that just means "this action has some interactions which you should familiarize yourself with". It would look something like this:

[ Block commit with message ][ ... ]                                           /!\

When you hover over the "" icon you get a tooltip like "When a commit is blocked, other actions usually do not execute. Click to learn more." and can click to jump to the action documentation page. Then it can spell out the interaction in detail. Since this will just be a little "Learn More" interaction, it's okay for us to over-communicate it.

We could do this for conditions too, then add these warnings, probably based on the object type?

  • [ Run build plan ]: On revisions, warn about draft mode. See also T13299.
  • [ Block commit ]: Warn that this stops most other actions, including all transactional actions.
  • [ Send an email ]: On revisions, warn about draft interactions.
  • [ Is newly created ]: Warn in general that "the first time this rule matches" is often a better fit, or just on revisions about draft interactions.

See PHI1977 for a somewhat-similar issue: a user was (probably) looking for an action available only in Global Herald rules, and didn't realize available actions depend on rule scope.

This could be more clear by allowing users to select actions (e.g., "Add auditors" from a personal rule) and then explaining why the action isn't available in that context ("You can only apply this action from a global rule. Create a global rule instead, or use "Add me as an auditor").

It looks like the case in PHI1977 was actually a situation of attempting to trigger an audit by writing a Differential rule, so the Global/Personal stuff may still be worth fixing but has zero known cases of actual confusion in the wild. I'm less sure how the UI could be clarified around the Audit/Differential issue.