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://ci.openlp.io/job/Branch-01-Pull/1915/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1826/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1767/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1500/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/109...

bzr-revno: 2674

openlp/plugins/songs/forms/editsongform.py

@@ -31,7 +31,8 @@ import shutil
 
 from PyQt5 import QtCore, QtWidgets
 
-from openlp.core.common import Registry, RegistryProperties, AppLocation, UiStrings, check_directory_exists, translate
+from openlp.core.common import Registry, RegistryProperties, AppLocation, UiStrings, check_directory_exists, \
+    translate, is_macosx
 from openlp.core.lib import FileDialog, PluginStatus, MediaType, create_separated_list
 from openlp.core.lib.ui import set_case_insensitive_completer, critical_error_message_box, find_and_set_in_combo_box
 from openlp.plugins.songs.lib import VerseType, clean_song
@@ -118,7 +119,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
             cache.append(obj.name)
             combo.setItemData(row, obj.id)
         set_case_insensitive_completer(cache, combo)
-        combo.setEditText('')
+        combo.setCurrentIndex(-1)
+        combo.setCurrentText('')
 
     def _add_author_to_list(self, author, author_type):
         """
@@ -352,7 +354,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
             self.authors_combo_box.setItemData(row, author.id)
             self.authors.append(author.display_name)
         set_case_insensitive_completer(self.authors, self.authors_combo_box)
-        self.authors_combo_box.setEditText('')
+        self.authors_combo_box.setCurrentIndex(-1)
+        self.authors_combo_box.setCurrentText('')
 
         # Types
         self.author_types_combo_box.clear()
@@ -382,7 +385,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
         self.themes = theme_list
         self.theme_combo_box.addItems(theme_list)
         set_case_insensitive_completer(self.themes, self.theme_combo_box)
-        self.theme_combo_box.setEditText('')
+        self.theme_combo_box.setCurrentIndex(-1)
+        self.theme_combo_box.setCurrentText('')
 
     def load_media_files(self):
         """
@@ -421,7 +425,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
         self.load_topics()
         self.load_songbooks()
         self.load_media_files()
-        self.theme_combo_box.setEditText('')
+        self.theme_combo_box.setCurrentIndex(-1)
+        self.theme_combo_box.setCurrentText('')
         # it's a new song to preview is not possible
         self.preview_button.setVisible(False)
 
@@ -446,8 +451,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
             find_and_set_in_combo_box(self.theme_combo_box, str(self.song.theme_name))
         else:
             # Clear the theme combo box in case it was previously set (bug #1212801)
-            self.theme_combo_box.setEditText('')
+            self.theme_combo_box.setCurrentIndex(-1)
-            self.theme_combo_box.setCurrentIndex(0)
+            self.theme_combo_box.setCurrentText('')
         self.copyright_edit.setText(self.song.copyright if self.song.copyright else '')
         self.comments_edit.setPlainText(self.song.comments if self.song.comments else '')
         self.ccli_number_edit.setText(self.song.ccli_number if self.song.ccli_number else '')
@@ -550,12 +555,7 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
         item = int(self.authors_combo_box.currentIndex())
         text = self.authors_combo_box.currentText().strip(' \r\n\t')
         author_type = self.author_types_combo_box.itemData(self.author_types_combo_box.currentIndex())
-        # This if statement is for OS X, which doesn't seem to work well with
+        if item == -1 and text:
-        # the QCompleter auto-completion class. See bug #812628.
-        if text in self.authors:
-            # Index 0 is a blank string, so add 1
-            item = self.authors.index(text) + 1
-        if item == 0 and text:
             if QtWidgets.QMessageBox.question(
                     self,
                     translate('SongsPlugin.EditSongForm', 'Add Author'),
@@ -570,10 +570,11 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
                 self.manager.save_object(author)
                 self._add_author_to_list(author, author_type)
                 self.load_authors()
-                self.authors_combo_box.setEditText('')
+                self.authors_combo_box.setCurrentIndex(-1)
+                self.authors_combo_box.setCurrentText('')
             else:
                 return
-        elif item > 0:
+        elif item >= 0:
             item_id = (self.authors_combo_box.itemData(item))
             author = self.manager.get_object(Author, item_id)
             if self.authors_list_view.findItems(author.get_display_name(author_type), QtCore.Qt.MatchExactly):
@@ -581,7 +582,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
                     message=translate('SongsPlugin.EditSongForm', 'This author is already in the list.'))
             else:
                 self._add_author_to_list(author, author_type)
