openlp-core team mailing list archive
-
openlp-core team
-
Mailing list archive
-
Message #26020
Re: [Merge] lp:~phill-ridout/openlp/bug1073931 into lp:openlp
Tim, could you see my comments please.
Diff comments:
> === 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:
Because the sha256 arg is optional:
def url_get_file(self, url, f_path, sha256=None):
if the sha256 is not supplied then we just want to accept the file as valid with out checking the hash.
> + 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)
I'm not quite sure what you mean could you give an example please?
> 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)
>
--
https://code.launchpad.net/~phill-ridout/openlp/bug1073931/+merge/248803
Your team OpenLP Core is subscribed to branch lp:openlp.
References
