Feature/network order widget #17598

cmmarslender · 2017-11-06T17:53:27Z

Adds a dashboard widget on multisite installs that shows orders that need an action from across the multisite, from any site the user has access to orders on.

This branch relies on changes to styling from the tweak/order-screens branch, so if you would like to test, things will look better if you test on the demo branch in my copy of the repo, which combines this branch with the tweak/order-screens branch.

The styling changes this relied on are now merged into master so no special branch is needed for testing.

To test, you need to have a multisite installation of WordPress with WooCommerce installed. Since the widget is only designed to show orders that are processing or on hold, you'll need orders with this status on at least one of the sites you would like to see order data from. The user you test with will also need to have access to the site (and appropriate permissions to see order data) or the orders will not show up.

…r dashboard widget

…eds resolve even if a user doesn't have access to a site that we ask for

…s active

…o on-hold and processing statuses

…, so its always on any page with the widget

…nabled

…no orders found" message

claudiosanches · 2017-11-06T18:00:53Z

@cmmarslender is this issue ready for review?

cmmarslender · 2017-11-06T18:05:49Z

@claudiosanches yep

codecov · 2017-11-06T19:16:53Z

Codecov Report

Merging #17598 into master will decrease coverage by 0.07%.
The diff coverage is 3.47%.

@@             Coverage Diff              @@
##             master   #17598      +/-   ##
============================================
- Coverage      34.8%   34.73%   -0.08%     
- Complexity    12370    12387      +17     
============================================
  Files           342      343       +1     
  Lines         50725    50839     +114     
============================================
+ Hits          17655    17658       +3     
- Misses        33070    33181     +111

Impacted Files	Coverage Δ	Complexity Δ
includes/class-wc-api.php	`20% <0%> (-0.18%)`	`15 <0> (ø)`
includes/admin/class-wc-admin-dashboard.php	`0% <0%> (ø)`	`33 <4> (+7)`	⬆️
includes/admin/class-wc-admin.php	`0% <0%> (ø)`	`49 <0> (+1)`	⬆️
includes/api/class-wc-rest-orders-controller.php	`88.8% <100%> (+0.01%)`	`127 <0> (ø)`	⬇️
...es/api/class-wc-rest-network-orders-controller.php	`5.66% <5.66%> (ø)`	`9 <9> (?)`
includes/class-wc-tax.php	`80.05% <0%> (-0.26%)`	`136% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a98c494...81d1136. Read the comment docs.

mikejolley · 2017-11-08T15:23:32Z

@cmmarslender Not had a chance to review this yet, but it would be good if you could include a short description in the PR as well as testing instructions should someone want to check this out or test :) Thanks!

claudiosanches · 2017-11-09T16:40:06Z

assets/js/admin/network-orders.js

+	}
+
+	var orders = [],
+		promises = [], // Track completion (pass or fail) of ajax requests


Missing period.

claudiosanches · 2017-11-09T16:40:09Z

assets/js/admin/network-orders.js

+
+	var orders = [],
+		promises = [], // Track completion (pass or fail) of ajax requests
+		deferred = [], // Tracks the ajax deferreds


Missing period.

claudiosanches · 2017-11-09T16:40:13Z

assets/js/admin/network-orders.js

+		$orderTable = $( document.getElementById( 'woocommerce-network-order-table' ) ),
+		$noneFound = $( document.getElementById( 'woocommerce-network-orders-no-orders' ) );
+
+	// No sites, so bail


Missing period.

claudiosanches · 2017-11-09T16:40:19Z

includes/admin/class-wc-admin-dashboard.php

@@ -37,6 +37,11 @@ public function init() {
 			wp_add_dashboard_widget( 'woocommerce_dashboard_recent_reviews', __( 'WooCommerce recent reviews', 'woocommerce' ), array( $this, 'recent_reviews' ) );
 		}
 		wp_add_dashboard_widget( 'woocommerce_dashboard_status', __( 'WooCommerce status', 'woocommerce' ), array( $this, 'status_widget' ) );
+
+		// Network Order Widget


Missing period.

claudiosanches · 2017-11-09T16:40:29Z

includes/admin/class-wc-admin-dashboard.php

+
+		// Network Order Widget
+		if ( is_multisite() ) {
+			wp_add_dashboard_widget( 'woocommerce_network_orders', __( "WooCommerce Network Orders", 'woocommerce' ), array( $this, 'network_orders' ) );


No need for double quotes here.

We are avoiding title case in WooCommerce, so this name need to be fixed.

claudiosanches · 2017-11-09T16:40:59Z

includes/admin/class-wc-admin-dashboard.php

@@ -286,6 +291,70 @@ public function recent_reviews() {
 			echo '<p>' . __( 'There are no product reviews yet.', 'woocommerce' ) . '</p>';
 		}
 	}
+
+	/**
+	 * Network orders widget


Missing period.

claudiosanches · 2017-11-09T16:41:15Z

includes/admin/class-wc-admin-dashboard.php

+			<table id="woocommerce-network-order-table" class="woocommerce-network-order-table wp-list-table">
+				<thead>
+					<tr>
+						<td>Order</td>


All this strings need to be translatable.

claudiosanches · 2017-11-09T16:41:35Z

includes/admin/class-wc-admin-dashboard.php

+			</table>
+			<div id="woocommerce-network-orders-no-orders" class="woocommerce-network-orders-no-orders">
+				<p>
+					<?php esc_html_e( "No orders found", 'woocommerce' ); ?>


No need for double quote.

claudiosanches

A few minor coding standards to fix, but the endpoint should be moved into a new class, since the schema includes a few new items.

claudiosanches · 2017-11-09T16:43:05Z

includes/api/class-wc-rest-orders-controller.php

@@ -121,6 +121,18 @@ public function register_routes() {
 			),
 			'schema' => array( $this, 'get_public_batch_schema' ),
 		) );
+


This changes should be moved into a new class, creating a new schema too, since includes new items to the current orders schema.

claudiosanches · 2017-11-09T16:44:27Z

includes/api/class-wc-rest-orders-controller.php

+		$items = $this->get_items( $request );
+		remove_filter( 'woocommerce_rest_orders_prepare_object_query', array( $this, 'network_orders_filter_args' ) );
+
+		foreach( $items->data as &$current_order ) {


Missing space after foreach.

claudiosanches · 2017-11-09T16:44:53Z

includes/api/class-wc-rest-orders-controller.php

+		foreach( $items->data as &$current_order ) {
+			$order = wc_get_order( $current_order['id'] );
+
+			$current_order['blog'] = get_blog_details( get_current_blog_id() );


This items included here that need to belong the schema and that's why will be better have a new class just for it.

claudiosanches · 2017-11-09T16:45:31Z

includes/api/class-wc-rest-orders-controller.php

+
+			$current_order['blog'] = get_blog_details( get_current_blog_id() );
+			$current_order['edit_url'] = get_admin_url( $blog_id, 'post.php?post=' . absint( $order->get_id() ) . '&action=edit' );
+			$current_order['buyer'] = trim( sprintf( _x( '%1$s %2$s', 'full name', 'woocommerce' ), $order->get_billing_first_name(), $order->get_billing_last_name() ) );


Missing /* translators */ comment to help translators with placeholders.

claudiosanches · 2017-11-09T16:46:03Z

includes/api/class-wc-rest-orders-controller.php

+		 * @param array           $args    Key value array of query var to query value.
+		 * @param WP_REST_Request $request The request used.
+		 */
+		$args = apply_filters( "woocommerce_rest_orders_prepare_object_query", $args, $request );


No need for double quotes here.

mikejolley

Nice job :) Added some feedback. I tested on a simply multisite and it worked first time on all subsites.

mikejolley · 2017-11-17T11:03:44Z

assets/js/admin/network-orders.js

+		template = _.template( $( document.getElementById( 'network-orders-row-template') ).text() ),
+		$loadingIndicator = $( document.getElementById( 'woocommerce-network-order-table-loading' ) ),
+		$orderTable = $( document.getElementById( 'woocommerce-network-order-table' ) ),
+		$noneFound = $( document.getElementById( 'woocommerce-network-orders-no-orders' ) );


Any reason for using ID rather than jQuery based selectors? You have class and ID set in the markup - just asking :)

@mikejolley Class was set to enable styling that wasn't too specific.

getElementById was used for performance reasons over the jQuery selectors. document.getElementsByClassName is faster than getElementById, but isn't supported on as many browsers (IE11+) whereas getElementById is supported on basically all browsers even back to IE6. jQuery selectors are slower than all of the above options.

mikejolley · 2017-11-17T11:07:51Z

includes/admin/class-wc-admin-dashboard.php

+
+		// Network Order Widget.
+		if ( is_multisite() ) {
+			wp_add_dashboard_widget( 'woocommerce_network_orders', __( 'WooCommerce network orders', 'woocommerce' ), array( $this, 'network_orders' ) );


In a case where WC is network activated, would this also work on the network dashboard without other changes? If so, it might be a quick nice addition. Just need to use the hook wp_network_dashboard_setup too. No biggy if that won't work.

mikejolley · 2017-11-17T11:08:15Z

includes/admin/class-wc-admin-dashboard.php

+	public function network_orders() {
+		$suffix = defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG ? '' : '.min';
+
+		wp_enqueue_script( 'network-orders', WC()->plugin_url() . '/assets/js/admin/network-orders' . $suffix . '.js', array( 'jquery', 'underscore' ), WC_VERSION, true );


Can we prefix the script handle with wc-.

mikejolley · 2017-11-17T11:17:29Z

includes/admin/wc-admin-functions.php

@@ -39,6 +39,7 @@ function wc_get_screen_ids() {
 		'edit-product_tag',
 		'profile',
 		'user-edit',
+		'dashboard',


Including this on the dashboard would work, but it will also add extra overhead from WC scripts and styles, and potentially introduce conflicts. It isn't ideal, but can we move the CSS needed in this widget to a stylesheet only used on dashboard?

It is possible, yes. In addition to the styles I've added, I'll also have to take some styles from your redesign of the order listing page, and include those as well, so there would be a bit of duplication.. Sound ok?

Yeah thats probably better than loading them all in and you can target the widget only.

mikejolley · 2017-11-17T11:20:01Z

includes/api/class-wc-rest-network-orders-controller.php

+ * @author   WooThemes
+ * @category API
+ * @package  WooCommerce/API
+ * @since    @todo


@claudiulodro 3.3 or 3.4?

3.3 is good. This just has small changes left.

mikejolley · 2017-11-17T11:20:55Z

includes/api/class-wc-rest-network-orders-controller.php

+ * @package WooCommerce/API
+ * @extends WC_REST_Orders_Controller
+ */
+class WC_REST_Network_Orders_Controller extends WC_REST_Orders_Controller {


@claudiosanches Are you happy for this to be added in v2 api or should we punt the feature until v3 api is introduced? It's more of an internal API.

It's very small and for internal use, we could include and not even include into the docs for now.

Not to mention that v3 will take a long time to be released, there is a lot of work there and things to be reviewed.

mikejolley · 2017-11-17T11:22:23Z

includes/api/class-wc-rest-network-orders-controller.php

+			'context'     => array( 'view' ),
+			'readonly'    => true,
+		);
+		$schema['properties']['buyer'][] = array(


customer more appropriate? It's the language used everywhere else.

mikejolley · 2017-11-17T11:26:16Z

includes/api/class-wc-rest-network-orders-controller.php

+	 * @return array
+	 */
+	public function network_orders_filter_args( $args ) {
+		$args['post_status'] = array(


If we allowed this to be passed in as an arg to the API, the widget could be improved in the future to allow status filtering.

Thought about this, and originally decided to keep it simple, and not open up the endpoint (for now) to more use cases than I needed. I can update this.

Actually, I did try and implement this initially, but ran into a case where the current API only supports one status - see here

In order to change to allowing multiple statuses, the args for the endpoint would need to be changed to type => array. I didn't make this change because I didn't want to potentially break or change existing API functionality.

You mentioned there is a v3 of the API in the works - Possibly target array support for status on the V3 api instead of just a single string value?

Yup good call. I've logged separately. woocommerce/wc-api-dev#73

mikejolley · 2017-11-17T11:28:32Z

assets/css/admin.scss

@@ -6234,3 +6234,25 @@ table.bar_chart {
 		}
 	}
 }
+
+#woocommerce_network_orders {


Styling issues:

Table within widget is not 100% width

TH padding is misaligned with TDs

Could use some negative margin, or lose the padding on .inside` so the table appears full width and more closely matches other widgets.

