From e0ec7c6209c81a8d9374cae192494f346a909a45 Mon Sep 17 00:00:00 2001 From: eileen Date: Mon, 26 Oct 2020 17:05:58 +1300 Subject: [PATCH] [REF] Simplify membership form code towards simplifying BAO On digging into the handling for the param relate_contribution_id in the BAO I found it is called only from the membership form & it's really a hack for the way price sets were super-imposed on the form. This has really solid testing in testTwoInheritedMembershipsViaPriceSetInBackend (Thanks @davejenx) and stepping through there is the best way to make sense of it. What is happening is that if there are 2 memberships in a price set Membership.create is called twice. However, because the parameter contribution_status_id is passed in it attempts to create the contribution twice. If we don't add this the second time then we wind up with the second membership payment not created because that membership didn't exist the first round. So, an extra param relate_contribution_id got added which says 'if this is passed in create the MembershipPayment record that didn't get created the first time with the contribution id that we just did a lookup on. All of this is needed because we create the contribution between the first Membership create and the second. This removes both magic params from the create, and instead calls recordMembershipContribution after both are done. I thought about calling Order.create or just Contribution.create here but there is stuff to unravel around the soft credit creation being done in RecordMembershipContribution and I am not yet clear if tests are adequate there --- CRM/Member/BAO/Membership.php | 5 ++-- CRM/Member/Form/Membership.php | 27 ++++++++++--------- .../CRM/Member/Form/MembershipTest.php | 8 +++--- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/CRM/Member/BAO/Membership.php b/CRM/Member/BAO/Membership.php index 46e62cbe30..540563f95a 100644 --- a/CRM/Member/BAO/Membership.php +++ b/CRM/Member/BAO/Membership.php @@ -335,12 +335,14 @@ class CRM_Member_BAO_Membership extends CRM_Member_DAO_Membership { $params['skipLineItem'] = TRUE; // Record contribution for this membership and create a MembershipPayment + // @todo deprecate this. if (!empty($params['contribution_status_id']) && empty($params['relate_contribution_id'])) { $memInfo = array_merge($params, ['membership_id' => $membership->id]); $params['contribution'] = self::recordMembershipContribution($memInfo); } // Add/update MembershipPayment record for this membership if it is a related contribution + // @todo remove this - called from one remaining place in CRM_Member_Form_Membership if (!empty($params['relate_contribution_id'])) { $membershipPaymentParams = [ 'membership_id' => $membership->id, @@ -2405,7 +2407,6 @@ WHERE {$whereClause}"; * @throws \CiviCRM_API3_Exception */ public static function recordMembershipContribution(&$params) { - $membershipId = $params['membership_id']; $contributionParams = []; $config = CRM_Core_Config::singleton(); $contributionParams['currency'] = $config->defaultCurrency; @@ -2487,7 +2488,7 @@ WHERE {$whereClause}"; ]); if (empty($membershipPayment['count'])) { civicrm_api3('MembershipPayment', 'create', [ - 'membership_id' => $membershipId, + 'membership_id' => $params['membership_id'], 'contribution_id' => $contribution->id, ]); } diff --git a/CRM/Member/Form/Membership.php b/CRM/Member/Form/Membership.php index f82bd2897d..b5846d2177 100644 --- a/CRM/Member/Form/Membership.php +++ b/CRM/Member/Form/Membership.php @@ -1465,33 +1465,34 @@ DESC limit 1"); } else { $params['action'] = $this->_action; - $count = 0; - foreach ($this->_memTypeSelected as $memType) { - if ($count && !empty($formValues['record_contribution']) && - ($relateContribution = CRM_Member_BAO_Membership::getMembershipContributionId($membership->id)) - ) { - $membershipTypeValues[$memType]['relate_contribution_id'] = $relateContribution; + foreach ($lineItem[$this->_priceSetId] as $id => $lineItemValues) { + if (empty($lineItemValues['membership_type_id'])) { + continue; } // @todo figure out why recieve_date isn't being set right here. if (empty($params['receive_date'])) { $params['receive_date'] = date('Y-m-d H:i:s'); } - $membershipParams = array_merge($params, $membershipTypeValues[$memType]); + $membershipParams = array_merge($params, $membershipTypeValues[$lineItemValues['membership_type_id']]); if (!empty($softParams)) { $membershipParams['soft_credit'] = $softParams; } + unset($membershipParams['contribution_status_id']); + $membershipParams['skipLineItem'] = TRUE; + unset($membershipParams['lineItems']); // @todo stop passing $ids (membership and userId only are set above) $membership = CRM_Member_BAO_Membership::create($membershipParams, $ids); - $params['contribution'] = $membershipParams['contribution'] ?? NULL; - unset($params['lineItems']); - // skip line item creation for next interation since line item(s) are already created. - $params['skipLineItem'] = TRUE; + $lineItem[$this->_priceSetId][$id]['entity_id'] = $membership->id; + $lineItem[$this->_priceSetId][$id]['entity_table'] = 'civicrm_membership'; $this->_membershipIDs[] = $membership->id; - $createdMemberships[$memType] = $membership; - $count++; + $createdMemberships[$membership->membership_type_id] = $membership; + } + $params['lineItems'] = $lineItem; + if (!empty($formValues['record_contribution'])) { + CRM_Member_BAO_Membership::recordMembershipContribution($params); } } $isRecur = $params['is_recur'] ?? NULL; diff --git a/tests/phpunit/CRM/Member/Form/MembershipTest.php b/tests/phpunit/CRM/Member/Form/MembershipTest.php index fb6bc5d7bd..18ece33ba5 100644 --- a/tests/phpunit/CRM/Member/Form/MembershipTest.php +++ b/tests/phpunit/CRM/Member/Form/MembershipTest.php @@ -582,7 +582,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception */ - public function testContributionUpdateOnMembershipTypeChange() { + public function testContributionUpdateOnMembershipTypeChange(): void { // Step 1: Create a Membership via backoffice whose with 50.00 payment $form = $this->getForm(); $form->preProcess(); @@ -647,9 +647,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { 'end_date' => '', // This format reflects the first number being the organisation & the 25 being the type. 'membership_type_id' => [$this->ids['contact']['organization'], $secondMembershipType['id']], - 'record_contribution' => 1, 'status_id' => 1, - 'total_amount' => 25, 'receive_date' => date('Y-m-d', time()) . ' 20:36:00', 'payment_instrument_id' => array_search('Check', $this->paymentInstruments), //Member dues, see data.xml @@ -668,7 +666,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { $this->assertEquals('Pending refund', $contribution['contribution_status']); // Earlier paid amount $this->assertEquals(50, $payment['paid']); - // balance remaning + // balance remaining $this->assertEquals(-25, $payment['balance']); } @@ -1098,6 +1096,8 @@ Expires: ', $this->assertEquals($contributionResult['values'][0]['id'], $membershipPayment['contribution_id'], "membership payment's contribution ID should be the ID of the organization's membership contribution."); $this->assertContains($membershipPayment['membership_id'], $primaryMembershipIds, "membership payment's membership ID should be the ID of a primary membership."); } + // Check for orphan line items. + $this->callAPISuccessGetCount('LineItem', [], 2); // CRM-20966: check that deleting relationship used for inheritance does not delete contribution. $this->callAPISuccess('relationship', 'delete', [ -- 2.25.1