openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #29417
[Merge] lp:~raoul-snyman/openlp/bug-1557514-2.4 into lp:openlp/2.4
Raoul Snyman has proposed merging lp:~raoul-snyman/openlp/bug-1557514-2.4 into lp:openlp/2.4.
Requested reviews:
OpenLP Core (openlp-core)
Related bugs:
Bug #1557514 in OpenLP: "Error when importing OLP Song Database"
https://bugs.launchpad.net/openlp/+bug/1557514
For more details, see:
https://code.launchpad.net/~raoul-snyman/openlp/bug-1557514-2.4/+merge/293160
Fix bug #1557514 by auto-detecting the columns of the tables in the songs database
Add this to your merge proposal:
--------------------------------
lp:~raoul-snyman/openlp/bug-1557514-2.4 (revision 2625)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1502/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1413/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1351/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1147/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/738/
[FAILURE] https://ci.openlp.io/job/Branch-05a-Code_Analysis/805/
Stopping after failure
--
Your team OpenLP Core is requested to review the proposed merge of lp:~raoul-snyman/openlp/bug-1557514-2.4 into lp:openlp/2.4.
=== modified file '.bzrignore'
--- .bzrignore 2016-02-11 21:05:41 +0000
+++ .bzrignore 2016-04-27 18:51:46 +0000
@@ -45,3 +45,4 @@
*.kdev4
coverage
tags
+output
=== modified file 'openlp/plugins/songs/lib/db.py'
--- openlp/plugins/songs/lib/db.py 2016-01-09 15:23:11 +0000
+++ openlp/plugins/songs/lib/db.py 2016-04-27 18:51:46 +0000
@@ -383,7 +383,7 @@
# Use lazy='joined' to always load authors when the song is fetched from the database (bug 1366198)
'authors': relation(Author, secondary=authors_songs_table, viewonly=True, lazy='joined'),
'media_files': relation(MediaFile, backref='songs', order_by=media_files_table.c.weight),
- 'songbook_entries': relation(SongBookEntry, backref='song', cascade="all, delete-orphan"),
+ 'songbook_entries': relation(SongBookEntry, backref='song', cascade='all, delete-orphan'),
'topics': relation(Topic, backref='songs', secondary=songs_topics_table)
})
mapper(Topic, topics_table)
=== modified file 'openlp/plugins/songs/lib/importers/openlp.py'
--- openlp/plugins/songs/lib/importers/openlp.py 2015-12-31 22:46:06 +0000
+++ openlp/plugins/songs/lib/importers/openlp.py 2016-04-27 18:51:46 +0000
@@ -51,7 +51,7 @@
:param manager: The song manager for the running OpenLP installation.
:param kwargs: The database providing the data to import.
"""
- SongImport.__init__(self, manager, **kwargs)
+ super(OpenLPSongImport, self).__init__(manager, **kwargs)
self.source_session = None
def do_import(self, progress_dialog=None):
@@ -63,49 +63,61 @@
class OldAuthor(BaseModel):
"""
- Author model
+ Maps to the authors table
"""
pass
class OldBook(BaseModel):
"""
- Book model
+ Maps to the songbooks table
"""
pass
class OldMediaFile(BaseModel):
"""
- MediaFile model
+ Maps to the media_files table
"""
pass
class OldSong(BaseModel):
"""
- Song model
+ Maps to the songs table
"""
pass
class OldTopic(BaseModel):
"""
- Topic model
+ Maps to the topics table
+ """
+ pass
+
+ class OldSongBookEntry(BaseModel):
+ """
+ Maps to the songs_songbooks table
"""
pass
# Check the file type
- if not self.import_source.endswith('.sqlite'):
+ if not isinstance(self.import_source, str) or not self.import_source.endswith('.sqlite'):
self.log_error(self.import_source, translate('SongsPlugin.OpenLPSongImport',
'Not a valid OpenLP 2 song database.'))
return
self.import_source = 'sqlite:///%s' % self.import_source
- # Load the db file
+ # Load the db file and reflect it
engine = create_engine(self.import_source)
source_meta = MetaData()
source_meta.reflect(engine)
self.source_session = scoped_session(sessionmaker(bind=engine))
+ # Run some checks to see which version of the database we have
if 'media_files' in list(source_meta.tables.keys()):
has_media_files = True
else:
has_media_files = False
+ if 'songs_songbooks' in list(source_meta.tables.keys()):
+ has_songs_books = True
+ else:
+ has_songs_books = False
+ # Load up the tabls and map them out
source_authors_table = source_meta.tables['authors']
source_song_books_table = source_meta.tables['song_books']
source_songs_table = source_meta.tables['songs']
@@ -113,6 +125,7 @@
source_authors_songs_table = source_meta.tables['authors_songs']
source_songs_topics_table = source_meta.tables['songs_topics']
source_media_files_songs_table = None
+ # Set up media_files relations
if has_media_files:
source_media_files_table = source_meta.tables['media_files']
source_media_files_songs_table = source_meta.tables.get('media_files_songs')
@@ -120,9 +133,15 @@
class_mapper(OldMediaFile)
except UnmappedClassError:
mapper(OldMediaFile, source_media_files_table)
+ if has_songs_books:
+ source_songs_songbooks_table = source_meta.tables['songs_songbooks']
+ try:
+ class_mapper(OldSongBookEntry)
+ except UnmappedClassError:
+ mapper(OldSongBookEntry, source_songs_songbooks_table, properties={'songbook': relation(OldBook)})
+ # Set up the songs relationships
song_props = {
'authors': relation(OldAuthor, backref='songs', secondary=source_authors_songs_table),
- 'book': relation(OldBook, backref='songs'),
'topics': relation(OldTopic, backref='songs', secondary=source_songs_topics_table)
}
if has_media_files:
@@ -134,6 +153,11 @@
relation(OldMediaFile, backref='songs',
foreign_keys=[source_media_files_table.c.song_id],
primaryjoin=source_songs_table.c.id == source_media_files_table.c.song_id)
+ if has_songs_books:
+ song_props['songbook_entries'] = relation(OldSongBookEntry, backref='song', cascade='all, delete-orphan')
+ else:
+ song_props['book'] = relation(OldBook, backref='songs')
+ # Map the rest of the tables
try:
class_mapper(OldAuthor)
except UnmappedClassError:
@@ -163,44 +187,54 @@
old_titles = song.search_title.split('@')
if len(old_titles) > 1:
new_song.alternate_title = old_titles[1]
- # Values will be set when cleaning the song.
+ # Transfer the values to the new song object
new_song.search_title = ''
new_song.search_lyrics = ''
- new_song.song_number = song.song_number
new_song.lyrics = song.lyrics
new_song.verse_order = song.verse_order
new_song.copyright = song.copyright
new_song.comments = song.comments
new_song.theme_name = song.theme_name
new_song.ccli_number = song.ccli_number
+ if hasattr(song, 'song_number') and song.song_number:
+ new_song.song_number = song.song_number
+ # Find or create all the authors and add them to the new song object
for author in song.authors:
existing_author = self.manager.get_object_filtered(Author, Author.display_name == author.display_name)
- if existing_author is None:
+ if not existing_author:
existing_author = Author.populate(
first_name=author.first_name,
last_name=author.last_name,
display_name=author.display_name)
new_song.add_author(existing_author)
- if song.book:
- existing_song_book = self.manager.get_object_filtered(Book, Book.name == song.book.name)
- if existing_song_book is None:
- existing_song_book = Book.populate(name=song.book.name, publisher=song.book.publisher)
- new_song.book = existing_song_book
+ # Find or create all the topics and add them to the new song object
if song.topics:
for topic in song.topics:
existing_topic = self.manager.get_object_filtered(Topic, Topic.name == topic.name)
- if existing_topic is None:
+ if not existing_topic:
existing_topic = Topic.populate(name=topic.name)
new_song.topics.append(existing_topic)
- if has_media_files:
- if song.media_files:
- for media_file in song.media_files:
- existing_media_file = self.manager.get_object_filtered(
- MediaFile, MediaFile.file_name == media_file.file_name)
- if existing_media_file:
- new_song.media_files.append(existing_media_file)
- else:
- new_song.media_files.append(MediaFile.populate(file_name=media_file.file_name))
+ # Find or create all the songbooks and add them to the new song object
+ if has_songs_books and song.songbook_entries:
+ for entry in song.songbook_entries:
+ existing_book = self.manager.get_object_filtered(Book, Book.name == entry.songbook.name)
+ if not existing_book:
+ existing_book = Book.populate(name=entry.songbook.name, publisher=entry.songbook.publisher)
+ new_song.add_songbook_entry(existing_book, entry.entry)
+ elif song.book:
+ existing_book = self.manager.get_object_filtered(Book, Book.name == song.book.name)
+ if not existing_book:
+ existing_book = Book.populate(name=song.book.name, publisher=song.book.publisher)
+ new_song.add_songbook_entry(existing_book, '')
+ # Find or create all the media files and add them to the new song object
+ if has_media_files and song.media_files:
+ for media_file in song.media_files:
+ existing_media_file = self.manager.get_object_filtered(
+ MediaFile, MediaFile.file_name == media_file.file_name)
+ if existing_media_file:
+ new_song.media_files.append(existing_media_file)
+ else:
+ new_song.media_files.append(MediaFile.populate(file_name=media_file.file_name))
clean_song(self.manager, new_song)
self.manager.save_object(new_song)
if progress_dialog:
=== added file 'tests/functional/openlp_plugins/songs/test_openlpimporter.py'
--- tests/functional/openlp_plugins/songs/test_openlpimporter.py 1970-01-01 00:00:00 +0000
+++ tests/functional/openlp_plugins/songs/test_openlpimporter.py 2016-04-27 18:51:46 +0000
@@ -0,0 +1,76 @@
+# -*- coding: utf-8 -*-
+# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
+
+###############################################################################
+# OpenLP - Open Source Lyrics Projection #
+# --------------------------------------------------------------------------- #
+# Copyright (c) 2008-2016 OpenLP Developers #
+# --------------------------------------------------------------------------- #
+# This program is free software; you can redistribute it and/or modify it #
+# under the terms of the GNU General Public License as published by the Free #
+# Software Foundation; version 2 of the License. #
+# #
+# This program is distributed in the hope that it will be useful, but WITHOUT #
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or #
+# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for #
+# more details. #
+# #
+# You should have received a copy of the GNU General Public License along #
+# with this program; if not, write to the Free Software Foundation, Inc., 59 #
+# Temple Place, Suite 330, Boston, MA 02111-1307 USA #
+###############################################################################
+"""
+This module contains tests for the OpenLP song importer.
+"""
+from unittest import TestCase
+
+from openlp.plugins.songs.lib.importers.openlp import OpenLPSongImport
+from openlp.core.common import Registry
+from tests.functional import patch, MagicMock
+
+
+class TestOpenLPImport(TestCase):
+ """
+ Test the functions in the :mod:`openlp` importer module.
+ """
+ def setUp(self):
+ """
+ Create the registry
+ """
+ Registry.create()
+
+ def create_importer_test(self):
+ """
+ Test creating an instance of the OpenLP database importer
+ """
+ # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+ with patch('openlp.plugins.songs.lib.importers.openlp.SongImport'):
+ mocked_manager = MagicMock()
+
+ # WHEN: An importer object is created
+ importer = OpenLPSongImport(mocked_manager, filenames=[])
+
+ # THEN: The importer object should not be None
+ self.assertIsNotNone(importer, 'Import should not be none')
+
+ def invalid_import_source_test(self):
+ """
+ Test OpenLPSongImport.do_import handles different invalid import_source values
+ """
+ # GIVEN: A mocked out SongImport class, and a mocked out "manager"
+ with patch('openlp.plugins.songs.lib.importers.openlp.SongImport'):
+ mocked_manager = MagicMock()
+ mocked_import_wizard = MagicMock()
+ importer = OpenLPSongImport(mocked_manager, filenames=[])
+ importer.import_wizard = mocked_import_wizard
+ importer.stop_import_flag = True
+
+ # WHEN: Import source is not a list
+ for source in ['not a list', 0]:
+ importer.import_source = source
+
+ # THEN: do_import should return none and the progress bar maximum should not be set.
+ self.assertIsNone(importer.do_import(), 'do_import should return None when import_source is not a list')
+ self.assertEqual(mocked_import_wizard.progress_bar.setMaximum.called, False,
+ 'setMaximum on import_wizard.progress_bar should not have been called')
+
Follow ups
