From 9cff1ee20562a4153704428ef938bdc8961ecf4f Mon Sep 17 00:00:00 2001 From: Carlos Capote Date: Thu, 8 Jul 2021 17:19:59 +0100 Subject: [PATCH] Check the billing fields returned by each payment processor (rather than a fixed list) Add missing newline after closing brace Add safer check for non-empty variable Test if billing fields presence is done respecting payment processors Pass civilint warnings for test file Add assertion for payment processor with uncomplete profile Apply civilint fix (remove extra line ending) --- CRM/Core/BAO/UFField.php | 35 ++- CRM/Core/Payment/ProcessorForm.php | 4 +- .../CRM/Core/Payment/ProcessorFormTest.php | 248 ++++++++++++++++++ 3 files changed, 273 insertions(+), 14 deletions(-) create mode 100644 tests/phpunit/CRM/Core/Payment/ProcessorFormTest.php diff --git a/CRM/Core/BAO/UFField.php b/CRM/Core/BAO/UFField.php index 2e1a4dc56d..965d8c6f2f 100644 --- a/CRM/Core/BAO/UFField.php +++ b/CRM/Core/BAO/UFField.php @@ -778,11 +778,13 @@ SELECT id * @param array $profileFilter * Filter to apply to profile fields - expected usage is to only fill based on. * the bottom profile per CRM-13726 + * @param array $paymentProcessorBillingFields + * Array of billing fields required by the payment processor. * * @return bool * Can the address block be hidden safe in the knowledge all fields are elsewhere collected (see CRM-15118) */ - public static function assignAddressField($key, &$profileAddressFields, $profileFilter) { + public static function assignAddressField($key, &$profileAddressFields, $profileFilter, $paymentProcessorBillingFields = NULL) { $billing_id = CRM_Core_BAO_LocationType::getBilling(); list($prefixName, $index) = CRM_Utils_System::explode('-', $key, 2); @@ -796,17 +798,22 @@ SELECT id ] )); //check for valid fields ( fields that are present in billing block ) - $validBillingFields = [ - 'first_name', - 'middle_name', - 'last_name', - 'street_address', - 'supplemental_address_1', - 'city', - 'state_province', - 'postal_code', - 'country', - ]; + if (!empty($paymentProcessorBillingFields)) { + $validBillingFields = $paymentProcessorBillingFields; + } + else { + $validBillingFields = [ + 'first_name', + 'middle_name', + 'last_name', + 'street_address', + 'supplemental_address_1', + 'city', + 'state_province', + 'postal_code', + 'country', + ]; + } $requiredBillingFields = array_diff($validBillingFields, ['middle_name', 'supplemental_address_1']); $validProfileFields = []; $requiredProfileFields = []; @@ -841,8 +848,10 @@ SELECT id } $potentiallyMissingRequiredFields = array_diff($requiredBillingFields, $requiredProfileFields); + $billingProfileIsHideable = empty($potentiallyMissingRequiredFields); CRM_Core_Resources::singleton() - ->addSetting(['billing' => ['billingProfileIsHideable' => empty($potentiallyMissingRequiredFields)]]); + ->addSetting(['billing' => ['billingProfileIsHideable' => $billingProfileIsHideable]]); + return $billingProfileIsHideable; } /** diff --git a/CRM/Core/Payment/ProcessorForm.php b/CRM/Core/Payment/ProcessorForm.php index 1649fa333a..a4062aef44 100644 --- a/CRM/Core/Payment/ProcessorForm.php +++ b/CRM/Core/Payment/ProcessorForm.php @@ -67,13 +67,15 @@ class CRM_Core_Payment_ProcessorForm { $form->_values['cancelSubscriptionUrl'] = $form->_paymentObject->subscriptionURL(NULL, NULL, 'cancel'); } + $paymentProcessorBillingFields = array_keys($form->_paymentProcessor['object']->getBillingAddressFields()); + if (!empty($form->_values['custom_pre_id'])) { $profileAddressFields = []; $fields = CRM_Core_BAO_UFGroup::getFields($form->_values['custom_pre_id'], FALSE, CRM_Core_Action::ADD, NULL, NULL, FALSE, NULL, FALSE, NULL, CRM_Core_Permission::CREATE, NULL); foreach ((array) $fields as $key => $value) { - CRM_Core_BAO_UFField::assignAddressField($key, $profileAddressFields, ['uf_group_id' => $form->_values['custom_pre_id']]); + CRM_Core_BAO_UFField::assignAddressField($key, $profileAddressFields, ['uf_group_id' => $form->_values['custom_pre_id']], $paymentProcessorBillingFields); } if (count($profileAddressFields)) { $form->set('profileAddressFields', $profileAddressFields); diff --git a/tests/phpunit/CRM/Core/Payment/ProcessorFormTest.php b/tests/phpunit/CRM/Core/Payment/ProcessorFormTest.php new file mode 100644 index 0000000000..c924c08c21 --- /dev/null +++ b/tests/phpunit/CRM/Core/Payment/ProcessorFormTest.php @@ -0,0 +1,248 @@ +standardProfile = $this->createStandardBillingProfile(); + $this->customProfile = $this->createCustomBillingProfile(); + + $this->standardProcessorType = $this->paymentProcessorTypeCreate([ + 'class_name' => 'PaymentProcessorWithStandardBillingRequirements', + 'name' => 'StandardBillingType', + ]); + + $this->customProcessorType = $this->paymentProcessorTypeCreate([ + 'class_name' => 'PaymentProcessorWithCustomBillingRequirements', + 'name' => 'CustomBillingType', + ]); + + $this->standardProcessor = $this->paymentProcessorCreate([ + 'name' => 'StandardBilling', + 'class_name' => 'PaymentProcessorWithStandardBillingRequirements', + 'payment_processor_type_id' => $this->standardProcessorType, + 'is_test' => 0, + ]); + + $this->customProcessor = $this->paymentProcessorCreate([ + 'name' => 'CustomBilling', + 'class_name' => 'PaymentProcessorWithCustomBillingRequirements', + 'payment_processor_type_id' => $this->customProcessorType, + 'is_test' => 0, + ]); + } + + public function tearDown(): void { + $this->callAPISuccess('PaymentProcessor', 'delete', [ + 'id' => $this->standardProcessor, + ]); + + $this->callAPISuccess('PaymentProcessor', 'delete', [ + 'id' => $this->customProcessor, + ]); + + $this->callAPISuccess('PaymentProcessorType', 'delete', [ + 'id' => $this->standardProcessorType, + ]); + + $this->callAPISuccess('PaymentProcessorType', 'delete', [ + 'id' => $this->customProcessorType, + ]); + + $this->quickCleanUpFinancialEntities(); + $this->quickCleanup(['civicrm_uf_group', 'civicrm_uf_field']); + + parent::tearDown(); + } + + public function createStandardBillingProfile() { + return $this->createTestableBillingProfile('standard', TRUE); + } + + public function createCustomBillingProfile() { + return $this->createTestableBillingProfile('custom', FALSE); + } + + public function createTestableBillingProfile($name, $withState) { + $billingId = CRM_Core_BAO_LocationType::getBilling(); + + $profile = $this->callAPISuccess('UFGroup', 'create', [ + 'group_type' => 'Contact', + 'title' => "Billing fields: $name", + 'name' => "${name}_billing", + ]); + + $this->callAPISuccess('UFField', 'create', [ + 'uf_group_id' => $profile['id'], + 'field_name' => 'first_name', + 'is_required' => TRUE, + ]); + + $this->callAPISuccess('UFField', 'create', [ + 'uf_group_id' => $profile['id'], + 'field_name' => 'last_name', + 'is_required' => TRUE, + ]); + + $this->callAPISuccess('UFField', 'create', [ + 'uf_group_id' => $profile['id'], + 'field_name' => 'street_address', + 'is_required' => TRUE, + ]); + + $this->callAPISuccess('UFField', 'create', [ + 'uf_group_id' => $profile['id'], + 'field_name' => 'city', + 'location_type_id' => $billingId, + 'is_required' => TRUE, + ]); + + if ($withState) { + $this->callAPISuccess('UFField', 'create', [ + 'uf_group_id' => $profile['id'], + 'field_name' => 'state_province', + 'location_type_id' => $billingId, + 'is_required' => TRUE, + ]); + } + + $this->callAPISuccess('UFField', 'create', [ + 'uf_group_id' => $profile['id'], + 'field_name' => 'postal_code', + 'location_type_id' => $billingId, + 'is_required' => TRUE, + ]); + + $this->callAPISuccess('UFField', 'create', [ + 'uf_group_id' => $profile['id'], + 'field_name' => 'country', + 'location_type_id' => $billingId, + 'is_required' => TRUE, + ]); + + return $profile; + } + + /** + * Checks a payment processor with either the standard, + * or the custom profile and returns a boolean that + * indicates whether the billing block can be hidden, or not. + */ + public function checkPaymentProcessorWithProfile($processorClass, $case) { + $whichProcessor = $case . "Processor"; + $whichProfile = $case . "Profile"; + + $processor = new $processorClass(); + $processor->id = $this->$whichProcessor; + + $missingBillingFields = []; + + $fields = array_column( + $this->callAPISuccess('UFField', 'get', ['uf_group_id' => $this->$whichProfile['id']])['values'], + 'field_name' + ); + + $fields = array_map(function($field) { + if (!isset($field['location_type_id'])) { + return "$field"; + } + return $field . "-" . $field['location_type_id']; + }, $fields); + + $canBeHidden = FALSE; + foreach ((array) $fields as $field) { + $canBeHidden = CRM_Core_BAO_UFField::assignAddressField( + $field, + $missingBillingFields, + ['uf_group_id' => $this->$whichProfile['id']], + array_keys($processor->getBillingAddressFields()) + ); + + if (!$canBeHidden) { + break; + } + } + + return $canBeHidden; + } + + /** + * Checks that, if a payment processor declares the standard + * billing fields as needed, they must be considered mandatory. + */ + public function testPaymentProcessorWithStandardBillingRequirements() { + $canBeHiddenWithTheStandardProfile = $this->checkPaymentProcessorWithProfile( + "PaymentProcessorWithStandardBillingRequirements", + "standard" + ); + + $canBeHiddenWithTheCustomProfile = $this->checkPaymentProcessorWithProfile( + "PaymentProcessorWithStandardBillingRequirements", + "custom" + ); + + $this->assertEquals(TRUE, $canBeHiddenWithTheStandardProfile); + $this->assertEquals(FALSE, $canBeHiddenWithTheCustomProfile); + } + + /** + * Checks that, if the payment processor doesn't declare a field + * as needed, the field shouldn't be considered mandatory. + */ + public function testPaymentProcessorWithCustomRequirements() { + $canBeHiddenWithTheCustomProfile = $this->checkPaymentProcessorWithProfile( + "PaymentProcessorWithCustomBillingRequirements", + "custom" + ); + + $this->assertEquals(TRUE, $canBeHiddenWithTheCustomProfile); + } + +} + +class PaymentProcessorWithStandardBillingRequirements extends CRM_Core_Payment { + + /** + * again, `checkConfig` is abstract in CRM_Core_Payment, so we are forced to implement it + */ + public function checkConfig() { + } + +} + +class PaymentProcessorWithCustomBillingRequirements extends CRM_Core_Payment { + + /** + * again, `checkConfig` is abstract in CRM_Core_Payment, so we are forced to implement it + */ + public function checkConfig() { + } + + public function getBillingAddressFields($billingLocationID = NULL) { + // Note that it intentionally misses the state_province field + return [ + 'first_name' => 'billing_first_name', + 'middle_name' => 'billing_middle_name', + 'last_name' => 'billing_last_name', + 'street_address' => "billing_street_address-{$billingLocationID}", + 'city' => "billing_city-{$billingLocationID}", + 'country' => "billing_country_id-{$billingLocationID}", + 'postal_code' => "billing_postal_code-{$billingLocationID}", + ]; + } + +} -- 2.25.1