Merge branch 'issue-1356' into 'master'

Fix an issue where an item's parent is None

Closes #1365

See merge request openlp/openlp!616

openlp/core/ui/library.py

@@ -23,6 +23,7 @@ Provides additional classes for working in the library
 """
 import os
 from pathlib import Path
+from typing import Any, List, Optional, Union
 
 from PyQt5 import QtCore, QtWidgets
 
@@ -73,7 +74,7 @@ class FolderLibraryItem(MediaManagerItem):
         self.add_folder_action.setText(UiStrings().AddFolder)
         self.add_folder_action.setToolTip(UiStrings().AddFolderDot)
 
-    def create_item_from_id(self, item_id):
+    def create_item_from_id(self, item_id: Any):
         """
         Create a media item from an item id.
 
@@ -126,7 +127,7 @@ class FolderLibraryItem(MediaManagerItem):
                 item = tree_item.data(0, QtCore.Qt.UserRole)
                 if isinstance(item, Item):
                     self.delete_item(item)
-                    if not item.folder_id:
+                    if not item.folder_id or not tree_item.parent():
                         self.list_view.takeTopLevelItem(self.list_view.indexOfTopLevelItem(tree_item))
                     else:
                         tree_item.parent().removeChild(tree_item)
@@ -225,7 +226,7 @@ class FolderLibraryItem(MediaManagerItem):
         self.add_folder_action = self.toolbar.add_toolbar_action(
             'add_folder_action', icon=UiIcons().folder, triggers=self.on_add_folder_click)
 
-    def add_sub_folders(self, folder_list, parent_id=None):
+    def add_sub_folders(self, folder_list: List[QtWidgets.QTreeWidgetItem], parent_id: Optional[int] = None):
         """
         Recursively add subfolders to the given parent folder in a QTreeWidget.
 
@@ -250,7 +251,7 @@ class FolderLibraryItem(MediaManagerItem):
             folder_list[folder.id] = folder_item
             self.add_sub_folders(folder_list, folder.id)
 
-    def expand_folder(self, folder_id, root_item=None):
+    def expand_folder(self, folder_id: int, root_item: Optional[QtWidgets.QTreeWidgetItem] = None):
         """
         Expand folders in the widget recursively.
 
@@ -288,7 +289,7 @@ class FolderLibraryItem(MediaManagerItem):
             self.recursively_delete_folder(child)
             self.manager.delete_object(Folder, child.id)
 
-    def file_to_item(self, filename):
+    def file_to_item(self, filename: Union[Path, str]):
         """
         This method allows the media item to convert a string filename into an item class
 
@@ -374,13 +375,13 @@ class FolderLibraryItem(MediaManagerItem):
         items = self.manager.get_all_objects(self.item_class, self.item_class.file_path.ilike('%' + string + '%'))
         return [self.format_search_result(item) for item in items]
 
-    def validate_and_load(self, file_paths, target_folder=None):
+    def validate_and_load(self, file_paths: List[Path], target_folder: Optional[QtWidgets.QTreeWidgetItem] = None):
         """
         Process a list for files either from the File Dialog or from Drag and Drop.
         This method is overloaded from MediaManagerItem.
 
         :param list[Path] file_paths: A List of paths  to be loaded
