search

google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 291 forks source link

Enhance Site Kit health checks to display actual error encountered #5939

Open nfmohit opened 2 years ago

nfmohit commented 2 years ago

Feature Description

See https://github.com/google/site-kit-wp/pull/5906#discussion_r982843176

We currently have a couple of health checks in Site Kit which when failed, shows an error code from a pre-defined set of error codes. These error codes may come up as vague in some instances. It would be convenient if these health checks output the actual error message encountered.

As an example, if a site has blocked external requests in their site (e.g. via the WP_HTTP_BLOCK_EXTERNAL constant), Site Kit will not be able to communicate with the service, and thus, one of our health checks that validates a connection between the plugin and the service will fail. As it fails, it surfaces the following error:

image

The paragraph conveys a vague message mentioning a "technical issue" and provides an error code. However, as discussed previously in https://github.com/google/site-kit-wp/pull/5906#discussion_r982843176, it would be good to actually provide the real error message here in addition to error our code as it would be much more useful, both for the user and our support team. In this scenario, the message would also include the actual error message, i.e. User has blocked requests through HTTP.

Steps to reproduce

  1. On a fresh site, add the following to the wp-config.php file: define( 'WP_HTTP_BLOCK_EXTERNAL', true );.
  2. Install Site Kit.
  3. Try to connect Site Kit (i.e. visit the SK screen).
  4. See that the connection fails at the health check step with the above mentioned error.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

aaemnnosttv commented 1 year ago

@nfmohit would you please elaborate on the scenario here? In which case do we not have sufficient context for the check?

mxbclang commented 11 months ago

@nfmohit This issue is over a year old now – checking to see if you can provide more detail here?

nfmohit commented 10 months ago

@nfmohit This issue is over a year old now – checking to see if you can provide more detail here?

Hi @bethanylang. Apologies, I got occupied with higher priorities and this got lost in the cracks. I've updated the description.

@nfmohit would you please elaborate on the scenario here? In which case do we not have sufficient context for the check?

Hi @aaemnnosttv. I have updated the description with further details on the issue. Please let me know if this looks good now, thank you!