Merge lp:~raoul-snyman/openlp/bug-1451237 into lp:openlp

[Raoul Snyman]

Fix bug#1451237 by overriding the create_item_from_id() method in the Images MediaItem.

Add this to your merge proposal:
--------------------------------
lp:~raoul-snyman/openlp/bug-1451237 (revision 2537)
[SUCCESS] https//ci.openlp.io/job/Branch-01-Pull/1019/
[SUCCESS] https//ci.openlp.io/job/Branch-02-Functional-Tests/942/
[SUCCESS] https//ci.openlp.io/job/Branch-03-Interface-Tests/884/
[SUCCESS] https//ci.openlp.io/job/Branch-04a-Windows_Functional_Tests/771/
[SUCCESS] https//ci.openlp.io/job/Branch-04b-Windows_Interface_Tests/369/
[SUCCESS] https//ci.openlp.io/job/Branch-05a-Code_Analysis/501/
[SUCCESS] https//ci.openlp.io/job/Branch-05b-Test_Coverage/372/

[Tim Bentley (trb143)]

review: Approve

=== modified file 'openlp/plugins/images/lib/mediaitem.py'
--- openlp/plugins/images/lib/mediaitem.py	2015-04-22 19:37:32 +0000
+++ openlp/plugins/images/lib/mediaitem.py	2015-05-17 21:27:07 +0000
@@ -551,6 +551,7 @@
             service_item.title = items[0].text(0)
         else:
             service_item.title = str(self.plugin.name_strings['plural'])
+
         service_item.add_capability(ItemCapabilities.CanMaintain)
         service_item.add_capability(ItemCapabilities.CanPreview)
         service_item.add_capability(ItemCapabilities.CanLoop)
@@ -697,3 +698,15 @@
             filename = os.path.split(str(file_object.filename))[1]
             results.append([file_object.filename, filename])
         return results
+
+    def create_item_from_id(self, item_id):
+        """
+        Create a media item from an item id. Overridden from the parent method to change the item type.
+
+        :param item_id: Id to make live
+        """
+        item = QtGui.QTreeWidgetItem()
+        item_data = self.manager.get_object_filtered(ImageFilenames, ImageFilenames.filename == item_id)
+        item.setText(0, os.path.basename(item_data.filename))
+        item.setData(0, QtCore.Qt.UserRole, item_data)
+        return item

