Merge lp:~phill-ridout/openlp/bug1125956 into lp:openlp

[Phill]

Fixed importing of songs with verses numbered 1A, or 1.5 etc. in SongShowPlus importer
Added Tests for SongShow Plus importer
Moved the song files as requested

Changed regex string type to raw
Comment added
Removed the use of the call helper object. It works however, it will only ensure that the method has been called wtih the expected calls and not catch any additional calls that maybe made to the method.

[Andreas Preikschat (googol-deactivatedaccount)]

427 === added file 'tests/resources/Amazing Grace.sbsong'
429 === added file 'tests/resources/Beautiful Garden Of Prayer.sbsong'

Please move them in a dir SongShowPlusSongs or something like that

review: Needs Fixing

[Andreas Preikschat (googol-deactivatedaccount)]

152 + match = re.match(u'(\D*)(\d+)', verse_name)

For regular expression you use r'' instead of u''

20 - self.verseOrderListGenerated.append(verse_def)
21 + if verse_def not in self.verseOrderListGenerated:
22 + self.verseOrderListGenerated.append(verse_def)

Why are you changing this? Docs say:
        Add a verse. This is the whole verse, lines split by \\n. It will also
        attempt to detect duplicates. In this case it will just add to the verse
        order.
So we should append the verse_def in any case, shouldn't we?

review: Needs Fixing

[Phill (phill-ridout)]

> 20 - self.verseOrderListGenerated.append(verse_def)
> 21 + if verse_def not in self.verseOrderListGenerated:
> 22 + self.verseOrderListGenerated.append(verse_def)
>
> Why are you changing this? Docs say:
> Add a verse. This is the whole verse, lines split by \\n. It will also
> attempt to detect duplicates. In this case it will just add to the
> verse
> order.
> So we should append the verse_def in any case, shouldn't we?

Because OpenLP does not support verse numbering such as V1A and V1B we are dropping the A and B so we have two verses called V1 being added to a song (instead of V1A and V1B)

Each time V1 appears in the verse order both verses called V1 (actually V1A and V1B) were being added to the verse order.

With out this change V1 was being added to the verse order twice (once for each verse called V1), however because of the above statement the actual verse order shown in the live or preview controller repeated both verses.

This is a bit hard to clearly explain. It is probably easier for you to see the result by checking out this branch and importing the song attached to http://support.openlp.org/issues/1897

Import it once with out the change on line 20 and once with the change and you will see the problem.

[Phill (phill-ridout)]

Ok, this is ready to merge providing you approve it!

[Andreas Preikschat (googol-deactivatedaccount)]

> Because OpenLP does not support verse numbering such as V1A and V1B we are dropping the A and B so
> we have two verses called V1 being added to a song (instead of V1A and V1B)

Then please add comment for this. Nobody will remember why this "if" was added.

review: Needs Fixing

[Raoul Snyman (raoul-snyman)]

Currently the tests fail because we're using an older version of the mock library on the Jenkins server which doesn't have the "call" function. I don't have access to the server right now to be able to fix anything, but I would prefer if you figure out how to get around the issue.

[Raoul Snyman (raoul-snyman)]

You can check for a certain *number* of calls to that method.

  assert mocked_parse_author.call_count == 0, u'This method should not be called'

review: Approve

[Phill (phill-ridout)]

Andreas,

Would you mind reviewing this?

[Andreas Preikschat (googol-deactivatedaccount)]

review: Approve

=== modified file 'openlp/plugins/songs/lib/__init__.py'
--- openlp/plugins/songs/lib/__init__.py	2013-03-11 21:10:29 +0000
+++ openlp/plugins/songs/lib/__init__.py	2013-04-05 18:34:21 +0000
@@ -168,6 +168,7 @@
         translate('SongsPlugin.VerseType', 'Intro'),
         translate('SongsPlugin.VerseType', 'Ending'),
         translate('SongsPlugin.VerseType', 'Other')]
+
     translated_tags = [name[0].lower() for name in translated_names]
 
     @staticmethod

=== modified file 'openlp/plugins/songs/lib/songimport.py'
--- openlp/plugins/songs/lib/songimport.py	2013-03-07 08:05:43 +0000
+++ openlp/plugins/songs/lib/songimport.py	2013-04-05 18:34:21 +0000
@@ -260,7 +260,10 @@
         elif int(verse_def[1:]) > self.verseCounts[verse_def[0]]:
             self.verseCounts[verse_def[0]] = int(verse_def[1:])
         self.verses.append([verse_def, verse_text.rstrip(), lang])
-        self.verseOrderListGenerated.append(verse_def)
+        # A verse_def refers to all verses with that name, adding it once adds every instance, so do not add if already
+        # used.
+        if verse_def not in self.verseOrderListGenerated:
+            self.verseOrderListGenerated.append(verse_def)
 
     def repeatVerse(self):
         """

=== modified file 'openlp/plugins/songs/lib/songshowplusimport.py'
--- openlp/plugins/songs/lib/songshowplusimport.py	2013-03-07 08:05:43 +0000
+++ openlp/plugins/songs/lib/songshowplusimport.py	2013-04-05 18:34:21 +0000
@@ -32,6 +32,7 @@
 """
 import os
 import logging
+import re
 import struct
 
 from openlp.core.ui.wizard import WizardStrings
@@ -44,43 +45,36 @@
 CCLI_NO = 5
 VERSE = 12
 CHORUS = 20
+BRIDGE = 24
 TOPIC = 29
 COMMENTS = 30
 VERSE_ORDER = 31
 SONG_BOOK = 35
 SONG_NUMBER = 36
 CUSTOM_VERSE = 37
-BRIDGE = 24
 
 log = logging.getLogger(__name__)
 
 class SongShowPlusImport(SongImport):
     """
-    The :class:`SongShowPlusImport` class provides the ability to import song
-    files from SongShow Plus.
+    The :class:`SongShowPlusImport` class provides the ability to import song files from SongShow Plus.
 
     **SongShow Plus Song File Format:**
 
     The SongShow Plus song file format is as follows:
 
-    * Each piece of data in the song file has some information that precedes
-    it.
+    * Each piece of data in the song file has some information that precedes it.
     * The general format of this data is as follows:
-    4 Bytes, forming a 32 bit number, a key if you will, this describes what
-    the data is (see blockKey below)
-    4 Bytes, forming a 32 bit number, which is the number of bytes until the
-    next block starts
+    4 Bytes, forming a 32 bit number, a key if you will, this describes what the data is (see blockKey below)
+    4 Bytes, forming a 32 bit number, which is the number of bytes until the next block starts
     1 Byte, which tells how many bytes follows
-    1 or 4 Bytes, describes how long the string is, if its 1 byte, the string
-    is less than 255
+    1 or 4 Bytes, describes how long the string is, if its 1 byte, the string is less than 255
     The next bytes are the actual data.
     The next block of data follows on.
 
-    This description does differ for verses. Which includes extra bytes
-    stating the verse type or number. In some cases a "custom" verse is used,
-    in that case, this block will in include 2 strings, with the associated
-    string length descriptors. The first string is the name of the verse, the
-    second is the verse content.
+    This description does differ for verses. Which includes extra bytes stating the verse type or number. In some cases
+    a "custom" verse is used, in that case, this block will in include 2 strings, with the associated string length
+    descriptors. The first string is the name of the verse, the second is the verse content.
 
     The file is ended with four null bytes.
 
@@ -88,8 +82,9 @@
 
     * .sbsong
     """
