Uploaded image for project: 'CiviCRM'
  1. CiviCRM
  2. CRM-6112

Inconsistency in the money value handling in web form input fields - leads to misinterpretation of values by a factor of 100

    Details

    • Type: Bug
    • Status: Done/Fixed
    • Priority: Major
    • Resolution: Fixed/Completed
    • Affects Version/s: 3.1.4
    • Fix Version/s: 3.2
    • Component/s: Internationalisation
    • Labels:
      None

      Description

      There is a inconsistency (or even conceptual problem?) in the handling of money values spread over different forms. In my case (de_DE locale), while renewing memberships CiviCRM stored the fee payed by a factor 100 to big (25'000.00 instead of 250.00).

      After studying some source code I think I found some inconsistencies in the money value handling. (I reference CiviRCM Version 3.1.4)

      1. case: http://.../civicrm/contact/view/contribution?reset=1&action=update&id=24&cid=13&context=contribution
      -------------------------------------------------------------------------------------------------------------------------------------------
      In templates/CRM/Contribute/Form/Contribution.tpl (line 439)

      {$form.total_amount.html|crmMoney:$currency|crmReplace:class:eight}

      In CRM/Contribute/Form/Contribution.php (line 500)

      require_once 'CRM/Utils/Money.php';
      // fix the display of the monetary value, CRM-4038
      if (isset($defaults['total_amount']))

      { $defaults['total_amount'] = CRM_Utils_Money::format($defaults['total_amount'], null, '%a'); }

      In the template the author tried to format the value as a money-value with the 'crmMoney' plugin. But this plugin cannot handle form input fields but only plain number values. This results in the following HTML code:

      CHF <input type="text" class="eight" id="total_amount" value="350,00" name="total_amount" maxlength="14" size="6">

      Which means the value field is NOT formatted by this plugin, only the currency is printed in front of the whole html input field. To correct this misbehavior the second code snippet in the setDefaultValues() function has been added.

      But in my understanding of the MVC pattern one should not format the values in the model but only in the view (tpl).

      How the entered value is then checked/post processed I have not checked.

      2. case http://.../civicrm/contact/view/membership?action=renew&reset=1&cid=16&id=5&context=membership&selectedChild=member
      --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
      In templates/CRM/Member/Form/MembershipRenewal.tpl (line 83)

      {$form.total_amount.html}

      In CRM/Member/Form/MembershipRenewal.php
      in function setDefaultValues() (line 166)

      $defaults['total_amount'] = CRM_Core_DAO::getFieldValue( 'CRM_Member_DAO_MembershipType',
      $this->_memType,
      'minimum_fee' );

      and in function buildQuickForm() (line 248)

      $this->addRule('total_amount', ts('Please enter a valid amount.'), 'money');

      => This template does no formatting of the money value at all. That means the HTML code generated looks like this:

      <input name="total_amount" value="350.00" class="form-text" id="total_amount">

      The value is not (money) formatted at all it come directly out of the database (CRM_Core_DAO::getFieldValue). I think thats the correct way of doing it.
      Bute the template should format the value but as much as I know there is no plugin (like crmMoney) which is able to format form input fields correctly.
      So in this case the value displayed in the form is not formatted which means has a '.' as decimal separator. But the rule (money) added does a value validation according to the given locale. The corresponding function is:

      money() in file CRM/Utils/Rule.php and calls:

      $value = self::cleanMoney( $value );

      The function cleanMoney(), which btw I think should go to the file Money.php, does the following:

      setlocale( LC_ALL, $config->lcMessages );
      $localeInfo = localeconv( );
      ...
      $value = str_replace( $mon_thousands_sep, '', $value );
      ...
      $value = str_replace( $mon_decimal_point, '.', $value );

      The intention is clear. It should transform a localized money value to normalized value. BUT there are 3 problems!

      • First it should not modify the process global environment variable LC_ALL at all.
      • If there would be no other solution than setting the environment it should only set LC_MONETARY
      • it uses $config->lcMessage instead of $config->lcMonetary!

      The needed mon_thousands_sep and mon_decimal_point values are alread set in the $config structure!! So I fixed the function like this:

      static function cleanMoney( $value )

      { // first remove all white space $value = str_replace( array( ' ', "\t", "\n" ), '', $value ); $config =& CRM_Core_Config::singleton( ); $mon_thousands_sep = $config->monetaryThousandSeparator; $value = str_replace( $mon_thousands_sep, '', $value ); $mon_decimal_point = $config->monetaryDecimalPoint; $value = str_replace( $mon_decimal_point, '.', $value ); return $value; }

      The template problem above I fixed this way:

      in file templates/CRM/Member/Form/MembershipRenewal.tpl I changed the 'total_amount' expression on line 83 to:

      <input id="total_amount" class="form-text" value="{$form.total_amount.value|crmMoney:'':'%a'}" name="total_amount" />

      which is not very elegant, I know! An other possibility would be to format the value in the setDefaultValues() function like in case 1 above, but I believe this would not be correct.

      3. money() vs. moneySigned()
      -----------------------------------------
      There seems to be an other problem!! The two functions money() and moneySigned() are nearly identical! But I think either they should do the same (eliminate one) or they should not do the same! moneySigned() does NOT accept '-.50' !!

      static function money($value)
      ....
      return preg_match( '/(?\d+\.\d?\d?$)|(?\.\d\d?$)/', $value ) ? true : false;

      static function moneySigned($value)
      ....
      return preg_match( '/(-?\d+\.\d?\d?$)|(\.\d\d?$)/', $value ) ? true : false;

        Attachments

          Activity

            People

            • Assignee:
              sushant Sushant Paste
              Reporter:
              this64 Matthias Loepfe
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 1 day
                1d
                Remaining:
                Remaining Estimate - 1 day
                1d
                Logged:
                Time Spent - Not Specified
                Not Specified