Development issueshttps://lab.civicrm.org/groups/dev/-/issues2020-01-14T05:39:52Zhttps://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/drupal/-/issues/101Error cancelling Drupal user2020-01-06T21:44:57ZedvanleeuwenError cancelling Drupal userWhen I cancel a Drupal user account, I get a non-descriptive error and cancellation has not been done. Doing this a second time does cancel the user, but the user relation is still visible in Civi because the entry in civicrm_uf_match is...When I cancel a Drupal user account, I get a non-descriptive error and cancellation has not been done. Doing this a second time does cancel the user, but the user relation is still visible in Civi because the entry in civicrm_uf_match is not removed.
Using Drupal 7.67 and CiviCRM 5.20.2.https://lab.civicrm.org/dev/release/-/issues/13Please consider using CVE identifies for security issues2020-01-03T14:13:55ZDmitry SmirnovPlease consider using CVE identifies for security issuesInstead of vendor identifiers (e.g. `CIVI-SA-2019-24`) please consider using standard CVE identifies for security issues.
Here is some reasoning from the Debian Security Team:
> The good thing on having a CVE id for the vulnerabilities...Instead of vendor identifiers (e.g. `CIVI-SA-2019-24`) please consider using standard CVE identifies for security issues.
Here is some reasoning from the Debian Security Team:
> The good thing on having a CVE id for the vulnerabilities is helping
> other vendors to track the issues properly 'cross-vendor' in an unique
> way. If every upstream would use individual identifiers to track their
> vulnerabilities, this makes the work of downsteams security teams much
> harder. Nowdays MITRE has improved a lot on their processes on
> assigning CVEs, and good filled reports at https://cveform.mitre.org/
> get fastly assigned a CVE respectively (this somehow depends though on
> how good the report is done). I know some upstreams did in past make
> frustrating experiations, and do not want to try that out again.
See also [Debian Upstream Guide](https://wiki.debian.org/UpstreamGuide).
Thank you.https://lab.civicrm.org/dev/release/-/issues/125.20: open source compliance: non-free 'vendor/togos/gitignore'2020-01-07T08:34:12ZDmitry Smirnov5.20: open source compliance: non-free 'vendor/togos/gitignore'Release management should exercise more care in regards to Open Source compliance of the sub-vendored re-distributed components.
For instance, in release 5.20 `vendor/togos/gitignore` is a non-free component due to absence of licensing ...Release management should exercise more care in regards to Open Source compliance of the sub-vendored re-distributed components.
For instance, in release 5.20 `vendor/togos/gitignore` is a non-free component due to absence of licensing and copyright information.
https://github.com/TOGoS/PHPGitIgnore/issues/1https://lab.civicrm.org/dev/core/-/issues/1481Revisiting deadlocks2022-09-01T10:27:33ZeileenRevisiting deadlocksSome time ago we added handling to the DataObject class to catch and retry deadlocks. This seems to work and be helpful in some cases but in others it doesn't work and is possibly harmful. I want to re-open the discussion on how & where ...Some time ago we added handling to the DataObject class to catch and retry deadlocks. This seems to work and be helpful in some cases but in others it doesn't work and is possibly harmful. I want to re-open the discussion on how & where we catch them & roll them back.
**Why do we have deadlocks**
Deadlocks are a 'natural' part of mysql scalability. Mysql tries to manage contention under load to the extent that can be done at the DB layer, allowing tables & rows to be locked & queuing transactions that need to use those resources to complete after that. However, in some cases mysql cannot resolve it and it returns a deadlock error. The expectation is that the application layer will handle & retry.
For example the query in this PR was causing deadlocks - https://github.com/civicrm/civicrm-core/pull/16080 - here is how I think the flow worked - note the queries are not slow but this is under high volume.
1) contact create is called by 3 different processes in pretty quick succession
2) The query to update the employer name field is called by all 3
3) the first query 'gets the lock' - the other 2 get shared locks while they wait
4) the first query finishes. Neither of the other 2 can get the exclusive lock as they both have shared locks
5) one is reverted & a deadlock is thrown
6) the deadlock is caught in packages/DB/Dataobject
7) the Dataobject code retries the query and succeeds. The transaction succeeds.
**When don't we do a good job with deadlocks**
The above scenario is good because once the deadlock was resolved we were able to retry and it worked. However, it turns out that mysql often rolls back more than just the query it was doing when it decided there was a deadlock. Per https://dev.mysql.com/doc/refman/8.0/en/innodb-deadlock-detection.html " InnoDB automatically detects transaction deadlocks and rolls back a transaction or transactions to break the deadlock."
We are seeing a consistent pattern where under high volume we see deadlocks on ```INSERT INTO civicrm_email``` - via contact.create api (called in turn from a job api by drush). The INSERT email fails but on retry it hits a constraint error - because unknown to the php layer the ```INSERT INTO civicrm_contact ``` statement was also rolled back.
In this case the DB state is hopefully still consistent as the failure should have triggered more rollbacks but it's at least theoretically possible to have lost data in the rollback & then have the query succeed when retried - so the database is in an inconsistent state.
My high level conclusion is that we should be catching deadlocks & retrying higher up the stack - but I'm still trying to figure out where.https://lab.civicrm.org/dev/drupal/-/issues/97Status report update for Drupal (similar to what was done for Backdrop)2020-01-29T14:53:02ZlarynStatus report update for Drupal (similar to what was done for Backdrop)I wrote a PR for Backdrop some time back and have been meaning to check if it would be desired for Drupal 7 as well -- if so I would be happy to base a MR here off of what I did there. Here's a visual of how it looks in Backdrop's Status...I wrote a PR for Backdrop some time back and have been meaning to check if it would be desired for Drupal 7 as well -- if so I would be happy to base a MR here off of what I did there. Here's a visual of how it looks in Backdrop's Status Report page (Drupal 7 status report would obviously look a little different):
![Screen_Shot_2019-12-04_at_7.10.28_PM](/uploads/0a83d103f2aec9652797d3ce71c41345/Screen_Shot_2019-12-04_at_7.10.28_PM.jpg)
Would a version of this for Drupal 7 be accepted?https://lab.civicrm.org/dev/core/-/issues/1437Creating relationship with start date in future incorrectly shows relationshi...2023-01-17T01:36:45Zellen_compucorpCreating relationship with start date in future incorrectly shows relationship as activeOverview
----------------------------------------
Creating relationship with start date in future incorrectly shows relationship as active
Reproduction steps
----------------------------------------
1. Go to a contact record, then go to...Overview
----------------------------------------
Creating relationship with start date in future incorrectly shows relationship as active
Reproduction steps
----------------------------------------
1. Go to a contact record, then go to the relationships tab
1. Create a new relationship of any type
1. Give the relationship a start date that is in the future
1. Save the new relationship
Current behaviour
----------------------------------------
The relationship is saved correctly, but displays within 'current relationships' in the top half of the relationships tab.
![6AD41E8B-5DD7-4497-84A5-96600B621E9A](/uploads/a43484418ac5b913033190c9e3a52f73/6AD41E8B-5DD7-4497-84A5-96600B621E9A.jpg)
Expected behaviour
----------------------------------------
As the relationship has not yet started, the relationship should show as inactive up until the start date; at which point it should show as current.
Environment information
----------------------------------------
* __Browser:__ Chrome
* __CiviCRM:__ 5.19 (also tested on 5.15 on this test site and contact record: https://civicrm.demo.civihosting.com/civicrm/contact/view?reset=1&cid=98)
* __PHP:__ _7.0
* __CMS:__ Drupal
Comments
----------------------------------------
Contact record from my testing on a demo site: https://civicrm.demo.civihosting.com/civicrm/contact/view?reset=1&cid=98
cc @jamienovick1 @shitijghttps://lab.civicrm.org/dev/core/-/issues/1408The DAO "FreshnessTest" in the jenkins test suite does nothing because setup....2023-01-08T05:19:21ZDaveDThe DAO "FreshnessTest" in the jenkins test suite does nothing because setup.sh runs before itThe test itself works (CRM_Core_CodeGen_FreshnessTest::testDAOs()), but because in jenkins runs setup.sh gets run near the start of the build, a DAO will appear to be updated even if the git tree doesn't have the updated DAO.
The purpos...The test itself works (CRM_Core_CodeGen_FreshnessTest::testDAOs()), but because in jenkins runs setup.sh gets run near the start of the build, a DAO will appear to be updated even if the git tree doesn't have the updated DAO.
The purpose of the freshness test seems to be to make sure you don't forget to include the DAO in PRs when you make changes to an xml schema to add/change/remove a field in core tables. But it's not doing that currently in jenkins runs because it's always comparing against DAOs after setup.sh/GenCode.php has run.
I'm not sure if there's an easy fix, or maybe the test should be removed and replaced with some developer documentation? It wouldn't even work in a local test suite run if you've run GenCode.php, which you would have had to do to physically try out your schema changes.https://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/core/-/issues/1380No easy way to import signatures to petition2022-11-30T19:32:57ZCoreyBurgerNo easy way to import signatures to petitionSo there is no easy way to mass import signatures to a petition.
When looking at Petitions on Campaign > Dashboard > Petitions on the right side under the more option, there should be an option to "import signatures", which should be a...So there is no easy way to mass import signatures to a petition.
When looking at Petitions on Campaign > Dashboard > Petitions on the right side under the more option, there should be an option to "import signatures", which should be a custom version of the Import Activities.
As a workaround, I had to manually lookup how to import it, include the required numeric fields for source_record_id and activity_id.
CiviCRM 5.15 on Wordpresshttps://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' => [],
];
```https://lab.civicrm.org/dev/financial/-/issues/92Change signature of CRM_Core_Payment::doRefund(&$params) to not receive $para...2019-11-01T12:04:35ZeileenChange signature of CRM_Core_Payment::doRefund(&$params) to not receive $params as a referenceThis is a proposal by @mattwire which is mostly stylistic
pros - we have been moving away from receiving as reference
- less scope for people to try to achieve odd things in there
cons - less consistent with doPayment
- would...This is a proposal by @mattwire which is mostly stylistic
pros - we have been moving away from receiving as reference
- less scope for people to try to achieve odd things in there
cons - less consistent with doPayment
- would need to do a civi universe or whatever that things is of Tims to find what processors might need to be patched to cope
- less opportunity for payment processors to tweak thingshttps://lab.civicrm.org/dev/financial/-/issues/91Discuss / maybe resolve schema issue on changing line items2019-10-25T22:17:31ZeileenDiscuss / maybe resolve schema issue on changing line itemsPer
https://docs.civicrm.org/dev/en/latest/financial/financialentities/
If the line items change then the items financial items have to be updated. Generally the rule is to zero-out the current line items
and reverse their financial ite...Per
https://docs.civicrm.org/dev/en/latest/financial/financialentities/
If the line items change then the items financial items have to be updated. Generally the rule is to zero-out the current line items
and reverse their financial items and create new ones. This is a pretty standard accounting approach.
Unfortunately there are some scenarios where the schema does not permit this which has resulted in
adjustment line items being created in these cases. The issue is that the civicrm_line_item table has a unique index for
entity_table + entity_id + contribution_id + price_field_value_id + price_field_id.
This means that if a line item with no price_field_values (ie a text / enter quantity line item) is altered it is not possible
to create a reversal line a new line within the schema. The same problem occurs when changing a line item with price_field_values
BACK to a price_field_value it previously held. In both these scenarios the work around is to have more than one 'valid' financial_item
against the resulting line item with an 'adjustment' entry - ie an additional financial_item.
I don't have any good answers except maybe add an 'is_current' field & add that to the index. Perhaps the current pain is unavoidable but it feels a bit like an own goal
@JoeMurray @monish.deb @lcdweb @kcristiano @pradeephttps://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/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/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/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/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)