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

Square Payments Gateway #3441

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

danwetherald
Copy link

Basic tests for both unit and remote working for all required methods. Let me know if you guys have any questions. Plan to add more functionalities and tests in the future.

Enjoy,

Dan

@alexdunae
Copy link
Contributor

alexdunae commented Nov 22, 2019

I would love to see this included.

I know Square was declined in 2017 ( #2242 ) with the rationale that Square had a separate JS tokenisation step:

As it relates to this PR, gateway adapters that only provide a token-based transaction flow are necessarily breaking the gateway agnosticism ActiveMerchant provides since the only way that token can be created is through some other ancillary gateway-specific flow.

Square ended up creating their own gem ( https://github.com/square/active_merchant_square ) but it's quite out of date and no longer works with the current version of their API.

As we approach 2020, more and more gateways are pushing a JS tokenisation-first workflow, so I wonder if it's time to revisit the original reason for declining the PR.

The PR itself looks solid to me (just a bystander who uses AM a lot). If the ActiveMerchant maintainers are interested in considering this I'd be happy to contribute tests and review.

Copy link
Contributor

@alexdunae alexdunae left a comment

Choose a reason for hiding this comment

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

A few random notes as I was spelunking through this in my own fork tonight, for what they're worth. No idea if AM will be into merging this, but I figured I should write them down somewhere.

lib/active_merchant/billing/gateways/square.rb Outdated Show resolved Hide resolved
lib/active_merchant/billing/gateways/square.rb Outdated Show resolved Hide resolved
@danwetherald
Copy link
Author

Thanks @alexdunae for taking a look. I plan to add the OAuth actions in order to acquire the access tokens and refresh them when needed.

Appreciate the review, I agree on all 3 items brought to attention and will make the appropriate changes today.

Thanks, hope to see this merged in as well 🙌

@alexdunae
Copy link
Contributor

@dan003400 sweet. I've bolted your PR onto my main app's test suite and am working through various test scenarios against the Square sandbox. I can add any more notes here if that's useful, or wait if it just adds noise.

@danwetherald
Copy link
Author

@alexdunae please add as much as you would like to the conversation, more the better. 👍

@danwetherald
Copy link
Author

danwetherald commented Dec 2, 2019

@alexdunae let me know if you like the changes.

@alexdunae
Copy link
Contributor

I did a round on this today and made some changes, mostly adding the proper AVS and CVV responses.

Application fees seem to be working properly. I'm not using the customer creation features currently but would be happy to spend more time on this if we got any indication from ActiveMerchant they're interested in potentially.

Changes here: alexdunae@05e88e6

@danwetherald
Copy link
Author

Just wanted to bump this, is there any interest in getting this into master?

@imgarylai
Copy link

Looking forward to seeing this PR be merged!

@danwetherald
Copy link
Author

I have add this in product for months and works flawlessly. Let me know how we can get this in.

@imgarylai
Copy link

I'm using @dan003400 's version and it works fine on production. However, in order to keep up with the latest active merchant update. I have created a repo based on @dan003400 works. Looking forward to seeing this PR to be merged!

Temp solution repo is here: https://github.com/machiyami/active_merchant_square

@danwetherald
Copy link
Author

I'm using @dan003400 's version and it works fine on production. However, in order to keep up with the latest active merchant update. I have created a repo based on @dan003400 works. Looking forward to seeing this PR to be merged!

Temp solution repo is here: https://github.com/machiyami/active_merchant_square

I will work on getting master merged into my branch so it has the latest changes included.

@danwetherald
Copy link
Author

@imgarylai - Sorry for the delay, got the latest version of master into this fork. 👍🏻

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