Skip to content
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

FIX Add test cases for SortPlugin #564

Merged

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova commented Nov 26, 2023

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

You have only one test validating that this sort plugin works when sorting multiple fields at once:
files(sort: { name: ASC, title: DESC }) {

Please also add one with those fields reversed, to validate that the order in which the fields are added in the query changes the actual sort order.
files(sort: { title: DESC, name: ASC }) {
Make sure this new scenario gives different results than the existing one.

@sabina-talipova sabina-talipova force-pushed the pulls/4.3/sortplugin-tests branch 4 times, most recently from b2e9018 to 94863fe Compare November 27, 2023 06:01
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Since this is now a bugfix, please update the commit message to use the FIX prefix and to have a message that explains what bug is being fixed.

src/Schema/Plugin/SortPlugin.php Outdated Show resolved Hide resolved
src/Schema/Plugin/SortPlugin.php Show resolved Hide resolved
src/Schema/Plugin/SortPlugin.php Outdated Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4.3/sortplugin-tests branch 3 times, most recently from d7cf2eb to 2cde1a1 Compare November 27, 2023 21:19
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Just one small change - everything else looks good.

src/Schema/Traits/SortTrait.php Show resolved Hide resolved
@sabina-talipova sabina-talipova force-pushed the pulls/4.3/sortplugin-tests branch from 2cde1a1 to f929eb6 Compare November 27, 2023 22:14
@sabina-talipova sabina-talipova changed the title MNT Add test cases for SortPlugin FIX Add test cases for SortPlugin Nov 27, 2023
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

@GuySartorelli GuySartorelli merged commit d677587 into silverstripe:4.3 Nov 27, 2023
12 checks passed
@GuySartorelli GuySartorelli deleted the pulls/4.3/sortplugin-tests branch November 27, 2023 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants