Skip to content

Load migration script in plugin setting page #920

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jan 8, 2024
Merged

Load migration script in plugin setting page #920

merged 2 commits into from
Jan 8, 2024

Conversation

mukeshpanchal27
Copy link
Member

Summary

In #899, we incorporated logic to load the migration script only when any module activated with the associated standalone plugins.

However, in #900, we further enhanced the experience when installing and activating standalone plugins. There's a need to ensure that this enhancement still functions properly if a module is deactivated and someone tries to install and activate the standalone plugins from the settings page.

To replicate the issue:

  1. Log in to the admin panel.
  2. Go to Settings > Performance and deactivate all modules.
  3. Install any module from the 'standalone performance plugins' list and activate the plugin.
  4. Observe if it redirects to the plugin list page instead of settings page.

Checklist

  • PR has either [Focus] or Infrastructure label.
  • PR has a [Type] label.
  • PR has a milestone or the no milestone label.

@mukeshpanchal27 mukeshpanchal27 added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release Creating standalone plugins labels Jan 5, 2024
@mukeshpanchal27 mukeshpanchal27 self-assigned this Jan 5, 2024
@mukeshpanchal27 mukeshpanchal27 marked this pull request as ready for review January 5, 2024 06:22
This was referenced Jan 5, 2024
Merged
Closed
@felixarntz
Copy link
Member

Thanks @mukeshpanchal27, good catch!

Regarding the fix though, I think there would be a cleaner solution:

  • The only reason we would need to load that script regardless of the migration is for the plugin installation enhancement. That part however has nothing to do with module migration anyway. So it's arguably not in the right JS file, or the JS file has a misleading name.
  • Therefore, I would suggest we move the logic related to the plugin installation/activation into another JS file (e.g. something like perflab-plugin-management.js). Then we can load that JS file unconditionally, while continuing to load the perflab-module-migration-notice.js file only when relevant.

@mukeshpanchal27
Copy link
Member Author

Thank you for the feedback! In the commit abc39aa, I've incorporated the new JS as per your suggestion and relocated both JS file into a designated JS folder.

felixarntz
felixarntz approved these changes Jan 8, 2024
View reviewed changes
Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

This looks great, thank you @mukeshpanchal27!

@felixarntz felixarntz merged commit 8c6d56d into feature/creating-standalone-plugins Jan 8, 2024
@felixarntz felixarntz deleted the load/migration-script-in-setting-page branch January 8, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swissspidy swissspidy swissspidy approved these changes

@felixarntz felixarntz felixarntz approved these changes

@joemcgill joemcgill Awaiting requested review from joemcgill

@JustinyAhin JustinyAhin Awaiting requested review from JustinyAhin

Assignees

@mukeshpanchal27 mukeshpanchal27

Labels
Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mukeshpanchal27 @felixarntz @swissspidy