Merge pull request #98 from ral-facilities/allow-edit-cat-parent-with…

…-child-#93

Allow edit CC to modify parent_id when it has children #93

inventory_management_system_api/repositories/catalogue_category.py

@@ -74,7 +74,7 @@ def delete(self, catalogue_category_id: str) -> None:
         :raises MissingRecordError: If the catalogue category doesn't exist.
         """
         catalogue_category_id = CustomObjectId(catalogue_category_id)
-        if self._has_child_elements(catalogue_category_id):
+        if self.has_child_elements(catalogue_category_id):
             raise ChildrenElementsExistError(
                 f"Catalogue category with ID {str(catalogue_category_id)} has children elements and cannot be deleted"
             )
@@ -130,15 +130,10 @@ def update(self, catalogue_category_id: str, catalogue_category: CatalogueCatego
         :param catalogue_category_id: The ID of the catalogue category to update.
         :param catalogue_category: The catalogue category containing the update data.
         :return: The updated catalogue category.
-        :raises ChildrenElementsExistError: If the catalogue category has children elements.
         :raises MissingRecordError: If the parent catalogue category specified by `parent_id` doesn't exist.
         :raises DuplicateRecordError: If a duplicate catalogue category is found within the parent catalogue category.
         """
         catalogue_category_id = CustomObjectId(catalogue_category_id)
-        if self._has_child_elements(catalogue_category_id):
-            raise ChildrenElementsExistError(
-                f"Catalogue category with ID {str(catalogue_category_id)} has children elements and cannot be updated"
-            )
 
         parent_id = str(catalogue_category.parent_id) if catalogue_category.parent_id else None
         if parent_id and not self.get(parent_id):
@@ -186,7 +181,7 @@ def _is_duplicate_catalogue_category(self, parent_id: Optional[str], code: str)
         count = self._catalogue_categories_collection.count_documents({"parent_id": parent_id, "code": code})
         return count > 0
 
-    def _has_child_elements(self, catalogue_category_id: CustomObjectId) -> bool:
+    def has_child_elements(self, catalogue_category_id: CustomObjectId) -> bool:
         """
         Check if a catalogue category has children elements based on its ID.
 

inventory_management_system_api/schemas/catalogue_category.py

@@ -62,6 +62,10 @@ class CatalogueCategoryPostRequestSchema(BaseModel):
     )
 
 
+# Special fields that are not allowed to be changed in a post request while the category has child elements
+CATALOGUE_CATEGORY_WITH_CHILDREN_NON_EDITABLE_FIELDS = ["is_leaf", "catalogue_item_properties"]
+
+
 class CatalogueCategoryPatchRequestSchema(CatalogueCategoryPostRequestSchema):
     """
     Schema model for a catalogue category update request.

inventory_management_system_api/services/catalogue_category.py

@@ -6,11 +6,17 @@
 
 from fastapi import Depends
 
-from inventory_management_system_api.core.exceptions import LeafCategoryError, MissingRecordError
+from inventory_management_system_api.core.custom_object_id import CustomObjectId
+from inventory_management_system_api.core.exceptions import (
+    ChildrenElementsExistError,
+    LeafCategoryError,
+    MissingRecordError,
+)
 from inventory_management_system_api.models.catalogue_category import CatalogueCategoryIn, CatalogueCategoryOut
 from inventory_management_system_api.repositories.catalogue_category import CatalogueCategoryRepo
 from inventory_management_system_api.schemas.breadcrumbs import BreadcrumbsGetSchema
 from inventory_management_system_api.schemas.catalogue_category import (
+    CATALOGUE_CATEGORY_WITH_CHILDREN_NON_EDITABLE_FIELDS,
     CatalogueCategoryPatchRequestSchema,
     CatalogueCategoryPostRequestSchema,
 )
@@ -112,6 +118,8 @@ def update(
         :raises MissingRecordError: If the catalogue category doesn't exist.
         :raises LeafCategoryError: If the parent catalogue category to which the catalogue category is attempted to be
             moved is a leaf catalogue category.
+        :raises ChildrenElementsExistError: If the catalogue category has child elements and attempting to update
+                                    either any of the disallowed properties (is_leaf or catalogue_item_properties)
         """
         update_data = catalogue_category.dict(exclude_unset=True)
 
@@ -128,6 +136,13 @@ def update(
             if parent_catalogue_category and parent_catalogue_category.is_leaf:
                 raise LeafCategoryError("Cannot add catalogue category to a leaf parent catalogue category")
 
+        # If any of these, need to ensure the category has no children
+        if any(key in update_data for key in CATALOGUE_CATEGORY_WITH_CHILDREN_NON_EDITABLE_FIELDS):
+            if self._catalogue_category_repository.has_child_elements(CustomObjectId(catalogue_category_id)):
+                raise ChildrenElementsExistError(
+                    f"Catalogue category with ID {str(catalogue_category_id)} has child elements and cannot be updated"
+                )
+
         stored_catalogue_category = stored_catalogue_category.copy(update=update_data)
         return self._catalogue_category_repository.update(
             catalogue_category_id, CatalogueCategoryIn(**stored_catalogue_category.dict())

test/e2e/test_catalogue_category.py

@@ -542,26 +542,26 @@ def test_partial_update_catalogue_category_change_name_duplicate(test_client):
     )
 
 
-def test_partial_update_catalogue_category_change_name_has_child_catalogue_categories(test_client):
+def test_partial_update_catalogue_category_change_valid_parameters_when_has_child_catalogue_categories(test_client):
     """
-    Test changing the name of a catalogue category which has child catalogue categories.
+    Test changing valid parameters of a catalogue category which has child catalogue categories.
     """
-    catalogue_category_post = {"name": "Category A", "is_leaf": False}
-    response = test_client.post("/v1/catalogue-categories", json=catalogue_category_post)
-    catalogue_category_id = response.json()["id"]
-    catalogue_category_post = {"name": "Category B", "is_leaf": False, "parent_id": catalogue_category_id}
-    test_client.post("/v1/catalogue-categories", json=catalogue_category_post)
+    category_a, _, _ = _post_catalogue_categories(test_client)
 
-    catalogue_category_patch = {"name": "Category A"}
-    response = test_client.patch(f"/v1/catalogue-categories/{catalogue_category_id}", json=catalogue_category_patch)
+    catalogue_category_patch = {"name": "Category D"}
+    response = test_client.patch(f"/v1/catalogue-categories/{category_a['id']}", json=catalogue_category_patch)
 
-    assert response.status_code == 409
-    assert response.json()["detail"] == "Catalogue category has child elements and cannot be updated"
+    assert response.status_code == 200
+    assert response.json() == {
+        **CATALOGUE_CATEGORY_POST_A_EXPECTED,
+        **catalogue_category_patch,
+        "code": "category-d",
+    }
 
 
-def test_partial_update_catalogue_category_change_name_has_child_catalogue_items(test_client):
+def test_partial_update_catalogue_category_change_valid_when_has_child_catalogue_items(test_client):
     """
-    Test changing the name of a catalogue category which has child catalogue items.
+    Test changing valid parameters of a catalogue category which has child catalogue items.
     """
     response = test_client.post("/v1/catalogue-categories", json=CATALOGUE_CATEGORY_POST_C)
 
@@ -579,11 +579,15 @@ def test_partial_update_catalogue_category_change_name_has_child_catalogue_items
     }
     test_client.post("/v1/catalogue-items", json=catalogue_item_post)
 
-    catalogue_category_patch = {"name": "Category B"}
+    catalogue_category_patch = {"name": "Category D"}
     response = test_client.patch(f"/v1/catalogue-categories/{catalogue_category_id}", json=catalogue_category_patch)
 
-    assert response.status_code == 409
-    assert response.json()["detail"] == "Catalogue category has child elements and cannot be updated"
+    assert response.status_code == 200
+    assert response.json() == {
+        **CATALOGUE_CATEGORY_POST_C_EXPECTED,
+        **catalogue_category_patch,
+        "code": "category-d",
+    }
 
 
 def test_partial_update_catalogue_category_change_from_non_leaf_to_leaf(test_client):
@@ -766,17 +770,23 @@ def test_partial_update_catalogue_category_change_parent_id_has_child_catalogue_
     catalogue_category_post = {"name": "Category A", "is_leaf": False}
     response = test_client.post("/v1/catalogue-categories", json=catalogue_category_post)
     catalogue_category_a_id = response.json()["id"]
-    catalogue_category_post = {"name": "Category B", "is_leaf": False, "parent_id": catalogue_category_a_id}
-    response = test_client.post("/v1/catalogue-categories", json=catalogue_category_post)
+    catalogue_category_b_post = {"name": "Category B", "is_leaf": False, "parent_id": catalogue_category_a_id}
+    response = test_client.post("/v1/catalogue-categories", json=catalogue_category_b_post)
     catalogue_category_b_id = response.json()["id"]
     catalogue_category_post = {"name": "Category C", "is_leaf": False, "parent_id": catalogue_category_b_id}
     test_client.post("/v1/catalogue-categories", json=catalogue_category_post)
 
     catalogue_category_patch = {"parent_id": None}
     response = test_client.patch(f"/v1/catalogue-categories/{catalogue_category_b_id}", json=catalogue_category_patch)
 
-    assert response.status_code == 409
-    assert response.json()["detail"] == "Catalogue category has child elements and cannot be updated"
+    assert response.status_code == 200
+    assert response.json() == {
+        **catalogue_category_b_post,
+        **catalogue_category_patch,
+        "catalogue_item_properties": [],
+        "id": ANY,
+        "code": "category-b",
+    }
 
 
 def test_partial_update_catalogue_category_change_parent_id_has_child_catalogue_items(test_client):
@@ -786,8 +796,8 @@ def test_partial_update_catalogue_category_change_parent_id_has_child_catalogue_
     catalogue_category_post = {"name": "Category A", "is_leaf": False}
     response = test_client.post("/v1/catalogue-categories", json=catalogue_category_post)
     catalogue_category_a_id = response.json()["id"]
-    catalogue_category_post = {"name": "Category B", "is_leaf": False}
-    response = test_client.post("/v1/catalogue-categories", json=catalogue_category_post)
+    catalogue_category_b_post = {"name": "Category B", "is_leaf": False}
+    response = test_client.post("/v1/catalogue-categories", json=catalogue_category_b_post)
     catalogue_category_b_id = response.json()["id"]
     catalogue_category_post = {
         "name": "Category C",
@@ -817,8 +827,14 @@ def test_partial_update_catalogue_category_change_parent_id_has_child_catalogue_
     catalogue_category_patch = {"parent_id": catalogue_category_a_id}
     response = test_client.patch(f"/v1/catalogue-categories/{catalogue_category_b_id}", json=catalogue_category_patch)
 
-    assert response.status_code == 409
-    assert response.json()["detail"] == "Catalogue category has child elements and cannot be updated"
+    assert response.status_code == 200
+    assert response.json() == {
+        **catalogue_category_b_post,
+        **catalogue_category_patch,
+        "catalogue_item_properties": [],
+        "id": ANY,
+        "code": "category-b",
+    }
 
 
 def test_partial_update_catalogue_category_change_parent_id_duplicate_name(test_client):

test/unit/repositories/test_catalogue_category.py

@@ -765,9 +765,6 @@ def test_update(test_helpers, database_mock, catalogue_category_repository):
     )
     # pylint: enable=duplicate-code
 
-    # Mock count_documents to return 0 (children elements not found)
-    test_helpers.mock_count_documents(database_mock.catalogue_categories, 0)
-    test_helpers.mock_count_documents(database_mock.catalogue_items, 0)
     # Mock `find_one` to return a catalogue category document
     test_helpers.mock_find_one(
         database_mock.catalogue_categories,
@@ -845,63 +842,6 @@ def test_update_with_invalid_id(catalogue_category_repository):
     assert str(exc.value) == f"Invalid ObjectId value '{catalogue_category_id}'"
 
 
-def test_update_has_children_catalogue_categories(test_helpers, database_mock, catalogue_category_repository):
-    """
-    Test updating a catalogue category with children catalogue categories.
-
-    Verify that the `update` method properly handles the update of a catalogue category with children catalogue
-    categories.
-    """
-    update_catalogue_category = CatalogueCategoryIn(
-        name="Category B",
-        code="category-b",
-        is_leaf=False,
-        parent_id=None,
-        catalogue_item_properties=[],
-    )
-
-    # Mock count_documents to return 1 (children catalogue categories found)
-    test_helpers.mock_count_documents(database_mock.catalogue_categories, 1)
-    # Mock count_documents to return 0 (children catalogue items not found)
-    test_helpers.mock_count_documents(database_mock.catalogue_items, 0)
-
-    catalogue_category_id = str(ObjectId())
-    with pytest.raises(ChildrenElementsExistError) as exc:
-        catalogue_category_repository.update(catalogue_category_id, update_catalogue_category)
-    assert (
-        str(exc.value)
-        == f"Catalogue category with ID {str(catalogue_category_id)} has children elements and cannot be updated"
-    )
-
-
-def test_update_has_children_catalogue_items(test_helpers, database_mock, catalogue_category_repository):
-    """
-    Test updating a catalogue category with children catalogue items.
-
-    Verify that the `update` method properly handles the update of a catalogue category with children catalogue items.
-    """
-    update_catalogue_category = CatalogueCategoryIn(
-        name="Category B",
-        code="category-b",
-        is_leaf=False,
-        parent_id=None,
-        catalogue_item_properties=[],
-    )
-
-    # Mock count_documents to return 0 (children catalogue categories not found)
-    test_helpers.mock_count_documents(database_mock.catalogue_categories, 0)
-    # Mock count_documents to return 1 (children catalogue items found)
-    test_helpers.mock_count_documents(database_mock.catalogue_items, 1)
-
-    catalogue_category_id = str(ObjectId())
-    with pytest.raises(ChildrenElementsExistError) as exc:
-        catalogue_category_repository.update(catalogue_category_id, update_catalogue_category)
-    assert (
-        str(exc.value)
-        == f"Catalogue category with ID {str(catalogue_category_id)} has children elements and cannot be updated"
-    )
-
-
 def test_update_with_nonexistent_parent_id(test_helpers, database_mock, catalogue_category_repository):
     """
     Test updating a catalogue category with non-existent parent ID.
@@ -918,9 +858,6 @@ def test_update_with_nonexistent_parent_id(test_helpers, database_mock, catalogu
     )
     # pylint: enable=duplicate-code
 
-    # Mock count_documents to return 0 (children elements not found)
-    test_helpers.mock_count_documents(database_mock.catalogue_categories, 0)
-    test_helpers.mock_count_documents(database_mock.catalogue_items, 0)
     # Mock `find_one` to not return a parent catalogue category document
     test_helpers.mock_find_one(database_mock.catalogue_categories, None)
 
@@ -946,9 +883,6 @@ def test_update_duplicate_name_within_parent(test_helpers, database_mock, catalo
     )
     # pylint: enable=duplicate-code
 
-    # Mock count_documents to return 0 (children elements not found)
-    test_helpers.mock_count_documents(database_mock.catalogue_categories, 0)
-    test_helpers.mock_count_documents(database_mock.catalogue_items, 0)
     catalogue_category_id = str(ObjectId())
     # Mock `find_one` to return a catalogue category document
     test_helpers.mock_find_one(
@@ -985,9 +919,6 @@ def test_update_duplicate_name_within_new_parent(test_helpers, database_mock, ca
         catalogue_item_properties=[],
     )
 
-    # Mock count_documents to return 0 (children elements not found)
-    test_helpers.mock_count_documents(database_mock.catalogue_categories, 0)
-    test_helpers.mock_count_documents(database_mock.catalogue_items, 0)
     # Mock `find_one` to return a parent catalogue category document
     test_helpers.mock_find_one(
         database_mock.catalogue_categories,
@@ -1019,3 +950,45 @@ def test_update_duplicate_name_within_new_parent(test_helpers, database_mock, ca
     with pytest.raises(DuplicateRecordError) as exc:
         catalogue_category_repository.update(catalogue_category_id, update_catalogue_category)
     assert str(exc.value) == "Duplicate catalogue category found within the parent catalogue category"
+
+
+def test_has_child_elements_with_no_child_categories(test_helpers, database_mock, catalogue_category_repository):
+    """
+    Test has_child_elements returns false when there are no child categories
+    """
+
+    # Mock count_documents to return 0 (children elements not found)
+    test_helpers.mock_count_documents(database_mock.catalogue_categories, 0)
+    test_helpers.mock_count_documents(database_mock.catalogue_items, 0)
+
+    result = catalogue_category_repository.has_child_elements(ObjectId())
+
+    assert result is False
+
+
+def test_has_child_elements_with_child_categories(test_helpers, database_mock, catalogue_category_repository):
+    """
+    Test has_child_elements returns true when there are child categories
+    """
+
+    # Mock count_documents to return 1 (child element found)
+    test_helpers.mock_count_documents(database_mock.catalogue_categories, 1)
+    test_helpers.mock_count_documents(database_mock.catalogue_items, 0)
+
+    result = catalogue_category_repository.has_child_elements(ObjectId())
+
+    assert result is True
+
+
+def test_has_child_elements_with_child_items(test_helpers, database_mock, catalogue_category_repository):
+    """
+    Test has_child_elements returns true when there are child categories
+    """
+
+    # Mock count_documents to return 1 (child element found)
+    test_helpers.mock_count_documents(database_mock.catalogue_categories, 0)
+    test_helpers.mock_count_documents(database_mock.catalogue_items, 1)
+
+    result = catalogue_category_repository.has_child_elements(ObjectId())
+
+    assert result is True