Repository navigation
Premium Analytics: keep HTTP status on failed API requests - #50727
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
…status on invalid JSON
…api-error-status # Conflicts: # projects/packages/premium-analytics/widgets/devices/use-device-views.ts # projects/packages/premium-analytics/widgets/top-platforms/use-platform-views.ts
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 7 files. Only the first 5 are listed here.
2 files are newly checked for coverage.
|
layoutd
left a comment
There was a problem hiding this comment.
I tried the 404 manual testing instructions, but kept seeing more than two requests. I saw eight in total: the primary and comparison requests were each attempted four times.
From what I can tell, this happens because WordPress 7.0.x’s initSinglePage() does not run initModules (I tested with 7.0.2). Support for that was added to WordPress 7.1/trunk in this WordPress core sync.
The new error-status middleware therefore is not registered on WordPress 7.0.x, the 404 status is lost, and React Query retries each request three times.
Should we ensure the middleware is registered when running on WordPress 7.0.x as well?
| // so the error UI shows at once instead of after the query's retry backoff. | ||
| return Promise.reject( { | ||
| code: 'stats_mock_error', | ||
| error: 'unauthorized', |
There was a problem hiding this comment.
Could we update the remaining error fixtures too? widgets/stories/force-stats-mock-state.ts and the two video-detail tests still use the old { code, data.status } shape instead of { error, status }.
There was a problem hiding this comment.
Done. force-stats-mock-state.ts and both video-detail tests now use { error, status }
* refactor: share Stats widget error descriptions * fix: keep no_connection 403 retryable, gate hook errors, use full-sentence retry copy * test: add retryable error stories for the shared Stats error mapper
…ror-status Co-authored-by: Justin P <228780+layoutd@users.noreply.github.com>
WP 7.0's Core boot ignores initModules in initSinglePage(), so the app's init() module never runs there and the middleware was not installed. Register it next to the retry policy that reads the status instead.
…tatus' into worktree-wooa7s-1770-api-error-status # Conflicts: # projects/packages/premium-analytics/widgets/search-terms/render.tsx # projects/packages/premium-analytics/widgets/stories/force-stats-mock-state.ts # projects/packages/premium-analytics/widgets/top-platforms/render.tsx # projects/packages/premium-analytics/widgets/utm-insights/render.tsx
Good catch! #50309 is the right fix for |
Fixes WOOA7S-1770
Proposed changes
Premium Analytics uses the HTTP status from failed Stats API requests to decide whether to retry and whether to show authentication or server-error states. However,
@wordpress/api-fetchnormally throws only the parsed response body. WordPress.com Stats errors use an{ error, message }response body without a status field, so the data layer could not distinguish deterministic failures such as 401, 403, and 404 from transient failures.This PR:
apiFetchmiddleware that preserves the HTTP status when a failed response is thrown to the data layer.getApiErrorCode()to support the WordPress.com{ error, message }error envelope in addition to WordPress REST API error shapes.getStatsPlanErrorReason()and the plan-specific branches from the Devices and Top Platforms widgets. A 403unauthorizedresponse can represent either plan gating or insufficient permissions, so treating every such response as an upgrade requirement could show misleading upgrade messaging.Plan entitlement should be determined before making the request rather than inferred from an ambiguous API error. That work is tracked separately in WOOA7S-1716.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Automated tests
cd projects/packages/premium-analytics && pnpm test— covers the new error-status middleware, the{ error, message }envelope support ingetApiErrorCode(), and the retry policy.Manual: 404 retry behavior
/reports/downloads.stats/file-downloads.Before:
After:
