Page MenuHomePhabricator

[Epic] CiviCRM upgrade: Adapt refund processing & reporting to reflect changes since the upgrade.
Closed, ResolvedPublic4 Story Points

Description

Change summary

Prior to 4.3 CiviCRM did not really have a method for handling refunds but in 4.3 a change was made so that refunds are recorded in the financial_trxn table against the contribution they correspond to. Prior to 4.3 the civicrm_financial_trxn table was only really used for recording credit card payments but in 4.3 it was expanded to record all payments.

CiviCRM 4.2 behaviour
No real core behaviour for refunds. WMF added functionality to add a refund button in the UI and also a behaviour via scripts. The scripts

  1. Validated the contribution exists & is not already refunded
  2. Validates the refund does not exceed the original amount
  3. Updated the status of the original contribution to 'Refunded'
  4. Adds custom data to the new contribution: 'finance_only' => 1,
  5. Creates a new contribution with a financial type that reflects the refund type. Status is set to Completed for offline ones & potentially '"pending" or "Completed" for UI driven changes
  6. Copies custom data over to the new contribution
  7. Adds extra data to the Contribution Extra custom group - ie. 'parent_contribution_id' => $contribution->id, 'finance_only' => 1, 'no_thank_you' => 'refund', 'source_type' 'source_name' 'source_host' 'source_version' 'source_run_id' 'source_enqueued_time'

Data gaps on wmf flow

  1. Was the pending status being used for refunds done by Donor Services. None of the scripts appear to use this option.
  2. The checks potentially prevent an over-refund from being recorded but presumably unless the over-refund is a data entry error on the contribution-edit form it is the 'truth' even if it is a rort?

Assumption on WMF flow
The code handing the possibility of source = 'RFD' (last used in 2012) or total_amount=0 (described as a legacy protocol in the code comments) can be entirely disregarded as legacy.

CiviCRM 4.6 behaviour

CiviCRM tracks records in the civicrm_financial_trxn table the query is

SELECT * FROM civicrm_contribution c
LEFT JOIN civicrm_entity_financial_trxn ef ON c.id = ef.entity_id AND ef.entity_table = 'civicrm_contribution'
LEFT JOIN civicrm_financial_trxn f ON f.id = ef.financial_trxn_id
LIMIT 2

There a reserved status of 'Refunded' with a special meaning and setting this status results in negative entries being created in the financial_trxn table. There is also a 'Pending Refund' status we can look at if Donor Services do use that concept. There is currently a bug in that flow that might need fixing.

The financial_trxn table is used for a bit more than just payments. It's also used for internal transfers - ie. if you move money from the 'Membership' account to the 'Donation' account it creates rows to reflect that. In 4.7 an extra column is_payment is added to disambiguate these. I think we might end up with the odd extra row in this table when doing data entry fixes but I don't think it will have much impact.

As part of this investigation I added the UI to be able to see the financial contributions (when looking at a contribution row you should see it + in the screen shots on this issue). It's worth noting this UI is now in 4.7 but is not on other 4.6 sites so if communicating with other CiviCRM users don't expect them to be familiar with it. Also it is because of this I noticed the oddities I mentioned in the previous paragraph & I expect others will start to notice & fix them once 4.7 goes out https://issues.civicrm.org/jira/browse/CRM-17678

4.6 Option 1 - Co-operate with CiviCRM

If we are going to respect CiviCRM's wishes for handling refunds in 4.6 I don't think we need any custom code on the UI side of things. Donor services would need to edit the contribution to have a status of 'Refunded'. That is currently very slow but there are 2 fixes now upstreamed for this & pending QA on our side https://gerrit.wikimedia.org/r/#/c/256241/ & https://gerrit.wikimedia.org/r/#/c/257271/

On the script side I believe we should fix our mark_refund function to call the CiviCRM api like this

civicrm_api3('Contribution', 'create', array(
  'id' => x,
  'contribution_status_id' => 'Refunded',
  'trxn_id' => 'yhn',
  'finance_only' => 1,
   'no_thank_you' => 'refund',
);

We would need to add unit tests to wmf code AND to core CiviCRM to ensure that the correct financial_trxn rows are created when that happens. We would probably also want some error handling but per my notes above I'm not that clear about the value of error handling if the data is coming in from scripts whereas it it's from the UI I suspect the form validation should & probably does take care of it.

The downsides

  1. We lose the custom data that is specific to the refund - mostly forensic I think but I'm not really sure how important that data is.
  2. I am not sure if Core accurately creates the right rows when changing status to refund via the api & whether I'll need to do bug fixes and / or add tests there. I suspect it won't have correct handling for trxn_id for the refund at a guess.
  3. We would only have one financial type for both at the contribution level. Although the financial trxns have type as well it's less accessible .
  4. I'm sure we'll have a few more core bugs before we truly get to the end of this
  5. Change
  6. The Financial transactions have less visibility & the trxn_id of the refund is harder to search by (TBH I believe the contribution search should be fixed to search for that trxn_id rather than the one on the contribution table & the one on the contribution table should possibly be removed from 4.8).

Upsides

  1. We can move responsibility to CiviCRM core & add testing so that future changes are predictable
  2. We are being good open source citizens
  3. We are not fighting the flow.

4.6 Option 2 Tell CiviCRM to stick it

This would basically mean creating a new UI button to create refunds and ignoring the financial transaction records forever more.

Downsides

  1. If we go down this path we are kind of committed to stay on it because we would be letting our financial transaction data get dirtier & dirtier. Currently both the pre-upgrade data & the data since the upgrade need cleaning up but if we are going to do that we should do it now.
  2. We would be fighting the flow & in future might hit issues / have to manage things over upgrades.

Upsides

  1. Less change
  2. Easier data access. If we use the 'old' approach the contributions have a 1-1 with payments which makes it a little more intuitive from contribution search etc. However, I have already gotten the gateway report to be accurate on the trxn data (https://gerrit.wikimedia.org/r/#/c/260320/)

Indexing issues

These would need to be fixed in our DB & upstream and 'handled' when the next upgrade happens by editing the upgrade script. Starting in 4.7 CiviCRM offers more granular upgrades & pre-running upgrade actions may not require this.

  1. civicrm_entity_financial_trxn needs a combined index on entity_id + entity_table.
  2. civicm_financial_trxn needs an index on trxn_id - since no-one has done one yet we could make it non-unique & smatter it with comments about that! ( https://issues.civicrm.org/jira/browse/CRM-17752)

Data Cleanup
Here is how a contribution + financial transactions 'should' look after being refunded

When the upgrade script ran it converted completed, pending & cancelled transactions to have the financial_trxns populated - but it didn't look at the status of 'Refunded' so our upgraded ones look like

My proposal with the upgraded ones is that we add the missing financial_trxns but leave the contributions untouched.

I believe there are 16k~ of these

MariaDB [(none)]> SELECT count(*) FROM civicrm.civicrm_contribution WHERE contribution_status_id = 9;
+----------+
| count(*) |
+----------+
|    16382 |

New ones since the upgrade

When we upgraded CiviCRM added an additional upgrade status of 'Refunded' not 'realising' WMF already had one. The WMF code has been picking up this version rather than the earlier one. Some combination of the the WMF code + the double status + ?? maybe a bug?? means the ones that have been created since the upgrade have an extra positive financial_trxn.

There are 1186 contributions currently affected

I propose to

  1. remove the extra financial trxns for these rows
  2. update the contribution_status_id on civicrm-contribution & civicrm-financial_trxn

eg

``
SET @originalRefundStatus = 9;
SET @duplicateRefundStatus = 10;

DELETE FROM civicrm.civicrm_financial_trxn WHERE status_id = @duplicateRefundStatus;

UPDATE civicrm_contribution SET contribution_status_id = @originalRefundStatus WHERE contribution_status_id = @duplicateRefundStatus;

``

  1. Remove the duplicate Refunded status (per https://gerrit.wikimedia.org/r/#/c/254364/)

Note that this will leave us with a contribution status that is Completed & one that is Refunded. The transaction records will be correct as at the $ totals. We could merge them - this would also apply to the historical ones. I'm on the fence about this but it could be a subsequent step.

Importing updates

On looking at Donor Services s/sheet I feel like CiviCRM should be providing that information in a usable way & we should probably work towards that. Some thoughts

  1. We could add gateway transaction id to quick search (the top left hand corner search box) or similar & bypass the find contributions screen.
  2. I think it makes sense to add back the Refund button rather than have to use Edit Contribution - but to do that in an way that we also upstream at the same time.
  3. It looks like there is an import csv file function - is that accessible to Michael.
  4. I feel like maybe a report like that spreadsheet makes sense
  5. I wonder it's it worth adding a little angular form with a box to search the transaction & then a button to mark refund if Donor Services do these in some sort of bulk

Related Objects

StatusAssignedTask
OpenNone
Resolvedmepps
ResolvedNone
ResolvedNone
ResolvedEileenmcnaughton
ResolvedEileenmcnaughton
ResolvedEileenmcnaughton
ResolvedEjegg
ResolvedEileenmcnaughton
ResolvedEileenmcnaughton
ResolvedEileenmcnaughton
ResolvedEileenmcnaughton
ResolvedEileenmcnaughton
InvalidEileenmcnaughton
ResolvedEileenmcnaughton
ResolvedEileenmcnaughton
ResolvedEileenmcnaughton
DuplicateEileenmcnaughton
InvalidEileenmcnaughton
ResolvedJgreen

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I logged an issue for at least one case of the slowness - https://issues.civicrm.org/jira/browse/CRM-17628

I'm experiencing slowness on staging that I suspect is because the dupe rules from live may not be on staging

@Eileenmcnaughton
I just talked to @MBeat33 and he says, it's perfectly livable for Donor Services to use this spreadsheet throughout Big English.

We should still figure out how we want this to work going forward. And we'll want to help import a big batch of refunds and cancellations in January, so DS doesn't have to do that manually.

regarding the importing - are there ids in the s/sheet for matching on?

Yes, the GC transaction ID is listed for each one.

So updates on this. There are a few issues from my point of view

  1. The way in which CiviCRM records financial data has changed. Data for payments is held in the civicrm_financial_trxn table & civicrm_entity_financial_trxn table. This data is hard to see in the UI so I have had a go at exposing it and on staging there is now an arrow to expose the payment data from the financial tab (https://issues.civicrm.org/jira/browse/CRM-17620).
  1. Data was added / upgraded to the new way for existing transactions during the upgrade. Looking on staging gives some visibility into how what data is there but at the moment we can only see 4.2-upgraded data and not the 4.6-new-import data. I feel like we need to be able to see both & hence I logged a phab to get staging data updated (https://phabricator.wikimedia.org/T119748)
  1. There is a slow query when actioning a refund (https://issues.civicrm.org/jira/browse/CRM-17640). The slow query is coming out of an api call & can be fixed at the api level - which might have flow-on improvements elsewhere.
  1. If you access the edit screen from a contribution search this can lead to you being returned to an unfiltered contribution search (https://issues.civicrm.org/jira/browse/CRM-17628 ) - which is an even slower query.
  1. The payment row is not currently picking up the correct date for the refund line item when you issue a refund (https://issues.civicrm.org/jira/browse/CRM-17627)
  1. If we do use CiviCRM-standard refund flow then we need to fix the issue of not being able over & under refund. No issue logged as yet.
  1. Probably the big meta-issue is what ways data is extracted and what impact will using the civi-way have on that. How do we identify the places where it might be an issue?

NB I'm having a bit of a look into #3 at the moment as it seems like a valuable fix regardless

Change 256168 had a related patch set uploaded (by Eileen):
CRM-17620 expose payment data for contributions in the UI

https://gerrit.wikimedia.org/r/256168

Change 256241 had a related patch set uploaded (by Eileen):
CRM-17640 Update contribution api to do a sensible query when getcount is being invoked

https://gerrit.wikimedia.org/r/256241

Change 256168 merged by jenkins-bot:
CRM-17620 expose payment data for contributions in the UI

https://gerrit.wikimedia.org/r/256168

One of the issues raised by @awight is that the amount refunded is not always an exact match for the amount donated. Is the only reason for this currency discrepancies? Are these always full refunds or are some of them along the lines of "I gave you $100 but give me $50 back please" @MBeat33

We never issue partial refunds @Eileenmcnaughton - I'm not sure why the amount discrepancies might be coming in.

@Eileenmcnaughton is that amount the one we're getting from the processor?

The discrepancy theory came from this comment

This isn't quite compatible with how we've been tracking refunds until now. See wmf_civicrm_mark_refund, we've been marking the old transaction "Refunded", but then we make a second contribution with a negative amount, note that this is not necessarily equal to 0 - ORIG_AMT. I'd like to migrate us to the new style, whatever that is, but it will involve migrating our old schema, and fixing some reports.

& another a bit lower down

Yes, partial or even incorrect refunds happen and we want to know the truth.
I think financial line items would be nice in principle, but is there a UI yet to show the separate items?
Eventually, we should even have a third line item for the processor's chargeback or refund fee.

I think I was wrong about the partial refunds. This query comes up empty:

select *
from civicrm_contribution r
join wmf_contribution_extra e
        on r.id=e.entity_id
join civicrm_contribution c
        on e.parent_contribution_id = c.id
where
        c.receive_date > '2015-01-01'
        and c.total_amount + r.total_amount > 0.01

I identified 7 issues above. Of those there are fixes pending review and / or merge in the queue for 1, 3 & 5. I have determined that 4 is not an issue in normal work (ie. it only occurs when you specifically open the link in a new tab). I have a patch in a PR for upstream & can backport that after feedback (https://issues.civicrm.org/jira/browse/CRM-17628).

item 2 is a QA issue pending Jeff having time.

Item 6 is now a non-issue.

So we are left with item 7 - ie. what implications might a change have in terms of reporting

I discussed this with @awight on IRC - the main outstanding thing here is to check how the gateway reconcilliation report looks. I'll take a look at the code for it but might need someone (@MBeat33 ?) to show me how that workflow looks

MBeat33 added a subscriber: Ppena.Dec 8 2015, 12:12 AM

@eileenmacnaughton, I think @Ppena is the one for the lowdown on gateway reconciliation reports

Eileenmcnaughton added a comment.EditedDec 21 2015, 3:10 AM

I have updated the gateway report to look at transactions rather than contributions and I think the report is ok in itself but refund data needs fixing before it can be merged.

The report is currently on staging are reports the same figures as the old one expect WRT the items I am covering off in the data tidy up section. The report is faster than the one on live due to changes around joins.

https://gerrit.wikimedia.org/r/#/c/260320/

In order to start entering refunds through the UI we need also to merge these 2:

https://gerrit.wikimedia.org/r/#/c/257271/ (data entry date fix, merged upstream)
https://gerrit.wikimedia.org/r/#/c/256241/ (fixes the slowness of doing a refund, merged upstream).

There is some data tidy up though :-(. I was a bit too relaxed about getting rid of the second contribution status = refunded type. When I first did this https://gerrit.wikimedia.org/r/#/c/254364/ there were no contributions with a status of 10 - ie. the second 'Refunded' status which is the one I intend to delete. However, now there are 1173 there. They all appear to have come in via script so I need to fix the script and fix the contributions.

Here is an example one - under the new system there would be only 1 contribution, the refunded row would be a negative value & have the refund trxn_id. The status id would be 9 not 10

Eileenmcnaughton renamed this task from CiviCRM upgrade: Donation Processing: no refund button after clicking Edit transaction to CiviCRM upgrade: Adapt refund processing & reporting to reflect changes since the upgrade..Dec 22 2015, 1:27 AM
Eileenmcnaughton updated the task description. (Show Details)
Eileenmcnaughton edited a custom field.
Ejegg added a subscriber: Ejegg.Dec 23 2015, 8:31 PM

One more thing: Some processors give refunds their own gateway transaction id, and it would be nice to preserve this. Would we store that in civicrm_financial_trxn.trxn_id ?

We are currently storing them in a custom field aren't we? I think it makes sense to store them in the civicrm_financial_trxn.trxn_id field - might need to make sure they can be accessed via search - but that's completely do-able

Ejegg added a comment.Dec 23 2015, 8:41 PM

Yep, that's currently in wmf_donation_extra, so we only get one gateway trxn id per donation. It would be nice to migrate some of those fields to the standard fields (also gateway -> payment processor) if possible.

I have opened this JIRA issue to track

https://issues.civicrm.org/jira/browse/CRM-17751 setting the financial_trxn_id correctly

and this one

https://issues.civicrm.org/jira/browse/CRM-17752 for adding an index

I did some tests on refunding transactions via the api & aside from the trxn_id not being set on the refund record (first issue above) I was happy with the rows created by

civicrm_api3('Contribution', 'create', array(

'id' => x,
'contribution_status_id' => 'Refunded',
'trxn_id' => 'yhn',
'finance_only' => 1,
 'no_thank_you' => 'refund',

);

Change 261599 had a related patch set uploaded (by Eileen):
Backport FinancialTrxn api

https://gerrit.wikimedia.org/r/261599

Change 261759 had a related patch set uploaded (by Eileen):
Backport EntityFinancialTrxn api

https://gerrit.wikimedia.org/r/261759

Eileenmcnaughton added a comment.EditedJan 4 2016, 9:15 PM

I'm getting into some clashes between the various patches & order & stuff.

I think these 4 should be merged soonish & have no risk / broader impact:

  1. https://gerrit.wikimedia.org/r/#/c/257271/4
  1. https://gerrit.wikimedia.org/r/#/c/261599/
    • this is a simple api backport from upstream
  1. https://gerrit.wikimedia.org/r/#/c/261600/2
  1. https://gerrit.wikimedia.org/r/#/c/261759/
    • Another api backport

Note there is a related issue on refund total https://phabricator.wikimedia.org/T89437 - this makes me feel like we can focus on accepting the status quo of 'only same amount refunds' for this ticket and potentially schedule that one soon

Change 263313 had a related patch set uploaded (by Eileen):
(merged into 4.7) CRM-17627 Set transaction date correctly for refunds

https://gerrit.wikimedia.org/r/263313

Change 263314 had a related patch set uploaded (by Eileen):
(merged into 4.7) CRM-17751 add api support for setting the trxn_id for a refund

https://gerrit.wikimedia.org/r/263314

Change 261599 merged by Ejegg:
(merged into 4.6.11) Backport FinancialTrxn api

https://gerrit.wikimedia.org/r/261599

Change 261759 merged by Ejegg:
(merged into 4.6.11) Backport EntityFinancialTrxn api

https://gerrit.wikimedia.org/r/261759

Change 256241 merged by Ejegg:
(patch already in 4.7) CRM-17640 Update contribution api to do a sensible query when getcount is being invoked

https://gerrit.wikimedia.org/r/256241

Change 263313 merged by jenkins-bot:
(merged into 4.7) CRM-17627 Set transaction date correctly for refunds

https://gerrit.wikimedia.org/r/263313

Change 263314 merged by Ejegg:
(merged into 4.7) CRM-17751 add api support for setting the trxn_id for a refund

https://gerrit.wikimedia.org/r/263314

Change 254364 abandoned by Eileen:
Add upgrade hook to remove duplicate 'Refunded' contribution status

Reason:
Superceded by https://gerrit.wikimedia.org/r/#/c/254364/

https://gerrit.wikimedia.org/r/254364

I'm moving this whopper into Review to get some eyes on. The goal at this point is to get somewhere stable so we are happy with how new refunds are coming in & being done.
The upgrades have been run on staging and all fixes are on staging.

I think we are going to need to have some talk time on this!

The dependency chain starts with https://gerrit.wikimedia.org/r/#/c/266467/ (which is possibly one of the few that can be agreed without really working through the full picture.

The mark refund patch is failing CI at the moment and the resolution needs to be discussed as the tests relate to a change we need to adapt for - ie. with only one contribution we can't have 2 * type & 2* status (https://gerrit.wikimedia.org/r/#/c/261757/)

04:30:28 1) RefundTest::testMarkRefund
04:30:28 This has failed on an unresolved issue - what should the financial type be?
04:30:28 Failed asserting that two strings are equal.
04:30:28 --- Expected
04:30:28 +++ Actual
04:30:28 @@ @@
04:30:28 -'Refund'
04:30:28 +'Cash'
04:30:28
04:30:28 /mnt/jenkins-workspace/workspace/wikimedia-fundraising-civicrm/src/wikimedia/fundraising/crm/sites/all/modules/wmf_civicrm/tests/phpunit/RefundTest.php:85
04:30:28
04:30:28 2) RefundTest::testMarkRefundWithType
04:30:28 Refund contribution has correct type
04:30:28 Failed asserting that two strings are equal.
04:30:28 --- Expected
04:30:28 +++ Actual
04:30:28 @@ @@
04:30:28 -'Chargeback'
04:30:28 +'Cash'

Steps from here:

  1. Agree how mark refund should be resolved
  2. Get high level agreement on approach & patches
  3. Try entering some of Michael's refunds on staging.
  4. Look at how they could be imported
  5. refresh staging from live & re-run the upgrade to make sure all my dependency chaining etc didn't break anything
  6. Do some results QA on the gateway report
awight renamed this task from CiviCRM upgrade: Adapt refund processing & reporting to reflect changes since the upgrade. to [Epic] CiviCRM upgrade: Adapt refund processing & reporting to reflect changes since the upgrade..Jan 27 2016, 9:59 PM
DStrine closed this task as Resolved.Mar 2 2016, 11:33 PM

Change 285795 had a related patch set uploaded (by Eileen):
CRM-17620 expose payment data for contributions in the UI

https://gerrit.wikimedia.org/r/285795

Change 285806 had a related patch set uploaded (by Eileen):
(merged into 4.6.11) Backport FinancialTrxn api

https://gerrit.wikimedia.org/r/285806

Change 285807 had a related patch set uploaded (by Eileen):
(merged into 4.6.11) Backport EntityFinancialTrxn api

https://gerrit.wikimedia.org/r/285807

Change 285813 had a related patch set uploaded (by Eileen):
(patch already in 4.7) CRM-17640 Update contribution api to do a sensible query when getcount is being invoked

https://gerrit.wikimedia.org/r/285813

Change 285814 had a related patch set uploaded (by Eileen):
(merged into 4.7) CRM-17627 Set transaction date correctly for refunds

https://gerrit.wikimedia.org/r/285814

Change 285815 had a related patch set uploaded (by Eileen):
(merged into 4.7) CRM-17751 add api support for setting the trxn_id for a refund

https://gerrit.wikimedia.org/r/285815

Change 285795 merged by Eileen:
CRM-17620 expose payment data for contributions in the UI

https://gerrit.wikimedia.org/r/285795

Change 285806 merged by Eileen:
(merged into 4.6.11) Backport FinancialTrxn api

https://gerrit.wikimedia.org/r/285806

Change 285807 merged by Eileen:
(merged into 4.6.11) Backport EntityFinancialTrxn api

https://gerrit.wikimedia.org/r/285807

Change 285813 merged by Eileen:
(patch already in 4.7) CRM-17640 Update contribution api to do a sensible query when getcount is being invoked

https://gerrit.wikimedia.org/r/285813

Change 285814 merged by Eileen:
(merged into 4.7) CRM-17627 Set transaction date correctly for refunds

https://gerrit.wikimedia.org/r/285814

Change 285815 merged by Eileen:
(merged into 4.7) CRM-17751 add api support for setting the trxn_id for a refund

https://gerrit.wikimedia.org/r/285815

mmodell removed a subscriber: awight.Jun 22 2017, 9:52 PM