-            self.authors_combo_box.setEditText('')
+            self.authors_combo_box.setCurrentIndex(-1)
+            self.authors_combo_box.setCurrentText('')
         else:
             QtWidgets.QMessageBox.warning(
                 self, UiStrings().NISs,
@@ -633,7 +635,7 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
     def on_topic_add_button_clicked(self):
         item = int(self.topics_combo_box.currentIndex())
         text = self.topics_combo_box.currentText()
-        if item == 0 and text:
+        if item == -1 and text:
             if QtWidgets.QMessageBox.question(
                     self, translate('SongsPlugin.EditSongForm', 'Add Topic'),
                     translate('SongsPlugin.EditSongForm', 'This topic does not exist, do you want to add it?'),
@@ -645,10 +647,11 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
                 topic_item.setData(QtCore.Qt.UserRole, topic.id)
                 self.topics_list_view.addItem(topic_item)
                 self.load_topics()
-                self.topics_combo_box.setEditText('')
+                self.topics_combo_box.setCurrentIndex(-1)
+                self.topics_combo_box.setCurrentText('')
             else:
                 return
-        elif item > 0:
+        elif item >= 0:
             item_id = (self.topics_combo_box.itemData(item))
             topic = self.manager.get_object(Topic, item_id)
             if self.topics_list_view.findItems(str(topic.name), QtCore.Qt.MatchExactly):
@@ -658,7 +661,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
                 topic_item = QtWidgets.QListWidgetItem(str(topic.name))
                 topic_item.setData(QtCore.Qt.UserRole, topic.id)
                 self.topics_list_view.addItem(topic_item)
-            self.topics_combo_box.setEditText('')
+            self.topics_combo_box.setCurrentIndex(-1)
+            self.topics_combo_box.setCurrentText('')
         else:
             QtWidgets.QMessageBox.warning(
                 self, UiStrings().NISs,
@@ -678,7 +682,7 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
     def on_songbook_add_button_clicked(self):
         item = int(self.songbooks_combo_box.currentIndex())
         text = self.songbooks_combo_box.currentText()
-        if item == 0 and text:
+        if item == -1 and text:
             if QtWidgets.QMessageBox.question(
                     self, translate('SongsPlugin.EditSongForm', 'Add Songbook'),
                     translate('SongsPlugin.EditSongForm', 'This Songbook does not exist, do you want to add it?'),
@@ -688,11 +692,12 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
                 self.manager.save_object(songbook)
                 self.add_songbook_entry_to_list(songbook.id, songbook.name, self.songbook_entry_edit.text())
                 self.load_songbooks()
-                self.songbooks_combo_box.setEditText('')
+                self.songbooks_combo_box.setCurrentIndex(-1)
+                self.songbooks_combo_box.setCurrentText('')
                 self.songbook_entry_edit.clear()
             else:
                 return
-        elif item > 0:
+        elif item >= 0:
             item_id = (self.songbooks_combo_box.itemData(item))
             songbook = self.manager.get_object(Book, item_id)
             if self.songbooks_list_view.findItems(str(songbook.name), QtCore.Qt.MatchExactly):
@@ -700,7 +705,8 @@ class EditSongForm(QtWidgets.QDialog, Ui_EditSongDialog, RegistryProperties):
                     message=translate('SongsPlugin.EditSongForm', 'This Songbook is already in the list.'))
             else:
                 self.add_songbook_entry_to_list(songbook.id, songbook.name, self.songbook_entry_edit.text())
-            self.songbooks_combo_box.setEditText('')
+            self.songbooks_combo_box.setCurrentIndex(-1)
+            self.songbooks_combo_box.setCurrentText('')
             self.songbook_entry_edit.clear()
         else:
             QtWidgets.QMessageBox.warning(

tests/functional/openlp_plugins/songs/test_editsongform.py

@@ -106,4 +106,5 @@ class TestEditSongForm(TestCase, TestMixin):
         mocked_cache.append.assert_called_once_with('Charles')
         mocked_combo.setItemData.assert_called_once_with(0, 1)
         mocked_set_case_insensitive_completer.assert_called_once_with(mocked_cache, mocked_combo)
-        mocked_combo.setEditText.assert_called_once_with('')
+        mocked_combo.setCurrentIndex.assert_called_once_with(-1)
+        mocked_combo.setCurrentText.assert_called_once_with('')

tests/interfaces/openlp_plugins/songs/forms/test_authorsform.py

@@ -23,6 +23,7 @@
 Package to test the openlp.plugins.songs.forms.authorsform package.
 """
 from unittest import TestCase
+from unittest.mock import patch
 
 from PyQt5 import QtWidgets
 
@@ -138,3 +139,217 @@ class TestAuthorsForm(TestCase, TestMixin):
 
         # THEN: The display_name_edit should have the correct value
         self.assertEqual(self.form.display_edit.text(), display_name, 'The display name should be set correctly')
+
+    @patch('openlp.plugins.songs.forms.authorsform.QtWidgets.QDialog.exec')
+    def test_exec(self, mocked_exec):
+        """
+        Test the exec() method
+        """
+        # GIVEN: An authors for and various mocked objects
+        with patch.object(self.form.first_name_edit, 'clear') as mocked_first_name_edit_clear, \
+                patch.object(self.form.last_name_edit, 'clear') as mocked_last_name_edit_clear, \
+                patch.object(self.form.display_edit, 'clear') as mocked_display_edit_clear, \
+                patch.object(self.form.first_name_edit, 'setFocus') as mocked_first_name_edit_setFocus:
+            # WHEN: The exec() method is called
+            self.form.exec(clear=True)
+
+            # THEN: The clear and exec() methods should have been called
+        mocked_first_name_edit_clear.assert_called_once_with()
+        mocked_last_name_edit_clear.assert_called_once_with()
+        mocked_display_edit_clear.assert_called_once_with()
+        mocked_first_name_edit_setFocus.assert_called_once_with()
+        mocked_exec.assert_called_once_with(self.form)
+
+    def test_first_name_edited(self):
+        """
+        Test the on_first_name_edited() method
+        """
+        # GIVEN: An author form
+        self.form.auto_display_name = True
+
+        with patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \
+                patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText:
+            mocked_last_name_edit_text.return_value = 'Newton'
+
+            # WHEN: on_first_name_edited() is called
+            self.form.on_first_name_edited('John')
+
+        # THEN: The display name should be updated
+        assert mocked_last_name_edit_text.call_count == 2
+        mocked_display_edit_setText.assert_called_once_with('John Newton')
+
+    def test_first_name_edited_no_auto(self):
+        """
+        Test the on_first_name_edited() method without auto_display_name
+        """
+        # GIVEN: An author form
+        self.form.auto_display_name = False
+
+        with patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \
+                patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText:
+
+            # WHEN: on_first_name_edited() is called
+            self.form.on_first_name_edited('John')
+
+        # THEN: The display name should not be updated
+        assert mocked_last_name_edit_text.call_count == 0
+        assert mocked_display_edit_setText.call_count == 0
+
+    def test_last_name_edited(self):
+        """
+        Test the on_last_name_edited() method
+        """
+        # GIVEN: An author form
+        self.form.auto_display_name = True
+
+        with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \
+                patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText:
+            mocked_first_name_edit_text.return_value = 'John'
+
+            # WHEN: on_last_name_edited() is called
+            self.form.on_last_name_edited('Newton')
+
+        # THEN: The display name should be updated
+        assert mocked_first_name_edit_text.call_count == 2
+        mocked_display_edit_setText.assert_called_once_with('John Newton')
+
+    def test_last_name_edited_no_auto(self):
+        """
+        Test the on_last_name_edited() method without auto_display_name
+        """
+        # GIVEN: An author form
+        self.form.auto_display_name = False
+
+        with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \
+                patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText:
+
+            # WHEN: on_last_name_edited() is called
+            self.form.on_last_name_edited('Newton')
+
+        # THEN: The display name should not be updated
+        assert mocked_first_name_edit_text.call_count == 0
+        assert mocked_display_edit_setText.call_count == 0
+
+    @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box')
+    def test_accept_no_first_name(self, mocked_critical_error):
+        """
+        Test the accept() method with no first name
+        """
+        # GIVEN: A form and no text in thefirst name edit
+        with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \
+                patch.object(self.form.first_name_edit, 'setFocus') as mocked_first_name_edit_setFocus:
+            mocked_first_name_edit_text.return_value = ''
+
+            # WHEN: accept() is called
+            result = self.form.accept()
+
+        # THEN: The result should be false and a critical error displayed
+        assert result is False
+        mocked_critical_error.assert_called_once_with(message='You need to type in the first name of the author.')
+        mocked_first_name_edit_text.assert_called_once_with()
+        mocked_first_name_edit_setFocus.assert_called_once_with()
+
+    @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box')
+    def test_accept_no_last_name(self, mocked_critical_error):
+        """
+        Test the accept() method with no last name
+        """
+        # GIVEN: A form and no text in the last name edit
+        with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \
+                patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \
+                patch.object(self.form.last_name_edit, 'setFocus') as mocked_last_name_edit_setFocus:
+            mocked_first_name_edit_text.return_value = 'John'
+            mocked_last_name_edit_text.return_value = ''
+
+            # WHEN: accept() is called
+            result = self.form.accept()
+
+        # THEN: The result should be false and a critical error displayed
+        assert result is False
+        mocked_critical_error.assert_called_once_with(message='You need to type in the last name of the author.')
+        mocked_first_name_edit_text.assert_called_once_with()
+        mocked_last_name_edit_text.assert_called_once_with()
+        mocked_last_name_edit_setFocus.assert_called_once_with()
+
+    @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box')
+    def test_accept_no_display_name_no_combine(self, mocked_critical_error):
+        """
+        Test the accept() method with no display name and no combining
+        """
+        # GIVEN: A form and no text in the display name edit
+        mocked_critical_error.return_value = QtWidgets.QMessageBox.No
+        with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \
+                patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \
+                patch.object(self.form.display_edit, 'text') as mocked_display_edit_text, \
+                patch.object(self.form.display_edit, 'setFocus') as mocked_display_edit_setFocus:
+            mocked_first_name_edit_text.return_value = 'John'
+            mocked_last_name_edit_text.return_value = 'Newton'
+            mocked_display_edit_text.return_value = ''
+
+            # WHEN: accept() is called
+            result = self.form.accept()
+
+        # THEN: The result should be false and a critical error displayed
+        assert result is False
+        mocked_critical_error.assert_called_once_with(
+            message='You have not set a display name for the author, combine the first and last names?',
+            parent=self.form, question=True)
+        mocked_first_name_edit_text.assert_called_once_with()
+        mocked_last_name_edit_text.assert_called_once_with()
+        mocked_display_edit_text.assert_called_once_with()
+        mocked_display_edit_setFocus.assert_called_once_with()
+
+    @patch('openlp.plugins.songs.forms.authorsform.critical_error_message_box')
+    @patch('openlp.plugins.songs.forms.authorsform.QtWidgets.QDialog.accept')
+    def test_accept_no_display_name(self, mocked_accept, mocked_critical_error):
+        """
+        Test the accept() method with no display name and auto-combine
+        """
+        # GIVEN: A form and no text in the display name edit
+        mocked_accept.return_value = True
+        mocked_critical_error.return_value = QtWidgets.QMessageBox.Yes
+        with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \
+                patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \
+                patch.object(self.form.display_edit, 'text') as mocked_display_edit_text, \
+                patch.object(self.form.display_edit, 'setText') as mocked_display_edit_setText:
+            mocked_first_name_edit_text.return_value = 'John'
+            mocked_last_name_edit_text.return_value = 'Newton'
+            mocked_display_edit_text.return_value = ''
+
+            # WHEN: accept() is called
+            result = self.form.accept()
+
+        # THEN: The result should be false and a critical error displayed
+        assert result is True
+        mocked_critical_error.assert_called_once_with(
+            message='You have not set a display name for the author, combine the first and last names?',
+            parent=self.form, question=True)
+        assert mocked_first_name_edit_text.call_count == 2
+        assert mocked_last_name_edit_text.call_count == 2
+        mocked_display_edit_text.assert_called_once_with()
+        mocked_display_edit_setText.assert_called_once_with('John Newton')
+        mocked_accept.assert_called_once_with(self.form)
+
+    @patch('openlp.plugins.songs.forms.authorsform.QtWidgets.QDialog.accept')
+    def test_accept(self, mocked_accept):
+        """
+        Test the accept() method
+        """
+        # GIVEN: A form and text in the right places
+        mocked_accept.return_value = True
+        with patch.object(self.form.first_name_edit, 'text') as mocked_first_name_edit_text, \
+                patch.object(self.form.last_name_edit, 'text') as mocked_last_name_edit_text, \
+                patch.object(self.form.display_edit, 'text') as mocked_display_edit_text:
+            mocked_first_name_edit_text.return_value = 'John'
+            mocked_last_name_edit_text.return_value = 'Newton'
+            mocked_display_edit_text.return_value = 'John Newton'
+
+            # WHEN: accept() is called
+            result = self.form.accept()
+
+        # THEN: The result should be false and a critical error displayed
+        assert result is True
+        mocked_first_name_edit_text.assert_called_once_with()
+        mocked_last_name_edit_text.assert_called_once_with()
+        mocked_display_edit_text.assert_called_once_with()
+        mocked_accept.assert_called_once_with(self.form)