Add some error handling around the getting a song.

Add some more tests.

openlp/plugins/songs/forms/songselectform.py

@@ -193,7 +193,7 @@ class SongSelectForm(QtGui.QDialog, Ui_SongSelectDialog):
         self.song_progress_bar.setValue(0)
         self.main_window.application.process_events()
         # Get the full song
-        self.song_select_importer.get_song(song, self._update_song_progress)
+        song = self.song_select_importer.get_song(song, self._update_song_progress)
         # Update the UI
         self.title_edit.setText(song['title'])
         self.copyright_edit.setText(song['copyright'])

openlp/plugins/songs/lib/songselect.py

@@ -137,38 +137,44 @@ class SongSelectImport(object):
         """
         Get the full song from SongSelect
 
-        :param song:
+        :param song: The song dictionary to update
-        :param callback:
+        :param callback: A callback which can be used to indicate progress
+        :return: The updated song dictionary
         """
         if callback:
             callback()
-        song_page = BeautifulSoup(self.opener.open(song['link']).read(), 'lxml')
+        try:
+            song_page = BeautifulSoup(self.opener.open(song['link']).read(), 'lxml')
+        except (TypeError, HTTPError) as e:
+            log.exception(u'Could not get song from SongSelect, %s', e)
+            return None
         if callback:
             callback()
         try:
             lyrics_page = BeautifulSoup(self.opener.open(song['link'] + '/lyrics').read(), 'lxml')
-        except HTTPError:
+        except (TypeError, HTTPError):
-            lyrics_page = None
+            log.exception(u'Could not get lyrics from SongSelect')
+            return None
         if callback:
             callback()
         song['copyright'] = '/'.join([li.string for li in song_page.find('ul', 'copyright').find_all('li')])
         song['copyright'] = self.html_parser.unescape(song['copyright'])
         song['ccli_number'] = song_page.find('ul', 'info').find('li').string.split(':')[1].strip()
         song['verses'] = []
-        if lyrics_page:
+        verses = lyrics_page.find('section', 'lyrics').find_all('p')
-            verses = lyrics_page.find('section', 'lyrics').find_all('p')
+        verse_labels = lyrics_page.find('section', 'lyrics').find_all('h3')
-            verse_labels = lyrics_page.find('section', 'lyrics').find_all('h3')
+        for counter in range(len(verses)):
-            for counter in range(len(verses)):
+            verse = {'label': verse_labels[counter].string, 'lyrics': ''}
-                verse = {'label': verse_labels[counter].string, 'lyrics': ''}
+            for v in verses[counter].contents:
-                for v in verses[counter].contents:
+                if isinstance(v, NavigableString):
-                    if isinstance(v, NavigableString):
+                    verse['lyrics'] = verse['lyrics'] + v.string
-                        verse['lyrics'] = verse['lyrics'] + v.string
+                else:
-                    else:
+                    verse['lyrics'] += '\n'
-                        verse['lyrics'] += '\n'
+            verse['lyrics'] = verse['lyrics'].strip(' \n\r\t')
-                verse['lyrics'] = verse['lyrics'].strip(' \n\r\t')
+            song['verses'].append(self.html_parser.unescape(verse))
-                song['verses'].append(self.html_parser.unescape(verse))
         for counter, author in enumerate(song['authors']):
             song['authors'][counter] = self.html_parser.unescape(author)
+        return song
 
     def save_song(self, song):
         """

tests/functional/__init__.py

@@ -1,3 +1,7 @@
+# -*- coding: utf-8 -*-
+"""
+Base directory for tests
+"""
 import sip
 sip.setapi('QDate', 2)
 sip.setapi('QDateTime', 2)
@@ -11,9 +15,11 @@ import sys
 from PyQt4 import QtGui
 
 if sys.version_info[1] >= 3:
-    from unittest.mock import MagicMock, patch, mock_open
+    from unittest.mock import MagicMock, patch, mock_open, call
 else:
-    from mock import MagicMock, patch, mock_open
+    from mock import MagicMock, patch, mock_open, call
 
 # Only one QApplication can be created. Use QtGui.QApplication.instance() when you need to "create" a  QApplication.
 application = QtGui.QApplication([])
+
+__all__ = ['MagicMock', 'patch', 'mock_open', 'call', 'application']

tests/functional/openlp_plugins/songs/test_songselect.py

@@ -30,10 +30,11 @@
 This module contains tests for the CCLI SongSelect importer.
 """
 from unittest import TestCase
+from urllib.error import URLError
 
-from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGIN_URL, LOGOUT_URL, BASE_URL
+from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGOUT_URL, BASE_URL
 
-from tests.functional import MagicMock, patch
+from tests.functional import MagicMock, patch, call
 
 
 class TestSongSelect(TestCase):
@@ -125,8 +126,8 @@ class TestSongSelect(TestCase):
         Test that when the search finds no results, it simply returns an empty list
         """
         # GIVEN: A bunch of mocked out stuff and an importer object
-        with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener, patch(
+        with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener, \
-                'openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup:
+                patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup:
             mocked_opener = MagicMock()
             mocked_build_opener.return_value = mocked_opener
             mocked_results_page = MagicMock()
@@ -150,8 +151,8 @@ class TestSongSelect(TestCase):
         Test that when the search finds 2 results, it simply returns a list with 2 results
         """
         # GIVEN: A bunch of mocked out stuff and an importer object
