Prepare owners package audit rules to become more flexible

Summary:
Ref T13244. See PHI1055. (Earlier, see D20091 and PHI1047.) Previously, we expanded the Owners package autoreview rules from "Yes/No" to several "Review (Blocking) If Non-Owner Author Not Subscribed via Package" kinds of rules. The sky didn't fall and this feature didn't turn into "Herald-in-Owners", so I'm comfortable doing something similar to the "Audit" rules.

PHI1055 is a request for a way to configure slightly different audit behavior, and expanding the options seems like a good approach to satisfy the use case.

Prepare to add more options by moving everything into a class that defines all the behavior of different states, and converting the "0/1" boolean column to a text column.

Test Plan:
  - Created several packages, some with and some without auditing.
  - Inspected database for: package state; and associated transactions.
  - Ran the migrations.
  - Inspected database to confirm that state and transactions migrated correctly.
  - Reviewed transaction logs.
  - Created and edited packages and audit state.
  - Viewed the "Package List" element in Diffusion.
  - Pulled package information with `owners.search`, got sensible results.
  - Edited package audit status with `owners.edit`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13244

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

resources/sql/autopatches/20190207.packages.01.state.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_package
+  ADD auditingState VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT};

resources/sql/autopatches/20190207.packages.02.migrate.sql

@@ -0,0 +1,2 @@
+UPDATE {$NAMESPACE}_owners.owners_package
+  SET auditingState = IF(auditingEnabled = 0, 'none', 'audit');

resources/sql/autopatches/20190207.packages.03.drop.sql

@@ -0,0 +1,2 @@
+ALTER TABLE {$NAMESPACE}_owners.owners_package
+  DROP auditingEnabled;

resources/sql/autopatches/20190207.packages.04.xactions.php

@@ -0,0 +1,41 @@
+<?php
+
+$table = new PhabricatorOwnersPackageTransaction();
+$conn = $table->establishConnection('w');
+$iterator = new LiskRawMigrationIterator($conn, $table->getTableName());
+
+// Migrate "Auditing State" transactions for Owners Packages from old values
+// (which were "0" or "1", as JSON integer literals, without quotes) to new
+// values (which are JSON strings, with quotes).
+
+foreach ($iterator as $row) {
+  if ($row['transactionType'] !== 'owners.auditing') {
+    continue;
+  }
+
+  $old_value = (int)$row['oldValue'];
+  $new_value = (int)$row['newValue'];
+
+  if (!$old_value) {
+    $old_value = 'none';
+  } else {
+    $old_value = 'audit';
+  }
+
+  if (!$new_value) {
+    $new_value = 'none';
+  } else {
+    $new_value = 'audit';
+  }
+
+  $old_value = phutil_json_encode($old_value);
+  $new_value = phutil_json_encode($new_value);
+
+  queryfx(
+    $conn,
+    'UPDATE %R SET oldValue = %s, newValue = %s WHERE id = %d',
+    $table,
+    $old_value,
+    $new_value,
+    $row['id']);
+}

src/__phutil_library_map__.php

