Merge lp:~raoul-snyman/openlp/settings-upgrade into lp:openlp

[Raoul Snyman]

Add multi-to-one settings upgrading, and merge all the post-2.4 settings changes into __setting_upgrade_2__

Add multi-to-one settings upgrading, and merge all the post-2.4 settings changes into __setting_upgrade_2__

Add this to your merge proposal:
--------------------------------------------------------------------------------
lp:~raoul-snyman/openlp/settings-upgrade (revision 2791)
https://ci.openlp.io/job/Branch-01-Pull/2295/ [SUCCESS]
https://ci.openlp.io/job/Branch-02-Functional-Tests/2196/ [SUCCESS]
https://ci.openlp.io/job/Branch-03-Interface-Tests/2074/ [SUCCESS]
https://ci.openlp.io/job/Branch-04a-Code_Analysis/1401/ [SUCCESS]
https://ci.openlp.io/job/Branch-04b-Test_Coverage/1225/ [SUCCESS]
https://ci.openlp.io/job/Branch-04c-Code_Analysis2/355/ [SUCCESS]
https://ci.openlp.io/job/Branch-05-AppVeyor-Tests/187/ [FAILURE]
Stopping after failure, see https://ci.openlp.io/job/Branch-05-AppVeyor-Tests/187/console for more details

[Phill (phill-ridout)]

Seems good to me, 1 minor comment, but I'd be happy for this to be merged

review: Approve

[Tim Bentley (trb143)]

review: Approve

=== modified file 'nose2.cfg'
--- nose2.cfg	2016-08-10 09:07:49 +0000
+++ nose2.cfg	2017-11-16 05:12:46 +0000
@@ -1,5 +1,5 @@
 [unittest]
-verbose = true
+verbose = True
 plugins = nose2.plugins.mp
 
 [log-capture]
@@ -9,14 +9,19 @@
 log-level = ERROR
 
 [test-result]
-always-on = true
-descriptions = true
+always-on = True
+descriptions = True
 
 [coverage]
-always-on = true
+always-on = True
 coverage = openlp
 coverage-report = html
 
 [multiprocess]
-always-on = false
+always-on = False
 processes = 4
+
+[output-buffer]
+always-on = True
+stderr = True
+stdout = False

=== modified file 'openlp/core/common/settings.py'
--- openlp/core/common/settings.py	2017-11-10 22:50:04 +0000
+++ openlp/core/common/settings.py	2017-11-16 05:12:46 +0000
@@ -40,7 +40,7 @@
 
 # Fix for bug #1014422.
 X11_BYPASS_DEFAULT = True
-if is_linux():
+if is_linux():                                                                              # pragma: no cover
     # Default to False on Gnome.
     X11_BYPASS_DEFAULT = bool(not os.environ.get('GNOME_DESKTOP_SESSION_ID'))
     # Default to False on Xfce.
@@ -206,11 +206,14 @@
         'projector/source dialog type': 0  # Source select dialog box type
     }
     __file_path__ = ''
+    # Settings upgrades prior to 3.0
     __setting_upgrade_1__ = [
-        # Changed during 2.2.x development.
         ('songs/search as type', 'advanced/search as type', []),
         ('media/players', 'media/players_temp', [(media_players_conv, None)]),  # Convert phonon to system
         ('media/players_temp', 'media/players', []),  # Move temp setting from above to correct setting
+    ]
+    # Settings upgrades for 3.0 (aka 2.6)
+    __setting_upgrade_2__ = [
         ('advanced/default color', 'core/logo background color', []),  # Default image renamed + moved to general > 2.4.
         ('advanced/default image', 'core/logo file', []),  # Default image renamed + moved to general after 2.4.
         ('remotes/https enabled', '', []),
@@ -231,9 +234,7 @@
         # Last search type was renamed to last used search type in 2.6 since Bible search value type changed in 2.6.
         ('songs/last search type', 'songs/last used search type', []),
         ('bibles/last search type', '', []),
-        ('custom/last search type', 'custom/last used search type', [])]
-
-    __setting_upgrade_2__ = [
+        ('custom/last search type', 'custom/last used search type', []),
         # The following changes are being made for the conversion to using Path objects made in 2.6 development
         ('advanced/data path', 'advanced/data path', [(str_to_path, None)]),
         ('crashreport/last directory', 'crashreport/last directory', [(str_to_path, None)]),
@@ -467,32 +468,38 @@
         for version in range(current_version, __version__):
             version += 1
             upgrade_list = getattr(self, '__setting_upgrade_{version}__'.format(version=version))
-            for old_key, new_key, rules in upgrade_list:
+            for old_keys, new_key, rules in upgrade_list:
                 # Once removed we don't have to do this again. - Can be removed once fully switched to the versioning
                 # system.
-                if not self.contains(old_key):
+                if not isinstance(old_keys, (tuple, list)):
+                    old_keys = [old_keys]
+                if any([not self.contains(old_key) for old_key in old_keys]):
+                    log.warning('One of {} does not exist, skipping upgrade'.format(old_keys))
                     continue
                 if new_key:
                     # Get the value of the old_key.
-                    old_value = super(Settings, self).value(old_key)
+                    old_values = [super(Settings, self).value(old_key) for old_key in old_keys]
                     # When we want to convert the value, we have to figure out the default value (because we cannot get
                     # the default value from the central settings dict.
                     if rules:
-                        default_value = rules[0][1]
-                        old_value = self._convert_value(old_value, default_value)
+                        default_values = rules[0][1]
+                        if not isinstance(default_values, (list, tuple)):
+                            default_values = [default_values]
+                        old_values = [self._convert_value(old_value, default_value)
+                                      for old_value, default_value in zip(old_values, default_values)]
                     # Iterate over our rules and check what the old_value should be "converted" to.
-                    for new, old in rules:
+                    new_value = None
+                    for new_rule, old_rule in rules:
                         # If the value matches with the condition (rule), then use the provided value. This is used to
                         # convert values. E. g. an old value 1 results in True, and 0 in False.
-                        if callable(new):
-                            old_value = new(old_value)
-                        elif old == old_value:
-                            old_value = new
+                        if callable(new_rule):
+                            new_value = new_rule(*old_values)
+                        elif old_rule in old_values:
+                            new_value = new_rule
                             break
-                    self.setValue(new_key, old_value)
-                if new_key != old_key:
-                    self.remove(old_key)
-        self.setValue('settings/version', version)
+                    self.setValue(new_key, new_value)
+                [self.remove(old_key) for old_key in old_keys if old_key != new_key]
+            self.setValue('settings/version', version)
 
     def value(self, key):
         """

=== modified file 'tests/functional/openlp_core/common/test_settings.py'
--- tests/functional/openlp_core/common/test_settings.py	2017-10-07 07:05:07 +0000
+++ tests/functional/openlp_core/common/test_settings.py	2017-11-16 05:12:46 +0000
@@ -22,10 +22,12 @@
 """
 Package to test the openlp.core.lib.settings package.
 """
+from pathlib import Path
 from unittest import TestCase
-from unittest.mock import patch
+from unittest.mock import call, patch
 
-from openlp.core.common.settings import Settings
+from openlp.core.common import settings
+from openlp.core.common.settings import Settings, media_players_conv
 
 from tests.helpers.testmixin import TestMixin
 
@@ -47,28 +49,58 @@
         """
         self.destroy_settings()
 
-    def test_settings_basic(self):
-        """
-        Test the Settings creation and its default usage
-        """
-        # GIVEN: A new Settings setup
+    def test_media_players_conv(self):
+        """Test the media players conversion function"""
+        # GIVEN: A list of media players
+        media_players = 'phonon,webkit,vlc'
+
+        # WHEN: The media converter function is called
+        result = media_players_conv(media_players)
+
+        # THEN: The list should have been converted correctly
+        assert result == 'system,webkit,vlc'
+
+    def test_default_value(self):
+        """Test reading a setting that doesn't exist yet"""
+        # GIVEN: A setting that doesn't exist yet
 
         # WHEN reading a setting for the first time
         default_value = Settings().value('core/has run wizard')
 
         # THEN the default value is returned
-        self.assertFalse(default_value, 'The default value should be False')
+        assert default_value is False, 'The default value should be False'
 
+    def test_save_new_value(self):
+        """Test saving a new setting"""
+        # GIVEN: A setting that hasn't been saved yet
         # WHEN a new value is saved into config
         Settings().setValue('core/has run wizard', True)
 
         # THEN the new value is returned when re-read
-        self.assertTrue(Settings().value('core/has run wizard'), 'The saved value should have been returned')
+        assert Settings().value('core/has run wizard') is True, 'The saved value should have been returned'
+
+    def test_set_up_default_values(self):
+        """Test that the default values are updated"""
+        # GIVEN: A Settings object with defaults
+        # WHEN: set_up_default_values() is called
+        Settings.set_up_default_values()
+
+        # THEN: The default values should have been added to the dictionary
+        assert 'advanced/default service name' in Settings.__default_settings__
+
+    def test_get_default_value(self):
+        """Test that the default value for a setting is returned"""
+        # GIVEN: A Settings class with a default value
+        Settings.__default_settings__['test/moo'] = 'baa'
+
+        # WHEN: get_default_value() is called
+        result = Settings().get_default_value('test/moo')
+
+        # THEN: The correct default value should be returned
+        assert result == 'baa'
 
     def test_settings_override(self):
-        """
-        Test the Settings creation and its override usage
-        """
+        """Test the Settings creation and its override usage"""
         # GIVEN: an override for the settings
         screen_settings = {
             'test/extend': 'very wide',
@@ -79,18 +111,22 @@
         extend = Settings().value('test/extend')
 
         # THEN the default value is returned
-        self.assertEqual('very wide', extend, 'The default value of "very wide" should be returned')
+        assert extend == 'very wide', 'The default value of "very wide" should be returned'
+
+    def test_save_existing_setting(self):
+        """Test that saving an existing setting returns the new value"""
+        # GIVEN: An existing setting
+        Settings().extend_default_settings({'test/existing value': None})
+        Settings().setValue('test/existing value', 'old value')
 
         # WHEN a new value is saved into config
-        Settings().setValue('test/extend', 'very short')
+        Settings().setValue('test/existing value', 'new value')
 
         # THEN the new value is returned when re-read
-        self.assertEqual('very short', Settings().value('test/extend'), 'The saved value should be returned')
+        assert Settings().value('test/existing value') == 'new value', 'The saved value should be returned'
 
     def test_settings_override_with_group(self):
-        """
-        Test the Settings creation and its override usage - with groups
-        """
+        """Test the Settings creation and its override usage - with groups"""
         # GIVEN: an override for the settings
         screen_settings = {
             'test/extend': 'very wide',
@@ -112,9 +148,7 @@
         self.assertEqual('very short', Settings().value('test/extend'), 'The saved value should be returned')
 
     def test_settings_nonexisting(self):
-        """
-        Test the Settings on query for non-existing value
-        """
+        """Test the Settings on query for non-existing value"""
         # GIVEN: A new Settings setup
         with self.assertRaises(KeyError) as cm:
             # WHEN reading a setting that doesn't exists
@@ -124,9 +158,7 @@
         self.assertEqual(str(cm.exception), "'core/does not exists'", 'We should get an exception')
 
     def test_extend_default_settings(self):
-        """
-        Test that the extend_default_settings method extends the default settings
-        """
+        """Test that the extend_default_settings method extends the default settings"""
         # GIVEN: A patched __default_settings__ dictionary
         with patch.dict(Settings.__default_settings__,
                         {'test/setting 1': 1, 'test/setting 2': 2, 'test/setting 3': 3}, True):
@@ -138,3 +170,125 @@
             self.assertEqual(
                 Settings.__default_settings__, {'test/setting 1': 1, 'test/setting 2': 2, 'test/setting 3': 4,
                                                 'test/extended 1': 1, 'test/extended 2': 2})
+
+    @patch('openlp.core.common.settings.QtCore.QSettings.contains')
+    @patch('openlp.core.common.settings.QtCore.QSettings.value')
+    @patch('openlp.core.common.settings.QtCore.QSettings.setValue')
+    @patch('openlp.core.common.settings.QtCore.QSettings.remove')
+    def test_upgrade_single_setting(self, mocked_remove, mocked_setValue, mocked_value, mocked_contains):
+        """Test that the upgrade mechanism for settings works correctly for single value upgrades"""
+        # GIVEN: A settings object with an upgrade step to take (99, so that we don't interfere with real ones)
+        local_settings = Settings()
+        local_settings.__setting_upgrade_99__ = [
+            ('single/value', 'single/new value', [(str, '')])
+        ]
+        settings.__version__ = 99
+        mocked_value.side_effect = [98, 10]
+        mocked_contains.return_value = True
+
+        # WHEN: upgrade_settings() is called
+        local_settings.upgrade_settings()
+
+        # THEN: The correct calls should have been made with the correct values
+        assert mocked_value.call_count == 2, 'Settings().value() should have been called twice'
+        assert mocked_value.call_args_list == [call('settings/version', 0), call('single/value')]
+        assert mocked_setValue.call_count == 2, 'Settings().setValue() should have been called twice'
+        assert mocked_setValue.call_args_list == [call('single/new value', '10'), call('settings/version', 99)]
+        mocked_contains.assert_called_once_with('single/value')
+        mocked_remove.assert_called_once_with('single/value')
+
+    @patch('openlp.core.common.settings.QtCore.QSettings.contains')
+    @patch('openlp.core.common.settings.QtCore.QSettings.value')
+    @patch('openlp.core.common.settings.QtCore.QSettings.setValue')
+    @patch('openlp.core.common.settings.QtCore.QSettings.remove')
+    def test_upgrade_setting_value(self, mocked_remove, mocked_setValue, mocked_value, mocked_contains):
+        """Test that the upgrade mechanism for settings correctly uses the new value when it's not a function"""
+        # GIVEN: A settings object with an upgrade step to take (99, so that we don't interfere with real ones)
+        local_settings = Settings()
+        local_settings.__setting_upgrade_99__ = [
+            ('values/old value', 'values/new value', [(True, 1)])
+        ]
+        settings.__version__ = 99
+        mocked_value.side_effect = [98, 1]
+        mocked_contains.return_value = True
+
+        # WHEN: upgrade_settings() is called
+        local_settings.upgrade_settings()
+
+        # THEN: The correct calls should have been made with the correct values
+        assert mocked_value.call_count == 2, 'Settings().value() should have been called twice'
+        assert mocked_value.call_args_list == [call('settings/version', 0), call('values/old value')]
+        assert mocked_setValue.call_count == 2, 'Settings().setValue() should have been called twice'
+        assert mocked_setValue.call_args_list == [call('values/new value', True), call('settings/version', 99)]
+        mocked_contains.assert_called_once_with('values/old value')
+        mocked_remove.assert_called_once_with('values/old value')
+
+    @patch('openlp.core.common.settings.QtCore.QSettings.contains')
+    @patch('openlp.core.common.settings.QtCore.QSettings.value')
+    @patch('openlp.core.common.settings.QtCore.QSettings.setValue')
+    @patch('openlp.core.common.settings.QtCore.QSettings.remove')
+    def test_upgrade_multiple_one_invalid(self, mocked_remove, mocked_setValue, mocked_value, mocked_contains):
+        """Test that the upgrade mechanism for settings works correctly for multiple values where one is invalid"""
+        # GIVEN: A settings object with an upgrade step to take
+        local_settings = Settings()
+        local_settings.__setting_upgrade_99__ = [
+            (['multiple/value 1', 'multiple/value 2'], 'single/new value', [])
+        ]
+        settings.__version__ = 99
+        mocked_value.side_effect = [98, 10]
+        mocked_contains.side_effect = [True, False]
+
+        # WHEN: upgrade_settings() is called
+        local_settings.upgrade_settings()
+
+        # THEN: The correct calls should have been made with the correct values
+        mocked_value.assert_called_once_with('settings/version', 0)
+        mocked_setValue.assert_called_once_with('settings/version', 99)
+        assert mocked_contains.call_args_list == [call('multiple/value 1'), call('multiple/value 2')]
+
+    def test_can_upgrade(self):
+        """Test the Settings.can_upgrade() method"""
+        # GIVEN: A Settings object
+        local_settings = Settings()
+
+        # WHEN: can_upgrade() is run
+        result = local_settings.can_upgrade()
+
+        # THEN: The result should be True
+        assert result is True, 'The settings should be upgradeable'
+
+    def test_convert_value_setting_none_str(self):
+        """Test the Settings._convert_value() method when a setting is None and the default value is a string"""
+        # GIVEN: A settings object
+        # WHEN: _convert_value() is run
+        result = Settings()._convert_value(None, 'string')
+
+        # THEN: The result should be an empty string
+        assert result == '', 'The result should be an empty string'
+
+    def test_convert_value_setting_none_list(self):
+        """Test the Settings._convert_value() method when a setting is None and the default value is a list"""
+        # GIVEN: A settings object
+        # WHEN: _convert_value() is run
+        result = Settings()._convert_value(None, [None])
+
+        # THEN: The result should be an empty list
+        assert result == [], 'The result should be an empty list'
+
+    def test_convert_value_setting_json_Path(self):
+        """Test the Settings._convert_value() method when a setting is JSON and represents a Path object"""
+        # GIVEN: A settings object
+        # WHEN: _convert_value() is run
+        result = Settings()._convert_value('{"__Path__": ["openlp", "core"]}', None)
+
+        # THEN: The result should be a Path object
+        assert isinstance(result, Path), 'The result should be a Path object'
+
+    def test_convert_value_setting_bool_str(self):
+        """Test the Settings._convert_value() method when a setting is supposed to be a boolean"""
+        # GIVEN: A settings object
+        # WHEN: _convert_value() is run
+        result = Settings()._convert_value('false', True)
+
+        # THEN: The result should be False
+        assert result is False, 'The result should be False'

=== modified file 'tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py'
--- tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py	2017-10-23 22:09:57 +0000
+++ tests/interfaces/openlp_core/ui/media/vendor/test_mediainfoWrapper.py	2017-11-16 05:12:46 +0000
@@ -29,7 +29,6 @@
 from openlp.core.ui.media.vendor.mediainfoWrapper import MediaInfoWrapper
 
 TEST_PATH = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', '..', '..', '..', '..', 'resources', 'media'))
-
 TEST_MEDIA = [['avi_file.avi', 61495], ['mp3_file.mp3', 134426], ['mpg_file.mpg', 9404], ['mp4_file.mp4', 188336]]