diff --git a/projects/packages/scheduled-updates/changelog/fix-su-multiple-sync-issue b/projects/packages/scheduled-updates/changelog/fix-su-multiple-sync-issue new file mode 100644 index 0000000000000..0bfc65f90e649 --- /dev/null +++ b/projects/packages/scheduled-updates/changelog/fix-su-multiple-sync-issue @@ -0,0 +1,4 @@ +Significance: patch +Type: fixed + +Fix multiple sync issue diff --git a/projects/packages/scheduled-updates/src/class-scheduled-updates.php b/projects/packages/scheduled-updates/src/class-scheduled-updates.php index 6bc39317f47aa..a94f6d9918673 100644 --- a/projects/packages/scheduled-updates/src/class-scheduled-updates.php +++ b/projects/packages/scheduled-updates/src/class-scheduled-updates.php @@ -29,6 +29,13 @@ class Scheduled_Updates { */ const PLUGIN_CRON_HOOK = 'jetpack_scheduled_plugins_update'; + /** + * The cron event filter for the scheduled plugins update. + * + * @var string + */ + const PLUGIN_CRON_SYNC_HOOK = 'jetpack_scheduled_plugins_update_sync'; + /** * Initialize the class. */ @@ -195,6 +202,11 @@ public static function delete_scheduled_update( $timestamp, $plugins ) { * Save the schedules for sync after cron option saving. */ public static function update_option_cron() { + // Do not update the option if the filter returns false. + if ( ! apply_filters( self::PLUGIN_CRON_SYNC_HOOK, true ) ) { + return; + } + Scheduled_Updates_Logs::add_log_fields(); Scheduled_Updates_Active::add_active_field(); Scheduled_Updates_Health_Paths::add_health_check_paths_field(); diff --git a/projects/packages/scheduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php b/projects/packages/scheduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php index 021f30c3e871d..54738462decbc 100644 --- a/projects/packages/scheduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php +++ b/projects/packages/scheduled-updates/src/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-update-schedules.php @@ -278,11 +278,17 @@ public function update_item( $request ) { return $verified_plugins; } + // Prevent the sync option to be updated during deletion. This will ensure that the sync is performed only once. + // Context: https://github.com/Automattic/jetpack/issues/27763 + add_filter( Scheduled_Updates::PLUGIN_CRON_SYNC_HOOK, '__return_false' ); $deleted = $this->delete_item( $request ); + if ( is_wp_error( $deleted ) ) { return $deleted; } + // Re-enable the sync option before creation. + remove_filter( Scheduled_Updates::PLUGIN_CRON_SYNC_HOOK, '__return_false' ); $item = $this->create_item( $request ); /** diff --git a/projects/packages/scheduled-updates/tests/php/class-wpcom-rest-api-v2-endpoint-update-schedules-test.php b/projects/packages/scheduled-updates/tests/php/class-wpcom-rest-api-v2-endpoint-update-schedules-test.php index 250e41f46c952..0584be1ea2c73 100644 --- a/projects/packages/scheduled-updates/tests/php/class-wpcom-rest-api-v2-endpoint-update-schedules-test.php +++ b/projects/packages/scheduled-updates/tests/php/class-wpcom-rest-api-v2-endpoint-update-schedules-test.php @@ -28,6 +28,13 @@ class WPCOM_REST_API_V2_Endpoint_Update_Schedules_Test extends \WorDBless\BaseTe */ public $editor_id; + /** + * Sync counter. + * + * @var int + */ + public static $sync_counter; + /** * Set up. */ @@ -52,6 +59,9 @@ public function set_up() { add_filter( 'jetpack_scheduled_update_verify_plugins', '__return_true', 11 ); Scheduled_Updates::init(); + + self::$sync_counter = 0; + add_action( 'update_option', array( __CLASS__, 'sync_callback' ) ); } /** @@ -67,6 +77,9 @@ public function tear_down() { delete_option( 'jetpack_scheduled_update_statuses' ); delete_option( Scheduled_Updates::PLUGIN_CRON_HOOK ); remove_filter( 'jetpack_scheduled_update_verify_plugins', '__return_true', 11 ); + + self::$sync_counter = 0; + remove_action( 'update_option', array( __CLASS__, 'sync_callback' ) ); } /** @@ -136,6 +149,7 @@ public function test_get_items() { ), $result->get_data() ); + $this->assertSame( 2, self::$sync_counter ); } /** @@ -196,6 +210,7 @@ public function test_create_item() { $this->assertSame( 403, $result->get_status() ); $this->assertSame( 'rest_forbidden', $result->get_data()['code'] ); + $this->assertSame( 1, self::$sync_counter ); } /** @@ -249,6 +264,7 @@ public function test_create_multiple_item() { $this->assertIsArray( $sync_option ); $this->assertIsObject( $sync_option[ $schedule_id ] ); $this->assertIsObject( $sync_option[ $schedule_id_2 ] ); + $this->assertSame( 2, self::$sync_counter ); } /** @@ -282,6 +298,7 @@ public function test_init_backward_compatibility() { Scheduled_Updates::init(); $post_sync_option = get_option( Scheduled_Updates::PLUGIN_CRON_HOOK ); $this->assertEquals( $pre_sync_option, $post_sync_option ); + $this->assertSame( 1, self::$sync_counter ); } /** @@ -311,6 +328,7 @@ public function test_creating_schedules_for_same_time() { $this->assertSame( 403, $result->get_status() ); $this->assertSame( 'rest_forbidden', $result->get_data()['code'] ); + $this->assertSame( 1, self::$sync_counter ); } /** @@ -339,6 +357,7 @@ public function test_updating_autoupdate_plugins_on_create() { rest_do_request( $request ); $this->assertEquals( $unscheduled_plugins, get_option( 'auto_update_plugins' ) ); + $this->assertSame( 1, self::$sync_counter ); } /** @@ -385,6 +404,7 @@ public function test_empty_last_run() { ), $result->get_data() ); + $this->assertSame( 1, self::$sync_counter ); } /** @@ -419,6 +439,7 @@ public function test_creating_schedule_with_more_than_ten_plugins() { $result = rest_do_request( $request ); $this->assertSame( 400, $result->get_status() ); $this->assertSame( 'rest_invalid_param', $result->get_data()['code'] ); + $this->assertSame( 0, self::$sync_counter ); } /** @@ -468,6 +489,7 @@ public function test_get_item() { ), $result->get_data() ); + $this->assertSame( 1, self::$sync_counter ); } /** @@ -483,6 +505,7 @@ public function test_get_invalid_item() { $this->assertSame( 404, $result->get_status() ); $this->assertSame( 'rest_invalid_schedule', $result->get_data()['code'] ); + $this->assertSame( 0, self::$sync_counter ); } /** @@ -530,6 +553,7 @@ public function test_update_item() { $this->assertIsArray( $sync_option ); $this->assertIsObject( $sync_option[ $schedule_id ] ); $this->assertSame( $plugins, $sync_option[ $schedule_id ]->args ); + $this->assertSame( 2, self::$sync_counter ); } /** @@ -548,6 +572,7 @@ public function test_update_item_with_status() { $schedule_id = Scheduled_Updates::generate_schedule_id( $plugins ); wp_schedule_event( strtotime( 'next Monday 8:00' ), 'weekly', Scheduled_Updates::PLUGIN_CRON_HOOK, $plugins ); + $this->assertSame( 1, self::$sync_counter ); // Log a start and success. Scheduled_Updates_Logs::log( @@ -564,6 +589,7 @@ public function test_update_item_with_status() { null, $timestamp ); + $this->assertSame( 3, self::$sync_counter ); $request = new WP_REST_Request( 'PUT', '/wpcom/v2/update-schedules/' . $schedule_id ); $request->set_body_params( @@ -597,6 +623,7 @@ public function test_update_item_with_status() { $this->assertSame( $plugins, $sync_option[ $schedule_id ]->args ); $this->assertSame( $timestamp, $sync_option[ $schedule_id ]->last_run_timestamp ); $this->assertSame( $status, $sync_option[ $schedule_id ]->last_run_status ); + $this->assertSame( 4, self::$sync_counter ); } } @@ -619,6 +646,7 @@ public function test_update_invalid_item() { $this->assertSame( 404, $result->get_status() ); $this->assertSame( 'rest_invalid_schedule', $result->get_data()['code'] ); + $this->assertSame( 0, self::$sync_counter ); } /** @@ -634,6 +662,7 @@ public function test_delete_item() { $schedule_id = Scheduled_Updates::generate_schedule_id( $plugins ); wp_schedule_event( strtotime( 'next Monday 8:00' ), 'weekly', Scheduled_Updates::PLUGIN_CRON_HOOK, $plugins ); + $this->assertSame( 1, self::$sync_counter ); // Unauthenticated request. wp_set_current_user( 0 ); @@ -649,6 +678,7 @@ public function test_delete_item() { $this->assertSame( 403, $result->get_status() ); $this->assertSame( 'rest_forbidden', $result->get_data()['code'] ); + $this->assertSame( 1, self::$sync_counter ); // Successful request. wp_set_current_user( $this->admin_id ); @@ -661,6 +691,7 @@ public function test_delete_item() { $sync_option = get_option( Scheduled_Updates::PLUGIN_CRON_HOOK ); $this->assertSame( array(), $sync_option ); + $this->assertSame( 2, self::$sync_counter ); } /** @@ -676,6 +707,7 @@ public function test_delete_invalid_item() { $this->assertSame( 404, $result->get_status() ); $this->assertSame( 'rest_invalid_schedule', $result->get_data()['code'] ); + $this->assertSame( 0, self::$sync_counter ); } /** @@ -703,12 +735,105 @@ public function test_updating_autoupdate_plugins_on_delete() { wp_schedule_event( strtotime( 'next Monday 8:00' ), 'weekly', Scheduled_Updates::PLUGIN_CRON_HOOK, $plugins_2 ); update_option( 'auto_update_plugins', $auto_update ); + $this->assertSame( 2, self::$sync_counter ); $request = new WP_REST_Request( 'DELETE', '/wpcom/v2/update-schedules/' . $schedule_id ); wp_set_current_user( $this->admin_id ); rest_do_request( $request ); $this->assertSame( $expected_result, get_option( 'auto_update_plugins' ) ); + $this->assertSame( 3, self::$sync_counter ); + } + + /** + * A CRUD cycle should sync only three times. + * + * @covers ::create_item + * @covers ::get_item + * @covers ::update_item + * @covers ::delete_item + */ + public function test_crud_should_sync_only_three_times() { + wp_set_current_user( $this->admin_id ); + $plugins = array( + 'custom-plugin/custom-plugin.php', + 'gutenberg/gutenberg.php', + ); + $request = new WP_REST_Request( 'POST', '/wpcom/v2/update-schedules' ); + $base_schedule = $this->get_schedule(); + $request->set_body_params( + array( + 'plugins' => $plugins, + 'schedule' => $base_schedule, + ) + ); + + // Create. + $schedule_id = Scheduled_Updates::generate_schedule_id( $plugins ); + $result = rest_do_request( $request ); + + $this->assertSame( 200, $result->get_status() ); + $this->assertSame( $schedule_id, $result->get_data() ); + // First sync. + $this->assertSame( 1, self::$sync_counter ); + + // Read. + $request = new WP_REST_Request( 'GET', '/wpcom/v2/update-schedules/' . $schedule_id ); + $result = rest_do_request( $request ); + $data = $result->get_data(); + + $this->assertSame( 200, $result->get_status() ); + $this->assertIsArray( $data ); + $this->assertSame( $base_schedule['timestamp'], $data['timestamp'] ); + // No sync during read. + $this->assertSame( 1, self::$sync_counter ); + + // Update. + $request = new WP_REST_Request( 'PUT', '/wpcom/v2/update-schedules/' . $schedule_id ); + $base_schedule = $this->get_schedule( 'next Monday 9:00' ); + $request->set_body_params( + array( + 'plugins' => $plugins, + 'schedule' => $base_schedule, + ) + ); + + $result = rest_do_request( $request ); + $data = $result->get_data(); + + $this->assertSame( 200, $result->get_status() ); + $this->assertSame( $schedule_id, $result->get_data() ); + + // Count here should be 2 despite the fact that the schedule is deleted and added. + $this->assertSame( 2, self::$sync_counter ); + + // Read again. + $request = new WP_REST_Request( 'GET', '/wpcom/v2/update-schedules/' . $schedule_id ); + $result = rest_do_request( $request ); + $data = $result->get_data(); + + $this->assertSame( $base_schedule['timestamp'], $data['timestamp'] ); + // No sync during read. + $this->assertSame( 2, self::$sync_counter ); + + // Delete. + $request = new WP_REST_Request( 'DELETE', '/wpcom/v2/update-schedules/' . $schedule_id ); + $result = rest_do_request( $request ); + + $this->assertTrue( $result->get_data() ); + // One sync during delete. + $this->assertSame( 3, self::$sync_counter ); + } + + /** + * A callback run when a sync is performed. + * + * @param string $option Option name. + */ + public static function sync_callback( $option ) { + if ( Scheduled_Updates::PLUGIN_CRON_HOOK === $option ) { + ++self::$sync_counter; + } } /**