Start on osis importer. Some further improvements of other importers

openlp/plugins/bibles/forms/bibleimportform.py

@@ -25,6 +25,7 @@ The bible import functions for OpenLP
 import logging
 import os
 import urllib.error
+from lxml import etree
 
 from PyQt5 import QtWidgets
 try:
@@ -33,14 +34,15 @@ try:
 except:
     PYSWORD_AVAILABLE = False
 
-from openlp.core.common import AppLocation, Settings, UiStrings, translate, clean_filename
+from openlp.core.common import AppLocation, Settings, UiStrings, trace_error_handler, translate
+from openlp.core.common.languagemanager import get_locale_key
 from openlp.core.lib.db import delete_database
+from openlp.core.lib.exceptions import ValidationError
 from openlp.core.lib.ui import critical_error_message_box
 from openlp.core.ui.lib.wizard import OpenLPWizard, WizardStrings
-from openlp.core.common.languagemanager import get_locale_key
-from openlp.plugins.bibles.lib.manager import BibleFormat
 from openlp.plugins.bibles.lib.db import clean_filename
 from openlp.plugins.bibles.lib.importers.http import CWExtract, BGExtract, BSExtract
+from openlp.plugins.bibles.lib.manager import BibleFormat
 
 log = logging.getLogger(__name__)
 
@@ -809,6 +811,8 @@ class BibleImportForm(OpenLPWizard):
                                                      sword_path=self.field('sword_zip_path'),
                                                      sword_key=self.sword_zipbible_combo_box.itemData(
                                                          self.sword_zipbible_combo_box.currentIndex()))
+
+        try:
             if importer.do_import(license_version):
                 self.manager.save_meta_data(license_version, license_version, license_copyright, license_permissions)
                 self.manager.reload_bibles()
@@ -818,7 +822,11 @@ class BibleImportForm(OpenLPWizard):
                                   'downloaded on demand and thus an internet connection is required.'))
                 else:
                     self.progress_label.setText(WizardStrings.FinishedImport)
-        else:
+                return
+        except (AttributeError, ValidationError, etree.XMLSyntaxError):
+            log.exception('Importing bible failed')
+            trace_error_handler(log)
+
         self.progress_label.setText(translate('BiblesPlugin.ImportWizardForm', 'Your Bible import failed.'))
         del self.manager.db_cache[importer.name]
         delete_database(self.plugin.settings_section, importer.file)

openlp/plugins/bibles/lib/bibleimport.py

@@ -25,8 +25,8 @@ import logging
 from lxml import etree, objectify
 from zipfile import is_zipfile
 
-from openlp.core.common import OpenLPMixin, languages
+from openlp.core.common import OpenLPMixin, languages, trace_error_handler, translate
-from openlp.core.lib import ValidationError, translate
+from openlp.core.lib import ValidationError
 from openlp.core.lib.ui import critical_error_message_box
 from openlp.plugins.bibles.lib.db import BibleDB, BiblesResourcesDB
 
@@ -103,8 +103,7 @@ class BibleImport(OpenLPMixin, BibleDB):
                                       'importing {file}'.format(book_ref=book_ref_id, file=self.filename))
         return self.create_book(name, book_ref_id, book_details['testament_id'])
 
-    @staticmethod
+    def parse_xml(self, filename, use_objectify=False, elements=None, tags=None):
-    def parse_xml(filename, use_objectify=False, elements=None, tags=None):
         """
         Parse and clean the supplied file by removing any elements or tags we don't use.
         :param filename: The filename of the xml file to parse. Str
@@ -113,6 +112,7 @@ class BibleImport(OpenLPMixin, BibleDB):
         :param tags: A tuple of element names (Str) to remove, preserving their content.
         :return: The root element of the xml document
         """
+        try:
             with open(filename, 'rb') as import_file:
                 # NOTE: We don't need to do any of the normal encoding detection here, because lxml does it's own encoding
                 # detection, and the two mechanisms together interfere with each other.
@@ -120,6 +120,9 @@ class BibleImport(OpenLPMixin, BibleDB):
                     tree = etree.parse(import_file, parser=etree.XMLParser(recover=True))
                 else:
                     tree = objectify.parse(import_file, parser=objectify.makeparser(recover=True))
+                if elements or tags:
+                    self.wizard.increment_progress_bar(translate('BiblesPlugin.OsisImport',
+                                                                 'Removing unused tags (this may take a few minutes)...'))
                 if elements:
                     # Strip tags we don't use - remove content
                     etree.strip_elements(tree, elements, with_tail=False)
@@ -127,3 +130,10 @@ class BibleImport(OpenLPMixin, BibleDB):
                     # Strip tags we don't use - keep content
                     etree.strip_tags(tree, tags)
                 return tree.getroot()
+        except OSError as e:
+            log.exception('Opening {file_name} failed.'.format(file_name=e.filename))
+            trace_error_handler(log)
+            critical_error_message_box( title='An Error Occured When Opening A File',
+                message='The following error occurred when trying to open\n{file_name}:\n\n{error}'
+                .format(file_name=e.filename, error=e.strerror))
+        return None

openlp/plugins/bibles/lib/importers/csvbible.py

@@ -128,7 +128,6 @@ class CSVBible(BibleImport):
                 translate('BiblesPlugin.CSVBible', 'Importing books... {book}').format(book=book.name))
             self.find_and_create_book(book.name, number_of_books, self.language_id)
             book_list.update({int(book.id): book.name})
-        self.application.process_events()
         return book_list
 
     def process_verses(self, verses, books):
@@ -153,7 +152,6 @@ class CSVBible(BibleImport):
                 self.session.commit()
             self.create_verse(book.id, verse.chapter_number, verse.number, verse.text)
         self.wizard.increment_progress_bar(translate('BiblesPlugin.CSVBible', 'Importing verses... done.'))
-        self.application.process_events()
         self.session.commit()
 
     def do_import(self, bible_name=None):
@@ -163,10 +161,9 @@ class CSVBible(BibleImport):
         :param bible_name: Optional name of the bible being imported. Str or None
         :return: True if the import was successful, False if it failed or was cancelled
         """
-        try:
         self.language_id = self.get_language(bible_name)
         if not self.language_id:
-                raise ValidationError(msg='Invalid language selected')
+            return False
         books = self.parse_csv_file(self.books_file, Book)
         self.wizard.progress_bar.setValue(0)
         self.wizard.progress_bar.setMinimum(0)
@@ -178,9 +175,4 @@ class CSVBible(BibleImport):
         self.wizard.progress_bar.setValue(0)
         self.wizard.progress_bar.setMaximum(len(books) + 1)
         self.process_verses(verses, book_list)
-            if self.stop_import_flag:
+        return not self.stop_import_flag
-                return False
-        except ValidationError:
-            log.exception('Could not import CSV bible')
-            return False
-        return True

openlp/plugins/bibles/lib/importers/opensong.py

@@ -23,7 +23,7 @@
 import logging
 from lxml import etree
 
-from openlp.core.common import translate, trace_error_handler
+from openlp.core.common import trace_error_handler, translate
 from openlp.core.lib.exceptions import ValidationError
 from openlp.core.lib.ui import critical_error_message_box
 from openlp.plugins.bibles.lib.bibleimport import BibleImport
@@ -146,6 +146,8 @@ class OpenSongBible(BibleImport):
         if BibleImport.is_compressed(filename):
             raise ValidationError(msg='Compressed file')
         bible = self.parse_xml(filename, use_objectify=True)
+        if bible is None:
+            raise ValidationError(msg='Error when opening file')
         root_tag = bible.tag.lower()
         if root_tag != 'bible':
             if root_tag == 'xmlbible':
@@ -165,20 +167,13 @@ class OpenSongBible(BibleImport):
         :return: True if import completed, False if import was unsuccessful
         """
         log.debug('Starting OpenSong import from "{name}"'.format(name=self.filename))
-        try:
         self.validate_file(self.filename)
         bible = self.parse_xml(self.filename, use_objectify=True)
-            # Check that we're not trying to import a Zefania XML bible, it is sometimes refered to as 'OpenSong'
+        if bible is None:
+            return False
         # No language info in the opensong format, so ask the user
         self.language_id = self.get_language_id(bible_name=self.filename)
         if not self.language_id:
             return False
         self.process_books(bible.b)
-            self.application.process_events()
+        return not self.stop_import_flag
-        except (AttributeError, ValidationError, etree.XMLSyntaxError):
-            log.exception('Loading Bible from OpenSong file failed')
-            trace_error_handler(log)
-            return False
-        if self.stop_import_flag:
-            return False
-        return True

openlp/plugins/bibles/lib/importers/osis.py

@@ -24,9 +24,9 @@ import logging
 from lxml import etree
 
 from openlp.core.common import translate, trace_error_handler
+from openlp.core.lib.exceptions import ValidationError
 from openlp.core.lib.ui import critical_error_message_box
 from openlp.plugins.bibles.lib.bibleimport import BibleImport
-from openlp.plugins.bibles.lib.db import BiblesResourcesDB
 
 log = logging.getLogger(__name__)
 
@@ -79,41 +79,44 @@ def replacement(match):
     return match.group(2).upper()
 
 
+# Precompile a few xpath-querys
+verse_in_chapter = etree.XPath('count(//ns:chapter[1]/ns:verse)', namespaces=NS)
+text_in_verse = etree.XPath('count(//ns:verse[1]/text())', namespaces=NS)
+
+
 class OSISBible(BibleImport):
     """
     `OSIS <http://www.bibletechnologies.net/>`_ Bible format importer class.
     """
-    def do_import(self, bible_name=None):
+    def process_books(self, bible_data):
         """
-        Loads a Bible from file.
+
+        :param bible_data:
+        :return:
         """
-        log.debug('Starting OSIS import from "{name}"'.format(name=self.filename))
+        no_of_books = int(bible_data.xpath("count(//ns:div[@type='book'])", namespaces=NS))
-        success = True
-        try:
-            self.wizard.increment_progress_bar(translate('BiblesPlugin.OsisImport',
-                                                         'Removing unused tags (this may take a few minutes)...'))
-            osis_bible_tree = self.parse_xml(self.filename, elements=REMOVABLE_ELEMENTS, tags=REMOVABLE_TAGS)
-            # Find bible language]
-            language = osis_bible_tree.xpath("//ns:osisText/@xml:lang", namespaces=NS)
-            language_id = self.get_language_id(language[0] if language else None, bible_name=self.filename)
-            if not language_id:
-                return False
-            no_of_books = int(osis_bible_tree.xpath("count(//ns:div[@type='book'])", namespaces=NS))
-            # Precompile a few xpath-querys
-            verse_in_chapter = etree.XPath('count(//ns:chapter[1]/ns:verse)', namespaces=NS)
-            text_in_verse = etree.XPath('count(//ns:verse[1]/text())', namespaces=NS)
         # Find books in the bible
-            bible_books = osis_bible_tree.xpath("//ns:div[@type='book']", namespaces=NS)
+        bible_books = bible_data.xpath("//ns:div[@type='book']", namespaces=NS)
         for book in bible_books:
             if self.stop_import_flag:
                 break
             # Remove div-tags in the book
             etree.strip_tags(book, '{http://www.bibletechnologies.net/2003/OSIS/namespace}div')
-                db_book = self.find_and_create_book(book.get('osisID'), no_of_books, language_id)
+            db_book = self.find_and_create_book(book.get('osisID'), no_of_books, self.language_id)
+            self.process_chapters_and_verses(db_book, book)
+            self.session.commit()
+
+    def process_chapters_and_verses(self, book, chapters):
+        """
+
+        :param book:
+        :param chapters:
+        :return:
+        """
         # Find out if chapter-tags contains the verses, or if it is used as milestone/anchor
-                if int(verse_in_chapter(book)) > 0:
+        if int(verse_in_chapter(chapters)) > 0:
             # The chapter tags contains the verses
-                    for chapter in book:
+            for chapter in chapters:
                 chapter_number = chapter.get("osisID").split('.')[1]
                 # Find out if verse-tags contains the text, or if it is used as milestone/anchor
                 if int(text_in_verse(chapter)) == 0:
@@ -125,26 +128,26 @@ class OSISBible(BibleImport):
                             verse_number = verse.get("osisID").split('.')[2]
                             verse_text = verse.tail
                             if verse_text:
-                                        self.create_verse(db_book.id, chapter_number, verse_number, verse_text.strip())
+                                self.create_verse(book.id, chapter_number, verse_number, verse_text.strip())
                 else:
                     # Verse-tags contains the text
                     for verse in chapter:
                         verse_number = verse.get("osisID").split('.')[2]
                         if verse.text:
-                                    self.create_verse(db_book.id, chapter_number, verse_number, verse.text.strip())
+                            self.create_verse(book.id, chapter_number, verse_number, verse.text.strip())
                 self.wizard.increment_progress_bar(
                     translate('BiblesPlugin.OsisImport', 'Importing %(bookname)s %(chapter)s...') %
-                            {'bookname': db_book.name, 'chapter': chapter_number})
+                    {'bookname': book.name, 'chapter': chapter_number})
         else:
             # The chapter tags is used as milestones. For now we assume verses is also milestones
             chapter_number = 0
-                    for element in book:
+            for element in chapters:
                 if element.tag == '{http://www.bibletechnologies.net/2003/OSIS/namespace}chapter' \
                         and element.get('sID'):
                     chapter_number = element.get("osisID").split('.')[1]
                     self.wizard.increment_progress_bar(
                         translate('BiblesPlugin.OsisImport', 'Importing %(bookname)s %(chapter)s...') %
-                                {'bookname': db_book.name, 'chapter': chapter_number})
+                        {'bookname': book.name, 'chapter': chapter_number})
                 elif element.tag == '{http://www.bibletechnologies.net/2003/OSIS/namespace}verse' \
                         and element.get('sID'):
                     # If this tag marks the start of a verse, the verse text is between this tag and
@@ -152,21 +155,43 @@ class OSISBible(BibleImport):
                     verse_number = element.get("osisID").split('.')[2]
                     verse_text = element.tail
                     if verse_text:
-                                self.create_verse(db_book.id, chapter_number, verse_number, verse_text.strip())
+                        self.create_verse(book.id, chapter_number, verse_number, verse_text.strip())
-                self.session.commit()
+
-            self.application.process_events()
+    def validate_file(self, filename):
-        except (ValueError, IOError):
+        """
-            log.exception('Loading bible from OSIS file failed')
+        Validate the supplied file
-            trace_error_handler(log)
+
-            success = False
+        :param filename: The supplied file
-        except etree.XMLSyntaxError as e:
+        :return: True if valid. ValidationError is raised otherwise.
-            log.exception('Loading bible from OSIS file failed')
+        """
-            trace_error_handler(log)
+        if BibleImport.is_compressed(filename):
-            success = False
+            raise ValidationError(msg='Compressed file')
-            critical_error_message_box(message=translate('BiblesPlugin.OsisImport',
+        bible = self.parse_xml(filename, use_objectify=True)
-                                                         'The file is not a valid OSIS-XML file:'
+        if bible is None:
-                                                         '\n{text}').format(text=e.msg))
+            raise ValidationError(msg='Error when opening file')
-        if self.stop_import_flag:
+        root_tag = bible.tag
+        tag_str = '{{{name_space}}}osis'.format(name_space=NS['ns'])
+        if root_tag != tag_str:
+            critical_error_message_box(
+                message=translate('BiblesPlugin.OpenSongImport',
+                                  'Incorrect Bible file type supplied. This looks like a Zefania XML bible, '
+                                  'please use the Zefania import option.'))
+            raise ValidationError(msg='Invalid xml.')
+        return True
+
+    def do_import(self, bible_name=None):
+        """
+        Loads a Bible from file.
+        """
+        log.debug('Starting OSIS import from "{name}"'.format(name=self.filename))
+        self.validate_file(self.filename)
+        bible = self.parse_xml(self.filename, elements=REMOVABLE_ELEMENTS, tags=REMOVABLE_TAGS)
+        if bible is None:
             return False
-        else:
+        # Find bible language
-            return success
+        language = bible.xpath("//ns:osisText/@xml:lang", namespaces=NS)
+        self.language_id = self.get_language_id(language[0] if language else None, bible_name=self.filename)
+        if not self.language_id:
+            return False
+        self.process_books(bible)
+        return not self.stop_import_flag

tests/functional/openlp_plugins/bibles/test_bibleimport.py

@@ -171,9 +171,12 @@ class TestBibleImport(TestCase):
         """
         Test BibleImport.parse_xml() when called with the use_objectify default value
         """
-        # GIVEN: A sample "file" to parse
+        # GIVEN: A sample "file" to parse and an instance of BibleImport
+        instance = BibleImport(MagicMock())
+        instance.wizard = MagicMock()
+
         # WHEN: Calling parse_xml
-        result = BibleImport.parse_xml('file.tst')
+        result = instance.parse_xml('file.tst')
 
         # THEN: The result returned should contain the correct data, and should be an instance of eetree_Element
         self.assertEqual(etree.tostring(result),
@@ -185,9 +188,12 @@ class TestBibleImport(TestCase):
         """
         Test BibleImport.parse_xml() when called with use_objectify set to True
         """
-        # GIVEN: A sample "file" to parse
+        # GIVEN: A sample "file" to parse and an instance of BibleImport
+        instance = BibleImport(MagicMock())
+        instance.wizard = MagicMock()
+
         # WHEN: Calling parse_xml
-        result = BibleImport.parse_xml('file.tst', use_objectify=True)
+        result = instance.parse_xml('file.tst', use_objectify=True)
 
         # THEN: The result returned should contain the correct data, and should be an instance of ObjectifiedElement
         self.assertEqual(etree.tostring(result),
@@ -199,11 +205,13 @@ class TestBibleImport(TestCase):
         """
         Test BibleImport.parse_xml() when given a tuple of elements to remove
         """
-        # GIVEN: A tuple of elements to remove
+        # GIVEN: A tuple of elements to remove and an instance of BibleImport
         elements = ('unsupported', 'x', 'y')
+        instance = BibleImport(MagicMock())
+        instance.wizard = MagicMock()
 
         # WHEN: Calling parse_xml, with a test file
-        result = BibleImport.parse_xml('file.tst', elements=elements)
+        result = instance.parse_xml('file.tst', elements=elements)
 
         # THEN: The result returned should contain the correct data
         self.assertEqual(etree.tostring(result),
@@ -213,11 +221,14 @@ class TestBibleImport(TestCase):
         """
         Test BibleImport.parse_xml() when given a tuple of tags to remove
         """
-        # GIVEN: A tuple of tags to remove
+        # GIVEN: A tuple of tags to remove and an instance of BibleImport
         tags = ('div', 'p', 'a')
+        instance = BibleImport(MagicMock())
+        instance.wizard = MagicMock()
+
 
         # WHEN: Calling parse_xml, with a test file
-        result = BibleImport.parse_xml('file.tst', tags=tags)
+        result = instance.parse_xml('file.tst', tags=tags)
 
         # THEN: The result returned should contain the correct data
         self.assertEqual(etree.tostring(result), b'<root>\n    <data>Testdatatokeep</data>\n    <data><unsupported>Test'
@@ -227,12 +238,14 @@ class TestBibleImport(TestCase):
         """
         Test BibleImport.parse_xml() when given a tuple of elements and of tags to remove
         """
-        # GIVEN: A tuple of elements and of tags to remove
+        # GIVEN: A tuple of elements and of tags to remove and an instacne of BibleImport
         elements = ('unsupported', 'x', 'y')
         tags = ('div', 'p', 'a')
+        instance = BibleImport(MagicMock())
+        instance.wizard = MagicMock()
 
         # WHEN: Calling parse_xml, with a test file
-        result = BibleImport.parse_xml('file.tst', elements=elements, tags=tags)
+        result = instance.parse_xml('file.tst', elements=elements, tags=tags)
 
         # THEN: The result returned should contain the correct data
         self.assertEqual(etree.tostring(result), b'<root>\n    <data>Testdatatokeep</data>\n    <data/>\n</root>')

tests/functional/openlp_plugins/bibles/test_csvimport.py

@@ -207,7 +207,6 @@ class TestCSVImport(TestCase):
         with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'),\
                 patch('openlp.plugins.bibles.lib.importers.csvbible.translate'):
             importer = CSVBible(mocked_manager, path='.', name='.', booksfile='books.csv', versefile='verse.csv')
-            type(importer).application = PropertyMock()
             importer.find_and_create_book = MagicMock()
             importer.language_id = 10
             importer.stop_import_flag = False
@@ -222,7 +221,6 @@ class TestCSVImport(TestCase):
             # 		The returned data should be a dictionary with both song's id and names.
             self.assertEqual(importer.find_and_create_book.mock_calls,
                              [call('1. Mosebog', 2, 10), call('2. Mosebog', 2, 10)])
-            importer.application.process_events.assert_called_once_with()
             self.assertDictEqual(result, {1: '1. Mosebog', 2: '2. Mosebog'})
 
     def process_verses_stopped_import_test(self):
@@ -233,7 +231,6 @@ class TestCSVImport(TestCase):
         mocked_manager = MagicMock()
         with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'):
             importer = CSVBible(mocked_manager, path='.', name='.', booksfile='books.csv', versefile='verse.csv')
-            type(importer).application = PropertyMock()
             importer.get_book_name = MagicMock()
             importer.session = MagicMock()
             importer.stop_import_flag = True
@@ -245,7 +242,6 @@ class TestCSVImport(TestCase):
             # THEN: get_book_name should not be called and the return value should be None
             self.assertFalse(importer.get_book_name.called)
             importer.wizard.increment_progress_bar.assert_called_once_with('Importing verses... done.')
-            importer.application.process_events.assert_called_once_with()
             self.assertIsNone(result)
 
     def process_verses_successful_test(self):
@@ -257,7 +253,6 @@ class TestCSVImport(TestCase):
         with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'),\
                 patch('openlp.plugins.bibles.lib.importers.csvbible.translate'):
             importer = CSVBible(mocked_manager, path='.', name='.', booksfile='books.csv', versefile='verse.csv')
-            type(importer).application = PropertyMock()
             importer.create_verse = MagicMock()
             importer.get_book = MagicMock(return_value=Book('1', '1', '1. Mosebog', '1Mos'))
             importer.get_book_name = MagicMock(return_value='1. Mosebog')
@@ -280,7 +275,6 @@ class TestCSVImport(TestCase):
                              [call('1', 1, 1, 'I Begyndelsen skabte Gud Himmelen og Jorden.'),
                               call('1', 1, 2, 'Og Jorden var øde og tom, og der var Mørke over Verdensdybet. '
                                               'Men Guds Ånd svævede over Vandene.')])
-            importer.application.process_events.assert_called_once_with()
 
     def do_import_invalid_language_id_test(self):
         """
@@ -299,7 +293,6 @@ class TestCSVImport(TestCase):
             # THEN: The log.exception method should have been called to show that it reached the except clause.
             # False should be returned.
             importer.get_language.assert_called_once_with('Bible Name')
-            mocked_log.exception.assert_called_once_with('Could not import CSV bible')
             self.assertFalse(result)
 
     def do_import_stop_import_test(self):

tests/functional/openlp_plugins/bibles/test_opensongimport.py

@@ -387,96 +387,55 @@ class TestOpenSongImport(TestCase, TestMixin):
         self.assertEqual(context.exception.msg, 'Invalid xml.')
         self.assertFalse(mocked_message_box.called)
 
-    @patch('openlp.plugins.bibles.lib.importers.opensong.log')
+    def do_import_parse_xml_fails_test(self):
-    @patch('openlp.plugins.bibles.lib.importers.opensong.trace_error_handler')
-    def do_import_attribute_error_test(self, mocked_trace_error_handler, mocked_log):
         """
-        Test do_import when an AttributeError exception is raised
+        Test do_import when parse_xml fails (returns None)
         """
-        # GIVEN: An instance of OpenSongBible and a mocked validate_file which raises an AttributeError
+        # GIVEN: An instance of OpenSongBible and a mocked parse_xml which returns False
+        with patch('openlp.plugins.bibles.lib.importers.opensong.log'), \
+            patch.object(OpenSongBible, 'validate_file'), \
+            patch.object(OpenSongBible, 'parse_xml', return_value=None), \
+            patch.object(OpenSongBible, 'get_language_id') as mocked_language_id:
             importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
-        importer.validate_file = MagicMock(**{'side_effect': AttributeError()})
-        importer.parse_xml = MagicMock()
 
             # WHEN: Calling do_import
             result = importer.do_import()
 
-        # THEN: do_import should return False after logging the exception
+            # THEN: do_import should return False and get_language_id should have not been called
-        mocked_log.exception.assert_called_once_with('Loading Bible from OpenSong file failed')
-        mocked_trace_error_handler.assert_called_once_with(mocked_log)
             self.assertFalse(result)
-        self.assertFalse(importer.parse_xml.called)
+            self.assertFalse(mocked_language_id.called)
 
-    @patch('openlp.plugins.bibles.lib.importers.opensong.log')
+    def do_import_no_language_test(self):
-    @patch('openlp.plugins.bibles.lib.importers.opensong.trace_error_handler')
-    def do_import_validation_error_test(self, mocked_trace_error_handler, mocked_log):
-        """
-        Test do_import when an ValidationError exception is raised
-        """
-        # GIVEN: An instance of OpenSongBible and a mocked validate_file which raises an ValidationError
-        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
-        importer.validate_file = MagicMock(**{'side_effect': ValidationError()})
-        importer.parse_xml = MagicMock()
-
-        # WHEN: Calling do_import
-        result = importer.do_import()
-
-        # THEN: do_import should return False after logging the exception. parse_xml should not be called.
-        mocked_log.exception.assert_called_once_with('Loading Bible from OpenSong file failed')
-        mocked_trace_error_handler.assert_called_once_with(mocked_log)
-        self.assertFalse(result)
-        self.assertFalse(importer.parse_xml.called)
-
-    @patch('openlp.plugins.bibles.lib.importers.opensong.log')
-    @patch('openlp.plugins.bibles.lib.importers.opensong.trace_error_handler')
-    def do_import_xml_syntax_error_test(self, mocked_trace_error_handler, mocked_log):
-        """
-        Test do_import when an etree.XMLSyntaxError exception is raised
-        """
-        # GIVEN: An instance of OpenSongBible and a mocked validate_file which raises an etree.XMLSyntaxError
-        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
-        importer.validate_file = MagicMock(**{'side_effect': etree.XMLSyntaxError(None, None, None, None)})
-        importer.parse_xml = MagicMock()
-
-        # WHEN: Calling do_import
-        result = importer.do_import()
-
-        # THEN: do_import should return False after logging the exception. parse_xml should not be called.
-        mocked_log.exception.assert_called_once_with('Loading Bible from OpenSong file failed')
-        mocked_trace_error_handler.assert_called_once_with(mocked_log)
-        self.assertFalse(result)
-        self.assertFalse(importer.parse_xml.called)
-
-    @patch('openlp.plugins.bibles.lib.importers.opensong.log')
-    def do_import_no_language_test(self, mocked_log):
         """
         Test do_import when the user cancels the language selection dialog
         """
         # GIVEN: An instance of OpenSongBible and a mocked get_language which returns False
+        with patch('openlp.plugins.bibles.lib.importers.opensong.log'), \
+                patch.object(OpenSongBible, 'validate_file'), \
+                patch.object(OpenSongBible, 'parse_xml'), \
+                patch.object(OpenSongBible, 'get_language_id', return_value=False), \
+                patch.object(OpenSongBible, 'process_books') as mocked_process_books:
             importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
-        importer.validate_file = MagicMock()
-        importer.parse_xml = MagicMock()
-        importer.get_language_id = MagicMock(**{'return_value': False})
-        importer.process_books = MagicMock()
 
             # WHEN: Calling do_import
             result = importer.do_import()
 
             # THEN: do_import should return False and process_books should have not been called
             self.assertFalse(result)
-        self.assertFalse(importer.process_books.called)
+            self.assertFalse(mocked_process_books.called)
 
-    @patch('openlp.plugins.bibles.lib.importers.opensong.log')
+    def do_import_stop_import_test(self):
-    def do_import_stop_import_test(self, mocked_log):
         """
         Test do_import when the stop_import_flag is set to True
         """
         # GIVEN: An instance of OpenSongBible and stop_import_flag set to True
+        with patch('openlp.plugins.bibles.lib.importers.opensong.log'), \
+                patch.object(OpenSongBible, 'validate_file'), \
+                patch.object(OpenSongBible, 'parse_xml'), \
+                patch.object(OpenSongBible, 'get_language_id', return_value=10), \
+                patch.object(OpenSongBible, 'process_books') as mocked_process_books:
+
             importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
-        importer.validate_file = MagicMock()
-        importer.parse_xml = MagicMock()
-        importer.get_language_id = MagicMock(**{'return_value': 10})
-        importer.process_books = MagicMock()
             importer.stop_import_flag = True
 
             # WHEN: Calling do_import
@@ -484,21 +443,19 @@ class TestOpenSongImport(TestCase, TestMixin):
 
             # THEN: do_import should return False and process_books should have not been called
             self.assertFalse(result)
-        self.assertTrue(importer.application.process_events.called)
+            self.assertTrue(mocked_process_books.called)
 
-        self.assertTrue(importer.application.process_events.called)
+    def do_import_completes_test(self):
-
-    @patch('openlp.plugins.bibles.lib.importers.opensong.log')
-    def do_import_completes_test(self, mocked_log):
         """
         Test do_import when it completes successfully
         """
-        # GIVEN: An instance of OpenSongBible and stop_import_flag set to True
+        # GIVEN: An instance of OpenSongBible and stop_import_flag set to False
+        with patch('openlp.plugins.bibles.lib.importers.opensong.log'), \
+                patch.object(OpenSongBible, 'validate_file'), \
+                patch.object(OpenSongBible, 'parse_xml'), \
+                patch.object(OpenSongBible, 'get_language_id', return_value=10), \
+                patch.object(OpenSongBible, 'process_books'):
             importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
-        importer.validate_file = MagicMock()
-        importer.parse_xml = MagicMock()
-        importer.get_language_id = MagicMock(**{'return_value': 10})
-        importer.process_books = MagicMock()
             importer.stop_import_flag = False
 
             # WHEN: Calling do_import

tests/functional/openlp_plugins/bibles/test_osisimport.py

@@ -42,14 +42,12 @@ class TestOsisImport(TestCase):
 
     def setUp(self):
         self.registry_patcher = patch('openlp.plugins.bibles.lib.db.Registry')
+        self.addCleanup(self.registry_patcher.stop)
         self.registry_patcher.start()
         self.manager_patcher = patch('openlp.plugins.bibles.lib.db.Manager')
+        self.addCleanup(self.manager_patcher.stop)
         self.manager_patcher.start()
 
-    def tearDown(self):
-        self.registry_patcher.stop()
-        self.manager_patcher.stop()
-
         def test_create_importer(self):
             """
             Test creating an instance of the OSIS file importer
@@ -63,6 +61,83 @@ class TestOsisImport(TestCase):
             # THEN: The importer should be an instance of BibleDB
             self.assertIsInstance(importer, BibleDB)
 
+    def do_import_parse_xml_fails_test(self):
+        """
+        Test do_import when parse_xml fails (returns None)
+        """
+        # GIVEN: An instance of OpenSongBible and a mocked parse_xml which returns False
+        with patch('openlp.plugins.bibles.lib.importers.opensong.log'), \
+                patch.object(OSISBible, 'validate_file'), \
+                patch.object(OSISBible, 'parse_xml', return_value=None), \
+                patch.object(OSISBible, 'get_language_id') as mocked_language_id:
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling do_import
+            result = importer.do_import()
+
+            # THEN: do_import should return False and get_language_id should have not been called
+            self.assertFalse(result)
+            self.assertFalse(mocked_language_id.called)
+
+    def do_import_no_language_test(self):
+        """
+        Test do_import when the user cancels the language selection dialog
+        """
+        # GIVEN: An instance of OpenSongBible and a mocked get_language which returns False
+        with patch('openlp.plugins.bibles.lib.importers.opensong.log'), \
+                patch.object(OSISBible, 'validate_file'), \
+                patch.object(OSISBible, 'parse_xml'), \
+                patch.object(OSISBible, 'get_language_id', **{'return_value': False}), \
+                patch.object(OSISBible, 'process_books') as mocked_process_books:
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling do_import
+            result = importer.do_import()
+
+            # THEN: do_import should return False and process_books should have not been called
+            self.assertFalse(result)
+            self.assertFalse(mocked_process_books.called)
+
+    def do_import_stop_import_test(self):
+        """
+        Test do_import when the stop_import_flag is set to True
+        """
+        # GIVEN: An instance of OpenSongBible and stop_import_flag set to True
+        with patch('openlp.plugins.bibles.lib.importers.opensong.log'), \
+                patch.object(OSISBible, 'validate_file'), \
+                patch.object(OSISBible, 'parse_xml'), \
+                patch.object(OSISBible, 'get_language_id', **{'return_value': 10}), \
+                patch.object(OSISBible, 'process_books'):
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+            importer.stop_import_flag = True
+
+            # WHEN: Calling do_import
+            result = importer.do_import()
+
+            # THEN: do_import should return False and process_books should have not been called
+            self.assertFalse(result)
+            self.assertTrue(importer.process_books.called)
+
+    @patch('openlp.plugins.bibles.lib.importers.opensong.log')
+    def do_import_completes_test(self, mocked_log):
+        """
+        Test do_import when it completes successfully
+        """
+        # GIVEN: An instance of OpenSongBible and stop_import_flag set to True
+        with patch('openlp.plugins.bibles.lib.importers.opensong.log'), \
+                patch.object(OSISBible, 'validate_file'), \
+                patch.object(OSISBible, 'parse_xml'), \
+                patch.object(OSISBible, 'get_language_id', **{'return_value': 10}), \
+                patch.object(OSISBible, 'process_books'):
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+            importer.stop_import_flag = False
+
+            # WHEN: Calling do_import
+            result = importer.do_import()
+
+            # THEN: do_import should return True
+            self.assertTrue(result)
+
         def test_file_import_nested_tags(self):
             """
             Test the actual import of OSIS Bible file, with nested chapter and verse tags