Merge lp:~raoul-snyman/openlp/fix-importers-2.4 into lp:openlp/2.4

[Raoul Snyman]

- Fix SongSelect so that it detects the login URL
- Fix PresentationManager importer to handle weird XML
- Pull in OpenLP song importer fixes from Olli's branch

[Tim Bentley (trb143)]

review: Approve

=== modified file 'openlp/core/__init__.py'
--- openlp/core/__init__.py	2017-01-22 20:01:53 +0000
+++ openlp/core/__init__.py	2017-03-02 21:00:50 +0000
@@ -317,7 +317,7 @@
         return QtWidgets.QApplication.event(self, event)
 
 
-def parse_options(args):
+def parse_options(args=None):
     """
     Parse the command line arguments
 

=== modified file 'openlp/plugins/songs/lib/importers/openlp.py'
--- openlp/plugins/songs/lib/importers/openlp.py	2016-12-31 11:05:48 +0000
+++ openlp/plugins/songs/lib/importers/openlp.py	2017-03-02 21:00:50 +0000
@@ -221,11 +221,17 @@
                     if not existing_book:
                         existing_book = Book.populate(name=entry.songbook.name, publisher=entry.songbook.publisher)
                     new_song.add_songbook_entry(existing_book, entry.entry)
-            elif song.book:
+            elif hasattr(song, 'book') and song.book:
                 existing_book = self.manager.get_object_filtered(Book, Book.name == song.book.name)
                 if not existing_book:
                     existing_book = Book.populate(name=song.book.name, publisher=song.book.publisher)
-                new_song.add_songbook_entry(existing_book, '')
+                # Get the song_number from "songs" table "song_number" field. (This is db structure from 2.2.1)
+                # If there's a number, add it to the song, otherwise it will be "".
+                existing_number = song.song_number if hasattr(song, 'song_number') else ''
+                if existing_number:
+                    new_song.add_songbook_entry(existing_book, existing_number)
+                else:
+                    new_song.add_songbook_entry(existing_book, "")
             # Find or create all the media files and add them to the new song object
             if has_media_files and song.media_files:
                 for media_file in song.media_files:

=== modified file 'openlp/plugins/songs/lib/importers/presentationmanager.py'
--- openlp/plugins/songs/lib/importers/presentationmanager.py	2016-12-31 11:05:48 +0000
+++ openlp/plugins/songs/lib/importers/presentationmanager.py	2017-03-02 21:00:50 +0000
@@ -23,7 +23,7 @@
 The :mod:`presentationmanager` module provides the functionality for importing
 Presentationmanager song files into the current database.
 """
-
+import logging
 import os
 import re
 import chardet
@@ -32,6 +32,8 @@
 from openlp.core.ui.wizard import WizardStrings
 from .songimport import SongImport
 
+log = logging.getLogger(__name__)
+
 
 class PresentationManagerImport(SongImport):
     """
@@ -62,15 +64,36 @@
                                              'File is not in XML-format, which is the only format supported.'))
                     continue
             root = objectify.fromstring(etree.tostring(tree))
-            self.process_song(root)
-
-    def process_song(self, root):
+            self.process_song(root, file_path)
+
+    def _get_attr(self, elem, name):
+        """
+        Due to PresentationManager's habit of sometimes capitilising the first letter of an element, we have to do
+        some gymnastics.
+        """
+        if hasattr(elem, name):
+            log.debug('%s: %s', name, getattr(elem, name))
+            return str(getattr(elem, name))
+        name = name[0].upper() + name[1:]
+        if hasattr(elem, name):
+            log.debug('%s: %s', name, getattr(elem, name))
+            return str(getattr(elem, name))
+        else:
+            return ''
+
+    def process_song(self, root, file_path):
         self.set_defaults()
-        self.title = str(root.attributes.title)
-        self.add_author(str(root.attributes.author))
-        self.copyright = str(root.attributes.copyright)
-        self.ccli_number = str(root.attributes.ccli_number)
-        self.comments = str(root.attributes.comments)
+        attrs = None
+        if hasattr(root, 'attributes'):
+            attrs = root.attributes
+        elif hasattr(root, 'Attributes'):
+            attrs = root.Attributes
+        if attrs is not None:
+            self.title = self._get_attr(root.attributes, 'title')
+            self.add_author(self._get_attr(root.attributes, 'author'))
+            self.copyright = self._get_attr(root.attributes, 'copyright')
+            self.ccli_number = self._get_attr(root.attributes, 'ccli_number')
+            self.comments = str(root.attributes.comments) if hasattr(root.attributes, 'comments') else None
         verse_order_list = []
         verse_count = {}
         duplicates = []
@@ -102,4 +125,4 @@
 
         self.verse_order_list = verse_order_list
         if not self.finish():
-            self.log_error(self.import_source)
+            self.log_error(os.path.basename(file_path))

=== modified file 'openlp/plugins/songs/lib/songselect.py'
--- openlp/plugins/songs/lib/songselect.py	2016-12-31 11:05:48 +0000
+++ openlp/plugins/songs/lib/songselect.py	2017-03-02 21:00:50 +0000
@@ -47,7 +47,7 @@
 BASE_URL = 'https://songselect.ccli.com'
 LOGIN_PAGE = 'https://profile.ccli.com/account/signin?appContext=SongSelect&returnUrl=' \
     'https%3a%2f%2fsongselect.ccli.com%2f'
-LOGIN_URL = 'https://profile.ccli.com/'
+LOGIN_URL = 'https://profile.ccli.com'
 LOGOUT_URL = BASE_URL + '/account/logout'
 SEARCH_URL = BASE_URL + '/search/results'
 
@@ -97,14 +97,27 @@
             'password': password,
             'RememberMe': 'false'
         })
+        login_form = login_page.find('form')
+        if login_form:
+            login_url = login_form.attrs['action']
+        else:
+            login_url = '/Account/SignIn'
+        if not login_url.startswith('http'):
+            if login_url[0] != '/':
+                login_url = '/' + login_url
+            login_url = LOGIN_URL + login_url
         try:
-            posted_page = BeautifulSoup(self.opener.open(LOGIN_URL, data.encode('utf-8')).read(), 'lxml')
+            posted_page = BeautifulSoup(self.opener.open(login_url, data.encode('utf-8')).read(), 'lxml')
         except (TypeError, URLError) as error:
             log.exception('Could not login to SongSelect, %s', error)
             return False
         if callback:
             callback()
-        return posted_page.find('input', id='SearchText') is not None
+        if posted_page.find('input', id='SearchText') is not None:
+            return True
+        else:
+            log.debug(posted_page)
+            return False
 
     def logout(self):
         """

=== added file 'tests/functional/openlp_core/__init__.py'
=== modified file 'tests/functional/openlp_core/test_init.py'
--- tests/functional/openlp_core/test_init.py	2016-12-31 11:05:48 +0000
+++ tests/functional/openlp_core/test_init.py	2017-03-02 21:00:50 +0000
@@ -22,12 +22,12 @@
 
 import sys
 from unittest import TestCase
-
-from openlp.core import parse_options
-from tests.helpers.testmixin import TestMixin
-
-
-class TestInitFunctions(TestMixin, TestCase):
+from unittest.mock import MagicMock, patch
+
+from openlp.core import OpenLP, parse_options
+
+
+class TestInitFunctions(TestCase):
 
     def parse_options_basic_test(self):
         """
@@ -116,7 +116,7 @@
 
     def parse_options_file_and_debug_test(self):
         """
-        Test the parse options process works with a file
+        Test the parse options process works with a file and the debug log level
 
         """
         # GIVEN: a a set of system arguments.
@@ -131,14 +131,28 @@
         self.assertEquals(args.style, None, 'There are no style flags to be processed')
         self.assertEquals(args.rargs, 'dummy_temp', 'The service file should not be blank')
 
-    def parse_options_two_files_test(self):
-        """
-        Test the parse options process works with a file
-
-        """
-        # GIVEN: a a set of system arguments.
-        sys.argv[1:] = ['dummy_temp', 'dummy_temp2']
-        # WHEN: We we parse them to expand to options
-        args = parse_options()
-        # THEN: the following fields will have been extracted.
-        self.assertEquals(args, None, 'The args should be None')
+
+class TestOpenLP(TestCase):
+    """
+    Test the OpenLP app class
+    """
+    @patch('openlp.core.QtWidgets.QApplication.exec')
+    def test_exec(self, mocked_exec):
+        """
+        Test the exec method
+        """
+        # GIVEN: An app
+        app = OpenLP([])
+        app.shared_memory = MagicMock()
+        mocked_exec.return_value = False
+
+        # WHEN: exec() is called
+        result = app.exec()
+
+        # THEN: The right things should be called
+        assert app.is_event_loop_active is True
+        mocked_exec.assert_called_once_with()
+        app.shared_memory.detach.assert_called_once_with()
+        assert result is False
+
+        del app

=== modified file 'tests/functional/openlp_plugins/remotes/test_router.py'
--- tests/functional/openlp_plugins/remotes/test_router.py	2016-12-31 11:05:48 +0000
+++ tests/functional/openlp_plugins/remotes/test_router.py	2017-03-02 21:00:50 +0000
@@ -25,11 +25,11 @@
 import os
 import urllib.request
 from unittest import TestCase
+from unittest.mock import MagicMock, patch, mock_open
 
 from openlp.core.common import Settings, Registry
 from openlp.core.ui import ServiceManager
 from openlp.plugins.remotes.lib.httpserver import HttpRouter
-from tests.functional import MagicMock, patch, mock_open
 from tests.helpers.testmixin import TestMixin
 
 __default_settings__ = {
@@ -313,11 +313,13 @@
         with patch.object(self.service_manager, 'setup_ui'), \
                 patch.object(self.router, 'do_json_header'):
             self.service_manager.bootstrap_initialise()
-            self.app.processEvents()
+            # Not sure why this is here, it doesn't make sense in the test
+            # self.app.processEvents()
 
             # WHEN: Remote next is received
             self.router.service(action='next')
-            self.app.processEvents()
+            # Not sure why this is here, it doesn't make sense in the test
+            # self.app.processEvents()
 
             # THEN: service_manager.next_item() should have been called
             self.assertTrue(mocked_next_item.called, 'next_item() should have been called in service_manager')
@@ -334,11 +336,13 @@
         with patch.object(self.service_manager, 'setup_ui'), \
                 patch.object(self.router, 'do_json_header'):
             self.service_manager.bootstrap_initialise()
-            self.app.processEvents()
+            # Not sure why this is here, it doesn't make sense in the test
+            # self.app.processEvents()
 
             # WHEN: Remote next is received
             self.router.service(action='previous')
-            self.app.processEvents()
+            # Not sure why this is here, it doesn't make sense in the test
+            # self.app.processEvents()
 
             # THEN: service_manager.next_item() should have been called
             self.assertTrue(mocked_previous_item.called, 'previous_item() should have been called in service_manager')

=== modified file 'tests/functional/openlp_plugins/songs/test_songselect.py'
--- tests/functional/openlp_plugins/songs/test_songselect.py	2016-12-31 11:05:48 +0000
+++ tests/functional/openlp_plugins/songs/test_songselect.py	2017-03-02 21:00:50 +0000
@@ -32,7 +32,7 @@
 from openlp.core import Registry
 from openlp.plugins.songs.forms.songselectform import SongSelectForm, SearchWorker
 from openlp.plugins.songs.lib import Song
-from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGOUT_URL, BASE_URL
+from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGIN_PAGE, LOGOUT_URL, BASE_URL
 
 from tests.functional import MagicMock, patch, call
 from tests.helpers.songfileimport import SongImportTestHelper
@@ -81,7 +81,7 @@
 
         # THEN: callback was called 3 times, open was called twice, find was called twice, and False was returned
         self.assertEqual(2, mock_callback.call_count, 'callback should have been called 3 times')
-        self.assertEqual(1, mocked_login_page.find.call_count, 'find should have been called twice')
+        self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice')
         self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice')
         self.assertFalse(result, 'The login method should have returned False')
 
@@ -96,7 +96,9 @@
         mocked_build_opener.return_value = mocked_opener
         mocked_login_page = MagicMock()
         mocked_login_page.find.side_effect = [{'value': 'blah'}, None]
-        MockedBeautifulSoup.return_value = mocked_login_page
+        mocked_posted_page = MagicMock()
+        mocked_posted_page.find.return_value = None
+        MockedBeautifulSoup.side_effect = [mocked_login_page, mocked_posted_page]
         mock_callback = MagicMock()
         importer = SongSelectImport(None)
 
@@ -105,7 +107,8 @@
 
         # THEN: callback was called 3 times, open was called twice, find was called twice, and False was returned
         self.assertEqual(3, mock_callback.call_count, 'callback should have been called 3 times')
-        self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice')
+        self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice on the login page')
+        self.assertEqual(1, mocked_posted_page.find.call_count, 'find should have been called once on the posted page')
         self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice')
         self.assertFalse(result, 'The login method should have returned False')
 
@@ -136,8 +139,10 @@
         mocked_opener = MagicMock()
         mocked_build_opener.return_value = mocked_opener
         mocked_login_page = MagicMock()
-        mocked_login_page.find.side_effect = [{'value': 'blah'}, MagicMock()]
-        MockedBeautifulSoup.return_value = mocked_login_page
+        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]
         mock_callback = MagicMock()
         importer = SongSelectImport(None)
 
@@ -146,11 +151,41 @@
 
         # THEN: callback was called 3 times, open was called twice, find was called twice, and True was returned
         self.assertEqual(3, mock_callback.call_count, 'callback should have been called 3 times')
-        self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice')
+        self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice on the login page')
+        self.assertEqual(1, mocked_posted_page.find.call_count, 'find should have been called once on the posted page')
         self.assertEqual(2, mocked_opener.open.call_count, 'opener should have been called twice')
         self.assertTrue(result, 'The login method should have returned True')
 
     @patch('openlp.plugins.songs.lib.songselect.build_opener')
+    @patch('openlp.plugins.songs.lib.songselect.BeautifulSoup')
+    def login_url_from_form_test(self, MockedBeautifulSoup, mocked_build_opener):
+        """
+        Test that the login URL is from the form
+        """
+        # GIVEN: A bunch of mocked out stuff and an importer object
+        mocked_opener = MagicMock()
+        mocked_build_opener.return_value = mocked_opener
+        mocked_form = MagicMock()
+        mocked_form.attrs = {'action': 'do/login'}
+        mocked_login_page = MagicMock()
+        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]
+        mock_callback = MagicMock()
+        importer = SongSelectImport(None)
+
+        # 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 True was returned
+        self.assertEqual(3, mock_callback.call_count, 'callback should have been called 3 times')
+        self.assertEqual(2, mocked_login_page.find.call_count, 'find should have been called twice on the login page')
+        self.assertEqual(1, mocked_posted_page.find.call_count, 'find should have been called once on the posted page')
+        self.assertEqual('https://profile.ccli.com/do/login', mocked_opener.open.call_args_list[1][0][0])
+        self.assertTrue(result, 'The login method should have returned True')
+
+    @patch('openlp.plugins.songs.lib.songselect.build_opener')
     def logout_test(self, mocked_build_opener):
         """
         Test that when the logout method is called, it logs the user out of SongSelect

=== added file 'tests/interfaces/openlp_plugins/bibles/forms/__init__.py'
=== modified file 'tests/interfaces/openlp_plugins/bibles/forms/test_bibleimportform.py'
--- tests/interfaces/openlp_plugins/bibles/forms/test_bibleimportform.py	2016-12-31 11:05:48 +0000
+++ tests/interfaces/openlp_plugins/bibles/forms/test_bibleimportform.py	2017-03-02 21:00:50 +0000
@@ -22,7 +22,7 @@
 """
 Package to test the openlp.plugins.bibles.forms.bibleimportform package.
 """
-from unittest import TestCase
+from unittest import TestCase, skip
 
 from PyQt5 import QtWidgets
 
@@ -48,12 +48,12 @@
         Registry().register('main_window', self.main_window)
         self.form = BibleImportForm(self.main_window, MagicMock(), MagicMock())
 
-    def tearDown(self):
-        """
-        Delete all the C++ objects at the end so that we don't have a segfault
-        """
-        del self.form
-        del self.main_window
+    # def tearDown(self):
+    #     """
+    #     Delete all the C++ objects at the end so that we don't have a segfault
+    #     """
+    #     del self.form
+    #     del self.main_window
 
     @patch('openlp.plugins.bibles.forms.bibleimportform.CWExtract.get_bibles_from_http')
     @patch('openlp.plugins.bibles.forms.bibleimportform.BGExtract.get_bibles_from_http')

=== added file 'tests/interfaces/openlp_plugins/songusage/__init__.py'
=== modified file 'tests/resources/presentationmanagersongs/Agnus Dei.sng'
--- tests/resources/presentationmanagersongs/Agnus Dei.sng	2014-10-13 11:38:13 +0000
+++ tests/resources/presentationmanagersongs/Agnus Dei.sng	2017-03-02 21:00:50 +0000
@@ -1,7 +1,7 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <song xmlns="creativelifestyles/song">
 <attributes>
-<title>Agnus Dei</title>
+<Title>Agnus Dei</Title>
 <author></author>
 <copyright></copyright>
 <ccli_number></ccli_number>