Implement RequestReview in the Merchant class #2411

puntope · 2024-05-24T16:27:11Z

Changes proposed in this Pull Request:

This PR implementers Get Account Review Status and Request a Review in the MErchant Class instead of doing it with WooCommerce Connect Server (via Middleware)

The reason of this is in order to avoid unnecessary manager quota requests

See slack context here: p1713778408402949-slack-C02BB3F30TG

Screenshots:

Screen.Recording.2024-05-24.at.20.23.49.mov

Detailed test instructions:

Checkout this PR
Connect MC with an account disapproved and eligible for review request
See the disapproved message in the product feed tab
See the request review notice
Request a review successfully and see how the status goes to "Under review"
Check now an approved or pending to approve account
See the review status message is correct in product feed tab

Additional details:

Changelog entry

Update - Implement Account Request Review Requests in the extension

codecov · 2024-05-24T16:31:10Z

Codecov Report

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

Project coverage is 64.5%. Comparing base (d678ed7) to head (25a3688).
Report is 141 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##             develop   #2411     +/-   ##
===========================================
  Coverage       64.5%   64.5%             
- Complexity      4252    4255      +3     
===========================================
  Files            458     459      +1     
  Lines          18041   16790   -1251     
===========================================
- Hits           11634   10835    -799     
+ Misses          6407    5955    -452

Flag	Coverage Δ
php-unit-tests	`64.5% <86.4%> (+<0.1%)`	⬆️

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

Files	Coverage Δ
src/API/Google/Merchant.php	`100.0% <100.0%> (ø)`
src/API/Google/Middleware.php	`94.4% <ø> (+7.6%)`	⬆️
src/Google/RequestReviewStatuses.php	`100.0% <100.0%> (ø)`
...ernal/DependencyManagement/RESTServiceProvider.php	`100.0% <100.0%> (ø)`
...rollers/MerchantCenter/RequestReviewController.php	`92.9% <74.3%> (-6.3%)`	⬇️

... and 190 files with indirect coverage changes

ianlin

Thanks for working on this, it tested well locally, I've left some comments that we could discuss about.

I also found there was a PHP Notice while requesting to GET /gla/mc/review:

Indirect modification of overloaded element of Automattic\WooCommerce\GoogleListingsAndAds\Vendor\Google\S ervice\ShoppingContent\FreeListingsProgramStatusRegionStatus has no effect in /var/www/html/wp-content/plugins/google-listings-and-ads/src/Google/RequestReviewStatuses.php on line 136"

The warning seems to be in src/Google/RequestReviewStatuses.php:136, a quick Google search suggests that we could create a copy of $region_status['regoinCodes'] before sorting the array in-place and it worked on my end. But what I don't understand is why this PR would have this warning but without this PR it's all good.

ianlin · 2024-05-27T04:30:59Z

src/API/Site/Controllers/MerchantCenter/RequestReviewController.php

@@ -251,14 +254,91 @@ private function get_cached_review_status(): ?array {
 	 * @throws Exception If the get_account_review_status API call fails.
 	 */
 	private function get_review_status(): ?array {
-		$review_status = $this->get_cached_review_status();
+		$review_status = null;


❓ Are we no longer using the transient? If that's the case, we could probably remove the get_cached_review_status and set_cached_review_status methods as well. But IMO we should continue using the transient as we did before.

Thx for this catch @ianlin I commited that accidentally (I set it as null for testing)

Fixed here 514bbe5

ianlin · 2024-05-27T05:06:50Z

src/API/Site/Controllers/MerchantCenter/RequestReviewController.php

+			do_action( 'woocommerce_gla_invalid_response', $response, __METHOD__ );
+			$error = $response['message'] ?? __( 'Invalid response getting account review status', 'google-listings-and-ads' );
+			throw new Exception( $error, $response->getCode() );


I was wondering if this code would ever be executed. In this PR, the merchant->get_account_review_status() method returns an array of responses for the statuses of freeListingsProgram and shoppingAdsProgram. If an error occurs, it throws an exception. So I believe lines 288 to 290 would never be reached.

Thats right. I tweaked it here.

7d9ed7d

ianlin · 2024-05-27T06:35:13Z

src/API/Site/Controllers/MerchantCenter/RequestReviewController.php

+			/** @var array| Exception $response */
+			$response = $this->merchant->get_account_review_status();
+
+			if ( isset( $response['freeListingsProgram'] ) || isset( $response['shoppingAdsProgram'] ) ) {


Maybe using && instead of || as it will always include both of them in the response? Moreover, what about checking the response status code for both $response['freeListingsProgram'] and $response['shoppingAdsProgram']?

get_account_review_status returns an array with both freeListingsProgram and shoppingAdsProgram we don't need both of them as we merge them afterwards in RequestReviewStatuses::get_statuses_from_response()

Regarding the status, the Google call will return FreeListingsProgramStatus or ShoppingAdsProgramStatus which is a Collection and doesn't contain statuses.

Ah that makes sense, thanks for clarifying.

puntope · 2024-05-27T07:46:22Z

The warning seems to be in src/Google/RequestReviewStatuses.php:136, a quick Google search suggests that we could create a copy of $region_status['regoinCodes'] before sorting the array in-place and it worked on my end. But what I don't understand is why this PR would have this problem but without this PR it's all good.

Thanks @ianlin Adjusted here c9078bd

But what I don't understand is why this PR would have this problem but without this PR it's all good.

Notice the response we got before via WCS has a JSON encode/decode process making a copy of those arrays. In the current implementation we operate with the response directly. I would bet that may be the reason.

puntope · 2024-05-27T07:46:48Z

Hi @ianlin Thanks for your great review and for catching those issues. THis is ready for a new round.

ianlin

Thanks for the prompt update, all good. 👍

ianlin · 2024-05-27T08:41:13Z

src/API/Google/Merchant.php

+
+	/**
+	 * Get the review status for an MC account
+	 *


Since it's a new method in this class maybe we could add since x.x.x.

ianlin · 2024-05-27T08:41:33Z

src/API/Google/Merchant.php

+
+	/**
+	 * Request a review for an MC account
+	 *


Same for adding since x.x.x.

Implement RequestReview in the Merchant class

88c2119

puntope requested a review from a team May 24, 2024 16:27

github-actions bot added the changelog: update Big changes to something that wasn't broken. label May 24, 2024

puntope self-assigned this May 24, 2024

puntope marked this pull request as ready for review May 24, 2024 16:27

ianlin reviewed May 27, 2024

View reviewed changes

puntope added 3 commits May 27, 2024 11:20

Restore get_cached_review_status()

514bbe5

Tweak logic in get status

7d9ed7d

Fix sort warning

c9078bd

puntope requested a review from ianlin May 27, 2024 07:46

ianlin approved these changes May 27, 2024

View reviewed changes

Add since tag

25a3688

puntope merged commit 6b810c1 into develop May 27, 2024
12 checks passed

puntope deleted the update/request-review-merchant branch May 27, 2024 10:27

jorgemd24 mentioned this pull request May 29, 2024

Release 2.7.1 #2416

Merged

21 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RequestReview in the Merchant class #2411

Implement RequestReview in the Merchant class #2411

puntope commented May 24, 2024

codecov bot commented May 24, 2024 •

edited

Loading

ianlin left a comment •

edited

Loading

ianlin May 27, 2024

puntope May 27, 2024

ianlin May 27, 2024

puntope May 27, 2024

ianlin May 27, 2024

puntope May 27, 2024

ianlin May 27, 2024

puntope commented May 27, 2024

puntope commented May 27, 2024

ianlin left a comment

ianlin May 27, 2024

ianlin May 27, 2024

Implement RequestReview in the Merchant class #2411

Implement RequestReview in the Merchant class #2411

Conversation

puntope commented May 24, 2024

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented May 24, 2024 • edited Loading

Codecov Report

ianlin left a comment • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

puntope commented May 27, 2024

puntope commented May 27, 2024

ianlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 24, 2024 •

edited

Loading

ianlin left a comment •

edited

Loading