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

Improve setup compatibility check when REST API disabled #2101

Closed jamesozzie closed 3 years ago

jamesozzie commented 4 years ago

Bug Description

Some users have plugins enabled which disable the WP REST API. While in most cases these plugins disable for non authenticated users only there are some plugins that perform this across the board. When this is identified there could be a notice during the compatibility check to inform users of this incompatibility.

Example experience of trying to activate Site Kit with the REST API disable. The "Checking compatibility" bar remains in a loading stage:

restapi

Console errors below:

/site1/wp-json/googl…-tag?_locale=user:1 Failed to load resource: the server responded with a status of 404 (Not Found)
googlesitekit-api.da…6da1ee23aecb8c.js:1 Google Site Kit API Error 
Object
additional_errors: []
code: "wp_die"
data: {status: 404}
message: "Page not found."
__proto__: Object
/site1/wp-json/googl…gin/?_locale=user:1 Failed to load resource: the server responded with a status of 404 (Not Found)
googlesitekit-api.da…6da1ee23aecb8c.js:1 Google Site Kit API Error 
Object
additional_errors: []
code: "wp_die"
data: {status: 404}
message: "Page not found."
__proto__: Object
googlesitekit-vendor…f97beee5846d93.js:1 Uncaught (in promise) 
Object
additional_errors: []
code: "wp_die"
data: {status: 404}
message: "Page not found."
__proto__: Object
  1. Install the Disable REST API for Real plugin (or any other plugin that disables the REST API)
  2. Attempt SK setup (or even try view the dashboards if already connected)

Additional Context


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

Acceptance criteria

Implementation Brief

QA Brief

  1. Install the Disable REST API for Real plugin (or any other plugin that disables the REST API)

  2. Attempt Site Kit setup

  3. See warning about REST API disabled image

  4. Now with the REST API disabling plugin deactivated, configure the wp-json/* requests to be blocked (e.g. using Chrome Network request blocking)

  5. Check the splash page again

  6. You should now see an alternate message about the staging environment image

Changelog entry

adamsilverstein commented 4 years ago

I tested this with the plugin you suggested and was able to reproduce the issue on the setup screen. We should be able to improve the experience here:

I also tried testing activating the plugin after setting up Site Kit. For the existing implementation, I saw pulsating boxes that never filled in; in our newer data layer implementation (still in development), I actually see an error message: "Data error in Search Console / Page not found." which matches the "page not found" response you get when this type of plugin is installed. I'm less concerned about addressing this case, since it will probably be obvious to users why Site Kit stopped working.

aaemnnosttv commented 3 years ago

Thanks @Hazlitte – a few points to address here:

edit assets/js/components/setup/CompatibilityChecks/constants.js and add a new constant: export const ERROR_API_ENABLED = 'check_api_enabled';

The error should be named in a way to indicate what is wrong (e.g. ERROR_INVALID_HOSTNAME or ERROR_FETCH_FAIL for those checks). So semantically we would want to throw the opposite "api disabled" error rather than "enabled". In this case, we don't really know if it is disabled or blocked for some other reason; maybe the site had an error, etc so we should make this more generic to indicate that the API is simply not available. Temporary or otherwise, this is a problem for Site Kit.

edit assets/js/components/setup/CompatibilityChecks/checks.js. Import the new constant ERROR_API_ENABLEDand update the checkHealthChecks function so that the catch() conditionally outputs a different error if the response is a 404

Similar to the first point, we should not scope this to a 404 error specifically. You're correct that we also want to account for the ERROR_FETCH_FAIL case here but apart from the fetch being blocked completely (i.e. fetch throws/catches) then any non-successful response should result in the API unavailable error.

Rather than checking the status, we should check if the caught error.code === 'fetch_error' (note catch blocks should always reference an error rather than an event). If it does, then throw ERROR_FETCH_FAIL, otherwise throw the API unavailable error since WP apiFetch will throw for any non-successful response.

edit assets/js/components/setup/CompatibilityChecks/index.js and change the order of the checks array returned by the createCompatibilityChecks function:

Good idea to use the existing call and make it the first one 👍


All-in-all, this is pretty close to ready, it just needs a few revisions 👍

Hazlitte commented 3 years ago

IB updated

aaemnnosttv commented 3 years ago

Thanks @Hazlitte – the IB is looking much better now, although it still references a 404 response in the second point.

I'll remove this for you since that seems to have been simply missed in the revision. Regarding the IB as a whole, we generally prefer the IB to be less copy-pasteable (if that makes sense). It reads a bit too much like a todo list rather than as an outline of the approach to use. It doesn't need to be rewritten, but please keep in mind for the future and of course let me know if you have any questions or ideas about IB writing 👍

I've updated your second point there accordingly, so this is good to go 👍

As for the estimate, 3 is generally what we use for the smallest issues since it also needs to account for CR + QA. In this case, I think we should allow a little more time for QA in particular since we might want to try different plugins/methods of blocking the REST API when testing, so I'm going to bump it up to a 7.

IB ✅

aaemnnosttv commented 3 years ago

@felixarntz added this to the release since it's a very small change and I basically QA'd it during CR.

wpdarren commented 3 years ago

QA Update: Pass ✅

Verified:

image

image

@aaemnnosttv thanks for these super useful instructions!!