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

Nuvei: Add ACH support #5269

Merged
merged 2 commits into from
Oct 18, 2024
Merged

Nuvei: Add ACH support #5269

merged 2 commits into from
Oct 18, 2024

Conversation

javierpedrozaing
Copy link
Collaborator

Description

SER-1403

This commit add ACH transaction for Nuvei

Unit test

Finished in 0.010487 seconds

10 tests, 34 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed

53.56 tests/s, 3242.11 assertions/s

Remote test

Finished in 36.783565 seconds.

18 tests, 48 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed

0.49 tests/s, 1.30 assertions/s

Rubocop

801 files inspected, no offenses detected

Copy link
Collaborator

@Heavyblade Heavyblade left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nince and clean @javierpedrozaing

@@ -315,7 +338,7 @@ def parse(body)
end

def success_from(response)
response[:status] == 'SUCCESS' && response[:transactionStatus] == 'APPROVED'
response[:status] == 'SUCCESS' && (response[:transactionStatus] == 'APPROVED' || response[:transactionStatus] == 'PENDING')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Question:

I'm seeing a notification_url field + a 'pending' status on the transaction, does that mean that ACH payments on this gateway are async?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Heavyblade yes this ensures that we receive transaction results that may not always be immediately available cause in the status response we can get PENDING to UPDATE to APPROVED.


def test_successful_authorize_with_bank_account
@options.update(billing_address: address.merge(country: 'US', state: 'MA'))
response = @gateway.authorize(1.25, @bank_account, @options)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Question:

Just out of curiosity => you are doing a transaction for 1.25 cents, no dollars, and it works?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the 1.25 is a Nuvei test value for Successful deposit

@@ -149,6 +151,19 @@ def test_successful_stored_credentials_merchant_recurring
end.respond_with(successful_purchase_response)
end

def test_successful_authorize_bank_account
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Comment:

It's not a big issue, bu what's the purpose of setting and instance variable @bank_account in the before block if it will just be used once?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's because the bank account can be used with payouts, so we will eventually add more tests for the bank account

@javierpedrozaing javierpedrozaing force-pushed the SER-1403_nuvei_ach_implementation branch from 97051e7 to 3c22922 Compare October 7, 2024 15:31
@javierpedrozaing javierpedrozaing marked this pull request as ready for review October 7, 2024 15:34
Copy link
Contributor

@naashton naashton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

gsub(%r(("merchantKey\\?":\\?")\d+), '\1[FILTERED]')
gsub(%r(("merchantKey\\?":\\?")\d+), '\1[FILTERED]').
gsub(%r(("accountNumber\\?":\\?")\d+), '\1[FILTERED]').
gsub(%r(("routingNumber\\?":\\?")\d+), '\1[FILTERED]')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove routingNumber from the scrub. This isn't a sensitive value we need to protect.

@naashton naashton force-pushed the SER-1403_nuvei_ach_implementation branch from 3c22922 to 1cf2c79 Compare October 18, 2024 15:59
Description
-------------------------
[SER-1403](https://spreedly.atlassian.net/browse/SER-1403)

This commit add ACH transaction for Nuvei

Unit test
-------------------------
Finished in 0.010487 seconds

10 tests, 34 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

53.56 tests/s, 3242.11 assertions/s

Remote test
-------------------------
Finished in 36.783565 seconds.

18 tests, 48 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed

0.49 tests/s, 1.30 assertions/s

Rubocop
-------------------------
801 files inspected, no offenses detected
@naashton naashton force-pushed the SER-1403_nuvei_ach_implementation branch from 1cf2c79 to 49868b3 Compare October 18, 2024 16:00
@naashton naashton force-pushed the SER-1403_nuvei_ach_implementation branch from 9bc376b to 0e8869e Compare October 18, 2024 16:02
@naashton naashton merged commit 74848ea into master Oct 18, 2024
5 checks passed
@naashton naashton deleted the SER-1403_nuvei_ach_implementation branch October 18, 2024 16:08
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

Successfully merging this pull request may close these issues.

3 participants