CRM-15055 Line item for membership entity should be created even when Record Payment is not checked

Sumitted PR at https://github.com/civicrm/civicrm-core/pull/3999/files

Fix includes
1. Front End Membership creation
– 0 Membership Amount - 0 amount contribution and line item
– Separate Membership case - when a contribution page is configured for separate payment the line item points to wrong entity
– Fix for price set for membership having fields with and without membership type
– Fixed Renew of Membership

2. For Back End
– Creating line item when record contribution is not checked
– Updating of contribution for membership leads to creation of bogus line item
– Fix for price set for membership having fields with and without membership type

3. Import of Membership Create/Update mode

4. Membership creation via api

5. IPN callback to handle membership related contribution.

The upgrade code to fix bad line item needs to be done. I am trying to list down various use cases in 4.5 that might have created bad line item with wrong entity id or contribution id. I will come up with the list of buggy use case and will try to fix if its not complicated. Creating a separate Issue to handle upgrade for bad line item (CRM-15204).

Tested items 1-3 with a patch from . However, I can not merge the patch yet due to one serious regression in email confirmations in Back End.

1. Front end
ALL ok

2. For Back End
REGRESSION – Fix for price set for membership having fields with and without membership type

• • • Some change in this code causes the membership line item to be MISSING from the confirmation email. Screenshots attached before ("Jack Arnold") and after the patch ("Pradeep Nayak"). Backoffice membership signup using the same price set. Membership form w/ price set also attached.

3-5 No details provided so I can't verify.

Dave,

I have committed my changes to same PR for regression issue.

for

#3 & #4 previously no line item was recorded when a membership is created/updated from import membership or api.

#5 If we have a membership and contribution having Pending from incomplete transaction/ pay later and if we update the status of contribution to completed by using contribution task action 'Update Pending Contribution Status' then wrong line item gets updated.

(Context: When auto-testing PRs, we run most but not all of the the test-suite. Some tests are left out and only run periodically in CiviCRM-Core-Matrix; you might call these second-tier tests. We don't notice the second-tier failures as quickly, and the second-tier failures aren't quite as urgent because they don't show up in code-review. Never-the-less, we should fix second-tier failures.)

It appears that one of the second-tier tests, CRM_Batch_Form_EntryTest.testProcessMembership, regressed. Running "git bisect", I traced it to https://github.com/civicrm/civicrm-core/commit/d5b95619b7ab9afa123b3db635128294514ea2ef .

Fixed at PR https://github.com/civicrm/civicrm-core/pull/4199

The regression wasn't caused by the commit. It was due to hard-coded values present in CRM_Batch_Form_EntryTest.testProcessMembership.

'membership_type' => Array (0 => 1, 1 => 1),// (I was unable to determine what these both mean but both are refered to in code

Using hard-coded values in webtest is a flaw, We should always try not to use Hard-coded values since in tearDown all the data for Membership type and Contact is truncated.

I'm just wondering if you could add some explanation to this issue as to why line items are created in the absence of a financial transaction. I thought it was a bug when I first hit null contribution_id membership line items - but tracking the code back brought me here.

Explanations and comments for above issue is in CRM-14918

hmm OK - I kind of don't like the idea of a null entry for contribution_id but I guess my main problem with the fix here is how it affects the api. If you try to update a membership it looks like it creates new lines. Also as the api doesn't really provide a way to appropriately create line items this kind of breaks existing code that might have tried to create the line items but adding extraneous ones.

Not that I think we should rush into any further changes - the problem that there is no way to create an appropriate transaction via the api is probably the more worthy problem to solve & the obvious 'half-fix' - adding contribution_id as a param on membership.create is not something I'd want to rush into.