Skip to content

Conversation

@leogermani
Copy link
Contributor

@leogermani leogermani commented May 19, 2022

Changes proposed in this Pull Request:

For Hybrid products use the new Modules class to activate modules and check if product is active

This allows hybrid products such as Search and Social to better integrate with My Jetpack, as they also use the Modules abstraction

Jetpack product discussion

p9dueE-55u-p2#comment-7978

Does this pull request change what data or activity we track or use?

No

Testing instructions:

  • Spin up a new JN site with Jetpack Beta
  • Activate Boost and Search on this branch
  • Deactivate and delete Jetpack
  • Connect Boost
  • Go to My Jetpack and connect your user (we need to do this while we don't fix the purchase flow)
  • Back to My Jetpack, click "Purchase" on the Search card and follow the flow. Buy Search
  • You should go back to the Search dashboard with all enabled
  • Go to My Jetpack and confirm the card shows Search as active with a "Manage" button
  • Click Manage and confirm you go to the Search plugin page
  • Deactivate Search (the toggle on the Search dashboard, not the plugin)
  • Go back to My Jetpack and confirm the product appears as not active, with an "Activate" button
  • Click Activate
  • Go to Search page and confirm Search and Instant Search are re-enabled

@leogermani leogermani added the [Status] Needs Review This PR is ready for review. label May 19, 2022
@leogermani leogermani self-assigned this May 19, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 19, 2022

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ⚠️ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖


The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.


Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

@kangzj kangzj requested review from a team and kangzj May 19, 2022 21:50
@kangzj kangzj added the [Feature] Search For all things related to Search label May 19, 2022
kangzj
kangzj previously approved these changes May 19, 2022
Copy link
Contributor

@kangzj kangzj left a comment

Choose a reason for hiding this comment

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

This works well and looks good to me, just need a version bump 🎉

@github-actions github-actions bot added the [Package] Search Contains core Search functionality for Jetpack and Search plugins label May 20, 2022
@leogermani
Copy link
Contributor Author

Also enabling Instant Search now

@kangzj
Copy link
Contributor

kangzj commented May 23, 2022

Hi @leogermani thanks for working on this, we are getting there yay 🎉

I found an issue testing. When clicking activate for search, the search plugin is activated but the UI doesn't show anything. It works on the second clicking. Steps to reproduce:

  • Prepare a site with Search plugin deactivated, Boost installed and site fully connected
  • Open My Jetpack admin page
  • Click Activate button on Search product card
  • Search is activated but the UI doesn't show success
  • Click Activate button on Search product card again
  • UI shows success

I had a look and it seems, Plugins_Installer::is_plugin_active( ) doesn't return true if called direct after activation. We possibly want to clear cache in get_plugins(). see:

return Plugins_Installer::is_plugin_active( static::get_installed_plugin_filename() );

https://github.com/WordPress/WordPress/blob/f6412575c890bf651cdaa8a2d60365c19f0d4173/wp-admin/includes/plugin.php#L276

And also I tweaked the available modules filter for search a bit, hope you don't mind.

Cheers.

@kangzj
Copy link
Contributor

kangzj commented May 23, 2022

Reverted my changes in 822bb1e which breaks tests, will do the refactoring in a another PR.

@leogermani
Copy link
Contributor Author

leogermani commented May 24, 2022

I found an issue testing. When clicking activate for search, the search plugin is activated but the UI doesn't show anything. It works on the second clicking. Steps to reproduce:

@kangzj I can't reproduce this. Do you have more details on how to reproduce it?

In anyway we can address this in a following PR if needed

retrofox
retrofox previously approved these changes May 24, 2022
Copy link
Contributor

@retrofox retrofox left a comment

Choose a reason for hiding this comment

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

I followed the testing steps described in the PR description, and it works as expected.
A pair of extra 👀 won't hurt.
Anyway, LGTM.

@leogermani
Copy link
Contributor Author

Sorry, just pushed a small fix. I was passing some useless parameters in is_active.. probably left overs of copying and pasting.

As you can see, these params don't exist: https://github.com/Automattic/jetpack/blob/update%2Fmy-jetpack-hybrid-module-activation/projects/packages/status/src/class-modules.php#L25

Copy link
Contributor

@retrofox retrofox left a comment

Choose a reason for hiding this comment

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

LGTM

@leogermani leogermani enabled auto-merge (squash) May 24, 2022 20:36
@leogermani leogermani merged commit 1490b97 into master May 24, 2022
@leogermani leogermani deleted the update/my-jetpack-hybrid-module-activation branch May 24, 2022 20:45
@github-actions github-actions bot removed the [Status] Needs Review This PR is ready for review. label May 24, 2022
@kangzj
Copy link
Contributor

kangzj commented May 24, 2022

Good job @leogermani 🎉
I cannot reproduce the issue on master, so I think all good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] Search For all things related to Search [Package] My Jetpack [Package] Search Contains core Search functionality for Jetpack and Search plugins

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants