openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #25999
[Merge] lp:~phill-ridout/openlp/bug1073931 into lp:openlp
Phill has proposed merging lp:~phill-ridout/openlp/bug1073931 into lp:openlp.
Requested reviews:
OpenLP Core (openlp-core)
For more details, see:
https://code.launchpad.net/~phill-ridout/openlp/bug1073931/+merge/248803
Fixes bug1073931 "Corrupted databases stop OpenLP from starting"
Checks if the database session is available before trying to use it.
Use a sha256 hash to verify downloaded files. See also: https://code.launchpad.net/~phill-ridout/openlp/sha256
Tests failed on the crosswalk tests. I have included my locally run test output.
Add this to your merge proposal:
--------------------------------
lp:~phill-ridout/openlp/bug1073931 (revision 2495)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/934/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/860/
[FAILURE] http://ci.openlp.org/job/Branch-03-Interface-Tests/805/
Stopping after failure
Process finished with exit code 0
E
Error
Traceback (most recent call last):
File "/usr/lib/python3.4/unittest/case.py", line 58, in testPartExecutor
yield
File "/usr/lib/python3.4/unittest/case.py", line 577, in run
testMethod()
File "/home/phill/Projects/openlp/bug1073931/tests/interfaces/openlp_plugins/bibles/test_lib_http.py", line 102, in crosswalk_extract_books_test
books = handler.get_books_from_http('niv')
File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 413, in get_books_from_http
content = content.find('ul', {'class': 'parent'})
AttributeError: 'NoneType' object has no attribute 'find'
-------------------- >> begin captured logging << --------------------
openlp.core.common.registry: INFO: Registry Initialising
openlp.plugins.bibles.lib.http: DEBUG: CWExtract.init("None")
openlp.plugins.bibles.lib.http: DEBUG: CWExtract.get_books_from_http("niv")
openlp.core.utils.__init__: DEBUG: Downloading URL = http://www.biblestudytools.com/niv/
openlp.core.utils.__init__: DEBUG: Downloaded URL = http://www.biblestudytools.com/niv/
openlp.core.utils.__init__: DEBUG: <http.client.HTTPResponse object at 0x7f66e37052b0>
--------------------- >> end captured logging << ---------------------
E
Error
Traceback (most recent call last):
File "/usr/lib/python3.4/unittest/case.py", line 58, in testPartExecutor
yield
File "/usr/lib/python3.4/unittest/case.py", line 577, in run
testMethod()
File "/home/phill/Projects/openlp/bug1073931/tests/interfaces/openlp_plugins/bibles/test_lib_http.py", line 115, in crosswalk_extract_verse_test
results = handler.get_bible_chapter('niv', 'john', 3)
File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 371, in get_bible_chapter
send_error_message('parse')
File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 649, in send_error_message
translate('BiblesPlugin.HTTPBible', 'There was a problem extracting your verse selection. If this error '
File "/home/phill/Projects/openlp/bug1073931/openlp/core/lib/ui.py", line 114, in critical_error_message_box
return Registry().get('main_window').error_message(title if title else UiStrings().Error, message)
AttributeError: 'NoneType' object has no attribute 'error_message'
-------------------- >> begin captured logging << --------------------
openlp.core.common.registry: INFO: Registry Initialising
openlp.plugins.bibles.lib.http: DEBUG: CWExtract.init("None")
openlp.plugins.bibles.lib.http: DEBUG: CWExtract.get_bible_chapter("niv", "john", "3")
openlp.core.utils.__init__: DEBUG: Downloading URL = http://www.biblestudytools.com/niv/john/3.html
openlp.core.utils.__init__: DEBUG: Downloaded URL = http://www.biblestudytools.com/niv/john/3.html
openlp.core.utils.__init__: DEBUG: <http.client.HTTPResponse object at 0x7f66e33ecd68>
openlp.plugins.bibles.lib.http: ERROR: No verses found in the CrossWalk response.
--------------------- >> end captured logging << ---------------------
======================================================================
ERROR: Test Crosswalk retrieval of book list for NIV bible
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/phill/Projects/openlp/bug1073931/tests/interfaces/openlp_plugins/bibles/test_lib_http.py", line 102, in crosswalk_extract_books_test
books = handler.get_books_from_http('niv')
File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 413, in get_books_from_http
content = content.find('ul', {'class': 'parent'})
nose.proxy.AttributeError: 'NoneType' object has no attribute 'find'
-------------------- >> begin captured logging << --------------------
openlp.core.common.registry: INFO: Registry Initialising
openlp.plugins.bibles.lib.http: DEBUG: CWExtract.init("None")
openlp.plugins.bibles.lib.http: DEBUG: CWExtract.get_books_from_http("niv")
openlp.core.utils.__init__: DEBUG: Downloading URL = http://www.biblestudytools.com/niv/
openlp.core.utils.__init__: DEBUG: Downloaded URL = http://www.biblestudytools.com/niv/
openlp.core.utils.__init__: DEBUG: <http.client.HTTPResponse object at 0x7f66e37052b0>
--------------------- >> end captured logging << ---------------------
======================================================================
ERROR: Test Crosswalk retrieval of verse list for NIV bible John 3
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/phill/Projects/openlp/bug1073931/tests/interfaces/openlp_plugins/bibles/test_lib_http.py", line 115, in crosswalk_extract_verse_test
results = handler.get_bible_chapter('niv', 'john', 3)
File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 371, in get_bible_chapter
send_error_message('parse')
File "/home/phill/Projects/openlp/bug1073931/openlp/plugins/bibles/lib/http.py", line 649, in send_error_message
translate('BiblesPlugin.HTTPBible', 'There was a problem extracting your verse selection. If this error '
File "/home/phill/Projects/openlp/bug1073931/openlp/core/lib/ui.py", line 114, in critical_error_message_box
return Registry().get('main_window').error_message(title if title else UiStrings().Error, message)
nose.proxy.AttributeError: 'NoneType' object has no attribute 'error_message'
-------------------- >> begin captured logging << --------------------
openlp.core.common.registry: INFO: Registry Initialising
openlp.plugins.bibles.lib.http: DEBUG: CWExtract.init("None")
openlp.plugins.bibles.lib.http: DEBUG: CWExtract.get_bible_chapter("niv", "john", "3")
openlp.core.utils.__init__: DEBUG: Downloading URL = http://www.biblestudytools.com/niv/john/3.html
openlp.core.utils.__init__: DEBUG: Downloaded URL = http://www.biblestudytools.com/niv/john/3.html
openlp.core.utils.__init__: DEBUG: <http.client.HTTPResponse object at 0x7f66e33ecd68>
openlp.plugins.bibles.lib.http: ERROR: No verses found in the CrossWalk response.
--------------------- >> end captured logging << ---------------------
----------------------------------------------------------------------
Ran 576 tests in 26.153s
FAILED (SKIP=4, errors=2)
Process finished with exit code 1
--
Your team OpenLP Core is requested to review the proposed merge of lp:~phill-ridout/openlp/bug1073931 into lp:openlp.
=== modified file 'openlp/core/lib/db.py'
--- openlp/core/lib/db.py 2015-01-19 08:34:29 +0000
+++ openlp/core/lib/db.py 2015-02-05 17:35:44 +0000
@@ -60,6 +60,35 @@
return session, metadata
+def get_db_path(plugin_name, db_file_name=None):
+ """
+ Create a path to a database from the plugin name and database name
+
+ :param plugin_name: Name of plugin
+ :param db_file_name: File name of database
+ :return: The path to the database as type str
+ """
+ if db_file_name is None:
+ return 'sqlite:///%s/%s.sqlite' % (AppLocation.get_section_data_path(plugin_name), plugin_name)
+ else:
+ return 'sqlite:///%s/%s' % (AppLocation.get_section_data_path(plugin_name), db_file_name)
+
+
+def handle_db_error(plugin_name, db_file_name):
+ """
+ Log and report to the user that a database cannot be loaded
+
+ :param plugin_name: Name of plugin
+ :param db_file_name: File name of database
+ :return: None
+ """
+ db_path = get_db_path(plugin_name, db_file_name)
+ log.exception('Error loading database: %s', db_path)
+ critical_error_message_box(translate('OpenLP.Manager', 'Database Error'),
+ translate('OpenLP.Manager', 'OpenLP cannot load your database.\n\nDatabase: %s')
+ % db_path)
+
+
def init_url(plugin_name, db_file_name=None):
"""
Return the database URL.
@@ -69,13 +98,9 @@
"""
settings = Settings()
settings.beginGroup(plugin_name)
- db_url = ''
db_type = settings.value('db type')
if db_type == 'sqlite':
- if db_file_name is None:
- db_url = 'sqlite:///%s/%s.sqlite' % (AppLocation.get_section_data_path(plugin_name), plugin_name)
- else:
- db_url = 'sqlite:///%s/%s' % (AppLocation.get_section_data_path(plugin_name), db_file_name)
+ db_url = get_db_path(plugin_name, db_file_name)
else:
db_url = '%s://%s:%s@%s/%s' % (db_type, urlquote(settings.value('db username')),
urlquote(settings.value('db password')),
@@ -212,7 +237,7 @@
try:
db_ver, up_ver = upgrade_db(self.db_url, upgrade_mod)
except (SQLAlchemyError, DBAPIError):
- log.exception('Error loading database: %s', self.db_url)
+ handle_db_error(plugin_name, db_file_name)
return
if db_ver > up_ver:
critical_error_message_box(
@@ -225,10 +250,7 @@
try:
self.session = init_schema(self.db_url)
except (SQLAlchemyError, DBAPIError):
- log.exception('Error loading database: %s', self.db_url)
- critical_error_message_box(translate('OpenLP.Manager', 'Database Error'),
- translate('OpenLP.Manager', 'OpenLP cannot load your database.\n\nDatabase: %s')
- % self.db_url)
+ handle_db_error(plugin_name, db_file_name)
def save_object(self, object_instance, commit=True):
"""
@@ -362,6 +384,8 @@
:param object_class: The type of objects to return.
:param filter_clause: The filter governing selection of objects to return. Defaults to None.
"""
+ if not self.session:
+ return
query = self.session.query(object_class)
if filter_clause is not None:
query = query.filter(filter_clause)
=== modified file 'openlp/core/ui/firsttimeform.py'
--- openlp/core/ui/firsttimeform.py 2015-01-20 21:38:34 +0000
+++ openlp/core/ui/firsttimeform.py 2015-02-05 17:35:44 +0000
@@ -22,6 +22,7 @@
"""
This module contains the first time wizard.
"""
+import hashlib
import logging
import os
import time
@@ -47,10 +48,10 @@
"""
This thread downloads a theme's screenshot
"""
- screenshot_downloaded = QtCore.pyqtSignal(str, str)
+ screenshot_downloaded = QtCore.pyqtSignal(str, str, str)
finished = QtCore.pyqtSignal()
- def __init__(self, themes_url, title, filename, screenshot):
+ def __init__(self, themes_url, title, filename, sha256, screenshot):
"""
Set up the worker object
"""
@@ -58,6 +59,7 @@
self.themes_url = themes_url
self.title = title
self.filename = filename
+ self.sha256 = sha256
self.screenshot = screenshot
super(ThemeScreenshotWorker, self).__init__()
@@ -71,7 +73,7 @@
urllib.request.urlretrieve('%s%s' % (self.themes_url, self.screenshot),
os.path.join(gettempdir(), 'openlp', self.screenshot))
# Signal that the screenshot has been downloaded
- self.screenshot_downloaded.emit(self.title, self.filename)
+ self.screenshot_downloaded.emit(self.title, self.filename, self.sha256)
except:
log.exception('Unable to download screenshot')
finally:
@@ -221,8 +223,9 @@
self.application.process_events()
title = self.config.get('songs_%s' % song, 'title')
filename = self.config.get('songs_%s' % song, 'filename')
+ sha256 = self.config.get('songs_%s' % song, 'sha256', fallback='')
item = QtGui.QListWidgetItem(title, self.songs_list_widget)
- item.setData(QtCore.Qt.UserRole, filename)
+ item.setData(QtCore.Qt.UserRole, (filename, sha256))
item.setCheckState(QtCore.Qt.Unchecked)
item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable)
bible_languages = self.config.get('bibles', 'languages')
@@ -237,8 +240,9 @@
self.application.process_events()
title = self.config.get('bible_%s' % bible, 'title')
filename = self.config.get('bible_%s' % bible, 'filename')
+ sha256 = self.config.get('bible_%s' % bible, 'sha256', fallback='')
item = QtGui.QTreeWidgetItem(lang_item, [title])
- item.setData(0, QtCore.Qt.UserRole, filename)
+ item.setData(0, QtCore.Qt.UserRole, (filename, sha256))
item.setCheckState(0, QtCore.Qt.Unchecked)
item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable)
self.bibles_tree_widget.expandAll()
@@ -249,8 +253,9 @@
self.application.process_events()
title = self.config.get('theme_%s' % theme, 'title')
filename = self.config.get('theme_%s' % theme, 'filename')
+ sha256 = self.config.get('theme_%s' % theme, 'sha256', fallback='')
screenshot = self.config.get('theme_%s' % theme, 'screenshot')
- worker = ThemeScreenshotWorker(self.themes_url, title, filename, screenshot)
+ worker = ThemeScreenshotWorker(self.themes_url, title, filename, sha256, screenshot)
self.theme_screenshot_workers.append(worker)
worker.screenshot_downloaded.connect(self.on_screenshot_downloaded)
thread = QtCore.QThread(self)
@@ -350,7 +355,7 @@
time.sleep(0.1)
self.application.set_normal_cursor()
- def on_screenshot_downloaded(self, title, filename):
+ def on_screenshot_downloaded(self, title, filename, sha256):
"""
Add an item to the list when a theme has been downloaded
@@ -358,7 +363,7 @@
:param filename: The filename of the theme
"""
item = QtGui.QListWidgetItem(title, self.themes_list_widget)
- item.setData(QtCore.Qt.UserRole, filename)
+ item.setData(QtCore.Qt.UserRole, (filename, sha256))
item.setCheckState(QtCore.Qt.Unchecked)
item.setFlags(item.flags() | QtCore.Qt.ItemIsUserCheckable)
@@ -372,7 +377,7 @@
Settings().setValue('core/has run wizard', True)
self.close()
- def url_get_file(self, url, f_path):
+ def url_get_file(self, url, f_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.
@@ -387,16 +392,24 @@
try:
url_file = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT)
filename = open(f_path, "wb")
+ if sha256:
+ hasher = hashlib.sha256()
# Download until finished or canceled.
while not self.was_cancelled:
data = url_file.read(block_size)
if not data:
break
filename.write(data)
+ if sha256:
+ hasher.update(data)
block_count += 1
self._download_progress(block_count, block_size)
filename.close()
- except ConnectionError:
+ if sha256 and hasher.hexdigest() != sha256:
+ log.error('sha256 sums did not match for file: {}'.format(f_path))
+ os.remove(f_path)
+ return False
+ except urllib.error.URLError:
trace_error_handler(log)
filename.close()
os.remove(f_path)
@@ -436,7 +449,7 @@
site = urllib.request.urlopen(url, timeout=CONNECTION_TIMEOUT)
meta = site.info()
return int(meta.get("Content-Length"))
- except ConnectionException:
+ except urllib.error.URLError:
if retries > CONNECTION_RETRIES:
raise
else:
@@ -478,7 +491,7 @@
self.application.process_events()
item = self.songs_list_widget.item(i)
if item.checkState() == QtCore.Qt.Checked:
- filename = item.data(QtCore.Qt.UserRole)
+ filename, _ = item.data(QtCore.Qt.UserRole)
size = self._get_file_size('%s%s' % (self.songs_url, filename))
self.max_progress += size
# Loop through the Bibles list and increase for each selected item
@@ -487,7 +500,7 @@
self.application.process_events()
item = iterator.value()
if item.parent() and item.checkState(0) == QtCore.Qt.Checked:
- filename = item.data(0, QtCore.Qt.UserRole)
+ filename, _ = item.data(0, QtCore.Qt.UserRole)
size = self._get_file_size('%s%s' % (self.bibles_url, filename))
self.max_progress += size
iterator += 1
@@ -496,10 +509,10 @@
self.application.process_events()
item = self.themes_list_widget.item(i)
if item.checkState() == QtCore.Qt.Checked:
- filename = item.data(QtCore.Qt.UserRole)
+ filename, _ = item.data(QtCore.Qt.UserRole)
size = self._get_file_size('%s%s' % (self.themes_url, filename))
self.max_progress += size
- except ConnectionError:
+ except urllib.error.URLError:
trace_error_handler(log)
critical_error_message_box(translate('OpenLP.FirstTimeWizard', 'Download Error'),
translate('OpenLP.FirstTimeWizard', 'There was a connection problem during '
@@ -595,31 +608,33 @@
for i in range(self.songs_list_widget.count()):
item = self.songs_list_widget.item(i)
if item.checkState() == QtCore.Qt.Checked:
- filename = item.data(QtCore.Qt.UserRole)
+ filename, sha256 = item.data(QtCore.Qt.UserRole)
self._increment_progress_bar(self.downloading % filename, 0)
self.previous_size = 0
destination = os.path.join(songs_destination, str(filename))
- if not self.url_get_file('%s%s' % (self.songs_url, filename), destination):
+ if not self.url_get_file('%s%s' % (self.songs_url, filename), destination, sha256):
return False
# Download Bibles
bibles_iterator = QtGui.QTreeWidgetItemIterator(self.bibles_tree_widget)
while bibles_iterator.value():
item = bibles_iterator.value()
if item.parent() and item.checkState(0) == QtCore.Qt.Checked:
- bible = item.data(0, QtCore.Qt.UserRole)
+ bible, sha256 = item.data(0, QtCore.Qt.UserRole)
self._increment_progress_bar(self.downloading % bible, 0)
self.previous_size = 0
- if not self.url_get_file('%s%s' % (self.bibles_url, bible), os.path.join(bibles_destination, bible)):
+ if not self.url_get_file('%s%s' % (self.bibles_url, bible), os.path.join(bibles_destination, bible),
+ sha256):
return False
bibles_iterator += 1
# Download themes
for i in range(self.themes_list_widget.count()):
item = self.themes_list_widget.item(i)
if item.checkState() == QtCore.Qt.Checked:
- theme = item.data(QtCore.Qt.UserRole)
+ theme, sha256 = item.data(QtCore.Qt.UserRole)
self._increment_progress_bar(self.downloading % theme, 0)
self.previous_size = 0
- if not self.url_get_file('%s%s' % (self.themes_url, theme), os.path.join(themes_destination, theme)):
+ if not self.url_get_file('%s%s' % (self.themes_url, theme), os.path.join(themes_destination, theme),
+ sha256):
return False
return True
=== modified file 'openlp/plugins/bibles/lib/db.py'
--- openlp/plugins/bibles/lib/db.py 2015-01-18 13:39:21 +0000
+++ openlp/plugins/bibles/lib/db.py 2015-02-05 17:35:44 +0000
@@ -131,6 +131,7 @@
log.info('BibleDB loaded')
QtCore.QObject.__init__(self)
self.bible_plugin = parent
+ self.session = None
if 'path' not in kwargs:
raise KeyError('Missing keyword argument "path".')
if 'name' not in kwargs and 'file' not in kwargs:
@@ -144,8 +145,9 @@
if 'file' in kwargs:
self.file = kwargs['file']
Manager.__init__(self, 'bibles', init_schema, self.file, upgrade)
- if 'file' in kwargs:
- self.get_name()
+ if self.session:
+ if 'file' in kwargs:
+ self.get_name()
if 'path' in kwargs:
self.path = kwargs['path']
self.wizard = None
=== modified file 'openlp/plugins/bibles/lib/manager.py'
--- openlp/plugins/bibles/lib/manager.py 2015-01-18 13:39:21 +0000
+++ openlp/plugins/bibles/lib/manager.py 2015-02-05 17:35:44 +0000
@@ -121,6 +121,8 @@
self.old_bible_databases = []
for filename in files:
bible = BibleDB(self.parent, path=self.path, file=filename)
+ if not bible.session:
+ continue
name = bible.get_name()
# Remove corrupted files.
if name is None:
=== modified file 'tests/functional/test_init.py'
--- tests/functional/test_init.py 2015-01-26 20:42:19 +0000
+++ tests/functional/test_init.py 2015-02-05 17:35:44 +0000
@@ -125,7 +125,7 @@
# WHEN: Calling parse_options
results = parse_options(options)
- # THEN: A tuple should be returned with the parsed options and left over args
+ # THEN: A tuple should be returned with the parsed options and left over options
self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True,
'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args']))
@@ -136,10 +136,51 @@
# GIVEN: A list of valid options
options = ['openlp.py', '-e', '-l', 'debug', '-pd', '-s', 'style', 'extra', 'qt', 'args']
- # WHEN: Passing in the options through sys.argv and calling parse_args with None
+ # WHEN: Passing in the options through sys.argv and calling parse_options with None
with patch.object(sys, 'argv', options):
results = parse_options(None)
- # THEN: parse_args should return a tuple of valid options and of left over options that OpenLP does not use
- self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True,
- 'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args']))
+ # THEN: parse_options should return a tuple of valid options and of left over options that OpenLP does not use
+ self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True,
+ 'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args']))
+
+ def test_parse_options_valid_long_options(self):
+ """
+ Test that parse_options parses valid long options correctly
+ """
+ # GIVEN: A list of valid options
+ options = ['--no-error-form', 'extra', '--log-level', 'debug', 'qt', '--portable', '--dev-version', 'args',
+ '--style=style']
+
+ # WHEN: Calling parse_options
+ results = parse_options(options)
+
+ # THEN: parse_options should return a tuple of valid options and of left over options that OpenLP does not use
+ self.assertEqual(results, (Values({'no_error_form': True, 'dev_version': True, 'portable': True,
+ 'style': 'style', 'loglevel': 'debug'}), ['extra', 'qt', 'args']))
+
+ def test_parse_options_help_option(self):
+ """
+ Test that parse_options raises an SystemExit exception when called with invalid options
+ """
+ # GIVEN: The --help option
+ options = ['--help']
+
+ # WHEN: Calling parse_options
+ # THEN: parse_options should raise an SystemExit exception with exception code 0
+ with self.assertRaises(SystemExit) as raised_exception:
+ parse_options(options)
+ self.assertEqual(raised_exception.exception.code, 0)
+
+ def test_parse_options_invalid_option(self):
+ """
+ Test that parse_options raises an SystemExit exception when called with invalid options
+ """
+ # GIVEN: A list including invalid options
+ options = ['-t']
+
+ # WHEN: Calling parse_options
+ # THEN: parse_options should raise an SystemExit exception with exception code 2
+ with self.assertRaises(SystemExit) as raised_exception:
+ parse_options(options)
+ self.assertEqual(raised_exception.exception.code, 2)
Follow ups
