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.

Sign up for GitHub

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

Boost: Fix ISA showing an error if no report was found #40660

Merged
merged 9 commits into from
Dec 20, 2024
Merged

Boost: Fix ISA showing an error if no report was found #40660

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
21f6c1b
Fix boost API client not properly parsing errors returned by wpcom
dilirity Dec 18, 2024
4d7d40e
Fix showing error when a report wasn't found
dilirity Dec 18, 2024
3265984
Add changelogs
dilirity Dec 18, 2024
afa7179
Revert boost core changes
dilirity Dec 20, 2024
870e13d
remove changelog
dilirity Dec 20, 2024
82f63d1
Revert "Revert boost core changes"
dilirity Dec 20, 2024
2d65c5a
Revert "remove changelog"
dilirity Dec 20, 2024
d22ffed
Clarify comment
dilirity Dec 20, 2024
bd06af7
Merge branch 'trunk' into fix/boost/isa-showing-error-if-no-report-pr…
haqadn Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions projects/packages/boost-core/changelog/fix-not-parsing-some-wpcom-errors-properly
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

General: Fixed not parsing error responses from WordPress.com properly.
21 changes: 17 additions & 4 deletions projects/packages/boost-core/src/lib/class-utils.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,11 +133,24 @@ public static function send_wpcom_request( $method, $endpoint, $args = null, $bo
$code
);

/*
* WordPress.com passes statusCode and error back from Boost Cloud.
* WordPress.com ISA returns code and message.
*/
// phpcs:disable WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase
$err_code = empty( $data['statusCode'] ) ? 'http_error' : $data['statusCode'];
$message = empty( $data['error'] ) ? $default_message : $data['error'];

return new \WP_Error( $err_code, $message );
if ( isset( $data['statusCode'] ) && isset( $data['error'] ) ) {
// phpcs:disable WordPress.NamingConventions.ValidVariableName.UsedPropertyNotSnakeCase
$data_code = $data['statusCode'];
$data_message = $data['error'];
} elseif ( isset( $data['code'] ) && isset( $data['message'] ) ) {
$data_code = $data['code'];
$data_message = $data['message'];
}

$error_code = empty( $data_code ) ? 'http_error' : $data_code;
$message = empty( $data_message ) ? $default_message : $data_message;

return new \WP_Error( $error_code, $message );
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should make it consistent on the wpcom part instead.

Copy link
Member Author

@dilirity dilirity Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure this is going to be an easy fix. The endpoints for ISA work with the default WP functions that return code and message.

We'd have to update quite a few places to make it so it returns statusCode and error instead of code and message.

I think we might cause more harm than good.

I'm also not sure what exactly and where exactly to update it... I could remove this from the PR, since the only change is to fix the boost UI and I think the JS update fixes it.

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out that the change to boost core was necessary to figure out that the response is not-found and not a generic error.

Copy link
Member Author

Choose a reason for hiding this comment

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

Left a comment detailing why we need the band-aid I removed in afa7179.

p1734690571772729-slack-C016BBAFHHS

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added the band-aid back into Boost.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated (d22ffed) comment explaining error normalization.

}

return $data;
Expand Down
2 changes: 1 addition & 1 deletion .../assets/src/js/features/image-size-analysis/recommendations-meta/recommendations-meta.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ const RecommendationsMeta: React.FC< Props > = ( { isCdnActive } ) => {
isaRequest.isError;

const getErrorMessage = ( report: typeof isaReport ) => {
if ( report?.status === 'error' || report?.status === 'not-found' ) {
if ( report?.status === 'error' ) {
return report.message;
}

Expand Down
4 changes: 4 additions & 0 deletions projects/plugins/boost/changelog/fix-isa-showing-error-if-no-report-present
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: fixed

UI: Fixed showing an error if no ISA report was found.
Loading