Refactor product issues #2277

jorgemd24 · 2024-02-23T20:13:21Z

Changes proposed in this Pull Request:

This PR fetches the product issues using the same background job used to fetch the product statuses. This allows us to reuse the WC products that were fetched while updating the product statuses.

The logic remains mostly unchanged from before. The main difference is that previously, Google IDs were stored in the database and used to fetch the issues. With the new sync mechanism, since we don't have the Google IDs until we fetch all product statuses, we now use the data from the product statuses to obtain the Google IDs and fetch the issues.

Screenshots:

Detailed test instructions:

Set your PHP memory limit low so you can catch any heavy process. For example, setting this up to 128MB.
Call GET wp-json/wc/gla/mc/product-statistics/refresh
Go to WooCommerce -> Status -> Action Scheduler -> Search for gla/jobs/update_merchant_product_statuses/process_item see how the jobs get completed successfully.
See how the table wp_gla_merchant_issues gets populated.
Once all jobs are completed call GET wp-json/wc/gla/mc/product-statistics and see that you get the right stats.

Additional details:

Account and pre-sync issues are not migrated yet.
After all the issues logic is refactored, I will remove any legacy code.
I wanted to address the scenario where the job fails and some issues are inserted. I used the "created at" field, but it appears that the field will be identical for all batches. Therefore, I'll need to see alternative solutions in a follow-up PR. Any suggestions are welcome too.

Changelog entry

…uplicate issues

codecov · 2024-02-23T20:17:55Z

Codecov Report

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

Project coverage is 60.3%. Comparing base (6f441d2) to head (c06f4e9).
Report is 2 commits behind head on feature/refactor-product-stats.

Additional details and impacted files

@@                        Coverage Diff                         @@
##             feature/refactor-product-stats   #2277     +/-   ##
==================================================================
+ Coverage                              60.0%   60.3%   +0.3%     
+ Complexity                             4184    4181      -3     
==================================================================
  Files                                   457     457             
  Lines                                 17748   17733     -15     
==================================================================
+ Hits                                  10647   10693     +46     
+ Misses                                 7101    7040     -61

Flag	Coverage Δ
php-unit-tests	`60.3% <83.1%> (+0.3%)`	⬆️

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

Files	Coverage Δ
src/API/Google/MerchantReport.php	`34.7% <100.0%> (+0.6%)`	⬆️
src/Jobs/UpdateMerchantProductStatuses.php	`92.6% <100.0%> (-0.3%)`	⬇️
src/DB/Table/MerchantIssueTable.php	`0.0% <0.0%> (ø)`
src/MerchantCenter/MerchantStatuses.php	`66.5% <88.1%> (+12.6%)`	⬆️

puntope

Thanks @jorgemd24 for working on this. I tested following the indications inn the description and worked for me.

I left some comments regarding the code.

I wanted to address the scenario where the job fails and some issues are inserted. I used the "created at" field, but it appears that the field will be identical for all batches. Therefore, I'll need to see alternative solutions in a follow-up PR. Any suggestions are welcome too.

Could you elaborate this? I'm not sure if I understand the issue.

src/MerchantCenter/MerchantStatuses.php

puntope · 2024-02-27T20:49:43Z

src/MerchantCenter/MerchantStatuses.php

+	 *
+	 * @since x.x.x
+	 */
+	protected function delete_stale_issues( string $compare = '<' ) {


💅 Maybe we might add : void to be consistent with the rest of the class signatures.

src/MerchantCenter/MerchantStatuses.php

puntope · 2024-02-27T20:58:13Z

src/DB/Table/MerchantIssueTable.php

@@ -55,9 +55,10 @@ public static function get_raw_name(): string {
 	 * Delete stale issue records.
 	 *
 	 * @param DateTime $created_before Delete all records created before this.
+	 * @param string   $compare         Comparison operator to use. Default is '<'.


💅 Maybe we can delete the extra tabulation between $compare and the description.

puntope · 2024-02-27T21:07:00Z

src/MerchantCenter/MerchantStatuses.php

 	 *
-	 * @param string[] $google_ids
+	 * @param [] $statuses The list of Product View statuses.


Shouldn't be array[] instead of []?

Additionally. PHPDoc comment doesn't contain all the necessary @throws tags derivated from $this->container->get

src/MerchantCenter/MerchantStatuses.php

puntope · 2024-02-27T21:34:07Z

src/MerchantCenter/MerchantStatuses.php

 	 *
-	 * @param GoogleProductStatus[] $validated_mc_statuses Product statuses of validated products.
+	 * @param [] $product_issues Array of product issues.


💅 This might be array instead of [] also some throw tags missing.

src/MerchantCenter/MerchantStatuses.php

jorgemd24 · 2024-03-04T18:18:12Z

Thanks @puntope for the review! I addressed all your comments and replied to some of your suggestions. Regards:

Could you elaborate this? I'm not sure if I understand the issue.

I was thinking a scenario where the job fails during inserting the issues. For instance, if the job inserts some issues but others fail, such as a query taking too long, the job itself fails, and Action Scheduler will retry it. This means it would reinsert the issues that were successfully added in the initial try. The update_or_insert will always insert the row because the key in the table is the ID and not the product_id and the issue so I decided to delete the issues for the products we were processing for that specific job, so if the job retries, it starts fresh without any stale data. See here: c75eb4d

Could you please take another look at it? Thanks!

puntope · 2024-03-05T14:53:24Z

I decided to delete the issues for the products we were processing for that specific job, so if the job retries, it starts fresh without any stale data.

Yes, I guess that might work. Other alternative option may be a duplicate cleanup after the AS Job batch finishes.

puntope · 2024-03-05T14:55:54Z

src/DB/Table/MerchantIssueTable.php

+	 * @param array  $products_ids Array of product IDs to delete issues for.
+	 * @param string $source       The source of the issues. Default is 'mc'.
+	 */
+	public function delete_specific_product_issues( array $products_ids, string $source = 'mc' ) {


💅 We can add :void as a return type in the signature to be consistent with the other functions. delete_stalealso missed it, but was not introduced in this PR tho

puntope · 2024-03-05T15:05:16Z

src/MerchantCenter/MerchantStatuses.php

+	 *
+	 * @since x.x.x
+	 */
+	protected function delete_stale_issues() {


💅 Missed :void

puntope · 2024-03-05T15:11:44Z

src/MerchantCenter/MerchantStatuses.php

+	 *
+	 * @since x.x.x
+	 */
+	public function clear_product_statuses_cache_and_issues(): void {


💅 Maybe we can add also some tests for this new logic.

puntope · 2024-03-05T15:12:16Z

src/MerchantCenter/MerchantStatuses.php

 				do_action(
 					'woocommerce_gla_debug_message',
 					sprintf( 'Merchant Center product %s not found in this WooCommerce store.', $mc_product_id ),
 					__METHOD__ . ' in remove_invalid_statuses()',
 				);
 				continue;
 			}
+			// Unsynced issues shouldn't be shown.
+			if ( ChannelVisibility::DONT_SYNC_AND_SHOW === $wc_product->get_meta( $visibility_meta_key ) ) {


💅 Seems that we are not covering this case in tests.

puntope

Thanks @jorgemd24 for all the changes.

I approve this as there are no blockers anymore and it works as defined in the PR description.

I left tho some comments regarding code and test coverage.

jorgemd24 added 8 commits February 22, 2024 20:32

Add mc_id to the get_product_view_report response

b2adf15

Refactor product issues

afcd5e5

Adjust merchant center report test

19cf5d0

Add delete_stale

6466bce

Refactor clear cache product statuses

d50f790

Add tests to refresh product issues

4634d2f

Rename function and delete issues if the job fails so we don't have d…

d87fb20

…uplicate issues

Fix issue with the comparision

6a5fcbe

jorgemd24 self-assigned this Feb 23, 2024

github-actions bot added changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement. labels Feb 23, 2024

jorgemd24 added 3 commits February 23, 2024 21:19

Add missing phpdoc

5dea916

Fix phpcs

f4c01b9

phpcs tests

f979216

jorgemd24 requested a review from a team February 23, 2024 20:43

Add @var definition

5417896

jorgemd24 marked this pull request as ready for review February 23, 2024 20:57

puntope reviewed Feb 28, 2024

View reviewed changes

jorgemd24 added 5 commits March 4, 2024 16:38

Update PHPDocs

6c73cbc

Update function description

907bd4a

Revert delete_stale with compare query

737351e

Add delete specific product issues

c75eb4d

Adjust tests for test_handle_failed_mc_statuses_fetching

7f3e1f5

puntope reviewed Mar 5, 2024

View reviewed changes

puntope approved these changes Mar 5, 2024

View reviewed changes

jorgemd24 added 3 commits March 5, 2024 18:02

Add missing return types

6848100

Add tests for clear_product_statuses_cache_and_issues

f185a48

Add tests for fetching issues for products with DONT_SYNC_AND_SHOW

c06f4e9

jorgemd24 merged commit f1ab4e3 into feature/refactor-product-stats Mar 5, 2024
11 checks passed

jorgemd24 deleted the add/refactor-product-issues branch March 5, 2024 17:53

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 #2277

Refactor product issues #2277

jorgemd24 commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 23, 2024 •

edited

Loading

puntope left a comment

puntope Feb 27, 2024

puntope Feb 27, 2024

puntope Feb 27, 2024

puntope Feb 27, 2024

puntope Feb 27, 2024

jorgemd24 commented Mar 4, 2024

puntope commented Mar 5, 2024

puntope Mar 5, 2024

puntope Mar 5, 2024

puntope Mar 5, 2024

puntope Mar 5, 2024

puntope left a comment

Refactor product issues #2277

Refactor product issues #2277

Conversation

jorgemd24 commented Feb 23, 2024 • edited Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Feb 23, 2024 • edited Loading

Codecov Report

puntope left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgemd24 commented Mar 4, 2024

puntope commented Mar 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope left a comment

Choose a reason for hiding this comment

jorgemd24 commented Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 23, 2024 •

edited

Loading