Financial issueshttps://lab.civicrm.org/dev/financial/-/issues2020-12-09T00:33:53Zhttps://lab.civicrm.org/dev/financial/-/issues/153Orange Paypal Pro button not redirecting properly after reCaptcha on Paypal2020-12-09T00:33:53ZStoobOrange Paypal Pro button not redirecting properly after reCaptcha on Paypal#151 related issue @eileen @seamuslee
The javascript errors are fixed but we have been able to reproduce other errors related to the reCaptcha system at Paypal.com. I am not sure if these can be fixed on CiviCRM's side or not but we sh...#151 related issue @eileen @seamuslee
The javascript errors are fixed but we have been able to reproduce other errors related to the reCaptcha system at Paypal.com. I am not sure if these can be fixed on CiviCRM's side or not but we should be aware of the following experience from user testing.
My guess is the from the Civi side we should confirm we are sending Paypal the proper auto-return URL to ensure that Paypal can find its way back to us after hitting us with the reCaptcha on their end.
In *Chrome and Safari* the Javascript errors are fixed -- when you click the Paypal button, there's no validation error requiring the user to input CC fields.
**But** when I go to Paypal, I'm asked to complete a ReCaptcha at this URL: https://www.paypal.com//cgi-bin/webscr?cmd=_express-checkout&token=EC-2JE64037K02097038. Once I complete the challenge, the page does not reload to the log-in screen. Instead it just sits with the challenge results accepted. If I then reload the page, it redirects me to the Paypal log-in page as it should at this URL: https://www.paypal.com//cgi-bin/webscr?cmd=_express-checkout&token=EC-2JE64037K02097038
In *Firefox*, the Javascript errors are fixed -- when I click the Paypal button, there's no validation error requiring the user to input CC fields. When you go to Paypal, I'm asked to complete a ReCaptcha. Once I complete the challenge, the Paypal experience is different page loads the sad dinosaur error screen "hmm we're having trouble finding that site"https://lab.civicrm.org/dev/financial/-/issues/146alterPaymentProcessorParams hook and propertyBag2020-09-15T15:35:00Zmattwiremjw@mjwconsult.co.ukalterPaymentProcessorParams hook and propertyBagSee https://github.com/civicrm/civicrm-core/pull/17507#issuecomment-690213686:
> 1. pass property bag to alterPaymentProcessorParams
>
> - ✔ the hook gets to deal with sensible, known data and does not have to (re-)implement all t...See https://github.com/civicrm/civicrm-core/pull/17507#issuecomment-690213686:
> 1. pass property bag to alterPaymentProcessorParams
>
> - ✔ the hook gets to deal with sensible, known data and does not have to (re-)implement all the quirks of checking different array keys for one thing.
> - ✔ any setting the hook does in a prop bag ensures forward consistency for the next process in the chain (another hook or the main process)
> - ✖ It's possible that the reason for a hook is something pretty darn weird needs doing; in which case the raw array may be more useful.
>
> 2. pass the original array to the hook
>
> - ✖ hook has to do all work arounds
> - ✖ hook is free to implement bad practise in altering the array
> - ✔ hook can do whatever it likes.
>
> I think (1).
And example from Stripe where we're currently passing an array for compatibility (but this should not be used as a model as Stripe ignores the return values from the hook anyway).
https://lab.civicrm.org/extensions/stripe/-/blob/master/CRM/Core/Payment/Stripe.php#L484-489
// @fixme DO NOT SET ANYTHING ON $propertyBag or $params BELOW THIS LINE (we are reading from both)
$params = $this->getPropertyBagAsArray($propertyBag);
// We don't actually use this hook with Stripe, but useful to trigger so listeners can see raw params
$newParams = [];
CRM_Utils_Hook::alterPaymentProcessorParams($this, $params, $newParams);https://lab.civicrm.org/dev/financial/-/issues/145Adding "standard" params to propertyBag2020-09-16T00:42:12Zmattwiremjw@mjwconsult.co.ukAdding "standard" params to propertyBagThis came out of https://github.com/civicrm/civicrm-core/pull/18425 and https://github.com/civicrm/civicrm-core/pull/17595.
Specifically, adding "standard" parameters to propertyBag when they are already in use by a payment processor th...This came out of https://github.com/civicrm/civicrm-core/pull/18425 and https://github.com/civicrm/civicrm-core/pull/17595.
Specifically, adding "standard" parameters to propertyBag when they are already in use by a payment processor that has been converted to use propertyBag is likely to break existing implementations.
Accessing params that are later added as "standard" params to propertyBag when a payment processor is already converted internally to propertyBag (eg. most of the ones written by me Stripe, authnetecheck, Smartdebit).
Example Issues:
- authnetecheck accesses `$params['credit_card_number']` which disappears once mapped to a propertyBag because only the standardised `cardNumber` field is available.
- If authnetecheck was using `$propertyBag->getCustomProperty('credit_card_number')` it would throw an "InvalidArgumentException" because you are not allowed to use `getCustomProperty` for a "standard" property.
Similar issues will apply to any other properties that are already in use by a payment processor.
@eileen @artfulrobot @seamuslee @tottenhttps://lab.civicrm.org/dev/financial/-/issues/143Convert core processors to use Guzzle and bring them under CI2020-09-05T01:27:07ZeileenConvert core processors to use Guzzle and bring them under CISubtask of https://lab.civicrm.org/dev/financial/-/issues/132Subtask of https://lab.civicrm.org/dev/financial/-/issues/132https://lab.civicrm.org/dev/financial/-/issues/142Order api proposal - require less line item parameters2020-12-03T10:37:27ZeileenOrder api proposal - require less line item parametersI'd like to propose that we add sensible defaults for line items when using the order api
1) price_field_id - if NONE of the rows have a price_field_id then use the default pricefield id for all of them (I don't want to open up technica...I'd like to propose that we add sensible defaults for line items when using the order api
1) price_field_id - if NONE of the rows have a price_field_id then use the default pricefield id for all of them (I don't want to open up technical analysis of dealing with multiple price sets, more complex config) - so this is just for that common use case
2) qty - if not set, default to 1
This would make the minimum required params for a 'quick config' (I hate that usage but I think we know what it means) order
```
$this->callAPISuccess('Order', 'create', [
'financial_type_id' => 'Donation',
'contact_id' => $contact1,
'line_items' => [
['line_item' => [
[
'financial_type_id' => CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'financial_type_id', 'Donation'),
'line_total' => 40,
],
[
'financial_type_id' => CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'financial_type_id', 'Member Dues'),
'line_total' => 50,
],
]],
]
]);
```
Ideally we would also accept
```
'financial_type_id' =>'Donation',
```https://lab.civicrm.org/dev/financial/-/issues/138Broken bookkeeping records: no financial trxn from Accounts Receivable to rev...2020-06-29T04:05:27ZRichBroken bookkeeping records: no financial trxn from Accounts Receivable to revenue account.According to the [docs](https://docs.civicrm.org/dev/en/latest/financial/financialentities/), when a Pending Order is created it's supposed to create a financial trxn transferring the total sum to accounts receivable. I'm not seeing this...According to the [docs](https://docs.civicrm.org/dev/en/latest/financial/financialentities/), when a Pending Order is created it's supposed to create a financial trxn transferring the total sum to accounts receivable. I'm not seeing this happening.
@JoeMurray [pointed out](https://chat.civicrm.org/civicrm/pl/zs556c7xqjdtjciry8qnipw7rc) that *There's a setting in CiviContribute Compnent Settings now which iirc disables this by default.*
And @eileen suggested that the default should be the other way around.
But even with this turned ON, the trxn is not created.
```bash
# Set this to a contact that exists:
CID=202
cv api Order.create financial_type_id=Donation contact_id=$CID contribution_status_id=Pending \
total_amount=30 receive_date=2020-05-29\ 15:30:00
```
At this point we would expect (according to my understanding) to see:
- ✔ 1 contribution record.
- ✔ 1 line item: directly linked to the contribution.
- ✔ 1 financial item, linked to the line item, which specifies the CREDIT financial account (*Donation*) and status 3 (pending).
- ✖ 1 financial trxn:
- `from_account` (the CREDIT): The revenue (income) account for the *Donation* financial type.
- `to_account` (the DEBIT): Accounts Receiveable
- ✖ related entity financial trxn records
Without this we end up with bookkeeping problems:
- all financial trxns come *from* Accounts Recievable, but there's nothing going *to* Accounts Receivable.
- there are no bookkeeping transactions for the revenue accounts.
- So you get info like "some money went into your bank" but not "a donation went into your bank".JoeMurrayJoeMurrayhttps://lab.civicrm.org/dev/financial/-/issues/137Billing Address params on propertyBag2020-09-14T16:45:36Zmattwiremjw@mjwconsult.co.ukBilling Address params on propertyBagWe currently have a set of `billingXxx` fields on propertyBag but they have no mappings to what actually gets passed in.
Because of `CRM_Core_Form::prepareParamsForPaymentProcessor` we can be pretty certain that fields will be available ...We currently have a set of `billingXxx` fields on propertyBag but they have no mappings to what actually gets passed in.
Because of `CRM_Core_Form::prepareParamsForPaymentProcessor` we can be pretty certain that fields will be available in the format without the `billing_` prefix.
But the mapping is still not completely clear:
```
'billingStreetAddress' => TRUE,
'street_address' => 'billingStreetAddress',
'billingSupplementalAddress1' => TRUE,
'billingSupplementalAddress2' => TRUE,
'billingSupplementalAddress3' => TRUE,
'billingCity' => TRUE,
'city' => 'billingCity',
'billingPostalCode' => TRUE,
'postal_code' => 'billingPostalCode',
'billingCounty' => TRUE,
'state_province' => 'billingCounty',
'billingCountry' => TRUE,
'country' => 'billingCountry',
```
In particular:
1) "County" has various meanings and we should probably standardise on `stateProvince` with `County` being the next level down.
2) Should the params be prefixed with `billingXxx` at all in propertyBag? Or `addressXxx` or just `xxx`.
3) Do we need the supplementalAddress params at all - I don't think anything (payments) uses them?
Ping @eileen @artfulrobot @KarinGhttps://lab.civicrm.org/dev/financial/-/issues/136Feature / Bug Cancel all pending contributions linked to a recurring contribu...2020-06-20T00:50:40ZseamusleeFeature / Bug Cancel all pending contributions linked to a recurring contribution when it is cancelledhttps://lab.civicrm.org/dev/financial/-/issues/133Enhancements to Payment PropertyBag2021-04-12T10:46:20Zmattwiremjw@mjwconsult.co.ukEnhancements to Payment PropertyBagPayment PropertyBag was added in 5.20 but it is only when attempting conversion of payment code/processors that we find things that are missing or still need consideration. I'm adding some things here but would encourage @eileen @artful...Payment PropertyBag was added in 5.20 but it is only when attempting conversion of payment code/processors that we find things that are missing or still need consideration. I'm adding some things here but would encourage @eileen @artfulrobot and others to edit/update this description once we are in agreement.
1. The following parameters should be added:
* trxn_id -> Maps to `transactionID`.
* installments
* recur start/end dates?
2. To support a phased conversion to propertyBag we should allow exporting as an array: https://github.com/civicrm/civicrm-core/pull/17507 or similar. Alternatively we should document using reflection to access the propertyBag array eg.:
```
if ($propertyBag instanceof \Civi\Payment\PropertyBag) {
$reflectionClass = new ReflectionClass($propertyBag);
$reflectionProperty = $reflectionClass->getProperty('props');
$reflectionProperty->setAccessible(TRUE);
$params = $reflectionProperty->getValue($propertyBag)['default'];
}
else {
$params = $propertyBag;
}
```
3. Some parameters should be either be ignored or added to propertyBag in some form during `mergeLegacyInputParams` such as:
* qfKey
* entryURL
* qf_default..
Note that both `qfKey` and `entryURL` can be useful for guessing context though it would be nice if there was a better way to identify the context in which a payment processor was being used.
Eg. see https://lab.civicrm.org/extensions/stripe/-/blob/6.4.1/CRM/Core/Payment/Stripe.php#L1063 for an example of `qfKey` usage which shows that we don't really need `qfKey` but do need a more formal way of obtaining an errorURL or removing the need for it at all.https://lab.civicrm.org/dev/financial/-/issues/132Model best practices in our core processors2021-05-17T20:43:14ZeileenModel best practices in our core processorsSubissue of https://lab.civicrm.org/dev/financial/-/issues/130
~~**Subtasks for this issue**
Throw exceptions, do not return error object~~
https://lab.civicrm.org/dev/financial/-/issues/131
**Implement doPayment, do not implement doTr...Subissue of https://lab.civicrm.org/dev/financial/-/issues/130
~~**Subtasks for this issue**
Throw exceptions, do not return error object~~
https://lab.civicrm.org/dev/financial/-/issues/131
**Implement doPayment, do not implement doTransferPayment, doDirectPayment**
https://lab.civicrm.org/dev/financial/-/issues/135
- this is currently blocked on clarifying return parameters https://lab.civicrm.org/dev/financial/-/issues/141
**Cast $params to a PropertyBag at the start of the function**
- note this is currently blocked on PropertyBag support for the full range of properties - see https://lab.civicrm.org/dev/financial/-/issues/130#note_42912
**Use Guzzle, not Curl**
https://lab.civicrm.org/dev/financial/-/issues/143
**Have tests**
So far the following processors have tests
- Paypal (all versions)
- Authorize.net
- Eway
- Payflow Pro
- Dummyhttps://lab.civicrm.org/dev/financial/-/issues/127Receipts for recurring transactions are missing links to modify future contri...2023-11-23T07:34:12ZEdselopezReceipts for recurring transactions are missing links to modify future contributionsOn a backoffice credit card contribution page, there is text indicating that the email sent for a recurring contribution will contain links to modify/cancel the recurring transaction. The receipt however, does not contain these links.On a backoffice credit card contribution page, there is text indicating that the email sent for a recurring contribution will contain links to modify/cancel the recurring transaction. The receipt however, does not contain these links.seamusleeseamusleehttps://lab.civicrm.org/dev/financial/-/issues/126Membership BAO does creepy stuff with line items2020-05-27T12:44:56ZeileenMembership BAO does creepy stuff with line itemsI've finally made sense of the code in the Membership BAO but it kinda scares me. The code is set up to create line items for memberships that do not have contributions.
It seems that a membership with no contribution will wind up creat...I've finally made sense of the code in the Membership BAO but it kinda scares me. The code is set up to create line items for memberships that do not have contributions.
It seems that a membership with no contribution will wind up creating a 'best effort' line item and deleting any line items already related to the membership.
In the code I'm looking at (the renewal form) the contribution is created later & the membership winds up with 2 line items (one with a NULL contribution id).
I used to think that line items had to have a contribution id but I've learnt that participants also support orphan line items. However, in this case they wind up with both.
I'm not clear that it was intentional that ALL memberships have line items - it looks from the code like it might have originally been for non-quick-config-only. However, I guess??? it's the model now - although I suspect it's inconsistent.....
I think we might need to start passing in skipLineItem a bit more.
I can replicate the MembershipRenewal one in a test so can process that onehttps://lab.civicrm.org/dev/financial/-/issues/124Contribution page profile for honoree (db error)2023-11-23T07:33:30ZMartinContribution page profile for honoree (db error)Hi there, we're getting a database error originating from how the profile is used in the honoree section of a contribution page.
No profile is selected here in our admin, so it appears to be using the "Honoree individual" reserved profi...Hi there, we're getting a database error originating from how the profile is used in the honoree section of a contribution page.
No profile is selected here in our admin, so it appears to be using the "Honoree individual" reserved profile, which includes some contact/individual fields. To this we've added a new field that is a custom field on the Contribution object (more specifically we created this field on the contribution so the end user could enter notes relating to why they are honoring this person, so that you have an idea of the use case).
With this configuration, when a user fills out the contribution page with an honoree and enters text in this field, the contribution fails with a database error. This is shown in the Civi log:
[message] => DB Error: constraint violation
[debug_info] => INSERT INTO civicrm_value_honoree_infor_17 ( `note_53`,`entity_id` ) VALUES ( 'my note text',32747 ) ON DUPLICATE KEY UPDATE note_53 = 'my note text' [nativecode=1452 ** Cannot add or update a child row: a foreign key constraint fails (`db_name`.`civicrm_value_honoree_infor_17`, CONSTRAINT `FK_civicrm_value_honoree_infor_17_entity_id` FOREIGN KEY (`entity_id`) REFERENCES `civicrm_contribution` (`id`) ON DELETE CASCADE)]
What appears to be happening is that the entity_id that's being passed through to create the custom field value is that for a contact, rather than the contribution's id. In this case 32747 is the ID for a contact that was attempting to be created, not a contribution.https://lab.civicrm.org/dev/financial/-/issues/122Offer api support for failed payments2022-10-16T08:10:45ZeileenOffer api support for failed paymentsCurrently the support we have for failed payments is on the BaseIPN class - it should be moved to the BAO / api layer. In general BaseIPN should just call apis ....
I'm thinking about what the api call should look like
I *think* what w...Currently the support we have for failed payments is on the BaseIPN class - it should be moved to the BAO / api layer. In general BaseIPN should just call apis ....
I'm thinking about what the api call should look like
I *think* what we we want is to support the following in Payment.create api/BAO
- status_id = 'Failed'
- 'is_update_contribution_status' = TRUE/ FALSE
This would add a Failed payment to the contribution and optionally (default yes I think) do all the flow on things that currently happen. If the default for is_update_contribution_status then it is closer to the current expectations.
I realise that not everyone thinks we should fail those things in all scenarios - but they feel kinda baked in & like a bigger change to address.
I also thought about using 'is_fail_contribution' - but then if we have status_id = 'Cancelled' we don't want 'is_cancel_contribution' & is a cancel the same as a fail?
As a follow on thought - we should possibly have a message template that could be sent out when a fail occurs.
@JoeMurray @mattwire @monish.debhttps://lab.civicrm.org/dev/financial/-/issues/121Financial Type ACLs don't work on soft credits2022-08-17T22:07:10ZJonGoldFinancial Type ACLs don't work on soft creditsTo replicate:
* Create a soft credit on any contribution. Note the financial type.
* Enable Financial ACLs.
* Log in as a user who doesn't have permission to view the financial type noted above.
* If you're using `civicrm-buildkit`, an...To replicate:
* Create a soft credit on any contribution. Note the financial type.
* Enable Financial ACLs.
* Log in as a user who doesn't have permission to view the financial type noted above.
* If you're using `civicrm-buildkit`, any non-administrative user who can view CiviCRM (e.g. with `CiviCRM Webtest User` role) qualifies.
* View the contact with the soft credit.
### Expected Result
* The soft credit is not visible.
### Actual Result
* The soft credit is visible. Clicking `View` to view the original contribution returns a "permission denied", which is correct (but bad UX).JonGoldJonGoldhttps://lab.civicrm.org/dev/financial/-/issues/114Support use of 'Chargeback Account is'2020-01-14T05:39:52ZJoeMurraySupport use of 'Chargeback Account is'Currently, trying to configure a Account Relationship for 'Chargeback Account is' leads to an error, "This financial account cannot have 'Chargeback Account is' relationship.' It comes from https://github.com/civicrm/civicrm-core/blob/ma...Currently, trying to configure a Account Relationship for 'Chargeback Account is' leads to an error, "This financial account cannot have 'Chargeback Account is' relationship.' It comes from https://github.com/civicrm/civicrm-core/blob/master/CRM/Financial/BAO/FinancialTypeAccount.php#L264. The problem is that https://github.com/civicrm/civicrm-core/edit/master/CRM/Financial/BAO/FinancialAccount.php#L282 does not define this account relationship. The reason for that was core does not support the bookkeeping around that.
Currently, when the Contribution status is changed to 'Chargeback', eg from completed, the bookkeeping transaction that is created uses a reversal transaction with the 'Revenue Account is' financial account for the financial type. The proper way to handle chargebacks would be at the Payment level rather than the Contribution level. The contribution status should only change to Chargeback when all unfailed and unrefunded payments, including partial ones, are in status chargeback. But that is too big a change to work on immediately, and should be left to when we refactor and maybe eliminate the contribution status field.
The current proposal is to better support a financial type with a 'Chargeback Account is' relationship defined as follows:
1) Don't throw an error when trying to define this relationship in the browser. Instead, add the following as L292:
'Chargeback Account is' => 'Revenue', // this is like a contra-revenue account and assumes the chargeback is for a donation rather than creating a bad debt bookkeeping entry for a sale of goods or services that had a chargeback
2) When the status is being changed to 'Chargeback' on a payment or contribution, use the chargeback account rather than a reversal transaction on the financial account with a 'Revenue Account is' relationship. Note that this will be at a line item level, which is okay.
So currently, creating a completed contribution and then changing its status to Chargeback creates the following two accounting entries:
```
Debit Account Credit Account Amount
Deposit Bank Account Donation 100.00
Deposit Bank Account Donation -100.00
```
After the change, the second transaction would be
```
Deposit Bank Account Chargeback -100.00
```Monish DebMonish Debhttps://lab.civicrm.org/dev/financial/-/issues/108Disabling a price option or price field causes to be zeroed out and financial...2019-11-19T09:22:41ZseamusleeDisabling a price option or price field causes to be zeroed out and financial integrity issues when changing fee selectionsTo reproduce this error:
1. Create a Price set with > 1 price fields
2. Register and pay for someone using just 1 of those price fields
3. Now disable the price field that was used in step 2
4. Navigate to change fee selections to now s...To reproduce this error:
1. Create a Price set with > 1 price fields
2. Register and pay for someone using just 1 of those price fields
3. Now disable the price field that was used in step 2
4. Navigate to change fee selections to now select an option from the 2nd price field
5. Notice that the qty for the first price field gets zeroed out and no new financial trxn is created sensibly.
I believe @KarinG replicated this in chat https://chat.civicrm.org/civicrm/pl/ap5bif4gsifeuc57yftbxnmynh a work around is to rather than disabling the price option is to set the visibility to be admin only. The situation we were having is we were offering a catered option and an uncatered option for an event and wanted to stop people registering for the catered option at a point in time.https://lab.civicrm.org/dev/financial/-/issues/106on Order API test, pass parameters required to create line items, not line items2020-05-28T06:51:36ZJoeMurrayon Order API test, pass parameters required to create line items, not line itemsThe idea of the order api is to provide a wrapper around creating a contribution and all of the line item objects, eg the membership created when buying a membership, or the event registration when buying a ticket.
In the specification ...The idea of the order api is to provide a wrapper around creating a contribution and all of the line item objects, eg the membership created when buying a membership, or the event registration when buying a ticket.
In the specification (https://wiki.civicrm.org/confluence/display/CRM/Order+API), the idea was that there would be array of of the parameters required to create line items. Note that although the entity_table is specified, the entity_id is set to null. Moreover, there is no contribution_id value. Calling the Order API is NOT supposed to require prior calls to create other objects.https://lab.civicrm.org/dev/financial/-/issues/95Update Payment.create api handling of payment instrument2019-11-01T12:04:03ZeileenUpdate Payment.create api handling of payment instrumentAt the moment the passed in payment_instrument_id will take precedent over the one associated with the relevant processor. I don't think that is how the design was intended and in other places we use the paymnet_processor_id.
Note that...At the moment the passed in payment_instrument_id will take precedent over the one associated with the relevant processor. I don't think that is how the design was intended and in other places we use the paymnet_processor_id.
Note that I think that the Payment.create api should require either the payment_instrument_id or the payment_processor_id.
Logically this is something 'known' when making a payment & not something we should 'guess'. Transitionally we would deprecate NOT passing in one of those values rathe than requiring it.
I do think we should add some handling to Order.create so that specifically when chaining Payment.create it automatically sets some values - this being one.
@JoeMurray @monish.deb @artfulrobothttps://lab.civicrm.org/dev/financial/-/issues/93Agree / implement handling of failure for doPayment/ doRefund / params for do...2019-11-01T11:58:47ZeileenAgree / implement handling of failure for doPayment/ doRefund / params for doRefundCurrently both doPayment and doRefund throw a PaymentProcessorException if there is a failure either in terms of config or in terms of processing. The flow we agreed back when we implemented this in 4.6 is we should be working towards
`...Currently both doPayment and doRefund throw a PaymentProcessorException if there is a failure either in terms of config or in terms of processing. The flow we agreed back when we implemented this in 4.6 is we should be working towards
```
Order.create.... status=pending
try {
$result = $paymentObject->doPayment();
if ($result['payment_status_id') === 1) {
Payment.create.....
}
}
else {
\\ Add code here to add failed payment
}
```
Currently the CRM_Core_Payment doPayment converts failed results for old processors to exceptions and throws them and new processors have been expected to throw exceptions.
Some of the forms implement things similar to the above although in other cases the exception is caught much further from the call to doPayment and in some cases we are still not creating the order before processing the payment.
Note that in this flow a payment that did not fail but was not successful would not result in a payment being created.
@mattwire is proposing that we change this for doRefund and stop throwing exceptions for failed payments (or any types) and instead return a status for failed for user-related reasons and an exception for system related reasons.
The options as I see them are
1) implement the same pattern for doRefund as doPayment
2) implement and extend the pattern - ie add a couple of things we probably would have done if implementing from scratch such as
- a PaymentProcessorException_PaymentFailed Exception which extends the existing PaymentProcessorException & can be throwing when the payment relates to card failure rather than a config error. Calling code can choose to catch these separately if it chooses. (we could use this for doPayment too)
- add a getter for the outcome ie. start a transition from returning 1 / the value that maps to contributionStatus = Completed (which is not actually a payment status) to ```$paymentObject->isCompleted()``` or ``` $paymentObject->isSuccessful()```. (Note Omnipay uses isSuccessful()). We could transition doPayment to this over time.
- add a property / getter for getRefundTrxnResult
- add a property / getter for 'processor_result' / 'passthrough_params' which is something that @mattwire has suggested although I have no specifics / examples.
- encourage the use of getters & setters rather than passing in params - ie ```$paymentObject->setAmount(20.40);``` Notte we have just added getters & setters to the class in general https://lab.civicrm.org/dev/financial/issues/82 - implement this in the api call & new core interactions with the class
3) switch to the pattern matt proposes - ie. for doRefund
```
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'),
// 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,
// 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' => [],
];
```