OpenLP

Merge lp:~raoul-snyman/openlp/bug-1557514 into lp:openlp

  1. bug-1557514
  2. Merge into trunk
Proposed by Raoul Snyman
Status: Merged
Merged at revision: 2654
Proposed branch: lp:~raoul-snyman/openlp/bug-1557514
Merge into: lp:openlp
Diff against target: 294 lines (+139/-28)
4 files modified
.bzrignore (+1/-0)
openlp/plugins/songs/lib/db.py (+1/-1)
openlp/plugins/songs/lib/importers/openlp.py (+61/-27)
tests/functional/openlp_plugins/songs/test_openlpimporter.py (+76/-0)
To merge this branch: bzr merge lp:~raoul-snyman/openlp/bug-1557514
Reviewer Review Type Date Requested Status
Tim Bentley Approve
Tomas Groth Pending
Review via email: mp+293176@code.launchpad.net

This proposal supersedes a proposal from 2016-04-27.

To post a comment you must log in.
Revision history for this message
Tomas Groth (tomasgroth) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Tim Bentley (trb143) : Posted in a previous version of this proposal
review: Approve
Revision history for this message
Tim Bentley (trb143) wrote : Posted in a previous version of this proposal

Missing test!

review: Needs Fixing
Revision history for this message
Raoul Snyman (raoul-snyman) wrote : Posted in a previous version of this proposal

Darn, forgot to add the file.

