From ee24eb60b765c1cf831a97aabff69afc9ca921be Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 4 Feb 2019 19:33:30 -0800
Subject: [PATCH] In Owners Packages, make the API representation of the
 "Auditing" field more consistent

Summary:
Ref T13244. See PHI1047. A while ago, the "Review" field changed from "yes/no" to 20 flavors of "Non-Owner Blocking Under A Full Moon". The sky didn't fall, so we'll probably do this to "Audit" eventually too.

The "owners.search" API method anticipates this and returns "none" or "audit" to describe package audit statuses, so it can begin returning "audit-non-owner-reviewers" or whatever in the future.

However, the "owners.edit" API method doesn't work the same way, and takes strings, and the strings have to be numbers. This is goofy and confusing and generally bad.

Make "owners.edit" take the same strings that "owners.search" emits. For now, continue accepting the old values of "0" and "1".

Test Plan:
  - Edited audit status of packages via API using "none", "audit", "0", "1" (worked), and invalid values like "quack" (helpful error).
  - Edited audit status of packages via web UI.
  - Used `owners.search` to retrieve package information.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20091
---
 .../PhabricatorOwnersPackageEditEngine.php    |  6 +--
 .../storage/PhabricatorOwnersPackage.php      | 14 ++++-
 ...icatorOwnersPackageAuditingTransaction.php | 53 ++++++++++++++++++-
 3 files changed, 67 insertions(+), 6 deletions(-)

diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
index 044cb8beda..c4ee026374 100644
--- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
+++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php
@@ -140,11 +140,11 @@ EOTEXT
         ->setTransactionType(
           PhabricatorOwnersPackageAuditingTransaction::TRANSACTIONTYPE)
         ->setIsCopyable(true)
-        ->setValue($object->getAuditingEnabled())
+        ->setValue($object->getAuditingState())
         ->setOptions(
           array(
-            '' => pht('Disabled'),
-            '1' => pht('Enabled'),
+            PhabricatorOwnersPackage::AUDITING_NONE => pht('No Auditing'),
+            PhabricatorOwnersPackage::AUDITING_AUDIT => pht('Audit Commits'),
           )),
       id(new PhabricatorRemarkupEditField())
         ->setKey('description')
diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php
index 564fc8a28b..207e0cb809 100644
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@@ -38,6 +38,9 @@ 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';
 
@@ -564,6 +567,14 @@ final class PhabricatorOwnersPackage
     return '/owners/package/'.$this->getID().'/';
   }
 
+  public function getAuditingState() {
+    if ($this->getAuditingEnabled()) {
+      return self::AUDITING_AUDIT;
+    } else {
+      return self::AUDITING_NONE;
+    }
+  }
+
 /* -(  PhabricatorPolicyInterface  )----------------------------------------- */
 
 
@@ -720,11 +731,10 @@ final class PhabricatorOwnersPackage
       'label' => $review_label,
     );
 
+    $audit_value = $this->getAuditingState();
     if ($this->getAuditingEnabled()) {
-      $audit_value = 'audit';
       $audit_label = pht('Auditing Enabled');
     } else {
-      $audit_value = 'none';
       $audit_label = pht('No Auditing');
     }
 
diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php
index df4f0feb01..d7ea7093f9 100644
--- a/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php
+++ b/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php
@@ -10,7 +10,15 @@ final class PhabricatorOwnersPackageAuditingTransaction
   }
 
   public function generateNewValue($object, $value) {
-    return (int)$value;
+    switch ($value) {
+      case PhabricatorOwnersPackage::AUDITING_AUDIT:
+        return 1;
+      case '1':
+        // TODO: Remove, deprecated.
+        return 1;
+      default:
+        return 0;
+    }
   }
 
   public function applyInternalEffects($object, $value) {
@@ -29,4 +37,47 @@ final class PhabricatorOwnersPackageAuditingTransaction
     }
   }
 
+  public function validateTransactions($object, array $xactions) {
+    $errors = array();
+
+    // See PHI1047. This transaction type accepted some weird stuff. Continue
+    // supporting it for now, but move toward sensible consistency.
+
+    $modern_options = array(
+      PhabricatorOwnersPackage::AUDITING_NONE =>
+        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();
+
+      if (isset($modern_options[$new_value])) {
+        continue;
+      }
+
+      if (isset($deprecated_options[$new_value])) {
+        continue;
+      }
+
+      $errors[] = $this->newInvalidError(
+        pht(
+          'Package auditing value "%s" is not supported. Supported options '.
+          'are: %s. Deprecated options are: %s.',
+          $new_value,
+          implode(', ', $modern_options),
+          implode(', ', $deprecated_options)),
+        $xaction);
+    }
+
+    return $errors;
+  }
+
 }