CRM-19594 further fix on creating correct line items for memberships

[eileenmcnaughton]

• CRM-19594: Wrong Membership Updated

[eileenmcnaughton]

This is a follow to issues identified by @davejenx on #9390 - I think it fixes some but not ALL of the identified problems. It locks in the ones it does fix with unit tests and if it passes I will merge and continue to try to ensure all scenarios are fixed. The one I suspect is not fixes it when a price set has more than one membership line

[eileenmcnaughton]

@davejenx I'm merging this to address at least some of the items you identified - can you re-test them?

[davejenx]

I started testing this PR, then found that there's another one, #9449 . Anyway here are the results I got for #9444 - they don't cover all the cases I tested on #9390 because I discovered #9449 mid test so will move onto that.

Testing with PR 9444, after it was committed, Thu 24 Nov

• checked out branch 4.7.14-rc
• HEAD = 017e412

Summary
Looks OK as far as I got, didn't test (explicit) price sets, query about line item for additional contribution with implicit/quick config price set.

Adjusted dates of previous memberships to start 04/01/2015, end 03/31/2016 -> expired

7. Member Signup and Renewal contrib page (no price set) as anon, no additional contrib entered, test payment processor.

Created:

1 contact, id 210

1 membership, id 103

2 membership_log:
mysql> select * from civicrm_membership_log where id > 53;
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
| id | membership_id | status_id | start_date | end_date | modified_id | modified_date | membership_type_id | max_related |
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
| 54 | 103 | 5 | NULL | NULL | 210 | 2016-11-24 | 1 | NULL |
| 55 | 103 | 1 | 2016-11-24 | 2018-11-23 | 210 | 2016-11-24 | 1 | NULL |
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
2 rows in set (0.00 sec)

• OK

1 contribution, id 101

1 membership_payment:
mysql> select * from civicrm_membership_payment where id > 38;
+----+---------------+-----------------+
| id | membership_id | contribution_id |
+----+---------------+-----------------+
| 39 | 103 | 101 |
+----+---------------+-----------------+
1 row in set (0.00 sec)

• OK

1 line_item:
mysql> select * from civicrm_line_item where contribution_id = 101\G
*************************** 1. row ***************************
id: 105
entity_table: civicrm_membership
entity_id: 103
contribution_id: 101
price_field_id: 5
label: General
qty: 1.00
unit_price: 100.00
line_total: 100.00
participant_count: 0
price_field_value_id: 10
financial_type_id: 2
non_deductible_amount: 0.00
tax_amount: NULL
1 row in set (0.00 sec)

• OK

financial_trxn:
mysql> select * from civicrm_financial_trxn where id > 105\G
*************************** 1. row ***************************
id: 106
from_financial_account_id: NULL
to_financial_account_id: 12
trxn_date: 2016-11-24 14:11:45
total_amount: 100.00
fee_amount: 0.00
net_amount: 100.00
currency: USD
is_payment: 1
trxn_id: live_7_5836e712531bc
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: 1
check_number: NULL
*************************** 2. row ***************************
id: 107
from_financial_account_id: 12
to_financial_account_id: 5
trxn_date: 2016-11-24 14:11:45
total_amount: 1.50
fee_amount: 0.00
net_amount: 0.00
currency: USD
is_payment: 1
trxn_id: live_7_5836e712531bc
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: NULL
check_number: NULL
2 rows in set (0.00 sec)

Checking other memberships:
mysql> select id, status_id, start_date, end_date, membership_type_id from civicrm_membership where id > 95; +-----+-----------+------------+------------+--------------------+
| id | status_id | start_date | end_date | membership_type_id |
+-----+-----------+------------+------------+--------------------+
| 96 | 4 | 2015-04-01 | 2016-03-31 | 1 |
| 97 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 98 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 99 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 100 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 101 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 102 | 4 | 2015-04-01 | 2016-03-31 | 4 |
| 103 | 1 | 2016-11-24 | 2018-11-23 | 1 |
+-----+-----------+------------+------------+--------------------+
8 rows in set (0.00 sec)

• OK

Adjusted dates of membership id 103 to start 04/01/2015, end 03/31/2016 -> expired

8. Member Signup and Renewal contrib page (no price set) + additional contrib as anon, test payment processor.