-        with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener, patch(
+        with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener, \
-                'openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup:
+                patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup:
             # first search result
             mocked_result1 = MagicMock()
             mocked_result1.find.side_effect = [MagicMock(string='Title 1'), {'href': '/url1'}]
@@ -188,8 +189,8 @@ class TestSongSelect(TestCase):
         Test that when the search finds MAX (2) results, it simply returns a list with those (2)
         """
         # GIVEN: A bunch of mocked out stuff and an importer object
-        with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener, patch(
+        with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener, \
-                'openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup:
+                patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup:
             # first search result
             mocked_result1 = MagicMock()
             mocked_result1.find.side_effect = [MagicMock(string='Title 1'), {'href': '/url1'}]
@@ -222,3 +223,88 @@ class TestSongSelect(TestCase):
             expected_list = [{'title': 'Title 1', 'authors': ['Author 1-1', 'Author 1-2'], 'link': BASE_URL + '/url1'},
                              {'title': 'Title 2', 'authors': ['Author 2-1', 'Author 2-2'], 'link': BASE_URL + '/url2'}]
             self.assertListEqual(expected_list, results, 'The search method should have returned two songs')
+
+    def get_song_page_raises_exception_test(self):
+        """
+        Test that when BeautifulSoup gets a bad song page the get_song() method returns None
+        """
+        # GIVEN: A bunch of mocked out stuff and an importer object
+        with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener:
+            mocked_opener = MagicMock()
+            mocked_build_opener.return_value = mocked_opener
+            mocked_opener.open.read.side_effect = URLError('[Errno -2] Name or service not known')
+            mocked_callback = MagicMock()
+            importer = SongSelectImport(None)
+
+            # WHEN: get_song is called
+            result = importer.get_song({'link': 'link'}, callback=mocked_callback)
+
+            # THEN: The callback should have been called once and None should be returned
+            mocked_callback.assert_called_with()
+            self.assertIsNone(result, 'The get_song() method should have returned None')
+
+    def get_song_lyrics_raise_exception_test(self):
+        """
+        Test that when BeautifulSoup gets a bad lyrics page the get_song() method returns None
+        """
+        # GIVEN: A bunch of mocked out stuff and an importer object
+        with patch('openlp.plugins.songs.lib.songselect.build_opener'), \
+                patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup:
+            MockedBeautifulSoup.side_effect = [None, TypeError('Test Error')]
+            mocked_callback = MagicMock()
+            importer = SongSelectImport(None)
+
+            # WHEN: get_song is called
+            result = importer.get_song({'link': 'link'}, callback=mocked_callback)
+
+            # THEN: The callback should have been called twice and None should be returned
+            self.assertEqual(2, mocked_callback.call_count, 'The callback should have been called twice')
+            self.assertIsNone(result, 'The get_song() method should have returned None')
+
+    def get_song_test(self):
+        """
+        Test that the get_song() method returns the correct song details
+        """
+        # GIVEN: A bunch of mocked out stuff and an importer object
+        with patch('openlp.plugins.songs.lib.songselect.build_opener'), \
+                patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup:
+            mocked_song_page = MagicMock()
+            mocked_copyright = MagicMock()
+            mocked_copyright.find_all.return_value = [MagicMock(string='Copyright 1'), MagicMock(string='Copyright 2')]
+            mocked_song_page.find.side_effect = [
+                mocked_copyright,
+                MagicMock(find=MagicMock(string='CCLI: 123456'))
+            ]
+            mocked_lyrics_page = MagicMock()
+            mocked_find_all = MagicMock()
+            mocked_find_all.side_effect = [
+                [
+                    MagicMock(contents='The Lord told Noah: there\'s gonna be a floody, floody'),
+                    MagicMock(contents='So, rise and shine, and give God the glory, glory'),
+                    MagicMock(contents='The Lord told Noah to build him an arky, arky')
+                ],
+                [MagicMock(string='Verse 1'), MagicMock(string='Chorus'), MagicMock(string='Verse 2')]
+            ]
+            mocked_lyrics_page.find.return_value = MagicMock(find_all=mocked_find_all)
+            MockedBeautifulSoup.side_effect = [mocked_song_page, mocked_lyrics_page]
+            mocked_callback = MagicMock()
+            importer = SongSelectImport(None)
+            fake_song = {'title': 'Title', 'authors': ['Author 1', 'Author 2'], 'link': 'url'}
+
+            # WHEN: get_song is called
+            result = importer.get_song(fake_song, callback=mocked_callback)
+
+            # THEN: The callback should have been called three times and the song should be returned
+            self.assertEqual(3, mocked_callback.call_count, 'The callback should have been called twice')
+            self.assertIsNotNone(result, 'The get_song() method should have returned a song dictionary')
+            self.assertEqual(2, mocked_lyrics_page.find.call_count, 'The find() method should have been called twice')
+            self.assertEqual(2, mocked_find_all.call_count, 'The find_all() method should have been called twice')
+            self.assertEqual([call('section', 'lyrics'), call('section', 'lyrics')],
+                             mocked_lyrics_page.find.call_args_list,
+                             'The find() method should have been called with the right arguments')
+            self.assertEqual([call('p'), call('h3')], mocked_find_all.call_args_list,
+                             'The find_all() method should have been called with the right arguments')
+            self.assertIn('copyright', result, 'The returned song should have a copyright')
+            self.assertIn('ccli_number', result, 'The returned song should have a CCLI number')
+            self.assertIn('verses', result, 'The returned song should have verses')
+            self.assertEqual(3, len(result['verses']), 'Three verses should have been returned')