From 48b9c91f43fd66b812a094e06bb65659758ac905 Mon Sep 17 00:00:00 2001 From: Philip James Elson Date: Wed, 21 Apr 2021 14:07:50 +0000 Subject: [PATCH] Clean up the FormData defaults and casting rules. Using this information we now shorten the FormData dictionary. The baseline form URL is now 490 characters compared to 1089 previously. Closes #143. --- cara/apps/calculator/__init__.py | 2 +- cara/apps/calculator/model_generator.py | 176 ++++++++++++++---- cara/apps/calculator/report_generator.py | 18 +- cara/apps/calculator/static/js/form.js | 2 + .../templates/calculator.form.html.j2 | 8 +- cara/dataclass_utils.py | 19 ++ .../apps/calculator/test_model_generator.py | 65 +++++++ cara/tests/apps/calculator/test_webapp.py | 2 +- 8 files changed, 238 insertions(+), 54 deletions(-) diff --git a/cara/apps/calculator/__init__.py b/cara/apps/calculator/__init__.py index 9690fed7..748545ae 100644 --- a/cara/apps/calculator/__init__.py +++ b/cara/apps/calculator/__init__.py @@ -23,7 +23,7 @@ from .user import AuthenticatedUser, AnonymousUser # calculator version. If the calculator needs to make breaking changes (e.g. change # form attributes) then it can also increase its MAJOR version without needing to # increase the overall CARA version (found at ``cara.__version__``). -__version__ = "1.5.0" +__version__ = "1.5.1" class BaseRequestHandler(RequestHandler): diff --git a/cara/apps/calculator/model_generator.py b/cara/apps/calculator/model_generator.py index b31528f4..6c545152 100644 --- a/cara/apps/calculator/model_generator.py +++ b/cara/apps/calculator/model_generator.py @@ -1,3 +1,4 @@ +import dataclasses from dataclasses import dataclass import html import logging @@ -14,6 +15,13 @@ LOG = logging.getLogger(__name__) minutes_since_midnight = typing.NewType('minutes_since_midnight', int) + + +# Used to declare when an attribute of a class must have a value provided, and +# there should be no default value used. +_NO_DEFAULT = object() + + @dataclass class FormData: activity_type: str @@ -60,51 +68,100 @@ class FormData: windows_number: int window_opening_regime: str + #: The default values for undefined fields. Note that the defaults here + #: and the defaults in the html form must not be contradictory. + _DEFAULTS: typing.ClassVar[typing.Dict[str, typing.Any]] = { + 'activity_type': 'office', + 'air_changes': 0., + 'air_supply': 0., + 'calculator_version': _NO_DEFAULT, + 'ceiling_height': 0., + 'exposed_coffee_break_option': 'coffee_break_0', + 'exposed_coffee_duration': 5, + 'exposed_finish': '17:30', + 'exposed_lunch_finish': '13:30', + 'exposed_lunch_option': True, + 'exposed_lunch_start': '12:30', + 'exposed_start': '08:30', + 'event_month': 'January', + 'floor_area': 0., + 'hepa_amount': 0., + 'hepa_option': False, + 'infected_coffee_break_option': 'coffee_break_0', + 'infected_coffee_duration': 5, + 'infected_dont_have_breaks_with_exposed': False, + 'infected_finish': '17:30', + 'infected_lunch_finish': '13:30', + 'infected_lunch_option': True, + 'infected_lunch_start': '12:30', + 'infected_people': _NO_DEFAULT, + 'infected_start': '08:30', + 'mask_type': 'Type I', + 'mask_wearing_option': 'mask_off', + 'mechanical_ventilation_type': 'not-applicable', + 'opening_distance': 0., + 'room_number': _NO_DEFAULT, + 'room_volume': 0., + 'simulation_name': _NO_DEFAULT, + 'total_people': _NO_DEFAULT, + 'ventilation_type': 'no_ventilation', + 'virus_type': 'SARS_CoV_2', + 'volume_type': _NO_DEFAULT, + 'window_type': 'window_sliding', + 'window_height': 0., + 'window_width': 0., + 'windows_duration': 0., + 'windows_frequency': 0., + 'windows_number': 0, + 'window_opening_regime': 'windows_open_permanently', + } + @classmethod def from_dict(cls, form_data: typing.Dict) -> "FormData": # Take a copy of the form data so that we can mutate it. form_data = form_data.copy() - - valid_na_values = ['window_opening_regime', 'window_type', 'mechanical_ventilation_type', 'infected_dont_have_breaks_with_exposed'] - for name in valid_na_values: - if not form_data.get(name, ''): - form_data[name] = 'not-applicable' - - for name in ['exposed_lunch_start', 'exposed_lunch_finish', 'infected_lunch_start', 'infected_lunch_finish']: - if not form_data.get(name, ''): - form_data[name] = '00:00' + form_data.pop('_xsrf', None) # Don't let arbitrary unescaped HTML through the net. for key, value in form_data.items(): if isinstance(value, str): form_data[key] = html.escape(value) - # TODO: This fixup is a problem with the form.html. + for key, default_value in cls._DEFAULTS.items(): + if form_data.get(key, '') == '': + if default_value is _NO_DEFAULT: + raise ValueError(f"{key} must be specified") + form_data[key] = default_value + for key, value in form_data.items(): - if value == "": - form_data[key] = "0" + if key in _CAST_RULES_FORM_ARG_TO_NATIVE: + form_data[key] = _CAST_RULES_FORM_ARG_TO_NATIVE[key](value) - for attr_name in BOOLEAN_ATTRIBUTES: - form_data[attr_name] = form_data[attr_name] == '1' - for attr_name in FLOAT_ATTRIBUTES: - form_data[attr_name] = float(form_data[attr_name]) - for attr_name in INT_ATTRIBUTES: - form_data[attr_name] = int(form_data[attr_name]) - for attr_name in TIME_ATTRIBUTES: - form_data[attr_name] = time_string_to_minutes(form_data[attr_name]) + if key not in cls._DEFAULTS: + raise ValueError(f'Invalid argument "{html.escape(key)}" given') - form_data.pop('_xsrf', None) instance = cls(**form_data) instance.validate() return instance @classmethod - def to_dict(self, form: "FormData") -> dict: - form_dict = form.__dict__.copy() - for attr_name in TIME_ATTRIBUTES: - form_dict[attr_name] = time_minutes_to_string(form_dict[attr_name]) - for attr_name in BOOLEAN_ATTRIBUTES: - form_dict[attr_name] = form_dict[attr_name] & 1 + def to_dict(cls, form: "FormData", strip_defaults: bool = False) -> dict: + form_dict = { + field.name: getattr(form, field.name) + for field in dataclasses.fields(form) + } + + for attr, value in form_dict.items(): + if attr in _CAST_RULES_NATIVE_TO_FORM_ARG: + form_dict[attr] = _CAST_RULES_NATIVE_TO_FORM_ARG[attr](value) + + if strip_defaults: + del form_dict['calculator_version'] + + for attr, value in list(form_dict.items()): + default = cls._DEFAULTS.get(attr, _NO_DEFAULT) + if default is not _NO_DEFAULT and value in [default, 'not-applicable']: + form_dict.pop(attr) return form_dict def validate(self): @@ -140,12 +197,22 @@ class FormData: if getattr(self, attr_name) not in valid_set: raise ValueError(f"{getattr(self, attr_name)} is not a valid value for {attr_name}") - if ( - self.ventilation_type == 'natural' - and self.window_type == 'not-applicable' - ): - raise ValueError("window_type cannot be 'not-applicable' if " - "ventilation_type is 'natural'") + if self.ventilation_type == 'natural_ventilation': + if self.window_type == 'not-applicable': + raise ValueError( + "window_type cannot be 'not-applicable' if " + "ventilation_type is 'natural_ventilation'" + ) + if self.window_opening_regime == 'not-applicable': + raise ValueError( + "window_opening_regime cannot be 'not-applicable' if " + "ventilation_type is 'natural_ventilation'" + ) + + if (self.ventilation_type == 'mechanical_ventilation' + and self.mechanical_ventilation_type == 'not-applicable'): + raise ValueError("mechanical_ventilation_type cannot be 'not-applicable' if " + "ventilation_type is 'mechanical_ventilation'") def build_model(self) -> models.ExposureModel: return model_from_form(self) @@ -582,14 +649,8 @@ VOLUME_TYPES = {'room_volume_explicit', 'room_volume_from_dimensions'} WINDOWS_OPENING_REGIMES = {'windows_open_permanently', 'windows_open_periodically', 'not-applicable'} WINDOWS_TYPES = {'window_sliding', 'window_hinged', 'not-applicable'} -COFFEE_OPTIONS_INT = {'coffee_break_0':0, 'coffee_break_1':1, 'coffee_break_2':2, 'coffee_break_4':4} +COFFEE_OPTIONS_INT = {'coffee_break_0': 0, 'coffee_break_1': 1, 'coffee_break_2': 2, 'coffee_break_4': 4} -BOOLEAN_ATTRIBUTES = {'hepa_option', 'exposed_lunch_option', 'infected_lunch_option', 'infected_dont_have_breaks_with_exposed'} -FLOAT_ATTRIBUTES = {'air_changes', 'air_supply', 'ceiling_height', 'floor_area', 'hepa_amount', 'opening_distance', - 'room_volume', 'windows_duration', 'windows_frequency', 'window_height', 'window_width'} -INT_ATTRIBUTES = {'exposed_coffee_duration', 'infected_coffee_duration', 'infected_people', 'total_people', 'windows_number'} -TIME_ATTRIBUTES = {'exposed_lunch_start', 'exposed_lunch_finish', 'exposed_start', 'exposed_finish', - 'infected_lunch_start', 'infected_lunch_finish', 'infected_start', 'infected_finish'} def time_string_to_minutes(time: str) -> minutes_since_midnight: """ @@ -599,6 +660,7 @@ def time_string_to_minutes(time: str) -> minutes_since_midnight: """ return minutes_since_midnight(60 * int(time[:2]) + int(time[3:])) + def time_minutes_to_string(time: int) -> str: """ Converts time from an integer number of minutes after 00:00 to string-format @@ -606,3 +668,37 @@ def time_minutes_to_string(time: int) -> str: :return: A string of the form "HH:MM" representing a time of day """ return "{0:0=2d}".format(int(time/60)) + ":" + "{0:0=2d}".format(time%60) + + +def _safe_int_cast(value) -> int: + if isinstance(value, int): + return value + elif isinstance(value, float) and int(value) == value: + return int(value) + elif isinstance(value, str) and value.isdecimal(): + return int(value) + else: + raise TypeError(f"Unable to safely cast {value} ({type(value)} type) to int") + + +#: Mapping of field name to a callable which can convert values from form +#: input (URL encoded arguments / string) into the correct type. +_CAST_RULES_FORM_ARG_TO_NATIVE: typing.Dict[str, typing.Callable] = {} + +#: Mapping of field name to callable which can convert native type to values +#: that can be encoded to URL arguments. +_CAST_RULES_NATIVE_TO_FORM_ARG: typing.Dict[str, typing.Callable] = {} + + +for _field in dataclasses.fields(FormData): + if _field.type is minutes_since_midnight: + _CAST_RULES_FORM_ARG_TO_NATIVE[_field.name] = time_string_to_minutes + _CAST_RULES_NATIVE_TO_FORM_ARG[_field.name] = time_minutes_to_string + elif _field.type is int: + _CAST_RULES_FORM_ARG_TO_NATIVE[_field.name] = _safe_int_cast + elif _field.type is float: + _CAST_RULES_FORM_ARG_TO_NATIVE[_field.name] = float + elif _field.type is bool: + _CAST_RULES_FORM_ARG_TO_NATIVE[_field.name] = lambda v: v == '1' + _CAST_RULES_NATIVE_TO_FORM_ARG[_field.name] = int + diff --git a/cara/apps/calculator/report_generator.py b/cara/apps/calculator/report_generator.py index 8effd90c..c57693f0 100644 --- a/cara/apps/calculator/report_generator.py +++ b/cara/apps/calculator/report_generator.py @@ -16,6 +16,7 @@ import numpy as np from cara import models from .model_generator import FormData +from ... import dataclass_utils @dataclasses.dataclass(frozen=True) @@ -47,7 +48,8 @@ def calculate_report_data(model: models.ExposureModel): repeated_events = [] for n in [1, 2, 3, 4, 5]: - repeat_model = dataclasses.replace(model, repeats=n) + + repeat_model = dataclass_utils.replace(model, repeats=n) repeated_events.append( RepeatEvents( repeats=n, @@ -70,7 +72,7 @@ def calculate_report_data(model: models.ExposureModel): def generate_qr_code(prefix, form: FormData): - form_dict = FormData.to_dict(form) + form_dict = FormData.to_dict(form, strip_defaults=True) # Generate the calculator URL arguments that would be needed to re-create this # form. @@ -198,12 +200,12 @@ def manufacture_alternative_scenarios(form: FormData) -> typing.Dict[str, models # The remaining scenarios are based on Type I masks (possibly not worn) # and no HEPA filtration. - form = dataclasses.replace(form, mask_type='Type I') + form = dataclass_utils.replace(form, mask_type='Type I') if form.hepa_option: - form = dataclasses.replace(form, hepa_option=False) + form = dataclass_utils.replace(form, hepa_option=False) - with_mask = dataclasses.replace(form, mask_wearing_option='mask_on') - without_mask = dataclasses.replace(form, mask_wearing_option='mask_off') + with_mask = dataclass_utils.replace(form, mask_wearing_option='mask_on') + without_mask = dataclass_utils.replace(form, mask_wearing_option='mask_off') if form.ventilation_type == 'mechanical_ventilation': scenarios['Mechanical ventilation with Type I masks'] = with_mask.build_model() @@ -214,8 +216,8 @@ def manufacture_alternative_scenarios(form: FormData) -> typing.Dict[str, models scenarios['Windows open without masks'] = without_mask.build_model() # No matter the ventilation scheme, we include scenarios which don't have any ventilation. - with_mask_no_vent = dataclasses.replace(with_mask, ventilation_type='no_ventilation') - without_mask_or_vent = dataclasses.replace(without_mask, ventilation_type='no_ventilation') + with_mask_no_vent = dataclass_utils.replace(with_mask, ventilation_type='no_ventilation') + without_mask_or_vent = dataclass_utils.replace(without_mask, ventilation_type='no_ventilation') scenarios['No ventilation with Type I masks'] = with_mask_no_vent.build_model() scenarios['Neither ventilation nor masks'] = without_mask_or_vent.build_model() diff --git a/cara/apps/calculator/static/js/form.js b/cara/apps/calculator/static/js/form.js index eb13c826..b15f2fc6 100644 --- a/cara/apps/calculator/static/js/form.js +++ b/cara/apps/calculator/static/js/form.js @@ -474,6 +474,8 @@ $(document).ready(function () { //Pre-select checked radios if (elemObj.type === 'radio') { + // Calculator <= 1.5.0 used to send not-applicable in the URL for radios that + // weren't set. Now those are not sent at all, but we keep the behaviour for compatibility. if (value !== 'not-applicable') { $('[name="'+name+'"][value="'+value+'"]').prop('checked',true); } diff --git a/cara/apps/calculator/templates/calculator.form.html.j2 b/cara/apps/calculator/templates/calculator.form.html.j2 index 7ecbf366..a47e8812 100644 --- a/cara/apps/calculator/templates/calculator.form.html.j2 +++ b/cara/apps/calculator/templates/calculator.form.html.j2 @@ -160,11 +160,11 @@ v{{ calculator_version }} Please sen
Exposed person(s) presence:
- Start:    - Finish:
+ Start:    + Finish:
Infected person(s) presence:
- Start:    - Finish:
+ Start:    + Finish:
Which month is the event? diff --git a/cara/dataclass_utils.py b/cara/dataclass_utils.py index cdfd867b..3915b9e8 100644 --- a/cara/dataclass_utils.py +++ b/cara/dataclass_utils.py @@ -24,3 +24,22 @@ def nested_replace(obj, new_values: typing.Dict[str, typing.Any]): # We have a plain old name. So set it. new_inst = dataclasses.replace(new_inst, **{name: value}) return new_inst + + +def replace(obj, **changes): + """ + A version of dataclasses.replace that handles ClassVar declarations. + + See https://bugs.python.org/issue33796. + + """ + + orig = obj.__dataclass_fields__ + object.__setattr__( + obj, '__dataclass_fields__', + {name: field for name, field in orig.items() + if field._field_type is not dataclasses._FIELD_CLASSVAR} + ) + new = dataclasses.replace(obj, **changes) + object.__setattr__(obj, '__dataclass_fields__', orig) + return new diff --git a/cara/tests/apps/calculator/test_model_generator.py b/cara/tests/apps/calculator/test_model_generator.py index 89a3c6e0..58c34092 100644 --- a/cara/tests/apps/calculator/test_model_generator.py +++ b/cara/tests/apps/calculator/test_model_generator.py @@ -1,3 +1,5 @@ +import dataclasses + import pytest from cara.apps.calculator import model_generator @@ -12,6 +14,12 @@ def test_model_from_dict(baseline_form_data): assert isinstance(form.build_model(), models.ExposureModel) +def test_model_from_dict_invalid(baseline_form_data): + baseline_form_data['invalid_item'] = 'foobar' + with pytest.raises(ValueError, match='Invalid argument "invalid_item" given'): + model_generator.FormData.from_dict(baseline_form_data) + + def test_blend_expiration(): blend = {'Breathing': 2, 'Talking': 1} r = model_generator.build_expiration(blend) @@ -381,3 +389,60 @@ def test_key_validation(baseline_form_data): baseline_form_data['activity_type'] = 'invalid key' with pytest.raises(ValueError): model_generator.FormData.from_dict(baseline_form_data) + + +def test_key_validation_natural_ventilation_window_type_na(baseline_form_data): + baseline_form_data['ventilation_type'] = 'natural_ventilation' + baseline_form_data['window_type'] = 'not-applicable' + with pytest.raises(ValueError, match='window_type cannot be \'not-applicable\''): + model_generator.FormData.from_dict(baseline_form_data) + + +def test_key_validation_natural_ventilation_window_opening_regime_na(baseline_form_data): + baseline_form_data['ventilation_type'] = 'natural_ventilation' + baseline_form_data['window_opening_regime'] = 'not-applicable' + with pytest.raises(ValueError, match='window_opening_regime cannot be \'not-applicable\''): + model_generator.FormData.from_dict(baseline_form_data) + + +def test_key_validation_mech_ventilation_type_na(baseline_form_data): + baseline_form_data['ventilation_type'] = 'mechanical_ventilation' + baseline_form_data['mechanical_ventilation_type'] = 'not-applicable' + with pytest.raises(ValueError, match='mechanical_ventilation_type cannot be \'not-applicable\''): + model_generator.FormData.from_dict(baseline_form_data) + + +def test_default_types(): + # Validate that FormData._DEFAULTS are complete and of the correct type. + # Validate that we have the right types and matching attributes to the DEFAULTS. + fields = {field.name: field for field in dataclasses.fields(model_generator.FormData)} + for field, value in model_generator.FormData._DEFAULTS.items(): + if field not in fields: + raise ValueError(f"Unmatched default {field}") + + field_type = fields[field].type + if not isinstance(field_type, type): + # Handle typing.NewType definitions. + field_type = field_type.__supertype__ + + if value is model_generator._NO_DEFAULT: + continue + + if field in model_generator._CAST_RULES_FORM_ARG_TO_NATIVE: + value = model_generator._CAST_RULES_FORM_ARG_TO_NATIVE[field](value) + + if not isinstance(value, field_type): + raise TypeError(f'{field} has type {field_type}, got {type(value)}') + + for field in fields.values(): + assert field.name in model_generator.FormData._DEFAULTS, f"No default set for field name {field.name}" + + +def test_form_to_dict(baseline_form): + full = baseline_form.to_dict(baseline_form) + stripped = baseline_form.to_dict(baseline_form, strip_defaults=True) + assert 1 < len(stripped) < len(full) + assert 'exposed_coffee_break_option' in stripped + # If we set the value to the default one, it should no longer turn up in the dictionary. + baseline_form.exposed_coffee_break_option = model_generator.FormData._DEFAULTS['exposed_coffee_break_option'] + assert 'exposed_coffee_break_option' not in baseline_form.to_dict(baseline_form, strip_defaults=True) diff --git a/cara/tests/apps/calculator/test_webapp.py b/cara/tests/apps/calculator/test_webapp.py index 24cb0819..2bb8d28a 100644 --- a/cara/tests/apps/calculator/test_webapp.py +++ b/cara/tests/apps/calculator/test_webapp.py @@ -26,7 +26,7 @@ async def test_calculator(http_server_client): async def test_qrcode_urls(http_server_client, baseline_form): prefix = 'proto://hostname/prefix' qr_data = generate_qr_code(prefix, baseline_form) - expected = f'{prefix}/calculator?activity_type=office&air_changes=0.0' + expected = f'{prefix}/calculator?exposed_coffee_break_option={baseline_form.exposed_coffee_break_option}&' assert qr_data['link'].startswith(expected) # We should get a 200 for the link.