openlp-core team mailing list archive

[Phill <phill.ridout@xxxxxxxxx>]

Phill has proposed merging lp:~phill-ridout/openlp/bug1419691 into lp:openlp.

Requested reviews:
  OpenLP Core (openlp-core)
Related bugs:
  Bug #1419691 in OpenLP: "Remove support for .theme files"
  https://bugs.launchpad.net/openlp/+bug/1419691

For more details, see:
https://code.launchpad.net/~phill-ridout/openlp/bug1419691/+merge/249735

Fixes bug1419691 by checking the theme version number (OpenLP1 themes didn't have a version no.) and by removing the *.theme filter from the file dialog.

Also added a ValidationError exception class, to tidy up the unzip code slightly

Tests are still failing on crosswalk, however all tests (except crosswalk) are passing locally.

Add this to your merge proposal:
--------------------------------
lp:~phill-ridout/openlp/bug1419691 (revision 2499)
[SUCCESS] http://ci.openlp.org/job/Branch-01-Pull/945/
[SUCCESS] http://ci.openlp.org/job/Branch-02-Functional-Tests/871/
[FAILURE] http://ci.openlp.org/job/Branch-03-Interface-Tests/816/
Stopping after failure

Process finished with exit code 0
-- 
Your team OpenLP Core is requested to review the proposed merge of lp:~phill-ridout/openlp/bug1419691 into lp:openlp.

=== modified file 'openlp/core/common/historycombobox.py'
--- openlp/core/common/historycombobox.py	2015-01-18 13:39:21 +0000
+++ openlp/core/common/historycombobox.py	2015-02-13 23:28:54 +0000
@@ -20,7 +20,7 @@
 # Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
 ###############################################################################
 """
-The :mod:`~openlp.core.lib.historycombobox` module contains the HistoryComboBox widget
+The :mod:`~openlp.core.common.historycombobox` module contains the HistoryComboBox widget
 """
 
 from PyQt4 import QtCore, QtGui
@@ -28,7 +28,7 @@
 
 class HistoryComboBox(QtGui.QComboBox):
     """
-    The :class:`~openlp.core.lib.historycombobox.HistoryComboBox` widget emulates the QLineEdit ``returnPressed`` signal
+    The :class:`~openlp.core.common.historycombobox.HistoryComboBox` widget emulates the QLineEdit ``returnPressed`` signal
     for when the :kbd:`Enter` or :kbd:`Return` keys are pressed, and saves anything that is typed into the edit box into
     its list.
     """

=== modified file 'openlp/core/lib/__init__.py'
--- openlp/core/lib/__init__.py	2015-01-18 13:39:21 +0000
+++ openlp/core/lib/__init__.py	2015-02-13 23:28:54 +0000
@@ -312,6 +312,7 @@
 
 
 from .colorbutton import ColorButton
+from .exceptions import ValidationError
 from .filedialog import FileDialog
 from .screen import ScreenList
 from .listwidgetwithdnd import ListWidgetWithDnD

=== added file 'openlp/core/lib/exceptions.py'
--- openlp/core/lib/exceptions.py	1970-01-01 00:00:00 +0000
+++ openlp/core/lib/exceptions.py	2015-02-13 23:28:54 +0000
@@ -0,0 +1,32 @@
+# -*- coding: utf-8 -*-
+# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
+
+###############################################################################
+# OpenLP - Open Source Lyrics Projection                                      #
+# --------------------------------------------------------------------------- #
+# Copyright (c) 2008-2015 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.lib.exceptions` module contains custom exceptions
+"""
+
+
+class ValidationError(Exception):
+    """
+    The :class:`~openlp.core.lib.exceptions.ValidationError` exception provides a custom exception for validating
+    import files.
+    """
+    pass

=== modified file 'openlp/core/ui/thememanager.py'
--- openlp/core/ui/thememanager.py	2015-01-18 13:39:21 +0000
+++ openlp/core/ui/thememanager.py	2015-02-13 23:28:54 +0000
@@ -31,7 +31,7 @@
 
 from openlp.core.common import Registry, RegistryProperties, AppLocation, Settings, OpenLPMixin, RegistryMixin, \
     check_directory_exists, UiStrings, translate, is_win
-from openlp.core.lib import FileDialog, ImageSource, OpenLPToolbar, get_text_file_string, build_icon, \
+from openlp.core.lib import FileDialog, ImageSource, OpenLPToolbar, ValidationError, get_text_file_string, build_icon, \
     check_item_selected, create_thumb, validate_thumb
 from openlp.core.lib.theme import ThemeXML, BackgroundType
 from openlp.core.lib.ui import critical_error_message_box, create_widget_action
@@ -414,13 +414,13 @@
     def on_import_theme(self, field=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 load both OpenLP version 1 and version 2 themes.
+        those files. This process will only load version 2 themes.
         :param field:
         """
         files = FileDialog.getOpenFileNames(self,
                                             translate('OpenLP.ThemeManager', 'Select Theme Import File'),
                                             Settings().value(self.settings_section + '/last directory import'),
-                                            translate('OpenLP.ThemeManager', 'OpenLP Themes (*.theme *.otz)'))
+                                            translate('OpenLP.ThemeManager', 'OpenLP Themes (*.otz)'))
         self.log_info('New Themes %s' % str(files))
         if not files:
             return
