Update and add more tests #417

inventory_management_system_api/services/catalogue_item.py

@@ -97,8 +97,6 @@ def create(self, catalogue_item: CatalogueItemPostSchema) -> CatalogueItemOut:
         # Perform actual creation in a transaction as we need to ensure they cannot be added when in the process of
         # recalculating spares through the spares definition being set
         with start_session_transaction("creating catalogue item") as session:
-            # TODO: Update tests to include this new logic
-            # TODO: Handle if setting doesn't exist
             # Write lock the spares definition - if this fails it either means another catalogue item is being created
             # (unlikely) or that the spares definition has been set and is still recalculating
             self._setting_repository.write_lock(SparesDefinitionOut, session)

test/e2e/test_setting.py

@@ -309,6 +309,13 @@ def check_catalogue_items_spares(self, expected_number_of_spares_list: list[Opti
 class TestSparesDefinitionDSL(SparesDefinitionDSL):
     """Tests for spares definition."""
 
+    def test_create_catalogue_item_after_spares_definition_set(self):
+        """Test creating a catalogue item after the spares definition is set."""
+
+        self.put_spares_definition_and_post_prerequisites(SETTING_SPARES_DEFINITION_DATA_NEW)
+        self.post_items_and_prerequisites_with_usage_statuses([[]])
+        self.check_catalogue_items_spares([0])
+
     def test_set_spares_definition_with_existing_items(self):
         """Test setting the spares definition when there are existing items."""
 

test/unit/repositories/test_setting.py

@@ -138,33 +138,33 @@ class GetDSL(SettingRepoDSL):
 
     _obtained_out_model_type: Type[SettingOutBaseT]
     _expected_setting_out: SettingOutBaseT
+    _expect_return_before_aggregate: bool
     _obtained_setting: Optional[SettingOutBaseT]
 
     def mock_get(
         self,
         out_model_type: Type[SettingOutBaseT],
+        setting_database_data: Optional[dict],
         setting_out_data: Optional[dict],
     ) -> None:
         """
         Mocks database methods appropriately to test the `get` repo method.
 
         :param out_model_type: The type of the setting's output model to be obtained.
+        :param setting_database_data: Either `None` or a dictionary containing the setting data as would be returned
+                                      by a `find_one` query.
         :param setting_out_data: Either `None` or a dictionary containing the setting data as would be required for the
                                  `Out` database model.
         """
         self._expected_setting_out = out_model_type(**setting_out_data) if setting_out_data is not None else None
+        self._expect_return_before_aggregate = setting_database_data is None or (
+            len(setting_database_data.keys()) == 2 and "_lock" in setting_database_data
+        )
+
+        RepositoryTestHelpers.mock_find_one(self.settings_collection, setting_database_data)
 
         if out_model_type is SparesDefinitionOut:
             self.settings_collection.aggregate.return_value = [setting_out_data] if setting_out_data is not None else []
-        else:
-            RepositoryTestHelpers.mock_find_one(
-                self.settings_collection,
-                (
-                    self._expected_setting_out.model_dump(by_alias=True)
-                    if self._expected_setting_out is not None
-                    else None
-                ),
-            )
 
     def call_get(self, out_model_type: Type[SettingOutBaseT]) -> None:
         """
@@ -179,14 +179,15 @@ def call_get(self, out_model_type: Type[SettingOutBaseT]) -> None:
     def check_get_success(self) -> None:
         """Checks that a prior call to `call_get` worked as expected."""
 
-        if self._obtained_out_model_type is SparesDefinitionOut:
-            self.settings_collection.aggregate.assert_called_once_with(
-                SPARES_DEFINITION_GET_AGGREGATION_PIPELINE, session=self.mock_session
-            )
-        else:
-            self.settings_collection.find_one.assert_called_once_with(
-                {"_id": self._obtained_out_model_type.SETTING_ID}, session=self.mock_session
-            )
+        self.settings_collection.find_one.assert_called_once_with(
+            {"_id": self._obtained_out_model_type.SETTING_ID}, session=self.mock_session
+        )
+
+        if not self._expect_return_before_aggregate:
+            if self._obtained_out_model_type is SparesDefinitionOut:
+                self.settings_collection.aggregate.assert_called_once_with(
+                    SPARES_DEFINITION_GET_AGGREGATION_PIPELINE, session=self.mock_session
+                )
 
         assert self._obtained_setting == self._expected_setting_out
 
@@ -197,28 +198,48 @@ class TestGet(GetDSL):
     def test_get(self):
         """Test getting a setting."""
 
-        self.mock_get(ExampleSettingOut, {"_id": "example_setting"})
+        self.mock_get(ExampleSettingOut, {"_id": "example_setting"}, {"_id": "example_setting"})
         self.call_get(ExampleSettingOut)
         self.check_get_success()
 
     def test_get_non_existent(self):
         """Test getting a setting that is non-existent."""
 
-        self.mock_get(ExampleSettingOut, None)
+        self.mock_get(ExampleSettingOut, None, None)
+        self.call_get(ExampleSettingOut)
+        self.check_get_success()
+
+    def test_get_existent_but_not_assigned(self):
+        """Test getting a setting that is existent but only due to a write lock."""
+
+        self.mock_get(ExampleSettingOut, {"_id": "example_setting", "_lock": None}, None)
         self.call_get(ExampleSettingOut)
         self.check_get_success()
 
     def test_get_spares_definition(self):
         """Test getting the spares definition setting."""
 
-        self.mock_get(SparesDefinitionOut, SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED)
+        self.mock_get(
+            SparesDefinitionOut,
+            SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED,
+            SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED,
+        )
         self.call_get(SparesDefinitionOut)
         self.check_get_success()
 
     def test_get_non_existent_spares_definition(self):
         """Test getting the spares definition setting when it is non-existent."""
 
-        self.mock_get(SparesDefinitionOut, None)
+        self.mock_get(SparesDefinitionOut, None, None)
+        self.call_get(SparesDefinitionOut)
+        self.check_get_success()
+
+    def test_get_existent_spares_definition_but_not_assinged(self):
+        """Test getting the spares definition setting when it is existent but only due to a write lock."""
+
+        self.mock_get(
+            SparesDefinitionOut, {"_id": SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED["_id"], "_lock": None}, None
+        )
         self.call_get(SparesDefinitionOut)
         self.check_get_success()
 

test/unit/services/conftest.py

@@ -161,18 +161,26 @@ def fixture_catalogue_category_property_service(
 
 @pytest.fixture(name="catalogue_item_service")
 def fixture_catalogue_item_service(
-    catalogue_item_repository_mock: Mock, catalogue_category_repository_mock: Mock, manufacturer_repository_mock: Mock
+    catalogue_item_repository_mock: Mock,
+    catalogue_category_repository_mock: Mock,
+    manufacturer_repository_mock: Mock,
+    setting_repository_mock: Mock,
 ) -> CatalogueItemService:
     """
-    Fixture to create a `CatalogueItemService` instance with a mocked `CatalogueItemRepo` and `CatalogueCategoryRepo`
-    dependencies.
+    Fixture to create a `CatalogueItemService` instance with a mocked `CatalogueItemRepo`, `CatalogueCategoryRepo`,
+    `ManufacturerRepo` and `SettingRepo` dependencies.
 
     :param catalogue_item_repository_mock: Mocked `CatalogueItemRepo` instance.
     :param catalogue_category_repository_mock: Mocked `CatalogueCategoryRepo` instance.
+    :param manufacturer_repository_mock: Mocked `ManufacturerRepo` instance.
+    :param setting_repository_mock: Mocked `SettingRepo` instance.
     :return: `CatalogueItemService` instance with the mocked dependencies.
     """
     return CatalogueItemService(
-        catalogue_item_repository_mock, catalogue_category_repository_mock, manufacturer_repository_mock
+        catalogue_item_repository_mock,
+        catalogue_category_repository_mock,
+        manufacturer_repository_mock,
+        setting_repository_mock,
     )
 
 

test/unit/services/test_catalogue_item.py

@@ -5,6 +5,9 @@
 # Expect some duplicate code inside tests as the tests for the different entities can be very similar
 # pylint: disable=too-many-lines
 # pylint: disable=duplicate-code
+# pylint:disable=too-many-arguments
+# pylint:disable=too-many-positional-arguments
+# pylint:disable=too-many-instance-attributes
 
 from test.mock_data import (
     BASE_CATALOGUE_CATEGORY_IN_DATA_WITH_PROPERTIES_MM,
@@ -18,6 +21,7 @@
     CATALOGUE_ITEM_DATA_WITH_ALL_PROPERTIES,
     MANUFACTURER_IN_DATA_A,
     PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42,
+    SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED,
 )
 from test.unit.services.conftest import BaseCatalogueServiceDSL, ServiceTestHelpers
 from typing import Optional
@@ -36,6 +40,7 @@
 from inventory_management_system_api.models.catalogue_category import CatalogueCategoryIn, CatalogueCategoryOut
 from inventory_management_system_api.models.catalogue_item import CatalogueItemIn, CatalogueItemOut
 from inventory_management_system_api.models.manufacturer import ManufacturerIn, ManufacturerOut
+from inventory_management_system_api.models.setting import SparesDefinitionOut
 from inventory_management_system_api.schemas.catalogue_item import (
     CATALOGUE_ITEM_WITH_CHILD_NON_EDITABLE_FIELDS,
     CatalogueItemPatchSchema,
@@ -53,6 +58,8 @@ class CatalogueItemServiceDSL(BaseCatalogueServiceDSL):
     mock_catalogue_category_repository: Mock
     mock_manufacturer_repository: Mock
     mock_unit_repository: Mock
+    mock_setting_repository: Mock
+    mock_start_session_transaction: Mock
     catalogue_item_service: CatalogueItemService
 
     # pylint:disable=too-many-arguments
@@ -64,6 +71,7 @@ def setup(
         catalogue_category_repository_mock,
         manufacturer_repository_mock,
         unit_repository_mock,
+        setting_repository_mock,
         catalogue_item_service,
         # Ensures all created and modified times are mocked throughout
         # pylint: disable=unused-argument
@@ -75,17 +83,23 @@ def setup(
         self.mock_catalogue_category_repository = catalogue_category_repository_mock
         self.mock_manufacturer_repository = manufacturer_repository_mock
         self.mock_unit_repository = unit_repository_mock
+        self.mock_setting_repository = setting_repository_mock
         self.catalogue_item_service = catalogue_item_service
 
         with patch("inventory_management_system_api.services.catalogue_item.utils", wraps=utils) as wrapped_utils:
-            self.wrapped_utils = wrapped_utils
-            yield
+            with patch(
+                "inventory_management_system_api.services.catalogue_item.start_session_transaction"
+            ) as mocked_start_session_transaction:
+                self.wrapped_utils = wrapped_utils
+                self.mock_start_session_transaction = mocked_start_session_transaction
+                yield
 
 
 class CreateDSL(CatalogueItemServiceDSL):
     """Base class for `create` tests."""
 
     _catalogue_category_out: Optional[CatalogueCategoryOut]
+    _spares_definition_out: Optional[SparesDefinitionOut]
     _catalogue_item_post: CatalogueItemPostSchema
     _expected_catalogue_item_in: CatalogueItemIn
     _expected_catalogue_item_out: CatalogueItemOut
@@ -98,6 +112,7 @@ def mock_create(
         catalogue_category_in_data: Optional[dict] = None,
         manufacturer_in_data: Optional[dict] = None,
         obsolete_replacement_catalogue_item_data: Optional[dict] = None,
+        spares_definition_out_data: Optional[dict] = None,
     ) -> None:
         """
         Mocks repo methods appropriately to test the `create` service method.
@@ -113,6 +128,8 @@ def mock_create(
                                      obsolete replacement as would be required for a `CatalogueItemPostSchema` but with
                                      any `unit_id`'s replaced by the `unit` value in its properties as the IDs will be
                                      added automatically.
+        :param spares_definition_out_data: Either `None` or a dictionary containing the spares definition data as would
+                                           be required for a `SparesDefinitionOut` database model.
         """
 
         # Generate mandatory IDs to be inserted where needed
@@ -181,6 +198,12 @@ def mock_create(
                 self._catalogue_category_out.properties, property_post_schemas
             )
 
+        # Mock the spares definition get
+        self._spares_definition_out = (
+            SparesDefinitionOut(**spares_definition_out_data) if spares_definition_out_data is not None else None
+        )
+        ServiceTestHelpers.mock_get(self.mock_setting_repository, self._spares_definition_out)
+
         self._catalogue_item_post = CatalogueItemPostSchema(
             **{**catalogue_item_data, **ids_to_insert, "properties": property_post_schemas}
         )
@@ -191,7 +214,7 @@ def mock_create(
                 **ids_to_insert,
                 "properties": expected_properties_in,
             },
-            number_of_spares=None,
+            number_of_spares=None if self._spares_definition_out is None else 0,
         )
         self._expected_catalogue_item_out = CatalogueItemOut(
             **self._expected_catalogue_item_in.model_dump(), id=ObjectId()
@@ -238,7 +261,15 @@ def check_create_success(self) -> None:
             self._catalogue_category_out.properties, self._catalogue_item_post.properties
         )
 
-        self.mock_catalogue_item_repository.create.assert_called_once_with(self._expected_catalogue_item_in)
+        self.mock_start_session_transaction.assert_called_with("creating catalogue item")
+        expected_session = self.mock_start_session_transaction.return_value.__enter__.return_value
+
+        # Ensure write locked the with spares definition
+        self.mock_setting_repository.write_lock.assert_called_once_with(SparesDefinitionOut, expected_session)
+
+        self.mock_catalogue_item_repository.create.assert_called_once_with(
+            self._expected_catalogue_item_in, session=expected_session
+        )
 
         assert self._created_catalogue_item == self._expected_catalogue_item_out
 
@@ -280,6 +311,18 @@ def test_create_with_all_properties(self):
         self.call_create()
         self.check_create_success()
 
+    def test_create_with_spares_definition_defined(self):
+        """Test creating a catalogue item when there is a spares definition defined."""
+
+        self.mock_create(
+            CATALOGUE_ITEM_DATA_REQUIRED_VALUES_ONLY,
+            catalogue_category_in_data=CATALOGUE_CATEGORY_IN_DATA_LEAF_NO_PARENT_NO_PROPERTIES,
+            manufacturer_in_data=MANUFACTURER_IN_DATA_A,
+            spares_definition_out_data=SETTING_SPARES_DEFINITION_OUT_DATA_NEW_USED,
+        )
+        self.call_create()
+        self.check_create_success()
+
     def test_create_with_non_existent_catalogue_category_id(self):
         """Test creating a catalogue item with a non-existent catalogue category ID."""