"Separate Membership Payment" not ticked in contrib page settings.

Created:

1 contact, id 211

1 membership, id 104

2 membership_log:
mysql> select * from civicrm_membership_log where id > 56;
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
| id | membership_id | status_id | start_date | end_date | modified_id | modified_date | membership_type_id | max_related |
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
| 57 | 104 | 5 | NULL | NULL | 211 | 2016-11-24 | 1 | NULL |
| 58 | 104 | 1 | 2016-11-24 | 2018-11-23 | 211 | 2016-11-24 | 1 | NULL |
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
2 rows in set (0.00 sec)

• OK

1 contribution, id 102

1 membership_payment:
mysql> select * from civicrm_membership_payment where id > 39;
+----+---------------+-----------------+
| id | membership_id | contribution_id |
+----+---------------+-----------------+
| 40 | 104 | 102 |
+----+---------------+-----------------+
1 row in set (0.00 sec)

• OK

1 line_item:
mysql> select * from civicrm_line_item where contribution_id = 102\G
*************************** 1. row ***************************
id: 106
entity_table: civicrm_membership
entity_id: 104
contribution_id: 102
price_field_id: 5
label: General
qty: 1.00
unit_price: 105.00
line_total: 105.00
participant_count: 0
price_field_value_id: 10
financial_type_id: 2
non_deductible_amount: 0.00
tax_amount: NULL
1 row in set (0.00 sec)
- QUERY: I thought there might be 2 line items: 1 for membership and 1 for additional amount. However "Separate Membership Payment" was not ticked in contrib page settings. Help text for that says "Check this box if you are including both Membership Signup/Renewal AND a Contribution Amount section, AND you want the membership fee to be handled as a separate transaction." So there's ambiguity about whether single transaction implies single line item.
This is an improvement from when the same scenario was tested with #9390, when the $5 additional contribution was not recorded at all in a line item.

2 financial_trxn:
mysql> select * from civicrm_financial_trxn where id > 107\G
*************************** 1. row ***************************
id: 108
from_financial_account_id: NULL
to_financial_account_id: 12
trxn_date: 2016-11-24 14:30:27
total_amount: 105.00
fee_amount: 0.00
net_amount: 105.00
currency: USD
is_payment: 1
trxn_id: live_8_5836eb73cebc6
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: 1
check_number: NULL
*************************** 2. row ***************************
id: 109
from_financial_account_id: 12
to_financial_account_id: 5
trxn_date: 2016-11-24 14:30:27
total_amount: 1.50
fee_amount: 0.00
net_amount: 0.00
currency: USD
is_payment: 1
trxn_id: live_8_5836eb73cebc6
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: NULL
check_number: NULL
2 rows in set (0.00 sec)

Checking other memberships:
mysql> select id, status_id, start_date, end_date, membership_type_id from civicrm_membership where id > 95; +-----+-----------+------------+------------+--------------------+
| id | status_id | start_date | end_date | membership_type_id |
+-----+-----------+------------+------------+--------------------+
| 96 | 4 | 2015-04-01 | 2016-03-31 | 1 |
| 97 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 98 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 99 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 100 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 101 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 102 | 4 | 2015-04-01 | 2016-03-31 | 4 |
| 103 | 4 | 2015-04-01 | 2016-03-31 | 1 |
| 104 | 1 | 2016-11-24 | 2018-11-23 | 1 |
+-----+-----------+------------+------------+--------------------+
9 rows in set (0.00 sec)

• OK

Adjusted dates of membership id 104 to start 04/01/2015, end 03/31/2016 -> expired

9. Member Signup and Renewal contrib page (no price set) + additional contrib as anon, test payment processor.

"Separate Membership Payment" ticked in contrib page settings.

Created:

1 contact, id 212

1 membership, id 105

2 membership_log:
mysql> select * from civicrm_membership_log where id > 59;
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
| id | membership_id | status_id | start_date | end_date | modified_id | modified_date | membership_type_id | max_related |
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
| 60 | 105 | 5 | NULL | NULL | 212 | 2016-11-24 | 1 | NULL |
| 61 | 105 | 1 | 2016-11-24 | 2018-11-23 | 212 | 2016-11-24 | 1 | NULL |
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
2 rows in set (0.00 sec)

• OK

2 contributions, id 103 & 104

1 membership_payment:
mysql> select * from civicrm_membership_payment where id > 40;
+----+---------------+-----------------+
| id | membership_id | contribution_id |
+----+---------------+-----------------+
| 41 | 105 | 104 |
+----+---------------+-----------------+
1 row in set (0.00 sec)

• OK

2 line_items, 1 per contribution:
mysql> select * from civicrm_line_item where contribution_id in (103, 104)\G
*************************** 1. row ***************************
id: 107
entity_table: civicrm_contribution
entity_id: 103
contribution_id: 103
price_field_id: 1
label: Contribution Amount
qty: 1.00
unit_price: 5.00
line_total: 5.00
participant_count: NULL
price_field_value_id: 1
financial_type_id: 1
non_deductible_amount: 0.00
tax_amount: NULL
*************************** 2. row ***************************
id: 108
entity_table: civicrm_membership
entity_id: 105
contribution_id: 104
price_field_id: 5
label: General
qty: 1.00
unit_price: 100.00
line_total: 100.00
participant_count: 0
price_field_value_id: 10
financial_type_id: 2
non_deductible_amount: 0.00
tax_amount: NULL
2 rows in set (0.01 sec)

• OK

4 financial_trxn:
mysql> select * from civicrm_financial_trxn where id > 109\G
*************************** 1. row ***************************
id: 110
from_financial_account_id: NULL
to_financial_account_id: 12
trxn_date: 2016-11-24 14:59:03
total_amount: 100.00
fee_amount: 0.00
net_amount: 100.00
currency: USD
is_payment: 1
trxn_id: live_9_5836f227759f9
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: 1
check_number: NULL
*************************** 2. row ***************************
id: 111
from_financial_account_id: 12
to_financial_account_id: 5
trxn_date: 2016-11-24 14:59:03
total_amount: 1.50
fee_amount: 0.00
net_amount: 0.00
currency: USD
is_payment: 1
trxn_id: live_9_5836f227759f9
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: NULL
check_number: NULL
*************************** 3. row ***************************
id: 112
from_financial_account_id: NULL
to_financial_account_id: 12
trxn_date: 2016-11-24 14:59:03
total_amount: 5.00
fee_amount: 0.00
net_amount: 5.00
currency: USD
is_payment: 1
trxn_id: live_9_5836f227b57e4
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: 1
check_number: NULL
*************************** 4. row ***************************
id: 113
from_financial_account_id: 12
to_financial_account_id: 5
trxn_date: 2016-11-24 14:59:03
total_amount: 1.50
fee_amount: 0.00
net_amount: 0.00
currency: USD
is_payment: 1
trxn_id: live_9_5836f227b57e4
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: NULL
check_number: NULL
4 rows in set (0.00 sec)

Checking other memberships:
mysql> select id, status_id, start_date, end_date, membership_type_id from civicrm_membership where id > 95; +-----+-----------+------------+------------+--------------------+
| id | status_id | start_date | end_date | membership_type_id |
+-----+-----------+------------+------------+--------------------+
| 96 | 4 | 2015-04-01 | 2016-03-31 | 1 |
| 97 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 98 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 99 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 100 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 101 | 4 | 2015-04-01 | 2016-03-31 | 2 |
| 102 | 4 | 2015-04-01 | 2016-03-31 | 4 |
| 103 | 4 | 2015-04-01 | 2016-03-31 | 1 |
| 104 | 4 | 2015-04-01 | 2016-03-31 | 1 |
| 105 | 1 | 2016-11-24 | 2018-11-23 | 1 |
+-----+-----------+------------+------------+--------------------+
10 rows in set (0.00 sec)

• OK

[eileenmcnaughton]

Thanks Dave - I merged the PR's so that they would all be in http://dist.civicrm.org/by-date/latest/4.7.14-rc/ -

Scenario 8 seems wrong I think

[eileenmcnaughton]

Hmm - just thinking about scenario 8 - I think maybe it's a 'feature' not a bug. I suspect it has always been like that, and perhaps some people would argue it should be that way, and unless I can find some reason to thin otherwise I'm not going to treat it as a bug (ie. a candidate to be added to the rc, or for me to prioritise attention to).

[davejenx]

I've been testing 4.6 to see if CRM-19594 occurred - it didn't, see my comment on JIRA.

