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

Fix: Display correctly barcode from tesco lotus on checkout success page. #180

Merged
merged 2 commits into from
Jan 17, 2019

Conversation

jacstn
Copy link
Contributor

@jacstn jacstn commented Jan 17, 2019

1. Objective

Display barcode fix.
There is error that barcode is not displayed on tesco lotus page.
This PR deliver fix for that functionality.

2. Description of change

Just renamed function to get proper array value.

array value we are pointing in:
TescoAdditionalInformation.php:39 is named tesco_code_image,
but we try to get by calling getTescoCodeUrl:
tesco_code_url

We removed img tag, because we display Tesco barcode using SVG code.
In that PR also was changed array key represents Tesco image displayed in checkout success page.
tesco_code_image => tesco_barcode_svg.

and getter function was renamed to getTescoBarcodeSvg.

3. Quality assurance

🔧 Environments:

  • Platform version: Magento CE 2.2.3.
  • Omise plugin version: Omise-Magento 2.6.
  • PHP version: 7.0.29.

✏️ Details:

Make Tesco Payment,
See if barcode is displayed properly.

4. Impact of the change

Solve issue

5. Priority of change

High,

6. Additional Notes

None

@jacstn jacstn requested a review from guzzilar January 17, 2019 09:38
@@ -1,5 +1,5 @@

<p><?= __('To finish payment, print the code below and bring it to make payment in any Tesco Lotus Store. Code is valid for the next <b>24 hours only</b>. After that time your order will be canceled.'); ?></p>
<p style="margin:.5rem 0"><?= $block->escapeHtml(__('Amount to pay'))?>: <strong><?= $block->getOrderAmount();?></b></strong>
<p><img src="<?= $block->escapeUrl($block->getTescoCodeUrl()) ?>" /></p>
<div><?= $block->getTescoCodeImage() ?></div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jacstn just wanna point out that actually looking at this change without understanding the context seems a bit strange.
While the code is changed to getTescoCodeImage() but we removed <img></img> tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after digging deeper into code, we still display in HTML - SVG code.

SVG is converted to HTML only when it is necessary - before sending it in email.

img with src was necessary when we were using URL to SVG file.
I renamed it to getTescoBarcodeSvg and corresponding array key to: tesco_barcode_svg

Copy link
Contributor

@guzzilar guzzilar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realised a small point that the code is inconsistent:

3: <p style="margin:.5rem 0"><?= $block->escapeHtml(__('Amount to pay'))?>: <strong><?= $block->getOrderAmount();?></b></strong>
4: <div><?= $block->getTescoBarcodeSvg() ?></div>

// 1 space after <?= but no space before ?>
<?= $block->escapeHtml(__('Amount to pay'))?>

// Same as above, but end with ;
<?= $block->getOrderAmount();?>

// 1 space after <?= and before ?>
<?= $block->getTescoBarcodeSvg() ?>

The change LGTM 👍

@jacstn jacstn force-pushed the display-tesco-code-fix branch from 42416ee to 527c171 Compare January 17, 2019 10:04
@jacstn jacstn merged commit 1b57610 into master Jan 17, 2019
@jacstn jacstn deleted the display-tesco-code-fix branch January 17, 2019 10:06
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.

3 participants