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.

Sign up for GitHub

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 sort-by with null values #377

Merged
merged 2 commits into from
Jul 5, 2020
Merged

Fix sort-by with null values #377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
68f1604
sort-by should sort null values last as well
wongpeiyi Jun 24, 2020
f3e1f77
Improve sort-by undefined values test
wongpeiyi Jun 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions addon/helpers/sort-by.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ function sortDesc(key, a, b) {
const aValue = get(a, key);
const bValue = get(b, key);

if (typeof bValue == 'undefined') {
if (typeof bValue == 'undefined' || bValue === null) {
// keep bValue last
return -1;
}
if (typeof aValue == 'undefined') {
if (typeof aValue == 'undefined' || aValue === null) {
// put aValue last
return 1;
}
Expand All @@ -47,11 +47,11 @@ function sortAsc(key, a, b) {
const aValue = get(a, key);
const bValue = get(b, key);

if (typeof bValue == 'undefined') {
if (typeof bValue == 'undefined' || bValue === null) {
// keep bValue last
return -1;
}
if (typeof aValue == 'undefined') {
if (typeof aValue == 'undefined' || aValue === null) {
// put aValue last
return 1;
}
Expand Down
33 changes: 25 additions & 8 deletions tests/integration/helpers/sort-by-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,21 +301,38 @@ module('Integration | Helper | {{sort-by}}', function(hooks) {
assert.equal(find('*').textContent.trim(), 'abc', 'cab is sorted to abc');
});

test('ignores undefined values sorting', async function (assert) {
test('it sorts undefined values last', async function(assert) {
this.set('array', [
{ name: 'c' },
{ name: 'a' },
{ name: undefined },
Copy link
Contributor

Choose a reason for hiding this comment

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

One behaviour of sort was to remove undefined values. We would probably want to keep this sort of test around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, not sure I understand -- what do you mean by "remove undefined values"?

This merely changes the test to assert that the undefined value is at the end of the resulting array. At present the test will assert "abc" regardless of what position undefined is in the resulting array since undefined renders nothing, so textContent will be abc no matter what

You could change sort-by to sort undefined first, sort undefined last, remove undefineds or even randomly add a bunch of undefineds, and this test will always pass before this change, so it doesn't really test anything at all

Copy link
Contributor

Choose a reason for hiding this comment

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

See original assertion...

assert.equal(find('*').textContent.trim(), 'abc');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I've seen that and I still don't understand. I've been relooking the test over and over again, could you please elaborate in more detail?

If sort-by returns any of these results, textContent is still 'abc':

{{#each (array "a" "b" "c") as |str|}}{{str}}{{/each}}
textContent === 'abc'

{{#each (array "a" "b" undefined "c") as |str|}}{{str}}{{/each}}
textContent === 'abc'

{{#each (array undefined "a" undefined undefined "b" undefined "c" undefined) as |str|}}{{str}}{{/each}}
textContent === 'abc'

I'm not sure what the original assertion is testing for?

Put another way: what result of sort-by would cause this assertion to fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

We simply want the original test around. The test assertions you added should be in a new test

{ name: 'b' },
{ id: 1, name: 'c' },
{ id: 2, name: 'a' },
{ id: 3, name: undefined },
{ id: 4, name: 'b' },
]);

await render(hbs`
{{~#each (sort-by 'name' array) as |pet|~}}
{{~pet.name~}}
{{~#each (sort-by 'name' array) as |user|~}}
{{~user.id~}}
{{~/each~}}
`);

assert.equal(find('*').textContent.trim(), '2413');
});

test('it sorts null values last', async function(assert) {
this.set('array', [
{ id: 1, name: 'c' },
{ id: 2, name: 'a' },
{ id: 3, name: null },
{ id: 4, name: 'b' },
]);

await render(hbs`
{{~#each (sort-by 'name' array) as |user|~}}
{{~user.id~}}
{{~/each~}}
`);

assert.equal(find('*').textContent.trim(), 'abc');
assert.equal(find('*').textContent.trim(), '2413');
});

test('It maintains order when values are the same', async function(assert) {
Expand Down