Merge lp:~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick into lp:openlp

[Azaziah]

Fixed the issue where web bible's result in traceback in "Search while typing"
(by removing one miss-placed line)

This branch also fixes the issue where the new "Clear Bible search results" and
"Lock button" give focus to Text search if it is used in "Select" tab.

lp:~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick (revision 2705)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1772/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1683/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1621/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1377/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/967/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/1035/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/903/
[SUCCESS] https://ci.openlp.io/job/Branch-05c-Code_Analysis2/66/

[Tim Bentley (trb143)]

Looks OK but needs tests!

review: Needs Fixing

[Raoul Snyman (raoul-snyman)]

Please see my comments.

review: Needs Fixing

[Tim Bentley (trb143)]

Tests? The rest looks good though

review: Needs Fixing

[Raoul Snyman (raoul-snyman)]

Please read the unittest.mock documentation, it has lots of examples on how to use it and the best way to do it.

https://docs.python.org/3/library/unittest.mock.html

Check my comments below for the best way to use the library.

review: Needs Fixing

[Azaziah (suutari-olli)]

> Please read the unittest.mock documentation, it has lots of examples on how to
> use it and the best way to do it.
>
> https://docs.python.org/3/library/unittest.mock.html
>
> Check my comments below for the best way to use the library.

We tried to change this test to use @patch with phill, but failed.

[Phill (phill-ridout)]

> > Please read the unittest.mock documentation, it has lots of examples on how
> to
> > use it and the best way to do it.
> >
> > https://docs.python.org/3/library/unittest.mock.html
> >
> > Check my comments below for the best way to use the library.
>
> We tried to change this test to use @patch with phill, but failed.

I spent a couple hours on this and couldn't figure it out.

media_item.list_view is set in a super class
media_item.quick_search_edit is set in a method called from _setup which is patched

All from memory that is.

[Tim Bentley (trb143)]

review: Approve

[Phill (phill-ridout)]

on_lock_button_toggled is such a small method, any changes of tests for that?

Also can you include the parameters in the doc string please. (See inline comments)

review: Needs Fixing

[Phill (phill-ridout)]

your test 'test_on_lock_button_toggled_search_tab' is only really testing the last two lines of on_lock_button_toggled

As, you've gone to all the trouble of setting this test up it would be easy to test the if condition which sets the appropriate icon. In your current test add:

    # GIVEN:
    self.media_item.lock_icon = 'lock icon'
    sender_instance_mock = MagicMock()
    self.media_item.sender = MagicMock(return_value=sender_instance_mock)

    # THEN:
    sender_instance_mock.setIcon.assert_called_once_with('lock icon')

Then you can just copy and paste your test, make a few modifications calling self.media_item.on_lock_button_toggled with False and you've provided 100% coverage for the on_lock_button_toggled method, for not too much extra effort.

See also my inline comment for one small issue with your current test.

[Azaziah (suutari-olli)]

It was my initial thought but I failed to set up the test within a reasonable time.

Iv'e managed to increase the coverage by your instructions, thank you.

> your test 'test_on_lock_button_toggled_search_tab' is only really testing the
> last two lines of on_lock_button_toggled
>
> As, you've gone to all the trouble of setting this test up it would be easy to
> test the if condition which sets the appropriate icon. In your current test
> add:
>
> # GIVEN:
> self.media_item.lock_icon = 'lock icon'
> sender_instance_mock = MagicMock()
> self.media_item.sender = MagicMock(return_value=sender_instance_mock)
>
> # THEN:
> sender_instance_mock.setIcon.assert_called_once_with('lock icon')
>
> Then you can just copy and paste your test, make a few modifications calling
> self.media_item.on_lock_button_toggled with False and you've provided 100%
> coverage for the on_lock_button_toggled method, for not too much extra effort.
>
> See also my inline comment for one small issue with your current test.

[lp:~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick]

2707. By Azaziah on 2016-09-17

- The checking for not_called_once does not work apparently, fails in jenkins but passes in nosetests.

Unmerged revisions

