CRM-19594 Wrong membership updated #9390

eileenmcnaughton · 2016-11-15T12:32:22Z

CRM-19594: Wrong Membership Updated

eileenmcnaughton · 2016-11-15T12:32:55Z

CRM/Contribute/Form/Contribution/Confirm.php

+   * @param $isEmailReceipt
+   * @return mixed
+   */
+  public static function handlePledge(&$form, $params, $contributionParams, $pledgeID, $contribution, $isEmailReceipt) {


This is all just an extraction - moving a chunk of code out of the main function

eileenmcnaughton · 2016-11-15T12:33:23Z

CRM/Contribute/Form/Contribution/Confirm.php

@@ -1445,6 +1460,7 @@ protected function postProcessMembership(
        }
      }

+      $membershipParams['skipLineItem'] = 1;


ie don't add line items while doing contribution

eileenmcnaughton · 2016-11-15T12:33:41Z

CRM/Member/BAO/Membership.php

@@ -321,6 +321,9 @@ public static function create(&$params, &$ids, $skipRedirect = FALSE, $activityT
      );
    }

+    if (empty($params['line_item']) && !empty($params['membership_type_id']) && empty($params['skipLineItem'])) {


Moved here from api

eileenmcnaughton · 2016-11-15T12:33:58Z

CRM/Member/BAO/Membership.php

+        $membership->id,
+        $params['line_item'],
+        CRM_Utils_Array::value('contribution', $params),
+        'civicrm_membership'


pass in entity

I guess we should check if this does something wierd with a multi-row set

eileenmcnaughton · 2016-11-15T12:35:20Z

CRM/Member/BAO/Membership.php

@@ -1835,48 +1844,20 @@ public static function renewMembership($contactID, $membershipTypeID, $is_test,
        // CRM-15475
        array_search('Cancelled', CRM_Member_PseudoConstant::membershipStatus(NULL, " name = 'Cancelled' ", 'name', FALSE, TRUE)),
      ))) {
-        $membership = new CRM_Member_DAO_Membership();


Instead of bypassing membership.create call it with basic params, including the contribution

eileenmcnaughton · 2016-11-15T12:36:18Z

@ginkgomzd I was able to replicate the issue in a test - this fixes the test in the 2 scenarios covered. Really we want more of them..

There are several but I have only touched the one where I was already working on the test coverage

eileenmcnaughton · 2016-11-16T04:22:25Z

@ginkgomzd I'll need you to QA this before I can merge it. I'm worried about what happens to the line items under different combos - I have only ensured memberships are good

eileenmcnaughton · 2016-11-16T04:22:48Z

sorry - single item membership pricesets

MegaphoneJon · 2016-11-16T16:27:51Z

Per the request of @eileenmcnaughton, I'm also taking a look at this to see if I can extend test coverage to handle the other cases she mentioned - but this is a lot to wrap my head around. I'll report in in a few hours if I've made any progress.

eileenmcnaughton · 2016-11-16T18:47:11Z

test this please

MegaphoneJon · 2016-11-16T20:57:17Z

I tried extending the test api_v3_ContributionPageTest::testSubmitMembershipBlockIsSeparatePayment() to test for this bug as well. However, it seems that before I even modify the test, it's creating two memberships but only one line item (for the membership, not the donation).

I tested that by adding these 3 lines to the bottom of the test:

    CRM_Core_Error::debug_var('contributions', $contributions);
    $lineItems = $this->callAPISuccess('line_item', 'get', array());//, array('contribution_id.contribution_page_id' => $this->_ids['contribution_page']));
    CRM_Core_Error::debug_var('lineItems', $lineItems);

If someone confirms that this is what's happening, it seems like it's not really possible to write assertions that pass in this test without fixing that bug first.

KarinG · 2016-11-17T02:30:01Z

Just added this to Mattermost - but pasting it bottom-line of that here as well:

I just had a look at public function testSubmitMembershipBlockIsSeparatePayment() { and:

a) good news is that on a new civibuild dmaster via GUI - doing separate Membership Payment + Contribution produces: 2 entries in the civicrm_contribution and 2 entries in civicrm_line_item; so it's working as advertised.

However b) when I run this test however - I get: 2 correct entries in the civicrm_contribution and only get a line_item entry for the Membership Amount and it's $1 (and it should be $2). So this test is quite broken - at least on the contribution line_item level. The actual CiviCRM code is correct though - it's really just the test that's failing; the way in which it creates the Contributions does not mimic how in the real code base these are (correctly) created.

