openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #20255
[Merge] lp:~phill-ridout/openlp/bug1125956 into lp:openlp
Phill has proposed merging lp:~phill-ridout/openlp/bug1125956 into lp:openlp.
Requested reviews:
Andreas Preikschat (googol)
Raoul Snyman (raoul-snyman)
Related bugs:
Bug #1125956 in OpenLP: "SongShowPlus importer does not handle verse labels such as 1A"
https://bugs.launchpad.net/openlp/+bug/1125956
For more details, see:
https://code.launchpad.net/~phill-ridout/openlp/bug1125956/+merge/156291
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
--
https://code.launchpad.net/~phill-ridout/openlp/bug1125956/+merge/156291
Your team OpenLP Core is subscribed to branch lp:openlp.
=== 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-03-31 10:44:22 +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-03-31 10:44:22 +0000
@@ -260,7 +260,8 @@
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)
+ 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-03-31 10:44:22 +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-03-31 10:44:22 +0000
@@ -0,0 +1,241 @@
+"""
+This module contains tests for the SongShow Plus song importer.
+"""
+
+import os
+from unittest import TestCase
+from mock import call, 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 import_source_test(self):
+ """
+ Test SongShowPlusImport.doImport handles different 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 string
+ importer.import_source = u'not a list'
+
+ # 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')
+
+ # 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 an int
+ importer.import_source = 0
+
+ # 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')
+
+ # 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
+ """
+ # 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))
+
+ # 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']
+ parse_author_calls = [call(author) for author in 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 = \
+ [call(verse_text, verse_tag) for verse_text, verse_tag in 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))
+ mocked_parse_author.assert_has_calls(parse_author_calls)
+ 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))
+ mocked_add_verse.assert_has_calls(add_verse_calls)
+ 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-03-31 10:44:22 +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-03-31 10:44:22 +0000 differ
