Refactor Product issues and fix issues when getting stats for a fresh MC account #2306

jorgemd24 · 2024-03-12T16:02:29Z

Changes proposed in this Pull Request:

While I was testing the new approach with a fresh Merchant Center account I found some issues that this PR fixes:

After the onboarding, we call mc/settings/sync, which kicks off the woocommerce_gla_mc_settings_sync hook and schedules the UpdateAllProducts job. As each batch gets processed, it triggers the woocommerce_gla_batch_updated_products hook, clearing the merchant statuses cache for each batch. This can create a loop: stats are fetched, and then deleted when the batch is processed. So, if the user refreshes the stats, it'll redo the calculation only to delete them again.

google-listings-and-ads/src/Event/ClearProductStatsCache.php

Lines 38 to 51 in f4aaafb

    
           public function register(): void { 
        
           	add_action( 
        
           		'woocommerce_gla_batch_updated_products', 
        
           		function () { 
        
           			$this->clear_stats_cache(); 
        
           		} 
        
           	); 
        
           	add_action( 
        
           		'woocommerce_gla_batch_deleted_products', 
        
           		function () { 
        
           			$this->clear_stats_cache(); 
        
           		} 
        
           	); 
        
           }

This PR checks if UpdateAllProducts or DeleteAllProducts jobs is running before clearing the cache and will refresh the data when the batches are completed.

With a fresh MC, I noticed that the results from MerchantProductViewReportQuery and get_productstatuses_batch are returned as null if there is no data, instead of an empty array, this PR fixes this using ?? [].

Also, this PR tweaks some parts of the code:

Remove get_synced_google_ids as it is not used anymore.
In case the job fails, it will restore the intermediate data to the previous state instead of deleting the intermediate data. So when AS retries the job, it will start with the correct data.
When handle_complete_mc_statuses_fetching gets called, we check if the intermediate data is set. If it is, we mark the process as complete. However, I reckon it's safer to return a default intermediate state just in case the data somehow gets deleted. This way, the process can always be completed.
I decided to remove $this->clear_cache(); from clear_product_statuses_cache_and_issues because when the product stats are updated, the transient gets updated automatically. This way, the user won't even notice the process happening.

Screenshots:

Detailed test instructions:

Disconnect all the account connections.
Complete the onboarding process with a new MC account.
Check that when all update_all_products jobs finish, it triggers update_merchant_product_statuses.
Once update_merchant_product_statuses is done, verify that the product statistics are displayed and that the account, pre-sync, and production issues are populated. (Dashboard -> Produc Feed -> Overview)
Initially, it may indicate that not all products are synced, as Google may require some time to process them.
After some time, refresh the statistics using GET wc/gla/mc/product-statistics/refresh and check if the stats and product issues are updated accordingly.

Additional details:

Changelog entry

…roducts

codecov · 2024-03-12T16:07:09Z

Codecov Report

Attention: Patch coverage is 97.67442% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 64.3%. Comparing base (6b7a614) to head (71b864b).
Report is 178 commits behind head on feature/refactor-product-stats.

Additional details and impacted files

@@                        Coverage Diff                         @@
##             feature/refactor-product-stats   #2306     +/-   ##
==================================================================
+ Coverage                              60.5%   64.3%   +3.7%     
- Complexity                             4183    4245     +62     
==================================================================
  Files                                   457     458      +1     
  Lines                                 17745   18024    +279     
==================================================================
+ Hits                                  10742   11584    +842     
+ Misses                                 7003    6440    -563

Flag	Coverage Δ
php-unit-tests	`64.3% <97.7%> (+3.7%)`	⬆️

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

Files	Coverage Δ
src/API/Google/MerchantReport.php	`35.3% <100.0%> (+0.5%)`	⬆️
src/DB/Table/MerchantIssueTable.php	`16.7% <100.0%> (+16.7%)`	⬆️
src/Jobs/AbstractProductSyncerBatchedJob.php	`100.0% <100.0%> (ø)`
src/Jobs/DeleteAllProducts.php	`100.0% <100.0%> (+100.0%)`	⬆️
src/Jobs/UpdateAllProducts.php	`100.0% <100.0%> (ø)`
src/Jobs/UpdateMerchantProductStatuses.php	`92.9% <100.0%> (ø)`
src/MerchantCenter/MerchantStatuses.php	`74.4% <100.0%> (+1.9%)`	⬆️
src/Notes/SetupCouponSharing.php	`100.0% <100.0%> (ø)`
src/Product/ProductRepository.php	`85.7% <ø> (ø)`
...ternal/DependencyManagement/JobServiceProvider.php	`0.0% <0.0%> (ø)`

... and 64 files with indirect coverage changes

…e-and-product-status-code

src/API/Google/MerchantReport.php

puntope · 2024-03-13T22:04:00Z

src/MerchantCenter/MerchantStatuses.php

+
+		// Clear the cache if we are not in the middle of updating/deleting all products. Otherwise, we might update the product stats for each individual batch.
+		// See: ClearProductStatsCache::register
+		if ( $update_all_products_job->can_schedule( null ) && $delete_all_products_job->can_schedule( null ) ) {


Why can_schedule and not is_running?

Because is_running is a protected method, I understand that maybe it would be a bit easier to read the code if we use a method name similar to is_running. Although I think the comment that I left explains what we are doing.

More than the naming I was more thinking about the logic in can_schedule. That also checks some conditions. If MC connected for example. But if that doesn't create a problem I think we are good with can_schedule

src/MerchantCenter/MerchantStatuses.php

puntope · 2024-03-14T23:26:42Z

Hi @jorgemd24 seems like is looking good. However, I cannot see the products populated (I waited a while) 🤔 I will check tomorrow morning to verify if they loaded already.

jorgemd24 · 2024-03-15T12:38:02Z

Thanks @puntope for the review!

Hi @jorgemd24 seems like is looking good. However, I cannot see the products populated (I waited a while) 🤔 I will check tomorrow morning to verify if they loaded already.

Do you mean that the products aren't synchronized in the MC? If that's the case, it's worth mentioning that we haven't changed any logic with the product syncer. We've only refactored how we retrieve the product stats.

puntope · 2024-03-15T17:15:01Z

Thanks @puntope for the review!

Hi @jorgemd24 seems like is looking good. However, I cannot see the products populated (I waited a while) 🤔 I will check tomorrow morning to verify if they loaded already.

Do you mean that the products aren't synchronized in the MC? If that's the case, it's worth mentioning that we haven't changed any logic with the product syncer. We've only refactored how we retrieve the product stats.

Ok, thanks @jorgemd24 Let's suppose that is another issue unrelated to this PR.

However, I synced a product manually via Connection test page, just to check:

When I run it on this branch I got

{
    "timestamp": 1710522408,
    "statistics": {},
    "scheduled_sync": 23,
    "loading": true,
    "error": null
}

if I checkout develop and run it again I got

{
    "timestamp": 1710522512,
    "statistics": {
        "active": 0,
        "expiring": 0,
        "pending": 0,
        "disapproved": 1,
        "not_synced": 0
    },
    "scheduled_sync": 23
}

In the dashboard. In develop I see the product as "disapproved", in this branch the product keeps as "Not synced" and the statistics shows N/A

Develeop

This branch

Just wonder if this is expected. And if not, I wonder if that's related to this PR or is a separated issue.

jorgemd24 · 2024-03-16T20:53:50Z

Hi @puntope,

Thanks for your comments, regards:

{
    "timestamp": 1710522408,
    "statistics": {},
    "scheduled_sync": 23,
    "loading": true,
    "error": null
}

On that response, it appears that the job update_merchant_product_statuses was either running or scheduled to run (see loading = true). Additionally, it seems that at that moment, there were 23 jobs scheduled to sync, so it's possible that the update_merchant_product_statuses job hadn't been executed yet due to other jobs in the queue. Once the job is executed and finishes fetching all the stats, you should get the correct stats.

Regarding the screenshots, that' expected. I'm currently working on adding some spinning icons to indicate when the update_merchant_product_statuses job is scheduled or running, instead of the "N/A" currently displayed.

puntope · 2024-03-17T15:50:23Z

Hi @jorgemd24 All the jobs mentioned are completed. But I still get the same response:

{
    "timestamp": 1710690602,
    "statistics": {},
    "scheduled_sync": 23,
    "loading": true,
    "error": null
}

However, I was able to see the stats finally in the dashboard. Let me know if that refresh stats response is expected or we should continue investigating.

jorgemd24 · 2024-03-18T17:34:48Z

Hi @puntope,

{
    "timestamp": 1710690602,
    "statistics": {},
    "scheduled_sync": 23,
    "loading": true,
    "error": null
}

That response indicates that the update_merchant_product_statuses job was scheduled so that's why no stats should be shown.

However, I was able to see the stats finally in the dashboard. Let me know if that refresh stats response is expected or we should continue investigating. But it also shows

I guess later on update_merchant_product_statuses job was completed and the response should look like this:

{
    "timestamp": 1710711097,
    "statistics": {
      "active" : 1
      "disapproved" : 2
      "expiring" : 3 
      "not_synced" : 4
      "pending" : 5
    },
    "scheduled_sync": 0,
    "loading": false,
    "error": null
}

If that's the case, it's expected. Keep in mind, that the UI isn't refreshing the data at the moment (at least that you refresh the page manually) because in this PR only contains the backend.

puntope

Hi @jorgemd24 Finally the issue was because I thought calling wc/gla/mc/product-statistics/refresh would return the stats

Calling wc/gla/mc/product-statistics return the right response

{
    "timestamp": 1710842811,
    "statistics": {
        "active": 0,
        "expiring": 0,
        "pending": 0,
        "disapproved": 1,
        "not_synced": 18
    },
    "scheduled_sync": 23,
    "loading": false,
    "error": null
}

jorgemd24 added 11 commits March 11, 2024 12:10

Remove unused method

48939d2

Simplify process product status

2e37bd8

Fix issue with unset active key

e794933

Check if there are productids when deleting product issues

b40d270

Remove unused dependency

72c8ba9

Handle the case where there are not products in MC

f5bd742

Handle null responses

925c3fb

Remove test line

7ca5d6a

Revert intermediate status if the job fails

021b6d7

Don't update stats for each single batch when updating/deleting all p…

6b767b4

…roducts

Adjust tests

f4aaafb

jorgemd24 added 4 commits March 12, 2024 17:38

Fix phpcs

3d2e060

Add tests for clear cache with bulk sync

cc2ff9b

Add tests for MerchantIssueTableTest

67f8c56

Add DeleteAllProductsTest

476873d

jorgemd24 changed the title ~~Refactor Product issues and fix issues when getting stats for a new MC~~ Refactor Product issues and fix issues when getting stats for a fresh MC account Mar 12, 2024

jorgemd24 self-assigned this Mar 12, 2024

jorgemd24 marked this pull request as ready for review March 12, 2024 17:45

jorgemd24 requested a review from a team March 12, 2024 17:45

Merge branch 'feature/refactor-product-stats' into remove/legacy-issu…

71b864b

…e-and-product-status-code

eason9487 added the changelog: fix Took care of something that wasn't working. label Mar 13, 2024

puntope reviewed Mar 13, 2024

View reviewed changes

src/API/Google/MerchantReport.php Show resolved Hide resolved

puntope reviewed Mar 13, 2024

View reviewed changes

src/MerchantCenter/MerchantStatuses.php Show resolved Hide resolved

puntope approved these changes Mar 19, 2024

View reviewed changes

jorgemd24 merged commit 6629f5b into feature/refactor-product-stats Mar 19, 2024
11 checks passed

jorgemd24 deleted the remove/legacy-issue-and-product-status-code branch March 19, 2024 13:11

eason9487 mentioned this pull request Mar 26, 2024

Release 2.6.4 #2333

Merged

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Product issues and fix issues when getting stats for a fresh MC account #2306

Refactor Product issues and fix issues when getting stats for a fresh MC account #2306

jorgemd24 commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

puntope Mar 13, 2024

jorgemd24 Mar 14, 2024

puntope Mar 14, 2024

puntope commented Mar 14, 2024

jorgemd24 commented Mar 15, 2024

puntope commented Mar 15, 2024

jorgemd24 commented Mar 16, 2024 •

edited

Loading

puntope commented Mar 17, 2024 •

edited

Loading

jorgemd24 commented Mar 18, 2024

puntope left a comment

	public function register(): void {
	add_action(
	'woocommerce_gla_batch_updated_products',
	function () {
	$this->clear_stats_cache();
	}
	);
	add_action(
	'woocommerce_gla_batch_deleted_products',
	function () {
	$this->clear_stats_cache();
	}
	);
	}

Refactor Product issues and fix issues when getting stats for a fresh MC account #2306

Refactor Product issues and fix issues when getting stats for a fresh MC account #2306

Conversation

jorgemd24 commented Mar 12, 2024 • edited Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Mar 12, 2024 • edited Loading

Codecov Report

puntope Mar 13, 2024

Choose a reason for hiding this comment

jorgemd24 Mar 14, 2024

Choose a reason for hiding this comment

puntope Mar 14, 2024

Choose a reason for hiding this comment

puntope commented Mar 14, 2024

jorgemd24 commented Mar 15, 2024

puntope commented Mar 15, 2024

jorgemd24 commented Mar 16, 2024 • edited Loading

puntope commented Mar 17, 2024 • edited Loading

jorgemd24 commented Mar 18, 2024

puntope left a comment

Choose a reason for hiding this comment

jorgemd24 commented Mar 12, 2024 •

edited

Loading

codecov bot commented Mar 12, 2024 •

edited

Loading

jorgemd24 commented Mar 16, 2024 •

edited

Loading

puntope commented Mar 17, 2024 •

edited

Loading