openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #20125
[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/156075
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
--
https://code.launchpad.net/~phill-ridout/openlp/bug1125956/+merge/156075
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-28 21:19:23 +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-28 21:19:23 +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-28 21:19:23 +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(u'(\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-28 21:19:23 +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-28 21:19:23 +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-28 21:19:23 +0000 differ
Follow ups
