From b7297a3278c65dcbf7dc7806831e1d20f133cb97 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Sat, 5 Oct 2013 12:04:31 -0700
Subject: [PATCH] Add an "Author's projects" Herald field to Differential

Summary:
Ref T1279. This allows installs to implement two different flavors of project review. They can either implement this rule:

  When:
    [ ... ] [ ... ]
  Take Action:
    [ Add blockign reviewers ] [ Security ]

...which means "every revision matching X needs to be signed off by someone else on the Security team, //even if the author is on that team//". The alternative is to implement this rule:

  When:
    [ Author's projects ] [ do not include ] [ Security ]
    [ ... ] [ ... ]
  Take Action:
    [ Add blocking reviewers ] [ Security ]

...which means that people on the Security team don't need a separate signoff from someone else on the team.

I think this weaker version maps to some of what, e.g., Google does (you need to be reviewed by someone with "readability" in a language, but if you have it that's good enough), but I could imagine cases like "Security" wanting to prevent self-review from satisfying the requirement.

@zeeg, not sure which of these use cases is relevant here, but either one should work after this.

Test Plan: Created rules with this field, verified it populated properly in the transcript.

Reviewers: btrahan

Reviewed By: btrahan

CC: zeeg, aran

Maniphest Tasks: T1279

Differential Revision: https://secure.phabricator.com/D7238
---
 src/applications/herald/adapter/HeraldAdapter.php   |  7 ++++++-
 .../adapter/HeraldDifferentialRevisionAdapter.php   | 13 +++++++++++++
 src/applications/herald/storage/HeraldRule.php      |  2 +-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php
index 9be426d48a..8edc3fe204 100644
--- a/src/applications/herald/adapter/HeraldAdapter.php
+++ b/src/applications/herald/adapter/HeraldAdapter.php
@@ -23,6 +23,7 @@ abstract class HeraldAdapter {
   const FIELD_AFFECTED_PACKAGE_OWNER = 'affected-package-owner';
   const FIELD_CONTENT_SOURCE         = 'contentsource';
   const FIELD_ALWAYS                 = 'always';
+  const FIELD_AUTHOR_PROJECTS        = 'authorprojects';
 
   const CONDITION_CONTAINS        = 'contains';
   const CONDITION_NOT_CONTAINS    = '!contains';
@@ -149,6 +150,7 @@ abstract class HeraldAdapter {
         pht("Any affected package's owner"),
       self::FIELD_CONTENT_SOURCE => pht('Content Source'),
       self::FIELD_ALWAYS => pht('Always'),
+      self::FIELD_AUTHOR_PROJECTS => pht("Author's projects"),
     );
   }
 
@@ -166,7 +168,7 @@ abstract class HeraldAdapter {
       self::CONDITION_IS_NOT_ANY      => pht('is not any of'),
       self::CONDITION_INCLUDE_ALL     => pht('include all of'),
       self::CONDITION_INCLUDE_ANY     => pht('include any of'),
-      self::CONDITION_INCLUDE_NONE    => pht('include none of'),
+      self::CONDITION_INCLUDE_NONE    => pht('do not include'),
       self::CONDITION_IS_ME           => pht('is myself'),
       self::CONDITION_IS_NOT_ME       => pht('is not myself'),
       self::CONDITION_REGEXP          => pht('matches regexp'),
@@ -201,6 +203,7 @@ abstract class HeraldAdapter {
       case self::FIELD_TAGS:
       case self::FIELD_REVIEWERS:
       case self::FIELD_CC:
+      case self::FIELD_AUTHOR_PROJECTS:
         return array(
           self::CONDITION_INCLUDE_ALL,
           self::CONDITION_INCLUDE_ANY,
@@ -588,6 +591,8 @@ abstract class HeraldAdapter {
             return self::VALUE_TAG;
           case self::FIELD_AFFECTED_PACKAGE:
             return self::VALUE_OWNERS_PACKAGE;
+          case self::FIELD_AUTHOR_PROJECTS:
+            return self::VALUE_PROJECT;
           default:
             return self::VALUE_USER;
         }
diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
index 06a5bf0683..8c0ce70c98 100644
--- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
+++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
@@ -43,6 +43,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
         self::FIELD_TITLE,
         self::FIELD_BODY,
         self::FIELD_AUTHOR,
+        self::FIELD_AUTHOR_PROJECTS,
         self::FIELD_REVIEWERS,
         self::FIELD_CC,
         self::FIELD_REPOSITORY,
@@ -287,6 +288,18 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
       case self::FIELD_AUTHOR:
         return $this->revision->getAuthorPHID();
         break;
+      case self::FIELD_AUTHOR_PROJECTS:
+        $author_phid = $this->revision->getAuthorPHID();
+        if (!$author_phid) {
+          return array();
+        }
+
+        $projects = id(new PhabricatorProjectQuery())
+          ->setViewer(PhabricatorUser::getOmnipotentUser())
+          ->withMemberPHIDs(array($author_phid))
+          ->execute();
+
+        return mpull($projects, 'getPHID');
       case self::FIELD_DIFF_FILE:
         return $this->loadAffectedPaths();
       case self::FIELD_CC:
diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php
index 52089c20e2..57b1974d7f 100644
--- a/src/applications/herald/storage/HeraldRule.php
+++ b/src/applications/herald/storage/HeraldRule.php
@@ -13,7 +13,7 @@ final class HeraldRule extends HeraldDAO
   protected $repetitionPolicy;
   protected $ruleType;
 
-  protected $configVersion = 12;
+  protected $configVersion = 13;
 
   private $ruleApplied = self::ATTACHABLE; // phids for which this rule has been applied
   private $validAuthor = self::ATTACHABLE;