From f08d0c28a58f4885a56c8dc52bbff376082a04b3 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Sun, 14 Aug 2016 11:00:27 +0100 Subject: [PATCH] further bible refactors --- openlp/plugins/bibles/bibleplugin.py | 8 +- openlp/plugins/bibles/lib/__init__.py | 4 +- openlp/plugins/bibles/lib/bibleimport.py | 1 - .../plugins/bibles/lib/importers/opensong.py | 7 +- openlp/plugins/bibles/lib/importers/osis.py | 9 +- .../plugins/bibles/lib/importers/zefania.py | 4 +- .../openlp_plugins/bibles/test_bibleimport.py | 85 ++++++++++++++++--- .../openlp_plugins/bibles/test_csvimport.py | 4 +- .../bibles/test_zefaniaimport.py | 6 +- 9 files changed, 86 insertions(+), 42 deletions(-) diff --git a/openlp/plugins/bibles/bibleplugin.py b/openlp/plugins/bibles/bibleplugin.py index f63b85a92..e9168d695 100644 --- a/openlp/plugins/bibles/bibleplugin.py +++ b/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() diff --git a/openlp/plugins/bibles/lib/__init__.py b/openlp/plugins/bibles/lib/__init__.py index 804755d18..e730009e7 100644 --- a/openlp/plugins/bibles/lib/__init__.py +++ b/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) diff --git a/openlp/plugins/bibles/lib/bibleimport.py b/openlp/plugins/bibles/lib/bibleimport.py index 4d015223b..7ebdcb170 100644 --- a/openlp/plugins/bibles/lib/bibleimport.py +++ b/openlp/plugins/bibles/lib/bibleimport.py @@ -35,7 +35,6 @@ class BibleImport(OpenLPMixin, 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 diff --git a/openlp/plugins/bibles/lib/importers/opensong.py b/openlp/plugins/bibles/lib/importers/opensong.py index 43c1cf8ca..10c0ed87e 100644 --- a/openlp/plugins/bibles/lib/importers/opensong.py +++ b/openlp/plugins/bibles/lib/importers/opensong.py @@ -73,12 +73,7 @@ class OpenSongBible(BibleImport): 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']) + db_book = self.find_and_create_book(str(book.attrib['n']), len(bible.b), language_id) chapter_number = 0 for chapter in book.c: if self.stop_import_flag: diff --git a/openlp/plugins/bibles/lib/importers/osis.py b/openlp/plugins/bibles/lib/importers/osis.py index c833277fe..db12bb7e9 100644 --- a/openlp/plugins/bibles/lib/importers/osis.py +++ b/openlp/plugins/bibles/lib/importers/osis.py @@ -98,7 +98,7 @@ class OSISBible(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(osis_bible_tree.xpath("count(//ns:div[@type='book'])", namespaces=NS)) + no_of_books = int(osis_bible_tree.xpath("count(//ns:div[@type='book'])", namespaces=NS)) # Precompile a few xpath-querys verse_in_chapter = etree.XPath('count(//ns:chapter[1]/ns:verse)', namespaces=NS) text_in_verse = etree.XPath('count(//ns:verse[1]/text())', namespaces=NS) @@ -109,12 +109,7 @@ class OSISBible(BibleImport): 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']) + db_book = self.find_and_create_book(book.get('osisID'), no_of_books, language_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 diff --git a/openlp/plugins/bibles/lib/importers/zefania.py b/openlp/plugins/bibles/lib/importers/zefania.py index 61ee41166..bc31a1664 100644 --- a/openlp/plugins/bibles/lib/importers/zefania.py +++ b/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) diff --git a/tests/functional/openlp_plugins/bibles/test_bibleimport.py b/tests/functional/openlp_plugins/bibles/test_bibleimport.py index e2076df55..127c6fd16 100644 --- a/tests/functional/openlp_plugins/bibles/test_bibleimport.py +++ b/tests/functional/openlp_plugins/bibles/test_bibleimport.py @@ -29,8 +29,10 @@ from lxml import etree, objectify from unittest import TestCase from openlp.core.common.languages import Language +from openlp.core.lib.exceptions import ValidationError from openlp.plugins.bibles.lib.bibleimport import BibleImport -from tests.functional import MagicMock, patch +from openlp.plugins.bibles.lib.db import BibleDB +from tests.functional import ANY, MagicMock, patch class TestBibleImport(TestCase): @@ -39,23 +41,79 @@ class TestBibleImport(TestCase): """ def setUp(self): - test_file = BytesIO(b'\n' - b'\n' - b'
Test

data

tokeep
\n' - b' Testdatatodiscard\n' - b'') + test_file = BytesIO( + b'\n' + b'\n' + b'
Test

data

tokeep
\n' + b' Testdatatodiscard\n' + b'' + ) self.file_patcher = patch('builtins.open', return_value=test_file) - self.log_patcher = patch('openlp.plugins.bibles.lib.bibleimport.log') - 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.log_patcher = patch('openlp.plugins.bibles.lib.bibleimport.log') + self.addCleanup(self.log_patcher.stop) self.mock_log = self.log_patcher.start() + self.setup_patcher = patch('openlp.plugins.bibles.lib.db.BibleDB._setup') + self.addCleanup(self.setup_patcher.stop) self.setup_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 check_for_compression_test(self): + """ + Test the check_for_compression method when called with a path to an uncompressed file + """ + # GIVEN: A mocked is_zipfile which returns False and an instance of BibleImport + with patch('openlp.plugins.bibles.lib.bibleimport.is_zipfile', return_value=False) as mocked_is_zip: + instance = BibleImport(MagicMock()) + + # WHEN: Calling check_for_compression + result = instance.check_for_compression('filename.tst') + + # THEN: None should be returned + self.assertIsNone(result) + mocked_is_zip.assert_called_once_with('filename.tst') + + def check_for_compression_zip_file_test(self): + """ + Test the check_for_compression method when called with a path to a compressed file + """ + # GIVEN: A patched is_zipfile which returns True and an instance of BibleImport + with patch('openlp.plugins.bibles.lib.bibleimport.is_zipfile', return_value=True),\ + patch('openlp.plugins.bibles.lib.bibleimport.critical_error_message_box') as mocked_message_box: + instance = BibleImport(MagicMock()) + + # WHEN: Calling check_for_compression + # THEN: A Validation error should be raised and the user should be notified. + with self.assertRaises(ValidationError) as context: + instance.check_for_compression('filename.tst') + self.assertTrue(mocked_message_box.called) + self.assertEqual(context.exception.msg, '"filename.tst" is compressed') + def get_language_id_language_found_test(self): """ Test get_language_id() when called with a name found in the languages list @@ -81,8 +139,7 @@ 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, \ + 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: instance = BibleImport(MagicMock()) instance.save_meta = MagicMock() diff --git a/tests/functional/openlp_plugins/bibles/test_csvimport.py b/tests/functional/openlp_plugins/bibles/test_csvimport.py index ada03a07d..7a56c1b85 100644 --- a/tests/functional/openlp_plugins/bibles/test_csvimport.py +++ b/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.db.Registry') + self.addCleanup(self.registry_patcher.stop) self.registry_patcher.start() def test_create_importer(self): diff --git a/tests/functional/openlp_plugins/bibles/test_zefaniaimport.py b/tests/functional/openlp_plugins/bibles/test_zefaniaimport.py index 200a36f45..5294b7f5c 100644 --- a/tests/functional/openlp_plugins/bibles/test_zefaniaimport.py +++ b/tests/functional/openlp_plugins/bibles/test_zefaniaimport.py @@ -42,14 +42,12 @@ class TestZefaniaImport(TestCase): def setUp(self): self.registry_patcher = patch('openlp.plugins.bibles.lib.db.Registry') + self.addCleanup(self.registry_patcher.stop) self.registry_patcher.start() self.manager_patcher = patch('openlp.plugins.bibles.lib.db.Manager') + self.addCleanup(self.manager_patcher.stop) self.manager_patcher.start() - def tearDown(self): - self.registry_patcher.stop() - self.manager_patcher.stop() - def test_create_importer(self): """ Test creating an instance of the Zefania file importer