silverstripe / silverstripe-graphql

Serves Silverstripe data as GraphQL representations
BSD 3-Clause "New" or "Revised" License
52 stars 61 forks source link

fix(SortPlugin): amend sorting to all fields #541

Closed flamerohr closed 1 year ago

flamerohr commented 1 year ago

Fix sorting to all fields in the args.

This is because in framework DataList each call to ->sort() actually resets previous sorts made, and the result is that only the end sort happens.

Issue

GuySartorelli commented 1 year ago

Thank you for this. This feels like something which ought to be tested. Could you please add a unit test to validate the sort args are being correctly applied?

flamerohr commented 1 year ago

is an existing unit test for this plugin I can start writing the tests on? I'm not familiar enough with the test setup to jump right in

GuySartorelli commented 1 year ago

Looks like IntegrationTest class in tests/Schema/IntegrationTest.php has a testFilterAndSort() method which is probably an appropriate place for this. There are also some pre-existing test schemas with sorts applied: https://github.com/search?q=repo%3Asilverstripe%2Fsilverstripe-graphql+sort+path%3A%2F%5Etests%5C%2F%2F&type=code

flamerohr commented 1 year ago

@GuySartorelli test added, the diff looks a bit weird because I needed to tweak the other tests to accomodate to a third row added to the db

GuySartorelli commented 1 year ago

Looks good, thank you!