Navigation Menu

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

Allow clearing modules cache for all shops #28445

Merged
merged 3 commits into from Jun 1, 2022

Conversation

sowbiba
Copy link
Contributor

@sowbiba sowbiba commented May 9, 2022

Questions Answers
Branch? develop
Description? Add parameter to allow clearing the modules list cache for all shops, in a multishop context
Type? bug fix
Category? CO
BC breaks? yes
Deprecations? no
Fixed ticket? Fixes #28446
How to test? Please use prestashopexamplemodule.zip which implements "enable/disable all shops at once"

BC Breaks

ModuleRepository::clearCache now have a second optional parameter $allShop, boolean type, default to false, to allow clearing the cache for all the shops of the BO

@sowbiba sowbiba requested a review from a team as a code owner May 9, 2022 11:27
@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix BC break Type: Introduces a backwards-incompatible break labels May 9, 2022
@sowbiba sowbiba force-pushed the clear-module-cache-all-shops branch from d43b557 to da3d940 Compare May 9, 2022 11:39
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

A small feedback

@sowbiba sowbiba force-pushed the clear-module-cache-all-shops branch from 5a655bf to d26b527 Compare May 9, 2022 12:22
@Progi1984
Copy link
Contributor

A small feedback

@sowbiba sowbiba requested a review from Progi1984 May 9, 2022 19:45
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

A small comment

src/Core/Module/ModuleRepository.php Outdated Show resolved Hide resolved
@sowbiba sowbiba requested a review from Progi1984 May 10, 2022 08:40
Progi1984
Progi1984 previously approved these changes May 11, 2022
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

@sowbiba If you have time for adding PHPDoc, it will be better

@sowbiba sowbiba requested a review from Progi1984 May 11, 2022 09:45
jolelievre
jolelievre previously approved these changes May 11, 2022
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @sowbiba

@Progi1984 Progi1984 added this to the 8.0.0 milestone May 12, 2022
Copy link
Contributor

@Progi1984 Progi1984 left a comment

Choose a reason for hiding this comment

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

A last feedback for inheritance

Co-authored-by: Progi1984 <progi1984@gmail.com>
@matks matks added the Needs documentation Needs an update of the developer documentation label May 17, 2022
@matks matks added the Waiting for QA Status: action required, waiting for test feedback label May 17, 2022
@marwachelly marwachelly self-assigned this May 17, 2022
@marwachelly
Copy link

Hello @sowbiba,

The issue is a regression 1.7.8.0 so I think it should be fixed on 1.7.8.x branch.
Also I noticed without the PR that the problem is not present on the branch develop following the scenario :

  1. Enable multistore
  2. Create a new shop
  3. All shop is selected

on develop :

When All shop is selected (we have only the options unistall and reset)
image

When a specific shop is selected(we have the options : Disable Mobile/Disable/reset and Uninstall)
image

on 1.7.8.x :

When All shop is selected(we have the options : Disable Mobile/Disable/reset and Uninstall)
image

When a specific shop is selected(we have the options : Disable Mobile/Disable/reset and Uninstall)
image

Ping @PrestaShop/product-team Is it a wanted behavior?

Thanks,

@marwachelly marwachelly added Waiting for author Status: action required, waiting for author feedback Waiting for PM Status: action required, waiting for product feedback and removed Waiting for QA Status: action required, waiting for test feedback labels May 18, 2022
@sowbiba
Copy link
Contributor Author

sowbiba commented May 26, 2022

I'm sorry @marwachelly but you are looking the wrong direction
The actions available are not the subject. Maybe there is an issue, I don't know

Here it's about the consistency of the module status for every shop (single shop context)

In my example from the issue, the module is supposed to be disabled in the 2 shops but it's displayed as active in Shop1 and inactive for Shop2. If you remove the cache, the statuses are correct

@sowbiba sowbiba added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback labels May 27, 2022
Copy link

@marwachelly marwachelly left a comment

Choose a reason for hiding this comment

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

I'm sorry @marwachelly but you are looking the wrong direction The actions available are not the subject. Maybe there is an issue, I don't know

Here it's about the consistency of the module status for every shop (single shop context)

In my example from the issue, the module is supposed to be disabled in the 2 shops but it's displayed as active in Shop1 and inactive for Shop2. If you remove the cache, the statuses are correct

Hello @sowbiba,

Setting the module active / inactive in the develop branch in all shop context is not posssible. (When All shop is selected (we have only the options unistall and reset) So the bug is not reproducible on the branch develop.

Untitled_.May.30.2022.2_52.PM.mp4

The bug is reproducible also with PS 1.7.8.x and incognito browsing:

Untitled_.May.30.2022.3_33.PM.mp4

The bug is reproducible with PS1.7.7.8:

Untitled_.May.30.2022.3_41.PM.mp4

@PrestaShop/product-team Could you please check my comment above?

Thanks,

@sowbiba
Copy link
Contributor Author

sowbiba commented May 30, 2022

Hello @marwachelly
Select Shop 1, activate the module
Select Shop 2, see that the module is still disabled

@marwachelly
Copy link

marwachelly commented May 30, 2022

Hello @marwachelly Select Shop 1, activate the module Select Shop 2, see that the module is still disabled

Did you mean that?

Untitled_.May.30.2022.3_45.PM.mp4

But I think this is the expected behavior. The shop1 shouldn't have an influence on shop2. Isn't it?

even with incognito browsing on develop and without PR i didn't manage to reproduce the issue

Untitled_.May.30.2022.4_09.PM.mp4

Did I miss something?

Thanks,

@marwachelly marwachelly added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels May 30, 2022
@sowbiba
Copy link
Contributor Author

sowbiba commented May 30, 2022

exactly you got it !!

It's the expected behaviour because the module is coded like that
In a module you can have your own logic about what to do when enabling/disabling the module in a multishop context

For my module given in the example, the status is consistant between all the shops : disabling in one shop will make it disabled for the whole Backoffice. But I can't do this because of the cache which is not cleared after the actions

@sowbiba sowbiba added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback labels May 30, 2022
Copy link

@marwachelly marwachelly left a comment

Choose a reason for hiding this comment

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

Hello @sowbiba ,

PR is tested and the issue is fixed.

Untitled_.Jun.1.2022.4_36.PM.mp4

I detected an exception relaed to the module. As discussed in slack, the exception is not blocking the PR (just it is related to the module).
image

So PR is QA ✔️

Many thanks,

@marwachelly marwachelly added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback Waiting for PM Status: action required, waiting for product feedback labels Jun 1, 2022
@sowbiba
Copy link
Contributor Author

sowbiba commented Jun 1, 2022

Thank you all ❤️

@sowbiba sowbiba merged commit edcf2ff into PrestaShop:develop Jun 1, 2022
@sowbiba sowbiba deleted the clear-module-cache-all-shops branch June 1, 2022 16:09
@kpodemski kpodemski added Documentation ✔️ Developer documentation is up-to-date and removed Needs documentation Needs an update of the developer documentation labels Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break Type: Introduces a backwards-incompatible break Bug fix Type: Bug fix develop Branch Documentation ✔️ Developer documentation is up-to-date QA ✔️ Status: check done, code approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modules list cache is not correctly cleared in a multishop context
7 participants