Fix some issues I ran into when importing our OpenSong 'database'

openlp/plugins/songs/lib/__init__.py

@@ -278,7 +278,7 @@ class VerseType(object):
             if verse_index is None:
                 verse_index = VerseType.from_string(verse_name, default)
         elif len(verse_name) == 1:
-            verse_index = VerseType.from_translated_tag(verse_name, None)
+            verse_index = VerseType.from_translated_tag(verse_name, default)
             if verse_index is None:
                 verse_index = VerseType.from_tag(verse_name, default)
         else:

openlp/plugins/songs/lib/opensongimport.py

@@ -162,8 +162,7 @@ class OpenSongImport(SongImport):
             if attr in fields:
                 ustring = str(root.__getattr__(attr))
                 if isinstance(fn_or_string, str):
-                    match = re.match('(\D*)(\d+.*)', ustring)
-                    if match:
+                    if attr in ['ccli']:
                         setattr(self, fn_or_string, int(ustring))
                     else:
                         setattr(self, fn_or_string, ustring)
@@ -261,7 +260,14 @@ class OpenSongImport(SongImport):
             verse_def = '%s%s' % (verse_tag, verse_num[:length])
             verse_joints[verse_def] = '%s\n[---]\n%s' % (verse_joints[verse_def], lines) \
                 if verse_def in verse_joints else lines
-        for verse_def, lines in verse_joints.items():
+        # Parsing the dictionary produces the elements in a non-intuitive order.  While it "works", it's not a
+        # natural layout should the user come back to edit the song.  Instead we sort by the verse type, so that we
+        # get all the verses in order (v1, v2, ...), then the chorus(es), bridge(s), pre-chorus(es) etc.  We use a
+        # tuple for the key, since tuples naturally sort in this manner.
+        verse_defs = sorted(verse_joints.keys(),
+                            key=lambda verse_def: (VerseType.from_tag(verse_def[0]), int(verse_def[1:])))
+        for verse_def in verse_defs:
+            lines = verse_joints[verse_def]
             self.add_verse(lines, verse_def)
         if not self.verses:
             self.add_verse('')
@@ -282,6 +288,8 @@ class OpenSongImport(SongImport):
                     # Assume it's no.1 if there are no digits
                     verse_tag = verse_def
                     verse_num = '1'
+                verse_index = VerseType.from_loose_input(verse_tag)
+                verse_tag = VerseType.tags[verse_index]
                 verse_def = '%s%s' % (verse_tag, verse_num)
                 if verse_num in verses.get(verse_tag, {}):
                     self.verse_order_list.append(verse_def)

openlp/plugins/songs/lib/songimport.py

@@ -202,7 +202,7 @@ class SongImport(QtCore.QObject):
         # Book name:'NRH' and
         # Song number: 231
         book_and_number = book_and_number.strip()
-        if book_and_number == '':
+        if not book_and_number:
             return
         book_and_number = book_and_number.replace('No.', ' ')
         if ' ' in book_and_number:
@@ -233,7 +233,7 @@ class SongImport(QtCore.QObject):
         """
         if self.comments.find(comment) >= 0:
             return
-        if comment != '':
+        if comment:
             self.comments += comment.strip() + '\n'
 
     def add_copyright(self, copyright):
@@ -242,7 +242,7 @@ class SongImport(QtCore.QObject):
         """
         if self.copyright.find(copyright) >= 0:
             return
-        if self.copyright != '':
+        if self.copyright:
             self.copyright += ' '
         self.copyright += copyright
 

tests/functional/openlp_plugins/songs/test_opensongimport.py

@@ -56,6 +56,8 @@ class TestOpenSongFileImport(SongImportTestHelper):
                          self.load_external_result_data(os.path.join(TEST_PATH, 'Amazing Grace.json')))
         self.file_import(os.path.join(TEST_PATH, 'Beautiful Garden Of Prayer'),
                          self.load_external_result_data(os.path.join(TEST_PATH, 'Beautiful Garden Of Prayer.json')))
+        self.file_import(os.path.join(TEST_PATH, 'One, Two, Three, Four, Five'),
+                         self.load_external_result_data(os.path.join(TEST_PATH, 'One, Two, Three, Four, Five.json')))
 
 
 class TestOpenSongImport(TestCase):

tests/helpers/songfileimport.py

@@ -33,7 +33,7 @@ song files from third party applications.
 import json
 from unittest import TestCase
 
-from tests.functional import patch, MagicMock
+from tests.functional import patch, MagicMock, call
 
 
 class SongImportTestHelper(TestCase):
@@ -118,8 +118,11 @@ class SongImportTestHelper(TestCase):
         if ccli_number:
             self.assertEqual(importer.ccli_number, ccli_number,
                              'ccli_number for %s should be %s' % (source_file_name, ccli_number))
+        expected_calls = []
         for verse_text, verse_tag in add_verse_calls:
             self.mocked_add_verse.assert_any_call(verse_text, verse_tag)
+            expected_calls.append(call(verse_text, verse_tag))
+        self.mocked_add_verse.assert_has_calls(expected_calls, any_order=False)
         if topics:
             self.assertEqual(importer.topics, topics, 'topics for %s should be %s' % (source_file_name, topics))
         if comments:

tests/resources/opensongsongs/Beautiful Garden Of Prayer

@@ -16,6 +16,7 @@
 ;Test breaks and newlines
 ;A single | on the end of a line adds an extra \n
 ;Blank lines are ignored, even with a space prefix
+;We also check that the chorus is added after the verses, despite the order in the file
 [V1]
  There's a garden where Jesus is waiting,
  
@@ -23,6 +24,14 @@
  For it glows with the light of His presence,|
  'Tis the beautiful garden of prayer.
 
+;A double || on a line adds a new slide
+[C]
+ O the beautiful garden, the garden of prayer,
+ O the beautiful garden of prayer.
+ There my Savior awaits, and He opens the gates
+ ||
+ To the beautiful garden of prayer.
+
 ;A double || on the end of a line adds a new slide
 [V2]
  There's a garden where Jesus is waiting,
@@ -37,14 +46,6 @@
  Just to bow and receive a new blessing,
  |
  In the beautiful garden of prayer.
-
-;A double || on a line adds a new slide
-[C]
- O the beautiful garden, the garden of prayer,
- O the beautiful garden of prayer.
- There my Savior awaits, and He opens the gates
- ||
- To the beautiful garden of prayer.
 </lyrics>
   <hymn_number>DS0</hymn_number>
   <key></key>

tests/resources/opensongsongs/One, Two, Three, Four, Five

@@ -0,0 +1,37 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<song>
+  <title>12345</title>
+  <author>Traditional</author>
+  <copyright>Public Domain </copyright>
+  <presentation>T</presentation>
+  <capo print="false"></capo>
+  <tempo></tempo>
+  <theme></theme>
+  <alttheme></alttheme>
+  <user1></user1>
+  <user2></user2>
+  <user3></user3>
+  <lyrics>
+;Test [T]ag element - should be turned into [o]ther
+;And lines beginning with numbers
+;And a title that contains only numeric characters
+;That isdiffernt to the filename
+;And most elements are empty
+[T]
+ 1, 2, 3, 4, 5,
+ Once I caught a fish alive.
+ 6, 7, 8, 9, 10,
+ Then I let it go again.
+ 
+ Why did you let it go?
+ Because it bit my finger so.
+ Which finger did it bite?
+ This little finger on my right.
+</lyrics>
+  <hymn_number></hymn_number>
+  <key></key>
+  <aka></aka>
+  <key_line></key_line>
+  <time_sig></time_sig>
+  <style index="default_style"></style>
+</song>

tests/resources/opensongsongs/One, Two, Three, Four, Five.json

@@ -0,0 +1,17 @@
+{
+    "authors": [
+        "Traditional"
+    ],
+    "comments": "",
+    "copyright": "Public Domain ",
+    "title": "12345",
+    "topics": [
+    ],
+    "verse_order_list": ["o1"],
+    "verses": [
+        [
+            "1, 2, 3, 4, 5,\nOnce I caught a fish alive.\n6, 7, 8, 9, 10,\nThen I let it go again.\nWhy did you let it go?\nBecause it bit my finger so.\nWhich finger did it bite?\nThis little finger on my right.",
+            "o1"
+        ]
+    ]
+}