@@ -535,7 +535,6 @@
         :param directory:
         """
         self.log_debug('Unzipping theme %s' % file_name)
-        file_name = str(file_name)
         theme_zip = None
         out_file = None
         file_xml = None
@@ -545,8 +544,12 @@
             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 "%s" XML files' % len(xml_file))
-                raise Exception('validation')
+                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()
             theme_folder = os.path.join(directory, theme_name)
             theme_exists = os.path.exists(theme_folder)
@@ -573,13 +576,10 @@
                 out_file.close()
         except (IOError, zipfile.BadZipfile):
             self.log_exception('Importing theme from zip failed %s' % file_name)
-            raise Exception('validation')
-        except Exception as info:
-            if str(info) == 'validation':
-                critical_error_message_box(translate('OpenLP.ThemeManager', 'Validation Error'),
-                                           translate('OpenLP.ThemeManager', 'File is not a valid theme.'))
-            else:
-                raise
+            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:

=== modified file 'tests/functional/openlp_core_ui/test_thememanager.py'
--- tests/functional/openlp_core_ui/test_thememanager.py	2015-01-18 13:39:21 +0000
+++ tests/functional/openlp_core_ui/test_thememanager.py	2015-02-13 23:28:54 +0000
@@ -27,14 +27,13 @@
 import shutil
 
 from unittest import TestCase
-from tests.functional import MagicMock
 from tempfile import mkdtemp
 
 from openlp.core.ui import ThemeManager
 from openlp.core.common import Registry
 
 from tests.utils.constants import TEST_RESOURCES_PATH
-from tests.functional import MagicMock, patch
+from tests.functional import ANY, MagicMock, patch
 
 
 class TestThemeManager(TestCase):
@@ -152,3 +151,23 @@
             self.assertTrue(os.path.exists(os.path.join(folder, 'Moss on tree', 'Moss on tree.xml')))
             self.assertEqual(mocked_critical_error_message_box.call_count, 0, 'No errors should have happened')
             shutil.rmtree(folder)
+
+    def unzip_theme_invalid_version_test(self):
+        """
+        Test that themes with invalid (< 2.0) or with no version attributes are rejected
+        """
+        # GIVEN: An instance of ThemeManager whilst mocking a theme that returns a theme with no version attribute
+        with patch('openlp.core.ui.thememanager.zipfile.ZipFile') as mocked_zip_file,\
+                patch('openlp.core.ui.thememanager.ElementTree.getroot') as mocked_getroot,\
+                patch('openlp.core.ui.thememanager.XML'),\
+                patch('openlp.core.ui.thememanager.critical_error_message_box') as mocked_critical_error_message_box:
+
+            mocked_zip_file.return_value = MagicMock(**{'namelist.return_value': [os.path.join('theme', 'theme.xml')]})
+            mocked_getroot.return_value = MagicMock(**{'get.return_value': None})
+            theme_manager = ThemeManager(None)
+
+            # WHEN: unzip_theme is called
+            theme_manager.unzip_theme('theme.file', 'folder')
+
+            # THEN: The critical_error_message_box should have been called
+            mocked_critical_error_message_box.assert_called_once(ANY, ANY)