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

Display error message if update_merchant_product_statuses job throws an error #2324

Merged

Conversation

jorgemd24
Copy link
Contributor

@jorgemd24 jorgemd24 commented Mar 19, 2024

Changes proposed in this Pull Request:

Part of #2146 & #2250

Now that the product statistics are calculated using an AS job, there's a possibility of throwing errors during the calculation process. If an error occurs, we need to notify the merchant and provide them with the option to retry the calculation.

This PR adds:

  • The UI to inform the merchant about the error.
  • Gives the option to re-try/refresh the stats if an error happens.
  • Stops the UI of polling the results if the AS job failure rate is bigger than the threshold.

Detailed test instructions:

Backend job completed with an error message:

  1. Simulate an error, adding the following code:
  $this->mc_statuses['error']      = 'My error';
  $this->mc_statuses['statistics'] = null;
  return $this->mc_statuses;

After this line:

$this->mc_statuses = $this->container->get( TransientsInterface::class )->get( Transients::MC_STATUSES );

  1. Go to admin.php?page=wc-admin&path=%2Fgoogle%2Fproduct-feed and check that it shows the error.

image

  1. Delete the lines you inserted in step 1, open the Dev console (Netowork tab), then click "Click to retry."
  2. Check that it sends a GET wc/gla/mc/product-statistics/refresh request.
  3. The AppSpinner in the stats will be visible, and every 15 seconds, the UI will attempt to fetch the results. Once the calculation is finished, the stats should be displayed.

AS Job - Failure rate is bigger than the threshold:

  1. Add the following code throw new \Exception( 'Error'); after this line:

public function process_items( array $items ) {
try {
$next_page_token = $items['next_page_token'] ?? null;

  1. Make a request to GET wc/gla/mc/product-statistics/refresh
  2. Go to WC -> Status -> Action Scheduler -> Search for update_merchant_product_statuses
  3. Wait until it shows The "update_merchant_product_statuses" job was stopped because its failure rate is above the allowed threshold.
  4. Go to admin.php?page=wc-admin&path=%2Fgoogle%2Fproduct-feed, and see that it shows the error:
    image
  5. Clicking "Click to retry" will simply display the error again if the failure rate remains high. For testing purposes, you can change the failure rate timeframe using the below filter and remove the Exception added in step 1.
add_filter( 'woocommerce_gla_job_failure_timeframe', function(){
	return 1;
} );

Maybe as an improvement, we could consider displaying a message like "Retry after X time." However, this would require additional work to calculate the remaining time based on the average timeframe and threshold and I am not sure about the frequency of this error neither.

  1. See that a request is made to GET wc/gla/mc/product-statistics/refresh and the stats will be available after the jobs are completed.

Changelog entry

@github-actions 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 Mar 19, 2024
Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.2%. Comparing base (8b79fc1) to head (cbdcba1).
Report is 3 commits behind head on feature/refactor-product-stats.

Additional details and impacted files

Impacted file tree graph

@@                         Coverage Diff                         @@
##             feature/refactor-product-stats   #2324      +/-   ##
===================================================================
- Coverage                              62.7%   55.2%    -7.4%     
===================================================================
  Files                                   750     293     -457     
  Lines                                 21759    3740   -18019     
  Branches                                542     542              
===================================================================
- Hits                                  13638    2066   -11572     
+ Misses                                 7665    1218    -6447     
  Partials                                456     456              
Flag Coverage Δ
js-unit-tests 55.2% <100.0%> (+0.2%) ⬆️
php-unit-tests ?

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

Files Coverage Δ
js/src/hooks/useMCProductStatistics.js 73.7% <100.0%> (+7.0%) ⬆️
js/src/product-feed/product-statistics/index.js 70.0% <100.0%> (ø)
...t-statistics/status-box/sync-product-statistics.js 100.0% <100.0%> (ø)

... and 459 files with indirect coverage changes

@jorgemd24 jorgemd24 self-assigned this Mar 19, 2024
Base automatically changed from update/ui-loading-product-statuses to feature/refactor-product-stats March 20, 2024 12:11
@jorgemd24 jorgemd24 marked this pull request as ready for review March 20, 2024 16:19
@jorgemd24 jorgemd24 requested a review from a team March 20, 2024 16:19
Copy link
Contributor

@ianlin ianlin left a comment

Choose a reason for hiding this comment

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

Thanks for handling the job error and displaying it on the UI. It worked well from my testing. I left two comments that we could have discussions on.

Also one thing that in the UI the error message is The "update_merchant_product_statuses" job was stopped because its failure rate is above the allowed threshold. I'm not sure whether the merchant needs to know the detail that much, especially the job name. But the message is coming from AS not sure if there's any way we could modify it.

Comment on lines 36 to 39
const refreshStats = async () => {
await refreshProductStats();
invalidateResolution();
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could wrap this function by useCallback, as there are dependencies used in the function. But I'm not sure if it is a best practice tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ianlin for the suggestion, I believe it won't make much difference in terms of performance, but it won't hurt either, so I've added it here: 345d019

title={ __( 'Overview Stats:', 'google-listings-and-ads' ) }
icon={ <ErrorIcon size={ 24 } /> }
label={
<Button
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using AppButton so we can also add the eventName to it for tracking.

@jorgemd24
Copy link
Contributor Author

Thanks @ianlin for your suggestions!

I'm not sure whether the merchant needs to know the detail that much, especially the job name. But the message is coming from AS not sure if there's any way we could modify it.

Yes, that's right, I have updated it here: 986c6d2

Could you take another look? Thanks!

Copy link
Contributor

@ianlin ianlin left a comment

Choose a reason for hiding this comment

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

Thanks for the extra changes, LGTM. 👍

API PULL - Overview Stats - delete stale mc statuses
@jorgemd24 jorgemd24 merged commit fd022f8 into feature/refactor-product-stats Mar 25, 2024
14 checks passed
@jorgemd24 jorgemd24 deleted the add/display-error-job-mc-status branch March 25, 2024 14:48
@eason9487 eason9487 mentioned this pull request Mar 26, 2024
21 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants