-
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
Normalize API versions #5371
base: master
Are you sure you want to change the base?
Normalize API versions #5371
Conversation
3838719
to
eafa72f
Compare
def inherited(subclass) | ||
super | ||
subclass.versions = {} | ||
end |
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.
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.
test/unit/versionable_test.rb
Outdated
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, 'Class should return the correct version' | ||
assert_equal 'v4', @dummy_instance.fetch_version(:some_feature), 'Class should return the correct version' | ||
assert_equal '2.0', @fake_instance.fetch_version, 'Class should return the correct version' | ||
assert_equal 'v3', @fake_instance.fetch_version(:some_feature), 'Class should return the correct version' | ||
assert_equal '3.0', @dummy_child_instance.fetch_version, 'Class should return the correct version' | ||
assert_equal 'v5', @dummy_child_instance.fetch_version(:some_feature), 'Class should return the correct version' | ||
end | ||
end |
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.
New tests to verify classes and instances don't share the same versions
hash.
subclass.versions = {} | ||
end | ||
|
||
def version(version, feature = :default_api) |
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.
How is this method used? Is it just for the versionable_test.rb
?
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 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) |
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.
Also this one, how is it used? What is the difference between this one and the one in line 25?
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.
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.
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.
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 😉
Summary: ------------------------------ This change centralizes version management at the class level, creating a consistent and reusable way to define and access API versions. Unit Tests: ------------------------------ 6155 tests, 80982 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed RuboCop: ------------------------------ 808 files inspected, no offenses detected
eafa72f
to
09e0e11
Compare
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 to me! I was able to verify it works by using Timecop.freeze and that inherited method you added solves the issue
Summary:
This change centralizes version management at the class level, creating a consistent and reusable way to define and access API versions.
Unit Tests:
6154 tests, 80974 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
RuboCop:
808 files inspected, no offenses detected