Mostly refactors of OSIS + tests

openlp/plugins/bibles/lib/bibleimport.py

@@ -41,6 +41,10 @@ class BibleImport(OpenLPMixin, BibleDB):
         super().__init__(*args, **kwargs)
         self.filename = kwargs['filename'] if 'filename' in kwargs else None
 
+    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))
+
     @staticmethod
     def is_compressed(file):
         """
@@ -114,15 +118,15 @@ class BibleImport(OpenLPMixin, BibleDB):
         """
         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
+                # NOTE: We don't need to do any of the normal encoding detection here, because lxml does it's own
-                # detection, and the two mechanisms together interfere with each other.
+                # 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',
+                    self.wizard.increment_progress_bar(
-                                                                 'Removing unused tags (this may take a few minutes)...'))
+                        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)
@@ -133,8 +137,10 @@ class BibleImport(OpenLPMixin, BibleDB):
         except OSError as e:
             log.exception('Opening {file_name} failed.'.format(file_name=e.filename))
             trace_error_handler(log)
-            critical_error_message_box( title='An Error Occured When Opening A File',
+            critical_error_message_box(
-                message='The following error occurred when trying to open\n{file_name}:\n\n{error}'.format(file_name=e.filename, error=e.strerror))
+                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 validate_xml_file(self, filename, tag):
@@ -142,6 +148,7 @@ class BibleImport(OpenLPMixin, BibleDB):
         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):
@@ -165,4 +172,3 @@ class BibleImport(OpenLPMixin, BibleDB):
                               '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/importers/csvbible.py

@@ -150,8 +150,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.create_verse(book.id, int(verse.chapter_number), int(verse.number), verse.text)
-        self.wizard.increment_progress_bar(translate('BiblesPlugin.CSVBible', 'Importing verses... done.'))
         self.session.commit()
 
     def do_import(self, bible_name=None):

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

@@ -115,10 +115,8 @@ class OpenSongBible(BibleImport):
             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)
-            self.wizard.increment_progress_bar(translate('BiblesPlugin.Opensong',
-                                                         'Importing {name} {chapter}...'
-                                                         ).format(name=book.name, chapter=chapter_number))
 
     def process_verses(self, book, chapter_number, verses):
         """

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

@@ -23,9 +23,6 @@
 import logging
 from lxml import etree
 
-from openlp.core.common import translate, trace_error_handler
-from openlp.core.lib.exceptions import ValidationError
-from openlp.core.lib.ui import critical_error_message_box
 from openlp.plugins.bibles.lib.bibleimport import BibleImport
 
 log = logging.getLogger(__name__)
@@ -74,14 +71,9 @@ 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('count(//ns:chapter[1]/ns:verse)', namespaces=NS)
+verse_in_chapter = etree.XPath('//ns:chapter[1]/ns:verse', namespaces=NS)
-text_in_verse = etree.XPath('count(//ns:verse[1]/text())', namespaces=NS)
+text_in_verse = etree.XPath('//ns:verse[1]/text()', namespaces=NS)
 
 
 class OSISBible(BibleImport):
@@ -90,70 +82,79 @@ class OSISBible(BibleImport):
     """
     def process_books(self, bible_data):
         """
+        Extract and create the bible books from the parsed xml
 
-        :param bible_data:
+        :param bible_data: parsed xml
-        :return:
+        :return: None
         """
-        no_of_books = int(bible_data.xpath("count(//ns:div[@type='book'])", namespaces=NS))
         # 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_and_verses(db_book, book)
+            self.process_chapters(db_book, book)
             self.session.commit()
 
-    def process_chapters_and_verses(self, book, chapters):
+    def process_chapters(self, book, chapters):
         """
+        Extract the chapters, and do some initial processing of the verses
 
-        :param book:
+        :param book: An OpenLP bible database book object
-        :param chapters:
+        :param chapters: parsed chapters
-        :return:
+        :return: None
         """
         # Find out if chapter-tags contains the verses, or if it is used as milestone/anchor
-        if int(verse_in_chapter(chapters)) > 0:
+        if verse_in_chapter(chapters):
             # The chapter tags contains the verses
             for chapter in chapters:
-                chapter_number = chapter.get("osisID").split('.')[1]
+                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 int(text_in_verse(chapter)) == 0:
+                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.
-                        if verse.get('sID'):
+                        self.process_verse(book, chapter_number, verse, use_milestones=True)
-                            verse_number = verse.get("osisID").split('.')[2]
-                            verse_text = verse.tail
-                            if verse_text:
-                                self.create_verse(book.id, chapter_number, verse_number, verse_text.strip())
                 else:
                     # Verse-tags contains the text
                     for verse in chapter:
