Call Payment.create from payment.cancel #13689

eileenmcnaughton · 2019-02-25T04:02:02Z

Overview

Code rationalisation Update Payment.cancel api to call Payment.create.

This includes adding handling to Payment.create for negative amounts (this handling was previously 'distributed' across the code base.

Before

Bespoke function called

After

std api called

Technical Details

@monishdeb I feel like if we can get this working then we'll have solved the blockers on the form without the risk - + we'll have only one place where the function is called.

Comments

civibot · 2019-02-25T04:02:05Z

(Standard links)

If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

monishdeb · 2019-02-25T04:43:20Z

api/v3/Payment.php


-  $trxn = CRM_Contribute_BAO_Contribution::recordAdditionalPayment($contributionId, $params, 'refund', NULL, FALSE);


@eileenmcnaughton I think this API should be called for cancelled contribution, isn't it? Because the refund handling here isn't quite right.

@monishdeb right - this PR can't be merged until we've resolved the refund handling - which we could add to this PR - I think just extracting makes sense - shall I add that so you can see what I mean?

Sure why not :)

monishdeb · 2019-02-26T13:08:08Z

Jenkins test this please

eileenmcnaughton · 2019-03-08T01:04:36Z

CRM/Financial/BAO/Payment.php

@@ -99,6 +99,9 @@ public static function create($params) {
        CRM_Contribute_BAO_Contribution::assignProportionalLineItems($params, $trxn->id, $contribution['total_amount']);
      }
    }
+    elseif ($params['total_amount'] < 0) {


@monishdeb I've updated this PR with this call to record refunds from here - hopefully that means payment_cancel can call Payment.create now & we are closer to being rid of recordAdditionalPaymen

@eileenmcnaughton Yes, that would be fine.

Also, let me share my thoughts about what I was supposed to do to support refund using payment processor. I am thinking of having a new api payment.refund which will do refund using payment processor and eventually will call Payment.cancel API to record financial records - https://github.com/civicrm/civicrm-core/compare/master...JMAConsulting:financial-38?expand=1#diff-8967f6e57d6209aa277ded6512413608R510

And then for refund w/o PP (i.e. Manual PP), only call to payment.cancel will do.

@monishdeb so I'm not sold on having one api do both the payment processor refund AND process the refund payment. I think we could sink into various edge cases so I lean towards having

PaymentProcessor.pay (already in Omnipay) and PaymentProcessor.refund (which would call doRefund on the processor class) and then the calling code can call either Payment.create or Payment.cancel - I'm not sure which makes more sense but they will eventually be more or less the same code anyway

(I also think we can add PaymentProcessor.refund first & then as we start to use it we can consider if we have needs not met by the Payment apis & the PaymentProcessor.refund api rather than decide that up front - as the work for PaymentProcessor.refund will be required regardless).

My worry is that traditionally the financial functions have been overloaded with ... all sort of stuff - so if we can keep them simple & clean we might have less problems

monishdeb · 2019-03-08T05:18:45Z

api/v3/Payment.php

+    }
+  }
+  $result = civicrm_api3('Payment', 'create', $paymentParams);
+  return civicrm_api3_create_success($result['values'], $params, 'Payment', 'cancel', $trxn);


@eileenmcnaughton test failure is due to presence of unknown variable $trxn variable. Can you please remove it?

monishdeb · 2019-03-08T05:29:21Z

api/v3/Payment.php

+    ],
+    'trxn_date' => [
+      'title' => 'Cancel Date',
+      'type' => CRM_Utils_Type::T_DATE + CRM_Utils_Type::T_TIME,


Also, trxn_id is important for canceling a payment.

@monishdeb logically I agree with you - the current api is not really doing 'Payment.cancel' it's doing 'cancel one payment associated with the contribution' - which really makes me question whether the api is sensible & worth having. That aside, I think this PR doesn't CHANGE the situation in that I think trxn_id is not used currently?

ohhh I think the only place Payment.cancel is currently called from is

function civicrm_api3_payment_create(&$params) { // Check if it is an update if (CRM_Utils_Array::value('id', $params)) { $amount = $params['total_amount']; civicrm_api3('Payment', 'cancel', $params); $params['total_amount'] = $amount; }

That is hurting my brain - I guess the idea is you can't alter a payment only reverse & recreate it?

Yes, my earlier point of not having that name as 'cancel' as we are not actually cancelling a existing payment but rather refunding the original payment. Reason why I was suggesting to use refund at it actually adds a new reverse transaction and doesn't affect the original payment. It keeps the clarity in bookkeeping report.

I agree PR doesn't change the workflow, I was suggesting to add more info. to this API so a dev. will be aware of what sort of parameter needed to refund a payment. Also Cancel Date is not appropriate it has to be termed as 'Refund Date' as we are actually refunding the payment. Like you earlier said Cancelling a contribution is something need to dealt at Form level ?

@monishdeb So I'm actually suggesting it's even simpler - ie it doesn't call

civicrm_api3('Payment', 'cancel', $result);

The calling code would call

civicrm_api3('PaymentProcessor', 'refund', $params); civicrm_api3('Payment', 'create', $result);

Whether we want an api that does those 2 lines depends a bit on how it looks as we get further down the track - but we could add Payment.ProcessRefund if we decide later on we need to

e.g here is the PaymentProcessor.pay api from Omnipay

/** * Action payment. * * @param array $params * * @return array * API result array. * @throws CiviCRM_API3_Exception */ function civicrm_api3_payment_processor_pay($params) { $processor = Civi\Payment\System::singleton()->getById($params['payment_processor_id']); $processor->setPaymentProcessor(civicrm_api3('PaymentProcessor', 'getsingle', array('id' => $params['payment_processor_id']))); $result = $processor->doPayment($params); if (is_a($result, 'CRM_Core_Error')) { throw API_Exception('Payment failed'); } return civicrm_api3_create_success(array($result), $params); }

Oops missed the fact that Payment.create does call the Payment.cancel after ensuring its refund payment. I agree with you :)

Yeah that's slightly terrifying - part of me is wondering if this PR will create a code loop that will crash the server.... But I guess it didn't before....

I feel like this is a BAD function but my goal in this PR is not to address that but just to get to the point where recordAdditionalPayment is only called from one place in the code....

Yes maybe I am seeing a bigger picture here to use Paymentprocessor to do partial/pending/refund payment all of which utilizes Payment.create API But that dream is after we simplify and strengthen the Payment.create BAO fn to handle all kind of additional payment and of course to get recordAdditionalPayment called from one place

Right & maybe once we've one that I'll feel more comfortable about a more complex api - all I can see at the moment is this spaghetti of code trying to do 25 things at once & hence I'm resistant to more multipurpose functions while this is in front of me

monishdeb · 2019-03-08T12:39:58Z

api/v3/Payment.php


-  $trxn = CRM_Contribute_BAO_Contribution::recordAdditionalPayment($contributionId, $params, 'refund', NULL, FALSE);
+  $paymentParams = [
+    'total_amount' => -$entity['amount'],


@eileenmcnaughton I think we shouldn't negate here? Test failure complains about the same

@monishdeb you are right - but I think we should be setting to negative 'up high' and handling that lower down so I have removed the expectation of a positive number in

https://github.com/civicrm/civicrm-core/pull/13689/files#diff-8967f6e57d6209aa277ded6512413608R316

and fixed the one other place that calls that to also pass in a negative number

https://github.com/civicrm/civicrm-core/pull/13689/files#diff-7a5b0e2d131dabc49178460fc63328c0R3882

There is still a test fail on status which I'm making sense of

eileenmcnaughton · 2019-03-14T23:50:06Z

@monishdeb I added a second commit because this has surfaced a behaviour difference between the Payment.create api handling of refunds (pre-existing) and the form handling and I believe the form handling to be wrong.

Basically the form was setting the status of the refund PAYMENT (not the contribution) to be 'refunded' whereas the api was setting the status to being 'completed'

I would argue that payments should only have the status of 'complete' or 'they just don't exist' - however I expect it makes sense to have a status for payments that are recorded & then turn out t be recorded in error (ie. payments that have been voided)

Regardless I think the status of a refund payment that has completed should be completed not refunded & the api is right & the form is wrong

Let's see where tests get to & then try to agree on that.

FWIW I think this reconciliation of the functionality in the 2 paths is pretty important & not just a side-issue

eileenmcnaughton · 2019-03-15T21:40:32Z

Test fail specifically relates to the discrepancy between the 2 processes.

@JoeMurray @pradpnayak can you check comments about about the difference between form & api handling of the status of negative (refund) payments made against a contribution

eileenmcnaughton · 2019-03-21T23:07:27Z

@JoeMurray ping - see comments in #13689 (comment) about discrepancy on financial_trxn.status_id between api & form

eileenmcnaughton · 2019-03-28T22:40:41Z

@JoeMurray ping - need input from you on this one - I can explain more if need be but see #13689 (comment)

pradpnayak · 2019-03-29T00:20:52Z

Hi @eileenmcnaughton

I think it should be Refunded(which means amount is refunded to user inshort completed). The use case i can think of is when you refund over-payment for example a user paid $1000 instead $100. Staff member will initiate refund of $900. So the contribution status will be Completed with total amount = $100, there will be -$900 entry in financial trxn table with status as Refunded. This will help accounting person to know $900 was actually refunded since there can be many reason of amount being negative.

If we want to use status as Completed than i think we should implement Refunds entries slightly differently like some what from Data flow doc https://wiki.civicrm.org/confluence/display/CRM/CiviAccounts+Data+Flow#CiviAccountsDataFlow-Extension:PendingRefund(PreparetorefundacompletedcontributionindirectlyviaAccountsPayable)

But I think @JoeMurray would have best answer for you than me(and i will agree on him;) ).

Cheers

Pradeep

eileenmcnaughton · 2019-03-29T00:26:32Z

@pradpnayak in other words Refunded means Completed to all accounts & purposes.

I guess it makes sense if we don't think of status as meaning status - which makes more sense than it perhaps sounds like since the real status of a financial_trxn is a BOOLEAN - ie. trxn happened or didn't (although I could make a case for VOID).

I think it's good to realise this weird 'status as a description' is a 'thing' because I think we've seen a few related bugs

eileenmcnaughton · 2019-03-29T22:44:38Z

Well I guess that is enough of a case to justify changing the Payment.create action to set the financial trxn status to Refunded when creating a negative payment - which seems like if I made that change we could get this merged @monishdeb - since finishing this payment.create cleanup seems to be in the path of various other pieces of work at the moment

JoeMurray · 2019-04-06T23:38:16Z

@monishdeb could you look at code on this? I will look as well on Monday at related comments to try to help unblock.

monishdeb · 2019-04-25T11:24:21Z

Tested working fine. Happy with the final patch also the test build passed. Merging now.

eileenmcnaughton · 2019-04-25T22:23:25Z

@monishdeb yay - I'll take a look & see what the next step is

civibot bot added the master label Feb 25, 2019

eileenmcnaughton mentioned this pull request Feb 25, 2019

Modify backend Record Payment forms to use payment create API #13647

Closed

eileenmcnaughton force-pushed the no_record_payment branch 2 times, most recently from e4bf596 to 1c61226 Compare February 25, 2019 04:14

monishdeb reviewed Feb 25, 2019

View reviewed changes

eileenmcnaughton force-pushed the no_record_payment branch from 1c61226 to e16a7d1 Compare February 25, 2019 06:38

magnolia61 mentioned this pull request Mar 6, 2019

dev/financial#20 - Extend pay now functionality for partially paid contribut… #12319

Closed

eileenmcnaughton force-pushed the no_record_payment branch from e16a7d1 to 2002ff9 Compare March 8, 2019 01:03

eileenmcnaughton commented Mar 8, 2019

View reviewed changes

monishdeb reviewed Mar 8, 2019

View reviewed changes

eileenmcnaughton force-pushed the no_record_payment branch 2 times, most recently from 0523df6 to d529efb Compare March 8, 2019 08:55

monishdeb reviewed Mar 8, 2019

View reviewed changes

eileenmcnaughton force-pushed the no_record_payment branch 3 times, most recently from 5393a8c to 783f38b Compare March 14, 2019 23:48

eileenmcnaughton force-pushed the no_record_payment branch from 783f38b to 27dfca7 Compare March 29, 2019 22:37

eileenmcnaughton force-pushed the no_record_payment branch from 27dfca7 to 8f55841 Compare March 29, 2019 22:40

eileenmcnaughton force-pushed the no_record_payment branch 2 times, most recently from 09289a0 to 632803d Compare April 1, 2019 04:28

Call Payment.create from payment.cancel

2561fc1

eileenmcnaughton force-pushed the no_record_payment branch from 632803d to 2561fc1 Compare April 1, 2019 06:14

This was referenced Apr 3, 2019

dev/financial#25 support partial payments params for event reg fronte… #12371

Closed

dev/financial#34 : Incorrect allocation of payment on an edited multi-line item event registration #13114

Closed

eileenmcnaughton mentioned this pull request Apr 24, 2019

dev/core#889 - Refund throws a fatal error if the main contribution a… #14103

Merged

monishdeb merged commit 144dacf into civicrm:master Apr 25, 2019

eileenmcnaughton deleted the no_record_payment branch April 25, 2019 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call Payment.create from payment.cancel #13689

Call Payment.create from payment.cancel #13689

eileenmcnaughton commented Feb 25, 2019 •

edited

Loading

civibot bot commented Feb 25, 2019

monishdeb Feb 25, 2019

eileenmcnaughton Feb 25, 2019

monishdeb Feb 25, 2019

monishdeb commented Feb 26, 2019

eileenmcnaughton Mar 8, 2019

monishdeb Mar 8, 2019

monishdeb Mar 8, 2019 •

edited

Loading

eileenmcnaughton Mar 8, 2019

eileenmcnaughton Mar 8, 2019 •

edited

Loading

monishdeb Mar 8, 2019

eileenmcnaughton Mar 8, 2019

monishdeb Mar 8, 2019

eileenmcnaughton Mar 8, 2019

eileenmcnaughton Mar 8, 2019

monishdeb Mar 8, 2019

monishdeb Mar 8, 2019

eileenmcnaughton Mar 8, 2019 •

edited

Loading

monishdeb Mar 8, 2019

eileenmcnaughton Mar 8, 2019

monishdeb Mar 8, 2019 •

edited

Loading

eileenmcnaughton Mar 8, 2019

monishdeb Mar 8, 2019

eileenmcnaughton Mar 14, 2019

eileenmcnaughton commented Mar 14, 2019 •

edited

Loading

eileenmcnaughton commented Mar 15, 2019

eileenmcnaughton commented Mar 21, 2019

eileenmcnaughton commented Mar 28, 2019

pradpnayak commented Mar 29, 2019

eileenmcnaughton commented Mar 29, 2019

eileenmcnaughton commented Mar 29, 2019

JoeMurray commented Apr 6, 2019

monishdeb commented Apr 25, 2019

eileenmcnaughton commented Apr 25, 2019


		$trxn = CRM_Contribute_BAO_Contribution::recordAdditionalPayment($contributionId, $params, 'refund', NULL, FALSE);

Call Payment.create from payment.cancel #13689

Call Payment.create from payment.cancel #13689

Conversation

eileenmcnaughton commented Feb 25, 2019 • edited Loading

Overview

Before

After

Technical Details

Comments

civibot bot commented Feb 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monishdeb Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eileenmcnaughton commented Mar 14, 2019 • edited Loading

eileenmcnaughton commented Mar 15, 2019

eileenmcnaughton commented Mar 21, 2019

eileenmcnaughton commented Mar 28, 2019

pradpnayak commented Mar 29, 2019

eileenmcnaughton commented Mar 29, 2019

eileenmcnaughton commented Mar 29, 2019

JoeMurray commented Apr 6, 2019

monishdeb commented Apr 25, 2019

eileenmcnaughton commented Apr 25, 2019

eileenmcnaughton commented Feb 25, 2019 •

edited

Loading

monishdeb Mar 8, 2019 •

edited

Loading

eileenmcnaughton Mar 8, 2019 •

edited

Loading

eileenmcnaughton Mar 8, 2019 •

edited

Loading

monishdeb Mar 8, 2019 •

edited

Loading

eileenmcnaughton commented Mar 14, 2019 •

edited

Loading