openlp-core team mailing list archive

[Samuel Mehrbrodt <s.mehrbrodt@xxxxxxxxx>]

Samuel Mehrbrodt has proposed merging lp:~sam92/openlp/fix-songbeamer-import into lp:openlp.

Requested reviews:
  Raoul Snyman (raoul-snyman)
  Jonathan Corwin (j-corwin)

For more details, see:
https://code.launchpad.net/~sam92/openlp/fix-songbeamer-import/+merge/189514

Fix SongBeamer Import and add testcase

The read() call failed when the file was not unicode. So I first open the file in binary mode without encoding. After the encoding has been detected, the file is read with the proper encoding.

I tested this with ~800 SongBeamer files
-- 
https://code.launchpad.net/~sam92/openlp/fix-songbeamer-import/+merge/189514
Your team OpenLP Core is subscribed to branch lp:openlp.

=== modified file 'openlp/plugins/songs/lib/songbeamerimport.py'
--- openlp/plugins/songs/lib/songbeamerimport.py	2013-08-31 18:17:38 +0000
+++ openlp/plugins/songs/lib/songbeamerimport.py	2013-10-06 22:10:30 +0000
@@ -102,10 +102,10 @@
         """
         Receive a single file or a list of files to import.
         """
+        if not isinstance(self.import_source, list):
+            return
         self.import_wizard.progress_bar.setMaximum(len(self.import_source))
-        if not isinstance(self.import_source, list):
-            return
-        for file in self.import_source:
+        for import_file in self.import_source:
             # TODO: check that it is a valid SongBeamer file
             if self.stop_import_flag:
                 return
@@ -113,12 +113,13 @@
             self.currentVerse = ''
             self.currentVerseType = VerseType.tags[VerseType.Verse]
             read_verses = False
-            file_name = os.path.split(file)[1]
-            if os.path.isfile(file):
-                detect_file = open(file, 'r')
+            file_name = os.path.split(import_file)[1]
+            if os.path.isfile(import_file):
+                # First open in binary mode to detect the encoding
+                detect_file = open(import_file, 'rb')
                 details = chardet.detect(detect_file.read())
                 detect_file.close()
-                infile = codecs.open(file, 'r', details['encoding'])
+                infile = codecs.open(import_file, 'r', details['encoding'])
                 song_data = infile.readlines()
                 infile.close()
             else:
@@ -149,7 +150,7 @@
                 self.replaceHtmlTags()
                 self.addVerse(self.currentVerse, self.currentVerseType)
             if not self.finish():
