New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store all order statuses #1285

Merged
merged 19 commits into from Jan 16, 2019

Conversation

Projects
None yet
2 participants
@joshuatf
Copy link
Contributor

joshuatf commented Jan 11, 2019

Fixes #1251

This PR:

  • Stores all order statuses
  • Adds an array of excluded statuses
  • Filters all products, coupons, categories, taxes, variations, and orders by the default (non-excluded) statuses if no status query is provided

Detailed test instructions:

  1. Make sure that you have regenerated lookup tables and that the status column is filled in wc_order_stats.
  2. Create requests to each of the endpoints.
  3. Check that stats returned included stats from all orders excluding the default excluded statuses (refunded, pending, failed, cancelled).
  4. Make a request to the /wp-json/wc/v3/reports/orders/stats endpoint and check that status_is and status_is_not query params override default excluded statuses (e.g., you can still query by failed order statuses).
  5. Check that all phpunit tests are passing.

@joshuatf joshuatf self-assigned this Jan 11, 2019

@joshuatf joshuatf requested a review from woocommerce/isotope Jan 14, 2019

@jeffstieler
Copy link
Contributor

jeffstieler left a comment

In testing the /reports/categories endpoint - I'm seeing 165 results with status_is = 'failed'. There are only 33 rows in my wc_order_stats table with a status of wc-failed.. is this correct?

global $wpdb;
$table_name = $wpdb->prefix . self::TABLE_NAME;
// Avoid ambigious column order_id in SQL query.
$this->report_columns['orders_count'] = str_replace( 'order_id', $table_name . '.order_id', $this->report_columns['orders_count'] );

This comment has been minimized.

@jeffstieler

jeffstieler Jan 14, 2019

Contributor

Why str_replace() here? I think it would be simpler to just define the column query here instead of modifying an incomplete/incorrect definition.

This comment has been minimized.

@joshuatf

joshuatf Jan 15, 2019

Author Contributor

It would be simpler, but I think it would cause more confusion when debugging and means we are defining this value twice instead of filtering it.

For example, if we redefine this variable entirely and someone changes the value in $report_columns instead of the construct method, no errors occur, but the code does not work as intended.

On the other hand, if we filter the value and the user changes the $report_columns value from:
'orders_count' => 'COUNT(DISTINCT order_id) as orders_count' to
'orders_count' => 'COUNT(order_id) as orders_count'
then the function continues to work as expected. And if we filter a wrong value like 'orders_count' => 'COUNT(DISTINCT order_id) as order_id_alias', we at least get an error message.

Neither one of these are very ideal solutions though, so I've opened another issue to address how we deal with ambiguous columns here. If it's okay with you, we can deal with that in another PR.

global $wpdb;
$table_name = $wpdb->prefix . self::TABLE_NAME;
// Avoid ambigious column order_id in SQL query.
$this->report_columns['orders_count'] = str_replace( 'order_id', $table_name . '.order_id', $this->report_columns['orders_count'] );

This comment has been minimized.

@jeffstieler

jeffstieler Jan 14, 2019

Contributor

Same question about str_replace() here.

global $wpdb;
$table_name = $wpdb->prefix . self::TABLE_NAME;
// Avoid ambigious column order_id in SQL query.
$this->report_columns['orders_count'] = str_replace( 'order_id', $table_name . '.order_id', $this->report_columns['orders_count'] );

This comment has been minimized.

@jeffstieler

jeffstieler Jan 14, 2019

Contributor

Same question about str_replace() here.

$excluded_statuses = array_map( array( $this, 'normalize_order_status' ), $this->get_excluded_report_order_statuses() );
$subqueries[] = "{$wpdb->prefix}wc_order_stats.status NOT IN ( '" . implode( "','", $excluded_statuses ) . "' )";
}

This comment has been minimized.

@jeffstieler

jeffstieler Jan 14, 2019

Contributor

Optional: there's an opportunity to avoid some copy-pasta here and combine this with the if() statement above it.

This comment has been minimized.

@joshuatf

joshuatf Jan 15, 2019

Author Contributor

Good idea. Updated in 930ab80

global $wpdb;
$table_name = $wpdb->prefix . self::TABLE_NAME;
// Avoid ambigious column order_id in SQL query.
$this->report_columns['orders_count'] = str_replace( 'order_id', $table_name . '.order_id', $this->report_columns['orders_count'] );

This comment has been minimized.

@jeffstieler

jeffstieler Jan 14, 2019

Contributor

Same question about str_replace().

global $wpdb;
$table_name = $wpdb->prefix . self::TABLE_NAME;
// Avoid ambigious column order_id in SQL query.
$this->report_columns['orders_count'] = str_replace( 'order_id', $table_name . '.order_id', $this->report_columns['orders_count'] );

This comment has been minimized.

@jeffstieler

jeffstieler Jan 14, 2019

Contributor

Same question about str_replace().

$this->report_columns['tax_rate_id'] = $table_name . '.' . $this->report_columns['tax_rate_id'];
// Avoid ambigious columns in SQL query.
$this->report_columns['tax_rate_id'] = $table_name . '.' . $this->report_columns['tax_rate_id'];
$this->report_columns['orders_count'] = str_replace( 'order_id', $table_name . '.order_id', $this->report_columns['orders_count'] );

This comment has been minimized.

@jeffstieler

jeffstieler Jan 14, 2019

Contributor

Same question about str_replace().

global $wpdb;
$table_name = $wpdb->prefix . self::TABLE_NAME;
// Avoid ambigious column order_id in SQL query.
$this->report_columns['orders_count'] = str_replace( 'order_id', $table_name . '.order_id', $this->report_columns['orders_count'] );

This comment has been minimized.

@jeffstieler

jeffstieler Jan 14, 2019

Contributor

Same question about str_replace().

$order_status_filter = $this->get_status_subquery( $query_args );
if ( $order_status_filter ) {
$sql_query_params['from_clause'] .= " JOIN {$wpdb->prefix}wc_order_stats ON {$order_product_lookup_table}.order_id = {$wpdb->prefix}wc_order_stats.order_id";
$sql_query_params['where_clause'] .= " AND ( {$order_status_filter} )";

This comment has been minimized.

@jeffstieler

jeffstieler Jan 14, 2019

Contributor

Is there an opportunity to DRY this up? Perhaps a base "joins on wc_order_stats" class that handles the from_clause, where_clause, normalize_order_by(), and the orders_count column?

This comment has been minimized.

@joshuatf

joshuatf Jan 15, 2019

Author Contributor

Hmm, I'm not sure another class would be the best solution here. Report data stores already extend from WC_Admin_Reports_Data_Store, but quite a few also override the normalize_order_by() and other methods.

The bigger issue here is that there are about 3-4 different patterns being used in these data stores to append query parameters which makes it difficult to create a universal class that does not result in overriding in most data stores. For example, here are a few different variations just for the "where" clause:

$sql_query_params['where_clause'] .= " AND ( {$order_status_filter} )";

$where_subclause = implode( " $operator ", array_filter( array( $where_subclause, $order_status_filter ) ) );

$products_where_clause .= " AND ( {$order_status_filter} )";

$where_filters[] = $order_status_filter;

$sql_query_params['where_clause'] .= " AND ( {$order_status_filter} )";
$sql_query_params['where_clause'] .= ' AND variation_id > 0';

It seems like we could make a call outside of and adjacent to the get_sql_query_params() that might work for most data stores, but that would also be semantically incorrect and we'd be introducing more patterns that may come back to haunt us later.

I do agree that it would be nice if this were dryer, but it seems like a fairly large refactor that might be outside the scope of this PR. If we decided to do something like that, it might be worth breaking into bite-size chunks (like just refactoring get_sql_query_params() to use the same patterns) to create more manageable PRs.

@joshuatf joshuatf force-pushed the update/store-all-orders branch from 9c3ead0 to beac330 Jan 15, 2019

@joshuatf

This comment has been minimized.

Copy link
Contributor Author

joshuatf commented Jan 15, 2019

@jeffstieler Thanks for taking the time to review.

I'm seeing 165 results with status_is = 'failed'. There are only 33 rows in my wc_order_stats table with a status of wc-failed.. is this correct?

That's very possible if each of those failed 33 orders had a couple categories with a few products, but it could also be a bug. It's hard to say without seeing the data, but feel free to drop me a DB dump file and I can help investigate if you'd like.

@jeffstieler

This comment has been minimized.

Copy link
Contributor

jeffstieler commented Jan 16, 2019

That's very possible if each of those failed 33 orders had a couple categories with a few products, but it could also be a bug. It's hard to say without seeing the data, but feel free to drop me a DB dump file and I can help investigate if you'd like.

I double checked the report data and it all looks good. 👍

@joshuatf joshuatf merged commit 90473cf into master Jan 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joshuatf joshuatf deleted the update/store-all-orders branch Jan 16, 2019

@joshuatf joshuatf referenced this pull request Jan 16, 2019

Open

Data store refactor and pattern changes #1320

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment