diff --git a/resources/sql/autopatches/20190207.packages.01.state.sql b/resources/sql/autopatches/20190207.packages.01.state.sql new file mode 100644 --- /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 --- /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 --- /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 --- /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 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3668,6 +3668,7 @@ '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 @@ '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 --- a/src/applications/diffusion/controller/DiffusionBrowseController.php +++ b/src/applications/diffusion/controller/DiffusionBrowseController.php @@ -566,11 +566,8 @@ $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 --- /dev/null +++ b/src/applications/owners/constants/PhabricatorOwnersAuditRule.php @@ -0,0 +1,92 @@ +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) { + $deprecated_map = self::getDeprecatedValueMap(); + return idx($deprecated_map, $value, $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 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -194,12 +194,8 @@ $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 --- a/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php +++ b/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php @@ -141,11 +141,7 @@ 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 --- a/src/applications/owners/storage/PhabricatorOwnersPackage.php +++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php @@ -13,7 +13,6 @@ PhabricatorNgramsInterface { protected $name; - protected $auditingEnabled; protected $autoReview; protected $description; protected $status; @@ -21,6 +20,7 @@ protected $editPolicy; protected $dominion; protected $properties = array(); + protected $auditingState; private $paths = self::ATTACHABLE; private $owners = self::ATTACHABLE; @@ -38,9 +38,6 @@ 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 @@ 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 @@ self::CONFIG_COLUMN_SCHEMA => array( 'name' => 'sort', 'description' => 'text', - 'auditingEnabled' => 'bool', + 'auditingState' => 'text32', 'status' => 'text32', 'autoReview' => 'text32', 'dominion' => 'text32', @@ -567,12 +564,8 @@ 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 @@ '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 --- a/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php +++ b/src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php @@ -6,35 +6,29 @@ 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 @@ // 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 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -133,7 +133,8 @@ $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; }