From 8fab8d8a18ef301d780af44dc920cbcfc50ff083 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 7 Feb 2019 08:00:43 -0800 Subject: [PATCH] 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 --- .../20190207.packages.01.state.sql | 2 + .../20190207.packages.02.migrate.sql | 2 + .../autopatches/20190207.packages.03.drop.sql | 2 + .../20190207.packages.04.xactions.php | 41 +++++++ src/__phutil_library_map__.php | 2 + .../controller/DiffusionBrowseController.php | 7 +- .../constants/PhabricatorOwnersAuditRule.php | 101 ++++++++++++++++++ .../PhabricatorOwnersDetailController.php | 8 +- .../PhabricatorOwnersPackageEditEngine.php | 6 +- .../storage/PhabricatorOwnersPackage.php | 28 ++--- ...icatorOwnersPackageAuditingTransaction.php | 48 +++------ ...habricatorRepositoryCommitOwnersWorker.php | 3 +- 12 files changed, 181 insertions(+), 69 deletions(-) create mode 100644 resources/sql/autopatches/20190207.packages.01.state.sql create mode 100644 resources/sql/autopatches/20190207.packages.02.migrate.sql create mode 100644 resources/sql/autopatches/20190207.packages.03.drop.sql create mode 100644 resources/sql/autopatches/20190207.packages.04.xactions.php create mode 100644 src/applications/owners/constants/PhabricatorOwnersAuditRule.php diff --git a/resources/sql/autopatches/20190207.packages.01.state.sql b/resources/sql/autopatches/20190207.packages.01.state.sql new file mode 100644 index 0000000000..0e74f269ba --- /dev/null +++ b/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}; diff --git a/resources/sql/autopatches/20190207.packages.02.migrate.sql b/resources/sql/autopatches/20190207.packages.02.migrate.sql new file mode 100644 index 0000000000..60bf364ac1 --- /dev/null +++ b/resources/sql/autopatches/20190207.packages.02.migrate.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_owners.owners_package + SET auditingState = IF(auditingEnabled = 0, 'none', 'audit'); diff --git a/resources/sql/autopatches/20190207.packages.03.drop.sql b/resources/sql/autopatches/20190207.packages.03.drop.sql new file mode 100644 index 0000000000..24d0ce1a4f --- /dev/null +++ b/resources/sql/autopatches/20190207.packages.03.drop.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_owners.owners_package + DROP auditingEnabled; diff --git a/resources/sql/autopatches/20190207.packages.04.xactions.php b/resources/sql/autopatches/20190207.packages.04.xactions.php new file mode 100644 index 0000000000..5a8609166e --- /dev/null +++ b/resources/sql/autopatches/20190207.packages.04.xactions.php @@ -0,0 +1,41 @@ +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']); +} diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b9d468ea3c..4f9d7123c1 100644 --- a/src/__phutil_library_map__.php +++ b/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', diff --git a/src/applications/diffusion/controller/DiffusionBrowseController.php b/src/applications/diffusion/controller/DiffusionBrowseController.php index 54be7dd7f1..6a863a4a92 100644 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/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()) { - $item->addIcon('fa-check', pht('Auditing Enabled')); - } else { - $item->addIcon('fa-ban', pht('No Auditing')); - } + $rule = $package->newAuditingRule(); + $item->addIcon($rule->getIconIcon(), $rule->getDisplayName()); if ($package->isArchived()) { $item->setDisabled(true); diff --git a/src/applications/owners/constants/PhabricatorOwnersAuditRule.php b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php new file mode 100644 index 0000000000..1f8fd0b1bd --- /dev/null +++ b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php @@ -0,0 +1,101 @@ +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"', + ), + ), + ); + } + + + +} diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php index f71009cf19..e28ae2b3bb 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/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()) { - $auditing = pht('Enabled'); - } else { - $auditing = pht('Disabled'); - } - $view->addProperty(pht('Auditing'), $auditing); + $rule = $package->newAuditingRule(); + $view->addProperty(pht('Auditing'), $rule->getDisplayName()); $ignored = $package->getIgnoredPathAttributes(); $ignored = array_keys($ignored); diff --git a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php index c4ee026374..13f896d3f0 100644 --- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php +++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php @@ -141,11 +141,7 @@ EOTEXT PhabricatorOwnersPackageAuditingTransaction::TRANSACTIONTYPE) ->setIsCopyable(true) ->setValue($object->getAuditingState()) - ->setOptions( - array( - PhabricatorOwnersPackage::AUDITING_NONE => pht('No Auditing'), - PhabricatorOwnersPackage::AUDITING_AUDIT => pht('Audit Commits'), - )), + ->setOptions(PhabricatorOwnersAuditRule::newSelectControlMap()), id(new PhabricatorRemarkupEditField()) ->setKey('description') ->setLabel(pht('Description')) diff --git a/src/applications/owners/storage/PhabricatorOwnersPackage.php b/src/applications/owners/storage/PhabricatorOwnersPackage.php index 207e0cb809..b9e91ef958 100644 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/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() { - if ($this->getAuditingEnabled()) { - return self::AUDITING_AUDIT; - } else { - return self::AUDITING_NONE; - } + public function newAuditingRule() { + return PhabricatorOwnersAuditRule::newFromState($this->getAuditingState()); } /* -( PhabricatorPolicyInterface )----------------------------------------- */ @@ -731,16 +724,11 @@ final class PhabricatorOwnersPackage 'label' => $review_label, ); - $audit_value = $this->getAuditingState(); - if ($this->getAuditingEnabled()) { - $audit_label = pht('Auditing Enabled'); - } else { - $audit_label = pht('No Auditing'); - } + $audit_rule = $this->newAuditingRule(); $audit = array( - 'value' => $audit_value, - 'label' => $audit_label, + 'value' => $audit_rule->getKey(), + 'label' => $audit_rule->getDisplayName(), ); $dominion_value = $this->getDominion(); diff --git a/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php b/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php index d7ea7093f9..7c16c850fd 100644 --- a/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php +++ b/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) { - case PhabricatorOwnersPackage::AUDITING_AUDIT: - return 1; - case '1': - // TODO: Remove, deprecated. - return 1; - default: - return 0; - } + return PhabricatorOwnersAuditRule::getStorageValueFromAPIValue($value); } public function applyInternalEffects($object, $value) { - $object->setAuditingEnabled($value); + $object->setAuditingState($value); } public function getTitle() { - if ($this->getNewValue()) { - return pht( - '%s enabled auditing for this package.', - $this->renderAuthor()); - } else { - return pht( - '%s disabled auditing for this package.', - $this->renderAuthor()); - } + $old_value = $this->getOldValue(); + $new_value = $this->getNewValue(); + + $old_rule = PhabricatorOwnersAuditRule::newFromState($old_value); + $new_rule = PhabricatorOwnersAuditRule::newFromState($new_value); + + return pht( + '%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( - 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)'), - ); + $modern_options = PhabricatorOwnersAuditRule::getModernValueMap(); + $deprecated_options = PhabricatorOwnersAuditRule::getDeprecatedValueMap(); foreach ($xactions as $xaction) { $new_value = $xaction->getNewValue(); diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php index 75ae0c9c14..219314c9d5 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/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; }