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.

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

Normalize API versions #5371

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
1 change: 1 addition & 0 deletions lib/active_merchant/billing.rb
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
require 'active_merchant/errors'
require 'active_merchant/versionable'

require 'active_merchant/billing/avs_result'
require 'active_merchant/billing/cvv_result'
Expand Down
1 change: 1 addition & 0 deletions lib/active_merchant/billing/gateway.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ module Billing # :nodoc:
class Gateway
include PostsData
include CreditCardFormatting
include Versionable

CREDIT_DEPRECATION_MESSAGE = 'Support for using credit to refund existing transactions is deprecated and will be removed from a future release of ActiveMerchant. Please use the refund method instead.'
RECURRING_DEPRECATION_MESSAGE = 'Recurring functionality in ActiveMerchant is deprecated and will be removed in a future version. Please contact the ActiveMerchant maintainers if you have an interest in taking ownership of a separate gem that continues support for it.'
Expand Down
11 changes: 6 additions & 5 deletions lib/active_merchant/billing/gateways/adyen.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ class AdyenGateway < Gateway
self.homepage_url = 'https://www.adyen.com/'
self.display_name = 'Adyen'

PAYMENT_API_VERSION = 'v68'
RECURRING_API_VERSION = 'v68'
version 'v68', :payment_api
version 'v68', :payout_api
version 'v68', :recurring_api

STANDARD_ERROR_CODE_MAPPING = {
'0' => STANDARD_ERROR_CODE[:processing_error],
Expand Down Expand Up @@ -870,11 +871,11 @@ def cvv_result_from(response)
def endpoint(action)
case action
when 'disable', 'storeToken'
"Recurring/#{RECURRING_API_VERSION}/#{action}"
"Recurring/#{fetch_version(:recurring_api)}/#{action}"
when 'payout'
"Payout/#{PAYMENT_API_VERSION}/#{action}"
"Payout/#{fetch_version(:payout_api)}/#{action}"
else
"Payment/#{PAYMENT_API_VERSION}/#{action}"
"Payment/#{fetch_version(:payment_api)}/#{action}"
end
end

Expand Down
3 changes: 3 additions & 0 deletions lib/active_merchant/billing/gateways/braintree_blue.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ class BraintreeBlueGateway < Gateway

self.display_name = 'Braintree (Blue Platform)'

version Braintree::Configuration::API_VERSION
version Braintree::Version::String, :gem

ERROR_CODES = {
cannot_refund_if_unsettled: 91506
}
Expand Down
5 changes: 4 additions & 1 deletion lib/active_merchant/billing/gateways/global_collect.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ class GlobalCollectGateway < Gateway
self.money_format = :cents
self.supported_cardtypes = %i[visa master american_express discover naranja cabal tuya patagonia_365]

version 'v1'
version 'v2', :ogone_direct

def initialize(options = {})
requires!(options, :merchant_id, :api_key_id, :secret_api_key)
super
Expand Down Expand Up @@ -443,7 +446,7 @@ def ogone_direct?
end

def uri(action, authorization)
version = ogone_direct? ? 'v2' : 'v1'
version = ogone_direct? ? fetch_version(:ogone_direct) : fetch_version
uri = "/#{version}/#{@options[:merchant_id]}/"
case action
when :authorize, :verify
Expand Down
29 changes: 29 additions & 0 deletions lib/active_merchant/versionable.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
module ActiveMerchant
module Versionable
def self.included(base)
if base.respond_to?(:class_attribute)
base.class_attribute :versions, default: {}
base.extend(ClassMethods)
end
end

module ClassMethods
def inherited(subclass)
super
subclass.versions = {}
end
Comment on lines +11 to +14
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After the recent issue with this change where gateways would share versions, I added this hook, so a new hash would be created each time the gateway class is inherited. Let me know what you think of this.


def version(version, feature = :default_api)
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this method used? Is it just for the versionable_test.rb?

Copy link
Collaborator Author

@Buitragox Buitragox Dec 20, 2024

Choose a reason for hiding this comment

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

This method is inside the ClassMethods module. This module is extended to the base class in line 6, so any methods inside this module will be class methods. This allows for the versions to be set and fetch without creating an instance.

The intended usage for this method is to be used at the top of any Gateway subclass.

class MyTestGateway < Gateway
  version 'v3' # This sets the :default_api key in the versions hash.
  version 'v5', :recurring_api # This sets the :recurring_api key in the versions hash
end

You can see this method being used in this PR in adyen.rb, braintree_blue.rb and global_collect.rb.

versions[feature] = version
end

def fetch_version(feature = :default_api)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also this one, how is it used? What is the difference between this one and the one in line 25?

Copy link
Collaborator Author

@Buitragox Buitragox Dec 20, 2024

Choose a reason for hiding this comment

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

The fetch_version method inside ClassMethods is intended to be used outside of instance methods.

For example, some gateways create live_url and test_url at the top of the file without creating an instance of the class, so you can do something like this:

module ActiveMerchant # :nodoc:
  module Billing # :nodoc:
    class ForteGateway < Gateway
      version 'v2'

      self.test_url = "https://sandbox.forte.net/api/#{fetch_version}"
      self.live_url = "https://api.forte.net/#{fetch_version}"

      # other methods ...
    end
  end
end

The fetch_version method outside ClassMethods is intended to be used inside of instance methods. There are a couple of examples of this in this PR, you can see it being used for the Adyen gateway and GlobalCollect gateway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe Something with less "moving parts" using class instance variables like this:

module ActiveMerchant
  module Versionable
    def self.included(base)
      base.extend(ClassMethods)
    end

    module ClassMethods
      def versions
        @versions ||= {}
      end

      def version(version, feature = :default_api)
        versions[feature] = version
      end

      def fetch_version(feature = :default_api)
        versions[feature]
      end
    end

    def fetch_version(feature = :default_api)
      self.class.versions[feature]
    end
  end
end

I Already ran unit tests and seems to work properly 😉

versions[feature]
end
end

def fetch_version(feature = :default_api)
versions[feature]
end
end
end
6 changes: 6 additions & 0 deletions test/unit/gateways/adyen_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,12 @@ def setup
# assert response
# assert_success response
# end
def test_endpoint
assert_equal 'Recurring/v68/disable', @gateway.send(:endpoint, 'disable')
assert_equal 'Recurring/v68/storeToken', @gateway.send(:endpoint, 'storeToken')
assert_equal 'Payout/v68/payout', @gateway.send(:endpoint, 'payout')
assert_equal 'Payment/v68/authorise', @gateway.send(:endpoint, 'authorise')
end

def test_supported_card_types
assert_equal AdyenGateway.supported_cardtypes, %i[visa master american_express diners_club jcb dankort maestro discover elo naranja cabal unionpay patagonia_365]
Expand Down
4 changes: 4 additions & 0 deletions test/unit/gateways/braintree_blue_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ def teardown
$VERBOSE = @old_verbose
end

def test_api_version
assert_equal '6', @gateway.fetch_version
end

def test_refund_legacy_method_signature
Braintree::TransactionGateway.any_instance.expects(:refund).
with('transaction_id', nil).
Expand Down
22 changes: 22 additions & 0 deletions test/unit/gateways/global_collect_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,28 @@ def setup
)
end

def test_url
url = @gateway.test? ? @gateway.test_url : @gateway.live_url
merchant_id = @gateway.options[:merchant_id]
assert_equal "#{url}/v1/#{merchant_id}/payments", @gateway.send(:url, :authorize, nil)
assert_equal "#{url}/v1/#{merchant_id}/payments", @gateway.send(:url, :verify, nil)
assert_equal "#{url}/v1/#{merchant_id}/payments/0000/approve", @gateway.send(:url, :capture, '0000')
assert_equal "#{url}/v1/#{merchant_id}/payments/0000/refund", @gateway.send(:url, :refund, '0000')
assert_equal "#{url}/v1/#{merchant_id}/payments/0000/cancel", @gateway.send(:url, :void, '0000')
assert_equal "#{url}/v1/#{merchant_id}/payments/0000", @gateway.send(:url, :inquire, '0000')
end

def test_ogone_url
url = @gateway_direct.test? ? @gateway_direct.ogone_direct_test : @gateway_direct.ogone_direct_live
merchant_id = @gateway_direct.options[:merchant_id]
assert_equal "#{url}/v2/#{merchant_id}/payments", @gateway_direct.send(:url, :authorize, nil)
assert_equal "#{url}/v2/#{merchant_id}/payments", @gateway_direct.send(:url, :verify, nil)
assert_equal "#{url}/v2/#{merchant_id}/payments/0000/capture", @gateway_direct.send(:url, :capture, '0000')
assert_equal "#{url}/v2/#{merchant_id}/payments/0000/refund", @gateway_direct.send(:url, :refund, '0000')
assert_equal "#{url}/v2/#{merchant_id}/payments/0000/cancel", @gateway_direct.send(:url, :void, '0000')
assert_equal "#{url}/v2/#{merchant_id}/payments/0000", @gateway_direct.send(:url, :inquire, '0000')
end

