Create product statuses jobs for each page token

[jorgemd24]

Changes proposed in this Pull Request:

Part of #2146 & #2250

In #2229 we've added a background job that handles calculating product stats, bringing all products from MC and updating their status in WooCommerce.

This PR will allow the background job to generate multiple tasks based on the $next_page_token, helping us spread out resource usage more effectively.

While working on this I noticed that we were changing the product status to "not synced" to calculate the unsynced products in MC. But, I think that's unnecessary. The actual count of unsynced products will simply be: NOT_SYNCED = WooCommerce Products ('simple', 'variable') - MC Products (with variants grouped).

In the case that we need to display the MC status, like in the Product Feed section, we can assume that if a WC product doesn't have an MC status, it's not synced in MC. I'm open to hearing other thoughts on this.

Screenshots:

1. Change the batch size here so you can create multiple batches depending on the number of products that you have or use the filter woocommerce_gla_product_view_report_page_size.
2. Make the following request to GET gla/mc/product-statistics/refresh
3. Go to WC -> Status -> Scheduled Actions -> Search for gla/jobs/update_merchant_product_statuses
4. See how multiple jobs are creating using a different page tokens.
5. Check the total stats in the Overview - Product Feed page makes sense.

Additional details:

• In another PR, I'll work with the WC_Product objects/CRUD classes and add the tests for MerchantStatuses.php and MerchantReport.php.

Changelog entry

[codecov]

Codecov Report

Attention: 143 lines in your changes are missing coverage. Please review.

Comparison is base (2185426) 58.9% compared to head (2e26242) 59.0%.
Report is 76 commits behind head on feature/refactor-product-stats.

Additional details and impacted files

@@                        Coverage Diff                         @@
##             feature/refactor-product-stats   #2255     +/-   ##
==================================================================
+ Coverage                              58.9%   59.0%   +0.1%     
+ Complexity                             4266    4178     -88     
==================================================================
  Files                                   460     457      -3     
  Lines                                 18055   17702    -353     
==================================================================
- Hits                                  10628   10443    -185     
+ Misses                                 7427    7259    -168     

Flag	Coverage Δ
php-unit-tests	59.0% <29.6%> (+0.1%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
google-listings-and-ads.php	75.0% <ø> (ø)
...ernal/DependencyManagement/CoreServiceProvider.php	0.0% <ø> (ø)
...ependencyManagement/IntegrationServiceProvider.php	0.0% <ø> (ø)
...ernal/DependencyManagement/RESTServiceProvider.php	100.0% <100.0%> (ø)
src/Notes/ReviewAfterConversions.php	100.0% <100.0%> (+100.0%)	⬆️
src/Notes/SetupCouponSharing.php	100.0% <100.0%> (+100.0%)	⬆️
src/Product/ProductHelper.php	95.4% <100.0%> (-0.5%)	⬇️
src/Product/ProductMetaHandler.php	84.1% <ø> (ø)
src/Product/SyncerHooks.php	89.3% <100.0%> (+1.5%)	⬆️
src/Product/WCProductAdapter.php	97.8% <100.0%> (+1.7%)	⬆️
... and 9 more

... and 7 files with indirect coverage changes

[mikkamp]

I didn't want to do a full review of this PR, but just had a few comments.

While working on this I noticed that we were changing the product status to "not synced" to calculate the unsynced products in MC. But, I think that's unnecessary. The actual count of unsynced products will simply be: NOT_SYNCED = WooCommerce Products ('simple', 'variable') - MC Products (with variants grouped).
In the case that we need to display the MC status, like in the Product Feed section, we can assume that if a WC product doesn't have an MC status, it's not synced in MC. I'm open to hearing other thoughts on this.

That does seem to be an accurate calculation moving forward, although I'm wondering if that's totally backwards compatible since there might be products with the NOT_SYNCED status already set. For the sync stats calculation that seems to be fine, but do we need to consider unsetting the NOT_SYNCED status for products?

Another question, is should total WC products be done with a product type filter? Wouldn't the Merchants perceive that an excluded product type would be unsynced?

Also for intermediate data in between jobs, I see it's being stored in a transient. Given that a transient can disappear at any time, it would be safer to use an option, like is done in the UpdateSyncableProductsCount.

I noticed ProductSyncStats was being removed, if we are going to merge this PR early, then shouldn't we leave that in, since it's still used in the UI.

[jorgemd24]

Thanks @mikkamp for your comments:

since there might be products with the NOT_SYNCED status already set. For the sync stats calculation that seems to be fine, but do we need to consider unsetting the NOT_SYNCED status for products?

Can you help me to see a scenario where having the NOT_SYNCED status already set could potentially cause an issue? Or vice versa? If it is an issue, we could consider creating a job that runs once to unset the NOT_SYNCED status for products.

Another question, is should total WC products be done with a product type filter? Wouldn't the Merchants perceive that an excluded product type would be unsynced?

Do you mean, that the total number of products should be the count of all products, regardless of their type (including variations)?

Also for intermediate data in between jobs, I see it's being stored in a transient. Given that a transient can disappear at any time,

TIL, I thought the transient would stay until its expiration time, but it looks like it can get deleted even sooner.

Furthermore, it is possible for the transient to not be available before the expiration time

[jorgemd24]

@mikkamp and @martynmjones, I changed the PR to use an option for the intermediate data and reverted the change of removing ProductSyncStats.

[mikkamp]

Can you help me to see a scenario where having the NOT_SYNCED status already set could potentially cause an issue? Or vice versa? If it is an issue, we could consider creating a job that runs once to unset the NOT_SYNCED status for products.

I was mainly thinking about the point where you mentioned:

In the case that we need to display the MC status, like in the Product Feed section, we can assume that if a WC product doesn't have an MC status, it's not synced in MC.

But I see now that's for individual products so we can combine both NOT_SYNCED and not set as the same values. The other thought I was worried about is the querying, but it seems find_mc_not_synced_product_ids is no longer there, so I assume that means we don't have anywhere we need to fetch all products that are "not synced".

Do you mean, that the total number of products should be the count of all products, regardless of their type (including variations)?

Yes exactly, because there could be lots of unsupported product types. Variations use a different post_type=product_variation. Do we need to do something special to fetch only the post_type=product in the query?

Thanks for switching the transient to an option, it's just a little more reliable and hard to troubleshoot if a transient ever gets cleared.

[martynmjones]

Hey @jorgemd24, thank you for improving the efficiency of this feature.

Aside from the comments that have already been addressed above I can't see any problems with these changes. Tested the branch in several different scenarios and have consistently had the correct stats built and the scheduled actions behave as to be expected. So LGTM ✅

[jorgemd24]

Thanks @martynmjones for your time reviewing this PR!