-    otherList = {}
-    otherCount = 0
+
+    other_count = 0
+    other_list = {}
 
     def __init__(self, manager, **kwargs):
         """
@@ -107,9 +102,9 @@
         for file in self.import_source:
             if self.stop_import_flag:
                 return
-            self.sspVerseOrderList = []
-            other_count = 0
-            other_list = {}
+            self.ssp_verse_order_list = []
+            self.other_count = 0
+            self.other_list = {}
             file_name = os.path.split(file)[1]
             self.import_wizard.increment_progress_bar(WizardStrings.ImportingType % file_name, 0)
             song_data = open(file, 'rb')
@@ -162,34 +157,37 @@
                 elif block_key == COMMENTS:
                     self.comments = unicode(data, u'cp1252')
                 elif block_key == VERSE_ORDER:
-                    verse_tag = self.toOpenLPVerseTag(data, True)
+                    verse_tag = self.to_openlp_verse_tag(data, True)
                     if verse_tag:
                         if not isinstance(verse_tag, unicode):
                             verse_tag = unicode(verse_tag, u'cp1252')
-                        self.sspVerseOrderList.append(verse_tag)
+                        self.ssp_verse_order_list.append(verse_tag)
                 elif block_key == SONG_BOOK:
                     self.songBookName = unicode(data, u'cp1252')
                 elif block_key == SONG_NUMBER:
                     self.songNumber = ord(data)
                 elif block_key == CUSTOM_VERSE:
-                    verse_tag = self.toOpenLPVerseTag(verse_name)
+                    verse_tag = self.to_openlp_verse_tag(verse_name)
                     self.addVerse(unicode(data, u'cp1252'), verse_tag)
                 else:
                     log.debug("Unrecognised blockKey: %s, data: %s" % (block_key, data))
                     song_data.seek(next_block_starts)
-            self.verseOrderList = self.sspVerseOrderList
+            self.verseOrderList = self.ssp_verse_order_list
             song_data.close()
             if not self.finish():
                 self.logError(file)
 
-    def toOpenLPVerseTag(self, verse_name, ignore_unique=False):
-        if verse_name.find(" ") != -1:
-            verse_parts = verse_name.split(" ")
-            verse_type = verse_parts[0]
-            verse_number = verse_parts[1]
+    def to_openlp_verse_tag(self, verse_name, ignore_unique=False):
+        # Have we got any digits? If so, verse number is everything from the digits to the end (OpenLP does not have
+        # concept of part verses, so just ignore any non integers on the end (including floats))
+        match = re.match(r'(\D*)(\d+)', verse_name)
+        if match:
+            verse_type = match.group(1).strip()
+            verse_number = match.group(2)
         else:
+            # otherwise we assume number 1 and take the whole prefix as the verse tag
             verse_type = verse_name
-            verse_number = "1"
+            verse_number = u'1'
         verse_type = verse_type.lower()
         if verse_type == "verse":
             verse_tag = VerseType.tags[VerseType.Verse]
@@ -200,11 +198,11 @@
         elif verse_type == "pre-chorus":
             verse_tag = VerseType.tags[VerseType.PreChorus]
         else:
-            if verse_name not in self.otherList:
+            if verse_name not in self.other_list:
                 if ignore_unique:
                     return None
-                self.otherCount += 1
-                self.otherList[verse_name] = str(self.otherCount)
+                self.other_count += 1
+                self.other_list[verse_name] = str(self.other_count)
             verse_tag = VerseType.tags[VerseType.Other]
-            verse_number = self.otherList[verse_name]
+            verse_number = self.other_list[verse_name]
         return verse_tag + verse_number

=== added file 'tests/functional/openlp_plugins/songs/test_songshowplusimport.py'
--- tests/functional/openlp_plugins/songs/test_songshowplusimport.py	1970-01-01 00:00:00 +0000
+++ tests/functional/openlp_plugins/songs/test_songshowplusimport.py	2013-04-05 18:34:21 +0000
@@ -0,0 +1,235 @@
+"""
+This module contains tests for the SongShow Plus song importer.
+"""
+
+import os
+from unittest import TestCase
+from mock import patch, MagicMock
+
+from openlp.plugins.songs.lib import VerseType
+from openlp.plugins.songs.lib.songshowplusimport import SongShowPlusImport
+
+TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), u'../../../resources/songshowplussongs'))
+SONG_TEST_DATA = {u'Amazing Grace.sbsong':
+        {u'title': u'Amazing Grace (Demonstration)',
+         u'authors': [u'John Newton', u'Edwin Excell', u'John P. Rees'],
+         u'copyright': u'Public Domain ',
+         u'ccli_number': 22025,
+         u'verses':
+             [(u'Amazing grace! How sweet the sound!\r\nThat saved a wretch like me!\r\n'
+               u'I once was lost, but now am found;\r\nWas blind, but now I see.', u'v1'),
+              (u'\'Twas grace that taught my heart to fear,\r\nAnd grace my fears relieved.\r\n'
+               u'How precious did that grace appear,\r\nThe hour I first believed.', u'v2'),
+              (u'The Lord has promised good to me,\r\nHis Word my hope secures.\r\n'
+               u'He will my shield and portion be\r\nAs long as life endures.', u'v3'),
+              (u'Thro\' many dangers, toils and snares\r\nI have already come.\r\n'
+               u'\'Tis grace that brought me safe thus far,\r\nAnd grace will lead me home.', u'v4'),
+              (u'When we\'ve been there ten thousand years,\r\nBright shining as the sun,\r\n'
+               u'We\'ve no less days to sing God\'s praise,\r\nThan when we first begun.', u'v5')],
+         u'topics': [u'Assurance', u'Grace', u'Praise', u'Salvation'],
+         u'comments': u'\n\n\n',
+         u'song_book_name': u'Demonstration Songs',
+         u'song_number': 0,
+         u'verse_order_list': []},
+    u'Beautiful Garden Of Prayer.sbsong':
+        {u'title': u'Beautiful Garden Of Prayer (Demonstration)',
+        u'authors': [u'Eleanor Allen Schroll', u'James H. Fillmore'],
+        u'copyright': u'Public Domain ',
+        u'ccli_number': 60252,
+        u'verses':
+           [(u'There\'s a garden where Jesus is waiting,\r\nThere\'s a place that is wondrously fair.\r\n'
+             u'For it glows with the light of His presence,\r\n\'Tis the beautiful garden of prayer.', u'v1'),
+            (u'There\'s a garden where Jesus is waiting,\r\nAnd I go with my burden and care.\r\n'
+             u'Just to learn from His lips, words of comfort,\r\nIn the beautiful garden of prayer.', u'v2'),
+            (u'There\'s a garden where Jesus is waiting,\r\nAnd He bids you to come meet Him there,\r\n'
+             u'Just to bow and receive a new blessing,\r\nIn the beautiful garden of prayer.', u'v3'),
+            (u'O the beautiful garden, the garden of prayer,\r\nO the beautiful garden of prayer.\r\n'
+             u'There my Savior awaits, and He opens the gates\r\nTo the beautiful garden of prayer.', u'c1')],
+        u'topics': [u'Devotion', u'Prayer'],
+        u'comments': u'',
+        u'song_book_name': u'',
+        u'song_number': 0,
+        u'verse_order_list': []}}
+
+
+class TestSongShowPlusImport(TestCase):
+    """
+    Test the functions in the :mod:`songshowplusimport` module.
+    """
+    def create_importer_test(self):
+        """
+        Test creating an instance of the SongShow Plus file importer
+        """
+        # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+        with patch(u'openlp.plugins.songs.lib.songshowplusimport.SongImport'):
+            mocked_manager = MagicMock()
+
+            # WHEN: An importer object is created
+            importer = SongShowPlusImport(mocked_manager)
+
+            # THEN: The importer object should not be None
+            self.assertIsNotNone(importer, u'Import should not be none')
+
+    def invalid_import_source_test(self):
+        """
+        Test SongShowPlusImport.doImport handles different invalid import_source values
+        """
+        # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+        with patch(u'openlp.plugins.songs.lib.songshowplusimport.SongImport'):
+            mocked_manager = MagicMock()
+            mocked_import_wizard = MagicMock()
+            importer = SongShowPlusImport(mocked_manager)
+            importer.import_wizard = mocked_import_wizard
+            importer.stop_import_flag = True
+
+            # WHEN: Import source is not a list
+            for source in [u'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(), u'doImport should return None when import_source is not a list')
+                self.assertEquals(mocked_import_wizard.progress_bar.setMaximum.called, False,
+                                  u'setMaxium on import_wizard.progress_bar should not have been called')
+
+    def valid_import_source_test(self):
+        """
+        Test SongShowPlusImport.doImport handles different invalid import_source values
+        """
+        # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+        with patch(u'openlp.plugins.songs.lib.songshowplusimport.SongImport'):
+            mocked_manager = MagicMock()
+            mocked_import_wizard = MagicMock()
+            importer = SongShowPlusImport(mocked_manager)
+            importer.import_wizard = mocked_import_wizard
+            importer.stop_import_flag = True
+
+            # WHEN: Import source is a list
+            importer.import_source = [u'List', u'of', u'files']
+
+            # THEN: doImport should return none and the progress bar setMaximum should be called with the length of
+            #       import_source.
+            self.assertIsNone(importer.doImport(),
+                u'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 to_openlp_verse_tag_test(self):
+        """
+        Test to_openlp_verse_tag method by simulating adding a verse
+        """
+        # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+        with patch(u'openlp.plugins.songs.lib.songshowplusimport.SongImport'):
+            mocked_manager = MagicMock()
+            importer = SongShowPlusImport(mocked_manager)
+
+            # WHEN: Supplied with the following arguments replicating verses being added
+            test_values = [(u'Verse 1', VerseType.tags[VerseType.Verse] + u'1'),
+                (u'Verse 2', VerseType.tags[VerseType.Verse] + u'2'),
+                (u'verse1', VerseType.tags[VerseType.Verse] + u'1'),
+                (u'Verse', VerseType.tags[VerseType.Verse] + u'1'),
+                (u'Verse1', VerseType.tags[VerseType.Verse] + u'1'),
+                (u'chorus 1', VerseType.tags[VerseType.Chorus] + u'1'),
+                (u'bridge 1', VerseType.tags[VerseType.Bridge] + u'1'),
+                (u'pre-chorus 1', VerseType.tags[VerseType.PreChorus] + u'1'),
+                (u'different 1', VerseType.tags[VerseType.Other] + u'1'),
+                (u'random 1', VerseType.tags[VerseType.Other] + u'2')]
+
+            # THEN: The returned value should should correlate with the input arguments
+            for original_tag, openlp_tag in test_values:
+                self.assertEquals(importer.to_openlp_verse_tag(original_tag), openlp_tag,
+                    u'SongShowPlusImport.to_openlp_verse_tag should return "%s" when called with "%s"'
+                    % (openlp_tag, original_tag))
+
+    def to_openlp_verse_tag_verse_order_test(self):
+        """
+        Test to_openlp_verse_tag method by simulating adding a verse to the verse order
+        """
+        # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+        with patch(u'openlp.plugins.songs.lib.songshowplusimport.SongImport'):
+            mocked_manager = MagicMock()
+            importer = SongShowPlusImport(mocked_manager)
+
+            # WHEN: Supplied with the following arguments replicating a verse order being added
+            test_values = [(u'Verse 1', VerseType.tags[VerseType.Verse] + u'1'),
+                (u'Verse 2', VerseType.tags[VerseType.Verse] + u'2'),
+                (u'verse1', VerseType.tags[VerseType.Verse] + u'1'),
+                (u'Verse', VerseType.tags[VerseType.Verse] + u'1'),
+                (u'Verse1', VerseType.tags[VerseType.Verse] + u'1'),
+                (u'chorus 1', VerseType.tags[VerseType.Chorus] + u'1'),
+                (u'bridge 1', VerseType.tags[VerseType.Bridge] + u'1'),
+                (u'pre-chorus 1', VerseType.tags[VerseType.PreChorus] + u'1'),
+                (u'different 1', VerseType.tags[VerseType.Other] + u'1'),
+                (u'random 1', VerseType.tags[VerseType.Other] + u'2'),
+                (u'unused 2', None)]
+
+            # THEN: The returned value should should correlate with the input arguments
+            for original_tag, openlp_tag in test_values:
+                self.assertEquals(importer.to_openlp_verse_tag(original_tag, ignore_unique=True), openlp_tag,
+                    u'SongShowPlusImport.to_openlp_verse_tag should return "%s" when called with "%s"'
+                    % (openlp_tag, original_tag))
+
+    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(u'openlp.plugins.songs.lib.songshowplusimport.SongImport'):
+            for song_file in SONG_TEST_DATA:
+                mocked_manager = MagicMock()
+                mocked_import_wizard = MagicMock()
+                mocked_parse_author = MagicMock()
+                mocked_add_copyright = MagicMock()
+                mocked_add_verse = MagicMock()
+                mocked_finish = MagicMock()
+                mocked_finish.return_value = True
+                importer = SongShowPlusImport(mocked_manager)
+                importer.import_wizard = mocked_import_wizard
+                importer.stop_import_flag = False
+                importer.parse_author = mocked_parse_author
+                importer.addCopyright = mocked_add_copyright
+                importer.addVerse = mocked_add_verse
+                importer.finish = mocked_finish
+                importer.topics = []
+
+                # WHEN: Importing each file
+                importer.import_source = [os.path.join(TEST_PATH, song_file)]
+                title = SONG_TEST_DATA[song_file][u'title']
+                author_calls = SONG_TEST_DATA[song_file][u'authors']
+                song_copyright = SONG_TEST_DATA[song_file][u'copyright']
+                ccli_number = SONG_TEST_DATA[song_file][u'ccli_number']
+                add_verse_calls = SONG_TEST_DATA[song_file][u'verses']
+                topics = SONG_TEST_DATA[song_file][u'topics']
+                comments = SONG_TEST_DATA[song_file][u'comments']
+                song_book_name = SONG_TEST_DATA[song_file][u'song_book_name']
+                song_number = SONG_TEST_DATA[song_file][u'song_number']
+                verse_order_list = SONG_TEST_DATA[song_file][u'verse_order_list']
+
+                # THEN: doImport should return none, the song data should be as expected, and finish should have been
+                #       called.
+                self.assertIsNone(importer.doImport(), u'doImport should return None when it has completed')
+                self.assertEquals(importer.title, title, u'title for %s should be "%s"' % (song_file, title))
+                for author in author_calls:
+                    mocked_parse_author.assert_any_call(author)
+                if song_copyright:
+                    mocked_add_copyright.assert_called_with(song_copyright)
+                if ccli_number:
+                    self.assertEquals(importer.ccliNumber, ccli_number, u'ccliNumber for %s should be %s'
+                        % (song_file, ccli_number))
+                for verse_text, verse_tag in add_verse_calls:
+                    mocked_add_verse.assert_any_call(verse_text, verse_tag)
+                if topics:
+                    self.assertEquals(importer.topics, topics, u'topics for %s should be %s' % (song_file, topics))
+                if comments:
+                    self.assertEquals(importer.comments, comments, u'comments for %s should be "%s"'
+                        % (song_file, comments))
+                if song_book_name:
+                    self.assertEquals(importer.songBookName, song_book_name, u'songBookName for %s should be "%s"'
+                        % (song_file, song_book_name))
+                if song_number:
+                    self.assertEquals(importer.songNumber, song_number, u'songNumber for %s should be %s'
+                        % (song_file, song_number))
+                if verse_order_list:
+                    self.assertEquals(importer.verseOrderList, [], u'verseOrderList for %s should be %s'
+                        % (song_file, verse_order_list))
+                mocked_finish.assert_called_with()

=== added directory 'tests/resources/songshowplussongs'
=== added file 'tests/resources/songshowplussongs/Amazing Grace.sbsong'
Binary files tests/resources/songshowplussongs/Amazing Grace.sbsong	1970-01-01 00:00:00 +0000 and tests/resources/songshowplussongs/Amazing Grace.sbsong	2013-04-05 18:34:21 +0000 differ
=== added file 'tests/resources/songshowplussongs/Beautiful Garden Of Prayer.sbsong'
Binary files tests/resources/songshowplussongs/Beautiful Garden Of Prayer.sbsong	1970-01-01 00:00:00 +0000 and tests/resources/songshowplussongs/Beautiful Garden Of Prayer.sbsong	2013-04-05 18:34:21 +0000 differ