Merge lp:~raoul-snyman/openlp/off-by-one-2.4 into lp:openlp/2.4
- off-by-one-2.4
- Merge into 2.4
Proposed by
Raoul Snyman
Status: | Merged | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Merged at revision: | 2674 | ||||||||||||
Proposed branch: | lp:~raoul-snyman/openlp/off-by-one-2.4 | ||||||||||||
Merge into: | lp:openlp/2.4 | ||||||||||||
Diff against target: |
410 lines (+247/-25) 3 files modified
openlp/plugins/songs/forms/editsongform.py (+30/-24) tests/functional/openlp_plugins/songs/test_editsongform.py (+2/-1) tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py (+215/-0) |
||||||||||||
To merge this branch: | bzr merge lp:~raoul-snyman/openlp/off-by-one-2.4 | ||||||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Tim Bentley | Approve | ||
Review via email: mp+318720@code.launchpad.net |
This proposal supersedes a proposal from 2017-03-02.
Commit message
Description of the change
Fix bug #1666005 and bug #1668994
Add this to your merge proposal:
-------
lp:~raoul-snyman/openlp/off-by-one-2.4 (revision 2676)
[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 | === modified file 'openlp/plugins/songs/forms/editsongform.py' |
2 | --- openlp/plugins/songs/forms/editsongform.py 2016-12-31 11:05:48 +0000 |
3 | +++ openlp/plugins/songs/forms/editsongform.py 2017-03-02 05:06:04 +0000 |
4 | @@ -31,7 +31,8 @@ |
5 | |
6 | from PyQt5 import QtCore, QtWidgets |
7 | |
8 | -from openlp.core.common import Registry, RegistryProperties, AppLocation, UiStrings, check_directory_exists, translate |
9 | +from openlp.core.common import Registry, RegistryProperties, AppLocation, UiStrings, check_directory_exists, \ |
10 | + translate, is_macosx |
11 | from openlp.core.lib import FileDialog, PluginStatus, MediaType, create_separated_list |
12 | from openlp.core.lib.ui import set_case_insensitive_completer, critical_error_message_box, find_and_set_in_combo_box |
13 | from openlp.plugins.songs.lib import VerseType, clean_song |
14 | @@ -118,7 +119,8 @@ |
15 | cache.append(obj.name) |
16 | combo.setItemData(row, obj.id) |
17 | set_case_insensitive_completer(cache, combo) |
18 | - combo.setEditText('') |
19 | + combo.setCurrentIndex(-1) |
20 | + combo.setCurrentText('') |
21 | |
22 | def _add_author_to_list(self, author, author_type): |
23 | """ |
24 | @@ -352,7 +354,8 @@ |
25 | self.authors_combo_box.setItemData(row, author.id) |
26 | self.authors.append(author.display_name) |
27 | set_case_insensitive_completer(self.authors, self.authors_combo_box) |
28 | - self.authors_combo_box.setEditText('') |
29 | + self.authors_combo_box.setCurrentIndex(-1) |
30 | + self.authors_combo_box.setCurrentText('') |
31 | |
32 | # Types |
33 | self.author_types_combo_box.clear() |
34 | @@ -382,7 +385,8 @@ |
35 | self.themes = theme_list |
36 | self.theme_combo_box.addItems(theme_list) |
37 | set_case_insensitive_completer(self.themes, self.theme_combo_box) |
38 | - self.theme_combo_box.setEditText('') |
39 | + self.theme_combo_box.setCurrentIndex(-1) |
40 | + self.theme_combo_box.setCurrentText('') |
41 | |
42 | def load_media_files(self): |
43 | """ |
44 | @@ -421,7 +425,8 @@ |
45 | self.load_topics() |
46 | self.load_songbooks() |
47 | self.load_media_files() |
48 | - self.theme_combo_box.setEditText('') |
49 | + self.theme_combo_box.setCurrentIndex(-1) |
50 | + self.theme_combo_box.setCurrentText('') |
51 | # it's a new song to preview is not possible |
52 | self.preview_button.setVisible(False) |
53 | |
54 | @@ -446,8 +451,8 @@ |
55 | find_and_set_in_combo_box(self.theme_combo_box, str(self.song.theme_name)) |
56 | else: |
57 | # Clear the theme combo box in case it was previously set (bug #1212801) |
58 | - self.theme_combo_box.setEditText('') |
59 | - self.theme_combo_box.setCurrentIndex(0) |
60 | + self.theme_combo_box.setCurrentIndex(-1) |
61 | + self.theme_combo_box.setCurrentText('') |
62 | self.copyright_edit.setText(self.song.copyright if self.song.copyright else '') |
63 | self.comments_edit.setPlainText(self.song.comments if self.song.comments else '') |
64 | self.ccli_number_edit.setText(self.song.ccli_number if self.song.ccli_number else '') |
65 | @@ -550,12 +555,7 @@ |
66 | item = int(self.authors_combo_box.currentIndex()) |
67 | text = self.authors_combo_box.currentText().strip(' \r\n\t') |
68 | author_type = self.author_types_combo_box.itemData(self.author_types_combo_box.currentIndex()) |
69 | - # This if statement is for OS X, which doesn't seem to work well with |
70 | - # the QCompleter auto-completion class. See bug #812628. |
71 | - if text in self.authors: |
72 | - # Index 0 is a blank string, so add 1 |
73 | - item = self.authors.index(text) + 1 |
74 | - if item == 0 and text: |
75 | + if item == -1 and text: |
76 | if QtWidgets.QMessageBox.question( |
77 | self, |
78 | translate('SongsPlugin.EditSongForm', 'Add Author'), |
79 | @@ -570,10 +570,11 @@ |
80 | self.manager.save_object(author) |
81 | self._add_author_to_list(author, author_type) |
82 | self.load_authors() |
83 | - self.authors_combo_box.setEditText('') |
84 | + self.authors_combo_box.setCurrentIndex(-1) |
85 | + self.authors_combo_box.setCurrentText('') |
86 | else: |
87 | return |
88 | - elif item > 0: |
89 | + elif item >= 0: |
90 | item_id = (self.authors_combo_box.itemData(item)) |
91 | author = self.manager.get_object(Author, item_id) |
92 | if self.authors_list_view.findItems(author.get_display_name(author_type), QtCore.Qt.MatchExactly): |
93 | @@ -581,7 +582,8 @@ |
94 | message=translate('SongsPlugin.EditSongForm', 'This author is already in the list.')) |
95 | else: |
96 | self._add_author_to_list(author, author_type) |
97 | - self.authors_combo_box.setEditText('') |
98 | + self.authors_combo_box.setCurrentIndex(-1) |
99 | + self.authors_combo_box.setCurrentText('') |
100 | else: |
101 | QtWidgets.QMessageBox.warning( |
102 | self, UiStrings().NISs, |
103 | @@ -633,7 +635,7 @@ |
104 | def on_topic_add_button_clicked(self): |
105 | item = int(self.topics_combo_box.currentIndex()) |
106 | text = self.topics_combo_box.currentText() |
107 | - if item == 0 and text: |
108 | + if item == -1 and text: |
109 | if QtWidgets.QMessageBox.question( |
110 | self, translate('SongsPlugin.EditSongForm', 'Add Topic'), |
111 | translate('SongsPlugin.EditSongForm', 'This topic does not exist, do you want to add it?'), |
112 | @@ -645,10 +647,11 @@ |
113 | topic_item.setData(QtCore.Qt.UserRole, topic.id) |
114 | self.topics_list_view.addItem(topic_item) |
115 | self.load_topics() |
116 | - self.topics_combo_box.setEditText('') |
117 | + self.topics_combo_box.setCurrentIndex(-1) |
118 | + self.topics_combo_box.setCurrentText('') |
119 | else: |
120 | return |
121 | - elif item > 0: |
122 | + elif item >= 0: |
123 | item_id = (self.topics_combo_box.itemData(item)) |
124 | topic = self.manager.get_object(Topic, item_id) |
125 | if self.topics_list_view.findItems(str(topic.name), QtCore.Qt.MatchExactly): |
126 | @@ -658,7 +661,8 @@ |
127 | topic_item = QtWidgets.QListWidgetItem(str(topic.name)) |
128 | topic_item.setData(QtCore.Qt.UserRole, topic.id) |
129 | self.topics_list_view.addItem(topic_item) |
130 | - self.topics_combo_box.setEditText('') |
131 | + self.topics_combo_box.setCurrentIndex(-1) |
132 | + self.topics_combo_box.setCurrentText('') |
133 | else: |
134 | QtWidgets.QMessageBox.warning( |
135 | self, UiStrings().NISs, |
136 | @@ -678,7 +682,7 @@ |
137 | def on_songbook_add_button_clicked(self): |
138 | item = int(self.songbooks_combo_box.currentIndex()) |
139 | text = self.songbooks_combo_box.currentText() |
140 | - if item == 0 and text: |
141 | + if item == -1 and text: |
142 | if QtWidgets.QMessageBox.question( |
143 | self, translate('SongsPlugin.EditSongForm', 'Add Songbook'), |
144 | translate('SongsPlugin.EditSongForm', 'This Songbook does not exist, do you want to add it?'), |
145 | @@ -688,11 +692,12 @@ |
146 | self.manager.save_object(songbook) |
147 | self.add_songbook_entry_to_list(songbook.id, songbook.name, self.songbook_entry_edit.text()) |
148 | self.load_songbooks() |
149 | - self.songbooks_combo_box.setEditText('') |
150 | + self.songbooks_combo_box.setCurrentIndex(-1) |
151 | + self.songbooks_combo_box.setCurrentText('') |
152 | self.songbook_entry_edit.clear() |
153 | else: |
154 | return |
155 | - elif item > 0: |
156 | + elif item >= 0: |
157 | item_id = (self.songbooks_combo_box.itemData(item)) |
158 | songbook = self.manager.get_object(Book, item_id) |
159 | if self.songbooks_list_view.findItems(str(songbook.name), QtCore.Qt.MatchExactly): |
160 | @@ -700,7 +705,8 @@ |
161 | message=translate('SongsPlugin.EditSongForm', 'This Songbook is already in the list.')) |
162 | else: |
163 | self.add_songbook_entry_to_list(songbook.id, songbook.name, self.songbook_entry_edit.text()) |
164 | - self.songbooks_combo_box.setEditText('') |
165 | + self.songbooks_combo_box.setCurrentIndex(-1) |
166 | + self.songbooks_combo_box.setCurrentText('') |
167 | self.songbook_entry_edit.clear() |
168 | else: |
169 | QtWidgets.QMessageBox.warning( |
170 | |
171 | === modified file 'tests/functional/openlp_plugins/songs/test_editsongform.py' |
172 | --- tests/functional/openlp_plugins/songs/test_editsongform.py 2016-12-31 11:05:48 +0000 |
173 | +++ tests/functional/openlp_plugins/songs/test_editsongform.py 2017-03-02 05:06:04 +0000 |
174 | @@ -106,4 +106,5 @@ |
175 | mocked_cache.append.assert_called_once_with('Charles') |
176 | mocked_combo.setItemData.assert_called_once_with(0, 1) |
177 | mocked_set_case_insensitive_completer.assert_called_once_with(mocked_cache, mocked_combo) |
178 | - mocked_combo.setEditText.assert_called_once_with('') |
179 | + mocked_combo.setCurrentIndex.assert_called_once_with(-1) |
180 | + mocked_combo.setCurrentText.assert_called_once_with('') |
181 | |
182 | === modified file 'tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py' |
183 | --- tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py 2016-12-31 11:05:48 +0000 |
184 | +++ tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py 2017-03-02 05:06:04 +0000 |
185 | @@ -23,6 +23,7 @@ |
186 | Package to test the openlp.plugins.songs.forms.authorsform package. |
187 | """ |
188 | from unittest import TestCase |
189 | +from unittest.mock import patch |
190 | |
191 | from PyQt5 import QtWidgets |
192 | |
193 | @@ -138,3 +139,217 @@ |
194 | |
195 | # THEN: The display_name_edit should have the correct value |
196 | self.assertEqual(self.form.display_edit.text(), display_name, 'The display name should be set correctly') |
197 | + |
198 | + @patch('openlp.plugins.songs.forms.authorsform.QtWidgets.QDialog.exec') |
199 | + def test_exec(self, mocked_exec): |
200 | + """ |
201 | + Test the exec() method |
202 | + """ |
203 | + # GIVEN: An authors for and various mocked objects |
204 | + with patch.object(self.form.first_name_edit, 'clear') as mocked_first_name_edit_clear, \ |
205 | + patch.object(self.form.last_name_edit, 'clear') as mocked_last_name_edit_clear, \ |
206 | + patch.object(self.form.display_edit, 'clear') as mocked_display_edit_clear, \ |
207 | + patch.object(self.form.first_name_edit, 'setFocus') as mocked_first_name_edit_setFocus: |
208 | + # WHEN: The exec() method is called |
209 | + self.form.exec(clear=True) |
210 | + |
211 | + # THEN: The clear and exec() methods should have been called |
212 | + mocked_first_name_edit_clear.assert_called_once_with() |
213 | + mocked_last_name_edit_clear.assert_called_once_with() |
214 | + mocked_display_edit_clear.assert_called_once_with() |
215 | + mocked_first_name_edit_setFocus.assert_called_once_with() |
216 | + mocked_exec.assert_called_once_with(self.form) |
217 | + |
218 | + def test_first_name_edited(self): |
219 | + """ |
220 | + Test the on_first_name_edited() method |
221 | + """ |
222 | + # GIVEN: An author form |
223 | + self.form.auto_display_name = True |
224 | + |
225 | + with patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ |
226 | + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: |
227 | + mocked_last_name_edit_text.return_value = 'Newton' |
228 | + |
229 | + # WHEN: on_first_name_edited() is called |
230 | + self.form.on_first_name_edited('John') |
231 | + |
232 | + # THEN: The display name should be updated |
233 | + assert mocked_last_name_edit_text.call_count == 2 |
234 | + mocked_display_edit_setText.assert_called_once_with('John Newton') |
235 | + |
236 | + def test_first_name_edited_no_auto(self): |
237 | + """ |
238 | + Test the on_first_name_edited() method without auto_display_name |
239 | + """ |
240 | + # GIVEN: An author form |
241 | + self.form.auto_display_name = False |
242 | + |
243 | + with patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ |
244 | + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: |
245 | + |
246 | + # WHEN: on_first_name_edited() is called |
247 | + self.form.on_first_name_edited('John') |
248 | + |
249 | + # THEN: The display name should not be updated |
250 | + assert mocked_last_name_edit_text.call_count == 0 |
251 | + assert mocked_display_edit_setText.call_count == 0 |
252 | + |
253 | + def test_last_name_edited(self): |
254 | + """ |
255 | + Test the on_last_name_edited() method |
256 | + """ |
257 | + # GIVEN: An author form |
258 | + self.form.auto_display_name = True |
259 | + |
260 | + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ |
261 | + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: |
262 | + mocked_first_name_edit_text.return_value = 'John' |
263 | + |
264 | + # WHEN: on_last_name_edited() is called |
265 | + self.form.on_last_name_edited('Newton') |
266 | + |
267 | + # THEN: The display name should be updated |
268 | + assert mocked_first_name_edit_text.call_count == 2 |
269 | + mocked_display_edit_setText.assert_called_once_with('John Newton') |
270 | + |
271 | + def test_last_name_edited_no_auto(self): |
272 | + """ |
273 | + Test the on_last_name_edited() method without auto_display_name |
274 | + """ |
275 | + # GIVEN: An author form |
276 | + self.form.auto_display_name = False |
277 | + |
278 | + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ |
279 | + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: |
280 | + |
281 | + # WHEN: on_last_name_edited() is called |
282 | + self.form.on_last_name_edited('Newton') |
283 | + |
284 | + # THEN: The display name should not be updated |
285 | + assert mocked_first_name_edit_text.call_count == 0 |
286 | + assert mocked_display_edit_setText.call_count == 0 |
287 | + |
288 | + @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box') |
289 | + def test_accept_no_first_name(self, mocked_critical_error): |
290 | + """ |
291 | + Test the accept() method with no first name |
292 | + """ |
293 | + # GIVEN: A form and no text in thefirst name edit |
294 | + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ |
295 | + patch.object(self.form.first_name_edit, 'setFocus') as mocked_first_name_edit_setFocus: |
296 | + mocked_first_name_edit_text.return_value = '' |
297 | + |
298 | + # WHEN: accept() is called |
299 | + result = self.form.accept() |
300 | + |
301 | + # THEN: The result should be false and a critical error displayed |
302 | + assert result is False |
303 | + mocked_critical_error.assert_called_once_with(message='You need to type in the first name of the author.') |
304 | + mocked_first_name_edit_text.assert_called_once_with() |
305 | + mocked_first_name_edit_setFocus.assert_called_once_with() |
306 | + |
307 | + @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box') |
308 | + def test_accept_no_last_name(self, mocked_critical_error): |
309 | + """ |
310 | + Test the accept() method with no last name |
311 | + """ |
312 | + # GIVEN: A form and no text in the last name edit |
313 | + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ |
314 | + patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ |
315 | + patch.object(self.form.last_name_edit, 'setFocus') as mocked_last_name_edit_setFocus: |
316 | + mocked_first_name_edit_text.return_value = 'John' |
317 | + mocked_last_name_edit_text.return_value = '' |
318 | + |
319 | + # WHEN: accept() is called |
320 | + result = self.form.accept() |
321 | + |
322 | + # THEN: The result should be false and a critical error displayed |
323 | + assert result is False |
324 | + mocked_critical_error.assert_called_once_with(message='You need to type in the last name of the author.') |
325 | + mocked_first_name_edit_text.assert_called_once_with() |
326 | + mocked_last_name_edit_text.assert_called_once_with() |
327 | + mocked_last_name_edit_setFocus.assert_called_once_with() |
328 | + |
329 | + @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box') |
330 | + def test_accept_no_display_name_no_combine(self, mocked_critical_error): |
331 | + """ |
332 | + Test the accept() method with no display name and no combining |
333 | + """ |
334 | + # GIVEN: A form and no text in the display name edit |
335 | + mocked_critical_error.return_value = QtWidgets.QMessageBox.No |
336 | + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ |
337 | + patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ |
338 | + patch.object(self.form.display_edit, 'text') as mocked_display_edit_text, \ |
339 | + patch.object(self.form.display_edit, 'setFocus') as mocked_display_edit_setFocus: |
340 | + mocked_first_name_edit_text.return_value = 'John' |
341 | + mocked_last_name_edit_text.return_value = 'Newton' |
342 | + mocked_display_edit_text.return_value = '' |
343 | + |
344 | + # WHEN: accept() is called |
345 | + result = self.form.accept() |
346 | + |
347 | + # THEN: The result should be false and a critical error displayed |
348 | + assert result is False |
349 | + mocked_critical_error.assert_called_once_with( |
350 | + message='You have not set a display name for the author, combine the first and last names?', |
351 | + parent=self.form, question=True) |
352 | + mocked_first_name_edit_text.assert_called_once_with() |
353 | + mocked_last_name_edit_text.assert_called_once_with() |
354 | + mocked_display_edit_text.assert_called_once_with() |
355 | + mocked_display_edit_setFocus.assert_called_once_with() |
356 | + |
357 | + @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box') |
358 | + @patch('openlp.plugins.songs.forms.authorsform.QtWidgets.QDialog.accept') |
359 | + def test_accept_no_display_name(self, mocked_accept, mocked_critical_error): |
360 | + """ |
361 | + Test the accept() method with no display name and auto-combine |
362 | + """ |
363 | + # GIVEN: A form and no text in the display name edit |
364 | + mocked_accept.return_value = True |
365 | + mocked_critical_error.return_value = QtWidgets.QMessageBox.Yes |
366 | + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ |
367 | + patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ |
368 | + patch.object(self.form.display_edit, 'text') as mocked_display_edit_text, \ |
369 | + patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText: |
370 | + mocked_first_name_edit_text.return_value = 'John' |
371 | + mocked_last_name_edit_text.return_value = 'Newton' |
372 | + mocked_display_edit_text.return_value = '' |
373 | + |
374 | + # WHEN: accept() is called |
375 | + result = self.form.accept() |
376 | + |
377 | + # THEN: The result should be false and a critical error displayed |
378 | + assert result is True |
379 | + mocked_critical_error.assert_called_once_with( |
380 | + message='You have not set a display name for the author, combine the first and last names?', |
381 | + parent=self.form, question=True) |
382 | + assert mocked_first_name_edit_text.call_count == 2 |
383 | + assert mocked_last_name_edit_text.call_count == 2 |
384 | + mocked_display_edit_text.assert_called_once_with() |
385 | + mocked_display_edit_setText.assert_called_once_with('John Newton') |
386 | + mocked_accept.assert_called_once_with(self.form) |
387 | + |
388 | + @patch('openlp.plugins.songs.forms.authorsform.QtWidgets.QDialog.accept') |
389 | + def test_accept(self, mocked_accept): |
390 | + """ |
391 | + Test the accept() method |
392 | + """ |
393 | + # GIVEN: A form and text in the right places |
394 | + mocked_accept.return_value = True |
395 | + with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \ |
396 | + patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \ |
397 | + patch.object(self.form.display_edit, 'text') as mocked_display_edit_text: |
398 | + mocked_first_name_edit_text.return_value = 'John' |
399 | + mocked_last_name_edit_text.return_value = 'Newton' |
400 | + mocked_display_edit_text.return_value = 'John Newton' |
401 | + |
402 | + # WHEN: accept() is called |
403 | + result = self.form.accept() |
404 | + |
405 | + # THEN: The result should be false and a critical error displayed |
406 | + assert result is True |
407 | + mocked_first_name_edit_text.assert_called_once_with() |
408 | + mocked_last_name_edit_text.assert_called_once_with() |
409 | + mocked_display_edit_text.assert_called_once_with() |
410 | + mocked_accept.assert_called_once_with(self.form) |