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/289566

Fixes bug 1280295 - 'Enable natural sorting for song book searches'
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 2510)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1326/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1248/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1187/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1022/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/613/
[FAILURE] https://ci.openlp.io/job/Branch-05a-Code_Analysis/680/
Stopping after failure
-- 
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-03-19 15:24:23 +0000
@@ -203,7 +203,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 + '%'
@@ -288,32 +294,19 @@
                 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: (songbook_entry.songbook.name, self._natural_sort_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)

=== 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-03-19 15:24:23 +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