-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add deepstack gateway #4830
Add deepstack gateway #4830
Conversation
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? |
Hello,
|
Is this gateway only for credit card processing? Additionally can you post the results of running the unit and remote tests for this gateway, |
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 |
I have resolved the comments left. Please let me know if there is anything else that needs to be done :) |
There was a problem hiding this 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?
Puts statements removed :) |
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? |
There was a problem hiding this 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.
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 :) |
Hello, is there any update on this PR like changes required? |
There was a problem hiding this 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!
Sounds good! Would I edit the CHANGELOG and then squash it again? |
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 :) |
Hello is there any update on the progress of this? I have not heard back since Monday and was curious |
I have not heard back in a couple days and was wondering if there is any update to this pr. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
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. |
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? |
@khoinguyendeepstack do you need this to be a new release? |
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
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. |
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 |
I also opened a new pull request here: #4850 This is just a small change to update our sandbox credentials in fixtures.yml |
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!