Page MenuHomePhabricator

Make a workflow that integrates nicely with list based email/patch reviews
Open, Needs TriagePublic

Description

Hi folks,

I think it's time to bring up this issue again, as I was made aware that our LLVM instance is not the only one that has similar issues with an open source crowd that comes from email/patch based reviews: FreeBSD people have told us they have similar problems and are also running a phab instance that they have to patch for that reason. So perhaps we can now make a better case for improving the email review compatibility of phab (and accept patches to that end) :)

Why email-based reviews?
Many people have set up nicely working workflows around patch+email reviews; note that this is also an accessibility problem - I know blind people who need to do all their work in emacs, and email is much easier to handle that way than a web app.

Features needed:

  • ability to enforce selecting a mailing list to cc
  • replies to review comments and the patch should be in the same email
  • control over what gets sent - sending everything somebody does in the app that is not relevant to the review is not adding value

We'd be happy to help with implementing things; we're looking for consensus that the feature set is desirable, and thus happiness to merge the features.

I'll lead the FreeBSD folks here in case they have more input.

Event Timeline

klimek created this task.May 28 2015, 8:23 AM
klimek raised the priority of this task from to Needs Triage.
klimek updated the task description. (Show Details)
klimek added a subscriber: klimek.
emaste added a subscriber: emaste.May 28 2015, 12:49 PM
asl added a subscriber: asl.May 28 2015, 2:21 PM

ability to enforce selecting a mailing list to cc

I think this probably remains too niche and complex to have a place in the upstream.

replies to review comments and the patch should be in the same email

This might be implementable as a Custom Field (i.e., an extension instead of a patch) now, which could reduce the cost of maintaining it. It's possible we could upstream it after T6030.

control over what gets sent - sending everything somebody does in the app that is not relevant to the review is not adding value

T5791 and T4103 are the paths forward for this in the upstream, depending on how much control you want.

ability to enforce selecting a mailing list to cc

I think this probably remains too niche and complex to have a place in the upstream.

replies to review comments and the patch should be in the same email

This might be implementable as a Custom Field (i.e., an extension instead of a patch) now, which could reduce the cost of maintaining it. It's possible we could upstream it after T6030.

control over what gets sent - sending everything somebody does in the app that is not relevant to the review is not adding value

T5791 and T4103 are the paths forward for this in the upstream, depending on how much control you want.

I think I don't understand how that would work yet; the control we need is basically full control over emails:

  • we want to decide which emails just do not get sent, because a list will be cc'ed
  • we need to be able to basically replace everything that goes into an email (our patch does if (ourSetting) { our-email } else { defaultBehavior }, which is what makes merging so hard if some of the stuff moves)

we want to decide which emails just do not get sent, because a list will be cc'ed

T5791 will let you write global Herald rules for outbound mail, like:

When [all] of these conditions match:
[Sending application][is][Differential]
[Recipients][include any of][mailing list a, mailing list b]
[Transactions][do not include][create, update, comment, accept, reject]

Take these actions:
[Do not deliver this message to][mailing list a, mailing list b]

If you don't need this level of power and the existing SettingsEmail Preferences controls are sufficient, you could possibly put the mailing list email address on a user account, then log in with that user and set their preferences.

I don't plan to give mailing lists their own Email Preferences panel, nor to implement any set of controls with a level of power between "Email Preferences" and "Herald".


we need to be able to basically replace everything that goes into an email

At HEAD, most of the default mail behavior is defined by custom fields, which can be disabled and reordered. Disabling a custom field removes that section of the mail. Specifically, you can edit differential.fields in Config to review, reorder, and disable fields.

These sections are currently not driven by custom fields: the actual transaction messages, INLINE COMMENTS, CHANGED PRIOR TO COMMIT, REVISION DETAIL, AFFECTED FILES, CHANGE DETAILS.

If you want to remove some of these sections, I'm comfortable with upstreaming changes which move the sections -- other than the main transaction section -- to custom fields, provided the changes themselves aren't an unreasonably hacky mess. For some sections (like REVISION DETAIL), this is trivial. For others, it may not be. This is a path forward to let you remove sections from email via configuration, without maintaining patches.

Similarly, you can add new sections by defining custom fields. These sections are invoked via addCustomFieldsToMailBody() and can live entirely in extension code. See D12997/D13000/D12993 for some recent examples in Diffusion.

This may or may not be helpful/useful in reducing or eliminating how much patch code you maintain, but I don't plan to make email construction completely modular and we're generally satisfied with how email works from the perspective of the upstream and feedback from all other installs. If you really want to replace the entire email, you can probably maintain a one-line patch which just does return build_my_mail_body_instead($object, $xactions, $body); near the top of DifferentialTransactionEditor->buildMailBody() and put everything else in an extension.

we want to decide which emails just do not get sent, because a list will be cc'ed

T5791 will let you write global Herald rules for outbound mail, like:

When [all] of these conditions match:
[Sending application][is][Differential]
[Recipients][include any of][mailing list a, mailing list b]
[Transactions][do not include][create, update, comment, accept, reject]
Take these actions:
[Do not deliver this message to][mailing list a, mailing list b]

If you don't need this level of power and the existing SettingsEmail Preferences controls are sufficient, you could possibly put the mailing list email address on a user account, then log in with that user and set their preferences.
I don't plan to give mailing lists their own Email Preferences panel, nor to implement any set of controls with a level of power between "Email Preferences" and "Herald".

That example sounds like pretty much what we need. That would be awesome.


we need to be able to basically replace everything that goes into an email

