Differential "Drafts" and Herald events

Hello,

I try to follow Phabricator development, and have reviewed both https://secure.phabricator.com/T2543 and https://secure.phabricator.com/T13010. I have prototypes enabled, and am finding that people’s Personal Herald rules are not firing and adding the users as Subscribers or Reviewers when Differential drafts are filed, or when the Harbormaster builds pass and they stop being Drafts.

STEP BY STEP

I used arc diff to file differential object D750 on my local install. I’ve attached a slightly redacted copy of the Herald transcript from the creation of that object.

Unit and lint tests passed, and the patch immediately showed the following build status:

Lint (Green Star) Lint OK
Unit No Unit Test Coverage
Build Status (Green Checkmark) Buildable 850
(Green Checkmark) Build 850: arc lint + arc unit

Based on your own T13010, I believe that this means it is no longer a “Draft”. However, Herald’s “All Transcripts” page only shows the one transcript for this object.

I made an edit to the Differential object (added a space to the description) to force Herald to fire again. This did cause all of the Herald rules to fire correctly, except for one user who used the condition “Failed Is newly created is true”

QUESTIONS

When a Differential object’s builds pass and it stops being a Draft, are Herald rules supposed to fire again automatically (to potentially add reviewers and subscribers)?

How should those Herald rules be written to only fire once (the first time a Differential object stops being a Draft), so that if the user removes themselves as a review or subscriber, the Herald rule doesn’t immediately add them again?

If there’s any more information that I’m able to provide, please let me know. Versions below: (Stable from 2017 Week 46)

Phabricator commit rPa9489fe5a
Arcanist commit rARC622862e0
libphutil commit rPHU3510bfb

I think this is a legitimate bug with Drafts, as you surmise. We must run Herald to trigger builds before we can make a decision about promoting out of “Draft”, but if we do decide to promote out of “Draft” we don’t re-evaluate rules even though the outcomes may change after promotion.

A workaround is to disable the Draft mode by disabling phabricator.show-prototypes, although this will also disable other prototype applications. Making any edit to a revision should also put it into the right state, as you noted.

Rules which you want to fire exactly once should be written like this:

When...
  (Conditions)
Take these actions [only the first time] this rule matches:
  (Actions)

That is, use “only the first time”, not “every time”, to choose when to apply actions. In comparison to using an “Is newly created” condition, this will interact properly with drafts and will trigger properly if a revision initially fails to meet the conditions but later changes to meet them (for example, Diff 1 affects README.txt but Diff 2 affects README.txt and super-important-security-core.cpp).

If you have a rule which is using “Is newly created” to express an intent other than “send only one email”, let me know what the details are.

This probably won’t get fixed for this week’s release since it isn’t trivial and the release is promoting in a couple hours, but I’ll file it upstream and possibly get it into next week’s release. I’d like to find a way to avoid explicitly re-running Herald after automatic promotion out of “Draft” but that may be the least bad approach we have available.

Filed upstream as https://secure.phabricator.com/T13027.

A post was split to a new topic: Herald “send an email to” action not firing when revisions leave draft