Row has cursor pointer, but is not really clickable.

Fixed these.

…idget

…k admin

mikejolley · 2017-11-21T13:23:06Z

I think this is good to go 👍 We're going to keep this back for WC 3.4 next year rather than push to 3.3 release. Good to have features in the bag ready for releases :)

Thanks again

mikejolley · 2017-11-21T13:25:30Z

https://trello.com/c/hJxcV441/129-a-multisite-dashboard-widget-to-show-orders-requiring-attention-across-the-network

cmmarslender added 11 commits October 24, 2017 19:51

very basic implementation of REST API Based network order fetching fo…

05f486e

…r dashboard widget

Pass actual user blogs to the front end and make sure that the deferr…

2cbdb5b

…eds resolve even if a user doesn't have access to a site that we ask for

Merge branch 'master' into feature/network-order-widget

59152d4

Only add the network order widget/register the route when multisite i…

54c7165

…s active

Allow filtering the final args for an order API request, and filter t…

af7ac97

…o on-hold and processing statuses

Once all orders are fetched client side, order them all, newest first

c9ce1a2

Leave the wp_enqueue_script for network orders in the widget callback…

a7f1894

…, so its always on any page with the widget

Use WC_VERSION for js version on enqueue

68f3092

Enqueue .min.js version of network order script if SCRIPT_DEBUG not e…

