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

REST API: Allow sorting by stock_status in stocks endpoint #1335

Merged
merged 5 commits into from Jan 21, 2019

Conversation

Projects
None yet
3 participants
@joshuatf
Copy link
Contributor

joshuatf commented Jan 17, 2019

Fixes #966

Allows sorting by stock_status on stocks endpoint.

Detailed test instructions:

  1. Make a request to /wp-json/wc/v3/reports/stock
  2. Pass stock_status as the orderby param.
  3. Check that sorting returns by status.
  4. Change the order param to desc and check that sorting by status is reversed.

@joshuatf joshuatf self-assigned this Jan 17, 2019

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

@Aljullu
Copy link
Contributor

Aljullu left a comment

This is testing well for me. But would it be possible to use stock_quantity as the second sort value?

Sorting by stock_status with order=desc, Out of stock products appears on top, so I would expect the products with a lower stock_quantity to appear above:

image

Same, when sorting in the other direction (when In stock products appear on top), I would expect products with the higher stock_quantity to appear above).

I don't know if that's possible, but that's what I would expect as a user.

I created #1340 as a follow-up of this PR, it updates the UI to use stock_status by default. Feel free to merge in into your branch.

@joshuatf

This comment has been minimized.

Copy link
Contributor Author

joshuatf commented Jan 18, 2019

@Aljullu That seems reasonable. I added a commit to do this and also reversed the order of status quantity in 3ca9b94 since it seems like you'd want to see the larger (absolutely bigger values) stock values first when sorting with this method. Let me know what you think.

@Aljullu
Copy link
Contributor

Aljullu left a comment

Thanks for making these changes @joshuatf, it looks much better now. The only last issue I noticed is that you need to click two times on the Status header to reverse the order, now:

status

@claudiosanches
Copy link
Member

claudiosanches left a comment

PHP part looks good.

@joshuatf

This comment has been minimized.

Copy link
Contributor Author

joshuatf commented Jan 21, 2019

Thanks for reviewing @Aljullu and @claudiosanches!

The only last issue I noticed is that you need to click two times on the Status header to reverse the order, now:

I noticed that issue across a number of reports as it seems the order is disconnected from tableQuery and query. Opened a separate issue here to handle that. Let me know if that sounds okay to you.

@Aljullu
Copy link
Contributor

Aljullu left a comment

I noticed that issue across a number of reports as it seems the order is disconnected from tableQuery and query. Opened a separate issue here to handle that. Let me know if that sounds okay to you.

Thanks for filling an issue for that @joshuatf!

@joshuatf joshuatf merged commit e42c5a0 into master Jan 21, 2019

1 check passed

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

@joshuatf joshuatf deleted the update/allow-stock-status-sort branch Jan 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment