Some more bible refactors

lp:~phill-ridout/openlp/even-more-refactors (revision 2707)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1769/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1680/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1618/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1374/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/964/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/1032/
[...

bzr-revno: 2696

openlp/core/common/openlpmixin.py

@@ -71,6 +71,12 @@ class OpenLPMixin(object):
         """
         self.logger.info(message)
 
+    def log_warning(self, message):
+        """
+        Common log warning handler
+        """
+        self.logger.warning(message)
+
     def log_error(self, message):
         """
         Common log error handler which prints the calling path

openlp/plugins/bibles/bibleplugin.py

@@ -140,10 +140,10 @@ class BiblePlugin(Plugin):
 
     def uses_theme(self, theme):
         """
-        Called to find out if the bible plugin is currently using a theme. Returns ``1`` if the theme is being used,
-        otherwise returns ``0``.
+        Called to find out if the bible plugin is currently using a theme.
 
         :param theme: The theme
+        :return: 1 if the theme is being used, otherwise returns 0
         """
         if str(self.settings_tab.bible_theme) == theme:
             return 1
@@ -151,11 +151,11 @@ class BiblePlugin(Plugin):
 
     def rename_theme(self, old_theme, new_theme):
         """
-        Rename the theme the bible plugin is using making the plugin use the
-        new name.
+        Rename the theme the bible plugin is using, making the plugin use the new name.
 
         :param old_theme: The name of the theme the plugin should stop using. Unused for this particular plugin.
         :param new_theme:  The new name the plugin should now use.
+        :return: None
         """
         self.settings_tab.bible_theme = new_theme
         self.settings_tab.save()

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,16 +811,22 @@ class BibleImportForm(OpenLPWizard):
                                                      sword_path=self.field('sword_zip_path'),
                                                      sword_key=self.sword_zipbible_combo_box.itemData(
                                                          self.sword_zipbible_combo_box.currentIndex()))
-        if importer.do_import(license_version):
-            self.manager.save_meta_data(license_version, license_version, license_copyright, license_permissions)
-            self.manager.reload_bibles()
-            if bible_type == BibleFormat.WebDownload:
-                self.progress_label.setText(
-                    translate('BiblesPlugin.ImportWizardForm', 'Registered Bible. Please note, that verses will be '
-                              'downloaded on demand and thus an internet connection is required.'))
-            else:
-                self.progress_label.setText(WizardStrings.FinishedImport)
-        else:
-            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)
+
+        try:
+            if importer.do_import(license_version) and not importer.stop_import_flag:
+                self.manager.save_meta_data(license_version, license_version, license_copyright, license_permissions)
+                self.manager.reload_bibles()
+                if bible_type == BibleFormat.WebDownload:
+                    self.progress_label.setText(
+                        translate('BiblesPlugin.ImportWizardForm', 'Registered Bible. Please note, that verses will be '
+                                  'downloaded on demand and thus an internet connection is required.'))
+                else:
+                    self.progress_label.setText(WizardStrings.FinishedImport)
+                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/__init__.py

@@ -173,7 +173,7 @@ class BibleStrings(object):
 
 def update_reference_separators():
     """
-    Updates separators and matches for parsing and formating scripture references.
+    Updates separators and matches for parsing and formatting scripture references.
     """
     default_separators = [
         '|'.join([
@@ -215,7 +215,7 @@ def update_reference_separators():
         # escape reserved characters
         for character in '\\.^$*+?{}[]()':
             source_string = source_string.replace(character, '\\' + character)
-        # add various unicode alternatives
+        # add various Unicode alternatives
         source_string = source_string.replace('-', '(?:[-\u00AD\u2010\u2011\u2012\u2014\u2014\u2212\uFE63\uFF0D])')
         source_string = source_string.replace(',', '(?:[,\u201A])')
         REFERENCE_SEPARATORS['sep_{role}'.format(role=role)] = '\s*(?:{source})\s*'.format(source=source_string)

openlp/plugins/bibles/lib/bibleimport.py

@@ -20,25 +20,84 @@
 # Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
 ###############################################################################
 
-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, Registry, RegistryProperties, languages, translate
 from openlp.core.lib import ValidationError
-from openlp.plugins.bibles.lib.db import BibleDB, BiblesResourcesDB
-
-log = logging.getLogger(__name__)
+from openlp.core.lib.ui import critical_error_message_box
+from openlp.plugins.bibles.lib.db import AlternativeBookNamesDB, BibleDB, BiblesResourcesDB
 
 
-class BibleImport(OpenLPMixin, BibleDB):
+class BibleImport(OpenLPMixin, RegistryProperties, BibleDB):
     """
     Helper class to import bibles from a third party source into OpenLP
     """
-    # TODO: Test
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
         self.filename = kwargs['filename'] if 'filename' in kwargs else None
+        self.wizard = None
+        self.stop_import_flag = False
+        Registry().register_function('openlp_stop_wizard', self.stop_import)
+
+    @staticmethod
+    def is_compressed(file):
+        """
+        Check if the supplied file is compressed
+
+        :param file: A path to the file to check
+        """
+        if is_zipfile(file):
+            critical_error_message_box(
+                message=translate('BiblesPlugin.BibleImport',
+                                  'The file "{file}" you supplied is compressed. You must decompress it before import.'
+                                  ).format(file=file))
+            return True
+        return False
+
+    def get_book_ref_id_by_name(self, book, maxbooks=66, language_id=None):
+        """
+        Find the book id from the name or abbreviation of the book. If it doesn't currently exist, ask the user.
+
+        :param book: The name or abbreviation of the book
+        :param maxbooks: The number of books in the bible
+        :param language_id: The language_id of the bible
+        :return: The id of the bible, or None
+        """
+        self.log_debug('BibleDB.get_book_ref_id_by_name:("{book}", "{lang}")'.format(book=book, lang=language_id))
+        book_temp = BiblesResourcesDB.get_book(book, True)
+        if book_temp:
+            return book_temp['id']
+        book_id = BiblesResourcesDB.get_alternative_book_name(book)
+        if book_id:
+            return book_id
+        book_id = AlternativeBookNamesDB.get_book_reference_id(book)
+        if book_id:
+            return book_id
+        from openlp.plugins.bibles.forms import BookNameForm
+        book_name = BookNameForm(self.wizard)
+        if book_name.exec(book, self.get_books(), maxbooks) and book_name.book_id:
+            AlternativeBookNamesDB.create_alternative_book_name(book, book_name.book_id, language_id)
+            return book_name.book_id
+
+    def get_language(self, bible_name=None):
+        """
+        If no language is given it calls a dialog window where the user could  select the bible language.
+        Return the language id of a bible.
+
+        :param bible_name: The language the bible is.
+        """
+        self.log_debug('BibleImpoer.get_language()')
+        from openlp.plugins.bibles.forms import LanguageForm
+        language_id = None
+        language_form = LanguageForm(self.wizard)
+        if language_form.exec(bible_name):
+            combo_box = language_form.language_combo_box
+            language_id = combo_box.itemData(combo_box.currentIndex())
+        if not language_id:
+            return None
+        self.save_meta('language_id', language_id)
+        return language_id
 
     def get_language_id(self, file_language=None, bible_name=None):
         """
@@ -58,8 +117,8 @@ class BibleImport(OpenLPMixin, BibleDB):
             language_id = self.get_language(bible_name)
         if not language_id:
             # User cancelled get_language dialog
-            log.error('Language detection failed when importing from "{name}". User aborted language selection.'
-                      .format(name=bible_name))
+            self.log_error('Language detection failed when importing from "{name}". User aborted language selection.'
+                           .format(name=bible_name))
             return None
         self.save_meta('language_id', language_id)
         return language_id
@@ -77,7 +136,7 @@ class BibleImport(OpenLPMixin, BibleDB):
         if name:
             book_ref_id = self.get_book_ref_id_by_name(name, no_of_books, language_id)
         else:
-            log.debug('No book name supplied. Falling back to guess_id')
+            self.log_debug('No book name supplied. Falling back to guess_id')
             book_ref_id = guess_id
         if not book_ref_id:
             raise ValidationError(msg='Could not resolve book_ref_id in "{}"'.format(self.filename))
@@ -87,8 +146,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(filename, use_objectify=False, elements=None, tags=None):
+    def parse_xml(self, 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
@@ -97,17 +155,80 @@ 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
         """
-        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.
-            if not use_objectify:
-                tree = etree.parse(import_file, parser=etree.XMLParser(recover=True))
-            else:
-                tree = objectify.parse(import_file, parser=objectify.makeparser(recover=True))
-            if elements:
-                # Strip tags we don't use - remove content
-                etree.strip_elements(tree, elements, with_tail=False)
-            if tags:
-                # Strip tags we don't use - keep content
-                etree.strip_tags(tree, tags)
-            return tree.getroot()
+        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.
+                if not use_objectify:
+                    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)
+                if tags:
+                    # Strip tags we don't use - keep content
+                    etree.strip_tags(tree, tags)
+                return tree.getroot()
+        except OSError as e:
+            self.log_exception('Opening {file_name} failed.'.format(file_name=e.filename))
+            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
+
+    def register(self, wizard):
+        """
+        This method basically just initialises the database. It is called from the Bible Manager when a Bible is
+        imported. Descendant classes may want to override this method to supply their own custom
+        initialisation as well.
+
+        :param wizard: The actual Qt wizard form.
+        """
+        self.wizard = wizard
+        return self.name
+
+    def set_current_chapter(self, book_name, chapter_name):
+        self.wizard.increment_progress_bar(translate('BiblesPlugin.OsisImport', 'Importing {book} {chapter}...')
+                                           .format(book=book_name, chapter=chapter_name))
+
+    def stop_import(self):
+        """
+        Stops the import of the Bible.
+        """
+        self.log_debug('Stopping import')
+        self.stop_import_flag = True
+
+    def validate_xml_file(self, filename, tag):
+        """
+        Validate the supplied file
+
+        :param filename: The supplied file
+        :param tag: The expected root tag type
+        :return: True if valid. ValidationError is raised otherwise.
+        """
+        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()
+        bible_type = translate('BiblesPlugin.BibleImport', 'unknown type of',
+                               'This looks like an unknown type of XML bible.')
+        if root_tag == tag:
+            return True
+        elif root_tag == 'bible':
+            bible_type = "OpenSong"
+        elif root_tag == '{http://www.bibletechnologies.net/2003/osis/namespace}osis':
+            bible_type = 'OSIS'
+        elif root_tag == 'xmlbible':
+            bible_type = 'Zefania'
+        critical_error_message_box(
+            message=translate('BiblesPlugin.BibleImport',
+                              'Incorrect Bible file type supplied. This looks like an {bible_type} XML bible.'
+                              .format(bible_type=bible_type)))
+        raise ValidationError(msg='Invalid xml.')

openlp/plugins/bibles/lib/db.py

@@ -33,7 +33,7 @@ from sqlalchemy.exc import OperationalError
 from sqlalchemy.orm import class_mapper, mapper, relation
 from sqlalchemy.orm.exc import UnmappedClassError
 
-from openlp.core.common import Registry, RegistryProperties, AppLocation, translate, clean_filename
+from openlp.core.common import AppLocation, translate, clean_filename
 from openlp.core.lib.db import BaseModel, init_db, Manager
 from openlp.core.lib.ui import critical_error_message_box
 from openlp.plugins.bibles.lib import upgrade
@@ -106,7 +106,7 @@ def init_schema(url):
     return session
 
 
-class BibleDB(Manager, RegistryProperties):
+class BibleDB(Manager):
     """
     This class represents a database-bound Bible. It is used as a base class for all the custom importers, so that
     the can implement their own import methods, but benefit from the database methods in here via inheritance,
@@ -140,7 +140,6 @@ class BibleDB(Manager, RegistryProperties):
             raise KeyError('Missing keyword argument "path".')
         if 'name' not in kwargs and 'file' not in kwargs:
             raise KeyError('Missing keyword argument "name" or "file".')
-        self.stop_import_flag = False
         if 'name' in kwargs:
             self.name = kwargs['name']
             if not isinstance(self.name, str):
@@ -153,15 +152,6 @@ class BibleDB(Manager, RegistryProperties):
                 self.get_name()
         if 'path' in kwargs:
             self.path = kwargs['path']
-        self.wizard = None
-        Registry().register_function('openlp_stop_wizard', self.stop_import)
-
-    def stop_import(self):
-        """
-        Stops the import of the Bible.
-        """
-        log.debug('Stopping import')
-        self.stop_import_flag = True
 
     def get_name(self):
         """
@@ -171,17 +161,6 @@ class BibleDB(Manager, RegistryProperties):
         self.name = version_name.value if version_name else None
         return self.name
 
-    def register(self, wizard):
-        """
-        This method basically just initialises the database. It is called from the Bible Manager when a Bible is
-        imported. Descendant classes may want to override this method to supply their own custom
-        initialisation as well.
-
-        :param wizard: The actual Qt wizard form.
-        """
-        self.wizard = wizard
-        return self.name
-
     def create_book(self, name, bk_ref_id, testament=1):
         """
         Add a book to the database.
@@ -306,26 +285,6 @@ class BibleDB(Manager, RegistryProperties):
         log.debug('BibleDB.get_book_by_book_ref_id("{ref}")'.format(ref=ref_id))
         return self.get_object_filtered(Book, Book.book_reference_id.like(ref_id))
 
-    def get_book_ref_id_by_name(self, book, maxbooks, language_id=None):
-        log.debug('BibleDB.get_book_ref_id_by_name:("{book}", "{lang}")'.format(book=book, lang=language_id))
-        book_id = None
-        if BiblesResourcesDB.get_book(book, True):
-            book_temp = BiblesResourcesDB.get_book(book, True)
-            book_id = book_temp['id']
-        elif BiblesResourcesDB.get_alternative_book_name(book):
-            book_id = BiblesResourcesDB.get_alternative_book_name(book)
-        elif AlternativeBookNamesDB.get_book_reference_id(book):
-            book_id = AlternativeBookNamesDB.get_book_reference_id(book)
-        else:
-            from openlp.plugins.bibles.forms import BookNameForm
-            book_name = BookNameForm(self.wizard)
-            if book_name.exec(book, self.get_books(), maxbooks):
-                book_id = book_name.book_id
-            if book_id:
-                AlternativeBookNamesDB.create_alternative_book_name(
-                    book, book_id, language_id)
-        return book_id
-
     def get_book_ref_id_by_localised_name(self, book, language_selection):
         """
         Return the id of a named book.
@@ -462,25 +421,6 @@ class BibleDB(Manager, RegistryProperties):
             return 0
         return count
 
-    def get_language(self, bible_name=None):
-        """
-        If no language is given it calls a dialog window where the user could  select the bible language.
-        Return the language id of a bible.
-
-        :param bible_name: The language the bible is.
-        """
-        log.debug('BibleDB.get_language()')
-        from openlp.plugins.bibles.forms import LanguageForm
-        language_id = None
-        language_form = LanguageForm(self.wizard)
-        if language_form.exec(bible_name):
-            combo_box = language_form.language_combo_box
-            language_id = combo_box.itemData(combo_box.currentIndex())
-        if not language_id:
-            return None
-        self.save_meta('language_id', language_id)
-        return language_id
-
     def dump_bible(self):
         """
         Utility debugging method to dump the contents of a bible.

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

@@ -50,7 +50,6 @@ There are two acceptable formats of the verses file.  They are:
 All CSV files are expected to use a comma (',') as the delimiter and double quotes ('"') as the quote symbol.
 """
 import csv
-import logging
 from collections import namedtuple
 
 from openlp.core.common import get_file_encoding, translate
@@ -58,8 +57,6 @@ from openlp.core.lib.exceptions import ValidationError
 from openlp.plugins.bibles.lib.bibleimport import BibleImport
 
 
-log = logging.getLogger(__name__)
-
 Book = namedtuple('Book', 'id, testament_id, name, abbreviation')
 Verse = namedtuple('Verse', 'book_id_name, chapter_number, number, text')
 
@@ -68,15 +65,13 @@ class CSVBible(BibleImport):
     """
     This class provides a specialisation for importing of CSV Bibles.
     """
-    log.info('CSVBible loaded')
-
     def __init__(self, *args, **kwargs):
         """
         Loads a Bible from a set of CSV files. This class assumes the files contain all the information and a clean
         bible is being loaded.
         """
-        log.info(self.__class__.__name__)
         super().__init__(*args, **kwargs)
+        self.log_info(self.__class__.__name__)
         self.books_file = kwargs['booksfile']
         self.verses_file = kwargs['versefile']
 
@@ -123,12 +118,11 @@ class CSVBible(BibleImport):
         number_of_books = len(books)
         for book in books:
             if self.stop_import_flag:
-                return None
+                break
             self.wizard.increment_progress_bar(
                 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):
@@ -142,7 +136,7 @@ class CSVBible(BibleImport):
         book_ptr = None
         for verse in verses:
             if self.stop_import_flag:
-                return None
+                break
             verse_book = self.get_book_name(verse.book_id_name, books)
             if book_ptr != verse_book:
                 book = self.get_book(verse_book)
@@ -151,9 +145,7 @@ class CSVBible(BibleImport):
                     translate('BiblesPlugin.CSVBible', 'Importing verses from {book}...',
                               'Importing verses from <book name>...').format(book=book.name))
                 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.create_verse(book.id, int(verse.chapter_number), int(verse.number), verse.text)
         self.session.commit()
 
     def do_import(self, bible_name=None):
@@ -163,24 +155,16 @@ 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')
-            books = self.parse_csv_file(self.books_file, Book)
-            self.wizard.progress_bar.setValue(0)
-            self.wizard.progress_bar.setMinimum(0)
-            self.wizard.progress_bar.setMaximum(len(books))
-            book_list = self.process_books(books)
-            if self.stop_import_flag:
-                return False
-            verses = self.parse_csv_file(self.verses_file, Verse)
-            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 False
-        except ValidationError:
-            log.exception('Could not import CSV bible')
+        self.language_id = self.get_language(bible_name)
+        if not self.language_id:
             return False
+        books = self.parse_csv_file(self.books_file, Book)
+        self.wizard.progress_bar.setValue(0)
+        self.wizard.progress_bar.setMinimum(0)
+        self.wizard.progress_bar.setMaximum(len(books))
+        book_list = self.process_books(books)
+        verses = self.parse_csv_file(self.verses_file, Verse)
+        self.wizard.progress_bar.setValue(0)
+        self.wizard.progress_bar.setMaximum(len(books) + 1)
+        self.process_verses(verses, book_list)
         return True

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

@@ -20,109 +20,126 @@
 # Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
 ###############################################################################
 
-import logging
-from lxml import etree, objectify
-
-from openlp.core.common import translate, trace_error_handler
-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 BibleDB, BiblesResourcesDB
 
 
-log = logging.getLogger(__name__)
+def get_text(element):
+    """
+    Recursively get all text in an objectify element and its child elements.
+
+    :param element: An objectify element to get the text from
+    :return: The text content of the element (str)
+    """
+    verse_text = ''
+    if element.text:
+        verse_text = element.text
+    for sub_element in element.iterchildren():
+        verse_text += get_text(sub_element)
+    if element.tail:
+        verse_text += element.tail
+    return verse_text
+
+
+def parse_chapter_number(number, previous_number):
+    """
+    Parse the chapter number
+
+    :param number: The raw data from the xml
+    :param previous_number: The previous chapter number
+    :return: Number of current chapter. (Int)
+    """
+    if number:
+        return int(number.split()[-1])
+    return previous_number + 1
 
 
 class OpenSongBible(BibleImport):
     """
     OpenSong Bible format importer class. This class is used to import Bibles from OpenSong's XML format.
     """
-    def get_text(self, element):
-        """
-        Recursively get all text in an objectify element and its child elements.
 
-        :param element: An objectify element to get the text from
+    def parse_verse_number(self, number, previous_number):
         """
-        verse_text = ''
-        if element.text:
-            verse_text = element.text
-        for sub_element in element.iterchildren():
-            verse_text += self.get_text(sub_element)
-        if element.tail:
-            verse_text += element.tail
-        return verse_text
+        Parse the verse number retrieved from the xml
+
+        :param number: The raw data from the xml
+        :param previous_number: The previous verse number
+        :return: Number of current verse. (Int)
+        """
+        if not number:
+            return previous_number + 1
+        try:
+            return int(number)
+        except ValueError:
+            verse_parts = number.split('-')
+            if len(verse_parts) > 1:
+                number = int(verse_parts[0])
+                return number
+        except TypeError:
+            self.log_warning('Illegal verse number: {verse_no}'.format(verse_no=str(number)))
+        return previous_number + 1
+
+    def process_books(self, books):
+        """
+        Extract and create the books from the objectified xml
+
+        :param books: Objectified xml
+        :return: None
+        """
+        for book in books:
+            if self.stop_import_flag:
+                break
+            db_book = self.find_and_create_book(str(book.attrib['n']), len(books), self.language_id)
+            self.process_chapters(db_book, book.c)
+            self.session.commit()
+
+    def process_chapters(self, book, chapters):
+        """
+        Extract and create the chapters from the objectified xml for the book `book`
+
+        :param book: A database Book object to add the chapters to
+        :param chapters: Objectified xml containing chapters
+        :return: None
+        """
+        chapter_number = 0
+        for chapter in chapters:
+            if self.stop_import_flag:
+                break
+            chapter_number = parse_chapter_number(chapter.attrib['n'], chapter_number)
+            self.set_current_chapter(book.name, chapter_number)
+            self.process_verses(book, chapter_number, chapter.v)
+
+    def process_verses(self, book, chapter_number, verses):
+        """
+        Extract and create the verses from the objectified xml
+
+        :param book: A database Book object
+        :param chapter_number: The chapter number to add the verses to (int)
+        :param verses: Objectified xml containing verses
+        :return: None
+        """
+        verse_number = 0
+        for verse in verses:
+            if self.stop_import_flag:
+                break
+            verse_number = self.parse_verse_number(verse.attrib['n'], verse_number)
+            self.create_verse(book.id, chapter_number, verse_number, get_text(verse))
 
     def do_import(self, bible_name=None):
         """
-        Loads a Bible from file.
+        Loads an Open Song Bible from a file.
+
+        :param bible_name: The name of the bible being imported
+        :return: True if import completed, False if import was unsuccessful
         """
-        log.debug('Starting OpenSong import from "{name}"'.format(name=self.filename))
-        success = True
-        try:
-            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.tag.upper() == 'XMLBIBLE':
-                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.'))
-                return False
-            # No language info in the opensong format, so ask the user
-            language_id = self.get_language_id(bible_name=self.filename)
-            if not language_id:
-                return False
-            for book in bible.b:
-                if self.stop_import_flag:
-                    break
-                book_ref_id = self.get_book_ref_id_by_name(str(book.attrib['n']), len(bible.b), language_id)
-                if not book_ref_id:
-                    log.error('Importing books from "{name}" failed'.format(name=self.filename))
-                    return False
-                book_details = BiblesResourcesDB.get_book_by_id(book_ref_id)
-                db_book = self.create_book(book.attrib['n'], book_ref_id, book_details['testament_id'])
-                chapter_number = 0
-                for chapter in book.c:
-                    if self.stop_import_flag:
-                        break
-                    number = chapter.attrib['n']
-                    if number:
-                        chapter_number = int(number.split()[-1])
-                    else:
-                        chapter_number += 1
-                    verse_number = 0
-                    for verse in chapter.v:
-                        if self.stop_import_flag:
-                            break
-                        number = verse.attrib['n']
-                        if number:
-                            try:
-                                number = int(number)
-                            except ValueError:
-                                verse_parts = number.split('-')
-                                if len(verse_parts) > 1:
-                                    number = int(verse_parts[0])
-                            except TypeError:
-                                log.warning('Illegal verse number: {verse:d}'.format(verse=verse.attrib['n']))
-                            verse_number = number
-                        else:
-                            verse_number += 1
-                        self.create_verse(db_book.id, chapter_number, verse_number, self.get_text(verse))
-                    self.wizard.increment_progress_bar(translate('BiblesPlugin.Opensong',
-                                                                 'Importing {name} {chapter}...'
-                                                                 ).format(name=db_book.name, chapter=chapter_number))
-                self.session.commit()
-            self.application.process_events()
-        except etree.XMLSyntaxError as inst:
-            trace_error_handler(log)
-            critical_error_message_box(
-                message=translate('BiblesPlugin.OpenSongImport',
-                                  'Incorrect Bible file type supplied. OpenSong Bibles may be '
-                                  'compressed. You must decompress them before import.'))
-            log.exception(inst)
-            success = False
-        except (IOError, AttributeError):
-            log.exception('Loading Bible from OpenSong file failed')
-            success = False
-        if self.stop_import_flag:
+        self.log_debug('Starting OpenSong import from "{name}"'.format(name=self.filename))
+        self.validate_xml_file(self.filename, 'bible')
+        bible = self.parse_xml(self.filename, use_objectify=True)
+        if bible is None:
             return False
-        else:
-            return success
+        # 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)
+        return True

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

@@ -20,15 +20,9 @@
 # Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
 ###############################################################################
 
-import logging
 from lxml import etree
 
-from openlp.core.common import translate, trace_error_handler
-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__)
 
 NS = {'ns': 'http://www.bibletechnologies.net/2003/OSIS/namespace'}
 # Tags we don't use and can remove the content
@@ -74,104 +68,106 @@ REMOVABLE_TAGS = (
     '{http://www.bibletechnologies.net/2003/OSIS/namespace}caption'
 )
 
-
-def replacement(match):
-    return match.group(2).upper()
+# Precompile a few xpath-querys
+verse_in_chapter = etree.XPath('//ns:chapter[1]/ns:verse', namespaces=NS)
+text_in_verse = etree.XPath('//ns:verse[1]/text()', namespaces=NS)
 
 
 class OSISBible(BibleImport):
     """
     `OSIS <http://www.bibletechnologies.net/>`_ Bible format importer class.
     """
+    def process_books(self, bible_data):
+        """
+        Extract and create the bible books from the parsed xml
+
+        :param bible_data: parsed xml
+        :return: None
+        """
+        # Find books in the bible
+        bible_books = bible_data.xpath("//ns:div[@type='book']", namespaces=NS)
+        no_of_books = len(bible_books)
+        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, self.language_id)
+            self.process_chapters(db_book, book)
+            self.session.commit()
+
+    def process_chapters(self, book, chapters):
+        """
+        Extract the chapters, and do some initial processing of the verses
+
+        :param book: An OpenLP bible database book object
+        :param chapters: parsed chapters
+        :return: None
+        """
+        # Find out if chapter-tags contains the verses, or if it is used as milestone/anchor
+        if verse_in_chapter(chapters):
+            # The chapter tags contains the verses
+            for chapter in chapters:
+                chapter_number = int(chapter.get("osisID").split('.')[1])
+                self.set_current_chapter(book.name, chapter_number)
+                # Find out if verse-tags contains the text, or if it is used as milestone/anchor
+                if not text_in_verse(chapter):
+                    # verse-tags are used as milestone
+                    for verse in chapter:
+                        # If this tag marks the start of a verse, the verse text is between this tag and
+                        # the next tag, which the "tail" attribute gives us.
+                        self.process_verse(book, chapter_number, verse, use_milestones=True)
+                else:
+                    # Verse-tags contains the text
+                    for verse in chapter:
+                        self.process_verse(book, chapter_number, verse)
+        else:
+            # The chapter tags is used as milestones. For now we assume verses is also milestones
+            chapter_number = 0
+            for element in chapters:
+                if element.tag == '{http://www.bibletechnologies.net/2003/OSIS/namespace}chapter' \
+                        and element.get('sID'):
+                    chapter_number = int(element.get("osisID").split('.')[1])
+                    self.set_current_chapter(book.name, chapter_number)
+                elif element.tag == '{http://www.bibletechnologies.net/2003/OSIS/namespace}verse':
+                    # If this tag marks the start of a verse, the verse text is between this tag and
+                    # the next tag, which the "tail" attribute gives us.
+                    self.process_verse(book, chapter_number, element, use_milestones=True)
+
+    def process_verse(self, book, chapter_number, element, use_milestones=False):
+        """
+        Process a verse element
+        :param book: A database Book object
+        :param chapter_number: The chapter number to add the verses to (int)
+        :param element: The verse element to process. (etree element type)
+        :param use_milestones: set to True to process a 'milestone' verse. Defaults to False
+        :return: None
+        """
+        osis_id = element.get("osisID")
+        if not osis_id:
+            return None
+        verse_number = int(osis_id.split('.')[2])
+        verse_text = ''
+        if use_milestones and element.get('sID'):
+            verse_text = element.tail
+        elif not use_milestones:
+            verse_text = element.text
+        if verse_text:
+            self.create_verse(book.id, chapter_number, verse_number, verse_text.strip())
+
     def do_import(self, bible_name=None):
         """
         Loads a Bible from file.
         """
-        log.debug('Starting OSIS import from "{name}"'.format(name=self.filename))
-        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
-            num_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)
-            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'))
-                book_ref_id = self.get_book_ref_id_by_name(book.get('osisID'), num_books, language_id)
-                if not book_ref_id:
-                    log.error('Importing books from "{name}" failed'.format(name=self.filename))
-                    return False
-                book_details = BiblesResourcesDB.get_book_by_id(book_ref_id)
-                db_book = self.create_book(book_details['name'], book_ref_id, book_details['testament_id'])
-                # Find out if chapter-tags contains the verses, or if it is used as milestone/anchor
-                if int(verse_in_chapter(book)) > 0:
-                    # The chapter tags contains the verses
-                    for chapter in book:
-                        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:
-                            # verse-tags are used as milestone
-                            for verse in chapter:
-                                # If this tag marks the start of a verse, the verse text is between this tag and
-                                # the next tag, which the "tail" attribute gives us.
-                                if verse.get('sID'):
-                                    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())
-                        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.wizard.increment_progress_bar(
-                            translate('BiblesPlugin.OsisImport', 'Importing %(bookname)s %(chapter)s...') %
-                            {'bookname': db_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:
-                        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})
-                        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
-                            # the next tag, which the "tail" attribute gives us.
-                            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.session.commit()
-            self.application.process_events()
-        except (ValueError, IOError):
-            log.exception('Loading bible from OSIS file failed')
-            trace_error_handler(log)
-            success = False
-        except etree.XMLSyntaxError as e:
-            log.exception('Loading bible from OSIS file failed')
-            trace_error_handler(log)
-            success = False
-            critical_error_message_box(message=translate('BiblesPlugin.OsisImport',
-                                                         'The file is not a valid OSIS-XML file:'
-                                                         '\n{text}').format(text=e.msg))
-        if self.stop_import_flag:
+        self.log_debug('Starting OSIS import from "{name}"'.format(name=self.filename))
+        self.validate_xml_file(self.filename, '{http://www.bibletechnologies.net/2003/osis/namespace}osis')
+        bible = self.parse_xml(self.filename, elements=REMOVABLE_ELEMENTS, tags=REMOVABLE_TAGS)
+        if bible is None:
             return False
-        else:
-            return success
+        # Find bible language
+        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 True

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

@@ -54,7 +54,7 @@ class ZefaniaBible(BibleImport):
             language_id = self.get_language_id(language[0] if language else None, bible_name=self.filename)
             if not language_id:
                 return False
-            num_books = int(xmlbible.xpath('count(//BIBLEBOOK)'))
+            no_of_books = int(xmlbible.xpath('count(//BIBLEBOOK)'))
             self.wizard.progress_bar.setMaximum(int(xmlbible.xpath('count(//CHAPTER)')))
             for BIBLEBOOK in xmlbible:
                 if self.stop_import_flag:
@@ -64,7 +64,7 @@ class ZefaniaBible(BibleImport):
                 if not bname and not bnumber:
                     continue
                 if bname:
-                    book_ref_id = self.get_book_ref_id_by_name(bname, num_books, language_id)
+                    book_ref_id = self.get_book_ref_id_by_name(bname, no_of_books, language_id)
                 else:
                     log.debug('Could not find a name, will use number, basically a guess.')
                     book_ref_id = int(bnumber)
@@ -79,7 +79,8 @@ class ZefaniaBible(BibleImport):
                     chapter_number = CHAPTER.get("cnumber")
                     for VERS in CHAPTER:
                         verse_number = VERS.get("vnumber")
-                        self.create_verse(db_book.id, chapter_number, verse_number, VERS.text.replace('<BR/>', '\n'))
+                        self.create_verse(
+                            db_book.id, int(chapter_number), int(verse_number), VERS.text.replace('<BR/>', '\n'))
                     self.wizard.increment_progress_bar(
                         translate('BiblesPlugin.Zefnia',
                                   'Importing {book} {chapter}...').format(book=db_book.name,

openlp/plugins/bibles/lib/manager.py

@@ -23,8 +23,8 @@
 import logging
 import os
 
-from openlp.core.common import RegistryProperties, AppLocation, Settings, translate, delete_file, UiStrings
-from openlp.plugins.bibles.lib import parse_reference, LanguageSelection
+from openlp.core.common import AppLocation, OpenLPMixin, RegistryProperties, Settings, translate, delete_file, UiStrings
+from openlp.plugins.bibles.lib import LanguageSelection, parse_reference
 from openlp.plugins.bibles.lib.db import BibleDB, BibleMeta
 from .importers.csvbible import CSVBible
 from .importers.http import HTTPBible
@@ -88,7 +88,7 @@ class BibleFormat(object):
         ]
 
 
-class BibleManager(RegistryProperties):
+class BibleManager(OpenLPMixin, RegistryProperties):
     """
     The Bible manager which holds and manages all the Bibles.
     """

openlp/plugins/bibles/lib/upgrade.py

@@ -24,8 +24,6 @@ The :mod:`upgrade` module provides a way for the database and schema that is the
 """
 import logging
 
-from sqlalchemy import delete, func, insert, select
-
 log = logging.getLogger(__name__)
 __version__ = 1
 
@@ -35,166 +33,6 @@ def upgrade_1(session, metadata):
     """
     Version 1 upgrade.
 
-    This upgrade renames a number of keys to a single naming convention.
+    This upgrade renamed a number of keys to a single naming convention.
     """
-    metadata_table = metadata.tables['metadata']
-    # Copy "Version" to "name" ("version" used by upgrade system)
-    try:
-        session.execute(insert(metadata_table).values(
-            key='name',
-            value=select(
-                [metadata_table.c.value],
-                metadata_table.c.key == 'Version'
-            ).as_scalar()
-        ))
-        session.execute(delete(metadata_table).where(metadata_table.c.key == 'Version'))
-    except:
-        log.exception('Exception when upgrading Version')
-    # Copy "Copyright" to "copyright"
-    try:
-        session.execute(insert(metadata_table).values(
-            key='copyright',
-            value=select(
-                [metadata_table.c.value],
-                metadata_table.c.key == 'Copyright'
-            ).as_scalar()
-        ))
-        session.execute(delete(metadata_table).where(metadata_table.c.key == 'Copyright'))
-    except:
-        log.exception('Exception when upgrading Copyright')
-    # Copy "Permissions" to "permissions"
-    try:
-        session.execute(insert(metadata_table).values(
-            key='permissions',
-            value=select(
-                [metadata_table.c.value],
-                metadata_table.c.key == 'Permissions'
-            ).as_scalar()
-        ))
-        session.execute(delete(metadata_table).where(metadata_table.c.key == 'Permissions'))
-    except:
-        log.exception('Exception when upgrading Permissions')
-    # Copy "Bookname language" to "book_name_language"
-    try:
-        value_count = session.execute(
-            select(
-                [func.count(metadata_table.c.value)],
-                metadata_table.c.key == 'Bookname language'
-            )
-        ).scalar()
-        if value_count > 0:
-            session.execute(insert(metadata_table).values(
-                key='book_name_language',
-                value=select(
-                    [metadata_table.c.value],
-                    metadata_table.c.key == 'Bookname language'
-                ).as_scalar()
-            ))
-            session.execute(delete(metadata_table).where(metadata_table.c.key == 'Bookname language'))
-    except:
-        log.exception('Exception when upgrading Bookname language')
-    # Copy "download source" to "download_source"
-    try:
-        value_count = session.execute(
-            select(
-                [func.count(metadata_table.c.value)],
-                metadata_table.c.key == 'download source'
-            )
-        ).scalar()
-        log.debug('download source: {count}'.format(count=value_count))
-        if value_count > 0:
-            session.execute(insert(metadata_table).values(
-                key='download_source',
-                value=select(
-                    [metadata_table.c.value],
-                    metadata_table.c.key == 'download source'
-                ).as_scalar()
-            ))
-            session.execute(delete(metadata_table).where(metadata_table.c.key == 'download source'))
-    except:
-        log.exception('Exception when upgrading download source')
-    # Copy "download name" to "download_name"
-    try:
-        value_count = session.execute(
-            select(
-                [func.count(metadata_table.c.value)],
-                metadata_table.c.key == 'download name'
-            )
-        ).scalar()
-        log.debug('download name: {count}'.format(count=value_count))
-        if value_count > 0:
-            session.execute(insert(metadata_table).values(
-                key='download_name',
-                value=select(
-                    [metadata_table.c.value],
-                    metadata_table.c.key == 'download name'
-                ).as_scalar()
-            ))
-            session.execute(delete(metadata_table).where(metadata_table.c.key == 'download name'))
-    except:
-        log.exception('Exception when upgrading download name')
-    # Copy "proxy server" to "proxy_server"
-    try:
-        value_count = session.execute(
-            select(
-                [func.count(metadata_table.c.value)],
-                metadata_table.c.key == 'proxy server'
-            )
-        ).scalar()
-        log.debug('proxy server: {count}'.format(count=value_count))
-        if value_count > 0:
-            session.execute(insert(metadata_table).values(
-                key='proxy_server',
-                value=select(
-                    [metadata_table.c.value],
-                    metadata_table.c.key == 'proxy server'
-                ).as_scalar()
-            ))
-            session.execute(delete(metadata_table).where(metadata_table.c.key == 'proxy server'))
-    except:
-        log.exception('Exception when upgrading proxy server')
-    # Copy "proxy username" to "proxy_username"
-    try:
-        value_count = session.execute(
-            select(
-                [func.count(metadata_table.c.value)],
-                metadata_table.c.key == 'proxy username'
-            )
-        ).scalar()
-        log.debug('proxy username: {count}'.format(count=value_count))
-        if value_count > 0:
-            session.execute(insert(metadata_table).values(
-                key='proxy_username',
-                value=select(
-                    [metadata_table.c.value],
-                    metadata_table.c.key == 'proxy username'
-                ).as_scalar()
-            ))
-            session.execute(delete(metadata_table).where(metadata_table.c.key == 'proxy username'))
-    except:
-        log.exception('Exception when upgrading proxy username')
-    # Copy "proxy password" to "proxy_password"
-    try:
-        value_count = session.execute(
-            select(
-                [func.count(metadata_table.c.value)],
-                metadata_table.c.key == 'proxy password'
-            )
-        ).scalar()
-        log.debug('proxy password: {count}'.format(count=value_count))
-        if value_count > 0:
-            session.execute(insert(metadata_table).values(
-                key='proxy_password',
-                value=select(
-                    [metadata_table.c.value],
-                    metadata_table.c.key == 'proxy password'
-                ).as_scalar()
-            ))
-            session.execute(delete(metadata_table).where(metadata_table.c.key == 'proxy password'))
-    except:
-        log.exception('Exception when upgrading proxy password')
-    try:
-        session.execute(delete(metadata_table).where(metadata_table.c.key == 'dbversion'))
-    except:
-        log.exception('Exception when deleting dbversion')
-    session.commit()
+    log.info('No upgrades to perform')

tests/functional/openlp_core_ui/test_exceptionform.py

@@ -24,18 +24,13 @@ Package to test the openlp.core.ui.exeptionform package.
 """
 
 import os
-import socket
 import tempfile
-import urllib
 from unittest import TestCase
 from unittest.mock import mock_open
 
-from PyQt5.QtCore import QUrlQuery
-
 from openlp.core.common import Registry
-from openlp.core.ui.firsttimeform import FirstTimeForm
 
-from tests.functional import MagicMock, patch
+from tests.functional import patch
 from tests.helpers.testmixin import TestMixin
 
 from openlp.core.ui import exceptionform

tests/functional/openlp_plugins/bibles/test_bibleimport.py

@@ -27,9 +27,12 @@ from io import BytesIO
 from lxml import etree, objectify
 
 from unittest import TestCase
+from PyQt5.QtWidgets import QDialog
 
 from openlp.core.common.languages import Language
+from openlp.core.lib.exceptions import ValidationError
 from openlp.plugins.bibles.lib.bibleimport import BibleImport
+from openlp.plugins.bibles.lib.db import BibleDB
 from tests.functional import MagicMock, patch
 
 
@@ -39,22 +42,103 @@ class TestBibleImport(TestCase):
     """
 
     def setUp(self):
-        test_file = BytesIO(b'<?xml version="1.0" encoding="UTF-8" ?>\n'
-                            b'<root>\n'
-                            b'    <data><div>Test<p>data</p><a>to</a>keep</div></data>\n'
-                            b'    <data><unsupported>Test<x>data</x><y>to</y>discard</unsupported></data>\n'
-                            b'</root>')
-        self.file_patcher = patch('builtins.open', return_value=test_file)
-        self.log_patcher = patch('openlp.plugins.bibles.lib.bibleimport.log')
+        self.test_file = BytesIO(
+            b'<?xml version="1.0" encoding="UTF-8" ?>\n'
+            b'<root>\n'
+            b'    <data><div>Test<p>data</p><a>to</a>keep</div></data>\n'
+            b'    <data><unsupported>Test<x>data</x><y>to</y>discard</unsupported></data>\n'
+            b'</root>'
+        )
+        self.open_patcher = patch('builtins.open')
+        self.addCleanup(self.open_patcher.stop)
+        self.mocked_open = self.open_patcher.start()
+        self.critical_error_message_box_patcher = \
+            patch('openlp.plugins.bibles.lib.bibleimport.critical_error_message_box')
+        self.addCleanup(self.critical_error_message_box_patcher.stop)
+        self.mocked_critical_error_message_box = self.critical_error_message_box_patcher.start()
         self.setup_patcher = patch('openlp.plugins.bibles.lib.db.BibleDB._setup')
-
-        self.addCleanup(self.file_patcher.stop)
-        self.addCleanup(self.log_patcher.stop)
         self.addCleanup(self.setup_patcher.stop)
-
-        self.file_patcher.start()
-        self.mock_log = self.log_patcher.start()
         self.setup_patcher.start()
+        self.translate_patcher = patch('openlp.plugins.bibles.lib.bibleimport.translate',
+                                       side_effect=lambda module, string_to_translate, *args: string_to_translate)
+        self.addCleanup(self.translate_patcher.stop)
+        self.mocked_translate = self.translate_patcher.start()
+        self.registry_patcher = patch('openlp.plugins.bibles.lib.bibleimport.Registry')
+        self.addCleanup(self.registry_patcher.stop)
+        self.registry_patcher.start()
+
+    def init_kwargs_none_test(self):
+        """
+        Test the initialisation of the BibleImport Class when no key word arguments are supplied
+        """
+        # GIVEN: A patched BibleDB._setup, BibleImport class and mocked parent
+        # WHEN: Creating an instance of BibleImport with no key word arguments
+        instance = BibleImport(MagicMock())
+
+        # THEN: The filename attribute should be None
+        self.assertIsNone(instance.filename)
+        self.assertIsInstance(instance, BibleDB)
+
+    def init_kwargs_set_test(self):
+        """
+        Test the initialisation of the BibleImport Class when supplied with select keyword arguments
+        """
+        # GIVEN: A patched BibleDB._setup, BibleImport class and mocked parent
+        # WHEN: Creating an instance of BibleImport with selected key word arguments
+        kwargs = {'filename': 'bible.xml'}
+        instance = BibleImport(MagicMock(), **kwargs)
+
+        # THEN: The filename keyword should be set to bible.xml
+        self.assertEqual(instance.filename, 'bible.xml')
+        self.assertIsInstance(instance, BibleDB)
+
+    def get_language_canceled_test(self):
+        """
+        Test the BibleImport.get_language method when the user rejects the dialog box
+        """
+        # GIVEN: A mocked LanguageForm with an exec method which returns QtDialog.Rejected and an instance of BibleDB
+        with patch.object(BibleDB, '_setup'), patch('openlp.plugins.bibles.forms.LanguageForm') as mocked_language_form:
+
+            # The integer value of QtDialog.Rejected is 0. Using the enumeration causes a seg fault for some reason
+            mocked_language_form_instance = MagicMock(**{'exec.return_value': 0})
+            mocked_language_form.return_value = mocked_language_form_instance
+            instance = BibleImport(MagicMock())
+            mocked_wizard = MagicMock()
+            instance.wizard = mocked_wizard
+
+            # WHEN: Calling get_language()
+            result = instance.get_language()
+
+            # THEN: get_language() should return False
+            mocked_language_form.assert_called_once_with(mocked_wizard)
+            mocked_language_form_instance.exec.assert_called_once_with(None)
+            self.assertFalse(result, 'get_language() should return False if the user rejects the dialog box')
+
+    def get_language_accepted_test(self):
+        """
+        Test the BibleImport.get_language method when the user accepts the dialog box
+        """
+        # GIVEN: A mocked LanguageForm with an exec method which returns QtDialog.Accepted an instance of BibleDB and
+        #       a combobox with the selected item data as 10
+        with patch.object(BibleDB, 'save_meta'), patch.object(BibleDB, '_setup'), \
+                patch('openlp.plugins.bibles.forms.LanguageForm') as mocked_language_form:
+
+            # The integer value of QtDialog.Accepted is 1. Using the enumeration causes a seg fault for some reason
+            mocked_language_form_instance = MagicMock(**{'exec.return_value': 1,
+                                                         'language_combo_box.itemData.return_value': 10})
+            mocked_language_form.return_value = mocked_language_form_instance
+            instance = BibleImport(MagicMock())
+            mocked_wizard = MagicMock()
+            instance.wizard = mocked_wizard
+
+            # WHEN: Calling get_language()
+            result = instance.get_language('Bible Name')
+
+            # THEN: get_language() should return the id of the selected language in the combo box
+            mocked_language_form.assert_called_once_with(mocked_wizard)
+            mocked_language_form_instance.exec.assert_called_once_with('Bible Name')
+            self.assertEqual(result, 10, 'get_language() should return the id of the language the user has chosen when '
+                                         'they accept the dialog box')
 
     def get_language_id_language_found_test(self):
         """
@@ -63,7 +147,7 @@ class TestBibleImport(TestCase):
         # GIVEN: A mocked languages.get_language which returns language and an instance of BibleImport
         with patch('openlp.core.common.languages.get_language', return_value=Language(30, 'English', 'en')) \
                 as mocked_languages_get_language, \
-                patch('openlp.plugins.bibles.lib.db.BibleDB.get_language') as mocked_db_get_language:
+                patch.object(BibleImport, 'get_language') as mocked_db_get_language:
             instance = BibleImport(MagicMock())
             instance.save_meta = MagicMock()
 
@@ -81,9 +165,8 @@ class TestBibleImport(TestCase):
         Test get_language_id() when called with a name not found in the languages list
         """
         # GIVEN: A mocked languages.get_language which returns language and an instance of BibleImport
-        with patch('openlp.core.common.languages.get_language', return_value=None) \
-                as mocked_languages_get_language, \
-                patch('openlp.plugins.bibles.lib.db.BibleDB.get_language', return_value=20) as mocked_db_get_language:
+        with patch('openlp.core.common.languages.get_language', return_value=None) as mocked_languages_get_language, \
+                patch.object(BibleImport, 'get_language', return_value=20) as mocked_db_get_language:
             instance = BibleImport(MagicMock())
             instance.save_meta = MagicMock()
 
@@ -103,8 +186,8 @@ class TestBibleImport(TestCase):
         # GIVEN: A mocked languages.get_language which returns None a mocked BibleDB.get_language which returns a
         #       language id.
         with patch('openlp.core.common.languages.get_language', return_value=None) as mocked_languages_get_language, \
-                patch('openlp.plugins.bibles.lib.db.BibleDB.get_language', return_value=40) as mocked_db_get_language:
-            self.mock_log.error.reset_mock()
+                patch.object(BibleImport, 'get_language', return_value=40) as mocked_db_get_language, \
+                patch.object(BibleImport, 'log_error') as mocked_log_error:
             instance = BibleImport(MagicMock())
             instance.save_meta = MagicMock()
 
@@ -114,7 +197,7 @@ class TestBibleImport(TestCase):
             # THEN: The id of the language returned from BibleDB.get_language should be returned
             mocked_languages_get_language.assert_called_once_with('English')
             mocked_db_get_language.assert_called_once_with('KJV')
-            self.assertFalse(self.mock_log.error.called)
+            self.assertFalse(mocked_log_error.error.called)
             instance.save_meta.assert_called_once_with('language_id', 40)
             self.assertEqual(result, 40)
 
@@ -125,8 +208,8 @@ class TestBibleImport(TestCase):
         # GIVEN: A mocked languages.get_language which returns None a mocked BibleDB.get_language which returns a
         #       language id.
         with patch('openlp.core.common.languages.get_language', return_value=None) as mocked_languages_get_language, \
-                patch('openlp.plugins.bibles.lib.db.BibleDB.get_language', return_value=None) as mocked_db_get_language:
-            self.mock_log.error.reset_mock()
+                patch.object(BibleImport, 'get_language', return_value=None) as mocked_db_get_language, \
+                patch.object(BibleImport, 'log_error') as mocked_log_error:
             instance = BibleImport(MagicMock())
             instance.save_meta = MagicMock()
 
@@ -136,18 +219,148 @@ class TestBibleImport(TestCase):
             # THEN: None should be returned and an error should be logged
             mocked_languages_get_language.assert_called_once_with('Qwerty')
             mocked_db_get_language.assert_called_once_with('KJV')
-            self.mock_log.error.assert_called_once_with('Language detection failed when importing from "KJV". '
-                                                        'User aborted language selection.')
+            mocked_log_error.assert_called_once_with(
+                'Language detection failed when importing from "KJV". User aborted language selection.')
             self.assertFalse(instance.save_meta.called)
             self.assertIsNone(result)
 
+    def get_book_ref_id_by_name_get_book_test(self):
+        """
+        Test get_book_ref_id_by_name when the book is found as a book in BiblesResourcesDB
+        """
+        # GIVEN: An instance of BibleImport and a mocked BiblesResourcesDB which returns a book id when get_book is
+        #        called
+        with patch.object(BibleImport, 'log_debug'), \
+                patch('openlp.plugins.bibles.lib.bibleimport.BiblesResourcesDB',
+                      **{'get_book.return_value': {'id': 20}}):
+            instance = BibleImport(MagicMock())
+
+            # WHEN: Calling get_book_ref_id_by_name
+            result = instance.get_book_ref_id_by_name('Gen', 66, 4)
+
+            # THEN: The bible id should be returned
+            self.assertEqual(result, 20)
+
+    def get_book_ref_id_by_name_get_alternative_book_name_test(self):
+        """
+        Test get_book_ref_id_by_name when the book is found as an alternative book in BiblesResourcesDB
+        """
+        # GIVEN: An instance of BibleImport and a mocked BiblesResourcesDB which returns a book id when
+        #        get_alternative_book_name is called
+        with patch.object(BibleImport, 'log_debug'), \
+                patch('openlp.plugins.bibles.lib.bibleimport.BiblesResourcesDB',
+                      **{'get_book.return_value': None, 'get_alternative_book_name.return_value': 30}):
+            instance = BibleImport(MagicMock())
+
+            # WHEN: Calling get_book_ref_id_by_name
+            result = instance.get_book_ref_id_by_name('Gen', 66, 4)
+
+            # THEN: The bible id should be returned
+            self.assertEqual(result, 30)
+
+    def get_book_ref_id_by_name_get_book_reference_id_test(self):
+        """
+        Test get_book_ref_id_by_name when the book is found as a book in AlternativeBookNamesDB
+        """
+        # GIVEN: An instance of BibleImport and a mocked AlternativeBookNamesDB which returns a book id when
+        #        get_book_reference_id is called
+        with patch.object(BibleImport, 'log_debug'), \
+                patch('openlp.plugins.bibles.lib.bibleimport.BiblesResourcesDB',
+                      **{'get_book.return_value': None, 'get_alternative_book_name.return_value': None}), \
+                patch('openlp.plugins.bibles.lib.bibleimport.AlternativeBookNamesDB',
+                      **{'get_book_reference_id.return_value': 40}):
+            instance = BibleImport(MagicMock())
+
+            # WHEN: Calling get_book_ref_id_by_name
+            result = instance.get_book_ref_id_by_name('Gen', 66, 4)
+
+            # THEN: The bible id should be returned
+            self.assertEqual(result, 40)
+
+    def get_book_ref_id_by_name_book_name_form_rejected_test(self):
+        """
+        Test get_book_ref_id_by_name when the user rejects the BookNameForm
+        """
+        # GIVEN: An instance of BibleImport and a mocked BookNameForm which simulates a user rejecting the dialog
+        with patch.object(BibleImport, 'log_debug'), patch.object(BibleImport, 'get_books'), \
+                patch('openlp.plugins.bibles.lib.bibleimport.BiblesResourcesDB',
+                      **{'get_book.return_value': None, 'get_alternative_book_name.return_value': None}), \
+                patch('openlp.plugins.bibles.lib.bibleimport.AlternativeBookNamesDB',
+                      **{'get_book_reference_id.return_value': None}), \
+                patch('openlp.plugins.bibles.forms.BookNameForm',
+                      return_value=MagicMock(**{'exec.return_value': QDialog.Rejected})):
+            instance = BibleImport(MagicMock())
+
+            # WHEN: Calling get_book_ref_id_by_name
+            result = instance.get_book_ref_id_by_name('Gen', 66, 4)
+
+            # THEN: None should be returned
+            self.assertIsNone(result)
+
+    def get_book_ref_id_by_name_book_name_form_accepted_test(self):
+        """
+        Test get_book_ref_id_by_name when the user accepts the BookNameForm
+        """
+        # GIVEN: An instance of BibleImport and a mocked BookNameForm which simulates a user accepting the dialog
+        with patch.object(BibleImport, 'log_debug'), patch.object(BibleImport, 'get_books'), \
+                patch('openlp.plugins.bibles.lib.bibleimport.BiblesResourcesDB',
+                      **{'get_book.return_value': None, 'get_alternative_book_name.return_value': None}), \
+                patch('openlp.plugins.bibles.lib.bibleimport.AlternativeBookNamesDB',
+                      **{'get_book_reference_id.return_value': None}) as mocked_alternative_book_names_db, \
+                patch('openlp.plugins.bibles.forms.BookNameForm',
+                      return_value=MagicMock(**{'exec.return_value': QDialog.Accepted, 'book_id': 50})):
+            instance = BibleImport(MagicMock())
+
+            # WHEN: Calling get_book_ref_id_by_name
+            result = instance.get_book_ref_id_by_name('Gen', 66, 4)
+
+            # THEN: An alternative book name should be created and a bible id should be returned
+            mocked_alternative_book_names_db.create_alternative_book_name.assert_called_once_with('Gen', 50, 4)
+            self.assertEqual(result, 50)
+
+    def is_compressed_compressed_test(self):
+        """
+        Test is_compressed when the 'file' being tested is compressed
+        """
+        # GIVEN: An instance of BibleImport and a mocked is_zipfile which returns True
+        with patch('openlp.plugins.bibles.lib.bibleimport.is_zipfile', return_value=True):
+            instance = BibleImport(MagicMock())
+
+            # WHEN: Calling is_compressed
+            result = instance.is_compressed('file.ext')
+
+            # THEN: Then critical_error_message_box should be called informing the user that the file is compressed and
+            #       True should be returned
+            self.mocked_critical_error_message_box.assert_called_once_with(
+                message='The file "file.ext" you supplied is compressed. You must decompress it before import.')
+            self.assertTrue(result)
+
+    def is_compressed_not_compressed_test(self):
+        """
+        Test is_compressed when the 'file' being tested is not compressed
+        """
+        # GIVEN: An instance of BibleImport and a mocked is_zipfile which returns False
+        with patch('openlp.plugins.bibles.lib.bibleimport.is_zipfile', return_value=False):
+            instance = BibleImport(MagicMock())
+
+            # WHEN: Calling is_compressed
+            result = instance.is_compressed('file.ext')
+
+            # THEN: False should be returned and critical_error_message_box should not have been called
+            self.assertFalse(result)
+            self.assertFalse(self.mocked_critical_error_message_box.called)
+
     def parse_xml_etree_test(self):
         """
         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
+        self.mocked_open.return_value = self.test_file
+        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),
@@ -159,9 +372,13 @@ 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
+        self.mocked_open.return_value = self.test_file
+        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),
@@ -173,11 +390,14 @@ 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
+        self.mocked_open.return_value = self.test_file
         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),
@@ -187,11 +407,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
+        self.mocked_open.return_value = self.test_file
         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'
@@ -201,12 +424,192 @@ 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
+        self.mocked_open.return_value = self.test_file
         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>')
+
+    def parse_xml_file_file_not_found_exception_test(self):
+        """
+        Test that parse_xml handles a FileNotFoundError exception correctly
+        """
+        with patch.object(BibleImport, 'log_exception') as mocked_log_exception:
+            # GIVEN: A mocked open which raises a FileNotFoundError and an instance of BibleImporter
+            exception = FileNotFoundError()
+            exception.filename = 'file.tst'
+            exception.strerror = 'No such file or directory'
+            self.mocked_open.side_effect = exception
+            importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling parse_xml
+            result = importer.parse_xml('file.tst')
+
+            # THEN: parse_xml should have caught the error, informed the user and returned None
+            mocked_log_exception.assert_called_once_with('Opening file.tst failed.')
+            self.mocked_critical_error_message_box.assert_called_once_with(
+                title='An Error Occured When Opening A File',
+                message='The following error occurred when trying to open\nfile.tst:\n\nNo such file or directory')
+            self.assertIsNone(result)
+
+    def parse_xml_file_permission_error_exception_test(self):
+        """
+        Test that parse_xml handles a PermissionError exception correctly
+        """
+        with patch.object(BibleImport, 'log_exception') as mocked_log_exception:
+            # GIVEN: A mocked open which raises a PermissionError and an instance of BibleImporter
+            exception = PermissionError()
+            exception.filename = 'file.tst'
+            exception.strerror = 'Permission denied'
+            self.mocked_open.side_effect = exception
+            importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling parse_xml
+            result = importer.parse_xml('file.tst')
+
+            # THEN: parse_xml should have caught the error, informed the user and returned None
+            mocked_log_exception.assert_called_once_with('Opening file.tst failed.')
+            self.mocked_critical_error_message_box.assert_called_once_with(
+                title='An Error Occured When Opening A File',
+                message='The following error occurred when trying to open\nfile.tst:\n\nPermission denied')
+            self.assertIsNone(result)
+
+    def set_current_chapter_test(self):
+        """
+        Test set_current_chapter
+        """
+        # GIVEN: An instance of BibleImport and a mocked wizard
+        importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+        importer.wizard = MagicMock()
+
+        # WHEN: Calling set_current_chapter
+        importer.set_current_chapter('Book_Name', 'Chapter')
+
+        # THEN: Increment_progress_bar should have been called with a text string
+        importer.wizard.increment_progress_bar.assert_called_once_with('Importing Book_Name Chapter...')
+
+    def validate_xml_file_compressed_file_test(self):
+        """
+        Test that validate_xml_file raises a ValidationError when is_compressed returns True
+        """
+        # GIVEN: A mocked parse_xml which returns None
+        with patch.object(BibleImport, 'is_compressed', return_value=True):
+            importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling is_compressed
+            # THEN: ValidationError should be raised, with the message 'Compressed file'
+            with self.assertRaises(ValidationError) as context:
+                importer.validate_xml_file('file.name', 'xbible')
+            self.assertEqual(context.exception.msg, 'Compressed file')
+
+    def validate_xml_file_parse_xml_fails_test(self):
+        """
+        Test that validate_xml_file raises a ValidationError when parse_xml returns None
+        """
+        # GIVEN: A mocked parse_xml which returns None
+        with patch.object(BibleImport, 'parse_xml', return_value=None), \
+                patch.object(BibleImport, 'is_compressed', return_value=False):
+            importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling validate_xml_file
+            # THEN: ValidationError should be raised, with the message 'Error when opening file'
+            #       the user that an OpenSong bible was found
+            with self.assertRaises(ValidationError) as context:
+                importer.validate_xml_file('file.name', 'xbible')
+            self.assertEqual(context.exception.msg, 'Error when opening file')
+
+    def validate_xml_file_success_test(self):
+        """
+        Test that validate_xml_file returns True with valid XML
+        """
+        # GIVEN: Some test data with an OpenSong Bible "bible" root tag
+        with patch.object(BibleImport, 'parse_xml', return_value=objectify.fromstring('<bible></bible>')), \
+                patch.object(BibleImport, 'is_compressed', return_value=False):
+            importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling validate_xml_file
+            result = importer.validate_xml_file('file.name', 'bible')
+
+            # THEN: True should be returned
+            self.assertTrue(result)
+
+    def validate_xml_file_opensong_root_test(self):
+        """
+        Test that validate_xml_file raises a ValidationError with an OpenSong root tag
+        """
+        # GIVEN: Some test data with an Zefania root tag and an instance of BibleImport
+        with patch.object(BibleImport, 'parse_xml', return_value=objectify.fromstring('<bible></bible>')), \
+                patch.object(BibleImport, 'is_compressed', return_value=False):
+            importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling validate_xml_file
+            # THEN: ValidationError should be raised, and the critical error message box should was called informing
+            #       the user that an OpenSong bible was found
+            with self.assertRaises(ValidationError) as context:
+                importer.validate_xml_file('file.name', 'xbible')
+            self.assertEqual(context.exception.msg, 'Invalid xml.')
+            self.mocked_critical_error_message_box.assert_called_once_with(
+                message='Incorrect Bible file type supplied. This looks like an OpenSong XML bible.')
+
+    def validate_xml_file_osis_root_test(self):
+        """
+        Test that validate_xml_file raises a ValidationError with an OSIS root tag
+        """
+        # GIVEN: Some test data with an Zefania root tag and an instance of BibleImport
+        with patch.object(BibleImport, 'parse_xml', return_value=objectify.fromstring(
+                '<osis xmlns=\'http://www.bibletechnologies.net/2003/OSIS/namespace\'></osis>')), \
+                patch.object(BibleImport, 'is_compressed', return_value=False):
+            importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling validate_xml_file
+            # THEN: ValidationError should be raised, and the critical error message box should was called informing
+            #       the user that an OSIS bible was found
+            with self.assertRaises(ValidationError) as context:
+                importer.validate_xml_file('file.name', 'xbible')
+            self.assertEqual(context.exception.msg, 'Invalid xml.')
+            self.mocked_critical_error_message_box.assert_called_once_with(
+                message='Incorrect Bible file type supplied. This looks like an OSIS XML bible.')
+
+    def validate_xml_file_zefania_root_test(self):
+        """
+        Test that validate_xml_file raises a ValidationError with an Zefania root tag
+        """
+        # GIVEN: Some test data with an Zefania root tag and an instance of BibleImport
+        with patch.object(BibleImport, 'parse_xml', return_value=objectify.fromstring('<xmlbible></xmlbible>')), \
+                patch.object(BibleImport, 'is_compressed', return_value=False):
+            importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling validate_xml_file
+            # THEN: ValidationError should be raised, and the critical error message box should was called informing
+            #       the user that an Zefania bible was found
+            with self.assertRaises(ValidationError) as context:
+                importer.validate_xml_file('file.name', 'xbible')
+            self.assertEqual(context.exception.msg, 'Invalid xml.')
+            self.mocked_critical_error_message_box.assert_called_once_with(
+                message='Incorrect Bible file type supplied. This looks like an Zefania XML bible.')
+
+    def validate_xml_file_unknown_root_test(self):
+        """
+        Test that validate_xml_file raises a ValidationError with an unknown root tag
+        """
+        # GIVEN: Some test data with an unknown root tag and an instance of BibleImport
+        with patch.object(
+                BibleImport, 'parse_xml', return_value=objectify.fromstring('<unknownbible></unknownbible>')), \
+                patch.object(BibleImport, 'is_compressed', return_value=False):
+            importer = BibleImport(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling validate_xml_file
+            # THEN: ValidationError should be raised, and the critical error message box should was called informing
+            #       the user that a unknown xml bible was found
+            with self.assertRaises(ValidationError) as context:
+                importer.validate_xml_file('file.name', 'xbible')
+            self.assertEqual(context.exception.msg, 'Invalid xml.')
+            self.mocked_critical_error_message_box.assert_called_once_with(
+                message='Incorrect Bible file type supplied. This looks like an unknown type of XML bible.')

tests/functional/openlp_plugins/bibles/test_csvimport.py

@@ -46,10 +46,10 @@ class TestCSVImport(TestCase):
 
     def setUp(self):
         self.manager_patcher = patch('openlp.plugins.bibles.lib.db.Manager')
-        self.registry_patcher = patch('openlp.plugins.bibles.lib.db.Registry')
         self.addCleanup(self.manager_patcher.stop)
-        self.addCleanup(self.registry_patcher.stop)
         self.manager_patcher.start()
+        self.registry_patcher = patch('openlp.plugins.bibles.lib.bibleimport.Registry')
+        self.addCleanup(self.registry_patcher.stop)
         self.registry_patcher.start()
 
     def test_create_importer(self):
@@ -194,9 +194,9 @@ class TestCSVImport(TestCase):
             # WHEN: Calling process_books
             result = importer.process_books(['Book 1'])
 
-            # THEN: increment_progress_bar should not be called and the return value should be None
+            # THEN: increment_progress_bar should not be called and the return value should be an empty dictionary
             self.assertFalse(importer.wizard.increment_progress_bar.called)
-            self.assertIsNone(result)
+            self.assertEqual(result, {})
 
     def process_books_test(self):
         """
@@ -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,19 +231,16 @@ 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
             importer.wizard = MagicMock()
 
             # WHEN: Calling process_verses
-            result = importer.process_verses([], [])
+            result = importer.process_verses(['Dummy Verse'], [])
 
             # 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 +252,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 +274,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):
         """
@@ -288,82 +281,24 @@ class TestCSVImport(TestCase):
         """
         # GIVEN: An instance of CSVBible and a mocked get_language which simulates the user cancelling the language box
         mocked_manager = MagicMock()
-        with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'),\
-                patch('openlp.plugins.bibles.lib.importers.csvbible.log') as mocked_log:
+        with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'):
             importer = CSVBible(mocked_manager, path='.', name='.', booksfile='books.csv', versefile='verse.csv')
             importer.get_language = MagicMock(return_value=None)
 
             # WHEN: Calling do_import
             result = importer.do_import('Bible Name')
 
-            # THEN: The log.exception method should have been called to show that it reached the except clause.
-            # False should be returned.
+            # THEN: The 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):
-        """
-        Test do_import when the import is stopped
-        """
-        # GIVEN: An instance of CSVBible with stop_import set to True
-        mocked_manager = MagicMock()
-        with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'),\
-                patch('openlp.plugins.bibles.lib.importers.csvbible.log') as mocked_log:
-            importer = CSVBible(mocked_manager, path='.', name='.', booksfile='books.csv', versefile='verse.csv')
-            importer.get_language = MagicMock(return_value=10)
-            importer.parse_csv_file = MagicMock(return_value=['Book 1', 'Book 2', 'Book 3'])
-            importer.process_books = MagicMock()
-            importer.stop_import_flag = True
-            importer.wizard = MagicMock()
-
-            # WHEN: Calling do_import
-            result = importer.do_import('Bible Name')
-
-            # THEN: log.exception should not be called, parse_csv_file should only be called once,
-            # and False should be returned.
-            self.assertFalse(mocked_log.exception.called)
-            importer.parse_csv_file.assert_called_once_with('books.csv', Book)
-            importer.process_books.assert_called_once_with(['Book 1', 'Book 2', 'Book 3'])
-            self.assertFalse(result)
-
-    def do_import_stop_import_2_test(self):
-        """
-        Test do_import when the import is stopped
-        """
-        # GIVEN: An instance of CSVBible with stop_import which is True the second time of calling
-        mocked_manager = MagicMock()
-        with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'),\
-                patch('openlp.plugins.bibles.lib.importers.csvbible.log') as mocked_log:
-            CSVBible.stop_import_flag = PropertyMock(side_effect=[False, True])
-            importer = CSVBible(mocked_manager, path='.', name='.', booksfile='books.csv', versefile='verses.csv')
-            importer.get_language = MagicMock(return_value=10)
-            importer.parse_csv_file = MagicMock(side_effect=[['Book 1'], ['Verse 1']])
-            importer.process_books = MagicMock(return_value=['Book 1'])
-            importer.process_verses = MagicMock(return_value=['Verse 1'])
-            importer.wizard = MagicMock()
-
-            # WHEN: Calling do_import
-            result = importer.do_import('Bible Name')
-
-            # THEN: log.exception should not be called, parse_csv_file should be called twice,
-            # and False should be returned.
-            self.assertFalse(mocked_log.exception.called)
-            self.assertEqual(importer.parse_csv_file.mock_calls, [call('books.csv', Book), call('verses.csv', Verse)])
-            importer.process_verses.assert_called_once_with(['Verse 1'], ['Book 1'])
-            self.assertFalse(result)
-
-            # Cleanup
-            del CSVBible.stop_import_flag
-
     def do_import_success_test(self):
         """
         Test do_import when the import succeeds
         """
         # GIVEN: An instance of CSVBible
         mocked_manager = MagicMock()
-        with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'),\
-                patch('openlp.plugins.bibles.lib.importers.csvbible.log') as mocked_log:
+        with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'):
             importer = CSVBible(mocked_manager, path='.', name='.', booksfile='books.csv', versefile='verses.csv')
             importer.get_language = MagicMock(return_value=10)
             importer.parse_csv_file = MagicMock(side_effect=[['Book 1'], ['Verse 1']])
@@ -376,9 +311,8 @@ class TestCSVImport(TestCase):
             # WHEN: Calling do_import
             result = importer.do_import('Bible Name')
 
-            # THEN: log.exception should not be called, parse_csv_file should be called twice,
+            # THEN: parse_csv_file should be called twice,
             # and True should be returned.
-            self.assertFalse(mocked_log.exception.called)
             self.assertEqual(importer.parse_csv_file.mock_calls, [call('books.csv', Book), call('verses.csv', Verse)])
             importer.process_books.assert_called_once_with(['Book 1'])
             importer.process_verses.assert_called_once_with(['Verse 1'], ['Book 1'])
@@ -413,6 +347,6 @@ class TestCSVImport(TestCase):
             # THEN: The create_verse() method should have been called with each verse in the file.
             self.assertTrue(importer.create_verse.called)
             for verse_tag, verse_text in test_data['verses']:
-                importer.create_verse.assert_any_call(importer.get_book().id, '1', verse_tag, verse_text)
+                importer.create_verse.assert_any_call(importer.get_book().id, 1, verse_tag, verse_text)
             importer.create_book.assert_any_call('1. Mosebog', importer.get_book_ref_id_by_name(), 1)
             importer.create_book.assert_any_call('1. Krønikebog', importer.get_book_ref_id_by_name(), 1)

tests/functional/openlp_plugins/bibles/test_db.py

@@ -25,63 +25,9 @@ This module contains tests for the db submodule of the Bibles plugin.
 
 from unittest import TestCase
 
-from openlp.plugins.bibles.lib.db import BibleDB
-from tests.functional import MagicMock, patch
-
 
 class TestBibleDB(TestCase):
     """
     Test the functions in the BibleDB class.
     """
-
-    def test_get_language_canceled(self):
-        """
-        Test the BibleDB.get_language method when the user rejects the dialog box
-        """
-        # GIVEN: A mocked LanguageForm with an exec method which returns QtDialog.Rejected and an instance of BibleDB
-        with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'),\
-                patch('openlp.plugins.bibles.forms.LanguageForm') as mocked_language_form:
-
-            # The integer value of QtDialog.Rejected is 0. Using the enumeration causes a seg fault for some reason
-            mocked_language_form_instance = MagicMock(**{'exec.return_value': 0})
-            mocked_language_form.return_value = mocked_language_form_instance
-            mocked_parent = MagicMock()
-            instance = BibleDB(mocked_parent)
-            mocked_wizard = MagicMock()
-            instance.wizard = mocked_wizard
-
-            # WHEN: Calling get_language()
-            result = instance.get_language()
-
-            # THEN: get_language() should return False
-            mocked_language_form.assert_called_once_with(mocked_wizard)
-            mocked_language_form_instance.exec.assert_called_once_with(None)
-            self.assertFalse(result, 'get_language() should return False if the user rejects the dialog box')
-
-    def test_get_language_accepted(self):
-        """
-        Test the BibleDB.get_language method when the user accepts the dialog box
-        """
-        # GIVEN: A mocked LanguageForm with an exec method which returns QtDialog.Accepted an instance of BibleDB and
-        #       a combobox with the selected item data as 10
-        with patch('openlp.plugins.bibles.lib.db.BibleDB._setup'), \
-                patch('openlp.plugins.bibles.lib.db.BibleDB.save_meta'), \
-                patch('openlp.plugins.bibles.forms.LanguageForm') as mocked_language_form:
-
-            # The integer value of QtDialog.Accepted is 1. Using the enumeration causes a seg fault for some reason
-            mocked_language_form_instance = MagicMock(**{'exec.return_value': 1,
-                                                         'language_combo_box.itemData.return_value': 10})
-            mocked_language_form.return_value = mocked_language_form_instance
-            mocked_parent = MagicMock()
-            instance = BibleDB(mocked_parent)
-            mocked_wizard = MagicMock()
-            instance.wizard = mocked_wizard
-
-            # WHEN: Calling get_language()
-            result = instance.get_language('Bible Name')
-
-            # THEN: get_language() should return the id of the selected language in the combo box
-            mocked_language_form.assert_called_once_with(mocked_wizard)
-            mocked_language_form_instance.exec.assert_called_once_with('Bible Name')
-            self.assertEqual(result, 10, 'get_language() should return the id of the language the user has chosen when '
-                                         'they accept the dialog box')
+    pass

tests/functional/openlp_plugins/bibles/test_opensongimport.py

@@ -23,32 +23,38 @@
 This module contains tests for the OpenSong Bible importer.
 """
 
-import os
 import json
+import os
 from unittest import TestCase
 
-from tests.functional import MagicMock, patch
-from openlp.plugins.bibles.lib.importers.opensong import OpenSongBible
-from openlp.plugins.bibles.lib.db import BibleDB
+from lxml import objectify
+
+from tests.functional import MagicMock, patch, call
+from tests.helpers.testmixin import TestMixin
+from openlp.core.common import Registry
+from openlp.plugins.bibles.lib.importers.opensong import OpenSongBible, get_text, parse_chapter_number
+from openlp.plugins.bibles.lib.bibleimport import BibleImport
 
 TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__),
                                          '..', '..', '..', 'resources', 'bibles'))
 
 
-class TestOpenSongImport(TestCase):
+class TestOpenSongImport(TestCase, TestMixin):
     """
     Test the functions in the :mod:`opensongimport` module.
     """
 
     def setUp(self):
-        self.registry_patcher = patch('openlp.plugins.bibles.lib.db.Registry')
-        self.registry_patcher.start()
+        self.find_and_create_book_patch = patch.object(BibleImport, 'find_and_create_book')
+        self.addCleanup(self.find_and_create_book_patch.stop)
+        self.mocked_find_and_create_book = self.find_and_create_book_patch.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()
+        self.setup_application()
+        self.app.process_events = MagicMock()
+        Registry.create()
+        Registry().register('application', self.app)
 
     def test_create_importer(self):
         """
@@ -61,7 +67,332 @@ class TestOpenSongImport(TestCase):
         importer = OpenSongBible(mocked_manager, path='.', name='.', filename='')
 
         # THEN: The importer should be an instance of BibleDB
-        self.assertIsInstance(importer, BibleDB)
+        self.assertIsInstance(importer, BibleImport)
+
+    def get_text_no_text_test(self):
+        """
+        Test that get_text handles elements containing text in a combination of text and tail attributes
+        """
+        # GIVEN: Some test data which contains an empty element and an instance of OpenSongBible
+        test_data = objectify.fromstring('<element></element>')
+
+        # WHEN: Calling get_text
+        result = get_text(test_data)
+
+        # THEN: A blank string should be returned
+        self.assertEqual(result, '')
+
+    def get_text_text_test(self):
+        """
+        Test that get_text handles elements containing text in a combination of text and tail attributes
+        """
+        # GIVEN: Some test data which contains all possible permutation of text and tail text possible and an instance
+        #        of OpenSongBible
+        test_data = objectify.fromstring('<element>Element text '
+                                         '<sub_text_tail>sub_text_tail text </sub_text_tail>sub_text_tail tail '
+                                         '<sub_text>sub_text text </sub_text>'
+                                         '<sub_tail></sub_tail>sub_tail tail</element>')
+
+        # WHEN: Calling get_text
+        result = get_text(test_data)
+
+        # THEN: The text returned should be as expected
+        self.assertEqual(result, 'Element text sub_text_tail text sub_text_tail tail sub_text text sub_tail tail')
+
+    def parse_chapter_number_test(self):
+        """
+        Test parse_chapter_number when supplied with chapter number and an instance of OpenSongBible
+        """
+        # GIVEN: The number 10 represented as a string
+        # WHEN: Calling parse_chapter_nnumber
+        result = parse_chapter_number('10', 0)
+
+        # THEN: The 10 should be returned as an Int
+        self.assertEqual(result, 10)
+
+    def parse_chapter_number_empty_attribute_test(self):
+        """
+        Testparse_chapter_number when the chapter number is an empty string. (Bug #1074727)
+        """
+        # GIVEN: An empty string, and the previous chapter number set as 12  and an instance of OpenSongBible
+        # WHEN: Calling parse_chapter_number
+        result = parse_chapter_number('', 12)
+
+        # THEN: parse_chapter_number should increment the previous verse number
+        self.assertEqual(result, 13)
+
+    def parse_verse_number_valid_verse_no_test(self):
+        """
+        Test parse_verse_number when supplied with a valid verse number
+        """
+        # GIVEN: An instance of OpenSongBible, the number 15 represented as a string and an instance of OpenSongBible
+        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: Calling parse_verse_number
+        result = importer.parse_verse_number('15', 0)
+
+        # THEN: parse_verse_number should return the verse number
+        self.assertEqual(result, 15)
+
+    def parse_verse_number_verse_range_test(self):
+        """
+        Test parse_verse_number when supplied with a verse range
+        """
+        # GIVEN: An instance of OpenSongBible, and the range 24-26 represented as a string
+        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: Calling parse_verse_number
+        result = importer.parse_verse_number('24-26', 0)
+
+        # THEN: parse_verse_number should return the first verse number in the range
+        self.assertEqual(result, 24)
+
+    def parse_verse_number_invalid_verse_no_test(self):
+        """
+        Test parse_verse_number when supplied with a invalid verse number
+        """
+        # GIVEN: An instance of OpenSongBible, a non numeric string represented as a string
+        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: Calling parse_verse_number
+        result = importer.parse_verse_number('invalid', 41)
+
+        # THEN: parse_verse_number should increment the previous verse number
+        self.assertEqual(result, 42)
+
+    def parse_verse_number_empty_attribute_test(self):
+        """
+        Test parse_verse_number when the verse number is an empty string. (Bug #1074727)
+        """
+        # GIVEN: An instance of OpenSongBible, an empty string, and the previous verse number set as 14
+        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+        # WHEN: Calling parse_verse_number
+        result = importer.parse_verse_number('', 14)
+
+        # THEN: parse_verse_number should increment the previous verse number
+        self.assertEqual(result, 15)
+
+    def parse_verse_number_invalid_type_test(self):
+        """
+        Test parse_verse_number when the verse number is an invalid type)
+        """
+        with patch.object(OpenSongBible, 'log_warning')as mocked_log_warning:
+            # GIVEN: An instanceofOpenSongBible, a Tuple, and the previous verse number set as 12
+            importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling parse_verse_number
+            result = importer.parse_verse_number((1, 2, 3), 12)
+
+            # THEN: parse_verse_number should log the verse number it was called with increment the previous verse
+            #       number
+            mocked_log_warning.assert_called_once_with('Illegal verse number: (1, 2, 3)')
+            self.assertEqual(result, 13)
+
+    def process_books_stop_import_test(self):
+        """
+        Test process_books when stop_import is set to True
+        """
+        # GIVEN: An instance of OpenSongBible
+        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: stop_import_flag is set to True
+        importer.stop_import_flag = True
+        importer.process_books(['Book'])
+
+        # THEN: find_and_create_book should not have been called
+        self.assertFalse(self.mocked_find_and_create_book.called)
+
+    def process_books_completes_test(self):
+        """
+        Test process_books when it processes all books
+        """
+        # GIVEN: An instance of OpenSongBible Importer and two mocked books
+        self.mocked_find_and_create_book.side_effect = ['db_book1', 'db_book2']
+        with patch.object(OpenSongBible, 'process_chapters') as mocked_process_chapters:
+            importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+
+            book1 = MagicMock()
+            book1.attrib = {'n': 'Name1'}
+            book1.c = 'Chapter1'
+            book2 = MagicMock()
+            book2.attrib = {'n': 'Name2'}
+            book2.c = 'Chapter2'
+            importer.language_id = 10
+            importer.session = MagicMock()
+            importer.stop_import_flag = False
+
+            # WHEN: Calling process_books with the two books
+            importer.process_books([book1, book2])
+
+            # THEN: find_and_create_book and process_books should be called with the details from the mocked books
+            self.assertEqual(self.mocked_find_and_create_book.call_args_list,
+                             [call('Name1', 2, 10), call('Name2', 2, 10)])
+            self.assertEqual(mocked_process_chapters.call_args_list,
+                             [call('db_book1', 'Chapter1'), call('db_book2', 'Chapter2')])
+            self.assertEqual(importer.session.commit.call_count, 2)
+
+    def process_chapters_stop_import_test(self):
+        """
+        Test process_chapters when stop_import is set to True
+        """
+        # GIVEN: An isntance of OpenSongBible
+        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+        importer.parse_chapter_number = MagicMock()
+
+        # WHEN: stop_import_flag is set to True
+        importer.stop_import_flag = True
+        importer.process_chapters('Book', ['Chapter1'])
+
+        # THEN: importer.parse_chapter_number not have been called
+        self.assertFalse(importer.parse_chapter_number.called)
+
+    @patch('openlp.plugins.bibles.lib.importers.opensong.parse_chapter_number', **{'side_effect': [1, 2]})
+    def process_chapters_completes_test(self, mocked_parse_chapter_number):
+        """
+        Test process_chapters when it completes
+        """
+        # GIVEN: An instance of OpenSongBible
+        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+        importer.wizard = MagicMock()
+
+        # WHEN: called with some valid data
+        book = MagicMock()
+        book.name = "Book"
+        chapter1 = MagicMock()
+        chapter1.attrib = {'n': '1'}
+        chapter1.c = 'Chapter1'
+        chapter1.v = ['Chapter1 Verses']
+        chapter2 = MagicMock()
+        chapter2.attrib = {'n': '2'}
+        chapter2.c = 'Chapter2'
+        chapter2.v = ['Chapter2 Verses']
+
+        importer.process_verses = MagicMock()
+        importer.stop_import_flag = False
+        importer.process_chapters(book, [chapter1, chapter2])
+
+        # THEN: parse_chapter_number, process_verses and increment_process_bar should have been called
+        self.assertEqual(mocked_parse_chapter_number.call_args_list, [call('1', 0), call('2', 1)])
+        self.assertEqual(
+            importer.process_verses.call_args_list,
+            [call(book, 1, ['Chapter1 Verses']), call(book, 2, ['Chapter2 Verses'])])
+        self.assertEqual(importer.wizard.increment_progress_bar.call_args_list,
+                         [call('Importing Book 1...'), call('Importing Book 2...')])
+
+    def process_verses_stop_import_test(self):
+        """
+        Test process_verses when stop_import is set to True
+        """
+        # GIVEN: An isntance of OpenSongBible
+        importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+        importer.parse_verse_number = MagicMock()
+
+        # WHEN: stop_import_flag is set to True
+        importer.stop_import_flag = True
+        importer.process_verses('Book', 1, 'Verses')
+
+        # THEN: importer.parse_verse_number not have been called
+        self.assertFalse(importer.parse_verse_number.called)
+
+    def process_verses_completes_test(self):
+        """
+        Test process_verses when it completes
+        """
+        with patch('openlp.plugins.bibles.lib.importers.opensong.get_text',
+                   **{'side_effect': ['Verse1 Text', 'Verse2 Text']}) as mocked_get_text, \
+                patch.object(OpenSongBible, 'parse_verse_number',
+                             **{'side_effect': [1, 2]}) as mocked_parse_verse_number:
+            # GIVEN: An instance of OpenSongBible
+            importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
+            importer.wizard = MagicMock()
+
+            # WHEN: called with some valid data
+            book = MagicMock()
+            book.id = 1
+            verse1 = MagicMock()
+            verse1.attrib = {'n': '1'}
+            verse1.c = 'Chapter1'
+            verse1.v = ['Chapter1 Verses']
+            verse2 = MagicMock()
+            verse2.attrib = {'n': '2'}
+            verse2.c = 'Chapter2'
+            verse2.v = ['Chapter2 Verses']
+
+            importer.create_verse = MagicMock()
+            importer.stop_import_flag = False
+            importer.process_verses(book, 1, [verse1, verse2])
+
+            # THEN: parse_chapter_number, process_verses and increment_process_bar should have been called
+            self.assertEqual(mocked_parse_verse_number.call_args_list, [call('1', 0), call('2', 1)])
+            self.assertEqual(mocked_get_text.call_args_list, [call(verse1), call(verse2)])
+            self.assertEqual(
+                importer.create_verse.call_args_list,
+                [call(1, 1, 1, 'Verse1 Text'), call(1, 1, 2, 'Verse2 Text')])
+
+    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.object(OpenSongBible, 'log_debug'), \
+                patch.object(OpenSongBible, 'validate_xml_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='')
+
+            # 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.object(OpenSongBible, 'log_debug'), \
+                patch.object(OpenSongBible, 'validate_xml_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='')
+
+            # 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_completes_test(self):
+        """
+        Test do_import when it completes successfully
+        """
+        # GIVEN: An instance of OpenSongBible
+        with patch.object(OpenSongBible, 'log_debug'), \
+                patch.object(OpenSongBible, 'validate_xml_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='')
+
+            # WHEN: Calling do_import
+            result = importer.do_import()
+
+            # THEN: do_import should return True
+            self.assertTrue(result)
+
+
+class TestOpenSongImportFileImports(TestCase, TestMixin):
+    """
+    Test the functions in the :mod:`opensongimport` module.
+    """
+    def setUp(self):
+        self.manager_patcher = patch('openlp.plugins.bibles.lib.db.Manager')
+        self.addCleanup(self.manager_patcher.stop)
+        self.manager_patcher.start()
 
     def test_file_import(self):
         """
@@ -92,22 +423,3 @@ class TestOpenSongImport(TestCase):
             self.assertTrue(importer.create_verse.called)
             for verse_tag, verse_text in test_data['verses']:
                 importer.create_verse.assert_any_call(importer.create_book().id, 1, int(verse_tag), verse_text)
-
-    def test_zefania_import_error(self):
-        """
-        Test that we give an error message if trying to import a zefania bible
-        """
-        # GIVEN: A mocked out "manager" and mocked out critical_error_message_box and an import
-        with patch('openlp.plugins.bibles.lib.importers.opensong.critical_error_message_box') as \
-                mocked_critical_error_message_box:
-            mocked_manager = MagicMock()
-            importer = OpenSongBible(mocked_manager, path='.', name='.', filename='')
-
-            # WHEN: An trying to import a zefania bible
-            importer.filename = os.path.join(TEST_PATH, 'zefania-dk1933.xml')
-            importer.do_import()
-
-            # THEN: The importer should have "shown" an error message
-            mocked_critical_error_message_box.assert_called_with(message='Incorrect Bible file type supplied. '
-                                                                 'This looks like a Zefania XML bible, '
-                                                                 'please use the Zefania import option.')

tests/functional/openlp_plugins/bibles/test_osisimport.py

@@ -27,29 +27,35 @@ import os
 import json
 from unittest import TestCase
 
-from tests.functional import MagicMock, patch
-from openlp.plugins.bibles.lib.importers.osis import OSISBible
+from tests.functional import MagicMock, call, patch
+from openlp.plugins.bibles.lib.bibleimport import BibleImport
 from openlp.plugins.bibles.lib.db import BibleDB
+from openlp.plugins.bibles.lib.importers.osis import OSISBible
 
-TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__),
-                                         '..', '..', '..', 'resources', 'bibles'))
+TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', '..', 'resources', 'bibles'))
 
 
 class TestOsisImport(TestCase):
     """
     Test the functions in the :mod:`osisimport` module.
     """
-
     def setUp(self):
-        self.registry_patcher = patch('openlp.plugins.bibles.lib.db.Registry')
+        self.etree_patcher = patch('openlp.plugins.bibles.lib.importers.osis.etree')
+        self.addCleanup(self.etree_patcher.stop)
+        self.mocked_etree = self.etree_patcher.start()
+        self.create_verse_patcher = patch('openlp.plugins.bibles.lib.db.BibleDB.create_verse')
+        self.addCleanup(self.create_verse_patcher.stop)
+        self.mocked_create_verse = self.create_verse_patcher.start()
+        self.find_and_create_book_patch = patch.object(BibleImport, 'find_and_create_book')
+        self.addCleanup(self.find_and_create_book_patch.stop)
+        self.mocked_find_and_create_book = self.find_and_create_book_patch.start()
+        self.registry_patcher = patch('openlp.plugins.bibles.lib.bibleimport.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 +69,353 @@ class TestOsisImport(TestCase):
         # THEN: The importer should be an instance of BibleDB
         self.assertIsInstance(importer, BibleDB)
 
+    def process_books_stop_import_test(self):
+        """
+        Test process_books when stop_import is set to True
+        """
+        # GIVEN: An instance of OSISBible adn some mocked data
+        importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+        mocked_data = MagicMock(**{'xpath.return_value': ['Book']})
+
+        # WHEN: stop_import_flag is set to True and process_books is called
+        importer.stop_import_flag = True
+        importer.process_books(mocked_data)
+
+        # THEN: find_and_create_book should not have been called
+        self.assertFalse(self.mocked_find_and_create_book.called)
+
+    def process_books_completes_test(self):
+        """
+        Test process_books when it processes all books
+        """
+        # GIVEN: An instance of OSISBible Importer and two mocked books
+        self.mocked_find_and_create_book.side_effect = ['db_book1', 'db_book2']
+        with patch.object(OSISBible, 'process_chapters') as mocked_process_chapters:
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+            book1 = MagicMock()
+            book1.get.return_value = 'Name1'
+            book2 = MagicMock()
+            book2.get.return_value = 'Name2'
+            mocked_data = MagicMock(**{'xpath.return_value': [book1, book2]})
+            importer.language_id = 10
+            importer.session = MagicMock()
+            importer.stop_import_flag = False
+
+            # WHEN: Calling process_books with the two books
+            importer.process_books(mocked_data)
+
+            # THEN: find_and_create_book and process_books should be called with the details from the mocked books
+            self.assertEqual(self.mocked_find_and_create_book.call_args_list,
+                             [call('Name1', 2, 10), call('Name2', 2, 10)])
+            self.assertEqual(mocked_process_chapters.call_args_list,
+                             [call('db_book1', book1), call('db_book2', book2)])
+            self.assertEqual(importer.session.commit.call_count, 2)
+
+    def process_chapters_verse_in_chapter_verse_text_test(self):
+        """
+        Test process_chapters when supplied with an etree element with a verse element nested in it
+        """
+        with patch('openlp.plugins.bibles.lib.importers.osis.verse_in_chapter', return_value=True), \
+                patch('openlp.plugins.bibles.lib.importers.osis.text_in_verse', return_value=True), \
+                patch.object(OSISBible, 'set_current_chapter') as mocked_set_current_chapter, \
+                patch.object(OSISBible, 'process_verse') as mocked_process_verse:
+
+            # GIVEN: Some test data and an instance of OSISBible
+            test_book = MagicMock()
+            test_verse = MagicMock()
+            test_verse.tail = '\n    '  # Whitespace
+            test_verse.text = 'Verse Text'
+            test_chapter = MagicMock()
+            test_chapter.__iter__.return_value = [test_verse]
+            test_chapter.get.side_effect = lambda x: {'osisID': '1.2.4', 'sID': '999'}.get(x)
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling process_chapters
+            importer.process_chapters(test_book, [test_chapter])
+
+            # THEN: set_current_chapter and process_verse should have been called with the test data
+            mocked_set_current_chapter.assert_called_once_with(test_book.name, 2)
+            mocked_process_verse.assert_called_once_with(test_book, 2, test_verse)
+
+    def process_chapters_verse_in_chapter_verse_milestone_test(self):
+        """
+        Test process_chapters when supplied with an etree element with a verse element nested, when the verse system is
+        based on milestones
+        """
+        with patch('openlp.plugins.bibles.lib.importers.osis.verse_in_chapter', return_value=True), \
+                patch('openlp.plugins.bibles.lib.importers.osis.text_in_verse', return_value=False), \
+                patch.object(OSISBible, 'set_current_chapter') as mocked_set_current_chapter, \
+                patch.object(OSISBible, 'process_verse') as mocked_process_verse:
+
+            # GIVEN: Some test data and an instance of OSISBible
+            test_book = MagicMock()
+            test_verse = MagicMock()
+            test_verse.tail = '\n    '  # Whitespace
+            test_verse.text = 'Verse Text'
+            test_chapter = MagicMock()
+            test_chapter.__iter__.return_value = [test_verse]
+            test_chapter.get.side_effect = lambda x: {'osisID': '1.2.4', 'sID': '999'}.get(x)
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling process_chapters
+            importer.process_chapters(test_book, [test_chapter])
+
+            # THEN: set_current_chapter and process_verse should have been called with the test data
+            mocked_set_current_chapter.assert_called_once_with(test_book.name, 2)
+            mocked_process_verse.assert_called_once_with(test_book, 2, test_verse, use_milestones=True)
+
+    def process_chapters_milestones_chapter_no_sid_test(self):
+        """
+        Test process_chapters when supplied with an etree element with a chapter and verse element in the milestone
+        configuration, where the chapter is the "closing" milestone. (Missing the sID attribute)
+        """
+        with patch('openlp.plugins.bibles.lib.importers.osis.verse_in_chapter', return_value=False), \
+                patch.object(OSISBible, 'set_current_chapter') as mocked_set_current_chapter, \
+                patch.object(OSISBible, 'process_verse') as mocked_process_verse:
+
+            # GIVEN: Some test data and an instance of OSISBible
+            test_book = MagicMock()
+            test_chapter = MagicMock()
+            test_chapter.tag = '{http://www.bibletechnologies.net/2003/OSIS/namespace}chapter'
+            test_chapter.get.side_effect = lambda x: {'osisID': '1.2.4'}.get(x)
+
+            # WHEN: Calling process_chapters
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+            importer.process_chapters(test_book, [test_chapter])
+
+            # THEN: neither set_current_chapter or process_verse should have been called
+            self.assertFalse(mocked_set_current_chapter.called)
+            self.assertFalse(mocked_process_verse.called)
+
+    def process_chapters_milestones_chapter_sid_test(self):
+        """
+        Test process_chapters when supplied with an etree element with a chapter and verse element in the milestone
+        configuration, where the chapter is the "opening" milestone. (Has the sID attribute)
+        """
+        with patch('openlp.plugins.bibles.lib.importers.osis.verse_in_chapter', return_value=False), \
+                patch.object(OSISBible, 'set_current_chapter') as mocked_set_current_chapter, \
+                patch.object(OSISBible, 'process_verse') as mocked_process_verse:
+
+            # GIVEN: Some test data and an instance of OSISBible
+            test_book = MagicMock()
+            test_chapter = MagicMock()
+            test_chapter.tag = '{http://www.bibletechnologies.net/2003/OSIS/namespace}chapter'
+            test_chapter.get.side_effect = lambda x: {'osisID': '1.2.4', 'sID': '999'}.get(x)
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+            # WHEN: Calling process_chapters
+            importer.process_chapters(test_book, [test_chapter])
+
+            # THEN: set_current_chapter should have been called with the test data
+            mocked_set_current_chapter.assert_called_once_with(test_book.name, 2)
+            self.assertFalse(mocked_process_verse.called)
+
+    def process_chapters_milestones_verse_tag_test(self):
+        """
+        Test process_chapters when supplied with an etree element with a chapter and verse element in the milestone
+        configuration, where the verse is the "opening" milestone. (Has the sID attribute)
+        """
+        with patch('openlp.plugins.bibles.lib.importers.osis.verse_in_chapter', return_value=False), \
+                patch.object(OSISBible, 'set_current_chapter') as mocked_set_current_chapter, \
+                patch.object(OSISBible, 'process_verse') as mocked_process_verse:
+
+            # GIVEN: Some test data and an instance of OSISBible
+            test_book = MagicMock()
+            test_verse = MagicMock()
+            test_verse.get.side_effect = lambda x: {'osisID': '1.2.4', 'sID': '999'}.get(x)
+            test_verse.tag = '{http://www.bibletechnologies.net/2003/OSIS/namespace}verse'
+            test_verse.tail = '\n    '  # Whitespace
+            test_verse.text = 'Verse Text'
+
+            # WHEN: Calling process_chapters
+            importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+            importer.process_chapters(test_book, [test_verse])
+
+            # THEN: process_verse should have been called with the test data
+            self.assertFalse(mocked_set_current_chapter.called)
+            mocked_process_verse.assert_called_once_with(test_book, 0, test_verse, use_milestones=True)
+
+    def process_verse_no_osis_id_test(self):
+        """
+        Test process_verse when the element supplied does not have and osisID attribute
+        """
+        # GIVEN: An instance of OSISBible, and some mocked test data
+        test_book = MagicMock()
+        test_verse = MagicMock()
+        test_verse.get.side_effect = lambda x: {}.get(x)
+        test_verse.tail = 'Verse Text'
+        test_verse.text = None
+        importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: Calling process_verse with the test data
+        importer.process_verse(test_book, 2, test_verse)
+
+        # THEN: create_verse should not have been called
+        self.assertFalse(self.mocked_create_verse.called)
+
+    def process_verse_use_milestones_no_s_id_test(self):
+        """
+        Test process_verse when called with use_milestones set to True, but the element supplied does not have and sID
+        attribute
+        """
+        # GIVEN: An instance of OSISBible, and some mocked test data
+        test_book = MagicMock()
+        test_verse = MagicMock()
+        test_verse.get.side_effect = lambda x: {}.get(x)
+        test_verse.tail = 'Verse Text'
+        test_verse.text = None
+        importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: Calling process_verse with the test data
+        importer.process_verse(test_book, 2, test_verse)
+
+        # THEN: create_verse should not have been called
+        self.assertFalse(self.mocked_create_verse.called)
+
+    def process_verse_use_milestones_no_tail_test(self):
+        """
+        Test process_verse when called with use_milestones set to True, but the element supplied does not have a 'tail'
+        """
+        # GIVEN: An instance of OSISBible, and some mocked test data
+        test_book = MagicMock()
+        test_verse = MagicMock()
+        test_verse.tail = None
+        test_verse.text = None
+        test_verse.get.side_effect = lambda x: {'osisID': '1.2.4', 'sID': '999'}.get(x)
+        importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: Calling process_verse with the test data
+        importer.process_verse(test_book, 2, test_verse, use_milestones=True)
+
+        # THEN: create_verse should not have been called
+        self.assertFalse(self.mocked_create_verse.called)
+
+    def process_verse_use_milestones_success_test(self):
+        """
+        Test process_verse when called with use_milestones set to True, and the verse element successfully imports
+        """
+        # GIVEN: An instance of OSISBible, and some mocked test data
+        test_book = MagicMock()
+        test_book.id = 1
+        test_verse = MagicMock()
+        test_verse.tail = 'Verse Text'
+        test_verse.text = None
+        test_verse.get.side_effect = lambda x: {'osisID': '1.2.4', 'sID': '999'}.get(x)
+        importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: Calling process_verse with the test data
+        importer.process_verse(test_book, 2, test_verse, use_milestones=True)
+
+        # THEN: create_verse should have been called with the test data
+        self.mocked_create_verse.assert_called_once_with(1, 2, 4, 'Verse Text')
+
+    def process_verse_no_text_test(self):
+        """
+        Test process_verse when called with an empty verse element
+        """
+        # GIVEN: An instance of OSISBible, and some mocked test data
+        test_book = MagicMock()
+        test_book.id = 1
+        test_verse = MagicMock()
+        test_verse.tail = '\n    '  # Whitespace
+        test_verse.text = None
+        test_verse.get.side_effect = lambda x: {'osisID': '1.2.4', 'sID': '999'}.get(x)
+        importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: Calling process_verse with the test data
+        importer.process_verse(test_book, 2, test_verse)
+
+        # THEN: create_verse should not have been called
+        self.assertFalse(self.mocked_create_verse.called)
+
+    def process_verse_success_test(self):
+        """
+        Test process_verse when called with an element with text set
+        """
+        # GIVEN: An instance of OSISBible, and some mocked test data
+        test_book = MagicMock()
+        test_book.id = 1
+        test_verse = MagicMock()
+        test_verse.tail = '\n    '  # Whitespace
+        test_verse.text = 'Verse Text'
+        test_verse.get.side_effect = lambda x: {'osisID': '1.2.4', 'sID': '999'}.get(x)
+        importer = OSISBible(MagicMock(), path='.', name='.', filename='')
+
+        # WHEN: Calling process_verse with the test data
+        importer.process_verse(test_book, 2, test_verse)
+
+        # THEN: create_verse should have been called with the test data
+        self.mocked_create_verse.assert_called_once_with(1, 2, 4, 'Verse Text')
+
+    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.object(OSISBible, 'log_debug'), \
+                patch.object(OSISBible, 'validate_xml_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.object(OSISBible, 'log_debug'), \
+                patch.object(OSISBible, 'validate_xml_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_completes_test(self):
+        """
+        Test do_import when it completes successfully
+        """
+        # GIVEN: An instance of OpenSongBible
+        with patch.object(OSISBible, 'log_debug'), \
+                patch.object(OSISBible, 'validate_xml_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='')
+
+            # WHEN: Calling do_import
+            result = importer.do_import()
+
+            # THEN: do_import should return True
+            self.assertTrue(result)
+
+
+class TestOsisImportFileImports(TestCase):
+    """
+    Test the functions in the :mod:`osisimport` module.
+    """
+    def setUp(self):
+        self.registry_patcher = patch('openlp.plugins.bibles.lib.bibleimport.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 test_file_import_nested_tags(self):
         """
         Test the actual import of OSIS Bible file, with nested chapter and verse tags
@@ -91,7 +444,7 @@ class TestOsisImport(TestCase):
             # THEN: The create_verse() method should have been called with each verse in the file.
             self.assertTrue(importer.create_verse.called)
             for verse_tag, verse_text in test_data['verses']:
-                importer.create_verse.assert_any_call(importer.create_book().id, '1', verse_tag, verse_text)
+                importer.create_verse.assert_any_call(importer.create_book().id, 1, verse_tag, verse_text)
 
     def test_file_import_mixed_tags(self):
         """
@@ -121,7 +474,7 @@ class TestOsisImport(TestCase):
             # THEN: The create_verse() method should have been called with each verse in the file.
             self.assertTrue(importer.create_verse.called)
             for verse_tag, verse_text in test_data['verses']:
-                importer.create_verse.assert_any_call(importer.create_book().id, '1', verse_tag, verse_text)
+                    importer.create_verse.assert_any_call(importer.create_book().id, 1, verse_tag, verse_text)
 
     def test_file_import_milestone_tags(self):
         """
@@ -151,7 +504,7 @@ class TestOsisImport(TestCase):
             # THEN: The create_verse() method should have been called with each verse in the file.
             self.assertTrue(importer.create_verse.called)
             for verse_tag, verse_text in test_data['verses']:
-                importer.create_verse.assert_any_call(importer.create_book().id, '1', verse_tag, verse_text)
+                importer.create_verse.assert_any_call(importer.create_book().id, 1, verse_tag, verse_text)
 
     def test_file_import_empty_verse_tags(self):
         """
@@ -181,4 +534,4 @@ class TestOsisImport(TestCase):
             # THEN: The create_verse() method should have been called with each verse in the file.
             self.assertTrue(importer.create_verse.called)
             for verse_tag, verse_text in test_data['verses']:
-                importer.create_verse.assert_any_call(importer.create_book().id, '1', verse_tag, verse_text)
+                importer.create_verse.assert_any_call(importer.create_book().id, 1, verse_tag, verse_text)

tests/functional/openlp_plugins/bibles/test_swordimport.py

@@ -46,7 +46,7 @@ class TestSwordImport(TestCase):
     """
 
     def setUp(self):
-        self.registry_patcher = patch('openlp.plugins.bibles.lib.db.Registry')
+        self.registry_patcher = patch('openlp.plugins.bibles.lib.bibleimport.Registry')
         self.registry_patcher.start()
         self.manager_patcher = patch('openlp.plugins.bibles.lib.db.Manager')
         self.manager_patcher.start()

tests/functional/openlp_plugins/bibles/test_zefaniaimport.py

@@ -41,15 +41,13 @@ class TestZefaniaImport(TestCase):
     """
 
     def setUp(self):
-        self.registry_patcher = patch('openlp.plugins.bibles.lib.db.Registry')
+        self.registry_patcher = patch('openlp.plugins.bibles.lib.bibleimport.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 Zefania file importer
@@ -90,7 +88,7 @@ class TestZefaniaImport(TestCase):
             # THEN: The create_verse() method should have been called with each verse in the file.
             self.assertTrue(importer.create_verse.called)
             for verse_tag, verse_text in test_data['verses']:
-                importer.create_verse.assert_any_call(importer.create_book().id, '1', verse_tag, verse_text)
+                importer.create_verse.assert_any_call(importer.create_book().id, 1, verse_tag, verse_text)
             importer.create_book.assert_any_call('Genesis', 1, 1)
 
     def test_file_import_no_book_name(self):
@@ -120,5 +118,5 @@ class TestZefaniaImport(TestCase):
             # THEN: The create_verse() method should have been called with each verse in the file.
             self.assertTrue(importer.create_verse.called)
             for verse_tag, verse_text in test_data['verses']:
-                importer.create_verse.assert_any_call(importer.create_book().id, '1', verse_tag, verse_text)
+                importer.create_verse.assert_any_call(importer.create_book().id, 1, verse_tag, verse_text)
             importer.create_book.assert_any_call('Exodus', 2, 1)

tests/resources/bibles/dk1933.json

@@ -2,15 +2,15 @@
     "book": "Genesis",
     "chapter": 1,
     "verses": [
-        [ "1", "I Begyndelsen skabte Gud Himmelen og Jorden."],
-        [ "2", "Og Jorden var øde og tom, og der var Mørke over Verdensdybet. Men Guds Ånd svævede over Vandene." ],
-        [ "3", "Og Gud sagde: \"Der blive Lys!\" Og der blev Lys." ],
-        [ "4", "Og Gud så, at Lyset var godt, og Gud satte Skel mellem Lyset og Mørket," ],
-        [ "5", "og Gud kaldte Lyset Dag, og Mørket kaldte han Nat. Og det blev Aften, og det blev Morgen, første Dag." ],
-        [ "6", "Derpå sagde Gud: \"Der blive en Hvælving midt i Vandene til at skille Vandene ad!\"" ],
-        [ "7", "Og således skete det: Gud gjorde Hvælvingen og skilte Vandet under Hvælvingen fra Vandet over Hvælvingen;" ],
-        [ "8", "og Gud kaldte Hvælvingen Himmel. Og det blev Aften, og det blev Morgen, anden Dag." ],
-        [ "9", "Derpå sagde Gud: \"Vandet under Himmelen samle sig på eet Sted, så det faste Land kommer til Syne!\" Og således skete det;" ],
-        [ "10", "og Gud kaldte det faste Land Jord, og Stedet, hvor Vandet samlede sig, kaldte han Hav. Og Gud så, at det var godt." ]
+        [ 1, "I Begyndelsen skabte Gud Himmelen og Jorden."],
+        [ 2, "Og Jorden var øde og tom, og der var Mørke over Verdensdybet. Men Guds Ånd svævede over Vandene." ],
+        [ 3, "Og Gud sagde: \"Der blive Lys!\" Og der blev Lys." ],
+        [ 4, "Og Gud så, at Lyset var godt, og Gud satte Skel mellem Lyset og Mørket," ],
+        [ 5, "og Gud kaldte Lyset Dag, og Mørket kaldte han Nat. Og det blev Aften, og det blev Morgen, første Dag." ],
+        [ 6, "Derpå sagde Gud: \"Der blive en Hvælving midt i Vandene til at skille Vandene ad!\"" ],
+        [ 7, "Og således skete det: Gud gjorde Hvælvingen og skilte Vandet under Hvælvingen fra Vandet over Hvælvingen;" ],
+        [ 8, "og Gud kaldte Hvælvingen Himmel. Og det blev Aften, og det blev Morgen, anden Dag." ],
+        [ 9, "Derpå sagde Gud: \"Vandet under Himmelen samle sig på eet Sted, så det faste Land kommer til Syne!\" Og således skete det;" ],
+        [ 10, "og Gud kaldte det faste Land Jord, og Stedet, hvor Vandet samlede sig, kaldte han Hav. Og Gud så, at det var godt." ]
     ]
 }

tests/resources/bibles/kjv.json

@@ -2,15 +2,15 @@
     "book": "Genesis",
     "chapter": 1,
     "verses": [
-        [ "1", "In the beginning God created the heaven and the earth."],
-        [ "2", "And the earth was without form, and void; and darkness was upon the face of the deep. And the Spirit of God moved upon the face of the waters." ],
-        [ "3", "And God said, Let there be light: and there was light." ],
-        [ "4", "And God saw the light, that it was good: and God divided the light from the darkness." ],
-        [ "5", "And God called the light Day, and the darkness he called Night. And the evening and the morning were the first day." ],
-        [ "6", "And God said, Let there be a firmament in the midst of the waters, and let it divide the waters from the waters." ],
-        [ "7", "And God made the firmament, and divided the waters which were under the firmament from the waters which were above the firmament: and it was so." ],
-        [ "8", "And God called the firmament Heaven. And the evening and the morning were the second day." ],
-        [ "9", "And God said, Let the waters under the heaven be gathered together unto one place, and let the dry land appear: and it was so." ],
-        [ "10", "And God called the dry land Earth; and the gathering together of the waters called he Seas: and God saw that it was good." ]
+        [ 1, "In the beginning God created the heaven and the earth."],
+        [ 2, "And the earth was without form, and void; and darkness was upon the face of the deep. And the Spirit of God moved upon the face of the waters." ],
+        [ 3, "And God said, Let there be light: and there was light." ],
+        [ 4, "And God saw the light, that it was good: and God divided the light from the darkness." ],
+        [ 5, "And God called the light Day, and the darkness he called Night. And the evening and the morning were the first day." ],
+        [ 6, "And God said, Let there be a firmament in the midst of the waters, and let it divide the waters from the waters." ],
+        [ 7, "And God made the firmament, and divided the waters which were under the firmament from the waters which were above the firmament: and it was so." ],
+        [ 8, "And God called the firmament Heaven. And the evening and the morning were the second day." ],
+        [ 9, "And God said, Let the waters under the heaven be gathered together unto one place, and let the dry land appear: and it was so." ],
+        [ 10, "And God called the dry land Earth; and the gathering together of the waters called he Seas: and God saw that it was good." ]
     ]
 }

tests/resources/bibles/rst.json

@@ -2,15 +2,15 @@
     "book": "Exodus",
     "chapter": 1,
     "verses": [
-        [ "1", "Вот имена сынов Израилевых, которые вошли в Египет с Иаковом, вошли каждый с домом своим:" ],
-        [ "2", "Рувим, Симеон, Левий и Иуда," ],
-        [ "3", "Иссахар, Завулон и Вениамин," ],
-        [ "4", "Дан и Неффалим, Гад и Асир." ],
-        [ "5", "Всех же душ, происшедших от чресл Иакова, было семьдесят, а Иосиф был [уже] в Египте." ],
-        [ "6", "И умер Иосиф и все братья его и весь род их;" ],
-        [ "7", "а сыны Израилевы расплодились и размножились, и возросли и усилились чрезвычайно, и наполнилась ими земля та." ],
-        [ "8", "И восстал в Египте новый царь, который не знал Иосифа," ],
-        [ "9", "и сказал народу своему: вот, народ сынов Израилевых многочислен и сильнее нас;" ],
-        [ "10", "перехитрим же его, чтобы он не размножался; иначе, когда случится война, соединится и он с нашими неприятелями, и вооружится против нас, и выйдет из земли [нашей]." ]
+        [ 1, "Вот имена сынов Израилевых, которые вошли в Египет с Иаковом, вошли каждый с домом своим:" ],
+        [ 2, "Рувим, Симеон, Левий и Иуда," ],
+        [ 3, "Иссахар, Завулон и Вениамин," ],
+        [ 4, "Дан и Неффалим, Гад и Асир." ],
+        [ 5, "Всех же душ, происшедших от чресл Иакова, было семьдесят, а Иосиф был [уже] в Египте." ],
+        [ 6, "И умер Иосиф и все братья его и весь род их;" ],
+        [ 7, "а сыны Израилевы расплодились и размножились, и возросли и усилились чрезвычайно, и наполнилась ими земля та." ],
+        [ 8, "И восстал в Египте новый царь, который не знал Иосифа," ],
+        [ 9, "и сказал народу своему: вот, народ сынов Израилевых многочислен и сильнее нас;" ],
+        [ 10, "перехитрим же его, чтобы он не размножался; иначе, когда случится война, соединится и он с нашими неприятелями, и вооружится против нас, и выйдет из земли [нашей]." ]
     ]
 }

tests/resources/bibles/web.json

@@ -2,15 +2,15 @@
     "book": "Genesis",
     "chapter": "1",
     "verses": [
-        [ "1", "In the beginning God created the heavens and the earth."],
-        [ "2", "Now the earth was formless and empty. Darkness was on the surface of the deep. God’s Spirit was hovering over the surface of the waters." ],
-        [ "3", "God said, “Let there be light,” and there was light." ],
-        [ "4", "God saw the light, and saw that it was good. God divided the light from the darkness." ],
-        [ "5", "God called the light “day,” and the darkness he called “night.” There was evening and there was morning, one day." ],
-        [ "6", "God said, “Let there be an expanse in the middle of the waters, and let it divide the waters from the waters.”" ],
-        [ "7", "God made the expanse, and divided the waters which were under the expanse from the waters which were above the expanse; and it was so." ],
-        [ "8", "God called the expanse “sky.” There was evening and there was morning, a second day." ],
-        [ "9", "God said, “Let the waters under the sky be gathered together to one place, and let the dry land appear;” and it was so." ],
-        [ "10", "God called the dry land “earth,” and the gathering together of the waters he called “seas.” God saw that it was good." ]
+        [ 1, "In the beginning God created the heavens and the earth."],
+        [ 2, "Now the earth was formless and empty. Darkness was on the surface of the deep. God’s Spirit was hovering over the surface of the waters." ],
+        [ 3, "God said, “Let there be light,” and there was light." ],
+        [ 4, "God saw the light, and saw that it was good. God divided the light from the darkness." ],
+        [ 5, "God called the light “day,” and the darkness he called “night.” There was evening and there was morning, one day." ],
+        [ 6, "God said, “Let there be an expanse in the middle of the waters, and let it divide the waters from the waters.”" ],
+        [ 7, "God made the expanse, and divided the waters which were under the expanse from the waters which were above the expanse; and it was so." ],
+        [ 8, "God called the expanse “sky.” There was evening and there was morning, a second day." ],
+        [ 9, "God said, “Let the waters under the sky be gathered together to one place, and let the dry land appear;” and it was so." ],
+        [ 10, "God called the dry land “earth,” and the gathering together of the waters he called “seas.” God saw that it was good." ]
     ]
 }