Code refactoring for a better i18n for strings that were in JS files. by guzzilar · Pull Request #147 · omise/omise-woocommerce

guzzilar · 2019-11-11T03:30:41Z

1. Objective

There are some parts of the plugin that cannot be translated as originally it wasn't implemented for the idea of translating to another language.

This pull request is to i18n the plugin. Preparing for the up-coming feature that requires the plugin to be translated to another language : )

Related information:
Related issue(s): T2778, T17731 (internal ticket)

2. Description of change

3. Quality assurance

🔧 Environments:

WooCommerce: v3.8.0
WordPress: v5.3.2
PHP version: 7.3.3

✏️ Details:

Checkout page
Expectation
omise-payment-form-handler.js should be able to display a warning message using strings that is passed via server-side properly in each of cases.

3.1. In a case where card fields are left in blank when place a new order.**

3.2. In a case of attempting to create a new token but card informations are invalid. (I.e. using an expired card).
Expectation

3.3. This is an edge case where the JS script cannot find a designated DOM to retrieve Omise card id at the checkout page (i.e. Omise credit card form is modified by a 3rd-party plugin or someone modified the form directly in Omise-WooCommerce code)

// assets/javascripts/omise-payment-form-handler.js
function validSelection(){
    $card_list = $("input[name='card_id']");
    $selected_card_id = $("input[name='card_id']:checked");

    ...
}

3.4. Failed to retrieve a response back from Omise API.

My Account page
Expectation
omise-myaccount-card-handler.js should be able to display a warning message using strings that is passed via server-side properly.

4. Impact of the change

Some messages have been changed, corrected, and improved.
This would affect the current translation of the plugin.
https://translate.wordpress.org/projects/wp-plugins/omise

However, the plugin hasn't been properly translated for a while, so we may consider this as a, 'revamp' the entire translation work.

5. Priority of change

Normal

6. Additional Notes

This is to prepare the code to be able to translate.
The translation process will be done in GlotPress. (https://translate.wordpress.org/projects/wp-plugins/omise)

Also note that during the review, you may find some part of code that could be improved or refactored. However, I would like to leave it as it is for now, as this pull request is aiming for i18n strains at the JS files.

Personally, I'm planning to clean up the code and polish It after Konbini payment is done.
But, any thoughts are welcomed : )

To be able to translate those invalid-input error messages (field is required) properly, we must not put an error message directly in a JS file.

guzzilar · 2019-11-12T22:21:03Z

