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