CRM-20899 New refund form: format amount up to 2 decimal places #10687
Conversation
| @@ -185,7 +185,7 @@ public function setDefaultValues() { | |||
| } | |||
|
|
|||
| if ($this->_refund) { | |||
| $defaults['total_amount'] = abs($this->_refund); | |||
| $defaults['total_amount'] = number_format(abs($this->_refund), 2); | |||
| } | |||
There was a problem hiding this comment.
Certain currencies support more than 2 decimal places (@eileenmcnaughton: eg. bitcoin) so not sure we should be hardcoding 2 here. See https://github.com/civicrm/civicrm-core/pull/10641/files#diff-9dacd15500c58030b03a70b7501fa5e5R87 or CRM_Utils_Money::format with onlyNumber=TRUE might be more appropriate?
There was a problem hiding this comment.
In this case the goal seems to be to have AT LEAST 2 decimal places , rather than 'not more than 2 decimal places. In this case it seems our primary goal is to pad it
(aside: That function you have mentioned looks nasty. ie. the function has been hacked to be 'two functions in one' -
it does something different if $onlyNumber is passed in which looks like a code smell to me)
There was a problem hiding this comment.
@eileenmcnaughton so for now, should we allow the current fix that is formatting refund money up to 2 decimal place?
There was a problem hiding this comment.
I think we should be using CRM_Utils_Money::format function shouldn't we?
There was a problem hiding this comment.
@eileenmcnaughton See the first comment :-) that you commented on. number_format($var,2) will round to 2decimal places - so will CRM_Utils_Money::format. I do agree that CRM_Utils_Money::format is a bit of a messy function, but that should probably be cleaned up as a separate exercise. All money amounts that are formatted for display should really go through that function so we have one place for formatting. Note that I can't use it when I want to write more than 2dp to the database eg. (https://github.com/civicrm/civicrm-core/pull/10641/files#diff-9bb0aa4010a43713521df361cca72fe6R841). Though it might make sense in that link to replace my filter_var with another CRM_Utils_money function!
In this case, as the value is ONLY destined for display, I think CRM_Utils_Money::format(...onlyNumber=TRUE...) is the right function to use.
27c4f68 to
7583896
Compare
| @@ -184,7 +184,7 @@ public function setDefaultValues() { | |||
| } | |||
|
|
|||
| if ($this->_refund) { | |||
| $defaults['total_amount'] = abs($this->_refund); | |||
| $defaults['total_amount'] = CRM_Utils_Money::format(abs($this->_refund), NULL, NULL, TRUE); | |||
There was a problem hiding this comment.
@eileenmcnaughton @mattwire I've made the changes. It's working fine on my local.
There was a problem hiding this comment.
@monishdeb I haven't looked at this in detail but I don't think you need the abs() in there?
There was a problem hiding this comment.
I thought so but $this->_refund holds negative amount and eventually, it results to (50.12) for -50.12 value, which is how the money_format(..) deals with negative amount (see here) So we need to use abs().
There was a problem hiding this comment.
Happy with this, I do think we need to look at refactoring that CRM_Utils_Money::format function - it's too vague with what it returns and we get wierd issues like this in different locales! See https://issues.civicrm.org/jira/browse/CRM-16460 and https://issues.civicrm.org/jira/browse/CRM-20772. But I agree this PR is ready to merge @JoeMurray
|
Jenkins test this please |
|
QA'd, Number is formatted correctly |
|
I tested this patch and it works for me. Merging. |
CRM-20899, formatted number upto 2 decimal place
https://issues.civicrm.org/jira/browse/CRM-20898
https://issues.civicrm.org/jira/browse/CRM-20899