Skip to content
This repository has been archived by the owner on Jun 7, 2022. It is now read-only.

Reduce order status change delay in Orders Panel. #2659

Merged
merged 7 commits into from Jul 18, 2019

Conversation

jeffstieler
Copy link
Contributor

@jeffstieler jeffstieler commented Jul 16, 2019

Fixes #2592 and #2309.

This PR seeks to reduce/eliminate the delay in order status transitions manifesting in the Orders Panel.

It does so by using the core WooCommerce Orders endpoint to determine if there are actionable orders, as well as trusting the core endpoint data over the reporting table data - both endpoints are hit and the data merged, with the core endpoint overriding the reporting endpoint. This hopefully ensures that we can catch order status (and date, etc) changes in any "direction" - processing to completed, or completed to pending, etc.

Additionally, this PR removes the default date range for the /reports/orders endpoint (since it's not present in the core endpoint, and we need parity). This solves #2309.

Detailed test instructions:

  • Have an order that has an "actionable" status and shows up in the activity panel.
  • Edit the order to a non-actionable status, i.e. mark it as complete.
  • Note after the POST of the edit order form is completed, the order is no longer in the panel.
  • Navigate to the wc-admin dashboard, verify the order is no longer in the panel.

Changelog Note:

Fix: completed orders lingering in activity panel.

@jeffstieler jeffstieler added [Status] Needs Review type: task The issue is an internally driven task (e.g. from another A8c team). focus: activity panel Issues about activity/task panels on the home screen. focus: wc rest api labels Jul 16, 2019
@jeffstieler jeffstieler requested a review from a team July 16, 2019 21:31
@jeffstieler jeffstieler added this to In Progress PRs (for automation purposes) in Isotope via automation Jul 16, 2019
Copy link
Contributor

@joshuatf joshuatf left a comment

Choose a reason for hiding this comment

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

This is looking pretty good, @jeffstieler! I'm having trouble seeing orders because of hasActionableOrders but I think moving your solution up to determine that prop will resolve this.

Also I noticed that orders seem to be in chronological order (with the oldest at top). This may be expected, but quick sanity check while we're here.

};
const actionableOrders = Array.from( getItems( 'orders', allOrdersQuery ).values() );

if ( hasActionableOrders && actionableOrders.length ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm unable to see orders here since hasActionableOrders is false. I'm thinking hasActionableOrders might not be needed here. Since we've already queried for actionableOrders we can just check its length.

On that same note, we may want to update the unread indicator to use the same test so those aren't out of sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm.. I'm not seeing hasActionableOrders as false. It ultimate comes from the unread indicator function:

return <OrdersPanel hasActionableOrders={ hasUnreadOrders } />;
,
const hasUnreadOrders = getUnreadOrders( select );

@@ -385,6 +387,24 @@ public function get_collection_params() {
'sanitize_callback' => 'wc_string_to_bool',
'validate_callback' => 'rest_validate_request_arg',
);
$params['order_includes'] = array(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👍 Thanks for adding these!

@@ -265,29 +274,37 @@ export default compose(
return { orders: [], isError: true, isRequesting: false, orderStatuses };
}

if ( hasActionableOrders ) {
// Query the core Orders endpoint for the most up-to-date statuses.
const allOrdersQuery = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope for this PR, but would be awesome if we could just return the IDs here - https://developer.wordpress.org/reference/classes/wp_query/#return-fields-parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 7a3f4fd.

@@ -151,8 +161,8 @@ public function get_data( $query_args ) {
'page' => 1,
'order' => 'DESC',
'orderby' => 'date_created',
'before' => WC_Admin_Reports_Interval::default_before(),
'after' => WC_Admin_Reports_Interval::default_after(),
'before' => '',
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of removing the default date limitations. Just want to make sure it matches up with what is done in #2636.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this PR is going to do the trick without implementing #2636.

@jeffstieler
Copy link
Contributor Author

Also I noticed that orders seem to be in chronological order (with the oldest at top). This may be expected, but quick sanity check while we're here.

Fixed in b5a47e1.

@jeffstieler
Copy link
Contributor Author

I accidentally broke the "non actionable" order query - fixed in 9a0f74d. This is ready for another review.

Copy link
Contributor

@joshuatf joshuatf left a comment

Choose a reason for hiding this comment

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

Excellent work, @jeffstieler! Testing well for me. Added one tiny nitpick about todo format, but otherwise LGTM.

page: 1,
per_page: QUERY_DEFAULTS.pageSize,
status: orderStatuses,
_fields: [ 'id', 'date_created_gmt', 'status' ],
Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome! Did not know the _fields property was an option here.


return { orders, isError, isRequesting, orderStatuses };
}

// Get a count of all orders for messaging purposes.
// @todo: add a property to wcSettings for this?
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nitpick: todos were update to format:

@todo Thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Amended the last commit to fix this. Thanks.

@jeffstieler jeffstieler merged commit b44ac2d into master Jul 18, 2019
Isotope automation moved this from In Progress PRs (for automation purposes) to Done Sprint 21 (July 9 - July 23) Jul 18, 2019
@jeffstieler jeffstieler deleted the fix/2592-orders-linger branch July 18, 2019 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
focus: activity panel Issues about activity/task panels on the home screen. focus: wc rest api type: task The issue is an internally driven task (e.g. from another A8c team).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Order Panel: Completed Orders Linger in List
3 participants