+				'retry_or_contact_support'       => wp_kses(
+					__( 'This incident could happen either from using an invalid card or the payment provider server is under maintainence.<br/>
+					    You may retry again in a couple seconds, or contact our support team if you have any question.', 'omise' ),
+					array( 'br' => array() )


tl;dr
Some of these strings are being used in 2 places. This could raise a potential case of unmatching translation.
However, I'm planning to have a dedicated PR to properly handle this topic as this might be a bit out of scope of this PR (and don't want to make this PR too big).

Note, some of these strings are being used in 2 places, at Omise_Payment_Creditcard and Omise_MyAccount.
This is because we're removing omise-util.js and move these invalidation messages back to server side so we can localize it.

This could raise a potential case where one place gets updated over another causing an unmatching translation.

I'm thinking of housing all the translation strings in one class (i.e. Omise_Localization) so we will have only one place to look for when these needed to be updated.

However, this might be too much for this PR.
I'm planning having a dedicated PR for this topic right after this PR (or, before releasing Konbini payment, at late – this PR may be released along with a PR that was solving 3-D Secure setting feature before I finish the Omise_Localization one).

jonrandy · 2019-11-15T03:24:20Z

-
-				var errors = OmiseUtil.validate_card(card);
-				if(errors.length > 0){
+				let errors            = [],


This code is identical to that in the other file. Feels like maybe we should be reusing?

Yup, I think so that we may somehow can merge these 2 JS files together. (however, please ignore the duplication here for a while as it is a bit out of scope of this PR)

jonrandy · 2019-11-15T03:26:24Z

+				'key'                            => Omise()->settings()->public_key(),
+				'ajax_url'                       => admin_url( 'admin-ajax.php' ),
+				'ajax_loader_url'                => plugins_url( '/assets/images/ajax-loader@2x.gif', dirname( __FILE__ ) ),
+				'required_card_name'             => __( 'Card Holder\'s name is a required field', 'omise' ),


Cardholder's

jonrandy · 2019-11-15T03:27:21Z

+				'ajax_loader_url'                => plugins_url( '/assets/images/ajax-loader@2x.gif', dirname( __FILE__ ) ),
+				'required_card_name'             => __( 'Card Holder\'s name is a required field', 'omise' ),
+				'required_card_number'           => __( 'Card Number is a required field', 'omise' ),
+				'required_card_expiration_month' => __( 'Card Expiry month is a required field', 'omise' ),


'Card' probably unnecessary here

and the two fields next

This error message is mixed with the other invalid form input's error messages.
If possible and not grammatically wrong, as for UX of WC, I think we better explicitly say that this error is coming from the credit card form, not others.

Ah, the error message isn't displayed next to the field?

jonrandy · 2019-11-15T03:28:37Z

+				'required_card_expiration_year'  => __( 'Card Expiry year is a required field', 'omise' ),
+				'required_card_security_code'    => __( 'Card Security code is a required field', 'omise' ),
+				'cannot_create_card'             => __( 'Unable to add a new card.', 'omise' ),
+				'cannot_connect_api'             => __( 'Currently, the payment provider server is under maintain.', 'omise' ),


'... undergoing maintenance.'

jonrandy · 2019-11-15T03:32:31Z

+				'check_internet_connection'      => __( 'Please make sure that your internet connection is stable.', 'omise' ),
+				'retry_or_contact_support'       => wp_kses(
+					__( 'This incident could happen either from using an invalid card or the payment provider server is under maintainence.<br/>
+					    You may retry again in a couple seconds, or contact our support team if you have any question.', 'omise' ),


This incident could occur either from the use of an invalid card, or the payment provider server is undergoing maintenance.
You may retry again in a couple of seconds, or contact our support team if you have any questions.

jonrandy · 2019-11-15T03:33:25Z

+
+				$omise_params = array(
+					'key'                            => $this->public_key(),
+					'required_card_name'             => __( 'Card Holder\'s name is a required field', 'omise' ),


jonrandy · 2019-11-15T03:34:15Z

+					'key'                            => $this->public_key(),
+					'required_card_name'             => __( 'Card Holder\'s name is a required field', 'omise' ),
+					'required_card_number'           => __( 'Card Number is a required field', 'omise' ),
+					'required_card_expiration_month' => __( 'Card Expiry month is a required field', 'omise' ),


Same as before - the word 'Card' is superfluous in this and the following 2 field messages

Also, inconsistent capitalisation of field name words. Some have all words capitalised, some don't. Need to pick a style and stay with it

jonrandy · 2019-11-15T03:36:01Z

+					'required_card_expiration_year'  => __( 'Card Expiry year is a required field', 'omise' ),
+					'required_card_security_code'    => __( 'Card Security code is a required field', 'omise' ),
+					'invalid_card'                   => __( 'Invalid card.', 'omise' ),
+					'no_card_selected'               => __( 'Please select a card or entering a new one.', 'omise' ),


Please select a card or enter a new one.

jonrandy · 2019-11-15T03:37:15Z

+					'required_card_security_code'    => __( 'Card Security code is a required field', 'omise' ),
+					'invalid_card'                   => __( 'Invalid card.', 'omise' ),
+					'no_card_selected'               => __( 'Please select a card or entering a new one.', 'omise' ),
+					'cannot_create_token'            => __( 'Unable to continue to proceed the payment.', 'omise' ),


Not even sure what you're trying to say here... Maybe 'Unable to proceed to the payment.'?

This one is for a case where the plugin cannot retrieve a response body back from Omise.js.
I'll change to the one you suggested.

jonrandy · 2019-11-15T03:37:49Z

+					'invalid_card'                   => __( 'Invalid card.', 'omise' ),
+					'no_card_selected'               => __( 'Please select a card or entering a new one.', 'omise' ),
+					'cannot_create_token'            => __( 'Unable to continue to proceed the payment.', 'omise' ),
+					'cannot_connect_api'             => __( 'Currently, the payment provider server is under maintain.', 'omise' ),


Currently, the payment provider server is undergoing maintenance.

jonrandy · 2019-11-15T03:38:12Z

+					'no_card_selected'               => __( 'Please select a card or entering a new one.', 'omise' ),
+					'cannot_create_token'            => __( 'Unable to continue to proceed the payment.', 'omise' ),
+					'cannot_connect_api'             => __( 'Currently, the payment provider server is under maintain.', 'omise' ),
+					'retry_checkout'                 => __( 'Please place your order again in a couple seconds.', 'omise' ),


...couple of...

jonrandy

See notes

jacstn · 2019-12-09T08:05:18Z

+						} );
+
 				    	data = {
 								action: "omise_create_card", 


is that code formatting correct? regarding indents.

the new one is correct, the old one is wrong.
But good catch 👍 I'm afraid to re-adjust the whole file cuz it's going to be super hard for you guys to review, also I was thinking that this file will be modified more when I work on the API translation stuff. So it's kinda mixed style now.

guzzilar · 2019-12-09T08:13:02Z

Thanks for the review, guys 👍

Decoupling omise-payment-form-handler.js from omise-util.js

138b327

To be able to translate those invalid-input error messages (field is required) properly, we must not put an error message directly in a JS file.

guzzilar added the Status: In progress label Nov 11, 2019

Credit Card form: i18n for those invalid-input error messages.

6a0044f

guzzilar force-pushed the better-i18n branch from 6f3b120 to 6a0044f Compare November 11, 2019 21:52

guzzilar added 4 commits November 13, 2019 03:50

omise-payment-form-handler.js : Correcting code indent.

48ea9c5

omise-payment-form-handler.js : i18n all the possible strings.

dbeba25

omise-myaccount-card-handler.js : i18n all the possible strings.

47626b0

Removing omise-util.js as we no longer used in any place.

7cbad19

guzzilar force-pushed the better-i18n branch from 7a32564 to 7cbad19 Compare November 12, 2019 21:55

guzzilar commented Nov 12, 2019

View reviewed changes

guzzilar changed the title ~~[WIP] Code refactoring for a better i18n~~ [WIP] Code refactoring for a better i18n for strings that were in JS files. Nov 12, 2019

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

guzzilar changed the title ~~[WIP] Code refactoring for a better i18n for strings that were in JS files.~~ Code refactoring for a better i18n for strings that were in JS files. Nov 13, 2019

jonrandy reviewed Nov 15, 2019

View reviewed changes

jonrandy suggested changes Nov 15, 2019

View reviewed changes

Addressing all the review feedback from PR 174

bc279c3

guzzilar force-pushed the better-i18n branch from 0380f7b to bc279c3 Compare November 27, 2019 02:30

jonrandy approved these changes Nov 27, 2019

View reviewed changes

jacstn approved these changes Dec 9, 2019

View reviewed changes

guzzilar added Status: Ready and removed Status: Review needed labels Dec 9, 2019

guzzilar merged commit 77e3a8b into master Dec 9, 2019

guzzilar deleted the better-i18n branch December 9, 2019 08:13

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

Conversation

guzzilar commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1. Objective

2. Description of change

3. Quality assurance

4. Impact of the change

5. Priority of change

6. Additional Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonrandy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guzzilar commented Dec 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

guzzilar commented Nov 11, 2019 •

edited

Loading