Address comments from CC repo unit tests PR #305

Author: joelvdavies

inventory_management_system_api/repositories/system.py

@@ -202,7 +202,7 @@ def _has_child_elements(self, system_id: CustomObjectId, session: ClientSession
 
         :param system_id: ID of the system to check
         :param session: PyMongo ClientSession to use for database operations
-        :return: True if the system has child elements, False otherwise
+        :return: `True` if the system has child elements, `False` otherwise
         """
         logger.info("Checking if system with ID '%s' has child elements", str(system_id))
 

test/e2e/test_catalogue_category.py

@@ -842,7 +842,7 @@ def test_partial_update_parent_id_to_one_with_a_duplicate_name(self):
         """Test updating the parent_id of a catalogue category so that its name conflicts with one already in that
         other catalogue category."""
 
-        # System with child
+        # Catalogue category with child
         parent_id = self.post_catalogue_category(CATALOGUE_CATEGORY_POST_DATA_NON_LEAF_NO_PARENT_NO_PROPERTIES_A)
         self.post_catalogue_category(
             {

test/unit/repositories/test_catalogue_category.py

@@ -72,7 +72,7 @@ def setup(self, database_mock):
 
         # Here we only wrap the utils so they keep their original functionality - this is done here
         # to avoid having to mock the code generation function as the output will be passed to
-        # CatalogueCategoryOut with pydantic validation and so will error otherwise
+        # `CatalogueCategoryOut` with pydantic validation and so will error otherwise
         with patch("inventory_management_system_api.repositories.catalogue_category.utils") as mock_utils:
             self.mock_utils = mock_utils
             yield
@@ -97,7 +97,7 @@ def check_has_child_elements_performed_expected_calls(self, expected_catalogue_c
         """
         Checks that a call to `has_child_elements` performed the expected function calls.
 
-        :param expected_catalogue_category_id: Expected catalogue category id used in the database calls.
+        :param expected_catalogue_category_id: Expected `catalogue_category_id` used in the database calls.
         """
 
         self.catalogue_categories_collection.find_one.assert_called_once_with(
@@ -137,7 +137,7 @@ def mock_create(
 
         inserted_catalogue_category_id = CustomObjectId(str(ObjectId()))
 
-        # Pass through CatalogueCategoryIn first as need creation and modified times
+        # Pass through `CatalogueCategoryIn` first as need creation and modified times
         self._catalogue_category_in = CatalogueCategoryIn(**catalogue_category_in_data)
 
         self._expected_catalogue_category_out = CatalogueCategoryOut(
@@ -177,7 +177,7 @@ def mock_create(
         )
 
     def call_create(self) -> None:
-        """Calls the CatalogueCategoryRepo `create` method with the appropriate data from a prior call to
+        """Calls the `CatalogueCategoryRepo` `create` method with the appropriate data from a prior call to
         `mock_create`"""
 
         self._created_catalogue_category = self.catalogue_category_repository.create(
@@ -186,7 +186,7 @@ def call_create(self) -> None:
 
     def call_create_expecting_error(self, error_type: type[BaseException]) -> None:
         """
-        Calls the CatalogueCategoryRepo `create` method with the appropriate data from a prior call to `mock_create`
+        Calls the `CatalogueCategoryRepo` `create` method with the appropriate data from a prior call to `mock_create`
         while expecting an error to be raised
 
         :param error_type: Expected exception to be raised.
@@ -280,7 +280,7 @@ def test_create_with_parent(self):
         self.check_create_success()
 
     def test_create_with_non_existent_parent_id(self):
-        """Test creating a catalogue category with a non existent parent_id."""
+        """Test creating a catalogue category with a non-existent `parent_id`."""
 
         parent_id = str(ObjectId())
 
@@ -490,7 +490,7 @@ def mock_list(self, catalogue_categories_in_data: list[dict]) -> None:
         """Mocks database methods appropriately to test the `list` repo method
 
         :param catalogue_categories_in_data: List of dictionaries containing the catalogue category data as would be
-                                             required for a `CatalogueCategoryIn` database model (i.e. no id or created
+                                             required for a `CatalogueCategoryIn` database model (i.e. no ID or created
                                              and modified times required)
         """
 
@@ -508,7 +508,7 @@ def mock_list(self, catalogue_categories_in_data: list[dict]) -> None:
 
     def call_list(self, parent_id: Optional[str]) -> None:
         """
-        Calls the CatalogueCategoryRepo `list` method.
+        Calls the `CatalogueCategoryRepo` `list` method.
 
         :param parent_id: ID of the parent catalogue category to query by, or `None`.
         """
@@ -534,7 +534,7 @@ class TestList(ListDSL):
     """Tests for listing Catalogue Category's."""
 
     def test_list(self):
-        """Test listing all Catalogue Categories."""
+        """Test listing all catalogue categories."""
 
         self.mock_list(
             [
@@ -546,7 +546,7 @@ def test_list(self):
         self.check_list_success()
 
     def test_list_with_parent_id_filter(self):
-        """Test listing all Catalogue Categories with a given parent_id."""
+        """Test listing all catalogue categories with a given `parent_id`."""
 
         self.mock_list(
             [
@@ -558,7 +558,7 @@ def test_list_with_parent_id_filter(self):
         self.check_list_success()
 
     def test_list_with_null_parent_id_filter(self):
-        """Test listing all Catalogue Categories with a 'null' parent_id."""
+        """Test listing all catalogue categories with a 'null' parent_id."""
 
         self.mock_list(
             [
@@ -570,7 +570,7 @@ def test_list_with_null_parent_id_filter(self):
         self.check_list_success()
 
     def test_list_with_parent_id_with_no_results(self):
-        """Test listing all Catalogue Categories with a `parent_id` filter returning no results."""
+        """Test listing all catalogue categories with a `parent_id` filter returning no results."""
 
         self.mock_list([])
         self.call_list(parent_id=str(ObjectId()))
@@ -625,7 +625,7 @@ def mock_update(
         :param duplicate_catalogue_category_in_data: Either `None` or a dictionary containing the data for a duplicate
                                                      catalogue category as would be required for a `CatalogueCategoryIn`
                                                      database model.
-        :param valid_move_result: Whether to mock in a valid or invalid move result i.e. when True will simulating
+        :param valid_move_result: Whether to mock in a valid or invalid move result i.e. when `True` will simulate
                                   moving the catalogue category to one of its own children.
         """
         self.set_update_data(new_catalogue_category_in_data)
@@ -700,7 +700,7 @@ def mock_update(
 
     def call_update(self, catalogue_category_id: str) -> None:
         """
-        Calls the CatalogueCategoryRepo `update` method with the appropriate data from a prior call to `mock_update`
+        Calls the `CatalogueCategoryRepo` `update` method with the appropriate data from a prior call to `mock_update`
         (or`set_update_data`).
 
         :param catalogue_category_id: ID of the catalogue category to be updated.
@@ -713,7 +713,7 @@ def call_update(self, catalogue_category_id: str) -> None:
 
     def call_update_expecting_error(self, catalogue_category_id: str, error_type: type[BaseException]) -> None:
         """
-        Calls the CatalogueCategoryRepo `update` method with the appropriate data from a prior call to `mock_update`
+        Calls the `CatalogueCategoryRepo` `update` method with the appropriate data from a prior call to `mock_update`
         (or `set_update_data`) while expecting an error to be raised.
 
         :param catalogue_category_id: ID of the catalogue category to be updated.
@@ -1024,7 +1024,7 @@ def call_delete(self, catalogue_category_id: str) -> None:
 
     def call_delete_expecting_error(self, catalogue_category_id: str, error_type: type[BaseException]) -> None:
         """
-        Calls the CatalogueCategoryRepo `delete` method while expecting an error to be raised.
+        Calls the `CatalogueCategoryRepo` `delete` method while expecting an error to be raised.
 
         :param catalogue_category_id: ID of the catalogue category to be deleted.
         :param error_type: Expected exception to be raised.

test/unit/repositories/test_system.py

@@ -59,7 +59,7 @@ def setup(self, database_mock):
         self.mock_session = MagicMock()
 
         # Here we only wrap the utils so they keep their original functionality - this is done here
-        # to avoid having to mock the code generation function as the output will be passed to SystemOut
+        # to avoid having to mock the code generation function as the output will be passed to `SystemOut`
         # with pydantic validation and so will error otherwise
         with patch("inventory_management_system_api.repositories.system.utils") as mock_utils:
             self.mock_utils = mock_utils
@@ -71,8 +71,8 @@ def mock_has_child_elements(
         """
         Mocks database methods appropriately for when the `_has_child_elements` repo method will be called.
 
-        :param child_system_data: Dictionary containing a child system's data (or None).
-        :param child_catalogue_item_data: Dictionary containing a child Item's data (or None).
+        :param child_system_data: Dictionary containing a child system's data (or `None`).
+        :param child_catalogue_item_data: Dictionary containing a child Item's data (or `None`).
         """
 
         self._mock_child_system_data = child_system_data
@@ -85,7 +85,7 @@ def check_has_child_elements_performed_expected_calls(self, expected_system_id:
         """
         Checks that a call to `_has_child_elements` performed the expected function calls.
 
-        :param expected_system_id: Expected system id used in the database calls.
+        :param expected_system_id: Expected `system_id` used in the database calls.
         """
 
         self.systems_collection.find_one.assert_called_once_with(
@@ -101,7 +101,7 @@ def mock_is_duplicate_system(self, duplicate_system_in_data: Optional[dict] = No
         """
         Mocks database methods appropriately for when the `_is_duplicate_system` repo method will be called.
 
-        :param duplicate_system_in_data: Either None or a dictionary containing system data for a duplicate system.
+        :param duplicate_system_in_data: Either `None` or a dictionary containing system data for a duplicate system.
         """
 
         RepositoryTestHelpers.mock_find_one(
@@ -117,10 +117,10 @@ def get_is_duplicate_system_expected_find_one_call(
         self, system_in: SystemIn, expected_system_id: Optional[CustomObjectId]
     ):
         """
-        Returns the expected find_one calls from that should occur when `_is_duplicate_system` is called.
+        Returns the expected `find_one` calls from that should occur when `_is_duplicate_system` is called.
 
         :param system_in: `SystemIn` model containing the data about the system.
-        :param expected_system_id: Expected system_id provided to `is_duplicate_system`.
+        :param expected_system_id: Expected `system_id` provided to `is_duplicate_system`.
         """
 
         return call(
@@ -158,7 +158,7 @@ def mock_create(
         """
         inserted_system_id = CustomObjectId(str(ObjectId()))
 
-        # Pass through SystemIn first as need creation and modified times
+        # Pass through `SystemIn` first as need creation and modified times
         self._system_in = SystemIn(**system_in_data)
 
         self._expected_system_out = SystemOut(**self._system_in.model_dump(), id=inserted_system_id)
@@ -377,7 +377,7 @@ def test_get_with_invalid_id(self):
 
 
 class GetBreadcrumbsDSL(SystemRepoDSL):
-    """Base class for `get_breadcrumbs` tests"""
+    """Base class for `get_breadcrumbs` tests."""
 
     _breadcrumbs_query_result: list[dict]
     _mock_aggregation_pipeline = MagicMock()
@@ -386,10 +386,10 @@ class GetBreadcrumbsDSL(SystemRepoDSL):
     _obtained_breadcrumbs: MagicMock
 
     def mock_breadcrumbs(self, breadcrumbs_query_result: list[dict]) -> None:
-        """Mocks database methods appropriately to test the `get_breadcrumbs` repo method
+        """Mocks database methods appropriately to test the `get_breadcrumbs` repo method.
 
         :param breadcrumbs_query_result: List of dictionaries containing the breadcrumbs query result from the
-                                         aggregation pipeline
+                                         aggregation pipeline.
         """
         self._breadcrumbs_query_result = breadcrumbs_query_result
         self._mock_aggregation_pipeline = MagicMock()
@@ -447,7 +447,7 @@ def mock_list(self, systems_in_data: list[dict]):
         """Mocks database methods appropriately to test the `list` repo method.
 
         :param systems_in_data: List of dictionaries containing the system data as would be required for a
-                                `SystemIn` database model (i.e. no id or created and modified times required).
+                                `SystemIn` database model (i.e. no ID or created and modified times required).
         """
 
         self._expected_systems_out = [
@@ -528,7 +528,7 @@ def set_update_data(self, new_system_in_data: dict):
         """
         Assigns the update data to use during a call to `call_update`.
 
-        :param new_system_data: New system data as would be required for a SystemIn database model to supply to the
+        :param new_system_data: New system data as would be required for a `SystemIn` database model to supply to the
                                 `SystemRepo` `update` method.
         """
         self._system_in = SystemIn(**new_system_in_data)
@@ -544,19 +544,19 @@ def mock_update(
         valid_move_result: bool = True,
     ) -> None:
         """
-        Mocks database methods appropriately to test the `update` repo method
+        Mocks database methods appropriately to test the `update` repo method.
 
-        :param system_id: ID of the system to be updated
-        :param new_system_in_data: Dictionary containing the new system data as would be required for a SystemIn
-                                   database model (i.e. no ID or created and modified times required)
+        :param system_id: ID of the system to be updated.
+        :param new_system_in_data: Dictionary containing the new system data as would be required for a `SystemIn`
+                                   database model (i.e. no ID or created and modified times required).
         :param stored_system_in_data: Dictionary containing the system data for the existing stored system
-                                      as would be required for a `SystemIn` database model
+                                      as would be required for a `SystemIn` database model.
         :param new_parent_system_in_data: Either `None` or a dictionary containing the new parent system data as would
-                                          be required for a `SystemIn` database model
+                                          be required for a `SystemIn` database model.
         :param duplicate_system_in_data: Either `None` or a dictionary containing the data for a duplicate system as
-                                         would be required for a `SystemIn` database model
-        :param valid_move_result: Whether to mock in a valid or invalid move result i.e. when True will simulating
-                                  moving the system to one of its own children
+                                         would be required for a `SystemIn` database model.
+        :param valid_move_result: Whether to mock in a valid or invalid move result i.e. when `True` will simulate
+                                  moving the system to one of its own children.
         """
         self.set_update_data(new_system_in_data)
 

test/unit/services/test_catalogue_category.py

@@ -170,7 +170,7 @@ def mock_create(
         :param catalogue_category_data: Dictionary containing the basic catalogue category data as would be required
                                         for a `CatalogueCategoryPostSchema` but with any unit_id's replaced by the
                                         'unit' value in its properties as the ids will be added automatically.
-        :param parent_catalogue_category_in_data: Either None or a dictionary containing the parent catalogue category
+        :param parent_catalogue_category_in_data: Either `None` or a dictionary containing the parent catalogue category
                                                   data as would be required for a `CatalogueCategoryIn` database model.
         :param units_in_data: Either `None` or a list of dictionaries (or `None`) containing the unit data as would be
                               required for a `UnitIn` database model. These values will be used for any unit look ups
@@ -506,7 +506,7 @@ def mock_update(
                                                'unit' value in its properties as the ids will be added automatically.
         :param stored_catalogue_category_post_data: Dictionary containing the catalogue category data for the existing
                                                stored catalogue category as would be required for a
-                                               `CatalogueCategoryPostSchema` (i.e. no id, code or created and modified
+                                               `CatalogueCategoryPostSchema` (i.e. no ID, code or created and modified
                                                times required).
         :param has_child_elements: Boolean of whether the category being updated has child elements or not
         :param new_parent_catalogue_category_in_data: Either `None` or a dictionary containing the new parent catalogue