Skip to content
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.

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

Sync: add a new method, do_only_first_initial_sync #21676

Merged
merged 12 commits into from
Nov 12, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Actions: prevent an initial sync when not registering and an intial sync has already been started
35 changes: 31 additions & 4 deletions projects/packages/sync/src/class-actions.php
Original file line number Diff line number Diff line change
Expand Up @@ -485,14 +485,12 @@ public static function send_data( $data, $codec_name, $sent_timestamp, $queue_id
* @return bool|null False if sync is not allowed.
*/
public static function do_initial_sync() {
// Lets not sync if we are not suppose to.
// Let's not sync if we are not supposed to.
if ( ! self::sync_allowed() ) {
return false;
}

// Don't start new sync if a full sync is in process.
$full_sync_module = Modules::get_module( 'full-sync' );
if ( $full_sync_module && $full_sync_module->is_started() && ! $full_sync_module->is_finished() ) {
if ( ! self::should_do_initial_sync() ) {
return false;
}

Expand All @@ -507,6 +505,35 @@ public static function do_initial_sync() {
self::do_full_sync( $initial_sync_config );
}

/**
* Whether the site should do an initial sync.
*
* An initial Sync should not be done under these circumstances:
* - A full sync is currently in progress.
* - The site is not registering and an initial sync has already been started.
*
* @return bool Whether to run an initial sync.
*/
public static function should_do_initial_sync() {
$full_sync_module = Modules::get_module( 'full-sync' );

if ( ! $full_sync_module ) {
return false;
}

// Don't start a new sync if a full sync is in progress.
if ( $full_sync_module->is_started() && ! $full_sync_module->is_finished() ) {
return false;
}

// Don't start a new sync if the site isn't registering and an initial sync was already started.
if ( ! doing_action( 'jetpack_site_registered' ) && $full_sync_module->is_started() ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@kbrown9 can you outline why this check on 'jetpack_site_registered' is added? Logically it does make sense but it is a departure from existing functionality where the action 'jetpack_user_authorized' can also trigger do_initial_sync.

Also won't this check on is_started cause issues with sites that delete Jetpack than re-install?

Would it make sense to add flag of $strict or similar that would preserve existing flow and then add this if statement only if strict is true? This way WC Pay can call do_initial_sync( true ) and get the flow they want but not change existing flows?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like your proposal to add a flag. I think an even better approach would be to introduce a new method, Actions::do_only_first_initial_sync, which consumer plugins could call when they want an initial sync run only when one has never been run before.

The Actions::do_intial_sync method is hooked to a few actions, and the jetpack_site_registered action passes a few arguments to the callback functions. We specify that Actions::do_initial_sync accepts 0 arguments from the action hook here, but if that ever needed to change, I think we would run into problems.

I think introducing the new Actions::do_only_first_initial_sync method will be both clearer and safer than adding a flag to the existing Actions::do_intial_sync method. What do you think of this approach?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree a new method is cleaner and safer for usage. 👍 from me on this approach

return false;
}

return true;
}

/**
* Kicks off a full sync.
*
Expand Down
2 changes: 1 addition & 1 deletion projects/packages/sync/src/class-package-version.php
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
*/
class Package_Version {

const PACKAGE_VERSION = '1.27.2';
const PACKAGE_VERSION = '1.27.3-alpha';

const PACKAGE_SLUG = 'sync';

Expand Down
95 changes: 95 additions & 0 deletions projects/packages/sync/tests/php/test-actions.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
<?php // phpcs:ignore WordPress.Files.FileName.InvalidClassFileName
kbrown9 marked this conversation as resolved.
Show resolved Hide resolved

namespace Automattic\Jetpack\Sync;

use Automattic\Jetpack\Constants;
use PHPUnit\Framework\TestCase;

/**
* Unit tests for the Actions class.
*
* @package automattic/jetpack-sync
*/
class Test_Actions extends TestCase {

/**
* Tests the should_do_initial_sync method.
*
* @param bool $doing_registered_action Whether the 'jetpack_site_registered' action is currently in progress.
* @param mixed $started The 'jetpack_sync_full_status' option's 'started' field value.
* @param mixed $finished The 'jetpack_sync_full_status' option's 'finished' field value.
* @param bool $expected_result The value that Actions::should_do_initial_sync is expected to return.
*
* @dataProvider data_provider_test_should_do_initial_sync
*/
public function test_should_do_intitial_sync( $doing_registered_action, $started, $finished, $expected_result ) {
Constants::set_constant( 'JETPACK_DISABLE_RAW_OPTIONS', true );
$full_sync_option = array(
'started' => $started,
'finished' => $finished,
'progress' => array(),
'config' => array(),
);

\Jetpack_Options::update_raw_option( Modules\Full_Sync_Immediately::STATUS_OPTION, $full_sync_option );

global $wp_current_filter;
$original_filter = $wp_current_filter;
if ( $doing_registered_action ) {
$wp_current_filter = array( 'jetpack_site_registered' ); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited
}

$result = Actions::should_do_initial_sync();

delete_option( Modules\Full_Sync_Immediately::STATUS_OPTION );
$wp_current_filter = $original_filter; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited

$this->assertSame( $expected_result, $result );
}

/**
* Data provider for the test_should_do_initial_sync method.
*
* @return array The test data with the format:
* [
* 'do_registered_action" => (bool) Whether the 'jetpack_site_registered' action is currently in progress.
* 'started' => (mixed) The 'jetpack_sync_full_status' option's 'started' field value.
* 'finished' => (mixed) The 'jetpack_sync_full_status' option's 'finished' field value.
* 'expected_result' => (bool) The value that Actions::should_do_initial_sync is expected to return.
* ]
*/
public function data_provider_test_should_do_initial_sync() {
return array(
'registering, full sync started' => array(
'do_registered_action' => true,
'started' => time(),
'finished' => false,
'expected_result' => false,
),
'registering, full sync finished' => array(
'do_registered_action' => true,
'started' => time(),
'finished' => time(),
'expected_result' => true,
),
'registering, full sync not started' => array(
'do_registered_action' => true,
'started' => false,
'finished' => false,
'expected_result' => true,
),
'not registering, full sync started' => array(
'do_registered_action' => false,
'started' => time(),
'finished' => time(),
'expected_result' => false,
),
'not registering, full sync not started' => array(
'do_registered_action' => false,
'started' => false,
'finished' => false,
'expected_result' => true,
),
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: other
Comment: Fix a sync unit test


Original file line number Diff line number Diff line change
Expand Up @@ -1160,7 +1160,12 @@ function test_initial_sync_doesnt_sync_subscribers() {
$this->full_sync->start( array( 'users' => 'initial' ) );
$this->sender->do_full_sync();
$this->assertEquals( 3, $this->server_replica_storage->user_count() );
// finally, let's make sure that the initial sync method actually invokes our initial sync user config

// Finally, let's make sure that the initial sync method actually invokes our initial sync user config.
global $wp_current_filter;
$original_wp_current_filter = $wp_current_filter;
$wp_current_filter[] = 'jetpack_site_registered'; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited

Actions::do_initial_sync();
$current_user = wp_get_current_user();

Expand All @@ -1173,13 +1178,15 @@ function test_initial_sync_doesnt_sync_subscribers() {
);

$full_sync_status = $this->full_sync->get_status();

$wp_current_filter = $original_wp_current_filter; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited

$this->assertEquals(
$expected_sync_config,
$full_sync_status['config']
);
}


function test_full_sync_sends_previous_interval_end_on_posts() {
$this->factory->post->create_many( 25 );

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1508,7 +1508,12 @@ function test_initial_sync_doesnt_sync_subscribers() {
$this->full_sync->start( array( 'users' => 'initial' ) );
$this->sender->do_full_sync();
$this->assertEquals( 3, $this->server_replica_storage->user_count() );
// finally, let's make sure that the initial sync method actually invokes our initial sync user config

// Finally, let's make sure that the initial sync method actually invokes our initial sync user config.
global $wp_current_filter;
$original_wp_current_filter = $wp_current_filter;
$wp_current_filter[] = 'jetpack_site_registered'; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited

Actions::do_initial_sync( '4.2', '4.1' );
$current_user = wp_get_current_user();

Expand All @@ -1521,6 +1526,9 @@ function test_initial_sync_doesnt_sync_subscribers() {
);

$full_sync_status = $this->full_sync->get_status();

$wp_current_filter = $original_wp_current_filter; // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited

$this->assertEquals(
$expected_sync_config,
$full_sync_status[ 'config' ]
Expand Down