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 html being displayed in download virtual product text mail #16014

Merged

Conversation

@atomiix
Copy link
Contributor

atomiix commented Oct 18, 2019

Questions Answers
Branch? develop
Description? uses partial mail to render text/html list of downloadable virtual products in the download product mail
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #10915
How to test? Order a virtual product with a file and see the text mail sent

This change is Reviewable

@atomiix atomiix requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 18, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Oct 18, 2019

Hello @atomiix!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@matks matks changed the title fix html being displayed in download virtual product text mail Fix html being displayed in download virtual product text mail Oct 18, 2019
$content = $this->getPartialRenderer()->render($template_name, $this->context->language, $var);
if ($mail_type == Mail::TYPE_TEXT) {
// delete html comments in text mode so it's not visible in the mail
$content = preg_replace('/\n?<!--.*-->\n?/i', '', $content);

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Oct 18, 2019

Contributor

Maybe any spaces types? Don't know if all templates contains only \n notations, or if a developer add an email created on Mac / Windows.
Add global and dot can match new lines with ungreedy notation
for this kind of comments:

<!-- blyat -->
<p>Htmlcontent</p>
<!-- 
test 
-->
Suggested change
$content = preg_replace('/\n?<!--.*-->\n?/i', '', $content);
$content = preg_replace('/\s?<!--.*?-->\s?/gs', '', $content);

This comment has been minimized.

Copy link
@atomiix

atomiix Oct 18, 2019

Author Contributor

I did that mainly because the partial renderer add the partial file being rendered in html comment like that :

return "\n<!-- begin $tpl -->\n"
            . parent::fetch($template, $cache_id, $compile_id, $parent, $display, $merge_tpl_vars, $no_output_filter)
            . "\n<!-- end $tpl -->\n";

This comment has been minimized.

Copy link
@PierreRambaud

PierreRambaud Oct 18, 2019

Contributor

Oh, I was more thinking about comments in custom html template ^^'

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 18, 2019

Contributor

I don't understand the need to remove html comment tags, when I look at your txt template download_product_virtual_products.txt there is no html content, so why do you need to remove it?

This comment has been minimized.

Copy link
@atomiix

atomiix Oct 18, 2019

Author Contributor

That's because when the download_product_virtual_products.txt template is rendered, the renderer adds comments. Here is what I get in my mailbox when I don't remove html comments :

http://localhost/prestashop/index.php

Hi thomas baccelli,

Thank you for your order with the reference JRNKCBIHJ from PrestaShop

Product(s) to download

You have 1 product(s) now available for download using the following link(s):


<!-- begin /Users/tBaccelli/Sites/prestashop/mails/_partials/download_product_virtual_products.txt -->
  [Capture d&rsquo;écran 2019-10-15 à 17.40.05.png](http://localhost/prestashop/index.php?controller=get-file&key=756cddd141c35f16ab816bb301b14b79de34182f-6d13e7f3990af715aeb737adb923a46b5ee456fa&id_order=1932&secure_key=24926c1a734a9822e6ad4f535668f657)

<!-- end /Users/tBaccelli/Sites/prestashop/mails/_partials/download_product_virtual_products.txt -->


Follow your order and download your invoice on our shop, go to the [Order history and details](http://localhost/prestashop/index.php?controller=history) section of your customer account.

If you have a guest account, you can follow your order via the [Guest Tracking](http://localhost/prestashop/index.php?controller=guest-tracking) section on our shop.

[PrestaShop](http://localhost/prestashop/index.php)

Powered by [PrestaShop](https://www.prestashop.com/?utm_source=marchandprestashop&utm_medium=e-mail=utm_campaign=footer_1-7)

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 18, 2019

Contributor

Oh damn, seems like it's coming from our own over layer on Smarty (probably SmartyDev in this case)

return "\n<!-- begin $template -->\n"

This is completely not needed in this case, so we shouldn't use the context Smarty instance but a normal one instead.. but the thing is smarty is configured in https://github.com/PrestaShop/PrestaShop/blob/develop/config/smarty.config.inc.php

Then maybe we should add a cleanComments parameter to the MailPartialTemplateRenderer::render method

*
* @return string
*/
protected function getEmailTemplateContent($template_name, $mail_type, $var)

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 18, 2019

Contributor

As I told you I don't think this extra function is useful, it was already present in PaymentModule so I just modified the last part about partiel rendering. But anyway in the Mail::send function we already check the PS_MAIL_TYPE configuration and this is where the appropriate content is selected (or not) inside the mail body.

So I think you could simply (even drop completely) this method and simply call $this->getPartialRenderer()->render($template_name, $this->context->language, $var) to create you two variables
Sure in cas the mail config is not for BOTH you will have generated one for nothin but I the performance cost is really minimal compared to the complexity of the code

This comment has been minimized.

Copy link
@jolelievre

jolelievre Oct 18, 2019

Contributor

Actually it's probably useless in PaymentModule as well, but I wanted to make the minimum changes possible I guess

@atomiix atomiix force-pushed the atomiix:fix-virutal-product-links-in-mail-template branch from 26bc451 to 557bf9c Oct 18, 2019
Copy link
Contributor

jolelievre left a comment

Nice job @atomiix

@atomiix atomiix merged commit b3719d6 into PrestaShop:develop Oct 18, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 18, 2019

@Robin-Fischer-PS My bad, I told @atomiix to merge this because it was approved, I forgot to mention the QA

@atomiix atomiix added this to the 1.7.7.0 milestone Oct 18, 2019
@Robin-Fischer-PS Robin-Fischer-PS self-assigned this Oct 18, 2019
@Robin-Fischer-PS

This comment has been minimized.

Copy link

Robin-Fischer-PS commented Oct 18, 2019

Hi @matks :)

One small defect : The link on the mail includes number of DL and expiration date. The link still works.

pr16014 link

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Oct 21, 2019

@atomiix @matks

I still have the issue with txt email

capture d'écran_2089

@marionf

This comment has been minimized.

Copy link
Contributor

marionf commented Oct 21, 2019

My bad, I wasn't on the good store
It's ok

capture d'écran_2090

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.