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

[Johannes Thomas Meyer]

added SongBook name and Song Number to "Entire Song" Search

added SongBook name and Song Number to "Entire Song" Search

lp:~samothjtm/openlp/bug-1695587 (revision 2748)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/2076/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1986/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1902/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Code_Analysis/1280/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Test_Coverage/1130/
[SUCCESS] https://ci.openlp.io/job/Branch-04c-Code_Analysis2/259/
[SUCCESS] https://ci.openlp.io/job/Branch-05-AppVeyor-Tests/105/

[Tomas Groth (tomasgroth)]

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

review: Needs Fixing

[Johannes Thomas Meyer (samothjtm)]

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?

[Johannes Thomas Meyer (samothjtm)]

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

[Johannes Thomas Meyer (samothjtm)]

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...

[Raoul Snyman (raoul-snyman)]

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.

[Raoul Snyman (raoul-snyman)]

review: Needs Fixing

[Raoul Snyman (raoul-snyman)]

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...

[Raoul Snyman (raoul-snyman)]

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

[Tomas Groth (tomasgroth)]

review: Approve

[Samuel Mehrbrodt (sam92)]

review: Approve

[Tim Bentley (trb143)]

Approved

review: Approve

=== modified file 'openlp/plugins/songs/lib/mediaitem.py'
--- openlp/plugins/songs/lib/mediaitem.py	2017-01-25 21:17:27 +0000
+++ openlp/plugins/songs/lib/mediaitem.py	2017-06-05 21:58:05 +0000
@@ -231,9 +231,14 @@
 
     def search_entire(self, search_keywords):
         search_string = '%{text}%'.format(text=clean_string(search_keywords))
-        return self.plugin.manager.get_all_objects(
-            Song, or_(Song.search_title.like(search_string), Song.search_lyrics.like(search_string),
-                      Song.comments.like(search_string)))
+        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),
+                        # hint: search_title contains alternate title
+                        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-05 21:58:05 +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,35 @@
 
         # 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, a keyword and some mocks
+        keyword = 'Jesus'
+        mocked_song = MagicMock()
+        mocked_or.side_effect = lambda a, b, c, d, e: ' '.join([a, b, c, d, e])
+        MockedSong.search_title.like.side_effect = lambda a: a
+        MockedSong.search_lyrics.like.side_effect = lambda a: a
+        MockedSong.comments.like.side_effect = lambda a: a
+        MockedSongBookEntry.entry.like.side_effect = lambda a: a
+        MockedBook.name.like.side_effect = lambda a: a
+
+        # WHEN: search_entire_song() is called with the keyword
+        self.media_item.search_entire(keyword)
+
+        # THEN: The correct calls were made
+        MockedSong.search_title.like.assert_called_once_with('%jesus%')
+        MockedSong.search_lyrics.like.assert_called_once_with('%jesus%')
+        MockedSong.comments.like.assert_called_once_with('%jesus%')
+        MockedSongBookEntry.entry.like.assert_called_once_with('%jesus%')
+        MockedBook.name.like.assert_called_once_with('%jesus%')
+        mocked_or.assert_called_once_with('%jesus%', '%jesus%', '%jesus%', '%jesus%', '%jesus%')
+        self.mocked_plugin.manager.session.query.assert_called_once_with(MockedSong)
+
+        self.assertEqual(self.mocked_plugin.manager.session.query.mock_calls[4][0], '().join().join().filter().all')