From 7067cd04d66baa0ae7a7f6183ec303999784e9ff Mon Sep 17 00:00:00 2001 From: James Rodger Date: Thu, 6 Feb 2020 12:39:23 +0000 Subject: [PATCH 1/9] Extract DB access from Stripe API client class This logic is required by other parts of the code base, so extracting it out allows it to be shared. The unit tests have been left as is for now, but this also allows for database access to be mocked more easily. --- includes/class-wc-payments-db.php | 49 +++++++++++++++++++ includes/class-wc-payments.php | 13 ++++- .../class-wc-payments-api-client.php | 49 +++++-------------- tests/bootstrap.php | 1 + .../test-class-wc-payments-api-client.php | 14 +++++- 5 files changed, 86 insertions(+), 40 deletions(-) create mode 100644 includes/class-wc-payments-db.php diff --git a/includes/class-wc-payments-db.php b/includes/class-wc-payments-db.php new file mode 100644 index 00000000000..f2e33a80e3d --- /dev/null +++ b/includes/class-wc-payments-db.php @@ -0,0 +1,49 @@ +order_id_from_charge_id( $charge_id ); + + if ( $order_id ) { + return wc_get_order( $order_id ); + } + return false; + } + + /** + * Retrieve an order ID from the DB using a corresponding Stripe charge ID. + * + * @param string $charge_id Charge ID corresponding to an order ID. + * + * @return null|string + */ + private function order_id_from_charge_id( $charge_id ) { + global $wpdb; + + // The order ID is saved to DB in `WC_Payment_Gateway_WCPay::process_payment()`. + $order_id = $wpdb->get_var( + $wpdb->prepare( + "SELECT DISTINCT ID FROM $wpdb->posts as posts LEFT JOIN $wpdb->postmeta as meta ON posts.ID = meta.post_id WHERE meta.meta_value = %s AND meta.meta_key = '_charge_id'", + $charge_id + ) + ); + return $order_id; + } +} diff --git a/includes/class-wc-payments.php b/includes/class-wc-payments.php index 22f15263582..e6c24500d57 100644 --- a/includes/class-wc-payments.php +++ b/includes/class-wc-payments.php @@ -30,6 +30,13 @@ class WC_Payments { */ private static $api_client; + /** + * Instance of WC_Payments_DB. + * + * @var WC_Payments_DB + */ + private static $db_helper; + /** * Instance of WC_Payments_Account, created in init function. * @@ -63,6 +70,9 @@ public static function init() { add_filter( 'plugin_action_links_' . plugin_basename( WCPAY_PLUGIN_FILE ), array( __CLASS__, 'add_plugin_links' ) ); + include_once dirname( __FILE__ ) . '/class-wc-payments-db.php'; + self::$db_helper = new WC_Payments_DB(); + self::$api_client = self::create_api_client(); include_once dirname( __FILE__ ) . '/class-wc-payments-account.php'; @@ -431,7 +441,8 @@ public static function create_api_client() { $payments_api_client = new WC_Payments_API_Client( 'WooCommerce Payments/' . WCPAY_VERSION_NUMBER, - new WC_Payments_Http() + new WC_Payments_Http(), + self::$db_helper ); return $payments_api_client; diff --git a/includes/wc-payment-api/class-wc-payments-api-client.php b/includes/wc-payment-api/class-wc-payments-api-client.php index 5cd9dc7dbd7..8ef4f1098da 100644 --- a/includes/wc-payment-api/class-wc-payments-api-client.php +++ b/includes/wc-payment-api/class-wc-payments-api-client.php @@ -46,15 +46,24 @@ class WC_Payments_API_Client { */ private $http_client; + /** + * DB access wrapper. + * + * @var WC_Payments_DB + */ + private $wcpay_db; + /** * WC_Payments_API_Client constructor. * * @param string $user_agent - User agent string to report in requests. * @param WC_Payments_Http $http_client - Used to send HTTP requests. + * @param WC_Payments_DB $wcpay_db - DB access wrapper. */ - public function __construct( $user_agent, $http_client ) { + public function __construct( $user_agent, $http_client, $wcpay_db ) { $this->user_agent = $user_agent; $this->http_client = $http_client; + $this->wcpay_db = $wcpay_db; } /** @@ -189,42 +198,6 @@ public function cancel_intention( $intention_id ) { return $this->deserialize_intention_object_from_array( $response_array ); } - /** - * Retrive an order ID from the DB using a corresponding Stripe charge ID. - * - * @param string $charge_id Charge ID corresponding to an order ID. - * - * @return null|string - */ - private function order_id_from_charge_id( $charge_id ) { - global $wpdb; - - // The order ID is saved to DB in `WC_Payment_Gateway_WCPay::process_payment()`. - $order_id = $wpdb->get_var( - $wpdb->prepare( - "SELECT DISTINCT ID FROM $wpdb->posts as posts LEFT JOIN $wpdb->postmeta as meta ON posts.ID = meta.post_id WHERE meta.meta_value = %s AND meta.meta_key = '_charge_id'", - $charge_id - ) - ); - return $order_id; - } - - /** - * Retrieve an order from the DB using a corresponding Stripe charge ID. - * - * @param string $charge_id Charge ID corresponding to an order ID. - * - * @return boolean|WC_Order|WC_Order_Refund - */ - private function order_from_charge_id( $charge_id ) { - $order_id = $this->order_id_from_charge_id( $charge_id ); - - if ( $order_id ) { - return wc_get_order( $order_id ); - } - return false; - } - /** * List deposits * @@ -667,7 +640,7 @@ protected function extract_response_body( $response ) { * @return array new object with order information. */ private function add_order_info_to_object( $charge_id, $object ) { - $order = $this->order_from_charge_id( $charge_id ); + $order = $this->wcpay_db->order_from_charge_id( $charge_id ); // Add order information to the `$transaction`. // If the order couldn't be retrieved, return an empty order. diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 0ce9006745b..cae376d49ab 100755 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -33,6 +33,7 @@ function _manually_load_plugin() { require dirname( dirname( __FILE__ ) ) . '/woocommerce-payments.php'; + require_once dirname( __FILE__ ) . '/../includes/class-wc-payments-db.php'; require_once dirname( __FILE__ ) . '/../includes/wc-payment-api/models/class-wc-payments-api-charge.php'; require_once dirname( __FILE__ ) . '/../includes/wc-payment-api/models/class-wc-payments-api-intention.php'; require_once dirname( __FILE__ ) . '/../includes/wc-payment-api/class-wc-payments-api-client.php'; diff --git a/tests/wc-payment-api/test-class-wc-payments-api-client.php b/tests/wc-payment-api/test-class-wc-payments-api-client.php index 31f36f414d4..2a0f4d7608a 100644 --- a/tests/wc-payment-api/test-class-wc-payments-api-client.php +++ b/tests/wc-payment-api/test-class-wc-payments-api-client.php @@ -24,6 +24,13 @@ class WC_Payments_API_Client_Test extends WP_UnitTestCase { */ private $mock_http_client; + /** + * Mock DB wrapper. + * + * @var WC_Payments_DB|PHPUnit_Framework_MockObject_MockObject + */ + private $mock_db_wrapper; + /** * Pre-test setup */ @@ -35,9 +42,14 @@ public function setUp() { ->setMethods( array( 'remote_request' ) ) ->getMock(); + $this->mock_db_wrapper = $this->getMockBuilder( 'WC_Payments_DB' ) + ->disableOriginalConstructor() + ->getMock(); + $this->payments_api_client = new WC_Payments_API_Client( 'Unit Test Agent/0.1.0', - $this->mock_http_client + $this->mock_http_client, + $this->mock_db_wrapper ); } From ee9c7e2db7f6fb7a8f40fcb9362c3c6679db749b Mon Sep 17 00:00:00 2001 From: James Rodger Date: Thu, 6 Feb 2020 12:40:49 +0000 Subject: [PATCH 2/9] Add REST endpoint for handling webhooks from WP.com The initial implementation just handles refund updated events and adds an order note in the event of a failure. --- ...ss-wc-rest-payments-webhook-controller.php | 148 ++++++++++ includes/class-wc-payments.php | 5 + .../class-wc-payments-rest-exception.php | 17 ++ .../test-class-wc-rest-payments-webhook.php | 258 ++++++++++++++++++ tests/bootstrap.php | 4 + 5 files changed, 432 insertions(+) create mode 100644 includes/admin/class-wc-rest-payments-webhook-controller.php create mode 100644 includes/exceptions/class-wc-payments-rest-exception.php create mode 100644 tests/admin/test-class-wc-rest-payments-webhook.php diff --git a/includes/admin/class-wc-rest-payments-webhook-controller.php b/includes/admin/class-wc-rest-payments-webhook-controller.php new file mode 100644 index 00000000000..d468ef7f15a --- /dev/null +++ b/includes/admin/class-wc-rest-payments-webhook-controller.php @@ -0,0 +1,148 @@ +wcpay_db = $wcpay_db; + } + + /** + * Configure REST API routes. + */ + public function register_routes() { + register_rest_route( + $this->namespace, + '/' . $this->rest_base, + array( + 'methods' => WP_REST_Server::CREATABLE, + 'callback' => array( $this, 'handle_webhook' ), + 'permission_callback' => array( $this, 'check_permission' ), + ) + ); + } + + /** + * Retrieve transactions to respond with via API. + * + * @param WP_REST_Request $request Full data about the request. + * + * @return WP_REST_Response + */ + public function handle_webhook( $request ) { + $body = $request->get_json_params(); + + try { + // Extract information about the webhook event. + $event_type = $this->read_property( $body, 'type' ); + $event_data = $this->read_property( $body, 'data' ); + $event_object = $this->read_property( $event_data, 'object' ); + + switch ( $event_type ) { + case 'charge.refund.updated': + $this->process_webhook_refund_updated( $event_object ); + break; + } + } catch ( WC_Payments_Rest_Exception $e ) { + Logger::error( $e ); + return new WP_REST_Response( array( 'result' => self::RESULT_BAD_REQUEST ), 400 ); + } catch ( Exception $e ) { + Logger::error( $e ); + return new WP_REST_Response( array( 'result' => self::RESULT_ERROR ), 500 ); + } + + return new WP_REST_Response( array( 'result' => self::RESULT_SUCCESS ) ); + } + + /** + * Process webhook refund updated. + * + * @param array $event_object The event that triggered the webhook. + * + * @throws WC_Payments_Rest_Exception Required parameters not found. + * @throws Exception Unable to resolve charge ID to order. + */ + private function process_webhook_refund_updated( $event_object ) { + // First, check the reason for the update. We're only interesting in a status of failed. + $status = $this->read_property( $event_object, 'status' ); + if ( 'failed' !== $status ) { + return; + } + + // Fetch the details of the failed refund so that we can find the associated order and write a note. + $charge_id = $this->read_property( $event_object, 'charge' ); + $refund_id = $this->read_property( $event_object, 'id' ); + $amount = $this->read_property( $event_object, 'amount' ); + + // Look up the order related to this charge. + $order = $this->wcpay_db->order_from_charge_id( $charge_id ); + if ( ! $order ) { + throw new Exception( 'Could not find order via charge ID: ' . $charge_id ); + } + + $note = sprintf( + /* translators: %1: the refund amount, %2: ID of the refund */ + __( 'A refund of %1$s was unsuccessful using WooCommerce Payments (%2$s).', 'woocommerce-payments' ), + wc_price( $amount / 100 ), + $refund_id + ); + $order->add_order_note( $note ); + } + + /** + * Get a property from a refund event. + * + * @param array $event_object Event object to read from. + * @param string $key Name of property to get. + * + * @return string|array + * @throws WC_Payments_Rest_Exception Thrown if property not found. + */ + private function read_property( $event_object, $key ) { + if ( ! isset( $event_object[ $key ] ) ) { + throw new WC_Payments_Rest_Exception( $key . ' property not found in refund event' ); + } + return $event_object[ $key ]; + } +} diff --git a/includes/class-wc-payments.php b/includes/class-wc-payments.php index e6c24500d57..c1001fd7473 100644 --- a/includes/class-wc-payments.php +++ b/includes/class-wc-payments.php @@ -452,6 +452,7 @@ public static function create_api_client() { * Initialize the REST API controllers. */ public static function init_rest_api() { + include_once WCPAY_ABSPATH . 'includes/exceptions/class-wc-payments-rest-exception.php'; include_once WCPAY_ABSPATH . 'includes/admin/class-wc-payments-rest-controller.php'; include_once WCPAY_ABSPATH . 'includes/admin/class-wc-rest-payments-deposits-controller.php'; @@ -473,6 +474,10 @@ public static function init_rest_api() { include_once WCPAY_ABSPATH . 'includes/admin/class-wc-rest-payments-timeline-controller.php'; $timeline_controller = new WC_REST_Payments_Timeline_Controller( self::$api_client ); $timeline_controller->register_routes(); + + include_once WCPAY_ABSPATH . 'includes/admin/class-wc-rest-payments-webhook-controller.php'; + $webhook_controller = new WC_REST_Payments_Webhook_Controller( self::$api_client, self::$db_helper ); + $webhook_controller->register_routes(); } /** diff --git a/includes/exceptions/class-wc-payments-rest-exception.php b/includes/exceptions/class-wc-payments-rest-exception.php new file mode 100644 index 00000000000..25d4b203d23 --- /dev/null +++ b/includes/exceptions/class-wc-payments-rest-exception.php @@ -0,0 +1,17 @@ +getMockBuilder( WC_Payments_API_Client::class ) + ->disableOriginalConstructor() + ->getMock(); + + $this->mock_db_wrapper = $this->getMockBuilder( WC_Payments_DB::class ) + ->disableOriginalConstructor() + ->setMethods( array( 'order_from_charge_id' ) ) + ->getMock(); + + $this->controller = new WC_REST_Payments_Webhook_Controller( $mock_api_client, $this->mock_db_wrapper ); + + // Setup a test request. + $this->request = new WP_REST_Request( + 'POST', + '/wc/v3/payments/webhook' + ); + + $this->request->set_header( 'Content-Type', 'application/json' ); + + // Build the test request data. + $event_object = array(); + + $event_data = array(); + $event_data['object'] = $event_object; + + $this->request_body = array(); + $this->request_body['data'] = $event_data; + } + + /** + * Test processing a webhook that requires no action. + */ + public function test_noop_webhook() { + // TODO: Test unauthenticated user - could put on base class? + // Setup test request data. + $this->request_body['type'] = 'unknown.webhook.event'; + $this->request->set_body( wp_json_encode( $this->request_body ) ); + + // Run the test. + $response = $this->controller->handle_webhook( $this->request ); + + // Check the response. + $response_data = $response->get_data(); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertEquals( array( 'result' => 'success' ), $response_data ); + } + + /** + * Test a webhook with no type property. + */ + public function test_webhook_with_no_type_property() { + // Setup test request data. + $this->request->set_body( wp_json_encode( $this->request_body ) ); + + // Run the test. + $response = $this->controller->handle_webhook( $this->request ); + + // Check the response. + $response_data = $response->get_data(); + + $this->assertEquals( 400, $response->get_status() ); + $this->assertEquals( array( 'result' => 'bad_request' ), $response_data ); + } + + /** + * Test a webhook with no object property. + */ + public function test_webhook_with_no_object_property() { + // Setup test request data. + $this->request_body['type'] = 'unknown.webhook.event'; + unset( $this->request_body['data']['object'] ); + $this->request->set_body( wp_json_encode( $this->request_body ) ); + + // Run the test. + $response = $this->controller->handle_webhook( $this->request ); + + // Check the response. + $response_data = $response->get_data(); + + $this->assertEquals( 400, $response->get_status() ); + $this->assertEquals( array( 'result' => 'bad_request' ), $response_data ); + } + + /** + * Test a webhook with no object property. + */ + public function test_webhook_with_no_data_property() { + // Setup test request data. + $this->request_body['type'] = 'unknown.webhook.event'; + unset( $this->request_body['data'] ); + $this->request->set_body( wp_json_encode( $this->request_body ) ); + + // Run the test. + $response = $this->controller->handle_webhook( $this->request ); + + // Check the response. + $response_data = $response->get_data(); + + $this->assertEquals( 400, $response->get_status() ); + $this->assertEquals( array( 'result' => 'bad_request' ), $response_data ); + } + + /** + * Test a valid failed refund update webhook. + */ + public function test_valid_failed_refund_update_webhook() { + // Setup test request data. + $this->request_body['type'] = 'charge.refund.updated'; + $this->request_body['data']['object'] = array( + 'status' => 'failed', + 'charge' => 'test_charge_id', + 'id' => 'test_refund_id', + 'amount' => 999, + ); + + $this->request->set_body( wp_json_encode( $this->request_body ) ); + + $mock_order = $this->getMockBuilder( WC_Order::class ) + ->disableOriginalConstructor() + ->setMethods( array( 'add_order_note' ) ) + ->getMock(); + + $mock_order + ->expects( $this->once() ) + ->method( 'add_order_note' ) + ->with( + 'A refund of £9.99 was unsuccessful using WooCommerce Payments (test_refund_id).' + ); + + $this->mock_db_wrapper + ->expects( $this->once() ) + ->method( 'order_from_charge_id' ) + ->with( 'test_charge_id' ) + ->willReturn( $mock_order ); + + // Run the test. + $response = $this->controller->handle_webhook( $this->request ); + + // Check the response. + $response_data = $response->get_data(); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertEquals( array( 'result' => 'success' ), $response_data ); + } + + /** + * Test a valid failed refund update webhook with an unknown charge ID. + */ + public function test_valid_failed_refund_update_webhook_with_unknown_charge_id() { + // Setup test request data. + $this->request_body['type'] = 'charge.refund.updated'; + $this->request_body['data']['object'] = array( + 'status' => 'failed', + 'charge' => 'unknown_charge_id', + 'id' => 'test_refund_id', + 'amount' => 999, + ); + + $this->request->set_body( wp_json_encode( $this->request_body ) ); + + $this->mock_db_wrapper + ->expects( $this->once() ) + ->method( 'order_from_charge_id' ) + ->with( 'unknown_charge_id' ) + ->willReturn( false ); + + // Run the test. + $response = $this->controller->handle_webhook( $this->request ); + + // Check the response. + $response_data = $response->get_data(); + + $this->assertEquals( 500, $response->get_status() ); + $this->assertEquals( array( 'result' => 'error' ), $response_data ); + } + + /** + * Test a valid non-failed refund update webhook + */ + public function test_non_failed_refund_update_webhook() { + // Setup test request data. + $this->request_body['type'] = 'charge.refund.updated'; + $this->request_body['data']['object'] = array( + 'status' => 'updated', + 'charge' => 'test_charge_id', + 'id' => 'test_refund_id', + 'amount' => 999, + ); + + $this->request->set_body( wp_json_encode( $this->request_body ) ); + + $this->mock_db_wrapper + ->expects( $this->never() ) + ->method( 'order_from_charge_id' ); + + // Run the test. + $response = $this->controller->handle_webhook( $this->request ); + + // Check the response. + $response_data = $response->get_data(); + + $this->assertEquals( 200, $response->get_status() ); + $this->assertEquals( array( 'result' => 'success' ), $response_data ); + } +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php index cae376d49ab..d391f3285a6 100755 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -38,6 +38,10 @@ function _manually_load_plugin() { require_once dirname( __FILE__ ) . '/../includes/wc-payment-api/models/class-wc-payments-api-intention.php'; require_once dirname( __FILE__ ) . '/../includes/wc-payment-api/class-wc-payments-api-client.php'; require_once dirname( __FILE__ ) . '/../includes/wc-payment-api/class-wc-payments-http.php'; + + require_once dirname( __FILE__ ) . '/../includes/exceptions/class-wc-payments-rest-exception.php'; + require_once dirname( __FILE__ ) . '/../includes/admin/class-wc-payments-rest-controller.php'; + require_once dirname( __FILE__ ) . '/../includes/admin/class-wc-rest-payments-webhook-controller.php'; } tests_add_filter( 'muplugins_loaded', '_manually_load_plugin' ); From 9eef7881d1476aeef2e4d13e906cc63b29a8196d Mon Sep 17 00:00:00 2001 From: James Rodger Date: Wed, 1 Apr 2020 18:24:12 +0100 Subject: [PATCH 3/9] Turn off Squiz.Commenting.FunctionCommentThrowTag.WrongNumber PHPCS rule This rule is generating some false positives. We can try turning it on again if it gets fixed in a later PHPCS version. Alternatively we could start relying on a static code analyser for more robust checks on how we're using exceptions. --- phpcs.xml.dist | 3 +++ 1 file changed, 3 insertions(+) diff --git a/phpcs.xml.dist b/phpcs.xml.dist index feb927d75c2..ad7c01004ae 100644 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -16,6 +16,9 @@ + + + From 51719b4378f7df2c7b4fb25b776064d3d66b0a21 Mon Sep 17 00:00:00 2001 From: James Rodger Date: Tue, 21 Apr 2020 11:08:44 +0100 Subject: [PATCH 4/9] Fix whitespace in phpcs.xml.dist --- phpcs.xml.dist | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpcs.xml.dist b/phpcs.xml.dist index ad7c01004ae..cc240b877f5 100644 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -18,7 +18,7 @@ - + From 0c02b1ede57da3c5da1437425d0ac98a1d046ecf Mon Sep 17 00:00:00 2001 From: James Rodger Date: Tue, 21 Apr 2020 11:14:08 +0100 Subject: [PATCH 5/9] Clean up webhook controller test class --- tests/admin/test-class-wc-rest-payments-webhook.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/tests/admin/test-class-wc-rest-payments-webhook.php b/tests/admin/test-class-wc-rest-payments-webhook.php index 594be695dd9..775ae394533 100644 --- a/tests/admin/test-class-wc-rest-payments-webhook.php +++ b/tests/admin/test-class-wc-rest-payments-webhook.php @@ -24,11 +24,6 @@ class WC_REST_Payments_Webhook_Controller_Test extends WP_UnitTestCase { */ private $mock_db_wrapper; - /** - * @var WP_REST_Server - */ - private $server; - /** * @var WP_REST_Request */ @@ -48,7 +43,6 @@ public function setUp() { // Set the user so that we can pass the authentication. wp_set_current_user( 1 ); - // TODO: Remove this as a requirement? /** @var WC_Payments_API_Client|MockObject $mock_api_client */ $mock_api_client = $this->getMockBuilder( WC_Payments_API_Client::class ) ->disableOriginalConstructor() @@ -83,7 +77,6 @@ public function setUp() { * Test processing a webhook that requires no action. */ public function test_noop_webhook() { - // TODO: Test unauthenticated user - could put on base class? // Setup test request data. $this->request_body['type'] = 'unknown.webhook.event'; $this->request->set_body( wp_json_encode( $this->request_body ) ); From 3b484371c48634c4a810a0b60eeb41a5ae4763ea Mon Sep 17 00:00:00 2001 From: James Rodger Date: Tue, 21 Apr 2020 11:15:10 +0100 Subject: [PATCH 6/9] Tweak naming of read_property function --- ...ss-wc-rest-payments-webhook-controller.php | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/includes/admin/class-wc-rest-payments-webhook-controller.php b/includes/admin/class-wc-rest-payments-webhook-controller.php index d468ef7f15a..5fab86a445f 100644 --- a/includes/admin/class-wc-rest-payments-webhook-controller.php +++ b/includes/admin/class-wc-rest-payments-webhook-controller.php @@ -75,9 +75,9 @@ public function handle_webhook( $request ) { try { // Extract information about the webhook event. - $event_type = $this->read_property( $body, 'type' ); - $event_data = $this->read_property( $body, 'data' ); - $event_object = $this->read_property( $event_data, 'object' ); + $event_type = $this->read_rest_property( $body, 'type' ); + $event_data = $this->read_rest_property( $body, 'data' ); + $event_object = $this->read_rest_property( $event_data, 'object' ); switch ( $event_type ) { case 'charge.refund.updated': @@ -105,15 +105,15 @@ public function handle_webhook( $request ) { */ private function process_webhook_refund_updated( $event_object ) { // First, check the reason for the update. We're only interesting in a status of failed. - $status = $this->read_property( $event_object, 'status' ); + $status = $this->read_rest_property( $event_object, 'status' ); if ( 'failed' !== $status ) { return; } // Fetch the details of the failed refund so that we can find the associated order and write a note. - $charge_id = $this->read_property( $event_object, 'charge' ); - $refund_id = $this->read_property( $event_object, 'id' ); - $amount = $this->read_property( $event_object, 'amount' ); + $charge_id = $this->read_rest_property( $event_object, 'charge' ); + $refund_id = $this->read_rest_property( $event_object, 'id' ); + $amount = $this->read_rest_property( $event_object, 'amount' ); // Look up the order related to this charge. $order = $this->wcpay_db->order_from_charge_id( $charge_id ); @@ -131,18 +131,24 @@ private function process_webhook_refund_updated( $event_object ) { } /** - * Get a property from a refund event. + * Safely get a value from the REST request body array. * - * @param array $event_object Event object to read from. - * @param string $key Name of property to get. + * @param array $array Array to read from. + * @param string $key ID to fetch on. * * @return string|array - * @throws WC_Payments_Rest_Exception Thrown if property not found. + * @throws WC_Payments_Rest_Exception Thrown if ID not set. */ - private function read_property( $event_object, $key ) { - if ( ! isset( $event_object[ $key ] ) ) { - throw new WC_Payments_Rest_Exception( $key . ' property not found in refund event' ); + private function read_rest_property( $array, $key ) { + if ( ! isset( $array[ $key ] ) ) { + throw new WC_Payments_Rest_Exception( + sprintf( + /* translators: %1: ID being fetched */ + __( '%1$s not found in array', 'woocommerce-payments' ), + $key + ) + ); } - return $event_object[ $key ]; + return $array[ $key ]; } } From 3468b3800acd067d6a645d51314a51f39155ea02 Mon Sep 17 00:00:00 2001 From: James Rodger Date: Tue, 21 Apr 2020 11:20:36 +0100 Subject: [PATCH 7/9] Make exception message translatable --- .../admin/class-wc-rest-payments-webhook-controller.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/includes/admin/class-wc-rest-payments-webhook-controller.php b/includes/admin/class-wc-rest-payments-webhook-controller.php index 5fab86a445f..3eb9d0167a6 100644 --- a/includes/admin/class-wc-rest-payments-webhook-controller.php +++ b/includes/admin/class-wc-rest-payments-webhook-controller.php @@ -118,7 +118,13 @@ private function process_webhook_refund_updated( $event_object ) { // Look up the order related to this charge. $order = $this->wcpay_db->order_from_charge_id( $charge_id ); if ( ! $order ) { - throw new Exception( 'Could not find order via charge ID: ' . $charge_id ); + throw new Exception( + sprintf( + /* translators: %1: charge ID */ + __( 'Could not find order via charge ID: %1$s', 'woocommerce-payments' ), + $charge_id + ) + ); } $note = sprintf( From e2fcaff031195415b66ad69dcdd61dc1683bcb9c Mon Sep 17 00:00:00 2001 From: James Rodger Date: Tue, 21 Apr 2020 11:25:01 +0100 Subject: [PATCH 8/9] Escape HTML in translation string --- .../class-wc-rest-payments-webhook-controller.php | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/includes/admin/class-wc-rest-payments-webhook-controller.php b/includes/admin/class-wc-rest-payments-webhook-controller.php index 3eb9d0167a6..43be12847a5 100644 --- a/includes/admin/class-wc-rest-payments-webhook-controller.php +++ b/includes/admin/class-wc-rest-payments-webhook-controller.php @@ -128,8 +128,14 @@ private function process_webhook_refund_updated( $event_object ) { } $note = sprintf( - /* translators: %1: the refund amount, %2: ID of the refund */ - __( 'A refund of %1$s was unsuccessful using WooCommerce Payments (%2$s).', 'woocommerce-payments' ), + WC_Payments_Utils::esc_interpolated_html( + /* translators: %1: the refund amount, %2: ID of the refund */ + __( 'A refund of %1$s was unsuccessful using WooCommerce Payments (%2$s).', 'woocommerce-payments' ), + array( + 'strong' => '', + 'code' => '', + ) + ), wc_price( $amount / 100 ), $refund_id ); From cefcaf1b05f1250822fe51e6e6509ffc652ba864 Mon Sep 17 00:00:00 2001 From: James Rodger Date: Wed, 22 Apr 2020 11:46:35 +0100 Subject: [PATCH 9/9] Rename WC_Payments_Rest_Exception to WC_Payments_Rest_Request_Exception --- .../class-wc-rest-payments-webhook-controller.php | 10 +++++----- includes/class-wc-payments.php | 2 +- ...hp => class-wc-payments-rest-request-exception.php} | 2 +- tests/bootstrap.php | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) rename includes/exceptions/{class-wc-payments-rest-exception.php => class-wc-payments-rest-request-exception.php} (81%) diff --git a/includes/admin/class-wc-rest-payments-webhook-controller.php b/includes/admin/class-wc-rest-payments-webhook-controller.php index 43be12847a5..eb6ba948441 100644 --- a/includes/admin/class-wc-rest-payments-webhook-controller.php +++ b/includes/admin/class-wc-rest-payments-webhook-controller.php @@ -5,7 +5,7 @@ * @package WooCommerce\Payments\Admin */ -use WCPay\Exceptions\WC_Payments_Rest_Exception; +use WCPay\Exceptions\WC_Payments_Rest_Request_Exception; use WCPay\Logger; defined( 'ABSPATH' ) || exit; @@ -84,7 +84,7 @@ public function handle_webhook( $request ) { $this->process_webhook_refund_updated( $event_object ); break; } - } catch ( WC_Payments_Rest_Exception $e ) { + } catch ( WC_Payments_Rest_Request_Exception $e ) { Logger::error( $e ); return new WP_REST_Response( array( 'result' => self::RESULT_BAD_REQUEST ), 400 ); } catch ( Exception $e ) { @@ -100,7 +100,7 @@ public function handle_webhook( $request ) { * * @param array $event_object The event that triggered the webhook. * - * @throws WC_Payments_Rest_Exception Required parameters not found. + * @throws WC_Payments_Rest_Request_Exception Required parameters not found. * @throws Exception Unable to resolve charge ID to order. */ private function process_webhook_refund_updated( $event_object ) { @@ -149,11 +149,11 @@ private function process_webhook_refund_updated( $event_object ) { * @param string $key ID to fetch on. * * @return string|array - * @throws WC_Payments_Rest_Exception Thrown if ID not set. + * @throws WC_Payments_Rest_Request_Exception Thrown if ID not set. */ private function read_rest_property( $array, $key ) { if ( ! isset( $array[ $key ] ) ) { - throw new WC_Payments_Rest_Exception( + throw new WC_Payments_Rest_Request_Exception( sprintf( /* translators: %1: ID being fetched */ __( '%1$s not found in array', 'woocommerce-payments' ), diff --git a/includes/class-wc-payments.php b/includes/class-wc-payments.php index c1001fd7473..7d17e693cb1 100644 --- a/includes/class-wc-payments.php +++ b/includes/class-wc-payments.php @@ -452,7 +452,7 @@ public static function create_api_client() { * Initialize the REST API controllers. */ public static function init_rest_api() { - include_once WCPAY_ABSPATH . 'includes/exceptions/class-wc-payments-rest-exception.php'; + include_once WCPAY_ABSPATH . 'includes/exceptions/class-wc-payments-rest-request-exception.php'; include_once WCPAY_ABSPATH . 'includes/admin/class-wc-payments-rest-controller.php'; include_once WCPAY_ABSPATH . 'includes/admin/class-wc-rest-payments-deposits-controller.php'; diff --git a/includes/exceptions/class-wc-payments-rest-exception.php b/includes/exceptions/class-wc-payments-rest-request-exception.php similarity index 81% rename from includes/exceptions/class-wc-payments-rest-exception.php rename to includes/exceptions/class-wc-payments-rest-request-exception.php index 25d4b203d23..cc82c60b709 100644 --- a/includes/exceptions/class-wc-payments-rest-exception.php +++ b/includes/exceptions/class-wc-payments-rest-request-exception.php @@ -14,4 +14,4 @@ /** * Exception for throwing errors in REST API controllers (e.g. issues with missing parameters in requests). */ -class WC_Payments_Rest_Exception extends Exception {} +class WC_Payments_Rest_Request_Exception extends Exception {} diff --git a/tests/bootstrap.php b/tests/bootstrap.php index d391f3285a6..0896f1b5978 100755 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -39,7 +39,7 @@ function _manually_load_plugin() { require_once dirname( __FILE__ ) . '/../includes/wc-payment-api/class-wc-payments-api-client.php'; require_once dirname( __FILE__ ) . '/../includes/wc-payment-api/class-wc-payments-http.php'; - require_once dirname( __FILE__ ) . '/../includes/exceptions/class-wc-payments-rest-exception.php'; + require_once dirname( __FILE__ ) . '/../includes/exceptions/class-wc-payments-rest-request-exception.php'; require_once dirname( __FILE__ ) . '/../includes/admin/class-wc-payments-rest-controller.php'; require_once dirname( __FILE__ ) . '/../includes/admin/class-wc-rest-payments-webhook-controller.php'; }