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.

Sign up for GitHub

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

Jump to bottom

Add deepstack gateway #4830

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Add deepstack gateway #4830

merged 1 commit into from
Aug 9, 2023

Conversation

khoinguyendeepstack
Copy link
Contributor

Hello, my name is Khoi Nguyen and I am a developer at Deepstack (powered by Banc of California) and we are looking to add our gateway onto Active Merchant. If there is anything I can do to help the process (bug fixes or anything), please let me know!

@khoinguyendeepstack
Copy link
Contributor Author

Hello, are there any pending steps that I should do currently?

@aenand
Copy link
Contributor

aenand commented Jul 21, 2023

Hello, are there any pending steps that I should do currently?

Hello! Thanks for creating this PR. Could you provide some insight into the onboarding process for merchants, future plans for this gateway in ActiveMerchant, and if you have had any contact with Spreedly or Shopify for this request?

@khoinguyendeepstack
Copy link
Contributor Author

Hello,

  1. The onboarding would be done through our operations team, so merchant credentials would already be generated by the time they pull from Active Merchant. If you need specifics, I can work with them to get those answers.
  2. Our plan is use this gateway for merchant's using Ruby. Our only future plans currently are to add payment-instrument creation/deletion on top of the existing tokenization.
  3. We have not had any contact with Spreedly or Shopify for this request

@aenand
Copy link
Contributor

aenand commented Jul 24, 2023

Is this gateway only for credit card processing? Additionally can you post the results of running the unit and remote tests for this gateway, bundle exec rake test to run all unit tests, and bundle exec rubocop for linting purposes.

@khoinguyendeepstack
Copy link
Contributor Author

khoinguyendeepstack commented Jul 24, 2023 •
edited
Loading

Hello, the gateway is currently only going to be for credit/debit card processing. There may be plans to implement ach in the future, but nothing for that has been implemented right now. I have attached the results of running those commands below.

There was is error with the unit test because I had to turn SSL off when running from my company device because our VPN self-signs the certificate. The test should work from any other device though.

I pushed a new commit to fix the existing Rubocop offenses

image

image

@khoinguyendeepstack
Copy link
Contributor Author

Update: was able to fix the SSL issue so now this is the result from running bundle exec rake test

image

The errors there are from the NMI tests, not from Deepstack

image

aenand
aenand requested changes Jul 25, 2023
View reviewed changes
test/remote/gateways/remote_deepstack_test.rb Outdated Show resolved Hide resolved
test/remote/gateways/remote_deepstack_test.rb Show resolved Hide resolved
test/remote/gateways/remote_deepstack_test.rb Outdated Show resolved Hide resolved
test/remote/gateways/remote_deepstack_test.rb Outdated Show resolved Hide resolved
test/unit/gateways/deepstack_test.rb Outdated Show resolved Hide resolved
lib/active_merchant/billing/gateways/deepstack.rb Outdated Show resolved Hide resolved
lib/active_merchant/billing/gateways/deepstack.rb Outdated Show resolved Hide resolved
lib/active_merchant/billing/gateways/deepstack.rb Outdated Show resolved Hide resolved
lib/active_merchant/billing/gateways/deepstack.rb Outdated Show resolved Hide resolved
lib/active_merchant/billing/gateways/deepstack.rb Outdated Show resolved Hide resolved
@khoinguyendeepstack
Copy link
Contributor Author

I have resolved the comments left. Please let me know if there is anything else that needs to be done :)

aenand
aenand requested changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@aenand aenand left a comment

Choose a reason for hiding this comment

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

Looks good! Do you mind removing all of the puts statements?

@khoinguyendeepstack
Copy link
Contributor Author

Puts statements removed :)

@khoinguyendeepstack
Copy link
Contributor Author

Hello, should I proceed and squash to one commit, or should I be waiting for a final confirmation

@aenand
Copy link
Contributor

aenand commented Jul 27, 2023

Hello, should I proceed and squash to one commit, or should I be waiting for a final confirmation

If you could please squash it that would be great! I will take a final look on Monday if that works for you?

BritneyS
BritneyS requested changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@BritneyS BritneyS left a comment

Choose a reason for hiding this comment

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

Hi, this looks good overall! Just a couple areas of concern and some commented code to address.

