Merge lp:~sam92/openlp/bug-1533081 into lp:openlp
- bug-1533081
- Merge into trunk
Proposed by
Samuel Mehrbrodt
Status: | Merged |
---|---|
Merged at revision: | 2609 |
Proposed branch: | lp:~sam92/openlp/bug-1533081 |
Merge into: | lp:openlp |
Diff against target: |
249 lines (+122/-23) 3 files modified
openlp/core/utils/__init__.py (+1/-1) openlp/plugins/songs/lib/importers/openlp.py (+32/-22) tests/utils/test_pep8.py (+89/-0) |
To merge this branch: | bzr merge lp:~sam92/openlp/bug-1533081 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Raoul Snyman | Approve | ||
Tim Bentley | Approve | ||
Review via email: mp+282817@code.launchpad.net |
Commit message
Description of the change
* Fix importing older song dbs by migrating them first
* Fix comparing songs without title
lp:~sam92/openlp/bug-1533081 (revision 2615)
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[FAILURE] https:/
Stopping after failure
To post a comment you must log in.
Revision history for this message
Tim Bentley (trb143) : | # |
review:
Approve
Revision history for this message
Raoul Snyman (raoul-snyman) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'openlp/core/utils/__init__.py' | |||
2 | --- openlp/core/utils/__init__.py 2015-12-31 22:46:06 +0000 | |||
3 | +++ openlp/core/utils/__init__.py 2016-01-15 21:49:07 +0000 | |||
4 | @@ -529,7 +529,7 @@ | |||
5 | 529 | key = [int(part) if part.isdigit() else get_locale_key(part) for part in key] | 529 | key = [int(part) if part.isdigit() else get_locale_key(part) for part in key] |
6 | 530 | # Python 3 does not support comparison of different types anymore. So make sure, that we do not compare str | 530 | # Python 3 does not support comparison of different types anymore. So make sure, that we do not compare str |
7 | 531 | # and int. | 531 | # and int. |
9 | 532 | if string[0].isdigit(): | 532 | if string and string[0].isdigit(): |
10 | 533 | return [b''] + key | 533 | return [b''] + key |
11 | 534 | return key | 534 | return key |
12 | 535 | 535 | ||
13 | 536 | 536 | ||
14 | === modified file 'openlp/plugins/songs/lib/importers/openlp.py' | |||
15 | --- openlp/plugins/songs/lib/importers/openlp.py 2015-12-31 22:46:06 +0000 | |||
16 | +++ openlp/plugins/songs/lib/importers/openlp.py 2016-01-15 21:49:07 +0000 | |||
17 | @@ -24,15 +24,18 @@ | |||
18 | 24 | song databases into the current installation database. | 24 | song databases into the current installation database. |
19 | 25 | """ | 25 | """ |
20 | 26 | import logging | 26 | import logging |
21 | 27 | import os | ||
22 | 28 | import shutil | ||
23 | 29 | import tempfile | ||
24 | 27 | 30 | ||
25 | 28 | from sqlalchemy import create_engine, MetaData, Table | 31 | from sqlalchemy import create_engine, MetaData, Table |
26 | 29 | from sqlalchemy.orm import class_mapper, mapper, relation, scoped_session, sessionmaker | 32 | from sqlalchemy.orm import class_mapper, mapper, relation, scoped_session, sessionmaker |
27 | 30 | from sqlalchemy.orm.exc import UnmappedClassError | 33 | from sqlalchemy.orm.exc import UnmappedClassError |
28 | 31 | 34 | ||
29 | 32 | from openlp.core.common import translate | 35 | from openlp.core.common import translate |
31 | 33 | from openlp.core.lib.db import BaseModel | 36 | from openlp.core.lib.db import BaseModel, upgrade_db |
32 | 34 | from openlp.core.ui.wizard import WizardStrings | 37 | from openlp.core.ui.wizard import WizardStrings |
34 | 35 | from openlp.plugins.songs.lib import clean_song | 38 | from openlp.plugins.songs.lib import clean_song, upgrade |
35 | 36 | from openlp.plugins.songs.lib.db import Author, Book, Song, Topic, MediaFile | 39 | from openlp.plugins.songs.lib.db import Author, Book, Song, Topic, MediaFile |
36 | 37 | from .songimport import SongImport | 40 | from .songimport import SongImport |
37 | 38 | 41 | ||
38 | @@ -67,12 +70,6 @@ | |||
39 | 67 | """ | 70 | """ |
40 | 68 | pass | 71 | pass |
41 | 69 | 72 | ||
42 | 70 | class OldBook(BaseModel): | ||
43 | 71 | """ | ||
44 | 72 | Book model | ||
45 | 73 | """ | ||
46 | 74 | pass | ||
47 | 75 | |||
48 | 76 | class OldMediaFile(BaseModel): | 73 | class OldMediaFile(BaseModel): |
49 | 77 | """ | 74 | """ |
50 | 78 | MediaFile model | 75 | MediaFile model |
51 | @@ -91,26 +88,38 @@ | |||
52 | 91 | """ | 88 | """ |
53 | 92 | pass | 89 | pass |
54 | 93 | 90 | ||
55 | 91 | class OldSongBookEntry(BaseModel): | ||
56 | 92 | """ | ||
57 | 93 | SongbookEntry model | ||
58 | 94 | """ | ||
59 | 95 | pass | ||
60 | 96 | |||
61 | 94 | # Check the file type | 97 | # Check the file type |
62 | 95 | if not self.import_source.endswith('.sqlite'): | 98 | if not self.import_source.endswith('.sqlite'): |
63 | 96 | self.log_error(self.import_source, translate('SongsPlugin.OpenLPSongImport', | 99 | self.log_error(self.import_source, translate('SongsPlugin.OpenLPSongImport', |
64 | 97 | 'Not a valid OpenLP 2 song database.')) | 100 | 'Not a valid OpenLP 2 song database.')) |
65 | 98 | return | 101 | return |
67 | 99 | self.import_source = 'sqlite:///%s' % self.import_source | 102 | # Operate on a copy - We might need to apply migrations to the db and don't want to change the existing db |
68 | 103 | import_db = os.path.join(tempfile.gettempdir(), os.path.basename(self.import_source)) | ||
69 | 104 | shutil.copy(self.import_source, import_db) | ||
70 | 105 | import_db_url = 'sqlite:///%s' % import_db | ||
71 | 106 | # Apply migrations | ||
72 | 107 | upgrade_db(import_db_url, upgrade) | ||
73 | 100 | # Load the db file | 108 | # Load the db file |
75 | 101 | engine = create_engine(self.import_source) | 109 | engine = create_engine(import_db_url) |
76 | 102 | source_meta = MetaData() | 110 | source_meta = MetaData() |
77 | 103 | source_meta.reflect(engine) | 111 | source_meta.reflect(engine) |
78 | 104 | self.source_session = scoped_session(sessionmaker(bind=engine)) | 112 | self.source_session = scoped_session(sessionmaker(bind=engine)) |
79 | 113 | |||
80 | 105 | if 'media_files' in list(source_meta.tables.keys()): | 114 | if 'media_files' in list(source_meta.tables.keys()): |
81 | 106 | has_media_files = True | 115 | has_media_files = True |
82 | 107 | else: | 116 | else: |
83 | 108 | has_media_files = False | 117 | has_media_files = False |
84 | 109 | source_authors_table = source_meta.tables['authors'] | 118 | source_authors_table = source_meta.tables['authors'] |
85 | 110 | source_song_books_table = source_meta.tables['song_books'] | ||
86 | 111 | source_songs_table = source_meta.tables['songs'] | 119 | source_songs_table = source_meta.tables['songs'] |
87 | 112 | source_topics_table = source_meta.tables['topics'] | 120 | source_topics_table = source_meta.tables['topics'] |
88 | 113 | source_authors_songs_table = source_meta.tables['authors_songs'] | 121 | source_authors_songs_table = source_meta.tables['authors_songs'] |
89 | 122 | source_songs_songbooks_table = source_meta.tables['songs_songbooks'] | ||
90 | 114 | source_songs_topics_table = source_meta.tables['songs_topics'] | 123 | source_songs_topics_table = source_meta.tables['songs_topics'] |
91 | 115 | source_media_files_songs_table = None | 124 | source_media_files_songs_table = None |
92 | 116 | if has_media_files: | 125 | if has_media_files: |
93 | @@ -122,8 +131,8 @@ | |||
94 | 122 | mapper(OldMediaFile, source_media_files_table) | 131 | mapper(OldMediaFile, source_media_files_table) |
95 | 123 | song_props = { | 132 | song_props = { |
96 | 124 | 'authors': relation(OldAuthor, backref='songs', secondary=source_authors_songs_table), | 133 | 'authors': relation(OldAuthor, backref='songs', secondary=source_authors_songs_table), |
99 | 125 | 'book': relation(OldBook, backref='songs'), | 134 | 'topics': relation(OldTopic, backref='songs', secondary=source_songs_topics_table), |
100 | 126 | 'topics': relation(OldTopic, backref='songs', secondary=source_songs_topics_table) | 135 | 'songbook_entries': relation(OldSongBookEntry, backref='song'), |
101 | 127 | } | 136 | } |
102 | 128 | if has_media_files: | 137 | if has_media_files: |
103 | 129 | if isinstance(source_media_files_songs_table, Table): | 138 | if isinstance(source_media_files_songs_table, Table): |
104 | @@ -139,10 +148,6 @@ | |||
105 | 139 | except UnmappedClassError: | 148 | except UnmappedClassError: |
106 | 140 | mapper(OldAuthor, source_authors_table) | 149 | mapper(OldAuthor, source_authors_table) |
107 | 141 | try: | 150 | try: |
108 | 142 | class_mapper(OldBook) | ||
109 | 143 | except UnmappedClassError: | ||
110 | 144 | mapper(OldBook, source_song_books_table) | ||
111 | 145 | try: | ||
112 | 146 | class_mapper(OldSong) | 151 | class_mapper(OldSong) |
113 | 147 | except UnmappedClassError: | 152 | except UnmappedClassError: |
114 | 148 | mapper(OldSong, source_songs_table, properties=song_props) | 153 | mapper(OldSong, source_songs_table, properties=song_props) |
115 | @@ -150,6 +155,10 @@ | |||
116 | 150 | class_mapper(OldTopic) | 155 | class_mapper(OldTopic) |
117 | 151 | except UnmappedClassError: | 156 | except UnmappedClassError: |
118 | 152 | mapper(OldTopic, source_topics_table) | 157 | mapper(OldTopic, source_topics_table) |
119 | 158 | try: | ||
120 | 159 | class_mapper(OldSongBookEntry) | ||
121 | 160 | except UnmappedClassError: | ||
122 | 161 | mapper(OldSongBookEntry, source_songs_songbooks_table) | ||
123 | 153 | 162 | ||
124 | 154 | source_songs = self.source_session.query(OldSong).all() | 163 | source_songs = self.source_session.query(OldSong).all() |
125 | 155 | if self.import_wizard: | 164 | if self.import_wizard: |
126 | @@ -166,7 +175,6 @@ | |||
127 | 166 | # Values will be set when cleaning the song. | 175 | # Values will be set when cleaning the song. |
128 | 167 | new_song.search_title = '' | 176 | new_song.search_title = '' |
129 | 168 | new_song.search_lyrics = '' | 177 | new_song.search_lyrics = '' |
130 | 169 | new_song.song_number = song.song_number | ||
131 | 170 | new_song.lyrics = song.lyrics | 178 | new_song.lyrics = song.lyrics |
132 | 171 | new_song.verse_order = song.verse_order | 179 | new_song.verse_order = song.verse_order |
133 | 172 | new_song.copyright = song.copyright | 180 | new_song.copyright = song.copyright |
134 | @@ -181,11 +189,12 @@ | |||
135 | 181 | last_name=author.last_name, | 189 | last_name=author.last_name, |
136 | 182 | display_name=author.display_name) | 190 | display_name=author.display_name) |
137 | 183 | new_song.add_author(existing_author) | 191 | new_song.add_author(existing_author) |
140 | 184 | if song.book: | 192 | for songbook_entry in song.songbook_entries: |
141 | 185 | existing_song_book = self.manager.get_object_filtered(Book, Book.name == song.book.name) | 193 | existing_song_book = self.manager.get_object_filtered(Book, Book.name == songbook_entry.book.name) |
142 | 186 | if existing_song_book is None: | 194 | if existing_song_book is None: |
145 | 187 | existing_song_book = Book.populate(name=song.book.name, publisher=song.book.publisher) | 195 | existing_song_book = Book.populate(name=songbook_entry.book.name, |
146 | 188 | new_song.book = existing_song_book | 196 | publisher=songbook_entry.book.publisher) |
147 | 197 | new_song.add_songbook_entry(existing_song_book, songbook_entry.entry) | ||
148 | 189 | if song.topics: | 198 | if song.topics: |
149 | 190 | for topic in song.topics: | 199 | for topic in song.topics: |
150 | 191 | existing_topic = self.manager.get_object_filtered(Topic, Topic.name == topic.name) | 200 | existing_topic = self.manager.get_object_filtered(Topic, Topic.name == topic.name) |
151 | @@ -212,3 +221,4 @@ | |||
152 | 212 | break | 221 | break |
153 | 213 | self.source_session.close() | 222 | self.source_session.close() |
154 | 214 | engine.dispose() | 223 | engine.dispose() |
155 | 224 | os.remove(import_db) | ||
156 | 215 | 225 | ||
157 | === added file 'tests/utils/test_pep8.py' | |||
158 | --- tests/utils/test_pep8.py 1970-01-01 00:00:00 +0000 | |||
159 | +++ tests/utils/test_pep8.py 2016-01-15 21:49:07 +0000 | |||
160 | @@ -0,0 +1,89 @@ | |||
161 | 1 | # -*- coding: utf-8 -*- | ||
162 | 2 | # vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4 | ||
163 | 3 | |||
164 | 4 | ############################################################################### | ||
165 | 5 | # OpenLP - Open Source Lyrics Projection # | ||
166 | 6 | # --------------------------------------------------------------------------- # | ||
167 | 7 | # Copyright (c) 2008-2016 OpenLP Developers # | ||
168 | 8 | # --------------------------------------------------------------------------- # | ||
169 | 9 | # This program is free software; you can redistribute it and/or modify it # | ||
170 | 10 | # under the terms of the GNU General Public License as published by the Free # | ||
171 | 11 | # Software Foundation; version 2 of the License. # | ||
172 | 12 | # # | ||
173 | 13 | # This program is distributed in the hope that it will be useful, but WITHOUT # | ||
174 | 14 | # ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or # | ||
175 | 15 | # FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for # | ||
176 | 16 | # more details. # | ||
177 | 17 | # # | ||
178 | 18 | # You should have received a copy of the GNU General Public License along # | ||
179 | 19 | # with this program; if not, write to the Free Software Foundation, Inc., 59 # | ||
180 | 20 | # Temple Place, Suite 330, Boston, MA 02111-1307 USA # | ||
181 | 21 | ############################################################################### | ||
182 | 22 | |||
183 | 23 | """ | ||
184 | 24 | Package to test for pep8 errors | ||
185 | 25 | Inspired by http://blog.jameskyle.org/2014/05/pep8-pylint-tests-with-nose-xunit/ | ||
186 | 26 | """ | ||
187 | 27 | |||
188 | 28 | import os | ||
189 | 29 | import pep8 | ||
190 | 30 | import re | ||
191 | 31 | |||
192 | 32 | from unittest import TestCase | ||
193 | 33 | |||
194 | 34 | from tests.utils.constants import OPENLP_PATH | ||
195 | 35 | from tests.utils import is_running_on_ci | ||
196 | 36 | |||
197 | 37 | PROJ_ROOT = 'openlp' | ||
198 | 38 | |||
199 | 39 | |||
200 | 40 | class TestPep8(TestCase): | ||
201 | 41 | |||
202 | 42 | def pep8_test(self): | ||
203 | 43 | """ | ||
204 | 44 | Test for pep8 conformance | ||
205 | 45 | """ | ||
206 | 46 | pattern = re.compile(r'.*({0}.*\.py)'.format(PROJ_ROOT)) | ||
207 | 47 | pep8style = pep8.StyleGuide(reporter=CustomReport, config_file=os.path.join(OPENLP_PATH, 'setup.cfg')) | ||
208 | 48 | |||
209 | 49 | report = pep8style.init_report() | ||
210 | 50 | pep8style.input_dir(OPENLP_PATH) | ||
211 | 51 | |||
212 | 52 | for error in report.results: | ||
213 | 53 | msg = "{path}: {code} {row}, {col} - {text}" | ||
214 | 54 | |||
215 | 55 | match = pattern.match(error['path']) | ||
216 | 56 | if match: | ||
217 | 57 | rel_path = match.group(1) | ||
218 | 58 | else: | ||
219 | 59 | rel_path = error['path'] | ||
220 | 60 | |||
221 | 61 | print(msg.format( | ||
222 | 62 | path=rel_path, | ||
223 | 63 | code=error['code'], | ||
224 | 64 | row=error['row'], | ||
225 | 65 | col=error['col'], | ||
226 | 66 | text=error['text'] | ||
227 | 67 | )) | ||
228 | 68 | if report.results: | ||
229 | 69 | self.fail("Please fix the PEP8 warnings.") | ||
230 | 70 | |||
231 | 71 | |||
232 | 72 | class CustomReport(pep8.StandardReport): | ||
233 | 73 | """ | ||
234 | 74 | Collect report into an array of results. | ||
235 | 75 | """ | ||
236 | 76 | results = [] | ||
237 | 77 | |||
238 | 78 | def get_file_results(self): | ||
239 | 79 | if self._deferred_print: | ||
240 | 80 | self._deferred_print.sort() | ||
241 | 81 | for line_number, offset, code, text, _ in self._deferred_print: | ||
242 | 82 | self.results.append({ | ||
243 | 83 | 'path': self.filename, | ||
244 | 84 | 'row': self.line_offset + line_number, | ||
245 | 85 | 'col': offset + 1, | ||
246 | 86 | 'code': code, | ||
247 | 87 | 'text': text, | ||
248 | 88 | }) | ||
249 | 89 | return self.file_errors |