diff --git a/openlp/plugins/songs/forms/songselectform.py b/openlp/plugins/songs/forms/songselectform.py index a340978e1..566932d42 100755 --- a/openlp/plugins/songs/forms/songselectform.py +++ b/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']) diff --git a/openlp/plugins/songs/lib/songselect.py b/openlp/plugins/songs/lib/songselect.py index 5d9840bcb..9c78680ca 100644 --- a/openlp/plugins/songs/lib/songselect.py +++ b/openlp/plugins/songs/lib/songselect.py @@ -137,38 +137,44 @@ class SongSelectImport(object): """ Get the full song from SongSelect - :param song: - :param callback: + :param song: The song dictionary to update + :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: - lyrics_page = None + except (TypeError, HTTPError): + 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') - verse_labels = lyrics_page.find('section', 'lyrics').find_all('h3') - for counter in range(len(verses)): - verse = {'label': verse_labels[counter].string, 'lyrics': ''} - for v in verses[counter].contents: - if isinstance(v, NavigableString): - verse['lyrics'] = verse['lyrics'] + v.string - else: - verse['lyrics'] += '\n' - verse['lyrics'] = verse['lyrics'].strip(' \n\r\t') - song['verses'].append(self.html_parser.unescape(verse)) + verses = lyrics_page.find('section', 'lyrics').find_all('p') + verse_labels = lyrics_page.find('section', 'lyrics').find_all('h3') + for counter in range(len(verses)): + verse = {'label': verse_labels[counter].string, 'lyrics': ''} + for v in verses[counter].contents: + if isinstance(v, NavigableString): + verse['lyrics'] = verse['lyrics'] + v.string + else: + verse['lyrics'] += '\n' + verse['lyrics'] = verse['lyrics'].strip(' \n\r\t') + 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): """ diff --git a/tests/functional/__init__.py b/tests/functional/__init__.py index f3b85b9b3..d20aae496 100644 --- a/tests/functional/__init__.py +++ b/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'] diff --git a/tests/functional/openlp_plugins/songs/test_songselect.py b/tests/functional/openlp_plugins/songs/test_songselect.py index c848d1206..a7a488102 100644 --- a/tests/functional/openlp_plugins/songs/test_songselect.py +++ b/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( - 'openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup: + with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener, \ + 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( - 'openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup: + with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener, \ + 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( - 'openlp.plugins.songs.lib.songselect.BeautifulSoup') as MockedBeautifulSoup: + with patch('openlp.plugins.songs.lib.songselect.build_opener') as mocked_build_opener, \ + 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')