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

Bill Payment, simplifying and updating relevant informations at the thank you page #129

Merged
merged 4 commits into from Aug 19, 2019

Conversation

@guzzilar
Copy link
Collaborator

guzzilar commented Aug 14, 2019

1. Objective

There are some missing informations on our Bill Payment barcode detail (on both printing, and on a screen).

This pull request is aiming to add all those details (as well as adjusting its UI)
– suggested by @jacstn, thanks :D

2. Description of change

  • Simplifying stylesheet and reconstruct HTML element to reduce duplicated code and unnecessary css-classes
  • Removing buyer's Email column from the printing detail.
  • Adding more information about expiring date and Tesco Lotus transaction fee.

3. Quality assurance

πŸ”§ Environments:

  • WooCommerce: v3.6.5
  • WordPress: v5.2.2
  • PHP: v7.2.21

✏️ Details:

Make a new purchase using Bill Payment payment method as normal.
At the thank you page, you should be able to see this new information at the page.
Screen Shot 2562-08-14 at 08 38 17

We may want to make sure that an expiration date can be formatted properly based on a store's time-zone by updating TimeZone option at WordPress General Setting page.
wordpress-setting-tz

Then, refresh the thank you page to make sure that an expiration date is formatted accordingly.
billpayment-tz-adjusted

Click "Print barcode" button, check its output.
Screen Shot 2562-08-14 at 08 40 07

Also, check its output for the email format.
Screen Shot 2562-08-14 at 08 48 15

4. Impact of the change

Nothing

5. Priority of change

Normal

6. Additional Notes

Just want to note that I think this code is relying too much on WooCommerce Helper class and method. Would be nice to somehow, organize it within Omise class, and provide an alternative in case if those WC helper methods get removed.

However, it would be way too big for this scope of its objective and concern.
Probably better to split it to another PR if we are really going to refactor it.

Also, this button and message at the thank you page may mislead users that they have only one option to print the barcode out without realizing that the barcode is also sent to their email. (again, I think it is out of scope for this objective)

@@ -137,84 +137,92 @@ public function display_barcode( $order, $context = 'view' ) {
$charge['source']['references']['reference_number_2'],
$charge['amount']
);
$expires_datetime = new WC_DateTime( $charge['source']['references']['expires_at'], new DateTimeZone( 'UTC' ) );
$expires_datetime->set_utc_offset( wc_timezone_offset() );
?>

<div class="omise omise-billpayment-tesco-details" <?php echo 'email' === $context ? 'style="margin-bottom: 4em; text-align:center;"' : ''; ?>>

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 14, 2019

Author Collaborator

βœ‹ Note: GitHub is showing bunch of changing, mostly because I re-ordered code structure, bringing the printing detail from bottom up to this line and so on.

