From 6e0091491a1677c75a256bca41cc133b2c4d561d Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Thu, 5 Dec 2024 12:28:42 +0000 Subject: [PATCH] Address review comments, replacing set with update #417 --- .../repositories/usage_status.py | 2 +- .../routers/v1/setting.py | 8 +-- .../schemas/catalogue_category.py | 4 +- .../schemas/setting.py | 14 +--- .../services/setting.py | 4 +- test/e2e/test_setting.py | 32 ++++----- test/unit/services/test_setting.py | 66 +++++++++---------- 7 files changed, 61 insertions(+), 69 deletions(-) diff --git a/inventory_management_system_api/repositories/usage_status.py b/inventory_management_system_api/repositories/usage_status.py index 63052b2a..e83b44d1 100644 --- a/inventory_management_system_api/repositories/usage_status.py +++ b/inventory_management_system_api/repositories/usage_status.py @@ -122,7 +122,7 @@ def _is_duplicate_usage_status( ) return usage_status is not None - def _is_usage_status_in_item(self, usage_status_id: str, session: ClientSession = None) -> bool: + def _is_usage_status_in_item(self, usage_status_id: CustomObjectId, session: ClientSession = None) -> bool: """Checks to see if any of the items in the database have a specific usage status ID. :param usage_status_id: The ID of the usage status that is looked for. diff --git a/inventory_management_system_api/routers/v1/setting.py b/inventory_management_system_api/routers/v1/setting.py index fd57e791..c3b44fda 100644 --- a/inventory_management_system_api/routers/v1/setting.py +++ b/inventory_management_system_api/routers/v1/setting.py @@ -20,18 +20,18 @@ @router.put( path="/spares_definition", - summary="Set the definition of a spare", + summary="Update the definition of a spare", response_description="Spares definition updated successfully", ) -def set_spares_definition( +def update_spares_definition( spares_definition: SparesDefinitionPutSchema, setting_service: SettingServiceDep ) -> SparesDefinitionSchema: # pylint: disable=missing-function-docstring - logger.info("Setting spares definition") + logger.info("Updating spares definition") logger.debug("Spares definition data: %s", spares_definition) try: - updated_spares_definition = setting_service.set_spares_definition(spares_definition) + updated_spares_definition = setting_service.update_spares_definition(spares_definition) return SparesDefinitionSchema(**updated_spares_definition.model_dump()) except (MissingRecordError, InvalidObjectIdError) as exc: message = "A specified usage status does not exist" diff --git a/inventory_management_system_api/schemas/catalogue_category.py b/inventory_management_system_api/schemas/catalogue_category.py index 2eba16de..b2288bfe 100644 --- a/inventory_management_system_api/schemas/catalogue_category.py +++ b/inventory_management_system_api/schemas/catalogue_category.py @@ -6,7 +6,7 @@ from numbers import Number from typing import Annotated, Any, List, Literal, Optional -from pydantic import BaseModel, Field, conlist, field_validator +from pydantic import BaseModel, Field, field_validator from pydantic_core.core_schema import ValidationInfo from inventory_management_system_api.schemas.mixins import CreatedModifiedSchemaMixin @@ -28,7 +28,7 @@ class AllowedValuesListSchema(BaseModel): """ type: Literal["list"] - values: conlist(Any, min_length=1) + values: list[Any] = Field(description="Value within the allowed values list", min_length=1) # Use discriminated union for any additional types of allowed values (so can use Pydantic's validation) diff --git a/inventory_management_system_api/schemas/setting.py b/inventory_management_system_api/schemas/setting.py index c8b5bac5..a54425a7 100644 --- a/inventory_management_system_api/schemas/setting.py +++ b/inventory_management_system_api/schemas/setting.py @@ -2,7 +2,7 @@ Module for defining the API schema models for representing settings. """ -from pydantic import BaseModel, Field, conlist, field_validator +from pydantic import BaseModel, Field, field_validator from inventory_management_system_api.schemas.usage_status import UsageStatusSchema @@ -15,21 +15,13 @@ class SparesDefinitionPutUsageStatusSchema(BaseModel): id: str = Field(description="The ID of the usage status.") -class SparesDefinitionUsageStatusSchema(SparesDefinitionPutUsageStatusSchema): - """ - Schema model for a usage status within a spares definition. - """ - - id: str = Field(description="The ID of the usage status.") - - class SparesDefinitionPutSchema(BaseModel): """ Schema model for a spares definition update request. """ - usage_statuses: conlist(SparesDefinitionPutUsageStatusSchema, min_length=1) = Field( - description="Usage statuses that classify items as a spare." + usage_statuses: list[SparesDefinitionPutUsageStatusSchema] = Field( + description="Usage statuses that classify items as a spare.", min_length=1 ) @field_validator("usage_statuses") diff --git a/inventory_management_system_api/services/setting.py b/inventory_management_system_api/services/setting.py index bba91a2f..84e40b3d 100644 --- a/inventory_management_system_api/services/setting.py +++ b/inventory_management_system_api/services/setting.py @@ -35,9 +35,9 @@ def __init__( self._setting_repository = setting_repository self._usage_status_repository = usage_status_repository - def set_spares_definition(self, spares_definition: SparesDefinitionPutSchema) -> SparesDefinitionOut: + def update_spares_definition(self, spares_definition: SparesDefinitionPutSchema) -> SparesDefinitionOut: """ - Sets the spares definition to a new value. + Updates the spares definition to a new value. :param spares_definition: The new spares definition. :return: The updated spares definition. diff --git a/test/e2e/test_setting.py b/test/e2e/test_setting.py index 66f4ea0e..7fc2b96a 100644 --- a/test/e2e/test_setting.py +++ b/test/e2e/test_setting.py @@ -17,8 +17,8 @@ from httpx import Response -class SetSparesDefinitionDSL(UsageStatusCreateDSL): - """Base class for set spares definition tests.""" +class UpdateSparesDefinitionDSL(UsageStatusCreateDSL): + """Base class for update spares definition tests.""" _put_response_spares_definition: Response @@ -90,18 +90,18 @@ def check_put_spares_definition_failed_with_validation_message(self, status_code assert self._put_response_spares_definition.json()["detail"][0]["msg"] == message -class TestSetSparesDefinition(SetSparesDefinitionDSL): - """Tests for setting the spares definition.""" +class TestUpdateSparesDefinition(UpdateSparesDefinitionDSL): + """Tests for updating the spares definition.""" - def test_set_spares_definition(self): - """Test setting the spares definition (for the first time).""" + def test_update_spares_definition(self): + """Test updating the spares definition (for the first time).""" self.put_spares_definition_and_post_prerequisites(SETTING_SPARES_DEFINITION_DATA_NEW_USED) self.check_put_spares_definition_success(SETTING_SPARES_DEFINITION_GET_DATA_NEW_USED) - def test_set_spares_definition_overwrite(self): - """Test setting the spares definition (when it has already been assigned before).""" + def test_update_spares_definition_overwrite(self): + """Test updating the spares definition (when it has already been assigned before).""" self.put_spares_definition_and_post_prerequisites(SETTING_SPARES_DEFINITION_DATA_NEW_USED) self.check_put_spares_definition_success(SETTING_SPARES_DEFINITION_GET_DATA_NEW_USED) @@ -109,8 +109,8 @@ def test_set_spares_definition_overwrite(self): self.put_spares_definition_and_post_prerequisites(SETTING_SPARES_DEFINITION_DATA_NEW) self.check_put_spares_definition_success(SETTING_SPARES_DEFINITION_GET_DATA_NEW) - def test_set_spares_definition_with_no_usage_statuses(self): - """Test setting the spares definition with no usage statuses given.""" + def test_update_spares_definition_with_no_usage_statuses(self): + """Test updating the spares definition with no usage statuses given.""" self.put_spares_definition({**SETTING_SPARES_DEFINITION_DATA_NEW_USED, "usage_statuses": []}) @@ -118,8 +118,8 @@ def test_set_spares_definition_with_no_usage_statuses(self): 422, "List should have at least 1 item after validation, not 0" ) - def test_set_spares_definition_with_duplicate_usage_statuses(self): - """Test setting the spares definition with no usage statuses given.""" + def test_update_spares_definition_with_duplicate_usage_statuses(self): + """Test updating the spares definition with no usage statuses given.""" self.put_spares_definition_and_post_prerequisites( { @@ -137,8 +137,8 @@ def test_set_spares_definition_with_duplicate_usage_statuses(self): f"{self.usage_status_value_id_dict[SETTING_SPARES_DEFINITION_DATA_NEW_USED['usage_statuses'][0]['value']]}", ) - def test_set_spares_definition_with_non_existent_usage_status_id(self): - """Test setting the spares definition when there is a non-existent usage status ID.""" + def test_update_spares_definition_with_non_existent_usage_status_id(self): + """Test updating the spares definition when there is a non-existent usage status ID.""" self.post_usage_status(USAGE_STATUS_POST_DATA_NEW) self.add_usage_status_value_and_id(USAGE_STATUS_POST_DATA_USED["value"], str(ObjectId())) @@ -146,8 +146,8 @@ def test_set_spares_definition_with_non_existent_usage_status_id(self): self.check_put_spares_definition_failed_with_detail(422, "A specified usage status does not exist") - def test_set_spares_definition_with_invalid_usage_status_id(self): - """Test setting the spares definition when there is an invalid usage status ID.""" + def test_update_spares_definition_with_invalid_usage_status_id(self): + """Test updating the spares definition when there is an invalid usage status ID.""" self.post_usage_status(USAGE_STATUS_POST_DATA_NEW) self.add_usage_status_value_and_id(USAGE_STATUS_POST_DATA_USED["value"], "invalid-id") diff --git a/test/unit/services/test_setting.py b/test/unit/services/test_setting.py index c10b2e7a..cc3436a5 100644 --- a/test/unit/services/test_setting.py +++ b/test/unit/services/test_setting.py @@ -37,20 +37,20 @@ def setup(self, setting_repository_mock, usage_status_repository_mock, setting_s self.setting_service = setting_service -class SetSparesDefinitionDSL(SettingServiceDSL): - """Base class for `set_spares_definition` tests.""" +class UpdateSparesDefinitionDSL(SettingServiceDSL): + """Base class for `update_spares_definition` tests.""" _spares_definition_put: SparesDefinitionPutSchema _expected_spares_definition_in: SparesDefinitionIn _expected_spares_definition_out: MagicMock - _set_spares_definition: MagicMock - _set_spares_definition_exception: pytest.ExceptionInfo + _updated_spares_definition: MagicMock + _update_spares_definition_exception: pytest.ExceptionInfo - def mock_set_spares_definition( + def mock_update_spares_definition( self, spares_definition_data: dict, usage_statuses_out_data: list[Optional[dict]] ) -> None: """ - Mocks repository methods appropriately to test the `set_spares_definition` service method. + Mocks repository methods appropriately to test the `update_spares_definition` service method. :param spares_definition_data: Dictionary containing the put data as would be required for a `SparesDefinitionPutSchema` but with any `id`'s replaced by the `value` as the @@ -83,26 +83,26 @@ def mock_set_spares_definition( self._expected_spares_definition_out = MagicMock() ServiceTestHelpers.mock_upsert(self.mock_setting_repository, self._expected_spares_definition_out) - def call_set_spares_definition(self) -> None: - """Calls the `SettingService` `set_spares_definition` method with the appropriate data from a prior call to - `mock_set_spares_definition`.""" + def call_update_spares_definition(self) -> None: + """Calls the `SettingService` `update_spares_definition` method with the appropriate data from a prior call to + `mock_update_spares_definition`.""" - self._set_spares_definition = self.setting_service.set_spares_definition(self._spares_definition_put) + self._updated_spares_definition = self.setting_service.update_spares_definition(self._spares_definition_put) - def call_set_spares_definition_expecting_error(self, error_type: type[BaseException]) -> None: + def call_update_spares_definition_expecting_error(self, error_type: type[BaseException]) -> None: """ - Calls the `SettingService` `set_spares_definition` method with the appropriate data from a prior call to - `mock_set_spares_definition` while expecting an error to be raised. + Calls the `SettingService` `update_spares_definition` method with the appropriate data from a prior call to + `mock_update_spares_definition` while expecting an error to be raised. :param error_type: Expected exception to be raised. """ with pytest.raises(error_type) as exc: - self.setting_service.set_spares_definition(self._spares_definition_put) - self._set_spares_definition_exception = exc + self.setting_service.update_spares_definition(self._spares_definition_put) + self._update_spares_definition_exception = exc - def check_set_spares_definition_success(self) -> None: - """Checks that a prior call to `call_set_spares_definition` worked as expected.""" + def check_update_spares_definition_success(self) -> None: + """Checks that a prior call to `call_update_spares_definition` worked as expected.""" # Ensure obtained all of the required usage statuses self.mock_usage_status_repository.get.assert_has_calls( @@ -116,11 +116,11 @@ def check_set_spares_definition_success(self) -> None: self._expected_spares_definition_in, SparesDefinitionOut ) - assert self._set_spares_definition == self._expected_spares_definition_out + assert self._updated_spares_definition == self._expected_spares_definition_out - def check_set_spares_definition_failed_with_exception(self, message: str) -> None: + def check_update_spares_definition_failed_with_exception(self, message: str) -> None: """ - Checks that a prior call to `call_set_spares_definition_expecting_error` worked as expected, raising an + Checks that a prior call to `call_update_spares_definition_expecting_error` worked as expected, raising an exception with the correct message. :param message: Expected message of the raised exception. @@ -128,27 +128,27 @@ def check_set_spares_definition_failed_with_exception(self, message: str) -> Non self.mock_setting_repository.upsert.assert_not_called() - assert str(self._set_spares_definition_exception.value) == message + assert str(self._update_spares_definition_exception.value) == message -class TestSetSpareDefinition(SetSparesDefinitionDSL): - """Tests for setting the spares definition.""" +class TestUpdateSpareDefinition(UpdateSparesDefinitionDSL): + """Tests for updating the spares definition.""" - def test_set_spare_definition(self): - """Test setting the spares definition.""" + def test_update_spare_definition(self): + """Test updating the spares definition.""" - self.mock_set_spares_definition( + self.mock_update_spares_definition( SETTING_SPARES_DEFINITION_DATA_NEW_USED, [USAGE_STATUS_OUT_DATA_NEW, USAGE_STATUS_OUT_DATA_USED] ) - self.call_set_spares_definition() - self.check_set_spares_definition_success() + self.call_update_spares_definition() + self.check_update_spares_definition_success() - def test_set_spare_definition_with_non_existent_usage_status_id(self): - """Test setting the spares definition with a non-existent usage status ID.""" + def test_update_spare_definition_with_non_existent_usage_status_id(self): + """Test updating the spares definition with a non-existent usage status ID.""" - self.mock_set_spares_definition(SETTING_SPARES_DEFINITION_DATA_NEW_USED, [USAGE_STATUS_OUT_DATA_NEW, None]) - self.call_set_spares_definition_expecting_error(MissingRecordError) - self.check_set_spares_definition_failed_with_exception( + self.mock_update_spares_definition(SETTING_SPARES_DEFINITION_DATA_NEW_USED, [USAGE_STATUS_OUT_DATA_NEW, None]) + self.call_update_spares_definition_expecting_error(MissingRecordError) + self.check_update_spares_definition_failed_with_exception( # Pydantic Field confuses pylint # pylint: disable=unsubscriptable-object f"No usage status found with ID: {self._spares_definition_put.usage_statuses[1].id}"