Merge lp:~raoul-snyman/openlp/fix-db-upgrade-2.4 into lp:openlp/2.4
- fix-db-upgrade-2.4
- Merge into 2.4
Status: | Merged |
---|---|
Merged at revision: | 2677 |
Proposed branch: | lp:~raoul-snyman/openlp/fix-db-upgrade-2.4 |
Merge into: | lp:openlp/2.4 |
Diff against target: |
398 lines (+304/-19) 6 files modified
CHANGELOG.rst (+3/-1) openlp/core/ui/maindisplay.py (+1/-1) openlp/core/ui/mainwindow.py (+4/-2) openlp/plugins/songs/lib/upgrade.py (+2/-15) tests/interfaces/openlp_plugins/songs/forms/test_editverseform.py (+1/-0) tests/interfaces/openlp_plugins/songs/forms/test_songmaintenanceform.py (+293/-0) |
To merge this branch: | bzr merge lp:~raoul-snyman/openlp/fix-db-upgrade-2.4 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tim Bentley | Approve | ||
Tomas Groth | Approve | ||
Review via email: mp+320587@code.launchpad.net |
Commit message
Description of the change
Revert the database upgrade, fix a few more bugs, and add some tests.
Bugs fixed:
- Sometimes the timer goes off as OpenLP is shutting down, and the application has already been deleted (reported via support system)
- Fix opening the data folder (KDE thought the old way was an SMB share)
- Fix a problem with the new QMediaPlayer not controlling the playlist anymore
Add this to your merge proposal:
-------
lp:~raoul-snyman/openlp/fix-db-upgrade-2.4 (revision 2681)
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
[SUCCESS] https:/
Tim Bentley (trb143) : | # |
Preview Diff
1 | === modified file 'CHANGELOG.rst' |
2 | --- CHANGELOG.rst 2017-03-09 05:36:03 +0000 |
3 | +++ CHANGELOG.rst 2017-03-22 06:21:55 +0000 |
4 | @@ -4,5 +4,7 @@ |
5 | * Fixed a bug where the author type upgrade was being ignore because it was looking at the wrong table |
6 | * Fixed a bug where the songs_songbooks table was not being created because the if expression was the wrong way round |
7 | * Changed the songs_songbooks migration SQL slightly to take into account a bug that has (hopefully) been fixed |
8 | -* Add another upgrade step to remove erroneous songs_songbooks entries due to the previous bug |
9 | +* Sometimes the timer goes off as OpenLP is shutting down, and the application has already been deleted (reported via support system) |
10 | +* Fix opening the data folder (KDE thought the old way was an SMB share) |
11 | +* Fix a problem with the new QMediaPlayer not controlling the playlist anymore |
12 | * Added importing of author types to the OpenLP 2 song importer |
13 | |
14 | === modified file 'openlp/core/ui/maindisplay.py' |
15 | --- openlp/core/ui/maindisplay.py 2016-12-31 11:05:48 +0000 |
16 | +++ openlp/core/ui/maindisplay.py 2017-03-22 06:21:55 +0000 |
17 | @@ -672,7 +672,7 @@ |
18 | """ |
19 | Skip forward to the next track in the list |
20 | """ |
21 | - self.player.next() |
22 | + self.playlist.next() |
23 | |
24 | def go_to(self, index): |
25 | """ |
26 | |
27 | === modified file 'openlp/core/ui/mainwindow.py' |
28 | --- openlp/core/ui/mainwindow.py 2017-01-31 17:41:35 +0000 |
29 | +++ openlp/core/ui/mainwindow.py 2017-03-22 06:21:55 +0000 |
30 | @@ -796,7 +796,7 @@ |
31 | Open data folder |
32 | """ |
33 | path = AppLocation.get_data_path() |
34 | - QtGui.QDesktopServices.openUrl(QtCore.QUrl("file:///" + path)) |
35 | + QtGui.QDesktopServices.openUrl(QtCore.QUrl.fromLocalFile(path)) |
36 | |
37 | def on_update_theme_images(self): |
38 | """ |
39 | @@ -1376,7 +1376,9 @@ |
40 | if event.timerId() == self.timer_id: |
41 | self.timer_id = 0 |
42 | self.load_progress_bar.hide() |
43 | - self.application.process_events() |
44 | + # Sometimes the timer goes off as OpenLP is shutting down, and the application has already been deleted |
45 | + if self.application: |
46 | + self.application.process_events() |
47 | |
48 | def set_new_data_path(self, new_data_path): |
49 | """ |
50 | |
51 | === modified file 'openlp/plugins/songs/lib/upgrade.py' |
52 | --- openlp/plugins/songs/lib/upgrade.py 2017-03-09 05:20:27 +0000 |
53 | +++ openlp/plugins/songs/lib/upgrade.py 2017-03-22 06:21:55 +0000 |
54 | @@ -32,7 +32,7 @@ |
55 | from openlp.core.utils.db import drop_columns |
56 | |
57 | log = logging.getLogger(__name__) |
58 | -__version__ = 6 |
59 | +__version__ = 5 |
60 | |
61 | |
62 | # TODO: When removing an upgrade path the ftw-data needs updating to the minimum supported version |
63 | @@ -102,8 +102,7 @@ |
64 | |
65 | This upgrade adds a column for author type to the authors_songs table |
66 | """ |
67 | - # Since SQLite doesn't support changing the primary key of a table, we need to recreate the table |
68 | - # and copy the old values |
69 | + # Due to an incorrect check, this step was always skipped. Moved this code into upgrade 6. |
70 | op = get_upgrade_op(session) |
71 | authors_songs = Table('authors_songs', metadata, autoload=True) |
72 | if 'author_type' not in [col.name for col in authors_songs.c.values()]: |
73 | @@ -152,15 +151,3 @@ |
74 | op.drop_constraint('songs_ibfk_1', 'songs', 'foreignkey') |
75 | op.drop_column('songs', 'song_book_id') |
76 | op.drop_column('songs', 'song_number') |
77 | - |
78 | - |
79 | -def upgrade_6(session, metadata): |
80 | - """ |
81 | - Version 6 upgrade. |
82 | - |
83 | - This is to fix an issue we had with songbooks with an id of "0" being imported in the previous upgrade. |
84 | - """ |
85 | - op = get_upgrade_op(session) |
86 | - songs_songbooks = Table('songs_songbooks', metadata, autoload=True) |
87 | - del_query = songs_songbooks.delete().where(songs_songbooks.c.songbook_id == 0) |
88 | - op.execute(del_query) |
89 | |
90 | === modified file 'tests/interfaces/openlp_plugins/songs/forms/test_editverseform.py' |
91 | --- tests/interfaces/openlp_plugins/songs/forms/test_editverseform.py 2016-12-31 11:05:48 +0000 |
92 | +++ tests/interfaces/openlp_plugins/songs/forms/test_editverseform.py 2017-03-22 06:21:55 +0000 |
93 | @@ -28,6 +28,7 @@ |
94 | |
95 | from openlp.core.common import Registry |
96 | from openlp.plugins.songs.forms.editverseform import EditVerseForm |
97 | + |
98 | from tests.helpers.testmixin import TestMixin |
99 | |
100 | |
101 | |
102 | === added file 'tests/interfaces/openlp_plugins/songs/forms/test_songmaintenanceform.py' |
103 | --- tests/interfaces/openlp_plugins/songs/forms/test_songmaintenanceform.py 1970-01-01 00:00:00 +0000 |
104 | +++ tests/interfaces/openlp_plugins/songs/forms/test_songmaintenanceform.py 2017-03-22 06:21:55 +0000 |
105 | @@ -0,0 +1,293 @@ |
106 | +# -*- coding: utf-8 -*- |
107 | +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4 |
108 | + |
109 | +############################################################################### |
110 | +# OpenLP - Open Source Lyrics Projection # |
111 | +# --------------------------------------------------------------------------- # |
112 | +# Copyright (c) 2008-2017 OpenLP Developers # |
113 | +# --------------------------------------------------------------------------- # |
114 | +# This program is free software; you can redistribute it and/or modify it # |
115 | +# under the terms of the GNU General Public License as published by the Free # |
116 | +# Software Foundation; version 2 of the License. # |
117 | +# # |
118 | +# This program is distributed in the hope that it will be useful, but WITHOUT # |
119 | +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or # |
120 | +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for # |
121 | +# more details. # |
122 | +# # |
123 | +# You should have received a copy of the GNU General Public License along # |
124 | +# with this program; if not, write to the Free Software Foundation, Inc., 59 # |
125 | +# Temple Place, Suite 330, Boston, MA 02111-1307 USA # |
126 | +############################################################################### |
127 | +""" |
128 | +Package to test the openlp.plugins.songs.forms.songmaintenanceform package. |
129 | +""" |
130 | +from unittest import TestCase |
131 | +from unittest.mock import MagicMock, patch, call |
132 | + |
133 | +from PyQt5 import QtCore, QtTest, QtWidgets |
134 | + |
135 | +from openlp.core.common import Registry, UiStrings |
136 | +from openlp.plugins.songs.forms.songmaintenanceform import SongMaintenanceForm |
137 | + |
138 | +from tests.helpers.testmixin import TestMixin |
139 | + |
140 | + |
141 | +class TestSongMaintenanceForm(TestCase, TestMixin): |
142 | + """ |
143 | + Test the SongMaintenanceForm class |
144 | + """ |
145 | + |
146 | + def setUp(self): |
147 | + """ |
148 | + Create the UI |
149 | + """ |
150 | + Registry.create() |
151 | + self.setup_application() |
152 | + self.main_window = QtWidgets.QMainWindow() |
153 | + Registry().register('main_window', self.main_window) |
154 | + self.mocked_manager = MagicMock() |
155 | + self.form = SongMaintenanceForm(self.mocked_manager) |
156 | + |
157 | + def tearDown(self): |
158 | + """ |
159 | + Delete all the C++ objects at the end so that we don't have a segfault |
160 | + """ |
161 | + del self.form |
162 | + del self.main_window |
163 | + |
164 | + def test_constructor(self): |
165 | + """ |
166 | + Test that a SongMaintenanceForm is created successfully |
167 | + """ |
168 | + # GIVEN: A SongMaintenanceForm |
169 | + # WHEN: The form is created |
170 | + # THEN: It should have some of the right components |
171 | + assert self.form is not None |
172 | + assert self.form.manager is self.mocked_manager |
173 | + |
174 | + @patch.object(QtWidgets.QDialog, 'exec') |
175 | + def test_exect(self, mocked_exec): |
176 | + """ |
177 | + Test the song maintenance form being executed |
178 | + """ |
179 | + # GIVEN: A song maintenance form |
180 | + mocked_exec.return_value = True |
181 | + |
182 | + # WHEN: The song mainetnance form is executed |
183 | + with patch.object(self.form, 'type_list_widget') as mocked_type_list_widget, \ |
184 | + patch.object(self.form, 'reset_authors') as mocked_reset_authors, \ |
185 | + patch.object(self.form, 'reset_topics') as mocked_reset_topics, \ |
186 | + patch.object(self.form, 'reset_song_books') as mocked_reset_song_books: |
187 | + result = self.form.exec(from_song_edit=True) |
188 | + |
189 | + # THEN: The correct methods should have been called |
190 | + assert self.form.from_song_edit is True |
191 | + mocked_type_list_widget.setCurrentRow.assert_called_once_with(0) |
192 | + mocked_reset_authors.assert_called_once_with() |
193 | + mocked_reset_topics.assert_called_once_with() |
194 | + mocked_reset_song_books.assert_called_once_with() |
195 | + mocked_type_list_widget.setFocus.assert_called_once_with() |
196 | + mocked_exec.assert_called_once_with(self.form) |
197 | + |
198 | + def test_get_current_item_id_no_item(self): |
199 | + """ |
200 | + Test _get_current_item_id() when there's no item |
201 | + """ |
202 | + # GIVEN: A song maintenance form without a selected item |
203 | + mocked_list_widget = MagicMock() |
204 | + mocked_list_widget.currentItem.return_value = None |
205 | + |
206 | + # WHEN: _get_current_item_id() is called |
207 | + result = self.form._get_current_item_id(mocked_list_widget) |
208 | + |
209 | + # THEN: The result should be -1 |
210 | + mocked_list_widget.currentItem.assert_called_once_with() |
211 | + assert result == -1 |
212 | + |
213 | + def test_get_current_item_id(self): |
214 | + """ |
215 | + Test _get_current_item_id() when there's a valid item |
216 | + """ |
217 | + # GIVEN: A song maintenance form with a selected item |
218 | + mocked_item = MagicMock() |
219 | + mocked_item.data.return_value = 7 |
220 | + mocked_list_widget = MagicMock() |
221 | + mocked_list_widget.currentItem.return_value = mocked_item |
222 | + |
223 | + # WHEN: _get_current_item_id() is called |
224 | + result = self.form._get_current_item_id(mocked_list_widget) |
225 | + |
226 | + # THEN: The result should be -1 |
227 | + mocked_list_widget.currentItem.assert_called_once_with() |
228 | + mocked_item.data.assert_called_once_with(QtCore.Qt.UserRole) |
229 | + assert result == 7 |
230 | + |
231 | + @patch('openlp.plugins.songs.forms.songmaintenanceform.critical_error_message_box') |
232 | + def test_delete_item_no_item_id(self, mocked_critical_error_message_box): |
233 | + """ |
234 | + Test the _delete_item() method when there is no item selected |
235 | + """ |
236 | + # GIVEN: Some mocked items |
237 | + mocked_item_class = MagicMock() |
238 | + mocked_list_widget = MagicMock() |
239 | + mocked_reset_func = MagicMock() |
240 | + dialog_title = 'Delete Item' |
241 | + delete_text = 'Are you sure you want to delete this item?' |
242 | + error_text = 'There was a problem deleting this item' |
243 | + |
244 | + # WHEN: _delete_item() is called |
245 | + with patch.object(self.form, '_get_current_item_id') as mocked_get_current_item_id: |
246 | + mocked_get_current_item_id.return_value = -1 |
247 | + self.form._delete_item(mocked_item_class, mocked_list_widget, mocked_reset_func, dialog_title, delete_text, |
248 | + error_text) |
249 | + |
250 | + # THEN: The right things should have been called |
251 | + mocked_get_current_item_id.assert_called_once_with(mocked_list_widget) |
252 | + mocked_critical_error_message_box.assert_called_once_with(dialog_title, UiStrings().NISs) |
253 | + |
254 | + @patch('openlp.plugins.songs.forms.songmaintenanceform.critical_error_message_box') |
255 | + def test_delete_item_invalid_item(self, mocked_critical_error_message_box): |
256 | + """ |
257 | + Test the _delete_item() method when the item doesn't exist in the database |
258 | + """ |
259 | + # GIVEN: Some mocked items |
260 | + self.mocked_manager.get_object.return_value = None |
261 | + mocked_item_class = MagicMock() |
262 | + mocked_list_widget = MagicMock() |
263 | + mocked_reset_func = MagicMock() |
264 | + dialog_title = 'Delete Item' |
265 | + delete_text = 'Are you sure you want to delete this item?' |
266 | + error_text = 'There was a problem deleting this item' |
267 | + |
268 | + # WHEN: _delete_item() is called |
269 | + with patch.object(self.form, '_get_current_item_id') as mocked_get_current_item_id: |
270 | + mocked_get_current_item_id.return_value = 1 |
271 | + self.form._delete_item(mocked_item_class, mocked_list_widget, mocked_reset_func, dialog_title, delete_text, |
272 | + error_text) |
273 | + |
274 | + # THEN: The right things should have been called |
275 | + mocked_get_current_item_id.assert_called_once_with(mocked_list_widget) |
276 | + self.mocked_manager.get_object.assert_called_once_with(mocked_item_class, 1) |
277 | + mocked_critical_error_message_box.assert_called_once_with(dialog_title, error_text) |
278 | + |
279 | + @patch('openlp.plugins.songs.forms.songmaintenanceform.critical_error_message_box') |
280 | + def test_delete_item(self, mocked_critical_error_message_box): |
281 | + """ |
282 | + Test the _delete_item() method |
283 | + """ |
284 | + # GIVEN: Some mocked items |
285 | + mocked_item = MagicMock() |
286 | + mocked_item.songs = [] |
287 | + mocked_item.id = 1 |
288 | + self.mocked_manager.get_object.return_value = mocked_item |
289 | + mocked_critical_error_message_box.return_value = QtWidgets.QMessageBox.Yes |
290 | + mocked_item_class = MagicMock() |
291 | + mocked_list_widget = MagicMock() |
292 | + mocked_reset_func = MagicMock() |
293 | + dialog_title = 'Delete Item' |
294 | + delete_text = 'Are you sure you want to delete this item?' |
295 | + error_text = 'There was a problem deleting this item' |
296 | + |
297 | + # WHEN: _delete_item() is called |
298 | + with patch.object(self.form, '_get_current_item_id') as mocked_get_current_item_id: |
299 | + mocked_get_current_item_id.return_value = 1 |
300 | + self.form._delete_item(mocked_item_class, mocked_list_widget, mocked_reset_func, dialog_title, delete_text, |
301 | + error_text) |
302 | + |
303 | + # THEN: The right things should have been called |
304 | + mocked_get_current_item_id.assert_called_once_with(mocked_list_widget) |
305 | + self.mocked_manager.get_object.assert_called_once_with(mocked_item_class, 1) |
306 | + mocked_critical_error_message_box.assert_called_once_with(dialog_title, delete_text, self.form, True) |
307 | + self.mocked_manager.delete_object(mocked_item_class, 1) |
308 | + mocked_reset_func.assert_called_once_with() |
309 | + |
310 | + @patch('openlp.plugins.songs.forms.songmaintenanceform.QtWidgets.QListWidgetItem') |
311 | + @patch('openlp.plugins.songs.forms.songmaintenanceform.Author') |
312 | + def test_reset_authors(self, MockedAuthor, MockedQListWidgetItem): |
313 | + """ |
314 | + Test the reset_authors() method |
315 | + """ |
316 | + # GIVEN: A mocked authors_list_widget and a few other mocks |
317 | + mocked_author1 = MagicMock() |
318 | + mocked_author1.display_name = 'John Newton' |
319 | + mocked_author1.id = 1 |
320 | + mocked_author2 = MagicMock() |
321 | + mocked_author2.display_name = None |
322 | + mocked_author2.first_name = 'John' |
323 | + mocked_author2.last_name = 'Wesley' |
324 | + mocked_author2.id = 2 |
325 | + mocked_authors = [mocked_author1, mocked_author2] |
326 | + mocked_author_item1 = MagicMock() |
327 | + mocked_author_item2 = MagicMock() |
328 | + MockedQListWidgetItem.side_effect = [mocked_author_item1, mocked_author_item2] |
329 | + MockedAuthor.display_name = None |
330 | + self.mocked_manager.get_all_objects.return_value = mocked_authors |
331 | + |
332 | + # WHEN: reset_authors() is called |
333 | + with patch.object(self.form, 'authors_list_widget') as mocked_authors_list_widget: |
334 | + self.form.reset_authors() |
335 | + |
336 | + # THEN: The authors list should be reset |
337 | + expected_widget_item_calls = [call('John Newton'), call('John Wesley')] |
338 | + mocked_authors_list_widget.clear.assert_called_once_with() |
339 | + self.mocked_manager.get_all_objects.assert_called_once_with(MockedAuthor, |
340 | + order_by_ref=MockedAuthor.display_name) |
341 | + assert MockedQListWidgetItem.call_args_list == expected_widget_item_calls, MockedQListWidgetItem.call_args_list |
342 | + mocked_author_item1.setData.assert_called_once_with(QtCore.Qt.UserRole, 1) |
343 | + mocked_author_item2.setData.assert_called_once_with(QtCore.Qt.UserRole, 2) |
344 | + mocked_authors_list_widget.addItem.call_args_list == [ |
345 | + call(mocked_author_item1), call(mocked_author_item2)] |
346 | + |
347 | + @patch('openlp.plugins.songs.forms.songmaintenanceform.QtWidgets.QListWidgetItem') |
348 | + @patch('openlp.plugins.songs.forms.songmaintenanceform.Topic') |
349 | + def test_reset_topics(self, MockedTopic, MockedQListWidgetItem): |
350 | + """ |
351 | + Test the reset_topics() method |
352 | + """ |
353 | + # GIVEN: Some mocked out objects and methods |
354 | + MockedTopic.name = 'Grace' |
355 | + mocked_topic = MagicMock() |
356 | + mocked_topic.id = 1 |
357 | + mocked_topic.name = 'Grace' |
358 | + self.mocked_manager.get_all_objects.return_value = [mocked_topic] |
359 | + mocked_topic_item = MagicMock() |
360 | + MockedQListWidgetItem.return_value = mocked_topic_item |
361 | + |
362 | + # WHEN: reset_topics() is called |
363 | + with patch.object(self.form, 'topics_list_widget') as mocked_topic_list_widget: |
364 | + self.form.reset_topics() |
365 | + |
366 | + # THEN: The topics list should be reset correctly |
367 | + mocked_topic_list_widget.clear.assert_called_once_with() |
368 | + self.mocked_manager.get_all_objects.assert_called_once_with(MockedTopic, order_by_ref=MockedTopic.name) |
369 | + MockedQListWidgetItem.assert_called_once_with('Grace') |
370 | + mocked_topic_item.setData.assert_called_once_with(QtCore.Qt.UserRole, 1) |
371 | + mocked_topic_list_widget.addItem.assert_called_once_with(mocked_topic_item) |
372 | + |
373 | + @patch('openlp.plugins.songs.forms.songmaintenanceform.QtWidgets.QListWidgetItem') |
374 | + @patch('openlp.plugins.songs.forms.songmaintenanceform.Book') |
375 | + def test_reset_song_books(self, MockedBook, MockedQListWidgetItem): |
376 | + """ |
377 | + Test the reset_song_books() method |
378 | + """ |
379 | + # GIVEN: Some mocked out objects and methods |
380 | + MockedBook.name = 'Hymnal' |
381 | + mocked_song_book = MagicMock() |
382 | + mocked_song_book.id = 1 |
383 | + mocked_song_book.name = 'Hymnal' |
384 | + mocked_song_book.publisher = 'Hymns and Psalms, Inc.' |
385 | + self.mocked_manager.get_all_objects.return_value = [mocked_song_book] |
386 | + mocked_song_book_item = MagicMock() |
387 | + MockedQListWidgetItem.return_value = mocked_song_book_item |
388 | + |
389 | + # WHEN: reset_song_books() is called |
390 | + with patch.object(self.form, 'song_books_list_widget') as mocked_song_book_list_widget: |
391 | + self.form.reset_song_books() |
392 | + |
393 | + # THEN: The song_books list should be reset correctly |
394 | + mocked_song_book_list_widget.clear.assert_called_once_with() |
395 | + self.mocked_manager.get_all_objects.assert_called_once_with(MockedBook, order_by_ref=MockedBook.name) |
396 | + MockedQListWidgetItem.assert_called_once_with('Hymnal (Hymns and Psalms, Inc.)') |
397 | + mocked_song_book_item.setData.assert_called_once_with(QtCore.Qt.UserRole, 1) |
398 | + mocked_song_book_list_widget.addItem.assert_called_once_with(mocked_song_book_item) |
Looks ok to me