Revision history for this message
Tim Bentley (trb143) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '.bzrignore'
2--- .bzrignore 2016-02-11 21:05:41 +0000
3+++ .bzrignore 2016-04-27 21:24:28 +0000
4@@ -45,3 +45,4 @@
5 *.kdev4
6 coverage
7 tags
8+output
9
10=== modified file 'openlp/plugins/songs/lib/db.py'
11--- openlp/plugins/songs/lib/db.py 2016-04-03 19:44:09 +0000
12+++ openlp/plugins/songs/lib/db.py 2016-04-27 21:24:28 +0000
13@@ -383,7 +383,7 @@
14 # Use lazy='joined' to always load authors when the song is fetched from the database (bug 1366198)
15 'authors': relation(Author, secondary=authors_songs_table, viewonly=True, lazy='joined'),
16 'media_files': relation(MediaFile, backref='songs', order_by=media_files_table.c.weight),
17- 'songbook_entries': relation(SongBookEntry, backref='song', cascade="all, delete-orphan"),
18+ 'songbook_entries': relation(SongBookEntry, backref='song', cascade='all, delete-orphan'),
19 'topics': relation(Topic, backref='songs', secondary=songs_topics_table)
20 })
21 mapper(Topic, topics_table)
22
23=== modified file 'openlp/plugins/songs/lib/importers/openlp.py'
24--- openlp/plugins/songs/lib/importers/openlp.py 2016-04-22 18:25:57 +0000
25+++ openlp/plugins/songs/lib/importers/openlp.py 2016-04-27 21:24:28 +0000
26@@ -51,7 +51,7 @@
27 :param manager: The song manager for the running OpenLP installation.
28 :param kwargs: The database providing the data to import.
29 """
30- SongImport.__init__(self, manager, **kwargs)
31+ super(OpenLPSongImport, self).__init__(manager, **kwargs)
32 self.source_session = None
33
34 def do_import(self, progress_dialog=None):
35@@ -63,49 +63,61 @@
36
37 class OldAuthor(BaseModel):
38 """
39- Author model
40+ Maps to the authors table
41 """
42 pass
43
44 class OldBook(BaseModel):
45 """
46- Book model
47+ Maps to the songbooks table
48 """
49 pass
50
51 class OldMediaFile(BaseModel):
52 """
53- MediaFile model
54+ Maps to the media_files table
55 """
56 pass
57
58 class OldSong(BaseModel):
59 """
60- Song model
61+ Maps to the songs table
62 """
63 pass
64
65 class OldTopic(BaseModel):
66 """
67- Topic model
68+ Maps to the topics table
69+ """
70+ pass
71+
72+ class OldSongBookEntry(BaseModel):
73+ """
74+ Maps to the songs_songbooks table
75 """
76 pass
77
78 # Check the file type
79- if not self.import_source.endswith('.sqlite'):
80+ if not isinstance(self.import_source, str) or not self.import_source.endswith('.sqlite'):
81 self.log_error(self.import_source, translate('SongsPlugin.OpenLPSongImport',
82 'Not a valid OpenLP 2 song database.'))
83 return
84 self.import_source = 'sqlite:///%s' % self.import_source
85- # Load the db file
86+ # Load the db file and reflect it
87 engine = create_engine(self.import_source)
88 source_meta = MetaData()
89 source_meta.reflect(engine)
90 self.source_session = scoped_session(sessionmaker(bind=engine))
91+ # Run some checks to see which version of the database we have
92 if 'media_files' in list(source_meta.tables.keys()):
93 has_media_files = True
94 else:
95 has_media_files = False
96+ if 'songs_songbooks' in list(source_meta.tables.keys()):
97+ has_songs_books = True
98+ else:
99+ has_songs_books = False
100+ # Load up the tabls and map them out
101 source_authors_table = source_meta.tables['authors']
102 source_song_books_table = source_meta.tables['song_books']
103 source_songs_table = source_meta.tables['songs']
104@@ -113,6 +125,7 @@
105 source_authors_songs_table = source_meta.tables['authors_songs']
106 source_songs_topics_table = source_meta.tables['songs_topics']
107 source_media_files_songs_table = None
108+ # Set up media_files relations
109 if has_media_files:
110 source_media_files_table = source_meta.tables['media_files']
111 source_media_files_songs_table = source_meta.tables.get('media_files_songs')
112@@ -120,9 +133,15 @@
113 class_mapper(OldMediaFile)
114 except UnmappedClassError:
115 mapper(OldMediaFile, source_media_files_table)
116+ if has_songs_books:
117+ source_songs_songbooks_table = source_meta.tables['songs_songbooks']
118+ try:
119+ class_mapper(OldSongBookEntry)
120+ except UnmappedClassError:
121+ mapper(OldSongBookEntry, source_songs_songbooks_table, properties={'songbook': relation(OldBook)})
122+ # Set up the songs relationships
123 song_props = {
124 'authors': relation(OldAuthor, backref='songs', secondary=source_authors_songs_table),
125- 'book': relation(OldBook, backref='songs'),
126 'topics': relation(OldTopic, backref='songs', secondary=source_songs_topics_table)
127 }
128 if has_media_files:
129@@ -134,6 +153,11 @@
130 relation(OldMediaFile, backref='songs',
131 foreign_keys=[source_media_files_table.c.song_id],
132 primaryjoin=source_songs_table.c.id == source_media_files_table.c.song_id)
133+ if has_songs_books:
134+ song_props['songbook_entries'] = relation(OldSongBookEntry, backref='song', cascade='all, delete-orphan')
135+ else:
136+ song_props['book'] = relation(OldBook, backref='songs')
137+ # Map the rest of the tables
138 try:
139 class_mapper(OldAuthor)
140 except UnmappedClassError:
141@@ -163,44 +187,54 @@
142 old_titles = song.search_title.split('@')
143 if len(old_titles) > 1:
144 new_song.alternate_title = old_titles[1]
145- # Values will be set when cleaning the song.
146+ # Transfer the values to the new song object
147 new_song.search_title = ''
148 new_song.search_lyrics = ''
149- new_song.song_number = song.song_number
150 new_song.lyrics = song.lyrics
151 new_song.verse_order = song.verse_order
152 new_song.copyright = song.copyright
153 new_song.comments = song.comments
154 new_song.theme_name = song.theme_name
155 new_song.ccli_number = song.ccli_number
156+ if hasattr(song, 'song_number') and song.song_number:
157+ new_song.song_number = song.song_number
158+ # Find or create all the authors and add them to the new song object
159 for author in song.authors:
160 existing_author = self.manager.get_object_filtered(Author, Author.display_name == author.display_name)
161- if existing_author is None:
162+ if not existing_author:
163 existing_author = Author.populate(
164 first_name=author.first_name,
165 last_name=author.last_name,
166 display_name=author.display_name)
167 new_song.add_author(existing_author)
168- if song.book:
169- existing_song_book = self.manager.get_object_filtered(Book, Book.name == song.book.name)
170- if existing_song_book is None:
171- existing_song_book = Book.populate(name=song.book.name, publisher=song.book.publisher)
172- new_song.book = existing_song_book
173+ # Find or create all the topics and add them to the new song object
174 if song.topics:
175 for topic in song.topics:
176 existing_topic = self.manager.get_object_filtered(Topic, Topic.name == topic.name)
177- if existing_topic is None:
178+ if not existing_topic:
179 existing_topic = Topic.populate(name=topic.name)
180 new_song.topics.append(existing_topic)
181- if has_media_files:
182- if song.media_files:
183- for media_file in song.media_files:
184- existing_media_file = self.manager.get_object_filtered(
185- MediaFile, MediaFile.file_name == media_file.file_name)
186- if existing_media_file:
187- new_song.media_files.append(existing_media_file)
188- else:
189- new_song.media_files.append(MediaFile.populate(file_name=media_file.file_name))
190+ # Find or create all the songbooks and add them to the new song object
191+ if has_songs_books and song.songbook_entries:
192+ for entry in song.songbook_entries:
193+ existing_book = self.manager.get_object_filtered(Book, Book.name == entry.songbook.name)
194+ if not existing_book:
195+ existing_book = Book.populate(name=entry.songbook.name, publisher=entry.songbook.publisher)
196+ new_song.add_songbook_entry(existing_book, entry.entry)
197+ elif song.book:
198+ existing_book = self.manager.get_object_filtered(Book, Book.name == song.book.name)
199+ if not existing_book:
200+ existing_book = Book.populate(name=song.book.name, publisher=song.book.publisher)
201+ new_song.add_songbook_entry(existing_book, '')
202+ # Find or create all the media files and add them to the new song object
203+ if has_media_files and song.media_files:
204+ for media_file in song.media_files:
205+ existing_media_file = self.manager.get_object_filtered(
206+ MediaFile, MediaFile.file_name == media_file.file_name)
207+ if existing_media_file:
208+ new_song.media_files.append(existing_media_file)
209+ else:
210+ new_song.media_files.append(MediaFile.populate(file_name=media_file.file_name))
211 clean_song(self.manager, new_song)
212 self.manager.save_object(new_song)
213 if progress_dialog:
214
215=== added file 'tests/functional/openlp_plugins/songs/test_openlpimporter.py'
216--- tests/functional/openlp_plugins/songs/test_openlpimporter.py 1970-01-01 00:00:00 +0000
217+++ tests/functional/openlp_plugins/songs/test_openlpimporter.py 2016-04-27 21:24:28 +0000
218@@ -0,0 +1,76 @@
219+# -*- coding: utf-8 -*-
220+# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
221+
222+###############################################################################
223+# OpenLP - Open Source Lyrics Projection #
224+# --------------------------------------------------------------------------- #
225+# Copyright (c) 2008-2016 OpenLP Developers #
226+# --------------------------------------------------------------------------- #
227+# This program is free software; you can redistribute it and/or modify it #
228+# under the terms of the GNU General Public License as published by the Free #
229+# Software Foundation; version 2 of the License. #
230+# #
231+# This program is distributed in the hope that it will be useful, but WITHOUT #
232+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or #
233+# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for #
234+# more details. #
235+# #
236+# You should have received a copy of the GNU General Public License along #
237+# with this program; if not, write to the Free Software Foundation, Inc., 59 #
238+# Temple Place, Suite 330, Boston, MA 02111-1307 USA #
239+###############################################################################
240+"""
241+This module contains tests for the OpenLP song importer.
242+"""
243+from unittest import TestCase
244+
245+from openlp.plugins.songs.lib.importers.openlp import OpenLPSongImport
246+from openlp.core.common import Registry
247+from tests.functional import patch, MagicMock
248+
249+
250+class TestOpenLPImport(TestCase):
251+ """
252+ Test the functions in the :mod:`openlp` importer module.
253+ """
254+ def setUp(self):
255+ """
256+ Create the registry
257+ """
258+ Registry.create()
259+
260+ def create_importer_test(self):
261+ """
262+ Test creating an instance of the OpenLP database importer
263+ """
264+ # GIVEN: A mocked out SongImport class, and a mocked out "manager"
265+ with patch('openlp.plugins.songs.lib.importers.openlp.SongImport'):
266+ mocked_manager = MagicMock()
267+
268+ # WHEN: An importer object is created
269+ importer = OpenLPSongImport(mocked_manager, filenames=[])
270+
271+ # THEN: The importer object should not be None
272+ self.assertIsNotNone(importer, 'Import should not be none')
273+
274+ def invalid_import_source_test(self):
275+ """
276+ Test OpenLPSongImport.do_import handles different invalid import_source values
277+ """
278+ # GIVEN: A mocked out SongImport class, and a mocked out "manager"
279+ with patch('openlp.plugins.songs.lib.importers.openlp.SongImport'):
280+ mocked_manager = MagicMock()
281+ mocked_import_wizard = MagicMock()
282+ importer = OpenLPSongImport(mocked_manager, filenames=[])
283+ importer.import_wizard = mocked_import_wizard
284+ importer.stop_import_flag = True
285+
286+ # WHEN: Import source is not a list
287+ for source in ['not a list', 0]:
288+ importer.import_source = source
289+
290+ # THEN: do_import should return none and the progress bar maximum should not be set.
291+ self.assertIsNone(importer.do_import(), 'do_import should return None when import_source is not a list')
292+ self.assertEqual(mocked_import_wizard.progress_bar.setMaximum.called, False,
293+ 'setMaximum on import_wizard.progress_bar should not have been called')
294+