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

Added segmenting support to REST API #1286

Merged
merged 61 commits into from Feb 1, 2019

Conversation

@peterfabian
Copy link
Contributor

peterfabian commented Jan 11, 2019

Fixes #1035.

First version of segmenting for /orders/stats (thus it should potentially also work for /revenue/stats, as that one uses the same data store).

Supports segmenting orders/stats by:

  • product: http://local.wordpress.test/wp-json/wc/v3/reports/orders/stats/?after=2018-01-11T12:52:01&before=2019-01-08T22:52:00&interval=month&order=desc&orderby=date&segmentby=product
  • variation (when one product is selected by a filter, e.g.: product_includes=X): http://local.wordpress.test/wp-json/wc/v3/reports/orders/stats/?after=2018-01-11T12:52:01&before=2019-01-08T22:52:00&interval=month&order=desc&orderby=date&segmentby=variation&product_includes=9
  • category: http://local.wordpress.test/wp-json/wc/v3/reports/orders/stats/?after=2018-01-11T12:52:01&before=2019-01-08T22:52:00&interval=month&order=desc&orderby=date&segmentby=category
  • coupon: http://local.wordpress.test/wp-json/wc/v3/reports/orders/stats/?after=2018-01-11T12:52:01&before=2019-01-08T22:52:00&interval=month&order=desc&orderby=date&segmentby=coupon
  • customer_type: http://local.wordpress.test/wp-json/wc/v3/reports/orders/stats/?after=2018-01-11T12:52:01&before=2019-01-08T22:52:00&interval=month&order=desc&orderby=date&segmentby=customer_type

Segmenting of products/stats by:

  • product
  • category
  • variation

Segmenting of coupons/stats by:

  • product
  • variation
  • coupon
  • category

Segmenting of taxes/stats by:

  • tax_rate_id
  • are other segments needed?

Detailed test instructions:

Notes:

  • At the moment, the paging and sorting only applies to intervals, not to segments (second dimension), i.e. it's adding all segments to totals and all intervals returned in the response. I'm not sure if that is a viable way to handle it, in case of 1000s of products or variations, it could be slow (?).
  • The segments are currently always sorted by segment_id--does this need to change somehow?
  • There is some creative math in some places, e.g. if shipping or coupons need to be split by product/variation, etc. Currently,
    • shipping and shipping_tax are split in wc_order_product_lookup according to item quantity, e.g. order 1 has 2 line items: 4 x product_1 and 3 x product_2. Shipping for product_1 is thus 4/7 * total shipping, for product_2 it's 3/7 of total shipping for the whole order.
    • coupon amount for wc_order_product_lookup is calculated as $order_item->get_subtotal( 'edit' ) - $order_item->get_total( 'edit' )
    • refunds are split based on line item id and handled in get_order_refund_items
  • I wasn't sure how the accounting should work in some cases, e.g. when segmenting /orders/stats by product, net_revenue and num_items_sold only include revenue/items for particular product, but avg_items_per_order and avg_order_value still refer to the whole order (in the sense that asking 'what is the average number of items per order for orders which contain product X' should probably include other products). I suggest creating a new issue/feature request, if this should be implemented differently.
  • I'm not sure all the edge cases are handled correctly, things to consider for /orders/stats (I plan to cover these by unit tests):
    • 2 records with the same product_id could appear in one order, e.g. when 2 variations of the same product are bought in the same order
    • 2 records with same product_id and variation_id could appear in one order, e.g. when hoodie variation is color=blue and size=any, then buying 2 sizes of blue hoodie will create this situation
    • multiple coupons can be assigned to one order
    • products can belong to multiple categories

TODOs:

  • update orders/stats to use new 'definition' of new/returning customer

peterfabian added some commits Dec 20, 2018

Merge branch 'master' into fix/1035
# Conflicts:
#	includes/wc-admin-order-functions.php
Added back updates to product lookup update routine after wc-admin-or…
…der-functions.php was removed and code moved to individual data stores.
FOrmula for shipping distribution among line items updated to reflect…
… qty, so that sum of shipping per line item equals total shipping amount.
@claudiosanches

This comment has been minimized.

Copy link
Member

claudiosanches commented Jan 24, 2019

