From a6324b6b7f3b6a0266eac81bd9e604fa6525be15 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 8 Sep 2017 22:19:22 -0700 Subject: [PATCH 01/15] Refactor version check threading --- openlp.py | 8 +- openlp/core/__init__.py | 40 ++--- openlp/core/lib/plugin.py | 4 +- openlp/core/threading.py | 55 ++++++ openlp/core/ui/aboutform.py | 4 +- openlp/core/ui/exceptionform.py | 4 +- openlp/core/ui/generaltab.py | 1 - openlp/core/ui/mainwindow.py | 17 +- .../{common/versionchecker.py => version.py} | 162 ++++++++---------- 9 files changed, 171 insertions(+), 124 deletions(-) create mode 100644 openlp/core/threading.py rename openlp/core/{common/versionchecker.py => version.py} (57%) diff --git a/openlp.py b/openlp.py index 7ede25519..68001f2d1 100755 --- a/openlp.py +++ b/openlp.py @@ -20,13 +20,17 @@ # with this program; if not, write to the Free Software Foundation, Inc., 59 # # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### - -import sys +""" +The entrypoint for OpenLP +""" +import faulthandler import multiprocessing +import sys from openlp.core.common import is_win, is_macosx from openlp.core import main +faulthandler.enable() if __name__ == '__main__': """ diff --git a/openlp/core/__init__.py b/openlp/core/__init__.py index af29cddae..d7c21026e 100644 --- a/openlp/core/__init__.py +++ b/openlp/core/__init__.py @@ -26,21 +26,19 @@ The :mod:`core` module provides all core application functions All the core functions of the OpenLP application including the GUI, settings, logging and a plugin framework are contained within the openlp.core module. """ - import argparse import logging import os import shutil import sys import time -from pathlib import Path from traceback import format_exception from PyQt5 import QtCore, QtGui, QtWidgets from openlp.core.common import Registry, OpenLPMixin, AppLocation, LanguageManager, Settings, UiStrings, \ check_directory_exists, is_macosx, is_win, translate -from openlp.core.common.versionchecker import VersionThread, get_application_version +from openlp.core.version import check_for_update, get_version from openlp.core.lib import ScreenList from openlp.core.resources import qInitResources from openlp.core.ui import SplashScreen @@ -154,8 +152,8 @@ class OpenLP(OpenLPMixin, QtWidgets.QApplication): self.processEvents() if not has_run_wizard: self.main_window.first_time() - version = VersionThread(self.main_window) - version.start() + if Settings().value('core/update check'): + check_for_update(self.main_window) self.main_window.is_display_blank() self.main_window.app_startup() return self.exec() @@ -183,22 +181,18 @@ class OpenLP(OpenLPMixin, QtWidgets.QApplication): data_folder_path = str(AppLocation.get_data_path()) if not os.path.exists(data_folder_path): log.critical('Database was not found in: ' + data_folder_path) - status = QtWidgets.QMessageBox.critical(None, translate('OpenLP', 'Data Directory Error'), - translate('OpenLP', 'OpenLP data folder was not found in:\n\n{path}' - '\n\nThe location of the data folder was ' - 'previously changed from the OpenLP\'s ' - 'default location. If the data was stored on ' - 'removable device, that device needs to be ' - 'made available.\n\nYou may reset the data ' - 'location back to the default location, ' - 'or you can try to make the current location ' - 'available.\n\nDo you want to reset to the ' - 'default data location? If not, OpenLP will be ' - 'closed so you can try to fix the the problem.') - .format(path=data_folder_path), - QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Yes | - QtWidgets.QMessageBox.No), - QtWidgets.QMessageBox.No) + status = QtWidgets.QMessageBox.critical( + None, translate('OpenLP', 'Data Directory Error'), + translate('OpenLP', 'OpenLP data folder was not found in:\n\n{path}\n\nThe location of the data ' + 'folder was previously changed from the OpenLP\'s default location. If the data was ' + 'stored on removable device, that device needs to be made available.\n\nYou may reset ' + 'the data location back to the default location, or you can try to make the current ' + 'location available.\n\nDo you want to reset to the default data location? If not, ' + 'OpenLP will be closed so you can try to fix the the problem.').format( + path=data_folder_path), + QtWidgets.QMessageBox.StandardButtons(QtWidgets.QMessageBox.Yes | QtWidgets.QMessageBox.No), + QtWidgets.QMessageBox.No + ) if status == QtWidgets.QMessageBox.No: # If answer was "No", return "True", it will shutdown OpenLP in def main log.info('User requested termination') @@ -239,7 +233,7 @@ class OpenLP(OpenLPMixin, QtWidgets.QApplication): :param can_show_splash: Should OpenLP show the splash screen """ data_version = Settings().value('core/application version') - openlp_version = get_application_version()['version'] + openlp_version = get_version()['version'] # New installation, no need to create backup if not has_run_wizard: Settings().setValue('core/application version', openlp_version) @@ -415,7 +409,7 @@ def main(args=None): Registry.create() Registry().register('application', application) Registry().set_flag('no_web_server', args.no_web_server) - application.setApplicationVersion(get_application_version()['version']) + application.setApplicationVersion(get_version()['version']) # Check if an instance of OpenLP is already running. Quit if there is a running instance and the user only wants one if application.is_already_running(): sys.exit() diff --git a/openlp/core/lib/plugin.py b/openlp/core/lib/plugin.py index b06e0fbd4..d06385864 100644 --- a/openlp/core/lib/plugin.py +++ b/openlp/core/lib/plugin.py @@ -27,7 +27,7 @@ import logging from PyQt5 import QtCore from openlp.core.common import Registry, RegistryProperties, Settings, UiStrings -from openlp.core.common.versionchecker import get_application_version +from openlp.core.version import get_version log = logging.getLogger(__name__) @@ -139,7 +139,7 @@ class Plugin(QtCore.QObject, RegistryProperties): if version: self.version = version else: - self.version = get_application_version()['version'] + self.version = get_version()['version'] self.settings_section = self.name self.icon = None self.media_item_class = media_item_class diff --git a/openlp/core/threading.py b/openlp/core/threading.py new file mode 100644 index 000000000..3eda2e436 --- /dev/null +++ b/openlp/core/threading.py @@ -0,0 +1,55 @@ +# -*- coding: utf-8 -*- +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4 + +############################################################################### +# OpenLP - Open Source Lyrics Projection # +# --------------------------------------------------------------------------- # +# Copyright (c) 2008-2017 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 # +############################################################################### +""" +The :mod:`openlp.core.threading` module contains some common threading code +""" +from PyQt5 import QtCore + + +def run_thread(parent, worker, prefix='', auto_start=True): + """ + Create a thread and assign a worker to it. This removes a lot of boilerplate code from the codebase. + + :param object parent: The parent object so that the thread and worker are not orphaned. + :param QObject worker: A QObject-based worker object which does the actual work. + :param str prefix: A prefix to be applied to the attribute names. + :param bool auto_start: Automatically start the thread. Defaults to True. + """ + # Set up attribute names + thread_name = 'thread' + worker_name = 'worker' + if prefix: + thread_name = '_'.join([prefix, thread_name]) + worker_name = '_'.join([prefix, worker_name]) + # Create the thread and add the thread and the worker to the parent + thread = QtCore.QThread() + setattr(parent, thread_name, thread) + setattr(parent, worker_name, worker) + # Move the worker into the thread's context + worker.moveToThread(thread) + # Connect slots and signals + parent.version_thread.started.connect(parent.version_worker.start) + parent.version_worker.quit.connect(parent.version_thread.quit) + parent.version_worker.quit.connect(parent.version_worker.deleteLater) + parent.version_thread.finished.connect(parent.version_thread.deleteLater) + if auto_start: + parent.version_thread.start() diff --git a/openlp/core/ui/aboutform.py b/openlp/core/ui/aboutform.py index e1768b127..bed83785b 100644 --- a/openlp/core/ui/aboutform.py +++ b/openlp/core/ui/aboutform.py @@ -26,7 +26,7 @@ import webbrowser from PyQt5 import QtCore, QtWidgets -from openlp.core.common.versionchecker import get_application_version +from openlp.core.version import get_version from openlp.core.lib import translate from .aboutdialog import UiAboutDialog @@ -49,7 +49,7 @@ class AboutForm(QtWidgets.QDialog, UiAboutDialog): Set up the dialog. This method is mocked out in tests. """ self.setup_ui(self) - application_version = get_application_version() + application_version = get_version() about_text = self.about_text_edit.toPlainText() about_text = about_text.replace('', application_version['version']) if application_version['build']: diff --git a/openlp/core/ui/exceptionform.py b/openlp/core/ui/exceptionform.py index 349352d92..c8a1753c2 100644 --- a/openlp/core/ui/exceptionform.py +++ b/openlp/core/ui/exceptionform.py @@ -71,7 +71,7 @@ except ImportError: VLC_VERSION = '-' from openlp.core.common import Settings, UiStrings, translate -from openlp.core.common.versionchecker import get_application_version +from openlp.core.version import get_version from openlp.core.common import RegistryProperties, is_linux from .exceptiondialog import Ui_ExceptionDialog @@ -110,7 +110,7 @@ class ExceptionForm(QtWidgets.QDialog, Ui_ExceptionDialog, RegistryProperties): """ Create an exception report. """ - openlp_version = get_application_version() + openlp_version = get_version() description = self.description_text_edit.toPlainText() traceback = self.exception_text_edit.toPlainText() system = translate('OpenLP.ExceptionForm', 'Platform: {platform}\n').format(platform=platform.platform()) diff --git a/openlp/core/ui/generaltab.py b/openlp/core/ui/generaltab.py index dc084eb2b..d3c44ff6c 100644 --- a/openlp/core/ui/generaltab.py +++ b/openlp/core/ui/generaltab.py @@ -164,7 +164,6 @@ class GeneralTab(SettingsTab): self.startup_layout.addWidget(self.show_splash_check_box) self.check_for_updates_check_box = QtWidgets.QCheckBox(self.startup_group_box) self.check_for_updates_check_box.setObjectName('check_for_updates_check_box') - self.check_for_updates_check_box.setVisible(False) self.startup_layout.addWidget(self.check_for_updates_check_box) self.right_layout.addWidget(self.startup_group_box) # Logo diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 54f70ccb2..a8f7d91c7 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -40,7 +40,6 @@ from openlp.core.api.http import server from openlp.core.common import Registry, RegistryProperties, AppLocation, LanguageManager, Settings, UiStrings, \ check_directory_exists, translate, is_win, is_macosx, add_actions from openlp.core.common.actions import ActionList, CategoryOrder -from openlp.core.common.versionchecker import get_application_version from openlp.core.lib import Renderer, PluginManager, ImageManager, PluginStatus, ScreenList, build_icon from openlp.core.lib.ui import create_action from openlp.core.ui import AboutForm, SettingsForm, ServiceManager, ThemeManager, LiveController, PluginForm, \ @@ -51,6 +50,7 @@ from openlp.core.ui.printserviceform import PrintServiceForm from openlp.core.ui.projector.manager import ProjectorManager from openlp.core.ui.lib.dockwidget import OpenLPDockWidget from openlp.core.ui.lib.mediadockmanager import MediaDockManager +from openlp.core.version import get_version log = logging.getLogger(__name__) @@ -487,7 +487,6 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): """ The main window. """ - openlp_version_check = QtCore.pyqtSignal(QtCore.QVariant) log.info('MainWindow loaded') def __init__(self): @@ -561,7 +560,6 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): self.application.set_busy_cursor() # Simple message boxes Registry().register_function('theme_update_global', self.default_theme_changed) - self.openlp_version_check.connect(self.version_notice) Registry().register_function('config_screen_changed', self.screen_changed) Registry().register_function('bootstrap_post_set_up', self.bootstrap_post_set_up) # Reset the cursor @@ -587,6 +585,13 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): if saved_plugin_id != -1: self.media_tool_box.setCurrentIndex(saved_plugin_id) + def on_new_version_number(self, version_number): + """ + Called when the version check thread completes and we need to check the version number + + :param str version_number: The version number downloaded from the OpenLP server. + """ + def on_search_shortcut_triggered(self): """ Called when the search shortcut has been pressed. @@ -606,7 +611,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): if widget: widget.on_focus() - def version_notice(self, version): + def on_new_version(self, version): """ Notifies the user that a newer version of OpenLP is available. Triggered by delay thread and cannot display popup. @@ -616,7 +621,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): log.debug('version_notice') version_text = translate('OpenLP.MainWindow', 'Version {new} of OpenLP is now available for download (you are ' 'currently running version {current}). \n\nYou can download the latest version from ' - 'http://openlp.org/.').format(new=version, current=get_application_version()[u'full']) + 'http://openlp.org/.').format(new=version, current=get_version()[u'full']) QtWidgets.QMessageBox.question(self, translate('OpenLP.MainWindow', 'OpenLP Version Updated'), version_text) def show(self): @@ -973,7 +978,7 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): # Add a header section. # This is to insure it's our conf file for import. now = datetime.now() - application_version = get_application_version() + application_version = get_version() # Write INI format using Qsettings. # Write our header. export_settings.beginGroup(self.header_section) diff --git a/openlp/core/common/versionchecker.py b/openlp/core/version.py similarity index 57% rename from openlp/core/common/versionchecker.py rename to openlp/core/version.py index 6129ee2aa..3fa6c006c 100644 --- a/openlp/core/common/versionchecker.py +++ b/openlp/core/version.py @@ -20,24 +20,21 @@ # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### """ -The :mod:`openlp.core.common` module downloads the version details for OpenLP. +The :mod:`openlp.core.version` module downloads the version details for OpenLP. """ import logging import os import platform import sys -import time -import urllib.error -import urllib.parse -import urllib.request from datetime import datetime from distutils.version import LooseVersion from subprocess import Popen, PIPE +import requests from PyQt5 import QtCore -from openlp.core.common import AppLocation, Registry, Settings -from openlp.core.common.httputils import ping +from openlp.core.common import AppLocation, Settings +from openlp.core.threading import run_thread log = logging.getLogger(__name__) @@ -46,42 +43,87 @@ CONNECTION_TIMEOUT = 30 CONNECTION_RETRIES = 2 -class VersionThread(QtCore.QThread): +class VersionWorker(QtCore.QObject): """ - A special Qt thread class to fetch the version of OpenLP from the website. - This is threaded so that it doesn't affect the loading time of OpenLP. + A worker class to fetch the version of OpenLP from the website. This is run from within a thread so that it + doesn't affect the loading time of OpenLP. """ - def __init__(self, main_window): - """ - Constructor for the thread class. + new_version = QtCore.pyqtSignal(dict) + no_internet = QtCore.pyqtSignal() + quit = QtCore.pyqtSignal() - :param main_window: The main window Object. + def __init__(self, last_check_date): """ - log.debug("VersionThread - Initialise") - super(VersionThread, self).__init__(None) - self.main_window = main_window + Constructor for the version check worker. - def run(self): + :param string last_check_date: The last day we checked for a new version of OpenLP """ - Run the thread. + log.debug('VersionWorker - Initialise') + super(VersionWorker, self).__init__(None) + self.last_check_date = last_check_date + + def start(self): """ - self.sleep(1) - log.debug('Version thread - run') - found = ping("openlp.io") - Registry().set_flag('internet_present', found) - update_check = Settings().value('core/update check') - if found: - Registry().execute('get_website_version') - if update_check: - app_version = get_application_version() - version = check_latest_version(app_version) - log.debug("Versions {version1} and {version2} ".format(version1=LooseVersion(str(version)), - version2=LooseVersion(str(app_version['full'])))) - if LooseVersion(str(version)) > LooseVersion(str(app_version['full'])): - self.main_window.openlp_version_check.emit('{version}'.format(version=version)) + Check the latest version of OpenLP against the version file on the OpenLP site. + + **Rules around versions and version files:** + + * If a version number has a build (i.e. -bzr1234), then it is a nightly. + * If a version number's minor version is an odd number, it is a development release. + * If a version number's minor version is an even number, it is a stable release. + """ + log.debug('VersionWorker - Start') + # I'm not entirely sure why this was here, I'm commenting it out until I hit the same scenario + # time.sleep(1) + current_version = get_version() + download_url = 'http://www.openlp.org/files/version.txt' + if current_version['build']: + download_url = 'http://www.openlp.org/files/nightly_version.txt' + elif int(current_version['version'].split('.')[1]) % 2 != 0: + download_url = 'http://www.openlp.org/files/dev_version.txt' + headers = { + 'User-Agent', 'OpenLP/{version} {system}/{release}; '.format(version=current_version['full'], + system=platform.system(), + release=platform.release()) + } + remote_version = None + retries = 0 + while retries < 3: + try: + response = requests.get(download_url, headers=headers) + remote_version = response.text + log.debug('New version found: %s', remote_version) + break + except requests.exceptions.ConnectionError: + log.exception('Unable to connect to OpenLP server to download version file') + self.no_internet.emit() + retries += 1 + except requests.exceptions.RequestException: + log.exception('Error occurred while connecting to OpenLP server to download version file') + retries += 1 + if remote_version and LooseVersion(remote_version) > LooseVersion(current_version['full']): + self.new_version.emit(remote_version) + self.quit.emit() -def get_application_version(): +def check_for_update(parent): + """ + Run a thread to download and check the version of OpenLP + + :param MainWindow parent: The parent object for the thread. Usually the OpenLP main window. + """ + last_check_date = Settings().value('core/last version test') + if datetime.date().strftime('%Y-%m-%d') <= last_check_date: + log.debug('Version check skipped, last checked today') + return + worker = VersionWorker(last_check_date) + worker.new_version.connect(parent.on_new_version) + # TODO: Use this to figure out if there's an Internet connection? + # worker.no_internet.connect(parent.on_no_internet) + run_thread(parent, worker, 'version') + + +def get_version(): """ Returns the application version of the running instance of OpenLP:: @@ -150,55 +192,3 @@ def get_application_version(): else: log.info('Openlp version {version}'.format(version=APPLICATION_VERSION['version'])) return APPLICATION_VERSION - - -def check_latest_version(current_version): - """ - Check the latest version of OpenLP against the version file on the OpenLP - site. - - **Rules around versions and version files:** - - * If a version number has a build (i.e. -bzr1234), then it is a nightly. - * If a version number's minor version is an odd number, it is a development release. - * If a version number's minor version is an even number, it is a stable release. - - :param current_version: The current version of OpenLP. - """ - version_string = current_version['full'] - # set to prod in the distribution config file. - settings = Settings() - settings.beginGroup('core') - last_test = settings.value('last version test') - this_test = str(datetime.now().date()) - settings.setValue('last version test', this_test) - settings.endGroup() - if last_test != this_test: - if current_version['build']: - req = urllib.request.Request('http://www.openlp.org/files/nightly_version.txt') - else: - version_parts = current_version['version'].split('.') - if int(version_parts[1]) % 2 != 0: - req = urllib.request.Request('http://www.openlp.org/files/dev_version.txt') - else: - req = urllib.request.Request('http://www.openlp.org/files/version.txt') - req.add_header('User-Agent', 'OpenLP/{version} {system}/{release}; '.format(version=current_version['full'], - system=platform.system(), - release=platform.release())) - remote_version = None - retries = 0 - while True: - try: - remote_version = str(urllib.request.urlopen(req, None, - timeout=CONNECTION_TIMEOUT).read().decode()).strip() - except (urllib.error.URLError, ConnectionError): - if retries > CONNECTION_RETRIES: - log.exception('Failed to download the latest OpenLP version file') - else: - retries += 1 - time.sleep(0.1) - continue - break - if remote_version: - version_string = remote_version - return version_string From 47f96a32860eaa328f756ced6df2da580a5f30e8 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 8 Sep 2017 22:57:06 -0700 Subject: [PATCH 02/15] Duh. Can't have the old names in there, now can we? --- openlp/core/threading.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/openlp/core/threading.py b/openlp/core/threading.py index 3eda2e436..36dfd481f 100644 --- a/openlp/core/threading.py +++ b/openlp/core/threading.py @@ -47,9 +47,9 @@ def run_thread(parent, worker, prefix='', auto_start=True): # Move the worker into the thread's context worker.moveToThread(thread) # Connect slots and signals - parent.version_thread.started.connect(parent.version_worker.start) - parent.version_worker.quit.connect(parent.version_thread.quit) - parent.version_worker.quit.connect(parent.version_worker.deleteLater) - parent.version_thread.finished.connect(parent.version_thread.deleteLater) + thread.started.connect(worker.start) + worker.quit.connect(thread.quit) + worker.quit.connect(worker.deleteLater) + thread.finished.connect(thread.deleteLater) if auto_start: - parent.version_thread.start() + thread.start() From d1a47036cf644828ef2cfa9ffddeca92dd877a32 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Mon, 11 Sep 2017 22:52:38 -0700 Subject: [PATCH 03/15] Remove unnecessary slot --- openlp/core/ui/mainwindow.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 4cee4a1f4..16328d63a 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -586,13 +586,6 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): if saved_plugin_id != -1: self.media_tool_box.setCurrentIndex(saved_plugin_id) - def on_new_version_number(self, version_number): - """ - Called when the version check thread completes and we need to check the version number - - :param str version_number: The version number downloaded from the OpenLP server. - """ - def on_search_shortcut_triggered(self): """ Called when the search shortcut has been pressed. From 2f8cdc81e03869ba72b5983e5848d9906cac8c56 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Tue, 12 Sep 2017 23:08:38 -0700 Subject: [PATCH 04/15] Fix up the version tests and remove old references to 'versionchecker' --- openlp/core/ui/mainwindow.py | 13 ++ openlp/core/version.py | 38 ++-- openlp/plugins/songs/lib/openlyricsxml.py | 4 +- scripts/check_dependencies.py | 7 +- tests/functional/openlp_core/test_version.py | 204 ++++++++++++++++++ .../openlp_core_common/test_versionchecker.py | 64 ------ .../openlp_core_ui/test_aboutform.py | 8 +- .../openlp_core_ui/test_exceptionform.py | 63 ++---- tests/functional/test_init.py | 12 +- 9 files changed, 273 insertions(+), 140 deletions(-) create mode 100644 tests/functional/openlp_core/test_version.py delete mode 100644 tests/functional/openlp_core_common/test_versionchecker.py diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 16328d63a..ae32c0347 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -496,6 +496,8 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): """ super(MainWindow, self).__init__() Registry().register('main_window', self) + self.version_thread = None + self.version_worker = None self.clipboard = self.application.clipboard() self.arguments = ''.join(self.application.args) # Set up settings sections for the main application (not for use by plugins). @@ -1009,6 +1011,17 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): if not self.application.is_event_loop_active: event.ignore() return + # Sometimes the version thread hasn't finished, let's wait for it + try: + if self.version_thread and self.version_thread.isRunning(): + wait_dialog = QtWidgets.QProgressDialog('Waiting for some things to finish...', '', 0, 0, self) + wait_dialog.setWindowModality(QtCore.Qt.WindowModal) + wait_dialog.setAutoClose(False) + self.version_thread.wait() + wait_dialog.close() + except RuntimeError: + # Ignore the RuntimeError that is thrown when Qt has already deleted the C++ thread object + pass # If we just did a settings import, close without saving changes. if self.settings_imported: self.clean_up(False) diff --git a/openlp/core/version.py b/openlp/core/version.py index 3fa6c006c..9a532fcd8 100644 --- a/openlp/core/version.py +++ b/openlp/core/version.py @@ -26,7 +26,8 @@ import logging import os import platform import sys -from datetime import datetime +import time +from datetime import date from distutils.version import LooseVersion from subprocess import Popen, PIPE @@ -52,7 +53,7 @@ class VersionWorker(QtCore.QObject): no_internet = QtCore.pyqtSignal() quit = QtCore.pyqtSignal() - def __init__(self, last_check_date): + def __init__(self, last_check_date, current_version): """ Constructor for the version check worker. @@ -61,6 +62,7 @@ class VersionWorker(QtCore.QObject): log.debug('VersionWorker - Initialise') super(VersionWorker, self).__init__(None) self.last_check_date = last_check_date + self.current_version = current_version def start(self): """ @@ -74,15 +76,14 @@ class VersionWorker(QtCore.QObject): """ log.debug('VersionWorker - Start') # I'm not entirely sure why this was here, I'm commenting it out until I hit the same scenario - # time.sleep(1) - current_version = get_version() + time.sleep(1) download_url = 'http://www.openlp.org/files/version.txt' - if current_version['build']: + if self.current_version['build']: download_url = 'http://www.openlp.org/files/nightly_version.txt' - elif int(current_version['version'].split('.')[1]) % 2 != 0: + elif int(self.current_version['version'].split('.')[1]) % 2 != 0: download_url = 'http://www.openlp.org/files/dev_version.txt' headers = { - 'User-Agent', 'OpenLP/{version} {system}/{release}; '.format(version=current_version['full'], + 'User-Agent': 'OpenLP/{version} {system}/{release}; '.format(version=self.current_version['full'], system=platform.system(), release=platform.release()) } @@ -94,18 +95,23 @@ class VersionWorker(QtCore.QObject): remote_version = response.text log.debug('New version found: %s', remote_version) break - except requests.exceptions.ConnectionError: + except IOError: log.exception('Unable to connect to OpenLP server to download version file') - self.no_internet.emit() retries += 1 - except requests.exceptions.RequestException: - log.exception('Error occurred while connecting to OpenLP server to download version file') - retries += 1 - if remote_version and LooseVersion(remote_version) > LooseVersion(current_version['full']): + else: + self.no_internet.emit() + if remote_version and LooseVersion(remote_version) > LooseVersion(self.current_version['full']): self.new_version.emit(remote_version) self.quit.emit() +def update_check_date(): + """ + Save when we last checked for an update + """ + Settings().setValue('core/last version test', date.today().strftime('%Y-%m-%d')) + + def check_for_update(parent): """ Run a thread to download and check the version of OpenLP @@ -113,11 +119,12 @@ def check_for_update(parent): :param MainWindow parent: The parent object for the thread. Usually the OpenLP main window. """ last_check_date = Settings().value('core/last version test') - if datetime.date().strftime('%Y-%m-%d') <= last_check_date: + if date.today().strftime('%Y-%m-%d') <= last_check_date: log.debug('Version check skipped, last checked today') return - worker = VersionWorker(last_check_date) + worker = VersionWorker(last_check_date, get_version()) worker.new_version.connect(parent.on_new_version) + worker.quit.connect(update_check_date) # TODO: Use this to figure out if there's an Internet connection? # worker.no_internet.connect(parent.on_no_internet) run_thread(parent, worker, 'version') @@ -132,6 +139,7 @@ def get_version(): global APPLICATION_VERSION if APPLICATION_VERSION: return APPLICATION_VERSION + print(sys.argv) if '--dev-version' in sys.argv or '-d' in sys.argv: # NOTE: The following code is a duplicate of the code in setup.py. Any fix applied here should also be applied # there. diff --git a/openlp/plugins/songs/lib/openlyricsxml.py b/openlp/plugins/songs/lib/openlyricsxml.py index 807ea5593..4819e61de 100644 --- a/openlp/plugins/songs/lib/openlyricsxml.py +++ b/openlp/plugins/songs/lib/openlyricsxml.py @@ -62,7 +62,7 @@ import re from lxml import etree, objectify from openlp.core.common import translate, Settings -from openlp.core.common.versionchecker import get_application_version +from openlp.core.version import get_version from openlp.core.lib import FormattingTags from openlp.plugins.songs.lib import VerseType, clean_song from openlp.plugins.songs.lib.db import Author, AuthorType, Book, Song, Topic @@ -234,7 +234,7 @@ class OpenLyrics(object): # Append the necessary meta data to the song. song_xml.set('xmlns', NAMESPACE) song_xml.set('version', OpenLyrics.IMPLEMENTED_VERSION) - application_name = 'OpenLP ' + get_application_version()['version'] + application_name = 'OpenLP ' + get_version()['version'] song_xml.set('createdIn', application_name) song_xml.set('modifiedIn', application_name) # "Convert" 2012-08-27 11:49:15 to 2012-08-27T11:49:15. diff --git a/scripts/check_dependencies.py b/scripts/check_dependencies.py index 253cb330c..1d8ffff3e 100755 --- a/scripts/check_dependencies.py +++ b/scripts/check_dependencies.py @@ -26,7 +26,7 @@ This script is used to check dependencies of OpenLP. It checks availability of required python modules and their version. To verify availability of Python modules, simply run this script:: - @:~$ ./check_dependencies.py + $ ./check_dependencies.py """ import os @@ -45,7 +45,7 @@ IS_MAC = sys.platform.startswith('dar') VERS = { - 'Python': '3.0', + 'Python': '3.4', 'PyQt5': '5.0', 'Qt5': '5.0', 'sqlalchemy': '0.5', @@ -97,7 +97,8 @@ MODULES = [ 'asyncio', 'waitress', 'six', - 'webob' + 'webob', + 'requests' ] diff --git a/tests/functional/openlp_core/test_version.py b/tests/functional/openlp_core/test_version.py new file mode 100644 index 000000000..c5790a9b6 --- /dev/null +++ b/tests/functional/openlp_core/test_version.py @@ -0,0 +1,204 @@ +# -*- coding: utf-8 -*- +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4 + +############################################################################### +# OpenLP - Open Source Lyrics Projection # +# --------------------------------------------------------------------------- # +# Copyright (c) 2008-2017 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 # +############################################################################### +""" +Package to test the openlp.core.version package. +""" +import sys +from datetime import date +from unittest.mock import MagicMock, patch + +from requests.exceptions import ConnectionError + +from openlp.core.version import VersionWorker, check_for_update, get_version, update_check_date + + +def test_worker_init(): + """Test the VersionWorker constructor""" + # GIVEN: A last check date and a current version + last_check_date = '1970-01-01' + current_version = '2.0' + + # WHEN: A worker is created + worker = VersionWorker(last_check_date, current_version) + + # THEN: The correct attributes should have been set + assert worker.last_check_date == last_check_date + assert worker.current_version == current_version + + +@patch('openlp.core.version.platform') +@patch('openlp.core.version.requests') +def test_worker_start(mock_requests, mock_platform): + """Test the VersionWorkder.start() method""" + # GIVEN: A last check date, current version, and an instance of worker + last_check_date = '1970-01-01' + current_version = {'full': '2.0', 'version': '2.0', 'build': None} + mock_platform.system.return_value = 'Linux' + mock_platform.release.return_value = '4.12.0-1-amd64' + mock_requests.get.return_value = MagicMock(text='2.4.6') + worker = VersionWorker(last_check_date, current_version) + + # WHEN: The worker is run + with patch.object(worker, 'new_version') as mock_new_version, \ + patch.object(worker, 'quit') as mock_quit: + worker.start() + + # THEN: The check completes and the signal is emitted + expected_download_url = 'http://www.openlp.org/files/version.txt' + expected_headers = {'User-Agent': 'OpenLP/2.0 Linux/4.12.0-1-amd64; '} + mock_requests.get.assert_called_once_with(expected_download_url, headers=expected_headers) + mock_new_version.emit.assert_called_once_with('2.4.6') + mock_quit.emit.assert_called_once_with() + + +@patch('openlp.core.version.platform') +@patch('openlp.core.version.requests') +def test_worker_start_dev_version(mock_requests, mock_platform): + """Test the VersionWorkder.start() method for dev versions""" + # GIVEN: A last check date, current version, and an instance of worker + last_check_date = '1970-01-01' + current_version = {'full': '2.1.3', 'version': '2.1.3', 'build': None} + mock_platform.system.return_value = 'Linux' + mock_platform.release.return_value = '4.12.0-1-amd64' + mock_requests.get.return_value = MagicMock(text='2.4.6') + worker = VersionWorker(last_check_date, current_version) + + # WHEN: The worker is run + with patch.object(worker, 'new_version') as mock_new_version, \ + patch.object(worker, 'quit') as mock_quit: + worker.start() + + # THEN: The check completes and the signal is emitted + expected_download_url = 'http://www.openlp.org/files/dev_version.txt' + expected_headers = {'User-Agent': 'OpenLP/2.1.3 Linux/4.12.0-1-amd64; '} + mock_requests.get.assert_called_once_with(expected_download_url, headers=expected_headers) + mock_new_version.emit.assert_called_once_with('2.4.6') + mock_quit.emit.assert_called_once_with() + + +@patch('openlp.core.version.platform') +@patch('openlp.core.version.requests') +def test_worker_start_nightly_version(mock_requests, mock_platform): + """Test the VersionWorkder.start() method for nightlies""" + # GIVEN: A last check date, current version, and an instance of worker + last_check_date = '1970-01-01' + current_version = {'full': '2.1-bzr2345', 'version': '2.1', 'build': '2345'} + mock_platform.system.return_value = 'Linux' + mock_platform.release.return_value = '4.12.0-1-amd64' + mock_requests.get.return_value = MagicMock(text='2.4.6') + worker = VersionWorker(last_check_date, current_version) + + # WHEN: The worker is run + with patch.object(worker, 'new_version') as mock_new_version, \ + patch.object(worker, 'quit') as mock_quit: + worker.start() + + # THEN: The check completes and the signal is emitted + expected_download_url = 'http://www.openlp.org/files/nightly_version.txt' + expected_headers = {'User-Agent': 'OpenLP/2.1-bzr2345 Linux/4.12.0-1-amd64; '} + mock_requests.get.assert_called_once_with(expected_download_url, headers=expected_headers) + mock_new_version.emit.assert_called_once_with('2.4.6') + mock_quit.emit.assert_called_once_with() + + +@patch('openlp.core.version.platform') +@patch('openlp.core.version.requests') +def test_worker_start_connection_error(mock_requests, mock_platform): + """Test the VersionWorkder.start() method when a ConnectionError happens""" + # GIVEN: A last check date, current version, and an instance of worker + last_check_date = '1970-01-01' + current_version = {'full': '2.0', 'version': '2.0', 'build': None} + mock_platform.system.return_value = 'Linux' + mock_platform.release.return_value = '4.12.0-1-amd64' + mock_requests.get.side_effect = ConnectionError('Could not connect') + worker = VersionWorker(last_check_date, current_version) + + # WHEN: The worker is run + with patch.object(worker, 'no_internet') as mocked_no_internet, \ + patch.object(worker, 'quit') as mocked_quit: + worker.start() + + # THEN: The check completes and the signal is emitted + expected_download_url = 'http://www.openlp.org/files/version.txt' + expected_headers = {'User-Agent': 'OpenLP/2.0 Linux/4.12.0-1-amd64; '} + mock_requests.get.assert_called_with(expected_download_url, headers=expected_headers) + assert mock_requests.get.call_count == 3 + mocked_no_internet.emit.assert_called_once_with() + mocked_quit.emit.assert_called_once_with() + + +@patch('openlp.core.version.Settings') +def test_update_check_date(MockSettings): + """Test that the update_check_date() function writes the correct date""" + # GIVEN: A mocked Settings object + mocked_settings = MagicMock() + MockSettings.return_value = mocked_settings + + # WHEN: update_check_date() is called + update_check_date() + + # THEN: The correct date should have been saved + mocked_settings.setValue.assert_called_once_with('core/last version test', date.today().strftime('%Y-%m-%d')) + + +@patch('openlp.core.version.Settings') +@patch('openlp.core.version.run_thread') +def test_check_for_update(mocked_run_thread, MockSettings): + """Test the check_for_update() function""" + # GIVEN: A mocked settings object + mocked_settings = MagicMock() + mocked_settings.value.return_value = '1970-01-01' + MockSettings.return_value = mocked_settings + + # WHEN: check_for_update() is called + check_for_update(MagicMock()) + + # THEN: The right things should have been called and a thread set in motion + assert mocked_run_thread.call_count == 1 + + +@patch('openlp.core.version.Settings') +@patch('openlp.core.version.run_thread') +def test_check_for_update_skipped(mocked_run_thread, MockSettings): + """Test that the check_for_update() function skips running if it already ran today""" + # GIVEN: A mocked settings object + mocked_settings = MagicMock() + mocked_settings.value.return_value = date.today().strftime('%Y-%m-%d') + MockSettings.return_value = mocked_settings + + # WHEN: check_for_update() is called + check_for_update(MagicMock()) + + # THEN: The right things should have been called and a thread set in motion + assert mocked_run_thread.call_count == 0 + + +def test_get_version_dev_version(): + """Test the get_version() function""" + # GIVEN: We're in dev mode + with patch.object(sys, 'argv', ['--dev-version']), \ + patch('openlp.core.version.APPLICATION_VERSION', None): + # WHEN: get_version() is run + version = get_version() + + # THEN: version is something + assert version diff --git a/tests/functional/openlp_core_common/test_versionchecker.py b/tests/functional/openlp_core_common/test_versionchecker.py deleted file mode 100644 index 022a5f06f..000000000 --- a/tests/functional/openlp_core_common/test_versionchecker.py +++ /dev/null @@ -1,64 +0,0 @@ -# -*- coding: utf-8 -*- -# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4 - -############################################################################### -# OpenLP - Open Source Lyrics Projection # -# --------------------------------------------------------------------------- # -# Copyright (c) 2008-2017 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 # -############################################################################### -""" -Package to test the openlp.core.common.versionchecker package. -""" -from unittest import TestCase -from unittest.mock import MagicMock, patch - -from openlp.core.common.settings import Settings -from openlp.core.common.versionchecker import VersionThread - -from tests.helpers.testmixin import TestMixin - - -class TestVersionchecker(TestMixin, TestCase): - - def setUp(self): - """ - Create an instance and a few example actions. - """ - self.build_settings() - - def tearDown(self): - """ - Clean up - """ - self.destroy_settings() - - def test_version_thread_triggered(self): - """ - Test the version thread call does not trigger UI - :return: - """ - # GIVEN: a equal version setup and the data is not today. - mocked_main_window = MagicMock() - Settings().setValue('core/last version test', '1950-04-01') - # WHEN: We check to see if the version is different . - with patch('PyQt5.QtCore.QThread'),\ - patch('openlp.core.common.versionchecker.get_application_version') as mocked_get_application_version: - mocked_get_application_version.return_value = {'version': '1.0.0', 'build': '', 'full': '2.0.4'} - version_thread = VersionThread(mocked_main_window) - version_thread.run() - # THEN: If the version has changed the main window is notified - self.assertTrue(mocked_main_window.openlp_version_check.emit.called, - 'The main windows should have been notified') diff --git a/tests/functional/openlp_core_ui/test_aboutform.py b/tests/functional/openlp_core_ui/test_aboutform.py index 817c13c48..c30ef588e 100644 --- a/tests/functional/openlp_core_ui/test_aboutform.py +++ b/tests/functional/openlp_core_ui/test_aboutform.py @@ -47,13 +47,13 @@ class TestFirstTimeForm(TestCase, TestMixin): # THEN: A web browser is opened mocked_webbrowser.open_new.assert_called_with('http://openlp.org/en/contribute') - @patch('openlp.core.ui.aboutform.get_application_version') - def test_about_form_build_number(self, mocked_get_application_version): + @patch('openlp.core.ui.aboutform.get_version') + def test_about_form_build_number(self, mocked_get_version): """ Test that the build number is added to the about form """ - # GIVEN: A mocked out get_application_version function - mocked_get_application_version.return_value = {'version': '3.1.5', 'build': '3000'} + # GIVEN: A mocked out get_version function + mocked_get_version.return_value = {'version': '3.1.5', 'build': '3000'} # WHEN: The about form is created about_form = AboutForm(None) diff --git a/tests/functional/openlp_core_ui/test_exceptionform.py b/tests/functional/openlp_core_ui/test_exceptionform.py index 37a040aa6..4bc83dbcd 100644 --- a/tests/functional/openlp_core_ui/test_exceptionform.py +++ b/tests/functional/openlp_core_ui/test_exceptionform.py @@ -53,7 +53,7 @@ MAIL_ITEM_TEXT = ('**OpenLP Bug Report**\nVersion: Trunk Test\n\n--- Details of @patch("openlp.core.ui.exceptionform.Qt.qVersion") @patch("openlp.core.ui.exceptionform.QtGui.QDesktopServices.openUrl") -@patch("openlp.core.ui.exceptionform.get_application_version") +@patch("openlp.core.ui.exceptionform.get_version") @patch("openlp.core.ui.exceptionform.sqlalchemy") @patch("openlp.core.ui.exceptionform.bs4") @patch("openlp.core.ui.exceptionform.etree") @@ -64,18 +64,10 @@ class TestExceptionForm(TestMixin, TestCase): """ Test functionality of exception form functions """ - def __method_template_for_class_patches(self, - __PLACEHOLDER_FOR_LOCAL_METHOD_PATCH_DECORATORS_GO_HERE__, - mocked_python_version, - mocked_platform, - mocked_is_linux, - mocked_etree, - mocked_bs4, - mocked_sqlalchemy, - mocked_application_version, - mocked_openlurl, - mocked_qversion, - ): + def __method_template_for_class_patches(self, __PLACEHOLDER_FOR_LOCAL_METHOD_PATCH_DECORATORS_GO_HERE__, + mocked_python_version, mocked_platform, mocked_is_linux, + mocked_etree, mocked_bs4, mocked_sqlalchemy, mocked_get_version, + mocked_openlurl, mocked_qversion): """ Template so you don't have to remember the layout of class mock options for methods """ @@ -86,7 +78,7 @@ class TestExceptionForm(TestMixin, TestCase): mocked_platform.return_value = 'Nose Test' mocked_qversion.return_value = 'Qt5 test' mocked_is_linux.return_value = False - mocked_application_version.return_value = 'Trunk Test' + mocked_get_version.return_value = 'Trunk Test' def setUp(self): self.setup_application() @@ -107,22 +99,10 @@ class TestExceptionForm(TestMixin, TestCase): @patch("openlp.core.ui.exceptionform.QtCore.QUrl") @patch("openlp.core.ui.exceptionform.QtCore.QUrlQuery.addQueryItem") @patch("openlp.core.ui.exceptionform.Qt") - def test_on_send_report_button_clicked(self, - mocked_qt, - mocked_add_query_item, - mocked_qurl, - mocked_file_dialog, - mocked_ui_exception_dialog, - mocked_python_version, - mocked_platform, - mocked_is_linux, - mocked_etree, - mocked_bs4, - mocked_sqlalchemy, - mocked_application_version, - mocked_openlurl, - mocked_qversion, - ): + def test_on_send_report_button_clicked(self, mocked_qt, mocked_add_query_item, mocked_qurl, mocked_file_dialog, + mocked_ui_exception_dialog, mocked_python_version, mocked_platform, + mocked_is_linux, mocked_etree, mocked_bs4, mocked_sqlalchemy, + mocked_get_version, mocked_openlurl, mocked_qversion): """ Test send report creates the proper system information text """ @@ -134,10 +114,10 @@ class TestExceptionForm(TestMixin, TestCase): mocked_platform.return_value = 'Nose Test' mocked_qversion.return_value = 'Qt5 test' mocked_is_linux.return_value = False - mocked_application_version.return_value = 'Trunk Test' + mocked_get_version.return_value = 'Trunk Test' mocked_qt.PYQT_VERSION_STR = 'PyQt5 Test' mocked_is_linux.return_value = False - mocked_application_version.return_value = 'Trunk Test' + mocked_get_version.return_value = 'Trunk Test' test_form = exceptionform.ExceptionForm() test_form.file_attachment = None @@ -157,19 +137,10 @@ class TestExceptionForm(TestMixin, TestCase): @patch("openlp.core.ui.exceptionform.FileDialog.getSaveFileName") @patch("openlp.core.ui.exceptionform.Qt") - def test_on_save_report_button_clicked(self, - mocked_qt, - mocked_save_filename, - mocked_python_version, - mocked_platform, - mocked_is_linux, - mocked_etree, - mocked_bs4, - mocked_sqlalchemy, - mocked_application_version, - mocked_openlurl, - mocked_qversion, - ): + def test_on_save_report_button_clicked(self, mocked_qt, mocked_save_filename, mocked_python_version, + mocked_platform, mocked_is_linux, mocked_etree, mocked_bs4, + mocked_sqlalchemy, mocked_get_version, mocked_openlurl, + mocked_qversion): """ Test save report saves the correct information to a file """ @@ -181,7 +152,7 @@ class TestExceptionForm(TestMixin, TestCase): mocked_qversion.return_value = 'Qt5 test' mocked_qt.PYQT_VERSION_STR = 'PyQt5 Test' mocked_is_linux.return_value = False - mocked_application_version.return_value = 'Trunk Test' + mocked_get_version.return_value = 'Trunk Test' mocked_save_filename.return_value = (Path('testfile.txt'), 'filter') test_form = exceptionform.ExceptionForm() diff --git a/tests/functional/test_init.py b/tests/functional/test_init.py index 4cb6e5a76..3f30b253e 100644 --- a/tests/functional/test_init.py +++ b/tests/functional/test_init.py @@ -24,11 +24,11 @@ Package to test the openlp.core.__init__ package. """ import os from unittest import TestCase -from unittest.mock import MagicMock, patch, call +from unittest.mock import MagicMock, patch from PyQt5 import QtCore, QtWidgets -from openlp.core import OpenLP, parse_options +from openlp.core import OpenLP from openlp.core.common import Settings from tests.helpers.testmixin import TestMixin @@ -96,9 +96,9 @@ class TestInit(TestCase, TestMixin): 'build': 'bzr000' } Settings().setValue('core/application version', '2.2.0') - with patch('openlp.core.get_application_version') as mocked_get_application_version,\ + with patch('openlp.core.get_version') as mocked_get_version,\ patch('openlp.core.QtWidgets.QMessageBox.question') as mocked_question: - mocked_get_application_version.return_value = MOCKED_VERSION + mocked_get_version.return_value = MOCKED_VERSION mocked_question.return_value = QtWidgets.QMessageBox.No # WHEN: We check if a backup should be created @@ -122,9 +122,9 @@ class TestInit(TestCase, TestMixin): Settings().setValue('core/application version', '2.0.5') self.openlp.splash = MagicMock() self.openlp.splash.isVisible.return_value = True - with patch('openlp.core.get_application_version') as mocked_get_application_version,\ + with patch('openlp.core.get_version') as mocked_get_version, \ patch('openlp.core.QtWidgets.QMessageBox.question') as mocked_question: - mocked_get_application_version.return_value = MOCKED_VERSION + mocked_get_version.return_value = MOCKED_VERSION mocked_question.return_value = QtWidgets.QMessageBox.No # WHEN: We check if a backup should be created From 15c8023357adf9dd82a032298e5accfedf43b640 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Tue, 19 Sep 2017 09:48:34 -0700 Subject: [PATCH 05/15] Moving over to requests --- openlp/core/api/poll.py | 2 +- openlp/core/common/httputils.py | 177 +++++------------- openlp/core/ui/firsttimeform.py | 20 +- openlp/core/version.py | 1 - openlp/plugins/bibles/lib/importers/http.py | 28 +-- openlp/plugins/remotes/deploy.py | 4 +- .../openlp_core_api/test_websockets.py | 13 +- .../openlp_core_common/test_httputils.py | 30 +-- 8 files changed, 84 insertions(+), 191 deletions(-) diff --git a/openlp/core/api/poll.py b/openlp/core/api/poll.py index b8a29e1f2..c4b25848a 100644 --- a/openlp/core/api/poll.py +++ b/openlp/core/api/poll.py @@ -52,7 +52,7 @@ class Poller(RegistryProperties): 'isSecure': Settings().value('api/authentication enabled'), 'isAuthorised': False, 'chordNotation': Settings().value('songs/chord notation'), - 'isStagedActive': self.is_stage_active(), + 'isStageActive': self.is_stage_active(), 'isLiveActive': self.is_live_active(), 'isChordsActive': self.is_chords_active() } diff --git a/openlp/core/common/httputils.py b/openlp/core/common/httputils.py index b0c9c1b2f..0a5c962f8 100644 --- a/openlp/core/common/httputils.py +++ b/openlp/core/common/httputils.py @@ -25,17 +25,12 @@ The :mod:`openlp.core.utils` module provides the utility libraries for OpenLP. import hashlib import logging import os -import platform -import socket import sys -import subprocess import time -import urllib.error -import urllib.parse -import urllib.request -from http.client import HTTPException from random import randint +import requests + from openlp.core.common import Registry, trace_error_handler log = logging.getLogger(__name__ + '.__init__') @@ -69,33 +64,6 @@ CONNECTION_TIMEOUT = 30 CONNECTION_RETRIES = 2 -class HTTPRedirectHandlerFixed(urllib.request.HTTPRedirectHandler): - """ - Special HTTPRedirectHandler used to work around http://bugs.python.org/issue22248 - (Redirecting to urls with special chars) - """ - def redirect_request(self, req, fp, code, msg, headers, new_url): - # - """ - Test if the new_url can be decoded to ascii - - :param req: - :param fp: - :param code: - :param msg: - :param headers: - :param new_url: - :return: - """ - try: - new_url.encode('latin1').decode('ascii') - fixed_url = new_url - except Exception: - # The url could not be decoded to ascii, so we do some url encoding - fixed_url = urllib.parse.quote(new_url.encode('latin1').decode('utf-8', 'replace'), safe='/:') - return super(HTTPRedirectHandlerFixed, self).redirect_request(req, fp, code, msg, headers, fixed_url) - - def get_user_agent(): """ Return a user agent customised for the platform the user is on. @@ -107,7 +75,7 @@ def get_user_agent(): return browser_list[random_index] -def get_web_page(url, header=None, update_openlp=False): +def get_web_page(url, headers=None, update_openlp=False, proxies=None): """ Attempts to download the webpage at url and returns that page or None. @@ -116,71 +84,37 @@ def get_web_page(url, header=None, update_openlp=False): :param update_openlp: Tells OpenLP to update itself if the page is successfully downloaded. Defaults to False. """ - # TODO: Add proxy usage. Get proxy info from OpenLP settings, add to a - # proxy_handler, build into an opener and install the opener into urllib2. - # http://docs.python.org/library/urllib2.html if not url: return None - # This is needed to work around http://bugs.python.org/issue22248 and https://bugs.launchpad.net/openlp/+bug/1251437 - opener = urllib.request.build_opener(HTTPRedirectHandlerFixed()) - urllib.request.install_opener(opener) - req = urllib.request.Request(url) - if not header or header[0].lower() != 'user-agent': - user_agent = get_user_agent() - req.add_header('User-Agent', user_agent) - if header: - req.add_header(header[0], header[1]) + if headers and 'user-agent' not in [key.lower() for key in headers.keys()]: + headers['User-Agent'] = get_user_agent() log.debug('Downloading URL = %s' % url) retries = 0 - while retries <= CONNECTION_RETRIES: - retries += 1 - time.sleep(0.1) + while retries < CONNECTION_RETRIES: + # Put this at the bottom + # retries += 1 + # time.sleep(0.1) try: - page = urllib.request.urlopen(req, timeout=CONNECTION_TIMEOUT) - log.debug('Downloaded page {text}'.format(text=page.geturl())) + response = requests.get(url, headers=headers, proxies=proxies, timeout=float(CONNECTION_TIMEOUT)) + log.debug('Downloaded page {url}'.format(url=response.url)) break - except urllib.error.URLError as err: - log.exception('URLError on {text}'.format(text=url)) - log.exception('URLError: {text}'.format(text=err.reason)) - page = None + except IOError: + # For now, catch IOError. All requests errors inherit from IOError + log.exception('Unable to connect to {url}'.format(url=url)) + response = None if retries > CONNECTION_RETRIES: - raise - except socket.timeout: - log.exception('Socket timeout: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise - except socket.gaierror: - log.exception('Socket gaierror: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise - except ConnectionRefusedError: - log.exception('ConnectionRefused: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise - break - except ConnectionError: - log.exception('Connection error: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise - except HTTPException: - log.exception('HTTPException error: {text}'.format(text=url)) - page = None - if retries > CONNECTION_RETRIES: - raise + raise ConnectionError('Unable to connect to {url}, see log for details'.format(url=url)) except: # Don't know what's happening, so reraise the original + log.exception('Unknown error when trying to connect to {url}'.format(url=url)) raise if update_openlp: Registry().get('application').process_events() - if not page: - log.exception('{text} could not be downloaded'.format(text=url)) + if not response or not response.text: + log.error('{url} could not be downloaded'.format(url=url)) return None - log.debug(page) - return page + log.debug(response.text) + return response.text def get_url_file_size(url): @@ -192,19 +126,18 @@ def get_url_file_size(url): retries = 0 while True: try: - site = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT) - meta = site.info() - return int(meta.get("Content-Length")) - except urllib.error.URLError: + response = requests.head(url, timeout=float(CONNECTION_TIMEOUT), allow_redirects=True) + return int(response.headers['Content-Length']) + except IOError: if retries > CONNECTION_RETRIES: - raise + raise ConnectionError('Unable to download {url}'.format(url=url)) else: retries += 1 time.sleep(0.1) continue -def url_get_file(callback, url, f_path, sha256=None): +def url_get_file(callback, url, file_path, sha256=None): """" Download a file given a URL. The file is retrieved in chunks, giving the ability to cancel the download at any point. Returns False on download error. @@ -217,56 +150,42 @@ def url_get_file(callback, url, f_path, sha256=None): block_count = 0 block_size = 4096 retries = 0 - log.debug("url_get_file: " + url) - while True: + log.debug('url_get_file: %s', url) + while retries < CONNECTION_RETRIES: try: - filename = open(f_path, "wb") - url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT) - if sha256: - hasher = hashlib.sha256() - # Download until finished or canceled. - while not callback.was_cancelled: - data = url_file.read(block_size) - if not data: - break - filename.write(data) + with open(file_path, 'wb') as saved_file: + response = requests.get(url, timeout=float(CONNECTION_TIMEOUT), stream=True) if sha256: - hasher.update(data) - block_count += 1 - callback._download_progress(block_count, block_size) - filename.close() + hasher = hashlib.sha256() + # Download until finished or canceled. + for chunk in response.iter_content(chunk_size=block_size): + if callback.was_cancelled: + break + saved_file.write(chunk) + if sha256: + hasher.update(chunk) + block_count += 1 + callback._download_progress(block_count, block_size) + response.close() if sha256 and hasher.hexdigest() != sha256: - log.error('sha256 sums did not match for file: {file}'.format(file=f_path)) - os.remove(f_path) + log.error('sha256 sums did not match for file %s, got %s, expected %s', file_path, hasher.hexdigest(), + sha256) + os.remove(file_path) return False - except (urllib.error.URLError, socket.timeout) as err: + break + except IOError: trace_error_handler(log) - filename.close() - os.remove(f_path) + os.remove(file_path) if retries > CONNECTION_RETRIES: return False else: retries += 1 time.sleep(0.1) continue - break # Delete file if cancelled, it may be a partial file. if callback.was_cancelled: - os.remove(f_path) + os.remove(file_path) return True -def ping(host): - """ - Returns True if host responds to a ping request - """ - # Ping parameters as function of OS - ping_str = "-n 1" if platform.system().lower() == "windows" else "-c 1" - args = "ping " + " " + ping_str + " " + host - need_sh = False if platform.system().lower() == "windows" else True - - # Ping - return subprocess.call(args, shell=need_sh) == 0 - - __all__ = ['get_web_page'] diff --git a/openlp/core/ui/firsttimeform.py b/openlp/core/ui/firsttimeform.py index 1ae923467..a44384a61 100644 --- a/openlp/core/ui/firsttimeform.py +++ b/openlp/core/ui/firsttimeform.py @@ -181,22 +181,16 @@ class FirstTimeForm(QtWidgets.QWizard, UiFirstTimeWizard, RegistryProperties): self.application.process_events() try: web_config = get_web_page('{host}{name}'.format(host=self.web, name='download.cfg'), - header=('User-Agent', user_agent)) - except (urllib.error.URLError, ConnectionError) as err: - msg = QtWidgets.QMessageBox() - title = translate('OpenLP.FirstTimeWizard', 'Network Error') - msg.setText('{title} {error}'.format(title=title, - error=err.code if hasattr(err, 'code') else '')) - msg.setInformativeText(translate('OpenLP.FirstTimeWizard', - 'There was a network error attempting to ' - 'connect to retrieve initial configuration information')) - msg.setStandardButtons(msg.Ok) - ans = msg.exec() + headers={'User-Agent': user_agent}) + except ConnectionError: + QtWidgets.QMessageBox.critical(self, translate('OpenLP.FirstTimeWizard', 'Network Error'), + translate('OpenLP.FirstTimeWizard', 'There was a network error attempting ' + 'to connect to retrieve initial configuration information'), + QtWidgets.QMessageBox.Ok) web_config = False if web_config: - files = web_config.read() try: - self.config.read_string(files.decode()) + self.config.read_string(web_config) self.web = self.config.get('general', 'base url') self.songs_url = self.web + self.config.get('songs', 'directory') + '/' self.bibles_url = self.web + self.config.get('bibles', 'directory') + '/' diff --git a/openlp/core/version.py b/openlp/core/version.py index 9a532fcd8..14ddc40ff 100644 --- a/openlp/core/version.py +++ b/openlp/core/version.py @@ -139,7 +139,6 @@ def get_version(): global APPLICATION_VERSION if APPLICATION_VERSION: return APPLICATION_VERSION - print(sys.argv) if '--dev-version' in sys.argv or '-d' in sys.argv: # NOTE: The following code is a duplicate of the code in setup.py. Any fix applied here should also be applied # there. diff --git a/openlp/plugins/bibles/lib/importers/http.py b/openlp/plugins/bibles/lib/importers/http.py index 1616ebcf7..c6dc90667 100644 --- a/openlp/plugins/bibles/lib/importers/http.py +++ b/openlp/plugins/bibles/lib/importers/http.py @@ -93,7 +93,7 @@ class BGExtract(RegistryProperties): NAME = 'BibleGateway' def __init__(self, proxy_url=None): - log.debug('BGExtract.init("{url}")'.format(url=proxy_url)) + log.debug('BGExtract.init(proxy_url="{url}")'.format(url=proxy_url)) self.proxy_url = proxy_url socket.setdefaulttimeout(30) @@ -285,15 +285,15 @@ class BGExtract(RegistryProperties): log.debug('BGExtract.get_books_from_http("{version}")'.format(version=version)) url_params = urllib.parse.urlencode({'action': 'getVersionInfo', 'vid': '{version}'.format(version=version)}) reference_url = 'http://www.biblegateway.com/versions/?{url}#books'.format(url=url_params) - page = get_web_page(reference_url) - if not page: + page_source = get_web_page(reference_url) + if not page_source: send_error_message('download') return None - page_source = page.read() - try: - page_source = str(page_source, 'utf8') - except UnicodeDecodeError: - page_source = str(page_source, 'cp1251') + # TODO: Is this even necessary anymore? + # try: + # page_source = str(page_source, 'utf8') + # except UnicodeDecodeError: + # page_source = str(page_source, 'cp1251') try: soup = BeautifulSoup(page_source, 'lxml') except Exception: @@ -759,7 +759,7 @@ class HTTPBible(BibleImport, RegistryProperties): return BiblesResourcesDB.get_verse_count(book_id, chapter) -def get_soup_for_bible_ref(reference_url, header=None, pre_parse_regex=None, pre_parse_substitute=None): +def get_soup_for_bible_ref(reference_url, headers=None, pre_parse_regex=None, pre_parse_substitute=None): """ Gets a webpage and returns a parsed and optionally cleaned soup or None. @@ -772,15 +772,15 @@ def get_soup_for_bible_ref(reference_url, header=None, pre_parse_regex=None, pre if not reference_url: return None try: - page = get_web_page(reference_url, header, True) + page_source = get_web_page(reference_url, headers, update_openlp=True) except Exception as e: - page = None - if not page: + log.exception('Unable to download Bible %s, unknown exception occurred', reference_url) + page_source = None + if not page_source: send_error_message('download') return None - page_source = page.read() if pre_parse_regex and pre_parse_substitute is not None: - page_source = re.sub(pre_parse_regex, pre_parse_substitute, page_source.decode()) + page_source = re.sub(pre_parse_regex, pre_parse_substitute, page_source) soup = None try: soup = BeautifulSoup(page_source, 'lxml') diff --git a/openlp/plugins/remotes/deploy.py b/openlp/plugins/remotes/deploy.py index d971499f0..d159c9b16 100644 --- a/openlp/plugins/remotes/deploy.py +++ b/openlp/plugins/remotes/deploy.py @@ -49,10 +49,10 @@ def download_sha256(): user_agent = 'OpenLP/' + Registry().get('application').applicationVersion() try: web_config = get_web_page('{host}{name}'.format(host='https://get.openlp.org/webclient/', name='download.cfg'), - header=('User-Agent', user_agent)) + headers={'User-Agent': user_agent}) except (urllib.error.URLError, ConnectionError) as err: return False - file_bits = web_config.read().decode('utf-8').split() + file_bits = web_config.split() return file_bits[0], file_bits[2] diff --git a/tests/functional/openlp_core_api/test_websockets.py b/tests/functional/openlp_core_api/test_websockets.py index 8f1356566..c66b73c64 100644 --- a/tests/functional/openlp_core_api/test_websockets.py +++ b/tests/functional/openlp_core_api/test_websockets.py @@ -70,7 +70,7 @@ class TestWSServer(TestCase, TestMixin): """ # GIVEN: A new httpserver # WHEN: I start the server - server = WebSocketServer() + WebSocketServer() # THEN: the api environment should have been created self.assertEquals(1, mock_qthread.call_count, 'The qthread should have been called once') @@ -93,7 +93,7 @@ class TestWSServer(TestCase, TestMixin): """ Test the poll function returns the correct JSON """ - # WHEN: the system is configured with a set of data + # GIVEN: the system is configured with a set of data mocked_service_manager = MagicMock() mocked_service_manager.service_id = 21 mocked_live_controller = MagicMock() @@ -105,8 +105,15 @@ class TestWSServer(TestCase, TestMixin): mocked_live_controller.desktop_screen.isChecked.return_value = False Registry().register('live_controller', mocked_live_controller) Registry().register('service_manager', mocked_service_manager) + # WHEN: The poller polls + with patch.object(self.poll, 'is_stage_active') as mocked_is_stage_active, \ + patch.object(self.poll, 'is_live_active') as mocked_is_live_active, \ + patch.object(self.poll, 'is_chords_active') as mocked_is_chords_active: + mocked_is_stage_active.return_value = True + mocked_is_live_active.return_value = True + mocked_is_chords_active.return_value = True + poll_json = self.poll.poll() # THEN: the live json should be generated and match expected results - poll_json = self.poll.poll() self.assertTrue(poll_json['results']['blank'], 'The blank return value should be True') self.assertFalse(poll_json['results']['theme'], 'The theme return value should be False') self.assertFalse(poll_json['results']['display'], 'The display return value should be False') diff --git a/tests/functional/openlp_core_common/test_httputils.py b/tests/functional/openlp_core_common/test_httputils.py index 26ac297dd..9cb32db25 100644 --- a/tests/functional/openlp_core_common/test_httputils.py +++ b/tests/functional/openlp_core_common/test_httputils.py @@ -28,7 +28,7 @@ import socket from unittest import TestCase from unittest.mock import MagicMock, patch -from openlp.core.common.httputils import get_user_agent, get_web_page, get_url_file_size, url_get_file, ping +from openlp.core.common.httputils import get_user_agent, get_web_page, get_url_file_size, url_get_file from tests.helpers.testmixin import TestMixin @@ -253,7 +253,7 @@ class TestHttpUtils(TestCase, TestMixin): fake_url = 'this://is.a.fake/url' # WHEN: The get_url_file_size() method is called - size = get_url_file_size(fake_url) + get_url_file_size(fake_url) # THEN: The correct methods are called with the correct arguments and a web page is returned mock_urlopen.assert_called_with(fake_url, timeout=30) @@ -272,29 +272,3 @@ class TestHttpUtils(TestCase, TestMixin): # THEN: socket.timeout should have been caught # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files self.assertFalse(os.path.exists(self.tempfile), 'FTW url_get_file should have caught socket.timeout') - - def test_ping_valid(self): - """ - Test ping for OpenLP - """ - # GIVEN: a valid url to test - url = "openlp.io" - - # WHEN: Attempt to check the url exists - url_found = ping(url) - - # THEN: It should be found - self.assertTrue(url_found, 'OpenLP.io is not found') - - def test_ping_invalid(self): - """ - Test ping for OpenLP - """ - # GIVEN: a valid url to test - url = "trb143.io" - - # WHEN: Attempt to check the url exists - url_found = ping(url) - - # THEN: It should be found - self.assertFalse(url_found, 'TRB143.io is found') From 99b7239bc0a743c1194cf7b1a2443da276f26fa9 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Tue, 19 Sep 2017 21:36:59 -0700 Subject: [PATCH 06/15] A little cleanup --- openlp/plugins/bibles/lib/importers/http.py | 5 ----- openlp/plugins/remotes/deploy.py | 14 +++++++------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/openlp/plugins/bibles/lib/importers/http.py b/openlp/plugins/bibles/lib/importers/http.py index c6dc90667..ffc2ddfbf 100644 --- a/openlp/plugins/bibles/lib/importers/http.py +++ b/openlp/plugins/bibles/lib/importers/http.py @@ -289,11 +289,6 @@ class BGExtract(RegistryProperties): if not page_source: send_error_message('download') return None - # TODO: Is this even necessary anymore? - # try: - # page_source = str(page_source, 'utf8') - # except UnicodeDecodeError: - # page_source = str(page_source, 'cp1251') try: soup = BeautifulSoup(page_source, 'lxml') except Exception: diff --git a/openlp/plugins/remotes/deploy.py b/openlp/plugins/remotes/deploy.py index d159c9b16..a56792f62 100644 --- a/openlp/plugins/remotes/deploy.py +++ b/openlp/plugins/remotes/deploy.py @@ -19,10 +19,11 @@ # with this program; if not, write to the Free Software Foundation, Inc., 59 # # Temple Place, Suite 330, Boston, MA 02111-1307 USA # ############################################################################### - +""" +Download and "install" the remote web client +""" import os -import zipfile -import urllib.error +from zipfile import ZipFile from openlp.core.common import AppLocation, Registry from openlp.core.common.httputils import url_get_file, get_web_page, get_url_file_size @@ -38,7 +39,7 @@ def deploy_zipfile(app_root, zip_name): :return: None """ zip_file = os.path.join(app_root, zip_name) - web_zip = zipfile.ZipFile(zip_file) + web_zip = ZipFile(zip_file) web_zip.extractall(app_root) @@ -48,9 +49,8 @@ def download_sha256(): """ user_agent = 'OpenLP/' + Registry().get('application').applicationVersion() try: - web_config = get_web_page('{host}{name}'.format(host='https://get.openlp.org/webclient/', name='download.cfg'), - headers={'User-Agent': user_agent}) - except (urllib.error.URLError, ConnectionError) as err: + web_config = get_web_page('https://get.openlp.org/webclient/download.cfg', headers={'User-Agent': user_agent}) + except ConnectionError: return False file_bits = web_config.split() return file_bits[0], file_bits[2] From c06cd39cab35e68ae0bfa5a865acfc08cc996baf Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Wed, 20 Sep 2017 09:55:21 -0700 Subject: [PATCH 07/15] Fix up all the tests --- openlp/core/common/httputils.py | 10 +- .../openlp_core_common/test_httputils.py | 208 ++++++++---------- .../openlp_core_ui/test_first_time.py | 26 +-- .../openlp_core_ui/test_firsttimeform.py | 25 +-- 4 files changed, 118 insertions(+), 151 deletions(-) diff --git a/openlp/core/common/httputils.py b/openlp/core/common/httputils.py index 0a5c962f8..d02626389 100644 --- a/openlp/core/common/httputils.py +++ b/openlp/core/common/httputils.py @@ -86,14 +86,13 @@ def get_web_page(url, headers=None, update_openlp=False, proxies=None): """ if not url: return None - if headers and 'user-agent' not in [key.lower() for key in headers.keys()]: + if not headers: + headers = {} + if 'user-agent' not in [key.lower() for key in headers.keys()]: headers['User-Agent'] = get_user_agent() log.debug('Downloading URL = %s' % url) retries = 0 while retries < CONNECTION_RETRIES: - # Put this at the bottom - # retries += 1 - # time.sleep(0.1) try: response = requests.get(url, headers=headers, proxies=proxies, timeout=float(CONNECTION_TIMEOUT)) log.debug('Downloaded page {url}'.format(url=response.url)) @@ -102,8 +101,9 @@ def get_web_page(url, headers=None, update_openlp=False, proxies=None): # For now, catch IOError. All requests errors inherit from IOError log.exception('Unable to connect to {url}'.format(url=url)) response = None - if retries > CONNECTION_RETRIES: + if retries >= CONNECTION_RETRIES: raise ConnectionError('Unable to connect to {url}, see log for details'.format(url=url)) + retries += 1 except: # Don't know what's happening, so reraise the original log.exception('Unknown error when trying to connect to {url}'.format(url=url)) diff --git a/tests/functional/openlp_core_common/test_httputils.py b/tests/functional/openlp_core_common/test_httputils.py index 9cb32db25..a32f19172 100644 --- a/tests/functional/openlp_core_common/test_httputils.py +++ b/tests/functional/openlp_core_common/test_httputils.py @@ -24,7 +24,6 @@ Functional tests to test the AppLocation class and related methods. """ import os import tempfile -import socket from unittest import TestCase from unittest.mock import MagicMock, patch @@ -67,7 +66,7 @@ class TestHttpUtils(TestCase, TestMixin): """ with patch('openlp.core.common.httputils.sys') as mocked_sys: - # GIVEN: The system is Linux + # GIVEN: The system is Windows mocked_sys.platform = 'win32' # WHEN: We call get_user_agent() @@ -82,7 +81,7 @@ class TestHttpUtils(TestCase, TestMixin): """ with patch('openlp.core.common.httputils.sys') as mocked_sys: - # GIVEN: The system is Linux + # GIVEN: The system is macOS mocked_sys.platform = 'darwin' # WHEN: We call get_user_agent() @@ -97,7 +96,7 @@ class TestHttpUtils(TestCase, TestMixin): """ with patch('openlp.core.common.httputils.sys') as mocked_sys: - # GIVEN: The system is Linux + # GIVEN: The system is something else mocked_sys.platform = 'freebsd' # WHEN: We call get_user_agent() @@ -119,156 +118,127 @@ class TestHttpUtils(TestCase, TestMixin): # THEN: None should be returned self.assertIsNone(result, 'The return value of get_web_page should be None') - def test_get_web_page(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + @patch('openlp.core.common.httputils.Registry') + def test_get_web_page(self, MockRegistry, mocked_get_user_agent, mocked_requests): """ Test that the get_web_page method works correctly """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent, \ - patch('openlp.core.common.Registry') as MockRegistry: - # GIVEN: Mocked out objects and a fake URL - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - fake_url = 'this://is.a.fake/url' + # GIVEN: Mocked out objects and a fake URL + mocked_requests.get.return_value = MagicMock(text='text') + mocked_get_user_agent.return_value = 'user_agent' + fake_url = 'this://is.a.fake/url' - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with('User-Agent', 'user_agent') - self.assertEqual(1, mocked_request_object.add_header.call_count, - 'There should only be 1 call to add_header') - mock_get_user_agent.assert_called_with() - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - self.assertEqual(0, MockRegistry.call_count, 'The Registry() object should have never been called') - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.get.assert_called_once_with(fake_url, headers={'User-Agent': 'user_agent'}, + proxies=None, timeout=30.0) + mocked_get_user_agent.assert_called_once_with() + assert MockRegistry.call_count == 0, 'The Registry() object should have never been called' + assert returned_page == 'text', 'The returned page should be the mock object' - def test_get_web_page_with_header(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + def test_get_web_page_with_header(self, mocked_get_user_agent, mocked_requests): """ Test that adding a header to the call to get_web_page() adds the header to the request """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent: - # GIVEN: Mocked out objects, a fake URL and a fake header - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - fake_url = 'this://is.a.fake/url' - fake_header = ('Fake-Header', 'fake value') + # GIVEN: Mocked out objects, a fake URL and a fake header + mocked_requests.get.return_value = MagicMock(text='text') + mocked_get_user_agent.return_value = 'user_agent' + fake_url = 'this://is.a.fake/url' + fake_headers = {'Fake-Header': 'fake value'} - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url, header=fake_header) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url, headers=fake_headers) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with(fake_header[0], fake_header[1]) - self.assertEqual(2, mocked_request_object.add_header.call_count, - 'There should only be 2 calls to add_header') - mock_get_user_agent.assert_called_with() - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + expected_headers = dict(fake_headers) + expected_headers.update({'User-Agent': 'user_agent'}) + mocked_requests.get.assert_called_once_with(fake_url, headers=expected_headers, + proxies=None, timeout=30.0) + mocked_get_user_agent.assert_called_with() + assert returned_page == 'text', 'The returned page should be the mock object' - def test_get_web_page_with_user_agent_in_headers(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + def test_get_web_page_with_user_agent_in_headers(self, mocked_get_user_agent, mocked_requests): """ Test that adding a user agent in the header when calling get_web_page() adds that user agent to the request """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent: - # GIVEN: Mocked out objects, a fake URL and a fake header - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - fake_url = 'this://is.a.fake/url' - user_agent_header = ('User-Agent', 'OpenLP/2.2.0') + # GIVEN: Mocked out objects, a fake URL and a fake header + mocked_requests.get.return_value = MagicMock(text='text') + fake_url = 'this://is.a.fake/url' + user_agent_headers = {'User-Agent': 'OpenLP/2.2.0'} - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url, header=user_agent_header) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url, headers=user_agent_headers) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with(user_agent_header[0], user_agent_header[1]) - self.assertEqual(1, mocked_request_object.add_header.call_count, - 'There should only be 1 call to add_header') - self.assertEqual(0, mock_get_user_agent.call_count, 'get_user_agent should not have been called') - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.get.assert_called_once_with(fake_url, headers=user_agent_headers, + proxies=None, timeout=30.0) + assert mocked_get_user_agent.call_count == 0, 'get_user_agent() should not have been called' + assert returned_page == 'text', 'The returned page should be "test"' - def test_get_web_page_update_openlp(self): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.get_user_agent') + @patch('openlp.core.common.httputils.Registry') + def test_get_web_page_update_openlp(self, MockRegistry, mocked_get_user_agent, mocked_requests): """ Test that passing "update_openlp" as true to get_web_page calls Registry().get('app').process_events() """ - with patch('openlp.core.common.httputils.urllib.request.Request') as MockRequest, \ - patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent, \ - patch('openlp.core.common.httputils.Registry') as MockRegistry: - # GIVEN: Mocked out objects, a fake URL - mocked_request_object = MagicMock() - MockRequest.return_value = mocked_request_object - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - mocked_registry_object = MagicMock() - mocked_application_object = MagicMock() - mocked_registry_object.get.return_value = mocked_application_object - MockRegistry.return_value = mocked_registry_object - fake_url = 'this://is.a.fake/url' + # GIVEN: Mocked out objects, a fake URL + mocked_requests.get.return_value = MagicMock(text='text') + mocked_get_user_agent.return_value = 'user_agent' + mocked_registry_object = MagicMock() + mocked_application_object = MagicMock() + mocked_registry_object.get.return_value = mocked_application_object + MockRegistry.return_value = mocked_registry_object + fake_url = 'this://is.a.fake/url' - # WHEN: The get_web_page() method is called - returned_page = get_web_page(fake_url, update_openlp=True) + # WHEN: The get_web_page() method is called + returned_page = get_web_page(fake_url, update_openlp=True) - # THEN: The correct methods are called with the correct arguments and a web page is returned - MockRequest.assert_called_with(fake_url) - mocked_request_object.add_header.assert_called_with('User-Agent', 'user_agent') - self.assertEqual(1, mocked_request_object.add_header.call_count, - 'There should only be 1 call to add_header') - mock_urlopen.assert_called_with(mocked_request_object, timeout=30) - mocked_page_object.geturl.assert_called_with() - mocked_registry_object.get.assert_called_with('application') - mocked_application_object.process_events.assert_called_with() - self.assertEqual(mocked_page_object, returned_page, 'The returned page should be the mock object') + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.get.assert_called_once_with(fake_url, headers={'User-Agent': 'user_agent'}, + proxies=None, timeout=30.0) + mocked_get_user_agent.assert_called_once_with() + mocked_registry_object.get.assert_called_with('application') + mocked_application_object.process_events.assert_called_with() + assert returned_page == 'text', 'The returned page should be the mock object' - def test_get_url_file_size(self): + @patch('openlp.core.common.httputils.requests') + def test_get_url_file_size(self, mocked_requests): """ - Test that passing "update_openlp" as true to get_web_page calls Registry().get('app').process_events() + Test that calling "get_url_file_size" works correctly """ - with patch('openlp.core.common.httputils.urllib.request.urlopen') as mock_urlopen, \ - patch('openlp.core.common.httputils.get_user_agent') as mock_get_user_agent: - # GIVEN: Mocked out objects, a fake URL - mocked_page_object = MagicMock() - mock_urlopen.return_value = mocked_page_object - mock_get_user_agent.return_value = 'user_agent' - fake_url = 'this://is.a.fake/url' + # GIVEN: Mocked out objects, a fake URL + mocked_requests.head.return_value = MagicMock(headers={'Content-Length': 100}) + fake_url = 'this://is.a.fake/url' - # WHEN: The get_url_file_size() method is called - get_url_file_size(fake_url) + # WHEN: The get_url_file_size() method is called + file_size = get_url_file_size(fake_url) - # THEN: The correct methods are called with the correct arguments and a web page is returned - mock_urlopen.assert_called_with(fake_url, timeout=30) + # THEN: The correct methods are called with the correct arguments and a web page is returned + mocked_requests.head.assert_called_once_with(fake_url, allow_redirects=True, timeout=30.0) + assert file_size == 100 - @patch('openlp.core.ui.firsttimeform.urllib.request.urlopen') - def test_socket_timeout(self, mocked_urlopen): + @patch('openlp.core.common.httputils.requests') + @patch('openlp.core.common.httputils.os.remove') + def test_socket_timeout(self, mocked_remove, mocked_requests): """ Test socket timeout gets caught """ # GIVEN: Mocked urlopen to fake a network disconnect in the middle of a download - mocked_urlopen.side_effect = socket.timeout() + mocked_requests.get.side_effect = IOError # WHEN: Attempt to retrieve a file - url_get_file(MagicMock(), url='http://localhost/test', f_path=self.tempfile) + url_get_file(MagicMock(), url='http://localhost/test', file_path=self.tempfile) # THEN: socket.timeout should have been caught # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files - self.assertFalse(os.path.exists(self.tempfile), 'FTW url_get_file should have caught socket.timeout') + mocked_remove.assert_called_with(self.tempfile) + assert mocked_remove.call_count == 3, 'os.remove() should have been called 3 times' diff --git a/tests/functional/openlp_core_ui/test_first_time.py b/tests/functional/openlp_core_ui/test_first_time.py index 35507e15e..eb9464375 100644 --- a/tests/functional/openlp_core_ui/test_first_time.py +++ b/tests/functional/openlp_core_ui/test_first_time.py @@ -22,9 +22,6 @@ """ Package to test the openlp.core.utils.__init__ package. """ -import urllib.request -import urllib.error -import urllib.parse from unittest import TestCase from unittest.mock import patch @@ -37,20 +34,21 @@ class TestFirstTimeWizard(TestMixin, TestCase): """ Test First Time Wizard import functions """ - def test_webpage_connection_retry(self): + @patch('openlp.core.common.httputils.requests') + def test_webpage_connection_retry(self, mocked_requests): """ Test get_web_page will attempt CONNECTION_RETRIES+1 connections - bug 1409031 """ # GIVEN: Initial settings and mocks - with patch.object(urllib.request, 'urlopen') as mocked_urlopen: - mocked_urlopen.side_effect = ConnectionError + mocked_requests.get.side_effect = IOError('Unable to connect') - # WHEN: A webpage is requested - try: - get_web_page(url='http://localhost') - except: - pass + # WHEN: A webpage is requested + try: + get_web_page('http://localhost') + except Exception as e: + assert isinstance(e, ConnectionError) - # THEN: urlopen should have been called CONNECTION_RETRIES + 1 count - self.assertEquals(mocked_urlopen.call_count, CONNECTION_RETRIES + 1, - 'get_web_page() should have tried {} times'.format(CONNECTION_RETRIES)) + # THEN: urlopen should have been called CONNECTION_RETRIES + 1 count + assert mocked_requests.get.call_count == CONNECTION_RETRIES, \ + 'get should have been called {} times, but was only called {} times'.format( + CONNECTION_RETRIES, mocked_requests.get.call_count) diff --git a/tests/functional/openlp_core_ui/test_firsttimeform.py b/tests/functional/openlp_core_ui/test_firsttimeform.py index 0f7c6be6a..c90cdd80b 100644 --- a/tests/functional/openlp_core_ui/test_firsttimeform.py +++ b/tests/functional/openlp_core_ui/test_firsttimeform.py @@ -34,7 +34,7 @@ from openlp.core.ui.firsttimeform import FirstTimeForm from tests.helpers.testmixin import TestMixin -FAKE_CONFIG = b""" +FAKE_CONFIG = """ [general] base url = http://example.com/frw/ [songs] @@ -45,7 +45,7 @@ directory = bibles directory = themes """ -FAKE_BROKEN_CONFIG = b""" +FAKE_BROKEN_CONFIG = """ [general] base url = http://example.com/frw/ [songs] @@ -54,7 +54,7 @@ directory = songs directory = bibles """ -FAKE_INVALID_CONFIG = b""" +FAKE_INVALID_CONFIG = """ This is not a config file Some text @@ -112,7 +112,7 @@ class TestFirstTimeForm(TestCase, TestMixin): patch('openlp.core.ui.firsttimeform.Settings') as MockedSettings, \ patch('openlp.core.ui.firsttimeform.gettempdir') as mocked_gettempdir, \ patch('openlp.core.ui.firsttimeform.check_directory_exists') as mocked_check_directory_exists, \ - patch.object(frw.application, 'set_normal_cursor') as mocked_set_normal_cursor: + patch.object(frw.application, 'set_normal_cursor'): mocked_settings = MagicMock() mocked_settings.value.return_value = True MockedSettings.return_value = mocked_settings @@ -192,7 +192,7 @@ class TestFirstTimeForm(TestCase, TestMixin): with patch('openlp.core.ui.firsttimeform.get_web_page') as mocked_get_web_page: first_time_form = FirstTimeForm(None) first_time_form.initialize(MagicMock()) - mocked_get_web_page.return_value.read.return_value = FAKE_BROKEN_CONFIG + mocked_get_web_page.return_value = FAKE_BROKEN_CONFIG # WHEN: The First Time Wizard is downloads the config file first_time_form._download_index() @@ -208,7 +208,7 @@ class TestFirstTimeForm(TestCase, TestMixin): with patch('openlp.core.ui.firsttimeform.get_web_page') as mocked_get_web_page: first_time_form = FirstTimeForm(None) first_time_form.initialize(MagicMock()) - mocked_get_web_page.return_value.read.return_value = FAKE_INVALID_CONFIG + mocked_get_web_page.return_value = FAKE_INVALID_CONFIG # WHEN: The First Time Wizard is downloads the config file first_time_form._download_index() @@ -225,14 +225,13 @@ class TestFirstTimeForm(TestCase, TestMixin): # GIVEN: Initial setup and mocks first_time_form = FirstTimeForm(None) first_time_form.initialize(MagicMock()) - mocked_get_web_page.side_effect = urllib.error.HTTPError(url='http//localhost', - code=407, - msg='Network proxy error', - hdrs=None, - fp=None) + mocked_get_web_page.side_effect = ConnectionError('') + mocked_message_box.Ok = 'OK' + # WHEN: the First Time Wizard calls to get the initial configuration first_time_form._download_index() # THEN: the critical_error_message_box should have been called - self.assertEquals(mocked_message_box.mock_calls[1][1][0], 'Network Error 407', - 'first_time_form should have caught Network Error') + mocked_message_box.critical.assert_called_once_with( + first_time_form, 'Network Error', 'There was a network error attempting to connect to retrieve ' + 'initial configuration information', 'OK') From e1ca15173d13b0f82d5e940134e316bde7b3d852 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 22 Sep 2017 20:50:45 -0700 Subject: [PATCH 08/15] Actually show the progress dialog when we're waiting for the thread to shut down. --- openlp.py | 3 ++- openlp/core/ui/mainwindow.py | 8 +++++++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/openlp.py b/openlp.py index 68001f2d1..73c0d9033 100755 --- a/openlp.py +++ b/openlp.py @@ -28,9 +28,10 @@ import multiprocessing import sys from openlp.core.common import is_win, is_macosx +from openlp.core.common.applocation import AppLocation from openlp.core import main -faulthandler.enable() +faulthandler.enable(open(str(AppLocation.get_directory(AppLocation.CacheDir) / 'error.log'), 'wb')) if __name__ == '__main__': """ diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index ae32c0347..14b34a715 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -1017,7 +1017,13 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): wait_dialog = QtWidgets.QProgressDialog('Waiting for some things to finish...', '', 0, 0, self) wait_dialog.setWindowModality(QtCore.Qt.WindowModal) wait_dialog.setAutoClose(False) - self.version_thread.wait() + wait_dialog.show() + retry = 0 + while self.version_thread.isRunning() and retry < 10: + self.application.processEvents() + self.version_thread.wait(500) + if self.version_thread.isRunning(): + self.version_thread.terminate() wait_dialog.close() except RuntimeError: # Ignore the RuntimeError that is thrown when Qt has already deleted the C++ thread object From a4bd979f19a9c9e8d82d6b9f978f67c2aed35417 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 22 Sep 2017 21:10:18 -0700 Subject: [PATCH 09/15] Hide the cancel button --- openlp/core/ui/mainwindow.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/openlp/core/ui/mainwindow.py b/openlp/core/ui/mainwindow.py index 4424944cf..508b6a34c 100644 --- a/openlp/core/ui/mainwindow.py +++ b/openlp/core/ui/mainwindow.py @@ -1017,11 +1017,13 @@ class MainWindow(QtWidgets.QMainWindow, Ui_MainWindow, RegistryProperties): wait_dialog = QtWidgets.QProgressDialog('Waiting for some things to finish...', '', 0, 0, self) wait_dialog.setWindowModality(QtCore.Qt.WindowModal) wait_dialog.setAutoClose(False) + wait_dialog.setCancelButton(None) wait_dialog.show() retry = 0 - while self.version_thread.isRunning() and retry < 10: + while self.version_thread.isRunning() and retry < 50: self.application.processEvents() - self.version_thread.wait(500) + self.version_thread.wait(100) + retry += 1 if self.version_thread.isRunning(): self.version_thread.terminate() wait_dialog.close() From 7239b1e400e947f180c912b528ec0c21804516cc Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Fri, 22 Sep 2017 22:23:56 -0700 Subject: [PATCH 10/15] Add a mock main_window --- tests/interfaces/openlp_plugins/bibles/test_lib_http.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/interfaces/openlp_plugins/bibles/test_lib_http.py b/tests/interfaces/openlp_plugins/bibles/test_lib_http.py index e4075ca19..859957f8d 100644 --- a/tests/interfaces/openlp_plugins/bibles/test_lib_http.py +++ b/tests/interfaces/openlp_plugins/bibles/test_lib_http.py @@ -38,6 +38,7 @@ class TestBibleHTTP(TestCase): Registry.create() Registry().register('service_list', MagicMock()) Registry().register('application', MagicMock()) + Registry().register('main_window', MagicMock()) def test_bible_gateway_extract_books(self): """ From cff194d320c3c89827d00ff5f3f7edf7dedc7faa Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Mon, 25 Sep 2017 11:50:24 -0700 Subject: [PATCH 11/15] Skip Bible HTTP tests on Jenkins to prevent the server from being blacklisted --- tests/interfaces/openlp_plugins/bibles/test_lib_http.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/interfaces/openlp_plugins/bibles/test_lib_http.py b/tests/interfaces/openlp_plugins/bibles/test_lib_http.py index 859957f8d..b896cd9e1 100644 --- a/tests/interfaces/openlp_plugins/bibles/test_lib_http.py +++ b/tests/interfaces/openlp_plugins/bibles/test_lib_http.py @@ -22,13 +22,15 @@ """ Package to test the openlp.plugin.bible.lib.https package. """ -from unittest import TestCase, skip +import os +from unittest import TestCase, skipIf from unittest.mock import MagicMock from openlp.core.common import Registry from openlp.plugins.bibles.lib.importers.http import BGExtract, CWExtract, BSExtract +@skipIf(os.environ.get('JENKINS_URL'), 'Skip Bible HTTP tests to prevent Jenkins from being blacklisted') class TestBibleHTTP(TestCase): def setUp(self): From 92551f4fa5a8d52fda4bf427c7e8ba569ab1b5e2 Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Mon, 25 Sep 2017 12:01:54 -0700 Subject: [PATCH 12/15] Add requests to AppVeyor --- scripts/appveyor.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/appveyor.yml b/scripts/appveyor.yml index 0405d0e2c..316e5f73b 100644 --- a/scripts/appveyor.yml +++ b/scripts/appveyor.yml @@ -12,7 +12,7 @@ environment: install: # Install dependencies from pypi - - "%PYTHON%\\python.exe -m pip install sqlalchemy alembic chardet beautifulsoup4 Mako nose mock pyodbc==4.0.8 psycopg2 pypiwin32 pyenchant websockets asyncio waitress six webob" + - "%PYTHON%\\python.exe -m pip install sqlalchemy alembic chardet beautifulsoup4 Mako nose mock pyodbc==4.0.8 psycopg2 pypiwin32 pyenchant websockets asyncio waitress six webob requests" # Install mysql dependency - "%PYTHON%\\python.exe -m pip install http://cdn.mysql.com/Downloads/Connector-Python/mysql-connector-python-2.0.4.zip#md5=3df394d89300db95163f17c843ef49df" # Download and install lxml and pyicu (originally from http://www.lfd.uci.edu/~gohlke/pythonlibs/) From e90836e8179dd5ef5043d2a72cc87ccb90827b5b Mon Sep 17 00:00:00 2001 From: Raoul Snyman Date: Mon, 25 Sep 2017 13:34:05 -0700 Subject: [PATCH 13/15] Fix up some issues with one of the tests --- openlp/core/common/httputils.py | 14 ++++++++------ .../openlp_core_common/test_httputils.py | 8 +++----- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/openlp/core/common/httputils.py b/openlp/core/common/httputils.py index 9a73659bd..a92cd92c9 100644 --- a/openlp/core/common/httputils.py +++ b/openlp/core/common/httputils.py @@ -142,7 +142,7 @@ def url_get_file(callback, url, file_path, sha256=None): :param callback: the class which needs to be updated :param url: URL to download - :param f_path: Destination file + :param file_path: Destination file :param sha256: The check sum value to be checked against the download value """ block_count = 0 @@ -151,7 +151,7 @@ def url_get_file(callback, url, file_path, sha256=None): log.debug('url_get_file: %s', url) while retries < CONNECTION_RETRIES: try: - with open(file_path, 'wb') as saved_file: + with file_path.open('wb') as saved_file: response = requests.get(url, timeout=float(CONNECTION_TIMEOUT), stream=True) if sha256: hasher = hashlib.sha256() @@ -168,20 +168,22 @@ def url_get_file(callback, url, file_path, sha256=None): if sha256 and hasher.hexdigest() != sha256: log.error('sha256 sums did not match for file %s, got %s, expected %s', file_path, hasher.hexdigest(), sha256) - os.remove(file_path) + if file_path.exists(): + file_path.unlink() return False break except IOError: trace_error_handler(log) - os.remove(file_path) if retries > CONNECTION_RETRIES: + if file_path.exists(): + file_path.unlink() return False else: retries += 1 time.sleep(0.1) continue - if callback.was_cancelled: - os.remove(file_path) + if callback.was_cancelled and file_path.exists(): + file_path.unlink() return True diff --git a/tests/functional/openlp_core_common/test_httputils.py b/tests/functional/openlp_core_common/test_httputils.py index 26edf4056..e620fa04e 100644 --- a/tests/functional/openlp_core_common/test_httputils.py +++ b/tests/functional/openlp_core_common/test_httputils.py @@ -228,8 +228,7 @@ class TestHttpUtils(TestCase, TestMixin): assert file_size == 100 @patch('openlp.core.common.httputils.requests') - @patch('openlp.core.common.httputils.os.remove') - def test_socket_timeout(self, mocked_remove, mocked_requests): + def test_socket_timeout(self, mocked_requests): """ Test socket timeout gets caught """ @@ -237,9 +236,8 @@ class TestHttpUtils(TestCase, TestMixin): mocked_requests.get.side_effect = IOError # WHEN: Attempt to retrieve a file - url_get_file(MagicMock(), url='http://localhost/test', file_path=self.tempfile) + url_get_file(MagicMock(), url='http://localhost/test', file_path=Path(self.tempfile)) # THEN: socket.timeout should have been caught # NOTE: Test is if $tmpdir/tempfile is still there, then test fails since ftw deletes bad downloaded files - mocked_remove.assert_called_with(self.tempfile) - assert mocked_remove.call_count == 3, 'os.remove() should have been called 3 times' + assert not os.path.exists(self.tempfile), 'tempfile should have been deleted' From 10b13872e50f8417caffad7375413d14199337f0 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Tue, 26 Sep 2017 17:39:13 +0100 Subject: [PATCH 14/15] Modify themes to work with pathlib --- openlp/core/lib/json/theme.json | 2 +- openlp/core/lib/renderer.py | 7 +- openlp/core/lib/theme.py | 34 +- openlp/core/ui/maindisplay.py | 8 +- openlp/core/ui/themeform.py | 50 ++- openlp/core/ui/thememanager.py | 396 ++++++++---------- openlp/core/ui/themestab.py | 4 +- openlp/plugins/images/lib/upgrade.py | 1 - .../functional/openlp_core_lib/test_theme.py | 11 +- .../openlp_core_ui/test_exceptionform.py | 50 ++- .../openlp_core_ui/test_maindisplay.py | 18 +- .../openlp_core_ui/test_themeform.py | 2 +- .../openlp_core_ui/test_thememanager.py | 51 +-- .../openlp_core_ui/test_thememanager.py | 25 +- 14 files changed, 326 insertions(+), 333 deletions(-) diff --git a/openlp/core/lib/json/theme.json b/openlp/core/lib/json/theme.json index e8862d0b4..b23593c6b 100644 --- a/openlp/core/lib/json/theme.json +++ b/openlp/core/lib/json/theme.json @@ -4,7 +4,7 @@ "color": "#000000", "direction": "vertical", "end_color": "#000000", - "filename": "", + "filename": null, "start_color": "#000000", "type": "solid" }, diff --git a/openlp/core/lib/renderer.py b/openlp/core/lib/renderer.py index 67d33ce04..8b8d5669c 100644 --- a/openlp/core/lib/renderer.py +++ b/openlp/core/lib/renderer.py @@ -26,6 +26,7 @@ from string import Template from PyQt5 import QtGui, QtCore, QtWebKitWidgets from openlp.core.common import Registry, RegistryProperties, OpenLPMixin, RegistryMixin, Settings +from openlp.core.common.path import path_to_str from openlp.core.lib import FormattingTags, ImageSource, ItemCapabilities, ScreenList, ServiceItem, expand_tags, \ build_lyrics_format_css, build_lyrics_outline_css, build_chords_css from openlp.core.common import ThemeLevel @@ -118,7 +119,7 @@ class Renderer(OpenLPMixin, RegistryMixin, RegistryProperties): theme_data, main_rect, footer_rect = self._theme_dimensions[theme_name] # if No file do not update cache if theme_data.background_filename: - self.image_manager.add_image(theme_data.background_filename, + self.image_manager.add_image(path_to_str(theme_data.background_filename), ImageSource.Theme, QtGui.QColor(theme_data.background_border_color)) def pre_render(self, override_theme_data=None): @@ -207,8 +208,8 @@ class Renderer(OpenLPMixin, RegistryMixin, RegistryProperties): service_item.raw_footer = FOOTER # if No file do not update cache if theme_data.background_filename: - self.image_manager.add_image( - theme_data.background_filename, ImageSource.Theme, QtGui.QColor(theme_data.background_border_color)) + self.image_manager.add_image(path_to_str(theme_data.background_filename), + ImageSource.Theme, QtGui.QColor(theme_data.background_border_color)) theme_data, main, footer = self.pre_render(theme_data) service_item.theme_data = theme_data service_item.main = main diff --git a/openlp/core/lib/theme.py b/openlp/core/lib/theme.py index 4a55b1e7e..8522225d9 100644 --- a/openlp/core/lib/theme.py +++ b/openlp/core/lib/theme.py @@ -22,13 +22,13 @@ """ Provide the theme XML and handling functions for OpenLP v2 themes. """ -import os -import logging import json +import logging from lxml import etree, objectify from openlp.core.common import AppLocation, de_hump - +from openlp.core.common.json import OpenLPJsonDecoder, OpenLPJsonEncoder +from openlp.core.common.path import Path, str_to_path from openlp.core.lib import str_to_bool, ScreenList, get_text_file_string log = logging.getLogger(__name__) @@ -160,9 +160,8 @@ class Theme(object): # basic theme object with defaults json_path = AppLocation.get_directory(AppLocation.AppDir) / 'core' / 'lib' / 'json' / 'theme.json' jsn = get_text_file_string(json_path) - jsn = json.loads(jsn) - self.expand_json(jsn) - self.background_filename = '' + self.load_theme(jsn) + self.background_filename = None def expand_json(self, var, prev=None): """ @@ -174,8 +173,6 @@ class Theme(object): for key, value in var.items(): if prev: key = prev + "_" + key - else: - key = key if isinstance(value, dict): self.expand_json(value, key) else: @@ -185,13 +182,13 @@ class Theme(object): """ Add the path name to the image name so the background can be rendered. - :param path: The path name to be added. + :param openlp.core.common.path.Path path: The path name to be added. + :rtype: None """ if self.background_type == 'image' or self.background_type == 'video': if self.background_filename and path: self.theme_name = self.theme_name.strip() - self.background_filename = self.background_filename.strip() - self.background_filename = os.path.join(path, self.theme_name, self.background_filename) + self.background_filename = path / self.theme_name / self.background_filename def set_default_header_footer(self): """ @@ -206,16 +203,21 @@ class Theme(object): self.font_footer_y = current_screen['size'].height() * 9 / 10 self.font_footer_height = current_screen['size'].height() / 10 - def load_theme(self, theme): + def load_theme(self, theme, theme_path=None): """ Convert the JSON file and expand it. :param theme: the theme string + :param openlp.core.common.path.Path theme_path: The path to the theme + :rtype: None """ - jsn = json.loads(theme) + if theme_path: + jsn = json.loads(theme, cls=OpenLPJsonDecoder, base_path=theme_path) + else: + jsn = json.loads(theme, cls=OpenLPJsonDecoder) self.expand_json(jsn) - def export_theme(self): + def export_theme(self, theme_path=None): """ Loop through the fields and build a dictionary of them @@ -223,7 +225,9 @@ class Theme(object): theme_data = {} for attr, value in self.__dict__.items(): theme_data["{attr}".format(attr=attr)] = value - return json.dumps(theme_data) + if theme_path: + return json.dumps(theme_data, cls=OpenLPJsonEncoder, base_path=theme_path) + return json.dumps(theme_data, cls=OpenLPJsonEncoder) def parse(self, xml): """ diff --git a/openlp/core/ui/maindisplay.py b/openlp/core/ui/maindisplay.py index 862fcd0bc..8c8940369 100644 --- a/openlp/core/ui/maindisplay.py +++ b/openlp/core/ui/maindisplay.py @@ -346,7 +346,7 @@ class MainDisplay(OpenLPMixin, Display, RegistryProperties): if not hasattr(self, 'service_item'): return False self.override['image'] = path - self.override['theme'] = self.service_item.theme_data.background_filename + self.override['theme'] = path_to_str(self.service_item.theme_data.background_filename) self.image(path) # Update the preview frame. if self.is_live: @@ -454,7 +454,7 @@ class MainDisplay(OpenLPMixin, Display, RegistryProperties): Registry().execute('video_background_replaced') self.override = {} # We have a different theme. - elif self.override['theme'] != service_item.theme_data.background_filename: + elif self.override['theme'] != path_to_str(service_item.theme_data.background_filename): Registry().execute('live_theme_changed') self.override = {} else: @@ -466,7 +466,7 @@ class MainDisplay(OpenLPMixin, Display, RegistryProperties): if self.service_item.theme_data.background_type == 'image': if self.service_item.theme_data.background_filename: self.service_item.bg_image_bytes = self.image_manager.get_image_bytes( - self.service_item.theme_data.background_filename, ImageSource.Theme) + path_to_str(self.service_item.theme_data.background_filename), ImageSource.Theme) if image_path: image_bytes = self.image_manager.get_image_bytes(image_path, ImageSource.ImagePlugin) created_html = build_html(self.service_item, self.screen, self.is_live, background, image_bytes, @@ -488,7 +488,7 @@ class MainDisplay(OpenLPMixin, Display, RegistryProperties): path = os.path.join(str(AppLocation.get_section_data_path('themes')), self.service_item.theme_data.theme_name) service_item.add_from_command(path, - self.service_item.theme_data.background_filename, + path_to_str(self.service_item.theme_data.background_filename), ':/media/slidecontroller_multimedia.png') self.media_controller.video(DisplayControllerType.Live, service_item, video_behind_text=True) self._hide_mouse() diff --git a/openlp/core/ui/themeform.py b/openlp/core/ui/themeform.py index cac45f28d..d2ebaa275 100644 --- a/openlp/core/ui/themeform.py +++ b/openlp/core/ui/themeform.py @@ -28,7 +28,6 @@ import os from PyQt5 import QtCore, QtGui, QtWidgets from openlp.core.common import Registry, RegistryProperties, UiStrings, translate, get_images_filter, is_not_image_file -from openlp.core.common.path import Path, path_to_str, str_to_path from openlp.core.lib.theme import BackgroundType, BackgroundGradientType from openlp.core.lib.ui import critical_error_message_box from openlp.core.ui import ThemeLayoutForm @@ -61,7 +60,7 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties): self.setupUi(self) self.registerFields() self.update_theme_allowed = True - self.temp_background_filename = '' + self.temp_background_filename = None self.theme_layout_form = ThemeLayoutForm(self) self.background_combo_box.currentIndexChanged.connect(self.on_background_combo_box_current_index_changed) self.gradient_combo_box.currentIndexChanged.connect(self.on_gradient_combo_box_current_index_changed) @@ -188,8 +187,7 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties): """ background_image = BackgroundType.to_string(BackgroundType.Image) if self.page(self.currentId()) == self.background_page and \ - self.theme.background_type == background_image and \ - is_not_image_file(Path(self.theme.background_filename)): + self.theme.background_type == background_image and is_not_image_file(self.theme.background_filename): QtWidgets.QMessageBox.critical(self, translate('OpenLP.ThemeWizard', 'Background Image Empty'), translate('OpenLP.ThemeWizard', 'You have not selected a ' 'background image. Please select one before continuing.')) @@ -273,7 +271,7 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties): Run the wizard. """ log.debug('Editing theme {name}'.format(name=self.theme.theme_name)) - self.temp_background_filename = '' + self.temp_background_filename = None self.update_theme_allowed = False self.set_defaults() self.update_theme_allowed = True @@ -318,11 +316,11 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties): self.setField('background_type', 1) elif self.theme.background_type == BackgroundType.to_string(BackgroundType.Image): self.image_color_button.color = self.theme.background_border_color - self.image_path_edit.path = str_to_path(self.theme.background_filename) + self.image_path_edit.path = self.theme.background_filename self.setField('background_type', 2) elif self.theme.background_type == BackgroundType.to_string(BackgroundType.Video): self.video_color_button.color = self.theme.background_border_color - self.video_path_edit.path = str_to_path(self.theme.background_filename) + self.video_path_edit.path = self.theme.background_filename self.setField('background_type', 4) elif self.theme.background_type == BackgroundType.to_string(BackgroundType.Transparent): self.setField('background_type', 3) @@ -402,14 +400,14 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties): self.theme.background_type = BackgroundType.to_string(index) if self.theme.background_type != BackgroundType.to_string(BackgroundType.Image) and \ self.theme.background_type != BackgroundType.to_string(BackgroundType.Video) and \ - self.temp_background_filename == '': + self.temp_background_filename is None: self.temp_background_filename = self.theme.background_filename - self.theme.background_filename = '' + self.theme.background_filename = None if (self.theme.background_type == BackgroundType.to_string(BackgroundType.Image) or self.theme.background_type != BackgroundType.to_string(BackgroundType.Video)) and \ - self.temp_background_filename != '': + self.temp_background_filename is not None: self.theme.background_filename = self.temp_background_filename - self.temp_background_filename = '' + self.temp_background_filename = None self.set_background_page_values() def on_gradient_combo_box_current_index_changed(self, index): @@ -450,18 +448,24 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties): """ self.theme.background_end_color = color - def on_image_path_edit_path_changed(self, file_path): + def on_image_path_edit_path_changed(self, new_path): """ - Background Image button pushed. + Handle the `pathEditChanged` signal from image_path_edit + + :param openlp.core.common.path.Path new_path: Path to the new image + :rtype: None """ - self.theme.background_filename = path_to_str(file_path) + self.theme.background_filename = new_path self.set_background_page_values() - def on_video_path_edit_path_changed(self, file_path): + def on_video_path_edit_path_changed(self, new_path): """ - Background video button pushed. + Handle the `pathEditChanged` signal from video_path_edit + + :param openlp.core.common.path.Path new_path: Path to the new video + :rtype: None """ - self.theme.background_filename = path_to_str(file_path) + self.theme.background_filename = new_path self.set_background_page_values() def on_main_color_changed(self, color): @@ -537,14 +541,14 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties): translate('OpenLP.ThemeWizard', 'Theme Name Invalid'), translate('OpenLP.ThemeWizard', 'Invalid theme name. Please enter one.')) return - save_from = None - save_to = None + source_path = None + destination_path = None if self.theme.background_type == BackgroundType.to_string(BackgroundType.Image) or \ self.theme.background_type == BackgroundType.to_string(BackgroundType.Video): - filename = os.path.split(str(self.theme.background_filename))[1] - save_to = os.path.join(self.path, self.theme.theme_name, filename) - save_from = self.theme.background_filename + file_name = self.theme.background_filename.name + destination_path = self.path / self.theme.theme_name / file_name + source_path = self.theme.background_filename if not self.edit_mode and not self.theme_manager.check_if_theme_exists(self.theme.theme_name): return - self.theme_manager.save_theme(self.theme, save_from, save_to) + self.theme_manager.save_theme(self.theme, source_path, destination_path) return QtWidgets.QDialog.accept(self) diff --git a/openlp/core/ui/thememanager.py b/openlp/core/ui/thememanager.py index 15e33cdb2..7e860ffca 100644 --- a/openlp/core/ui/thememanager.py +++ b/openlp/core/ui/thememanager.py @@ -24,14 +24,14 @@ The Theme Manager manages adding, deleteing and modifying of themes. """ import os import zipfile -import shutil - from xml.etree.ElementTree import ElementTree, XML + from PyQt5 import QtCore, QtGui, QtWidgets from openlp.core.common import Registry, RegistryProperties, AppLocation, Settings, OpenLPMixin, RegistryMixin, \ - UiStrings, check_directory_exists, translate, is_win, get_filesystem_encoding, delete_file -from openlp.core.common.path import Path, path_to_str, str_to_path + UiStrings, check_directory_exists, translate, delete_file +from openlp.core.common.languagemanager import get_locale_key +from openlp.core.common.path import Path, copyfile, path_to_str, rmtree from openlp.core.lib import ImageSource, ValidationError, get_text_file_string, build_icon, \ check_item_selected, create_thumb, validate_thumb from openlp.core.lib.theme import Theme, BackgroundType @@ -39,7 +39,6 @@ from openlp.core.lib.ui import critical_error_message_box, create_widget_action from openlp.core.ui import FileRenameForm, ThemeForm from openlp.core.ui.lib import OpenLPToolbar from openlp.core.ui.lib.filedialog import FileDialog -from openlp.core.common.languagemanager import get_locale_key class Ui_ThemeManager(object): @@ -135,7 +134,7 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage self.settings_section = 'themes' # Variables self.theme_list = [] - self.old_background_image = None + self.old_background_image_path = None def bootstrap_initialise(self): """ @@ -145,25 +144,41 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage self.global_theme = Settings().value(self.settings_section + '/global theme') self.build_theme_path() self.load_first_time_themes() + self.upgrade_themes() def bootstrap_post_set_up(self): """ process the bootstrap post setup request """ self.theme_form = ThemeForm(self) - self.theme_form.path = self.path + self.theme_form.path = self.theme_path self.file_rename_form = FileRenameForm() Registry().register_function('theme_update_global', self.change_global_from_tab) self.load_themes() + def upgrade_themes(self): + """ + Upgrade the xml files to json. + + :rtype: None + """ + xml_file_paths = AppLocation.get_section_data_path('themes').glob('*/*.xml') + for xml_file_path in xml_file_paths: + theme_data = get_text_file_string(xml_file_path) + theme = self._create_theme_from_xml(theme_data, self.theme_path) + self._write_theme(theme) + xml_file_path.unlink() + def build_theme_path(self): """ Set up the theme path variables + + :rtype: None """ - self.path = str(AppLocation.get_section_data_path(self.settings_section)) - check_directory_exists(Path(self.path)) - self.thumb_path = os.path.join(self.path, 'thumbnails') - check_directory_exists(Path(self.thumb_path)) + self.theme_path = AppLocation.get_section_data_path(self.settings_section) + check_directory_exists(self.theme_path) + self.thumb_path = self.theme_path / 'thumbnails' + check_directory_exists(self.thumb_path) def check_list_state(self, item, field=None): """ @@ -298,17 +313,18 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage """ Takes a theme and makes a new copy of it as well as saving it. - :param theme_data: The theme to be used - :param new_theme_name: The new theme name to save the data to + :param Theme theme_data: The theme to be used + :param str new_theme_name: The new theme name of the theme + :rtype: None """ - save_to = None - save_from = None + destination_path = None + source_path = None if theme_data.background_type == 'image' or theme_data.background_type == 'video': - save_to = os.path.join(self.path, new_theme_name, os.path.split(str(theme_data.background_filename))[1]) - save_from = theme_data.background_filename + destination_path = self.theme_path / new_theme_name / theme_data.background_filename.name + source_path = theme_data.background_filename theme_data.theme_name = new_theme_name - theme_data.extend_image_filename(self.path) - self.save_theme(theme_data, save_from, save_to) + theme_data.extend_image_filename(self.theme_path) + self.save_theme(theme_data, source_path, destination_path) self.load_themes() def on_edit_theme(self, field=None): @@ -322,10 +338,10 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage item = self.theme_list_widget.currentItem() theme = self.get_theme_data(item.data(QtCore.Qt.UserRole)) if theme.background_type == 'image' or theme.background_type == 'video': - self.old_background_image = theme.background_filename + self.old_background_image_path = theme.background_filename self.theme_form.theme = theme self.theme_form.exec(True) - self.old_background_image = None + self.old_background_image_path = None self.renderer.update_theme(theme.theme_name) self.load_themes() @@ -355,77 +371,76 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage """ self.theme_list.remove(theme) thumb = '{name}.png'.format(name=theme) - delete_file(Path(self.path, thumb)) - delete_file(Path(self.thumb_path, thumb)) + delete_file(self.theme_path / thumb) + delete_file(self.thumb_path / thumb) try: - # Windows is always unicode, so no need to encode filenames - if is_win(): - shutil.rmtree(os.path.join(self.path, theme)) - else: - encoding = get_filesystem_encoding() - shutil.rmtree(os.path.join(self.path, theme).encode(encoding)) - except OSError as os_error: - shutil.Error = os_error + rmtree(self.theme_path / theme) + except OSError: self.log_exception('Error deleting theme {name}'.format(name=theme)) - def on_export_theme(self, field=None): + def on_export_theme(self, checked=None): """ - Export the theme in a zip file - :param field: + Export the theme to a zip file + + :param bool checked: Sent by the QAction.triggered signal. It's not used in this method. + :rtype: None """ item = self.theme_list_widget.currentItem() if item is None: critical_error_message_box(message=translate('OpenLP.ThemeManager', 'You have not selected a theme.')) return - theme = item.data(QtCore.Qt.UserRole) + theme_name = item.data(QtCore.Qt.UserRole) export_path, filter_used = \ FileDialog.getSaveFileName(self.main_window, - translate('OpenLP.ThemeManager', 'Save Theme - ({name})'). - format(name=theme), - Settings().value(self.settings_section + '/last directory export'), - translate('OpenLP.ThemeManager', 'OpenLP Themes (*.otz)')) + translate('OpenLP.ThemeManager', + 'Save Theme - ({name})').format(name=theme_name), + Settings().value(self.settings_section + '/last directory export'), + translate('OpenLP.ThemeManager', 'OpenLP Themes (*.otz)'), + translate('OpenLP.ThemeManager', 'OpenLP Themes (*.otz)')) self.application.set_busy_cursor() if export_path: Settings().setValue(self.settings_section + '/last directory export', export_path.parent) - if self._export_theme(str(export_path), theme): + if self._export_theme(export_path.with_suffix('.otz'), theme_name): QtWidgets.QMessageBox.information(self, translate('OpenLP.ThemeManager', 'Theme Exported'), translate('OpenLP.ThemeManager', 'Your theme has been successfully exported.')) self.application.set_normal_cursor() - def _export_theme(self, theme_path, theme): + def _export_theme(self, theme_path, theme_name): """ Create the zipfile with the theme contents. - :param theme_path: Location where the zip file will be placed - :param theme: The name of the theme to be exported + + :param openlp.core.common.path.Path theme_path: Location where the zip file will be placed + :param str theme_name: The name of the theme to be exported + :return: The success of creating the zip file + :rtype: bool """ - theme_zip = None try: - theme_zip = zipfile.ZipFile(theme_path, 'w') - source = os.path.join(self.path, theme) - for files in os.walk(source): - for name in files[2]: - theme_zip.write(os.path.join(source, name), os.path.join(theme, name)) - theme_zip.close() + with zipfile.ZipFile(str(theme_path), 'w') as theme_zip: + source_path = self.theme_path / theme_name + for file_path in source_path.iterdir(): + theme_zip.write(str(file_path), os.path.join(theme_name, file_path.name)) return True except OSError as ose: self.log_exception('Export Theme Failed') critical_error_message_box(translate('OpenLP.ThemeManager', 'Theme Export Failed'), - translate('OpenLP.ThemeManager', 'The theme export failed because this error ' - 'occurred: {err}').format(err=ose.strerror)) - if theme_zip: - theme_zip.close() - shutil.rmtree(theme_path, True) + translate('OpenLP.ThemeManager', + 'The theme_name export failed because this error occurred: {err}') + .format(err=ose.strerror)) + if theme_path.exists(): + rmtree(theme_path, True) return False - def on_import_theme(self, field=None): + def on_import_theme(self, checked=None): """ Opens a file dialog to select the theme file(s) to import before attempting to extract OpenLP themes from those files. This process will only load version 2 themes. - :param field: + + :param bool checked: Sent by the QAction.triggered signal. It's not used in this method. + :rtype: None """ - file_paths, selected_filter = FileDialog.getOpenFileNames( + file_paths, filter_used = FileDialog.getOpenFileNames( self, translate('OpenLP.ThemeManager', 'Select Theme Import File'), Settings().value(self.settings_section + '/last directory import'), @@ -435,8 +450,8 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage return self.application.set_busy_cursor() for file_path in file_paths: - self.unzip_theme(path_to_str(file_path), self.path) - Settings().setValue(self.settings_section + '/last directory import', file_path) + self.unzip_theme(file_path, self.theme_path) + Settings().setValue(self.settings_section + '/last directory import', file_path.parent) self.load_themes() self.application.set_normal_cursor() @@ -445,17 +460,17 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage Imports any themes on start up and makes sure there is at least one theme """ self.application.set_busy_cursor() - files = AppLocation.get_files(self.settings_section, '.otz') - for theme_file in files: - theme_file = os.path.join(self.path, str(theme_file)) - self.unzip_theme(theme_file, self.path) - delete_file(Path(theme_file)) - files = AppLocation.get_files(self.settings_section, '.png') + theme_paths = AppLocation.get_files(self.settings_section, '.otz') + for theme_path in theme_paths: + theme_path = self.theme_path / theme_path + self.unzip_theme(theme_path, self.theme_path) + delete_file(theme_path) + theme_paths = AppLocation.get_files(self.settings_section, '.png') # No themes have been found so create one - if not files: + if not theme_paths: theme = Theme() theme.theme_name = UiStrings().Default - self._write_theme(theme, None, None) + self._write_theme(theme) Settings().setValue(self.settings_section + '/global theme', theme.theme_name) self.application.set_normal_cursor() @@ -471,22 +486,21 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage # Sort the themes by its name considering language specific files.sort(key=lambda file_name: get_locale_key(str(file_name))) # now process the file list of png files - for name in files: - name = str(name) + for file in files: # check to see file is in theme root directory - theme = os.path.join(self.path, name) - if os.path.exists(theme): - text_name = os.path.splitext(name)[0] + theme_path = self.theme_path / file + if theme_path.exists(): + text_name = theme_path.stem if text_name == self.global_theme: name = translate('OpenLP.ThemeManager', '{name} (default)').format(name=text_name) else: name = text_name - thumb = os.path.join(self.thumb_path, '{name}.png'.format(name=text_name)) + thumb = self.thumb_path / '{name}.png'.format(name=text_name) item_name = QtWidgets.QListWidgetItem(name) - if validate_thumb(Path(theme), Path(thumb)): + if validate_thumb(theme_path, thumb): icon = build_icon(thumb) else: - icon = create_thumb(theme, thumb) + icon = create_thumb(str(theme_path), str(thumb)) item_name.setIcon(icon) item_name.setData(QtCore.Qt.UserRole, text_name) self.theme_list_widget.addItem(item_name) @@ -507,27 +521,19 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage def get_theme_data(self, theme_name): """ - Returns a theme object from an XML or JSON file + Returns a theme object from a JSON file - :param theme_name: Name of the theme to load from file - :return: The theme object. + :param str theme_name: Name of the theme to load from file + :return: The theme object. + :rtype: Theme """ - self.log_debug('get theme data for theme {name}'.format(name=theme_name)) - theme_file_path = Path(self.path, str(theme_name), '{file_name}.json'.format(file_name=theme_name)) + theme_name = str(theme_name) + theme_file_path = self.theme_path / theme_name / '{file_name}.json'.format(file_name=theme_name) theme_data = get_text_file_string(theme_file_path) - jsn = True - if not theme_data: - theme_file_path = theme_file_path.with_suffix('.xml') - theme_data = get_text_file_string(theme_file_path) - jsn = False if not theme_data: self.log_debug('No theme data - using default theme') return Theme() - else: - if jsn: - return self._create_theme_from_json(theme_data, self.path) - else: - return self._create_theme_from_xml(theme_data, self.path) + return self._create_theme_from_json(theme_data, self.theme_path) def over_write_message_box(self, theme_name): """ @@ -543,172 +549,149 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage defaultButton=QtWidgets.QMessageBox.No) return ret == QtWidgets.QMessageBox.Yes - def unzip_theme(self, file_name, directory): + def unzip_theme(self, file_path, directory_path): """ Unzip the theme, remove the preview file if stored. Generate a new preview file. Check the XML theme version and upgrade if necessary. - :param file_name: - :param directory: + :param openlp.core.common.path.Path file_path: + :param openlp.core.common.path.Path directory_path: """ - self.log_debug('Unzipping theme {name}'.format(name=file_name)) - theme_zip = None - out_file = None + self.log_debug('Unzipping theme {name}'.format(name=file_path)) file_xml = None abort_import = True json_theme = False theme_name = "" try: - theme_zip = zipfile.ZipFile(file_name) - json_file = [name for name in theme_zip.namelist() if os.path.splitext(name)[1].lower() == '.json'] - if len(json_file) != 1: - # TODO: remove XML handling at some point but would need a auto conversion to run first. - xml_file = [name for name in theme_zip.namelist() if os.path.splitext(name)[1].lower() == '.xml'] - if len(xml_file) != 1: - self.log_error('Theme contains "{val:d}" theme files'.format(val=len(xml_file))) - raise ValidationError - xml_tree = ElementTree(element=XML(theme_zip.read(xml_file[0]))).getroot() - theme_version = xml_tree.get('version', default=None) - if not theme_version or float(theme_version) < 2.0: - self.log_error('Theme version is less than 2.0') - raise ValidationError - theme_name = xml_tree.find('name').text.strip() - else: - new_theme = Theme() - new_theme.load_theme(theme_zip.read(json_file[0]).decode("utf-8")) - theme_name = new_theme.theme_name - json_theme = True - theme_folder = os.path.join(directory, theme_name) - theme_exists = os.path.exists(theme_folder) - if theme_exists and not self.over_write_message_box(theme_name): - abort_import = True - return - else: - abort_import = False - for name in theme_zip.namelist(): - out_name = name.replace('/', os.path.sep) - split_name = out_name.split(os.path.sep) - if split_name[-1] == '' or len(split_name) == 1: - # is directory or preview file - continue - full_name = os.path.join(directory, out_name) - check_directory_exists(Path(os.path.dirname(full_name))) - if os.path.splitext(name)[1].lower() == '.xml' or os.path.splitext(name)[1].lower() == '.json': - file_xml = str(theme_zip.read(name), 'utf-8') - out_file = open(full_name, 'w', encoding='utf-8') - out_file.write(file_xml) + with zipfile.ZipFile(str(file_path)) as theme_zip: + json_file = [name for name in theme_zip.namelist() if os.path.splitext(name)[1].lower() == '.json'] + if len(json_file) != 1: + # TODO: remove XML handling after the 2.6 release. + xml_file = [name for name in theme_zip.namelist() if os.path.splitext(name)[1].lower() == '.xml'] + if len(xml_file) != 1: + self.log_error('Theme contains "{val:d}" theme files'.format(val=len(xml_file))) + raise ValidationError + xml_tree = ElementTree(element=XML(theme_zip.read(xml_file[0]))).getroot() + theme_version = xml_tree.get('version', default=None) + if not theme_version or float(theme_version) < 2.0: + self.log_error('Theme version is less than 2.0') + raise ValidationError + theme_name = xml_tree.find('name').text.strip() else: - out_file = open(full_name, 'wb') - out_file.write(theme_zip.read(name)) - out_file.close() + new_theme = Theme() + new_theme.load_theme(theme_zip.read(json_file[0]).decode("utf-8")) + theme_name = new_theme.theme_name + json_theme = True + theme_folder = directory_path / theme_name + if theme_folder.exists() and not self.over_write_message_box(theme_name): + abort_import = True + return + else: + abort_import = False + for zipped_file in theme_zip.namelist(): + zipped_file_rel_path = Path(zipped_file) + split_name = zipped_file_rel_path.parts + if split_name[-1] == '' or len(split_name) == 1: + # is directory or preview file + continue + full_name = directory_path / zipped_file_rel_path + check_directory_exists(full_name.parent) + if zipped_file_rel_path.suffix.lower() == '.xml' or zipped_file_rel_path.suffix.lower() == '.json': + file_xml = str(theme_zip.read(zipped_file), 'utf-8') + with full_name.open('w', encoding='utf-8') as out_file: + out_file.write(file_xml) + else: + with full_name.open('wb') as out_file: + out_file.write(theme_zip.read(zipped_file)) except (IOError, zipfile.BadZipfile): - self.log_exception('Importing theme from zip failed {name}'.format(name=file_name)) + self.log_exception('Importing theme from zip failed {name}'.format(name=file_path)) raise ValidationError except ValidationError: critical_error_message_box(translate('OpenLP.ThemeManager', 'Validation Error'), translate('OpenLP.ThemeManager', 'File is not a valid theme.')) finally: - # Close the files, to be able to continue creating the theme. - if theme_zip: - theme_zip.close() - if out_file: - out_file.close() if not abort_import: # As all files are closed, we can create the Theme. if file_xml: if json_theme: - theme = self._create_theme_from_json(file_xml, self.path) + theme = self._create_theme_from_json(file_xml, self.theme_path) else: - theme = self._create_theme_from_xml(file_xml, self.path) + theme = self._create_theme_from_xml(file_xml, self.theme_path) self.generate_and_save_image(theme_name, theme) - # Only show the error message, when IOError was not raised (in - # this case the error message has already been shown). - elif theme_zip is not None: - critical_error_message_box( - translate('OpenLP.ThemeManager', 'Validation Error'), - translate('OpenLP.ThemeManager', 'File is not a valid theme.')) - self.log_error('Theme file does not contain XML data {name}'.format(name=file_name)) def check_if_theme_exists(self, theme_name): """ Check if theme already exists and displays error message - :param theme_name: Name of the Theme to test + :param str theme_name: Name of the Theme to test :return: True or False if theme exists + :rtype: bool """ - theme_dir = os.path.join(self.path, theme_name) - if os.path.exists(theme_dir): + if (self.theme_path / theme_name).exists(): critical_error_message_box( translate('OpenLP.ThemeManager', 'Validation Error'), translate('OpenLP.ThemeManager', 'A theme with this name already exists.')) return False return True - def save_theme(self, theme, image_from, image_to): + def save_theme(self, theme, image_source_path, image_destination_path): """ Called by theme maintenance Dialog to save the theme and to trigger the reload of the theme list - :param theme: The theme data object. - :param image_from: Where the theme image is currently located. - :param image_to: Where the Theme Image is to be saved to + + :param Theme theme: The theme data object. + :param openlp.core.common.path.Path image_source_path: Where the theme image is currently located. + :param openlp.core.common.path.Path image_destination_path: Where the Theme Image is to be saved to + :rtype: None """ - self._write_theme(theme, image_from, image_to) + self._write_theme(theme, image_source_path, image_destination_path) if theme.background_type == BackgroundType.to_string(BackgroundType.Image): - self.image_manager.update_image_border(theme.background_filename, + self.image_manager.update_image_border(path_to_str(theme.background_filename), ImageSource.Theme, QtGui.QColor(theme.background_border_color)) self.image_manager.process_updates() - def _write_theme(self, theme, image_from, image_to): + def _write_theme(self, theme, image_source_path=None, image_destination_path=None): """ Writes the theme to the disk and handles the background image if necessary - :param theme: The theme data object. - :param image_from: Where the theme image is currently located. - :param image_to: Where the Theme Image is to be saved to + :param Theme theme: The theme data object. + :param openlp.core.common.path.Path image_source_path: Where the theme image is currently located. + :param openlp.core.common.path.Path image_destination_path: Where the Theme Image is to be saved to + :rtype: None """ name = theme.theme_name - theme_pretty = theme.export_theme() - theme_dir = os.path.join(self.path, name) - check_directory_exists(Path(theme_dir)) - theme_file = os.path.join(theme_dir, name + '.json') - if self.old_background_image and image_to != self.old_background_image: - delete_file(Path(self.old_background_image)) - out_file = None + theme_pretty = theme.export_theme(self.theme_path) + theme_dir = self.theme_path / name + check_directory_exists(theme_dir) + theme_path = theme_dir / '{file_name}.json'.format(file_name=name) try: - out_file = open(theme_file, 'w', encoding='utf-8') - out_file.write(theme_pretty) + theme_path.write_text(theme_pretty) except IOError: self.log_exception('Saving theme to file failed') - finally: - if out_file: - out_file.close() - if image_from and os.path.abspath(image_from) != os.path.abspath(image_to): - try: - # Windows is always unicode, so no need to encode filenames - if is_win(): - shutil.copyfile(image_from, image_to) - else: - encoding = get_filesystem_encoding() - shutil.copyfile(image_from.encode(encoding), image_to.encode(encoding)) - except IOError as xxx_todo_changeme: - shutil.Error = xxx_todo_changeme - self.log_exception('Failed to save theme image') + if image_source_path and image_destination_path: + if self.old_background_image_path and image_destination_path != self.old_background_image_path: + delete_file(self.old_background_image_path) + if image_source_path != image_destination_path: + try: + copyfile(image_source_path, image_destination_path) + except IOError: + self.log_exception('Failed to save theme image') self.generate_and_save_image(name, theme) - def generate_and_save_image(self, name, theme): + def generate_and_save_image(self, theme_name, theme): """ Generate and save a preview image - :param name: The name of the theme. + :param str theme_name: The name of the theme. :param theme: The theme data object. """ frame = self.generate_image(theme) - sample_path_name = os.path.join(self.path, name + '.png') - if os.path.exists(sample_path_name): - os.unlink(sample_path_name) - frame.save(sample_path_name, 'png') - thumb = os.path.join(self.thumb_path, '{name}.png'.format(name=name)) - create_thumb(sample_path_name, thumb, False) + sample_path_name = self.theme_path / '{file_name}.png'.format(file_name=theme_name) + if sample_path_name.exists(): + sample_path_name.unlink() + frame.save(str(sample_path_name), 'png') + thumb_path = self.thumb_path / '{name}.png'.format(name=theme_name) + create_thumb(str(sample_path_name), str(thumb_path), False) def update_preview_images(self): """ @@ -730,39 +713,32 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage """ return self.renderer.generate_preview(theme_data, force_page) - def get_preview_image(self, theme): - """ - Return an image representing the look of the theme - - :param theme: The theme to return the image for. - """ - return os.path.join(self.path, theme + '.png') - @staticmethod def _create_theme_from_xml(theme_xml, image_path): """ Return a theme object using information parsed from XML :param theme_xml: The Theme data object. - :param image_path: Where the theme image is stored + :param openlp.core.common.path.Path image_path: Where the theme image is stored :return: Theme data. + :rtype: Theme """ theme = Theme() theme.parse(theme_xml) theme.extend_image_filename(image_path) return theme - @staticmethod - def _create_theme_from_json(theme_json, image_path): + def _create_theme_from_json(self, theme_json, image_path): """ Return a theme object using information parsed from JSON :param theme_json: The Theme data object. - :param image_path: Where the theme image is stored + :param openlp.core.common.path.Path image_path: Where the theme image is stored :return: Theme data. + :rtype: Theme """ theme = Theme() - theme.load_theme(theme_json) + theme.load_theme(theme_json, self.theme_path) theme.extend_image_filename(image_path) return theme diff --git a/openlp/core/ui/themestab.py b/openlp/core/ui/themestab.py index f3b5bbb71..bf4be809c 100644 --- a/openlp/core/ui/themestab.py +++ b/openlp/core/ui/themestab.py @@ -211,8 +211,8 @@ class ThemesTab(SettingsTab): """ Utility method to update the global theme preview image. """ - image = self.theme_manager.get_preview_image(self.global_theme) - preview = QtGui.QPixmap(str(image)) + image_path = self.theme_manager.theme_path / '{file_name}.png'.format(file_name=self.global_theme) + preview = QtGui.QPixmap(str(image_path)) if not preview.isNull(): preview = preview.scaled(300, 255, QtCore.Qt.KeepAspectRatio, QtCore.Qt.SmoothTransformation) self.default_list_view.setPixmap(preview) diff --git a/openlp/plugins/images/lib/upgrade.py b/openlp/plugins/images/lib/upgrade.py index 63690d404..d467e9d3c 100644 --- a/openlp/plugins/images/lib/upgrade.py +++ b/openlp/plugins/images/lib/upgrade.py @@ -48,7 +48,6 @@ def upgrade_2(session, metadata): """ Version 2 upgrade - Move file path from old db to JSON encoded path to new db. Added during 2.5 dev """ - # TODO: Update tests log.debug('Starting upgrade_2 for file_path to JSON') old_table = Table('image_filenames', metadata, autoload=True) if 'file_path' not in [col.name for col in old_table.c.values()]: diff --git a/tests/functional/openlp_core_lib/test_theme.py b/tests/functional/openlp_core_lib/test_theme.py index bb90e574a..93bc06f24 100644 --- a/tests/functional/openlp_core_lib/test_theme.py +++ b/tests/functional/openlp_core_lib/test_theme.py @@ -22,8 +22,9 @@ """ Package to test the openlp.core.lib.theme package. """ -from unittest import TestCase import os +from pathlib import Path +from unittest import TestCase from openlp.core.lib.theme import Theme @@ -79,16 +80,16 @@ class TestTheme(TestCase): """ # GIVEN: A theme object theme = Theme() - theme.theme_name = 'MyBeautifulTheme ' - theme.background_filename = ' video.mp4' + theme.theme_name = 'MyBeautifulTheme' + theme.background_filename = Path('video.mp4') theme.background_type = 'video' - path = os.path.expanduser('~') + path = Path.home() # WHEN: Theme.extend_image_filename is run theme.extend_image_filename(path) # THEN: The filename of the background should be correct - expected_filename = os.path.join(path, 'MyBeautifulTheme', 'video.mp4') + expected_filename = path / 'MyBeautifulTheme' / 'video.mp4' self.assertEqual(expected_filename, theme.background_filename) self.assertEqual('MyBeautifulTheme', theme.theme_name) diff --git a/tests/functional/openlp_core_ui/test_exceptionform.py b/tests/functional/openlp_core_ui/test_exceptionform.py index 40eb19ac8..1b5c5fb59 100644 --- a/tests/functional/openlp_core_ui/test_exceptionform.py +++ b/tests/functional/openlp_core_ui/test_exceptionform.py @@ -22,11 +22,10 @@ """ Package to test the openlp.core.ui.exeptionform package. """ - import os import tempfile from unittest import TestCase -from unittest.mock import mock_open, patch +from unittest.mock import call, patch from openlp.core.common import Registry from openlp.core.common.path import Path @@ -142,15 +141,15 @@ class TestExceptionForm(TestMixin, TestCase): test_form = exceptionform.ExceptionForm() test_form.file_attachment = None - with patch.object(test_form, '_pyuno_import') as mock_pyuno: - with patch.object(test_form.exception_text_edit, 'toPlainText') as mock_traceback: - with patch.object(test_form.description_text_edit, 'toPlainText') as mock_description: - mock_pyuno.return_value = 'UNO Bridge Test' - mock_traceback.return_value = 'openlp: Traceback Test' - mock_description.return_value = 'Description Test' + with patch.object(test_form, '_pyuno_import') as mock_pyuno, \ + patch.object(test_form.exception_text_edit, 'toPlainText') as mock_traceback, \ + patch.object(test_form.description_text_edit, 'toPlainText') as mock_description: + mock_pyuno.return_value = 'UNO Bridge Test' + mock_traceback.return_value = 'openlp: Traceback Test' + mock_description.return_value = 'Description Test' - # WHEN: on_save_report_button_clicked called - test_form.on_send_report_button_clicked() + # WHEN: on_save_report_button_clicked called + test_form.on_send_report_button_clicked() # THEN: Verify strings were formatted properly mocked_add_query_item.assert_called_with('body', MAIL_ITEM_TEXT) @@ -182,25 +181,24 @@ class TestExceptionForm(TestMixin, TestCase): mocked_qt.PYQT_VERSION_STR = 'PyQt5 Test' mocked_is_linux.return_value = False mocked_application_version.return_value = 'Trunk Test' - mocked_save_filename.return_value = (Path('testfile.txt'), 'filter') - test_form = exceptionform.ExceptionForm() - test_form.file_attachment = None + with patch.object(Path, 'open') as mocked_path_open: + x = Path('testfile.txt') + mocked_save_filename.return_value = x, 'ext' - with patch.object(test_form, '_pyuno_import') as mock_pyuno: - with patch.object(test_form.exception_text_edit, 'toPlainText') as mock_traceback: - with patch.object(test_form.description_text_edit, 'toPlainText') as mock_description: - with patch("openlp.core.ui.exceptionform.open", mock_open(), create=True) as mocked_open: - mock_pyuno.return_value = 'UNO Bridge Test' - mock_traceback.return_value = 'openlp: Traceback Test' - mock_description.return_value = 'Description Test' + test_form = exceptionform.ExceptionForm() + test_form.file_attachment = None - # WHEN: on_save_report_button_clicked called - test_form.on_save_report_button_clicked() + with patch.object(test_form, '_pyuno_import') as mock_pyuno, \ + patch.object(test_form.exception_text_edit, 'toPlainText') as mock_traceback, \ + patch.object(test_form.description_text_edit, 'toPlainText') as mock_description: + mock_pyuno.return_value = 'UNO Bridge Test' + mock_traceback.return_value = 'openlp: Traceback Test' + mock_description.return_value = 'Description Test' + + # WHEN: on_save_report_button_clicked called + test_form.on_save_report_button_clicked() # THEN: Verify proper calls to save file # self.maxDiff = None - check_text = "call().write({text})".format(text=MAIL_ITEM_TEXT.__repr__()) - write_text = "{text}".format(text=mocked_open.mock_calls[1]) - mocked_open.assert_called_with('testfile.txt', 'w') - self.assertEquals(check_text, write_text, "Saved information should match test text") + mocked_path_open.assert_has_calls([call().__enter__().write(MAIL_ITEM_TEXT)]) diff --git a/tests/functional/openlp_core_ui/test_maindisplay.py b/tests/functional/openlp_core_ui/test_maindisplay.py index 8da2dbd55..c3f798982 100644 --- a/tests/functional/openlp_core_ui/test_maindisplay.py +++ b/tests/functional/openlp_core_ui/test_maindisplay.py @@ -27,10 +27,10 @@ from unittest.mock import MagicMock, patch from PyQt5 import QtCore -from openlp.core.common import Registry, is_macosx, Settings +from openlp.core.common import Registry, is_macosx +from openlp.core.common.path import Path from openlp.core.lib import ScreenList, PluginManager from openlp.core.ui import MainDisplay, AudioPlayer -from openlp.core.ui.media import MediaController from openlp.core.ui.maindisplay import TRANSPARENT_STYLESHEET, OPAQUE_STYLESHEET from tests.helpers.testmixin import TestMixin @@ -184,7 +184,7 @@ class TestMainDisplay(TestCase, TestMixin): self.assertEqual(pyobjc_nsview.window().collectionBehavior(), NSWindowCollectionBehaviorManaged, 'Window collection behavior should be NSWindowCollectionBehaviorManaged') - @patch(u'openlp.core.ui.maindisplay.Settings') + @patch('openlp.core.ui.maindisplay.Settings') def test_show_display_startup_logo(self, MockedSettings): # GIVEN: Mocked show_display, setting for logo visibility display = MagicMock() @@ -204,7 +204,7 @@ class TestMainDisplay(TestCase, TestMixin): # THEN: setVisible should had been called with "True" main_display.setVisible.assert_called_once_with(True) - @patch(u'openlp.core.ui.maindisplay.Settings') + @patch('openlp.core.ui.maindisplay.Settings') def test_show_display_hide_startup_logo(self, MockedSettings): # GIVEN: Mocked show_display, setting for logo visibility display = MagicMock() @@ -224,8 +224,8 @@ class TestMainDisplay(TestCase, TestMixin): # THEN: setVisible should had not been called main_display.setVisible.assert_not_called() - @patch(u'openlp.core.ui.maindisplay.Settings') - @patch(u'openlp.core.ui.maindisplay.build_html') + @patch('openlp.core.ui.maindisplay.Settings') + @patch('openlp.core.ui.maindisplay.build_html') def test_build_html_no_video(self, MockedSettings, Mocked_build_html): # GIVEN: Mocked display display = MagicMock() @@ -252,8 +252,8 @@ class TestMainDisplay(TestCase, TestMixin): self.assertEquals(main_display.media_controller.video.call_count, 0, 'Media Controller video should not have been called') - @patch(u'openlp.core.ui.maindisplay.Settings') - @patch(u'openlp.core.ui.maindisplay.build_html') + @patch('openlp.core.ui.maindisplay.Settings') + @patch('openlp.core.ui.maindisplay.build_html') def test_build_html_video(self, MockedSettings, Mocked_build_html): # GIVEN: Mocked display display = MagicMock() @@ -270,7 +270,7 @@ class TestMainDisplay(TestCase, TestMixin): service_item.theme_data = MagicMock() service_item.theme_data.background_type = 'video' service_item.theme_data.theme_name = 'name' - service_item.theme_data.background_filename = 'background_filename' + service_item.theme_data.background_filename = Path('background_filename') mocked_plugin = MagicMock() display.plugin_manager = PluginManager() display.plugin_manager.plugins = [mocked_plugin] diff --git a/tests/functional/openlp_core_ui/test_themeform.py b/tests/functional/openlp_core_ui/test_themeform.py index cff097893..e487742cc 100644 --- a/tests/functional/openlp_core_ui/test_themeform.py +++ b/tests/functional/openlp_core_ui/test_themeform.py @@ -49,5 +49,5 @@ class TestThemeManager(TestCase): self.instance.on_image_path_edit_path_changed(Path('/', 'new', 'pat.h')) # THEN: The theme background file should be set and `set_background_page_values` should have been called - self.assertEqual(self.instance.theme.background_filename, '/new/pat.h') + self.assertEqual(self.instance.theme.background_filename, Path('/', 'new', 'pat.h')) mocked_set_background_page_values.assert_called_once_with() diff --git a/tests/functional/openlp_core_ui/test_thememanager.py b/tests/functional/openlp_core_ui/test_thememanager.py index d778fb8ef..e4b044b29 100644 --- a/tests/functional/openlp_core_ui/test_thememanager.py +++ b/tests/functional/openlp_core_ui/test_thememanager.py @@ -30,8 +30,9 @@ from unittest.mock import ANY, MagicMock, patch from PyQt5 import QtWidgets -from openlp.core.ui import ThemeManager from openlp.core.common import Registry +from openlp.core.common.path import Path +from openlp.core.ui import ThemeManager from tests.utils.constants import TEST_RESOURCES_PATH @@ -57,13 +58,13 @@ class TestThemeManager(TestCase): """ # GIVEN: A new ThemeManager instance. theme_manager = ThemeManager() - theme_manager.path = os.path.join(TEST_RESOURCES_PATH, 'themes') + theme_manager.theme_path = Path(TEST_RESOURCES_PATH, 'themes') with patch('zipfile.ZipFile.__init__') as mocked_zipfile_init, \ patch('zipfile.ZipFile.write') as mocked_zipfile_write: mocked_zipfile_init.return_value = None # WHEN: The theme is exported - theme_manager._export_theme(os.path.join('some', 'path', 'Default.otz'), 'Default') + theme_manager._export_theme(Path('some', 'path', 'Default.otz'), 'Default') # THEN: The zipfile should be created at the given path mocked_zipfile_init.assert_called_with(os.path.join('some', 'path', 'Default.otz'), 'w') @@ -86,57 +87,49 @@ class TestThemeManager(TestCase): """ Test that we don't try to overwrite a theme background image with itself """ - # GIVEN: A new theme manager instance, with mocked builtins.open, shutil.copyfile, + # GIVEN: A new theme manager instance, with mocked builtins.open, copyfile, # theme, check_directory_exists and thememanager-attributes. - with patch('builtins.open') as mocked_open, \ - patch('openlp.core.ui.thememanager.shutil.copyfile') as mocked_copyfile, \ + with patch('openlp.core.ui.thememanager.copyfile') as mocked_copyfile, \ patch('openlp.core.ui.thememanager.check_directory_exists'): - mocked_open.return_value = MagicMock() theme_manager = ThemeManager(None) theme_manager.old_background_image = None theme_manager.generate_and_save_image = MagicMock() - theme_manager.path = '' + theme_manager.theme_path = MagicMock() mocked_theme = MagicMock() mocked_theme.theme_name = 'themename' mocked_theme.extract_formatted_xml = MagicMock() mocked_theme.extract_formatted_xml.return_value = 'fake_theme_xml'.encode() # WHEN: Calling _write_theme with path to the same image, but the path written slightly different - file_name1 = os.path.join(TEST_RESOURCES_PATH, 'church.jpg') - # Do replacement from end of string to avoid problems with path start - file_name2 = file_name1[::-1].replace(os.sep, os.sep + os.sep, 2)[::-1] - theme_manager._write_theme(mocked_theme, file_name1, file_name2) + file_name1 = Path(TEST_RESOURCES_PATH, 'church.jpg') + theme_manager._write_theme(mocked_theme, file_name1, file_name1) # THEN: The mocked_copyfile should not have been called - self.assertFalse(mocked_copyfile.called, 'shutil.copyfile should not be called') + self.assertFalse(mocked_copyfile.called, 'copyfile should not be called') def test_write_theme_diff_images(self): """ Test that we do overwrite a theme background image when a new is submitted """ - # GIVEN: A new theme manager instance, with mocked builtins.open, shutil.copyfile, + # GIVEN: A new theme manager instance, with mocked builtins.open, copyfile, # theme, check_directory_exists and thememanager-attributes. - with patch('builtins.open') as mocked_open, \ - patch('openlp.core.ui.thememanager.shutil.copyfile') as mocked_copyfile, \ + with patch('openlp.core.ui.thememanager.copyfile') as mocked_copyfile, \ patch('openlp.core.ui.thememanager.check_directory_exists'): - mocked_open.return_value = MagicMock() theme_manager = ThemeManager(None) theme_manager.old_background_image = None theme_manager.generate_and_save_image = MagicMock() - theme_manager.path = '' + theme_manager.theme_path = MagicMock() mocked_theme = MagicMock() mocked_theme.theme_name = 'themename' mocked_theme.filename = "filename" - # mocked_theme.extract_formatted_xml = MagicMock() - # mocked_theme.extract_formatted_xml.return_value = 'fake_theme_xml'.encode() # WHEN: Calling _write_theme with path to different images - file_name1 = os.path.join(TEST_RESOURCES_PATH, 'church.jpg') - file_name2 = os.path.join(TEST_RESOURCES_PATH, 'church2.jpg') + file_name1 = Path(TEST_RESOURCES_PATH, 'church.jpg') + file_name2 = Path(TEST_RESOURCES_PATH, 'church2.jpg') theme_manager._write_theme(mocked_theme, file_name1, file_name2) # THEN: The mocked_copyfile should not have been called - self.assertTrue(mocked_copyfile.called, 'shutil.copyfile should be called') + self.assertTrue(mocked_copyfile.called, 'copyfile should be called') def test_write_theme_special_char_name(self): """ @@ -146,7 +139,7 @@ class TestThemeManager(TestCase): theme_manager = ThemeManager(None) theme_manager.old_background_image = None theme_manager.generate_and_save_image = MagicMock() - theme_manager.path = self.temp_folder + theme_manager.theme_path = Path(self.temp_folder) mocked_theme = MagicMock() mocked_theme.theme_name = 'theme 愛 name' mocked_theme.export_theme.return_value = "{}" @@ -208,17 +201,17 @@ class TestThemeManager(TestCase): theme_manager = ThemeManager(None) theme_manager._create_theme_from_xml = MagicMock() theme_manager.generate_and_save_image = MagicMock() - theme_manager.path = '' - folder = mkdtemp() - theme_file = os.path.join(TEST_RESOURCES_PATH, 'themes', 'Moss_on_tree.otz') + theme_manager.theme_path = None + folder = Path(mkdtemp()) + theme_file = Path(TEST_RESOURCES_PATH, 'themes', 'Moss_on_tree.otz') # WHEN: We try to unzip it theme_manager.unzip_theme(theme_file, folder) # THEN: Files should be unpacked - self.assertTrue(os.path.exists(os.path.join(folder, 'Moss on tree', 'Moss on tree.xml'))) + self.assertTrue((folder / 'Moss on tree' / 'Moss on tree.xml').exists()) self.assertEqual(mocked_critical_error_message_box.call_count, 0, 'No errors should have happened') - shutil.rmtree(folder) + shutil.rmtree(str(folder)) def test_unzip_theme_invalid_version(self): """ diff --git a/tests/interfaces/openlp_core_ui/test_thememanager.py b/tests/interfaces/openlp_core_ui/test_thememanager.py index b560df154..0797aa9b8 100644 --- a/tests/interfaces/openlp_core_ui/test_thememanager.py +++ b/tests/interfaces/openlp_core_ui/test_thememanager.py @@ -26,7 +26,8 @@ from unittest import TestCase from unittest.mock import patch, MagicMock from openlp.core.common import Registry, Settings -from openlp.core.ui import ThemeManager, ThemeForm, FileRenameForm +from openlp.core.common.path import Path +from openlp.core.ui import ThemeManager from tests.helpers.testmixin import TestMixin @@ -91,6 +92,23 @@ class TestThemeManager(TestCase, TestMixin): assert self.theme_manager.thumb_path.startswith(self.theme_manager.path) is True, \ 'The thumb path and the main path should start with the same value' + def test_build_theme_path(self): + """ + Test the thememanager build_theme_path - basic test + """ + # GIVEN: A new a call to initialise + with patch('openlp.core.common.AppLocation.get_section_data_path', return_value=Path('test/path')): + Settings().setValue('themes/global theme', 'my_theme') + + self.theme_manager.theme_form = MagicMock() + self.theme_manager.load_first_time_themes = MagicMock() + + # WHEN: the build_theme_path is run + self.theme_manager.build_theme_path() + + # THEN: The thumbnail path should be a sub path of the test path + self.assertEqual(self.theme_manager.thumb_path, Path('test/path/thumbnails')) + def test_click_on_new_theme(self): """ Test the on_add_theme event handler is called by the UI @@ -109,17 +127,16 @@ class TestThemeManager(TestCase, TestMixin): @patch('openlp.core.ui.themeform.ThemeForm._setup') @patch('openlp.core.ui.filerenameform.FileRenameForm._setup') - def test_bootstrap_post(self, mocked_theme_form, mocked_rename_form): + def test_bootstrap_post(self, mocked_rename_form, mocked_theme_form): """ Test the functions of bootstrap_post_setup are called. """ # GIVEN: self.theme_manager.load_themes = MagicMock() - self.theme_manager.path = MagicMock() + self.theme_manager.theme_path = MagicMock() # WHEN: self.theme_manager.bootstrap_post_set_up() # THEN: - self.assertEqual(self.theme_manager.path, self.theme_manager.theme_form.path) self.assertEqual(1, self.theme_manager.load_themes.call_count, "load_themes should have been called once") From dfcd95b9d91bc45104b9261d4cc41e295b47e491 Mon Sep 17 00:00:00 2001 From: Philip Ridout Date: Tue, 26 Sep 2017 18:02:56 +0100 Subject: [PATCH 15/15] pep fixes --- openlp/core/ui/themeform.py | 4 ++-- openlp/core/ui/thememanager.py | 11 +++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/openlp/core/ui/themeform.py b/openlp/core/ui/themeform.py index d2ebaa275..37978d10a 100644 --- a/openlp/core/ui/themeform.py +++ b/openlp/core/ui/themeform.py @@ -451,7 +451,7 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties): def on_image_path_edit_path_changed(self, new_path): """ Handle the `pathEditChanged` signal from image_path_edit - + :param openlp.core.common.path.Path new_path: Path to the new image :rtype: None """ @@ -461,7 +461,7 @@ class ThemeForm(QtWidgets.QWizard, Ui_ThemeWizard, RegistryProperties): def on_video_path_edit_path_changed(self, new_path): """ Handle the `pathEditChanged` signal from video_path_edit - + :param openlp.core.common.path.Path new_path: Path to the new video :rtype: None """ diff --git a/openlp/core/ui/thememanager.py b/openlp/core/ui/thememanager.py index 7e860ffca..dffa82d47 100644 --- a/openlp/core/ui/thememanager.py +++ b/openlp/core/ui/thememanager.py @@ -392,11 +392,11 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage theme_name = item.data(QtCore.Qt.UserRole) export_path, filter_used = \ FileDialog.getSaveFileName(self.main_window, - translate('OpenLP.ThemeManager', - 'Save Theme - ({name})').format(name=theme_name), - Settings().value(self.settings_section + '/last directory export'), - translate('OpenLP.ThemeManager', 'OpenLP Themes (*.otz)'), - translate('OpenLP.ThemeManager', 'OpenLP Themes (*.otz)')) + translate('OpenLP.ThemeManager', + 'Save Theme - ({name})').format(name=theme_name), + Settings().value(self.settings_section + '/last directory export'), + translate('OpenLP.ThemeManager', 'OpenLP Themes (*.otz)'), + translate('OpenLP.ThemeManager', 'OpenLP Themes (*.otz)')) self.application.set_busy_cursor() if export_path: Settings().setValue(self.settings_section + '/last directory export', export_path.parent) @@ -637,7 +637,6 @@ class ThemeManager(OpenLPMixin, RegistryMixin, QtWidgets.QWidget, Ui_ThemeManage """ Called by theme maintenance Dialog to save the theme and to trigger the reload of the theme list - :param Theme theme: The theme data object. :param openlp.core.common.path.Path image_source_path: Where the theme image is currently located. :param openlp.core.common.path.Path image_destination_path: Where the Theme Image is to be saved to