From cf0e9f925cad5322ed8182692e315ec6cb9b7ad3 Mon Sep 17 00:00:00 2001 From: Mike Jolley Date: Wed, 13 Feb 2019 16:20:09 +0000 Subject: [PATCH 1/7] Refactor should_deliver logic into managable chunks --- includes/class-wc-webhook.php | 170 ++++++++++++++++++++++++---------- 1 file changed, 120 insertions(+), 50 deletions(-) diff --git a/includes/class-wc-webhook.php b/includes/class-wc-webhook.php index 9170415044c..1cdd57aaa9a 100644 --- a/includes/class-wc-webhook.php +++ b/includes/class-wc-webhook.php @@ -123,62 +123,132 @@ class WC_Webhook extends WC_Legacy_Webhook { * @return bool True if webhook should be delivered, false otherwise. */ private function should_deliver( $arg ) { - $should_deliver = true; - $current_action = current_action(); + $should_deliver = $this->is_active() && $this->is_valid_topic() && $this->is_valid_action( $arg ); - // Only active webhooks can be delivered. - if ( 'active' !== $this->get_status() ) { - $should_deliver = false; - } elseif ( in_array( $current_action, array( 'delete_post', 'wp_trash_post', 'untrashed_post' ), true ) ) { - // Only deliver deleted/restored event for coupons, orders, and products. - if ( isset( $GLOBALS['post_type'] ) && ! in_array( $GLOBALS['post_type'], array( 'shop_coupon', 'shop_order', 'product' ), true ) ) { - $should_deliver = false; - } - - // Check if is delivering for the correct resource. - if ( isset( $GLOBALS['post_type'] ) && str_replace( 'shop_', '', $GLOBALS['post_type'] ) !== $this->get_resource() ) { - $should_deliver = false; - } - } elseif ( 'delete_user' === $current_action ) { - $user = get_userdata( absint( $arg ) ); - - // Only deliver deleted customer event for users with customer role. - if ( ! $user || ! in_array( 'customer', (array) $user->roles, true ) ) { - $should_deliver = false; - } - } elseif ( 'order' === $this->get_resource() && ! in_array( get_post_type( absint( $arg ) ), wc_get_order_types( 'order-webhooks' ), true ) ) { - // Only if the custom order type has chosen to exclude order webhooks from triggering along with its own webhooks. - $should_deliver = false; - - } elseif ( 0 === strpos( $current_action, 'woocommerce_process_shop' ) || 0 === strpos( $current_action, 'woocommerce_process_product' ) ) { - // The `woocommerce_process_shop_*` and `woocommerce_process_product_*` hooks - // fire for create and update of products and orders, so check the post - // creation date to determine the actual event. - $resource = get_post( absint( $arg ) ); - - // Drafts don't have post_date_gmt so calculate it here. - $gmt_date = get_gmt_from_date( $resource->post_date ); - - // A resource is considered created when the hook is executed within 10 seconds of the post creation date. - $resource_created = ( ( time() - 10 ) <= strtotime( $gmt_date ) ); - - if ( 'created' === $this->get_event() && ! $resource_created ) { - $should_deliver = false; - } elseif ( 'updated' === $this->get_event() && $resource_created ) { - $should_deliver = false; - } - } - - if ( ! wc_is_webhook_valid_topic( $this->get_topic() ) ) { - $should_deliver = false; - } - - /* + /** * Let other plugins intercept deliver for some messages queue like rabbit/zeromq. + * + * @param bool $should_deliver True if the webhook should be sent, or false to not send it. + * @param WC_Webhook $this The current webhook class. + * @param mixed $arg First hook argument. */ return apply_filters( 'woocommerce_webhook_should_deliver', $should_deliver, $this, $arg ); } + /** + * Returns if webhook is active. + * + * @since 3.6.0 + * @return bool True if validation passes. + */ + private function is_active() { + return 'active' === $this->get_status(); + } + + /** + * Returns if topic is valid. + * + * @since 3.6.0 + * @return bool True if validation passes. + */ + private function is_valid_topic() { + return wc_is_webhook_valid_topic( $this->get_topic() ); + } + + /** + * Validates the criteria for certain actions. + * + * @since 3.6.0 + * @param mixed $arg First hook argument. + * @return bool True if validation passes. + */ + private function is_valid_action( $arg ) { + $current_action = current_action(); + $return = true; + + switch ( $current_action ) { + case 'delete_post': + case 'wp_trash_post': + case 'untrashed_post': + $return = $this->is_valid_post_action( $arg ); + break; + case 'delete_user': + $return = $this->is_valid_user_action( $arg ); + break; + } + + if ( 0 === strpos( $current_action, 'woocommerce_process_shop' ) || 0 === strpos( $current_action, 'woocommerce_process_product' ) ) { + $return = $this->is_valid_processing_action( $arg ); + } + + return $return; + } + + /** + * Validates post actions. + * + * @since 3.6.0 + * @param mixed $arg First hook argument. + * @return bool True if validation passes. + */ + private function is_valid_post_action( $arg ) { + // Only deliver deleted/restored event for coupons, orders, and products. + if ( isset( $GLOBALS['post_type'] ) && ! in_array( $GLOBALS['post_type'], array( 'shop_coupon', 'shop_order', 'product' ), true ) ) { + return false; + } + + // Check if is delivering for the correct resource. + if ( isset( $GLOBALS['post_type'] ) && str_replace( 'shop_', '', $GLOBALS['post_type'] ) !== $this->get_resource() ) { + return false; + } + return true; + } + + /** + * Validates user actions. + * + * @since 3.6.0 + * @param mixed $arg First hook argument. + * @return bool True if validation passes. + */ + private function is_valid_user_action( $arg ) { + $user = get_userdata( absint( $arg ) ); + + // Only deliver deleted customer event for users with customer role. + if ( ! $user || ! in_array( 'customer', (array) $user->roles, true ) ) { + return false; + } + + return true; + } + + /** + * Validates WC processing actions. + * + * @since 3.6.0 + * @param mixed $arg First hook argument. + * @return bool True if validation passes. + */ + private function is_valid_processing_action( $arg ) { + // The `woocommerce_process_shop_*` and `woocommerce_process_product_*` hooks + // fire for create and update of products and orders, so check the post + // creation date to determine the actual event. + $resource = get_post( absint( $arg ) ); + + // Drafts don't have post_date_gmt so calculate it here. + $gmt_date = get_gmt_from_date( $resource->post_date ); + + // A resource is considered created when the hook is executed within 10 seconds of the post creation date. + $resource_created = ( ( time() - 10 ) <= strtotime( $gmt_date ) ); + + if ( 'created' === $this->get_event() && ! $resource_created ) { + return false; + } elseif ( 'updated' === $this->get_event() && $resource_created ) { + return false; + } + return true; + } + /** * Deliver the webhook payload using wp_safe_remote_request(). * From 311449e943478348b1105f99d99471b3245f349b Mon Sep 17 00:00:00 2001 From: Mike Jolley Date: Wed, 13 Feb 2019 16:46:49 +0000 Subject: [PATCH 2/7] Create is_valid_resource method to check for invalid statuses --- includes/class-wc-webhook.php | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/includes/class-wc-webhook.php b/includes/class-wc-webhook.php index 1cdd57aaa9a..5747ecfcefe 100644 --- a/includes/class-wc-webhook.php +++ b/includes/class-wc-webhook.php @@ -123,7 +123,7 @@ class WC_Webhook extends WC_Legacy_Webhook { * @return bool True if webhook should be delivered, false otherwise. */ private function should_deliver( $arg ) { - $should_deliver = $this->is_active() && $this->is_valid_topic() && $this->is_valid_action( $arg ); + $should_deliver = $this->is_active() && $this->is_valid_topic() && $this->is_valid_action( $arg ) && $this->is_valid_resource( $arg ); /** * Let other plugins intercept deliver for some messages queue like rabbit/zeromq. @@ -249,6 +249,32 @@ class WC_Webhook extends WC_Legacy_Webhook { return true; } + /** + * Checks the resource for this webhook is valid e.g. valid post status. + * + * @since 3.6.0 + * @param mixed $arg First hook argument. + * @return bool True if validation passes. + */ + private function is_valid_resource( $arg ) { + $resource = $this->get_resource(); + + if ( in_array( $resource, array( 'order', 'product', 'coupon' ), true ) ) { + $status = get_post_status( absint( $arg ) ); + + // Ignore auto drafts for all resources. + if ( in_array( $status, array( 'auto-draft', 'wc-auto-draft', 'new' ), true ) ) { + return false; + } + + // Ignore standard drafts for orders. + if ( 'order' === $resource && 'draft' === $status ) { + return false; + } + } + return true; + } + /** * Deliver the webhook payload using wp_safe_remote_request(). * From ae17d6f3bdbe4f55b5d9f569dc9624bcca588618 Mon Sep 17 00:00:00 2001 From: Mike Jolley Date: Wed, 13 Feb 2019 17:52:28 +0000 Subject: [PATCH 3/7] Avoid handling wc-auto-draft WC was erroneously adding wc- prefix to the core WP auto-draft status. #22380 registered it formally but we don't need it. I've reverted #22380 and handled the prefix correctly. --- includes/class-wc-post-types.php | 8 -------- .../abstract-wc-order-data-store-cpt.php | 14 ++++++++++---- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/includes/class-wc-post-types.php b/includes/class-wc-post-types.php index 213cfc4992c..616b3292bb3 100644 --- a/includes/class-wc-post-types.php +++ b/includes/class-wc-post-types.php @@ -484,14 +484,6 @@ class WC_Post_Types { $order_statuses = apply_filters( 'woocommerce_register_shop_order_post_statuses', array( - 'wc-auto-draft' => array( - 'public' => false, - 'exclude_from_search' => false, - 'show_in_admin_all_list' => true, - 'show_in_admin_status_list' => true, - /* translators: %s: number of orders */ - 'label_count' => _n_noop( 'Draft (%s)', 'Draft (%s)', 'woocommerce' ), - ), 'wc-pending' => array( 'label' => _x( 'Pending payment', 'Order status', 'woocommerce' ), 'public' => false, diff --git a/includes/data-stores/abstract-wc-order-data-store-cpt.php b/includes/data-stores/abstract-wc-order-data-store-cpt.php index adcb7406d9c..d28a23b6d06 100644 --- a/includes/data-stores/abstract-wc-order-data-store-cpt.php +++ b/includes/data-stores/abstract-wc-order-data-store-cpt.php @@ -58,6 +58,9 @@ abstract class Abstract_WC_Order_Data_Store_CPT extends WC_Data_Store_WP impleme $order->set_date_created( current_time( 'timestamp', true ) ); $order->set_currency( $order->get_currency() ? $order->get_currency() : get_woocommerce_currency() ); + $raw_status = $order->get_status( 'edit' ) ? $order->get_status( 'edit' ) : apply_filters( 'woocommerce_default_order_status', 'pending' ); + $status = wc_is_order_status( 'wc-' . $raw_status ) ? 'wc-' . $raw_status : $raw_status; + $id = wp_insert_post( apply_filters( 'woocommerce_new_order_data', @@ -65,7 +68,7 @@ abstract class Abstract_WC_Order_Data_Store_CPT extends WC_Data_Store_WP impleme 'post_date' => gmdate( 'Y-m-d H:i:s', $order->get_date_created( 'edit' )->getOffsetTimestamp() ), 'post_date_gmt' => gmdate( 'Y-m-d H:i:s', $order->get_date_created( 'edit' )->getTimestamp() ), 'post_type' => $order->get_type( 'edit' ), - 'post_status' => 'wc-' . ( $order->get_status( 'edit' ) ? $order->get_status( 'edit' ) : apply_filters( 'woocommerce_default_order_status', 'pending' ) ), + 'post_status' => $status, 'ping_status' => 'closed', 'post_author' => 1, 'post_title' => $this->get_post_title(), @@ -73,7 +76,8 @@ abstract class Abstract_WC_Order_Data_Store_CPT extends WC_Data_Store_WP impleme 'post_parent' => $order->get_parent_id( 'edit' ), 'post_excerpt' => $this->get_post_excerpt( $order ), ) - ), true + ), + true ); if ( $id && ! is_wp_error( $id ) ) { @@ -119,7 +123,7 @@ abstract class Abstract_WC_Order_Data_Store_CPT extends WC_Data_Store_WP impleme * stored. @todo When meta is flattened, handle this during migration. */ if ( version_compare( $order->get_version( 'edit' ), '2.3.7', '<' ) && $order->get_prices_include_tax( 'edit' ) ) { - $order->set_discount_total( (double) get_post_meta( $order->get_id(), '_cart_discount', true ) - (double) get_post_meta( $order->get_id(), '_cart_discount_tax', true ) ); + $order->set_discount_total( (float) get_post_meta( $order->get_id(), '_cart_discount', true ) - (float) get_post_meta( $order->get_id(), '_cart_discount_tax', true ) ); } } @@ -140,10 +144,12 @@ abstract class Abstract_WC_Order_Data_Store_CPT extends WC_Data_Store_WP impleme // Only update the post when the post data changes. if ( array_intersect( array( 'date_created', 'date_modified', 'status', 'parent_id', 'post_excerpt' ), array_keys( $changes ) ) ) { + $raw_status = $order->get_status( 'edit' ) ? $order->get_status( 'edit' ) : apply_filters( 'woocommerce_default_order_status', 'pending' ); + $status = wc_is_order_status( 'wc-' . $raw_status ) ? 'wc-' . $raw_status : $raw_status; $post_data = array( 'post_date' => gmdate( 'Y-m-d H:i:s', $order->get_date_created( 'edit' )->getOffsetTimestamp() ), 'post_date_gmt' => gmdate( 'Y-m-d H:i:s', $order->get_date_created( 'edit' )->getTimestamp() ), - 'post_status' => 'wc-' . ( $order->get_status( 'edit' ) ? $order->get_status( 'edit' ) : apply_filters( 'woocommerce_default_order_status', 'pending' ) ), + 'post_status' => $status, 'post_parent' => $order->get_parent_id(), 'post_excerpt' => $this->get_post_excerpt( $order ), 'post_modified' => isset( $changes['date_modified'] ) ? gmdate( 'Y-m-d H:i:s', $order->get_date_modified( 'edit' )->getOffsetTimestamp() ) : current_time( 'mysql' ), From 13612ef3f1ce15a6e05ba3c7fa2c3e7b0edbebda Mon Sep 17 00:00:00 2001 From: Mike Jolley Date: Wed, 13 Feb 2019 17:55:30 +0000 Subject: [PATCH 4/7] Update CRUD update hook based on status transition --- .../class-wc-order-data-store-cpt.php | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/includes/data-stores/class-wc-order-data-store-cpt.php b/includes/data-stores/class-wc-order-data-store-cpt.php index 1f4dee00ad0..a73d10f27c9 100644 --- a/includes/data-stores/class-wc-order-data-store-cpt.php +++ b/includes/data-stores/class-wc-order-data-store-cpt.php @@ -156,10 +156,20 @@ class WC_Order_Data_Store_CPT extends Abstract_WC_Order_Data_Store_CPT implement $order->set_date_paid( $order->get_date_created( 'edit' ) ); } + // Also grab the current status so we can compare. + $previous_status = get_post_status( $order->get_id() ); + // Update the order. parent::update( $order ); - do_action( 'woocommerce_update_order', $order->get_id() ); + // Fire a hook depending on the status - this should be considered a creation if it was previously draft status. + $new_status = $order->get_status( 'edit' ); + + if ( $new_status !== $previous_status && in_array( $previous_status, array( 'new', 'auto-draft', 'draft' ), true ) ) { + do_action( 'woocommerce_new_order', $order->get_id() ); + } else { + do_action( 'woocommerce_update_order', $order->get_id() ); + } } /** @@ -482,8 +492,10 @@ class WC_Order_Data_Store_CPT extends Abstract_WC_Order_Data_Store_CPT implement * @var array */ $search_fields = array_map( - 'wc_clean', apply_filters( - 'woocommerce_shop_order_search_fields', array( + 'wc_clean', + apply_filters( + 'woocommerce_shop_order_search_fields', + array( '_billing_address_index', '_shipping_address_index', '_billing_last_name', From 588b5903f7845b90a9debbb05003d25c2ebe9bbc Mon Sep 17 00:00:00 2001 From: Mike Jolley Date: Wed, 13 Feb 2019 17:55:47 +0000 Subject: [PATCH 5/7] Ignore old pre-crud actions --- includes/class-wc-webhook.php | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/includes/class-wc-webhook.php b/includes/class-wc-webhook.php index 5747ecfcefe..ddb3fa1bc93 100644 --- a/includes/class-wc-webhook.php +++ b/includes/class-wc-webhook.php @@ -263,7 +263,7 @@ class WC_Webhook extends WC_Legacy_Webhook { $status = get_post_status( absint( $arg ) ); // Ignore auto drafts for all resources. - if ( in_array( $status, array( 'auto-draft', 'wc-auto-draft', 'new' ), true ) ) { + if ( in_array( $status, array( 'auto-draft', 'new' ), true ) ) { return false; } @@ -964,11 +964,9 @@ class WC_Webhook extends WC_Legacy_Webhook { 'delete_user', ), 'order.created' => array( - 'woocommerce_process_shop_order_meta', 'woocommerce_new_order', ), 'order.updated' => array( - 'woocommerce_process_shop_order_meta', 'woocommerce_update_order', 'woocommerce_order_refunded', ), From 84299ff5ed361650b88cb6071a106dbe20824e7b Mon Sep 17 00:00:00 2001 From: Mike Jolley Date: Wed, 13 Feb 2019 17:56:41 +0000 Subject: [PATCH 6/7] Update the tests --- tests/unit-tests/webhooks/crud.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit-tests/webhooks/crud.php b/tests/unit-tests/webhooks/crud.php index 46d5b6fc867..7e7c733e15f 100644 --- a/tests/unit-tests/webhooks/crud.php +++ b/tests/unit-tests/webhooks/crud.php @@ -147,7 +147,6 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { $object = new WC_Webhook(); $object->set_topic( 'order.created' ); $expected = array( - 'woocommerce_process_shop_order_meta', 'woocommerce_new_order', ); $this->assertEquals( $expected, $object->get_hooks() ); From c01e334500fe9720407f2d60f79ad165c3d59457 Mon Sep 17 00:00:00 2001 From: Claudio Sanches Date: Mon, 18 Feb 2019 17:59:34 -0300 Subject: [PATCH 7/7] Fixed coding standards --- tests/unit-tests/webhooks/crud.php | 43 ++++++++++++++++-------------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/tests/unit-tests/webhooks/crud.php b/tests/unit-tests/webhooks/crud.php index 7e7c733e15f..4c8675c560e 100644 --- a/tests/unit-tests/webhooks/crud.php +++ b/tests/unit-tests/webhooks/crud.php @@ -1,15 +1,18 @@ save(); $this->assertEquals( $id, $object->get_id() ); @@ -19,7 +22,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_data */ - function test_get_data() { + public function test_get_data() { $object = new WC_Webhook(); $this->assertInternalType( 'array', $object->get_data() ); } @@ -27,7 +30,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_name */ - function test_get_name() { + public function test_get_name() { $object = new WC_Webhook(); $expected = 'test'; $object->set_name( $expected ); @@ -37,7 +40,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_date_created */ - function test_get_date_created() { + public function test_get_date_created() { $object = new WC_Webhook(); $object->set_date_created( '2016-12-12' ); $this->assertEquals( '1481500800', $object->get_date_created()->getOffsetTimestamp() ); @@ -49,7 +52,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_date_modified */ - function test_get_date_modified() { + public function test_get_date_modified() { $object = new WC_Webhook(); $object->set_date_modified( '2016-12-12' ); $this->assertEquals( '1481500800', $object->get_date_modified()->getOffsetTimestamp() ); @@ -61,8 +64,8 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_status */ - function test_get_status() { - $object = new WC_Webhook(); + public function test_get_status() { + $object = new WC_Webhook(); $this->assertEquals( 'disabled', $object->get_status() ); $expected = 'active'; @@ -73,7 +76,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_secret */ - function test_get_secret() { + public function test_get_secret() { $object = new WC_Webhook(); $expected = 'secret'; $object->set_secret( $expected ); @@ -83,7 +86,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_topic */ - function test_get_topic() { + public function test_get_topic() { $object = new WC_Webhook(); $expected = 'order.created'; $object->set_topic( $expected ); @@ -93,7 +96,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_delivery_url */ - function test_get_delivery_url() { + public function test_get_delivery_url() { $object = new WC_Webhook(); $expected = 'https://woocommerce.com'; $object->set_delivery_url( $expected ); @@ -103,7 +106,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_user_id */ - function test_get_user_id() { + public function test_get_user_id() { $object = new WC_Webhook(); $expected = 1; $object->set_user_id( $expected ); @@ -113,7 +116,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_api_version */ - function test_get_api_version() { + public function test_get_api_version() { $object = new WC_Webhook(); $expected = 'wp_api_v2'; $object->set_api_version( $expected ); @@ -123,7 +126,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_failure_count */ - function test_get_failure_count() { + public function test_get_failure_count() { $object = new WC_Webhook(); $expected = 1; $object->set_failure_count( $expected ); @@ -133,7 +136,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_pending_delivery */ - function test_get_pending_delivery() { + public function test_get_pending_delivery() { $object = new WC_Webhook(); $expected = true; $object->set_pending_delivery( $expected ); @@ -143,7 +146,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_hooks */ - function test_get_hooks() { + public function test_get_hooks() { $object = new WC_Webhook(); $object->set_topic( 'order.created' ); $expected = array( @@ -155,7 +158,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_resource */ - function test_get_resource() { + public function test_get_resource() { $object = new WC_Webhook(); $object->set_topic( 'order.created' ); $this->assertEquals( 'order', $object->get_resource() ); @@ -164,7 +167,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_event */ - function test_get_event() { + public function test_get_event() { $object = new WC_Webhook(); $object->set_topic( 'order.created' ); $this->assertEquals( 'created', $object->get_event() ); @@ -173,7 +176,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: get_i18n_status */ - function test_get_i18n_status() { + public function test_get_i18n_status() { $object = new WC_Webhook(); $object->set_status( 'active' ); $this->assertEquals( 'Active', $object->get_i18n_status() ); @@ -182,7 +185,7 @@ class WC_Tests_CRUD_Webhooks extends WC_Unit_Test_Case { /** * Test: generate_signature */ - function test_generate_signature() { + public function test_generate_signature() { $object = new WC_Webhook(); $this->assertEquals( 'GBDo00G55h6IiV+6CxqivQPLbI//KzaOZm747971tPs=', $object->generate_signature( 'secret' ) ); }