Tweaks to generic API error messaging #1339

dechov · 2018-03-15T17:51:32Z

Aims to make sure error messaging is adequate in all cases where the server couldn't be reached or returns a 500 status.

Generic message

If the server couldn't be reached

Instead of:

...the API client responds with a more appropriate message — though if and how it shows up on the client depends on the context. Examples:

Edit: this now says:

The WooCommerce Services API encountered an error. Please try again.

The reference to the server is changed from "WooCommerce Services server" (kind of inelegant as a result of the renaming to "WooCommerce Services") to "WooCommerce Services API", based on a precedent in the connection test tool:

woocommerce-services/classes/class-wc-connect-debug-tools.php

Lines 28 to 35 in 3624a2c

    
           function test_connection() { 
        
               $test_request = $this->api_client->auth_test(); 
        
               if ( $test_request && ! is_wp_error( $test_request ) && $test_request->authorized ) { 
        
                   echo '<div class="updated inline"><p>' . __( 'Your site is succesfully communicating to the WooCommerce Services API.', 'woocommerce-services' ) . '</p></div>'; 
        
               } else { 
        
                   echo '<div class="error inline"><p>' . __( 'ERROR: Your site has a problem connecting to the WooCommerce Services API. Please make sure your Jetpack connection is working.', 'woocommerce-services' ) . '</p></div>'; 
        
               } 
        
           }

(Some discussion in p1521044361000197-slack-hydra of the suggestion to check whether requests can go out at all. If possible, we should consider only showing this if we have particular reason to believe this might be the case — perhaps only if no server requests have yet succeeded.)

Internal server error

If a 500 error comes back, there is generally no information specific to the error we can communicate via the client, so instead of:

...we can simplify the message:

Edit: other 5xx errors (such as 502 during server downtime) now yield:

The WooCommerce Services API is unavailable. Please try again in just a moment.

4xx errors

If available, the raw error message is passed through, without the HTTP status or meta-error message "Error: The WooCommerce Services server returned [...]":

Error communication per context

Manually tested all relevant cases by searching for $this->api_client-> in the codebase. Some e2e tests might be helpful so as to be confident errors are and will continue to be properly communicated.

To test global notices:

For internal server error message, can point payment proxy on local server to a URL where there is no server responding, and then trigger any of these:
- Label status fetch on order page load
- Label refund request
For unreachable server message, can set WOOCOMMERCE_CONNECT_SERVER_URL to a URL where there is no WCS server responding, and then trigger any of the above, or:
- Shipping rate fetch (on cart/checkout page, with debug mode enabled)
- Label rate fetch
- Label purchase request

~~TODO:~~

Show some error if manual service schema fetch fails

TaxJar proxy fails silently if WCS down. Might best be addressed by Output tax debug messages on the Cart and Checkout pages #1313

PPEC gives only an unhelpful message if WCS proxy is down: https://cloudup.com/c3OTHZxIMaj

Test print shows a print dialog with only a WP_Error in JSON form. Show a notice instead

Edit: now captured in #1364

~~Also:~~

~~On failure to fetch payment methods, might want to communicate more about how it failed~~

…eneric message

jeffstieler

Questions about message verbiage.

jeffstieler · 2018-04-20T21:04:48Z

classes/class-wc-connect-api-client.php

