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

MAG1: Refactoring payment processor classes #92

Merged
merged 8 commits into from Oct 26, 2017

Conversation

guzzilar
Copy link
Contributor

@guzzilar guzzilar commented Oct 24, 2017

1. Objective

As for coming of the new payment method, Alipay payment, and Webhook feature.
Now we can see some of common logics that will be used again and again in another new payment method. Means, if we just continue implement those features right away with the previous approach, it will just make code more huge and hard to maintain.

So, this PR aims to clean up unnecessary/unused code, refactoring and preparing structure for those new coming features.

2. Description of change

There are 3 main parts that you should consider.

  1. Get rid of duplicated code, locate methods to proper class / location.

  2. Refactoring payment processor classes.

    1. Remove all 'strategy' patterns / classes.
      The main purpose of having 'strategy' class was to avoid authorize, capture methods to take care too much of tasks. For example 'check if this payment requires 3-D Secure payment', 'build request params', 'create charge', 'validate result', 'handle if failed case', 'handle if successful case'.

      However, I've figured out that it would be more simple if just have another class, 'Model/Api/Charge' to taking care some of those validation part (and it makes totally sense that 'Charge object itself taking care on validation itself) and delegate 'common' logic that use to create charge into 'parent' method (please check Model/Payment::process() method for more detail).

    2. Introduced 'Model/Api/Charge'.
      According to [2.1], now we can make those payment processors more simple, also more readable by moving those validations back into Model_Api_Charge object (as mentioned in [1], it makes totally sense that Charge object itself taking care of its validation).

      It also allows us to write some methods inside to make code can be reusable.
      Considering:

      // In your payment processor:
      public function authorize()
      {
          $charge = Mage::getModel('omise_gateway/api_charge')->create([...]);
          
          if ($charge->isAwaitForCapture()) {
              // do update order status to 'pending'.
          }
      }

      Here, those Credit Card, InternetBanking, Alipay can reuse this class without worrying about maintain repeatedly writing 'validation' code again and again.

    3. Move some charge creation logics into parent class (Model/Payment.php).

  3. Refactoring offsite validation controllers (Credit Card 3-D Secure, Internet Banking), move some duplicated code into the parent class. This is a preparation to reduce duplicated code when Alipay and Webhook feature come.

3. Quality assurance

🔧 Environments:

  • PHP: v5.6.30
  • Magento: v1.9.2.4, v1.9.3.6

✏️ Details:

  1. Make sure that you can still make charge with Credit Card payment method.

    1. Authorize only.
    2. Manual capture from admin page.
    3. Authorize and capture.
    4. (3-D Secure) Authorize only.
    5. (3-D Secure) Manual capture from admin page.
    6. (3-D Secure) Authorize and capture.
  2. Make sure that you can still make charge with Internet Banking payment method on both failed and successful cases.

  3. Make sure that those payment methods can still handle error and failed case properly.

4. Impact of the change

No

5. Priority of change

Normal

6. Additional Notes

No

Note that this commit is quite big because there are 3 parts in 1 commit.
1. Remove all 'strategy' patterns / classes.
  The main purpose of having 'strategy' class was to avoid 'authorize', 'capture' methods to take care lots of tasks, like, 'check if this payment requires 3-D Secure payment', 'build request params', 'create charge', 'validate result', 'handle if failed case', 'handle if successful case'.
  However, I've figured out that it would be more simple if just have another class, 'Model/Api/Charge' to taking care some of those validation part (and it makes totally sense that 'Charge object itself taking care on validation itself) and delegate 'common' logic that use to create charge into 'parent' method (please check Model/Payment::process() method for more detail).

2. Refactor payment processors (Model/Payment/Creditcard.php, Model/Payment/Offsiteinternetbanking.php).
  According to [1], once introduced 'Model/Api/Charge' and removed 'Model/Strategies/'. Now we can make those payment processors more simple, also more readable.

3. Clean up some unused properties / methods in those payment processor and parent classes.
Since now we are using 'Model/Api/Charge' and 'Model/Api/Error' classes in the payment processor classes.
This commit is to apply those approach to 'manual capture' action.
@guzzilar
Copy link
Contributor Author

tested on Magento v1.9.3.6

@guzzilar
Copy link
Contributor Author

👍

@guzzilar guzzilar merged commit e6ba6ef into 1-stable Oct 26, 2017
@guzzilar guzzilar deleted the 1-refactor-payment-processor branch October 26, 2017 07:18
@guzzilar guzzilar changed the title [WIP] MAG1: Refactoring payment processor classes MAG1: Refactoring payment processor classes Oct 26, 2017
@guzzilar guzzilar mentioned this pull request Nov 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant