From 2a6378f489b03fa58640babfbc9292812b1b226e Mon Sep 17 00:00:00 2001 From: Bob Luursema Date: Mon, 4 Mar 2019 21:37:11 +0100 Subject: [PATCH 1/4] Almost fixed bug 1800761 --- .bzrignore | 1 + openlp/plugins/songs/forms/songselectform.py | 5 ++-- openlp/plugins/songs/lib/songselect.py | 21 ++++++++++---- .../openlp_plugins/songs/test_songselect.py | 29 +++++++++++++------ 4 files changed, 39 insertions(+), 17 deletions(-) diff --git a/.bzrignore b/.bzrignore index f60d6cfff..2b43ce13a 100644 --- a/.bzrignore +++ b/.bzrignore @@ -7,6 +7,7 @@ cover .coverage coverage .directory +.vscode dist *.dll documentation/build/doctrees diff --git a/openlp/plugins/songs/forms/songselectform.py b/openlp/plugins/songs/forms/songselectform.py index d8b4db300..423f1f17c 100644 --- a/openlp/plugins/songs/forms/songselectform.py +++ b/openlp/plugins/songs/forms/songselectform.py @@ -263,8 +263,9 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog, RegistryProperties) self.login_progress_bar.setVisible(True) self.application.process_events() # Log the user in - if not self.song_select_importer.login( - self.username_edit.text(), self.password_edit.text(), self._update_login_progress): + subscription_level = self.song_select_importer.login( + self.username_edit.text(), self.password_edit.text(), self._update_login_progress) + if not subscription_level: QtWidgets.QMessageBox.critical( self, translate('SongsPlugin.SongSelectForm', 'Error Logging In'), diff --git a/openlp/plugins/songs/lib/songselect.py b/openlp/plugins/songs/lib/songselect.py index bde9b7bf9..01b9a635b 100644 --- a/openlp/plugins/songs/lib/songselect.py +++ b/openlp/plugins/songs/lib/songselect.py @@ -81,7 +81,7 @@ class SongSelectImport(object): :param username: SongSelect username :param password: SongSelect password :param callback: Method to notify of progress. - :return: True on success, False on failure. + :return: subscription level on success, None on failure. """ if callback: callback() @@ -115,11 +115,20 @@ class SongSelectImport(object): return False if callback: callback() - if posted_page.find('input', id='SearchText') is not None: - return True + if posted_page.find('input', id='SearchText') is not None or posted_page.find('div', id="select-organization") is not None: + try: + home_page = BeautifulSoup(self.opener.open(BASE_URL).read(), 'lxml') + except (TypeError, URLError) as error: + log.exception('Could not reach SongSelect, {error}'.format(error=error)) + subscription_element = home_page.find('div', {'class': 'subscriptionlevel'}) + if subscription_element is not None: + self.subscription_level = subscription_element.string.strip() + else: + self.subscription_level = 'premium' + return self.subscription_level else: log.debug(posted_page) - return False + return None def logout(self): """ @@ -128,7 +137,7 @@ class SongSelectImport(object): try: self.opener.open(LOGOUT_URL) except (TypeError, URLError) as error: - log.exception('Could not log of SongSelect, {error}'.format(error=error)) + log.exception('Could not log out of SongSelect, {error}'.format(error=error)) def search(self, search_text, max_results, callback=None): """ @@ -145,7 +154,7 @@ class SongSelectImport(object): 'PrimaryLanguage': '', 'Keys': '', 'Themes': '', - 'List': '', + 'List': '' if self.subscription_level == 'premium' else 'publicdomain', 'Sort': '', 'SearchText': search_text } diff --git a/tests/functional/openlp_plugins/songs/test_songselect.py b/tests/functional/openlp_plugins/songs/test_songselect.py index 1351faf51..4ef122c40 100644 --- a/tests/functional/openlp_plugins/songs/test_songselect.py +++ b/tests/functional/openlp_plugins/songs/test_songselect.py @@ -64,7 +64,7 @@ class TestSongSelectImport(TestCase, TestMixin): @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') def test_login_fails(self, MockedBeautifulSoup, mocked_build_opener): """ - Test that when logging in to SongSelect fails, the login method returns False + Test that when logging in to SongSelect fails, the login method returns None """ # GIVEN: A bunch of mocked out stuff and an importer object mocked_opener = MagicMock() @@ -80,12 +80,12 @@ class TestSongSelectImport(TestCase, TestMixin): # WHEN: The login method is called after being rigged to fail result = importer.login('username', 'password', mock_callback) - # THEN: callback was called 3 times, open was called twice, find was called twice, and False was returned + # THEN: callback was called 3 times, open was called twice, find was called twice, and None was returned assert 3 == mock_callback.call_count, 'callback should have been called 3 times' assert 2 == mocked_login_page.find.call_count, 'find should have been called twice' - assert 1 == mocked_posted_page.find.call_count, 'find should have been called once' + assert 2 == mocked_posted_page.find.call_count, 'find should have been called twice' assert 2 == mocked_opener.open.call_count, 'opener should have been called twice' - assert result is False, 'The login method should have returned False' + assert result is None, 'The login method should have returned None' @patch('openlp.plugins.songs.lib.songselect.build_opener') def test_login_except(self, mocked_build_opener): @@ -117,7 +117,11 @@ class TestSongSelectImport(TestCase, TestMixin): mocked_login_page.find.side_effect = [{'value': 'blah'}, None] mocked_posted_page = MagicMock() mocked_posted_page.find.return_value = MagicMock() - MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page] + mocked_home_page = MagicMock() + mocked_return = MagicMock() + mocked_return.string = 'premium' + mocked_home_page.find.return_value = mocked_return + MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page, mocked_home_page] mock_callback = MagicMock() importer = SongSelectImport(None) @@ -128,8 +132,8 @@ class TestSongSelectImport(TestCase, TestMixin): assert 3 == mock_callback.call_count, 'callback should have been called 3 times' assert 2 == mocked_login_page.find.call_count, 'find should have been called twice on the login page' assert 1 == mocked_posted_page.find.call_count, 'find should have been called once on the posted page' - assert 2 == mocked_opener.open.call_count, 'opener should have been called twice' - assert result is True, 'The login method should have returned True' + assert 3 == mocked_opener.open.call_count, 'opener should have been called 3 times' + assert result is 'premium', 'The login method should have returned the subscription level' @patch('openlp.plugins.songs.lib.songselect.build_opener') @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') @@ -146,7 +150,11 @@ class TestSongSelectImport(TestCase, TestMixin): mocked_login_page.find.side_effect = [{'value': 'blah'}, mocked_form] mocked_posted_page = MagicMock() mocked_posted_page.find.return_value = MagicMock() - MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page] + mocked_home_page = MagicMock() + mocked_return = MagicMock() + mocked_return.string = 'premium' + mocked_home_page.find.return_value = mocked_return + MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page, mocked_home_page] mock_callback = MagicMock() importer = SongSelectImport(None) @@ -158,7 +166,7 @@ class TestSongSelectImport(TestCase, TestMixin): assert 2 == mocked_login_page.find.call_count, 'find should have been called twice on the login page' assert 1 == mocked_posted_page.find.call_count, 'find should have been called once on the posted page' assert 'https://profile.ccli.com/do/login', mocked_opener.open.call_args_list[1][0][0] - assert result is True, 'The login method should have returned True' + assert result is 'premium', 'The login method should have returned the subscription level' @patch('openlp.plugins.songs.lib.songselect.build_opener') def test_logout(self, mocked_build_opener): @@ -191,6 +199,7 @@ class TestSongSelectImport(TestCase, TestMixin): MockedBeautifulSoup.return_value = mocked_results_page mock_callback = MagicMock() importer = SongSelectImport(None) + importer.subscription_level = 'premium' # WHEN: The login method is called after being rigged to fail results = importer.search('text', 1000, mock_callback) @@ -231,6 +240,7 @@ class TestSongSelectImport(TestCase, TestMixin): MockedBeautifulSoup.return_value = mocked_results_page mock_callback = MagicMock() importer = SongSelectImport(None) + importer.subscription_level = 'premium' # WHEN: The search method is called results = importer.search('text', 1000, mock_callback) @@ -282,6 +292,7 @@ class TestSongSelectImport(TestCase, TestMixin): MockedBeautifulSoup.return_value = mocked_results_page mock_callback = MagicMock() importer = SongSelectImport(None) + importer.subscription_level = 'premium' # WHEN: The search method is called results = importer.search('text', 2, mock_callback) From c68d6dc3de3c7165e43b8b666d179f64e18a0b30 Mon Sep 17 00:00:00 2001 From: Bob Luursema Date: Tue, 5 Mar 2019 21:55:37 +0100 Subject: [PATCH 2/4] Fixed bug 1800761 --- openlp/plugins/songs/forms/songselectform.py | 7 ++++++ openlp/plugins/songs/lib/songselect.py | 23 ++++++++++++++----- .../openlp_plugins/songs/test_songselect.py | 15 ++++-------- 3 files changed, 28 insertions(+), 17 deletions(-) diff --git a/openlp/plugins/songs/forms/songselectform.py b/openlp/plugins/songs/forms/songselectform.py index 423f1f17c..46d27e2af 100644 --- a/openlp/plugins/songs/forms/songselectform.py +++ b/openlp/plugins/songs/forms/songselectform.py @@ -273,6 +273,13 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog, RegistryProperties) 'There was a problem logging in, perhaps your username or password is incorrect?') ) else: + if subscription_level == 'Free': + QtWidgets.QMessageBox.information( + self, + translate('SongsPlugin.SongSelectForm', 'Free user'), + translate('SongsPlugin.SongSelectForm', + 'You logged in with a free account, the search will be limited to songs in the public domain.') + ) if self.save_password_checkbox.isChecked(): Settings().setValue(self.plugin.settings_section + '/songselect username', self.username_edit.text()) Settings().setValue(self.plugin.settings_section + '/songselect password', self.password_edit.text()) diff --git a/openlp/plugins/songs/lib/songselect.py b/openlp/plugins/songs/lib/songselect.py index 01b9a635b..5d5307cd9 100644 --- a/openlp/plugins/songs/lib/songselect.py +++ b/openlp/plugins/songs/lib/songselect.py @@ -115,20 +115,31 @@ class SongSelectImport(object): return False if callback: callback() - if posted_page.find('input', id='SearchText') is not None or posted_page.find('div', id="select-organization") is not None: + # Page if user is in an organization + if posted_page.find('input', id='SearchText') is not None: + self.subscription_level = self.find_subscription_level(posted_page) + return self.subscription_level + # Page if user is not in an organization + elif posted_page.find('div', id="select-organization") is not None: try: home_page = BeautifulSoup(self.opener.open(BASE_URL).read(), 'lxml') except (TypeError, URLError) as error: log.exception('Could not reach SongSelect, {error}'.format(error=error)) - subscription_element = home_page.find('div', {'class': 'subscriptionlevel'}) - if subscription_element is not None: - self.subscription_level = subscription_element.string.strip() - else: - self.subscription_level = 'premium' + self.subscription_level = self.find_subscription_level(home_page) return self.subscription_level else: log.debug(posted_page) return None + + def find_subscription_level(self, page): + scripts = page.find_all('script') + for tag in scripts: + if tag.string: + match = re.search("'Subscription': '(?P[^']+)", tag.string) + if match: + return match.group('subscription_level') + log.error('Could not determine SongSelect subscription level') + return 'unkown' def logout(self): """ diff --git a/tests/functional/openlp_plugins/songs/test_songselect.py b/tests/functional/openlp_plugins/songs/test_songselect.py index 4ef122c40..d12d84e46 100644 --- a/tests/functional/openlp_plugins/songs/test_songselect.py +++ b/tests/functional/openlp_plugins/songs/test_songselect.py @@ -117,11 +117,7 @@ class TestSongSelectImport(TestCase, TestMixin): mocked_login_page.find.side_effect = [{'value': 'blah'}, None] mocked_posted_page = MagicMock() mocked_posted_page.find.return_value = MagicMock() - mocked_home_page = MagicMock() - mocked_return = MagicMock() - mocked_return.string = 'premium' - mocked_home_page.find.return_value = mocked_return - MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page, mocked_home_page] + MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page] mock_callback = MagicMock() importer = SongSelectImport(None) @@ -132,8 +128,8 @@ class TestSongSelectImport(TestCase, TestMixin): assert 3 == mock_callback.call_count, 'callback should have been called 3 times' assert 2 == mocked_login_page.find.call_count, 'find should have been called twice on the login page' assert 1 == mocked_posted_page.find.call_count, 'find should have been called once on the posted page' - assert 3 == mocked_opener.open.call_count, 'opener should have been called 3 times' - assert result is 'premium', 'The login method should have returned the subscription level' + assert 2 == mocked_opener.open.call_count, 'opener should have been called twice' + assert result is 'unkown', 'The login method should have returned the subscription level' @patch('openlp.plugins.songs.lib.songselect.build_opener') @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') @@ -151,9 +147,6 @@ class TestSongSelectImport(TestCase, TestMixin): mocked_posted_page = MagicMock() mocked_posted_page.find.return_value = MagicMock() mocked_home_page = MagicMock() - mocked_return = MagicMock() - mocked_return.string = 'premium' - mocked_home_page.find.return_value = mocked_return MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page, mocked_home_page] mock_callback = MagicMock() importer = SongSelectImport(None) @@ -166,7 +159,7 @@ class TestSongSelectImport(TestCase, TestMixin): assert 2 == mocked_login_page.find.call_count, 'find should have been called twice on the login page' assert 1 == mocked_posted_page.find.call_count, 'find should have been called once on the posted page' assert 'https://profile.ccli.com/do/login', mocked_opener.open.call_args_list[1][0][0] - assert result is 'premium', 'The login method should have returned the subscription level' + assert result is 'unkown', 'The login method should have returned the subscription level' @patch('openlp.plugins.songs.lib.songselect.build_opener') def test_logout(self, mocked_build_opener): From 40ceb07f18aa56a5d37e8495031aad2bcc98de01 Mon Sep 17 00:00:00 2001 From: Bob Luursema Date: Tue, 5 Mar 2019 22:02:49 +0100 Subject: [PATCH 3/4] Hotfix --- openlp/plugins/songs/lib/songselect.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openlp/plugins/songs/lib/songselect.py b/openlp/plugins/songs/lib/songselect.py index 5d5307cd9..7aa1519a2 100644 --- a/openlp/plugins/songs/lib/songselect.py +++ b/openlp/plugins/songs/lib/songselect.py @@ -165,7 +165,7 @@ class SongSelectImport(object): 'PrimaryLanguage': '', 'Keys': '', 'Themes': '', - 'List': '' if self.subscription_level == 'premium' else 'publicdomain', + 'List': 'publicdomain' if self.subscription_level == 'Free' else '', 'Sort': '', 'SearchText': search_text } From 490f9bbe15b4f6d8c291449b5bfc1ff38d04418f Mon Sep 17 00:00:00 2001 From: Bob Luursema Date: Tue, 5 Mar 2019 22:21:12 +0100 Subject: [PATCH 4/4] Fix linting --- openlp/plugins/songs/forms/songselectform.py | 11 ++++++----- openlp/plugins/songs/lib/songselect.py | 4 ++-- .../openlp_plugins/songs/test_songselect.py | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/openlp/plugins/songs/forms/songselectform.py b/openlp/plugins/songs/forms/songselectform.py index 46d27e2af..68e393a82 100644 --- a/openlp/plugins/songs/forms/songselectform.py +++ b/openlp/plugins/songs/forms/songselectform.py @@ -264,7 +264,7 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog, RegistryProperties) self.application.process_events() # Log the user in subscription_level = self.song_select_importer.login( - self.username_edit.text(), self.password_edit.text(), self._update_login_progress) + self.username_edit.text(), self.password_edit.text(), self._update_login_progress) if not subscription_level: QtWidgets.QMessageBox.critical( self, @@ -275,10 +275,11 @@ class SongSelectForm(QtWidgets.QDialog, Ui_SongSelectDialog, RegistryProperties) else: if subscription_level == 'Free': QtWidgets.QMessageBox.information( - self, - translate('SongsPlugin.SongSelectForm', 'Free user'), - translate('SongsPlugin.SongSelectForm', - 'You logged in with a free account, the search will be limited to songs in the public domain.') + self, + translate('SongsPlugin.SongSelectForm', 'Free user'), + translate('SongsPlugin.SongSelectForm', 'You logged in with a free account, ' + 'the search will be limited to songs ' + 'in the public domain.') ) if self.save_password_checkbox.isChecked(): Settings().setValue(self.plugin.settings_section + '/songselect username', self.username_edit.text()) diff --git a/openlp/plugins/songs/lib/songselect.py b/openlp/plugins/songs/lib/songselect.py index 7aa1519a2..71e093cb1 100644 --- a/openlp/plugins/songs/lib/songselect.py +++ b/openlp/plugins/songs/lib/songselect.py @@ -130,7 +130,7 @@ class SongSelectImport(object): else: log.debug(posted_page) return None - + def find_subscription_level(self, page): scripts = page.find_all('script') for tag in scripts: @@ -139,7 +139,7 @@ class SongSelectImport(object): if match: return match.group('subscription_level') log.error('Could not determine SongSelect subscription level') - return 'unkown' + return None def logout(self): """ diff --git a/tests/functional/openlp_plugins/songs/test_songselect.py b/tests/functional/openlp_plugins/songs/test_songselect.py index d12d84e46..1affd66c2 100644 --- a/tests/functional/openlp_plugins/songs/test_songselect.py +++ b/tests/functional/openlp_plugins/songs/test_songselect.py @@ -129,7 +129,7 @@ class TestSongSelectImport(TestCase, TestMixin): assert 2 == mocked_login_page.find.call_count, 'find should have been called twice on the login page' assert 1 == mocked_posted_page.find.call_count, 'find should have been called once on the posted page' assert 2 == mocked_opener.open.call_count, 'opener should have been called twice' - assert result is 'unkown', 'The login method should have returned the subscription level' + assert result is None, 'The login method should have returned the subscription level' @patch('openlp.plugins.songs.lib.songselect.build_opener') @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup') @@ -159,7 +159,7 @@ class TestSongSelectImport(TestCase, TestMixin): assert 2 == mocked_login_page.find.call_count, 'find should have been called twice on the login page' assert 1 == mocked_posted_page.find.call_count, 'find should have been called once on the posted page' assert 'https://profile.ccli.com/do/login', mocked_opener.open.call_args_list[1][0][0] - assert result is 'unkown', 'The login method should have returned the subscription level' + assert result is None, 'The login method should have returned the subscription level' @patch('openlp.plugins.songs.lib.songselect.build_opener') def test_logout(self, mocked_build_opener):