…really)

…ership

KarinG · 2016-11-29T03:00:21Z

civicrm_contribution looks good:

| 3581 | 7 | 1 | 19 | 1 | 2016-11-28 21:42:19 | NULL | 3.33 | 0.00 | 3.33 | A9535988:1480387340 | c0c497b9d997690fb413fa32cd47e5e2-2 | CAD | NULL | 0 | 2016-11-28 21:42:21 | NULL | Online Contribution:

| 3582 | 7 | 2 | 19 | 1 | 2016-11-28 21:42:20 | NULL | 10.00 | 0.00 | 10.00 | A9535986:1480387339 | 35a0de91f22255b56427a0e79a123304 | CAD | NULL | 0 | 2016-11-28 21:42:21 | NULL | Online Contribution:

civicrm_line_item does not: 3582: was $10 - civicrm_line_item for contribution_id = 3582 has $3.33 - and has it as an add on Donation - financial_type id = 1 (Donation) should be Member Dues.

MariaDB [civicrm_semperit_com]> select * from civicrm_line_item where contribution_id=3581;
| 3872 | civicrm_contribution | 3581 | 3581 | 1 | Contribution Amount | 1.00 | 3.33 | 3.33 | NULL | 1 | 1 | 0.00 | NULL |
+------+----------------------+-----------+-----------------+----------------+---------------------+------+------------+------------+-------------------+----------------------+-------------------+-----------------------+------------+
1 row in set (0.00 sec)

MariaDB [civicrm_semperit_com]> select * from civicrm_line_item where contribution_id=3582;
| 3873 | civicrm_contribution | 3582 | 3582 | 63 | add on Donation | 1.00 | 3.33 | 3.33 | 0 | 114 | 1 | 0.00 | NULL |
+------+----------------------+-----------+-----------------+----------------+-----------------+------+------------+------------+-------------------+----------------------+-------------------+-----------------------+------------+
1 row in set (0.00 sec)

KarinG · 2016-11-29T03:01:15Z

Let me know if this is helpful and/or if you'd like me to look at other bits.

KarinG · 2016-11-29T03:04:20Z

And the receipt is completely wrong:

Receipt 1: $3.33
Receipt 2: $20: note it's adding up transaction A9535986:1480387339 and A9535986:1480387339 (effectively counting it twice)

KarinG · 2016-11-29T03:06:53Z

For sake of completeness: monies in the bank of iATS: total: $13.33:

eileenmcnaughton · 2016-11-29T03:21:23Z

@KarinG how are you transacting this? Contribution page configured with membership + separate payment?

eileenmcnaughton · 2016-11-29T03:21:29Z

Or price set?

KarinG · 2016-11-29T03:24:02Z

eileenmcnaughton · 2016-11-29T03:25:00Z

So, you have configured it as a price set on a contribution page?

KarinG · 2016-11-29T03:26:31Z

No in the Configuration it's:

Memberships tab: Membership Section Enabled and
Separate Membership Payment checked
Amounts tab: Contribution Amounts Enabled:
Fixed Contribution Options:
add on Donation $3.33

KarinG · 2016-11-29T03:38:27Z

So not using any Price Sets [not in the Memberships tab and not in the Contribution tab of the Contribution Page config]

eileenmcnaughton · 2016-11-29T03:41:22Z

Whoever added that separate payment option has a lot to answer for - I swear it has cost us high-five digits over the years!

KarinG · 2016-11-29T03:46:03Z

I am successfully using it for a 4.6 project though: College of HCP: Membership fee + Late fee (and I have some code that then automatically decides the correct Late fee based on date/time and hides the other options) - and there it's working on a Contribution and Receipting level [not worried about line_items or financial_items for this project];

KarinG · 2016-11-29T03:48:10Z

Not even sure if this PR broke that - or if this is completely different.

eileenmcnaughton · 2016-11-29T03:49:46Z

It could well be different - but I'll look anyway

KarinG · 2016-11-29T03:51:53Z

The receipt issue for sure is a regression [something that was working in 4.6] - but this may not be the issue here or caused by this PR. Every time I think I understand a few more things about CiviContribute I prove myself wrong...

eileenmcnaughton · 2016-11-29T11:33:13Z

@KarinG I have merged a fix into the rc - hopefully it works now....

davejenx · 2016-11-29T16:22:12Z

Just checked my previous testing notes and I hadn't re-tested with "Separate Membership Payment" ticked after applying #9449, so it looks like that re-broke it. Have now done that test, with #9449 but without #9464 and confirmed @KarinG's results:

Re-testing #9449 without #9464

Member Signup and Renewal contrib page (no price set) + additional contrib as anon, test payment processor.
"Separate Membership Payment" ticked in contrib page settings.

Summary
membership, membership_log, membership_payment OK;
line_items: BROKEN on several counts:

line_total: both have $5, should be one $50 and one $5.
label: one should be Student but we have Contribution Amount + Additional contribution
entity_table: one should have 'civicrm_membership' but both have 'civicrm_contribution'
financial_type_id: both are 1 = Donation, one should be 2 = Member Dues

Testing with latest RC branch code, #9464 merged

Member Signup and Renewal contrib page (no price set) + additional contrib as anon, test payment processor.
"Separate Membership Payment" ticked in contrib page settings.

Summary
Looks good. :-)

Gory details
Created:

1 contact, id 218

1 membership, id 114

2 membership_log:
mysql> select * from civicrm_membership_log where id > 84;
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
| id | membership_id | status_id | start_date | end_date | modified_id | modified_date | membership_type_id | max_related |
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
| 85 | 114 | 5 | NULL | NULL | 218 | 2016-11-29 | 2 | NULL |
| 86 | 114 | 1 | 2016-11-29 | 2017-11-28 | 218 | 2016-11-29 | 2 | NULL |
+----+---------------+-----------+------------+------------+-------------+---------------+--------------------+-------------+
2 rows in set (0.00 sec)

OK

2 contributions, id 112 & 113

1 membership_payment:
mysql> select * from civicrm_membership_payment where contribution_id in (112, 113);
+----+---------------+-----------------+
| id | membership_id | contribution_id |
+----+---------------+-----------------+
| 48 | 114 | 113 |
+----+---------------+-----------------+
1 row in set (0.00 sec)

OK

2 line_items:
mysql> select * from civicrm_line_item where contribution_id in (112, 113) order by contribution_id, id\G
*************************** 1. row ***************************
id: 123
entity_table: civicrm_contribution
entity_id: 112
contribution_id: 112
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: 124
entity_table: civicrm_membership
entity_id: 114
contribution_id: 113
price_field_id: 5
label: Student
qty: 1.00
unit_price: 50.00
line_total: 50.00
participant_count: 0
price_field_value_id: 11
financial_type_id: 2
non_deductible_amount: 0.00
tax_amount: NULL
2 rows in set (0.00 sec)

OK.

