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

dilirity · 2024-12-18T11:56:07Z

Fixes an error in the ISA description on freshly installed premium sites:

The reason this happens is because the UI makes a request for the latest report to WP.com. There's no report, and WP.com reports not-found for that report. Due to a fix of a bug in a previous PR, this started showing as an error since Boost 3.6.0 was released.

This changes the UI to ignore the not-found status from WP.com (just like it did prior to the fixing the above mentioned bug).

Proposed changes:

Update Boost API client to support responses from WordPress.com ISA endpoints;
Fix not-found reports showing as an error in the UI.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

p1734518619728809-slack-C016BBAFHHS

Does this pull request change what data or activity we track or use?

no

Testing instructions:

Test that you can see an error:

Setup Boost premium from trunk (or latest production version - 3.6.1);
Open the Boost dashboard - there should be an error under "Image Size Analysis" like the one from the screenshot above.

Test that the error is not showing:

Setup Boost premium (this branch);
Open the Boost dashboard. There should be no error under "Image Size Analysis".

User might not have requested an analysis yet. That doesn't mean the UI should show an error. This restores behavior from pre-Boost 3.6.0.

github-actions · 2024-12-18T11:57:02Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

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 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

Boost plugin:

Next scheduled release: none scheduled.

If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack.

haqadn

The fix works! Let's fix the error code inconsistency in wpcom instead of working around the inconsistency though.

haqadn · 2024-12-18T17:28:00Z

projects/packages/boost-core/src/lib/class-utils.php

+			/*
+			 * 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 );


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

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.

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.

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

p1734690571772729-slack-C016BBAFHHS

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

Updated (d22ffed) comment explaining error normalization.

This reverts commit afa7179.

This reverts commit 870e13d.

github-actions · 2024-12-20T10:34:02Z

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the fix/boost/isa-showing-error-if-no-report-present branch.

To test on Simple, run the following command on your sandbox:

bin/jetpack-downloader test jetpack fix/boost/isa-showing-error-if-no-report-present

Interested in more tips and information?

In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
Read more about our development workflow here: PCYsg-eg0-p2
Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

…esent

haqadn

LGTM

dilirity added 3 commits December 18, 2024 13:50

Fix boost API client not properly parsing errors returned by wpcom

21f6c1b

Fix showing error when a report wasn't found

4d7d40e

User might not have requested an analysis yet. That doesn't mean the UI should show an error. This restores behavior from pre-Boost 3.6.0.

Add changelogs

3265984

dilirity self-assigned this Dec 18, 2024

dilirity added [Status] In Progress [Plugin] Boost A feature to speed up the site and improve performance. [Package] Boost Core labels Dec 18, 2024

dilirity added this to the boost/next milestone Dec 18, 2024

dilirity requested a review from a team December 18, 2024 13:43

dilirity added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Team Review and removed [Status] In Progress labels Dec 18, 2024

github-actions bot added the [Status] In Progress label Dec 18, 2024

dilirity added [Type] Bug When a feature is broken and / or not performing as intended [Boost Feature] Image Size Analysis and removed [Type] Bug When a feature is broken and / or not performing as intended [Status] In Progress labels Dec 18, 2024

github-actions bot added the [Status] In Progress label Dec 18, 2024

dilirity marked this pull request as ready for review December 18, 2024 13:45

dilirity removed the [Status] In Progress label Dec 18, 2024

github-actions bot added the [Status] In Progress label Dec 18, 2024

haqadn requested changes Dec 18, 2024

View reviewed changes

dilirity added 4 commits December 20, 2024 12:12

Revert boost core changes

afa7179

remove changelog

870e13d

Revert "Revert boost core changes"

82f63d1

This reverts commit afa7179.

Revert "remove changelog"

2d65c5a

This reverts commit 870e13d.

Clarify comment

d22ffed

dilirity requested a review from haqadn December 20, 2024 14:09

dilirity added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Type] Bug When a feature is broken and / or not performing as intended [Status] In Progress labels Dec 20, 2024

Merge branch 'trunk' into fix/boost/isa-showing-error-if-no-report-pr…

bd06af7

…esent

haqadn approved these changes Dec 20, 2024

View reviewed changes

haqadn merged commit e564fe0 into trunk Dec 20, 2024
73 checks passed

haqadn deleted the fix/boost/isa-showing-error-if-no-report-present branch December 20, 2024 21:36

github-actions bot removed the [Status] Needs Team Review label Dec 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dilirity commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

haqadn left a comment

haqadn Dec 18, 2024

dilirity Dec 19, 2024 •

edited

Loading

dilirity Dec 20, 2024

dilirity Dec 20, 2024

dilirity Dec 20, 2024

dilirity Dec 20, 2024

github-actions bot commented Dec 20, 2024 •

edited

Loading

haqadn left a comment

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

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

Conversation

dilirity commented Dec 18, 2024 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Test that you can see an error:

Test that the error is not showing:

github-actions bot commented Dec 18, 2024 • edited Loading

haqadn left a comment

Choose a reason for hiding this comment

haqadn Dec 18, 2024

Choose a reason for hiding this comment

dilirity Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

dilirity Dec 20, 2024

Choose a reason for hiding this comment

dilirity Dec 20, 2024

Choose a reason for hiding this comment

dilirity Dec 20, 2024

Choose a reason for hiding this comment

dilirity Dec 20, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 20, 2024 • edited Loading

haqadn left a comment

Choose a reason for hiding this comment

dilirity commented Dec 18, 2024 •

edited

Loading

github-actions bot commented Dec 18, 2024 •

edited

Loading

dilirity Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading