From f2dd7b23c6a359ee62f6b34e2d1d8da16684dfc5 Mon Sep 17 00:00:00 2001 From: Rich Lott / Artful Robot Date: Fri, 1 Nov 2019 10:59:02 +0000 Subject: [PATCH] Introduce PropertyBag to help standardise payment params WIP: proof of concept for payment property bag WIP: keep civilint happy WIP: keep civilint happy again Add other getters, setters, tests Add require() method and test some wip code linter fixes remove :void from function declaration in Civi/Payment/PropertyBag fix PropertyBagTest.php tests fix linter Re-do payment code lost in rebase linting fixes More WIP on payment property bag one day i'll run civilint before committing WIP checkin Correct test which referred to Credit card instead of Credit Card causing a fail with new property bag which is stricter Payment PropertyBag: allow week as recurFrequencyUnit; accept ZLS for feeAmount for legacy sake CRM_Core_Payment: remove stuff cut from release remove test no longer needed remove un-needed code because feature dropped before it was released remove comment Remove/disarm payment instrument setter restore getPaymentInstrumentID behaviour remove not required function --- CRM/Core/Payment.php | 99 ++- CRM/Core/Payment/Form.php | 4 +- Civi/Payment/PropertyBag.php | 642 ++++++++++++++++++ .../CRM/Contribute/Form/ContributionTest.php | 2 +- .../phpunit/Civi/Payment/PropertyBagTest.php | 210 ++++++ 5 files changed, 920 insertions(+), 37 deletions(-) create mode 100644 Civi/Payment/PropertyBag.php create mode 100644 tests/phpunit/Civi/Payment/PropertyBagTest.php diff --git a/CRM/Core/Payment.php b/CRM/Core/Payment.php index 798daac060a9..57fa1830aaab 100644 --- a/CRM/Core/Payment.php +++ b/CRM/Core/Payment.php @@ -11,6 +11,7 @@ use Civi\Payment\System; use Civi\Payment\Exception\PaymentProcessorException; +use Civi\Payment\PropertyBag; /** * Class CRM_Core_Payment. @@ -134,32 +135,41 @@ abstract class CRM_Core_Payment { protected $backOffice = FALSE; /** - * @return bool - */ - public function isBackOffice() { - return $this->backOffice; - } - - /** - * Set back office property. + * This is only needed during the transitional phase. In future you should + * pass your own PropertyBag into the method you're calling. * - * @param bool $isBackOffice + * New code should NOT use $this->propertyBag. + * + * @var Civi\Payment\PropertyBag */ - public function setBackOffice($isBackOffice) { - $this->backOffice = $isBackOffice; - } + protected $propertyBag; /** - * Get payment instrument id. + * Return the payment instrument ID to use. + * + * Note: + * We normally SHOULD be returning the payment instrument of the payment processor. + * However there is an outstanding case where this needs overriding, which is + * when using CRM_Core_Payment_Manual which uses the pseudoprocessor (id = 0). + * + * i.e. If you're writing a Payment Processor you should NOT be using + * setPaymentInstrumentID() at all. + * + * @todo + * Ideally this exception-to-the-rule should be handled outside of this class + * i.e. this class's getPaymentInstrumentID method should return it from the + * payment processor and CRM_Core_Payment_Manual could override it to provide 0. * * @return int */ public function getPaymentInstrumentID() { - return $this->paymentInstrumentID ? $this->paymentInstrumentID : $this->_paymentProcessor['payment_instrument_id']; + return isset($this->paymentInstrumentID) + ? $this->paymentInstrumentID + : (int) ($this->_paymentProcessor['payment_instrument_id'] ?? 0); } /** - * Getter for the id. + * Getter for the id Payment Processor ID. * * @return int */ @@ -168,14 +178,33 @@ public function getID() { } /** - * Set payment Instrument id. + * @deprecated Set payment Instrument id - see note on getPaymentInstrumentID. * - * By default we actually ignore the form value. The manual processor takes it more seriously. + * By default we actually ignore the form value. The manual processor takes + * it more seriously. * * @param int $paymentInstrumentID */ public function setPaymentInstrumentID($paymentInstrumentID) { - $this->paymentInstrumentID = $this->_paymentProcessor['payment_instrument_id']; + $this->paymentInstrumentID = (int) $paymentInstrumentID; + // See note on getPaymentInstrumentID(). + return $this->getPaymentInstrumentID(); + } + + /** + * @return bool + */ + public function isBackOffice() { + return $this->backOffice; + } + + /** + * Set back office property. + * + * @param bool $isBackOffice + */ + public function setBackOffice($isBackOffice) { + $this->backOffice = $isBackOffice; } /** @@ -669,6 +698,8 @@ protected function getMandatoryFields() { * Get the metadata of all the fields configured for this processor. * * @return array + * + * @throws \CiviCRM_API3_Exception */ protected function getAllFields() { $paymentFields = array_intersect_key($this->getPaymentFormFieldsMetadata(), array_flip($this->getPaymentFormFields())); @@ -1042,28 +1073,32 @@ protected function getBaseReturnUrl() { } /** - * Get the currency for the transaction. + * Get the currency for the transaction from the params. * - * Handle any inconsistency about how it is passed in here. + * Legacy wrapper. Better for a method to work on its own PropertyBag. + * + * This code now uses PropertyBag to allow for old inputs like currencyID. * * @param $params * * @return string */ - protected function getCurrency($params) { - return CRM_Utils_Array::value('currencyID', $params, CRM_Utils_Array::value('currency', $params)); + protected function getCurrency($params = []) { + $localPropertyBag = new PropertyBag(); + $localPropertyBag->mergeLegacyInputParams($params); + return $localPropertyBag->getCurrency(); } /** - * Get the currency for the transaction. - * - * Handle any inconsistency about how it is passed in here. + * Legacy. Better for a method to work on its own PropertyBag, + * but also, this function does not do very much. * - * @param $params + * @param array $params * * @return string + * @throws \CRM_Core_Exception */ - protected function getAmount($params) { + protected function getAmount($params = []) { return CRM_Utils_Money::format($params['amount'], NULL, NULL, TRUE); } @@ -1611,7 +1646,7 @@ public function subscriptionURL($entityID = NULL, $entity = NULL, $action = 'can * * @return string */ - protected function getPaymentDescription($params, $length = 24) { + protected function getPaymentDescription($params = [], $length = 24) { $parts = [ 'contactID', 'contributionID', @@ -1620,13 +1655,7 @@ protected function getPaymentDescription($params, $length = 24) { 'billing_last_name', ]; $validParts = []; - if (isset($params['description'])) { - $uninformativeStrings = [ - ts('Online Event Registration: '), - ts('Online Contribution: '), - ]; - $params['description'] = str_replace($uninformativeStrings, '', $params['description']); - } + $params['description'] = $this->getDescription(); foreach ($parts as $part) { if ((!empty($params[$part]))) { $validParts[] = $params[$part]; diff --git a/CRM/Core/Payment/Form.php b/CRM/Core/Payment/Form.php index b8bb4408456e..3d1b2513ecea 100644 --- a/CRM/Core/Payment/Form.php +++ b/CRM/Core/Payment/Form.php @@ -48,7 +48,9 @@ public static function setPaymentFieldsByProcessor(&$form, $processor, $billing_ $processor['object']->setBillingProfile($billing_profile_id); $processor['object']->setBackOffice($isBackOffice); - $processor['object']->setPaymentInstrumentID($paymentInstrumentID); + if ($paymentInstrumentID) { + $processor['object']->setPaymentInstrumentID($paymentInstrumentID); + } $paymentTypeName = self::getPaymentTypeName($processor); $form->assign('paymentTypeName', $paymentTypeName); $form->assign('paymentTypeLabel', self::getPaymentLabel($processor['object'])); diff --git a/Civi/Payment/PropertyBag.php b/Civi/Payment/PropertyBag.php new file mode 100644 index 000000000000..57867c2dafd2 --- /dev/null +++ b/Civi/Payment/PropertyBag.php @@ -0,0 +1,642 @@ + []]; + + protected static $propMap = [ + 'contactID' => TRUE, + 'contact_id' => 'contactID', + 'contributionID' => TRUE, + 'contribution_id' => 'contributionID', + 'contributionRecurID' => TRUE, + 'contribution_recur_id' => 'contributionRecurID', + 'currency' => TRUE, + 'currencyID' => 'currency', + 'description' => TRUE, + 'feeAmount' => TRUE, + 'fee_amount' => 'feeAmount', + 'invoiceID' => TRUE, + 'invoice_id' => 'invoiceID', + 'isBackOffice' => TRUE, + 'is_back_office' => 'isBackOffice', + 'isRecur' => TRUE, + 'is_recur' => 'isRecur', + 'paymentToken' => TRUE, + 'payment_token' => 'paymentToken', + 'recurFrequencyInterval' => TRUE, + 'frequency_interval' => 'recurFrequencyInterval', + 'recurFrequencyUnit' => TRUE, + 'frequency_unit' => 'recurFrequencyUnit', + 'transactionID' => TRUE, + 'transaction_id' => 'transactionID', + 'trxnResultCode' => TRUE, + ]; + + /** + * @var string Just for unit testing. + */ + public $lastWarning; + + /** + * Implements ArrayAccess::offsetExists + * + * @param mixed $offset + * @return bool TRUE if we have that value (on our default store) + */ + public function offsetExists ($offset): bool { + $prop = $this->handleLegacyPropNames($offset); + return isset($this->props['default'][$prop]); + } + + /** + * Implements ArrayAccess::offsetGet + * + * @param mixed $offset + * @return mixed + */ + public function offsetGet ($offset) { + $prop = $this->handleLegacyPropNames($offset); + return $this->get($prop, 'default'); + } + + /** + * Implements ArrayAccess::offsetSet + * + * @param mixed $offset + * @param mixed $value + */ + public function offsetSet($offset, $value) { + try { + $prop = $this->handleLegacyPropNames($offset); + } + catch (InvalidArgumentException $e) { + // We need to make a lot of noise here, we're being asked to merge in + // something we can't validate because we don't know what this property is. + // This is fine if it's something particular to a payment processor + // (which should be using setCustomProperty) however it could also lead to + // things like 'my_weirly_named_contact_id'. + $this->legacyWarning($e->getMessage() . " We have merged this in for now as a custom property. Please rewrite your code to use PropertyBag->setCustomProperty if it is a genuinely custom property, or a standardised setter like PropertyBag->setContactID for standard properties"); + $this->setCustomProperty($offset, $value, 'default'); + return; + } + + // Coerce legacy values that were in use but shouldn't be in our new way of doing things. + if ($prop === 'feeAmount' && $value === '') { + // At least the following classes pass in ZLS for feeAmount. + // CRM_Core_Payment_AuthorizeNetTest::testCreateSingleNowDated + // CRM_Core_Payment_AuthorizeNetTest::testCreateSinglePostDated + $value = 0; + } + + // These lines are here (and not in try block) because the catch must only + // catch the case when the prop is custom. + $setter = 'set' . ucfirst($prop); + $this->$setter($value, 'default'); + } + + /** + * Implements ArrayAccess::offsetUnset + * + * @param mixed $offset + */ + public function offsetUnset ($offset) { + $prop = $this->handleLegacyPropNames($offset); + unset($this->props['default'][$prop]); + } + + /** + * @param string $message + */ + protected function legacyWarning($message) { + $message = "Deprecated code: $message"; + $this->lastWarning = $message; + Civi::log()->warning($message); + } + + /** + * @param string $prop + * @return string canonical name. + * @throws \InvalidArgumentException if prop name not known. + */ + protected function handleLegacyPropNames($prop) { + $newName = static::$propMap[$prop] ?? NULL; + if ($newName === TRUE) { + // Good, modern name. + return $prop; + } + if ($newName === NULL) { + throw new \InvalidArgumentException("Unknown property '$prop'."); + } + // Remaining case is legacy name that's been translated. + $this->legacyWarning("We have translated '$prop' to '$newName' for you, but please update your code to use the propper setters and getters."); + return $newName; + } + + /** + * Internal getter. + * + * @param mixed $prop Valid property name + * @param string $label e.g. 'default' + */ + protected function get($prop, $label) { + if (isset($this->props['default'][$prop])) { + return $this->props[$label][$prop]; + } + throw new \BadMethodCallException("Property '$prop' has not been set."); + } + + /** + * Internal setter. + * + * @param mixed $prop Valid property name + * @param string $label e.g. 'default' + * @param mixed $value + * + * @return PropertyBag $this object so you can chain set setters. + */ + protected function set($prop, $label = 'default', $value) { + $this->props[$label][$prop] = $value; + return $this; + } + + /** + * DRY code. + */ + protected function coercePseudoConstantStringToInt(string $baoName, string $field, $input) { + if (is_numeric($input)) { + // We've been given a numeric ID. + $_ = (int) $input; + } + elseif (is_string($input)) { + // We've been given a named instrument. + $_ = (int) CRM_Core_PseudoConstant::getKey($baoName, $field, $input); + } + else { + throw new InvalidArgumentException("Expected an integer ID or a String name for $field."); + } + if (!($_ > 0)) { + throw new InvalidArgumentException("Expected an integer greater than 0 for $field."); + } + return $_; + } + + /** + */ + public function has($prop, $label = 'default') { + // We do NOT translate legacy prop names since only new code should be + // using this method, and new code should be using canonical names. + // $prop = $this->handleLegacyPropNames($prop); + return isset($this->props[$label][$prop]); + } + + /** + * This is used to merge values from an array. + * It's a transitional function and should not be used! + * + * @param array $data + */ + public function mergeLegacyInputParams($data) { + $this->legacyWarning("We have merged input params into the property bag for now but please rewrite code to not use this."); + foreach ($data as $key => $value) { + if ($value !== NULL) { + $this->offsetSet($key, $value); + } + } + } + + /** + * Throw an exception if any of the props is unset. + * + * @param array $props Array of proper property names (no legacy aliases allowed). + * + * @return PropertyBag + */ + public function require(array $props) { + $missing = []; + foreach ($props as $prop) { + if (!isset($this->props['default'][$prop])) { + $missing[] = $prop; + } + } + if ($missing) { + throw new \InvalidArgumentException("Required properties missing: " . implode(', ', $missing)); + } + return $this; + } + + // Public getters, setters. + + /** + * Get the monetary amount. + */ + public function getAmount($label = 'default') { + return $this->get('amount', $label); + } + + /** + * Get the monetary amount. + */ + public function setAmount($value, $label = 'default') { + if (!is_numeric($value)) { + throw new \InvalidArgumentException("setAmount requires a numeric amount value"); + } + + return $this->set('amount', CRM_Utils_Money::format($value, NULL, NULL, TRUE), $label); + } + + /** + * @return int + */ + public function getContactID($label = 'default'): int { + return $this->get('contactID', $label); + } + + /** + * @param int $contactID + * @param string $label + */ + public function setContactID($contactID, $label = 'default') { + // We don't use this because it counts zero as positive: CRM_Utils_Type::validate($contactID, 'Positive'); + if (!($contactID > 0)) { + throw new InvalidArgumentException("ContactID must be a positive integer"); + } + + return $this->set('contactID', $label, (int) $contactID); + } + + /** + * Getter for contributionID. + * + * @return int|null + * @param string $label + */ + public function getContributionID($label = 'default') { + return $this->get('contributionID', $label); + } + + /** + * @param int $contributionID + * @param string $label e.g. 'default' + */ + public function setContributionID($contributionID, $label = 'default') { + // We don't use this because it counts zero as positive: CRM_Utils_Type::validate($contactID, 'Positive'); + if (!($contributionID > 0)) { + throw new InvalidArgumentException("ContributionID must be a positive integer"); + } + + return $this->set('contributionID', $label, (int) $contributionID); + } + + /** + * Getter for contributionRecurID. + * + * @return int|null + * @param string $label + */ + public function getContributionRecurID($label = 'default') { + return $this->get('contributionRecurID', $label); + } + + /** + * @param int $contributionRecurID + * @param string $label e.g. 'default' + */ + public function setContributionRecurID($contributionRecurID, $label = 'default') { + // We don't use this because it counts zero as positive: CRM_Utils_Type::validate($contactID, 'Positive'); + if (!($contributionRecurID > 0)) { + throw new InvalidArgumentException("ContributionRecurID must be a positive integer"); + } + + return $this->set('contributionRecurID', $label, (int) $contributionRecurID); + } + + /** + * Three character currency code. + * + * https://en.wikipedia.org/wiki/ISO_3166-1_alpha-3 + * + * @param string $label e.g. 'default' + */ + public function getCurrency($label = 'default') { + return $this->get('currency', $label); + } + + /** + * Three character currency code. + * + * @param string $value + * @param string $label e.g. 'default' + */ + public function setCurrency($value, $label = 'default') { + if (!preg_match('/^[A-Z]{3}$/', $value)) { + throw new \InvalidArgumentException("Attemted to setCurrency with a value that was not an ISO 3166-1 alpha 3 currency code"); + } + return $this->set('currency', $label, $value); + } + + /** + * + * @param string $label + * + * @return string + */ + public function getDescription($label = 'default') { + return $this->get('description', $label); + } + + /** + * @param string $description + * @param string $label e.g. 'default' + */ + public function setDescription($description, $label = 'default') { + // @todo this logic was copied from a commit that then got deleted. Is it good? + $uninformativeStrings = [ + ts('Online Event Registration: '), + ts('Online Contribution: '), + ]; + $cleanedDescription = str_replace($uninformativeStrings, '', $description); + return $this->set('description', $label, $cleanedDescription); + } + + /** + * Amount of money charged in fees by the payment processor. + * + * This is notified by (some) payment processers. + * + * @param string $label + * + * @return float + */ + public function getFeeAmount($label = 'default') { + return $this->get('feeAmount', $label); + } + + /** + * @param string $feeAmount + * @param string $label e.g. 'default' + */ + public function setFeeAmount($feeAmount, $label = 'default') { + if (!is_numeric($feeAmount)) { + throw new \InvalidArgumentException("feeAmount must be a number."); + } + return $this->set('feeAmount', $label, (float) $feeAmount); + } + + /** + * Getter for invoiceID. + * + * @param string $label + * + * @return string|null + */ + public function getInvoiceID($label = 'default') { + return $this->get('invoiceID', $label); + } + + /** + * @param string $invoiceID + * @param string $label e.g. 'default' + */ + public function setInvoiceID($invoiceID, $label = 'default') { + return $this->set('invoiceID', $label, $invoiceID); + } + + /** + * Getter for isBackOffice. + * + * @param string $label + * + * @return bool|null + */ + public function getIsBackOffice($label = 'default'):bool { + // @todo should this return FALSE instead of exception to keep current situation? + return $this->get('isBackOffice', $label); + } + + /** + * @param bool $isBackOffice + * @param string $label e.g. 'default' + */ + public function setIsBackOffice($isBackOffice, $label = 'default') { + if (is_null($isBackOffice)) { + throw new \InvalidArgumentException("isBackOffice must be a bool, received NULL."); + } + return $this->set('isBackOffice', $label, (bool) $isBackOffice); + } + + /** + * Getter for isRecur. + * + * @param string $label + * + * @return bool|null + */ + public function getIsRecur($label = 'default'):bool { + return $this->get('isRecur', $label); + } + + /** + * @param bool $isRecur + * @param string $label e.g. 'default' + */ + public function setIsRecur($isRecur, $label = 'default') { + if (is_null($isRecur)) { + throw new \InvalidArgumentException("isRecur must be a bool, received NULL."); + } + return $this->set('isRecur', $label, (bool) $isRecur); + } + + /** + * Getter for payment processor generated string for charging. + * + * A payment token could be a single use token (e.g generated by + * a client side script) or a token that permits recurring or on demand charging. + * + * The key thing is it is passed to the processor in lieu of card details to + * initiate a payment. + * + * Generally if a processor is going to pass in a payment token generated through + * javascript it would add 'payment_token' to the array it returns in it's + * implementation of getPaymentFormFields. This will add a hidden 'payment_token' field to + * the form. A good example is client side encryption where credit card details are replaced by + * an encrypted token using a gateway provided javascript script. In this case the javascript will + * remove the credit card details from the form before submitting and populate the payment_token field. + * + * A more complex example is used by paypal checkout where the payment token is generated + * via a pre-approval process. In that case the doPreApproval function is called on the processor + * class to get information to then interact with paypal via js, finally getting a payment token. + * (at this stage the pre-approve api is not in core but that is likely to change - we just need + * to think about the permissions since we don't want to expose to anonymous user without thinking + * through any risk of credit-card testing using it. + * + * If the token is not totally transient it would be saved to civicrm_payment_token.token. + * + * @param string $label + * + * @return string|null + */ + public function getPaymentToken($label = 'default') { + return $this->get('paymentToken', $label); + } + + /** + * @param string $paymentToken + * @param string $label e.g. 'default' + */ + public function setPaymentToken($paymentToken, $label = 'default') { + return $this->set('paymentToken', $label, $paymentToken); + } + + /** + * Combined with recurFrequencyUnit this gives how often billing will take place. + * + * e.g every if this is 1 and recurFrequencyUnit is 'month' then it is every 1 month. + * @return int|null + */ + public function getRecurFrequencyInterval($label = 'default') { + return $this->get('recurFrequencyInterval', $label); + } + + /** + * @param int $recurFrequencyInterval + * @param string $label e.g. 'default' + */ + public function setRecurFrequencyInterval($recurFrequencyInterval, $label = 'default') { + if (!($recurFrequencyInterval > 0)) { + throw new InvalidArgumentException("recurFrequencyInterval must be a positive integer"); + } + + return $this->set('recurFrequencyInterval', $label, (int) $recurFrequencyInterval); + } + + /** + * Getter for recurFrequencyUnit. + * Combined with recurFrequencyInterval this gives how often billing will take place. + * + * e.g every if this is 'month' and recurFrequencyInterval is 1 then it is every 1 month. + * + * + * @param string $label + * + * @return string month|day|year + */ + public function getRecurFrequencyUnit($label = 'default') { + return $this->get('recurFrequencyUnit', $label); + } + + /** + * @param string $recurFrequencyUnit month|day|week|year + * @param string $label e.g. 'default' + */ + public function setRecurFrequencyUnit($recurFrequencyUnit, $label = 'default') { + if (!preg_match('/^day|week|month|year$/', $recurFrequencyUnit)) { + throw new \InvalidArgumentException("recurFrequencyUnit must be day|week|month|year"); + } + return $this->set('recurFrequencyUnit', $label, $recurFrequencyUnit); + } + + /** + * Getter for payment processor generated string for the transaction ID. + * + * Note some gateways generate a reference for the order and one for the + * payment. This is for the payment reference and is saved to + * civicrm_financial_trxn.trxn_id. + * + * @param string $label + * + * @return string|null + */ + public function getTransactionID($label = 'default') { + return $this->get('transactionID', $label); + } + + /** + * @param string $transactionID + * @param string $label e.g. 'default' + */ + public function setTransactionID($transactionID, $label = 'default') { + return $this->set('transactionID', $label, $transactionID); + } + + /** + * Getter for trxnResultCode. + * + * Additional information returned by the payment processor regarding the + * payment outcome. + * + * This would normally be saved in civicrm_financial_trxn.trxn_result_code. + * + * + * @param string $label + * + * @return string|null + */ + public function getTrxnResultCode($label = 'default') { + return $this->get('trxnResultCode', $label); + } + + /** + * @param string $trxnResultCode + * @param string $label e.g. 'default' + */ + public function setTrxnResultCode($trxnResultCode, $label = 'default') { + return $this->set('trxnResultCode', $label, $trxnResultCode); + } + + // Custom Properties. + + /** + * Sometimes we may need to pass in things that are specific to the Payment + * Processor. + * + * @param string $prop + * @param string $label e.g. 'default' or 'old' or 'new' + * @return mixed + * + * @throws InvalidArgumentException if trying to use this against a non-custom property. + */ + public function getCustomProperty($prop, $label = 'default') { + if (isset(static::$propMap[$prop])) { + throw new \InvalidArgumentException("Attempted to get '$prop' via getCustomProperty - must use using its getter."); + } + return $this->props[$label][$prop] ?? NULL; + } + + /** + * We have to leave validation to the processor, but we can still give them a + * way to store their data on this PropertyBag + * + * @param string $prop + * @param mixed $value + * @param string $label e.g. 'default' or 'old' or 'new' + * + * @throws InvalidArgumentException if trying to use this against a non-custom property. + */ + public function setCustomProperty($prop, $value, $label = 'default') { + if (isset(static::$propMap[$prop])) { + throw new \InvalidArgumentException("Attempted to set '$prop' via setCustomProperty - must use using its setter."); + } + $this->props[$label][$prop] = $value; + } + +} diff --git a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php index 34ad24b5b72f..17e2647bb086 100644 --- a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php @@ -1010,7 +1010,7 @@ public function testPartialPaymentWithCreditCard() { 'total_amount' => 40, 'currency' => 'USD', 'financial_type_id' => 1, - 'payment_instrument_id' => array_search('Credit card', $this->paymentInstruments), + 'payment_instrument_id' => array_search('Credit Card', $this->paymentInstruments), 'payment_processor_id' => $this->paymentProcessorID, 'credit_card_exp_date' => ['M' => 5, 'Y' => 2025], 'credit_card_number' => '411111111111111', diff --git a/tests/phpunit/Civi/Payment/PropertyBagTest.php b/tests/phpunit/Civi/Payment/PropertyBagTest.php new file mode 100644 index 000000000000..42462e4087d5 --- /dev/null +++ b/tests/phpunit/Civi/Payment/PropertyBagTest.php @@ -0,0 +1,210 @@ +apply(); + } + + protected function setUp() { + parent::setUp(); + // $this->useTransaction(TRUE); + } + + public function tearDown() { + parent::tearDown(); + } + + /** + * Test we can set a contact ID. + */ + public function testSetContactID() { + // Do things proper. + $propertyBag = new PropertyBag(); + $propertyBag->setContactID(123); + $this->assertEquals(123, $propertyBag->getContactID()); + + // Same but this time set contact ID with string. + // (php should throw its own warnings about this because of the signature) + $propertyBag = new PropertyBag(); + $propertyBag->setContactID('123'); + $this->assertInternalType('int', $propertyBag->getContactID()); + $this->assertEquals(123, $propertyBag->getContactID()); + + // Test we can have different labels + $propertyBag = new PropertyBag(); + $propertyBag->setContactID(123); + $propertyBag->setContactID(456, 'new'); + $this->assertEquals(123, $propertyBag->getContactID()); + $this->assertEquals(456, $propertyBag->getContactID('new')); + } + + /** + * Test we cannot set an invalid contact ID. + * + * @expectedException \InvalidArgumentException + */ + public function testSetContactIDFailsIfInvalid() { + $propertyBag = new PropertyBag(); + $propertyBag->setContactID(0); + } + + /** + * Test we can set a contact ID the wrong way + */ + public function testSetContactIDLegacyWay() { + $propertyBag = new PropertyBag(); + $propertyBag['contactID'] = 123; + $this->assertEquals(123, $propertyBag->getContactID()); + $this->assertEquals(123, $propertyBag['contactID']); + // There should not be any warnings yet. + $this->assertEquals("", $propertyBag->lastWarning); + + // Now access via legacy name - should work but generate warning. + $this->assertEquals(123, $propertyBag['contact_id']); + $this->assertEquals("Deprecated code: We have translated 'contact_id' to 'contactID' for you, but please update your code to use the propper setters and getters.", $propertyBag->lastWarning); + + // Repeat but this time set the property using a legacy name, fetch by new name. + $propertyBag = new PropertyBag(); + $propertyBag['contact_id'] = 123; + $this->assertEquals("Deprecated code: We have translated 'contact_id' to 'contactID' for you, but please update your code to use the propper setters and getters.", $propertyBag->lastWarning); + $this->assertEquals(123, $propertyBag->getContactID()); + $this->assertEquals(123, $propertyBag['contactID']); + $this->assertEquals(123, $propertyBag['contact_id']); + } + + /** + */ + public function testMergeInputs() { + $propertyBag = new PropertyBag(); + $propertyBag->mergeLegacyInputParams([ + 'contactID' => 123, + 'contributionRecurID' => 456, + ]); + $this->assertEquals("Deprecated code: We have merged input params into the property bag for now but please rewrite code to not use this.", $propertyBag->lastWarning); + $this->assertEquals(123, $propertyBag->getContactID()); + $this->assertEquals(456, $propertyBag->getContributionRecurID()); + } + + /** + * Test we can set and access custom props. + */ + public function testSetCustomProp() { + $propertyBag = new PropertyBag(); + $propertyBag->setCustomProperty('customThingForMyProcessor', 'fidget'); + $this->assertEquals('fidget', $propertyBag->getCustomProperty('customThingForMyProcessor')); + $this->assertEquals('', $propertyBag->lastWarning); + + // Test we can do this with array, although we should get a warning. + $propertyBag = new PropertyBag(); + $propertyBag['customThingForMyProcessor'] = 'fidget'; + $this->assertEquals('fidget', $propertyBag->getCustomProperty('customThingForMyProcessor')); + $this->assertEquals("Deprecated code: Unknown property 'customThingForMyProcessor'. We have merged this in for now as a custom property. Please rewrite your code to use PropertyBag->setCustomProperty if it is a genuinely custom property, or a standardised setter like PropertyBag->setContactID for standard properties", $propertyBag->lastWarning); + } + + /** + * Test we can't set a custom prop that we know about. + * + * @expectedException \InvalidArgumentException + * @expectedExceptionMessage Attempted to set 'contactID' via setCustomProperty - must use using its setter. + */ + public function testSetCustomPropFails() { + $propertyBag = new PropertyBag(); + $propertyBag->setCustomProperty('contactID', 123); + } + + /** + * + * @dataProvider otherParamsDataProvider + */ + public function testOtherParams($prop, $legacy_names, $valid_values, $invalid_values) { + $setter = 'set' . ucfirst($prop); + $getter = 'get' . ucfirst($prop); + + // Using the setter and getter, check we can pass stuff in and get expected out. + foreach ($valid_values as $_) { + list($given, $expect) = $_; + $propertyBag = new PropertyBag(); + $propertyBag->$setter($given); + $this->assertEquals($expect, $propertyBag->$getter()); + } + // Using the setter and getter, check we get an error for invalid data. + foreach ($invalid_values as $given) { + try { + $propertyBag = new PropertyBag(); + $propertyBag->$setter($given); + } + catch (\InvalidArgumentException $e) { + // counts this assertion. + $this->assertTrue(TRUE); + continue; + } + $this->fail("Expected an error trying to set $prop to " . json_encode($given) . " but did not get one."); + } + + // Check array access for the proper property name and any aliases. + foreach (array_merge([$prop], $legacy_names) as $name) { + // Check array access + foreach ($valid_values as $_) { + list($given, $expect) = $_; + $propertyBag = new PropertyBag(); + $propertyBag[$name] = $given; + $this->assertEquals($expect, $propertyBag->$getter(), "Failed to set $prop via array access on $name"); + // Nb. I don't feel the need to repeat all the checks above for every alias. + // We only really need to test that the array access works for each alias. + break; + } + } + } + + /** + * Test the require method works. + */ + public function testRequire() { + $propertyBag = new PropertyBag(); + $propertyBag->setContactID(123); + $propertyBag->setDescription('foo'); + // This one should not error. + $propertyBag->require(['contactID', 'description']); + try { + $propertyBag->require(['contactID', 'description', 'contributionID', 'somethingthatdoesntexist']); + } + catch (\InvalidArgumentException $e) { + $this->assertEquals('Required properties missing: contributionID, somethingthatdoesntexist', $e->getMessage()); + } + } + + /** + * + * Data provider for testOtherParams + * + */ + public function otherParamsDataProvider() { + $valid_bools = [['0' , FALSE], ['', FALSE], [0, FALSE], [FALSE, FALSE], [TRUE, TRUE], [1, TRUE], ['1', TRUE]]; + $valid_strings = [['foo' , 'foo'], ['', '']]; + $valid_ints = [[123, 123], ['123', 123]]; + $invalid_ints = [-1, 0, NULL, '']; + return [ + ['contributionID', ['contribution_id'], $valid_ints, $invalid_ints], + ['contributionRecurID', ['contribution_recur_id'], $valid_ints, $invalid_ints], + ['description', [], [['foo' , 'foo'], ['', '']], []], + ['feeAmount', ['fee_amount'], [[1.23, 1.23], ['4.56', 4.56]], [NULL]], + ['invoiceID', ['invoice_id'], $valid_strings, []], + ['isBackOffice', ['is_back_office'], $valid_bools, [NULL]], + ['isRecur', ['is_recur'], $valid_bools, [NULL]], + ['paymentToken', [], $valid_strings, []], + ['recurFrequencyInterval', ['frequency_interval'], $valid_ints, $invalid_ints], + ['recurFrequencyUnit', [], [['month', 'month'], ['day', 'day'], ['year', 'year']], ['', NULL, 0]], + ['transactionID', ['transaction_id'], $valid_strings, []], + ['trxnResultCode', [], $valid_strings, []], + ]; + } + +}