Fix non thread safe gateway initialization

[bitberry-dev]

Hi!

Noticed non thread safe code here

solidus/core/app/models/spree/payment_method.rb

Line 134 in e7260a2

ActiveMerchant::Billing::Base.mode = gateway_options[:server].to_sym

It can cause problems with payments. And this kind of setup should be used only in initializers.

Instead of this I instantiate gateway object with test option based on :server and :test_mode.

btw, :test_mode option has no effect in active merchant gateways.

[houndci-bot]

Style/AndOr: Use || instead of or.

[mdesantis]

Current implementation is not thread safe, but ||= operator isn't thread safe neither: https://tenderlovemaking.com/2011/10/20/connection-management-in-activerecord.html

We should use a mutex in order to make it thread safe: https://lucaguidi.com/2014/03/27/thread-safety-with-ruby/

[bitberry-dev]

Wow, the question here is not in ||= operator used to set instance variable :)) but that you can't change the class variable (global) and expect that the gateway instance will use exactly this value. In next call this class variable will be overwritten and all gateway instances will use it, not one from their preference.

[mdesantis]

Oh ok, I got it now :) my apologies!

[mdesantis]

Oh ok, I got it now :) my apologies!

[spaghetticode]

@bitberryru I think the changes make sense, thank you! 👏

For reference and understandability, as the changes deal with the activemerchant gem codebase, I'm cut & pasting the relevant code from the currently bundled version:

# from lib/active_merchant/billing/gateway.rb

# Initialize a new gateway.
#
# See the documentation for the gateway you will be using to make sure there are no other
# required options.
def initialize(options = {})
  @options = options
end

# Are we running in test mode?
def test?
  (@options.has_key?(:test) ? @options[:test] : Base.test?)
end

[kennyadsl]

@bitberryru thanks for your PR! 🎉

I also noticed we are doing something similar here but I'm not sure we are actually using that class anywhere, maybe we can deprecate it... 🤔

Correct me if I'm wrong but I think that in terms of backward compatibility, this is not going to break any store since, if now "test mode" is set by using the name of the :server preference, we can say working stores have all the gateways configured with the :server preference set to production. If that's true, they will continue working correctly, right?

[bitberry-dev]

@kennyadsl Yes, it should not break any store, but when updating it is still worth checking the settings of payment methods :) Only payment methods where server preference set to production and test_mode turned off will work in "production" mode.

I also noticed we are doing something similar here but I'm not sure we are actually using that class anywhere, maybe we can deprecate it... 🤔

Wow, I didn't see it before and never use 😃 This code must also be fixed, if you are not going to delete this class, I can fix it.

[bitberry-dev]

Spree::BillingIntegration also fixed.