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

epriestley 7 years ago ee24eb60 46753066

+67 -6

3 changed files

expand all

src

applications

owners

editor

PhabricatorOwnersPackageEditEngine.php

storage

PhabricatorOwnersPackage.php

xaction

PhabricatorOwnersPackageAuditingTransaction.php

+3 -3

src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php

··· 140 140 ->setTransactionType( 141 141 PhabricatorOwnersPackageAuditingTransaction::TRANSACTIONTYPE) 142 142 ->setIsCopyable(true) 143 - ->setValue($object->getAuditingEnabled()) 143 + ->setValue($object->getAuditingState()) 144 144 ->setOptions( 145 145 array( 146 - '' => pht('Disabled'), 147 - '1' => pht('Enabled'), 146 + PhabricatorOwnersPackage::AUDITING_NONE => pht('No Auditing'), 147 + PhabricatorOwnersPackage::AUDITING_AUDIT => pht('Audit Commits'), 148 148 )), 149 149 id(new PhabricatorRemarkupEditField()) 150 150 ->setKey('description')

+12 -2

src/applications/owners/storage/PhabricatorOwnersPackage.php

··· 38 38 const AUTOREVIEW_BLOCK = 'block'; 39 39 const AUTOREVIEW_BLOCK_ALWAYS = 'block-always'; 40 40 41 + const AUDITING_NONE = 'none'; 42 + const AUDITING_AUDIT = 'audit'; 43 + 41 44 const DOMINION_STRONG = 'strong'; 42 45 const DOMINION_WEAK = 'weak'; 43 46 ··· 564 567 return '/owners/package/'.$this->getID().'/'; 565 568 } 566 569 570 + public function getAuditingState() { 571 + if ($this->getAuditingEnabled()) { 572 + return self::AUDITING_AUDIT; 573 + } else { 574 + return self::AUDITING_NONE; 575 + } 576 + } 577 + 567 578 /* -( PhabricatorPolicyInterface )----------------------------------------- */ 568 579 569 580 ··· 720 731 'label' => $review_label, 721 732 ); 722 733 734 + $audit_value = $this->getAuditingState(); 723 735 if ($this->getAuditingEnabled()) { 724 - $audit_value = 'audit'; 725 736 $audit_label = pht('Auditing Enabled'); 726 737 } else { 727 - $audit_value = 'none'; 728 738 $audit_label = pht('No Auditing'); 729 739 } 730 740

+52 -1

src/applications/owners/xaction/PhabricatorOwnersPackageAuditingTransaction.php

··· 10 10 } 11 11 12 12 public function generateNewValue($object, $value) { 13 - return (int)$value; 13 + switch ($value) { 14 + case PhabricatorOwnersPackage::AUDITING_AUDIT: 15 + return 1; 16 + case '1': 17 + // TODO: Remove, deprecated. 18 + return 1; 19 + default: 20 + return 0; 21 + } 14 22 } 15 23 16 24 public function applyInternalEffects($object, $value) { ··· 27 35 '%s disabled auditing for this package.', 28 36 $this->renderAuthor()); 29 37 } 38 + } 39 + 40 + public function validateTransactions($object, array $xactions) { 41 + $errors = array(); 42 + 43 + // See PHI1047. This transaction type accepted some weird stuff. Continue 44 + // supporting it for now, but move toward sensible consistency. 45 + 46 + $modern_options = array( 47 + PhabricatorOwnersPackage::AUDITING_NONE => 48 + sprintf('"%s"', PhabricatorOwnersPackage::AUDITING_NONE), 49 + PhabricatorOwnersPackage::AUDITING_AUDIT => 50 + sprintf('"%s"', PhabricatorOwnersPackage::AUDITING_AUDIT), 51 + ); 52 + 53 + $deprecated_options = array( 54 + '0' => '"0"', 55 + '1' => '"1"', 56 + '' => pht('"" (empty string)'), 57 + ); 58 + 59 + foreach ($xactions as $xaction) { 60 + $new_value = $xaction->getNewValue(); 61 + 62 + if (isset($modern_options[$new_value])) { 63 + continue; 64 + } 65 + 66 + if (isset($deprecated_options[$new_value])) { 67 + continue; 68 + } 69 + 70 + $errors[] = $this->newInvalidError( 71 + pht( 72 + 'Package auditing value "%s" is not supported. Supported options '. 73 + 'are: %s. Deprecated options are: %s.', 74 + $new_value, 75 + implode(', ', $modern_options), 76 + implode(', ', $deprecated_options)), 77 + $xaction); 78 + } 79 + 80 + return $errors; 30 81 } 31 82 32 83 }

Configure Feed

Configure Feed