Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CRM-20145 line_item $0 entity_financial_trxn fix #9866

Merged
merged 5 commits into from
Feb 27, 2017

Conversation

lcdservices
Copy link
Contributor

@lcdservices lcdservices commented Feb 21, 2017

@@ -5418,7 +5418,8 @@ public static function getSalesTaxFinancialAccounts() {
public static function createProportionalEntry($entityParams, $eftParams) {
$paid = $entityParams['line_item_amount'] * ($entityParams['trxn_total_amount'] / $entityParams['contribution_total_amount']);
// Record Entity Financial Trxn
$eftParams['amount'] = round($paid, 2);
// CRM-20145
$eftParams['amount'] = number_format((float)round($paid, 2), 2, '.', '');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lcdservices could we make this into a tiny utility function that can be used wherever we are about to store a money value in db? Currently we should always be storing 2 digits after the decimal in core, but cryptocurrencies will need more. @pradpnayak could you ensure we create/use the same utility function throughout work on sales taxes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the PR accordingly.

@eileenmcnaughton
Copy link
Contributor

@KarinG since you have had your head deep into this code can you comment as to whether you have any reason to think this should, or shouldn't be merged?

@KarinG
Copy link
Contributor

KarinG commented Feb 26, 2017

Actually - @eileenmcnaughton sorry... I've not been here yet... To be frank: I'm not sure what proportional (as opposed to non-proportional ones?) financial transactions represent in terms of sales taxes - or which kind is needed when? Eh... I find eftParams very confusing - EFT in financial language is always used for Electronic Funds Transfer - and pretty sure that's not what's going on here. However if @lcdservices says this (whatever this is) is working for him if with this edit - then let's just get this in for him? Ok - I've got to get back to my own corporate Sales Tax return - due tomorrow - for real :-)

@eileenmcnaughton
Copy link
Contributor

Well I think that is a blessing from Karin (of sorts) - still need the style issues resolved

@lcdservices
Copy link
Contributor Author

@eileenmcnaughton style issues are fixed.
FYI, this PR doesn't really change anything about the proportional entry process aside from making sure the amount is formatted correctly.

@eileenmcnaughton eileenmcnaughton merged commit e05f487 into civicrm:master Feb 27, 2017
monishdeb pushed a commit to monishdeb/civicrm-core that referenced this pull request May 2, 2017
CRM-20145 line_item $0 entity_financial_trxn fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants