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

Scan translation keys from email themes #14719

Merged
merged 11 commits into from Aug 8, 2019

Conversation

@jolelievre
Copy link
Contributor

commented Jul 17, 2019

Questions Answers
Branch? 1.7.6.x
Description? Change config to scan mails themes folders, update catalog with missing keys for emails, fix a few wording that were wrong in templates
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #14633 and fixes #14337
How to test? ~

This change is Reviewable

@jolelievre jolelievre added this to the 1.7.6.1 milestone Jul 17, 2019
@jolelievre jolelievre requested a review from PrestaShop/prestashop-core-developers as a code owner Jul 17, 2019
.t9n.yml Outdated Show resolved Hide resolved
Copy link
Member

left a comment

LGTM, waiting for @LouiseBonnard

@LouiseBonnard

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2019

Hi there, can you confirm it is too late to apply the changes requested here? Can I milestone it for 1.7.7?

@@ -1163,17 +1163,17 @@ Comment: check if some ids are in list_skip_actions and forbid deletion</note>
<trans-unit id="3452da55a90ccedd184c7ba264b2afd5">
<source>Due to memory limit restrictions, this image cannot be loaded. Please increase your memory_limit value via your server's configuration settings. </source>

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jul 19, 2019

Contributor

With the space at the end?

<note>Line: 537</note>
</trans-unit>
<trans-unit id="c7e728f436eee2692d6c6f756621a70e">
<source>An error occured, please check your zip file</source>

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jul 19, 2019

Contributor
Suggested change
<source>An error occured, please check your zip file</source>
<source>An error occurred, please check your zip file</source>

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jul 19, 2019

Contributor

But I think that this shouldn't be in the catalog, it has been removed from Crowdin since 1.7.4.

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jul 19, 2019

Contributor

Actually, all these keys from the Theme Custo module do not have to be here. I remember that it has been removed because after the 1.7.4 release because it came with its own translation. Anyway, as seen with @colinegin and @eternoendless lately, a small cleaning among the native modules needs to be done since there should not be all modules translatable via Crowdin.

app/Resources/translations/default/messages.xlf Outdated Show resolved Hide resolved
@jolelievre jolelievre dismissed stale reviews from eternoendless and PierreRambaud via 984a908 Jul 19, 2019
@jolelievre jolelievre force-pushed the jolelievre:catalog-emails branch from 7665de2 to 1e522d0 Jul 19, 2019
@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

Alright this should be good, @LouiseBonnard @eternoendless if you can please check it one last time Thank you

