Merge lp:~raoul-snyman/openlp/off-by-one-2.4 into lp:openlp/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
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.

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)

Subscribers

People subscribed via source and target branches

to all changes: