Merge lp:~raoul-snyman/openlp/db-upgrades-2.4 into lp:openlp/2.4
- db-upgrades-2.4
- Merge into 2.4
Proposed by
Raoul Snyman
Status: | Merged |
---|---|
Merged at revision: | 2676 |
Proposed branch: | lp:~raoul-snyman/openlp/db-upgrades-2.4 |
Merge into: | lp:openlp/2.4 |
Diff against target: |
251 lines (+120/-20) 4 files modified
CHANGELOG.rst (+8/-0) openlp/plugins/songs/lib/importers/openlp.py (+31/-2) openlp/plugins/songs/lib/upgrade.py (+32/-16) tests/interfaces/openlp_plugins/songs/forms/test_editsongform.py (+49/-2) |
To merge this branch: | bzr merge lp:~raoul-snyman/openlp/db-upgrades-2.4 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tim Bentley | Approve | ||
Review via email: mp+319385@code.launchpad.net |
Commit message
Description of the change
Add this to your merge proposal:
-------
lp:~raoul-snyman/openlp/db-upgrades-2.4 (revision 2681)
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
To post a comment you must log in.
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 | === added file 'CHANGELOG.rst' | |||
2 | --- CHANGELOG.rst 1970-01-01 00:00:00 +0000 | |||
3 | +++ CHANGELOG.rst 2017-03-09 05:46:09 +0000 | |||
4 | @@ -0,0 +1,8 @@ | |||
5 | 1 | OpenLP 2.4.6 | ||
6 | 2 | ============ | ||
7 | 3 | |||
8 | 4 | * Fixed a bug where the author type upgrade was being ignore because it was looking at the wrong table | ||
9 | 5 | * Fixed a bug where the songs_songbooks table was not being created because the if expression was the wrong way round | ||
10 | 6 | * Changed the songs_songbooks migration SQL slightly to take into account a bug that has (hopefully) been fixed | ||
11 | 7 | * Add another upgrade step to remove erroneous songs_songbooks entries due to the previous bug | ||
12 | 8 | * Added importing of author types to the OpenLP 2 song importer | ||
13 | 0 | 9 | ||
14 | === modified file 'openlp/plugins/songs/lib/importers/openlp.py' | |||
15 | --- openlp/plugins/songs/lib/importers/openlp.py 2017-03-01 18:24:27 +0000 | |||
16 | +++ openlp/plugins/songs/lib/importers/openlp.py 2017-03-09 05:46:09 +0000 | |||
17 | @@ -61,6 +61,12 @@ | |||
18 | 61 | :param progress_dialog: The QProgressDialog used when importing songs from the FRW. | 61 | :param progress_dialog: The QProgressDialog used when importing songs from the FRW. |
19 | 62 | """ | 62 | """ |
20 | 63 | 63 | ||
21 | 64 | class OldAuthorSong(BaseModel): | ||
22 | 65 | """ | ||
23 | 66 | Maps to the authors table | ||
24 | 67 | """ | ||
25 | 68 | pass | ||
26 | 69 | |||
27 | 64 | class OldAuthor(BaseModel): | 70 | class OldAuthor(BaseModel): |
28 | 65 | """ | 71 | """ |
29 | 66 | Maps to the authors table | 72 | Maps to the authors table |
30 | @@ -117,6 +123,10 @@ | |||
31 | 117 | has_songs_books = True | 123 | has_songs_books = True |
32 | 118 | else: | 124 | else: |
33 | 119 | has_songs_books = False | 125 | has_songs_books = False |
34 | 126 | if 'authors_songs' in list(source_meta.tables.keys()): | ||
35 | 127 | has_authors_songs = True | ||
36 | 128 | else: | ||
37 | 129 | has_authors_songs = False | ||
38 | 120 | # Load up the tabls and map them out | 130 | # Load up the tabls and map them out |
39 | 121 | source_authors_table = source_meta.tables['authors'] | 131 | source_authors_table = source_meta.tables['authors'] |
40 | 122 | source_song_books_table = source_meta.tables['song_books'] | 132 | source_song_books_table = source_meta.tables['song_books'] |
41 | @@ -139,6 +149,10 @@ | |||
42 | 139 | class_mapper(OldSongBookEntry) | 149 | class_mapper(OldSongBookEntry) |
43 | 140 | except UnmappedClassError: | 150 | except UnmappedClassError: |
44 | 141 | mapper(OldSongBookEntry, source_songs_songbooks_table, properties={'songbook': relation(OldBook)}) | 151 | mapper(OldSongBookEntry, source_songs_songbooks_table, properties={'songbook': relation(OldBook)}) |
45 | 152 | if has_authors_songs and 'author_type' in source_authors_songs_table.c.values(): | ||
46 | 153 | has_author_type = True | ||
47 | 154 | else: | ||
48 | 155 | has_author_type = False | ||
49 | 142 | # Set up the songs relationships | 156 | # Set up the songs relationships |
50 | 143 | song_props = { | 157 | song_props = { |
51 | 144 | 'authors': relation(OldAuthor, backref='songs', secondary=source_authors_songs_table), | 158 | 'authors': relation(OldAuthor, backref='songs', secondary=source_authors_songs_table), |
52 | @@ -157,6 +171,8 @@ | |||
53 | 157 | song_props['songbook_entries'] = relation(OldSongBookEntry, backref='song', cascade='all, delete-orphan') | 171 | song_props['songbook_entries'] = relation(OldSongBookEntry, backref='song', cascade='all, delete-orphan') |
54 | 158 | else: | 172 | else: |
55 | 159 | song_props['book'] = relation(OldBook, backref='songs') | 173 | song_props['book'] = relation(OldBook, backref='songs') |
56 | 174 | if has_authors_songs: | ||
57 | 175 | song_props['authors_songs'] = relation(OldAuthorSong) | ||
58 | 160 | # Map the rest of the tables | 176 | # Map the rest of the tables |
59 | 161 | try: | 177 | try: |
60 | 162 | class_mapper(OldAuthor) | 178 | class_mapper(OldAuthor) |
61 | @@ -174,6 +190,11 @@ | |||
62 | 174 | class_mapper(OldTopic) | 190 | class_mapper(OldTopic) |
63 | 175 | except UnmappedClassError: | 191 | except UnmappedClassError: |
64 | 176 | mapper(OldTopic, source_topics_table) | 192 | mapper(OldTopic, source_topics_table) |
65 | 193 | if has_authors_songs: | ||
66 | 194 | try: | ||
67 | 195 | class_mapper(OldAuthorSong) | ||
68 | 196 | except UnmappedClassError: | ||
69 | 197 | mapper(OldAuthorSong, source_authors_songs_table) | ||
70 | 177 | 198 | ||
71 | 178 | source_songs = self.source_session.query(OldSong).all() | 199 | source_songs = self.source_session.query(OldSong).all() |
72 | 179 | if self.import_wizard: | 200 | if self.import_wizard: |
73 | @@ -205,8 +226,16 @@ | |||
74 | 205 | existing_author = Author.populate( | 226 | existing_author = Author.populate( |
75 | 206 | first_name=author.first_name, | 227 | first_name=author.first_name, |
76 | 207 | last_name=author.last_name, | 228 | last_name=author.last_name, |
79 | 208 | display_name=author.display_name) | 229 | display_name=author.display_name |
80 | 209 | new_song.add_author(existing_author) | 230 | ) |
81 | 231 | # if this is a new database, we need to import the author type too | ||
82 | 232 | author_type = None | ||
83 | 233 | if has_author_type: | ||
84 | 234 | for author_song in song.authors_songs: | ||
85 | 235 | if author_song.author_id == author.id: | ||
86 | 236 | author_type = author_song.author_type | ||
87 | 237 | break | ||
88 | 238 | new_song.add_author(existing_author, author_type) | ||
89 | 210 | # Find or create all the topics and add them to the new song object | 239 | # Find or create all the topics and add them to the new song object |
90 | 211 | if song.topics: | 240 | if song.topics: |
91 | 212 | for topic in song.topics: | 241 | for topic in song.topics: |
92 | 213 | 242 | ||
93 | === modified file 'openlp/plugins/songs/lib/upgrade.py' | |||
94 | --- openlp/plugins/songs/lib/upgrade.py 2016-12-31 11:05:48 +0000 | |||
95 | +++ openlp/plugins/songs/lib/upgrade.py 2017-03-09 05:46:09 +0000 | |||
96 | @@ -26,13 +26,13 @@ | |||
97 | 26 | import logging | 26 | import logging |
98 | 27 | 27 | ||
99 | 28 | from sqlalchemy import Table, Column, ForeignKey, types | 28 | from sqlalchemy import Table, Column, ForeignKey, types |
101 | 29 | from sqlalchemy.sql.expression import func, false, null, text | 29 | from sqlalchemy.sql.expression import func, false, null, text, and_, select |
102 | 30 | 30 | ||
103 | 31 | from openlp.core.lib.db import get_upgrade_op | 31 | from openlp.core.lib.db import get_upgrade_op |
104 | 32 | from openlp.core.utils.db import drop_columns | 32 | from openlp.core.utils.db import drop_columns |
105 | 33 | 33 | ||
106 | 34 | log = logging.getLogger(__name__) | 34 | log = logging.getLogger(__name__) |
108 | 35 | __version__ = 5 | 35 | __version__ = 6 |
109 | 36 | 36 | ||
110 | 37 | 37 | ||
111 | 38 | # TODO: When removing an upgrade path the ftw-data needs updating to the minimum supported version | 38 | # TODO: When removing an upgrade path the ftw-data needs updating to the minimum supported version |
112 | @@ -105,13 +105,15 @@ | |||
113 | 105 | # Since SQLite doesn't support changing the primary key of a table, we need to recreate the table | 105 | # Since SQLite doesn't support changing the primary key of a table, we need to recreate the table |
114 | 106 | # and copy the old values | 106 | # and copy the old values |
115 | 107 | op = get_upgrade_op(session) | 107 | op = get_upgrade_op(session) |
123 | 108 | songs_table = Table('songs', metadata) | 108 | authors_songs = Table('authors_songs', metadata, autoload=True) |
124 | 109 | if 'author_type' not in [col.name for col in songs_table.c.values()]: | 109 | if 'author_type' not in [col.name for col in authors_songs.c.values()]: |
125 | 110 | op.create_table('authors_songs_tmp', | 110 | authors_songs_tmp = op.create_table( |
126 | 111 | Column('author_id', types.Integer(), ForeignKey('authors.id'), primary_key=True), | 111 | 'authors_songs_tmp', |
127 | 112 | Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True), | 112 | Column('author_id', types.Integer(), ForeignKey('authors.id'), primary_key=True), |
128 | 113 | Column('author_type', types.Unicode(255), primary_key=True, | 113 | Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True), |
129 | 114 | nullable=False, server_default=text('""'))) | 114 | Column('author_type', types.Unicode(255), primary_key=True, |
130 | 115 | nullable=False, server_default=text('""')) | ||
131 | 116 | ) | ||
132 | 115 | op.execute('INSERT INTO authors_songs_tmp SELECT author_id, song_id, "" FROM authors_songs') | 117 | op.execute('INSERT INTO authors_songs_tmp SELECT author_id, song_id, "" FROM authors_songs') |
133 | 116 | op.drop_table('authors_songs') | 118 | op.drop_table('authors_songs') |
134 | 117 | op.rename_table('authors_songs_tmp', 'authors_songs') | 119 | op.rename_table('authors_songs_tmp', 'authors_songs') |
135 | @@ -126,20 +128,22 @@ | |||
136 | 126 | This upgrade adds support for multiple songbooks | 128 | This upgrade adds support for multiple songbooks |
137 | 127 | """ | 129 | """ |
138 | 128 | op = get_upgrade_op(session) | 130 | op = get_upgrade_op(session) |
141 | 129 | songs_table = Table('songs', metadata) | 131 | songs_table = Table('songs', metadata, autoload=True) |
142 | 130 | if 'song_book_id' in [col.name for col in songs_table.c.values()]: | 132 | if 'song_book_id' not in [col.name for col in songs_table.c.values()]: |
143 | 131 | log.warning('Skipping upgrade_5 step of upgrading the song db') | 133 | log.warning('Skipping upgrade_5 step of upgrading the song db') |
144 | 132 | return | 134 | return |
145 | 133 | 135 | ||
146 | 134 | # Create the mapping table (songs <-> songbooks) | 136 | # Create the mapping table (songs <-> songbooks) |
151 | 135 | op.create_table('songs_songbooks', | 137 | songs_songbooks_table = op.create_table( |
152 | 136 | Column('songbook_id', types.Integer(), ForeignKey('song_books.id'), primary_key=True), | 138 | 'songs_songbooks', |
153 | 137 | Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True), | 139 | Column('songbook_id', types.Integer(), ForeignKey('song_books.id'), primary_key=True), |
154 | 138 | Column('entry', types.Unicode(255), primary_key=True, nullable=False)) | 140 | Column('song_id', types.Integer(), ForeignKey('songs.id'), primary_key=True), |
155 | 141 | Column('entry', types.Unicode(255), primary_key=True, nullable=False) | ||
156 | 142 | ) | ||
157 | 139 | 143 | ||
158 | 140 | # Migrate old data | 144 | # Migrate old data |
159 | 141 | op.execute('INSERT INTO songs_songbooks SELECT song_book_id, id, song_number FROM songs\ | 145 | op.execute('INSERT INTO songs_songbooks SELECT song_book_id, id, song_number FROM songs\ |
161 | 142 | WHERE song_book_id IS NOT NULL AND song_number IS NOT NULL') | 146 | WHERE song_book_id IS NOT NULL AND song_book_id <> 0 AND song_number IS NOT NULL') |
162 | 143 | 147 | ||
163 | 144 | # Drop old columns | 148 | # Drop old columns |
164 | 145 | if metadata.bind.url.get_dialect().name == 'sqlite': | 149 | if metadata.bind.url.get_dialect().name == 'sqlite': |
165 | @@ -148,3 +152,15 @@ | |||
166 | 148 | op.drop_constraint('songs_ibfk_1', 'songs', 'foreignkey') | 152 | op.drop_constraint('songs_ibfk_1', 'songs', 'foreignkey') |
167 | 149 | op.drop_column('songs', 'song_book_id') | 153 | op.drop_column('songs', 'song_book_id') |
168 | 150 | op.drop_column('songs', 'song_number') | 154 | op.drop_column('songs', 'song_number') |
169 | 155 | |||
170 | 156 | |||
171 | 157 | def upgrade_6(session, metadata): | ||
172 | 158 | """ | ||
173 | 159 | Version 6 upgrade. | ||
174 | 160 | |||
175 | 161 | This is to fix an issue we had with songbooks with an id of "0" being imported in the previous upgrade. | ||
176 | 162 | """ | ||
177 | 163 | op = get_upgrade_op(session) | ||
178 | 164 | songs_songbooks = Table('songs_songbooks', metadata, autoload=True) | ||
179 | 165 | del_query = songs_songbooks.delete().where(songs_songbooks.c.songbook_id == 0) | ||
180 | 166 | op.execute(del_query) | ||
181 | 151 | 167 | ||
182 | === modified file 'tests/interfaces/openlp_plugins/songs/forms/test_editsongform.py' | |||
183 | --- tests/interfaces/openlp_plugins/songs/forms/test_editsongform.py 2016-12-31 11:05:48 +0000 | |||
184 | +++ tests/interfaces/openlp_plugins/songs/forms/test_editsongform.py 2017-03-09 05:46:09 +0000 | |||
185 | @@ -23,13 +23,13 @@ | |||
186 | 23 | Package to test the openlp.plugins.songs.forms.editsongform package. | 23 | Package to test the openlp.plugins.songs.forms.editsongform package. |
187 | 24 | """ | 24 | """ |
188 | 25 | from unittest import TestCase | 25 | from unittest import TestCase |
189 | 26 | from unittest.mock import MagicMock, patch | ||
190 | 26 | 27 | ||
192 | 27 | from PyQt5 import QtWidgets | 28 | from PyQt5 import QtCore, QtWidgets |
193 | 28 | 29 | ||
194 | 29 | from openlp.core.common import Registry | 30 | from openlp.core.common import Registry |
195 | 30 | from openlp.core.common.uistrings import UiStrings | 31 | from openlp.core.common.uistrings import UiStrings |
196 | 31 | from openlp.plugins.songs.forms.editsongform import EditSongForm | 32 | from openlp.plugins.songs.forms.editsongform import EditSongForm |
197 | 32 | from tests.interfaces import MagicMock | ||
198 | 33 | from tests.helpers.testmixin import TestMixin | 33 | from tests.helpers.testmixin import TestMixin |
199 | 34 | 34 | ||
200 | 35 | 35 | ||
201 | @@ -157,3 +157,50 @@ | |||
202 | 157 | 157 | ||
203 | 158 | # THEN: The verse order should be converted to uppercase | 158 | # THEN: The verse order should be converted to uppercase |
204 | 159 | self.assertEqual(form.verse_order_edit.text(), 'V1 V2 C1 V3 C1 V4 C1') | 159 | self.assertEqual(form.verse_order_edit.text(), 'V1 V2 C1 V3 C1 V4 C1') |
205 | 160 | |||
206 | 161 | @patch('openlp.plugins.songs.forms.editsongform.QtWidgets.QListWidgetItem') | ||
207 | 162 | def test_add_author_to_list(self, MockedQListWidgetItem): | ||
208 | 163 | """ | ||
209 | 164 | Test the _add_author_to_list() method | ||
210 | 165 | """ | ||
211 | 166 | # GIVEN: A song edit form and some mocked stuff | ||
212 | 167 | mocked_author = MagicMock() | ||
213 | 168 | mocked_author.id = 1 | ||
214 | 169 | mocked_author.get_display_name.return_value = 'John Newton' | ||
215 | 170 | mocked_author_type = 'words' | ||
216 | 171 | mocked_widget_item = MagicMock() | ||
217 | 172 | MockedQListWidgetItem.return_value = mocked_widget_item | ||
218 | 173 | |||
219 | 174 | # WHEN: _add_author_to_list() is called | ||
220 | 175 | with patch.object(self.form.authors_list_view, 'addItem') as mocked_add_item: | ||
221 | 176 | self.form._add_author_to_list(mocked_author, mocked_author_type) | ||
222 | 177 | |||
223 | 178 | # THEN: All the correct methods should have been called | ||
224 | 179 | mocked_author.get_display_name.assert_called_once_with('words') | ||
225 | 180 | MockedQListWidgetItem.assert_called_once_with('John Newton') | ||
226 | 181 | mocked_widget_item.setData.assert_called_once_with(QtCore.Qt.UserRole, (1, mocked_author_type)) | ||
227 | 182 | mocked_add_item.assert_called_once_with(mocked_widget_item) | ||
228 | 183 | |||
229 | 184 | @patch('openlp.plugins.songs.forms.editsongform.SongBookEntry') | ||
230 | 185 | @patch('openlp.plugins.songs.forms.editsongform.QtWidgets.QListWidgetItem') | ||
231 | 186 | def test_add_songbook_entry_to_list(self, MockedQListWidgetItem, MockedSongbookEntry): | ||
232 | 187 | """ | ||
233 | 188 | Test the add_songbook_entry_to_list() method | ||
234 | 189 | """ | ||
235 | 190 | # GIVEN: A song edit form and some mocked stuff | ||
236 | 191 | songbook_id = 1 | ||
237 | 192 | songbook_name = 'Hymnal' | ||
238 | 193 | entry = '546' | ||
239 | 194 | MockedSongbookEntry.get_display_name.return_value = 'Hymnal #546' | ||
240 | 195 | mocked_widget_item = MagicMock() | ||
241 | 196 | MockedQListWidgetItem.return_value = mocked_widget_item | ||
242 | 197 | |||
243 | 198 | # WHEN: _add_author_to_list() is called | ||
244 | 199 | with patch.object(self.form.songbooks_list_view, 'addItem') as mocked_add_item: | ||
245 | 200 | self.form.add_songbook_entry_to_list(songbook_id, songbook_name, entry) | ||
246 | 201 | |||
247 | 202 | # THEN: All the correct methods should have been called | ||
248 | 203 | MockedSongbookEntry.get_display_name.assert_called_once_with(songbook_name, entry) | ||
249 | 204 | MockedQListWidgetItem.assert_called_once_with('Hymnal #546') | ||
250 | 205 | mocked_widget_item.setData.assert_called_once_with(QtCore.Qt.UserRole, (songbook_id, entry)) | ||
251 | 206 | mocked_add_item.assert_called_once_with(mocked_widget_item) |