-
Notifications
You must be signed in to change notification settings - Fork 21
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
Display error message if update_merchant_product_statuses job throws an error #2324
Conversation
…ay-error-job-mc-status
…ay-error-job-mc-status
…ror-job-mc-status
…ror-job-mc-status
…ror-job-mc-status
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this 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.
const refreshStats = async () => { | ||
await refreshProductStats(); | ||
invalidateResolution(); | ||
}; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
title={ __( 'Overview Stats:', 'google-listings-and-ads' ) } | ||
icon={ <ErrorIcon size={ 24 } /> } | ||
label={ | ||
<Button |
There was a problem hiding this comment.
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.
Thanks @ianlin for your suggestions!
Yes, that's right, I have updated it here: 986c6d2 Could you take another look? Thanks! |
There was a problem hiding this 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
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:
Detailed test instructions:
Backend job completed with an error message:
After this line:
google-listings-and-ads/src/MerchantCenter/MerchantStatuses.php
Line 140 in 065ff6d
admin.php?page=wc-admin&path=%2Fgoogle%2Fproduct-feed
and check that it shows the error.GET wc/gla/mc/product-statistics/refresh
request.AS Job - Failure rate is bigger than the threshold:
throw new \Exception( 'Error');
after this line:google-listings-and-ads/src/Jobs/UpdateMerchantProductStatuses.php
Lines 85 to 87 in 065ff6d
GET wc/gla/mc/product-statistics/refresh
update_merchant_product_statuses
The "update_merchant_product_statuses" job was stopped because its failure rate is above the allowed threshold.
admin.php?page=wc-admin&path=%2Fgoogle%2Fproduct-feed
, and see that it shows the error: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.
GET wc/gla/mc/product-statistics/refresh
and the stats will be available after the jobs are completed.Changelog entry