diff --git a/openlp/plugins/songs/forms/songselectform.py b/openlp/plugins/songs/forms/songselectform.py index d81f06cee..09c7b72c0 100644 --- a/openlp/plugins/songs/forms/songselectform.py +++ b/openlp/plugins/songs/forms/songselectform.py @@ -32,7 +32,6 @@ from openlp.plugins.songs.forms.songselectdialog import Ui_SongSelectDialog from openlp.plugins.songs.lib.db import Song from openlp.plugins.songs.lib.songselect import SongSelectImport, Pages - log = logging.getLogger(__name__) @@ -100,8 +99,15 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog, RegistryProperties) self.song_progress_bar.setMaximum(3) self.song_progress_bar.setValue(0) self.song = self.song_select_importer.get_song(self._update_song_progress) - self.import_button.setEnabled(True) - self.view_button.setEnabled(True) + if self.song: + self.import_button.setEnabled(True) + self.view_button.setEnabled(True) + else: + QtWidgets.QMessageBox.critical( + self, translate('SongsPlugin.SongSelectForm', 'No access to song'), + translate('SongsPlugin.SongSelectForm', 'This song cannot be read. Perhaps your CCLI account ' + 'does not give you access to this song.'), + QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Ok), QtWidgets.QMessageBox.Ok) self.back_button.setEnabled(True) if page_type == Pages.Other: self.back_button.setEnabled(True) diff --git a/openlp/plugins/songs/lib/songselect.py b/openlp/plugins/songs/lib/songselect.py index 788139988..49bd3dac3 100644 --- a/openlp/plugins/songs/lib/songselect.py +++ b/openlp/plugins/songs/lib/songselect.py @@ -182,7 +182,11 @@ class SongSelectImport(object): except (TypeError, URLError) as error: log.exception('Could not get song from SongSelect, {error}'.format(error=error)) return None - lyrics_link = song_page.find('section', 'page-section').find('a')['href'] + try: + lyrics_link = song_page.find('section', 'page-section').find('a')['href'] + except KeyError: + # can't find a link to the song - most likely the user account has no access to it + return None if callback: callback() try: diff --git a/tests/functional/openlp_plugins/songs/test_songselect.py b/tests/functional/openlp_plugins/songs/test_songselect.py index cf4b9b31c..b83e4c856 100644 --- a/tests/functional/openlp_plugins/songs/test_songselect.py +++ b/tests/functional/openlp_plugins/songs/test_songselect.py @@ -299,6 +299,52 @@ class TestSongSelectImport(TestCase, TestMixin): assert 2 == mocked_callback.call_count, 'The callback should have been called twice' assert result is None, 'The get_song() method should have returned None' + @patch('openlp.plugins.songs.lib.songselect.log.exception') + @patch('openlp.plugins.songs.lib.songselect.SongSelectImport.get_song_number_from_url') + @patch('openlp.plugins.songs.lib.songselect.SongSelectImport.get_page') + def test_get_song_no_access(self, mocked_get_page, mock_get_num, mock_log_exception): + """ + Test that the get_song() handles the case when the user's CCLI account has no access to the song + """ + fake_song_page = ''' +
+

Song Title

+ +
+
+
+ +
+ + + ''' + fake_lyrics_page = ''' +
+

Verse 1

+

verse thing 1
line 2

+

Verse 2

+

verse thing 2

+
+ + + ''' + mocked_get_page.side_effect = [fake_song_page, fake_lyrics_page] + mocked_callback = MagicMock() + importer = SongSelectImport(None, MagicMock()) + + # WHEN: get_song is called + result = importer.get_song(callback=mocked_callback) + + # THEN: None should be returned + assert result is None, 'The get_song() method should have returned None' + @patch('openlp.plugins.songs.lib.songselect.SongSelectImport.get_song_number_from_url') @patch('openlp.plugins.songs.lib.songselect.SongSelectImport.get_page') def test_get_song(self, mocked_get_page, mock_get_num):