diff --git a/openlp/plugins/songs/forms/songmaintenanceform.py b/openlp/plugins/songs/forms/songmaintenanceform.py index 54b37bd98..e4ea1e558 100644 --- a/openlp/plugins/songs/forms/songmaintenanceform.py +++ b/openlp/plugins/songs/forms/songmaintenanceform.py @@ -30,7 +30,7 @@ from openlp.core.lib.ui import critical_error_message_box from openlp.plugins.songs.forms.authorsform import AuthorsForm from openlp.plugins.songs.forms.songbookform import SongBookForm from openlp.plugins.songs.forms.topicsform import TopicsForm -from openlp.plugins.songs.lib.db import Author, Book, Song, Topic +from openlp.plugins.songs.lib.db import Author, Book, Song, Topic, SongBookEntry from .songmaintenancedialog import Ui_SongMaintenanceDialog @@ -473,11 +473,41 @@ class SongMaintenanceForm(QtWidgets.QDialog, Ui_SongMaintenanceDialog, RegistryP Book.id != old_song_book.id ) ) - # Find the songs, which have the old_song_book as book. - songs = self.manager.get_all_objects(Song, Song.song_book_id == old_song_book.id) - for song in songs: - song.song_book_id = existing_book.id + if existing_book is None: + return False + # Find the songs which have the old_song_book as book, via matching entries in the songs_songbooks table + songbook_entries = self.manager.get_all_objects(SongBookEntry, SongBookEntry.songbook_id == old_song_book.id) + affected_songs = [] + for songbook_entry in songbook_entries: + affected_songs.append(songbook_entry.song_id) + for song_id in affected_songs: + song = self.manager.get_object(Song, song_id) + # Go through the song's song/songbook link records to look for the link to the old songbook. + # Store the song number in that book so we can apply it in the link record to the existing songbook. + old_book_song_number = '' + for index, record in enumerate(song.songbook_entries): + if record.songbook_id == old_song_book.id: + old_book_song_number = record.entry + break + # Look through the same link records to see if there's a link to the duplicate (existing) songbook + # If so, transfer the song number in 'entry' field (if appropriate) ... + found = False + for record in song.songbook_entries: + if record.songbook_id == existing_book.id: + if not record.entry and old_book_song_number: + record.entry = old_book_song_number + found = True + break + # ... otherwise create a new link record + if not found: + new_songbook_entry = SongBookEntry() + new_songbook_entry.songbook_id = existing_book.id + new_songbook_entry.song_id = song.id + new_songbook_entry.entry = old_book_song_number + song.songbook_entries.append(new_songbook_entry) self.manager.save_object(song) + self.manager.delete_object(SongBookEntry, (old_song_book.id, song_id, old_book_song_number)) + self.manager.delete_object(Book, old_song_book.id) def on_delete_author_button_clicked(self): diff --git a/openlp/plugins/songs/lib/db.py b/openlp/plugins/songs/lib/db.py index f2ec41423..ad4d04656 100644 --- a/openlp/plugins/songs/lib/db.py +++ b/openlp/plugins/songs/lib/db.py @@ -23,8 +23,10 @@ The :mod:`db` module provides the database and schema that is the backend for the Songs plugin """ from sqlalchemy import Column, ForeignKey, Table, types -from sqlalchemy.orm import mapper, reconstructor, relation +from sqlalchemy.orm import class_mapper, mapper, reconstructor, relation from sqlalchemy.sql.expression import func, text +from sqlalchemy.orm.exc import UnmappedClassError + from openlp.core.common.i18n import get_natural_key, translate from openlp.core.lib.db import BaseModel, PathType, init_db @@ -364,30 +366,52 @@ def init_schema(url): Column('topic_id', types.Integer(), ForeignKey('topics.id'), primary_key=True) ) - mapper(Author, authors_table, properties={ - 'songs': relation(Song, secondary=authors_songs_table, viewonly=True) - }) - mapper(AuthorSong, authors_songs_table, properties={ - 'author': relation(Author) - }) - mapper(SongBookEntry, songs_songbooks_table, properties={ - 'songbook': relation(Book) - }) - mapper(Book, song_books_table, properties={ - 'songs': relation(Song, secondary=songs_songbooks_table) - }) - mapper(MediaFile, media_files_table) - mapper(Song, songs_table, properties={ - # Use the authors_songs relation when you need access to the 'author_type' attribute - # or when creating new relations - 'authors_songs': relation(AuthorSong, cascade="all, delete-orphan"), - # 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'), - 'topics': relation(Topic, backref='songs', secondary=songs_topics_table) - }) - mapper(Topic, topics_table) + # try/except blocks are for the purposes of tests - the mappers could have been defined in a previous test + try: + class_mapper(Author) + except UnmappedClassError: + mapper(Author, authors_table, properties={ + 'songs': relation(Song, secondary=authors_songs_table, viewonly=True) + }) + try: + class_mapper(AuthorSong) + except UnmappedClassError: + mapper(AuthorSong, authors_songs_table, properties={ + 'author': relation(Author) + }) + try: + class_mapper(SongBookEntry) + except UnmappedClassError: + mapper(SongBookEntry, songs_songbooks_table, properties={ + 'songbook': relation(Book) + }) + try: + class_mapper(Book) + except UnmappedClassError: + mapper(Book, song_books_table, properties={ + 'songs': relation(Song, secondary=songs_songbooks_table) + }) + try: + class_mapper(MediaFile) + except UnmappedClassError: + mapper(MediaFile, media_files_table) + try: + class_mapper(Song) + except UnmappedClassError: + mapper(Song, songs_table, properties={ + # Use the authors_songs relation when you need access to the 'author_type' attribute + # or when creating new relations + 'authors_songs': relation(AuthorSong, cascade="all, delete-orphan"), + # 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'), + 'topics': relation(Topic, backref='songs', secondary=songs_topics_table) + }) + try: + class_mapper(Topic) + except UnmappedClassError: + mapper(Topic, topics_table) metadata.create_all(checkfirst=True) return session diff --git a/tests/openlp_plugins/songs/forms/test_songmaintenanceform.py b/tests/openlp_plugins/songs/forms/test_songmaintenanceform.py index 0e1d1ba0c..624f2ede8 100644 --- a/tests/openlp_plugins/songs/forms/test_songmaintenanceform.py +++ b/tests/openlp_plugins/songs/forms/test_songmaintenanceform.py @@ -22,14 +22,20 @@ Package to test the openlp.plugins.songs.forms.songmaintenanceform package. """ import pytest +import os + from unittest.mock import MagicMock, call, patch, ANY from PyQt5 import QtCore, QtWidgets from openlp.core.common.i18n import UiStrings from openlp.core.common.registry import Registry +from openlp.core.lib.db import Manager +from openlp.plugins.songs.lib.db import init_schema, Book, Song, SongBookEntry from openlp.plugins.songs.forms.songmaintenanceform import SongMaintenanceForm +from sqlalchemy.sql import and_ + @pytest.fixture() def form_env(settings): @@ -448,3 +454,88 @@ def test_check_object_exists(form_env): # THEN: The result should be False assert result is True + + +def test_merge_song_books(registry, settings, temp_folder): + """ + Test the functionality of merging 2 song books. + """ + # GIVEN a test database populated with test data, and a song maintenance form + db_tmp_path = os.path.join(temp_folder, 'test-songs-2.9.2.sqlite') + manager = Manager('songs', init_schema, db_file_path=db_tmp_path) + + # create 2 song books, both with the same name + book1 = Book() + book1.name = 'test book1' + book1.publisher = '' + manager.save_object(book1) + book2 = Book() + book2.name = 'test book1' + book2.publisher = '' + manager.save_object(book2) + + # create 3 songs, all with same search_title + song1 = Song() + song1.title = 'test song1' + song1.lyrics = 'lyrics1' + song1.search_title = 'test song' + song1.search_lyrics = 'lyrics1' + manager.save_object(song1) + song2 = Song() + song2.title = 'test song2' + song2.lyrics = 'lyrics2' + song2.search_title = 'test song' + song2.search_lyrics = 'lyrics2' + manager.save_object(song2) + song3 = Song() + song3.title = 'test song3' + song3.lyrics = 'lyrics3' + song3.search_title = 'test song' + song3.search_lyrics = 'lyrics3' + manager.save_object(song3) + + # associate songs with song books + song1.add_songbook_entry(book1, '10') + song2.add_songbook_entry(book1, '20') + song2.add_songbook_entry(book2, '30') + song3.add_songbook_entry(book1, '40') + song3.add_songbook_entry(book2, '') + + song_maintenance_form = SongMaintenanceForm(manager) + + # WHEN the song books are merged, getting rid of book1 + song_maintenance_form.merge_song_books(book1) + + # THEN the database should reflect correctly the merge + + songs = manager.get_all_objects(Song, Song.search_title == 'test song') + songbook1_entries = manager.get_all_objects(SongBookEntry, SongBookEntry.songbook_id == book1.id) + songbook2_entries = manager.get_all_objects(SongBookEntry, SongBookEntry.songbook_id == book2.id) + song1_book2_entry = manager.get_all_objects(SongBookEntry, and_(SongBookEntry.songbook_id == book2.id, + SongBookEntry.song_id == song1.id)) + song2_book2_entry = manager.get_all_objects(SongBookEntry, and_(SongBookEntry.songbook_id == book2.id, + SongBookEntry.song_id == song2.id)) + song3_book2_entry = manager.get_all_objects(SongBookEntry, and_(SongBookEntry.songbook_id == book2.id, + SongBookEntry.song_id == song3.id)) + books = manager.get_all_objects(Book, Book.name == 'test book1') + + # song records should not be deleted + assert len(songs) == 3 + # the old book should have been deleted, with its songs_songbooks records + assert len(books) == 1 + assert len(songbook1_entries) == 0 + + # each of the 3 songs should be associated with book2 + assert len(songbook2_entries) == 3 + + # the individual SongBookEntry records should be correct + assert len(song1_book2_entry) == 1 + assert song1_book2_entry[0].entry == '10' + + assert len(song2_book2_entry) == 1 + # entry field should not be overridden, as it was set previously for book2 + assert song2_book2_entry[0].entry == '30' + + assert len(song3_book2_entry) == 1 + # entry field should be overridden, as it was not set previously + assert song3_book2_entry[0].entry == '40'