Clean up duplicate error messages and update repo get types #74

joelvdavies · joelvdavies · commit f776aa35dff6 · 2025-05-29T08:35:43.000Z
diff --git a/inventory_management_system_api/core/database.py b/inventory_management_system_api/core/database.py
@@ -51,8 +51,7 @@ def start_session_transaction(action_description: str) -> Generator[ClientSessio
             except OperationFailure as exc:
                 if "write conflict" in str(exc).lower():
                     raise WriteConflictError(
-                        f"Write conflict while {action_description}. Please try again later.",
-                        response_detail=f"Write conflict while {action_description}. Please try again later.",
+                        f"Write conflict while {action_description}. Please try again later."
                     ) from exc
                 raise exc
 
diff --git a/inventory_management_system_api/core/exceptions.py b/inventory_management_system_api/core/exceptions.py
@@ -33,6 +33,9 @@ def __init__(self, detail: str, response_detail: Optional[str] = None, status_co
 
         if response_detail is not None:
             self.response_detail = response_detail
+        # If there is no response detail defined just use the detail
+        elif not hasattr(self, "response_detail"):
+            self.response_detail = detail
         if status_code is not None:
             self.status_code = status_code
 
@@ -81,16 +84,6 @@ class InvalidPropertyTypeError(BaseAPIException):
 
     status_code = status.HTTP_422_UNPROCESSABLE_ENTITY
 
-    def __init__(self, detail: str):
-        """
-        Initialise the exception.
-
-        :param detail: Specific detail of the exception (just like Exception would take).
-        """
-        super().__init__(detail)
-
-        self.response_detail = detail
-
 
 class MissingMandatoryProperty(BaseAPIException):
     """
@@ -99,16 +92,6 @@ class MissingMandatoryProperty(BaseAPIException):
 
     status_code = status.HTTP_422_UNPROCESSABLE_ENTITY
 
-    def __init__(self, detail: str):
-        """
-        Initialise the exception.
-
-        :param detail: Specific detail of the exception (just like Exception would take).
-        """
-        super().__init__(detail)
-
-        self.response_detail = detail
-
 
 class DuplicateRecordError(DatabaseError):
     """The record being added to the database is a duplicate."""
@@ -183,7 +166,6 @@ class InvalidActionError(BaseAPIException):
     """
 
     status_code = status.HTTP_422_UNPROCESSABLE_ENTITY
-    response_detail = "Invalid action"
 
 
 class WriteConflictError(DatabaseError):
@@ -216,13 +198,3 @@ class PropertyValueError(BaseAPIException, ValueError):
     """Exception raised when there is an error caused by a property value"""
 
     status_code = status.HTTP_422_UNPROCESSABLE_ENTITY
-
-    def __init__(self, detail: str):
-        """
-        Initialise the exception.
-
-        :param detail: Specific detail of the exception (just like Exception would take).
-        """
-        super().__init__(detail)
-
-        self.response_detail = detail
diff --git a/inventory_management_system_api/repositories/catalogue_category.py b/inventory_management_system_api/repositories/catalogue_category.py
@@ -84,7 +84,7 @@ def get(
         catalogue_category_id: str,
         entity_type_modifier: Optional[str] = None,
         session: Optional[ClientSession] = None,
-    ) -> Optional[CatalogueCategoryOut]:
+    ) -> CatalogueCategoryOut:
         """
         Retrieve a catalogue category by its ID from a MongoDB database.
 
@@ -213,10 +213,7 @@ def update(
                     )
                 )
             ):
-                raise InvalidActionError(
-                    "Cannot move a catalogue category to one of its own children",
-                    response_detail="Cannot move a catalogue category to one of its own children",
-                )
+                raise InvalidActionError("Cannot move a catalogue category to one of its own children")
 
         logger.info("Updating catalogue category with ID: %s in the database", catalogue_category_id)
         self._catalogue_categories_collection.update_one(
diff --git a/inventory_management_system_api/repositories/catalogue_item.py b/inventory_management_system_api/repositories/catalogue_item.py
@@ -52,7 +52,7 @@ def get(
         catalogue_item_id: str,
         entity_type_modifier: Optional[str] = None,
         session: Optional[ClientSession] = None,
-    ) -> Optional[CatalogueItemOut]:
+    ) -> CatalogueItemOut:
         """
         Retrieve a catalogue item by its ID from a MongoDB database.
 
@@ -61,7 +61,7 @@ def get(
                                      e.g. parent if its for a parent system.
         :param session: PyMongo ClientSession to use for database operations
         :return: The retrieved catalogue item, or `None` if not found.
-        :raises MissingRecordError: If the supplied `system_id` is non-existent.
+        :raises MissingRecordError: If the supplied `catalogue_item_id` is non-existent.
         """
         entity_type = f"{entity_type_modifier} catalogue item" if entity_type_modifier else "catalogue item"
         catalogue_item_id = CustomObjectId(
diff --git a/inventory_management_system_api/repositories/item.py b/inventory_management_system_api/repositories/item.py
@@ -50,15 +50,16 @@ def create(self, item: ItemIn, session: Optional[ClientSession] = None) -> ItemO
 
     def get(
         self, item_id: str, entity_type_modifier: Optional[str] = None, session: Optional[ClientSession] = None
-    ) -> Optional[ItemOut]:
+    ) -> ItemOut:
         """
         Retrieve an item by its ID from a MongoDB database.
 
         :param item_id: The ID of the item to retrieve
         :param entity_type_modifier: String value to put at the start of the entity type used in error messages
                                      e.g. specified if its for a specified item.
         :param session: PyMongo ClientSession to use for database operations
-        :return: The retrieved item, or `None` if not found.
+        :return: The retrieved item.
+        :raises MissingRecordError: If the supplied `item_id` is non-existent.
         """
 
         entity_type = f"{entity_type_modifier} item" if entity_type_modifier else "item"
diff --git a/inventory_management_system_api/repositories/manufacturer.py b/inventory_management_system_api/repositories/manufacturer.py
@@ -57,7 +57,7 @@ def create(self, manufacturer: ManufacturerIn, session: Optional[ClientSession]
 
     def get(
         self, manufacturer_id: str, entity_type_modifier: Optional[str] = None, session: Optional[ClientSession] = None
-    ) -> Optional[ManufacturerOut]:
+    ) -> ManufacturerOut:
         """
         Retrieve a manufacturer by its ID from a MondoDB database.
 
diff --git a/inventory_management_system_api/repositories/system.py b/inventory_management_system_api/repositories/system.py
@@ -70,7 +70,7 @@ def create(self, system: SystemIn, session: Optional[ClientSession] = None) -> S
 
     def get(
         self, system_id: str, entity_type_modifier: Optional[str] = None, session: Optional[ClientSession] = None
-    ) -> Optional[SystemOut]:
+    ) -> SystemOut:
         """
         Retrieve a system by its ID from a MongoDB database
 
@@ -177,10 +177,7 @@ def update(self, system_id: str, system: SystemIn, session: Optional[ClientSessi
                     )
                 )
             ):
-                raise InvalidActionError(
-                    "Cannot move a system to one of its own children",
-                    response_detail="Cannot move a system to one of its own children",
-                )
+                raise InvalidActionError("Cannot move a system to one of its own children")
 
         logger.info("Updating system with ID: %s in the database", system_id)
         self._systems_collection.update_one({"_id": system_id}, {"$set": system.model_dump()}, session=session)
diff --git a/inventory_management_system_api/repositories/unit.py b/inventory_management_system_api/repositories/unit.py
@@ -69,7 +69,7 @@ def list(self, session: Optional[ClientSession] = None) -> list[UnitOut]:
 
     def get(
         self, unit_id: str, entity_type_modifier: Optional[str] = None, session: Optional[ClientSession] = None
-    ) -> Optional[UnitOut]:
+    ) -> UnitOut:
         """
         Retrieve a Unit by its ID from a MongoDB database.
 
@@ -78,7 +78,7 @@ def get(
                                      e.g. specified if its for a specified unit.
         :param session: PyMongo ClientSession to use for database operations
         :return: The retrieved unit.
-        :raises MissingRecordError: If the supplied `system_id` is non-existent.
+        :raises MissingRecordError: If the supplied `unit_id` is non-existent.
         """
 
         entity_type = f"{entity_type_modifier} unit" if entity_type_modifier else "unit"
diff --git a/inventory_management_system_api/repositories/usage_status.py b/inventory_management_system_api/repositories/usage_status.py
@@ -67,7 +67,7 @@ def list(self, session: Optional[ClientSession] = None) -> list[UsageStatusOut]:
 
     def get(
         self, usage_status_id: str, entity_type_modifier: Optional[str] = None, session: Optional[ClientSession] = None
-    ) -> Optional[UsageStatusOut]:
+    ) -> UsageStatusOut:
         """
         Retrieve a usage status by its ID from a MongoDB database.
 
diff --git a/inventory_management_system_api/services/catalogue_category_property.py b/inventory_management_system_api/services/catalogue_category_property.py
@@ -78,20 +78,14 @@ def create(
         # Mandatory properties must have a default value that is not None as they would need to be
         # populated down the subtree
         if catalogue_category_property.mandatory and catalogue_category_property.default_value is None:
-            raise InvalidActionError(
-                "Cannot add a mandatory property without a default value",
-                response_detail="Cannot add a mandatory property without a default value",
-            )
+            raise InvalidActionError("Cannot add a mandatory property without a default value")
 
         # Obtain the existing catalogue category to validate against
         stored_catalogue_category = self._catalogue_category_repository.get(catalogue_category_id)
 
         # Must be a leaf catalogue category in order to have properties
         if not stored_catalogue_category.is_leaf:
-            raise InvalidActionError(
-                "Cannot add a property to a non-leaf catalogue category",
-                response_detail="Cannot add a property to a non-leaf catalogue category",
-            )
+            raise InvalidActionError("Cannot add a property to a non-leaf catalogue category")
 
         # Ensure the property is actually valid
         utils.check_duplicate_property_names(stored_catalogue_category.properties + [catalogue_category_property])
@@ -156,34 +150,23 @@ def _check_valid_allowed_values_update(
 
         # Prevent adding allowed_values to an existing property
         if existing_allowed_values is None and new_allowed_values is not None:
-            raise InvalidActionError(
-                "Cannot add allowed_values to an existing property",
-                response_detail="Cannot add allowed_values to an existing property",
-            )
+            raise InvalidActionError("Cannot add allowed_values to an existing property")
 
         # Prevent removing allowed_values from an existing property
         if existing_allowed_values is not None and new_allowed_values is None:
-            raise InvalidActionError(
-                "Cannot remove allowed_values from an existing property",
-                response_detail="Cannot remove allowed_values from an existing property",
-            )
+            raise InvalidActionError("Cannot remove allowed_values from an existing property")
 
         # Prevent changing an allowed_values' type
         if existing_allowed_values.type != new_allowed_values.type:
-            raise InvalidActionError(
-                "Cannot modify a properties' allowed_values to have a different type",
-                response_detail="Cannot modify a properties' allowed_values to have a different type",
-            )
+            raise InvalidActionError("Cannot modify a properties' allowed_values to have a different type")
 
         # Ensure that a list type adds to the existing values (order doesn't matter)
         if existing_allowed_values.type == "list":
             for existing_value in existing_allowed_values.values:
                 if existing_value not in new_allowed_values.values:
                     raise InvalidActionError(
                         "Cannot modify existing values inside allowed_values of type 'list', you may only add more "
-                        "values",
-                        response_detail="Cannot modify existing values inside allowed_values of type 'list', you may "
-                        "only add more values",
+                        "values"
                     )
 
     def update(
diff --git a/inventory_management_system_api/services/catalogue_item.py b/inventory_management_system_api/services/catalogue_item.py
@@ -173,16 +173,14 @@ def update(self, catalogue_item_id: str, catalogue_item: CatalogueItemPatchSchem
                     "specifying the new properties"
                 )
                 if len(current_catalogue_category.properties) != len(catalogue_category.properties):
-                    raise InvalidActionError(invalid_action_error_message, response_detail=invalid_action_error_message)
+                    raise InvalidActionError(invalid_action_error_message)
 
                 old_to_new_id_map = {}
                 for current_catalogue_category_prop, catalogue_category_prop in zip(
                     current_catalogue_category.properties, catalogue_category.properties
                 ):
                     if not current_catalogue_category_prop.is_equal_without_id(catalogue_category_prop):
-                        raise InvalidActionError(
-                            invalid_action_error_message, response_detail=invalid_action_error_message
-                        )
+                        raise InvalidActionError(invalid_action_error_message)
                     old_to_new_id_map[current_catalogue_category_prop.id] = catalogue_category_prop.id
 
                 # The IDs of the properties need to be updated to those of the new catalogue category
diff --git a/inventory_management_system_api/services/item.py b/inventory_management_system_api/services/item.py
@@ -143,10 +143,7 @@ def update(self, item_id: str, item: ItemPatchSchema) -> ItemOut:
             raise MissingRecordError(f"No item found with ID: {item_id}")
 
         if "catalogue_item_id" in update_data and item.catalogue_item_id != stored_item.catalogue_item_id:
-            raise InvalidActionError(
-                "Cannot change the catalogue item the item belongs to",
-                response_detail="Cannot change the catalogue item of an item",
-            )
+            raise InvalidActionError("Cannot change the catalogue item of an item")
 
         if "system_id" in update_data and item.system_id != stored_item.system_id:
             self._system_repository.get(item.system_id, entity_type_modifier="specified")
diff --git a/inventory_management_system_api/services/manufacturer.py b/inventory_management_system_api/services/manufacturer.py
@@ -3,16 +3,13 @@
 """
 
 import logging
-
 from typing import Annotated, List, Optional
+
 from fastapi import Depends
-from inventory_management_system_api.core.exceptions import MissingRecordError
+
 from inventory_management_system_api.models.manufacturer import ManufacturerIn, ManufacturerOut
 from inventory_management_system_api.repositories.manufacturer import ManufacturerRepo
-from inventory_management_system_api.schemas.manufacturer import (
-    ManufacturerPatchSchema,
-    ManufacturerPostSchema,
-)
+from inventory_management_system_api.schemas.manufacturer import ManufacturerPatchSchema, ManufacturerPostSchema
 from inventory_management_system_api.services import utils
 
 logger = logging.getLogger()
@@ -76,8 +73,6 @@ def update(self, manufacturer_id: str, manufacturer: ManufacturerPatchSchema) ->
         :return: The updated manufacturer.
         """
         stored_manufacturer = self.get(manufacturer_id)
-        if not stored_manufacturer:
-            raise MissingRecordError(f"No manufacturer found with ID: {manufacturer_id}")
 
         update_data = manufacturer.model_dump(exclude_unset=True)
 
diff --git a/inventory_management_system_api/services/utils.py b/inventory_management_system_api/services/utils.py
@@ -50,10 +50,7 @@ def check_duplicate_property_names(
     for prop in properties:
         property_name = prop.name.lower().strip()
         if property_name in seen_property_names:
-            raise DuplicateCatalogueCategoryPropertyNameError(
-                f"Duplicate property name: {prop.name.strip()}",
-                response_detail=f"Duplicate property name: {prop.name.strip()}",
-            )
+            raise DuplicateCatalogueCategoryPropertyNameError(f"Duplicate property name: {prop.name.strip()}")
         seen_property_names.add(property_name)
 
 
diff --git a/test/unit/services/test_item.py b/test/unit/services/test_item.py
@@ -773,7 +773,7 @@ def test_update_catalogue_item_id(self):
             stored_item_data=ITEM_DATA_REQUIRED_VALUES_ONLY,
         )
         self.call_update_expecting_error(item_id, InvalidActionError)
-        self.check_update_failed_with_exception("Cannot change the catalogue item the item belongs to")
+        self.check_update_failed_with_exception("Cannot change the catalogue item of an item")
 
     def test_update_system_id(self):
         """Test updating an item's `system_id`."""
diff --git a/test/unit/services/test_manufacturer.py b/test/unit/services/test_manufacturer.py
@@ -5,17 +5,15 @@
 # Expect some duplicate code inside tests as the tests for the different entities can be very similar
 # pylint: disable=duplicate-code
 
-from typing import Optional
-from unittest.mock import MagicMock, Mock, patch
-
 from test.mock_data import MANUFACTURER_POST_DATA_A, MANUFACTURER_POST_DATA_B
 from test.unit.services.conftest import ServiceTestHelpers
+from typing import Optional
+from unittest.mock import MagicMock, Mock, patch
 
 import pytest
 from bson import ObjectId
 
 from inventory_management_system_api.core.custom_object_id import CustomObjectId
-from inventory_management_system_api.core.exceptions import MissingRecordError
 from inventory_management_system_api.models.manufacturer import ManufacturerIn, ManufacturerOut
 from inventory_management_system_api.schemas.manufacturer import ManufacturerPatchSchema, ManufacturerPostSchema
 from inventory_management_system_api.services import utils
@@ -292,16 +290,6 @@ def test_update_address_only(self) -> None:
         self.call_update(manufacturer_id)
         self.check_update_success()
 
-    def test_update_with_non_existent_id(self):
-        """Test updating a manufacturer with a non-existent ID."""
-        manufacturer_id = str(ObjectId())
-
-        self.mock_update(
-            manufacturer_id, manufacturer_patch_data=MANUFACTURER_POST_DATA_A, stored_manufacturer_post_data=None
-        )
-        self.call_update_expecting_error(manufacturer_id, MissingRecordError)
-        self.check_update_failed_with_exception(f"No manufacturer found with ID: {manufacturer_id}")
-
 
 class DeleteDSL(ManufacturerServiceDSL):
     """Base class for `delete` tests."""
