Configurable warning when not subscribing a mailing list
Closed, ResolvedPublic

Description

In the LLVM project, code reviews always need to go to one of 2 public mailing lists (either llvm-commits or cfe-commits). The problem is that people using phabricator often forget to add one of those lists, which is the primary way how other developers discover and follow code reviews that are going on.
It would be great if we had a way to configure that if somebody creates a patch without cc'ing one of the lists to pop up a window saying "you did not add llvm-commits or cfe-dev as subscribers - do you want to do a private review?"

klimek created this task.Jun 27 2014, 7:35 AM
klimek updated the task description. (Show Details)
klimek raised the priority of this task from to Needs Triage.
klimek added a subscriber: klimek.

I don't immediately see a reasonable approach to implement this in a general way. In other cases, I think rules like this are usually expressed as "always use Herald to CC <some list>", but it sounds like there is no way to automatically determine the correct list in this case?

We have a buildRevisionWarnings() method, and we can move that into CustomFields and then I can write you a tiny piece of plugin code. This wouldn't prompt or pop a dialog, but would look like this:

After T4631, this could also prompt from the CLI on the arc diff workflow, similar to the existing "You didn't add any reviewers, do you want to continue anyway?" prompt:

$ arc diff
...
...

You did not CC llvm-commits or cfe-commits. If you continue,
you will create a private review. Continue anyway? [y/N]

Would those cover your use case?

That definitely sounds like a very good first step. We would reevaluate how often unintentional private reviews happen afterwards. Thanks!

epriestley edited this Maniphest Task.Jun 27 2014, 3:35 PM
emaste added a subscriber: emaste.Jun 27 2014, 8:09 PM
epriestley edited this Maniphest Task.Jul 2 2014, 2:06 AM
epriestley edited this Maniphest Task.Jul 2 2014, 11:58 AM

Okay, you should be able to drop something like this in phabricator/src/extensions/:

<?php

final class MailingListWarningCustomField extends DifferentialCustomField {

  public function getFieldKey() {
    return 'mycompany:mycustomfield';
  }

  public function shouldAppearInPropertyView() {
    // This is required or the field won't be loaded on the detail page.
    return true;
  }

  public function renderPropertyViewValue() {
    // This prevents it from actually rendering a property.
    return null;
  }

  public function getWarningsForRevisionHeader() {
    $revision = $this->getObject();

    $ccs = PhabricatorSubscribersQuery::loadSubscribersForPHID(
      $revision->getPHID());

    $warnings = array();
    if (true) {
      $warnings[] = pht(
        "You have not added X@lists or Y@lists to CC.");
    }

    return $warnings;
  }

}

The $ccs will be a list of CC'd PHIDs, so you could reasonably hard-code the list PHIDs, look for them in $ccs, and add a warning if they're not present. It will look like this (yellow header bar):

Oh, this line:

public function getWarningsForRevisionHeader() {

...should actually be:

public function getWarningsForRevisionHeader(array $handles) {

Erk, this one too:

public function renderPropertyViewValue() {

Should be:

public function renderPropertyViewValue(array $handles) {
epriestley triaged this task as Wishlist priority.Jul 12 2014, 2:14 PM
jarrpa added a subscriber: jarrpa.Jul 22 2014, 3:14 PM
klimek added a comment.Aug 4 2014, 3:24 PM

Ah, this works, but only after-the-fact. The problem is when we add a mailing list to cc' after the fact, the mailing list doesn't get a mail that looks like the "request for review" mail, so people on the list are missing context (which is another thing we want to fix but haven't gotten around to).

klimek added a comment.Aug 4 2014, 3:43 PM

Actually works quite nice (I have worded the text to suggest to create a new revision). Now I have to find out where I can make the warning field screaming red ;)

epriestley closed this task as Resolved.Aug 6 2014, 10:05 PM
epriestley claimed this task.

I'm going to close this since I think T5786 covers the next steps.