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
5 changes: 0 additions & 5 deletions src/Jobs/DeleteProducts.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,6 @@ 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 );

// Exclude any ID's which are not ready to delete.
$product_id_map = array_intersect( $product_id_map, $ready_ids );

Copy link
Contributor

Choose a reason for hiding this comment

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

By removing these lines we are reverting the PR #1238
I can see the flaws in the filter, but I don't think we should revert it completely otherwise we'll reintroduce the endless retry upon failure. I can see the logic of adding the status equals trash, but if we look at the sync hooks. It's hooked into both when a product is trashed and when it's deleted completely. In the latter case it won't be able to query the product data at all. Which means:

  • we should still try the initial delete_by_batch_request
  • we should not retry a product that is not available in the database

In that case maybe we should move the filtering from process_items to the code block where the retries are scheduled. If we filter the ID map at that point then we can remove any no longer existing products and ones that retried too many times.

Although I would move this to a separate issue/PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, my brain wasn't working properly yesterday, I actually forgot to add back the deleted code so even though I added "status equals trash" logic in find_delete_product_ids method from ProductRepository.php, there is no one calling this method. So if I added it back like below It would try to find the product with status trash then exclude the ones which are not ready to delete. Note that array_intersect( $product_id_map, $ready_ids ); can work as array_intersect is working through the array values.

--- a/src/Jobs/DeleteProducts.php
+++ b/src/Jobs/DeleteProducts.php
@@ -37,6 +37,12 @@ class DeleteProducts extends AbstractProductSyncerJob implements StartOnHookInte
   * @throws ProductSyncerException If an error occurs. The exception will be logged by ActionScheduler.
   */
  public function process_items( array $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 );
+
    $product_entries = BatchProductIDRequestEntry::create_from_id_map( new ProductIDMap( $product_id_map ) );
    $this->product_syncer->delete_by_batch_requests( $product_entries );
  }

but if we look at the sync hooks. It's hooked into both when a product is trashed and when it's deleted completely. In the latter case it won't be able to query the product data at all.

Correct me if I'm wrong, from the WooCommerce admin products page I can only delete a trash product completely. Which means based on the above code, we would call delete_by_batch_request when a product status is becoming trash, and we would not call delete_by_batch_request again when the product is deleted completely. Is it correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

A product can still be force deleted (without going to trash) either through WP-CLI, API or making use of a plugin.
In that case we have the ID mapping scheduled for removal, but we can't load any product details when the scheduled task actually starts running. So I imagine in that case the filter wouldn't work and the product would get excluded from the batch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I think in this case it would be better to move this into different issue/PR so we don't block this PR. Do you think I should revert the commits 3f7a2cc and 7dfd462 then create an issue, or adding a new commit including the change in my previous comment and create an issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think fixing it temporarily by changing the status to trash (as long as we aren't reverting previous behaviour) is a good option. And then create a separate followup issue for filtering later, since that's covering behaviour that was already broken.

Copy link
Member Author

Choose a reason for hiding this comment

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

The follow up issue is created in #1785.

$product_entries = BatchProductIDRequestEntry::create_from_id_map( new ProductIDMap( $product_id_map ) );
$this->product_syncer->delete_by_batch_requests( $product_entries );
}
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
12 changes: 12 additions & 0 deletions 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 @@ -289,6 +290,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
*/
public function get_wc_product_by_wp_post( int $product_id ): WP_Post {
Copy link
Contributor

Choose a reason for hiding this comment

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

After we call this function we double check to make sure the product has a value (is not null), but the return type of this function does not allow null. Do we need a nullable return type?

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in 6c84398.

return get_post( $product_id );
}

/**
* @param WC_Product $product
*
Expand Down
14 changes: 8 additions & 6 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,9 @@ public function find_mc_not_synced_product_ids( int $limit = -1, int $offset = 0
'type' => $types,
'meta_query' => [
[
'key' => ProductMetaHandler::KEY_MC_STATUS,
'compare' => '=',
'value' => MCStatus::NOT_SYNCED,
'key' => ProductMetaHandler::KEY_SYNC_STATUS,
'compare' => '!=',
'value' => SyncStatus::SYNCED,
],
],
];
Expand Down
Loading