Track how many columns use a particular trigger

Summary:
Ref T5474. In 99% of cases, a separate "archived/active" status for triggers probably doesn't make much sense: there's not much reason to ever disable/archive a trigger explcitly, and the archival rule is really just "is this trigger used by anything?".

(The one reason I can think of to disable a trigger manually is because you want to put something in a column and skip trigger rules, but you can already do this from the task detail page anyway, and disabling the trigger globally is a bad way to accomplish this if it's in use by other columns.)

Instead of adding a separate "status", just track how many columns a trigger is used by and consider it "inactive" if it is not used by any active columns.

Test Plan: This is slightly hard to test exhaustively since you can't share a trigger across multiple columns right now, but: rebuild indexes, poked around the trigger list and trigger details, added/removed triggers.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T5474

Differential Revision: https://secure.phabricator.com/D20308

resources/sql/autopatches/20190322.triggers.01.usage.sql

@@ -0,0 +1,8 @@
+CREATE TABLE {$NAMESPACE}_project.project_triggerusage (
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  triggerPHID VARBINARY(64) NOT NULL,
+  examplePHID VARBINARY(64),
+  columnCount INT UNSIGNED NOT NULL,
+  activeColumnCount INT UNSIGNED NOT NULL,
+  UNIQUE KEY `key_trigger` (triggerPHID)
+) ENGINE=InnoDB DEFAULT CHARSET={$CHARSET} COLLATE {$COLLATE_TEXT};

src/__phutil_library_map__.php

@@ -4193,6 +4193,8 @@ phutil_register_library_map(array(
     'PhabricatorProjectTriggerTransactionQuery' => 'applications/project/query/PhabricatorProjectTriggerTransactionQuery.php',
     'PhabricatorProjectTriggerTransactionType' => 'applications/project/xaction/trigger/PhabricatorProjectTriggerTransactionType.php',
     'PhabricatorProjectTriggerUnknownRule' => 'applications/project/trigger/PhabricatorProjectTriggerUnknownRule.php',
+    'PhabricatorProjectTriggerUsage' => 'applications/project/storage/PhabricatorProjectTriggerUsage.php',
+    'PhabricatorProjectTriggerUsageIndexEngineExtension' => 'applications/project/engineextension/PhabricatorProjectTriggerUsageIndexEngineExtension.php',
     'PhabricatorProjectTriggerViewController' => 'applications/project/controller/trigger/PhabricatorProjectTriggerViewController.php',
     'PhabricatorProjectTypeTransaction' => 'applications/project/xaction/PhabricatorProjectTypeTransaction.php',
     'PhabricatorProjectUIEventListener' => 'applications/project/events/PhabricatorProjectUIEventListener.php',
@@ -10307,6 +10309,7 @@ phutil_register_library_map(array(
       'PhabricatorProjectDAO',
       'PhabricatorApplicationTransactionInterface',
       'PhabricatorPolicyInterface',
+      'PhabricatorIndexableInterface',
       'PhabricatorDestructibleInterface',
     ),
     'PhabricatorProjectTriggerController' => 'PhabricatorProjectController',
@@ -10328,6 +10331,8 @@ phutil_register_library_map(array(
     'PhabricatorProjectTriggerTransactionQuery' => 'PhabricatorApplicationTransactionQuery',
     'PhabricatorProjectTriggerTransactionType' => 'PhabricatorModularTransactionType',
     'PhabricatorProjectTriggerUnknownRule' => 'PhabricatorProjectTriggerRule',
+    'PhabricatorProjectTriggerUsage' => 'PhabricatorProjectDAO',
+    'PhabricatorProjectTriggerUsageIndexEngineExtension' => 'PhabricatorIndexEngineExtension',
     'PhabricatorProjectTriggerViewController' => 'PhabricatorProjectTriggerController',
     'PhabricatorProjectTypeTransaction' => 'PhabricatorProjectTransactionType',
     'PhabricatorProjectUIEventListener' => 'PhabricatorEventListener',

src/applications/project/controller/trigger/PhabricatorProjectTriggerViewController.php

@@ -71,17 +71,23 @@ final class PhabricatorProjectTriggerViewController
     // so we load only the columns they can actually see, but have a list of
     // all the impacted column PHIDs.
 
+    // (We're also exposing the status of columns the user might not be able
+    // to see. This technically violates policy, but the trigger usage table
+    // hints at it anyway and it seems unlikely to ever have any security
+    // impact, but is useful in assessing whether a trigger is really in use
+    // or not.)
+
     $omnipotent_viewer = PhabricatorUser::getOmnipotentUser();
     $all_columns = id(new PhabricatorProjectColumnQuery())
       ->setViewer($omnipotent_viewer)
       ->withTriggerPHIDs(array($trigger->getPHID()))
       ->execute();
-    $column_phids = mpull($all_columns, 'getPHID');
+    $column_map = mpull($all_columns, 'getStatus', 'getPHID');
 
-    if ($column_phids) {
+    if ($column_map) {
       $visible_columns = id(new PhabricatorProjectColumnQuery())
         ->setViewer($viewer)
-        ->withPHIDs($column_phids)
+        ->withPHIDs(array_keys($column_map))
         ->execute();
       $visible_columns = mpull($visible_columns, null, 'getPHID');
     } else {
@@ -89,7 +95,7 @@ final class PhabricatorProjectTriggerViewController
     }
 
     $rows = array();
-    foreach ($column_phids as $column_phid) {
+    foreach ($column_map as $column_phid => $column_status) {
       $column = idx($visible_columns, $column_phid);
 
       if ($column) {
@@ -113,7 +119,18 @@ final class PhabricatorProjectTriggerViewController
         $column_name = phutil_tag('em', array(), pht('Restricted Column'));
       }
 
+      if ($column_status == PhabricatorProjectColumn::STATUS_ACTIVE) {
+        $status_icon = id(new PHUIIconView())
+          ->setIcon('fa-columns', 'blue')
+          ->setTooltip(pht('Active Column'));
+      } else {
+        $status_icon = id(new PHUIIconView())
+          ->setIcon('fa-eye-slash', 'grey')
+          ->setTooltip(pht('Hidden Column'));
+      }
+
       $rows[] = array(
+        $status_icon,
         $project_name,
         $column_name,
       );
@@ -123,11 +140,13 @@ final class PhabricatorProjectTriggerViewController
       ->setNoDataString(pht('This trigger is not used by any columns.'))
       ->setHeaders(
         array(
+          null,
           pht('Project'),
           pht('Column'),
         ))
       ->setColumnClasses(
         array(
+          null,
           null,
           'wide pri',
         ));

src/applications/project/editor/PhabricatorProjectTriggerEditor.php

@@ -27,4 +27,8 @@ final class PhabricatorProjectTriggerEditor
     return $types;
   }
 
+  protected function supportsSearch() {
+    return true;
+  }
+
 }

src/applications/project/engineextension/PhabricatorProjectTriggerUsageIndexEngineExtension.php

@@ -0,0 +1,69 @@
+<?php
+
+final class PhabricatorProjectTriggerUsageIndexEngineExtension
+  extends PhabricatorIndexEngineExtension {
+
+  const EXTENSIONKEY = 'trigger.usage';
+
+  public function getExtensionName() {
+    return pht('Trigger Usage');
+  }
+
+  public function shouldIndexObject($object) {
+    if (!($object instanceof PhabricatorProjectTrigger)) {
+      return false;
+    }
+
+    return true;
+  }
+
+  public function indexObject(
+    PhabricatorIndexEngine $engine,
+    $object) {
+
+    $usage_table = new PhabricatorProjectTriggerUsage();
+    $column_table = new PhabricatorProjectColumn();
+
+    $conn_w = $object->establishConnection('w');
+
+    $active_statuses = array(
+      PhabricatorProjectColumn::STATUS_ACTIVE,
+    );
+
+    // Select summary information to populate the usage index. When picking
+    // an "examplePHID", we try to pick an active column.
+    $row = queryfx_one(
+      $conn_w,
+      'SELECT phid, COUNT(*) N, SUM(IF(status IN (%Ls), 1, 0)) M FROM %R
+        WHERE triggerPHID = %s
+        ORDER BY IF(status IN (%Ls), 1, 0) DESC, id ASC',
+      $active_statuses,
+      $column_table,
+      $object->getPHID(),
+      $active_statuses);
+    if ($row) {
+      $example_phid = $row['phid'];
+      $column_count = $row['N'];
+      $active_count = $row['M'];
+    } else {
+      $example_phid = null;
+      $column_count = 0;
+      $active_count = 0;
+    }
+
+    queryfx(
+      $conn_w,
+      'INSERT INTO %R (triggerPHID, examplePHID, columnCount, activeColumnCount)
+        VALUES (%s, %ns, %d, %d)
+        ON DUPLICATE KEY UPDATE
+          examplePHID = VALUES(examplePHID),
+          columnCount = VALUES(columnCount),
+          activeColumnCount = VALUES(activeColumnCount)',
+      $usage_table,
+      $object->getPHID(),
+      $example_phid,
+      $column_count,
+      $active_count);
+  }
+
+}

src/applications/project/query/PhabricatorProjectTriggerQuery.php

@@ -5,6 +5,10 @@ final class PhabricatorProjectTriggerQuery
 
   private $ids;
   private $phids;
+  private $activeColumnMin;
+  private $activeColumnMax;
+
+  private $needUsage;
 
   public function withIDs(array $ids) {
     $this->ids = $ids;
@@ -16,6 +20,17 @@ final class PhabricatorProjectTriggerQuery
     return $this;
   }
 
+  public function needUsage($need_usage) {
+    $this->needUsage = $need_usage;
+    return $this;
+  }
+
+  public function withActiveColumnCountBetween($min, $max) {
+    $this->activeColumnMin = $min;
+    $this->activeColumnMax = $max;
+    return $this;
+  }
+
   public function newResultObject() {
     return new PhabricatorProjectTrigger();
   }
@@ -30,22 +45,91 @@ final class PhabricatorProjectTriggerQuery
     if ($this->ids !== null) {
       $where[] = qsprintf(
         $conn,
-        'id IN (%Ld)',
+        'trigger.id IN (%Ld)',
         $this->ids);
     }
 
     if ($this->phids !== null) {
       $where[] = qsprintf(
         $conn,
-        'phid IN (%Ls)',
+        'trigger.phid IN (%Ls)',
         $this->phids);
     }
 
+    if ($this->activeColumnMin !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'trigger_usage.activeColumnCount >= %d',
+        $this->activeColumnMin);
+    }
+
+    if ($this->activeColumnMax !== null) {
+      $where[] = qsprintf(
+        $conn,
+        'trigger_usage.activeColumnCount <= %d',
+        $this->activeColumnMax);
+    }
+
     return $where;
   }
 
+  protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
+    $joins = parent::buildJoinClauseParts($conn);
+
+    if ($this->shouldJoinUsageTable()) {
+      $joins[] = qsprintf(
+        $conn,
+        'JOIN %R trigger_usage ON trigger.phid = trigger_usage.triggerPHID',
+        new PhabricatorProjectTriggerUsage());
+    }
+
+    return $joins;
+  }
+
+  private function shouldJoinUsageTable() {
+    if ($this->activeColumnMin !== null) {
+      return true;
+    }
+
+    if ($this->activeColumnMax !== null) {
+      return true;
+    }
+
+    return false;
+  }
+
+  protected function didFilterPage(array $triggers) {
+    if ($this->needUsage) {
+      $usage_map = id(new PhabricatorProjectTriggerUsage())->loadAllWhere(
+        'triggerPHID IN (%Ls)',
+        mpull($triggers, 'getPHID'));
+      $usage_map = mpull($usage_map, null, 'getTriggerPHID');
+
+      foreach ($triggers as $trigger) {
+        $trigger_phid = $trigger->getPHID();
+
+        $usage = idx($usage_map, $trigger_phid);
+        if (!$usage) {
+          $usage = id(new PhabricatorProjectTriggerUsage())
+            ->setTriggerPHID($trigger_phid)
+            ->setExamplePHID(null)
+            ->setColumnCount(0)
+            ->setActiveColumnCount(0);
+        }
+
+        $trigger->attachUsage($usage);
+      }
+    }
+
+    return $triggers;
+  }
+
   public function getQueryApplicationClass() {
     return 'PhabricatorProjectApplication';
   }
 
+  protected function getPrimaryTableAlias() {
+    return 'trigger';
+  }
+
 }

src/applications/project/query/PhabricatorProjectTriggerSearchEngine.php

@@ -12,16 +12,33 @@ final class PhabricatorProjectTriggerSearchEngine
   }
 
   public function newQuery() {
-    return new PhabricatorProjectTriggerQuery();
+    return id(new PhabricatorProjectTriggerQuery())
+      ->needUsage(true);
   }
 
   protected function buildCustomSearchFields() {
-    return array();
+    return array(
+      id(new PhabricatorSearchThreeStateField())
+        ->setLabel(pht('Active'))
+        ->setKey('isActive')
+        ->setOptions(
+          pht('(Show All)'),
+          pht('Show Only Active Triggers'),
+          pht('Show Only Inactive Triggers')),
+    );
   }
 
   protected function buildQueryFromParameters(array $map) {
     $query = $this->newQuery();
 
+    if ($map['isActive'] !== null) {
+      if ($map['isActive']) {
+        $query->withActiveColumnCountBetween(1, null);
+      } else {
+        $query->withActiveColumnCountBetween(null, 0);
+      }
+    }
+
     return $query;
   }
 
@@ -32,7 +49,8 @@ final class PhabricatorProjectTriggerSearchEngine
   protected function getBuiltinQueryNames() {
     $names = array();
 
-    $names['all'] = pht('All');
+    $names['active'] = pht('Active Triggers');
+    $names['all'] = pht('All Triggers');
 
     return $names;
   }
@@ -42,6 +60,8 @@ final class PhabricatorProjectTriggerSearchEngine
     $query->setQueryKey($query_key);
 
     switch ($query_key) {
+      case 'active':
+        return $query->setParameter('isActive', true);
       case 'all':
         return $query;
     }
@@ -56,13 +76,73 @@ final class PhabricatorProjectTriggerSearchEngine
     assert_instances_of($triggers, 'PhabricatorProjectTrigger');
     $viewer = $this->requireViewer();
 
+    $example_phids = array();
+    foreach ($triggers as $trigger) {
+      $example_phid = $trigger->getUsage()->getExamplePHID();
+      if ($example_phid) {
+        $example_phids[] = $example_phid;
+      }
+    }
+
+    $handles = $viewer->loadHandles($example_phids);
+
     $list = id(new PHUIObjectItemListView())
       ->setViewer($viewer);
     foreach ($triggers as $trigger) {
+      $usage = $trigger->getUsage();
+
+      $column_handle = null;
+      $have_column = false;
+      $example_phid = $usage->getExamplePHID();
+      if ($example_phid) {
+        $column_handle = $handles[$example_phid];
+        if ($column_handle->isComplete()) {
+          if (!$column_handle->getPolicyFiltered()) {
+            $have_column = true;
+          }
+        }
+      }
+
+      $column_count = $usage->getColumnCount();
+      $active_count = $usage->getActiveColumnCount();
+
+      if ($have_column) {
+        if ($active_count > 1) {
+          $usage_description = pht(
+            'Used on %s and %s other active column(s).',
+            $column_handle->renderLink(),
+            new PhutilNumber($active_count - 1));
+        } else if ($column_count > 1) {
+          $usage_description = pht(
+            'Used on %s and %s other column(s).',
+            $column_handle->renderLink(),
+            new PhutilNumber($column_count - 1));
+        } else {
+          $usage_description = pht(
+            'Used on %s.',
+            $column_handle->renderLink());
+        }
+      } else {
+        if ($active_count) {
+          $usage_description = pht(
+            'Used on %s active column(s).',
+            new PhutilNumber($active_count));
+        } else if ($column_count) {
+          $usage_description = pht(
+            'Used on %s column(s).',
+            new PhutilNumber($column_count));
+        } else {
+          $usage_description = pht(
+            'Unused trigger.');
+        }
+      }
+
       $item = id(new PHUIObjectItemView())
         ->setObjectName($trigger->getObjectName())
         ->setHeader($trigger->getDisplayName())
-        ->setHref($trigger->getURI());
+        ->setHref($trigger->getURI())
+        ->addAttribute($usage_description)
+        ->setDisabled(!$active_count);
 
       $list->addItem($item);
     }

src/applications/project/storage/PhabricatorProjectTrigger.php

@@ -5,6 +5,7 @@ final class PhabricatorProjectTrigger
   implements
     PhabricatorApplicationTransactionInterface,
     PhabricatorPolicyInterface,
+    PhabricatorIndexableInterface,
     PhabricatorDestructibleInterface {
 
   protected $name;
@@ -12,6 +13,7 @@ final class PhabricatorProjectTrigger
   protected $editPolicy;
 
   private $triggerRules;
+  private $usage = self::ATTACHABLE;
 
   public static function initializeNewTrigger() {
     $default_edit = PhabricatorPolicies::POLICY_USER;
@@ -257,6 +259,15 @@ final class PhabricatorProjectTrigger
     return $sounds;
   }
 
+  public function getUsage() {
+    return $this->assertAttached($this->usage);
+  }
+
+  public function attachUsage(PhabricatorProjectTriggerUsage $usage) {
+    $this->usage = $usage;
+    return $this;
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 
@@ -310,6 +321,13 @@ final class PhabricatorProjectTrigger
         new PhabricatorProjectColumn(),
         $this->getPHID());
 
+      // Remove the usage index row for this trigger, if one exists.
+      queryfx(
+        $conn,
+        'DELETE FROM %R WHERE triggerPHID = %s',
+        new PhabricatorProjectTriggerUsage(),
+        $this->getPHID());
+
       $this->delete();
 
     $this->saveTransaction();

src/applications/project/storage/PhabricatorProjectTriggerUsage.php

@@ -0,0 +1,28 @@
+<?php
+
+final class PhabricatorProjectTriggerUsage
+  extends PhabricatorProjectDAO {
+
+  protected $triggerPHID;
+  protected $examplePHID;
+  protected $columnCount;
+  protected $activeColumnCount;
+
+  protected function getConfiguration() {
+    return array(
+      self::CONFIG_TIMESTAMPS => false,
+      self::CONFIG_COLUMN_SCHEMA => array(
+        'examplePHID' => 'phid?',
+        'columnCount' => 'uint32',
+        'activeColumnCount' => 'uint32',
+      ),
+      self::CONFIG_KEY_SCHEMA => array(
+        'key_trigger' => array(
+          'columns' => array('triggerPHID'),
+          'unique' => true,
+        ),
+      ),
+    ) + parent::getConfiguration();
+  }
+
+}

src/applications/project/xaction/column/PhabricatorProjectColumnStatusTransaction.php

@@ -13,6 +13,15 @@ final class PhabricatorProjectColumnStatusTransaction
     $object->setStatus($value);
   }
 
+  public function applyExternalEffects($object, $value) {
+    // Update the trigger usage index, which cares about whether columns are
+    // active or not.
+    $trigger_phid = $object->getTriggerPHID();
+    if ($trigger_phid) {
+      PhabricatorSearchWorker::queueDocumentForIndexing($trigger_phid);
+    }
+  }
+
   public function getTitle() {
     $new = $this->getNewValue();
 

src/applications/project/xaction/column/PhabricatorProjectColumnTriggerTransaction.php

@@ -13,6 +13,25 @@ final class PhabricatorProjectColumnTriggerTransaction
     $object->setTriggerPHID($value);
   }
 
+  public function applyExternalEffects($object, $value) {
+    // After we change the trigger attached to a column, update the search
+    // indexes for the old and new triggers so we update the usage index.
+    $old = $this->getOldValue();
+    $new = $this->getNewValue();
+
+    $column_phids = array();
+    if ($old) {
+      $column_phids[] = $old;
+    }
+    if ($new) {
+      $column_phids[] = $new;
+    }
+
+    foreach ($column_phids as $phid) {
+      PhabricatorSearchWorker::queueDocumentForIndexing($phid);
+    }
+  }
+
   public function getTitle() {
     $old = $this->getOldValue();
     $new = $this->getNewValue();

src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php

@@ -136,7 +136,13 @@ final class PhabricatorSearchManagementIndexWorkflow
 
         if ($track_skips) {
           $new_versions = $this->loadIndexVersions($phid);
-          if ($old_versions !== $new_versions) {
+
+          if (!$old_versions && !$new_versions) {
+            // If the document doesn't use an index version, both the lists
+            // of versions will be empty. We still rebuild the index in this
+            // case.
+            $count_updated++;
+          } else if ($old_versions !== $new_versions) {
             $count_updated++;
           } else {
             $count_skipped++;