Add error class to checkout endpoint response

[hsingyuc]

Submission Review Guidelines:

• I have followed the WooCommerce Contributing Guidelines and the WordPress Coding Standards.
• I have checked to ensure there aren't other open Pull Requests for the same update/change.
• I have reviewed my code for security best practices.
• Following the above guidelines will result in quick merges and clear and detailed feedback when appropriate.

Changes proposed in this Pull Request:

I chose to use previous inside the additional data because we can't guarantee what interfaces we are getting on thrown exceptions from process_payment.

Alternative example that won't work::

catch ( $e ) {
    $exception_class = get_class( $e );
    // We can't guarantee what interface we get with the exception class, so the arguments might differ.
    $exception = new $exception_class( WC_Payments_Utils::get_filtered_error_message( $e, $blocked_by_fraud_rules ) 
    throw $exception;
);

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

1. Throw an error using the snippet below:

use Automattic\WooCommerce\StoreApi\Exceptions\RouteException;

add_action( 
	'woocommerce_rest_checkout_process_payment_with_context', 
	function() {
		throw new \Exception( 
			'woocommerce_test_exception',
			0,
			new RouteException(
				'woocommerce_test_previous',
				'exception message',
				0
			)
		);
	}
);

add_action( 'woocommerce_store_api_disable_nonce_check', '__return_true' ); // make sure to remove this after you are down with testing.

2. Add an item to your cart by POSTing to {YOUR_DOMAIN}/wp-json/wc/store/v1/cart/add-item

Body - Raw - JSON
{
  "id": 2040,
  "quantity": 1
}

3. Checkout with the cart by POSTing to {YOUR_DOMAIN}/wp-json/wc/store/v1/checkout

{
  "billing_address": {
        "first_name": "Test",
        "last_name": "Billing",
        "company": "",
        "address_1": "123 test",
        "address_2": "",
        "city": "San Diego",
        "state": "CA",
        "postcode": "92103",
        "country": "US",
        "email": "test@test.com",
        "phone": "+14708274627"
    },
    "shipping_address": {
        "first_name": "Test",
        "last_name": "Shipping",
        "company": "",
        "address_1": "123 test",
        "address_2": "",
        "city": "San Diego",
        "state": "CA",
        "postcode": "92103",
        "country": "US",
        "phone": "+14708274627"
    },
  "payment_method": "woocommerce_payments"
}

4. Should see

{
    "code": "woocommerce_rest_checkout_process_payment_error",
    "message": "woocommerce_test_exception",
    "data": {
        "previous": "Automattic\\WooCommerce\\StoreApi\\Exceptions\\RouteException",
        "status": 400
    }
}

Changelog entry

• Automatically create a changelog entry from the details below.

Significance

• Patch
• Minor
• Major

Type

• Fix - Fixes an existing bug
• Add - Adds functionality
• Update - Update existing functionality
• Dev - Development related task
• Tweak - A minor adjustment to the codebase
• Performance - Address performance issues
• Enhancement - Improvement to existing functionality

Message

Add previous error class to checkout endpoint response

Comment

[github-actions]

Test using WordPress Playground

The changes in this pull request can be previewed and tested using a WordPress Playground instance.
WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Test this pull request with WordPress Playground.

Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit.

[github-actions]

Hi @vedanshujain,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

[malithsen]

I've reviewed these changes along with the changes proposed in Automattic/woocommerce-payments#8824 and confirmed that it helps address the issue in WooPayments/WooPay.

Requesting changes due to an out-of-scope variable issue that this could run into, although I haven't been able to replicate it myself.

Let's also get a review from the core devs, team Proton.

[malithsen]

$additional_data is defined inside the branch. Line 94 will error out if the exception doesn't go into the condition, as the variable will be undefined.

[hsingyuc]

Thank you for catching that! Fixed here 86d1eef

[vedanshujain]

@hsingyuc this will send out the class name in the API response, which is usually an internal detail. Are we sure we want this, instead of just logging this or only sending out when Constants::is_true( 'WP_DEBUG' ) is true.

Additionally, the status code should be 500

[vedanshujain]

Closing and reopening the PR to trigger CI.

[hsingyuc]

@hsingyuc this will send out the class name in the API response, which is usually an internal detail. Are we sure we want this, instead of just logging this or only sending out when Constants::is_true( 'WP_DEBUG' ) is true.

@vedanshujain Thank you for the review! We'll use the response in WooPay, so we can't just log it or be on WP_DEBUG mode. Do you think there's another way to pass additional data for plugin consumption via the Store API response?

Additionally, the status code should be 500

This is the error code that was already there. Are you sure we should change it? This normally happens because of bad input and not because of a server error.

[vedanshujain]

We'll use the response in WooPay, so we can't just log it or be on WP_DEBUG mode. Do you think there's another way to pass additional data for plugin consumption via the Store API response?

I see, maybe we can send in some sort of error code, as prefix to the error message. So something like wc_pay_card_not_valid: Card provided is not valid in the error message? wdyt?

This normally happens because of bad input and not because of a server error.

Ah ok, in this case 400 is fine

[hsingyuc]

I see, maybe we can send in some sort of error code, as prefix to the error message. So something like wc_pay_card_not_valid: Card provided is not valid in the error message? wdyt?

I don't quite understand where we should add the prefix, can you expand on what you mean? Do you want this added to the exception message? This is shown to customers, so I'm not sure we should add it there.

The issue we are trying to solve is from this generic error message and error code. So, we updated Exceptions to more unique exception types and we pass this as the previous exception so we have data to be able to figure out where the error is coming from exactly.

We need to be careful to not change the original exception because this is shown to customers.

[vedanshujain]

LGTM and tests well, note that we are still discussing whether it's ok to expose the error class names. There's also a lint workflow that's failing, but other than these two things we should be good to merge.

[vedanshujain]

Also updated testing instructions to remove email from shipping, use correct quotes, use statement and disabling nonce check.