From 0ab55950a85ae82ebb479b19f115da37f48eb4b8 Mon Sep 17 00:00:00 2001 From: Daniel Martin Date: Sun, 25 Apr 2021 06:18:50 +0000 Subject: [PATCH] Fix service save failure --- openlp/core/ui/servicemanager.py | 6 ++- tests/openlp_core/ui/test_servicemanager.py | 56 ++++++++++++++++++--- 2 files changed, 53 insertions(+), 9 deletions(-) diff --git a/openlp/core/ui/servicemanager.py b/openlp/core/ui/servicemanager.py index 7b8da7204..5de86e580 100644 --- a/openlp/core/ui/servicemanager.py +++ b/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() - os.link(temp_file.name, file_path) + # 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)) diff --git a/tests/openlp_core/ui/test_servicemanager.py b/tests/openlp_core/ui/test_servicemanager.py index b02dfe57b..30d4a3582 100644 --- a/tests/openlp_core/ui/test_servicemanager.py +++ b/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', mocked_settings) + 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_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')