Repository navigation
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 1 file.
1 file is newly checked for coverage.
|
kraftbj
left a comment
There was a problem hiding this comment.
Three small things inline.
Separately, nothing in the package checks that init() wires up the activation and unlink/disconnect hooks. The plugin's REST tests cover that through the shim today, but once Protect calls Monitor::init() directly (PR 53685), nothing will. Could the package get a test for that, plus one for activation still succeeding when WordPress.com rejects the subscription?
|
Thanks! Added both in ccb2805:
I mutation-checked both: dropping a hook from |
ccb2805 to
f9a8e5c
Compare
Move the monitor module's logic (per-user email alerts kept in step with WordPress.com, and the cleanup on unlink, user deletion and disconnect) into a new automattic/jetpack-monitor package, so plugins other than Jetpack can offer Downtime Monitor. Jetpack_Monitor stays as a shim that forwards to it.
… leftovers Test that init() wires its hooks and that activation subscribes the user without failing when WordPress.com rejects it. Drop a standalone-module assertion Modules::get_available() already guarantees, the empty README headings, and the placeholder build scripts.
f9a8e5c to
1cac1f5
Compare
# Conflicts: # projects/plugins/jetpack/composer.lock
kraftbj
left a comment
There was a problem hiding this comment.
Approving. One non-blocking note inline.
| // A failure doesn't block activation: with no local copy saved, the next settings read asks WordPress.com. | ||
| self::set_notifications( true ); |
There was a problem hiding this comment.
Not blocking. This holds only when the user has no saved copy yet. Deactivating doesn't delete it, so if Monitor is turned off and back on while WordPress.com rejects the call, the user keeps whatever they had before, with no error. (Trunk leaves the same stale value, but at least the wp_die() surfaced it.) Deleting the copy on failure would make the comment true:
if ( is_wp_error( self::set_notifications( true ) ) ) {
self::delete_notifications_copy( get_current_user_id() );
}
Fixes #
Proposed changes
Split out of #53685, the exploration that runs the shared Protect dashboard without the Jetpack plugin. This PR only adds the package and moves the Jetpack plugin onto it. Behaviour is unchanged, except for the two points under "Behaviour changes" below.
automattic/jetpack-monitorpackage withAutomattic\Jetpack\Monitor\Monitor. It holds whatmodules/monitor.phpdid:jetpack.monitor.setNotifications/isUserInNotifications), with a local copy inmonitor_receive_notifications<user_id>;Monitor::init()also addsmonitortojetpack_get_available_standalone_modules, so a plugin other than Jetpack (such as Protect, in Protect: Run the shared Protect dashboard without the Jetpack plugin #53685) can switch the module on.Jetpack_Monitorin the Jetpack plugin becomes a shim: it callsMonitor::init()and every method forwards to the package, so existing callers (the REST settings endpoint, Abilities) keep working.set_notifications()returnstrue|WP_Errorinstead of dying. The shim stillwp_die()s where the module used to.Behaviour changes
wp_die()s when WordPress.com rejects the subscription. The module turns on, no local copy is saved, and the next settings read asks WordPress.com.Jetpack_Monitoris constructed. The REST endpoints construct it again, and on trunk each construction added a second set of callbacks.Follow-up
Monitor_Abilitiesstill makes its ownsetNotifications,isUserInNotificationsandgetLastDowntimecalls. Moving it onto the package is left for a separate PR.The mirror repo Automattic/jetpack-monitor is set up, with
trunkholding onlycomposer.json, theAPI_TOKEN_GITHUBsecret for autotagger, and the Packagist package.Related product discussion/links
Does this pull request change what data or activity we track or use?
No. The same XML-RPC calls and options are used as before.
Testing instructions
On a site running this branch of the Jetpack plugin, connected, with your user linked to WordPress.com:
wp option get monitor_receive_notifications<your user id>returns1.0/1).monitor_receive_notifications<your user id>is deleted.Monitor.