openlp-core team mailing list archive

[Tim Bentley <tim.bentley@xxxxxxxxx>]

Review: Needs Fixing



Diff comments:

> === modified file 'openlp/plugins/songs/lib/importer.py'
> --- openlp/plugins/songs/lib/importer.py	2014-12-31 10:58:13 +0000
> +++ openlp/plugins/songs/lib/importer.py	2015-01-10 18:16:33 +0000
> @@ -216,7 +216,7 @@
>              'class': CCLIFileImport,
>              'name': 'CCLI/SongSelect',
>              'prefix': 'ccli',
> -            'filter': '%s (*.usr *.txt)' % translate('SongsPlugin.ImportWizardForm', 'CCLI SongSelect Files')
> +            'filter': '%s (*.usr *.txt *.bin)' % translate('SongsPlugin.ImportWizardForm', 'CCLI SongSelect Files')
>          },
>          DreamBeam: {
>              'class': DreamBeamImport,
> 
> === modified file 'openlp/plugins/songs/lib/importers/cclifile.py'
> --- openlp/plugins/songs/lib/importers/cclifile.py	2014-12-31 10:58:13 +0000
> +++ openlp/plugins/songs/lib/importers/cclifile.py	2015-01-10 18:16:33 +0000
> @@ -42,7 +42,10 @@
>  class CCLIFileImport(SongImport):
>      """
>      The :class:`CCLIFileImport` class provides OpenLP with the ability to import CCLI SongSelect song files in
> -    both .txt and .usr formats. See `<http://www.ccli.com/>`_ for more details.
> +    TEXT and USR formats. See `<http://www.ccli.com/>`_ for more details.
> +
> +    NOTE: Sometime before 2015, CCLI/SongSelect has changed the USR filename to a .bin extension; however,
> +     the file format remained the same as the .usr file format.
>      """
>  
>      def __init__(self, manager, **kwargs):
> @@ -56,7 +59,7 @@
>  
>      def do_import(self):
>          """
> -        Import either a ``.usr`` or a ``.txt`` SongSelect file.
> +        Import either a USR or TEXT SongSelect file.
>          """
>          log.debug('Starting CCLI File Import')
>          self.import_wizard.progress_bar.setMaximum(len(self.import_source))
> @@ -79,12 +82,12 @@
>                  lines = infile.readlines()
>                  infile.close()
>                  ext = os.path.splitext(filename)[1]
> -                if ext.lower() == '.usr':
> -                    log.info('SongSelect .usr format file found: %s', filename)
> +                if ext.lower() == '.usr' or ext.lower() == '.bin':
> +                    log.info('SongSelect USR format file found: %s', filename)
>                      if not self.do_import_usr_file(lines):
>                          self.log_error(filename)
>                  elif ext.lower() == '.txt':
> -                    log.info('SongSelect .txt format file found: %s', filename)
> +                    log.info('SongSelect TEXT format file found: %s', filename)
>                      if not self.do_import_txt_file(lines):
>                          self.log_error(filename)
>                  else:
> @@ -99,7 +102,7 @@
>          The :func:`doImport_usr_file` method provides OpenLP with the ability
>          to import CCLI SongSelect songs in *USR* file format.
>  
> -        **SongSelect .usr file format**
> +        **SongSelect USR file format**
>  
>          ``[File]``
>              USR file format first line
> @@ -160,7 +163,7 @@
>                      self.ccli_number = ccli[4:].strip()
>                  else:
>                      self.ccli_number = ccli[3:].strip()
> -            if line.startswith('Title='):

Why change when the previous if can change the value of line.

> +            elif line.startswith('Title='):
>                  self.title = line[6:].strip()
>              elif line.startswith('Author='):
>                  song_author = line[7:].strip()
> 
> === modified file 'tests/functional/openlp_plugins/songs/test_songselect.py'
> --- tests/functional/openlp_plugins/songs/test_songselect.py	2014-12-31 10:58:13 +0000
> +++ tests/functional/openlp_plugins/songs/test_songselect.py	2015-01-10 18:16:33 +0000
> @@ -29,16 +29,18 @@
>  """
>  This module contains tests for the CCLI SongSelect importer.
>  """
> +import os
> +
>  from unittest import TestCase
>  from urllib.error import URLError
> +from tests.functional import MagicMock, patch, call
> +from tests.helpers.testmixin import TestMixin
> +
>  from openlp.core import Registry
>  from openlp.plugins.songs.forms.songselectform import SongSelectForm
> -from openlp.plugins.songs.lib import Author, Song
> -
> +from openlp.plugins.songs.lib import Author, Song, VerseType
>  from openlp.plugins.songs.lib.songselect import SongSelectImport, LOGOUT_URL, BASE_URL
> -
> -from tests.functional import MagicMock, patch, call
> -from tests.helpers.testmixin import TestMixin
> +from openlp.plugins.songs.lib.importers.cclifile import CCLIFileImport
>  
>  
>  class TestSongSelectImport(TestCase, TestMixin):
> @@ -467,3 +469,79 @@
>                  mocked_critical.assert_called_with(ssform, 'Error Logging In', 'There was a problem logging in, '
>                                                                                 'perhaps your username or password is '
>                                                                                 'incorrect?')
> +
> +
> +class TestSongSelectFileImport(TestCase, TestMixin):
> +    """
> +    Test SongSelect file import
> +    """
> +    def setUp(self):
> +        """
> +        Initial setups
> +        :return:
> +        """
> +        Registry.create()
> +        test_song_name = 'TestSong'
> +        self.file_name = os.path.join('tests', 'resources', 'songselect', test_song_name)
> +        self.title = 'Test Song'
> +        self.ccli_number = '0000000'
> +        self.authors = ['Author One', 'Author Two']
> +        self.topics = ['Adoration', 'Praise']
> +
> +    def tearDown(self):
> +        """
> +        Test cleanups
> +        :return:
> +        """
> +        pass
> +
> +    def songselect_import_usr_file_test(self):
> +        """
> +        Verify import SongSelect USR file parses file properly
> +        :return:
> +        """
> +        # GIVEN: Text file to import and mocks
> +        copyright = '2011 OpenLP Programmer One (Admin. by OpenLP One) | ' \
> +                    'Openlp Programmer Two (Admin. by OpenLP Two)'
> +        verses = [
> +            ['v1', 'Line One Verse One\nLine Two Verse One\nLine Three Verse One\nLine Four Verse One', None],
> +            ['v2', 'Line One Verse Two\nLine Two Verse Two\nLine Three Verse Two\nLine Four Verse Two', None]
> +        ]
> +
> +        song_import = CCLIFileImport(manager=None, filename=['{}.bin'.format(self.file_name)], )
> +        with patch.object(song_import, 'import_wizard'), patch.object(song_import, 'finish'):
> +
> +            # WHEN: We call the song importer
> +            song_import.do_import()
> +            print(song_import.verses)
> +            # THEN: Song values should be equal to test values in setUp
> +            self.assertEquals(song_import.title, self.title, 'Song title should match')
> +            self.assertEquals(song_import.ccli_number, self.ccli_number, 'CCLI Song Number should match')
> +            self.assertEquals(song_import.authors, self.authors, 'Author(s) should match')
> +            self.assertEquals(song_import.copyright, self.copyright_usr, 'Copyright should match')
> +            self.assertEquals(song_import.topics, self.topics, 'Theme(s) should match')
> +            self.assertEquals(song_import.verses, self.verses, 'Verses should match with test verses')
> +
> +    def songselect_import_usr_file_test(self):
> +        """
> +        Verify import SongSelect USR file parses file properly
> +        :return:
> +        """
> +        # GIVEN: Text file to import and mocks
> +        copyright = '© 2011 OpenLP Programmer One (Admin. by OpenLP One)'
> +        verses = [
> +            ['v1', 'Line One Verse One\r\nLine Two Verse One\r\nLine Three Verse One\r\nLine Four Verse One', None],
> +            ['v2', 'Line One Verse Two\r\nLine Two Verse Two\r\nLine Three Verse Two\r\nLine Four Verse Two', None]
> +        ]
> +        song_import = CCLIFileImport(manager=None, filename=['{}.txt'.format(self.file_name)], )
> +        with patch.object(song_import, 'import_wizard'), patch.object(song_import, 'finish'):
> +
> +            # WHEN: We call the song importer
> +            song_import.do_import()
> +
> +            # THEN: Song values should be equal to test values in setUp
> +            self.assertEquals(song_import.title, self.title, 'Song title should match')
> +            self.assertEquals(song_import.ccli_number, self.ccli_number, 'CCLI Song Number should match')
> +            self.assertEquals(song_import.authors, self.authors, 'Author(s) should match')
> +            self.assertEquals(song_import.copyright, copyright, 'Copyright should match')
> +            self.assertEquals(song_import.verses, verses, 'Verses should match with test verses')
> 
> === added directory 'tests/resources/songselect'
> === added file 'tests/resources/songselect/TestSong.bin'
> --- tests/resources/songselect/TestSong.bin	1970-01-01 00:00:00 +0000
> +++ tests/resources/songselect/TestSong.bin	2015-01-10 18:16:33 +0000
> @@ -0,0 +1,12 @@
> +[File]
> +Type=SongSelect Import File
> +Version=3.0
> +[S A0000000]
> +Title=Test Song
> +Author=Author One | Author Two
> +Copyright=2011 OpenLP Programmer One (Admin. by OpenLP One) | Openlp Programmer Two (Admin. by OpenLP Two)
> +Admin=OpenLP One
> +Themes=Adoration/tPraise
> +Keys=G
> +Fields=Verse 1/tVerse 2
> +Words=Line One Verse One/nLine Two Verse One/nLine Three Verse One/nLine Four Verse One/n/tLine One Verse Two/nLine Two Verse Two/nLine Three Verse Two/nLine Four Verse Two/n/t
> 
> === added file 'tests/resources/songselect/TestSong.txt'
> --- tests/resources/songselect/TestSong.txt	1970-01-01 00:00:00 +0000
> +++ tests/resources/songselect/TestSong.txt	2015-01-10 18:16:33 +0000
> @@ -0,0 +1,22 @@
> +Test Song
> +
> +
> +Verse 1
> +Line One Verse One
> +Line Two Verse One
> +Line Three Verse One
> +Line Four Verse One
> +
> +
> +Verse 2
> +Line One Verse Two
> +Line Two Verse Two
> +Line Three Verse Two
> +Line Four Verse Two
> +
> +CCLI Song # 0000000
> +Author One | Author Two
> +© 2011 OpenLP Programmer One (Admin. by OpenLP One)
> +OpenLP Programmer Two (Admin. by OpenLP Two)
> +For use solely with the OpenLP Song Test Suite.  All rights reserved. http://openlp.org
> +CCLI License # 00000
> 


-- 
https://code.launchpad.net/~alisonken1/openlp/ccli-usr-bin-fix/+merge/246037
Your team OpenLP Core is subscribed to branch lp:openlp.