Migrated attribute page compatibility

[jolelievre]

Questions	Answers
Description?	This PR integrates the module into the migrated page for AttributeGroup and Attribute
Type?	new feature
BC breaks?	no
Deprecations?	no
Fixed ticket?	Partially fixes #10508
How to test?	Test that the module integrates its custom fields properly into the net migrate AttributeGroup and Attribute pages It should still work appropriately on legacy pages (so to be tested on 1.7.8 or 8)

[Hlavtox]

@PrestaShop/qa-functional @jolelievre

Guys, please test one scenario:

• I am on 1.7.8
• I upgrade this module to 3.15.0 or install it fresh.
• I upgrade to 9.0.0
• Do the hooks work or they need to be rehooked?

[jolelievre]

@PrestaShop/qa-functional @jolelievre

Guys, please test one scenario:

• I am on 1.7.8
• I upgrade this module to 3.15.0 or install it fresh.
• I upgrade to 9.0.0
• Do the hooks work or they need to be rehooked?

Hi @Hlavtox,

They should already be in place even if they don't exist yet in the database in 1.7.8, the Module::registerHook method adds the hook in DB as long as it doesn't exist yet and then it includes the association with the modules hook. So after the module upgrade the hooks should be present (although useless) and associated to the module correctly.

Then when we upgrade to 9.0.0 the upgrade script will likely try to include those new hooks (based on the generated SQL queries we add in each major/minor versions) like here: https://github.com/PrestaShop/autoupgrade/blob/5e2c4cafb93b874a6adbd4e324d140603118b123/upgrade/sql/8.0.0.sql#L50

Since we use an INSERT IGNORE there shouldn't be an error either, the only drawback I can think of is that since the hook was added empty (no title or description with the automatic hook registration), so the content won't be updated and the hook details in UX will remain empty. Unless INSERT IGNORE can also update a matching row but I doubt it.

I'm not sure it's a huge issue though, but if we want to fix this I can think of a few solutions:

• the module could register the hook along with its details, but it feels weird it's not supposed to be the hook
• we can include in the 9.0.0 upgrade script an update query after the hook insertions just to make sure the description is correctly added
• we can create a function in the autoupgrade module to add hooks, it would be able to detect if the hook exists and would update its content with the provided details
• we can also decide that this module's next version should be a major and require v9.0 minimum, this way we have no risk of it being installed in an older version, but it's too bad because the current modification is retrocompatible

In any case, all the acceptable solutions will require some additional PR in other repositories, besides this PR is currently blocking the PR that finishes and activates the attribute migrated page in this PR, so I don't think it deserves to be blocked for the reason you mention. However, we should decide what solution we'll apply and at least create an issue for it to make sure we don't forget it.

[Hlavtox]

@jolelievre I resolved this with adding an ON DUPLICATE KEY UPDATE statements to insertions. 👍 PrestaShop/autoupgrade#669

[jolelievre]

@jolelievre I resolved this with adding an ON DUPLICATE KEY UPDATE statements to insertions. 👍 PrestaShop/autoupgrade#669

Wow! Nice and efficient, I love it!

[florine2623]

Hello @jolelievre ,

Tested the module faceted search.

Bump to 3.1.5.0 successful ✅

Addition of new hooks successful ✅

CRUD new products with new attributes/new attributes values
Configure filter template
Check in FO if filter with faceted search works

I have found a regression. It shouldn't be blocking this PR. It can be fixed in another PR. Here's the issue : PrestaShop/PrestaShop#35331
This issue is 'fixed' after I did an upgrade of PS.

Here are the steps to reproduce :
Create an attribute group + some attribute values
Create a combination product with these attributes
Go to faceted_search module > Enable attribute group > Build attributes and features indexed
Go to FO > Filter the attributes
Go to BO > Delete one attribute > Configure faceted_search > Rebuild the index
Go to FO > See that the attribute is still displayed and the page loads with no filter ❌

Screen.Recording.2024-02-12.at.11.32.16.mov

Upgrade to 8.1.x ✅

It is QA ✅

[jolelievre]

Thanks @florine2623