Contribution deleted when relationship deleted, incorrect line items & membership_payments for second inherited membership created in back end via price set

Summary

Issue occurs when there are two membership types, each having the same relationship type configured, with different membership organisations so that a contact can have memberships of both types.

If a contact has memberships of both types and a relationship of the relevant type, the related contact should inherit both memberships.

However, when creating the two memberships in the back end using a price set, the line_item and membership_payment records for the second membership are created incorrectly.

As a consequence of the incorrect membership_payment records, if the relationship between the primary & secondary members is deleted, this results in the primary member's membership contribution being deleted.

I've set priority to Critical because data loss can occur (incorrect deletion of contribution). As it's a pretty uncommon scenario though, fair enough to downgrade it.

Affected versions

Affects various 4.6 versions tested.
In 4.7, the problem was masked by CRM-20955, which stops the 2nd membership inheriting, which avoids this issue. However with that issue fixed, this issue affects current 4.7 too. To reflect that, I've set Affects Version to (unreleased) 4.7.23 (+ various 4.6).

Steps to replicate

Note: in 4.7, in order to replicate you will need to have applied the fix for CRM-20955 = PR https://github.com/civicrm/civicrm-core/pull/10745 . Otherwise the second membership won't inherit and so the incorrect line_item and membership_payment records will not occur.

Steps 1-4 are the same as for CRM-20955.

1. Create membership type "Org Main Membership", with fee, 1 year fixed, Relationship Type "Employer of"

2. Create membership type "Org Additional Membership" with different member org, with fee, 1 year rolling, Relationship Type "Employer of"

3. Create membership price set "Org memberships" with field:

• Membership: CheckBox, with options:
    - membership type: Org Main Membership
    - membership type: Org Additional Membership

4. Create memberships for an org that has an employee, via back end, using price set:
Add Membership -> Choose price set -> Org memberships
Select Org Main Membership + Org Additional Membership, tick Record Membership Payment?   

5. Check civicrm_membership_payment records:
Expected:
2 records for above contribution, with each of the org's 2 primary membership ids.
Actual:
3 records for above contribution, for each of the org's 2 primary memberships + 1 of the inherited memberships

6. Check civicrm_line_item records:
Expected:
2 records for above contribution, for each of the org's 2 memberships, with entity_table = 'civicrm_membership', entity_id = primary membership id
Actual:
2 records for above contribution, for each of the org's 2 memberships, with entity_table = 'civicrm_membership', one with entity_id = primary membership id, the other with entity id = the id of an inherited membership.

7. Delete the "Employee of" relationship between the org & the employee.
Expected:
Employee loses their 2 inherited memberships; org unchanged.
Actual:
Employee loses their 2 inherited memberships; org's membership contribution deleted.

Diagnosis

Found that when the 2nd inherited membership is created, a membership_payment record is (incorrectly) created for it. When this happens, the (previously correct) line item for the 2nd membership gets its entity_id updated from the primary membership to the secondary membership.

Once the incorrect membership_payment record exists, deleting the relationship leads to the contribution being deleted as follows.

• CRM_Contact_BAO_Relationship::del() calls
• CRM_Contact_BAO_Relationship::relatedMemberships() calls
• CRM_Member_BAO_Membership::deleteRelatedMemberships() calls
• CRM_Member_BAO_Membership::deleteMembership() calls
• CRM_Member_BAO_Membership::deleteMembershipPayment() calls
• CRM_Contribute_BAO_Contribution::deleteContribution()

You could argue there are two things wrong there.

(a) There shouldn't be a membership_payment record for an inherited membership, so deleteMembershipPayment() shouldn't be activated. Fixing this issue resolves that.

(b) If a contribution covers two memberships, then deleting one of those memberships arguably should not result in the contribution being deleted. When this is done through the UI, at least there is a big warning but none in this case. This is part of a bigger discussion about whether deleting a membership should result in the contribution being deleted, e.g. CRM-7188, CRM-19150. That discussion is separate from this issue.