+				} elseif ( 500 < $response_code ) {
+					return new WP_Error( 'wcc_server_error', 'The WooCommerce Services API is unavailable. Please try again in just a moment.' );
+				} elseif ( 400 <= $response_code ) {
+					return new WP_Error( 'wcc_server_error', 'The WooCommerce Services API could not process the request.' );


Can you explain your reasoning for these error messages? (and the ones below)

At a glance it looks like all of these could be one of two messages - either "could not be reached" or "encountered an error".

Why do some not prompt the user to try again?

This is definitely wide open to revision but here's the understanding on which I based it:

No error code means the server wasn't reached at all, for reasons unrelated to WCS. Even though it may be something like a network hiccup, there's no WCS-specific reason to think trying again might help.

500 means something went wrong on the server, perhaps a bug in the server application, and there's no way to know whether trying again will help but there's nothing else we can suggest without manual investigation.

502 means the WCS server is down. If due to a deploy, it'll be back up within a few seconds, and otherwise, it'll be back up some time soon — trying again is the only thing you can do and it should eventually work.

4xx means there's something about the request the server can't or won't process — no need to try again with the same request.

I'm actually a little bit uneasy about showing "Please try again." for a 500 error over and over each time they try, since it could well be that it will never work. I think — ideally — this response would indicate a very exceptional case, in which contacting support would be justified.

Ok so how about we:

Remove "please try again" from all error messages.

Show The WooCommerce Services API encountered an error. for all 5xx errors

Leave no response as is (in this PR)

Restore 404 error but otherwise leave 4xx as is (in this PR)

@jeffstieler This had fallen off my radar – sorry about that.

I'm good with those changes except that I think "The WooCommerce Services API encountered an error." could be a little bit misleading, at least for the 502 case: as far as I understand, it's the HTTP server that "encountered an error", the error being that the WCS application hasn't properly started (yet?).

It seems worth communicating that it has nothing to do with content of the request itself. For 502 responses, proposing "The WooCommerce Services API is momentarily unavailable." – what do you think? If 👎, I'll go ahead with the change as you'd described.

jeffstieler · 2018-04-20T21:06:04Z

classes/class-wc-connect-api-client.php

+					return new WP_Error( 'wcc_server_error', 'The WooCommerce Services API encountered an error. Please try again.' );
+				} elseif ( 500 < $response_code ) {
+					return new WP_Error( 'wcc_server_error', 'The WooCommerce Services API is unavailable. Please try again in just a moment.' );
+				} elseif ( 400 <= $response_code ) {


I don't know how common/possible it would be in non-development environments, but I see 404s a lot on the order details page when switching between my local WCS server and staging/production.

This change removes the meaningful "not found" I saw before.

👍 Will bring back that string for 4xx responses at least.

jeffstieler · 2018-05-03T22:23:44Z

classes/class-wc-connect-api-client.php

+				} elseif ( 500 < $response_code ) {
+					return new WP_Error( 'wcc_server_error', 'The WooCommerce Services API is unavailable. Please try again in just a moment.' );
+				} elseif ( 400 <= $response_code ) {
+					return new WP_Error( 'wcc_server_error', 'The WooCommerce Services API could not process the request.' );


Ok so how about we:

Remove "please try again" from all error messages.

Show The WooCommerce Services API encountered an error. for all 5xx errors

Leave no response as is (in this PR)

Restore 404 error but otherwise leave 4xx as is (in this PR)

jeffstieler · 2018-05-03T22:23:56Z

classes/class-wc-connect-api-client.php

+					return new WP_Error( 'wcc_server_error_response', 'The WooCommerce Services API is unavailable. Please try again in just a moment.' );
+				}
+
+				return new WP_Error( 'wcc_server_error_response', $message, $data );


Can we keep this DRY?

Just to be sure, are you referring to the repetition of the code, the message, or both?

jeffstieler · 2018-05-14T21:46:26Z

Ping @dechov 👋

Edit API client error message wording for syntax and redundancy

3624a2c

dechov added the [Status] In Progress label Mar 15, 2018

dechov added 4 commits March 30, 2018 19:19

Only suggest checking server access if client receives 403

bda979e

Omit HTTP status code from error, in favor of raw (if available) or g…

5e6fae0

…eneric message

Add an error message covering 502 (and similar) error response

687251d

Revert unhelpful 403-specific message in client API module

7dfe3a3

dechov added [Status] Needs Review and removed [Status] In Progress labels Apr 9, 2018

jeffstieler suggested changes Apr 20, 2018

View reviewed changes

jeffstieler added [Status] Needs Author Reply and removed [Status] Needs Review labels Apr 20, 2018

jeffstieler suggested changes May 3, 2018

View reviewed changes

brezocordero changed the base branch from master to trunk April 21, 2021 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Tweaks to generic API error messaging #1339

Tweaks to generic API error messaging #1339

Uh oh!

dechov commented Mar 15, 2018 •

edited

Loading

Uh oh!

jeffstieler left a comment

Uh oh!

jeffstieler Apr 20, 2018

Uh oh!

dechov Apr 23, 2018

Uh oh!

jeffstieler May 3, 2018

Uh oh!

dechov May 16, 2018

Uh oh!

jeffstieler Apr 20, 2018

Uh oh!

dechov Apr 23, 2018

Uh oh!

jeffstieler May 3, 2018

Uh oh!

jeffstieler May 3, 2018

Uh oh!

dechov May 23, 2018

Uh oh!

jeffstieler commented May 14, 2018

Uh oh!

Uh oh!

	function test_connection() {
	$test_request = $this->api_client->auth_test();
	if ( $test_request && ! is_wp_error( $test_request ) && $test_request->authorized ) {
	echo '<div class="updated inline"><p>' . __( 'Your site is succesfully communicating to the WooCommerce Services API.', 'woocommerce-services' ) . '</p></div>';
	} else {
	echo '<div class="error inline"><p>' . __( 'ERROR: Your site has a problem connecting to the WooCommerce Services API. Please make sure your Jetpack connection is working.', 'woocommerce-services' ) . '</p></div>';
	}
	}

Tweaks to generic API error messaging #1339

Are you sure you want to change the base?

Tweaks to generic API error messaging #1339

Uh oh!

Conversation

dechov commented Mar 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Generic message

If the server couldn't be reached

Internal server error

4xx errors

Error communication per context

Uh oh!

jeffstieler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffstieler commented May 14, 2018

Uh oh!

Uh oh!

dechov commented Mar 15, 2018 •

edited

Loading