diff --git a/resources/sql/autopatches/20190307.herald.01.comments.sql b/resources/sql/autopatches/20190307.herald.01.comments.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20190307.herald.01.comments.sql @@ -0,0 +1 @@ +DROP TABLE {$NAMESPACE}_herald.herald_ruletransaction_comment; 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 @@ -1535,10 +1535,13 @@ 'HeraldRuleAdapterField' => 'applications/herald/field/rule/HeraldRuleAdapterField.php', 'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php', 'HeraldRuleDatasource' => 'applications/herald/typeahead/HeraldRuleDatasource.php', + 'HeraldRuleDisableTransaction' => 'applications/herald/xaction/HeraldRuleDisableTransaction.php', + 'HeraldRuleEditTransaction' => 'applications/herald/xaction/HeraldRuleEditTransaction.php', 'HeraldRuleEditor' => 'applications/herald/editor/HeraldRuleEditor.php', 'HeraldRuleField' => 'applications/herald/field/rule/HeraldRuleField.php', 'HeraldRuleFieldGroup' => 'applications/herald/field/rule/HeraldRuleFieldGroup.php', 'HeraldRuleListController' => 'applications/herald/controller/HeraldRuleListController.php', + 'HeraldRuleNameTransaction' => 'applications/herald/xaction/HeraldRuleNameTransaction.php', 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', 'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php', 'HeraldRuleReplyHandler' => 'applications/herald/mail/HeraldRuleReplyHandler.php', @@ -1546,7 +1549,7 @@ 'HeraldRuleSerializer' => 'applications/herald/editor/HeraldRuleSerializer.php', 'HeraldRuleTestCase' => 'applications/herald/storage/__tests__/HeraldRuleTestCase.php', 'HeraldRuleTransaction' => 'applications/herald/storage/HeraldRuleTransaction.php', - 'HeraldRuleTransactionComment' => 'applications/herald/storage/HeraldRuleTransactionComment.php', + 'HeraldRuleTransactionType' => 'applications/herald/xaction/HeraldRuleTransactionType.php', 'HeraldRuleTranscript' => 'applications/herald/storage/transcript/HeraldRuleTranscript.php', 'HeraldRuleTypeConfig' => 'applications/herald/config/HeraldRuleTypeConfig.php', 'HeraldRuleTypeDatasource' => 'applications/herald/typeahead/HeraldRuleTypeDatasource.php', @@ -7188,18 +7191,21 @@ 'HeraldRuleAdapterField' => 'HeraldRuleField', 'HeraldRuleController' => 'HeraldController', 'HeraldRuleDatasource' => 'PhabricatorTypeaheadDatasource', + 'HeraldRuleDisableTransaction' => 'HeraldRuleTransactionType', + 'HeraldRuleEditTransaction' => 'HeraldRuleTransactionType', 'HeraldRuleEditor' => 'PhabricatorApplicationTransactionEditor', 'HeraldRuleField' => 'HeraldField', 'HeraldRuleFieldGroup' => 'HeraldFieldGroup', 'HeraldRuleListController' => 'HeraldController', + 'HeraldRuleNameTransaction' => 'HeraldRuleTransactionType', 'HeraldRulePHIDType' => 'PhabricatorPHIDType', 'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HeraldRuleReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler', 'HeraldRuleSearchEngine' => 'PhabricatorApplicationSearchEngine', 'HeraldRuleSerializer' => 'Phobject', 'HeraldRuleTestCase' => 'PhabricatorTestCase', - 'HeraldRuleTransaction' => 'PhabricatorApplicationTransaction', - 'HeraldRuleTransactionComment' => 'PhabricatorApplicationTransactionComment', + 'HeraldRuleTransaction' => 'PhabricatorModularTransaction', + 'HeraldRuleTransactionType' => 'PhabricatorModularTransactionType', 'HeraldRuleTranscript' => 'Phobject', 'HeraldRuleTypeConfig' => 'Phobject', 'HeraldRuleTypeDatasource' => 'PhabricatorTypeaheadDatasource', diff --git a/src/applications/herald/controller/HeraldDisableController.php b/src/applications/herald/controller/HeraldDisableController.php --- a/src/applications/herald/controller/HeraldDisableController.php +++ b/src/applications/herald/controller/HeraldDisableController.php @@ -31,7 +31,7 @@ if ($request->isFormPost()) { $xaction = id(new HeraldRuleTransaction()) - ->setTransactionType(HeraldRuleTransaction::TYPE_DISABLE) + ->setTransactionType(HeraldRuleDisableTransaction::TRANSACTIONTYPE) ->setNewValue($is_disable); id(new HeraldRuleEditor()) diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -359,11 +359,21 @@ $repetition_policy); $xactions = array(); + + // Until this moves to EditEngine, manually add a "CREATE" transaction + // if we're creating a new rule. This improves rendering of the initial + // group of transactions. + $is_new = (bool)(!$rule->getID()); + if ($is_new) { + $xactions[] = id(new HeraldRuleTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_CREATE); + } + $xactions[] = id(new HeraldRuleTransaction()) - ->setTransactionType(HeraldRuleTransaction::TYPE_EDIT) + ->setTransactionType(HeraldRuleEditTransaction::TRANSACTIONTYPE) ->setNewValue($new_state); $xactions[] = id(new HeraldRuleTransaction()) - ->setTransactionType(HeraldRuleTransaction::TYPE_NAME) + ->setTransactionType(HeraldRuleNameTransaction::TRANSACTIONTYPE) ->setNewValue($new_name); try { diff --git a/src/applications/herald/editor/HeraldRuleEditor.php b/src/applications/herald/editor/HeraldRuleEditor.php --- a/src/applications/herald/editor/HeraldRuleEditor.php +++ b/src/applications/herald/editor/HeraldRuleEditor.php @@ -11,82 +11,6 @@ return pht('Herald Rules'); } - public function getTransactionTypes() { - $types = parent::getTransactionTypes(); - - $types[] = PhabricatorTransactions::TYPE_COMMENT; - $types[] = HeraldRuleTransaction::TYPE_EDIT; - $types[] = HeraldRuleTransaction::TYPE_NAME; - $types[] = HeraldRuleTransaction::TYPE_DISABLE; - - return $types; - } - - protected function getCustomTransactionOldValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case HeraldRuleTransaction::TYPE_DISABLE: - return (int)$object->getIsDisabled(); - case HeraldRuleTransaction::TYPE_EDIT: - return id(new HeraldRuleSerializer()) - ->serializeRule($object); - case HeraldRuleTransaction::TYPE_NAME: - return $object->getName(); - } - - } - - protected function getCustomTransactionNewValue( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case HeraldRuleTransaction::TYPE_DISABLE: - return (int)$xaction->getNewValue(); - case HeraldRuleTransaction::TYPE_EDIT: - case HeraldRuleTransaction::TYPE_NAME: - return $xaction->getNewValue(); - } - } - - protected function applyCustomInternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - - switch ($xaction->getTransactionType()) { - case HeraldRuleTransaction::TYPE_DISABLE: - return $object->setIsDisabled($xaction->getNewValue()); - case HeraldRuleTransaction::TYPE_NAME: - return $object->setName($xaction->getNewValue()); - case HeraldRuleTransaction::TYPE_EDIT: - $new_state = id(new HeraldRuleSerializer()) - ->deserializeRuleComponents($xaction->getNewValue()); - $object->setMustMatchAll((int)$new_state['match_all']); - $object->attachConditions($new_state['conditions']); - $object->attachActions($new_state['actions']); - - $new_repetition = $new_state['repetition_policy']; - $object->setRepetitionPolicyStringConstant($new_repetition); - - return $object; - } - - } - - protected function applyCustomExternalTransaction( - PhabricatorLiskDAO $object, - PhabricatorApplicationTransaction $xaction) { - switch ($xaction->getTransactionType()) { - case HeraldRuleTransaction::TYPE_EDIT: - $object->saveConditions($object->getConditions()); - $object->saveActions($object->getActions()); - break; - } - return; - } - protected function shouldApplyHeraldRules( PhabricatorLiskDAO $object, array $xactions) { @@ -137,7 +61,6 @@ return pht('[Herald]'); } - protected function buildMailBody( PhabricatorLiskDAO $object, array $xactions) { diff --git a/src/applications/herald/storage/HeraldRuleTransaction.php b/src/applications/herald/storage/HeraldRuleTransaction.php --- a/src/applications/herald/storage/HeraldRuleTransaction.php +++ b/src/applications/herald/storage/HeraldRuleTransaction.php @@ -1,11 +1,9 @@ getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_DISABLE: - if ($new) { - return 'red'; - } else { - return 'green'; - } - } - - return parent::getColor(); - } - - public function getActionName() { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_DISABLE: - if ($new) { - return pht('Disabled'); - } else { - return pht('Enabled'); - } - case self::TYPE_NAME: - return pht('Renamed'); - } - - return parent::getActionName(); - } - - public function getIcon() { - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_DISABLE: - if ($new) { - return 'fa-ban'; - } else { - return 'fa-check'; - } - } - - return parent::getIcon(); - } - - - public function getTitle() { - $author_phid = $this->getAuthorPHID(); - - $old = $this->getOldValue(); - $new = $this->getNewValue(); - - switch ($this->getTransactionType()) { - case self::TYPE_DISABLE: - if ($new) { - return pht( - '%s disabled this rule.', - $this->renderHandleLink($author_phid)); - } else { - return pht( - '%s enabled this rule.', - $this->renderHandleLink($author_phid)); - } - case self::TYPE_NAME: - if ($old == null) { - return pht( - '%s created this rule.', - $this->renderHandleLink($author_phid)); - } else { - return pht( - '%s renamed this rule from "%s" to "%s".', - $this->renderHandleLink($author_phid), - $old, - $new); - } - case self::TYPE_EDIT: - return pht( - '%s edited this rule.', - $this->renderHandleLink($author_phid)); - } - - return parent::getTitle(); - } - - public function hasChangeDetails() { - switch ($this->getTransactionType()) { - case self::TYPE_EDIT: - return true; - } - return parent::hasChangeDetails(); - } - - public function renderChangeDetails(PhabricatorUser $viewer) { - $json = new PhutilJSON(); - switch ($this->getTransactionType()) { - case self::TYPE_EDIT: - return $this->renderTextCorpusChangeDetails( - $viewer, - $json->encodeFormatted($this->getOldValue()), - $json->encodeFormatted($this->getNewValue())); - } - - return $this->renderTextCorpusChangeDetails( - $viewer, - $this->getOldValue(), - $this->getNewValue()); + public function getBaseTransactionClass() { + return 'HeraldRuleTransactionType'; } } diff --git a/src/applications/herald/storage/HeraldRuleTransactionComment.php b/src/applications/herald/storage/HeraldRuleTransactionComment.php deleted file mode 100644 --- a/src/applications/herald/storage/HeraldRuleTransactionComment.php +++ /dev/null @@ -1,10 +0,0 @@ -getIsDisabled(); + } + + public function generateNewValue($object, $value) { + return (bool)$value; + } + + public function applyInternalEffects($object, $value) { + $object->setIsDisabled((int)$value); + } + + public function getTitle() { + if ($this->getNewValue()) { + return pht( + '%s disabled this rule.', + $this->renderAuthor()); + } else { + return pht( + '%s enabled this rule.', + $this->renderAuthor()); + } + } + +} diff --git a/src/applications/herald/xaction/HeraldRuleEditTransaction.php b/src/applications/herald/xaction/HeraldRuleEditTransaction.php new file mode 100644 --- /dev/null +++ b/src/applications/herald/xaction/HeraldRuleEditTransaction.php @@ -0,0 +1,56 @@ +serializeRule($object); + } + + public function applyInternalEffects($object, $value) { + $new_state = id(new HeraldRuleSerializer()) + ->deserializeRuleComponents($value); + + $object->setMustMatchAll((int)$new_state['match_all']); + $object->attachConditions($new_state['conditions']); + $object->attachActions($new_state['actions']); + + $new_repetition = $new_state['repetition_policy']; + $object->setRepetitionPolicyStringConstant($new_repetition); + } + + public function applyExternalEffects($object, $value) { + $object->saveConditions($object->getConditions()); + $object->saveActions($object->getActions()); + } + + public function getTitle() { + return pht( + '%s edited this rule.', + $this->renderAuthor()); + } + + public function hasChangeDetailView() { + return true; + } + + public function newChangeDetailView() { + $viewer = $this->getViewer(); + + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + $json = new PhutilJSON(); + $old_json = $json->encodeFormatted($old); + $new_json = $json->encodeFormatted($new); + + return id(new PhabricatorApplicationTransactionTextDiffDetailView()) + ->setViewer($viewer) + ->setOldText($old_json) + ->setNewText($new_json); + } + +} diff --git a/src/applications/herald/xaction/HeraldRuleNameTransaction.php b/src/applications/herald/xaction/HeraldRuleNameTransaction.php new file mode 100644 --- /dev/null +++ b/src/applications/herald/xaction/HeraldRuleNameTransaction.php @@ -0,0 +1,48 @@ +getName(); + } + + public function applyInternalEffects($object, $value) { + $object->setName($value); + } + + public function getTitle() { + return pht( + '%s renamed this rule from %s to %s.', + $this->renderAuthor(), + $this->renderOldValue(), + $this->renderNewValue()); + } + + public function validateTransactions($object, array $xactions) { + $errors = array(); + + if ($this->isEmptyTextTransaction($object->getName(), $xactions)) { + $errors[] = $this->newRequiredError( + pht('Rules must have a name.')); + } + + $max_length = $object->getColumnMaximumByteLength('name'); + foreach ($xactions as $xaction) { + $new_value = $xaction->getNewValue(); + + $new_length = strlen($new_value); + if ($new_length > $max_length) { + $errors[] = $this->newInvalidError( + pht( + 'Rule names can be no longer than %s characters.', + new PhutilNumber($max_length))); + } + } + + return $errors; + } + +} diff --git a/src/applications/herald/xaction/HeraldRuleTransactionType.php b/src/applications/herald/xaction/HeraldRuleTransactionType.php new file mode 100644 --- /dev/null +++ b/src/applications/herald/xaction/HeraldRuleTransactionType.php @@ -0,0 +1,4 @@ +