def test_supported_card_types
assert_equal GlobalCollectGateway.supported_cardtypes, %i[visa master american_express discover naranja cabal tuya patagonia_365]
end
Expand Down
89 changes: 89 additions & 0 deletions test/unit/versionable_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
require 'test_helper'

class VersionableTest < Test::Unit::TestCase
class ParentClass
include ActiveMerchant::Versionable
end

class DummyClass < ParentClass
end

class FakeClass < ParentClass
end

class DummyChildClass < DummyClass
end

def setup
@dummy_instance = DummyClass.new
@fake_instance = FakeClass.new
@dummy_child_instance = DummyChildClass.new
end

def test_class_can_set_and_fetch_default_version
DummyClass.version('1.0')
assert_equal '1.0', DummyClass.fetch_version, 'Class should return the correct version'
end

def test_class_can_set_and_fetch_custom_feature_version
DummyClass.version('2.0', :custom_api)
DummyClass.version('v2', :some_feature)
assert_equal '2.0', DummyClass.fetch_version(:custom_api), 'Class should return the correct version'
assert_equal 'v2', DummyClass.fetch_version(:some_feature), 'Class should return the correct version'
end

def test_instance_can_fetch_default_version
DummyClass.version('v3')
assert_equal 'v3', @dummy_instance.fetch_version, 'Instance should return the correct version'
end

def test_instance_can_fetch_custom_feature_version
DummyClass.version('v4', :custom_api)
DummyClass.version('4.0', :some_feature)
assert_equal 'v4', @dummy_instance.fetch_version(:custom_api), 'Instance should return the correct version'
assert_equal '4.0', @dummy_instance.fetch_version(:some_feature), 'Instance should return the correct version'
end

def test_fetch_version_returns_nil_for_unset_feature
assert_nil DummyClass.fetch_version(:nonexistent_feature), 'Class should return nil for an unset feature'
assert_nil @dummy_instance.fetch_version(:nonexistent_feature), 'Instance should return nil for an unset feature'
end

def test_classes_dont_share_versions
# Default key
DummyClass.version('1.0')
FakeClass.version('2.0')
DummyChildClass.version('3.0')

# Custom key :some_feature
DummyChildClass.version('v5', :some_feature)
FakeClass.version('v3', :some_feature)
DummyClass.version('v4', :some_feature)

assert_equal '1.0', DummyClass.fetch_version, 'Class should return the correct version'
assert_equal 'v4', DummyClass.fetch_version(:some_feature), 'Class should return the correct version'
assert_equal '2.0', FakeClass.fetch_version, 'Class should return the correct version'
assert_equal 'v3', FakeClass.fetch_version(:some_feature), 'Class should return the correct version'
assert_equal '3.0', DummyChildClass.fetch_version, 'Class should return the correct version'
assert_equal 'v5', DummyChildClass.fetch_version(:some_feature), 'Class should return the correct version'
end

def test_instances_dont_share_versions
# Default key
DummyClass.version('1.0')
FakeClass.version('2.0')
DummyChildClass.version('3.0')

# Custom key :some_feature
DummyChildClass.version('v5', :some_feature)
FakeClass.version('v3', :some_feature)
DummyClass.version('v4', :some_feature)

assert_equal '1.0', @dummy_instance.fetch_version, 'Instance should return the correct version'
assert_equal 'v4', @dummy_instance.fetch_version(:some_feature), 'Instance should return the correct version'
assert_equal '2.0', @fake_instance.fetch_version, 'Instance should return the correct version'
assert_equal 'v3', @fake_instance.fetch_version(:some_feature), 'Instance should return the correct version'
assert_equal '3.0', @dummy_child_instance.fetch_version, 'Instance should return the correct version'
assert_equal 'v5', @dummy_child_instance.fetch_version(:some_feature), 'Instance should return the correct version'
end
end
Loading