Merge lp:~bob-luursema/openlp/bug-1800761 into lp:openlp

[Bob Luursema]

Fix bug when using SongSelect import with a free account.

This change checks the subscription level of the login that the user has from a piece of JavaScript on the SongSelect page. If it is a free subscription it will display a messagebox informing the user that their searches are limited to songs in the public domain (as that is what a free user can access).

[Bob Luursema (bob-luursema)]

I didn't yet test whether the import still works for paid users, since I don't have a paid account. Would it be good to add some unit tests for that by mocking the external call and inputting an HTML file via the mock?

[Raoul Snyman (raoul-snyman)]

Linux tests passed!

[Raoul Snyman (raoul-snyman)]

Linting failed, please see https://ci.openlp.io/job/MP-03-Linting/42/ for more details

[Raoul Snyman (raoul-snyman)]

This looks great! One small change that I think you should make (inline in the diff), and then I think we're good to go!

review: Needs Fixing

[Raoul Snyman (raoul-snyman)]

Linux tests passed!

[Raoul Snyman (raoul-snyman)]

Linting passed!

[Raoul Snyman (raoul-snyman)]

macOS tests passed!

[Raoul Snyman (raoul-snyman)]

review: Approve

[Tomas Groth (tomasgroth)]

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2018-09-07 06:43:01 +0000
+++ .bzrignore	2019-03-05 21:31:58 +0000
@@ -7,6 +7,7 @@
 .coverage
 coverage
 .directory
+.vscode
 dist
 *.dll
 documentation/build/doctrees

=== modified file 'openlp/plugins/songs/forms/songselectform.py'
--- openlp/plugins/songs/forms/songselectform.py	2019-02-14 15:09:09 +0000
+++ openlp/plugins/songs/forms/songselectform.py	2019-03-05 21:31:58 +0000
@@ -263,8 +263,9 @@
         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'),
@@ -272,6 +273,14 @@
                           '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())

=== modified file 'openlp/plugins/songs/lib/songselect.py'
--- openlp/plugins/songs/lib/songselect.py	2019-02-14 15:09:09 +0000
+++ openlp/plugins/songs/lib/songselect.py	2019-03-05 21:31:58 +0000
@@ -81,7 +81,7 @@
         :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,31 @@
             return False
         if callback:
             callback()
+        # Page if user is in an organization
         if posted_page.find('input', id='SearchText') is not None:
-            return True
+            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))
+            self.subscription_level = self.find_subscription_level(home_page)
+            return self.subscription_level
         else:
             log.debug(posted_page)
-            return False
+            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<subscription_level>[^']+)", tag.string)
+                if match:
+                    return match.group('subscription_level')
+        log.error('Could not determine SongSelect subscription level')
+        return None
 
     def logout(self):
         """
@@ -128,7 +148,7 @@
         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 +165,7 @@
             'PrimaryLanguage': '',
             'Keys': '',
             'Themes': '',
-            'List': '',
+            'List': 'publicdomain' if self.subscription_level == 'Free' else '',
             'Sort': '',
             'SearchText': search_text
         }

=== modified file 'tests/functional/openlp_plugins/songs/test_songselect.py'
--- tests/functional/openlp_plugins/songs/test_songselect.py	2019-02-14 15:09:09 +0000
+++ tests/functional/openlp_plugins/songs/test_songselect.py	2019-03-05 21:31:58 +0000
@@ -64,7 +64,7 @@
     @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 @@
         # 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):
@@ -129,7 +129,7 @@
         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 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')
@@ -146,7 +146,8 @@
         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()
+        MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page, mocked_home_page]
         mock_callback = MagicMock()
         importer = SongSelectImport(None)
 
@@ -158,7 +159,7 @@
         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 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):
@@ -191,6 +192,7 @@
         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 +233,7 @@
         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 +285,7 @@
         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)