openlp-core team mailing list archive

[Chris Hill <chris@xxxxxxxxxxxx>]

Chris Hill has proposed merging lp:~minkus/openlp/naturalsortsongs into lp:openlp.

Commit message:
Fix bug #1280295 - 'Enable natural sorting for song book searches', refactors for database querying

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1280295 in OpenLP: "Enable natural sorting for song book searches"
  https://bugs.launchpad.net/openlp/+bug/1280295

For more details, see:
https://code.launchpad.net/~minkus/openlp/naturalsortsongs/+merge/290815

Fixes bug #1280295 - 'Enable natural sorting for song book searches' using get_natural_key
Now also includes natural sorting for author, topic, theme & CCLI number as well
Also refactors Songbook Search to make the database do filtering for performance rather than querying all then filtering
I've tested it on my database of 500-odd songs and it seems a little faster
Includes unit tests

--------------------------------
lp:~minkus/openlp/naturalsortsongs (revision 2519)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1376/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1294/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1233/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1065/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/656/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/723/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/591/
-- 
Your team OpenLP Core is requested to review the proposed merge of lp:~minkus/openlp/naturalsortsongs into lp:openlp.

=== modified file 'openlp/plugins/songs/lib/mediaitem.py'
--- openlp/plugins/songs/lib/mediaitem.py	2016-03-13 18:37:08 +0000
+++ openlp/plugins/songs/lib/mediaitem.py	2016-04-03 11:20:32 +0000
@@ -32,6 +32,7 @@
 from openlp.core.lib import MediaManagerItem, ItemCapabilities, PluginStatus, ServiceItemContext, \
     check_item_selected, create_separated_list
 from openlp.core.lib.ui import create_widget_action
+from openlp.core.utils import get_natural_key
 from openlp.plugins.songs.forms.editsongform import EditSongForm
 from openlp.plugins.songs.forms.songmaintenanceform import SongMaintenanceForm
 from openlp.plugins.songs.forms.songimportform import SongImportForm
@@ -203,7 +204,13 @@
             self.display_results_topic(search_results)
         elif search_type == SongSearch.Books:
             log.debug('Songbook Search')
-            self.display_results_book(search_keywords)
+            search_keywords = search_keywords.rpartition(' ')
+            search_book = search_keywords[0] + '%'
+            search_entry = search_keywords[2] + '%'
+            search_results = (self.plugin.manager.session.query(SongBookEntry)
+                              .join(Book)
+                              .filter(Book.name.like(search_book), SongBookEntry.entry.like(search_entry)).all())
+            self.display_results_book(search_results)
         elif search_type == SongSearch.Themes:
             log.debug('Theme Search')
             search_string = '%' + search_keywords + '%'
@@ -278,8 +285,10 @@
         """
         log.debug('display results Author')
         self.list_view.clear()
+        search_results = sorted(search_results, key=lambda author: get_natural_key(author.display_name))
         for author in search_results:
-            for song in author.songs:
+            songs = sorted(author.songs, key=lambda song: song.sort_key)
+            for song in songs:
                 # Do not display temporary songs
                 if song.temporary:
                     continue
@@ -288,32 +297,20 @@
                 song_name.setData(QtCore.Qt.UserRole, song.id)
                 self.list_view.addItem(song_name)
 
-    def display_results_book(self, search_keywords):
+    def display_results_book(self, search_results):
         """
-        Display the song search results in the media manager list, grouped by book
+        Display the song search results in the media manager list, grouped by book and entry
 
-        :param search_keywords: A list of search keywords - book first, then number
+        :param search_results: A list of db SongBookEntry objects
         :return: None
         """
-
         log.debug('display results Book')
         self.list_view.clear()
-
-        search_keywords = search_keywords.rpartition(' ')
-        search_book = search_keywords[0]
-        search_entry = re.sub(r'[^0-9]', '', search_keywords[2])
-
-        songbook_entries = (self.plugin.manager.session.query(SongBookEntry)
-                            .join(Book)
-                            .order_by(Book.name)
-                            .order_by(SongBookEntry.entry))
-        for songbook_entry in songbook_entries:
+        search_results = sorted(search_results, key=lambda songbook_entry:
+                                (get_natural_key(songbook_entry.songbook.name), get_natural_key(songbook_entry.entry)))
+        for songbook_entry in search_results:
             if songbook_entry.song.temporary:
                 continue
-            if search_book.lower() not in songbook_entry.songbook.name.lower():
-                continue
-            if search_entry not in songbook_entry.entry:
-                continue
             song_detail = '%s #%s: %s' % (songbook_entry.songbook.name, songbook_entry.entry, songbook_entry.song.title)
             song_name = QtWidgets.QListWidgetItem(song_detail)
             song_name.setData(QtCore.Qt.UserRole, songbook_entry.song.id)
@@ -328,7 +325,7 @@
         """
         log.debug('display results Topic')
         self.list_view.clear()
-        search_results = sorted(search_results, key=lambda topic: self._natural_sort_key(topic.name))
+        search_results = sorted(search_results, key=lambda topic: get_natural_key(topic.name))
         for topic in search_results:
             songs = sorted(topic.songs, key=lambda song: song.sort_key)
             for song in songs:
@@ -349,6 +346,8 @@
         """
         log.debug('display results Themes')
         self.list_view.clear()
+        search_results = sorted(search_results, key=lambda song: (get_natural_key(song.theme_name),
+                                song.sort_key))
         for song in search_results:
             # Do not display temporary songs
             if song.temporary:
@@ -367,7 +366,8 @@
         """
         log.debug('display results CCLI number')
         self.list_view.clear()
-        songs = sorted(search_results, key=lambda song: self._natural_sort_key(song.ccli_number))
+        songs = sorted(search_results, key=lambda song: (get_natural_key(song.ccli_number),
+                       song.sort_key))
         for song in songs:
             # Do not display temporary songs
             if song.temporary:
@@ -694,14 +694,6 @@
         # List must be empty at the end
         return not author_list
 
-    def _natural_sort_key(self, s):
-        """
-        Return a tuple by which s is sorted.
-        :param s: A string value from the list we want to sort.
-        """
-        return [int(text) if text.isdecimal() else text.lower()
-                for text in re.split('(\d+)', s)]
-
     def search(self, string, show_error):
         """
         Search for some songs

=== modified file 'tests/functional/openlp_plugins/songs/test_mediaitem.py'
--- tests/functional/openlp_plugins/songs/test_mediaitem.py	2016-03-13 18:37:08 +0000
+++ tests/functional/openlp_plugins/songs/test_mediaitem.py	2016-04-03 11:20:32 +0000
@@ -127,6 +127,38 @@
             mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_song.id)
             self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
 
+    def display_results_book_test(self):
+        """
+        Test displaying song search results grouped by book and entry with basic song
+        """
+        # GIVEN: Search results grouped by book and entry, plus a mocked QtListWidgetItem
+        with patch('openlp.core.lib.QtWidgets.QListWidgetItem') as MockedQListWidgetItem, \
+                patch('openlp.core.lib.QtCore.Qt.UserRole') as MockedUserRole:
+            mock_search_results = []
+            mock_songbook_entry = MagicMock()
+            mock_songbook = MagicMock()
+            mock_song = MagicMock()
+            mock_songbook_entry.entry = '1'
+            mock_songbook.name = 'My Book'
+            mock_song.id = 1
+            mock_song.title = 'My Song'
+            mock_song.sort_key = 'My Song'
+            mock_song.temporary = False
+            mock_songbook_entry.song = mock_song
+            mock_songbook_entry.songbook = mock_songbook
+            mock_search_results.append(mock_songbook_entry)
+            mock_qlist_widget = MagicMock()
+            MockedQListWidgetItem.return_value = mock_qlist_widget
+
+            # WHEN: I display song search results grouped by book
+            self.media_item.display_results_book(mock_search_results)
+
+            # THEN: The current list view is cleared, the widget is created, and the relevant attributes set
+            self.media_item.list_view.clear.assert_called_with()
+            MockedQListWidgetItem.assert_called_with('My Book #1: My Song')
+            mock_qlist_widget.setData.assert_called_with(MockedUserRole, mock_songbook_entry.song.id)
+            self.media_item.list_view.addItem.assert_called_with(mock_qlist_widget)
+
     def display_results_topic_test(self):
         """
         Test displaying song search results grouped by topic with basic song
@@ -416,19 +448,6 @@
         # THEN: They should not match
         self.assertFalse(result, "Authors should not match")
 
-    def natural_sort_key_test(self):
-        """
-        Test the _natural_sort_key function
-        """
-        # GIVEN: A string to be converted into a sort key
-        string_sort_key = 'A1B12C'
-
-        # WHEN: We attempt to create a sort key
-        sort_key_result = self.media_item._natural_sort_key(string_sort_key)
-
-        # THEN: We should get back a tuple split on integers
-        self.assertEqual(sort_key_result, ['a', 1, 'b', 12, 'c'])
-
     def build_remote_search_test(self):
         """
         Test results for the remote search api