openlp-core team mailing list archive

[Raoul Snyman <raoul@xxxxxxxxxxx>]

Review: Needs Fixing

A couple of things you need to fix, check out the inline comments.

Diff comments:

> === modified file 'openlp/core/ui/projector/editform.py'
> --- openlp/core/ui/projector/editform.py	2014-12-31 10:58:13 +0000
> +++ openlp/core/ui/projector/editform.py	2015-01-04 17:48:05 +0000
> @@ -132,6 +132,8 @@
>          self.location_label.setText(translate('OpenLP.ProjectorEditForm', 'Location'))
>          self.location_text.setText(self.projector.location)
>          self.notes_label.setText(translate('OpenLP.ProjectorEditForm', 'Notes'))
> +        self.notes_text.selectAll()
> +        self.notes_text.cut()
>          self.notes_text.insertPlainText(self.projector.notes)

This is the wrong way to do this. You're using the clipboard, and the user will have their previous notes in the clipboard, potentially overwriting anything they wanted to actually paste into the edit box. There should be a "clear()" method that you  can use, or else "setText('')"

>  
>  
> @@ -158,7 +160,7 @@
>          self.button_box.helpRequested.connect(self.help_me)
>          self.button_box.rejected.connect(self.cancel_me)
>  
> -    def exec_(self, projector=None):
> +    def exec_(self, projector=None, test=False):
>          if projector is None:
>              self.projector = Projector()
>              self.new_projector = True
> @@ -167,7 +169,8 @@
>              self.new_projector = False
>          self.retranslateUi(self)
>          reply = QDialog.exec_(self)
> -        self.projector = None
> +        if not test:
> +            self.projector = None
>          return reply
>  
>      @pyqtSlot()
> 
> === modified file 'openlp/core/ui/projector/manager.py'
> --- openlp/core/ui/projector/manager.py	2014-12-31 10:58:13 +0000
> +++ openlp/core/ui/projector/manager.py	2015-01-04 17:48:05 +0000
> @@ -562,9 +562,8 @@
>              return
>          self.old_projector = projector
>          projector.link.disconnect_from_host()
> -        record = self.projectordb.get_projector_by_ip(projector.link.ip)
> -        self.projector_form.exec_(record)
> -        new_record = self.projectordb.get_projector_by_id(record.id)
> +        self.projector_form.exec_(projector.db_item)
> +        projector.db_item = self.projectordb.get_projector_by_id(self.old_projector.db_item.id)
>  
>      def on_poweroff_projector(self, opt=None):
>          """
> 
> === added file 'tests/interfaces/openlp_core_ui/test_projectoreditform.py'
> --- tests/interfaces/openlp_core_ui/test_projectoreditform.py	1970-01-01 00:00:00 +0000
> +++ tests/interfaces/openlp_core_ui/test_projectoreditform.py	2015-01-04 17:48:05 +0000
> @@ -0,0 +1,116 @@
> +# -*- coding: utf-8 -*-
> +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
> +
> +###############################################################################
> +# OpenLP - Open Source Lyrics Projection                                      #
> +# --------------------------------------------------------------------------- #
> +# Copyright (c) 2008-2015 Raoul Snyman                                        #
> +# Portions copyright (c) 2008-2015 Tim Bentley, Gerald Britton, Jonathan      #
> +# Corwin, Samuel Findlay, Michael Gorven, Scott Guerrieri, Matthias Hub,      #
> +# Meinert Jordan, Armin Köhler, Erik Lundin, Edwin Lunando, Brian T. Meyer.   #
> +# Joshua Miller, Stevan Pettit, Andreas Preikschat, Mattias Põldaru,          #
> +# Christian Richter, Philip Ridout, Ken Roberts, Simon Scudder,               #
> +# Jeffrey Smith, Maikel Stuivenberg, Martin Thompson, Jon Tibble,             #
> +# Dave Warnock, Frode Woldsund, Martin Zibricky, Patrick Zimmermann           #
> +# --------------------------------------------------------------------------- #
> +# 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                          #
> +###############################################################################
> +"""
> +Interface tests to test the openlp.core.ui.projector.editform.ProjectorEditForm()
> +class and methods.
> +"""
> +
> +import os
> +from unittest import TestCase
> +
> +from openlp.core.common import Registry, Settings
> +from openlp.core.lib.projector.db import Projector, ProjectorDB
> +from openlp.core.ui import ProjectorEditForm
> +
> +from tests.functional import patch
> +from tests.helpers.testmixin import TestMixin
> +from tests.resources.projector.data import TEST1_DATA, TEST2_DATA
> +
> +tmpfile = '/tmp/openlp-test-projectormanager.sql'

This is going to fail on Windows. Use the Python functions to get you a temporary file name.

> +
> +
> +class TestProjectorEditForm(TestCase, TestMixin):
> +    """
> +    Test the methods in the ProjectorEditForm class
> +    """
> +    def setUp(self):
> +        """
> +        Create the UI and setup necessary options
> +
> +        :return: None
> +        """
> +        self.build_settings()
> +        self.setup_application()
> +        Registry.create()
> +        if not hasattr(self, 'projector_form'):
> +            with patch('openlp.core.lib.projector.db.init_url') as mocked_init_url:
> +                mocked_init_url.start()
> +                mocked_init_url.return_value = 'sqlite:///{}'.format(tmpfile)
> +                self.projectordb = ProjectorDB()
> +                if not hasattr(self, 'projector_edit_form'):
> +                    self.projector_form = ProjectorEditForm(projectordb=self.projectordb)
> +
> +    def tearDown(self):
> +        """
> +        Close database session.
> +        Delete all C++ objects at end so we don't segfault.
> +
> +        :return: None
> +        """
> +        self.projectordb.session.close()
> +        del(self.projector_form)
> +        self.destroy_settings()
> +
> +    def edit_form_add_projector_test(self):
> +        """
> +        Test projector edit form with no parameters creates a new entry.
> +
> +        :return: None
> +        """
> +        # GIVEN: Mocked setup
> +        with patch('openlp.core.ui.projector.editform.QDialog.exec_'):
> +
> +            # WHEN: Calling edit form with no parameters
> +            self.projector_form.exec_(test=True)
> +            item = self.projector_form.projector
> +
> +            # THEN: Should be creating a new instance
> +            self.assertTrue(self.projector_form.new_projector,
> +                            'Projector edit form should be marked as a new entry')
> +            self.assertTrue((item.ip is None and item.name is None),
> +                            'Projector edit form should have a new Projector() instance to edit')
> +
> +    def edit_form_edit_projector_test(self):
> +        """
> +        Test projector edit form with existing projector entry
> +
> +        :return:
> +        """
> +        # GIVEN: Mocked setup
> +        with patch('openlp.core.ui.projector.editform.QDialog.exec_'):
> +
> +            # WHEN: Calling edit form with existing projector instance
> +            self.projector_form.exec_(projector=TEST1_DATA, test=True)
> +            item = self.projector_form.projector
> +
> +            # THEN: Should be editing an existing entry
> +            self.assertFalse(self.projector_form.new_projector,
> +                             'Projector edit form should be marked as existing entry')
> +            self.assertTrue((item.ip is TEST1_DATA.ip and item.name is TEST1_DATA.name),
> +                            'Projector edit form should have TEST1_DATA() instance to edit')
> 


-- 
https://code.launchpad.net/~alisonken1/openlp/bug-1386951/+merge/245517
Your team OpenLP Core is subscribed to branch lp:openlp.