Merge branch 'fix_service_save_failure' into 'master'

Fix service save failure

Closes #816

See merge request openlp/openlp!322

openlp/core/ui/servicemanager.py

@@ -660,7 +660,11 @@ class ServiceManager(QtWidgets.QWidget, RegistryBase, Ui_ServiceManager, LogMixi
                     self.main_window.increment_progress_bar(local_file_item.stat().st_size / total_size * 1000)
                 with suppress(FileNotFoundError):
                     file_path.unlink()
+                # Try to link rather than copy to prevent writing another file
+                try:
                     os.link(temp_file.name, file_path)
+                except OSError:
+                    shutil.copyfile(temp_file.name, file_path)
             self.settings.setValue('servicemanager/last directory', file_path.parent)
         except (PermissionError, OSError) as error:
             self.log_exception('Failed to save service to disk: {name}'.format(name=file_path))

tests/openlp_core/ui/test_servicemanager.py

@@ -635,7 +635,10 @@ def test_single_click_timeout_double(mocked_make_live, mocked_make_preview, regi
 @patch('openlp.core.ui.servicemanager.zipfile')
 @patch('openlp.core.ui.servicemanager.ServiceManager.save_file_as')
 @patch('openlp.core.ui.servicemanager.os')
-def test_save_file_raises_permission_error(mocked_os, mocked_save_file_as, mocked_zipfile, registry):
+@patch('openlp.core.ui.servicemanager.shutil')
+@patch('openlp.core.ui.servicemanager.NamedTemporaryFile')
+def test_save_file_raises_permission_error(mocked_temp_file, mocked_shutil, mocked_os, mocked_save_file_as,
+                                           mocked_zipfile, registry):
     """
     Test that when a PermissionError is raised when trying to save a file, it is handled correctly
     """
@@ -643,29 +646,34 @@ def test_save_file_raises_permission_error(mocked_os, mocked_save_file_as, mocke
     mocked_main_window = MagicMock()
     Registry().register('main_window', mocked_main_window)
     Registry().register('application', MagicMock())
+    Registry().register('settings', MagicMock())
     service_manager = ServiceManager(None)
     service_manager._service_path = MagicMock()
     service_manager._save_lite = False
     service_manager.service_items = []
     service_manager.service_theme = 'Default'
     service_manager.service_manager_list = MagicMock()
-    mocked_save_file_as.return_value = True
+    mocked_save_file_as.return_value = False
     mocked_zipfile.ZipFile.return_value = MagicMock()
     mocked_os.link.side_effect = PermissionError
+    mocked_shutil.copyfile.side_effect = PermissionError
 
     # WHEN: The service is saved and a PermissionError is raised
     result = service_manager.save_file()
 
     # THEN: The "save_as" method is called to save the service
-    assert result is True
+    assert result is False
     mocked_save_file_as.assert_called_with()
 
 
 @patch('openlp.core.ui.servicemanager.zipfile')
 @patch('openlp.core.ui.servicemanager.ServiceManager.save_file_as')
 @patch('openlp.core.ui.servicemanager.os')
+@patch('openlp.core.ui.servicemanager.shutil')
 @patch('openlp.core.ui.servicemanager.len')
-def test_save_file_large_file(mocked_len, mocked_os, mocked_save_file_as, mocked_zipfile, registry):
+@patch('openlp.core.ui.servicemanager.NamedTemporaryFile')
+def test_save_file_large_file(mocked_temp_file, mocked_len, mocked_shutil, mocked_os, mocked_save_file_as,
+                              mocked_zipfile, registry):
     """
     Test that when a file size size larger than a 32bit signed int is attempted to save, the progress bar
     should be provided a value that fits in a 32bit int (because it's passed to C++ as a 32bit unsigned int)
@@ -678,15 +686,14 @@ def test_save_file_large_file(mocked_len, mocked_os, mocked_save_file_as, mocked
     mocked_main_window.display_progress_bar.side_effect = check_for_i32_overflow
     Registry().register('main_window', mocked_main_window)
     Registry().register('application', MagicMock())
-    mocked_settings = MagicMock()
+    Registry().register('settings', MagicMock())
-    Registry().register('settings', mocked_settings)
     service_manager = ServiceManager(None)
     service_manager._service_path = MagicMock()
     service_manager._save_lite = False
     service_manager.service_items = []
     service_manager.service_theme = 'Default'
     service_manager.service_manager_list = MagicMock()
-    mocked_save_file_as.return_value = True
+    mocked_save_file_as.return_value = False
     mocked_zipfile.ZipFile.return_value = MagicMock()
     mocked_len.return_value = 10000000000000
 
@@ -695,7 +702,40 @@ def test_save_file_large_file(mocked_len, mocked_os, mocked_save_file_as, mocked
 
     # THEN: The "save_as" method is called to save the service
     assert result is True
-    mocked_save_file_as.assert_called_with()
+    mocked_save_file_as.assert_not_called()
+
+
+@patch('openlp.core.ui.servicemanager.zipfile')
+@patch('openlp.core.ui.servicemanager.ServiceManager.save_file_as')
+@patch('openlp.core.ui.servicemanager.os')
+@patch('openlp.core.ui.servicemanager.shutil')
+@patch('openlp.core.ui.servicemanager.NamedTemporaryFile')
+def test_save_file_falls_back_to_shutil(mocked_temp_file, mocked_shutil, mocked_os, mocked_save_file_as, mocked_zipfile,
+                                        registry):
+    """
+    Test that when a PermissionError is raised when trying to save a file, it is handled correctly
+    """
+    # GIVEN: A service manager, a service to save
+    mocked_main_window = MagicMock()
+    Registry().register('main_window', mocked_main_window)
+    Registry().register('application', MagicMock())
+    Registry().register('settings', MagicMock())
+    service_manager = ServiceManager(None)
+    service_manager._service_path = MagicMock()
+    service_manager._save_lite = False
+    service_manager.service_items = []
+    service_manager.service_theme = 'Default'
+    service_manager.service_manager_list = MagicMock()
+    mocked_save_file_as.return_value = False
+    mocked_zipfile.ZipFile.return_value = MagicMock()
+    mocked_os.link.side_effect = OSError
+
+    # WHEN: The service is saved and a PermissionError is raised
+    result = service_manager.save_file()
+
+    # THEN: The result is true
+    assert result is True
+    mocked_shutil.copyfile.assert_called_once()
 
 
 @patch('openlp.core.ui.servicemanager.ServiceManager.regenerate_service_items')