openlp-core team mailing list archive

[Stefan Strasser <strada@xxxxxxxxxxx>]

Stefan Strasser has proposed merging lp:~strada/openlp/bug-1312629 into lp:openlp.

Requested reviews:
  Raoul Snyman (raoul-snyman)
Related bugs:
  Bug #1312629 in OpenLP: "Error on entering invalid character in verse-order"
  https://bugs.launchpad.net/openlp/+bug/1312629

For more details, see:
https://code.launchpad.net/~strada/openlp/bug-1312629/+merge/217327

fixed bug #1312629 Error on entering invalid character in verse-order

- entering a non-existing abbreviation in the verse-order field caused an exception because an if-statement was called which tried to compare two different data-types
- usually the comparison is int-int, but in this situation the function from_translated_tag() is being called with None as fallback-type instead of a valid VerseType (which makes sense imho) which leads to a comparison int-None
- now changed from_tag() and from_translated_tag() to work well even if a non-int VerseType-fallback is provided on calling the function (changed also from_tag() even if not directly involved, but this is nearly an identical function)
- added tests for this case (only for from_tag(), until now no tests exist for from_translated_tag(), I don't know if it would make really sense to copy/paste all tests for from_tag() to exist also for from_translated_tag() because these functions are nearly identical
-- 
https://code.launchpad.net/~strada/openlp/bug-1312629/+merge/217327
Your team OpenLP Core is subscribed to branch lp:openlp.

=== modified file 'openlp/plugins/songs/lib/__init__.py'
--- openlp/plugins/songs/lib/__init__.py	2014-03-21 21:38:08 +0000
+++ openlp/plugins/songs/lib/__init__.py	2014-04-26 05:09:32 +0000
@@ -206,14 +206,14 @@
         Return the VerseType for a given tag
 
         :param verse_tag: The string to return a VerseType for
-        :param default: Default return value if no matching tag is found
+        :param default: Default return value if no matching tag is found (a valid VerseType or None)
         :return: A VerseType of the tag
         """
         verse_tag = verse_tag[0].lower()
         for num, tag in enumerate(VerseType.tags):
             if verse_tag == tag:
                 return num
-        if len(VerseType.names) > default:
+        if default in range(0, len(VerseType.names)) or default is None:
             return default
         else:
             return VerseType.Other
@@ -231,7 +231,7 @@
         for num, tag in enumerate(VerseType.translated_tags):
             if verse_tag == tag:
                 return num
-        if len(VerseType.names) > default:
+        if default in range(0, len(VerseType.names)) or default is None:
             return default
         else:
             return VerseType.Other

=== modified file 'tests/functional/openlp_plugins/songs/test_lib.py'
--- tests/functional/openlp_plugins/songs/test_lib.py	2014-04-16 19:56:54 +0000
+++ tests/functional/openlp_plugins/songs/test_lib.py	2014-04-26 05:09:32 +0000
@@ -445,6 +445,20 @@
             # THEN: The result should be VerseType.Chorus
             self.assertEqual(result, VerseType.Chorus, 'The result should be VerseType.Chorus, but was "%s"' % result)
 
+    def from_tag_with_invalid_intdefault_test(self):
+        """
+        Test that the from_tag() method returns a sane default when passed an invalid tag and an invalid int default.
+        """
+        # GIVEN: A mocked out translate() function that just returns what it was given
+        with patch('openlp.plugins.songs.lib.translate') as mocked_translate:
+            mocked_translate.side_effect = lambda x, y: y
+
+            # WHEN: We run the from_tag() method with an invalid verse type, we get the specified default back
+            result = VerseType.from_tag('m', 29)
+
+            # THEN: The result should be VerseType.Other
+            self.assertEqual(result, VerseType.Other, 'The result should be VerseType.Other, but was "%s"' % result)
+
     def from_tag_with_invalid_default_test(self):
         """
         Test that the from_tag() method returns a sane default when passed an invalid tag and an invalid default.
@@ -454,7 +468,21 @@
             mocked_translate.side_effect = lambda x, y: y
 
             # WHEN: We run the from_tag() method with an invalid verse type, we get the specified default back
-            result = VerseType.from_tag('m', 29)
+            result = VerseType.from_tag('@', 'asdf')
 
             # THEN: The result should be VerseType.Other
             self.assertEqual(result, VerseType.Other, 'The result should be VerseType.Other, but was "%s"' % result)
+
+    def from_tag_with_none_default_test(self):
+        """
+        Test that the from_tag() method returns a sane default when passed an invalid tag and None as default.
+        """
+        # GIVEN: A mocked out translate() function that just returns what it was given
+        with patch('openlp.plugins.songs.lib.translate') as mocked_translate:
+            mocked_translate.side_effect = lambda x, y: y
+
+            # WHEN: We run the from_tag() method with an invalid verse type, we get the specified default back
+            result = VerseType.from_tag('m', None)
+
+            # THEN: The result should be None
+            self.assertIsNone(result, 'The result should be None, but was "%s"' % result)