Page MenuHomePhabricator

Differential email does not contain reviewers/subscribers information anymore
Closed, ResolvedPublic


I rencently upgraded our Phabricator instance ( from "(stable) Promote 2016 Week 43" to "(stable) Promote 2017 Week 2 0426ce73f0e63f1900f1cc285cfa1465ea72317e".

I noticed that information about reviewers/subscribers is not attached in the review request email anymore and wonder if there is a way to re-enable this? Thanks!



Note that the similar information about reviewers is missing in the new email, although they were set when the patch was created.

JDevlieghere added reviewers: hokein, Prazek, aaron.ballman, alexfh.
JDevlieghere added a subscriber: cfe-commits.
JDevlieghere set the repository for this revision to rL LLVM.
JDevlieghere added a project: clang-tools-extra.

Event Timeline

For context, this is a side effect of the move to EditEngine in T11114.

When an object is created, we apply a number of transactions to an empty object in order to create the initial state. Most of these transactions are redundant and uninteresting. If we showed them all, the initial email would read:

username set title to X.
username updated the summary.
username updated the test plan.
username set reviewers to Y.
username set subscribers to Z.
username set projects to Q.
username attached diff 1.
username set view policy to "All Users".
username set edit policy to "All Users".

Since most of these are never interesting, we hide the majority of them. Prior to EditEngine, this hiding was largely accomplished in a hacky, ad-hoc way on a transaction-by-transaction basis where some signpost value like null meant "the object didn't have a value before". As you might imagine, led to a lot of minor bugs and inconsistencies, especially when null was a supported field value.

After EditEngine, we flag creation-time transactions explicitly, and default to folding them into a single transaction:

username created this object.

There are some exceptions to this folding behavior:

  • We don't fold project tags in mail, for reasons similar to this request (T10493).
  • When policy or space transactions change the value from the default, even during creation, we do not fold them.

On the transactions, specifically:

  • Projects already has an exception, and doesn't fold. The "After" revision just didn't have any project tags.
  • I am not inclined to add an exception for subscribers. We haven't received any other feedback about users missing them after making similar changes in other applications, and they are normally visible in the "To" and/or "Cc" mail footer. Your install has disabled this footer (either by forking or with but I think this is the best approach for installs that want to include overall recipient information in mail.
  • I am not inclined to add an exception for repositories. This information is already present in the body of the message.
  • I think adding an exception for reviewers -- like the exception for projects -- is reasonable.

After a quick look, ModularTransactions doesn't currently expose a shouldHideFormMail() hook and I may want to do something a little more tailored here, so fixing this isn't a one-liner.

Thanks for the detailed explanation!

I agree with you - "reviewers" is the most interesting information. It would be really useful to have this in the email IMO.

neilfitz added a subscriber: neilfitz.

Our users are also missing this, since they can't tell whether they're a blocking/lone reviewer from the e-mail.

See also T5853 (distinguish between "blocking" and "reviewing" in email) and T10574 (provide more guidance around the pathway toward a revision becoming accepted).

(I don't think you could tell if you were a blocking vs nonblocking reviewer before, other than by inferring it from information you otherwise know about how you're likely to have become a reviewer for the change.)

For me, I often need to click into the link to check if I am a reviewer or a subscriber. It would be nice if I could know who are the reviewers by looking at the email.