diff --git a/src/API/Site/Controllers/MerchantCenter/SyncableProductsCountController.php b/src/API/Site/Controllers/MerchantCenter/SyncableProductsCountController.php index 3c8b48ee7f..753adc8497 100644 --- a/src/API/Site/Controllers/MerchantCenter/SyncableProductsCountController.php +++ b/src/API/Site/Controllers/MerchantCenter/SyncableProductsCountController.php @@ -88,6 +88,7 @@ protected function get_syncable_products_count_callback(): callable { protected function update_syncable_products_count_callback(): callable { return function( Request $request ) { $this->options->delete( OptionsInterface::SYNCABLE_PRODUCTS_COUNT ); + $this->options->delete( OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA ); $job = $this->job_repository->get( UpdateSyncableProductsCount::class ); $job->schedule(); diff --git a/src/Internal/DependencyManagement/JobServiceProvider.php b/src/Internal/DependencyManagement/JobServiceProvider.php index 523739c846..0992d00891 100644 --- a/src/Internal/DependencyManagement/JobServiceProvider.php +++ b/src/Internal/DependencyManagement/JobServiceProvider.php @@ -143,7 +143,7 @@ public function register(): void { $this->share_product_syncer_job( CleanupProductTargetCountriesJob::class ); // Share update syncable products count job - $this->share_action_scheduler_job( UpdateSyncableProductsCount::class, ProductRepository::class ); + $this->share_action_scheduler_job( UpdateSyncableProductsCount::class, ProductRepository::class, ProductHelper::class ); } /** diff --git a/src/Jobs/UpdateSyncableProductsCount.php b/src/Jobs/UpdateSyncableProductsCount.php index 40bb7cff9a..17d33018eb 100644 --- a/src/Jobs/UpdateSyncableProductsCount.php +++ b/src/Jobs/UpdateSyncableProductsCount.php @@ -10,6 +10,7 @@ use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsAwareInterface; use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsAwareTrait; use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsInterface; +use Automattic\WooCommerce\GoogleListingsAndAds\Product\ProductHelper; use Automattic\WooCommerce\GoogleListingsAndAds\Product\ProductRepository; defined( 'ABSPATH' ) || exit; @@ -31,16 +32,23 @@ class UpdateSyncableProductsCount extends AbstractBatchedActionSchedulerJob impl */ protected $product_repository; + /** + * @var ProductHelper + */ + protected $product_helper; + /** * UpdateSyncableProductsCount constructor. * * @param ActionSchedulerInterface $action_scheduler * @param ActionSchedulerJobMonitor $monitor * @param ProductRepository $product_repository + * @param ProductHelper $product_helper */ - public function __construct( ActionSchedulerInterface $action_scheduler, ActionSchedulerJobMonitor $monitor, ProductRepository $product_repository ) { + public function __construct( ActionSchedulerInterface $action_scheduler, ActionSchedulerJobMonitor $monitor, ProductRepository $product_repository, ProductHelper $product_helper ) { parent::__construct( $action_scheduler, $monitor ); $this->product_repository = $product_repository; + $this->product_helper = $product_helper; } /** @@ -78,7 +86,9 @@ protected function process_items( array $items ) { $product_ids = []; } - $this->options->update( OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA, [ ...$product_ids, ...$items ] ); + $grouped_items = $this->product_helper->maybe_swap_for_parent_ids( $items ); + + $this->options->update( OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA, array_unique( [ ...$product_ids, ...$grouped_items ] ) ); } /** diff --git a/src/Product/ProductHelper.php b/src/Product/ProductHelper.php index ee8045252e..1eb61809de 100644 --- a/src/Product/ProductHelper.php +++ b/src/Product/ProductHelper.php @@ -465,12 +465,46 @@ public function get_mc_status( WC_Product $wc_product ): ?string { } } + /** + * If an item from the provided list of products has a parent, replace it with the parent ID. + * + * @param int[] $product_ids A list of WooCommerce product ID. + * @param bool $check_product_status (Optional) Check if the product status is publish. + * @param bool $ignore_product_on_error (Optional) Ignore the product when invalid value error occurs. + * + * @return int[] A list of parent ID or product ID if it doesn't have a parent. + * + * @throws InvalidValue If the given param ignore_product_on_error is false and any of a given ID doesn't reference a valid product. + * Or if a variation product does not have a valid parent ID (i.e. it's an orphan). + * + * @since x.x.x + */ + public function maybe_swap_for_parent_ids( array $product_ids, bool $check_product_status = true, bool $ignore_product_on_error = true ) { + $new_product_ids = []; + + foreach ( $product_ids as $index => $product_id ) { + try { + $product = $this->get_wc_product( $product_id ); + $new_product = $this->maybe_swap_for_parent( $product ); + if ( ! $check_product_status || 'publish' === $new_product->get_status() ) { + $new_product_ids[ $index ] = $new_product->get_id(); + } + } catch ( InvalidValue $exception ) { + if ( ! $ignore_product_on_error ) { + throw $exception; + } + } + } + + return array_unique( $new_product_ids ); + } + /** * If the provided product has a parent, return its ID. Otherwise, return the given (valid product) ID. * * @param int $product_id WooCommerce product ID. * - * @return int The parent ID or product ID of it doesn't have a parent. + * @return int The parent ID or product ID if it doesn't have a parent. * * @throws InvalidValue If a given ID doesn't reference a valid product. Or if a variation product does not have a * valid parent ID (i.e. it's an orphan). diff --git a/tests/Tools/HelperTrait/ProductTrait.php b/tests/Tools/HelperTrait/ProductTrait.php index b7cef6a679..ebec30a8da 100644 --- a/tests/Tools/HelperTrait/ProductTrait.php +++ b/tests/Tools/HelperTrait/ProductTrait.php @@ -90,14 +90,16 @@ public function generate_google_product_mock( $id = null, $target_country = null /** * Generates and returns a mock of a simple WC_Product object * + * @param int|null $product_id + * * @return MockObject|WC_Product */ - public function generate_simple_product_mock() { + public function generate_simple_product_mock( $product_id = null ) { $product = $this->createMock( WC_Product::class ); $product->expects( $this->any() ) ->method( 'get_id' ) - ->willReturn( rand() ); + ->willReturn( $product_id ?: rand() ); $product->expects( $this->any() ) ->method( 'get_type' ) @@ -320,6 +322,10 @@ public function get_sample_attributes(): array { * @return WC_Product[] */ protected function generate_simple_product_mocks_set( int $number ) { - return array_fill( 0, $number, $this->generate_simple_product_mock() ); + $products = []; + for ( $i = 0; $i < $number; ++$i ) { + $products[] = $this->generate_simple_product_mock(); + } + return $products; } } diff --git a/tests/Unit/Jobs/UpdateSyncableProductsCountTest.php b/tests/Unit/Jobs/UpdateSyncableProductsCountTest.php index 5e1dfe1b5b..a0c8456d0c 100644 --- a/tests/Unit/Jobs/UpdateSyncableProductsCountTest.php +++ b/tests/Unit/Jobs/UpdateSyncableProductsCountTest.php @@ -9,10 +9,12 @@ use Automattic\WooCommerce\GoogleListingsAndAds\Jobs\UpdateSyncableProductsCount; use Automattic\WooCommerce\GoogleListingsAndAds\Options\OptionsInterface; use Automattic\WooCommerce\GoogleListingsAndAds\Product\FilteredProductList; +use Automattic\WooCommerce\GoogleListingsAndAds\Product\ProductHelper; use Automattic\WooCommerce\GoogleListingsAndAds\Product\ProductRepository; use Automattic\WooCommerce\GoogleListingsAndAds\Tests\Framework\UnitTest; use Automattic\WooCommerce\GoogleListingsAndAds\Tests\Tools\HelperTrait\ProductTrait; use PHPUnit\Framework\MockObject\MockObject; +use WC_Helper_Product; /** * Class UpdateSyncableProductsCountTest @@ -21,6 +23,7 @@ * * @property MockObject|ActionScheduler $action_scheduler * @property MockObject|ActionSchedulerJobMonitor $monitor + * @property MockObject|ProductHelper $product_helper * @property MockObject|ProductRepository $product_repository * @property MockObject|OptionsInterface $options * @property UpdateSyncableProductsCount $job @@ -42,12 +45,14 @@ public function setUp(): void { $this->action_scheduler = $this->createMock( ActionSchedulerInterface::class ); $this->monitor = $this->createMock( ActionSchedulerJobMonitor::class ); + $this->product_helper = $this->createMock( ProductHelper::class ); $this->product_repository = $this->createMock( ProductRepository::class ); $this->options = $this->createMock( OptionsInterface::class ); $this->job = new UpdateSyncableProductsCount( $this->action_scheduler, $this->monitor, - $this->product_repository + $this->product_repository, + $this->product_helper ); $this->job->set_options_object( $this->options ); @@ -94,6 +99,11 @@ public function test_update_syncable_products_count() { ->withConsecutive( [ [], self::BATCH_SIZE, 0 ], [ [], self::BATCH_SIZE, 2 ], [ [], self::BATCH_SIZE, 4 ] ) ->willReturnOnConsecutiveCalls( $batch_a, $batch_b, $batch_c ); + $this->product_helper->expects( $this->exactly( 2 ) ) + ->method( 'maybe_swap_for_parent_ids' ) + ->withConsecutive( [ $batch_a->get_product_ids() ], [ $batch_b->get_product_ids() ] ) + ->willReturnOnConsecutiveCalls( $batch_a->get_product_ids(), $batch_b->get_product_ids() ); + $this->options->expects( $this->exactly( 3 ) ) ->method( 'get' ) ->with( OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA ) @@ -120,6 +130,74 @@ public function test_update_syncable_products_count() { do_action( self::CREATE_BATCH_HOOK, 3 ); } + public function test_update_syncable_products_count_duplicate_product_ids() { + // syncable products count: 2, total products count: 2 + $batch_a = new filteredproductlist( + [ + $this->generate_simple_product_mock( 12345 ), + $this->generate_simple_product_mock( 12345 ), + ], + 2 + ); + + // Syncable products count: 2, total products count: 2 + $batch_b = new filteredproductlist( + [ + $this->generate_simple_product_mock( 23456 ), + $this->generate_simple_product_mock( 23456 ), + ], + 2 + ); + + // Syncable products count: 0, total products count: 0 + $batch_c = new FilteredProductList( [], 0 ); + + $this->action_scheduler->expects( $this->exactly( 5 ) ) + ->method( 'schedule_immediate' ) + ->withConsecutive( + [ self::CREATE_BATCH_HOOK, [ 1 ] ], + [ self::PROCESS_ITEM_HOOK, [ $batch_a->get_product_ids() ] ], + [ self::CREATE_BATCH_HOOK, [ 2 ] ], + [ self::PROCESS_ITEM_HOOK, [ $batch_b->get_product_ids() ] ], + [ self::CREATE_BATCH_HOOK, [ 3 ] ], + ); + + $this->product_repository->expects( $this->exactly( 3 ) ) + ->method( 'find_sync_ready_products' ) + ->withConsecutive( [ [], self::BATCH_SIZE, 0 ], [ [], self::BATCH_SIZE, 2 ], [ [], self::BATCH_SIZE, 4 ] ) + ->willReturnOnConsecutiveCalls( $batch_a, $batch_b, $batch_c ); + + $this->product_helper->expects( $this->exactly( 2 ) ) + ->method( 'maybe_swap_for_parent_ids' ) + ->withConsecutive( [ $batch_a->get_product_ids() ], [ $batch_b->get_product_ids() ] ) + ->willReturnOnConsecutiveCalls( $batch_a->get_product_ids(), $batch_b->get_product_ids() ); + + $this->options->expects( $this->exactly( 3 ) ) + ->method( 'get' ) + ->with( OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA ) + ->willReturnOnConsecutiveCalls( + null, + [ 12345 ], + [ 12345, 23456 ] + ); + + $this->options->expects( $this->exactly( 3 ) ) + ->method( 'update' ) + ->withConsecutive( + [ OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA, [ 12345 ] ], + [ OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA, [ 12345, 23456 ] ], + [ OptionsInterface::SYNCABLE_PRODUCTS_COUNT, 2 ] + ); + + $this->job->schedule(); + + do_action( self::CREATE_BATCH_HOOK, 1 ); + do_action( self::PROCESS_ITEM_HOOK, $batch_a->get_product_ids() ); + do_action( self::CREATE_BATCH_HOOK, 2 ); + do_action( self::PROCESS_ITEM_HOOK, $batch_b->get_product_ids() ); + do_action( self::CREATE_BATCH_HOOK, 3 ); + } + public function test_update_syncable_products_count_no_syncable_products_single_batch() { // Syncable products count: 0, total products count: 0 $batch = new FilteredProductList( [], 0 ); @@ -189,4 +267,83 @@ public function test_update_syncable_products_count_no_syncable_products_multipl do_action( self::CREATE_BATCH_HOOK, 2 ); do_action( self::CREATE_BATCH_HOOK, 3 ); } + + public function test_update_syncable_products_count_product_gets_deleted_after_batches_get_created() { + // This is a product in the first batch that will be deleted after the first batch gets created. + $product_to_be_deleted = WC_Helper_Product::create_simple_product( true, [ 'status' => 'publish' ] ); + + // syncable products count: 2, total products count: 2 + $batch_a = new filteredproductlist( + [ + $product_to_be_deleted, + $this->generate_simple_product_mock( 23456 ), + ], + 2 + ); + + // Syncable products count: 2, total products count: 2 + $batch_b = new filteredproductlist( + [ + $this->generate_simple_product_mock( 34567 ), + $this->generate_simple_product_mock( 45678 ), + ], + 2 + ); + + // Syncable products count: 0, total products count: 0 + $batch_c = new FilteredProductList( [], 0 ); + + $this->action_scheduler->expects( $this->exactly( 5 ) ) + ->method( 'schedule_immediate' ) + ->withConsecutive( + [ self::CREATE_BATCH_HOOK, [ 1 ] ], + [ self::PROCESS_ITEM_HOOK, [ $batch_a->get_product_ids() ] ], + [ self::CREATE_BATCH_HOOK, [ 2 ] ], + [ self::PROCESS_ITEM_HOOK, [ $batch_b->get_product_ids() ] ], + [ self::CREATE_BATCH_HOOK, [ 3 ] ], + ); + + $this->product_repository->expects( $this->exactly( 3 ) ) + ->method( 'find_sync_ready_products' ) + ->withConsecutive( [ [], self::BATCH_SIZE, 0 ], [ [], self::BATCH_SIZE, 2 ], [ [], self::BATCH_SIZE, 4 ] ) + ->willReturnOnConsecutiveCalls( $batch_a, $batch_b, $batch_c ); + + $this->product_helper->expects( $this->exactly( 2 ) ) + ->method( 'maybe_swap_for_parent_ids' ) + ->withConsecutive( [ $batch_a->get_product_ids(), true ], [ $batch_b->get_product_ids(), true ] ) + ->willReturnOnConsecutiveCalls( + // The first product gets deleted after the first batch gets created, + // so only return the second product of batch a after calling maybe_swap_for_parent_ids. + [ $batch_a->get_product_ids()[1] ], + $batch_b->get_product_ids() + ); + + $this->options->expects( $this->exactly( 3 ) ) + ->method( 'get' ) + ->with( OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA ) + ->willReturnOnConsecutiveCalls( + null, + [ 23456 ], + [ 23456, 34567, 45678 ] + ); + + $this->options->expects( $this->exactly( 3 ) ) + ->method( 'update' ) + ->withConsecutive( + [ OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA, [ 23456 ] ], + [ OptionsInterface::SYNCABLE_PRODUCTS_COUNT_INTERMEDIATE_DATA, [ 23456, 34567, 45678 ] ], + [ OptionsInterface::SYNCABLE_PRODUCTS_COUNT, 3 ] + ); + + $this->job->schedule(); + + do_action( self::CREATE_BATCH_HOOK, 1 ); + wp_trash_post( $product_to_be_deleted->get_id() ); + $product_to_be_deleted->set_status( 'trash' ); + $product_to_be_deleted->save(); + do_action( self::PROCESS_ITEM_HOOK, $batch_a->get_product_ids() ); + do_action( self::CREATE_BATCH_HOOK, 2 ); + do_action( self::PROCESS_ITEM_HOOK, $batch_b->get_product_ids() ); + do_action( self::CREATE_BATCH_HOOK, 3 ); + } } diff --git a/tests/Unit/Product/ProductHelperTest.php b/tests/Unit/Product/ProductHelperTest.php index 1d56fad266..24a85a12bc 100644 --- a/tests/Unit/Product/ProductHelperTest.php +++ b/tests/Unit/Product/ProductHelperTest.php @@ -3,6 +3,7 @@ namespace Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\Product; +use Automattic\WooCommerce\GoogleListingsAndAds\Exception\InvalidValue; use Automattic\WooCommerce\GoogleListingsAndAds\Google\GoogleProductService; use Automattic\WooCommerce\GoogleListingsAndAds\MerchantCenter\TargetAudience; use Automattic\WooCommerce\GoogleListingsAndAds\Product\ProductHelper; @@ -815,6 +816,55 @@ public function test_get_mc_status_variation_product_returns_null_if_orphan() { $this->assertNull( $this->product_helper->get_mc_status( $variation ) ); } + public function test_maybe_swap_for_parent_ids() { + $simple_publish = WC_Helper_Product::create_simple_product(); + $simple_trash = WC_Helper_Product::create_simple_product( true, [ 'status' => 'trash' ] ); + $variable = WC_Helper_Product::create_variation_product(); + $variation = $this->wc->get_product( $variable->get_children()[0] ); + + $product_ids = [ + $simple_publish->get_id(), + $simple_trash->get_id(), + $variable->get_id(), + $variation->get_id(), + 999999, // not exist + ]; + + // - Check product status + // - Ignore product on error + $new_product_ids = $this->product_helper->maybe_swap_for_parent_ids( $product_ids ); + $this->assertEquals( + [ + $simple_publish->get_id(), + $variable->get_id(), + ], + array_values( $new_product_ids ), + ); + + // - Do not check product status + // - Ignore product on error + $new_product_ids = $this->product_helper->maybe_swap_for_parent_ids( $product_ids, false ); + $this->assertEquals( + [ + $simple_publish->get_id(), + $simple_trash->get_id(), + $variable->get_id(), + ], + array_values( $new_product_ids ), + ); + + // - Do not check product status + // - Do not ignore product on error + try { + $new_product_ids = $this->product_helper->maybe_swap_for_parent_ids( $product_ids, false, false ); + } catch ( InvalidValue $exception ) { + $this->assertEquals( + 'Invalid product ID: 999999', + $exception->getMessage() + ); + } + } + public function test_maybe_swap_for_parent_id() { $simple = WC_Helper_Product::create_simple_product();