<?php
echo sprintf(
wp_kses(
__( 'Please bring this barcode to pay at Tesco Lotus within:<br/><strong>%1$s</strong> at <strong>%2$s</strong>.', 'omise' ),

This comment has been minimized.

Copy link
@jonrandy

jonrandy Aug 14, 2019

'Please bring this barcode to pay at Tesco Lotus by:<br/><strong>%1$s %2$s</strong>.'

This comment has been minimized.

Copy link
@jonrandy

jonrandy Aug 14, 2019

Maybe even consider just one formatted datetime string using the stores i18n settings

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 14, 2019

Author Collaborator

@jonrandy how about by > before? would it more clear?
Please bring this barcode to pay at Tesco Lotus before: Aug 15 8.37 am.

This comment has been minimized.

Copy link
@jonrandy

jonrandy Aug 14, 2019

'by' is the normal way to say this kind of thing in English

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 15, 2019

Author Collaborator

πŸ‘ ic

<?php echo $barcode_html; ?>
<?php if ( 'email' !== $context ) : ?>
<div class="omise-billpayment-tesco-print-button">
<button onClick="window.print()" class="button button-primary">Print barcode</button>

This comment has been minimized.

Copy link
@jonrandy

jonrandy Aug 14, 2019

'Print barcode' is hardcoded in English

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 15, 2019

Author Collaborator

πŸ‘ nice catch

'
Tesco Lotus may charge a small fee for the transaction.<br/>
Failing to make payment within the mentioned date and time, your order will be automatically canceled.
', 'omise'

This comment has been minimized.

Copy link
@jonrandy

jonrandy Aug 14, 2019

'If you fail to make payment by the stated time, your order will be automatically canceled.'

Or cancelled if we're going for British English πŸ˜›

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 15, 2019

Author Collaborator

πŸ˜‚ Btw, @jonrandy wonder why If you fail to make payment is better than Failing to make payment?

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 15, 2019

Author Collaborator

Or if we want to make it more formal, how about
Please note that failing to make payment by the stated time, your order will be automatically canceled.?
Or it is not about formality?

This comment has been minimized.

Copy link
@jonrandy

jonrandy Aug 15, 2019

If you want to use 'failing to make payment', you need some further rewording...
Please note that failing to make payment by the stated time will result in your order being automatically canceled.
Which is getting a little bit long winded

Copy link

jonrandy left a comment

Mainly English and translation fixes

@jacstn

This comment has been minimized.

Copy link
Contributor

jacstn commented Aug 14, 2019

@guzzilar are we sure we have 24 hours to make a payment, not 48 hours? or 36

@guzzilar

This comment has been minimized.

Copy link
Collaborator Author

guzzilar commented Aug 14, 2019

@jacstn according to OmiseCharge API > source.references.expires_at, guess I have no choice but to believe so πŸ˜‚ .

@guzzilar guzzilar force-pushed the billpayment-improve-printing-info branch from c784edc to 70857e9 Aug 15, 2019
@guzzilar

This comment has been minimized.

Copy link
Collaborator Author

guzzilar commented Aug 15, 2019

@jonrandy done. Can you help review the latest commit?

Screen Shot 2562-08-15 at 11 11 05

Screen Shot 2562-08-15 at 11 11 15

Screen Shot 2562-08-15 at 11 09 39

echo sprintf(
wp_kses(
__( 'Please bring this barcode to pay at Tesco Lotus by:<br/><strong>%1$s %2$s</strong>.', 'omise' ),
array( 'br' => array(), 'strong' => array() )

This comment has been minimized.

Copy link
@jonrandy

jonrandy Aug 15, 2019

Do WooCommerce stores have a set DATETIME format in their settings?

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 15, 2019

Author Collaborator

@jonrandy yup (in fact, WordPress does)
πŸ‘‡ from WordPress General Setting page.
Screen Shot 2562-08-15 at 12 32 01

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 15, 2019

Author Collaborator

Just to give you a bit more context.
The following lines

wc_format_datetime( $expires_datetime, wc_date_format() );
wc_format_datetime( $expires_datetime, wc_time_format() );

The $expires_datetime variable comes from

$expires_datetime   = new WC_DateTime( $charge['source']['references']['expires_at'], new DateTimeZone( 'UTC' ) );

And, WC_DateTime extends from a built-in PHP DateTime class.

So basically we pass Omise's expires_at (as ISO 8601 Zulu time format) to WC_DateTime class (that extended from DateTime) and set its default TZ to UTC.

Then, we use wc_date_format() and wc_time_format() to retrieve WordPress Date and Time formatting from its configuration and pass those to wc_format_datetime() function, which does a DateTime formatting job.

This comment has been minimized.

Copy link
@jonrandy

jonrandy Aug 15, 2019

So, given the above image, it would appear that Wordpress has a date format, and a time format, but no specific datetime format. Making the answer to my original question 'no'. In which case, our code is correct :)

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 15, 2019

Author Collaborator

Forgot to mention, WordPress also provides TimeZone configuration, which we retrieve and config to WC_DateTime at the earlier part of code.

$expires_datetime = new WC_DateTime( $charge['source']['references']['expires_at'], new DateTimeZone( 'UTC' ) );
$expires_datetime->set_utc_offset( wc_timezone_offset() );

Screen Shot 2562-08-15 at 12 41 50

This comment has been minimized.

Copy link
@guzzilar

guzzilar Aug 15, 2019

Author Collaborator

@jonrandy ok I'm not followed now πŸ˜‚
--edited
Got it. You were looking at <strong>%1$s %2$s</strong> right?

This comment has been minimized.

Copy link
@jonrandy

jonrandy Aug 15, 2019

Yes. If there was a DATETIME format available, I would expect a single string here

@jacstn
jacstn approved these changes Aug 19, 2019
@guzzilar guzzilar merged commit a15a117 into master Aug 19, 2019
@guzzilar guzzilar deleted the billpayment-improve-printing-info branch Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.