From 477371047bef14c2ee8ffc73a5f9cc06ffd13593 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Sun, 4 Jan 2015 22:05:40 -0500 Subject: [PATCH] CRM-15759 - Improve crmEditable api validation and error handling --- api/v3/Generic/Setvalue.php | 45 ++++++++++++++++++++----- api/v3/utils.php | 4 +-- js/Common.js | 7 ++-- js/jquery/jquery.crmeditable.js | 58 +++++++++++++++++---------------- 4 files changed, 72 insertions(+), 42 deletions(-) diff --git a/api/v3/Generic/Setvalue.php b/api/v3/Generic/Setvalue.php index 53c136d850..f7545a1287 100644 --- a/api/v3/Generic/Setvalue.php +++ b/api/v3/Generic/Setvalue.php @@ -26,23 +26,35 @@ function civicrm_api3_generic_setValue($apiRequest) { return $fields; $fields = $fields['values']; - if (!array_key_exists($field, $fields)) { + $isCustom = strpos($field, 'custom_') === 0; + // Trim off the id portion of a multivalued custom field name + $fieldKey = $isCustom && substr_count($field, '_') > 1 ? rtrim(rtrim($field, '1234567890'), '_') : $field; + if (!array_key_exists($fieldKey, $fields)) { return civicrm_api3_create_error("Param 'field' ($field) is invalid. must be an existing field", array("error_code" => "invalid_field", "fields" => array_keys($fields))); } - $def = $fields[$field]; + $def = $fields[$fieldKey]; + $title = CRM_Utils_Array::value('title', $def, ts('Field')); // Disallow empty values except for the number zero. // TODO: create a utility for this since it's needed in many places - // if (array_key_exists('required', $def) && CRM_Utils_System::isNull($value)) { - if (array_key_exists('required', $def) && empty($value) && $value !== '0' && $value !== 0) { - return civicrm_api3_create_error(ts("This can't be empty, please provide a value"), array("error_code" => "required", "field" => $field)); + if (!empty($def['required']) || !empty($def['is_required'])) { + if ((empty($value) || $value === 'null') && $value !== '0' && $value !== 0) { + return civicrm_api3_create_error(ts('%1 is a required field.', array(1 => $title)), array("error_code" => "required", "field" => $field)); + } } switch ($def['type']) { + case CRM_Utils_Type::T_FLOAT: + if (!is_numeric($value) && !empty($value) && $value !== 'null') { + return civicrm_api3_create_error(ts('%1 must be a number.', array(1 => $title)), array('error_code' => 'NaN')); + } + break; + case CRM_Utils_Type::T_INT: - if (!is_numeric($value)) { - return civicrm_api3_create_error("Param '$field' must be a number", array('error_code' => 'NaN')); + if (!CRM_Utils_Rule::integer($value) && !empty($value) && $value !== 'null') { + return civicrm_api3_create_error(ts('%1 must be a number.', array(1 => $title)), array('error_code' => 'NaN')); } + break; case CRM_Utils_Type::T_STRING: case CRM_Utils_Type::T_TEXT: @@ -61,7 +73,13 @@ function civicrm_api3_generic_setValue($apiRequest) { break; case CRM_Utils_Type::T_BOOLEAN: - $value = (boolean) $value; + // Allow empty value for non-required fields + if ($value === '' || $value === 'null') { + $value = ''; + } + else { + $value = (boolean) $value; + } break; default: @@ -70,11 +88,20 @@ function civicrm_api3_generic_setValue($apiRequest) { $dao_name = _civicrm_api3_get_DAO($entity); $params = array('id' => $id, $field => $value); + + if ((!empty($def['pseudoconstant']) || !empty($def['option_group_id'])) && $value !== '' && $value !== 'null') { + _civicrm_api3_api_match_pseudoconstant($params, $entity, $field, $def); + } + CRM_Utils_Hook::pre('edit', $entity, $id, $params); // Custom fields - if (strpos($field, 'custom_') === 0) { + if ($isCustom) { CRM_Utils_Array::crmReplaceKey($params, 'id', 'entityID'); + // Treat 'null' as empty value. This is awful but the rest of the code supports it. + if ($params[$field] === 'null') { + $params[$field] = ''; + } CRM_Core_BAO_CustomValueTable::setValues($params); CRM_Utils_Hook::post('edit', $entity, $id, CRM_Core_DAO::$_nullObject); return civicrm_api3_create_success($params); diff --git a/api/v3/utils.php b/api/v3/utils.php index 4786be51e2..78ea5b4be7 100644 --- a/api/v3/utils.php +++ b/api/v3/utils.php @@ -1803,7 +1803,7 @@ function _civicrm_api3_resolve_contactID($contactIdExpr) { function _civicrm_api3_validate_html(&$params, &$fieldName, $fieldInfo) { if ($value = CRM_Utils_Array::value($fieldName, $params)) { if (!CRM_Utils_Rule::xssString($value)) { - throw new API_Exception('Illegal characters in input (potential scripting attack)', array("field"=>$fieldName,"error_code"=>"xss")); + throw new API_Exception(ts('Illegal characters in input (potential scripting attack)'), array("field"=>$fieldName,"error_code"=>"xss")); } } } @@ -1830,7 +1830,7 @@ function _civicrm_api3_validate_string(&$params, &$fieldName, &$fieldInfo, $enti } if ($value ) { if (!CRM_Utils_Rule::xssString($value)) { - throw new Exception('Illegal characters in input (potential scripting attack)'); + throw new Exception(ts('Illegal characters in input (potential scripting attack)')); } if ($fieldName == 'currency') { if (!CRM_Utils_Rule::currencyCode($value)) { diff --git a/js/Common.js b/js/Common.js index 9dc4d20929..79cc9feb21 100644 --- a/js/Common.js +++ b/js/Common.js @@ -841,8 +841,9 @@ CRM.strings = CRM.strings || {}; var opts = $.extend({ start: ts('Saving...'), success: ts('Saved'), - error: function() { - CRM.alert(ts('Sorry an error occurred and your information was not saved'), ts('Error')); + error: function(data) { + var msg = $.isPlainObject(data) && data.error_message; + CRM.alert(msg || ts('Sorry an error occurred and your information was not saved'), ts('Error'), 'error'); } }, options || {}); var $msg = $('
' + opts.start + '
') @@ -1029,7 +1030,7 @@ CRM.strings = CRM.strings || {}; title = $label.text(); } } - $(this).addClass('error'); + $(this).addClass('crm-error'); } var msg = CRM.alert(text, title, 'error', $.extend(extra, options)); if ($(this).length) { diff --git a/js/jquery/jquery.crmeditable.js b/js/jquery/jquery.crmeditable.js index f1e0495519..860f9f3d69 100644 --- a/js/jquery/jquery.crmeditable.js +++ b/js/jquery/jquery.crmeditable.js @@ -49,37 +49,33 @@ field: info.field, value: $el.is(':checked') ? 1 : 0 }; - CRM.api3(info.entity, info.action, params, true) - .fail(function(data) { - editableSettings.error.call($el[0], info.entity, info.field, checked, data); - }) - .done(function(data) { - editableSettings.success.call($el[0], info.entity, info.field, checked, data); - }); + CRM.api3(info.entity, info.action, params, true); }); } - var defaults = { - error: function(entity, field, value, data) { - $(this).crmError(data.error_message, ts('Error')); - $(this).removeClass('crm-editable-saving'); - }, - success: function(entity, field, value, data, settings) { - var $i = $(this); - if ($i.data('refresh')) { - CRM.refreshParent($i); - } else { - $i.removeClass('crm-editable-saving crm-error crm-editable-editing'); - value = value === '' ? settings.placeholder : value; - $i.html(value); - } - } - }; - - var editableSettings = $.extend({}, defaults, options); return this.each(function() { var $i, - fieldName = ""; + fieldName = "", + defaults = { + error: function(entity, field, value, data) { + restoreContainer(); + $(this).html(originalValue || settings.placeholder).click(); + var msg = $.isPlainObject(data) && data.error_message; + errorMsg = $(':input', this).first().crmError(msg || ts('Sorry an error occurred and your information was not saved'), ts('Error')); + }, + success: function(entity, field, value, data, settings) { + restoreContainer(); + if ($i.data('refresh')) { + CRM.refreshParent($i); + } else { + value = value === '' ? settings.placeholder : value; + $i.html(value); + } + } + }, + originalValue = '', + errorMsg, + editableSettings = $.extend({}, defaults, options); if ($(this).hasClass('crm-editable-enabled')) { return; @@ -148,8 +144,11 @@ else { params[info.field] = value; } - CRM.api3(info.entity, action, params, true) + CRM.api3(info.entity, action, params, {error: null}) .done(function(data) { + if (data.is_error) { + return editableSettings.error.call($el[0], info.entity, info.field, value, data); + } if ($el.data('options')) { value = $el.data('options')[value] || ''; } @@ -188,6 +187,8 @@ // FIXME: This should be a response to an event instead of coupled with this function but jeditable 1.7.1 doesn't trigger any events :( $i.addClass('crm-editable-editing'); + originalValue = value; + if ($i.data('type') == 'select' || $i.data('type') == 'boolean') { if ($i.data('options')) { return formatOptions($i.data('options')); @@ -223,7 +224,8 @@ } function restoreContainer() { - $i.removeClass('crm-editable-editing'); + errorMsg && errorMsg.close && errorMsg.close(); + $i.removeClass('crm-editable-saving crm-editable-editing'); } }); -- 2.25.1