diff --git a/openlp/core/ui/servicemanager.py b/openlp/core/ui/servicemanager.py index 624a98fec..b0fde9b98 100644 --- a/openlp/core/ui/servicemanager.py +++ b/openlp/core/ui/servicemanager.py @@ -29,7 +29,6 @@ import zipfile from contextlib import suppress from datetime import datetime, timedelta from pathlib import Path -from tempfile import NamedTemporaryFile from PyQt5 import QtCore, QtGui, QtWidgets @@ -690,8 +689,8 @@ class ServiceManager(QtWidgets.QWidget, RegistryBase, Ui_ServiceManager, LogMixi self.log_debug('ServiceManager.save_file - ZIP contents size is %i bytes' % total_size) self.main_window.display_progress_bar(1000) try: - with NamedTemporaryFile(dir=str(file_path.parent), prefix='.') as temp_file, \ - zipfile.ZipFile(temp_file, 'w', zipfile.ZIP_DEFLATED) as zip_file: + tmp_file_path = str(file_path) + ".saving" + with zipfile.ZipFile(tmp_file_path, 'w', zipfile.ZIP_DEFLATED) as zip_file: # First we add service contents.. zip_file.writestr('service_data.osj', service_content) self.main_window.increment_progress_bar(service_content_size / total_size * 1000) @@ -701,11 +700,8 @@ 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) + # Move rather than copy to prevent writing another file if possible + shutil.move(tmp_file_path, 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 e42fc06ff..e71d13bda 100644 --- a/tests/openlp_core/ui/test_servicemanager.py +++ b/tests/openlp_core/ui/test_servicemanager.py @@ -690,11 +690,8 @@ def test_single_click_timeout_double(mocked_make_live, mocked_make_preview, sett @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_raises_permission_error(mocked_temp_file, mocked_shutil, mocked_os, mocked_save_file_as, - mocked_zipfile, settings): +def test_save_file_raises_permission_error(mocked_shutil, mocked_save_file_as, mocked_zipfile, settings): """ Test that when a PermissionError is raised when trying to save a file, it is handled correctly """ @@ -709,8 +706,7 @@ def test_save_file_raises_permission_error(mocked_temp_file, mocked_shutil, mock service_manager.service_manager_list = MagicMock() 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 + mocked_shutil.move.side_effect = PermissionError # WHEN: The service is saved and a PermissionError is raised result = service_manager.save_file() @@ -725,9 +721,7 @@ def test_save_file_raises_permission_error(mocked_temp_file, mocked_shutil, mock @patch('openlp.core.ui.servicemanager.os') @patch('openlp.core.ui.servicemanager.shutil') @patch('openlp.core.ui.servicemanager.len') -@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): +def test_save_file_large_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) @@ -759,39 +753,6 @@ def test_save_file_large_file(mocked_temp_file, mocked_len, mocked_shutil, mocke 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') def test_theme_change_global(mocked_regenerate_service_items, settings): """