Refactoring offsite payment methods (introducing abstract offsite class) #143

guzzilar · 2019-11-04T22:55:56Z

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

Grouping all repetitive offsite methods into one abstract Omise_Payment_Offsite class.
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.
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.

…ss).

jonrandy · 2019-11-05T05:58:51Z

"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 · 2019-11-05T08:08:06Z

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

…extended from its abstract parent class. Removing abstract 'charge' method out from 'Omise_Payment_Offsite'.

guzzilar · 2019-11-05T20:41:40Z

For the recent commit (bfed4a4)
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.

jonrandy · 2019-11-06T04:05:16Z

includes/gateway/abstract-omise-payment-offsite.php

+		if ( ! $order ) {
+			$message = __(
+				'<strong>We cannot validate your payment result:</strong><br/>
+				 Note that your payment might already has been processed.<br/>


"Note that your payment may have already been processed."

jonrandy · 2019-11-06T04:07:38Z

includes/gateway/abstract-omise-payment-offsite.php

+
+			if ( self::STATUS_PENDING === $charge['status'] && ! $charge['paid'] ) {
+				$message = __(
+					'Omise: The payment has been processing.<br/>


"Omise: The payment is being processed."

@jonrandy just wonder what is the nuance that these 2 sentences are giving. Not similar, if not the same?

"The payment has been processing" implies that it was being processed, but now has stopped for some reason - which I don't think is what you wanted to say

ic. Thanks 👍

jonrandy · 2019-11-06T04:08:17Z

includes/gateway/abstract-omise-payment-offsite.php

+			if ( self::STATUS_PENDING === $charge['status'] && ! $charge['paid'] ) {
+				$message = __(
+					'Omise: The payment has been processing.<br/>
+					 Due to the payment provider, this may take some time to process.<br/>


"Depending on the payment provider, this may take some time to process."

jonrandy · 2019-11-06T04:09:31Z

includes/gateway/abstract-omise-payment-offsite.php

+				$message = __(
+					'Omise: The payment has been processing.<br/>
+					 Due to the payment provider, this may take some time to process.<br/>
+					 Please do a manual \'Sync Payment Status\' action from the <strong>Order Actions</strong> panel or check the payment status directly at Omise dashboard again later.',


"Please do a manual 'Sync Payment Status' action from the Order Actions panel, or check the payment status directly at the Omise dashboard later."

jonrandy · 2019-11-06T04:24:26Z

includes/gateway/abstract-omise-payment-offsite.php

+			}
+
+			$message = __(
+				'Note that your payment might already has been processed.<br/>


"Note that your payment may have already been processed."

jonrandy

English fixes needed (see notes)

…earer

guzzilar · 2019-11-06T20:06:40Z

@jonrandy Hi, I just pushed a new commit. Can you help review c5546e3 ?
Thanks : )

guzzilar added Status: In progress Type: Enhancement labels Nov 4, 2019

guzzilar force-pushed the refactoring-offsite-methods branch 3 times, most recently from 6a3d161 to 3c3a55c Compare November 5, 2019 01:59

Refactoring offsite payment methods (introducing abstract offsite cla…

dc877d7

…ss).

guzzilar force-pushed the refactoring-offsite-methods branch from 3c3a55c to dc877d7 Compare November 5, 2019 02:46

guzzilar added Status: Review needed and removed Status: In progress labels Nov 5, 2019

guzzilar requested review from jacstn and jonrandy November 5, 2019 04:05

guzzilar changed the title ~~[WIP] Refactoring offsite payment methods (introducing abstract offsite class)~~ Refactoring offsite payment methods (introducing abstract offsite class) Nov 5, 2019

PHP prior 7.2 abstract class cannot override abstract method that is …

bfed4a4

…extended from its abstract parent class. Removing abstract 'charge' method out from 'Omise_Payment_Offsite'.

jonrandy reviewed Nov 6, 2019

View reviewed changes

jonrandy suggested changes Nov 6, 2019

View reviewed changes

abstract-omise-payment-offsite.php: Tweaking English. Make it more cl…

c5546e3

…earer

jonrandy approved these changes Nov 11, 2019

View reviewed changes

jacstn approved these changes Nov 11, 2019

View reviewed changes

guzzilar added Status: Ready and removed Status: Review needed labels Nov 11, 2019

guzzilar merged commit faf544a into master Nov 11, 2019

guzzilar deleted the refactoring-offsite-methods branch November 11, 2019 07:02

guzzilar mentioned this pull request Jan 15, 2020

✨ Releasing Omise-WooCommerce v3.10 #150

Merged

guzzilar added Status: Released and removed Status: Ready labels Jan 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring offsite payment methods (introducing abstract offsite class) #143

Refactoring offsite payment methods (introducing abstract offsite class) #143

guzzilar commented Nov 4, 2019 •

edited

Loading

jonrandy commented Nov 5, 2019

guzzilar commented Nov 5, 2019

guzzilar commented Nov 5, 2019

jonrandy Nov 6, 2019

jonrandy Nov 6, 2019

guzzilar Nov 6, 2019

jonrandy Nov 11, 2019

guzzilar Nov 11, 2019

jonrandy Nov 6, 2019

jonrandy Nov 6, 2019

jonrandy Nov 6, 2019

jonrandy left a comment

guzzilar commented Nov 6, 2019

Refactoring offsite payment methods (introducing abstract offsite class) #143

Refactoring offsite payment methods (introducing abstract offsite class) #143

Conversation

guzzilar commented Nov 4, 2019 • edited Loading

1. Objective

2. Description of change

3. Quality assurance

4. Impact of the change

5. Priority of change

6. Additional Notes

jonrandy commented Nov 5, 2019

guzzilar commented Nov 5, 2019

guzzilar commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonrandy left a comment

Choose a reason for hiding this comment

guzzilar commented Nov 6, 2019

guzzilar commented Nov 4, 2019 •

edited

Loading