Skip to content

Commit

Permalink
Fix sorting of track numbers when case insensitive
Browse files Browse the repository at this point in the history
`LOWER()` implicitly converted numerical columns to strings,
causing the ordering of ['1', '10', '2'] to be correct.

The type of the column is now checked in the sql query
using `CASE WHEN..` construct. This ensures the column is
only `LOWER()`'d when dealing with TEXT or BLOB.

- Add a test to check for the track number behavior
- Add changelog entry

Fix #1511
  • Loading branch information
tomjaspers committed Jul 5, 2015
1 parent cc03c55 commit 18bd447
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 4 deletions.
5 changes: 4 additions & 1 deletion beets/dbcore/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -766,7 +766,10 @@ class FixedFieldSort(FieldSort):
def order_clause(self):
order = "ASC" if self.ascending else "DESC"
if self.case_insensitive:
field = 'LOWER({})'.format(self.field)
field = '(CASE ' \
'WHEN TYPEOF({0})="text" THEN LOWER({0}) ' \
'WHEN TYPEOF({0})="blob" THEN LOWER({0}) ' \
'ELSE {0} END)'.format(self.field)
else:
field = self.field
return "{0} {1}".format(field, order)
Expand Down
1 change: 1 addition & 0 deletions docs/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Fixes:
:user:`Somasis`. :bug:`1512`
* Some messages in the console UI now use plural nouns correctly. Thanks to
:user:`JesseWeinstein`. :bug:`1521`
* Sorting numerical fields (such as track) now works again. :bug:`1511`


1.3.13 (April 24, 2015)
Expand Down
22 changes: 19 additions & 3 deletions test/test_sort.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,21 @@ def setUp(self):
albums = [_common.album() for _ in range(3)]
albums[0].album = "Album A"
albums[0].genre = "Rock"
albums[0].year = "2001"
albums[0].year = 2001
albums[0].flex1 = "Flex1-1"
albums[0].flex2 = "Flex2-A"
albums[0].albumartist = "Foo"
albums[0].albumartist_sort = None
albums[1].album = "Album B"
albums[1].genre = "Rock"
albums[1].year = "2001"
albums[1].year = 2001
albums[1].flex1 = "Flex1-2"
albums[1].flex2 = "Flex2-A"
albums[1].albumartist = "Bar"
albums[1].albumartist_sort = None
albums[2].album = "Album C"
albums[2].genre = "Jazz"
albums[2].year = "2005"
albums[2].year = 2005
albums[2].flex1 = "Flex1-1"
albums[2].flex2 = "Flex2-B"
albums[2].albumartist = "Baz"
Expand All @@ -67,6 +67,7 @@ def setUp(self):
items[0].album_id = albums[0].id
items[0].artist_sort = None
items[0].path = "/path0.mp3"
items[0].track = 1
items[1].title = 'Baz qux'
items[1].artist = 'Two'
items[1].album = 'Baz'
Expand All @@ -77,6 +78,7 @@ def setUp(self):
items[1].album_id = albums[0].id
items[1].artist_sort = None
items[1].path = "/patH1.mp3"
items[1].track = 2
items[2].title = 'Beets 4 eva'
items[2].artist = 'Three'
items[2].album = 'Foo'
Expand All @@ -87,6 +89,7 @@ def setUp(self):
items[2].album_id = albums[1].id
items[2].artist_sort = None
items[2].path = "/paTH2.mp3"
items[2].track = 3
items[3].title = 'Beets 4 eva'
items[3].artist = 'Three'
items[3].album = 'Foo2'
Expand All @@ -97,6 +100,7 @@ def setUp(self):
items[3].album_id = albums[2].id
items[3].artist_sort = None
items[3].path = "/PATH3.mp3"
items[3].track = 4
for item in items:
self.lib.add(item)

Expand Down Expand Up @@ -399,6 +403,7 @@ def setUp(self):
item.flex2 = "flex2-A"
item.album_id = album.id
item.artist_sort = None
item.track = 10
self.lib.add(item)

self.new_album = album
Expand Down Expand Up @@ -451,6 +456,17 @@ def test_flex_field_case_sensitive(self):
self.assertEqual(results[0].flex1, 'Flex1-0')
self.assertEqual(results[-1].flex1, 'flex1')

def test_case_sensitive_only_affects_text(self):
config['sort_case_insensitive'] = True
q = 'track+'
results = list(self.lib.items(q))
# If the numerical values were sorted as strings,
# then ['1', '10', '2'] would be valid.
print([r.track for r in results])
self.assertEqual(results[0].track, 1)
self.assertEqual(results[1].track, 2)
self.assertEqual(results[-1].track, 10)


def suite():
return unittest.TestLoader().loadTestsFromName(__name__)
Expand Down

0 comments on commit 18bd447

Please sign in to comment.