eileenmcnaughton / nz.co.fuzion.omnipaymultiprocessor

Omnipay Multi Processor Payment Processor For CiviCRM
Other
13 stars 44 forks source link

Error with payments > 1000 and Windcave processor #227

Open stesi561 opened 2 years ago

stesi561 commented 2 years ago

To replicate set up Omnipay with Windcave Try and donate $1000 Get an annoying error about a comma. Be puzzled as to why someone would reject your generous donation (or payment), and the source of said comma.

As per discussion on chat -> https://chat.civicrm.org/civicrm/pl/kprwsk6btfgo5kzi8t5cicmeih

It looks like Windcave doesn't like formatted strings as a value!

A work around is to replace the comma in the value before it gets sent through to Windcave, however this assumes a comma for a separator - it might be something else depending on your locale.

--- a/nz.co.fuzion.omnipaymultiprocessor/CRM/Core/Payment/OmnipayMultiProcessor.php
+++ b/nz.co.fuzion.omnipaymultiprocessor/CRM/Core/Payment/OmnipayMultiProcessor.php
@@ -556,7 +556,7 @@ private function getSensitiveCreditCardObjectOptions($params) {
    */
   protected function getCreditCardOptions(array $params): array {
     $creditCardOptions = [
-      'amount' => $this->getAmount($params),
+      'amount' => str_replace(',', '', $this->getAmount($params)),
       'currency' => $this->getCurrency($params),
       'description' => $this->getPaymentDescription($params),
       'transactionId' => $this->formatted_transaction_id,
eileenmcnaughton commented 2 years ago

@stesi561 that should be machine money formatted by the time it reaches that point in the code - I'm not sure if the getAmount function is adding the comma or it is already there?


protected function getAmount($params = []) {
    if (!CRM_Utils_Rule::numeric($params['amount'])) {
      CRM_Core_Error::deprecatedWarning('Passing Amount value that is not numeric is deprecated please report this in gitlab');
      return CRM_Utils_Money::formatUSLocaleNumericRounded(filter_var($params['amount'], FILTER_SANITIZE_NUMBER_FLOAT, FILTER_FLAG_ALLOW_FRACTION), 2);
    }
    return CRM_Utils_Money::formatUSLocaleNumericRounded($params['amount'], 2);
  }```
stesi561 commented 2 years ago

yes but when it leaves formatUSLocaleNumericRounded it comes back as a string with a comma in it - I assume only if you have a locale that has a thousands separator with a comma.

eileenmcnaughton commented 2 years ago

@stesi561 I think this might be right https://github.com/civicrm/civicrm-core/pull/23356

eileenmcnaughton commented 2 years ago

Note that Omnipay doesn't need to call getAmount - it could just use $params['amount'] since that should be reliably formatted now & that is probably the right fix for Omnipay

eileenmcnaughton commented 2 years ago

I updated Omnipay to just access $params['amount'] - it turns out that doesn't pad out the decimals so I also did a fix in core so getAmount works a bit better - but I doubt the padding will matter for Omnipay- I would expect the Omnipay library to handle any padding

petednz commented 2 years ago

Eileen - Luke says you submitted a PR for a permanent fix - i assume that means to Core - any idea where this sits now? I think we are still running a patched version (r26392 for my own records)

eileenmcnaughton commented 2 years ago

I think the latest Omnipay is OK without the core fix from the above?