f4ad8b8

…nabled

Add loading indicator for the network order widget

68e42ab

When a user can't access any sites, or no orders are returned, show "…

77721a1

…no orders found" message

mikejolley added the Status: Needs Review label Nov 6, 2017

claudiosanches reviewed Nov 9, 2017

View reviewed changes

claudiosanches suggested changes Nov 9, 2017

View reviewed changes

claudiosanches reviewed Nov 9, 2017

View reviewed changes

claudiosanches added the needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. label Nov 9, 2017

cmmarslender added 4 commits November 16, 2017 15:27

Merge branch 'master' into feature/network-order-widget

bd9ebdf

Fixes some phpcs issues

839c09d

Fix shorthand ternary incompatibility with php 5.2

e68a918

Fix shorthand ternary notation in network orders permission check

14a0f33

mikejolley requested changes Nov 17, 2017

View reviewed changes

mikejolley added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed Status: Needs Review labels Nov 17, 2017

cmmarslender added 10 commits November 17, 2017 15:27

Merge branch 'master' into feature/network-order-widget

4d85695

Fix alignment/styling issues with the table

34f9069

Cursor default on the table rows except for clickable areas

0c98176

Prefix script with wc-

23ef2af

Change buyer to customer in the API response

678e5c7

Update @SInCE to 3.3

9339eed

Move network order styles to their own file, and load this with the w…

24e9811

…idget

Match list table styling in the widget stylesheet

41493f6

Set up the dashboard class to load the network order widget on networ…

d94f69d

…k admin

Avoid core list class, so that small screen styling doesn't break

81d1136

mikejolley mentioned this pull request Nov 20, 2017

Support querying orders by multiple statuses woocommerce/wc-api-dev#73

Closed

mikejolley added Status: Needs Review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Nov 20, 2017

claudiulodro added this to the 3.4.0 milestone Nov 20, 2017

mikejolley added Release: Minor status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. and removed Status: Needs Review labels Nov 21, 2017

mikejolley removed the Release: Minor label Jan 4, 2018

mikejolley removed the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Jan 29, 2018

claudiosanches merged commit 81d1136 into woocommerce:master Feb 2, 2018

Feature/network order widget #17598

Feature/network order widget #17598

Conversation

cmmarslender commented Nov 6, 2017 • edited

claudiosanches commented Nov 6, 2017

cmmarslender commented Nov 6, 2017

codecov bot commented Nov 6, 2017 • edited

Codecov Report

mikejolley commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

claudiosanches left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikejolley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikejolley commented Nov 21, 2017

mikejolley commented Nov 21, 2017

cmmarslender commented Nov 6, 2017 •

edited

codecov bot commented Nov 6, 2017 •

edited