Re scenario 8, the behaviour in 4.6 with "Separate Membership Payment" not ticked in contrib page settings was the same: a single line item with amount = membership + additional contribution. So this doesn't seem to be a newly-introduced issue. My take is that "Separate Membership Payment" comes from an era before line items, offering a choice between creating one contribution or two: two whole contributions (+ fees) from one contrib page submission, crikey! Now that we have line items, "Separate Membership Payment" seems like a relic and you could argue it's no longer needed and we should just create one contribution with two line items - that seems the obvious behaviour. Currently we offer the choice of one contribution with one line item or two contributions, each with one line item. But I think this is for another day.

[eileenmcnaughton]

Actually 'separate membership payment' creates 2 actual payments - I believe this is useful in some cases for differentiating between a charitable payment & a membership payment at the payer's end. I don't fully understand the use case @GinkgoFJG might...

[universalhandle]

I think the above mention was meant for @ginkgomzd, but I might be able to answer this anyway: as I understand it, "separate membership payment" does intentionally create two payments. I believe this occurs all the way through the stack, so to speak, meaning that you will find two transactions in your payment processor as well as two contribution records in CiviCRM.

As for the usefulness of this "feature," I believe the idea is that it makes it easier to distinguish between membership payments, which may or may not be tax-deductible, and additional donations, which typically are. (There may be other reasons to track them separately as well.) I wonder if this is vestigial from before we had line items; it seems that nowadays you could have a single contribution with one line item associated with financial type X and another with financial type Y. There may be some questions about whether or not line item reporting is as advanced as contribution reporting, but aside from those questions I think it's hard to justify the continued existence of the feature.

[davejenx]

@GinkgoFJG Those were exactly my thoughts at #9444 (comment) .

[universalhandle]

@davejenx: Whoops, didn't read the whole thread! I think with an audit of sorts of the line items reporting capabilities would need to occur before we dropped the feature, but I think it's a bit more complex than it seems. I seem to recall that contributions have a financial type, too -- we probably should eliminate that if we're definitively moving the classification to line items.

[davejenx]

@GinkgoFJG Agreed that reporting would need to be audited/improved before dropping this feature. Yes, contributions have financial_type_id too. There was discussion on the dev list in April re possibly getting rid of this, in thread "What sort of leap is this?". It's a very long thread. :-)

[universalhandle]

Yes, that's what I was thinking of.

[eileenmcnaughton]

There is also some REALLY funky behaviour around recurring with separate payment. I am not sure I understand it but it is USED by some small number of sites

[universalhandle]

I'm not sure that it's working as intended. One of my clients has had issues with this. The idea is: sign up for a recurring membership and make a one-time gift. The implementation... well, not so much.

Maybe @eileenmcnaughton's point, however, is that we aren't quite ready to retire the separate membership payments feature, because there's no such thing as a recurring line item -- recurrence is handled at the contribution level. However, if the feature doesn't work under the existing architecture, then I'd argue that it shouldn't hold up re-architecture.

Really there's no reason users should have to check that config to handle this use case. If the conditions are "right" -- i.e., a membership registration is set to recur, and a second price field has a non-zero value -- the code could branch and create the additional contribution.

[eileenmcnaughton]

"The idea is: sign up for a recurring membership and make a one-time gift."

We have a lot of demand for variants of this - once off membership + recurring contribution & vice versa - monthly gift & annual membership recurring - there is a world of pain in here

[universalhandle]

Yup. If I recall correctly, combining recurrences is a prohibited configuration (e.g., you can't configure a form for auto-renewing memberships and a recurring donation). That's one way to deal with it.

¯_(ツ)_/¯

[twomice]

FYI, this appears to have caused the regression described in https://issues.civicrm.org/jira/browse/CRM-20172/. I'm working on a fix now. Any comments there are most welcome.

[eileenmcnaughton]

Thanks @twomice - you know what I'm going to say don't you.... let's lock in every fix with a test

[twomice]

@eileenmcnaughton Yes, that's also how we sold it to the sponsor: we're going to fix it with tests so it doesn't. regress. evarrr. (Well, anyway, less likely to regress.)

I'll probably have questions about how to test this, so you won't mind if I @ you in the WIP PR -- right?

[eileenmcnaughton]

no worries