Use GenerateThemeMailTemplatesCommand during upgrade #434

jolelievre · 2021-11-02T14:40:24Z

Questions	Answers
Description?	Use GenerateThemeMailTemplatesCommand during upgrade instead of legacy removed Language::installEmailsLanguagePack function
Type?	bug fix
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes PrestaShop/PrestaShop#26503
How to test?	PrestaShop/PrestaShop#27276 must be merged before. Build the module based on this branch Install it and try to upgrade to 8.0.0 (you will probably need to create a release from the most recent develop branch). You shouldn't have anymore error related to mail generation (there might be other errors down on the road but it's not the purpose of this PR)
Possible impacts?	The module now relies on `GenerateThemeMailTemplatesCommand` which was introduced in PrestaShop 1.7.6 which means the module would now be only compatible with target versions above 1.7.6 I don't know if we should handle retrocompatibility with older versions? If so then we might need to extract this new code in a dedicated class or service to handle the switch more cleanly

jolelievre · 2021-11-02T14:41:20Z

classes/UpgradeTools/CoreUpgrader/CoreUpgrader17.php

@@ -28,6 +28,9 @@
 namespace PrestaShop\Module\AutoUpgrade\UpgradeTools\CoreUpgrader;

 use PrestaShop\Module\AutoUpgrade\UpgradeException;
+use PrestaShop\PrestaShop\Core\CommandBus\CommandBusInterface;


I don't know if we are allowed to use core classes like this in the upgrade module Especially since these classes may not be available in the source version and/or the target version.

Afaik you can do it:

It will only raise an error if the class is not found and executed by PHP

jolelievre · 2021-11-02T14:42:17Z

classes/UpgradeTools/CoreUpgrader/CoreUpgrader17.php

@@ -87,7 +90,33 @@ protected function upgradeLanguage($lang)
        \Language::installSfLanguagePack($lang_pack['locale'], $errorsLanguage);

        if (!$this->container->getUpgradeConfiguration()->shouldKeepMails()) {
-            \Language::installEmailsLanguagePack($lang_pack, $errorsLanguage);
+            $mailTheme = \Configuration::get('PS_MAIL_THEME', null, null, null, 'modern');


Should we handle backward compatibility and use the Language::installEmailsLanguagePack for versions older than the 1.7.6? If so maybe this should be extracted in a dedicated tool class or service.

I'm in favor to keep the backward compatibilty!

Yes keep it

classes/UpgradeTools/CoreUpgrader/CoreUpgrader80.php

matks

PR is OK IMO, if you apply latest feedbacks from @PierreRambaud

matks · 2021-12-03T14:31:35Z

PHPStan issue not related to change PrestaShop/PrestaShop#26861

classes/UpgradeTools/CoreUpgrader/CoreUpgrader80.php

PierreRambaud · 2021-12-16T15:18:25Z

classes/TaskRunner/Upgrade/UpgradeDb.php

            return new CoreUpgrader16($this->container, $this->logger);
        }