@@ -3668,6 +3668,7 @@ phutil_register_library_map(array(
     'PhabricatorOwnerPathQuery' => 'applications/owners/query/PhabricatorOwnerPathQuery.php',
     'PhabricatorOwnersApplication' => 'applications/owners/application/PhabricatorOwnersApplication.php',
     'PhabricatorOwnersArchiveController' => 'applications/owners/controller/PhabricatorOwnersArchiveController.php',
+    'PhabricatorOwnersAuditRule' => 'applications/owners/constants/PhabricatorOwnersAuditRule.php',
     'PhabricatorOwnersConfigOptions' => 'applications/owners/config/PhabricatorOwnersConfigOptions.php',
     'PhabricatorOwnersConfiguredCustomField' => 'applications/owners/customfield/PhabricatorOwnersConfiguredCustomField.php',
     'PhabricatorOwnersController' => 'applications/owners/controller/PhabricatorOwnersController.php',
@@ -9613,6 +9614,7 @@ phutil_register_library_map(array(
     'PhabricatorOwnerPathQuery' => 'Phobject',
     'PhabricatorOwnersApplication' => 'PhabricatorApplication',
     'PhabricatorOwnersArchiveController' => 'PhabricatorOwnersController',
+    'PhabricatorOwnersAuditRule' => 'Phobject',
     'PhabricatorOwnersConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorOwnersConfiguredCustomField' => array(
       'PhabricatorOwnersCustomField',

src/applications/diffusion/controller/DiffusionBrowseController.php

@@ -566,11 +566,8 @@ final class DiffusionBrowseController extends DiffusionController {
         $name = idx($spec, 'name', $auto);
         $item->addIcon('fa-code', $name);
 
-        if ($package->getAuditingEnabled()) {
+        $rule = $package->newAuditingRule();
-          $item->addIcon('fa-check', pht('Auditing Enabled'));
+        $item->addIcon($rule->getIconIcon(), $rule->getDisplayName());
-        } else {
-          $item->addIcon('fa-ban', pht('No Auditing'));
-        }
 
         if ($package->isArchived()) {
           $item->setDisabled(true);

src/applications/owners/constants/PhabricatorOwnersAuditRule.php

@@ -0,0 +1,101 @@
+<?php
+
+final class PhabricatorOwnersAuditRule
+  extends Phobject {
+
+  const AUDITING_NONE = 'none';
+  const AUDITING_AUDIT = 'audit';
+
+  private $key;
+  private $spec;
+
+  public static function newFromState($key) {
+    $specs = self::newSpecifications();
+    $spec = idx($specs, $key, array());
+
+    $rule = new self();
+    $rule->key = $key;
+    $rule->spec = $spec;
+
+    return $rule;
+  }
+
+  public function getKey() {
+    return $this->key;
+  }
+
+  public function getDisplayName() {
+    return idx($this->spec, 'name', $this->key);
+  }
+
+  public function getIconIcon() {
+    return idx($this->spec, 'icon.icon');
+  }
+
+  public static function newSelectControlMap() {
+    $specs = self::newSpecifications();
+    return ipull($specs, 'name');
+  }
+
+  public static function getStorageValueFromAPIValue($value) {
+    $specs = self::newSpecifications();
+
+    $map = array();
+    foreach ($specs as $key => $spec) {
+      $deprecated = idx($spec, 'deprecated', array());
+      if (isset($deprecated[$value])) {
+        return $key;
+      }
+    }
+
+    return $value;
+  }
+
+  public static function getModernValueMap() {
+    $specs = self::newSpecifications();
+
+    $map = array();
+    foreach ($specs as $key => $spec) {
+      $map[$key] = pht('"%s"', $key);
+    }
+
+    return $map;
+  }
+
+  public static function getDeprecatedValueMap() {
+    $specs = self::newSpecifications();
+
+    $map = array();
+    foreach ($specs as $key => $spec) {
+      $deprecated_map = idx($spec, 'deprecated', array());
+      foreach ($deprecated_map as $deprecated_key => $label) {
+        $map[$deprecated_key] = $label;
+      }
+    }
+
+    return $map;
+  }
+
+  private static function newSpecifications() {
+    return array(
+      self::AUDITING_NONE => array(
+        'name' => pht('No Auditing'),
+        'icon.icon' => 'fa-ban',
+        'deprecated' => array(
+          '' => pht('"" (empty string)'),
+          '0' => '"0"',
+        ),
+      ),
+      self::AUDITING_AUDIT => array(
+        'name' => pht('Audit Commits'),
+        'icon.icon' => 'fa-check',
+        'deprecated' => array(
+          '1' => '"1"',
+        ),
+      ),
+    );
+  }
+
+
+
+}

src/applications/owners/controller/PhabricatorOwnersDetailController.php

@@ -194,12 +194,8 @@ final class PhabricatorOwnersDetailController
     $name = idx($spec, 'name', $auto);
     $view->addProperty(pht('Auto Review'), $name);
 
-    if ($package->getAuditingEnabled()) {
+    $rule = $package->newAuditingRule();
-      $auditing = pht('Enabled');
+    $view->addProperty(pht('Auditing'), $rule->getDisplayName());
-    } else {
-      $auditing = pht('Disabled');
-    }
-    $view->addProperty(pht('Auditing'), $auditing);
 
     $ignored = $package->getIgnoredPathAttributes();
     $ignored = array_keys($ignored);

src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php

@@ -141,11 +141,7 @@ EOTEXT
           PhabricatorOwnersPackageAuditingTransaction::TRANSACTIONTYPE)
         ->setIsCopyable(true)
         ->setValue($object->getAuditingState())
-        ->setOptions(
+        ->setOptions(PhabricatorOwnersAuditRule::newSelectControlMap()),
-          array(
-            PhabricatorOwnersPackage::AUDITING_NONE => pht('No Auditing'),
-            PhabricatorOwnersPackage::AUDITING_AUDIT => pht('Audit Commits'),
-          )),
       id(new PhabricatorRemarkupEditField())
         ->setKey('description')
         ->setLabel(pht('Description'))

src/applications/owners/storage/PhabricatorOwnersPackage.php

@@ -13,7 +13,6 @@ final class PhabricatorOwnersPackage
     PhabricatorNgramsInterface {
 
   protected $name;
-  protected $auditingEnabled;
   protected $autoReview;
   protected $description;
   protected $status;
@@ -21,6 +20,7 @@ final class PhabricatorOwnersPackage
   protected $editPolicy;
   protected $dominion;
   protected $properties = array();
+  protected $auditingState;
 
   private $paths = self::ATTACHABLE;
   private $owners = self::ATTACHABLE;
@@ -38,9 +38,6 @@ final class PhabricatorOwnersPackage
   const AUTOREVIEW_BLOCK = 'block';
   const AUTOREVIEW_BLOCK_ALWAYS = 'block-always';
 
-  const AUDITING_NONE = 'none';
-  const AUDITING_AUDIT = 'audit';
-
   const DOMINION_STRONG = 'strong';
   const DOMINION_WEAK = 'weak';
 
@@ -58,7 +55,7 @@ final class PhabricatorOwnersPackage
       PhabricatorOwnersDefaultEditCapability::CAPABILITY);
 
     return id(new PhabricatorOwnersPackage())
-      ->setAuditingEnabled(0)
+      ->setAuditingState(PhabricatorOwnersAuditRule::AUDITING_NONE)
       ->setAutoReview(self::AUTOREVIEW_NONE)
       ->setDominion(self::DOMINION_STRONG)
       ->setViewPolicy($view_policy)
@@ -129,7 +126,7 @@ final class PhabricatorOwnersPackage
       self::CONFIG_COLUMN_SCHEMA => array(
         'name' => 'sort',
         'description' => 'text',
-        'auditingEnabled' => 'bool',
+        'auditingState' => 'text32',
         'status' => 'text32',
         'autoReview' => 'text32',
         'dominion' => 'text32',
@@ -567,12 +564,8 @@ final class PhabricatorOwnersPackage
     return '/owners/package/'.$this->getID().'/';
   }
 
-  public function getAuditingState() {
+  public function newAuditingRule() {
-    if ($this->getAuditingEnabled()) {
+    return PhabricatorOwnersAuditRule::newFromState($this->getAuditingState());
-      return self::AUDITING_AUDIT;
-    } else {
-      return self::AUDITING_NONE;
-    }
   }
 
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
@@ -731,16 +724,11 @@ final class PhabricatorOwnersPackage
       'label' => $review_label,
     );
 
-    $audit_value = $this->getAuditingState();
+    $audit_rule = $this->newAuditingRule();
-    if ($this->getAuditingEnabled()) {
-      $audit_label = pht('Auditing Enabled');
-    } else {
-      $audit_label = pht('No Auditing');
-    }
 
     $audit = array(
-      'value' => $audit_value,
+      'value' => $audit_rule->getKey(),
-      'label' => $audit_label,
+      'label' => $audit_rule->getDisplayName(),
     );
 
     $dominion_value = $this->getDominion();

src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php

@@ -6,35 +6,29 @@ final class PhabricatorOwnersPackageAuditingTransaction
   const TRANSACTIONTYPE = 'owners.auditing';
 
   public function generateOldValue($object) {
-    return (int)$object->getAuditingEnabled();
+    return $object->getAuditingState();
   }
 
   public function generateNewValue($object, $value) {
-    switch ($value) {
+    return PhabricatorOwnersAuditRule::getStorageValueFromAPIValue($value);
-      case PhabricatorOwnersPackage::AUDITING_AUDIT:
-        return 1;
-      case '1':
-        // TODO: Remove, deprecated.
-        return 1;
-      default:
-        return 0;
-    }
   }
 
   public function applyInternalEffects($object, $value) {
-    $object->setAuditingEnabled($value);
+    $object->setAuditingState($value);
   }
 
   public function getTitle() {
-    if ($this->getNewValue()) {
+    $old_value = $this->getOldValue();
-      return pht(
+    $new_value = $this->getNewValue();
-        '%s enabled auditing for this package.',
+
-        $this->renderAuthor());
+    $old_rule = PhabricatorOwnersAuditRule::newFromState($old_value);
-    } else {
+    $new_rule = PhabricatorOwnersAuditRule::newFromState($new_value);
-      return pht(
+
-        '%s disabled auditing for this package.',
+    return pht(
-        $this->renderAuthor());
+      '%s changed the audit rule for this package from %s to %s.',
-    }
+      $this->renderAuthor(),
+      $this->renderValue($old_rule->getDisplayName()),
+      $this->renderValue($new_rule->getDisplayName()));
   }
 
   public function validateTransactions($object, array $xactions) {
@@ -43,18 +37,8 @@ final class PhabricatorOwnersPackageAuditingTransaction
     // See PHI1047. This transaction type accepted some weird stuff. Continue
     // supporting it for now, but move toward sensible consistency.
 
-    $modern_options = array(
+    $modern_options = PhabricatorOwnersAuditRule::getModernValueMap();
-      PhabricatorOwnersPackage::AUDITING_NONE =>
+    $deprecated_options = PhabricatorOwnersAuditRule::getDeprecatedValueMap();
-        sprintf('"%s"', PhabricatorOwnersPackage::AUDITING_NONE),
-      PhabricatorOwnersPackage::AUDITING_AUDIT =>
-        sprintf('"%s"', PhabricatorOwnersPackage::AUDITING_AUDIT),
-    );
-
-    $deprecated_options = array(
-      '0' => '"0"',
-      '1' => '"1"',
-      '' => pht('"" (empty string)'),
-    );
 
     foreach ($xactions as $xaction) {
       $new_value = $xaction->getNewValue();

src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php

@@ -133,7 +133,8 @@ final class PhabricatorRepositoryCommitOwnersWorker
     $revision) {
 
     // Don't trigger an audit if auditing isn't enabled for the package.
-    if (!$package->getAuditingEnabled()) {
+    $rule = $package->newAuditingRule();
+    if ($rule->getKey() === PhabricatorOwnersAuditRule::AUDITING_NONE) {
       return false;
     }