-                self.logError(file)
+                self.logError(import_file)
 
     def replaceHtmlTags(self):
         """

=== added file 'tests/functional/openlp_plugins/songs/test_songbeamerimport.py'
--- tests/functional/openlp_plugins/songs/test_songbeamerimport.py	1970-01-01 00:00:00 +0000
+++ tests/functional/openlp_plugins/songs/test_songbeamerimport.py	2013-10-06 22:10:30 +0000
@@ -0,0 +1,127 @@
+"""
+This module contains tests for the Songbeamer song importer.
+"""
+
+import os
+from unittest import TestCase
+from mock import patch, MagicMock
+
+from openlp.plugins.songs.lib.songbeamerimport import SongBeamerImport
+
+TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '../../../resources/songbeamersongs'))
+SONG_TEST_DATA = {'Lobsinget dem Herrn.sng':
+        {'title': 'GL 1 - Lobsinget dem Herrn',
+         'verses':
+             [('1. Lobsinget dem Herrn,\no preiset Ihn gern!\n'
+               'Anbetung und Lob Ihm gebühret.\n', 'v'),
+              ('2. Lobsingt Seiner Lieb´,\ndie einzig ihn trieb,\n'
+               'zu sterben für unsere Sünden!\n', 'v'),
+              ('3. Lobsingt Seiner Macht!\nSein Werk ist vollbracht:\n'
+               'Er sitzet zur Rechten des Vaters.\n', 'v'),
+              ('4. Lobsingt seiner Treu´,\ndie immerdar neu,\n'
+               'bis Er uns zur Herrlichket führet!\n\n', 'v')],
+         'song_book_name': 'Glaubenslieder I',
+         'song_number': "1"}
+        }
+
+
+class TestSongBeamerImport(TestCase):
+    """
+    Test the functions in the :mod:`songbeamerimport` module.
+    """
+    def create_importer_test(self):
+        """
+        Test creating an instance of the SongBeamer file importer
+        """
+        # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+        with patch('openlp.plugins.songs.lib.songbeamerimport.SongImport'):
+            mocked_manager = MagicMock()
+
+            # WHEN: An importer object is created
+            importer = SongBeamerImport(mocked_manager)
+
+            # THEN: The importer object should not be None
+            self.assertIsNotNone(importer, 'Import should not be none')
+
+    def invalid_import_source_test(self):
+        """
+        Test SongBeamerImport.doImport handles different invalid import_source values
+        """
+        # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+        with patch('openlp.plugins.songs.lib.songbeamerimport.SongImport'):
+            mocked_manager = MagicMock()
+            mocked_import_wizard = MagicMock()
+            importer = SongBeamerImport(mocked_manager)
+            importer.import_wizard = mocked_import_wizard
+            importer.stop_import_flag = True
+
+            # WHEN: Import source is not a list
+            for source in ['not a list', 0]:
+                importer.import_source = source
+
+                # THEN: doImport should return none and the progress bar maximum should not be set.
+                self.assertIsNone(importer.doImport(), 'doImport should return None when import_source is not a list')
+                self.assertEquals(mocked_import_wizard.progress_bar.setMaximum.called, False,
+                                  'setMaxium on import_wizard.progress_bar should not have been called')
+
+    def valid_import_source_test(self):
+        """
+        Test SongBeamerImport.doImport handles different invalid import_source values
+        """
+        # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+        with patch('openlp.plugins.songs.lib.songbeamerimport.SongImport'):
+            mocked_manager = MagicMock()
+            mocked_import_wizard = MagicMock()
+            importer = SongBeamerImport(mocked_manager)
+            importer.import_wizard = mocked_import_wizard
+            importer.stop_import_flag = True
+
+            # WHEN: Import source is a list
+            importer.import_source = ['List', 'of', 'files']
+
+            # THEN: doImport should return none and the progress bar setMaximum should be called with the length of
+            #       import_source.
+            self.assertIsNone(importer.doImport(),
+                'doImport should return None when import_source is a list and stop_import_flag is True')
+            mocked_import_wizard.progress_bar.setMaximum.assert_called_with(len(importer.import_source))
+
+    def file_import_test(self):
+        """
+        Test the actual import of real song files and check that the imported data is correct.
+        """
+
+        # GIVEN: Test files with a mocked out SongImport class, a mocked out "manager", a mocked out "import_wizard",
+        #       and mocked out "author", "add_copyright", "add_verse", "finish" methods.
+        with patch('openlp.plugins.songs.lib.songbeamerimport.SongImport'):
+            for song_file in SONG_TEST_DATA:
+                mocked_manager = MagicMock()
+                mocked_import_wizard = MagicMock()
+                mocked_add_verse = MagicMock()
+                mocked_finish = MagicMock()
+                mocked_finish.return_value = True
+                importer = SongBeamerImport(mocked_manager)
+                importer.import_wizard = mocked_import_wizard
+                importer.stop_import_flag = False
+                importer.addVerse = mocked_add_verse
+                importer.finish = mocked_finish
+
+                # WHEN: Importing each file
+                importer.import_source = [os.path.join(TEST_PATH, song_file)]
+                title = SONG_TEST_DATA[song_file]['title']
+                add_verse_calls = SONG_TEST_DATA[song_file]['verses']
+                song_book_name = SONG_TEST_DATA[song_file]['song_book_name']
+                song_number = SONG_TEST_DATA[song_file]['song_number']
+
+                # THEN: doImport should return none, the song data should be as expected, and finish should have been
+                #       called.
+                self.assertIsNone(importer.doImport(), 'doImport should return None when it has completed')
+                self.assertEquals(importer.title, title, 'title for %s should be "%s"' % (song_file, title))
+                for verse_text, verse_tag in add_verse_calls:
+                    mocked_add_verse.assert_any_call(verse_text, verse_tag)
+                if song_book_name:
+                    self.assertEquals(importer.songBookName, song_book_name, 'songBookName for %s should be "%s"'
+                        % (song_file, song_book_name))
+                if song_number:
+                    self.assertEquals(importer.songNumber, song_number, 'songNumber for %s should be %s'
+                        % (song_file, song_number))
+                mocked_finish.assert_called_with()

=== added directory 'tests/resources/songbeamersongs'
=== added file 'tests/resources/songbeamersongs/Lobsinget dem Herrn.sng'
--- tests/resources/songbeamersongs/Lobsinget dem Herrn.sng	1970-01-01 00:00:00 +0000
+++ tests/resources/songbeamersongs/Lobsinget dem Herrn.sng	2013-10-06 22:10:30 +0000
@@ -0,0 +1,25 @@
+#LangCount=1
+#Title=GL 1 - Lobsinget dem Herrn
+#Editor=SongBeamer 4.20
+#Version=3
+#Format=F/K//
+#TitleFormat=U
+#ChurchSongID=0001
+#Songbook=Glaubenslieder I / 1
+---
+1. Lobsinget dem Herrn,
+o preiset Ihn gern!
+Anbetung und Lob Ihm geb�
+               ---
+2. Lobsingt Seiner Lieb�,
+die einzig ihn trieb,
+zu sterben f�ere S�
+              ---
+3. Lobsingt Seiner Macht!
+Sein Werk ist vollbracht:
+Er sitzet zur Rechten des Vaters.
+                ---
+4. Lobsingt seiner Treu�,
+die immerdar neu,
+bis Er uns zur Herrlichket f�
+