From 8599145b5eecb4e63fcab660aaa4088fd8410a98 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 19 Feb 2015 10:38:05 -0800
Subject: [PATCH] Implement more consistent publishing rules for repositories

Summary:
Ref T7298. We are currently inconsistent about when we publish feed, email, notifications, audits and Herald rules.

Specifically, there are two settings which impact these things:

  - The "importing" flag, which is set when we're importing old commits.
  - The "herald-disabled" flag, which was expanded in scope some time ago and now actually means "disable publishing".

Various parts of the pipeline were checking only one of these flags. Instead, all of them should check both.

(For example, we should never email users about importing repositories, nor trigger audits on them.)

Test Plan: See next revision.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7298

Differential Revision: https://secure.phabricator.com/D11826
---
 .../audit/editor/PhabricatorAuditEditor.php   |  9 ++++-----
 .../storage/PhabricatorRepository.php         | 19 +++++++++++++++++++
 ...habricatorRepositoryCommitOwnersWorker.php |  2 +-
 .../PhabricatorRepositoryPushMailWorker.php   |  7 +------
 4 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php
index 74e460afdf..81382071ca 100644
--- a/src/applications/audit/editor/PhabricatorAuditEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditEditor.php
@@ -524,12 +524,14 @@ final class PhabricatorAuditEditor
     array $xactions) {
 
     // not every code path loads the repository so tread carefully
+    // TODO: They should, and then we should simplify this.
     if ($object->getRepository($assert_attached = false)) {
       $repository = $object->getRepository();
-      if ($repository->isImporting()) {
+      if (!$repository->shouldPublish()) {
         return false;
       }
     }
+
     return $this->isCommitMostlyImported($object);
   }
 
@@ -822,10 +824,7 @@ final class PhabricatorAuditEditor
       switch ($xaction->getTransactionType()) {
         case PhabricatorAuditTransaction::TYPE_COMMIT:
           $repository = $object->getRepository();
-          if ($repository->isImporting()) {
-            return false;
-          }
-          if ($repository->getDetail('herald-disabled')) {
+          if (!$repository->shouldPublish()) {
             return false;
           }
           return true;
diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php
index 170914171d..fa40a4d96f 100644
--- a/src/applications/repository/storage/PhabricatorRepository.php
+++ b/src/applications/repository/storage/PhabricatorRepository.php
@@ -676,6 +676,25 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
   }
 
 
+  /**
+   * Should this repository publish feed, notifications, audits, and email?
+   *
+   * We do not publish information about repositories during initial import,
+   * or if the repository has been set not to publish.
+   */
+  public function shouldPublish() {
+    if ($this->isImporting()) {
+      return false;
+    }
+
+    if ($this->getDetail('disable-herald')) {
+      return false;
+    }
+
+    return true;
+  }
+
+
 /* -(  Autoclose  )---------------------------------------------------------- */
 
 
diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
index 550aec2758..9866273886 100644
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php
@@ -25,7 +25,7 @@ final class PhabricatorRepositoryCommitOwnersWorker
     PhabricatorRepository $repository,
     PhabricatorRepositoryCommit $commit) {
 
-    if ($repository->getDetail('herald-disabled')) {
+    if (!$repository->shouldPublish()) {
       return;
     }
 
diff --git a/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php b/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php
index a70ed055ac..4a56b6bc96 100644
--- a/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryPushMailWorker.php
@@ -22,16 +22,11 @@ final class PhabricatorRepositoryPushMailWorker
       ->executeOne();
 
     $repository = $event->getRepository();
-    if ($repository->isImporting()) {
+    if (!$repository->shouldPublish()) {
       // If the repository is still importing, don't send email.
       return;
     }
 
-    if ($repository->getDetail('herald-disabled')) {
-      // If publishing is disabled, don't send email.
-      return;
-    }
-
     $logs = $event->getLogs();
 
     list($ref_lines, $ref_list) = $this->renderRefs($logs);