=== modified file 'tests/functional/openlp_plugins/images/test_lib.py'
--- tests/functional/openlp_plugins/images/test_lib.py	2015-04-22 21:05:46 +0000
+++ tests/functional/openlp_plugins/images/test_lib.py	2015-05-17 21:27:07 +0000
@@ -24,6 +24,8 @@
 """
 from unittest import TestCase
 
+from PyQt4 import QtCore, QtGui
+
 from openlp.core.common import Registry
 from openlp.plugins.images.lib.db import ImageFilenames, ImageGroups
 from openlp.plugins.images.lib.mediaitem import ImageMediaItem
@@ -48,123 +50,121 @@
             self.media_item = ImageMediaItem(None, mocked_plugin)
             self.media_item.settings_section = 'images'
 
-    def validate_and_load_test(self):
+    @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_list')
+    @patch('openlp.plugins.images.lib.mediaitem.Settings')
+    def validate_and_load_test(self, mocked_settings, mocked_load_list):
         """
         Test that the validate_and_load_test() method when called without a group
         """
         # GIVEN: A list of files
         file_list = ['/path1/image1.jpg', '/path2/image2.jpg']
 
-        with patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_list') as mocked_load_list, \
-                patch('openlp.plugins.images.lib.mediaitem.Settings') as mocked_settings:
-
-            # WHEN: Calling validate_and_load with the list of files
-            self.media_item.validate_and_load(file_list)
-
-            # THEN: load_list should have been called with the file list and None,
-            #       the directory should have been saved to the settings
-            mocked_load_list.assert_called_once_with(file_list, None)
-            mocked_settings().setValue.assert_called_once_with(ANY, '/path1')
-
-    def validate_and_load_group_test(self):
+        # WHEN: Calling validate_and_load with the list of files
+        self.media_item.validate_and_load(file_list)
+
+        # THEN: load_list should have been called with the file list and None,
+        #       the directory should have been saved to the settings
+        mocked_load_list.assert_called_once_with(file_list, None)
+        mocked_settings().setValue.assert_called_once_with(ANY, '/path1')
+
+    @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_list')
+    @patch('openlp.plugins.images.lib.mediaitem.Settings')
+    def validate_and_load_group_test(self, mocked_settings, mocked_load_list):
         """
         Test that the validate_and_load_test() method when called with a group
         """
         # GIVEN: A list of files
         file_list = ['/path1/image1.jpg', '/path2/image2.jpg']
 
-        with patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_list') as mocked_load_list, \
-                patch('openlp.plugins.images.lib.mediaitem.Settings') as mocked_settings:
-
-            # WHEN: Calling validate_and_load with the list of files and a group
-            self.media_item.validate_and_load(file_list, 'group')
-
-            # THEN: load_list should have been called with the file list and the group name,
-            #       the directory should have been saved to the settings
-            mocked_load_list.assert_called_once_with(file_list, 'group')
-            mocked_settings().setValue.assert_called_once_with(ANY, '/path1')
-
-    def save_new_images_list_empty_list_test(self):
+        # WHEN: Calling validate_and_load with the list of files and a group
+        self.media_item.validate_and_load(file_list, 'group')
+
+        # THEN: load_list should have been called with the file list and the group name,
+        #       the directory should have been saved to the settings
+        mocked_load_list.assert_called_once_with(file_list, 'group')
+        mocked_settings().setValue.assert_called_once_with(ANY, '/path1')
+
+    @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list')
+    def save_new_images_list_empty_list_test(self, mocked_load_full_list):
         """
         Test that the save_new_images_list() method handles empty lists gracefully
         """
         # GIVEN: An empty image_list
         image_list = []
-        with patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list'):
-            self.media_item.manager = MagicMock()
-
-            # WHEN: We run save_new_images_list with the empty list
-            self.media_item.save_new_images_list(image_list)
-
-            # THEN: The save_object() method should not have been called
-            self.assertEquals(self.media_item.manager.save_object.call_count, 0,
-                              'The save_object() method should not have been called')
-
-    def save_new_images_list_single_image_with_reload_test(self):
+        self.media_item.manager = MagicMock()
+
+        # WHEN: We run save_new_images_list with the empty list
+        self.media_item.save_new_images_list(image_list)
+
+        # THEN: The save_object() method should not have been called
+        self.assertEquals(self.media_item.manager.save_object.call_count, 0,
+                          'The save_object() method should not have been called')
+
+    @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list')
+    def save_new_images_list_single_image_with_reload_test(self, mocked_load_full_list):
         """
         Test that the save_new_images_list() calls load_full_list() when reload_list is set to True
         """
         # GIVEN: A list with 1 image and a mocked out manager
         image_list = ['test_image.jpg']
-        with patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list') as mocked_load_full_list:
-            ImageFilenames.filename = ''
-            self.media_item.manager = MagicMock()
-
-            # WHEN: We run save_new_images_list with reload_list=True
-            self.media_item.save_new_images_list(image_list, reload_list=True)
-
-            # THEN: load_full_list() should have been called
-            self.assertEquals(mocked_load_full_list.call_count, 1, 'load_full_list() should have been called')
-
-            # CLEANUP: Remove added attribute from ImageFilenames
-            delattr(ImageFilenames, 'filename')
-
-    def save_new_images_list_single_image_without_reload_test(self):
+        ImageFilenames.filename = ''
+        self.media_item.manager = MagicMock()
+
+        # WHEN: We run save_new_images_list with reload_list=True
+        self.media_item.save_new_images_list(image_list, reload_list=True)
+
+        # THEN: load_full_list() should have been called
+        self.assertEquals(mocked_load_full_list.call_count, 1, 'load_full_list() should have been called')
+
+        # CLEANUP: Remove added attribute from ImageFilenames
+        delattr(ImageFilenames, 'filename')
+
+    @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list')
+    def save_new_images_list_single_image_without_reload_test(self, mocked_load_full_list):
         """
         Test that the save_new_images_list() doesn't call load_full_list() when reload_list is set to False
         """
         # GIVEN: A list with 1 image and a mocked out manager
         image_list = ['test_image.jpg']
-        with patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list') as mocked_load_full_list:
-            self.media_item.manager = MagicMock()
-
-            # WHEN: We run save_new_images_list with reload_list=False
-            self.media_item.save_new_images_list(image_list, reload_list=False)
-
-            # THEN: load_full_list() should not have been called
-            self.assertEquals(mocked_load_full_list.call_count, 0, 'load_full_list() should not have been called')
-
-    def save_new_images_list_multiple_images_test(self):
+        self.media_item.manager = MagicMock()
+
+        # WHEN: We run save_new_images_list with reload_list=False
+        self.media_item.save_new_images_list(image_list, reload_list=False)
+
+        # THEN: load_full_list() should not have been called
+        self.assertEquals(mocked_load_full_list.call_count, 0, 'load_full_list() should not have been called')
+
+    @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list')
+    def save_new_images_list_multiple_images_test(self, mocked_load_full_list):
         """
         Test that the save_new_images_list() saves all images in the list
         """
         # GIVEN: A list with 3 images
         image_list = ['test_image_1.jpg', 'test_image_2.jpg', 'test_image_3.jpg']
-        with patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list') as mocked_load_full_list:
-            self.media_item.manager = MagicMock()
-
-            # WHEN: We run save_new_images_list with the list of 3 images
-            self.media_item.save_new_images_list(image_list, reload_list=False)
-
-            # THEN: load_full_list() should not have been called
-            self.assertEquals(self.media_item.manager.save_object.call_count, 3,
-                              'load_full_list() should have been called three times')
-
-    def save_new_images_list_other_objects_in_list_test(self):
+        self.media_item.manager = MagicMock()
+
+        # WHEN: We run save_new_images_list with the list of 3 images
+        self.media_item.save_new_images_list(image_list, reload_list=False)
+
+        # THEN: load_full_list() should not have been called
+        self.assertEquals(self.media_item.manager.save_object.call_count, 3,
+                          'load_full_list() should have been called three times')
+
+    @patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list')
+    def save_new_images_list_other_objects_in_list_test(self, mocked_load_full_list):
         """
         Test that the save_new_images_list() ignores everything in the provided list except strings
         """
         # GIVEN: A list with images and objects
         image_list = ['test_image_1.jpg', None, True, ImageFilenames(), 'test_image_2.jpg']
-        with patch('openlp.plugins.images.lib.mediaitem.ImageMediaItem.load_full_list') as mocked_load_full_list:
-            self.media_item.manager = MagicMock()
-
-            # WHEN: We run save_new_images_list with the list of images and objects
-            self.media_item.save_new_images_list(image_list, reload_list=False)
-
-            # THEN: load_full_list() should not have been called
-            self.assertEquals(self.media_item.manager.save_object.call_count, 2,
-                              'load_full_list() should have been called only once')
+        self.media_item.manager = MagicMock()
+
+        # WHEN: We run save_new_images_list with the list of images and objects
+        self.media_item.save_new_images_list(image_list, reload_list=False)
+
+        # THEN: load_full_list() should not have been called
+        self.assertEquals(self.media_item.manager.save_object.call_count, 2,
+                          'load_full_list() should have been called only once')
 
     def on_reset_click_test(self):
         """
