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.

Sign up for GitHub

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

Full Sync :: limit to contributors and above #21648

Merged
merged 6 commits into from
Nov 5, 2021
Merged

Full Sync :: limit to contributors and above #21648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
16079f8
Full Sync :: limit to contributors and above
mdbitz Nov 4, 2021
14045ff
Unit Tests update
mdbitz Nov 4, 2021
929482f
Another test that needed role specified.
mdbitz Nov 4, 2021
a5b2225
multisite test update
mdbitz Nov 4, 2021
9557492
Legacy Full Sync Unit Tests
mdbitz Nov 4, 2021
95a2900
duplicate tests, should purge legacy
mdbitz Nov 4, 2021
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
4 changes: 4 additions & 0 deletions projects/packages/sync/changelog/fix-full-sync-user_level
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: changed

Full Sync : limit included users to contributors and above (based on wp_user_limit)
2 changes: 1 addition & 1 deletion projects/packages/sync/src/modules/class-users.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ public function estimate_full_sync_actions( $config ) {
public function get_where_sql( $config ) {
global $wpdb;

$query = "meta_key = '{$wpdb->prefix}capabilities'";
$query = "meta_key = '{$wpdb->prefix}user_level' AND meta_value > 0";

// The $config variable is a list of user IDs to sync.
if ( is_array( $config ) ) {
Expand Down
4 changes: 4 additions & 0 deletions projects/plugins/jetpack/changelog/fix-full-sync-user_level
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: compat

Unit Tests : Update Full Sync tests to align with limitation on users that are synced.
31 changes: 18 additions & 13 deletions projects/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-full-immediately.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,10 +312,15 @@ function test_full_sync_sends_all_term_relationships_with_previous_interval_end(
) );
}

/**
* Full Sync is limited to contributor and above users based on wp_user_level.
* This test verifies only contributors are sent.
*/
function test_full_sync_sends_all_users() {
$first_user_id = $this->factory->user->create();
$this->factory->user->create( array( 'role' => 'subscriber' ) );
$first_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
for ( $i = 0; $i < 9; $i += 1 ) {
$user_id = $this->factory->user->create();
$user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
}

update_user_meta( $user_id, 'locale', 'en_GB' );
Expand Down Expand Up @@ -363,7 +368,7 @@ function test_full_sync_sends_only_current_blog_users_in_multisite() {

$original_blog_id = get_current_blog_id();

$user_id = $this->factory->user->create();
$user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );

// NOTE this is necessary because WPMU causes certain assumptions about transients
// to be wrong, and tests to explode. @see: https://github.com/sheabunge/WordPress/commit/ff4f1bb17095c6af8a0f35ac304f79074f3c3ff6
Expand All @@ -375,8 +380,8 @@ function test_full_sync_sends_only_current_blog_users_in_multisite() {

// let's create some users on the other blog
switch_to_blog( $other_blog_id );
$mu_blog_user_id = $this->factory->user->create();
$added_mu_blog_user_id = $this->factory->user->create();
$mu_blog_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
$added_mu_blog_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
restore_current_blog();

// add one of the users to our current blog
Expand Down Expand Up @@ -1026,9 +1031,9 @@ function test_full_sync_can_sync_individual_comments() {
}

function test_full_sync_can_sync_individual_users() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a test here to confirm that a less-than-contributor is not included?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test_full_sync_sends_all_users creates a subscriber and verifies via the count that it doesn't get included.

I'm of the opinion that new tests have diminishing returns due to Full Sync being replaced with checksums.

$sync_user_id = $this->factory->user->create();
$sync_user_id_2 = $this->factory->user->create();
$no_sync_user_id = $this->factory->user->create();
$sync_user_id = $this->factory->user->create( array( 'role' => 'editor' ) );
$sync_user_id_2 = $this->factory->user->create( array( 'role' => 'editor' ) );
$this->factory->user->create( array( 'role' => 'editor' ) );

$this->full_sync->start( array( 'users' => array( $sync_user_id, $sync_user_id_2 ) ) );
$this->sender->do_full_sync();
Expand All @@ -1049,8 +1054,8 @@ function test_full_sync_can_sync_individual_users() {
function test_full_sync_doesnt_send_deleted_posts() {
// previously, the behavior was to send false or throw errors - we
// should actively detect false values and remove them
$keep_post_id = $this->factory->post->create();
$delete_post_id = $this->factory->post->create();
$keep_post_id = $this->factory->post->create( array( 'role' => 'editor' ) );
$delete_post_id = $this->factory->post->create( array( 'role' => 'editor' ) );

$this->full_sync->start();

Expand Down Expand Up @@ -1090,8 +1095,8 @@ function test_full_sync_doesnt_send_deleted_users() {

// previously, the behavior was to send false or throw errors - we
// should actively detect false values and remove them
$keep_user_id = $this->factory->user->create();
$delete_user_id = $this->factory->user->create();
$keep_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
$delete_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );

$this->full_sync->start();

Expand Down Expand Up @@ -1147,7 +1152,7 @@ function test_initial_sync_doesnt_sync_subscribers() {
}
$this->full_sync->start();
$this->sender->do_full_sync();
$this->assertEquals( 13, $this->server_replica_storage->user_count() );
$this->assertEquals( 3, $this->server_replica_storage->user_count() );
$this->server_replica_storage->reset();
$this->assertEquals( 0, $this->server_replica_storage->user_count() );
$user_ids = Modules::get_module( 'users' )->get_initial_sync_user_config();
Expand Down
27 changes: 14 additions & 13 deletions projects/plugins/jetpack/tests/php/sync/test_class.jetpack-sync-full.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,9 +445,10 @@ function test_full_sync_sends_all_term_relationships_with_previous_interval_end(
}

function test_full_sync_sends_all_users() {
$first_user_id = $this->factory->user->create();
$this->factory->user->create( array( 'role' => 'subscriber' ) );
$first_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
for ( $i = 0; $i < 9; $i += 1 ) {
$user_id = $this->factory->user->create();
$user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
}

update_user_meta( $user_id, 'locale', 'en_GB' );
Expand All @@ -474,7 +475,7 @@ function test_full_sync_sends_previous_interval_end_for_users() {
Settings::update_settings( array( 'max_queue_size_full_sync' => 1, 'max_enqueue_full_sync' => 10 ) );

for ( $i = 0; $i < 45; $i += 1 ) {
$user_ids[] = $this->factory->user->create();
$this->factory->user->create( array( 'role' => 'contributor' ) );
}

// The first event is for full sync start.
Expand Down Expand Up @@ -529,7 +530,7 @@ function test_full_sync_sends_only_current_blog_users_in_multisite() {

$original_blog_id = get_current_blog_id();

$user_id = $this->factory->user->create();
$user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );

// NOTE this is necessary because WPMU causes certain assumptions about transients
// to be wrong, and tests to explode. @see: https://github.com/sheabunge/WordPress/commit/ff4f1bb17095c6af8a0f35ac304f79074f3c3ff6
Expand All @@ -541,8 +542,8 @@ function test_full_sync_sends_only_current_blog_users_in_multisite() {

// let's create some users on the other blog
switch_to_blog( $other_blog_id );
$mu_blog_user_id = $this->factory->user->create();
$added_mu_blog_user_id = $this->factory->user->create();
$mu_blog_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
$added_mu_blog_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
restore_current_blog();

// add one of the users to our current blog
Expand Down Expand Up @@ -1276,9 +1277,9 @@ function test_full_sync_can_sync_individual_comments() {
}

function test_full_sync_can_sync_individual_users() {
$sync_user_id = $this->factory->user->create();
$sync_user_id_2 = $this->factory->user->create();
$no_sync_user_id = $this->factory->user->create();
$sync_user_id = $this->factory->user->create( array( 'role' => 'editor' ) );
$sync_user_id_2 = $this->factory->user->create( array( 'role' => 'editor' ) );
$this->factory->user->create( array( 'role' => 'editor' ) );

$this->full_sync->start( array( 'users' => array( $sync_user_id, $sync_user_id_2) ) );
$this->sender->do_full_sync();
Expand Down Expand Up @@ -1338,13 +1339,13 @@ function test_full_sync_doesnt_send_deleted_comments() {

function test_full_sync_doesnt_send_deleted_users() {

$user_counts = count_users();
$user_counts = count_users();
$existing_user_count = $user_counts['total_users'];

// previously, the behavior was to send false or throw errors - we
// should actively detect false values and remove them
$keep_user_id = $this->factory->user->create();
$delete_user_id = $this->factory->user->create();
$keep_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );
$delete_user_id = $this->factory->user->create( array( 'role' => 'contributor' ) );

$this->full_sync->start();

Expand Down Expand Up @@ -1499,7 +1500,7 @@ function test_initial_sync_doesnt_sync_subscribers() {
}
$this->full_sync->start();
$this->sender->do_full_sync();
$this->assertEquals( 13, $this->server_replica_storage->user_count() );
$this->assertEquals( 3, $this->server_replica_storage->user_count() );
$this->server_replica_storage->reset();
$this->assertEquals( 0, $this->server_replica_storage->user_count() );
$user_ids = Modules::get_module( 'users' )->get_initial_sync_user_config();
Expand Down