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

Fix incorrect product statistics count #1728

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
3 changes: 2 additions & 1 deletion src/Jobs/DeleteProducts.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ public function get_name(): string {
* @throws ProductSyncerException If an error occurs. The exception will be logged by ActionScheduler.
*/
public function process_items( array $product_id_map ) {
$ready_ids = $this->product_repository->find_delete_product_ids( $product_id_map );
$product_ids = array_values( $product_id_map );
$ready_ids = $this->product_repository->find_delete_product_ids( $product_ids );

// Exclude any ID's which are not ready to delete.
$product_id_map = array_intersect( $product_id_map, $ready_ids );
Expand Down
55 changes: 49 additions & 6 deletions src/MerchantCenter/MerchantStatuses.php
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ protected function filter_valid_statuses( array $google_ids ): array {
continue;
}

$wc_product = get_post( $wc_product_id );
$wc_product = $product_helper->get_wc_product_by_wp_post( $wc_product_id );
if ( ! $wc_product || 'product' !== substr( $wc_product->post_type, 0, 7 ) ) {
// Should never reach here since the products IDS are retrieved from postmeta.
do_action(
Expand Down Expand Up @@ -599,24 +599,67 @@ protected function sum_status_counts( array $validated_mc_statuses ): void {
}

/**
* Calculate the product status statistics and update the transient.
* Calculate the synced product status statistics. It will group
* the variations for the same parent.
*
* For the case that one variation is approved and the other disapproved:
* 1. Give each status a priority.
* 2. Store the last highest priority status in `$parent_statuses`.
* 3. Compare if a higher priority status is found for that variable product.
* 4. Loop through the `$parent_statuses` array at the end to add the final status counts.
*
* @return array Product status statistics.
*/
protected function update_mc_statuses() {
protected function calculate_synced_product_statistics(): array {
$product_statistics = [
MCStatus::APPROVED => 0,
MCStatus::PARTIALLY_APPROVED => 0,
MCStatus::EXPIRING => 0,
MCStatus::PENDING => 0,
MCStatus::DISAPPROVED => 0,
MCSTATUS::NOT_SYNCED => 0,
MCStatus::NOT_SYNCED => 0,
];

$product_statistics_priority = [
MCStatus::APPROVED => 6,
MCStatus::PARTIALLY_APPROVED => 5,
MCStatus::EXPIRING => 4,
MCStatus::PENDING => 3,
MCStatus::DISAPPROVED => 2,
MCStatus::NOT_SYNCED => 1,
];

foreach ( $this->product_statuses['products'] as $statuses ) {
$parent_statuses = [];

foreach ( $this->product_statuses['products'] as $product_id => $statuses ) {
foreach ( $statuses as $status => $num_products ) {
$product_statistics[ $status ] += $num_products;
$parent_id = $this->product_data_lookup[ $product_id ]['parent_id'];
if ( ! $parent_id ) {
$product_statistics[ $status ] += $num_products;
} elseif ( ! isset( $parent_statuses[ $parent_id ] ) ) {
$parent_statuses[ $parent_id ] = $status;
} else {
$current_parent_status = $parent_statuses[ $parent_id ];
if ( $product_statistics_priority[ $status ] < $product_statistics_priority[ $current_parent_status ] ) {
$parent_statuses[ $parent_id ] = $status;
}
}
}
}

foreach ( $parent_statuses as $parent_status ) {
$product_statistics[ $parent_status ] += 1;
}

return $product_statistics;
}

/**
* Calculate the product status statistics and update the transient.
*/
protected function update_mc_statuses() {
$product_statistics = $this->calculate_synced_product_statistics();

/** @var ProductRepository $product_repository */
$product_repository = $this->container->get( ProductRepository::class );
$product_statistics[ MCStatus::NOT_SYNCED ] = count( $product_repository->find_mc_not_synced_product_ids() );
Expand Down
18 changes: 17 additions & 1 deletion src/Product/ProductHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
use Google\Service\ShoppingContent\Product as GoogleProduct;
use WC_Product;
use WC_Product_Variation;
use WP_Post;

defined( 'ABSPATH' ) || exit;

Expand Down Expand Up @@ -102,7 +103,11 @@ public function mark_as_synced( WC_Product $product, GoogleProduct $google_produ
*/
public function mark_as_unsynced( WC_Product $product ) {
$this->meta_handler->delete_synced_at( $product );
$this->meta_handler->update_sync_status( $product, SyncStatus::NOT_SYNCED );
if ( ! $this->is_sync_ready( $product ) ) {
$this->meta_handler->delete_sync_status( $product );
} else {
$this->meta_handler->update_sync_status( $product, SyncStatus::NOT_SYNCED );
}
$this->meta_handler->delete_google_ids( $product );
$this->meta_handler->delete_errors( $product );
$this->meta_handler->delete_failed_sync_attempts( $product );
Expand Down Expand Up @@ -289,6 +294,17 @@ public function get_wc_product( int $product_id ): WC_Product {
return $this->wc->get_product( $product_id );
}

/**
* Get WooCommerce product by WP get_post
*
* @param int $product_id
*
* @return WP_Post|null
*/
public function get_wc_product_by_wp_post( int $product_id ): ?WP_Post {
return get_post( $product_id );
}

/**
* @param WC_Product $product
*
Expand Down
17 changes: 12 additions & 5 deletions src/Product/ProductRepository.php
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
use Automattic\WooCommerce\GoogleListingsAndAds\Infrastructure\Service;
use Automattic\WooCommerce\GoogleListingsAndAds\PluginHelper;
use Automattic\WooCommerce\GoogleListingsAndAds\Value\ChannelVisibility;
use Automattic\WooCommerce\GoogleListingsAndAds\Value\MCStatus;
use Automattic\WooCommerce\GoogleListingsAndAds\Value\SyncStatus;
use WC_Product;

defined( 'ABSPATH' ) || exit;
Expand Down Expand Up @@ -81,12 +81,13 @@ public function find_ids( array $args = [], int $limit = -1, int $offset = 0 ):
* Find and return an array of WooCommerce product objects based on the provided product IDs.
*
* @param int[] $ids Array of WooCommerce product IDs
* @param array $args Array of WooCommerce args (except 'return'), and product metadata.
* @param int $limit Maximum number of results to retrieve or -1 for unlimited.
* @param int $offset Amount to offset product results.
*
* @return WC_Product[] Array of WooCommerce product objects
*/
public function find_by_ids( array $ids, int $limit = -1, int $offset = 0 ): array {
public function find_by_ids( array $ids, array $args = [], int $limit = -1, int $offset = 0 ): array {
$args['include'] = $ids;

return $this->find( $args, $limit, $offset );
Expand Down Expand Up @@ -163,7 +164,8 @@ public function find_sync_ready_products( array $args = [], int $limit = - 1, in
* @return array
*/
public function find_delete_product_ids( array $ids, int $limit = - 1, int $offset = 0 ): array {
$results = $this->find_by_ids( $ids, $limit, $offset );
$args = [ 'status' => 'trash' ];
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now working for items that need to be deleted because they are in the trash. But for items that get deleted from Merchant Center for other reasons are not found/updated. For example I set a product to catalog visibility = hidden, which will trigger it to be deleted from the Merchant Center, but it's status remains "published".

Without any status it was previously querying the default WP statuses so it seems we should include trash with the default array.

Suggested change
$args = [ 'status' => 'trash' ];
$args = [ 'status' => [ 'draft', 'pending', 'private', 'publish', 'trash' ] ];

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching this, I've updated the issue #1785 so it will be handled in the next PR. Merging this PR first.

$results = $this->find_by_ids( $ids, $args, $limit, $offset );
return $this->product_filter->filter_products_for_delete( $results )->get_product_ids();
}

Expand Down Expand Up @@ -264,9 +266,14 @@ public function find_mc_not_synced_product_ids( int $limit = -1, int $offset = 0
'type' => $types,
'meta_query' => [
[
'key' => ProductMetaHandler::KEY_MC_STATUS,
'key' => ProductMetaHandler::KEY_SYNC_STATUS,
'compare' => '!=',
'value' => SyncStatus::SYNCED,
],
[
'key' => ProductMetaHandler::KEY_VISIBILITY,
'compare' => '=',
'value' => MCStatus::NOT_SYNCED,
'value' => ChannelVisibility::SYNC_AND_SHOW,
],
],
];
Expand Down
Loading