-        return new CoreUpgrader17($this->container, $this->logger);
+        if (version_compare($this->container->getState()->getInstallVersion(), '8.0.0.0', '<')) {


jolelievre

Thanks @atomiix for the finishing touch 😉
I'd like to approve but I can't approve my own PR 😅

jolelievre · 2022-01-07T13:04:55Z

Oh and it seems like PHPStan is not happy about something

jolelievre

I can't approve this PR, seems legitimate I just have one suggestion

I didn't see any modif related to the bug you mentioned regarding LegacyLoader, is it fixed in the core?

jolelievre · 2022-01-10T09:03:23Z

classes/UpgradeTools/CoreUpgrader/CoreUpgrader80.php

@@ -90,8 +90,8 @@ protected function upgradeLanguage($lang)
                $mailTheme,
                $lang_pack['locale'],
                true,
-                $frontThemeMailsFolder,
-                $frontThemeModulesFolder
+                is_dir($frontThemeMailsFolder) ? $frontThemeMailsFolder : '',


Aren't these folders supposed to always exist? Is it really needed to check their existence?

I just checked the handler, which actually handles the default folder internally when the command parameters are empty, so maybe specifying these parameters is actually not useful here? We could ignore this and let that handler handle?

These folders aren't supposed to always exist, if you check the themes/classic/ folder, you'll see no mails folder.
Also, I see that's what we do in src/PrestaShopBundle/Controller/Admin/Improve/Design/MailThemeController.php

I didn't see any modif related to the bug you mentioned regarding LegacyLoader, is it fixed in the core?

@jolelievre yep, it's fixed in the core ;)

kpodemski

I'm wondering if ModuleAdapter:: getCommandBus shouldn't throw an error if we're not in the context of 1.7+, but of course, this is an edge case scenario.

khouloudbelguith · 2022-01-11T15:01:42Z

Hi @jolelievre,

I created a release from this PR: Remove useless legacyContextLoader PrestaShop#27276
I installed PS1782 with English language & France country
I added the Algeria pack
I enabled multistore & created a new shop
I created some order
I installed the autoupgrade module using this PR
During the upgrade, I have an error

What do you think, this error is related to the PR module or the develop PR?

PS: I tried to make an upgrade from PS1778 to PS1782 using this PR => ok

Thanks!

jolelievre · 2022-01-12T09:46:55Z

Hi @khouloudbelguith

sorry I cowardly left this PR's responsibility to @atomiix so I believe he is looking into it but can't reproduce the bug...

khouloudbelguith · 2022-01-12T14:04:20Z

Hi @jolelievre, @atomiix,

I tried again with PS1782 fresh install & an upgrade to the PR & same error

Untitled_.Jan.12.2022.2_59.PM.mp4

Thanks!

atomiix · 2022-01-12T14:18:12Z

@khouloudbelguith it seems that your PHP version is 7.1, can you try with PHP 7.2 or above as the develop branch is not compatible with PHP < 7.2?

Also, you'll have to delete ps_facebook & ps_metrics before the upgrade, they are not compatible yet with develop

khouloudbelguith

Hi @atomiix,

Yes, It is ok with PHP7.3

I tried to make an upgrade from PS1782 to PrestaShop/PrestaShop#27276
But after the upgrade, I have a lot of exceptions:

Go to BO > Orders > View Order > See exception
Go to BO > Orders > Shopping cart page > See exception
Go to BO > Customers page > View customer > see exception
Go to BO > Modules > Module Catalog > See exception
Go to FO > Sign in > Se exception

Thanks!

atomiix · 2022-01-14T11:13:21Z

@khouloudbelguith I think we might be in an infinite loop as this PR #441 seems to be solving the issue you just had but to validate it you need this one 😅
Maybe we can merge this one as the upgrade itself went well, see if #441 solve the issue you had and if not, create a new issue. WDYT?

khouloudbelguith

Hi @atomiix, @jolelievre,

Could you please check the Conflicting files?

Thanks!

…y removed Language::installEmailsLanguagePack function

…of the target version

Co-authored-by: GoT <PierreRambaud@users.noreply.github.com>

khouloudbelguith

Hi @atomiix, @jolelievre,

It is ok ✔️

I tried to upgrade PS1778 to PS1783 using this PR => OK (PHP7.3)
I tried to generate modern/classic emails in different languages (RTL & non RTL)
I tried to check the HTTP of emails 'Modern template / classic template)
I tried to translate some emails and resend them

I tried to upgrade from PS1769 to 178x branch using this PR
I tried to generate some emails with RTL to check this issue: => ok

I tried to upgrade from PS1783 to this PR: PrestaShop/PrestaShop#27276 => OK
I tried to upgrade from 178x branch to this PR: PrestaShop/PrestaShop#27276 => OK

I tried to generate some emails in Arabic language & English language
I checked Gmail, outlook web version, outlook desktop version, maildev server

Untitled_.Feb.16.2022.12_56.PM.mp4

I found some bugs but not the scope of this PR, which are blocking

PS:

Before the upgrade to this PR: PrestaShop/PrestaShop#27276
I uninstall those modules: ps_metrics, ps_facebook, Prestashop Checkout, Google marketing, welcome
After the upgrade

Go to BO > Orders > View Order > See exception & Add new order from BO, will be fixed with this Add SQL upgrade queries for new unit price column #441
Go to BO > Orders > Shopping cart page > See exception => will be fixed with this Add SQL upgrade queries for new unit price column #441
Go to BO > Customers page > View customer > see exception => will be fixed with this Add SQL upgrade queries for new unit price column #441
Go to BO > Modules > Module Catalog > See exception => I will create a new issue.
Go to FO > Sign in > See exception => will be fixed with this Add SQL upgrade queries for new unit price column #441
Go to FO as a visitor & add a product to cart > see exception => will be fixed with this Add SQL upgrade queries for new unit price column #441
Go to BO > SEO & URLs > Reffers Tab > see exception => will be fixed here: Remove the page Referrers PrestaShop#27098
Go to BO > SEO & URLs page > see exception => I will create a new issue
Go to BO > Design > Email theme > choose any theme (Classic or modern) > View Htpp or any action for order_conf > see exception => will be fixed with this Add SQL upgrade queries for new unit price column #441
The Security menu is not displayed => I will create a new issue.

Thanks!

Progi1984 · 2022-02-16T12:40:12Z

Thanks @jolelievre & @khouloudbelguith

…mand Use GenerateThemeMailTemplatesCommand during upgrade

jolelievre commented Nov 2, 2021

View reviewed changes

jolelievre mentioned this pull request Nov 2, 2021

Autoupgrade - Error during upgrade to develop PrestaShop/PrestaShop#26503

Closed

jolelievre force-pushed the mail-generation-command branch from 9b79f29 to 689fd56 Compare November 3, 2021 09:33

PierreRambaud reviewed Nov 8, 2021

View reviewed changes

classes/UpgradeTools/CoreUpgrader/CoreUpgrader80.php Outdated Show resolved Hide resolved

matks reviewed Nov 10, 2021

View reviewed changes

matks mentioned this pull request Dec 3, 2021

CI is broken on autoupgrade - PHPStan with 1.6.1.18 PrestaShop/PrestaShop#26861

Closed

2 tasks

atomiix closed this Dec 13, 2021

atomiix reopened this Dec 13, 2021

atomiix previously approved these changes Dec 16, 2021

View reviewed changes

atomiix reviewed Dec 16, 2021

View reviewed changes

classes/UpgradeTools/CoreUpgrader/CoreUpgrader80.php Outdated Show resolved Hide resolved

PierreRambaud reviewed Dec 16, 2021

View reviewed changes

atomiix dismissed their stale review via d63643d January 7, 2022 10:38

jolelievre commented Jan 7, 2022

View reviewed changes

atomiix force-pushed the mail-generation-command branch 2 times, most recently from 7607803 to 21978bf Compare January 7, 2022 18:54

jolelievre commented Jan 10, 2022

View reviewed changes

jolelievre mentioned this pull request Jan 10, 2022

Remove useless legacyContextLoader PrestaShop/PrestaShop#27276

Merged

kpodemski previously approved these changes Jan 10, 2022

View reviewed changes

kpodemski added the waiting for QA label Jan 10, 2022

khouloudbelguith self-assigned this Jan 11, 2022

khouloudbelguith added waiting for author and removed waiting for QA labels Jan 11, 2022

jolelievre assigned atomiix Jan 12, 2022

khouloudbelguith suggested changes Jan 14, 2022

View reviewed changes

khouloudbelguith added waiting for author and removed waiting for QA labels Jan 14, 2022

khouloudbelguith suggested changes Jan 31, 2022

View reviewed changes

atomiix dismissed kpodemski’s stale review via de5d119 January 31, 2022 13:43

atomiix force-pushed the mail-generation-command branch from 21978bf to de5d119 Compare January 31, 2022 13:43

atomiix removed the waiting for author label Jan 31, 2022

jolelievre and others added 5 commits February 9, 2022 17:49

Use GenerateThemeMailTemplatesCommand during upgrade instead of legac…

abe45b1

…y removed Language::installEmailsLanguagePack function

Create new CoreUpgrade80 to handle backward compatibility regardless …

6ea0622

…of the target version

Remove comment

ddf316d

Co-authored-by: GoT <PierreRambaud@users.noreply.github.com>

Get command bus from moduleAdapter

be6d365

Fix ps version detection & mail generation output folders

b99c34d

atomiix force-pushed the mail-generation-command branch from de5d119 to b99c34d Compare February 9, 2022 16:54

Fix tests

86678ea

atomiix force-pushed the mail-generation-command branch from 1956b15 to 86678ea Compare February 9, 2022 17:49

PierreRambaud approved these changes Feb 15, 2022

View reviewed changes

Progi1984 approved these changes Feb 15, 2022

View reviewed changes

Progi1984 requested a review from khouloudbelguith February 15, 2022 10:36

Progi1984 added the waiting for QA label Feb 15, 2022

Progi1984 added this to the 4.15.0 milestone Feb 15, 2022

khouloudbelguith approved these changes Feb 16, 2022

View reviewed changes

khouloudbelguith added QA ✔️ and removed waiting for QA labels Feb 16, 2022

khouloudbelguith mentioned this pull request Feb 16, 2022

Add SQL upgrade queries for new unit price column #441

Merged

Progi1984 merged commit 7bf3dfa into PrestaShop:dev Feb 16, 2022

Progi1984 mentioned this pull request Feb 25, 2022

Database update command - Wrong SQL Query generated PrestaShop/PrestaShop#24584

Merged

matks mentioned this pull request Apr 25, 2022

Release 4.15.0 #481

Closed

atomiix pushed a commit to atomiix/autoupgrade that referenced this pull request Sep 16, 2022

Merge pull request PrestaShop#434 from jolelievre/mail-generation-com…

cbe7ee5

…mand Use GenerateThemeMailTemplatesCommand during upgrade

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use GenerateThemeMailTemplatesCommand during upgrade #434

Use GenerateThemeMailTemplatesCommand during upgrade #434

jolelievre commented Nov 2, 2021 •

edited by atomiix

Loading

jolelievre Nov 2, 2021

PierreRambaud Nov 8, 2021

jolelievre Nov 2, 2021

atomiix Nov 2, 2021

PierreRambaud Nov 8, 2021

matks left a comment

matks commented Dec 3, 2021

PierreRambaud Dec 16, 2021

jolelievre left a comment

jolelievre commented Jan 7, 2022

jolelievre left a comment

jolelievre Jan 10, 2022

atomiix Jan 10, 2022

atomiix Jan 10, 2022

kpodemski left a comment

khouloudbelguith commented Jan 11, 2022

jolelievre commented Jan 12, 2022

khouloudbelguith commented Jan 12, 2022

atomiix commented Jan 12, 2022 •

edited

Loading

khouloudbelguith left a comment

atomiix commented Jan 14, 2022 •

edited

Loading

khouloudbelguith left a comment

khouloudbelguith left a comment

Progi1984 commented Feb 16, 2022

Use GenerateThemeMailTemplatesCommand during upgrade #434

Use GenerateThemeMailTemplatesCommand during upgrade #434

Conversation

jolelievre commented Nov 2, 2021 • edited by atomiix Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matks left a comment

Choose a reason for hiding this comment

matks commented Dec 3, 2021

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

jolelievre commented Jan 7, 2022

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kpodemski left a comment

Choose a reason for hiding this comment

khouloudbelguith commented Jan 11, 2022

jolelievre commented Jan 12, 2022

khouloudbelguith commented Jan 12, 2022

atomiix commented Jan 12, 2022 • edited Loading

khouloudbelguith left a comment

Choose a reason for hiding this comment

atomiix commented Jan 14, 2022 • edited Loading

khouloudbelguith left a comment

Choose a reason for hiding this comment

khouloudbelguith left a comment

Choose a reason for hiding this comment

Progi1984 commented Feb 16, 2022

jolelievre commented Nov 2, 2021 •

edited by atomiix

Loading

atomiix commented Jan 12, 2022 •

edited

Loading

atomiix commented Jan 14, 2022 •

edited

Loading