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

Remove module list item when force deletion #32662

Merged
merged 1 commit into from Jun 27, 2023

Conversation

M0rgan01
Copy link
Contributor

@M0rgan01 M0rgan01 commented May 23, 2023

Questions Answers
Branch? develop
Description? When deleting a module, we can dynamically remove the module from the list, or force the reload of the page. I chose to do it dynamically in JS for user experience.
We do not remove the module from the list if it has a download url, to be able to re-download it if necessary
I also modified the behavior of the dropdowns for the buttons after uninstallation, in fact it was possible to delete a module that was already deleted
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
How to test? see #30952
Fixed ticket? Fixes #30952
Related PRs -
Sponsor company -

Attention, for the modifications to work correctly, the url of the distribution API must be modified for the purposes of the tests. Indeed the recovery of the list of modules for 8.1.x is not yet possible. See https://api.prestashop-project.org/prestashop for versions and modify the PrestaShop/modules/ps_distributionapiclient/src/DistributionApi.php file
image

@M0rgan01 M0rgan01 requested a review from a team as a code owner May 23, 2023 07:58
@prestonBot prestonBot added develop Branch Bug fix Type: Bug fix labels May 23, 2023
Copy link
Member

@boherm boherm left a comment

Choose a reason for hiding this comment

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

LGTM, after ESLint fix!
Just one question for .hide() instead of .remove(). 😄

@@ -423,6 +423,10 @@ export default class ModuleCard {
mainElement.attr('data-installed', '0');
mainElement.attr('data-active', '0');

if (forceDeletion === 'true' || forceDeletion === true && moduleTechName !== 'ps_distributionapiclient') {
mainElement.hide();
Copy link
Member

Choose a reason for hiding this comment

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

I'm not blocking for this, but why not use .remove() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, remove is better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The transition from hide to remove required a small additional tweak. You have to send the event before removing.
( this.eventEmitter.emit('Module Uninstalled', mainElement);)

Now it is good 😄

Copy link
Contributor

@Hlavtox Hlavtox left a comment

Choose a reason for hiding this comment

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

We should not be hardcoding a module name in the core, other distributions can have a different marketplace.

Also, we should check if this module is available from an API before removing it from the list, because it will reappear after refresh. 👍

tleon
tleon previously approved these changes May 23, 2023
boherm
boherm previously approved these changes May 23, 2023
0x346e3730
0x346e3730 previously approved these changes May 30, 2023
boherm
boherm previously approved these changes May 30, 2023
@boherm boherm added the Waiting for QA Status: action required, waiting for test feedback label May 30, 2023
FabienPapet
FabienPapet previously approved these changes May 30, 2023
@0x346e3730 0x346e3730 requested a review from Hlavtox May 30, 2023 09:17
Copy link

@AchrafKchaou1991 AchrafKchaou1991 left a comment

Choose a reason for hiding this comment

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

Hello @M0rgan01
It seems the issue still persist, see the attached screen record below:

Pr32662.webm

Waiting for your feedback.
Thanks!

@MhiriFaten MhiriFaten 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 31, 2023
@M0rgan01 M0rgan01 dismissed stale reviews from FabienPapet, boherm, and 0x346e3730 via 0724667 May 31, 2023 07:53
@M0rgan01 M0rgan01 force-pushed the fix/30952 branch 4 times, most recently from 95066d1 to 84868df Compare May 31, 2023 08:24
@hibatallahAouadni hibatallahAouadni added the Waiting for author Status: action required, waiting for author feedback label Jun 8, 2023
@hibatallahAouadni hibatallahAouadni removed their assignment Jun 8, 2023
@M0rgan01 M0rgan01 dismissed stale reviews from kpodemski and boherm via 7e32597 June 12, 2023 16:18
@M0rgan01 M0rgan01 force-pushed the fix/30952 branch 2 times, most recently from 7e32597 to 9e4bf66 Compare June 12, 2023 16:22
@M0rgan01 M0rgan01 requested a review from a team as a code owner June 12, 2023 16:22
@M0rgan01 M0rgan01 changed the base branch from develop to 8.1.x June 12, 2023 16:22
@M0rgan01 M0rgan01 force-pushed the fix/30952 branch 2 times, most recently from 8430d50 to b7ad705 Compare June 14, 2023 14:31
@matks
Copy link
Contributor

matks commented Jun 14, 2023

@hibatallahAouadni It is important to be aware that the behavior for a development branch and the behavior for a live version are not the same.

If you are on a live version, let's say you use 8.0.4

If you uninstall the module pagenotfound and you choose to delete the files from the disk then the module is not available anymore in your modules/ folder. However PrestaShop is able to poll the API endpoint https://api.prestashop-project.org/modules/8.0.4 and to find it. Then PrestaShop is able to display a "Install" button, because it is capable of retrieving the module from the API.

If you are on a live version, let's say you use develop branch -> it means version is 9.0.0

There is no endpoint https://api.prestashop-project.org/modules/9.0.0

If you uninstall the module pagenotfound and you choose to delete the files from the disk then the module is not available anymore in your modules/ folder. And PrestaShop is NOT able to poll the API endpoint https://api.prestashop-project.org/modules/9.0.0 to find it. Then PrestaShop is NOT able to display a "Install" button for pagenotfound module, because it is not capable of retrieving the module from the API. So the pagenotfound module is not shown in the modules list, and it is the expected behavior

@M0rgan01
Copy link
Contributor Author

@Hlavtox I propose a new version of my PR :)

@prestashop-issue-bot prestashop-issue-bot bot removed the Waiting for author Status: action required, waiting for author feedback label Jun 15, 2023
Copy link
Contributor

@tleon tleon left a comment

Choose a reason for hiding this comment

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

LGTM, I'm not the best at js / ts would be great if others could have a look.

@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Jun 27, 2023
Copy link

@aniszr aniszr left a comment

Choose a reason for hiding this comment

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

Hello @M0rgan01

Thanks for your PR,the module display issue is solved.LGTM ✔️

See attached screenvideo:

Module.manager.testAniss.mp4

Thanks!

@AureRita AureRita added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Jun 27, 2023
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@kpodemski kpodemski added this to the 8.1.1 milestone Jun 27, 2023
@kpodemski kpodemski dismissed Hlavtox’s stale review June 27, 2023 19:49

not relevant anymore

@kpodemski kpodemski merged commit 1a8fdfb into PrestaShop:8.1.x Jun 27, 2023
38 checks passed
@kpodemski
Copy link
Contributor

thank you @M0rgan01

@aniszr aniszr self-assigned this Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug fix Type: Bug fix develop Branch QA ✔️ Status: check done, code approved
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Module does not disappear from module list after uninstalling and deleting it