Details
-
Type:
Bug
-
Status: Done/Fixed
-
Priority:
Major
-
Resolution: Fixed/Completed
-
Affects Version/s: 4.3.4
-
Fix Version/s: 4.4.3
-
Component/s: Accounting Integration, CiviContribute
-
Labels:
Description
Tasks to address issue:
1. If more than one of the relevant fields change (ie fields labelled: Finanacial Type, Total Amount, Contribution Status, or Paid By), then each change should create its own appropriate transaction(s) before the next change goes on to create its transaction(s) in the civicrm_financial_trxn table. We should likely change the amount last, and amongst the other changes, let's order them, perhaps, as follows: the account first, the status second, the payment instrument (Paid By) third, and, to repeat, the amount last. All but the Paid By field change are similar to other change transactions we have implemented.
2. If the Paid By changes, we have to do things differently because we can't tell the external system that a change transaction for the new Paid By relates to the original transaction. In this case, create two financial transaction entries: the first reverses the debit and credit accounts from the original transaction, leaving the other fields unchanged, which cancels completely the original transaction from an accounting point of view; the second is the same as the original transaction except for the change in the Paid By field, which is basically recreating the original transaction with the new Paid By info.
Original issue description:
In testing some batch functionality on the demo site (D7 Civi 4.3), a contribution that I had edited appeared twice in the batch selection page, and I was able to attach it to two different batches.
Steps to reproduce:
1) Create a contribution and leave pay method blank.
2) Create a second contribution and set pay method to check.
3) Edit the first contribution and set pay method to cash.
4) Create a new batch.
5) At this point, the edited contribution is listed twice in the contributions that can be added to the batch: http://screencast.com/t/sfj9l1XtW
6) Added the two contribution listings with payment method indicated to batch and closed.
7) Created a new batch - the original contribution listing (the one from before I edited the payment method) was still listed and I was able to add it to my new batch.
Attachments
Issue Links
- is blocked by
-
-
- Done/Fixed
-
Activity
Can u review to confirm what the behavior should be for cases like this where there's no change to the INCOME account during an edit.
Joe – if you want to discuss please contact me tomorrow (Aug 30). I'd like to help logic this out. I was playing around with a bunch of other contribution edits payment method, amount, status and I don't think I see the behavior DaveG is suggesting 
Also, on the page in Lesley's screen shot, are the detail items "financial transactions" (not contributions)? notice the different time stamps. Also, if the edit did not change the $$ amount nor the fund, then I would not expect that there should be a corresponding financial transaction with an empty payment method (for a total of 3 transaction line items for the contribution)
I believe, adjustments should always yield 2 transactions if changing payment types, 1 transaction for changes in amount, either 0 or 2 for changes in status.
1. If more than one of the relevant fields change (ie fields labelled: Finanacial Type, Total Amount, Contribution Status, or Paid By), then each change should create its own appropriate transaction(s) before the next change goes on to create its transaction(s) in the civicrm_financial_trxn table. We should likely change the amount last, and amongst the other changes, let's order them, perhaps, as follows: the account first, the status second, the payment instrument (Paid By) third, and, to repeat, the amount last.
2. If the Paid By changes then the transaction should have the same account on both sides of the transaction. It's basically a null transaction from the perspective of external systems like QuickBooks that only look at the Account field. In terms of the csv that Dave uploaded, cells B3-D3 should be copied over to M3-O3 in the 'change' transaction.
NB: I am assuming that the change in Paid By was done within a minute of the original transaction, since the date in A3 is normally going to be different than the original transaction's date in A2. We see this in times in Lesley's two $100 transactions are different by 1 minute.
3. However, some external systems will be making use of the Paid By field, and they will need to be able to figure out that the two transactions relate to each other, and I am not sure how that could be done. Until we have a definite need for this, I would prefer not to 'gold-plate' by solving this potential problem. Paul Keogan - if this is your use case, could you comment on what is needed to fix this?
Joe, I agree we don't need #3 right now. If we do need to add this functionality I believe we would create two financial tranx entries the first would reverse the debit and credit accounts from the original transaction, leaving the other fields unchanged and the second would be the same as the original transaction except for the change in the Paid By field. Does that make sense?
I think your approach is simple and elegant and does the job. Let's wait to see if it is needed - it won't be nearly as hard to implement as the approaches I was considering.
Please proceed to make these changes. We can discuss details that come up Monday or preferrably Tuesday.
Pradeep, I have incorporated Paul's response into the Description of work for the issue, which is now what you should work from.
We need the payment instrument to be non-null or the change of payment instrument will lead to invalid values.
Submitted PR at https://github.com/civicrm/civicrm-core/pull/2044
Changed code to create two transaction when Payment instrument(Paid by) is changed. And also followed order when multiple fields(Finanacial Type, Total Amount, Contribution Status, or Paid By) are changed.
The manual tests I ran look good. Tested various scenarios including payment instrument and amount and fee changes.
However, a key api test now fails. Pretty sure the test needs to be updated to reflect the new behavior (i.e. the 'bug' is in the test). Can you please check and fix:
1) api_v3_ContributionTest::testCreateUpdateContributionPaymentInstrument
Attribute 'from_financial_account_id' not present in actual array.
/Users/dgg/git/crm_v4.4/tests/phpunit/CiviTest/CiviUnitTestCase.php:586
/Users/dgg/git/crm_v4.4/tests/phpunit/CiviTest/CiviUnitTestCase.php:526
/Users/dgg/git/crm_v4.4/tests/phpunit/api/v3/ContributionTest.php:1506
/Users/dgg/git/crm_v4.4/tests/phpunit/api/v3/ContributionTest.php:898
/Users/dgg/git/crm_v4.4/tools/scripts/phpunit:75
Fixed api test and submitted PR at https://github.com/civicrm/civicrm-core/pull/2137
API test testCreateUpdateContributionPaymentInstrument runs properly.
Leslie and Joe - I think the second item is an adjustment for the edited contribution. New financial transactions are created when contributions are edited IF the payment method, amount, and/or in some cases the status is modified. The batch llst shows a row for each financial transaction. So in general it's not a bug if a contribution appears > 1 time. HOWEVER, in this specific case where the payment method was changed from EMPTY to Cash, the export output looks wrong to me since there's no reflection of a transfer of funds from one income account to another. I've attached my CSV output from a test where I added a contribution w/ no payment method and then updated it to set payment method to Cash.