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

Resolve Issue with Payment Allocation to Contribution Line Item #29350

Conversation

olayiwola-compucorp
Copy link
Contributor

@olayiwola-compucorp olayiwola-compucorp commented Feb 9, 2024

Overview

This pull request addresses an issue where incorrect amounts were allocated to contribution line items during payment processing.

Considering the contribution entity table provided:

Line Price Quantity Subtotal exc Tax Tax Tax total Total inc Tax
Line 1 100 1 100 0% 0 100
Line 2 200 1 200 0% 0 200
Line 3 300 1 300 20% 60 360
Total 660

Before

Before this PR, the following EntityFinancialTrxn was created:

Subtotal exc Tax Payment 1 (200) Payment 2 (200) Payment 3 (200)
Line 1 100 30.30 30.30 30.30
Line 2 200 60.61 60.60 60.60
Line 3 300 90.91 83.00 69.02
Tax line 1 0 0.00 0.00 0.00
Tax line 2 0 0.00 0.00 0.00
Tax line 3 60 18.18 18.18 18.18
Total 660 200.00 192.08 178.10

From this table, it's evident that incorrect amounts are assigned to line item 3 following the initial payment.

After

After this PR, the following EntityFinancialTrxn is created:

Subtotal exc Tax Payment 1 (200) Payment 2 (200) Payment 3 (200)
Line 1 100 30.30 30.30 30.30
Line 2 200 60.61 60.61 60.61
Line 3 300 90.91 90.91 90.91
Tax line 1 0 0.00 0.00 0.00
Tax line 2 0 0.00 0.00 0.00
Tax line 3 60 18.18 18.18 18.18
Total 660 200.00 200.00 200.00

Line Item 3 now receives the accurate allocation with subsequent payments.

Replication Step

  • Create a pending contribution with a tax line item (maybe 100 + tax 20)
  • Make the first partial payment (maybe 60) - Check that the EntityFinancialTrxns created for the payment has the correct total
  • Make the second partial payment (maybe 30) - Check that the EntityFinancialTrxns created for the payment has an incorrect total

Copy link

civibot bot commented Feb 9, 2024

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Feb 9, 2024
@eileenmcnaughton
Copy link
Contributor

tests didn't complete - test this please

@olayiwola-compucorp how do we replicate this

@JoeMurray are you able to get someone to look - we should get a test if we do merge this

@jamienovick
Copy link

@olayiwola-compucorp can you add some details of how to recreate the issue via the UI?

@olayiwola-compucorp
Copy link
Contributor Author

@eileenmcnaughton I've updated the PR desc with replication step, I also added a test

cc @jamienovick

@jamienovick
Copy link

jamienovick commented Feb 19, 2024

@eileenmcnaughton Do you have what you need to validate this? Let me know if you have more questions and I can add some more details. Would be great to close it off. Best

olayiwola-compucorp added a commit to compucorp/civicrm-core that referenced this pull request Mar 8, 2024
@eileenmcnaughton
Copy link
Contributor

@olayiwola-compucorp I did have a go at an alternate to this - but just got a bit stuck at the end #29503 on the test - it's a bigger change but this https://github.com/civicrm/civicrm-core/pull/29503/files#diff-74b281ac668a837edcbfabd5cb0d74b5647c92d24f63777ab964bc6376291e25L167 kinda points out where the real probelm is

@olayiwola-compucorp
Copy link
Contributor Author

Thanks, @eileenmcnaughton, I moved the work done here to a more complete PR #29823, it also tackled the problem https://github.com/civicrm/civicrm-core/pull/29823/files#diff-74b281ac668a837edcbfabd5cb0d74b5647c92d24f63777ab964bc6376291e25, but in a different way.

cc @jamienovick

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.

3 participants