From 6dc30ecc8ef1006b4b2614ae7e0506414c49722b Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Mon, 28 Mar 2016 23:02:41 +0000 Subject: [PATCH] Drive Herald edits via transactions Summary: This is kinda bad in terms of UI (It just makes a json of the thing and diffs that), but it's a start. Test Plan: edit rule, create rule, add/remove/edit conditions, actions Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D15542 --- src/__phutil_library_map__.php | 2 + .../controller/HeraldRuleController.php | 49 +++++++++---- .../herald/editor/HeraldRuleEditor.php | 28 ++++++- .../herald/editor/HeraldRuleSerializer.php | 73 +++++++++++++++++++ .../herald/storage/HeraldRuleTransaction.php | 43 +++++++++++ 5 files changed, 179 insertions(+), 16 deletions(-) create mode 100644 src/applications/herald/editor/HeraldRuleSerializer.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index df9a2dd3d6..c21a044fbc 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1230,6 +1230,7 @@ phutil_register_library_map(array( 'HeraldRulePHIDType' => 'applications/herald/phid/HeraldRulePHIDType.php', 'HeraldRuleQuery' => 'applications/herald/query/HeraldRuleQuery.php', 'HeraldRuleSearchEngine' => 'applications/herald/query/HeraldRuleSearchEngine.php', + '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', @@ -5497,6 +5498,7 @@ phutil_register_library_map(array( 'HeraldRulePHIDType' => 'PhabricatorPHIDType', 'HeraldRuleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'HeraldRuleSearchEngine' => 'PhabricatorApplicationSearchEngine', + 'HeraldRuleSerializer' => 'Phobject', 'HeraldRuleTestCase' => 'PhabricatorTestCase', 'HeraldRuleTransaction' => 'PhabricatorApplicationTransaction', 'HeraldRuleTransactionComment' => 'PhabricatorApplicationTransactionComment', diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index d7963a5193..0289dce2e6 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -259,18 +259,15 @@ final class HeraldRuleController extends HeraldController { } private function saveRule(HeraldAdapter $adapter, $rule, $request) { - $rule->setName($request->getStr('name')); + $new_name = $request->getStr('name'); $match_all = ($request->getStr('must_match') == 'all'); - $rule->setMustMatchAll((int)$match_all); $repetition_policy_param = $request->getStr('repetition_policy'); - $rule->setRepetitionPolicy( - HeraldRepetitionPolicyConfig::toInt($repetition_policy_param)); $e_name = true; $errors = array(); - if (!strlen($rule->getName())) { + if (!strlen($new_name)) { $e_name = pht('Required'); $errors[] = pht('Rule must have a name.'); } @@ -343,19 +340,41 @@ final class HeraldRuleController extends HeraldController { $actions[] = $obj; } + if (!$errors) { + $new_state = id(new HeraldRuleSerializer())->serializeRuleComponents( + $match_all, + $conditions, + $actions, + $repetition_policy_param); + + $xactions = array(); + $xactions[] = id(new HeraldRuleTransaction()) + ->setTransactionType(HeraldRuleTransaction::TYPE_EDIT) + ->setNewValue($new_state); + $xactions[] = id(new HeraldRuleTransaction()) + ->setTransactionType(HeraldRuleTransaction::TYPE_NAME) + ->setNewValue($new_name); + + try { + id(new HeraldRuleEditor()) + ->setActor($this->getViewer()) + ->setContinueOnNoEffect(true) + ->setContentSourceFromRequest($request) + ->applyTransactions($rule, $xactions); + return array(null, null); + } catch (Exception $ex) { + $errors[] = $ex->getMessage(); + } + } + + // mutate current rule, so it would be sent to the client in the right state + $rule->setMustMatchAll((int)$match_all); + $rule->setName($new_name); + $rule->setRepetitionPolicy( + HeraldRepetitionPolicyConfig::toInt($repetition_policy_param)); $rule->attachConditions($conditions); $rule->attachActions($actions); - if (!$errors) { - $edit_action = $rule->getID() ? 'edit' : 'create'; - - $rule->openTransaction(); - $rule->save(); - $rule->saveConditions($conditions); - $rule->saveActions($actions); - $rule->saveTransaction(); - } - return array($e_name, $errors); } diff --git a/src/applications/herald/editor/HeraldRuleEditor.php b/src/applications/herald/editor/HeraldRuleEditor.php index 635c24654c..de9bb01ef2 100644 --- a/src/applications/herald/editor/HeraldRuleEditor.php +++ b/src/applications/herald/editor/HeraldRuleEditor.php @@ -15,6 +15,8 @@ final class HeraldRuleEditor $types = parent::getTransactionTypes(); $types[] = PhabricatorTransactions::TYPE_COMMENT; + $types[] = HeraldRuleTransaction::TYPE_EDIT; + $types[] = HeraldRuleTransaction::TYPE_NAME; $types[] = HeraldRuleTransaction::TYPE_DISABLE; return $types; @@ -27,6 +29,11 @@ final class HeraldRuleEditor 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(); } } @@ -38,8 +45,10 @@ final class HeraldRuleEditor 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( @@ -49,6 +58,17 @@ final class HeraldRuleEditor 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']); + $object->setRepetitionPolicy( + HeraldRepetitionPolicyConfig::toInt($new_state['repetition_policy'])); + return $object; } } @@ -56,6 +76,12 @@ final class HeraldRuleEditor protected function applyCustomExternalTransaction( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { + switch ($xaction->getTransactionType()) { + case HeraldRuleTransaction::TYPE_EDIT: + $object->saveConditions($object->getConditions()); + $object->saveActions($object->getActions()); + break; + } return; } diff --git a/src/applications/herald/editor/HeraldRuleSerializer.php b/src/applications/herald/editor/HeraldRuleSerializer.php new file mode 100644 index 0000000000..d1b2527aaa --- /dev/null +++ b/src/applications/herald/editor/HeraldRuleSerializer.php @@ -0,0 +1,73 @@ +serializeRuleComponents( + (bool)$rule->getMustMatchAll(), + $rule->getConditions(), + $rule->getActions(), + HeraldRepetitionPolicyConfig::toString($rule->getRepetitionPolicy())); + } + + public function serializeRuleComponents( + $match_all, + array $conditions, + array $actions, + $repetition_policy) { + + assert_instances_of($conditions, 'HeraldCondition'); + assert_instances_of($actions, 'HeraldActionRecord'); + + $conditions_array = array(); + foreach ($conditions as $condition) { + $conditions_array[] = array( + 'field' => $condition->getFieldName(), + 'condition' => $condition->getFieldCondition(), + 'value' => $condition->getValue(), + ); + } + + $actions_array = array(); + foreach ($actions as $action) { + $actions_array[] = array( + 'action' => $action->getAction(), + 'target' => $action->getTarget(), + ); + } + + return array( + 'match_all' => $match_all, + 'conditions' => $conditions_array, + 'actions' => $actions_array, + 'repetition_policy' => $repetition_policy, + ); + } + + public function deserializeRuleComponents(array $serialized) { + $deser_conditions = array(); + foreach ($serialized['conditions'] as $condition) { + $deser_conditions[] = id(new HeraldCondition()) + ->setFieldName($condition['field']) + ->setFieldCondition($condition['condition']) + ->setValue($condition['value']); + } + + $deser_actions = array(); + foreach ($serialized['actions'] as $action) { + $deser_actions[] = id(new HeraldActionRecord()) + ->setAction($action['action']) + ->setTarget($action['target']); + } + + return array( + 'match_all' => $serialized['match_all'], + 'conditions' => $deser_conditions, + 'actions' => $deser_actions, + 'repetition_policy' => $serialized['repetition_policy'], + ); + } + +} diff --git a/src/applications/herald/storage/HeraldRuleTransaction.php b/src/applications/herald/storage/HeraldRuleTransaction.php index 8d4201faf6..b1bd563749 100644 --- a/src/applications/herald/storage/HeraldRuleTransaction.php +++ b/src/applications/herald/storage/HeraldRuleTransaction.php @@ -4,6 +4,7 @@ final class HeraldRuleTransaction extends PhabricatorApplicationTransaction { const TYPE_EDIT = 'herald:edit'; + const TYPE_NAME = 'herald:name'; const TYPE_DISABLE = 'herald:disable'; public function getApplicationName() { @@ -45,6 +46,8 @@ final class HeraldRuleTransaction } else { return pht('Enabled'); } + case self::TYPE_NAME: + return pht('Renamed'); } return parent::getActionName(); @@ -84,9 +87,49 @@ final class HeraldRuleTransaction '%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()); + } + }