Rename Gateway into PaymentMethod::CreditCard #2001
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
houndci-bot
reviewed
Jun 7, 2017
|
|
||
| describe 'ActiveMerchant methods' do | ||
| class PaymentGateway | ||
| def initialize(options) |
There was a problem hiding this comment.
Put empty method definitions on a single line.
| Spree::Deprecation.warn \ | ||
| "provider_class is deprecated and will be removed from Solidus 3.0 " \ | ||
| "(use gateway_class instead)" | ||
| self.public_send :provider_class |
| # Represents the gateway class of this payment method | ||
| # | ||
| def gateway_class | ||
| if self.respond_to? :provider_class |
mamhoff
approved these changes
Jun 7, 2017
| # Uses STI (single table inheritance) to store all implemented payment methods | ||
| # in one table (+spree_payment_methods+). | ||
| # | ||
| # This class is not ment to be instantiated. Please create instances of concrete payment methods. |
There was a problem hiding this comment.
This is excellent class documentation. Wow.
There's a typo in here though: "ment" should read "meant".
| # The form your customer enters the payment information in during checkout | ||
| # | ||
| # 2. app/views/spree/checkout/existing_payment/_{method_type}.html.erb | ||
| # The payment information of your customers resuable sources during checkout |
There was a problem hiding this comment.
Typo: "resuable" -> "reusable"
To remove confusion around payment methods, gateways and providers we rename the `Spree::Gateway::Bogus` and `Spree::Gateway::BogusSimple` payment methods into `Spree::PaymentMethod::BogusCreditCard` and `Spree::PaymentMethod::SimpleBogusCreditCard`.
Use `Spree:: PaymentMethod::BogusCreditCard` and `Spree::PaymentMethod::SimpleBogusCreditCard ` instead.
Running
rake solidus:migrations:rename_gateways:up
helps migrating your data to new bogus payment method class names.
rake solidus:migrations:rename_gateways:down
reverts this.
Also includes a migration that invokes that task, so you don't need to care when deploying this change.
`Spree::Gateway` holds many methods that actually belongs into its parent class - `Spree::PaymentMethod`. It rather should only hold commonalities that are relevant for credit card payment methods. All payment methods that are not a credit card should inherit directly from `Spree::PaymentMethod` - like `Spree::PaymentMethod::StoreCredit` and friends already does.
In order to remove confusing parts of our current payment methods implementation we rename the `provider` methods from `Spree::PaymentMethod` into `gateway`. A gateway is a class that implements the API communication to the provider. See the `active_merchant` gem for a set of gateway classes we use in `solidus_gateway`. A provider on the other hand is a service vendor that offers a payment gateway and multiple payment methods (like Braintree). Therefore we plan to introduce a provider class later on. This then holds commonalities for mulitple payment methods, like credentials. Also it will be an ActiveRecord model so that payment methods can be grouped under one provider, which is very useful for BI and reporting.
This corrects the documentation on the Spree::PaymentMethod class.
Moves all `Spree::PaymentMethod` class methods to top of class instead of having them distributed all over the file.
The `Spree::Gateway` is an implementation of a credit card payment method. The current name is confusing. With moving this class into the `PaymentMethod` namespace and changing its name to `CreditCard` it is now more obvious what this class actually is. Several hints inside the class prove that: - It inherits from `Spree::PaymentMethod` - The `source_class` is `Spree::CreditCard` - In `supports?` it asks the gateway if it supports a certain credit card brand Further actions are required. First on foremost `solidus_gateway` needs to be changed in a way that all credit card based payment methods inherit from `Spree::PaymentMethod::CreditCard` from now on and all non credit card payment methods inherit from `Spree::PaymentMethod` directly.
Use `Spree::PaymentMethod::CreditCard` instead.
|
Indeed! I very much admire your guts to make such a major change in the name of making it all easier for us to work with the platform! I guess a big role in having the courage to do the refactoring play the tests, so thanks all as well for writing tests so good! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
Spree::Gatewayis an implementation of a credit card payment method. The current name is confusing.With moving this class into the
PaymentMethodnamespace and changing its name toCreditCardit is now more obvious what this class actually is.Several hints inside the class prove that:
Spree::PaymentMethodsource_classisSpree::CreditCardsupports?it asks the gateway if it supports a certain credit card brandFurther actions are required. First on foremost
solidus_gatewayneeds to be changed in a way that all credit card based payment methods inherit fromSpree::PaymentMethod::CreditCardfrom now on and all non credit card payment methods inherit fromSpree::PaymentMethoddirectly.includes #2000