openlp-core team mailing list archive

[Tim Bentley <tim.bentley@xxxxxxxxx>]

Review: Needs Fixing

Actually 3 queries!

Diff comments:

> 
> === modified file 'openlp/core/common/settings.py'
> --- openlp/core/common/settings.py	2018-05-03 14:58:50 +0000
> +++ openlp/core/common/settings.py	2018-06-08 21:20:10 +0000
> @@ -116,6 +124,11 @@
>          'advanced/print file meta data': False,
>          'advanced/print notes': False,
>          'advanced/print slide text': False,
> +        'advanced/proxy mode': ProxyMode.SYSTEM_PROXY,

Why are we defaulting to System?  My machines all have manual and this is the default.
Will not people need to set this?

> +        'advanced/proxy http': '',
> +        'advanced/proxy https': '',
> +        'advanced/proxy username': '',
> +        'advanced/proxy password': '',
>          'advanced/recent file count': 4,
>          'advanced/save current plugin': False,
>          'advanced/slide limits': SlideLimits.End,
> 
> === added file 'openlp/core/widgets/widgets.py'
> --- openlp/core/widgets/widgets.py	1970-01-01 00:00:00 +0000
> +++ openlp/core/widgets/widgets.py	2018-06-08 21:20:10 +0000
> @@ -0,0 +1,134 @@
> +# -*- coding: utf-8 -*-
> +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
> +
> +###############################################################################
> +# OpenLP - Open Source Lyrics Projection                                      #
> +# --------------------------------------------------------------------------- #
> +# Copyright (c) 2008-2018 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.widgets.widgets` module contains custom widgets used in OpenLP
> +"""
> +from PyQt5 import QtWidgets
> +
> +from openlp.core.common.i18n import translate
> +from openlp.core.common.settings import ProxyMode, Settings
> +
> +
> +class ProxyWidget(QtWidgets.QGroupBox):
> +    """
> +    A proxy settings widget that implements loading and saving its settings.
> +    """
> +    def __init__(self, parent=None):
> +        """
> +        Initialise the widget.
> +
> +        :param QtWidgets.QWidget | None parent: The widgets parent
> +        """
> +        super().__init__(parent)
> +        self._setup()
> +
> +    def _setup(self):
> +        """
> +        A setup method seperate from __init__ to allow easier testing
> +        """
> +        self.setup_ui()
> +        self.load()
> +
> +    def setup_ui(self):
> +        """
> +        Create the widget layout and sub widgets
> +        """
> +        self.layout = QtWidgets.QFormLayout(self)
> +        self.radio_group = QtWidgets.QButtonGroup(self)
> +        self.no_proxy_radio = QtWidgets.QRadioButton('', self)
> +        self.radio_group.addButton(self.no_proxy_radio, ProxyMode.NO_PROXY)
> +        self.layout.setWidget(0, QtWidgets.QFormLayout.SpanningRole, self.no_proxy_radio)
> +        self.use_sysem_proxy_radio = QtWidgets.QRadioButton('', self)
> +        self.radio_group.addButton(self.use_sysem_proxy_radio, ProxyMode.SYSTEM_PROXY)
> +        self.layout.setWidget(1, QtWidgets.QFormLayout.SpanningRole, self.use_sysem_proxy_radio)
> +        self.manual_proxy_radio = QtWidgets.QRadioButton('', self)
> +        self.radio_group.addButton(self.manual_proxy_radio, ProxyMode.MANUAL_PROXY)
> +        self.layout.setWidget(2, QtWidgets.QFormLayout.SpanningRole, self.manual_proxy_radio)
> +        self.http_edit = QtWidgets.QLineEdit(self)
> +        self.layout.addRow('HTTP:', self.http_edit)
> +        self.https_edit = QtWidgets.QLineEdit(self)
> +        self.layout.addRow('HTTPS:', self.https_edit)
> +        self.username_edit = QtWidgets.QLineEdit(self)
> +        self.layout.addRow('Username:', self.username_edit)
> +        self.password_edit = QtWidgets.QLineEdit(self)
> +        self.password_edit.setEchoMode(QtWidgets.QLineEdit.Password)
> +        self.layout.addRow('Password:', self.password_edit)
> +        # Signal / Slots
> +        self.radio_group.buttonToggled.connect(self.on_radio_group_button_toggled)
> +
> +    # @QtCore.pyqtSlot(int, bool)  For some reason PyQt doesn't think this signature exists.
> +    #                              (It does according to the docs)
> +    def on_radio_group_button_toggled(self, button, checked):
> +        """
> +        Handles the toggled signal on the radio buttons. The signal is emitted twice if a radio butting being toggled on
> +        causes another radio button in the group to be toggled off.
> +
> +        En/Disables the `Manual Proxy` line edits depending on the currently selected radio button
> +
> +        :param QtWidgets.QRadioButton button: The button that has toggled
> +        :param bool checked: The buttons new state
> +        """
> +        id = self.radio_group.id(button)  # The work around (see above comment)
> +        enable_manual_edits = id == ProxyMode.MANUAL_PROXY and checked
> +        self.http_edit.setEnabled(enable_manual_edits)
> +        self.https_edit.setEnabled(enable_manual_edits)
> +        self.username_edit.setEnabled(enable_manual_edits)
> +        self.password_edit.setEnabled(enable_manual_edits)
> +
> +    def retranslate_ui(self):
> +        """
> +        Translate the Ui
> +        """
> +        self.setTitle(translate('OpenLP.ProxyWidget', 'Proxy Server Settings'))
> +        self.no_proxy_radio.setText(translate('OpenLP.ProxyWidget', 'No prox&y'))
> +        self.use_sysem_proxy_radio.setText(translate('OpenLP.ProxyWidget', '&Use system proxy'))
> +        self.manual_proxy_radio.setText(translate('OpenLP.ProxyWidget', '&Manual proxy configuration'))
> +        proxy_example = translate('OpenLP.ProxyWidget', 'e.g. proxy_server_address:port_no')
> +        self.layout.labelForField(self.http_edit).setText('HTTP:')
> +        self.http_edit.setPlaceholderText(proxy_example)
> +        self.layout.labelForField(self.https_edit).setText('HTTPS:')
> +        self.https_edit.setPlaceholderText(proxy_example)
> +        self.layout.labelForField(self.username_edit).setText('Username:')

Translate?

> +        self.layout.labelForField(self.password_edit).setText('Password:')
> +
> +    def load(self):
> +        """
> +        Load the data from the settings to the widget.
> +        """
> +        settings = Settings()
> +        checked_radio = self.radio_group.button(settings.value('advanced/proxy mode'))
> +        checked_radio.setChecked(True)
> +        self.http_edit.setText(settings.value('advanced/proxy http'))
> +        self.https_edit.setText(settings.value('advanced/proxy https'))
> +        self.username_edit.setText(settings.value('advanced/proxy username'))
> +        self.password_edit.setText(settings.value('advanced/proxy password'))
> +
> +    def save(self):
> +        """
> +        Save the widget data to the settings
> +        """
> +        settings = Settings()  # TODO: Migrate from old system
> +        settings.setValue('advanced/proxy mode', self.radio_group.checkedId())
> +        settings.setValue('advanced/proxy http', self.http_edit.text())
> +        settings.setValue('advanced/proxy https', self.https_edit.text())
> +        settings.setValue('advanced/proxy username', self.username_edit.text())
> +        settings.setValue('advanced/proxy password', self.password_edit.text())
> 
> === added file 'tests/interfaces/openlp_core/widgets/test_widgets.py'
> --- tests/interfaces/openlp_core/widgets/test_widgets.py	1970-01-01 00:00:00 +0000
> +++ tests/interfaces/openlp_core/widgets/test_widgets.py	2018-06-08 21:20:10 +0000
> @@ -0,0 +1,155 @@
> +# -*- coding: utf-8 -*-
> +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
> +
> +###############################################################################
> +# OpenLP - Open Source Lyrics Projection                                      #
> +# --------------------------------------------------------------------------- #
> +# Copyright (c) 2008-2018 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                          #
> +###############################################################################
> +"""
> +Module to test the custom widgets.
> +"""
> +from unittest import TestCase
> +from unittest.mock import MagicMock, call, patch
> +
> +from openlp.core.common.registry import Registry
> +from openlp.core.common.settings import ProxyMode
> +from openlp.core.widgets.widgets import ProxyWidget
> +from tests.helpers.testmixin import TestMixin
> +
> +
> +class TestProxyWidget(TestCase, TestMixin):
> +    """
> +    Test the EditCustomForm.
> +    """
> +    def setUp(self):
> +        """
> +        Create the UI
> +        """
> +        Registry.create()
> +        self.setup_application()
> +
> +    def test_radio_button_exclusivity(self):

Normally this should be one method per test.
ARe these chained tests?

> +        """
> +        Test that only one radio button can be checked at a time, and that the line edits are only enabled when the
> +        `manual_proxy_radio` is checked
> +        """
> +        # GIVEN: An instance of the `openlp.core.common.widgets.widgets.ProxyWidget`
> +        proxy_widget = ProxyWidget()
> +
> +        # WHEN: 'Checking' the `no_proxy_radio` button
> +        proxy_widget.no_proxy_radio.setChecked(True)
> +
> +        # THEN: The other radio buttons should not be checked and the line edits should not be enabled
> +        assert proxy_widget.use_sysem_proxy_radio.isChecked() is False
> +        assert proxy_widget.manual_proxy_radio.isChecked() is False
> +        assert proxy_widget.http_edit.isEnabled() is False
> +        assert proxy_widget.https_edit.isEnabled() is False
> +        assert proxy_widget.username_edit.isEnabled() is False
> +        assert proxy_widget.password_edit.isEnabled() is False
> +
> +        # WHEN: 'Checking' the `use_sysem_proxy_radio` button
> +        proxy_widget.use_sysem_proxy_radio.setChecked(True)
> +
> +        # THEN: The other radio buttons should not be checked and the line edits should not be enabled
> +        assert proxy_widget.no_proxy_radio.isChecked() is False
> +        assert proxy_widget.manual_proxy_radio.isChecked() is False
> +        assert proxy_widget.http_edit.isEnabled() is False
> +        assert proxy_widget.https_edit.isEnabled() is False
> +        assert proxy_widget.username_edit.isEnabled() is False
> +        assert proxy_widget.password_edit.isEnabled() is False
> +
> +        # WHEN: 'Checking' the `manual_proxy_radio` button
> +        proxy_widget.manual_proxy_radio.setChecked(True)
> +
> +        # THEN: The other radio buttons should not be checked and the line edits should be enabled
> +        assert proxy_widget.no_proxy_radio.isChecked() is False
> +        assert proxy_widget.use_sysem_proxy_radio.isChecked() is False
> +        assert proxy_widget.http_edit.isEnabled() is True
> +        assert proxy_widget.https_edit.isEnabled() is True
> +        assert proxy_widget.username_edit.isEnabled() is True
> +        assert proxy_widget.password_edit.isEnabled() is True
> +
> +    def test_proxy_widget_load_default_settings(self):
> +        """
> +        Test that the default settings are loaded from the config correctly
> +        """
> +        # GIVEN: And instance of the widget with default settings
> +        proxy_widget = ProxyWidget()
> +
> +        # WHEN: Calling the `load` method
> +        proxy_widget.load()
> +
> +        # THEN: The widget should be in its default state
> +        assert proxy_widget.use_sysem_proxy_radio.isChecked() is True
> +        assert proxy_widget.http_edit.text() == ''
> +        assert proxy_widget.https_edit.text() == ''
> +        assert proxy_widget.username_edit.text() == ''
> +        assert proxy_widget.password_edit.text() == ''
> +
> +    @patch.object(ProxyWidget, 'load')
> +    @patch('openlp.core.widgets.widgets.Settings')
> +    def test_proxy_widget_save_no_proxy_settings(self, settings_patcher, proxy_widget_load_patcher):
> +        """
> +        Test that the settings are saved correctly
> +        """
> +        # GIVEN: A Mocked settings instance of the proxy widget with some known values set
> +        settings_instance = MagicMock()
> +        settings_patcher.return_value = settings_instance
> +        proxy_widget = ProxyWidget()
> +        proxy_widget.no_proxy_radio.setChecked(True)
> +        proxy_widget.http_edit.setText('')
> +        proxy_widget.https_edit.setText('')
> +        proxy_widget.username_edit.setText('')
> +        proxy_widget.password_edit.setText('')
> +
> +        # WHEN: Calling save
> +        proxy_widget.save()
> +
> +        # THEN: The settings should be set as expected
> +        settings_instance.setValue.assert_has_calls(
> +            [call('advanced/proxy mode', ProxyMode.NO_PROXY),
> +             call('advanced/proxy http', ''),
> +             call('advanced/proxy https', ''),
> +             call('advanced/proxy username', ''),
> +             call('advanced/proxy password', '')])
> +
> +    @patch.object(ProxyWidget, 'load')
> +    @patch('openlp.core.widgets.widgets.Settings')
> +    def test_proxy_widget_save_manual_settings(self, settings_patcher, proxy_widget_load_patcher):
> +        """
> +        Test that the settings are saved correctly
> +        """
> +        # GIVEN: A Mocked and instance of the proxy widget with some known values set
> +        settings_instance = MagicMock()
> +        settings_patcher.return_value = settings_instance
> +        proxy_widget = ProxyWidget()
> +        proxy_widget.manual_proxy_radio.setChecked(True)
> +        proxy_widget.http_edit.setText('http_proxy_server:port')
> +        proxy_widget.https_edit.setText('https_proxy_server:port')
> +        proxy_widget.username_edit.setText('username')
> +        proxy_widget.password_edit.setText('password')
> +
> +        # WHEN: Calling save
> +        proxy_widget.save()
> +
> +        # THEN: The settings should be set as expected
> +        settings_instance.setValue.assert_has_calls(
> +            [call('advanced/proxy mode', ProxyMode.MANUAL_PROXY),
> +             call('advanced/proxy http', 'http_proxy_server:port'),
> +             call('advanced/proxy https', 'https_proxy_server:port'),
> +             call('advanced/proxy username', 'username'),
> +             call('advanced/proxy password', 'password')])


-- 
https://code.launchpad.net/~phill-ridout/openlp/proxies/+merge/347720
Your team OpenLP Core is subscribed to branch lp:openlp.