From 7593a265d5931877d38756282ddd66daf6ec99b8 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 9 Sep 2019 13:09:46 -0700
Subject: [PATCH] When Herald changes object subscribers, always hide the feed
 story

Summary:
Fixes T8952. These feed stories are not interesting and tend to be generated as collateral damage when a non-story update is made to an old task and someone has a "subscribe me" Herald rule.

Also clean up some of the Herald field/condition indexing behavior slightly.

Test Plan: Wrote a "Subscribe X" herald rule, made a trivial update to a task. Before: low-value feed story; after: no feed story.

Maniphest Tasks: T8952

Differential Revision: https://secure.phabricator.com/D20797
---
 src/applications/herald/field/HeraldField.php | 23 ++++++++++---------
 .../PhabricatorApplicationTransaction.php     | 13 ++++++-----
 2 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/src/applications/herald/field/HeraldField.php b/src/applications/herald/field/HeraldField.php
index 3df242712e..6f14091aff 100644
--- a/src/applications/herald/field/HeraldField.php
+++ b/src/applications/herald/field/HeraldField.php
@@ -177,25 +177,26 @@ abstract class HeraldField extends Phobject {
   }
 
   public function getPHIDsAffectedByCondition(HeraldCondition $condition) {
-    $phids = array();
+    try {
+      $standard_type = $this->getHeraldFieldStandardType();
+    } catch (PhutilMethodNotImplementedException $ex) {
+      $standard_type = null;
+    }
 
-    $standard_type = $this->getHeraldFieldStandardType();
     switch ($standard_type) {
       case self::STANDARD_PHID:
       case self::STANDARD_PHID_NULLABLE:
-        $phid = $condition->getValue();
-        if ($phid) {
-          $phids[] = $phid;
-        }
-        break;
       case self::STANDARD_PHID_LIST:
-        foreach ($condition->getValue() as $phid) {
-          $phids[] = $phid;
+        $phids = $condition->getValue();
+
+        if (!is_array($phids)) {
+          $phids = array();
         }
-        break;
+
+        return $phids;
     }
 
-    return $phids;
+    return array();
   }
 
   final public function setAdapter(HeraldAdapter $adapter) {
diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
index 7c327393f8..5fa4562164 100644
--- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
+++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php
@@ -775,6 +775,13 @@ abstract class PhabricatorApplicationTransaction
       case PhabricatorTransactions::TYPE_TOKEN:
       case PhabricatorTransactions::TYPE_MFA:
         return true;
+      case PhabricatorTransactions::TYPE_SUBSCRIBERS:
+        // See T8952. When an application (usually Herald) modifies
+        // subscribers, this tends to be very uninteresting.
+        if ($this->isApplicationAuthor()) {
+          return true;
+        }
+        break;
       case PhabricatorTransactions::TYPE_EDGE:
         $edge_type = $this->getMetadataValue('edge:type');
         switch ($edge_type) {
@@ -1387,12 +1394,6 @@ abstract class PhabricatorApplicationTransaction
           return 25;
         }
 
-        if ($this->isApplicationAuthor()) {
-          // When applications (most often: Herald) change subscriptions it
-          // is very uninteresting.
-          return 1;
-        }
-
         // In other cases, subscriptions are more interesting than comments
         // (which are shown anyway) but less interesting than any other type of
         // transaction.