I just read all the code and looks good, just see that need update TODO to @todo, so we can use any tool to find it later, of even searching in the editor makes hard if we use multiple formats.
Now going to run this code.

@peterfabian

This comment has been minimized.

Copy link
Contributor Author

peterfabian commented Jan 25, 2019

Work on segmenting for taxes/stats depends on #1394.

@timmyc

This comment has been minimized.

Copy link
Collaborator

timmyc commented Jan 31, 2019

Is this ready for another review @peterfabian ?

@peterfabian

This comment has been minimized.

Copy link
Contributor Author

peterfabian commented Jan 31, 2019

Yes please! @timmyc

@timmyc

timmyc approved these changes Jan 31, 2019

Copy link
Collaborator

timmyc left a comment

@peterfabian I think we should get this in once it gets rebased - even though Github is saying it can be merged in okay, when attempt to rebase locally it seems to have a few conflicts.

@justinshreve
Copy link
Collaborator

justinshreve left a comment

I have a few minor comments related to the schema, but otherwise this is looking really good to me! All phpunit tests pass. Pre-approving pending those minor changes.

I've also been testing product/variation segmentation heavily with #1391 which has been working great.

'properties' => array(
'segment_id' => array(
'description' => __( 'Segment identificator.', 'wc-admin' ),
'type' => 'string',

This comment has been minimized.

@justinshreve

justinshreve Feb 1, 2019

Collaborator

segment_id should be an int.

'type' => 'string',
'context' => array( 'view', 'edit' ),
'readonly' => true,
'enum' => array( 'day', 'week', 'month', 'year' ),

This comment has been minimized.

@justinshreve

justinshreve Feb 1, 2019

Collaborator

The value for this will be an object ID, right? So we can remove this.

'properties' => array(
'segment_id' => array(
'description' => __( 'Segment identificator.', 'wc-admin' ),
'type' => 'string',

This comment has been minimized.

@justinshreve

justinshreve Feb 1, 2019

Collaborator

Same changes here for int type and no enum.

'properties' => array(
'segment_id' => array(
'description' => __( 'Segment identificator.', 'wc-admin' ),
'type' => 'string',

This comment has been minimized.

@justinshreve

justinshreve Feb 1, 2019

Collaborator

Same changes here for int type and no enum.

'properties' => array(
'segment_id' => array(
'description' => __( 'Segment identificator.', 'wc-admin' ),
'type' => 'string',

This comment has been minimized.

@justinshreve

justinshreve Feb 1, 2019

Collaborator

Same changes here for int type and no enum.

'properties' => array(
'segment_id' => array(
'description' => __( 'Segment identificator.', 'wc-admin' ),
'type' => 'string',

This comment has been minimized.

@justinshreve

justinshreve Feb 1, 2019

Collaborator

Same changes here for int type and no enum.

@peterfabian

This comment has been minimized.

Copy link
Contributor Author

peterfabian commented Feb 1, 2019

@timmyc as I prefer merging over rebasing, so I merged master into this branch whenever a conflict appeared. Last time master was merged is 3 days ago. If you think it's a good idea, I can rebase it, but I would prefer not to, unless necessary, as I used merging throughout the life of the branch.

@peterfabian

This comment has been minimized.

Copy link
Contributor Author

peterfabian commented Feb 1, 2019

Thanks @justinshreve . Should be solved by 994c9b8.

@justinshreve

This comment has been minimized.

Copy link
Collaborator

justinshreve commented Feb 1, 2019

Thanks! Looks good to me! 👍

@justinshreve

This comment has been minimized.

Copy link
Collaborator

justinshreve commented Feb 1, 2019

As I prefer merging over rebasing, so I merged master into this branch whenever a conflict appeared. Last time master was merged is 3 days ago. If you think it's a good idea, I can rebase it, but I would prefer not to, unless necessary, as I used merging throughout the life of the branch.

I think the conflicts Timmy saw are from earlier commits (since git rebase master applies commits one at a time) -- so I agree it would be more trouble then it is worth to rebase this.

Since GitHub here is clean, I would say go ahead and merge 👍.

@claudiosanches
Copy link
Member

claudiosanches left a comment

Great work!

@peterfabian peterfabian merged commit d63dc6a into master Feb 1, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment