Switch recordAdditionalPayment fully over to api

[eileenmcnaughton]

Let's see what fails first

Overview

Code consolidation on payment handling

Before

Disparate code paths

After

Consolidated code paths

Technical Details

The payment code was originally tightly coupled with the additional payment form. Later a payment.create api was split off but the form was never changed to use it & they grew up separately. Fortunately both have pretty good testing by now

Comments

[civibot]

(Standard links)

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

[eileenmcnaughton]

Looks like 2 receipts are being sent now

CRM_Contribute_Form_AdditionalPaymentTest::testMultiplePaymentForPartiallyPaidContributionWithOneCreditCardPayment
Incorrect subjects: Array
(
    [expected] => Array
        (
            [0] => Payment Receipt -
        )

    [actual] => Array
        (
            [0] => Payment Receipt -
            [1] => Receipt - Contribution
        )

)

Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'Payment Receipt -'
+    1 => 'Receipt - Contribution'
 )

One is coming from completetransaction

I think we need a is_send_receipt = 0 to go through to payment.create & from there to confirmcontribution from this form

[eileenmcnaughton]

failure are on dedupe - ie

Stacktrace
CRM_Dedupe_MergerTest::testBatchMergeSelectedDuplicates
Undefined offset: 3

/home/jenkins/bknix-dfl/build/core-14408-2pa8a/sites/all/modules/civicrm/CRM/Contact/BAO/Contact.php:2091
/home/jenkins/bknix-dfl/build/core-14408-2pa8a/sites/all/modules/civicrm/CRM/Contact/BAO/Contact.php:1995
/home/jenkins/bknix-dfl/build/core-14408-2pa8a/sites/all/modules/civicrm/CRM/Dedupe/Merger.php:1683
/home/jenkins/bknix-dfl/build/core-14408-2pa8a/sites/all/modules/civicrm/CRM/Dedupe/Merger.php:2123
/home/jenkins/bknix-dfl/build/core-14408-2pa8a/sites/all/modules/civicrm/CRM/Dedupe/Merger.php:877
/home/jenkins/bknix-dfl/build/core-14408-2pa8a/sites/all/modules/civicrm/CRM/Dedupe/Merger.php:706
/home/jenkins/bknix-dfl/build/core-14408-2pa8a/sites/all/modules/civicrm/tests/phpunit/CRM/Dedupe/MergerTest.php:190
/home/jenkins/bknix-dfl/build/core-14408-2pa8a/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php:215
/home/jenkins/bknix-dfl/civicrm-buildkit/extern/phpunit5/phpunit5.phar:598

I can't replicate locally running all CRM & I can't see how it relates - but I also think it is consistent

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[seamuslee001]

@eileenmcnaughton needs a rebase

[eileenmcnaughton]

test this please

[eileenmcnaughton]

test this please

[seamuslee001]

this seems to make sense to me @monishdeb ?

[eileenmcnaughton]

@seamuslee001 tests were a total wash until the null Array work - yay for fixing that!

[kcristiano]

@eileenmcnaughton Looks like we need a rebase after #14589

[eileenmcnaughton]

@kcristiano rebased

[kcristiano]

@eileenmcnaughton I am trying to determine the best way to do r-run on this PR. My current thinking is that we need to go through the full checklist of payments and refunds in each entity.

To start I did a back end contribution using a price set. The price set had multiple financial types. Recording was correct. I edited and cancelled a line item. Looked good. I went to record the refund and the error Mandatory key(s) missing from params array: contribution_id occurred. This is the same error we had in 5.16 RC and backported to 5.15.1.

I'll rebuild master and start testing, but wanted your thoughts on the testing methodology.

Edit: I rebuilt master. Without patch Refunds work with the patch refunds fail.

[eileenmcnaughton]

@kcristiano so we DO have a swag of tests in

civicrm-core/tests/phpunit/CRM/Contribute/Form/AdditionalPaymentTest.php

Line 35 in f59c3d8

class CRM_Contribute_Form_AdditionalPaymentTest extends CiviUnitTestCase {

- they didn't pick this bug up because obviously the tests ensure contribution_id is passed into the form submit bug whereas that was not the case.

Ideally we WOULD go through a full suite of tests but we do also have some unit test support and testing 'everything' might be unachievable

I have updated this PR with what I think will fix the refunds issue

[kcristiano]

Thanks @eileenmcnaughton PR applied.

Made contributions, partially paid and then fully paid. Works perfectly. Added Contribution with multiple financial Types, partially paid, then fully paid. Payments are prorated against Accts Receivable as that is the only way - This is expected. Once fully paid Income is correct, as are cash and AR.

Then did a refund of one line item. Refund works. Credited the proper Financial Type for Income.

When repeating for events after doing partial payments a Credit card payment inserts itelf

MariaDB [wpcv34civi_s52lk]> select e.*,f.id,f.from_financial_account_id,f.to_financial_account_id,f.total_amount,f.is_payment,f.status_id,f.payment_processor_id,f.payment_instrument_id from civicrm_contribution c join civicrm_entity_financial_trxn e on e.entity_id = c.id  join civicrm_financial_trxn f on f.id = e.financial_trxn_id  where c.id =98;
+-----+------------------------+-----------+-------------------+----------+-----+---------------------------+-------------------------+--------------+------------+-----------+----------------------+-----------------------+
| id  | entity_table           | entity_id | financial_trxn_id | amount   | id  | from_financial_account_id | to_financial_account_id | total_amount | is_payment | status_id | payment_processor_id | payment_instrument_id |
+-----+------------------------+-----------+-------------------+----------+-----+---------------------------+-------------------------+--------------+------------+-----------+----------------------+-----------------------+
| 205 | civicrm_financial_item |        98 |               101 | -57.00   | 101 |                         7 |                       6 | -57.00       |          1 |         7 |                 NULL |                     3 |
| 218 | civicrm_contribution   |        98 |               108 | 1000.00  | 108 |                      NULL |                       7 | 1000.00      |          0 |         1 |                 NULL |                     4 |
| 219 | civicrm_contribution   |        98 |               109 | 100.00   | 109 |                         7 |                       6 | 100.00       |          1 |         1 |                 NULL |                     4 |
| 222 | civicrm_contribution   |        98 |               110 | 800.00   | 110 |                         7 |                       6 | 800.00       |          1 |         1 |                 NULL |                     4 |
| 224 | civicrm_contribution   |        98 |               111 | 100.00   | 111 |                         7 |                       6 | 100.00       |          1 |         1 |                 NULL |                     4 |
| 226 | civicrm_contribution   |        98 |               112 | -1000.00 | 112 |                         7 |                       6 | -1000.00     |          1 |         1 |                 NULL |                     4 |
| 228 | civicrm_contribution   |        98 |               113 | 1000.00  | 113 |                      NULL |                      12 | 1000.00      |          1 |         1 |                    1 |                     1 |
+-----+------------------------+-----------+-------------------+----------+-----+---------------------------+-------------------------+--------------+------------+-----------+----------------------+-----------------------+
7 rows in set (0.00 sec)

The last record is the false Credit Card Payment method

[eileenmcnaughton]

@kcristiano thanks - I will try to replicate that in a unit test

[eileenmcnaughton]

@kcristiano although you are commenting on this PR I assume you are using the 3 prs together?

[kcristiano]

@eileenmcnaughton I had not included #14763 But let me do that and retest. I did not realize that this was dependent on that PR,I though only the reverse.

[kcristiano]

Ok Tested with all 3 (#14589 is merged) Still getting issue with a credit card line being inserted

mysql root@localhost:wpcvmastercivi_olond> select e.*,f.id,f.from_financial_account_id,f.to_financial_account_id,f.total_amount,f.is_payment,f.status_id,f.payment_processor_id,
                                        -> f.payment_instrument_id from civicrm_contribution c join civicrm_entity_financial_trxn e on e.entity_id = c.id  join civicrm_financia
                                        -> l_trxn f on f.id = e.financial_trxn_id  where c.id =95;
+------+----------------------+-------------+---------------------+----------+------+-----------------------------+---------------------------+----------------+--------------+-------------+------------------------+-------------------------+
|   id | entity_table         |   entity_id |   financial_trxn_id |   amount |   id |   from_financial_account_id |   to_financial_account_id |   total_amount |   is_payment |   status_id |   payment_processor_id |   payment_instrument_id |
|------+----------------------+-------------+---------------------+----------+------+-----------------------------+---------------------------+----------------+--------------+-------------+------------------------+-------------------------|
|  187 | civicrm_contribution |          95 |                  94 |     1000 |   94 |                      <null> |                         7 |           1000 |            0 |           1 |                 <null> |                       4 |
|  188 | civicrm_contribution |          95 |                  95 |      100 |   95 |                           7 |                         6 |            100 |            1 |           1 |                 <null> |                       4 |
|  191 | civicrm_contribution |          95 |                  96 |      400 |   96 |                           7 |                         6 |            400 |            1 |           1 |                 <null> |                       4 |
|  193 | civicrm_contribution |          95 |                  97 |      500 |   97 |                           7 |                         6 |            500 |            1 |           1 |                 <null> |                       4 |
|  195 | civicrm_contribution |          95 |                  98 |    -1000 |   98 |                           7 |                         6 |          -1000 |            1 |           1 |                 <null> |                       4 |
|  197 | civicrm_contribution |          95 |                  99 |     1000 |   99 |                      <null> |                        12 |           1000 |            1 |           1 |                      1 |                       1 |
+------+----------------------+-------------+---------------------+----------+------+-----------------------------+---------------------------+----------------+--------------+-------------+------------------------+-------------------------+
6 rows in set

[eileenmcnaughton]

@kcristiano they are all independent of each other to some extent - but you can't see the impact of them from additional payment form without the combo deal

[eileenmcnaughton]

@kcristiano I should also note that 'success' / mergeable for this PR is 'not worse' - ie it's a refactoring PR that changes the code path. It is the other PR that is expected to improve behaviour. This PR will simplify code and greatly improve test coverage

[eileenmcnaughton]

OK I think this is what you are describing -

I'm now a little unsure about the best order to proceed with things.

I've encountered a specific issue with the financial items - namely what should happen when the payment exceeds the line items total. This can happen when
a) the price set selections are altered, downwards and
b) an overpayment is received.
b) A payment is updated - in effect this means it is cancelled & a new financial_trxn created

This means that the line item total is less than the contribution total

There are 2 options

1. the overage is allocated proportionally across the line items - so that the amount paid is equal to the sum of the financial items for line items for the given contribution or
2. the overage is not allocated and the sum of the financial items for the line items for the contribution is less than the amount paid against the contribution
3. we continue to do different things in different places

The former is locked in & tested in UpdatePaymentTest whereas the tests @monishdeb wrote for dev/core#310 assume the latter is the case

One for @JoeMurray @monishdeb @pradpnayak to have thoughts on...

[eileenmcnaughton]

@kcristiano I realise that this pr #14673 was the one I thought might have a fix. I tested and it does fix the bug you identified

I think #14673 is a pretty safe one to merge as it does not affect many existing code paths whereas this does consolidate paths.

It would be good if @monishdeb and / or @pradpnayak could check that one as I think we could get it merged & simplify things while we discuss my issue above. In the meantime I've pulled it into this PR

[eileenmcnaughton]

This actually incorporates #14673 so to progress we need @pradpnayak or @monishdeb to review this PR (combining the 2) or just that PR & we can merge this PR after

[kcristiano]

@eileenmcnaughton can you let me know what I can to move these PRs along? If I should do mre r-run on them, let me know and I'll set up the testing that can duplicate the issuesthat these are trying to solve.

I have this PR, #14763 and #14892 as a group - is that correct?

[eileenmcnaughton]

@kcristiano yes this one needs someone who gets the code to take a look - I know you have done some testing & we have some tests too - I was hoping @monishdeb or @pradpnayak would have time (@JoeMurray ) but we seem to be struggling on availablility.

The other issue stalling us is clarification of which of the 2 methods currently in use in different places for recording overage we should use (#14408 (comment) is about this). Since we have tests locking in each of the 2 variants (proportionally assigning overages & not assigning overages) it's a blocker to consolidation until we can confirm which is right - I need @JoeMurray to agree here

[eileenmcnaughton]

@kcristiano so with the other PR merged I think this could be merged if your testing reveals no regressions - it still does not fix the allocation issue and I need @JoeMurray to answer the question above (#14408 (comment)) to deal with that.

As an aside I found out @pradpnayak no longer gets github pings for some reason - perhaps that affects Joe too?

[JoeMurray]

So let me try to articulate some objectives:

When the sum of payments exceeds the line items total, then the result should be that there is a full allocation for each of the line items for the amount that they are worth, and the overage is not allocated. I think that when JMA and @lcdservices implemented the overpayment support we likely missed handling of UpdatePaymentTest, or perhaps it was added later.

[JoeMurray]

@monishdeb please spend up to 3h on reviewing / QA r-run for this set of linked PRs tomorrow.

[JoeMurray]

In any case where there is an overpayment (due to extra payment, overpayment, reduction of line item total below what has been paid, edit of payment amount, ), then I believe core is supposed to from a bookkeeping perspective have the money end up in the right bank accounts (or other asset accounts), and leave the excess revenue as a negative amount for Accounts Receivable. Refunds either in core or extensions and / or Credit Notes, and applying credit notes to outstanding amounts by that contact or another (via extension) work on this excess to bring it back to zero. @lcdservices may want to correct me, but I think that is how things are supposed to work.

[lcdservices]

I agree with @JoeMurray

[eileenmcnaughton]

@JoeMurray @lcdservices I went back to the unit test for UpdatePaymentTest and annotated the behaviour it is locking in - #15143

I read from your comments above that the goal is to allocate the payments to the line items. However, looking at the test I see it starts off by creating a partially paid contribution & then allocating the Accounts receivable amount, rather than the paid amount, to the line items.

I had previously understood the goal to be to determine which items (or what proportion of them) are actually PAID for rather than intended to be paid for.

I'm pretty sure something is wrong with the final resulting data documented over at #15143 but I'm a little unsure what since it's hard to see what 'right' would look like from the resulting data.

[monishdeb]

As per PR description, it indicates that it doesn't fix the misallocation of line-items (which is handled in a separate PR #14763) but it actually focuses on improving the workflow about handling the additional payment eventually to Payment.create API from BAO function recordAdditionalPayment when a refund/owed amount is submitted. I have reviewed the patch and doesn't cause any regression or bring any unexpected behavior on paying refund/owed amount.

I am happy to merge this PR after code review and r-run.

[eileenmcnaughton]

woot - this was a big code consolidation to get to this point but we now have less (I want to say only one but I'm not sure) ways of processing an additional payment