-        :param target_group: The QTreeWidgetItem of the group that will be the parent of the added files
+        :param target_folder: The QTreeWidgetItem of the folder that will be the parent of the added files
         """
         self.application.set_normal_cursor()
         if target_folder:

tests/openlp_core/ui/test_library.py

@@ -27,14 +27,26 @@ from PyQt5 import QtCore, QtWidgets
 from openlp.core.ui.library import FolderLibraryItem
 
 
-class MockItem(MagicMock):
+class MockItem:
+    id = 1
+    name = 'video.mp4'
     file_path = 'path/to/video.mp4'
     folder_id = None
 
+    def __init__(self, *args, **kwargs):
+        for key, value in kwargs.items():
+            setattr(self, key, value)
 
-class MockFolder(MagicMock):
+
+class MockFolder:
+    id = 1
+    name = 'folder'
     parent_id = None
 
+    def __init__(self, *args, **kwargs):
+        for key, value in kwargs.items():
+            setattr(self, key, value)
+
 
 @pytest.fixture
 def folder_library_item(registry, settings):
@@ -43,6 +55,7 @@ def folder_library_item(registry, settings):
     with patch('openlp.core.lib.mediamanageritem.MediaManagerItem._setup'), \
             patch('openlp.core.lib.mediamanageritem.MediaManagerItem.setup_item'):
         library_item = FolderLibraryItem(None, MagicMock(manager=mocked_manager), MockFolder, MockItem)
+        library_item.list_view = MagicMock(**{'selectedItems.return_value': []})
     return library_item
 
 
@@ -59,6 +72,36 @@ def test_folderlibrary_retranslate_ui(folder_library_item):
     folder_library_item.add_folder_action.setToolTip.assert_called_once_with('Add folder.')
 
 
+def test_folderlibrary_add_custom_context_actions(folder_library_item):
+    """Test that the folder_library_item.add_custom_context_actions does nothing"""
+    folder_library_item.add_custom_context_actions()
+
+
+def test_folderlibrary_load_item(folder_library_item):
+    """Test that the load_item() method raises a NotImplementedError"""
+    with pytest.raises(NotImplementedError):
+        folder_library_item.load_item(MockItem())
+
+
+def test_folderlibrary_delete_item(folder_library_item):
+    """Test that the delete_item() method raises a NotImplementedError"""
+    with pytest.raises(NotImplementedError):
+        folder_library_item.delete_item(MockItem())
+
+
+def test_folderlibrary_add_middle_header_bar(folder_library_item):
+    """Test that an action is created for the header bar"""
+    # GIVEN: A mocked action and toolbar
+    mocked_action = MagicMock()
+    folder_library_item.toolbar = MagicMock(**{'add_toolbar_action.return_value': mocked_action})
+
+    # WHEN: add_middle_header_bar() is called
+    folder_library_item.add_middle_header_bar()
+
+    # THEN: The action should have been created
+    assert folder_library_item.add_folder_action is mocked_action
+
+
 def test_folderlibrary_create_item_from_id_path(folder_library_item):
     """Test the create_item_from_id method"""
     # GIVEN: An instance of the FolderLibraryItem
@@ -94,7 +137,6 @@ def test_folderlibrary_current_folder(folder_library_item):
     mocked_item = MockItem()
     mocked_list_folder = MagicMock(**{'data.return_value': mocked_folder}, spec=QtWidgets.QTreeWidgetItem)
     mocked_list_item = MagicMock(**{'data.return_value': mocked_item, 'parent.return_value': mocked_list_folder})
-    folder_library_item.list_view = MagicMock()
     folder_library_item.list_view.selectedItems.return_value = [mocked_list_item]
 
     # WHEN: We access the property
@@ -104,6 +146,34 @@ def test_folderlibrary_current_folder(folder_library_item):
     assert folder is mocked_folder
 
 
+def test_file_to_item_as_path(folder_library_item):
+    """Test the file_to_item() method returns an Item from the filename as a Path"""
+    # GIVEN: A FolderLibraryItem
+    folder_library_item.manager.save_object = MagicMock()
+
+    # WHEN: A Path object is passed to file_to_item()
+    result = folder_library_item.file_to_item(Path('path/to/file.mp4'))
+
+    # THEN: An Item should be returned
+    assert isinstance(result, MockItem)
+    assert result.name == 'file.mp4'
+    assert result.file_path == 'path/to/file.mp4'
+
+
+def test_file_to_item_as_str(folder_library_item):
+    """Test the file_to_item() method returns an Item from the filename as a str"""
+    # GIVEN: A FolderLibraryItem
+    folder_library_item.manager.save_object = MagicMock()
+
+    # WHEN: A Path object is passed to file_to_item()
+    result = folder_library_item.file_to_item('path/to/file.mp4')
+
+    # THEN: An Item should be returned
+    assert isinstance(result, MockItem)
+    assert result.name == 'file.mp4'
+    assert result.file_path == 'path/to/file.mp4'
+
+
 def test_validate_and_load(folder_library_item):
     """
     Test that the validate_and_load() method when called with a folder
@@ -113,7 +183,7 @@ def test_validate_and_load(folder_library_item):
     expected_list = [MagicMock(file_path=fp) for fp in file_list]
     folder_library_item.manager.get_all_objects.return_value = expected_list
     folder_library_item.load_list = MagicMock()
-    folder_library_item.list_view = MagicMock(**{'selectedItems.return_value': None})
+    folder_library_item.list_view.selectedItems.return_value = None
     folder_library_item.settings_section = 'tests'
     folder_library_item.choose_folder_form = MagicMock(**{'exec.return_value': QtWidgets.QDialog.Accepted,
                                                           'folder': None})
@@ -126,7 +196,7 @@ def test_validate_and_load(folder_library_item):
     folder_library_item.load_list.assert_called_once_with(expected_list, target_folder=None)
 
 
-@patch('openlp.core.lib.serviceitem.sha256_file_hash')
+@patch('openlp.core.ui.library.sha256_file_hash')
 def test_recursively_delete_folder(mocked_sha256_file_hash, folder_library_item):
     """
     Test that recursively_delete_folder() works
@@ -156,7 +226,7 @@ def test_recursively_delete_folder(mocked_sha256_file_hash, folder_library_item)
     assert folder_library_item.delete_item.call_count == 5, 'delete_item() should have been called 5 times'
 
 
-@patch('openlp.core.lib.serviceitem.sha256_file_hash')
+@patch('openlp.core.ui.library.sha256_file_hash')
 def test_on_delete_click(mocked_sha256_file_hash, folder_library_item):
     """
     Test that on_delete_click() works
@@ -166,11 +236,36 @@ def test_on_delete_click(mocked_sha256_file_hash, folder_library_item):
     folder_library_item.delete_item = MagicMock()
     mocked_item = MockItem()
     mocked_item.id = 1
-    mocked_item.group_id = 1
     mocked_item.file_path = 'imagefile.png'
     mocked_item.file_hash = 'abcd'
     folder_library_item.service_path = Path()
-    folder_library_item.list_view = MagicMock()
+    mocked_row_item = MagicMock()
+    mocked_row_item.data.return_value = mocked_item
+    mocked_row_item.text.return_value = ''
+    folder_library_item.list_view.selectedItems.return_value = [None, mocked_row_item]
+    mocked_sha256_file_hash.return_value = 'abcd'
+
+    # WHEN: Calling on_delete_click
+    folder_library_item.on_delete_click()
+
+    # THEN: delete_file should have been called twice
+    assert folder_library_item.delete_item.call_count == 1, 'delete_item() should have been called once'
+
+
+@patch('openlp.core.ui.library.sha256_file_hash')
+def test_on_delete_click_with_parent(mocked_sha256_file_hash, folder_library_item):
+    """
+    Test that on_delete_click() works
+    """
+    # GIVEN: An ImageGroups object and mocked functions
+    folder_library_item.check_item_selected = MagicMock(return_value=True)
+    folder_library_item.delete_item = MagicMock()
+    mocked_item = MockItem()
+    mocked_item.id = 1
+    mocked_item.folder_id = 1
+    mocked_item.file_path = 'imagefile.png'
+    mocked_item.file_hash = 'abcd'
+    folder_library_item.service_path = Path()
     mocked_row_item = MagicMock()
     mocked_row_item.data.return_value = mocked_item
     mocked_row_item.text.return_value = ''
@@ -182,3 +277,105 @@ def test_on_delete_click(mocked_sha256_file_hash, folder_library_item):
 
     # THEN: delete_file should have been called twice
     assert folder_library_item.delete_item.call_count == 1, 'delete_item() should have been called once'
