Merge lp://qastaging/~samothjtm/openlp/bug-1695587 into lp://qastaging/openlp

Hi Johannes,

Thank you for contributing to OpenLP!
To get your code merged you must create a test that proves it actually works. Have a look at the wiki for inspiration: https://wiki.openlp.org/Development:Unit_Tests

I did some manual testing...

My new code is only using one database call - I can't find an example of how and if at all any database calls are being tested. I'd need some help on this... is there a test database with example data that I can just query to proof my code working?

There are still some other tests failing not done by me - even after merging the trunk with some test fixing

lp:~samothjtm/openlp/bug-1695587 (revision 2747)
[←[1;32mSUCCESS←[1;m] https://ci.openlp.io/job/Branch-01-Pull/2060/
[←[1;31mFAILURE←[1;m] https://ci.openlp.io/job/Branch-02-Functional-Tests/1970/
Stopping after failure

I did a test search with 1629 songs in a Database taking aproximately 1 second searching for "1" in Entire Song with my patch. It only seems to take this long to load all the results into the GUI as searching for "11" takes only an instant...

I've got a couple of comments below for you to take a look at.

Also, as previously noted, you need a code test before we'll accept your merge proposal. I'm afraid that manual testing, while nice, does not constitute a test for merging purposes.

Here, I've fixed your issues for you and written a test for you. Next time it's on you.

=== modified file 'openlp/plugins/songs/lib/mediaitem.py'
--- openlp/plugins/songs/lib/mediaitem.py 2017-06-03 10:44:28 +0000
+++ openlp/plugins/songs/lib/mediaitem.py 2017-06-04 20:18:23 +0000
@@ -231,13 +231,13 @@

     def search_entire(self, search_keywords):
         search_string = '%{text}%'.format(text=clean_string(search_keywords))
- return (self.plugin.manager.session.query(Song)
- .join(SongBookEntry, isouter=True)
- .join(Book, isouter=True)
- .filter(or_(Book.name.like(search_string), SongBookEntry.entry.like(search_string),
- Song.search_title.like(search_string), Song.search_lyrics.like(search_string),
- Song.comments.like(search_string), Song.alternate_title.like(search_string)))
- .all())
+ return self.plugin.manager.session.query(Song)\
+ .join(SongBookEntry, isouter=True)\
+ .join(Book, isouter=True)\
+ .filter(or_(Book.name.like(search_string), SongBookEntry.entry.like(search_string),
+ Song.search_title.like(search_string), Song.search_lyrics.like(search_string),
+ Song.comments.like(search_string)))\
+ .all()

     def on_song_list_load(self):
         """

=== modified file 'tests/functional/openlp_plugins/songs/test_mediaitem.py'
--- tests/functional/openlp_plugins/songs/test_mediaitem.py 2017-04-24 05:17:55 +0000
+++ tests/functional/openlp_plugins/songs/test_mediaitem.py 2017-06-04 20:21:52 +0000
@@ -46,9 +46,10 @@
         Registry.create()
         Registry().register('service_list', MagicMock())
         Registry().register('main_window', MagicMock())
+ self.mocked_plugin = MagicMock()
         with patch('openlp.core.lib.mediamanageritem.MediaManagerItem._setup'), \
                 patch('openlp.plugins.songs.forms.editsongform.EditSongForm.__init__'):
- self.media_item = SongMediaItem(None, MagicMock())
+ self.media_item = SongMediaItem(None, self.mocked_plugin)
             self.media_item.save_auto_select_id = MagicMock()
             self.media_item.list_view = MagicMock()
             self.media_item.list_view.save_auto_select_id = MagicMock()
@@ -558,3 +559,36 @@

         # THEN: The correct formatted results are returned
         self.assertEqual(search_results, [[123, 'My Song', 'My alternative']])
+
+ @patch('openlp.plugins.songs.lib.mediaitem.Book')
+ @patch('openlp.plugins.songs.lib.mediaitem.SongBookEntry')
+ @patch('openlp.plugins.songs.lib.mediaitem.Song')
+ @patch('openlp.plugins.songs.lib.mediaitem.or_')
+ def test_entire_song_search(self, mocked_or, MockedSong, MockedSongBookEntry, MockedBook):
+ """
+ Test that searching the entire song does the right queries
+ """
+ # GIVEN: A song media item, some keywords and some mocks
+ keywords = 'Jesus'
+ mocked_song = MagicMock()
+ mocked_or.side_effect = lambda a, b, c, d, e: ' '.join([a, b, c, d, e])
+ MockedSong.search_title.li...

Once you've fixed up the issues, you'll need to resubmit the merge proposal (link in the top right hand corner)