Add parameters to PaymentProcessor doRefund #15479
Conversation
|
(Standard links)
|
mattwire
force-pushed
the
paymentprocessor_dorefund_addparams
branch
4 times, most recently
from
1923d71 to
75d7206
Compare
| // amount: The amount to refund (eg. 12.05). This should always be specified in "dot" notation with no currency symbol. | ||
| return [ | ||
| // refund_status_id would normally return the contribution_status_id Completed or Failed | ||
| 'refund_status_id' => CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'), |
There was a problem hiding this comment.
My preference & the current assumption is that it will throw an exception on fail or no change implies success. Are there more than 2 possible outcomes?
There was a problem hiding this comment.
Using stripe as an example, yes. pending, succeeded, failed, or canceled. We may choose not to handle all of them in CiviCRM but at least pending, succeeded and failed seem like something we might handle. I've copied what we do in doPayment here as we want to record the outcome on the contribution whether it succeeded or not. So I don't think throwing an exception is correct because failed is a valid response. If the actual request to the payment processor failed (eg. invalid params etc) then yes an exception would be appropriate.
There was a problem hiding this comment.
OK - I've confirmed that we DO throw Exceptions for failed payments & catch them currently - e.g on Event Confirm.
I can see a case for differentiating between completed & 'no outcome' but I think we should continue to throw exceptions for all sorts of failures (we could introduce a separate exception type if we want?)
Also - I've been just hard coding '1' rather than referring to the contribution status as it is not a contribution status - it's a payment status. I guess the most correct would be a class constant - although I also like a 'word' describing the outcome.
There was a problem hiding this comment.
@eileenmcnaughton We can add class constants and switch doPayment to them too. I'm still not sure what is best in terms of throwing exceptions. To me, an exception generally means something went wrong. A failed payment (refund) means it was successful in that it communicated with the payment processor and returned a valid response. In the case of Stripe and latest changes for PSD2 (so all credit card payments in Europe) success / failure has become very grey because there are now a whole range of outcomes which may change depending on whether the user is still present to respond to authentication challenges or whether they can respond offline (eg. via email). So for example we could get a "failed" refund back from the processor which might become successful a minute or an hour later. However, if we'd been unable to communicate with the processor, or we'd not sent sufficient authentication / params to the processor I'd expect an exception to be raised and returned.
There was a problem hiding this comment.
@mattwire so generally there are 3 things that the calling code would do
- create a successful payment
- create a failed payment
- do nothing.
Currently the calling code actually never does 2 - so the difference between 2 & 3 is basically immaterial.
I guess we DO have an ambition that the calling code would at some point start to create failed payments rather than do nothing. In which case we kind of need to have 3 possible outcomes.
Currently in doPayment these are represented by
- return payment_status_id = 1
- throw PaymentProcessorException
- no exception but payment_status_id !== 1
- the code generally doesn't create the failed payments for 2 - but I would argue it should as that is valuable info for someone dealing with a customer enquiry
There was a problem hiding this comment.
In the case of doPayment we already return (at least) two possible statuses - "Completed" or "Pending". Here is my proposed implementation for doRefund for stripe: https://lab.civicrm.org/extensions/stripe/blob/master/CRM/Core/Payment/Stripe.php#L705-750 - I'm throwing exceptions when something went wrong, returning statuses when the "processing" was successful (not necessarily the outcome).
Looking through various callers of doPayment it seems that there are a multitude of different ways it is called and subsequently handled. Some check if the payment status is completed, others just pass the params through regardless and others throw an exception if doPayment threw an exception.
In the case of Stripe a single payment may be successful immediately or it may be successful (or fail) at some point later on (which would be updated by the IPN callbacks). This makes the definition of success more and more blurred and I would prefer to see exceptions thrown in doPayment when something actually went wrong, rather than the action not being allowed at this time.
For example a refund might return "Card Expired" and therefore the status of the refund should be failed/cancelled. The refund didn't happen, but nothing went wrong - Civi successfully communicated with the payment processor and the payment processor gave a valid response. If instead the payment processor had responded with "Bad request" or "Internal Server Error" I'd expect an exception to be triggered within doPayment. Thoughts @eileenmcnaughton @JoeMurray ? It would be nice to get something merged and build on it.
There was a problem hiding this comment.
@mattwire given this is a significant change of approach from what we originally determined let's discuss & agree on gitlab first - I've opened https://lab.civicrm.org/dev/financial/issues/93
| 'refund_trxn_id' => NULL, | ||
| // Array of params returned by the payment processor. The contents of this will vary by processor and should not be relied on. | ||
| // However, they can be very useful for logging or providing specific feedback. | ||
| 'processor_result' => [], |
There was a problem hiding this comment.
OK that makes sense
| 'refund_status_id' => CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'), | ||
| // The refund trxn_id may be different from the trxn_id of the original payment. | ||
| // If it is the same then trxn_id should be copied to refund_trxn_id | ||
| 'refund_trxn_id' => NULL, |
There was a problem hiding this comment.
OK - that makes sense - you have changed it from altering params to having defined return vars here too - I'm OK with switching over
mattwire
force-pushed
the
paymentprocessor_dorefund_addparams
branch
2 times, most recently
from
0c2d4d0 to
f7cfa8b
Compare
mattwire
force-pushed
the
paymentprocessor_dorefund_addparams
branch
from
f7cfa8b to
2cbd504
Compare
mattwire
force-pushed
the
paymentprocessor_dorefund_addparams
branch
from
2cbd504 to
c3b4d86
Compare
| * @throws \Civi\Payment\Exception\PaymentProcessorException | ||
| */ | ||
| public function doRefund(&$params) {} | ||
| public function doRefund($params) { |
There was a problem hiding this comment.
I have created an issue for discussion / approval of this change https://lab.civicrm.org/dev/financial/issues/92
|
@mattwire changing how we expect processors to 'express' failure is not a trivial change - let's have the 'concept needs approval' concept first & get more real about pushing back to gitlab for tracking the conversation when we are agreeing changes - closing for now until we get more input https://lab.civicrm.org/dev/financial/issues/93 |
Overview
Builds on #15478
This adds a prototype set of return values for the doRefund function. It will probably need to evolve further but this works for Stripe and mirrors what we must return from doPayment. The code comments explain in detail what is expected.
Before
No parameters specified for doRefund.
After
Refund parameters that are sufficient to implement doRefund in Stripe specified.
Technical Details
This may need to evolve further but this seems like the minimum required set of parameters. And we should ensure that we define them / return params before we start using it!
Comments