From 5a604538ca4020c78548eab6d405ec2ffe384961 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 22 Mar 2016 09:08:26 -0700 Subject: [PATCH] Fix an initialization issue in Herald rules in Chrome Summary: Fixes T10646. When you load the page or click "New Condition" or "New Action", we try to add a condition and action with some default values. Currently, the logic just sets everything to `null` or `'default'`. This technically works in Safari, but is less successful in Chrome. (I think Safari prevents you from picking an invalid value.) Instead of relying on the browser to pick the right value, set the correct value explicitly. Test Plan: - Created a new rule in Chrome, Safari. - Added fields and conditions in Chrome, Safari. - Edited existing rules in Chrome, Safari. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10646 Differential Revision: https://secure.phabricator.com/D15507 --- resources/celerity/map.php | 22 ++--- .../controller/HeraldRuleController.php | 85 +++++++++++-------- .../js/application/herald/HeraldRuleEditor.js | 16 +++- 3 files changed, 73 insertions(+), 50 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 48a8367edb..d5d2616f0c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -401,7 +401,7 @@ return array( 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef', 'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', - 'rsrc/js/application/herald/HeraldRuleEditor.js' => '746ca158', + 'rsrc/js/application/herald/HeraldRuleEditor.js' => 'd6a7e717', 'rsrc/js/application/herald/PathTypeahead.js' => 'f7fc67ec', 'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3', 'rsrc/js/application/maniphest/behavior-batch-editor.js' => '782ab6e7', @@ -563,7 +563,7 @@ return array( 'global-drag-and-drop-css' => '5c1b47c2', 'harbormaster-css' => '834879db', 'herald-css' => 'dc31f6e9', - 'herald-rule-editor' => '746ca158', + 'herald-rule-editor' => 'd6a7e717', 'herald-test-css' => 'a52e323e', 'inline-comment-summary-css' => '51efda3a', 'javelin-aphlict' => '5359e785', @@ -1435,15 +1435,6 @@ return array( 'javelin-vector', 'javelin-dom', ), - '746ca158' => array( - 'multirow-row-manager', - 'javelin-install', - 'javelin-util', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-json', - 'phabricator-prefab', - ), '76b9fc3e' => array( 'javelin-behavior', 'javelin-stratcom', @@ -1923,6 +1914,15 @@ return array( 'javelin-dom', 'javelin-stratcom', ), + 'd6a7e717' => array( + 'multirow-row-manager', + 'javelin-install', + 'javelin-util', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-json', + 'phabricator-prefab', + ), 'd75709e6' => array( 'javelin-behavior', 'javelin-workflow', diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index d8e5e42df3..d7963a5193 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -364,43 +364,6 @@ final class HeraldRuleController extends HeraldController { array $handles, HeraldAdapter $adapter) { - $serial_conditions = array( - array('default', 'default', ''), - ); - - if ($rule->getConditions()) { - $serial_conditions = array(); - foreach ($rule->getConditions() as $condition) { - $value = $adapter->getEditorValueForCondition( - $this->getViewer(), - $condition); - - $serial_conditions[] = array( - $condition->getFieldName(), - $condition->getFieldCondition(), - $value, - ); - } - } - - $serial_actions = array( - array('default', ''), - ); - - if ($rule->getActions()) { - $serial_actions = array(); - foreach ($rule->getActions() as $action) { - $value = $adapter->getEditorValueForAction( - $this->getViewer(), - $action); - - $serial_actions[] = array( - $action->getAction(), - $value, - ); - } - } - $all_rules = $this->loadRulesThisRuleMayDependUpon($rule); $all_rules = mpull($all_rules, 'getName', 'getPHID'); asort($all_rules); @@ -492,10 +455,58 @@ final class HeraldRuleController extends HeraldController { $config_info['targets'][$action] = $value_key; } + $default_group = head($config_info['fields']); + $default_field = head_key($default_group['options']); + $default_condition = head($config_info['conditionMap'][$default_field]); + $default_actions = head($config_info['actions']); + $default_action = head_key($default_actions['options']); + + if ($rule->getConditions()) { + $serial_conditions = array(); + foreach ($rule->getConditions() as $condition) { + $value = $adapter->getEditorValueForCondition( + $this->getViewer(), + $condition); + + $serial_conditions[] = array( + $condition->getFieldName(), + $condition->getFieldCondition(), + $value, + ); + } + } else { + $serial_conditions = array( + array($default_field, $default_condition, null), + ); + } + + if ($rule->getActions()) { + $serial_actions = array(); + foreach ($rule->getActions() as $action) { + $value = $adapter->getEditorValueForAction( + $this->getViewer(), + $action); + + $serial_actions[] = array( + $action->getAction(), + $value, + ); + } + } else { + $serial_actions = array( + array($default_action, null), + ); + } + Javelin::initBehavior( 'herald-rule-editor', array( 'root' => 'herald-rule-edit-form', + 'default' => array( + 'field' => $default_field, + 'condition' => $default_condition, + 'action' => $default_action, + ), 'conditions' => (object)$serial_conditions, 'actions' => (object)$serial_actions, 'template' => $this->buildTokenizerTemplates() + array( diff --git a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js index ba01c9bef3..8c6b46ba44 100644 --- a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js +++ b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js @@ -323,7 +323,14 @@ JX.install('HeraldRuleEditor', { _newCondition : function(data) { var row = this._conditionsRowManager.addRow([]); var row_id = this._conditionsRowManager.getRowID(row); - this._config.conditions[row_id] = data || [null, null, '']; + + var default_condition = [ + this._config.default.field, + this._config.default.condition, + null + ]; + this._config.conditions[row_id] = data || default_condition; + var r = this._conditionsRowManager.updateRow( row_id, this._renderCondition(row_id)); @@ -369,7 +376,12 @@ JX.install('HeraldRuleEditor', { }, _newAction : function(data) { - data = data || []; + var default_action = [ + this._config.default.action, + null + ]; + + data = data || default_action; var temprow = this._actionsRowManager.addRow([]); var row_id = this._actionsRowManager.getRowID(temprow); this._config.actions[row_id] = data;