-                        verse_number = verse.get("osisID").split('.')[2]
+                        self.process_verse(book, chapter_number, verse)
-                        if verse.text:
-                            self.create_verse(book.id, chapter_number, verse_number, verse.text.strip())
-                self.wizard.increment_progress_bar(
-                    translate('BiblesPlugin.OsisImport', 'Importing %(bookname)s %(chapter)s...') %
-                    {'bookname': 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 chapters:
                 if element.tag == '{http://www.bibletechnologies.net/2003/OSIS/namespace}chapter' \
                         and element.get('sID'):
-                    chapter_number = element.get("osisID").split('.')[1]
+                    chapter_number = int(element.get("osisID").split('.')[1])
-                    self.wizard.increment_progress_bar(
+                    self.set_current_chapter(book.name, chapter_number)
-                        translate('BiblesPlugin.OsisImport', 'Importing %(bookname)s %(chapter)s...') %
+                elif element.tag == '{http://www.bibletechnologies.net/2003/OSIS/namespace}verse':
-                        {'bookname': book.name, 'chapter': chapter_number})
-                elif element.tag == '{http://www.bibletechnologies.net/2003/OSIS/namespace}verse' \
-                        and element.get('sID'):
                     # If this tag marks the start of a verse, the verse text is between this tag and
                     # the next tag, which the "tail" attribute gives us.
-                    verse_number = element.get("osisID").split('.')[2]
+                    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())
 
@@ -162,7 +163,7 @@ class OSISBible(BibleImport):
         Loads a Bible from file.
         """
         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')
+        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

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

@@ -79,7 +79,7 @@ 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,

tests/functional/openlp_plugins/bibles/test_csvimport.py

@@ -241,7 +241,6 @@ class TestCSVImport(TestCase):
 
             # THEN: get_book_name should not be called and the return value should be None
             self.assertFalse(importer.get_book_name.called)
-            importer.wizard.increment_progress_bar.assert_called_once_with('Importing verses... done.')
             self.assertIsNone(result)
 
     def process_verses_successful_test(self):
@@ -352,6 +351,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_opensongimport.py

@@ -47,6 +47,9 @@ class TestOpenSongImport(TestCase, TestMixin):
     """
 
     def setUp(self):
+        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()
@@ -177,12 +180,11 @@ class TestOpenSongImport(TestCase, TestMixin):
         mocked_log.warning.assert_called_once_with('Illegal verse number: (1, 2, 3)')
         self.assertEqual(result, 13)
 
-    @patch('openlp.plugins.bibles.lib.bibleimport.BibleImport.find_and_create_book')
+    def process_books_stop_import_test(self):
-    def process_books_stop_import_test(self, mocked_find_and_create_book):
         """
         Test process_books when stop_import is set to True
         """
-        # GIVEN: An isntance of OpenSongBible
+        # GIVEN: An instance of OpenSongBible
         importer = OpenSongBible(MagicMock(), path='.', name='.', filename='')
 
         # WHEN: stop_import_flag is set to True
@@ -190,15 +192,15 @@ class TestOpenSongImport(TestCase, TestMixin):
         importer.process_books(['Book'])
 
         # THEN: find_and_create_book should not have been called
-        self.assertFalse(mocked_find_and_create_book.called)
+        self.assertFalse(self.mocked_find_and_create_book.called)
 
-    @patch('openlp.plugins.bibles.lib.bibleimport.BibleImport.find_and_create_book',
+    def process_books_completes_test(self):
-           **{'side_effect': ['db_book1', 'db_book2']})
-    def process_books_completes_test(self, mocked_find_and_create_book):
         """
         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()
@@ -208,7 +210,6 @@ class TestOpenSongImport(TestCase, TestMixin):
             book2.attrib = {'n': 'Name2'}
             book2.c = 'Chapter2'
             importer.language_id = 10
-        importer.process_chapters = MagicMock()
             importer.session = MagicMock()
             importer.stop_import_flag = False
 
@@ -216,8 +217,8 @@ class TestOpenSongImport(TestCase, TestMixin):
             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(mocked_find_and_create_book.call_args_list, [call('Name1', 2, 10), call('Name2', 2, 10)])
+            self.assertEqual(self.mocked_find_and_create_book.call_args_list, [call('Name1', 2, 10), call('Name2', 2, 10)])
-        self.assertEqual(importer.process_chapters.call_args_list,
+            self.assertEqual(mocked_process_chapters.call_args_list,
                              [call('db_book1', 'Chapter1'), call('db_book2', 'Chapter2')])
             self.assertEqual(importer.session.commit.call_count, 2)
 
@@ -373,6 +374,15 @@ class TestOpenSongImport(TestCase, TestMixin):
             # 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):
         """
         Test the actual import of OpenSong Bible file

tests/functional/openlp_plugins/bibles/test_osisimport.py

@@ -27,9 +27,10 @@ import os
 import json
 from unittest import TestCase
 
-from tests.functional import MagicMock, patch
+from tests.functional import MagicMock, call, patch
-from openlp.plugins.bibles.lib.importers.osis import OSISBible
+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'))
@@ -39,8 +40,16 @@ class TestOsisImport(TestCase):
     """
     Test the functions in the :mod:`osisimport` module.
     """
-
     def setUp(self):
+        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.db.Registry')
         self.addCleanup(self.registry_patcher.stop)
         self.registry_patcher.start()
@@ -61,6 +70,285 @@ 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)
@@ -98,8 +386,7 @@ class TestOsisImport(TestCase):
             self.assertFalse(result)
             self.assertFalse(mocked_process_books.called)
 
-    @patch('openlp.plugins.bibles.lib.importers.opensong.log')
+    def do_import_completes_test(self):
-    def do_import_completes_test(self, mocked_log):
         """
         Test do_import when it completes successfully
         """
@@ -117,6 +404,19 @@ class TestOsisImport(TestCase):
             # 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.db.Registry')
+        self.addCleanup(self.registry_patcher.stop)
+        self.registry_patcher.start()
+        self.manager_patcher = patch('openlp.plugins.bibles.lib.db.Manager')
+        self.addCleanup(self.manager_patcher.stop)
+        self.manager_patcher.start()
+
     def test_file_import_nested_tags(self):
         """
         Test the actual import of OSIS Bible file, with nested chapter and verse tags
@@ -145,7 +445,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):
         """
@@ -175,7 +475,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):
         """
@@ -205,7 +505,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):
         """
@@ -235,4 +535,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_zefaniaimport.py

@@ -88,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):
@@ -118,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."],
+        [ 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." ],
+        [ 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." ],
+        [ 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," ],
+        [ 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." ],
+        [ 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!\"" ],
+        [ 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;" ],
+        [ 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." ],
+        [ 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;" ],
+        [ 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." ]
+        [ 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."],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ]
+        [ 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", "Вот имена сынов Израилевых, которые вошли в Египет с Иаковом, вошли каждый с домом своим:" ],
+        [ 1, "Вот имена сынов Израилевых, которые вошли в Египет с Иаковом, вошли каждый с домом своим:" ],
-        [ "2", "Рувим, Симеон, Левий и Иуда," ],
+        [ 2, "Рувим, Симеон, Левий и Иуда," ],
-        [ "3", "Иссахар, Завулон и Вениамин," ],
+        [ 3, "Иссахар, Завулон и Вениамин," ],
-        [ "4", "Дан и Неффалим, Гад и Асир." ],
+        [ 4, "Дан и Неффалим, Гад и Асир." ],
-        [ "5", "Всех же душ, происшедших от чресл Иакова, было семьдесят, а Иосиф был [уже] в Египте." ],
+        [ 5, "Всех же душ, происшедших от чресл Иакова, было семьдесят, а Иосиф был [уже] в Египте." ],
-        [ "6", "И умер Иосиф и все братья его и весь род их;" ],
+        [ 6, "И умер Иосиф и все братья его и весь род их;" ],
-        [ "7", "а сыны Израилевы расплодились и размножились, и возросли и усилились чрезвычайно, и наполнилась ими земля та." ],
+        [ 7, "а сыны Израилевы расплодились и размножились, и возросли и усилились чрезвычайно, и наполнилась ими земля та." ],
-        [ "8", "И восстал в Египте новый царь, который не знал Иосифа," ],
+        [ 8, "И восстал в Египте новый царь, который не знал Иосифа," ],
-        [ "9", "и сказал народу своему: вот, народ сынов Израилевых многочислен и сильнее нас;" ],
+        [ 9, "и сказал народу своему: вот, народ сынов Израилевых многочислен и сильнее нас;" ],
-        [ "10", "перехитрим же его, чтобы он не размножался; иначе, когда случится война, соединится и он с нашими неприятелями, и вооружится против нас, и выйдет из земли [нашей]." ]
+        [ 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."],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ],
+        [ 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.”" ],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ],
+        [ 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." ]
+        [ 10, "God called the dry land “earth,” and the gathering together of the waters he called “seas.” God saw that it was good." ]
     ]
 }