Further cleanup #312

ral-facilities · Jul 11, 2024 · 38edd54 · 38edd54
1 parent 7af8942
commit 38edd54
Show file tree

Hide file tree

Showing 6 changed files with 137 additions and 273 deletions.
diff --git a/inventory_management_system_api/repositories/manufacturer.py b/inventory_management_system_api/repositories/manufacturer.py
@@ -21,9 +21,7 @@
 
 
 class ManufacturerRepo:
-    """
-    Repository for managing manufacturers in a MongoDb database.
-    """
+    """Repository for managing manufacturers in a MongoDb database."""
 
     def __init__(self, database: DatabaseDep) -> None:
         """
@@ -89,8 +87,8 @@ def update(
         :param manufacturer_id: The ID of the manufacturer to update.
         :param manufacturer: The manufacturer containing the update data.
         :param session: PyMongo ClientSession to use for database operations.
-        :raises: DuplicateRecordError: if a duplicate manufacturer is found.
-        :return: the updated manufacturer.
+        :raises DuplicateRecordError: If a duplicate manufacturer is found.
+        :return: The updated manufacturer.
         """
         manufacturer_id = CustomObjectId(manufacturer_id)
 
@@ -111,13 +109,13 @@ def delete(self, manufacturer_id: str, session: ClientSession = None) -> None:
         """
         Delete a manufacturer by its ID from a MongoDB database.
 
-        The method Checks if the manufacturer is part of a catalogue item, and raises a `PartOfCatalogueItemError` if it
+        The method checks if the manufacturer is part of a catalogue item, and raises a `PartOfCatalogueItemError` if it
         is.
 
         :param manufacturer_id: The ID of the manufacturer to delete.
         :param session: PyMongo ClientSession to use for database operations.
-        :raises PartOfCatalogueItemError: if the manufacturer is part of a catalogue item.
-        :raises MissingRecordError: if the manufacturer doesn't exist.
+        :raises PartOfCatalogueItemError: If the manufacturer is part of a catalogue item.
+        :raises MissingRecordError: If the manufacturer doesn't exist.
         """
         manufacturer_id = CustomObjectId(manufacturer_id)
         if self._is_manufacturer_in_catalogue_item(str(manufacturer_id), session=session):
