Refactoring offsite payment methods (introducing abstract offsite class)

[guzzilar]

1. Objective

Those implemented offsite payment methods (Alipay, Installment, Internet Banking, and TrueMoney Wallet) have almost an identical function on result and callback methods. Having to implement each of them separately increasing the risk of human-error and potential malfunctions as the code could be changed/enhanced overtime.

As you can see from the screenshot below

These 2, "TrueMoney" on the left, and "Alipay" on the right, are doing exactly the same thing. However this code introduces an inconsistency as the code on the left (TrueMoney) has been developed and improved overtime while the code on the right (Alipay) screen remains the same as original, and becomes obsolete.

It must be a risk or unnecessarily if this abstract offsite class was introduced at the very beginning, as fixing one place will effect to all methods, and that may not be what we wanted.

However, after repetitively implemented 4 offsite payment methods, we can now be sure that they are doing the exact same thing and it should be safe to group them now.

Bonus point. This is to reduce the work on localization as well, as it will be easier to make sure that we don't miss any translation or having any typo here and there in each of payment classes as we can group them into one single file, Omise_Payment_Offsite class.

2. Description of change

1. Grouping all repetitive offsite methods into one abstract Omise_Payment_Offsite class.

2. Removing all redundant code from class-omise-payment-alipay.php, class-omise-payment-installment.php, class-omise-payment-internetbanking.php, class-omise-payment-truemoney.php.

3. Generalizing all offsite-payment messages, making it more generic and can be used all across the offsite payment methods.

3. Quality assurance

🔧 Environments:

• WooCommerce: v3.7.1
• WordPress: v5.3.2
• PHP version: 7.3.3

✏️ Details:

The test will be conducted and categorized into 5 main topics.

Note, the information below might be overwhelmed, basically, it is focusing on making sure that all 4 offsite payment methods are working properly in cases of "successful", "failure", and "pending".

1. General tests 1.1.) All payment methods can be enabled properly.

2. Placing order using Alipay payment method We will go through the normal WooCommerce checkout process and test mark a particular charge as "successful", "failed", and "pending". Successful case

Failure case Failure code and message can be shown properly as before.

Pending case

3. Placing order using Installment payment method We will go through the normal WooCommerce checkout process and test mark a particular charge as "successful", "failed", and "pending". Successful case

Failure case

Pending case

4. Placing order using Internet Banking payment method We will go through the normal WooCommerce checkout process and test mark a particular charge as "successful", "failed", and "pending". Successful case

Failure case

Pending case

5. Placing order using TrueMoney Wallet payment method We will go through the normal WooCommerce checkout process and test mark a particular charge as "successful", "failed", and "pending". Successful case

Failure case

Pending case

4. Impact of the change

Should be none.

5. Priority of change

Normal

6. Additional Notes

The translation will be corrected and done in another different time, when we start working on Konbini payment. This pull request is focusing only for refactoring the offsite payment classes.

[jonrandy]

"The translation will be corrected and done in another different time, when we start working on Konbini payment."

Am I missing something? There don't appear to be any translations in the PR. Some of the English needs fixing though - is that something I should do in this review?

[guzzilar]

Am I missing something? There don't appear to be any translations in the PR. Some of the English needs fixing though - is that something I should do in this review?

@jonrandy This pull request is tweaking some messages here and there. i.e. before

We cannot validate your payment result: Note that your payment might already has been processed. Please contact our support team if you have any questions.

after

We cannot validate your payment result: Note that your payment might already has been processed. Please contact our support team if you have any questions.

and these affects to the localisation as its translation is paired and matched with the previous 'before' version.

However, for English fixing, yeah, help me (in this PR) 🆘 Thanks :D

[guzzilar]

For the recent commit (https://github.com/omise/omise-woocommerce/pull/143/commits/bfed4a425ea1a12683c9d820c6f9501e6f9ce16e) Thanks to @jacstn, we have found that; PHP prior 7.2 can’t override-declare an abstract method inside an extended abstract class from an original (parent) abstract class

This won’t work on PHP prior 7.2

abstract class class1 {
  abstract public function someFunc();
}
abstract class class2 extends class1 {
  abstract public function someFunc();
}

ref: https://www.php.net/manual/en/language.oop5.abstract.php#78388 https://github.com/php/php-src/blob/php-7.2.0alpha1/UPGRADING (section, “2. New Features > Core”)

---

By removing the abstract charge() method out from Omise_Payment_Offsite, it should work as designed now. I've tested on PHP v5.6.40, worked as well.

[guzzilar]

@jonrandy Hi, I just pushed a new commit. Can you help review https://github.com/omise/omise-woocommerce/pull/143/commits/c5546e39711c3fe50a84540b810129d6980151b1 ? Thanks : )