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-17281 Pledge payments: fix rounding bug with miscalculation for multiple payments against a pledge #10861

Merged
merged 7 commits into from
Aug 16, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Aug 15, 2017

Overview

FIX miscalculation of remaining pledge payments when a payment is made that exceeds them and the cent values do not accurately spread over the payments (e.g 12 payments to pay $100)
FROM @mlutfy #9585

Before

No test, payment miscalculates remaining payments when a payment is made that pays off multiple payments

screenshot 2017-08-15 16 46 36

After

Payments covered are set to zero rather than the overhang value remaining outstanding

Technical Details

Note that we added a placeholder function for the number of decimal places a payment can be in, rather than hard-coding it.

Comments


@mlutfy mlutfy changed the title Add unit test for multiple payments against a pledge CRM-17281 Pledge payments: fix rounding bug with miscalculation for multiple payments against a pledge Aug 15, 2017
@eileenmcnaughton
Copy link
Contributor Author

This code is from #9585 - that patch also involved quite a lot of cleanup - unfortunately a lot of that cleanup can't be used now as it is stale :-(

The following comments are ones I added to the last commit but copying here to make them easier to see

This PR fixes a bug where payments are miscalculated with weird results. If a payment
changes the amount of future payments in such a way as to make an irregular amount
e.g 10 is split over 3 payments, the payment amount needs to be recorded as .33 not
.333333, assuming a currency with 2 decimal places. This commit
ensures the rounding takes place & is locked in by the accompanying unit test.

Note that we discussed this & agreed to use a function as a placeholder for the number
of decimal places.

mlutfy and others added 4 commits August 15, 2017 16:28
…g pledge payments.

This PR fixes a bug where payments are miscalculated with weird results. If a payment
changes the amount of future payments in such a way as to make an irregular amount
e.g 10 is split over 3 payments, the payment amount needs to be recorded as .33 not
.333333, assuming a currency with 2 decimal places. This commit
ensures the rounding takes place & is locked in by the accompanying unit test.

Note that we discussed this & agreed to use a function as a placeholder for the number
of decimal places.
I'm having serious serious issues reading this code. Renaming the variable is a step towards making sense of
it.  actually refers to the next pledge payment installment that is unpaid,
matching from the oldest
@eileenmcnaughton
Copy link
Contributor Author

I spent a lot of time trying to understand why the removed line

      $oldestPayment = self::getOldestPledgePayment($pledgeID);

3e0b390

Was there in the first place, & tbh I really couldn't. It's worth noting that there is a clear reproducable problem that this line causes which can be seen in the UI or the unit test & which removing it fixes in both instances. While I don't think it will, even if we just move the bug I think this is a step forwards since the test means this variant cannot come back & the code makes baby-steps towards legibility in this PR.

As a reviewers collaboration I would normally merge this at this stage but at the moment we have an unresolved process here so @colemanw would you be able to merge this please?

@mlutfy can you please confirm there is nothing in the final version you are uncomfortable with.

@eileenmcnaughton
Copy link
Contributor Author

Screenshot of old borked behaviour - this is after paying $100 against a $100 pledge, still showing $0.04 on the last payment and pledge status not being Completed. This is fixed after this and we also tested making 2 payments which total $100 & confirmed no change.

screenshot 2017-08-15 16 46 36

@mlutfy
Copy link
Member

mlutfy commented Aug 15, 2017

I reviewed the changes and looks good!

@colemanw
Copy link
Member

@civicrm-builder retest this please

@colemanw colemanw merged commit 46bba86 into civicrm:master Aug 16, 2017
@eileenmcnaughton eileenmcnaughton deleted the bgm branch August 16, 2017 20:52
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.

4 participants