Review: Needs Fixing
A few inline comments, nothing major Sorry if it seems like I be tripping on my
new status! :-p
Diff comments:
> === modified file 'openlp/core/projectors/manager.py'
> --- openlp/core/projectors/manager.py 2017-11-10 11:59:38 +0000
> +++ openlp/core/projectors/manager.py 2017-11-24 08:38:36 +0000
> @@ -672,14 +672,16 @@
>
> data=projector.model_filter)
> count = 1
> for item in projector.link.lamp:
> + if item['On'] is None:
> + onoff = translate('OpenLP.ProjectorManager',
> 'Unavailable')
'onoff' should really be 'on_off', though if I'm going to ask you to change it,
could you change it to 'status'?
> + elif item['On']:
> + onoff = translate('OpenLP.ProjectorManager', 'ON')
> + else:
> + onoff = translate('OpenLP.ProjectorManager', 'OFF')
> message += '<b>{title} {count}</b> {status}
> '.format(title=translate('OpenLP.ProjectorManager',
>
> 'Lamp'),
>
> count=count,
> -
> status=translate('OpenLP.ProjectorManager',
> -
> 'ON')
> - if
> item['On']
> - else
> translate('OpenLP.ProjectorManager',
> -
> 'OFF'))
> +
> status=onoff)
>
> message += '<b>{title}</b>: {hours}<br
> />'.format(title=translate('OpenLP.ProjectorManager', 'Hours'),
>
> hours=item['Hours'])
>
> === modified file 'openlp/core/projectors/pjlink.py'
> --- openlp/core/projectors/pjlink.py 2017-11-10 11:59:38 +0000
> +++ openlp/core/projectors/pjlink.py 2017-11-24 08:38:36 +0000
> @@ -403,16 +403,19 @@
> """
> lamps = []
> data_dict = data.split()
I'm assuming that data is a string, or a bytes object? In which case calling
split will create a list rather than a dict. Could you rename this something
more appropriate please?
> - while data_dict:
> - try:
> - fill = {'Hours': int(data_dict[0]), 'On': False if
> data_dict[1] == '0' else True}
> - except ValueError:
> - # In case of invalid entry
> - log.warning('({ip}) process_lamp(): Invalid data
> "{data}"'.format(ip=self.ip, data=data))
> - return
> - lamps.append(fill)
> - data_dict.pop(0) # Remove lamp hours
> - data_dict.pop(0) # Remove lamp on/off
> + if len(data_dict) < 2:
> + lamps.append({'Hours': int(data_dict[0]), 'On': None})
> + else:
> + while data_dict:
> + try:
> + fill = {'Hours': int(data_dict[0]), 'On': False if
> data_dict[1] == '0' else True}
> + except ValueError:
> + # In case of invalid entry
> + log.warning('({ip}) process_lamp(): Invalid data
> "{data}"'.format(ip=self.ip, data=data))
> + return
> + lamps.append(fill)
> + data_dict.pop(0) # Remove lamp hours
> + data_dict.pop(0) # Remove lamp on/off
> self.lamp = lamps
> return
>
>
> === added file
> 'tests/functional/openlp_core/projectors/test_projector_bugfixes_01.py'
> --- tests/functional/openlp_core/projectors/test_projector_bugfixes_01.py
> 1970-01-01 00:00:00 +0000
> +++ tests/functional/openlp_core/projectors/test_projector_bugfixes_01.py
> 2017-11-24 08:38:36 +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-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
> #
> +###############################################################################
> +"""
> +Package to test the openlp.core.projectors.pjlink base package.
> +"""
> +from unittest import TestCase
> +from unittest.mock import patch
> +
> +from openlp.core.projectors.db import Projector
> +from openlp.core.projectors.pjlink import PJLink
> +
> +from tests.resources.projector.data import TEST_PIN,
> TEST_CONNECT_AUTHENTICATE, TEST_HASH, TEST1_DATA
> +
> +pjlink_test = PJLink(Projector(**TEST1_DATA), no_poll=True)
This should be in the 'setUp' method in your 'TestPJLinkBugs' class so that it
is created each time a test method is run. Otherwise one test method may change
the instance, which could affect a different test method passing or failing.
> +
> +
> +class TestPJLinkBugs(TestCase):
> + """
> + Tests for the PJLink module bugfixes
> + """
> + def test_bug_1550891_process_clss_nonstandard_reply_1(self):
> + """
> + Bugfix 1550891: CLSS request returns non-standard reply with
> Optoma/Viewsonic projector
> + """
> + # GIVEN: Test object
> + pjlink = pjlink_test
> +
> + # WHEN: Process non-standard reply
> + pjlink.process_clss('Class 1')
> +
> + # THEN: Projector class should be set with proper value
> + self.assertEqual(pjlink.pjlink_class, '1',
> + 'Non-standard class reply should have set class=1')
> +
> + def test_bug_1550891_process_clss_nonstandard_reply_2(self):
> + """
> + Bugfix 1550891: CLSS request returns non-standard reply with BenQ
> projector
> + """
> + # GIVEN: Test object
> + pjlink = pjlink_test
> +
> + # WHEN: Process non-standard reply
> + pjlink.process_clss('Version2')
> +
> + # THEN: Projector class should be set with proper value
> + # NOTE: At this time BenQ is Class 1, but we're trying a different
> value to verify
> + self.assertEqual(pjlink.pjlink_class, '2',
> + 'Non-standard class reply should have set class=2')
> +
> + @patch.object(pjlink_test, 'send_command')
> + @patch.object(pjlink_test, 'waitForReadyRead')
> + @patch.object(pjlink_test, 'projectorAuthentication')
> + @patch.object(pjlink_test, 'timer')
> + @patch.object(pjlink_test, 'socket_timer')
> + def test_bug_1593882_no_pin_authenticated_connection(self,
> + mock_socket_timer,
> + mock_timer,
> + mock_authentication,
> + mock_ready_read,
> + mock_send_command):
> + """
> + Test bug 1593882 no pin and authenticated request exception
> + """
> + # GIVEN: Test object and mocks
> + pjlink = pjlink_test
> + pjlink.pin = None
> + mock_ready_read.return_value = True
> +
> + # WHEN: call with authentication request and pin not set
> + pjlink.check_login(data=TEST_CONNECT_AUTHENTICATE)
> +
> + # THEN: 'No Authentication' signal should have been sent
> + mock_authentication.emit.assert_called_with(pjlink.ip)
> +
> + @patch.object(pjlink_test, 'waitForReadyRead')
> + @patch.object(pjlink_test, 'state')
> + @patch.object(pjlink_test, '_send_command')
> + @patch.object(pjlink_test, 'timer')
> + @patch.object(pjlink_test, 'socket_timer')
> + def test_bug_1593883_pjlink_authentication(self,
> + mock_socket_timer,
> + mock_timer,
> + mock_send_command,
> + mock_state,
> + mock_waitForReadyRead):
> + """
> + Test bugfix 1593883 pjlink authentication
> + """
> + # GIVEN: Test object and data
> + pjlink = pjlink_test
> + pjlink.pin = TEST_PIN
> + mock_state.return_value = pjlink.ConnectedState
> + mock_waitForReadyRead.return_value = True
> +
> + # WHEN: Athenticated connection is called
> + pjlink.check_login(data=TEST_CONNECT_AUTHENTICATE)
> +
> + # THEN: send_command should have the proper authentication
> + self.assertEqual("{test}".format(test=mock_send_command.call_args),
> + "call(data='{hash}%1CLSS
> ?\\r')".format(hash=TEST_HASH))
> +
> + def test_bug_1734275_pjlink_nonstandard_lamp(self):
> + """
> + Test bugfix 17342785 non-standard LAMP response
> + """
> + # GIVEN: Test object
> + pjlink = pjlink_test
> +
> + # WHEN: Process lamp command called with only hours and no lamp
> power state
> + pjlink.process_lamp("45")
> +
> + # THEN: Lamp should show hours as 45 and lamp power as Unavailable
> + self.assertEqual(len(pjlink.lamp), 1, 'There should only be 1 lamp
> available')
> + self.assertEqual(pjlink.lamp[0]['Hours'], 45, 'Lamp hours should
> have equalled 45')
> + self.assertIsNone(pjlink.lamp[0]['On'], 'Lamp power should be
> "Unavailable"')
--
https://code.launchpad.net/~alisonken1/openlp/bug-1734275/+merge/334224
Your team OpenLP Core is subscribed to branch lp:openlp.
_______________________________________________
Mailing list: https://launchpad.net/~openlp-core
Post to : [email protected]
Unsubscribe : https://launchpad.net/~openlp-core
More help : https://help.launchpad.net/ListHelp
