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

Fix Payment.create to update (recalculate) contribution fee_amount #20008

Merged
merged 1 commit into from
Apr 9, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Apr 8, 2021

Overview

Fixes a bug identified by @ananelson (I think) on chat whereby adding a payment to a contribution does not update the contributions fee_amount

Before

Payment create does not alter fee_amount & net_amount at the contribution level

After

Payment create forces a recalculate of fee_amount & net_amount based on the sum of the payments

Technical Details

Our data integrity expects the fee amount on the contribution to equal the sum of the fee amount on the payments. However, it's possible a pending contribution has an 'estimated' fee amount so we can't just add on
the fee_amount and assume it will be right.

I had some different thoughts about how to calculate the fee amount but I also hit some gaps when I tried so this approach seems simple & the test locks it in

Comments

@mattwire @monishdeb

@civibot
Copy link

civibot bot commented Apr 8, 2021

(Standard links)

@civibot civibot bot added the master label Apr 8, 2021
This recalculates the fee amount when a payment is received. Our data integrity expects the fee
amount on the contribution to equal the sum of the fee amount on the payments. However,
it's possible a pending contribution has an 'estimated' fee amount so we can't just add on
the fee_amount and assume it will be right
@mattwire
Copy link
Contributor

mattwire commented Apr 9, 2021

This looks like the right approach - it is probably the fix for this issue too: https://lab.civicrm.org/extensions/stripe/-/issues/267

@eileenmcnaughton
Copy link
Contributor Author

@mattwire I was assuming the reason Ana pinged you on chat was because she thought it related to Stripe

@monishdeb
Copy link
Member

@mattwire I have tested the patch in my local and does the right thing by recalculating the fee and net amount. Also there is a uni-test which passed. Is there any concern which need to be addressed before merging this PR?

@mattwire
Copy link
Contributor

mattwire commented Apr 9, 2021

@monishdeb No, just needed a review which you have now done :-)

@mattwire mattwire merged commit 59553ea into civicrm:master Apr 9, 2021
@eileenmcnaughton eileenmcnaughton deleted the pay_fee branch April 9, 2021 19:38
@eileenmcnaughton
Copy link
Contributor Author

thanks @mattwire @monishdeb

olayiwola-compucorp added a commit to compucorp/civicrm-core that referenced this pull request Aug 3, 2021
olayiwola-compucorp added a commit to compucorp/civicrm-core that referenced this pull request Aug 3, 2021
olayiwola-compucorp added a commit to compucorp/civicrm-core that referenced this pull request Aug 3, 2021
olayiwola-compucorp added a commit to compucorp/civicrm-core that referenced this pull request Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants