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

2016-09-18 Thread Phill 
Review: Approve

Looks good! Thanks for the extra effort!
-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/306037
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-09-17 Thread Azaziah 
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.
-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/306015
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-09-17 Thread Phill 
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.





Diff comments:

> 
> === modified file 'tests/functional/openlp_plugins/bibles/test_mediaitem.py'
> --- tests/functional/openlp_plugins/bibles/test_mediaitem.py  2016-06-14 
> 21:55:37 +
> +++ tests/functional/openlp_plugins/bibles/test_mediaitem.py  2016-09-16 
> 21:53:08 +
> @@ -150,3 +150,36 @@
>  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(self):
> +"""
> +Test that "on_lock_button_toggled" gives focus to the right field.
> +"""
> +# GIVEN: Mocked functions
> +self.media_item.sender = MagicMock()
> +self.media_item.quickTab = MagicMock()
> +self.media_item.quick_search_edit = MagicMock()
> +
> +# WHEN: on_lock_button_toggled is called and quickTab.isVisible() 
> returns = True.

quickTab.isVisible() isn't returning True, its returning an instance of 
MagicMock. For it to return True, you need to set the return_value on isVisable 
to True.

You could do it like this:
self.media_item.quickTab = MagicMock()
self.media_item.quickTab.isVisable = MagicMock(return_value=True)

but a as MagicMock automatically creates mocks for every method you call, you 
could also do it like:
self.media_item.quickTab = MagicMock(**{'isVisible.return_value': True})

note in the above example we're unpacking a dictionary, because an keyword 
argument with a '.' in it isn't valid

> +self.media_item.on_lock_button_toggled(True)
> +
> +# THEN: on_quick_search_edit should receive focus.
> +self.media_item.quick_search_edit.setFocus.assert_called_once_with()


-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/306015
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-09-14 Thread Phill 
Review: Needs Fixing

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)

Diff comments:

> 
> === modified file 'openlp/plugins/bibles/lib/mediaitem.py'
> --- openlp/plugins/bibles/lib/mediaitem.py2016-08-10 18:31:33 +
> +++ openlp/plugins/bibles/lib/mediaitem.py2016-09-14 23:28:20 +
> @@ -548,19 +548,32 @@
>  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, if Select tab is used,
> +give focus to Bible book name field.

:param checked: The state of the toggle button (qt checked type maybe? Or 
Boolean) 
: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()
> +else:
> +self.advanced_book_combo_box.setFocus()
>  
>  def on_quick_style_combo_box_changed(self):
>  self.settings.layout_style = self.quickStyleComboBox.currentIndex()


-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/305770
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-09-10 Thread Tim Bentley 
Review: Approve


-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/305302
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-09-02 Thread Phill 
> > 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. 
-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/303487
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-09-02 Thread Azaziah 
> 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.
-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/303487
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-08-21 Thread Raoul Snyman 
Review: Needs Fixing

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.

Diff comments:

> 
> === modified file 'tests/functional/openlp_plugins/bibles/test_mediaitem.py'
> --- tests/functional/openlp_plugins/bibles/test_mediaitem.py  2016-06-14 
> 21:55:37 +
> +++ tests/functional/openlp_plugins/bibles/test_mediaitem.py  2016-08-20 
> 19:41:00 +
> @@ -150,3 +150,22 @@
>  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()

For all of the above mocks, you should be using patch.object.

with patch.object(self.media_item, 'list_view') as mocked_list_view, \
patch.object(self.media_item, 'check_search_result') as 
mocked_check_search_result:
# WHEN: ...

> +
> +# 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.assertEqual(1, self.media_item.list_view.clear.call_count, 
> 'List_view.clear should had been called once.')

You can actually test it with
 self.media_item.list_view.clear.assert_called_once_with()

> +self.assertEqual(1, self.media_item.check_search_result.call_count, 
> 'Check results Should had been called once')

Same here

> +self.assertEqual(1, 
> self.media_item.quick_search_edit.clear.call_count, 'Should had been called 
> once')

Same here

> +self.assertEqual(1, 
> self.media_item.quick_search_edit.setFocus.call_count, 'Should had been 
> called once')

Same here



-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/303487
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-08-14 Thread Tim Bentley 
Review: Needs Fixing

Tests?  The rest looks good though
-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/302891
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-08-12 Thread Raoul Snyman 
Review: Needs Fixing

Please see my comments.

Diff comments:

> === modified file 'openlp/plugins/bibles/lib/mediaitem.py'
> --- openlp/plugins/bibles/lib/mediaitem.py2016-08-10 18:31:33 +
> +++ openlp/plugins/bibles/lib/mediaitem.py2016-08-10 21:34:12 +
> @@ -254,7 +254,7 @@
>  
> 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.advancedClearButton.clicked.connect(self.on_clear_button_advanced)

This should actually be "self.on_advanced_clear_button_clicked" -- I'm not sure 
why the original one lacked the "clicked", but please just fix this one and 
name it correctly.

>  self.quickClearButton.clicked.connect(self.on_clear_button)
>  
> self.advancedSearchButton.clicked.connect(self.on_advanced_search_button)
>  self.quickSearchButton.clicked.connect(self.on_quick_search_button)
> @@ -555,6 +555,11 @@
>  self.quick_search_edit.clear()
>  self.quick_search_edit.setFocus()
>  
> +def on_clear_button_advanced(self):

on_advanced_clear_button_advanced_clicked, as above.

> +# The same as the on_clear_button, but does not give focus to Quick 
> search field.
> +self.list_view.clear()
> +self.check_search_result()
> +
>  def on_lock_button_toggled(self, checked):
>  self.quick_search_edit.setFocus()
>  if checked:


-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/302597
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp

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

2016-08-11 Thread Tim Bentley 
Review: Needs Fixing

Looks OK but needs tests!
-- 
https://code.launchpad.net/~suutari-olli/openlp/fix-advanced-bible-search-clear-button-giving-focus-to-quick/+merge/302597
Your team OpenLP Core is subscribed to branch lp:openlp.

___
Mailing list: https://launchpad.net/~openlp-core
Post to : openlp-core@lists.launchpad.net
Unsubscribe : https://launchpad.net/~openlp-core
More help   : https://help.launchpad.net/ListHelp