4 financial_trxn:
mysql> select * from civicrm_financial_trxn where trxn_date >= '2016-11-29 16:40'\G
*************************** 1. row ***************************
id: 128
from_financial_account_id: NULL
to_financial_account_id: 12
trxn_date: 2016-11-29 16:40:31
total_amount: 50.00
fee_amount: 0.00
net_amount: 50.00
currency: USD
is_payment: 1
trxn_id: live_10_583da16f89f4b
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: 1
check_number: NULL
*************************** 2. row ***************************
id: 129
from_financial_account_id: 12
to_financial_account_id: 5
trxn_date: 2016-11-29 16:40:31
total_amount: 1.50
fee_amount: 0.00
net_amount: 0.00
currency: USD
is_payment: 1
trxn_id: live_10_583da16f89f4b
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: NULL
check_number: NULL
*************************** 3. row ***************************
id: 130
from_financial_account_id: NULL
to_financial_account_id: 12
trxn_date: 2016-11-29 16:40:31
total_amount: 5.00
fee_amount: 0.00
net_amount: 5.00
currency: USD
is_payment: 1
trxn_id: live_10_583da16fbf1c7
trxn_result_code: NULL
status_id: 1
payment_processor_id: 1
payment_instrument_id: 1
check_number: NULL
*************************** 4. row ***************************
id: 131
from_financial_account_id: 12
to_financial_account_id: 5
trxn_date: 2016-11-29 16:40:31
total_amount: 1.50
fee_amount: 0.00
net_amount: 0.00
currency: USD
is_payment: 1
trxn_id: live_10_583da16fbf1c7
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 | contact_id +-----+------------+ | 96 | 202 | | 97 | 204 | | 98 | 205 | | 99 | 206 | | 100 | 207 | | 101 | 209 | | 102 | 209 | | 103 | 210 | | 104 | 211 | | 105 | 212 | | 106 | 213 | | 107 | 214 | | 108 | 214 | | 109 | 215 | | 110 | 216 | | 111 | 217 | | 112 | 202 | | 113 | 200 | | 114 | 218 | +-----+------------+ 19 rows in set (0.05 sec) id, contact_id, status_id, start_date, end_date, membership_type_id from civicrm_membership where id >= 95;
-----------+------------+------------+--------------------+
| status_id | start_date | end_date | membership_type_id |
-----------+------------+------------+--------------------+
4 | 2015-04-01 | 2016-03-31 | 1 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 4 |
4 | 2015-04-01 | 2016-03-31 | 1 |
4 | 2015-04-01 | 2016-03-31 | 1 |
4 | 2015-04-01 | 2016-03-31 | 1 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 4 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 2 |
4 | 2015-04-01 | 2016-03-31 | 4 |
4 | 2015-04-01 | 2016-03-31 | 4 |
1 | 2016-11-29 | 2017-11-28 | 2 |
-----------+------------+------------+--------------------+

OK

KarinG · 2016-11-30T03:46:49Z

Ok - happy dance on Type = Member Dues now [for the Membership]:

KarinG · 2016-11-30T03:49:50Z

Email receipt still shows double/double:

KarinG · 2016-11-30T04:07:31Z

civicrm_contribution: fine - ids: 3584 $3.33 and 3585 $10
civicrm_line_item: happy dance here too:

davejenx · 2016-11-30T11:42:03Z

Hmm, I didn't receive email receipts so didn't see that issue. Something up with mail on localhost. :-(

litespeedmarc · 2016-11-30T12:48:01Z

For anyone still on or about to go live on 4.7.13 (and unable/unwilling to upgrade at the very last minute), is there a workaround or something that can be done to avoid this? It sounds like it happens when membership IDs & contribution IDs collide? Perhaps bump up the contribution ID sequence as a workaround?

davejenx · 2016-11-30T12:58:50Z

@litespeedmarc yes, the updating of wrong memberships happens when membership IDs & contribution IDs collide. Bumping up the contribution ID sequence would I think avoid this, as a workaround. I think line items could still get created with incorrect entity_ids even so, with entity_table = 'civicrm_membership' but with entity_id = the contribution id instead of the membership id. But if there's no membership with that id, at least it avoids also updating the wrong membership.

eileenmcnaughton · 2016-11-30T19:26:00Z

I think you might still get line item inaccuracies. I would encourage you to test the rc.

eileenmcnaughton · 2016-11-30T21:18:03Z

@KarinG @davejenx I'm starting to think the wrong email details on separate payment issue might be present in 4.7.13 rather than being a 4.7.14 regression - can either of you confirm that?

If so I'm inclined to NOT attempt to fix this for 4.7.14 but give it more time in 4.7.15 - the code seems particularly gnarly and if it's actually a NEW can of worms I might not open it just yet

CRM-19594 test for line items

51f8c59

civicrm-builder added the master label Nov 15, 2016

eileenmcnaughton commented Nov 15, 2016

View reviewed changes

eileenmcnaughton force-pushed the members branch from c3ec81e to dfcdd27 Compare November 15, 2016 20:45

eileenmcnaughton mentioned this pull request Nov 15, 2016

CRM-19594 Wrong Membership Updated #9349

Closed

eileenmcnaughton force-pushed the members branch from dfcdd27 to f325b96 Compare November 15, 2016 23:57

CRM-19594 Wrong membership updated

356bfca

eileenmcnaughton force-pushed the members branch from f325b96 to 9d8ef4b Compare November 16, 2016 01:54

CRM-19213 fix one instance of mis-setting payment_instrument_id.

f69a9ac

There are several but I have only touched the one where I was already working on the test coverage

eileenmcnaughton force-pushed the members branch from 9d8ef4b to f69a9ac Compare November 16, 2016 04:17

CRM-19213 fix another instance of mis-set payment_instrument_id

204efed

eileenmcnaughton force-pushed the members branch 3 times, most recently from 6759a09 to 4309bfd Compare November 16, 2016 09:24

CRM-19213 another instance of mis-set payment_instrument_id

16f3bd0

eileenmcnaughton force-pushed the members branch from 4309bfd to 16f3bd0 Compare November 16, 2016 09:27

CRM-19213 fix payment_instrument_id on event & add first test (shell …

632196e

…really)

eileenmcnaughton force-pushed the members branch from 604f8ea to 632196e Compare November 18, 2016 03:26

CRM-19594 extend test to ensure 2 line items created on separate memb…

417c683

…ership

eileenmcnaughton mentioned this pull request Nov 29, 2016

CRM-19594 fix line items for separate payments, non-price set #9464

Merged

eileenmcnaughton mentioned this pull request Nov 30, 2016

CRM-19621 fix address field to show country & state. #9466

Merged

This was referenced Dec 1, 2016

CRM-19298, (relates to testing on CRM-19621) fix receipts on separate… … #9474

Merged

Revert "CRM-19626 - Event Registration page allows registration even … #9477

Merged

mattwire mentioned this pull request Jan 11, 2017

Breaks all payment processors in 4.7.14: Function renewMembership() does not exist veda-consulting/uk.co.vedaconsulting.payment.ukdirectdebit#53

Open

CRM-19594 Wrong membership updated #9390

CRM-19594 Wrong membership updated #9390

Conversation

eileenmcnaughton commented Nov 15, 2016 • edited by civicrm-builder Loading

eileenmcnaughton Nov 15, 2016

Choose a reason for hiding this comment

eileenmcnaughton Nov 15, 2016

Choose a reason for hiding this comment

eileenmcnaughton Nov 15, 2016

Choose a reason for hiding this comment

eileenmcnaughton Nov 15, 2016

Choose a reason for hiding this comment

eileenmcnaughton Nov 15, 2016

Choose a reason for hiding this comment

eileenmcnaughton Nov 15, 2016

Choose a reason for hiding this comment

eileenmcnaughton commented Nov 15, 2016

eileenmcnaughton commented Nov 16, 2016

eileenmcnaughton commented Nov 16, 2016

MegaphoneJon commented Nov 16, 2016

eileenmcnaughton commented Nov 16, 2016

MegaphoneJon commented Nov 16, 2016

KarinG commented Nov 17, 2016

KarinG commented Nov 29, 2016

KarinG commented Nov 29, 2016

KarinG commented Nov 29, 2016 • edited Loading

KarinG commented Nov 29, 2016

eileenmcnaughton commented Nov 29, 2016

eileenmcnaughton commented Nov 29, 2016

KarinG commented Nov 29, 2016

eileenmcnaughton commented Nov 29, 2016

KarinG commented Nov 29, 2016

KarinG commented Nov 29, 2016

eileenmcnaughton commented Nov 29, 2016

KarinG commented Nov 29, 2016

KarinG commented Nov 29, 2016

eileenmcnaughton commented Nov 29, 2016

KarinG commented Nov 29, 2016

eileenmcnaughton commented Nov 29, 2016

davejenx commented Nov 29, 2016

Re-testing #9449 without #9464

Testing with latest RC branch code, #9464 merged

KarinG commented Nov 30, 2016

KarinG commented Nov 30, 2016

KarinG commented Nov 30, 2016

davejenx commented Nov 30, 2016

litespeedmarc commented Nov 30, 2016

davejenx commented Nov 30, 2016

eileenmcnaughton commented Nov 30, 2016

eileenmcnaughton commented Nov 30, 2016

eileenmcnaughton commented Nov 15, 2016 •

edited by civicrm-builder

Loading

KarinG commented Nov 29, 2016 •

edited

Loading