From abcfbce695ba361b7c0cc310a686eb5d5e5c3325 Mon Sep 17 00:00:00 2001 From: Graham Lee Date: Wed, 27 Jul 2022 16:13:25 +0100 Subject: [PATCH] Fields with the enumerated value other permit free text entry #2714 The free text is stored in an associated string field --- .../data_service/model/age_range.py | 22 +++++++---- .../data_service/model/case.py | 4 +- .../data_service/model/field.py | 19 +++++++--- .../tests/test_age_range.py | 2 +- .../tests/test_case_end_to_end.py | 6 +-- .../tests/test_case_model.py | 18 ++++----- .../tests/test_case_schema_integration.py | 38 +++++++++++++++++++ .../tests/test_geojson_model.py | 1 + .../reusable-data-service/tests/util.py | 1 + 9 files changed, 84 insertions(+), 27 deletions(-) diff --git a/data-serving/reusable-data-service/data_service/model/age_range.py b/data-serving/reusable-data-service/data_service/model/age_range.py index 7f2f1903d..f5c77b7bd 100644 --- a/data-serving/reusable-data-service/data_service/model/age_range.py +++ b/data-serving/reusable-data-service/data_service/model/age_range.py @@ -3,11 +3,13 @@ from data_service.model.document import Document from data_service.util.errors import ValidationError + @dataclasses.dataclass class AgeRange(Document): """I represent a numerical range within which a person's age lies (inclusive of both limits). To avoid reidentifying people who have been anonymised by this application, I will only tell you their age to within five years (unless they are infants).""" + lower: int = None upper: int = None @@ -21,7 +23,7 @@ def fix_my_boundaries(self): self.lower = (self.lower // 5) * 5 + 1 if self.upper is not None and self.upper != 1 and self.upper % 5 != 0: self.upper = ((self.upper // 5) + 1) * 5 - + def validate(self): """I must represent the range [0,1], or a range greater than five years, and must have a positive lower bound and an upper bound below 121.""" @@ -31,11 +33,17 @@ def validate(self): if self.upper is None: raise ValidationError("Age Range must have an upper bound") if self.lower < 0: - raise ValidationError(f"Lower bound {self.lower} is below the minimum permissible 0") + raise ValidationError( + f"Lower bound {self.lower} is below the minimum permissible 0" + ) if self.upper < 1: - raise ValidationError(f"Upper bound {self.upper} is below the minimum permissible 1") + raise ValidationError( + f"Upper bound {self.upper} is below the minimum permissible 1" + ) if self.upper > 120: - raise ValidationError(f"Upper bound {self.upper} is above the maximum permissible 120") + raise ValidationError( + f"Upper bound {self.upper} is above the maximum permissible 120" + ) # deal with the special case first if self.lower == 0 and self.upper == 1: return @@ -47,10 +55,10 @@ def validate(self): def from_dict(cls, dict_description): ages = cls() # age ranges can be open-ended according to the data dictionary, which we map onto our absolute limits - ages.lower = dict_description.get('lower', 0) - ages.upper = dict_description.get('upper', 120) + ages.lower = dict_description.get("lower", 0) + ages.upper = dict_description.get("upper", 120) return ages @classmethod def none_field_values(cls): - return ['', ''] \ No newline at end of file + return ["", ""] diff --git a/data-serving/reusable-data-service/data_service/model/case.py b/data-serving/reusable-data-service/data_service/model/case.py index efa79a61a..045548ac6 100644 --- a/data-serving/reusable-data-service/data_service/model/case.py +++ b/data-serving/reusable-data-service/data_service/model/case.py @@ -25,7 +25,9 @@ def make_custom_case_class(name: str, field_models=[]) -> type: field_models is a list of model objects describing the fields for the data dictionary and for validation.""" global Case - fields = [f.dataclasses_tuple() for f in field_models] + fields = [] + for f in field_models: + fields += f.dataclasses_tuples() try: new_case_class = dataclasses.make_dataclass(name, fields, bases=(Document,)) except TypeError as e: diff --git a/data-serving/reusable-data-service/data_service/model/field.py b/data-serving/reusable-data-service/data_service/model/field.py index 85c72e57c..a79ea5d12 100644 --- a/data-serving/reusable-data-service/data_service/model/field.py +++ b/data-serving/reusable-data-service/data_service/model/field.py @@ -60,14 +60,21 @@ def from_dict(cls, dictionary): def python_type(self) -> type: return self.model_type(self.type) - def dataclasses_tuple(self) -> (str, type, dataclasses.Field): + def dataclasses_tuples(self): # Note that the default value here is always None, even if I have a default value! # That's because the meaning of "required" in a field model is "a user _is required_ to # supply a value" and the meaning of "default" is "for cases that don't already have this # key, use the default value"; if I give every Case the default value then there's no sense # in which a user is required to define it themselves. - return ( - self.key, - self.python_type(), - dataclasses.field(init=False, default=None), - ) + fields = [ + ( + self.key, + self.python_type(), + dataclasses.field(init=False, default=None), + ) + ] + if self.values is not None and "other" in self.values: + fields.append( + (f"{self.key}_other", str, dataclasses.field(init=False, default=None)) + ) + return fields diff --git a/data-serving/reusable-data-service/tests/test_age_range.py b/data-serving/reusable-data-service/tests/test_age_range.py index d5852d7ea..71e021843 100644 --- a/data-serving/reusable-data-service/tests/test_age_range.py +++ b/data-serving/reusable-data-service/tests/test_age_range.py @@ -7,7 +7,7 @@ def test_age_range_massages_input_to_match_buckets(): - ages = AgeRange(2,6) + ages = AgeRange(2, 6) assert ages.lower == 1 assert ages.upper == 10 diff --git a/data-serving/reusable-data-service/tests/test_case_end_to_end.py b/data-serving/reusable-data-service/tests/test_case_end_to_end.py index 036cce108..7700d3210 100644 --- a/data-serving/reusable-data-service/tests/test_case_end_to_end.py +++ b/data-serving/reusable-data-service/tests/test_case_end_to_end.py @@ -174,9 +174,9 @@ def test_post_case_list_cases_geojson_round_trip(client_with_patched_mongo): def test_post_case_list_cases_with_age_round_trip(client_with_patched_mongo): with open("./tests/data/case.minimal.json") as case_file: case_doc = json.load(case_file) - case_doc['age'] = { - 'lower': 4, - 'upper': 12, + case_doc["age"] = { + "lower": 4, + "upper": 12, } post_response = client_with_patched_mongo.post( "/api/cases", diff --git a/data-serving/reusable-data-service/tests/test_case_model.py b/data-serving/reusable-data-service/tests/test_case_model.py index 3d493d798..b886740d0 100644 --- a/data-serving/reusable-data-service/tests/test_case_model.py +++ b/data-serving/reusable-data-service/tests/test_case_model.py @@ -29,9 +29,9 @@ def test_case_with_geojson_is_valid(): def test_csv_header(): header_line = Case.csv_header() - header_fields = header_line.split(',') - assert 'caseStatus' in header_fields - assert 'location.latitude' in header_fields + header_fields = header_line.split(",") + assert "caseStatus" in header_fields + assert "location.latitude" in header_fields def test_csv_row_with_no_id(): @@ -45,9 +45,9 @@ def test_csv_row_with_no_id(): case.caseStatus = "probable" case.pathogenStatus = "emerging" csv = case.to_csv() - csv_fields = csv.split(',') - assert 'probable' in csv_fields - assert '2022-06-13' in csv_fields + csv_fields = csv.split(",") + assert "probable" in csv_fields + assert "2022-06-13" in csv_fields def test_csv_row_with_id(): @@ -64,9 +64,9 @@ def test_csv_row_with_id(): case.pathogenStatus = "unknown" csv = case.to_csv() csv = case.to_csv() - csv_fields = csv.split(',') - assert 'probable' in csv_fields - assert '2022-06-13' in csv_fields + csv_fields = csv.split(",") + assert "probable" in csv_fields + assert "2022-06-13" in csv_fields def test_apply_update_to_case(): diff --git a/data-serving/reusable-data-service/tests/test_case_schema_integration.py b/data-serving/reusable-data-service/tests/test_case_schema_integration.py index 5641f3e97..1706caf06 100644 --- a/data-serving/reusable-data-service/tests/test_case_schema_integration.py +++ b/data-serving/reusable-data-service/tests/test_case_schema_integration.py @@ -157,3 +157,41 @@ def test_field_enumerating_allowed_values_forbids_other_value( }, ) assert response.status_code == 422 + + +def test_adding_enumerated_field_with_other_value(client_with_patched_mongo): + response = client_with_patched_mongo.post( + "/api/schema", + json={ + "name": "customPathogenStatus", + "type": "string", + "description": "Whether the infection is associated with an endemic or emerging incidence", + "values": ["Endemic", "Emerging", "Unknown", "other"], + "required": False, + }, + ) + assert response.status_code == 201 + response = client_with_patched_mongo.post( + "/api/cases", + json={ + "confirmationDate": "2022-06-01T00:00:00.000Z", + "caseReference": { + "status": "UNVERIFIED", + "sourceId": "24680135792468013579fedc", + }, + "caseStatus": "probable", + "customPathogenStatus": "other", + "customPathogenStatus_other": "Neopanspermia", + }, + ) + assert response.status_code == 201 + response = client_with_patched_mongo.post( + "/api/cases/download", json={"format": "csv"} + ) + assert response.status_code == 200 + csv_file = io.StringIO(response.get_data().decode("utf-8")) + csv_reader = csv.DictReader(csv_file) + cases = [c for c in csv_reader] + assert len(cases) == 1 + case = cases[0] + assert case["customPathogenStatus_other"] == "Neopanspermia" diff --git a/data-serving/reusable-data-service/tests/test_geojson_model.py b/data-serving/reusable-data-service/tests/test_geojson_model.py index 6ae19f204..cfe98d7d5 100644 --- a/data-serving/reusable-data-service/tests/test_geojson_model.py +++ b/data-serving/reusable-data-service/tests/test_geojson_model.py @@ -5,6 +5,7 @@ from tests.util import does_not_raise + def test_point_needs_two_coordinates(): p = Point() p.coordinates = [] diff --git a/data-serving/reusable-data-service/tests/util.py b/data-serving/reusable-data-service/tests/util.py index f0825eb68..c1413bf5e 100644 --- a/data-serving/reusable-data-service/tests/util.py +++ b/data-serving/reusable-data-service/tests/util.py @@ -2,6 +2,7 @@ from contextlib import contextmanager + @contextmanager def does_not_raise(exception): try: