Financial issueshttps://lab.civicrm.org/dev/financial/-/issues2019-10-25T08:12:07Zhttps://lab.civicrm.org/dev/financial/-/issues/90Add new api Order functionality to update line items2019-10-25T08:12:07ZeileenAdd new api Order functionality to update line itemsI'm pretty sure that despite @JoeMurray's optimism the current apiv3 Order.create doesn't support this. @monish.deb & I discussed adding it back here
https://github.com/civicrm/civicrm-core/pull/11380
I don't think we should add it o a...I'm pretty sure that despite @JoeMurray's optimism the current apiv3 Order.create doesn't support this. @monish.deb & I discussed adding it back here
https://github.com/civicrm/civicrm-core/pull/11380
I don't think we should add it o apiv3 now - I think we should write a v4 api that does it. I think the object oriented approach will allow us to come up with cleaner methods. I'm kinda keen to try to build those up on a ORDER pseudoBAO class & play a bit before we expose in the api so we can learn about about what works & doesn't before locking it inhttps://lab.civicrm.org/dev/financial/-/issues/89A contribution batch containing transactions of more than one currencies caus...2022-01-09T23:43:19ZkleehkA contribution batch containing transactions of more than one currencies causes fatal errorMy setup: CiviCRM 5.18.3 on Drupal 7.
I created a Contribution batch. At transactions I chose two transactions of difference currencies e.g. HKD and USD and Assign them to the batch.
Then I no longer be able to retrieve the list of bat...My setup: CiviCRM 5.18.3 on Drupal 7.
I created a Contribution batch. At transactions I chose two transactions of difference currencies e.g. HKD and USD and Assign them to the batch.
Then I no longer be able to retrieve the list of batches because of fatal error: Dialog: DataTables warning: table id=crm-batch-selector-2 - Ajax error. For more information about this error, please see http://datatables.net/tn/7
Even I cancel the offending transaction it does not help. I had to delete the transaction in that batch to revive the batch's functioning.
Originally posted at StackExchange
[Link here](https://civicrm.stackexchange.com/questions/33382/a-contribution-batch-containing-transactions-of-more-than-one-currencies-causes)
```
Oct 17 09:53:08 [debug] $backTrace = #0
/var/www/c53/web/sites/all/modules/civicrm/CRM/Core/Error.php(463):
CRM_Core_Error::backtrace("backTrace", TRUE)
#1 /var/www/c53/web/sites/all/modules/civicrm/CRM/Core/Invoke.php(55): CRM_Core_Error::handleUnhandledException(Object(CRM_Core_Exception))
#2 /var/www/c53/web/sites/all/modules/civicrm/drupal/civicrm.module(444): CRM_Core_Invoke::invoke((Array:3))
#3 /var/www/c53/web/includes/menu.inc(527): civicrm_invoke("ajax", "batchlist")
#4 /var/www/c53/web/index.php(21): menu_execute_active_handler()
#5 {main}
```
Attached please see the error screen capture![MixedCurrencyError](/uploads/a472c5fec8af87b6b62e8e13342c9c99/MixedCurrencyError.png).5.47.0JoeMurrayJoeMurrayhttps://lab.civicrm.org/dev/financial/-/issues/88Should we add is_deleted to the civicrm_contribution table?2019-10-24T11:38:33ZeileenShould we add is_deleted to the civicrm_contribution table?Despite @JoeMurray's best efforts we DO all contributions to be deleted. We do have precedent on other tables for soft deletions via an is_deleted flag. The difficulty with switching to that is it takes time to filter out possible places...Despite @JoeMurray's best efforts we DO all contributions to be deleted. We do have precedent on other tables for soft deletions via an is_deleted flag. The difficulty with switching to that is it takes time to filter out possible places of leakage.
However, we have just added is_template to the civicrm_contribution table (https://lab.civicrm.org/dev/financial/issues/72) on the understanding we will put the time into filtering out leakage over a few releases before starting to use it (& initially not in core) - which makes me wonder if we should add is_deleted & do the work on that at the same time & perhaps add an extension for soft deleting for those who want to be ahead of the game.
@JoeMurray @BjoernE @mattwire @ejegg @pradeep @monish.deb @ayduns @artfulrobot - any thoughts?https://lab.civicrm.org/dev/financial/-/issues/87Partial Refunds2022-06-14T18:38:44Zmattwiremjw@mjwconsult.co.ukPartial RefundsStripe supports partial refunds via the Stripe Dashboard. I think other processors support similar.
To record a partial refund in CiviCRM we can record a negative payment on the contribution using API `Payment.create`.
Currently nothin...Stripe supports partial refunds via the Stripe Dashboard. I think other processors support similar.
To record a partial refund in CiviCRM we can record a negative payment on the contribution using API `Payment.create`.
Currently nothing changes on the contribution:
* Total amount still shows the full amount but payments are recorded correctly so the sum of payments does not equal the contribution total amount.
* Contribution status remains Completed.
What should happen(?):
* A partial refund means that the total_amount paid, tax_amount and the fee_amount paid may be reduced.
* The contribution status is no longer "Completed" and should be `Partially refunded` (does not exist)? `Partially paid` (already exists).
* The `Financial Type` should be displayed correctly for the refund payment.
Currently, viewing a contribution, either in the summary or detail gives no indication of the actual amount paid:
![image](/uploads/39a50dd48d946207feda438321850a39/image.png)
![image](/uploads/63cc3abf7df50ac1babcbff85f2d4c86/image.png)
@JoeMurray @eileen @artfulrobot @ayduns Thoughts please?https://lab.civicrm.org/dev/financial/-/issues/86Make 'Record Payment' & 'Record Refund' visible regardless of whether the bal...2020-09-16T00:27:09ZeileenMake 'Record Payment' & 'Record Refund' visible regardless of whether the balance 'requires' onePer https://lab.civicrm.org/dev/financial/issues/85
@JoeMurray agreed that it is OK to ALWAYS show 'record payment' & 'record refund' as opposed to currently it only shows 'add payment' if there it a balance to pay & add 'record refund'...Per https://lab.civicrm.org/dev/financial/issues/85
@JoeMurray agreed that it is OK to ALWAYS show 'record payment' & 'record refund' as opposed to currently it only shows 'add payment' if there it a balance to pay & add 'record refund' if there is a balance to refund. However, we need to be able to overpay & over refund - I'm going to work on this issue & seeing the UI afterwards (just having the links more often visible) might affect what people think 'makes sense'
I did start on this but discovered a regression when I started writing my preliminary test so it's on hold while that gets merged through to masterhttps://lab.civicrm.org/dev/financial/-/issues/85UI in core for processing refunds2019-10-21T21:48:01ZeileenUI in core for processing refundsWe looked at adding the ability to process refunds via the UI at the sprint.
We looked at
1) Changing the existing record refund form (Additional Payment form) to provide the option of processing the refund if offered by the processor ...We looked at adding the ability to process refunds via the UI at the sprint.
We looked at
1) Changing the existing record refund form (Additional Payment form) to provide the option of processing the refund if offered by the processor if a check box was selected
1) Changing the existing record refund form (Additional Payment form) to provide the option of processing the refund if offered by the processor if a differently named button was pressed
1) Having a second action for 'Process Refund' or similar
Opinions at Barcelona were pretty evenly divided between the 3!
A couple of things that might make a difference
1) here is a screen shot of how adding a payment currently looks
![Screen_Shot_2019-10-21_at_7.57.12_PM](/uploads/08d0eb711f4dd11fe0a980b8f92f949c/Screen_Shot_2019-10-21_at_7.57.12_PM.png)
1) @JoeMurray was keen that it be a case of the option showing if ALL payments on a contribution could be refunded. In the edge case of payments from more than one processor the submit refund option would not show
1) @JoeMurray agreed that it is OK to ALWAYS show 'record payment' & 'record refund' as opposed to currently it only shows 'add payment' if there it a balance to pay & add 'record refund' if there is a balance to refund. However, we need to be able to overpay & over refund - I'm going to work on this issue & seeing the UI afterwards (just having the links more often visible) might affect what people think 'makes sense' - issue for that is https://lab.civicrm.org/dev/financial/issues/86https://lab.civicrm.org/dev/financial/-/issues/84Making the poor performance associated with the creditnote_id field opt in r...2021-03-31T02:07:56ZeileenMaking the poor performance associated with the creditnote_id field opt in rather than opt outSometime before the joys of Lexim code was introduced to Core to add a creditnote_id to any contribution whose status was changed to Refunded, or Cancelled (later Chargeback was added).
This fitted a specific use case - the numbers pe...Sometime before the joys of Lexim code was introduced to Core to add a creditnote_id to any contribution whose status was changed to Refunded, or Cancelled (later Chargeback was added).
This fitted a specific use case - the numbers permitted a prefix & had to be sequential. However, anecdotally most large sites have hacked it out because the performance is so bad.
During the Barcelona sprint a [patch was merged](https://github.com/civicrm/civicrm-core/pull/15232) making it possible to install [an extension](https://github.com/greenpeace-cee/at.greenpeace.creditnope) to opt out of having a credit note sloooowwwly calculated.
However, in the world of Lexim use-case specific functionality should be OPT IN rather than opt out. Ie it should be the case that you need to install an extension to GET the slow behaviour not to NOT get it.
This is also a good test case for the general practice of moving functionality out of core into an extension. Here are the steps I believe are required
1) Determine where the code interacts with core. I did this & determined that all the places where createCreditNoteID is called from other than Contribution.create are not hit & I added a [pr to deprecate them](https://github.com/civicrm/civicrm-core/pull/15492) & fix the one place where it is hit. As a result of this the only interaction required would be in the pre_hook so we don't need to add a hook to generate this field (per https://lab.civicrm.org/dev/core/issues/1308) - this is a good thing as the assumption that credit notes & contributions would not be many to one is flawed & we don't want to bake this into core - we want to get it out of core
2) Create an extension that can be installed to add the credit note id in the pre hook.
3) Get the extension reviewed & approved for automatic installation
4) Add this new extension so it is installed on upgrade for existing sites - this step will require some input from @totten on how to do it & not knowing how is a blocker but probably getting the extension ready is a necessary pre-step & is probably only a couple of hours of work
Also we should consider this proposed performance improvement https://github.com/civicrm/civicrm-core/pull/15235https://lab.civicrm.org/dev/financial/-/issues/83Barcelona sprint financials2019-11-01T12:05:38ZeileenBarcelona sprint financialsI'm creating this issue to document & track the discussions we had at the Barcelona sprint.
We had a financial team of @mattwire @JoeMurray @ayduns @artfulrobot @BjoernE @andrei @ejegg present
Main issues
1. Ensuring there were ...I'm creating this issue to document & track the discussions we had at the Barcelona sprint.
We had a financial team of @mattwire @JoeMurray @ayduns @artfulrobot @BjoernE @andrei @ejegg present
Main issues
1. Ensuring there were no blockers to people using the Order.create->PaymentProcessor.pay->Payment.create flow https://lab.civicrm.org/dev/financial/issues/76
1. Making the poor performance associated with the creditnote_id field opt in rather than opt out https://lab.civicrm.org/dev/financial/issues/84
1. Addressing Stripe's need to store some Stripe generated information for which there is no suitable field. We resolved this at the data level by adding the field civicrm_financial_trxn.result_code. However docs pending! https://lab.civicrm.org/dev/financial/issues/57
1. Having a plan to get off our broken contribution.template model https://lab.civicrm.org/dev/financial/issues/6
1. UI for permitting refunds processing for payments. https://lab.civicrm.org/dev/financial/issues/85
Not actually from the sprint but topical from discussions - add getters & setters to payment processor base class https://lab.civicrm.org/dev/financial/issues/82https://lab.civicrm.org/dev/financial/-/issues/82Develop getter & setter structure for Payment classes2021-05-31T22:31:04ZeileenDevelop getter & setter structure for Payment classesI don't think anyone designing Payment Processing subsystem would include the spec that you pass an undefined set of parameters to the 'doPayment' method, but that is how ours grew up.
This has caused us issues with inconsistencies arou...I don't think anyone designing Payment Processing subsystem would include the spec that you pass an undefined set of parameters to the 'doPayment' method, but that is how ours grew up.
This has caused us issues with inconsistencies around how payment processors determine variables. It also makes it really hard to clean up the code that interacts with payment processors as the code spends a lot of time buying arrays which go off into the void.
I'm pretty sure that if we were designing from scratch we would make it look much more like
```
$paymentObject = $this->getPaymentObject();
$paymentObject
->setContributionID(5)
->setContactID->(6)
->setAmount(50)
->setInvoiceID('xyz')
->setRecurringFrequency(1)
->setRecurringUnit('week')
....
->doPayment();
$feeAmount = $paymentObject->getFeeAmount();
```
We might provide some helpers etc but that seems much more inline with modern coding conventions and with our desire for standardisation.
I believe we should start adding getters & setters & encouraging payment processors to use them. The getters would do any normalisation of parameters (I'm looking at you currency vs currencyID).
It would be quite some time before this usage has 'flowed through the system' & we switch to relying on it when making payments but I think that adding this structure sets us up well for the future.
See PR for proposed change - which will not ACTUALLY change anything other than make these methods available in the first instance
https://github.com/civicrm/civicrm-core/pull/15509
@artfulrobot @KarinG @adixon @mattwire @andrewhunt
(Payment\PropertyBag)https://lab.civicrm.org/dev/financial/-/issues/81Update Contribution Payment Instrument Id to match first payment when the st...2019-11-01T20:59:49ZeileenUpdate Contribution Payment Instrument Id to match first payment when the status is PendingIf we are asking people to create a pending Order they may not know the payment instrument at that time. We should adjust Payment.Create to update it when a payment comes in & it is the first one.
(Alternatively we could always update ...If we are asking people to create a pending Order they may not know the payment instrument at that time. We should adjust Payment.Create to update it when a payment comes in & it is the first one.
(Alternatively we could always update it so it becomes the latest - probably an edge case)https://lab.civicrm.org/dev/financial/-/issues/80Noisily deprecate transact api2019-10-24T21:16:11ZeileenNoisily deprecate transact apiWe should add in deprecation warnings using CRM_Core_Error::deprecatedFunctionWarning - these show on dev sites but not live
First step is https://lab.civicrm.org/dev/financial/issues/79We should add in deprecation warnings using CRM_Core_Error::deprecatedFunctionWarning - these show on dev sites but not live
First step is https://lab.civicrm.org/dev/financial/issues/795.20.0https://lab.civicrm.org/dev/financial/-/issues/79Deprecate Contribute.transact api2019-10-24T21:15:57ZeileenDeprecate Contribute.transact apiThe minimum we should do is deprecate from api explorer & I will do that in this PR. (Agreed in Barcelona)
I think we probably should do a noisier deprecation than that but that will stale-out some PRs currently under discussionThe minimum we should do is deprecate from api explorer & I will do that in this PR. (Agreed in Barcelona)
I think we probably should do a noisier deprecation than that but that will stale-out some PRs currently under discussion5.20.0https://lab.civicrm.org/dev/financial/-/issues/78Order api should probably create an invoice ID2020-01-13T09:38:31ZeileenOrder api should probably create an invoice IDCurrently contribute.transact api does this -it makes sense to do it at the order level - for create.Currently contribute.transact api does this -it makes sense to do it at the order level - for create.https://lab.civicrm.org/dev/financial/-/issues/77PaymentProcessor.pay should handle 'invoice_id' & map it to a getter so pro...2019-11-04T23:47:09ZeileenPaymentProcessor.pay should handle 'invoice_id' & map it to a getter so processors can retrieve it5.20.0https://lab.civicrm.org/dev/financial/-/issues/76Metaissue for ensuring our preferred order->pay->createPayment flow is used ...2023-11-23T06:59:31ZeileenMetaissue for ensuring our preferred order->pay->createPayment flow is used everywhere.We have long had the principle (since 4.6 was pre-alpha) that we want to work towards the following flow at a code level.
1. Create a pending Order using Order.create api
1. Optionally process a payment against that order using PaymentP...We have long had the principle (since 4.6 was pre-alpha) that we want to work towards the following flow at a code level.
1. Create a pending Order using Order.create api
1. Optionally process a payment against that order using PaymentProcessor.pay api
1. Create a payment record (either from above or just recording an offline payment) using Payment.create api
The expectation is that the code correctly creates a pending contribution (order) with all the related entities when Order.create is called.
When a payment is added the related entities are updated & if appropriate notification emails are sent out.
(Note an earlier iteration of this was to create a pending contribution always & then update it with contribution.completetransaction - this maps to the above as the Order.create creates the pending contribution and Payment.create calls contribution.completetransaction when the payment completes the order).
This issue is a metaissue to link to & categorise all the things that need to be done to reach this. 'High' Priority are any issues that stand before people adopting the recommended flow.
| Item | Status| Priority| Blockers|Notes|
| ------ | ------ |------ | ------ |------ |
| Document order api / Pseudoentity | In progress|High| - |At the [sprint this was started](https://docs.civicrm.org/dev/en/latest/financial/OrderAPI/)[Current tweak](https://github.com/civicrm/civicrm-dev-docs/pull/704)|
|~~Deprecate creating orders with non-pending status~~|Done|High||||
|~~Order.create should not require total amount~~|Done|High|||https://lab.civicrm.org/dev/financial/issues/73|
|Identify any issues that would block someone moving off a non-preferred flow to our preferred flow|Started|High||This is really what this issue is about|
| Update payment instrument id when adding a payment to a pending order|To discuss| High| Discussion| https://lab.civicrm.org/dev/financial/issues/81 and possibly https://lab.civicrm.org/dev/financial/issues/47|
|~~Support invoice_id in PaymentProcessor.pay~~|done|high||https://lab.civicrm.org/dev/financial/issues/77|
| ~~Support chaining Payment.create from Order~~|PR|High||Needed to ease adaption to us deprecating creating Orders as non-pending [pr](https://github.com/civicrm/civicrm-core/pull/15548)|
|Fix identified core bug on ParticipantPayment creation|To do|High|Tests not yet passing| https://lab.civicrm.org/dev/financial/issues/74|
|~~Make contribution_id mandatory for PaymentProcessor.pay~~|Done|High||[PR](https://github.com/civicrm/civicrm-core/pull/15477) - since we have agreed for policy (not technical) reasons to do this we should try to get a test & merged|
| ~~Document moving off transact api~~| Done | Medium||[PR here](https://github.com/civicrm/civicrm-dev-docs/pull/705)|
| Consider generating default invoice ID in Order.create|To do|Medium|Discussion|https://lab.civicrm.org/dev/financial/issues/78|
|~~Add getters & setters to Payment Class~~|Done - could do more maybe| Medium||https://lab.civicrm.org/dev/financial/issues/82|
| ~~Deprecate transact api from explorer~~ | Done |Medium||https://lab.civicrm.org/dev/financial/issues/79|
|~~Deprecate & remove calls to CRM_Contribute_BAO_Contribution::addPayments~~|To do|Low||This is part of consolidating all payment creation on Payment.create|
|Fix all remaining places in core to create a pending contribution/order & then complete|Needs chipping away at - tacking events at the moment|In progress|Low|https://lab.civicrm.org/dev/financial/issues/53|
| ~~Noisily deprecate transact api~~ |Done| low||https://lab.civicrm.org/dev/financial/issues/80|https://lab.civicrm.org/dev/financial/-/issues/75on order.get, line item has obsoleted contribution_type_id2019-10-19T22:49:23ZJoeMurrayon order.get, line item has obsoleted contribution_type_idThe line item table does not have a contribution_type_id. But when the results of a contribution.get are displayed, the line item fields includes both financial_type_id and contribution_type_id. This seems like an historical artifact tha...The line item table does not have a contribution_type_id. But when the results of a contribution.get are displayed, the line item fields includes both financial_type_id and contribution_type_id. This seems like an historical artifact that should be removed.https://lab.civicrm.org/dev/financial/-/issues/74Order.create - registering multiple Participants (participantPayment records ...2020-03-04T23:10:14ZAndrei Mondocandreimondoc@gmail.comOrder.create - registering multiple Participants (participantPayment records not created properly)Creating an Order to register two (or more) participants creates two (or more) `ParticipantPayment` records, one for each participant line item, registering two participants via an Event page creates only one `ParticipantPayment` record ...Creating an Order to register two (or more) participants creates two (or more) `ParticipantPayment` records, one for each participant line item, registering two participants via an Event page creates only one `ParticipantPayment` record regardless of the number of participants registered.
Which is the correct one/what is expected behaviour?
### Steps to reproduce
Call Order.create with parameters similar to:
``` php
$params = [
'total_amount' => 100.00,
'financial_type_id' => 4,
'contribution_status_id' => 'Pending',
'payment_instrument_id' => 1,
'currency' => 'USD',
'source' => 'Multiple participants (Order.create)',
'contact_id' => 2,
'line_items' => [
1 => [
'line_item' => [
0 => [
'price_field_id' => 3,
'label' => 'Tickets',
'financial_type_id' => 4,
'price_field_value_id' => 3,
'qty' => 1,
'field_title' => '1 Ticket',
'unit_price' => 50.000000000,
'line_total' => 50,
'entity_table' => 'civicrm_participant',
],
],
'params' => [
'contact_id' => 2,
'event_id' => 1,
'role_id' => 1,
'source' => 'Multiple participants (Order.create)',
'price_set_id' => 7,
'fee_level' => '1 Ticket',
'fee_amount' => 50,
],
],
2 => [
'line_item' => [
0 => [
'price_field_id' => 3,
'label' => 'Tickets',
'financial_type_id' => 4,
'price_field_value_id' => 3,
'qty' => 1,
'field_title' => '1 Ticket',
'unit_price' => 50.000000000,
'line_total' => 50,
'entity_table' => 'civicrm_participant',
],
],
'params' => [
'contact_id' => 5,
'event_id' => 1,
'role_id' => 1,
'source' => 'Multiple participants (Order.create)',
'price_set_id' => 7,
'fee_level' => '1 Ticket',
'fee_amount' => 50,
],
],
],
];
$order = civicrm_api3('Order', 'create', $params);
```
This results in two ParticipantPayment records:
``` json
// civicrm_participant_payment
[
{
"id": 5,
"participant_id": 5,
"contribution_id": 18
},
{
"id": 6,
"participant_id": 6,
"contribution_id": 18
}
]
```
Registering two or more participants via an Event page, always creates one ParticipantPayment regardless of the number of participants registered.
``` json
// civicrm_participant_payment
{
"id": 7,
"participant_id": 7,
"contribution_id": 19
}
```https://lab.civicrm.org/dev/financial/-/issues/73Order.create should not require total amount2019-10-19T23:49:21ZJoeMurrayOrder.create should not require total amountThe spec for Order API create indicates that the total_amount is not required. If it is not provided then the total is calculated for the line item totals. However, https://dmaster.demo.civicrm.org/civicrm/api#explorer shows that total_a...The spec for Order API create indicates that the total_amount is not required. If it is not provided then the total is calculated for the line item totals. However, https://dmaster.demo.civicrm.org/civicrm/api#explorer shows that total_amount is a required parameter. Please fix the implementation of the order api create functionality to not require total_amount.5.20.0Monish DebMonish Debhttps://lab.civicrm.org/dev/financial/-/issues/72New methodology for storing template contributions2023-03-18T05:06:58ZeileenNew methodology for storing template contributionsRecurring contributions create new contributions based on a template.
Currently the template is the most recent contribution with a value in contribution_recur_id field equivalent to the recurring contribution.
The main issue with this...Recurring contributions create new contributions based on a template.
Currently the template is the most recent contribution with a value in contribution_recur_id field equivalent to the recurring contribution.
The main issue with this is that if the contribution is deleted (e.g because it's far in the future & confusing) there is no way to create contributions against the recurring profile.
From a data model this idea of an 'implicit template' is far from ideal. However, a key challenge is that the template contribution holds references to custom data which is keyed by entity_id so using a table other than civicrm_contribution to store the template is highly fraught
At the sprint we concluded that the correct approach is to have the ability to store template contributions in a non-exposed way. To identify them we want to do BOTH
- create a new contribution status 'Template-only'
- add a new field is_template to the table
The reason for both is that we think is_template is the correct maintainable approach. However we think that many existing things won't search on it but WILL filter on contribution_status_id.
We will mitigate any impacts further by
1) making is_template=0 a default criteria on apiv3, apiv4, BAO_Query
2) not actually starting to create template transactions in core processes initially - processors that use it can drive the next stage of development
3) BAO_Contribution::create() will ensure is_template & status are in sync.
4) not permitting any UI editing of this field/ template contributions (until we build a specific UI)
The way this field WILL be initially usable is that the BAO_Contribution::getTemplateContribution function will attempt
1) get contribution with is_template = 1 AND contribution_recur_id = x
2) failing that fall back on existing latest contribution with contribution_recur_id = xhttps://lab.civicrm.org/dev/financial/-/issues/70Enabling more than one payment methods for checks2019-10-09T12:32:48ZJoeMurrayEnabling more than one payment methods for checksSome organizations have more than one bank account that they deposit checks into. It would be appropriate to configure one payment method / payment instrument for each of these bank accounts so that a different Financial Account can be c...Some organizations have more than one bank account that they deposit checks into. It would be appropriate to configure one payment method / payment instrument for each of these bank accounts so that a different Financial Account can be created for each.
Unfortunately, there are a number of places in core that bake in the idea that there is only ever a single payment method for checks. While we have worked around this with an extension that uses an override (specifically of this line: https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment/Manual.php#L95), JMA would like to remove this deficiency, or at least limitation, or core.
Here are some notes from Monish on implementation ideas. We'd like to get approval here, potentially after refinement, before we start.
1. Add hook to add/override payment fields against respective payment instrument.
https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment.php#L609 this would be the place to add the new hook say:
CRM_Utils_Hook::extendPaymentFields($paymentInstrumentID, &$paymentFields);
Another place to extend metadata via hook https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/Payment.php#L724
2. Change the hardcoded name ‘Check’ in various places, on basis of which check number information is fetched. In order to generalise it, we need to have a UI where we can define payment fields against each payment instrument. In that way it will be easy to gather payment information for a given payment instrument otherwise it will be a problem when the existing PI (payment instrument) name or value is changed like it occurred in case of Check PI whose name was changed to cheque.
Places where Check name was explicitly called (found using grep -irn "'Check'" CRM/ ):
`
CRM//Financial/Page/AJAX.php:371: if ($row[$financialItem->id][$columnKey] == 'Check') {
CRM//Financial/Form/PaymentEdit.php:178: elseif ($paymentInstrumentName == 'Check') {
CRM//Contribute/BAO/Contribution.php:4140: if ($paidByName == 'Check') {
CRM//Contribute/Form/Contribution.php:649: $checkPaymentID = array_search('Check', CRM_Contribute_PseudoConstant::paymentInstrument('name'));
CRM//Contribute/BAO/Contribution.php:185: if ($params['payment_instrument_id'] != array_search('Check', $paymentInstruments)) {
And there are various other places (that hard to find using grep) which have an exclusive condition on payment instrument type where that is using in fetching particular payment field(s) for them.
In order to generalise the flow, we should have
2.1. a separate UI to say 'New Payment Instrument' form
2.2. This UI should have basic payment instrument fields, label, value and a section called payment fields where one can add multiple payment fields against that payment instrument and also select the html type. Say for CC, pan_truncation and card_type_id is text and select field respectively. It could be pretty much similar to the 'New Custom field' form where we can choose name, html type and data type.
3. Upon submission it will save the data in uf_join.module_data as
INSERT INTO `civicrm_uf_join` ( `is_active`, `module`, `entity_table`, `entity_id`, `weight`, `uf_group_id`, `module_data`)
VALUES ( '1', 'payment_instrument', 'civicrm_option_value', '17', '1', '', {"check_number":"ABC123"})
4. After declaring the payment fields, it will render them in backoffice contribution/membership/event forms when the appropriate payment method is selected.
5. To avoid a schema change, on submit the payment information could be stored in civicrm_financial_trxn.payment_data in serialised format. Obviously there are alternatives, such as doing the work to support custom fields on civicrm_financial_trxn, even though much of the existing custom field code would be problematic when applied to a table never exposed through search or directly through forms (closest is payments).
6. Handle install and upgrade to adapt this new change
7. Make changes across financial core files to fetch payment field
8. Add UTs
So main objective: enable more than one payment method to be of type check, and thus prompt the display on view and create/edit of the check number field.
@eileen @monish.deb