From 3ff851b0646318764550b5e4d80500f395c86cd0 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 8 Mar 2017 16:31:57 -0700 Subject: [PATCH 1/6] Fix an issue with invalid songbook entries being created --- openlp/plugins/songs/lib/upgrade.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/openlp/plugins/songs/lib/upgrade.py b/openlp/plugins/songs/lib/upgrade.py index 8d04cbf2b..cda7cdad6 100644 --- a/openlp/plugins/songs/lib/upgrade.py +++ b/openlp/plugins/songs/lib/upgrade.py @@ -32,7 +32,7 @@ from openlp.core.lib.db import get_upgrade_op from openlp.core.utils.db import drop_columns log = logging.getLogger(__name__) -__version__ = 5 +__version__ = 6 # TODO: When removing an upgrade path the ftw-data needs updating to the minimum supported version @@ -139,7 +139,7 @@ def upgrade_5(session, metadata): # Migrate old data op.execute('INSERT INTO songs_songbooks SELECT song_book_id, id, song_number FROM songs\ - WHERE song_book_id IS NOT NULL AND song_number IS NOT NULL') + WHERE song_book_id IS NOT NULL AND song_book_id <> 0 AND song_number IS NOT NULL') # Drop old columns if metadata.bind.url.get_dialect().name == 'sqlite': @@ -148,3 +148,12 @@ def upgrade_5(session, metadata): op.drop_constraint('songs_ibfk_1', 'songs', 'foreignkey') op.drop_column('songs', 'song_book_id') op.drop_column('songs', 'song_number') + +def upgrade_6(session, metadata): + """ + Version 6 upgrade. + + This is to fix an issue we had with songbooks with an id of "0" being imported in the previous upgrade. + """ + op = get_upgrade_op(session) + op.execute('DELETE FROM songs_songbooks WHERE songbook_id = 0') From 8850ab2fe3e4dff5157a7fbda8887491c5564d3e Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 8 Mar 2017 16:32:16 -0700 Subject: [PATCH 2/6] Add support for author types to the OpenLP importer --- openlp/plugins/songs/lib/importers/openlp.py | 33 ++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/openlp/plugins/songs/lib/importers/openlp.py b/openlp/plugins/songs/lib/importers/openlp.py index 4a7a847a3..0910f376c 100644 --- a/openlp/plugins/songs/lib/importers/openlp.py +++ b/openlp/plugins/songs/lib/importers/openlp.py @@ -61,6 +61,12 @@ class OpenLPSongImport(SongImport): :param progress_dialog: The QProgressDialog used when importing songs from the FRW. """ + class OldAuthorSong(BaseModel): + """ + Maps to the authors table + """ + pass + class OldAuthor(BaseModel): """ Maps to the authors table @@ -117,6 +123,10 @@ class OpenLPSongImport(SongImport): has_songs_books = True else: has_songs_books = False + if 'authors_songs' in list(source_meta.tables.keys()): + has_authors_songs = True + else: + has_authors_songs = 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'] @@ -139,6 +149,10 @@ class OpenLPSongImport(SongImport): class_mapper(OldSongBookEntry) except UnmappedClassError: mapper(OldSongBookEntry, source_songs_songbooks_table, properties={'songbook': relation(OldBook)}) + if has_authors_songs and 'author_type' in source_authors_songs_table.c.values(): + has_author_type = True + else: + has_author_type = False # Set up the songs relationships song_props = { 'authors': relation(OldAuthor, backref='songs', secondary=source_authors_songs_table), @@ -157,6 +171,8 @@ class OpenLPSongImport(SongImport): song_props['songbook_entries'] = relation(OldSongBookEntry, backref='song', cascade='all, delete-orphan') else: song_props['book'] = relation(OldBook, backref='songs') + if has_authors_songs: + song_props['authors_songs'] = relation(OldAuthorSong) # Map the rest of the tables try: class_mapper(OldAuthor) @@ -174,6 +190,11 @@ class OpenLPSongImport(SongImport): class_mapper(OldTopic) except UnmappedClassError: mapper(OldTopic, source_topics_table) + if has_authors_songs: + try: + class_mapper(OldAuthorSong) + except UnmappedClassError: + mapper(OldAuthorSong, source_authors_songs_table) source_songs = self.source_session.query(OldSong).all() if self.import_wizard: @@ -205,8 +226,16 @@ class OpenLPSongImport(SongImport): 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) + display_name=author.display_name + ) + # if this is a new database, we need to import the author type too + author_type = None + if has_author_type: + for author_song in song.authors_songs: + if author_song.author_id == author.id: + author_type = author_song.author_type + break + new_song.add_author(existing_author, author_type) # Find or create all the topics and add them to the new song object if song.topics: for topic in song.topics: From c5835a73622756547bb9d5638f8ae83c84b4793f Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 8 Mar 2017 22:20:27 -0700 Subject: [PATCH 3/6] Fix a bunch of errors in the song database upgrades --- openlp/plugins/songs/lib/upgrade.py | 37 +++++++++++++++++------------ 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/openlp/plugins/songs/lib/upgrade.py b/openlp/plugins/songs/lib/upgrade.py index cda7cdad6..ec27d8243 100644 --- a/openlp/plugins/songs/lib/upgrade.py +++ b/openlp/plugins/songs/lib/upgrade.py @@ -26,7 +26,7 @@ backend for the Songs plugin import logging from sqlalchemy import Table, Column, ForeignKey, types -from sqlalchemy.sql.expression import func, false, null, text +from sqlalchemy.sql.expression import func, false, null, text, and_, select from openlp.core.lib.db import get_upgrade_op from openlp.core.utils.db import drop_columns @@ -105,13 +105,15 @@ def upgrade_4(session, metadata): # Since SQLite doesn't support changing the primary key of a table, we need to recreate the table # and copy the old values op = get_upgrade_op(session) - songs_table = Table('songs', metadata) - if 'author_type' not in [col.name for col in songs_table.c.values()]: - op.create_table('authors_songs_tmp', - Column('author_id', types.Integer(), ForeignKey('authors.id'), primary_key=True), - Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True), - Column('author_type', types.Unicode(255), primary_key=True, - nullable=False, server_default=text('""'))) + authors_songs = Table('authors_songs', metadata, autoload=True) + if 'author_type' not in [col.name for col in authors_songs.c.values()]: + authors_songs_tmp = op.create_table( + 'authors_songs_tmp', + Column('author_id', types.Integer(), ForeignKey('authors.id'), primary_key=True), + Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True), + Column('author_type', types.Unicode(255), primary_key=True, + nullable=False, server_default=text('""')) + ) op.execute('INSERT INTO authors_songs_tmp SELECT author_id, song_id, "" FROM authors_songs') op.drop_table('authors_songs') op.rename_table('authors_songs_tmp', 'authors_songs') @@ -126,16 +128,18 @@ def upgrade_5(session, metadata): This upgrade adds support for multiple songbooks """ op = get_upgrade_op(session) - songs_table = Table('songs', metadata) - if 'song_book_id' in [col.name for col in songs_table.c.values()]: + songs_table = Table('songs', metadata, autoload=True) + if 'song_book_id' not in [col.name for col in songs_table.c.values()]: log.warning('Skipping upgrade_5 step of upgrading the song db') return # Create the mapping table (songs <-> songbooks) - op.create_table('songs_songbooks', - Column('songbook_id', types.Integer(), ForeignKey('song_books.id'), primary_key=True), - Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True), - Column('entry', types.Unicode(255), primary_key=True, nullable=False)) + songs_songbooks_table = op.create_table( + 'songs_songbooks', + Column('songbook_id', types.Integer(), ForeignKey('song_books.id'), primary_key=True), + Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True), + Column('entry', types.Unicode(255), primary_key=True, nullable=False) + ) # Migrate old data op.execute('INSERT INTO songs_songbooks SELECT song_book_id, id, song_number FROM songs\ @@ -149,6 +153,7 @@ def upgrade_5(session, metadata): op.drop_column('songs', 'song_book_id') op.drop_column('songs', 'song_number') + def upgrade_6(session, metadata): """ Version 6 upgrade. @@ -156,4 +161,6 @@ def upgrade_6(session, metadata): This is to fix an issue we had with songbooks with an id of "0" being imported in the previous upgrade. """ op = get_upgrade_op(session) - op.execute('DELETE FROM songs_songbooks WHERE songbook_id = 0') + songs_songbooks = Table('songs_songbooks', metadata, autoload=True) + del_query = songs_songbooks.delete().where(songs_songbooks.c.songbook_id == 0) + op.execute(del_query) From 3635cc360370668bf3b21e13a0390b0f57f6ad7c Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 8 Mar 2017 22:23:37 -0700 Subject: [PATCH 4/6] Write some tests --- .../songs/forms/test_editsongform.py | 51 ++++++++++++++++++- 1 file changed, 49 insertions(+), 2 deletions(-) diff --git a/tests/interfaces/openlp_plugins/songs/forms/test_editsongform.py b/tests/interfaces/openlp_plugins/songs/forms/test_editsongform.py index bd5b61850..c8a59c59b 100644 --- a/tests/interfaces/openlp_plugins/songs/forms/test_editsongform.py +++ b/tests/interfaces/openlp_plugins/songs/forms/test_editsongform.py @@ -23,13 +23,13 @@ Package to test the openlp.plugins.songs.forms.editsongform package. """ from unittest import TestCase +from unittest.mock import MagicMock, patch -from PyQt5 import QtWidgets +from PyQt5 import QtCore, QtWidgets from openlp.core.common import Registry from openlp.core.common.uistrings import UiStrings from openlp.plugins.songs.forms.editsongform import EditSongForm -from tests.interfaces import MagicMock from tests.helpers.testmixin import TestMixin @@ -157,3 +157,50 @@ class TestEditSongForm(TestCase, TestMixin): # THEN: The verse order should be converted to uppercase self.assertEqual(form.verse_order_edit.text(), 'V1 V2 C1 V3 C1 V4 C1') + + @patch('openlp.plugins.songs.forms.editsongform.QtWidgets.QListWidgetItem') + def test_add_author_to_list(self, MockedQListWidgetItem): + """ + Test the _add_author_to_list() method + """ + # GIVEN: A song edit form and some mocked stuff + mocked_author = MagicMock() + mocked_author.id = 1 + mocked_author.get_display_name.return_value = 'John Newton' + mocked_author_type = 'words' + mocked_widget_item = MagicMock() + MockedQListWidgetItem.return_value = mocked_widget_item + + # WHEN: _add_author_to_list() is called + with patch.object(self.form.authors_list_view, 'addItem') as mocked_add_item: + self.form._add_author_to_list(mocked_author, mocked_author_type) + + # THEN: All the correct methods should have been called + mocked_author.get_display_name.assert_called_once_with('words') + MockedQListWidgetItem.assert_called_once_with('John Newton') + mocked_widget_item.setData.assert_called_once_with(QtCore.Qt.UserRole, (1, mocked_author_type)) + mocked_add_item.assert_called_once_with(mocked_widget_item) + + @patch('openlp.plugins.songs.forms.editsongform.SongBookEntry') + @patch('openlp.plugins.songs.forms.editsongform.QtWidgets.QListWidgetItem') + def test_add_songbook_entry_to_list(self, MockedQListWidgetItem, MockedSongbookEntry): + """ + Test the add_songbook_entry_to_list() method + """ + # GIVEN: A song edit form and some mocked stuff + songbook_id = 1 + songbook_name = 'Hymnal' + entry = '546' + MockedSongbookEntry.get_display_name.return_value = 'Hymnal #546' + mocked_widget_item = MagicMock() + MockedQListWidgetItem.return_value = mocked_widget_item + + # WHEN: _add_author_to_list() is called + with patch.object(self.form.songbooks_list_view, 'addItem') as mocked_add_item: + self.form.add_songbook_entry_to_list(songbook_id, songbook_name, entry) + + # THEN: All the correct methods should have been called + MockedSongbookEntry.get_display_name.assert_called_once_with(songbook_name, entry) + MockedQListWidgetItem.assert_called_once_with('Hymnal #546') + mocked_widget_item.setData.assert_called_once_with(QtCore.Qt.UserRole, (songbook_id, entry)) + mocked_add_item.assert_called_once_with(mocked_widget_item) From 808875b683747a4da25432cb3a8ee9c38308239d Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 8 Mar 2017 22:36:03 -0700 Subject: [PATCH 5/6] Added a (long overdue) changelog --- CHANGELOG.rst | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 CHANGELOG.rst diff --git a/CHANGELOG.rst b/CHANGELOG.rst new file mode 100644 index 000000000..f263c4157 --- /dev/null +++ b/CHANGELOG.rst @@ -0,0 +1,8 @@ +OpenLP 2.4.6 +============ + +* Fixed a bug where the author type upgrade was being ignore because it was looking at the wrong table +* Fixed a bug where the songs_songbooks table was not being created because the if expression was the wrong way round +* Changed the songs_songbooks migration SQL slightly to take into account a bug that has (hopefully) been fixed +* Add another upgrade step to remove erroneous songs_songbooks entries due to the previous bug +* Added importing of author types to the OpenLP 2 song importer From 91a32d33a716d0754027da7ac3d3302acde2fd34 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 8 Mar 2017 22:36:11 -0700 Subject: [PATCH 6/6] Fix a linting issue --- openlp/plugins/songs/lib/importers/openlp.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openlp/plugins/songs/lib/importers/openlp.py b/openlp/plugins/songs/lib/importers/openlp.py index 0910f376c..f13ed4620 100644 --- a/openlp/plugins/songs/lib/importers/openlp.py +++ b/openlp/plugins/songs/lib/importers/openlp.py @@ -172,7 +172,7 @@ class OpenLPSongImport(SongImport): else: song_props['book'] = relation(OldBook, backref='songs') if has_authors_songs: - song_props['authors_songs'] = relation(OldAuthorSong) + song_props['authors_songs'] = relation(OldAuthorSong) # Map the rest of the tables try: class_mapper(OldAuthor)