At HEAD, most of the default mail behavior is defined by custom fields, which can be disabled and reordered. Disabling a custom field removes that section of the mail. Specifically, you can edit differential.fields in Config to review, reorder, and disable fields.
These sections are currently not driven by custom fields: the actual transaction messages, INLINE COMMENTS, CHANGED PRIOR TO COMMIT, REVISION DETAIL, AFFECTED FILES, CHANGE DETAILS.
If you want to remove some of these sections, I'm comfortable with upstreaming changes which move the sections -- other than the main transaction section -- to custom fields, provided the changes themselves aren't an unreasonably hacky mess. For some sections (like REVISION DETAIL), this is trivial. For others, it may not be. This is a path forward to let you remove sections from email via configuration, without maintaining patches.
Similarly, you can add new sections by defining custom fields. These sections are invoked via addCustomFieldsToMailBody() and can live entirely in extension code. See D12997/D13000/D12993 for some recent examples in Diffusion.
This may or may not be helpful/useful in reducing or eliminating how much patch code you maintain, but I don't plan to make email construction completely modular and we're generally satisfied with how email works from the perspective of the upstream and feedback from all other installs. If you really want to replace the entire email, you can probably maintain a one-line patch which just does return build_my_mail_body_instead($object, $xactions, $body); near the top of DifferentialTransactionEditor->buildMailBody() and put everything else in an extension.

The problem with that is again the merging - not in the sense that we get conflicts, but that I've found the "core API" to change regularly (which I understand well from a dev point of view ;) and it's really hard to figure out where to get the data from after something changed...

But I'll play around with the custom fields for email and see whether that gives us what we need first (I'll need to actually look at what patches we still have first)

chad renamed this task from Make a workflow that integrates nicely with list based email/patch reviews. to Make a workflow that integrates nicely with list based email/patch reviews.May 28 2015, 5:00 PM
chad added a project: Herald.
eadler added a subscriber: eadler.May 28 2015, 5:53 PM

I don't plan to give mailing lists their own Email Preferences panel

I think everyone got roped into the other tasks, but for completeness: plans changed in T8387 and mailing lists are now "mailing list users", which means administrators have access to this panel and can adjust settings. So this might serve as a reasonable stopgap until T5791.

eadler moved this task from Backlog to Nice To Have on the FreeBSD board.Jun 14 2015, 4:39 AM
klimek added a comment.Jul 2 2015, 8:13 AM

we want to decide which emails just do not get sent, because a list will be cc'ed

T5791 will let you write global Herald rules for outbound mail, like:

When [all] of these conditions match:
[Sending application][is][Differential]
[Recipients][include any of][mailing list a, mailing list b]
[Transactions][do not include][create, update, comment, accept, reject]
Take these actions:
[Do not deliver this message to][mailing list a, mailing list b]

If you don't need this level of power and the existing SettingsEmail Preferences controls are sufficient, you could possibly put the mailing list email address on a user account, then log in with that user and set their preferences.

I don't plan to give mailing lists their own Email Preferences panel, nor to implement any set of controls with a level of power between "Email Preferences" and "Herald".

we need to be able to basically replace everything that goes into an email

At HEAD, most of the default mail behavior is defined by custom fields, which can be disabled and reordered. Disabling a custom field removes that section of the mail. Specifically, you can edit differential.fields in Config to review, reorder, and disable fields.
These sections are currently not driven by custom fields: the actual transaction messages, INLINE COMMENTS, CHANGED PRIOR TO COMMIT, REVISION DETAIL, AFFECTED FILES, CHANGE DETAILS.

I'm currently playing around with this (as integrating broke our minimal email mode in a non-trivial way). We're getting very close to being able to get rid of most of our customizations - there are currently 2 things that I can't switch off (or I'm missing something):

  • the actions at the top of the mail (never want to see those)
  • the email prefs at the bottom (for people on the mailing list who don't have a phab account those are unnecessary)

The one change I needed was to special-case empty headers in text sections, with that, the emails start looking pretty nice.

we want to decide which emails just do not get sent, because a list will be cc'ed

T5791 will let you write global Herald rules for outbound mail, like:

When [all] of these conditions match:
[Sending application][is][Differential]
[Recipients][include any of][mailing list a, mailing list b]
[Transactions][do not include][create, update, comment, accept, reject]
Take these actions:
[Do not deliver this message to][mailing list a, mailing list b]

If you don't need this level of power and the existing SettingsEmail Preferences controls are sufficient, you could possibly put the mailing list email address on a user account, then log in with that user and set their preferences.

I don't plan to give mailing lists their own Email Preferences panel, nor to implement any set of controls with a level of power between "Email Preferences" and "Herald".

we need to be able to basically replace everything that goes into an email

At HEAD, most of the default mail behavior is defined by custom fields, which can be disabled and reordered. Disabling a custom field removes that section of the mail. Specifically, you can edit differential.fields in Config to review, reorder, and disable fields.
These sections are currently not driven by custom fields: the actual transaction messages, INLINE COMMENTS, CHANGED PRIOR TO COMMIT, REVISION DETAIL, AFFECTED FILES, CHANGE DETAILS.

I'm currently playing around with this (as integrating broke our minimal email mode in a non-trivial way). We're getting very close to being able to get rid of most of our customizations - there are currently 2 things that I can't switch off (or I'm missing something):

  • the actions at the top of the mail (never want to see those)
  • the email prefs at the bottom (for people on the mailing list who don't have a phab account those are unnecessary)

Just found out this is already doable by reading the code. Apparently you violated your own rules of "don't add an option" there ;)

emaste added a comment.Jul 2 2015, 8:21 PM

A related request from FreeBSD is the ability to add arbitrary email addresses to the CC.

eadler moved this task from Backlog to Important on the llvm board.