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