Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error with payments > 1000 and Windcave processor #227

Open
stesi561 opened this issue May 4, 2022 · 7 comments
Open

Error with payments > 1000 and Windcave processor #227

stesi561 opened this issue May 4, 2022 · 7 comments

Comments

@stesi561
Copy link

stesi561 commented May 4, 2022

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
Copy link
Owner

eileenmcnaughton commented May 4, 2022

@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
Copy link
Author

stesi561 commented May 4, 2022

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 added a commit to eileenmcnaughton/civicrm-core that referenced this issue May 4, 2022
Probably the function doesn't add anything as params['amount']
should be correctly formatted - however,
it shouldn't add a comma - per
eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#227
@eileenmcnaughton
Copy link
Owner

@stesi561 I think this might be right civicrm/civicrm-core#23356

@eileenmcnaughton
Copy link
Owner

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 added a commit to eileenmcnaughton/civicrm-core that referenced this issue May 4, 2022
Probably the function doesn't add anything as params['amount']
should be correctly formatted - however,
it shouldn't add a comma - per
eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#227
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this issue May 5, 2022
Probably the function doesn't add anything as params['amount']
should be correctly formatted - however,
it shouldn't add a comma - per
eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#227
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this issue May 5, 2022
Probably the function doesn't add anything as params['amount']
should be correctly formatted - however,
it shouldn't add a comma - per
eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#227
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this issue May 5, 2022
Probably the function doesn't add anything as params['amount']
should be correctly formatted - however,
it shouldn't add a comma - per
eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#227
eileenmcnaughton added a commit to eileenmcnaughton/civicrm-core that referenced this issue May 5, 2022
Probably the function doesn't add anything as params['amount']
should be correctly formatted - however,
it shouldn't add a comma - per
eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor#227
@eileenmcnaughton
Copy link
Owner

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
Copy link
Contributor

petednz commented Aug 3, 2022

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants