diff --git a/projects/plugins/jetpack/_inc/lib/class.core-rest-api-endpoints.php b/projects/plugins/jetpack/_inc/lib/class.core-rest-api-endpoints.php index 66e72127cc9c..f520fa399068 100644 --- a/projects/plugins/jetpack/_inc/lib/class.core-rest-api-endpoints.php +++ b/projects/plugins/jetpack/_inc/lib/class.core-rest-api-endpoints.php @@ -3572,7 +3572,8 @@ public static function cast_value( $value, $definition ) { */ public static function get_remote_value( $module, $option ) { - if ( in_array( $module, array( 'post-by-email' ), true ) ) { + // These are per-user values on WordPress.com, so each user gets their own local copy. + if ( in_array( $module, array( 'monitor', 'post-by-email' ), true ) ) { $option .= get_current_user_id(); } @@ -3597,6 +3598,12 @@ public static function get_remote_value( $module, $option ) { return false; } $value = Jetpack_Monitor::user_receives_notifications( false ); + // Not saved: the user may not be connected yet, and would be stuck with this answer. + if ( is_wp_error( $value ) ) { + return false; + } + // update_option() won't create an option whose value is `false`. + $value = (int) (bool) $value; break; case 'post-by-email': @@ -3611,11 +3618,6 @@ public static function get_remote_value( $module, $option ) { break; } - // Normalize value to boolean. - if ( is_wp_error( $value ) || $value === null ) { - $value = false; - } - // Save option to use it next time. update_option( $option, $value ); diff --git a/projects/plugins/jetpack/changelog/fix-monitor-notifications-per-user b/projects/plugins/jetpack/changelog/fix-monitor-notifications-per-user new file mode 100644 index 000000000000..492e4ac81df6 --- /dev/null +++ b/projects/plugins/jetpack/changelog/fix-monitor-notifications-per-user @@ -0,0 +1,4 @@ +Significance: patch +Type: bugfix + +Monitor: Show each user their own downtime email setting instead of the one another admin saved last. diff --git a/projects/plugins/jetpack/modules/monitor.php b/projects/plugins/jetpack/modules/monitor.php index 5d2486ed95d7..943e0dbe66c0 100644 --- a/projects/plugins/jetpack/modules/monitor.php +++ b/projects/plugins/jetpack/modules/monitor.php @@ -41,6 +41,63 @@ class Jetpack_Monitor { public function __construct() { add_action( 'jetpack_modules_loaded', array( $this, 'jetpack_modules_loaded' ) ); add_action( 'jetpack_activate_module_monitor', array( $this, 'activate_module' ) ); + add_action( 'jetpack_unlinked_user', array( $this, 'delete_notifications_copy' ) ); + add_action( 'deleted_user', array( $this, 'delete_notifications_copy' ) ); + add_action( 'jetpack_site_before_disconnected', array( $this, 'delete_all_notifications_copies' ) ); + } + + /** + * Forgets every linked user's saved notification choice, while a site disconnect can still say who was linked. + * + * @since $$next-version$$ + * + * @return void + */ + public function delete_all_notifications_copies() { + $user_tokens = Jetpack_Options::get_option( 'user_tokens' ); + foreach ( array_keys( is_array( $user_tokens ) ? $user_tokens : array() ) as $user_id ) { + $this->delete_notifications_copy( $user_id ); + } + } + + /** + * Forgets a user's saved notification choice, which belongs to the WordPress.com account they had linked. + * + * @since $$next-version$$ + * + * @param int $user_id User ID. + * + * @return void + */ + public function delete_notifications_copy( $user_id ) { + delete_option( self::notifications_copy_option_name( $user_id ) ); + } + + /** + * Name of the option holding a user's local copy of their notification choice. + * + * @since $$next-version$$ + * + * @param int $user_id User ID. + * + * @return string + */ + public static function notifications_copy_option_name( $user_id ) { + return 'monitor_receive_notifications' . (int) $user_id; + } + + /** + * Saves the current user's notification choice where Jetpack_Core_Json_Api_Endpoints::get_remote_value reads it. + * + * @since $$next-version$$ + * + * @param bool $enabled Whether the current user receives notifications. + * + * @return void + */ + public static function save_notifications_copy( $enabled ) { + // An integer: see Jetpack_Core_Json_Api_Endpoints::get_remote_value. + update_option( self::notifications_copy_option_name( get_current_user_id() ), (int) (bool) $enabled ); } /** @@ -82,8 +139,7 @@ public function update_option_receive_jetpack_monitor_notification( $value ) { wp_die( sprintf( '%s: %s', esc_html( $xml->getErrorCode() ), esc_html( $xml->getErrorMessage() ) ) ); } - // To be used only in Jetpack_Core_Json_Api_Endpoints::get_remote_value. - update_option( 'monitor_receive_notifications', (bool) $value ); + self::save_notifications_copy( $value ); return true; } diff --git a/projects/plugins/jetpack/src/abilities/class-monitor-abilities.php b/projects/plugins/jetpack/src/abilities/class-monitor-abilities.php index c9f94dfc3b0f..d223eb211e17 100644 --- a/projects/plugins/jetpack/src/abilities/class-monitor-abilities.php +++ b/projects/plugins/jetpack/src/abilities/class-monitor-abilities.php @@ -13,6 +13,7 @@ use Automattic\Jetpack\WP_Abilities\Registrar; use Jetpack; use Jetpack_IXR_Client; +use Jetpack_Monitor; if ( ! defined( 'ABSPATH' ) ) { exit( 0 ); @@ -225,10 +226,10 @@ public static function get_monitor_status( $input = null ) { // phpcs:ignore Var /** * Execute: declarative state-setter. Idempotent — compares desired vs current - * and returns changed=false when they match. Either way the local - * `monitor_receive_notifications` option is synced to the remote value (after - * the write on a change, and on the no-op path) so the legacy REST reader, - * which trusts that option first, never reports a stale state. + * and returns changed=false when they match. Either way the current user's + * local copy of the value is synced to the remote one (after the write on a + * change, and on the no-op path) so the legacy REST reader, which trusts that + * copy first, never reports a stale state. * * @param array|null $input Input matching the ability's input_schema. * @return array|\WP_Error @@ -270,14 +271,8 @@ public static function set_notifications( $input = null ) { } if ( $desired === $current ) { - // Sync the local `monitor_receive_notifications` option to the - // known-good remote value even on a no-op. The legacy - // `Jetpack_Core_Json_Api_Endpoints::get_remote_value` reader trusts - // this option before falling back to a remote read, so a stale local - // value would let it report the wrong state. The changed=true path - // below mirrors the option after a write; mirroring here keeps the - // unchanged path self-healing too. - update_option( 'monitor_receive_notifications', $current ); + // Sync even on a no-op, so a stale local copy heals itself. + Jetpack_Monitor::save_notifications_copy( $current ); return array( 'enabled' => $current, @@ -290,10 +285,8 @@ public static function set_notifications( $input = null ) { return $applied; } - // Mirror the write to the `monitor_receive_notifications` option so the - // legacy `Jetpack_Core_Json_Api_Endpoints::get_remote_value` reader — the - // only other reader of this option — stays in sync with the remote state. - update_option( 'monitor_receive_notifications', $desired ); + // Keep the legacy REST reader's local copy in sync with the write. + Jetpack_Monitor::save_notifications_copy( $desired ); return array( 'enabled' => $desired, diff --git a/projects/plugins/jetpack/tests/php/_inc/lib/Jetpack_REST_API_endpoints_Test.php b/projects/plugins/jetpack/tests/php/_inc/lib/Jetpack_REST_API_endpoints_Test.php index 3adc897feee3..d7059bd2a033 100644 --- a/projects/plugins/jetpack/tests/php/_inc/lib/Jetpack_REST_API_endpoints_Test.php +++ b/projects/plugins/jetpack/tests/php/_inc/lib/Jetpack_REST_API_endpoints_Test.php @@ -6,6 +6,7 @@ * @package automattic/jetpack */ +use Automattic\Jetpack\Connection\Manager as Connection_Manager; use Automattic\Jetpack\Connection\REST_Connector; use Automattic\Jetpack\Status\Cache as StatusCache; use PHPUnit\Framework\Attributes\DataProvider; @@ -1052,6 +1053,114 @@ public function test_settings_retrieve() { $this->assertTrue( $response_data['carousel'] ); } + /** + * Each admin reads their own Monitor email choice, not the one another admin saved. + */ + public function test_settings_retrieve_monitor_notifications_per_user() { + $subscribed = $this->create_and_get_user( 'administrator' ); + $unsubscribed = $this->create_and_get_user( 'administrator' ); + update_option( 'monitor_receive_notifications' . $subscribed->ID, 1 ); + update_option( 'monitor_receive_notifications' . $unsubscribed->ID, 0 ); + + wp_set_current_user( $subscribed->ID ); + $this->assertTrue( $this->create_and_get_request( 'settings' )->get_data()['monitor_receive_notifications'] ); + + wp_set_current_user( $unsubscribed->ID ); + $this->assertFalse( $this->create_and_get_request( 'settings' )->get_data()['monitor_receive_notifications'] ); + } + + /** + * A first read saves what WordPress.com answered, and saves nothing when the lookup fails. + * + * @dataProvider provider_monitor_notification_lookups + * + * @param string|null $answer XML-RPC boolean WordPress.com answers with, or null for a failed request. + * @param string $expected The saved copy afterwards. + */ + #[DataProvider( 'provider_monitor_notification_lookups' )] + public function test_settings_retrieve_monitor_notifications_remote_lookup( $answer, $expected ) { + $user = $this->create_and_get_user( 'administrator' ); + wp_set_current_user( $user->ID ); + Jetpack_Options::update_option( 'id', 1234 ); + Jetpack_Options::update_option( 'blog_token', 'asd.qwe.1' ); + Jetpack_Options::update_option( 'user_tokens', array( $user->ID => "honey.badger.$user->ID" ) ); + + add_filter( + 'pre_http_request', + static function () use ( $answer ) { + if ( null === $answer ) { + return new WP_Error( 'http_request_failed', 'Could not connect.' ); + } + return array( + 'body' => "$answer", + 'response' => array( 'code' => 200 ), + ); + } + ); + + $this->assertSame( '1' === $answer, $this->create_and_get_request( 'settings' )->get_data()['monitor_receive_notifications'] ); + $this->assertSame( $expected, (string) get_option( 'monitor_receive_notifications' . $user->ID, 'not saved' ) ); + } + + public static function provider_monitor_notification_lookups() { + return array( + 'subscribed' => array( '1', '1' ), + 'unsubscribed' => array( '0', '0' ), + 'failed lookup' => array( null, 'not saved' ), + ); + } + + /** + * Unlinking or deleting a user forgets their saved Monitor email choice. + * + * @dataProvider provider_monitor_forgetting_hooks + * + * @param string $hook Action that fires with the user ID. + */ + #[DataProvider( 'provider_monitor_forgetting_hooks' )] + public function test_monitor_notifications_copy_is_forgotten( $hook ) { + require_once JETPACK__PLUGIN_DIR . 'modules/monitor.php'; + new Jetpack_Monitor(); + $user = $this->create_and_get_user( 'administrator' ); + update_option( 'monitor_receive_notifications' . $user->ID, 1 ); + + // Core's `deleted_user` passes three arguments, and its own multisite callback requires them. + do_action( $hook, $user->ID, null, $user ); + + $this->assertFalse( get_option( 'monitor_receive_notifications' . $user->ID ) ); + } + + public static function provider_monitor_forgetting_hooks() { + return array( + 'unlinked' => array( 'jetpack_unlinked_user' ), + 'deleted' => array( 'deleted_user' ), + ); + } + + /** + * Disconnecting the site forgets the saved Monitor email choice of every linked user. + */ + public function test_monitor_notifications_copies_are_forgotten_on_site_disconnect() { + require_once JETPACK__PLUGIN_DIR . 'modules/monitor.php'; + new Jetpack_Monitor(); + $first = $this->create_and_get_user( 'administrator' ); + $second = $this->create_and_get_user( 'administrator' ); + Jetpack_Options::update_option( + 'user_tokens', + array( + $first->ID => "honey.badger.$first->ID", + $second->ID => "honey.badger.$second->ID", + ) + ); + update_option( 'monitor_receive_notifications' . $first->ID, 1 ); + update_option( 'monitor_receive_notifications' . $second->ID, 0 ); + + ( new Connection_Manager( 'jetpack' ) )->disconnect_site( false ); + + $this->assertFalse( get_option( 'monitor_receive_notifications' . $first->ID ) ); + $this->assertFalse( get_option( 'monitor_receive_notifications' . $second->ID ) ); + } + /** * Test fetching milestone widget data. * diff --git a/projects/plugins/jetpack/tests/php/src/Monitor_Abilities_Test.php b/projects/plugins/jetpack/tests/php/src/Monitor_Abilities_Test.php index 0275bd40e5a5..676426ba2bc2 100644 --- a/projects/plugins/jetpack/tests/php/src/Monitor_Abilities_Test.php +++ b/projects/plugins/jetpack/tests/php/src/Monitor_Abilities_Test.php @@ -36,6 +36,9 @@ class Monitor_Abilities_Test extends WP_UnitTestCase { public function set_up() { parent::set_up(); + // The abilities save through Jetpack_Monitor, which only loads with the module. + require_once JETPACK__PLUGIN_DIR . 'modules/monitor.php'; + $this->admin_id = wp_insert_user( array( 'user_login' => 'monitor_abilities_admin_' . wp_generate_password( 8, false, false ), @@ -79,7 +82,7 @@ public function tear_down() { // registrar never registers and must never tear down. delete_transient( 'monitor_last_downtime' ); - delete_option( 'monitor_receive_notifications' ); + delete_option( 'monitor_receive_notifications' . $this->admin_id ); parent::tear_down(); } @@ -460,8 +463,13 @@ static function ( $mods ) { * `Monitor_Abilities_Test_Stub` test double (defined below this class) overrides * the protected seams so we can drive `set_notifications()` through its * end-to-end logic without standing up a Jetpack token fixture. + * + * @dataProvider provider_flipped_states + * + * @param bool $desired The state to flip to. */ - public function test_set_notifications_returns_changed_true_when_state_flips() { + #[DataProvider( 'provider_flipped_states' )] + public function test_set_notifications_returns_changed_true_when_state_flips( bool $desired ) { wp_set_current_user( $this->admin_id ); add_filter( @@ -473,18 +481,24 @@ static function ( $mods ) { } ); - Monitor_Abilities_Test_Stub::reset( false ); + Monitor_Abilities_Test_Stub::reset( ! $desired ); - $result = Monitor_Abilities_Test_Stub::set_notifications( array( 'enabled' => true ) ); + $result = Monitor_Abilities_Test_Stub::set_notifications( array( 'enabled' => $desired ) ); $this->assertIsArray( $result ); - $this->assertTrue( $result['enabled'] ); + $this->assertSame( $desired, $result['enabled'] ); $this->assertTrue( $result['changed'] ); $this->assertSame( 1, Monitor_Abilities_Test_Stub::$apply_calls, 'IXR setNotifications should have been called exactly once.' ); - $this->assertTrue( Monitor_Abilities_Test_Stub::$last_applied, 'apply_notifications_update should have been called with the desired value.' ); - // `update_option` stores booleans as '1'/'' through the DB roundtrip; assert truthiness - // rather than the exact string so this test is robust to env differences. - $this->assertTrue( (bool) get_option( 'monitor_receive_notifications' ), 'monitor_receive_notifications option should mirror the new state.' ); + $this->assertSame( $desired, Monitor_Abilities_Test_Stub::$last_applied, 'apply_notifications_update should have been called with the desired value.' ); + $this->assertSame( $desired ? '1' : '0', (string) get_option( 'monitor_receive_notifications' . $this->admin_id, 'not stored' ), 'The user\'s monitor_receive_notifications option should mirror the new state.' ); + $this->assertFalse( get_option( 'monitor_receive_notifications' ), 'The value must not be stored where other users would read it.' ); + } + + public static function provider_flipped_states(): array { + return array( + 'off to on' => array( true ), + 'on to off' => array( false ), + ); } public function test_set_notifications_returns_changed_false_when_state_matches() { @@ -503,7 +517,7 @@ static function ( $mods ) { // Seed a stale local option (false) that disagrees with the remote state // (true) to exercise the self-healing sync on the no-op path. - update_option( 'monitor_receive_notifications', false ); + update_option( 'monitor_receive_notifications' . $this->admin_id, 0 ); $result = Monitor_Abilities_Test_Stub::set_notifications( array( 'enabled' => true ) ); @@ -514,7 +528,7 @@ static function ( $mods ) { // Even on a no-op the local option is synced to the known-good remote // value (true here) so the legacy REST reader, which trusts this option // before going remote, can't surface a stale state. - $this->assertTrue( (bool) get_option( 'monitor_receive_notifications' ), 'No-op should still sync the mirrored option to the remote value.' ); + $this->assertSame( '1', (string) get_option( 'monitor_receive_notifications' . $this->admin_id ), 'No-op should still sync the mirrored option to the remote value.' ); } /**