@@ -137,7 +135,7 @@ def _is_duplicate_manufacturer(
         :param code: The code of the manufacturer to check for duplicates.
         :param manufacturer_id: The ID of the manufacturer to check if the duplicate manufacturer found is itself.
         :param session: PyMongo ClientSession to use for database operations.
-        :return `True` if a duplicate manufacturer is found, `False` otherwise.
+        :return: `True` if a duplicate manufacturer is found, `False` otherwise.
         """
         logger.info("Checking if manufacturer with code '%s' already exists", code)
         manufacturer = self._manufacturers_collection.find_one(
@@ -151,7 +149,7 @@ def _is_manufacturer_in_catalogue_item(self, manufacturer_id: str, session: Clie
 
         :param manufacturer_id: The ID of the manufacturer to check.
         :param session: PyMongo ClientSession to use for database operations.
-        :return: Returns `True` if the manufacturer is part of a catalogue item, `False` otherwise.
+        :return: `True` if the manufacturer is part of a catalogue item, `False` otherwise.
         """
         manufacturer_id = CustomObjectId(manufacturer_id)
         return (

diff --git a/inventory_management_system_api/services/manufacturer.py b/inventory_management_system_api/services/manufacturer.py
@@ -19,9 +19,7 @@
 
 
 class ManufacturerService:
-    """
-    Service for managing manufacturers.
-    """
+    """Service for managing manufacturers."""
 
     def __init__(
         self,

diff --git a/test/e2e/test_manufacturer.py b/test/e2e/test_manufacturer.py
@@ -1,6 +1,4 @@
-"""
-End-to-End tests for the manufacturer router.
-"""
+"""End-to-End tests for the manufacturer router."""
 
 # Expect some duplicate code inside tests as the tests for the different entities can be very similar
 # pylint: disable=duplicate-code
@@ -36,7 +34,8 @@ def post_manufacturer(self, manufacturer_post_data: dict) -> Optional[str]:
         """
         Posts a manufacturer with the given data, returns the ID of the created manufacturer if successful.
 
-        :param manufacturer_post_data: Dictionary containing the manufacturer data that should be posted.
+        :param manufacturer_post_data: Dictionary containing the manufacturer data as would be required for a
+            `ManufacturerPostSchema`.
         :return: ID of the created manufacturer (or `None` if not successful).
         """
         self._post_response = self.test_client.post("/v1/manufacturers", json=manufacturer_post_data)
@@ -46,8 +45,8 @@ def check_post_manufacturer_success(self, expected_manufacturer_get_data: dict)
         """
         Checks that a prior call to `post_manufacturer` gave a successful response with the expected data returned.
 
-        :param expected_manufacturer_get_data: Dictionary containing the expected manufacturer data that should be
-            returned.
+        :param expected_manufacturer_get_data: Dictionary containing the expected manufacturer data as would be required
+            for a `ManufacturerSchema`.
         """
         assert self._post_response.status_code == 201
         assert self._post_response.json() == expected_manufacturer_get_data
@@ -77,7 +76,7 @@ def check_post_manufacturer_failed_with_validation_message(self, status_code: in
 class TestCreate(CreateDSL):
     """Tests for creating a manufacturer."""
 
-    def test_create_with_only_require_values_provided(self):
+    def test_create_with_only_required_values_provided(self):
         """Test creating a manufacturer with only required values provided."""
         self.post_manufacturer(MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY)
         self.check_post_manufacturer_success(MANUFACTURER_GET_DATA_REQUIRED_VALUES_ONLY)
@@ -111,8 +110,8 @@ def check_get_manufacturer_success(self, expected_manufacturer_get_data: dict) -
         """
         Checks that a prior call to `get_manufacturer` gave a successful response with the expected data returned.
 
-        :param expected_manufacturer_get_data: Dictionary containing the expected manufacturer data that should be
-            returned.
+        :param expected_manufacturer_get_data: Dictionary containing the expected manufacturer data as would be required
+            for a `ManufacturerSchema`.
         """
         assert self._get_response.status_code == 200
         assert self._get_response.json() == expected_manufacturer_get_data
@@ -159,8 +158,8 @@ def check_get_manufacturers_success(self, expected_manufacturers_get_data: list[
         """
         Checks that a prior call to `get_manufacturers` gave a successful response with the expected data returned.
 
-        :param expected_manufacturers_get_data: List of dictionaries containing the expected manufacturer data that
-            should be returned.
+        :param expected_manufacturers_get_data: List of dictionaries containing the expected manufacturer data as would
+            be required for a `ManufacturerSchema`.
         """
         assert self._get_response.status_code == 200
         assert self._get_response.json() == expected_manufacturers_get_data
@@ -194,7 +193,8 @@ def patch_manufacturer(self, manufacturer_id: str, manufacturer_patch_data: dict
         Updates a manufacturer with the given ID.
 
         :param manufacturer_id: ID of the manufacturer to be updated.
-        :param manufacturer_patch_data: Dictionary containing the manufacturer patch data.
+        :param manufacturer_patch_data: Dictionary containing the manufacturer patch data as would be required for a
+            `ManufacturerPatchSchema`.
         """
         self._patch_response = self.test_client.patch(
             f"/v1/manufacturers/{manufacturer_id}", json=manufacturer_patch_data
@@ -204,8 +204,8 @@ def check_patch_manufacturer_success(self, expected_manufacturer_get_data: dict)
         """
         Checks that a prior call to `patch_manufacturer` gave a successful response with the expected data returned.
 
-        :param expected_manufacturer_get_data: Dictionaries containing the expected manufacturer data that should be
-            returned.
+        :param expected_manufacturer_get_data: Dictionaries containing the expected manufacturer data as would be
+            required for a `ManufacturerSchema`.
         """
         assert self._patch_response.status_code == 200
         assert self._patch_response.json() == expected_manufacturer_get_data
@@ -232,7 +232,6 @@ def test_partial_update_all_fields(self):
 
     def test_partial_update_name_to_duplicate(self):
         """Test updating the name of a manufacturer to conflict with a pre-existing one."""
-
         self.post_manufacturer(MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY)
         system_id = self.post_manufacturer(MANUFACTURER_POST_DATA_ALL_VALUES)
         self.patch_manufacturer(system_id, {"name": MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY["name"]})
@@ -251,7 +250,6 @@ def test_partial_update_name_capitalisation(self):
 
     def test_partial_update_with_non_existent_id(self):
         """Test updating a non-existent manufacturer."""
-
         self.patch_manufacturer(str(ObjectId()), {})
         self.check_patch_manufacturer_failed_with_detail(404, "Manufacturer not found")
 
@@ -280,7 +278,7 @@ def check_delete_manufacturer_success(self) -> None:
 
     def check_delete_manufacturer_failed_with_detail(self, status_code: int, detail: str) -> None:
         """
-        Checks that a prior call to 'delete_manufacturer' gave a failed response with the expected code and detail.
+        Checks that a prior call to `delete_manufacturer` gave a failed response with the expected code and detail.
 
         :param status_code: Expected status code to be returned.
         :param detail: Expected detail to be returned.

diff --git a/test/mock_data.py b/test/mock_data.py
@@ -18,41 +18,7 @@
 
 # --------------------------------- MANUFACTURERS ---------------------------------
 
-MANUFACTURER_POST_DATA_A = {
-    "name": "Manufacturer A",
-    "url": "http://testurl.co.uk/",
-    "address": {
-        "address_line": "1 Example Street",
-        "town": "Oxford",
-        "county": "Oxfordshire",
-        "country": "United Kingdom",
-        "postcode": "OX1 2AB",
-    },
-    "telephone": "0932348348",
-}
-
-MANUFACTURER_POST_DATA_B = {
-    "name": "Manufacturer B",
-    "url": "http://example.co.uk/",
-    "address": {
-        "address_line": "2 Example Street",
-        "town": "Oxford",
-        "county": "Oxfordshire",
-        "country": "United Kingdom",
-        "postcode": "OX1 2AB",
-    },
-    "telephone": "073434394",
-}
-
-MANUFACTURER_IN_DATA_A = {
-    **MANUFACTURER_POST_DATA_A,
-    "code": "manufacturer-a",
-}
-
-MANUFACTURER_IN_DATA_B = {
-    **MANUFACTURER_POST_DATA_B,
-    "code": "manufacturer-b",
-}
+# Required values only
 
 MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY = {
     "name": "Manufacturer Test Required Values Only",
@@ -73,9 +39,14 @@
     "telephone": None,
 }
 
+# All values
+
 MANUFACTURER_POST_DATA_ALL_VALUES = {
-    **MANUFACTURER_POST_DATA_A,
+    **MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY,
+    "address": {**MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY["address"], "town": "Oxford", "county": "Oxfordshire"},
     "name": "Manufacturer Test All Values",
+    "url": "http://testurl.co.uk/",
+    "telephone": "0932348348",
 }
 
 MANUFACTURER_GET_DATA_ALL_VALUES = {
@@ -85,6 +56,29 @@
     "code": "manufacturer-test-all-values",
 }
 
+MANUFACTURER_POST_DATA_A = {
+    **MANUFACTURER_POST_DATA_ALL_VALUES,
+    "name": "Manufacturer A",
+}
+
+MANUFACTURER_IN_DATA_A = {
+    **MANUFACTURER_POST_DATA_A,
+    "code": "manufacturer-a",
+}
+
+MANUFACTURER_POST_DATA_B = {
+    **MANUFACTURER_POST_DATA_ALL_VALUES,
+    "address": {**MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY["address"], "address_line": "2 Example Street"},
+    "name": "Manufacturer B",
+    "url": "http://example.co.uk/",
+    "telephone": "073434394",
+}
+
+MANUFACTURER_IN_DATA_B = {
+    **MANUFACTURER_POST_DATA_B,
+    "code": "manufacturer-b",
+}
+
 # --------------------------------- SYSTEMS ---------------------------------
 
 SYSTEM_POST_DATA_NO_PARENT_A = {