+
+
+@patch('openlp.core.ui.library.sha256_file_hash')
+@patch('openlp.core.ui.library.QtWidgets.QMessageBox.question')
+def test_on_delete_click_as_folder(mocked_question, mocked_sha256_file_hash, folder_library_item):
+    """
+    Test that on_delete_click() works
+    """
+    # GIVEN: An ImageGroups object and mocked functions
+    folder_library_item.check_item_selected = MagicMock(return_value=True)
+    folder_library_item.delete_item = MagicMock()
+    folder_library_item.recursively_delete_folder = MagicMock()
+    mocked_folder = MockFolder()
+    mocked_folder.id = 1
+    folder_library_item.service_path = Path()
+    mocked_row_item = MagicMock()
+    mocked_row_item.data.return_value = mocked_folder
+    mocked_row_item.text.return_value = ''
+    folder_library_item.list_view.selectedItems.return_value = [mocked_row_item]
+    mocked_sha256_file_hash.return_value = 'abcd'
+    mocked_question.return_value = QtWidgets.QMessageBox.Yes
+
+    # WHEN: Calling on_delete_click
+    folder_library_item.on_delete_click()
+
+    # THEN: delete_file should have been called twice
+    assert folder_library_item.recursively_delete_folder.call_count == 1, \
+        'recursively_delete_folder() should have been called once'
+
+
+@patch('openlp.core.ui.library.sha256_file_hash')
+@patch('openlp.core.ui.library.QtWidgets.QMessageBox.question')
+def test_on_delete_click_as_folder_with_parent(mocked_question, mocked_sha256_file_hash, folder_library_item):
+    """
+    Test that on_delete_click() works
+    """
+    # GIVEN: An ImageGroups object and mocked functions
+    folder_library_item.check_item_selected = MagicMock(return_value=True)
+    folder_library_item.delete_item = MagicMock()
+    folder_library_item.recursively_delete_folder = MagicMock()
+    mocked_folder = MockFolder()
+    mocked_folder.id = 1
+    mocked_folder.parent_id = 1
+    folder_library_item.service_path = Path()
+    mocked_row_item = MagicMock()
+    mocked_row_item.data.return_value = mocked_folder
+    mocked_row_item.text.return_value = ''
+    folder_library_item.list_view.selectedItems.return_value = [mocked_row_item]
+    mocked_sha256_file_hash.return_value = 'abcd'
+    mocked_question.return_value = QtWidgets.QMessageBox.Yes
+
+    # WHEN: Calling on_delete_click
+    folder_library_item.on_delete_click()
+
+    # THEN: delete_file should have been called twice
+    assert folder_library_item.recursively_delete_folder.call_count == 1, \
+        'recursively_delete_folder() should have been called once'
+
+
+def test_on_add_folder_click(folder_library_item):
+    """Test that the on_add_folder_click adds a folder to the list"""
+    # GIVEN: A folder library item and some mocks
+    mocked_folder = MockFolder()
+    mocked_folder.id = 1
+    mocked_item = MockItem()
+    folder_library_item.add_folder_form = MagicMock(**{'exec.return_value': True, 'new_folder': mocked_folder})
+    folder_library_item.manager.save_object = MagicMock(return_value=True)
+    folder_library_item.manager.get_all_objects = MagicMock(return_value=[mocked_item])
+    folder_library_item.load_list = MagicMock()
+    folder_library_item.expand_folder = MagicMock()
+
+    # WHEN: on_add_folder_click() is called
+    folder_library_item.on_add_folder_click()
+
+    # THEN: Things should happen
+    folder_library_item.manager.save_object.assert_called_once_with(mocked_folder)
+    folder_library_item.manager.get_all_objects.assert_called_once()
+    folder_library_item.load_list.assert_called_once_with([mocked_item])
+    folder_library_item.expand_folder.assert_called_once_with(1)
+
+
+@patch('openlp.core.ui.library.critical_error_message_box')
+def test_on_add_folder_click_fails(mocked_critical_error_message_box, folder_library_item):
+    """Test that the on_add_folder_click adds a folder to the list"""
+    # GIVEN: A folder library item and some mocks
+    mocked_folder = MockFolder()
+    mocked_folder.id = 1
+    folder_library_item.add_folder_form = MagicMock(**{'exec.return_value': True, 'new_folder': mocked_folder})
+    folder_library_item.manager.save_object = MagicMock(return_value=False)
+    folder_library_item.manager.get_all_objects = MagicMock()
+    folder_library_item.load_list = MagicMock()
+    folder_library_item.expand_folder = MagicMock()
+
+    # WHEN: on_add_folder_click() is called
+    folder_library_item.on_add_folder_click()
+
+    # THEN: Things should happen
+    folder_library_item.manager.save_object.assert_called_once_with(mocked_folder)
+    folder_library_item.manager.get_all_objects.assert_not_called()
+    folder_library_item.load_list.assert_not_called()
+    folder_library_item.expand_folder.assert_not_called()
+    mocked_critical_error_message_box.assert_called_once_with(message='Could not add the new folder.')