Merge lp:~sam92/openlp/bug-1552563 into lp:openlp

[Samuel Mehrbrodt]

Fix performance regression with Songbook search

The problem was that in each iteration the database was accessed (the song object).
Fixed this by loading all neccessary information directly in the query.

[Samuel Mehrbrodt (sam92)]

lp:~sam92/openlp/bug-1552563 (revision 2659)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1508/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1419/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1357/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1153/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/744/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/811/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/679/

[Raoul Snyman (raoul-snyman)]

review: Approve

[Tim Bentley (trb143)]

review: Approve

=== modified file 'openlp/plugins/songs/lib/mediaitem.py'
--- openlp/plugins/songs/lib/mediaitem.py	2016-04-25 11:01:32 +0000
+++ openlp/plugins/songs/lib/mediaitem.py	2016-04-29 17:34:59 +0000
@@ -21,7 +21,6 @@
 ###############################################################################
 
 import logging
-import re
 import os
 import shutil
 
@@ -207,9 +206,11 @@
             search_keywords = search_keywords.rpartition(' ')
             search_book = search_keywords[0] + '%'
             search_entry = search_keywords[2] + '%'
-            search_results = (self.plugin.manager.session.query(SongBookEntry)
+            search_results = (self.plugin.manager.session.query(SongBookEntry.entry, Book.name, Song.title, Song.id)
+                              .join(Song)
                               .join(Book)
-                              .filter(Book.name.like(search_book), SongBookEntry.entry.like(search_entry)).all())
+                              .filter(Book.name.like(search_book), SongBookEntry.entry.like(search_entry),
+                                      Song.temporary.is_(False)).all())
             self.display_results_book(search_results)
         elif search_type == SongSearch.Themes:
             log.debug('Theme Search')
@@ -313,23 +314,20 @@
         """
         Display the song search results in the media manager list, grouped by book and entry
 
-        :param search_results: A list of db SongBookEntry objects
+        :param search_results: A tuple containing (songbook entry, book name, song title, song id)
         :return: None
         """
-        def get_songbook_key(songbook_entry):
+        def get_songbook_key(result):
             """Get the key to sort by"""
-            return (get_natural_key(songbook_entry.songbook.name), get_natural_key(songbook_entry.entry))
+            return (get_natural_key(result[1]), get_natural_key(result[0]), get_natural_key(result[2]))
 
         log.debug('display results Book')
         self.list_view.clear()
         search_results.sort(key=get_songbook_key)
-        for songbook_entry in search_results:
-            # Do not display temporary songs
-            if songbook_entry.song.temporary:
-                continue
-            song_detail = '%s #%s: %s' % (songbook_entry.songbook.name, songbook_entry.entry, songbook_entry.song.title)
+        for result in search_results:
+            song_detail = '%s #%s: %s' % (result[1], result[0], result[2])
             song_name = QtWidgets.QListWidgetItem(song_detail)
-            song_name.setData(QtCore.Qt.UserRole, songbook_entry.song.id)
+            song_name.setData(QtCore.Qt.UserRole, result[3])
             self.list_view.addItem(song_name)
 
     def display_results_topic(self, search_results):

=== modified file 'tests/functional/openlp_plugins/songs/test_mediaitem.py'
--- tests/functional/openlp_plugins/songs/test_mediaitem.py	2016-04-17 21:22:30 +0000
+++ tests/functional/openlp_plugins/songs/test_mediaitem.py	2016-04-29 17:34:59 +0000
@@ -23,6 +23,7 @@
 This module contains tests for the lib submodule of the Songs plugin.
 """
 from unittest import TestCase
+from unittest.mock import call
 
 from PyQt5 import QtCore
 
@@ -151,29 +152,7 @@
         # 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_entry_temp = MagicMock()
-            mock_songbook = MagicMock()
-            mock_song = MagicMock()
-            mock_song_temp = MagicMock()
-            mock_songbook_entry.entry = '1'
-            mock_songbook_entry_temp.entry = '2'
-            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_song_temp.id = 2
-            mock_song_temp.title = 'My Temporary'
-            mock_song_temp.sort_key = 'My Temporary'
-            mock_song_temp.temporary = True
-            mock_songbook_entry.song = mock_song
-            mock_songbook_entry.songbook = mock_songbook
-            mock_songbook_entry_temp.song = mock_song_temp
-            mock_songbook_entry_temp.songbook = mock_songbook
-            mock_search_results.append(mock_songbook_entry)
-            mock_search_results.append(mock_songbook_entry_temp)
+            mock_search_results = [('1', 'My Book', 'My Song', 1)]
             mock_qlist_widget = MagicMock()
             MockedQListWidgetItem.return_value = mock_qlist_widget
 
@@ -183,9 +162,35 @@
             # 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_once_with('My Book #1: My Song')
-            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, mock_songbook_entry.song.id)
+            mock_qlist_widget.setData.assert_called_once_with(MockedUserRole, 1)
             self.media_item.list_view.addItem.assert_called_once_with(mock_qlist_widget)
 
+    def songbook_natural_sorting_test(self):
+        """
+        Test that songbooks are sorted naturally
+        """
+        # GIVEN: Search results grouped by book and entry, plus a mocked QtListWidgetItem
+        with patch('openlp.core.lib.QtWidgets.QListWidgetItem') as MockedQListWidgetItem:
+            mock_search_results = [('2', 'Thy Book', 'Thy Song', 50),
+                                   ('2', 'My Book', 'Your Song', 7),
+                                   ('10', 'My Book', 'Our Song', 12),
+                                   ('1', 'My Book', 'My Song', 1),
+                                   ('2', 'Thy Book', 'A Song', 8)]
+            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 songbooks are inserted in the right (natural) order,
+            #       grouped first by book, then by number, then by song title
+            calls = [call('My Book #1: My Song'), call().setData(QtCore.Qt.UserRole, 1),
+                     call('My Book #2: Your Song'), call().setData(QtCore.Qt.UserRole, 7),
+                     call('My Book #10: Our Song'), call().setData(QtCore.Qt.UserRole, 12),
+                     call('Thy Book #2: A Song'), call().setData(QtCore.Qt.UserRole, 8),
+                     call('Thy Book #2: Thy Song'), call().setData(QtCore.Qt.UserRole, 50)]
+            MockedQListWidgetItem.assert_has_calls(calls)
+
     def display_results_topic_test(self):
         """
         Test displaying song search results grouped by topic with basic song

=== modified file 'tests/functional/openlp_plugins/songs/test_openlpimporter.py'
--- tests/functional/openlp_plugins/songs/test_openlpimporter.py	2016-04-27 21:23:16 +0000
+++ tests/functional/openlp_plugins/songs/test_openlpimporter.py	2016-04-29 17:34:59 +0000
@@ -73,4 +73,3 @@
                 self.assertIsNone(importer.do_import(), 'do_import should return None when import_source is not a list')
                 self.assertEqual(mocked_import_wizard.progress_bar.setMaximum.called, False,
                                  'setMaximum on import_wizard.progress_bar should not have been called')
-