Merge lp:~alisonken1/openlp/bug-1734275 into lp:openlp

[Ken Roberts]

Bugfix 1734275 - PJLink non-standard response to LAMP command

- Fix non-standard LAMP response (bug 1734275)
- Moved bugfix tests to separate file
- Added test for non-standard LAMP response
- Refactor mocks
- Cleanups

--------------------------------------------------------------------------------
lp:~alisonken1/openlp/bug-1734275 (revision 2792)
https://ci.openlp.io/job/Branch-01-Pull/2312/ [SUCCESS]
https://ci.openlp.io/job/Branch-02-Functional-Tests/2213/ [SUCCESS]
https://ci.openlp.io/job/Branch-03-Interface-Tests/2089/ [SUCCESS]
https://ci.openlp.io/job/Branch-04a-Code_Analysis/1415/ [SUCCESS]
https://ci.openlp.io/job/Branch-04b-Test_Coverage/1237/ [SUCCESS]
https://ci.openlp.io/job/Branch-04c-Code_Analysis2/367/ [SUCCESS]
https://ci.openlp.io/job/Branch-05-AppVeyor-Tests/198/ [FAILURE]

[Phill (phill-ridout)]

A few inline comments, nothing major Sorry if it seems like I be tripping on my new status! :-p

review: Needs Fixing

[Tim Bentley (trb143)]

review: Approve

[Phill (phill-ridout)]

You've just missed removing the pjlink instance in the test file. (see inline)

[Phill (phill-ridout)]

review: Needs Fixing

[Phill (phill-ridout)]

review: Approve

=== 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 19:16:10 +0000
@@ -672,14 +672,16 @@
                                                                        data=projector.model_filter)
             count = 1
             for item in projector.link.lamp:
+                if item['On'] is None:
+                    status = translate('OpenLP.ProjectorManager', 'Unavailable')
+                elif item['On']:
+                    status = translate('OpenLP.ProjectorManager', 'ON')
+                else:
+                    status = 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=status)
 
                 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 19:16:10 +0000
@@ -402,17 +402,20 @@
         :param data: Lamp(s) status.
         """
         lamps = []
-        data_dict = data.split()
-        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
+        lamp_list = data.split()
+        if len(lamp_list) < 2:
+            lamps.append({'Hours': int(lamp_list[0]), 'On': None})
+        else:
+            while lamp_list:
+                try:
+                    fill = {'Hours': int(lamp_list[0]), 'On': False if lamp_list[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)
+                lamp_list.pop(0)  # Remove lamp hours
+                lamp_list.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 19:16: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-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
+
+
+class TestPJLinkBugs(TestCase):
+    """
+    Tests for the PJLink module bugfixes
+    """
+    def setUp(self):
+        '''
+        Initialization
+        '''
+        self.pjlink_test = PJLink(Projector(**TEST1_DATA), no_poll=True)
+
+    def tearDown(self):
+        '''
+        Cleanups
+        '''
+        self.pjlink_test = None
+
+    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 = self.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 = self.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')
+
+    def test_bug_1593882_no_pin_authenticated_connection(self):
+        """
+        Test bug 1593882 no pin and authenticated request exception
+        """
+        # GIVEN: Test object and mocks
+        mock_socket_timer = patch.object(self.pjlink_test, 'socket_timer').start()
+        mock_timer = patch.object(self.pjlink_test, 'timer').start()
+        mock_authentication = patch.object(self.pjlink_test, 'projectorAuthentication').start()
+        mock_ready_read = patch.object(self.pjlink_test, 'waitForReadyRead').start()
+        mock_send_command = patch.object(self.pjlink_test, 'send_command').start()
+        pjlink = self.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)
+
+    def test_bug_1593883_pjlink_authentication(self):
+        """
+        Test bugfix 1593883 pjlink authentication
+        """
+        # GIVEN: Test object and data
+        mock_socket_timer = patch.object(self.pjlink_test, 'socket_timer').start()
+        mock_timer = patch.object(self.pjlink_test, 'timer').start()
+        mock_send_command = patch.object(self.pjlink_test, 'write').start()
+        mock_state = patch.object(self.pjlink_test, 'state').start()
+        mock_waitForReadyRead = patch.object(self.pjlink_test, 'waitForReadyRead').start()
+        pjlink = self.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(b'{hash}%1CLSS ?\\r')".format(hash=TEST_HASH))
+
+    def test_bug_1734275_process_lamp_nonstandard_reply(self):
+        """
+        Test bugfix 17342785 non-standard LAMP response
+        """
+        # GIVEN: Test object
+        pjlink = self.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 "None"')

=== modified file 'tests/functional/openlp_core/projectors/test_projector_pjlink_base.py'
--- tests/functional/openlp_core/projectors/test_projector_pjlink_base.py	2017-11-16 23:53:53 +0000
+++ tests/functional/openlp_core/projectors/test_projector_pjlink_base.py	2017-11-24 19:16:10 +0000
@@ -29,7 +29,7 @@
 from openlp.core.projectors.db import Projector
 from openlp.core.projectors.pjlink import PJLink
 
-from tests.resources.projector.data import TEST_PIN, TEST_SALT, TEST_CONNECT_AUTHENTICATE, TEST_HASH, TEST1_DATA
+from tests.resources.projector.data import TEST_PIN, TEST_SALT, TEST_CONNECT_AUTHENTICATE, TEST1_DATA
 
 pjlink_test = PJLink(Projector(**TEST1_DATA), no_poll=True)
 
@@ -79,58 +79,6 @@
                                        'change_status should have been called with "{}"'.format(
                                            ERROR_STRING[E_PARAMETER]))
 
-    @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))
-
     @patch.object(pjlink_test, 'disconnect_from_host')
     def test_socket_abort(self, mock_disconnect):
         """

=== modified file 'tests/functional/openlp_core/projectors/test_projector_pjlink_commands.py'
--- tests/functional/openlp_core/projectors/test_projector_pjlink_commands.py	2017-11-16 23:53:53 +0000
+++ tests/functional/openlp_core/projectors/test_projector_pjlink_commands.py	2017-11-24 19:16:10 +0000
@@ -570,35 +570,6 @@
         self.assertEqual(pjlink.pjlink_class, '2',
                          'Projector should have set class=2')
 
-    def test_projector_process_clss_nonstandard_reply_optoma(self):
-        """
-        Bugfix 1550891: CLSS request returns non-standard reply with Optoma 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_projector_process_clss_nonstandard_reply_benq(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(openlp.core.projectors.pjlink, 'log')
     def test_projector_process_clss_invalid_nan(self, mock_log):
         """