Skip to content

Tweaks to generic API error messaging #1339

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
Open

Tweaks to generic API error messaging #1339

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3624a2c
Edit API client error message wording for syntax and redundancy
dechov Mar 14, 2018
bda979e
Only suggest checking server access if client receives 403
dechov Mar 30, 2018
5e6fae0
Omit HTTP status code from error, in favor of raw (if available) or g…
dechov Mar 30, 2018
687251d
Add an error message covering 502 (and similar) error response
dechov Mar 30, 2018
7dfe3a3
Revert unhelpful 403-specific message in client API module
dechov Apr 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 17 additions & 23 deletions classes/class-wc-connect-api-client.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,11 +372,14 @@ protected function request( $method, $path, $body = array() ) {
// If the received response is not JSON, return the raw response
$content_type = wp_remote_retrieve_header( $response, 'content-type' );
if ( false === strpos( $content_type, 'application/json' ) ) {
if ( 200 != $response_code ) {
return new WP_Error(
'wcc_server_error',
sprintf( 'Error: The WooCommerce Services server returned HTTP code: %d', $response_code )
);
if ( ! $response_code ) {
return new WP_Error( 'wcc_server_error', 'The WooCommerce Services API could not be reached.' );
} elseif ( 500 == $response_code ) {
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 ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return new WP_Error( 'wcc_server_error', 'The WooCommerce Services API could not process the request.' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

}
return $response;
}
Expand All @@ -386,31 +389,22 @@ protected function request( $method, $path, $body = array() ) {
$response_body = json_decode( $response_body );
}

if ( 200 != $response_code ) {
if ( 400 <= $response_code ) {
if ( empty( $response_body ) ) {
return new WP_Error(
'wcc_server_empty_response',
sprintf(
'Error: The WooCommerce Services server returned ( %d ) and an empty response body.',
$response_code
)
);
return new WP_Error( 'wcc_server_empty_response', 'The WooCommerce Services API could not process the request.' );
}

$error = property_exists( $response_body, 'error' ) ? $response_body->error : '';
$message = property_exists( $response_body, 'message' ) ? $response_body->message : '';
$data = property_exists( $response_body, 'data' ) ? $response_body->data : '';

return new WP_Error(
'wcc_server_error_response',
sprintf(
'Error: The WooCommerce Services server returned: %s %s ( %d )',
$error,
$message,
$response_code
),
$data
);
if ( 500 == $response_code ) {
return new WP_Error( 'wcc_server_error_response', 'The WooCommerce Services API encountered an error. Please try again.' );
} elseif ( 500 < $response_code ) {
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 );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep this DRY?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return $response_body;
Expand Down