@@ -180,31 +180,31 @@
         self.media_item.reset_action.setVisible.assert_called_with(False)
         self.media_item.live_controller.display.reset_image.assert_called_with()
 
-    def recursively_delete_group_test(self):
+    @patch('openlp.plugins.images.lib.mediaitem.delete_file')
+    def recursively_delete_group_test(self, mocked_delete_file):
         """
         Test that recursively_delete_group() works
         """
         # GIVEN: An ImageGroups object and mocked functions
-        with patch('openlp.plugins.images.lib.mediaitem.delete_file') as mocked_delete_file:
-            ImageFilenames.group_id = 1
-            ImageGroups.parent_id = 1
-            self.media_item.manager = MagicMock()
-            self.media_item.manager.get_all_objects.side_effect = self._recursively_delete_group_side_effect
-            self.media_item.service_path = ''
-            test_group = ImageGroups()
-            test_group.id = 1
-
-            # WHEN: recursively_delete_group() is called
-            self.media_item.recursively_delete_group(test_group)
-
-            # THEN: delete_file() should have been called 12 times and manager.delete_object() 7 times.
-            self.assertEquals(mocked_delete_file.call_count, 12, 'delete_file() should have been called 12 times')
-            self.assertEquals(self.media_item.manager.delete_object.call_count, 7,
-                              'manager.delete_object() should be called exactly 7 times')
-
-            # CLEANUP: Remove added attribute from Image Filenames and ImageGroups
-            delattr(ImageFilenames, 'group_id')
-            delattr(ImageGroups, 'parent_id')
+        ImageFilenames.group_id = 1
+        ImageGroups.parent_id = 1
+        self.media_item.manager = MagicMock()
+        self.media_item.manager.get_all_objects.side_effect = self._recursively_delete_group_side_effect
+        self.media_item.service_path = ''
+        test_group = ImageGroups()
+        test_group.id = 1
+
+        # WHEN: recursively_delete_group() is called
+        self.media_item.recursively_delete_group(test_group)
+
+        # THEN: delete_file() should have been called 12 times and manager.delete_object() 7 times.
+        self.assertEquals(mocked_delete_file.call_count, 12, 'delete_file() should have been called 12 times')
+        self.assertEquals(self.media_item.manager.delete_object.call_count, 7,
+                          'manager.delete_object() should be called exactly 7 times')
+
+        # CLEANUP: Remove added attribute from Image Filenames and ImageGroups
+        delattr(ImageFilenames, 'group_id')
+        delattr(ImageGroups, 'parent_id')
 
     def _recursively_delete_group_side_effect(*args, **kwargs):
         """
@@ -231,28 +231,51 @@
             return [returned_object1]
         return []
 
-    def on_delete_click_test(self):
+    @patch('openlp.plugins.images.lib.mediaitem.delete_file')
+    @patch('openlp.plugins.images.lib.mediaitem.check_item_selected')
+    def on_delete_click_test(self, mocked_check_item_selected, mocked_delete_file):
         """
         Test that on_delete_click() works
         """
         # GIVEN: An ImageGroups object and mocked functions
-        with patch('openlp.plugins.images.lib.mediaitem.delete_file') as mocked_delete_file, \
-                patch('openlp.plugins.images.lib.mediaitem.check_item_selected') as mocked_check_item_selected:
-            mocked_check_item_selected.return_value = True
-            test_image = ImageFilenames()
-            test_image.id = 1
-            test_image.group_id = 1
-            test_image.filename = 'imagefile.png'
-            self.media_item.manager = MagicMock()
-            self.media_item.service_path = ''
-            self.media_item.list_view = MagicMock()
-            mocked_row_item = MagicMock()
-            mocked_row_item.data.return_value = test_image
-            mocked_row_item.text.return_value = ''
-            self.media_item.list_view.selectedItems.return_value = [mocked_row_item]
-
-            # WHEN: Calling on_delete_click
-            self.media_item.on_delete_click()
-
-            # THEN: delete_file should have been called twice
-            self.assertEquals(mocked_delete_file.call_count, 2, 'delete_file() should have been called twice')
+        mocked_check_item_selected.return_value = True
+        test_image = ImageFilenames()
+        test_image.id = 1
+        test_image.group_id = 1
+        test_image.filename = 'imagefile.png'
+        self.media_item.manager = MagicMock()
+        self.media_item.service_path = ''
+        self.media_item.list_view = MagicMock()
+        mocked_row_item = MagicMock()
+        mocked_row_item.data.return_value = test_image
+        mocked_row_item.text.return_value = ''
+        self.media_item.list_view.selectedItems.return_value = [mocked_row_item]
+
+        # WHEN: Calling on_delete_click
+        self.media_item.on_delete_click()
+
+        # THEN: delete_file should have been called twice
+        self.assertEquals(mocked_delete_file.call_count, 2, 'delete_file() should have been called twice')
+
+    def create_item_from_id_test(self):
+        """
+        Test that the create_item_from_id() method returns a valid QTreeWidgetItem with a pre-created ImageFilenames
+        """
+        # GIVEN: An ImageFilenames that already exists in the database
+        image_file = ImageFilenames()
+        image_file.id = 1
+        image_file.filename = '/tmp/test_file_1.jpg'
+        self.media_item.manager = MagicMock()
+        self.media_item.manager.get_object_filtered.return_value = image_file
+        ImageFilenames.filename = ''
+
+        # WHEN: create_item_from_id() is called
+        item = self.media_item.create_item_from_id(1)
+
+        # THEN: A QTreeWidgetItem should be created with the above model object as it's data
+        self.assertIsInstance(item, QtGui.QTreeWidgetItem)
+        self.assertEqual('test_file_1.jpg', item.text(0))
+        item_data = item.data(0, QtCore.Qt.UserRole)
+        self.assertIsInstance(item_data, ImageFilenames)
+        self.assertEqual(1, item_data.id)
+        self.assertEqual('/tmp/test_file_1.jpg', item_data.filename)