Is there a reason why the original 'if' statement on the second check
is not consistent with the purpose of the rest of the checks?
On Sat, Jan 10, 2015 at 11:40 AM, Tim Bentley <tim.bent...@gmail.com> wrote:
> 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
> You are the owner of lp:~alisonken1/openlp/ccli-usr-bin-fix.
--
- Ken
Registered Linux user 296561
Slackin' since 1993
Slackware Linux (http://www.slackware.com)
https://code.launchpad.net/~alisonken1/openlp/ccli-usr-bin-fix/+merge/246037
Your team OpenLP Core is subscribed to branch lp:openlp.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
