Fix bug #1557514 by autodetecting the columns of the tables in the songs database

Fixes: https://launchpad.net/bugs/1557514

.bzrignore

@@ -45,3 +45,4 @@ cover
 *.kdev4
 coverage
 tags
+output

openlp/plugins/songs/lib/db.py

@@ -383,7 +383,7 @@ def init_schema(url):
         # 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)

openlp/plugins/songs/lib/importers/openlp.py

@@ -51,7 +51,7 @@ class OpenLPSongImport(SongImport):
         :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 OpenLPSongImport(SongImport):
 
         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 @@ class OpenLPSongImport(SongImport):
         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 OpenLPSongImport(SongImport):
                 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 @@ class OpenLPSongImport(SongImport):
                     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 @@ class OpenLPSongImport(SongImport):
                 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:
+            # Find or create all the topics and add them to the new song object
-                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
             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:
+            # Find or create all the songbooks and add them to the new song object
-                if song.media_files:
+            if has_songs_books and song.songbook_entries:
-                    for media_file in song.media_files:
+                for entry in song.songbook_entries:
-                        existing_media_file = self.manager.get_object_filtered(
+                    existing_book = self.manager.get_object_filtered(Book, Book.name == entry.songbook.name)
-                            MediaFile, MediaFile.file_name == media_file.file_name)
+                    if not existing_book:
-                        if existing_media_file:
+                        existing_book = Book.populate(name=entry.songbook.name, publisher=entry.songbook.publisher)
-                            new_song.media_files.append(existing_media_file)
+                    new_song.add_songbook_entry(existing_book, entry.entry)
-                        else:
+            elif song.book:
-                            new_song.media_files.append(MediaFile.populate(file_name=media_file.file_name))
+                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:

tests/functional/openlp_plugins/songs/test_openlpimporter.py

@@ -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')
+