openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Phill has proposed merging lp:~phill-ridout/openlp/bug1623711 into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1623711 in OpenLP: "Unable to delete web bibles or more than one bible"
  https://bugs.launchpad.net/openlp/+bug/1623711

For more details, see:
https://code.launchpad.net/~phill-ridout/openlp/bug1623711/+merge/309724

lp:~phill-ridout/openlp/bug1623711 (revision 2702)
[SUCCESS] https://ci.openlp.io/job/Branch-01-Pull/1805/
[SUCCESS] https://ci.openlp.io/job/Branch-02-Functional-Tests/1716/
[SUCCESS] https://ci.openlp.io/job/Branch-03-Interface-Tests/1654/
[SUCCESS] https://ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/1410/
[SUCCESS] https://ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/1000/
[SUCCESS] https://ci.openlp.io/job/Branch-05a-Code_Analysis/1068/
[SUCCESS] https://ci.openlp.io/job/Branch-05b-Test_Coverage/936/
[SUCCESS] https://ci.openlp.io/job/Branch-05c-Code_Analysis2/97/
-- 
Your team OpenLP Core is requested to review the proposed merge of lp:~phill-ridout/openlp/bug1623711 into lp:openlp.

=== modified file 'openlp/plugins/bibles/lib/manager.py'
--- openlp/plugins/bibles/lib/manager.py	2016-09-16 19:54:25 +0000
+++ openlp/plugins/bibles/lib/manager.py	2016-11-01 07:01:03 +0000
@@ -131,7 +131,7 @@
             name = bible.get_name()
             # Remove corrupted files.
             if name is None:
-                bible.session.close()
+                bible.session.close_all()
                 delete_file(os.path.join(self.path, filename))
                 continue
             log.debug('Bible Name: "{name}"'.format(name=name))
@@ -178,7 +178,7 @@
         """
         log.debug('BibleManager.delete_bible("{name}")'.format(name=name))
         bible = self.db_cache[name]
-        bible.session.close()
+        bible.session.close_all()
         bible.session = None
         return delete_file(os.path.join(bible.path, bible.file))
 

=== modified file 'tests/functional/openlp_core_ui/test_servicemanager.py'
--- tests/functional/openlp_core_ui/test_servicemanager.py	2016-09-19 18:51:48 +0000
+++ tests/functional/openlp_core_ui/test_servicemanager.py	2016-11-01 07:01:03 +0000
@@ -545,8 +545,8 @@
         self.assertEqual(service_manager.theme_menu.menuAction().setVisible.call_count, 1,
                          'Should have be called once')
 
-    @patch('openlp.core.ui.servicemanager.Settings')
-    @patch('PyQt5.QtCore.QTimer.singleShot')
+    @patch(u'openlp.core.ui.servicemanager.Settings')
+    @patch(u'PyQt5.QtCore.QTimer.singleShot')
     def test_single_click_preview_true(self, mocked_singleShot, MockedSettings):
         """
         Test that when "Preview items when clicked in Service Manager" enabled the preview timer starts
@@ -562,8 +562,8 @@
         mocked_singleShot.assert_called_with(PyQt5.QtWidgets.QApplication.instance().doubleClickInterval(),
                                              service_manager.on_single_click_preview_timeout)
 
-    @patch('openlp.core.ui.servicemanager.Settings')
-    @patch('PyQt5.QtCore.QTimer.singleShot')
+    @patch(u'openlp.core.ui.servicemanager.Settings')
+    @patch(u'PyQt5.QtCore.QTimer.singleShot')
     def test_single_click_preview_false(self, mocked_singleShot, MockedSettings):
         """
         Test that when "Preview items when clicked in Service Manager" disabled the preview timer doesn't start
@@ -578,9 +578,9 @@
         # THEN: timer should not be started
         self.assertEqual(mocked_singleShot.call_count, 0, 'Should not be called')
 
-    @patch('openlp.core.ui.servicemanager.Settings')
-    @patch('PyQt5.QtCore.QTimer.singleShot')
-    @patch('openlp.core.ui.servicemanager.ServiceManager.make_live')
+    @patch(u'openlp.core.ui.servicemanager.Settings')
+    @patch(u'PyQt5.QtCore.QTimer.singleShot')
+    @patch(u'openlp.core.ui.servicemanager.ServiceManager.make_live')
     def test_single_click_preview_double(self, mocked_make_live, mocked_singleShot, MockedSettings):
         """
         Test that when a double click has registered the preview timer doesn't start
@@ -597,7 +597,7 @@
         mocked_make_live.assert_called_with()
         self.assertEqual(mocked_singleShot.call_count, 0, 'Should not be called')
 
-    @patch('openlp.core.ui.servicemanager.ServiceManager.make_preview')
+    @patch(u'openlp.core.ui.servicemanager.ServiceManager.make_preview')
     def test_single_click_timeout_single(self, mocked_make_preview):
         """
         Test that when a single click has been registered, the item is sent to preview
@@ -610,8 +610,8 @@
         self.assertEqual(mocked_make_preview.call_count, 1,
                          'ServiceManager.make_preview() should have been called once')
 
-    @patch('openlp.core.ui.servicemanager.ServiceManager.make_preview')
-    @patch('openlp.core.ui.servicemanager.ServiceManager.make_live')
+    @patch(u'openlp.core.ui.servicemanager.ServiceManager.make_preview')
+    @patch(u'openlp.core.ui.servicemanager.ServiceManager.make_live')
     def test_single_click_timeout_double(self, mocked_make_live, mocked_make_preview):
         """
         Test that when a double click has been registered, the item does not goes to preview
@@ -624,9 +624,9 @@
         # THEN: make_preview() should not have been called
         self.assertEqual(mocked_make_preview.call_count, 0, 'ServiceManager.make_preview() should not be called')
 
-    @patch('openlp.core.ui.servicemanager.shutil.copy')
-    @patch('openlp.core.ui.servicemanager.zipfile')
-    @patch('openlp.core.ui.servicemanager.ServiceManager.save_file_as')
+    @patch(u'openlp.core.ui.servicemanager.shutil.copy')
+    @patch(u'openlp.core.ui.servicemanager.zipfile')
+    @patch(u'openlp.core.ui.servicemanager.ServiceManager.save_file_as')
     def test_save_file_raises_permission_error(self, mocked_save_file_as, mocked_zipfile, mocked_shutil_copy):
         """
         Test that when a PermissionError is raised when trying to save a file, it is handled correctly
@@ -653,9 +653,9 @@
         self.assertTrue(result)
         mocked_save_file_as.assert_called_with()
 
-    @patch('openlp.core.ui.servicemanager.shutil.copy')
-    @patch('openlp.core.ui.servicemanager.zipfile')
-    @patch('openlp.core.ui.servicemanager.ServiceManager.save_file_as')
+    @patch(u'openlp.core.ui.servicemanager.shutil.copy')
+    @patch(u'openlp.core.ui.servicemanager.zipfile')
+    @patch(u'openlp.core.ui.servicemanager.ServiceManager.save_file_as')
     def test_save_local_file_raises_permission_error(self, mocked_save_file_as, mocked_zipfile, mocked_shutil_copy):
         """
         Test that when a PermissionError is raised when trying to save a local file, it is handled correctly

=== added file 'tests/functional/openlp_plugins/bibles/test_manager.py'
--- tests/functional/openlp_plugins/bibles/test_manager.py	1970-01-01 00:00:00 +0000
+++ tests/functional/openlp_plugins/bibles/test_manager.py	2016-11-01 07:01:03 +0000
@@ -0,0 +1,69 @@
+# -*- coding: utf-8 -*-
+# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
+
+###############################################################################
+# OpenLP - Open Source Lyrics Projection                                      #
+# --------------------------------------------------------------------------- #
+# Copyright (c) 2008-2016 OpenLP Developers                                   #
+# --------------------------------------------------------------------------- #
+# This program is free software; you can redistribute it and/or modify it     #
+# under the terms of the GNU General Public License as published by the Free  #
+# Software Foundation; version 2 of the License.                              #
+#                                                                             #
+# This program is distributed in the hope that it will be useful, but WITHOUT #
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or       #
+# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for    #
+# more details.                                                               #
+#                                                                             #
+# You should have received a copy of the GNU General Public License along     #
+# with this program; if not, write to the Free Software Foundation, Inc., 59  #
+# Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
+###############################################################################
+"""
+This module contains tests for the manager submodule of the Bibles plugin.
+"""
+from unittest import TestCase
+from unittest.mock import MagicMock, patch
+
+from openlp.plugins.bibles.lib.manager import BibleManager
+
+
+class TestManager(TestCase):
+    """
+    Test the functions in the :mod:`manager` module.
+    """
+
+    def setUp(self):
+        app_location_patcher = patch('openlp.plugins.bibles.lib.manager.AppLocation')
+        self.addCleanup(app_location_patcher.stop)
+        app_location_patcher.start()
+        log_patcher = patch('openlp.plugins.bibles.lib.manager.log')
+        self.addCleanup(log_patcher.stop)
+        self.mocked_log = log_patcher.start()
+        settings_patcher = patch('openlp.plugins.bibles.lib.manager.Settings')
+        self.addCleanup(settings_patcher.stop)
+        settings_patcher.start()
+
+    def test_delete_bible(self):
+        """
+        Test the BibleManager delete_bible method
+        """
+        # GIVEN: An instance of BibleManager and a mocked bible
+        with patch.object(BibleManager, 'reload_bibles'), \
+                patch('openlp.plugins.bibles.lib.manager.os.path.join', side_effect=lambda x, y: '{}/{}'.format(x, y)),\
+                patch('openlp.plugins.bibles.lib.manager.delete_file', return_value=True) as mocked_delete_file:
+            instance = BibleManager(MagicMock())
+            # We need to keep a reference to the mock for close_all as it gets set to None later on!
+            mocked_close_all = MagicMock()
+            mocked_bible = MagicMock(file='KJV.sqlite', path='bibles', **{'session.close_all': mocked_close_all})
+            instance.db_cache = {'KJV': mocked_bible}
+
+            # WHEN: Calling delete_bible with 'KJV'
+            result = instance.delete_bible('KJV')
+
+            # THEN: The session should have been closed and set to None, the bible should be deleted, and the result of
+            #       the deletion returned.
+            self.assertTrue(result)
+            mocked_close_all.assert_called_once_with()
+            self.assertIsNone(mocked_bible.session)
+            mocked_delete_file.assert_called_once_with('bibles/KJV.sqlite')