From 23f0a3fd6fc984fd99ca3c273f32a13be154dcc0 Mon Sep 17 00:00:00 2001 From: eileen Date: Thu, 25 Mar 2021 16:11:35 +1300 Subject: [PATCH] [REF] Simplify determination on contribution recur contact id Instead of passing it around use a consistent method --- CRM/Member/Form.php | 22 ++++- CRM/Member/Form/Membership.php | 8 +- CRM/Member/Form/MembershipRenewal.php | 2 +- .../CRM/Member/Form/MembershipRenewalTest.php | 81 +++++++++---------- .../CRM/Member/Form/MembershipTest.php | 38 ++++----- 5 files changed, 78 insertions(+), 73 deletions(-) diff --git a/CRM/Member/Form.php b/CRM/Member/Form.php index 5b65b75372..c406c05bb1 100644 --- a/CRM/Member/Form.php +++ b/CRM/Member/Form.php @@ -329,20 +329,29 @@ class CRM_Member_Form extends CRM_Contribute_Form_AbstractEditPayment { } [$this->_memberDisplayName, $this->_memberEmail] = CRM_Contact_BAO_Contact_Location::getEmailDetails($this->_contactID); - + $this->_contributorContactID = $this->getContributionContactID(); //CRM-10375 Where the payer differs to the member the payer should get the email. // here we store details in order to do that if (!empty($formValues['soft_credit_contact_id'])) { - $this->_receiptContactId = $this->_contributorContactID = $formValues['soft_credit_contact_id']; + $this->_receiptContactId = $formValues['soft_credit_contact_id']; [$this->_contributorDisplayName, $this->_contributorEmail] = CRM_Contact_BAO_Contact_Location::getEmailDetails($this->_contributorContactID); } else { - $this->_receiptContactId = $this->_contributorContactID = $this->_contactID; + $this->_receiptContactId = $this->_contactID; $this->_contributorDisplayName = $this->_memberDisplayName; $this->_contributorEmail = $this->_memberEmail; } } + /** + * Get the contact id for the contribution. + * + * @return int + */ + protected function getContributionContactID(): int { + return (int) ($this->getSubmittedValue('soft_credit_contact_id') ?: $this->getSubmittedValue('contact_id')); + } + /** * Set variables in a way that can be accessed from different places. * @@ -494,7 +503,12 @@ class CRM_Member_Form extends CRM_Contribute_Form_AbstractEditPayment { * * @param array $formValues */ - public function testSubmit(array $formValues): void { + public function testSubmit(array $formValues = []): void { + if (empty($formValues)) { + // If getForm is used these will be set - this is now + // preferred. + $formValues = $this->controller->exportValues($this->_name); + } $this->exportedValues = $formValues; $this->setContextVariables($formValues); $this->_memType = !empty($formValues['membership_type_id']) ? $formValues['membership_type_id'][1] : NULL; diff --git a/CRM/Member/Form/Membership.php b/CRM/Member/Form/Membership.php index 31bcac4792..d6779d6de4 100644 --- a/CRM/Member/Form/Membership.php +++ b/CRM/Member/Form/Membership.php @@ -1771,8 +1771,7 @@ DESC limit 1"); $params, $contributionParams ) { - $contactID = $contributionParams['contact_id']; - $contributionParams['contribution_recur_id'] = $this->legacyProcessRecurringContribution($params, $contactID); + $contributionParams['contribution_recur_id'] = $this->legacyProcessRecurringContribution($params); return CRM_Contribute_BAO_Contribution::add($contributionParams); } @@ -1782,16 +1781,15 @@ DESC limit 1"); * This function was copied from another form & needs cleanup. * * @param array $params - * @param int $contactID * * @return int|null * @throws \CiviCRM_API3_Exception */ - protected function legacyProcessRecurringContribution(array $params, $contactID): ?int { + protected function legacyProcessRecurringContribution(array $params): ?int { if (!$this->isCreateRecurringContribution()) { return NULL; } - $recurParams = ['contact_id' => $contactID]; + $recurParams = ['contact_id' => $this->getContributionContactID()]; $recurParams['amount'] = $this->order->getTotalAmount(); $recurParams['auto_renew'] = $params['auto_renew'] ?? NULL; $recurParams['frequency_unit'] = $params['frequency_unit'] ?? NULL; diff --git a/CRM/Member/Form/MembershipRenewal.php b/CRM/Member/Form/MembershipRenewal.php index ed017fdb28..afe70a28ab 100644 --- a/CRM/Member/Form/MembershipRenewal.php +++ b/CRM/Member/Form/MembershipRenewal.php @@ -458,7 +458,7 @@ class CRM_Member_Form_MembershipRenewal extends CRM_Member_Form { * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception */ - public function postProcess() { + public function postProcess(): void { // get the submitted form values. $this->_params = $this->controller->exportValues($this->_name); $this->assignBillingName(); diff --git a/tests/phpunit/CRM/Member/Form/MembershipRenewalTest.php b/tests/phpunit/CRM/Member/Form/MembershipRenewalTest.php index 0b4963ff81..2c9c3c0013 100644 --- a/tests/phpunit/CRM/Member/Form/MembershipRenewalTest.php +++ b/tests/phpunit/CRM/Member/Form/MembershipRenewalTest.php @@ -140,10 +140,10 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * @throws \CiviCRM_API3_Exception */ public function testSubmit(): void { - $form = $this->getForm(); $loggedInUserID = $this->createLoggedInUser(); $loggedInUserDisplayName = Contact::get()->addWhere('id', '=', $loggedInUserID)->addSelect('display_name')->execute()->first()['display_name']; $params = $this->getBaseSubmitParams(); + $form = $this->getForm(array_merge($params, ['total_amount' => 50])); $form->_contactID = $this->_individualId; $form->testSubmit(array_merge($params, ['total_amount' => 50])); @@ -190,10 +190,10 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { public function testSubmitWithTax(): void { $this->enableTaxAndInvoicing(); $this->addTaxAccountToFinancialType($this->financialTypeID); - $form = $this->getForm(); - $form->testSubmit(array_merge($this->getBaseSubmitParams(), [ + $form = $this->getForm(array_merge($this->getBaseSubmitParams(), [ 'total_amount' => '50.00', ])); + $form->testSubmit(); $contribution = $this->callAPISuccessGetSingle('Contribution', ['contact_id' => $this->_individualId, 'is_test' => TRUE, 'return' => ['total_amount', 'tax_amount']]); $this->assertEquals(50, $contribution['total_amount']); $this->assertEquals(4.55, $contribution['tax_amount']); @@ -257,7 +257,6 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * @throws \CiviCRM_API3_Exception */ public function testSubmitRecur(): void { - $form = $this->getForm(); $this->callAPISuccess('MembershipType', 'create', [ 'id' => $this->membershipTypeAnnualFixedID, @@ -265,10 +264,8 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'duration_interval' => 1, 'auto_renew' => TRUE, ]); - $form->preProcess(); - $this->createLoggedInUser(); - $params = [ - 'cid' => $this->_individualId, + $form = $this->getForm([ + 'contact_id' => $this->_individualId, 'price_set_id' => 0, 'join_date' => CRM_Utils_Time::date('m/d/Y'), 'start_date' => '', @@ -290,7 +287,7 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'cvv2' => '123', 'credit_card_exp_date' => [ 'M' => '9', - 'Y' => CRM_Utils_Time::date('Y') + 1, + 'Y' => (int) CRM_Utils_Time::date('Y') + 1, ], 'credit_card_type' => 'Visa', 'billing_first_name' => 'Test', @@ -301,11 +298,12 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'billing_postal_code-5' => '90210', 'billing_country_id-5' => '1228', 'send_receipt' => 1, - ]; + ]); + $this->createLoggedInUser(); $form->_mode = 'test'; $form->_contactID = $this->_individualId; - $form->testSubmit($params); + $form->testSubmit(); $membership = $this->callAPISuccessGetSingle('Membership', ['contact_id' => $this->_individualId]); $contributionRecur = $this->callAPISuccessGetSingle('ContributionRecur', ['contact_id' => $this->_individualId]); $this->assertEquals(1, $contributionRecur['is_email_receipt']); @@ -349,7 +347,6 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * @throws \CiviCRM_API3_Exception */ public function testSubmitRecurCompleteInstant(): void { - $form = $this->getForm(); /** @var \CRM_Core_Payment_Dummy $processor */ $processor = Civi\Payment\System::singleton()->getById($this->_paymentProcessorID); $processor->setDoDirectPaymentResult([ @@ -365,13 +362,12 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'auto_renew' => TRUE, ]); $this->createLoggedInUser(); - $form->preProcess(); + $form = $this->getForm(array_merge($this->getBaseSubmitParams(), ['is_recur' => 1, 'auto_renew' => '1'])); $form->_contactID = $this->_individualId; - $params = array_merge($this->getBaseSubmitParams(), ['is_recur' => 1, 'auto_renew' => '1']); $form->_mode = 'test'; - $form->testSubmit($params); + $form->testSubmit(); $membership = $this->callAPISuccessGetSingle('Membership', ['contact_id' => $this->_individualId]); $this->assertEquals('2020-04-13', $membership['join_date']); $this->assertEquals(CRM_Utils_Time::date('Y-01-01'), $membership['start_date']); @@ -430,7 +426,6 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * @throws \API_Exception * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception - * @throws \Civi\API\Exception\UnauthorizedException * @dataProvider getThousandSeparators */ public function testSubmitRecurCompleteInstantWithMail(string $thousandSeparator): void { @@ -438,7 +433,6 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { // Visibility is 'Public Pages and Listings' in order to try to get to a specific line // of code to ensure it's tested - it might not be a 'real' use case. $this->createCustomGroupWithFieldOfType(['extends' => 'Membership'], 'multi_country', NULL, ['visibility' => 'Public Pages and Listings']); - $form = $this->getForm(); $this->mut = new CiviMailUtils($this, TRUE); /** @var \CRM_Core_Payment_Dummy $processor */ $processor = Civi\Payment\System::singleton()->getById($this->_paymentProcessorID); @@ -455,17 +449,17 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'auto_renew' => TRUE, ]); $this->createLoggedInUser(); - $form->preProcess(); - - $form->_contactID = $this->_individualId; - $form->_mode = 'live'; - - $form->testSubmit(array_merge($this->getBaseSubmitParams(), [ + $form = $this->getForm(array_merge($this->getBaseSubmitParams(), [ 'is_recur' => 1, 'send_receipt' => 1, 'auto_renew' => 1, $this->getCustomFieldName('multi_country') => [1006, 1007], ])); + + $form->_contactID = $this->_individualId; + $form->_mode = 'live'; + + $form->testSubmit(); $contributionRecur = $this->callAPISuccessGetSingle('ContributionRecur', ['contact_id' => $this->_individualId]); $this->assertEquals(1, $contributionRecur['is_email_receipt']); $this->mut->checkMailLog([ @@ -483,11 +477,10 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * @throws \CiviCRM_API3_Exception */ public function testSubmitPayLater(): void { - $form = $this->getForm(NULL); $this->createLoggedInUser(); $originalMembership = $this->callAPISuccessGetSingle('membership', []); - $params = [ - 'cid' => $this->_individualId, + $form = $this->getForm([ + 'contact_id' => $this->_individualId, 'join_date' => CRM_Utils_Time::date('m/d/Y'), 'start_date' => '', 'end_date' => '', @@ -505,10 +498,10 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'record_contribution' => TRUE, 'trxn_id' => 777, 'contribution_status_id' => 2, - ]; + ], NULL); $form->_contactID = $this->_individualId; - $form->testSubmit($params); + $form->testSubmit(); $membership = $this->callAPISuccessGetSingle('Membership', ['contact_id' => $this->_individualId]); $this->assertEquals(strtotime($membership['end_date']), strtotime($originalMembership['end_date'])); $contribution = $this->callAPISuccessGetSingle('Contribution', [ @@ -533,11 +526,10 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * @throws \CiviCRM_API3_Exception */ public function testSubmitPayLaterWithBilling(): void { - $form = $this->getForm(NULL); $this->createLoggedInUser(); $originalMembership = $this->callAPISuccessGetSingle('membership', []); - $params = [ - 'cid' => $this->_individualId, + $form = $this->getForm([ + 'contact_id' => $this->_individualId, 'start_date' => '', 'end_date' => '', // This format reflects the first value being the organisation & the second being the type. @@ -561,10 +553,10 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'billing_state_province_id-5' => '1003', 'billing_postal_code-5' => '90210', 'billing_country_id-5' => '1228', - ]; + ], NULL); $form->_contactID = $this->_individualId; - $form->testSubmit($params); + $form->testSubmit(); $membership = $this->callAPISuccessGetSingle('Membership', ['contact_id' => $this->_individualId]); $this->assertEquals(strtotime($membership['end_date']), strtotime($originalMembership['end_date'])); $this->assertEquals(10, $membership['max_related']); @@ -594,11 +586,10 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * @throws \CiviCRM_API3_Exception */ public function testSubmitComplete(): void { - $form = $this->getForm(NULL); $this->createLoggedInUser(); $originalMembership = $this->callAPISuccessGetSingle('membership', []); - $params = [ - 'cid' => $this->_individualId, + $form = $this->getForm([ + 'contact_id' => $this->_individualId, 'join_date' => CRM_Utils_Time::date('m/d/Y'), 'start_date' => '', 'end_date' => '', @@ -617,10 +608,10 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'trxn_id' => 777, 'contribution_status_id' => 1, 'fee_amount' => .5, - ]; + ], NULL); $form->_contactID = $this->_individualId; - $form->testSubmit($params); + $form->testSubmit(); $membership = $this->callAPISuccessGetSingle('Membership', ['contact_id' => $this->_individualId]); $this->assertEquals(strtotime($membership['end_date']), strtotime('+ 2 years', strtotime($originalMembership['end_date']))); @@ -643,6 +634,7 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * * We need to instantiate the form to run preprocess, which means we have to trick it about the request method. * + * @param array $formValues * @param string $mode * * @return \CRM_Member_Form_MembershipRenewal @@ -650,10 +642,10 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception */ - protected function getForm($mode = 'test'): CRM_Member_Form_MembershipRenewal { - $form = new CRM_Member_Form_MembershipRenewal(); - $_SERVER['REQUEST_METHOD'] = 'GET'; - $form->controller = new CRM_Core_Controller(); + protected function getForm($formValues = [], $mode = 'test'): CRM_Member_Form_MembershipRenewal { + /* @var CRM_Member_Form_MembershipRenewal $form */ + $form = $this->getFormObject('CRM_Member_Form_MembershipRenewal', $formValues); + $form->_bltID = 5; $form->_mode = $mode; $form->setEntityId($this->_membershipID); @@ -669,6 +661,7 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { protected function getBaseSubmitParams(): array { return [ 'cid' => $this->_individualId, + 'contact_id' => $this->_individualId, // This format reflects the key being the organisation & the value being the type. 'membership_type_id' => [$this->ids['contact']['organization'], $this->membershipTypeAnnualFixedID], 'num_terms' => '1', @@ -682,7 +675,7 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { 'cvv2' => '123', 'credit_card_exp_date' => [ 'M' => '9', - 'Y' => CRM_Utils_Time::date('Y') + 1, + 'Y' => (int) CRM_Utils_Time::date('Y') + 1, ], 'credit_card_type' => 'Visa', 'billing_first_name' => 'Test', @@ -702,7 +695,7 @@ class CRM_Member_Form_MembershipRenewalTest extends CiviUnitTestCase { * @throws \CiviCRM_API3_Exception */ public function testSubmitRenewExpired(): void { - $form = $this->getForm(NULL); + $form = $this->getForm([], NULL); $this->createLoggedInUser(); $originalMembership = $this->callAPISuccessGetSingle('membership', []); $this->callAPISuccess('Membership', 'create', [ diff --git a/tests/phpunit/CRM/Member/Form/MembershipTest.php b/tests/phpunit/CRM/Member/Form/MembershipTest.php index c85d66598e..0e75800d68 100644 --- a/tests/phpunit/CRM/Member/Form/MembershipTest.php +++ b/tests/phpunit/CRM/Member/Form/MembershipTest.php @@ -487,7 +487,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { * * @dataProvider getThousandSeparators */ - public function testSubmit(string $thousandSeparator) { + public function testSubmit(string $thousandSeparator): void { CRM_Core_Session::singleton()->getStatus(TRUE); $this->setCurrencySeparators($thousandSeparator); $form = $this->getForm(); @@ -496,6 +496,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { $this->createLoggedInUser(); $params = [ 'cid' => $this->_individualId, + 'contact_id' => $this->_individualId, 'join_date' => date('Y-m-d'), 'start_date' => '', 'end_date' => '', @@ -786,7 +787,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { $params = $this->getBaseSubmitParams(); // Change financial_type_id to test our override flows through to the line item. $params['financial_type_id'] = FinancialType::get(FALSE)->addWhere('id', '!=', $params['financial_type_id'])->addSelect('id')->execute()->first()['id']; - $form = $this->getForm(); + $form = $this->getForm($params); $this->createLoggedInUser(); $form->_mode = 'test'; $form->_contactID = $this->_individualId; @@ -905,12 +906,11 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { * * @throws \CRM_Core_Exception */ - public function testSubmitPayLaterWithBilling() { + public function testSubmitPayLaterWithBilling(): void { $form = $this->getForm(); - $form->preProcess(); $this->createLoggedInUser(); $params = [ - 'cid' => $this->_individualId, + 'contact_id' => $this->_individualId, 'join_date' => date('Y-m-d'), 'start_date' => '', 'end_date' => '', @@ -933,7 +933,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { 'trxn_id' => 777, 'contribution_status_id' => 2, 'billing_first_name' => 'Test', - 'billing_middlename' => 'Last', + 'billing_middle_name' => 'Last', 'billing_street_address-5' => '10 Test St', 'billing_city-5' => 'Test', 'billing_state_province_id-5' => '1003', @@ -1010,9 +1010,9 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { * @throws \CiviCRM_API3_Exception * @throws \CRM_Core_Exception */ - public function testSubmitRecurCompleteInstant() { - $form = $this->getForm(); + public function testSubmitRecurCompleteInstant(): void { $mut = new CiviMailUtils($this, TRUE); + /* @var \CRM_Core_Payment_Dummy $processor */ $processor = Civi\Payment\System::singleton()->getById($this->_paymentProcessorID); $processor->setDoDirectPaymentResult([ 'payment_status_id' => 1, @@ -1026,12 +1026,11 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { 'duration_interval' => 1, 'auto_renew' => TRUE, ]); - $form->preProcess(); + $form = $this->getForm($this->getBaseSubmitParams()); $this->createLoggedInUser(); - $params = $this->getBaseSubmitParams(); $form->_mode = 'test'; $form->_contactID = $this->_individualId; - $form->testSubmit($params); + $form->testSubmit(); $membership = $this->callAPISuccessGetSingle('Membership', ['contact_id' => $this->_individualId]); $this->callAPISuccessGetCount('ContributionRecur', ['contact_id' => $this->_individualId], 1); @@ -1053,7 +1052,7 @@ class CRM_Member_Form_MembershipTest extends CiviUnitTestCase { '=========================================================== Billing Name and Address =========================================================== -Test +Test Last 10 Test St Test, AR 90210 US', @@ -1218,15 +1217,16 @@ Expires: ', * We need to instantiate the form to run preprocess, which means we have to * trick it about the request method. * + * @param array $formValues + * * @return \CRM_Member_Form_Membership * @throws \CRM_Core_Exception - * @throws \CiviCRM_API3_Exception */ - protected function getForm() { + protected function getForm($formValues = []) { if (isset($_REQUEST['cid'])) { unset($_REQUEST['cid']); } - $form = $this->getFormObject('CRM_Member_Form_Membership'); + $form = $this->getFormObject('CRM_Member_Form_Membership', $formValues); $form->preProcess(); return $form; } @@ -1234,8 +1234,9 @@ Expires: ', /** * @return array */ - protected function getBaseSubmitParams() { - $params = [ + protected function getBaseSubmitParams(): array { + return [ + 'contact_id' => $this->_individualId, 'cid' => $this->_individualId, 'price_set_id' => 0, 'join_date' => date('Y-m-d'), @@ -1265,7 +1266,7 @@ Expires: ', ], 'credit_card_type' => 'Visa', 'billing_first_name' => 'Test', - 'billing_middlename' => 'Last', + 'billing_middle_name' => 'Last', 'billing_street_address-5' => '10 Test St', 'billing_city-5' => 'Test', 'billing_state_province_id-5' => '1003', @@ -1273,7 +1274,6 @@ Expires: ', 'billing_country_id-5' => '1228', 'send_receipt' => 1, ]; - return $params; } /** -- 2.25.1