app/Resources/translations/default/messages.xlf Outdated Show resolved Hide resolved
app/Resources/translations/default/messages.xlf Outdated Show resolved Hide resolved
app/Resources/translations/default/messages.xlf Outdated Show resolved Hide resolved
mails/themes/classic/components/footer.html.twig Outdated Show resolved Hide resolved
mails/themes/classic/core/log_alert.html.twig Outdated Show resolved Hide resolved
@@ -189,7 +189,7 @@
<tr>
<td align="left" style="font-size:0px;padding:10px 25px;word-break:break-word;">
<div style="font-family:Open sans, arial, sans-serif;font-size:14px;line-height:25px;text-align:left;color:#363A41;">
{{ 'Replenish your inventory, go to the <span class="label">"Catalog" &gt; "Stocks"</span> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'Replenish your inventory, go to the <span class="label">"Catalog" > "Stocks"</span> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jul 22, 2019

Contributor
Suggested change
{{ 'Replenish your inventory, go to the <span class="label">"Catalog" > "Stocks"</span> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'Replenish your inventory, go to the <span class="label">Catalog > Stocks</span> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jul 22, 2019

Contributor

Shouldn't it be the same balises (I don't know the English word, aha) than before?

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jul 23, 2019

Author Contributor

It is the same tag (😉) <span class="label"> If you compare it with the classic theme where we use <span><strong> then yes it is normal, the theme css and integration are different. I'm not a big fan of including html in the translation though because it means they are not compatible with other themes..
But this would require to split the message into three parts, or maybe include placeholders like this:

'Replenish your inventory, go to the [bold]Catalog > Stocks[\bold] section of your back office to manage your stock.'

@@ -255,7 +255,7 @@
<tr>
<td align="left" style="font-size:0px;padding:10px 25px;padding-top:0;word-break:break-word;">
<div style="font-family:Open sans, arial, sans-serif;font-size:14px;line-height:25px;text-align:left;color:#363A41;">
{{ 'Replenish your inventory, go to the <span class="label">"Catalog" &gt; "Stocks"</span> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'Replenish your inventory, go to the <span class="label">"Catalog" > "Stocks"</span> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jul 22, 2019

Contributor

Same.

app/Resources/translations/default/messages.xlf Outdated Show resolved Hide resolved
</file>
<file original="modules/ps_emailsubscription/ps_emailsubscription.php" source-language="en-US" target-language="en" datatype="plaintext">
<body>
<trans-unit id="2df860f82be52de57fc5e3f7ecac9431">

This comment has been minimized.

Copy link
@LouiseBonnard

LouiseBonnard Jul 22, 2019

Contributor

Same than Theme Custo, the two following strings have been removed from the Crowdin catalog since the 1.7.4 version, perhaps because they already have their own translation. I can see three solutions:

  • we can properly localize them and we put them in the Modules.Emailsubscription.Admin file, where they belong
  • we keep them as-is because we cannot change domain at this point
  • we delete them from the code here, but it could be risky

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jul 23, 2019

Author Contributor

The new domain is managed in v2.4.0, but the one included in 176 is the 2.3.0
So we would need to update the included module, but I'm not sure we should do it in this version. I'm afraid we need to wait for the 177 version

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jul 23, 2019

Author Contributor

Oh and this case is different from ps_themecusto which includes its own translations, this module has no translations embedded so we need to manage them in the core.
By the way even for ps_themecusto although it includes some translations, there are only three languages managed, but I guess it can be managed via the backoffice then?

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 26, 2019

Member

Same comment as with gamification here

@sarahdib

This comment has been minimized.

Copy link

commented Jul 22, 2019

@jolelievre the issue 14633 is not solved :
Capture d’écran 2019-07-22 à 15 14 44

For this issue 14337

Wording is ok for classic template but not for modern.

@eternoendless eternoendless changed the title Scan trad keys from email themes Scan translation keys from email themes Jul 26, 2019
<note>Line: 380</note>
</trans-unit>
</body>
</file>

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 26, 2019

Member

Why are these wordings being deleted? We shouldn't delete any wording during the 1.7 lifecycle.

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jul 30, 2019

Author Contributor

Maybe because the file was removed, but I'll revert this change so that nothing is removed

app/Resources/translations/default/messages.xlf Outdated Show resolved Hide resolved
app/Resources/translations/default/messages.xlf Outdated Show resolved Hide resolved
</file>
<file original="modules/ps_emailsubscription/ps_emailsubscription.php" source-language="en-US" target-language="en" datatype="plaintext">
<body>
<trans-unit id="2df860f82be52de57fc5e3f7ecac9431">

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 26, 2019

Member

Same comment as with gamification here

@@ -27,7 +27,7 @@
{% endif %}
<span>
{{ '<span><strong>Warning:</strong></span> you have received a new log alert in your Back Office.'|trans({}, 'Emails.Body', locale)|raw }}<br/><br/>
{{ 'You can check for it in the <span><strong>"Tools" &gt; "Logs"</strong></span> section of your Back Office.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'You can check for it in the <span><strong>Advanced Parameters > Logs</strong></span> section of your back office.'|trans({}, 'Emails.Body', locale)|raw }}

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 26, 2019

Member

Since this wording is being used raw, you need to keep the &gt;

Suggested change
{{ 'You can check for it in the <span><strong>Advanced Parameters > Logs</strong></span> section of your back office.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'You can check for it in the <span><strong>Advanced Parameters &gt; Logs</strong></span> section of your back office.'|trans({}, 'Emails.Body', locale)|raw }}

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 26, 2019

Member

It would be best to get rid of the html in the wording, don't you think?

Suggested change
{{ 'You can check for it in the <span><strong>Advanced Parameters > Logs</strong></span> section of your back office.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'You can check for it in the [1]Advanced Parameters &gt; Logs[/1] section of your back office.'|trans({'[1]' => '<span><strong>', '[/1]' => '</strong></span>'}, 'Emails.Body', locale)|raw }}
@@ -28,7 +28,7 @@
<span>
{{ 'The remaining stock is now less than the specified minimum of'|trans({}, 'Emails.Body', locale)|raw }} <strong><span>{last_qty}.</span></strong><br/><br/>
<strong><span>{{ 'Remaining stock:'|trans({}, 'Emails.Body', locale)|raw }}</span></strong> {qty}<br/><br/>
{{ 'You are advised to open the product\'s admin Product Page in order to replenish your inventory.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'Replenish your inventory, go to the <span><strong>Catalog > Stocks</span></strong> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 26, 2019

Member
Suggested change
{{ 'Replenish your inventory, go to the <span><strong>Catalog > Stocks</span></strong> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'Replenish your inventory, go to the <span><strong>Catalog &gt; Stocks</span></strong> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}
@@ -28,7 +28,7 @@
<span>
{{ 'The remaining stock is now less than the specified minimum of'|trans({}, 'Emails.Body', locale)|raw }} <strong><span>{last_qty}.</span></strong><br/><br/>
<strong><span>{{ 'Remaining stock:'|trans({}, 'Emails.Body', locale)|raw }}</span></strong> {qty}<br/><br/>
{{ 'You are advised to open the product\'s admin Product Page in order to replenish your inventory.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'Replenish your inventory, go to the <span><strong>Catalog > Stocks</span></strong> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 26, 2019

Member
Suggested change
{{ 'Replenish your inventory, go to the <span><strong>Catalog > Stocks</span></strong> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}
{{ 'Replenish your inventory, go to the <span><strong>Catalog &gt; Stocks</span></strong> section of your back office to manage your stock.'|trans({}, 'Emails.Body', locale)|raw }}
<tr>
<td align="left" style="font-size:0px;padding:10px 25px;word-break:break-word;">
<div style="font-family:Open sans, arial, sans-serif;font-size:14px;line-height:25px;text-align:left;color:#363A41;">
{{ 'Go to your customer account to learn more about it.'|trans({}, 'Emails.Body', locale)|raw }}

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 26, 2019

Member

There's no HTML in this one, so why the raw?

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jul 30, 2019

Author Contributor

You're right, removed!

<td
class="" width="604px"
>

This comment has been minimized.

Copy link
@eternoendless

eternoendless Jul 26, 2019

Member

I suppose this removal is wanted?

This comment has been minimized.

Copy link
@jolelievre

jolelievre Jul 30, 2019

Author Contributor

Yes a text was moved inside inside a border, so the generated template was modified

@jolelievre jolelievre force-pushed the jolelievre:catalog-emails branch 3 times, most recently from e8a5bfd to 3c9afb1 Jul 30, 2019
@jolelievre jolelievre force-pushed the jolelievre:catalog-emails branch from 350cdf7 to 5da0043 Aug 5, 2019
Copy link
Member

left a comment

LGTM

@marionf

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Is it ready for QA ? I see no answer to this comment: #14719 (comment)

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

@marionf the PR is ready, although it's not simple to test because it mainly contains catalog updates, which then need to be sent to crowdin and translated so that the translation is effective.

You can test it manually for a few keys though, you need to copy a translation block from the default catalog, copy it in your fr-FR catalog - for example - and then modify the target field to check that it works.

Thus for the comment you mentioned you can copy this:

<file original="mails/themes/modern/core/order_conf.html.twig" source-language="en-US" target-language="en" datatype="plaintext">
    <body>
      <trans-unit id="fb077ecba55e5552916bde26d8b9e794">
        <source>Order confirmation</source>
        <target>Order confirmation</target>
        <note>Line: 3</note>
      </trans-unit>
      <trans-unit id="ca102bf009d3e0d62a4fc6918b20110e">
        <source>Thank you for shopping on [1]{shop_name}[/1]!</source>
        <target>Merci pour votre achat sur [1]{shop_name}[/1]!</target>
        <note>Line: 53</note>
      </trans-unit>
    </body>
  </file>

inside your app/Resources/translations/fr-FR/EmailsBody.fr-FR.xlf file

BUT on the other side the modif for this issue #14337 should be easier to test

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

By the way @eternoendless I just tested to translate manually a key containing &gt; as we said, and as I feared it didn't match with the catalog key. So I had to modify the templates so that their keys contain a >
This is a quick fix but we should be careful about it in the next version, I don't know if the problem comes from the translator or the keys extraction (my guess is the later).

@eternoendless

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

This should be investigated later, tracked by PrestaShop/TranslationToolsBundle#66

Copy link
Member

left a comment

The catalogue must be re-exported again.

app/Resources/translations/default/EmailsBody.xlf Outdated Show resolved Hide resolved
@jolelievre jolelievre force-pushed the jolelievre:catalog-emails branch from e14a8ea to 3d26fb6 Aug 8, 2019
@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

Hi @jolelievre,

This ticket: #14337 is fixed except his issue: #14719 (comment)
To test this issue: #14633
I tried with this steps:

<file original="mails/themes/modern/core/order_conf.html.twig" source-language="en-US" target-language="en" datatype="plaintext">
    <body>
      <trans-unit id="fb077ecba55e5552916bde26d8b9e794">
        <source>Order confirmation</source>
        <target>Order confirmation</target>
        <note>Line: 3</note>
      </trans-unit>
      <trans-unit id="ca102bf009d3e0d62a4fc6918b20110e">
        <source>Thank you for shopping on [1]{shop_name}[/1]!</source>
        <target>Merci pour votre achat sur [1]{shop_name}[/1]!</target>
        <note>Line: 53</note>
      </trans-unit>
    </body>
  </file>

inside your app/Resources/translations/fr-FR/EmailsBody.fr-FR.xlf file

I tried to launch new installation, but I have this error:

Warning: Uncaught Symfony\Component\Debug\Exception\ContextErrorException: Warning: require(/projet/QA/QA/var/cache/dev/ContainerOkc5fca/getPrestashop_Core_CommandBusService.php): failed to open stream: No such file or directory in /projet/QA/QA/var/cache/dev/ContainerOkc5fca/appDevDebugProjectContainer.php:1692 Stack trace: #0 /projet/QA/QA/var/cache/dev/ContainerOkc5fca/appDevDebugProjectContainer.php(1692): require() #1 /projet/QA/QA/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php(304): ContainerOkc5fca\appDevDebugProjectContainer->load('getPrestashop_C...') #2 /projet/QA/QA/classes/Language.php(1165): Symfony\Component\DependencyInjection\Container->get('prestashop.core...') #3 /projet/QA/QA/classes/Language.php(1226): LanguageCore::generateEmailsLanguagePack(Array, Array, false) #4 /projet/QA/QA/classes/Language.php(1077): LanguageCore::updateLanguagePack('en', Array) #5 /projet/QA/QA/classes/Language.php(1268): LanguageCore::downloadAndInstallLanguagePack('en', NULL, NULL, false) # in /projet/QA/QA/var/cache/dev/ContainerOkc5fca/appDevDebugProjectContainer.php on line 1692
image

Thanks!

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Hi @khouloudbelguith ,

you don't need to reinstall the shop, once you updated the xlf catalogue, you can see the change either in the Email theme preview page, or you can regenerate the emails for the french language and check the static files in mails/fr

@jolelievre

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

About the error you get in the install process, it seems like a permission error on your cache folder during the install but I don't think it is linked to this issue nor the modif from this PR

@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@jolelievre, thanks!
Yes, clear the cache => installation => OK
It is OK
image

Thanks!

@khouloudbelguith

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@jolelievre, In fact, in the BO => International => translation =>

Type of translation => Email translations
Select the type of email content => Body
Select your theme => Core
Select your language => Français

I tried to change the translations for the order_conf => successfully updated.
But in these changes are not applied
https://drive.google.com/file/d/18OwYleSBzuYBepmHJw1oXZ2zCDQuo-k5/view

Thanks!

@jolelievre jolelievre merged commit 49b5b95 into PrestaShop:1.7.6.x Aug 8, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jolelievre jolelievre deleted the jolelievre:catalog-emails branch Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.