From 878054bf1d3f7bbae35cf44dca0281b3e29c13ad Mon Sep 17 00:00:00 2001 From: Joel Davies Date: Wed, 7 Aug 2024 13:08:29 +0000 Subject: [PATCH] Resolve outstanding TODOs and fix linting #342 --- test/e2e/conftest.py | 27 ++++++++++++++++++++-- test/e2e/test_catalogue_category.py | 23 +++++++++++++------ test/e2e/test_catalogue_item.py | 28 +++++++++++++---------- test/mock_data.py | 20 +++++++--------- test/unit/services/test_catalogue_item.py | 4 ++-- 5 files changed, 67 insertions(+), 35 deletions(-) diff --git a/test/e2e/conftest.py b/test/e2e/conftest.py index e016f03d..cdf8e81b 100644 --- a/test/e2e/conftest.py +++ b/test/e2e/conftest.py @@ -108,7 +108,7 @@ def check_created_and_modified_times_updated_correctly(post_response: Response, @staticmethod def replace_unit_values_with_ids_in_properties(data: dict, unit_value_id_dict: dict[str, str]) -> dict: - """Inserts unit IDs into some data that may have a 'properties' list within it. + """Inserts unit IDs into some data that may have a 'properties' list within it while removing the unit value. :param data: Dictionary of data that could have a 'properties' value within it. :param unit_value_id_dict: Dictionary of unit value and ID pairs for unit ID lookups. @@ -129,9 +129,32 @@ def replace_unit_values_with_ids_in_properties(data: dict, unit_value_id_dict: d return {**data, "properties": new_properties} return data + @staticmethod + def add_unit_ids_to_properties(data: dict, unit_value_id_dict: dict[str, str]) -> dict: + """Inserts unit IDs into some data that may have a 'properties' list within it. + + :param data: Dictionary of data that could have a 'properties' value within it. + :param unit_value_id_dict: Dictionary of unit value and ID pairs for unit ID lookups. + :return: The data with any needed unit IDs inserted. + """ + + if "properties" in data and data["properties"]: + new_properties = [] + for prop in data["properties"]: + new_property = {**prop} + if "unit" in prop: + if prop["unit"] is not None: + new_property["unit_id"] = unit_value_id_dict[prop["unit"]] + else: + new_property["unit_id"] = None + new_properties.append(new_property) + return {**data, "properties": new_properties} + return data + @staticmethod def replace_property_names_with_ids_in_properties(data: dict, property_name_id_dict: dict[str, str]) -> dict: - """Inserts property IDs into some data that may have a 'properties' list within it. + """Inserts property IDs into some data that may have a 'properties' list within it while removing the property + name. :param data: Dictionary of data that could have a 'properties' value within it. :param property_name_id_dict: Dictionary of property name and ID pairs for property ID lookups. diff --git a/test/e2e/test_catalogue_category.py b/test/e2e/test_catalogue_category.py index f4db38e9..b2caba25 100644 --- a/test/e2e/test_catalogue_category.py +++ b/test/e2e/test_catalogue_category.py @@ -130,12 +130,15 @@ def check_post_catalogue_category_success(self, expected_catalogue_category_get_ Checks that a prior call to `post_catalogue_category` gave a successful response with the expected data returned. - :param expected_catalogue_category_get_data: Dictionary containing the expected catalogue category data returned - as would be required for a `CatalogueCategorySchema`. + :param expected_catalogue_category_get_data: Dictionary containing the expected system data returned as would + be required for a `CatalogueCategorySchema`. Does not need unit IDs + as they will be added automatically to check they are as expected. """ assert self._post_response_catalogue_category.status_code == 201 - assert self._post_response_catalogue_category.json() == expected_catalogue_category_get_data + assert self._post_response_catalogue_category.json() == E2ETestHelpers.add_unit_ids_to_properties( + expected_catalogue_category_get_data, self.unit_value_id_dict + ) def check_post_catalogue_category_failed_with_detail(self, status_code: int, detail: str) -> None: """ @@ -403,11 +406,14 @@ def check_get_catalogue_category_success(self, expected_catalogue_category_get_d Checks that a prior call to `get_catalogue_category` gave a successful response with the expected data returned. :param expected_catalogue_category_get_data: Dictionary containing the expected system data returned as would - be required for a `CatalogueCategorySchema`. + be required for a `CatalogueCategorySchema`. Does not need unit IDs + as they will be added automatically to check they are as expected. """ assert self._get_response_catalogue_category.status_code == 200 - assert self._get_response_catalogue_category.json() == expected_catalogue_category_get_data + assert self._get_response_catalogue_category.json() == E2ETestHelpers.add_unit_ids_to_properties( + expected_catalogue_category_get_data, self.unit_value_id_dict + ) def check_get_catalogue_category_failed_with_detail(self, status_code: int, detail: str) -> None: """ @@ -771,11 +777,14 @@ def check_patch_catalogue_category_response_success(self, expected_catalogue_cat returned. :param expected_catalogue_category_get_data: Dictionary containing the expected system data returned as would - be required for a `CatalogueCategorySchema`. + be required for a `CatalogueCategorySchema`. Does not need unit IDs + as they will be added automatically to check they are as expected. """ assert self._patch_response_catalogue_category.status_code == 200 - assert self._patch_response_catalogue_category.json() == expected_catalogue_category_get_data + assert self._patch_response_catalogue_category.json() == E2ETestHelpers.add_unit_ids_to_properties( + expected_catalogue_category_get_data, self.unit_value_id_dict + ) E2ETestHelpers.check_created_and_modified_times_updated_correctly( self._post_response_catalogue_category, self._patch_response_catalogue_category diff --git a/test/e2e/test_catalogue_item.py b/test/e2e/test_catalogue_item.py index 0cc5fe74..20a1da86 100644 --- a/test/e2e/test_catalogue_item.py +++ b/test/e2e/test_catalogue_item.py @@ -34,14 +34,14 @@ MANUFACTURER_POST_DATA_ALL_VALUES, MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY, PROPERTY_DATA_BOOLEAN_MANDATORY_TRUE, - PROPERTY_DATA_NUMBER_NON_MANDATORY_42, - PROPERTY_DATA_NUMBER_NON_MANDATORY_NONE, PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_1, + PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42, + PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_NONE, PROPERTY_DATA_STRING_MANDATORY_TEXT, PROPERTY_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_NONE, PROPERTY_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_VALUE1, - PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_42, PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_1, + PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42, PROPERTY_GET_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_VALUE1, UNIT_POST_DATA_MM, ) @@ -308,7 +308,7 @@ def check_post_catalogue_item_failed_with_validation_message(self, status_code: class TestCreate(CreateDSL): - """Tests for creating a catalogue category.""" + """Tests for creating a catalogue item.""" def test_create_with_only_required_values_provided(self): """Test creating a catalogue item with only required values provided.""" @@ -426,7 +426,7 @@ def test_create_with_non_mandatory_properties_given_none(self): **CATALOGUE_ITEM_DATA_WITH_MANDATORY_PROPERTIES_ONLY, "properties": [ PROPERTY_DATA_BOOLEAN_MANDATORY_TRUE, - PROPERTY_DATA_NUMBER_NON_MANDATORY_NONE, + PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_NONE, PROPERTY_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_NONE, ], } @@ -1000,12 +1000,15 @@ def test_partial_update_catalogue_category_id_and_properties_with_different_defi self.patch_catalogue_item( catalogue_item_id, - {"catalogue_category_id": new_catalogue_category_id, "properties": [PROPERTY_DATA_NUMBER_NON_MANDATORY_42]}, + { + "catalogue_category_id": new_catalogue_category_id, + "properties": [PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42], + }, ) self.check_patch_catalogue_item_response_success( { **CATALOGUE_ITEM_GET_DATA_WITH_ALL_PROPERTIES, - "properties": [PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_42], + "properties": [PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42], } ) @@ -1219,7 +1222,7 @@ def test_partial_update_properties_with_non_mandatory_properties_given_none(self { "properties": [ PROPERTY_DATA_BOOLEAN_MANDATORY_TRUE, - PROPERTY_DATA_NUMBER_NON_MANDATORY_NONE, + PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_NONE, PROPERTY_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_NONE, ] }, @@ -1305,16 +1308,17 @@ def test_partial_update_properties_with_number_property_with_invalid_value_type( catalogue_item_id = self.post_catalogue_item_and_prerequisites_with_given_properties( catalogue_category_properties_data=[CATALOGUE_CATEGORY_PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT], - catalogue_item_properties_data=[PROPERTY_DATA_NUMBER_NON_MANDATORY_42], + catalogue_item_properties_data=[PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42], ) self.patch_catalogue_item( - catalogue_item_id, {"properties": [{**PROPERTY_DATA_NUMBER_NON_MANDATORY_42, "value": "42"}]} + catalogue_item_id, {"properties": [{**PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42, "value": "42"}]} ) self.check_patch_catalogue_item_failed_with_detail( 422, "Invalid value type for property with ID " - f"'{self.property_name_id_dict[PROPERTY_DATA_NUMBER_NON_MANDATORY_42['name']]}'. Expected type: number.", + f"'{self.property_name_id_dict[PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42['name']]}'. " + "Expected type: number.", ) def test_partial_update_properties_with_boolean_property_with_invalid_value_type(self): @@ -1511,7 +1515,7 @@ def test_delete(self): self.check_get_catalogue_item_failed_with_detail(404, "Catalogue item not found") def test_delete_with_child_item(self): - """Test deleting a catalogue category with a child catalogue item.""" + """Test deleting a catalogue item with a child item.""" self.post_catalogue_category(CATALOGUE_CATEGORY_POST_DATA_LEAF_NO_PARENT_NO_PROPERTIES) self.post_manufacturer(MANUFACTURER_POST_DATA_REQUIRED_VALUES_ONLY) diff --git a/test/mock_data.py b/test/mock_data.py index bf531b93..1973b91b 100644 --- a/test/mock_data.py +++ b/test/mock_data.py @@ -40,11 +40,9 @@ CATALOGUE_CATEGORY_PROPERTY_IN_DATA_BOOLEAN_MANDATORY = {**CATALOGUE_CATEGORY_PROPERTY_DATA_BOOLEAN_MANDATORY} -# TODO: Unit ids should be known so ideally should be inserted I think - check for other ANY's like this CATALOGUE_CATEGORY_PROPERTY_GET_DATA_BOOLEAN_MANDATORY = { **CATALOGUE_CATEGORY_PROPERTY_DATA_BOOLEAN_MANDATORY, "id": ANY, - "unit_id": ANY, "unit": None, "allowed_values": None, } @@ -66,11 +64,11 @@ CATALOGUE_CATEGORY_PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT = { **CATALOGUE_CATEGORY_PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT, "id": ANY, - "unit_id": ANY, "allowed_values": None, } # Number, Non Mandatory, Allowed values list + CATALOGUE_CATEGORY_PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST = { "name": "Non Mandatory Number Property With Allowed Values", "type": "number", @@ -103,7 +101,6 @@ CATALOGUE_CATEGORY_PROPERTY_GET_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST = { **CATALOGUE_CATEGORY_PROPERTY_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST, "id": ANY, - "unit_id": ANY, "unit": None, } @@ -267,22 +264,21 @@ # Number, Non Mandatory, 42 -# TODO: Rename? Specific to the CATALOGUE_CATEGORY_PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT -PROPERTY_DATA_NUMBER_NON_MANDATORY_42 = { +PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42 = { "name": CATALOGUE_CATEGORY_PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT["name"], "value": 42, } -PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_42 = {**PROPERTY_DATA_NUMBER_NON_MANDATORY_42} +PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42 = {**PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42} # Number, Non Mandatory, None -PROPERTY_DATA_NUMBER_NON_MANDATORY_NONE = { +PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_NONE = { "name": CATALOGUE_CATEGORY_PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT["name"], "value": None, } -PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_NONE = {**PROPERTY_DATA_NUMBER_NON_MANDATORY_NONE} +PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_NONE = {**PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_NONE} # String, Mandatory, text PROPERTY_DATA_STRING_MANDATORY_TEXT = { @@ -402,7 +398,7 @@ "name": "Catalogue Item With All Properties", "properties": [ PROPERTY_DATA_BOOLEAN_MANDATORY_TRUE, - PROPERTY_DATA_NUMBER_NON_MANDATORY_42, + PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42, PROPERTY_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_VALUE1, ], } @@ -412,7 +408,7 @@ **CATALOGUE_ITEM_DATA_WITH_ALL_PROPERTIES, "properties": [ PROPERTY_GET_DATA_BOOLEAN_MANDATORY_TRUE, - PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_42, + PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42, PROPERTY_GET_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_VALUE1, ], } @@ -432,7 +428,7 @@ **CATALOGUE_ITEM_DATA_WITH_MANDATORY_PROPERTIES_ONLY, "properties": [ PROPERTY_GET_DATA_BOOLEAN_MANDATORY_TRUE, - PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_NONE, + PROPERTY_GET_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_NONE, PROPERTY_GET_DATA_STRING_NON_MANDATORY_WITH_ALLOWED_VALUES_LIST_NONE, ], } diff --git a/test/unit/services/test_catalogue_item.py b/test/unit/services/test_catalogue_item.py index 929ed025..0d1b4504 100644 --- a/test/unit/services/test_catalogue_item.py +++ b/test/unit/services/test_catalogue_item.py @@ -17,7 +17,7 @@ CATALOGUE_ITEM_DATA_REQUIRED_VALUES_ONLY, CATALOGUE_ITEM_DATA_WITH_ALL_PROPERTIES, MANUFACTURER_IN_DATA_A, - PROPERTY_DATA_NUMBER_NON_MANDATORY_42, + PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42, ) from test.unit.services.conftest import ServiceTestHelpers from typing import Optional @@ -969,7 +969,7 @@ def test_update_catalogue_category_id_and_properties_with_different_defined_prop catalogue_item_id, catalogue_item_update_data={ "catalogue_category_id": str(ObjectId()), - "properties": [PROPERTY_DATA_NUMBER_NON_MANDATORY_42], + "properties": [PROPERTY_DATA_NUMBER_NON_MANDATORY_WITH_MM_UNIT_42], }, stored_catalogue_item_data=CATALOGUE_ITEM_DATA_WITH_ALL_PROPERTIES, stored_catalogue_category_in_data=CATALOGUE_CATEGORY_IN_DATA_LEAF_NO_PARENT_WITH_PROPERTIES_MM,