Skip to content
This repository has been archived by the owner on Sep 10, 2019. It is now read-only.

adds comments about confusing test mode nomenclature mis-match… #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion app/models/spree/gateway/authorize_net.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,27 @@ module Spree
class Gateway::AuthorizeNet < Gateway
preference :login, :string
preference :password, :string
preference :server, :string, :default => "live"

# WARNING: Unlike other payment integrations, 'test mode' in Authorize Net's termiminology does not mean use test server
# instead, use preferred server set to either 'live' or 'test'
# DO NOT TURN TEST MODE TO ON (EVEN IN QA/STAGING ENVIRONMENTS), it will return 0 as transaction ids under all circumstances

# here, it overloads the setting on the base class, which confusingly defaults to true
preference :test_mode, :boolean, :default => false
Copy link
Member

Choose a reason for hiding this comment

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

If we want this to always be false, maybe we should override the implementation of preferred_test_mode to always return false.


def provider_class
ActiveMerchant::Billing::AuthorizeNetGateway
end

def options_with_test_preference
options_without_test_preference.merge(test: self.preferred_test_mode)
if !['live','test'].include?(self.preferred_server)
ActiveSupport::Deprecation.warn("You must set your preferred server to either 'live' or 'test'")
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be more appropriate to throw an exception here. If someone has this misconfigured it would not behave as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My original implementation was to raise but @jhawthorn asked me to change it to be a Warning.

end
# warning: 'test' parameter indicates live or test server; it DOES NOT indicate Authorize.net test-mode
options_without_test_preference.merge(:test => (self.preferred_server != "live") )
end

alias_method_chain :options, :test_preference

def credit(amount, response_code, refund, gateway_options = {})
Expand Down
16 changes: 15 additions & 1 deletion app/models/spree/gateway/authorize_net_cim.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,30 @@ module Spree
class Gateway::AuthorizeNetCim < Gateway
preference :login, :string
preference :password, :string

# WARNING: Unlike other payment integrations, 'test mode' in Authorize Net's termiminology does not mean use test server
# instead, use preferred server set to either 'live' or 'test'
# DO NOT TURN TEST MODE TO ON (EVEN IN QA/STAGING ENVIRONMENTS), it will return 0 as transaction ids under all circumstances

# here, it overloads the setting on the base class, which confusingly defaults to true
preference :test_mode, :boolean, :default => false

preference :validate_on_profile_create, :boolean, :default => false
preference :server, :string, :default => "live"


def provider_class
self.class
end

def options
if !['live','test'].include?(self.preferred_server)
ActiveSupport::Deprecation.warn("You must set your preferred server to either 'live' or 'test'")
end
# add :test key in the options hash, as that is what the ActiveMerchant::Billing::AuthorizeNetGateway expects
if self.preferred_test_mode
# warning: when passed to the gateway code, :test indicates live/test server; it DOES NOT indicate authorize.net test-mode

if self.preferred_server != "live"
self.preferences[:test] = true
else
self.preferences.delete(:test)
Expand Down
8 changes: 4 additions & 4 deletions spec/models/gateway/authorize_net_cim_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@
end

describe 'options' do
it 'include :test => true when :test_mode is true' do
gateway.preferred_test_mode = true
it 'include :test => true when test server is "test"' do
gateway.preferred_server = "test"
expect(gateway.options[:test]).to be true
end

it 'does not include :test when :test_mode is false' do
gateway.preferred_test_mode = false
it 'does not include :test when test server is "live"' do
gateway.preferred_server = "live"
expect(gateway.options[:test]).to be_nil
end
end
Expand Down
12 changes: 6 additions & 6 deletions spec/models/gateway/authorize_net_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,14 @@
end

describe 'options' do
it 'include :test => true when :test_mode is true' do
gateway.preferred_test_mode = true
expect(gateway.options[:test]).to be true
it 'include :test => true when server is "test"' do
gateway.preferred_server = "test"
expect(gateway.options[:test]).to be_truthy
end

it 'does not include :test when test_mode is false' do
gateway.preferred_test_mode = false
expect(gateway.options[:test]).to be false
it 'does not include :test when server is "live"' do
gateway.preferred_server = "live"
expect(gateway.options[:test]).to be_falsy
end
end
end