lib/active_merchant/billing/gateways/deepstack.rb Show resolved Hide resolved
lib/active_merchant/billing/gateways/deepstack.rb Outdated Show resolved Hide resolved
lib/active_merchant/billing/gateways/deepstack.rb Outdated Show resolved Hide resolved
lib/active_merchant/billing/gateways/deepstack.rb Outdated Show resolved Hide resolved
lib/active_merchant/billing/gateways/deepstack.rb Outdated Show resolved Hide resolved
test/unit/gateways/deepstack_test.rb Outdated Show resolved Hide resolved
@khoinguyendeepstack
Copy link
Contributor Author

I have resolved the issues by Britney in my most recent commit. If there are no issues I will squash the branch at the end of the day today :)

@khoinguyendeepstack
Copy link
Contributor Author

Hello, is there any update on this PR like changes required?

aenand
aenand approved these changes Jul 31, 2023
View reviewed changes
Copy link
Contributor

@aenand aenand left a comment

Choose a reason for hiding this comment

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

This looks good to me! Once @BritneyS gives her approval I think you're good to create a CHANGELOG entry for it and then one of us will merge it for you. Thank you!

@khoinguyendeepstack
Copy link
Contributor Author

Sounds good! Would I edit the CHANGELOG and then squash it again?

@khoinguyendeepstack
Copy link
Contributor Author

Is there an estimation of when the final review from @BritneyS will be? I would like to have the gateway added as soon as possible :)

@khoinguyendeepstack
Copy link
Contributor Author

Hello is there any update on the progress of this? I have not heard back since Monday and was curious

@khoinguyendeepstack
Copy link
Contributor Author

I have not heard back in a couple days and was wondering if there is any update to this pr.

BritneyS
BritneyS approved these changes Aug 7, 2023
View reviewed changes
Copy link
Contributor

@BritneyS BritneyS left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@khoinguyendeepstack
Copy link
Contributor Author

Thanks! Would the next step be to add to changelog and squash again? Or would that be separate from this branch

@aenand
Copy link
Contributor

aenand commented Aug 8, 2023

Thanks! Would the next step be to add to changelog and squash again? Or would that be separate from this branch

Yep! Once you add a changelog entry, squash, and rebase on master we can merge this for you.

@khoinguyendeepstack
Copy link
Contributor Author

Done! Let me know if anything else is needed. Thank you for all the help!!!

@aenand
Copy link
Contributor

aenand commented Aug 9, 2023

Done! Let me know if anything else is needed. Thank you for all the help!!!

Looks good! There seems to be some merge conflict 👀 do you mind resolving that?

@aenand
Copy link
Contributor

aenand commented Aug 9, 2023

@khoinguyendeepstack do you need this to be a new release?

@khoinguyendeepstack
Adding deepstack gateway
a9e3378 
Baseline for future store feature

Remove some comments

Updating urls

Added shipping

Remove some comments

Updating urls

Added shipping

Remove some comments

Updating urls

Added shipping

Remove some comments

Updating urls

Added shipping

Rubocop offense fixes

Fixing PR comments

Remove unused credentials

Added missing tests. Removed :sandbox for test?

Removing puts statements

Move set capture true to separate function + remove code comments

Update changelog
@khoinguyendeepstack
Copy link
Contributor Author

Fixed the merge conflict! I guess a new changelog update was pushed in between our messages haha... I do not require this to be a new release as long as it is merged in.

@aenand aenand merged commit 3eaa3ca into activemerchant:master Aug 9, 2023
5 checks passed
@khoinguyendeepstack khoinguyendeepstack deleted the add_deepstack_gateway branch August 9, 2023 23:51
@khoinguyendeepstack
Copy link
Contributor Author

Hello, just wondering what the timeline is for new feature releases? Previously I said that a new release wouldn't be required, but wanted to know when the newest release with our gateway added would be

@khoinguyendeepstack
Copy link
Contributor Author

I also opened a new pull request here: #4850

This is just a small change to update our sandbox credentials in fixtures.yml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BritneyS BritneyS BritneyS approved these changes

@aenand aenand aenand approved these changes

@mexpadmar mexpadmar Awaiting requested review from mexpadmar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@khoinguyendeepstack @aenand @BritneyS @mexpadmar