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.

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

Clarify proxy request error message #5038

Closed
felixarntz opened this issue Apr 5, 2022 · 10 comments
Closed

Clarify proxy request error message #5038

felixarntz opened this issue Apr 5, 2022 · 10 comments
Labels
P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Apr 5, 2022

When the proxy request to register the site or sync site fields fails, the plugin currently shows a generic message: The request to the authentication proxy has failed. Please, try again later.

This message lacks any detail about the actual error that occurred, so this should be enhanced.


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

Acceptance criteria

  • The error message The request to the authentication proxy has failed. Please, try again later. should be refined to include information about the actual error that occurred, if available:
    • If there is a WP_Error, the message should include detail from there as follows: The request to the authentication proxy has failed with an error: %s
    • The placeholder should be the WP_Error's error message if not empty, or its error code otherwise (as a fallback in case there's no error message).
    • In case the error is just due to the filter_var call, the original message can remain.

Implementation Brief

  • In includes/Core/Authentication/Setup.php, within the handle_action_setup_start() method:
    • Split out the condition that checks if $oauth_setup_redirect is a WP_Error.
    • If $oauth_setup_redirect is a WP_Error, get the error message using WP_Error::get_error_message() and if it is empty, get the error code WP_Error::get_error_code() and die with the message as per the AC.

Test Coverage

  • Add a PHPUnit test similar to Setup_Test::test_handle_action_setup_start__syncs_site_fields() that returns a WP_Error when a request to the Google Proxy server is made. It can be called test_handle_action_setup_start__dies_if_proxy_server_errors(). The logic to mock a WP_Error when an HTTP request is made can be similar to the AuthenticationTest::test_cron_update_remote_features__wp_erro() method.

QA Brief

  • To reproduce the error that this issue enhances:
    • Disconnect SiteKit and then navigate again to the Google SiteKit Dashboard. You should be on the "Setup SiteKit" splash screen with "Sign in with Google" as the CTA.
    • Disconnect your computer from the internet and then click the CTA (which will prevent the next immediate request being made to the proxy server). The request should fail with a WP Die / Error screen like below. Verify the error is as per the AC.
    Screenshot 2022-05-23 at 12 28 40

Changelog entry

  • Update the setup error screen to include an error message returned from the proxy server.
@felixarntz felixarntz added P1 Medium priority Type: Enhancement Improvement of an existing feature labels Apr 5, 2022
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Apr 5, 2022
@jimmymadon jimmymadon assigned jimmymadon and unassigned jimmymadon Apr 18, 2022
@eugene-manuilov eugene-manuilov self-assigned this Apr 26, 2022
@eugene-manuilov
Copy link
Collaborator

IB ✅

@eugene-manuilov eugene-manuilov removed their assignment Apr 26, 2022
@jimmymadon jimmymadon self-assigned this May 22, 2022
@jimmymadon jimmymadon removed their assignment May 23, 2022
@wpdarren wpdarren self-assigned this May 25, 2022
@wpdarren
Copy link
Collaborator

QA Update: ⚠️

@jimmymadon hello! I am unable to recreate the message:

Disconnect your computer from the internet and then click the CTA (which will prevent the next immediate request being made to the proxy server). The request should fail with a WP Die / Error screen like below. Verify the error is as per the AC.

I tried two ways, 1) disconnecting my internet connection and 2) going to the network tab in chrome and selecting offline in the throttling setting. All that happens is when I click on the CTA on the splash screen, it just changes to the no internet message on Chrome.

No Internet Try: Checking the network cables, modem and router. Reconnecting to Wi-Fi ERR_INTERNET_DISCONNECTED

Any other ideas on how I can generate this message to QA? 🤔

@wpdarren wpdarren assigned jimmymadon and unassigned jimmymadon May 25, 2022
@eclarke1 eclarke1 added the Rollover Issues which role over to the next sprint label Jun 6, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Jun 8, 2022

@mohitwp please could you have a look at this ticket since it is non-dashboard sharing. Read the QAB, and see if you can recreate the error message.

@wpdarren wpdarren assigned mohitwp and unassigned wpdarren Jun 8, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Jun 8, 2022

@wpdarren I tried to replicate issue following the steps mentioned under QAB.
I'm also redirecting to 'No internet page' after clicking on 'Sign in with Google' button. Tried on both 'revoke site kit' and 'setup sitekit' splash screen. I'm not getting any error as per AC.

@mohitwp mohitwp assigned jimmymadon and unassigned mohitwp Jun 10, 2022
@wpdarren wpdarren self-assigned this Jun 14, 2022
@wpdarren
Copy link
Collaborator

@jimmymadon @aaemnnosttv any ideas how we could trigger the error message? 🤔 I've tried a few different ways.

@jimmymadon
Copy link
Collaborator

@wpdarren I tried this again yesterday and was able to get the error after disabling the Wi-fi on my Mac and then immediately clicking on the CTA. For some reason, Chrome does not think I am offline when I do this. However, when I use the throttle feature in Chrome Dev Tools, it also shows me the "No Internet" Chrome Error page. At this stage, I think we could add the QA:Eng label and let an engineer test this by preventing a successful request using a Firewall rule / Proxy software. Even when using Proxyman, completely blocking the request causes a "Connection closed" Chrome error instead of the WP Error. Mocking a wp_error as a response to the request here is something I have already done locally - so not sure if doing this in QA:Eng is the right way to proceed.
@aaemnnosttv Any ideas from the top of your head on how else to trigger an error here?

@aaemnnosttv
Copy link
Collaborator

@jimmymadon the easiest way would be to use a filter to override the request to the proxy but this is only possible for requests that are made via WP HTTP (i.e. Google_Proxy::request) using https://developer.wordpress.org/reference/hooks/pre_http_request/. This covers almost all requests to the proxy except those that are done by the Google Client which uses Guzzle internally. So we should be able to create a mini plugin to force an error for requests to the proxy.

One option that just came to mind would be to use the WP_HTTP_BLOCK_EXTERNAL constant which will block all HTTP requests and return a WP_Error. This can be set in your wp-config with define( 'WP_HTTP_BLOCK_EXTERNAL', true );. You may need to setup SK first but this is the easiest way to force a WP Error for a request 👍 This constant also will not affect dashboard requests, again because the client uses Guzzle.

@wpdarren
Copy link
Collaborator

QA Update: ⚠️

@jimmymadon @aaemnnosttv going through changing the wp-config file with define( 'WP_HTTP_BLOCK_EXTERNAL', true ); I do see a similar error message but not as per the screenshot in the QAB.

image

To get this I had to disconnect Site Kit and get to the splash screen and then sign in with Google.

I was unable to trigger any error while Site Kit was connected.

@jimmymadon
Copy link
Collaborator

@wpdarren It is expected to have the error message change to whatever is returned in the message from WP_Error which now is "user blocking requests". It does pass the AC. I do not get the Query Monitor message in my tests as I do not have that plugin. So this should be sufficient.
Screenshot 2022-06-16 at 13 27 06

@jimmymadon jimmymadon removed their assignment Jun 16, 2022
@wpdarren
Copy link
Collaborator

QA Update: ✅

Going through changing the wp-config file with define( 'WP_HTTP_BLOCK_EXTERNAL', true ); I do see a similar error message as per the screenshot in the QAB.

image

@wpdarren wpdarren removed their assignment Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants