Skip to content

Remove server and test_mode from README#66

Merged
spaghetticode merged 1 commit into
solidusio:masterfrom
adammathys:patch-1
Sep 25, 2020
Merged

Remove server and test_mode from README#66
spaghetticode merged 1 commit into
solidusio:masterfrom
adammathys:patch-1

Conversation

@adammathys

Copy link
Copy Markdown
Member

As it turns out, neither of these values are used by any of the Stripe gateway code. They aren't referenced anywhere in this extension nor in either of the ActiveMerchant gateways. [1,2] Including them only serves to confuse users. (I know I was certainly confused when I came across some configuration that had them set to use "production" when they were instead connecting to a sandbox account.)

  1. https://github.com/activemerchant/active_merchant/blob/master/lib/active_merchant/billing/gateways/stripe.rb
  2. https://github.com/activemerchant/active_merchant/blob/master/lib/active_merchant/billing/gateways/stripe_payment_intents.rb

@spaghetticode

Copy link
Copy Markdown
Member

@adammathys I think they're used by Solidus in the gateway method and passed down to Active Merchant via the test option that is used in te hStripeGateway superclass.

@adammathys

Copy link
Copy Markdown
Member Author

@spaghetticode That is correct, but Gateway#test? isn't used by any of the code. It's not exposed by this extension or Solidus. And neither of the Stripe gateways use it.

Right now you can set both values to indicate that you want to be in production and have it still connect to a sandbox environment. Or possibly more worryingly, set them as if you're running against a test account and have all of your requests go to a production one instead. There are thankfully other safeguards in place on Stripe's end to protect you from doing anything too damaging in those scenarios, but I'd still maintain that the settings serve no real purpose for this extension other than to possibly confuse users.

@aldesantis

Copy link
Copy Markdown
Member

I agree that, even though the values are passed to ActiveMerchant, we shouldn't give people a false impression that they're actually having any effect. At some point, we'll have to completely rethink those two options by consolidating them into one and only showing them when it makes sense, but that will have to happen at the core level. For the time being, I'm happy to remove them from the readme.

@spaghetticode

Copy link
Copy Markdown
Member

@adammathys can you please rebase with master? CI was failing due to some Stripe UI changes that are now addressed (at least until they change something else again 😭) . Thanks 🙏

As it turns out, neither of these values are used by any of the Stripe gateway code. They aren't referenced anywhere in this extension nor in either of the `ActiveMerchant` gateways. [1,2] Including them only serves to confuse users. (I know I was certainly confused when I came across some configuration that had them set to use "production" when they were instead connecting to a sandbox account.)


1. https://github.com/activemerchant/active_merchant/blob/master/lib/active_merchant/billing/gateways/stripe.rb
2. https://github.com/activemerchant/active_merchant/blob/master/lib/active_merchant/billing/gateways/stripe_payment_intents.rb
@spaghetticode spaghetticode merged commit 8130e09 into solidusio:master Sep 25, 2020
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