=== modified file 'openlp/core/ui/exceptiondialog.py'
--- openlp/core/ui/exceptiondialog.py	2016-08-03 21:19:14 +0000
+++ openlp/core/ui/exceptiondialog.py	2016-09-17 23:08:24 +0000
@@ -97,9 +97,9 @@
             translate('OpenLP.ExceptionDialog', '<strong>Please describe what you were trying to do.</strong> '
                                                 '&nbsp;If possible, write in English.'))
         exception_part1 = (translate('OpenLP.ExceptionDialog',
-                                     '<strong>Oops, OpenLP hit a problem and couldn\'t recover!</strong> <br><br>'
-                                     '<strong>You can help </strong> the OpenLP developers to <strong>fix this</strong>'
-                                     ' by<br> sending them a <strong>bug report</strong> to {email}{newlines}'
+                                     '<strong>Oops, OpenLP hit a problem and couldn\'t recover!<br><br>'
+                                     'You can help </strong> the OpenLP developers to <strong>fix this</strong>'
+                                     ' by<br> sending them a <strong>bug report to {email}</strong>{newlines}'
                                      ).format(email='<a href = "mailto:[e-mail]" > [e-mail]</a>',
                                               newlines='<br><br>'))
         self.message_label.setText(

=== modified file 'openlp/plugins/bibles/lib/manager.py'
--- openlp/plugins/bibles/lib/manager.py	2016-09-04 16:15:57 +0000
+++ openlp/plugins/bibles/lib/manager.py	2016-09-17 23:08:24 +0000
@@ -367,7 +367,6 @@
             second_web_bible = self.db_cache[second_bible].get_object(BibleMeta, 'download_source')
         if web_bible or second_web_bible:
             # If either Bible is Web, cursor is reset to normal and search ends w/o any message.
-            self.check_search_result()
             self.application.set_normal_cursor()
             return None
         # Fetch the results from db. If no results are found, return None, no message is given for this.

=== modified file 'openlp/plugins/bibles/lib/mediaitem.py'
--- openlp/plugins/bibles/lib/mediaitem.py	2016-08-10 18:31:33 +0000
+++ openlp/plugins/bibles/lib/mediaitem.py	2016-09-17 23:08:24 +0000
@@ -254,8 +254,8 @@
         self.quickStyleComboBox.activated.connect(self.on_quick_style_combo_box_changed)
         self.advancedStyleComboBox.activated.connect(self.on_advanced_style_combo_box_changed)
         # Buttons
-        self.advancedClearButton.clicked.connect(self.on_clear_button)
-        self.quickClearButton.clicked.connect(self.on_clear_button)
+        self.advancedClearButton.clicked.connect(self.on_advanced_clear_button_clicked)
+        self.quickClearButton.clicked.connect(self.on_clear_button_clicked)
         self.advancedSearchButton.clicked.connect(self.on_advanced_search_button)
         self.quickSearchButton.clicked.connect(self.on_quick_search_button)
         # Other stuff
@@ -548,19 +548,31 @@
             self.advancedTab.setVisible(True)
             self.advanced_book_combo_box.setFocus()
 
-    def on_clear_button(self):
+    def on_clear_button_clicked(self):
         # Clear the list, then set the "No search Results" message, then clear the text field and give it focus.
         self.list_view.clear()
         self.check_search_result()
         self.quick_search_edit.clear()
         self.quick_search_edit.setFocus()
 
+    def on_advanced_clear_button_clicked(self):
+        # The same as the on_clear_button_clicked, but gives focus to Book name field in "Select" (advanced).
+        self.list_view.clear()
+        self.check_search_result()
+        self.advanced_book_combo_box.setFocus()
+
     def on_lock_button_toggled(self, checked):
-        self.quick_search_edit.setFocus()
+        """
+        Toggle the lock button, if Search tab is used, set focus to search field.
+        :param checked: The state of the toggle button. bool
+        :return: None
+        """
         if checked:
             self.sender().setIcon(self.lock_icon)
         else:
             self.sender().setIcon(self.unlock_icon)
+        if self.quickTab.isVisible():
+            self.quick_search_edit.setFocus()
 
     def on_quick_style_combo_box_changed(self):
         self.settings.layout_style = self.quickStyleComboBox.currentIndex()

=== modified file 'tests/functional/openlp_plugins/bibles/test_mediaitem.py'
--- tests/functional/openlp_plugins/bibles/test_mediaitem.py	2016-06-14 21:55:37 +0000
+++ tests/functional/openlp_plugins/bibles/test_mediaitem.py	2016-09-17 23:08:24 +0000
@@ -114,7 +114,7 @@
                 self.assertEqual(self.media_item.search_results, {})
                 self.assertEqual(self.media_item.second_search_results, {})
 
-    def on_quick_search_button_general_test(self):
+    def test_on_quick_search_button_general(self):
         """
         Test that general things, which should be called on all Quick searches are called.
         """
@@ -150,3 +150,60 @@
         self.assertEqual(2, self.media_item.quickSearchButton.setEnabled.call_count, 'Disable and Enable the button')
         self.assertEqual(1, self.media_item.check_search_result.call_count, 'Check results Should had been called once')
         self.assertEqual(1, self.app.set_normal_cursor.call_count, 'Normal cursor should had been called once')
+
+    def test_on_clear_button_clicked(self):
+        """
+        Test that the on_clear_button_clicked works properly. (Used by Bible search tab)
+        """
+        # GIVEN: Mocked list_view, check_search_results & quick_search_edit.
+        self.media_item.list_view = MagicMock()
+        self.media_item.check_search_result = MagicMock()
+        self.media_item.quick_search_edit = MagicMock()
+
+        # WHEN: on_clear_button_clicked is called
+        self.media_item.on_clear_button_clicked()
+
+        # THEN: Search result should be reset and search field should receive focus.
+        self.media_item.list_view.clear.assert_called_once_with(),
+        self.media_item.check_search_result.assert_called_once_with(),
+        self.media_item.quick_search_edit.clear.assert_called_once_with(),
+        self.media_item.quick_search_edit.setFocus.assert_called_once_with()
+
+    def test_on_lock_button_toggled_search_tab_lock_icon(self):
+        """
+        Test that "on_lock_button_toggled" gives focus to the right field and toggles the lock properly.
+        """
+        # GIVEN: Mocked sender & Search edit, quickTab returning value = True on isVisible.
+        self.media_item.sender = MagicMock()
+        self.media_item.quick_search_edit = MagicMock()
+        self.media_item.quickTab = MagicMock(**{'isVisible.return_value': True})
+
+        self.media_item.lock_icon = 'lock icon'
+        sender_instance_mock = MagicMock()
+        self.media_item.sender = MagicMock(return_value=sender_instance_mock)
+
+        # WHEN: on_lock_button_toggled is called and checked returns = True.
+        self.media_item.on_lock_button_toggled(True)
+
+        # THEN: on_quick_search_edit should receive focus and Lock icon should be set.
+        self.media_item.quick_search_edit.setFocus.assert_called_once_with()
+        sender_instance_mock.setIcon.assert_called_once_with('lock icon')
+
+    def test_on_lock_button_toggled_unlock_icon(self):
+        """
+         Test that lock button unlocks properly and lock toggles properly.
+        """
+        # GIVEN: Mocked sender & Search edit, quickTab returning value = False on isVisible.
+        self.media_item.sender = MagicMock()
+        self.media_item.quick_search_edit = MagicMock()
+        self.media_item.quickTab = MagicMock()
+        self.media_item.quickTab.isVisible = MagicMock()
+        self.media_item.unlock_icon = 'unlock icon'
+        sender_instance_mock = MagicMock()
+        self.media_item.sender = MagicMock(return_value=sender_instance_mock)
+
+        # WHEN: on_lock_button_toggled is called and checked returns = False.
+        self.media_item.on_lock_button_toggled(False)
+
+        # THEN: Unlock icon should be set.
+        sender_instance_mock.setIcon.assert_called_once_with('unlock icon')