From 9e4c04186fca606be7cff99c9d3bdeda5e26edac Mon Sep 17 00:00:00 2001 From: Juho Kettunen Date: Wed, 20 Nov 2024 14:04:14 +0200 Subject: [PATCH] Overhaul override_receivable_type input validation --- leasing/serializers/rent.py | 199 +++++++++++---- .../tests/models/test_calculate_invoices.py | 7 +- leasing/tests/serializers/test_rent.py | 229 ++++++++++++++---- 3 files changed, 351 insertions(+), 84 deletions(-) diff --git a/leasing/serializers/rent.py b/leasing/serializers/rent.py index 3dffbc68..36105ec4 100644 --- a/leasing/serializers/rent.py +++ b/leasing/serializers/rent.py @@ -1,3 +1,5 @@ +from typing import Any + from django.utils.translation import gettext_lazy as _ from enumfields.drf import EnumSupportSerializerMixin from rest_framework import serializers @@ -5,13 +7,7 @@ from rest_framework.serializers import ListSerializer from field_permissions.serializers import FieldPermissionsSerializerMixin -from leasing.enums import ( - DueDatesType, - RentAdjustmentAmountType, - RentCycle, - RentType, - is_akv_or_kuva_service_unit_id, -) +from leasing.enums import DueDatesType, RentAdjustmentAmountType, RentCycle, RentType from leasing.models import ( ContractRent, Decision, @@ -578,61 +574,182 @@ def validate_seasonal_values(self, rent_data: dict) -> None: validate_seasonal_day_for_month(start_day, start_month) validate_seasonal_day_for_month(end_day, end_month) - def validate_override_receivable_type_value(self, rent_data: dict) -> None: + def validate_override_receivable_type_value( + self, rent_data: dict[str, Any] + ) -> None: """ - Override receivabletype is mandatory for AKV and KuVa leases, and not - used by MaKe/Tontit. + Currently, the rent override receivabletype is mandatory for AKV and + KuVa leases, and not used by MaKe/Tontit. It is only used in index rents, fixed rents, and manual rents, because these rent types can generate automatic invoices. + TODO Blind spot: this implementation cannot reject missing override + receivable type when creating a new rent, because the validator doesn't + receive enough details from the frontend. + - Cannot read service unit from the receivable type if it's missing + - Cannot find out the service unit from rent's lease if rent + doesn't exist yet. + Raises: serializers.ValidationError """ - override_receivable_type = rent_data.get("override_receivable_type") + rent_type = rent_data.get("type", {}) + rent_types_that_can_use_override_receivable_type = [ + RentType.INDEX, + RentType.INDEX2022, + RentType.FIXED, + RentType.MANUAL, + ] + # These rent types can generate automatic invoices, so they can utilize + # the override receivabletype, if required by service unit. + rent_type_uses_override = ( + rent_type in rent_types_that_can_use_override_receivable_type + ) - if not override_receivable_type: - # Empty override receivabletype input should be allowed, because it - # is always empty for all MaKe rents, and those must be allowed. + rent_id = rent_data.get("id") + if rent_id is None: + # The ID is not present in creation flow, and is removed from + # rent_data before later validations also during the update flow. + # Try get ID from the rent instance, which is present if an existing + # rent is being updated. + rent_id = self.instance.id if self.instance else None + + if rent_id is not None: + self.full_validate_override_receivable_type( + rent_data, rent_id, rent_type_uses_override + ) + else: + self.minimal_validate_override_receivable_type( + rent_data, rent_type_uses_override + ) + + def full_validate_override_receivable_type( + self, + rent_data: dict[str, Any], + rent_id: int, + rent_type_uses_override: bool, + ) -> None: + """ + Perform full validation based on the service unit, receivabletype, + and rent type. + + We have all necessary details to check whether the lease's service unit + uses the override receivabletype feature or not. + + Raises: serializers.ValidationError + """ + rent = Rent.objects.select_related("lease__service_unit").get(pk=rent_id) + use_rent_override_receivable_type: bool = ( + rent.lease.service_unit.use_rent_override_receivable_type + ) + override_receivable_type: ReceivableType | None = rent_data.get( + "override_receivable_type" + ) + + if (not use_rent_override_receivable_type) and (not override_receivable_type): + # This service unit does not use the override receivabletype feature, + # and none was supplied -> all good. + # For example, service unit MaKe/Tontit. return - elif not is_akv_or_kuva_service_unit_id( - override_receivable_type.service_unit_id - ): - # All MaKe receivabletypes should be rejected regardless of rent - # type, because MaKe doesn't use the override feature, and if any - # other service unit would use MaKe's receivabletypes, it would be - # a mistake. - raise serializers.ValidationError( - _( - f'Override receivabletype "{override_receivable_type.name}" was unexpected. ' - "Override receivabletype is not used by MaKe/Tontit." + if use_rent_override_receivable_type and override_receivable_type: + if rent_type_uses_override: + # Override receivabletype is required, was supplied, and rent + # type is correct -> all good. + # For example, service units AKV and KuVa. + return + else: + raise serializers.ValidationError( + _( + f'Override receivabletype "{override_receivable_type.name}" was unexpected. ' + f"This rent type does not generate automatic invoices. " + "Please contact MVJ developers about this error." + ) ) - ) - elif is_akv_or_kuva_service_unit_id(override_receivable_type.service_unit_id): - rent_type = rent_data.get("type", {}) - - if rent_type in [ - RentType.INDEX, - RentType.INDEX2022, - RentType.FIXED, - RentType.MANUAL, - ]: - # These rent types can generate automatic invoices, so they can - # utilize the override receivabletype. + if use_rent_override_receivable_type and (not override_receivable_type): + if not rent_type_uses_override: + # This rent type does not utilize rent_override_receivable_type, + # even if the service unit generally uses it. return else: raise serializers.ValidationError( _( - f'Override receivabletype "{override_receivable_type.name}" was unexpected. ' - f'Rent type "{rent_type.name}" does not generate automatic invoices.' + f'Override receivabletype is required for service unit "{rent.lease.service_unit.name}", ' + f"and for this rent type." ) ) - else: + if (not use_rent_override_receivable_type) and override_receivable_type: raise serializers.ValidationError( _( - "Unhandled case in override receivabletype validation. Rejecting just in case." + f'Override receivabletype "{override_receivable_type.name}" was unexpected. ' + f'Service unit "{rent.lease.service_unit.name}" does not use this feature. ' + "Please contact MVJ developers about this error." + ) + ) + + raise serializers.ValidationError( + _( + "Unhandled case in override receivabletype validation. Rejecting just in case. " + "Please contact MVJ developers about this error." + ) + ) + + def minimal_validate_override_receivable_type( + self, + rent_data: dict[str, Any], + rent_type_uses_override: bool, + ) -> None: + """ + Only perform minimal validation based on the receivabletype, and rent + type. + + This rent might be a new rent being created, so we can't reference the + containing lease and its service unit with the rent ID because the ID + doesn't exist yet. + + This method is also visited during rent updates, when the logic performs + additional validation before saving the updated details to database. + In this case the rent ID is also not available, because it is stripped + from the input before calling validation. + + Raises: serializers.ValidationError + """ + override_receivable_type: ReceivableType | None = rent_data.get( + "override_receivable_type" + ) + if override_receivable_type is None: + # Empty override receivabletype input must be allowed, because it + # is always empty for all MaKe rents, and those must be allowed. + # Without it, we cannot make further validations about its properties. + return + + service_unit = override_receivable_type.service_unit + receivabletypes_service_unit_uses_override = ( + service_unit.use_rent_override_receivable_type + ) + + if receivabletypes_service_unit_uses_override and rent_type_uses_override: + # Service unit and rent types require an override receivabletype, and it was supplied + # --> all good. + return + + if not receivabletypes_service_unit_uses_override: + raise serializers.ValidationError( + _( + f'Override receivabletype "{override_receivable_type.name}" was unexpected. ' + f'Override receivabletype is not used by service unit "{service_unit.name}". ' + "Please contact MVJ developers about this error." + ) + ) + + if not rent_type_uses_override: + raise serializers.ValidationError( + _( + f'Override receivabletype "{override_receivable_type.name}" was unexpected. ' + f"This rent type does not generate automatic invoices. " + "Please contact MVJ developers about this error." ) ) diff --git a/leasing/tests/models/test_calculate_invoices.py b/leasing/tests/models/test_calculate_invoices.py index a6c3d688..a6716047 100644 --- a/leasing/tests/models/test_calculate_invoices.py +++ b/leasing/tests/models/test_calculate_invoices.py @@ -493,6 +493,10 @@ def test_calculate_invoices_uses_correct_receivable_type( rent_factory, contract_rent_factory, ): + """ + By default, invoice generation uses the service unit's default + receivable type for rents, if no other receivable types are specified. + """ service_units = [ service_unit_factory(name="First service unit"), service_unit_factory(name="Second service unit"), @@ -593,7 +597,8 @@ def test_calculate_invoices_uses_override_receivable_type( ): """ If an override_receivable_type is defined in a rent, that receivable type is - used in the invoice over the default value. + used in the invoice generation over the service unit's + default_receivable_type_rent. """ # Mandatory set up service_unit: ServiceUnit = service_unit_factory(name="ServiceUnitName") diff --git a/leasing/tests/serializers/test_rent.py b/leasing/tests/serializers/test_rent.py index 7f518aaf..1685168d 100644 --- a/leasing/tests/serializers/test_rent.py +++ b/leasing/tests/serializers/test_rent.py @@ -1,83 +1,228 @@ import pytest from rest_framework.exceptions import ValidationError -from leasing.enums import RentType, ServiceUnitId -from leasing.models import ServiceUnit +from leasing.enums import RentType +from leasing.models import Rent from leasing.serializers.rent import RentCreateUpdateSerializer -@pytest.mark.django_db -def test_is_valid_override_receivable_type(django_db_setup, receivable_type_factory): +@pytest.fixture +def rent_types_with_override() -> list[str]: + """These rent types can use the rent override receivabletype during invoicing.""" + return [ + RentType.FIXED, + RentType.INDEX, + RentType.INDEX2022, + RentType.MANUAL, + ] + + +@pytest.fixture +def rent_types_without_override() -> list[str]: + """These rent types do not use the rent override receivabletype during invoicing.""" + return [RentType.FREE, RentType.ONE_TIME] + + +@pytest.fixture +def rent_without_override_receivable_type( + service_unit_factory, + lease_factory, + rent_factory, +) -> Rent: """ - Test that the requirements described in the target function docstring hold. + Test data fixture for a rent with a lease and service unit that doesn't use + the override receivabletype feature. """ - make = ServiceUnit.objects.get(pk=ServiceUnitId.MAKE) - akv = ServiceUnit.objects.get(pk=ServiceUnitId.AKV) - kuva_lipa = ServiceUnit.objects.get(pk=ServiceUnitId.KUVA_LIPA) - kuva_upa = ServiceUnit.objects.get(pk=ServiceUnitId.KUVA_UPA) - kuva_nup = ServiceUnit.objects.get(pk=ServiceUnitId.KUVA_NUP) + service_unit_without_override = service_unit_factory( + name="Service unit without rent override receivabletype", + use_rent_override_receivable_type=False, + ) + lease_from_unit_without_override = lease_factory( + service_unit=service_unit_without_override, + ) + return rent_factory(lease=lease_from_unit_without_override) - serializer = RentCreateUpdateSerializer() - # Validator should reject all MaKe receivable types regardless of rent type. - rent_datas_make = [ +@pytest.fixture +def rent_with_override_receivable_type( + service_unit_factory, + lease_factory, + rent_factory, +) -> Rent: + """ + Test data fixture for a rent with a lease and service unit that uses the + override receivabletype feature. + """ + service_unit_with_override = service_unit_factory( + name="Service unit with rent override receivabletype", + use_rent_override_receivable_type=True, + ) + lease_from_unit_with_override = lease_factory( + service_unit=service_unit_with_override, + ) + return rent_factory(lease=lease_from_unit_with_override) + + +@pytest.mark.django_db +def test_validate_override_receivable_type_invalid_service_unit( + django_db_setup, + receivable_type_factory, + rent_without_override_receivable_type, +): + """ + RentCreateUpdateSerializer should reject receivable types from service units + that don't use override receivabletype, regardless of rent type. + """ + serializer = RentCreateUpdateSerializer() + rent_datas_invalid_service_unit = [ { "type": rent_type, - "override_receivable_type": receivable_type_factory(service_unit=make), + "override_receivable_type": receivable_type_factory( + service_unit=rent_without_override_receivable_type.lease.service_unit + ), } for rent_type in RentType ] - for data in rent_datas_make: + for data in rent_datas_invalid_service_unit: + # Input without a rent ID (e.g. during create) with pytest.raises(ValidationError): serializer.validate(data) - # Validator should allow empty override receivable type input. - rent_datas_empty = [ + # Input with a rent ID (e.g. during update) + data["id"] = rent_without_override_receivable_type.id + with pytest.raises(ValidationError): + serializer.validate(data) + + +@pytest.mark.django_db +def test_validate_override_receivable_type_valid_inputs( + django_db_setup, + receivable_type_factory, + rent_with_override_receivable_type, + rent_types_with_override, +): + """ + RentCreateUpdateSerializer should allow override receivabletype from service + units that use the feature, if rent type is valid. + """ + serializer = RentCreateUpdateSerializer() + rent_datas_valid_types_valid_unit = [ + { + "type": rent_type, + "override_receivable_type": receivable_type_factory( + service_unit=rent_with_override_receivable_type.lease.service_unit + ), + } + for rent_type in rent_types_with_override + ] + for data in rent_datas_valid_types_valid_unit: + # Input without a rent ID (e.g. during create) + assert serializer.validate(data) + + # Input with a rent ID (e.g. during update) + data["id"] = rent_with_override_receivable_type.id + assert serializer.validate(data) + + +@pytest.mark.django_db +def test_validate_override_receivable_type_lacking_inputs( + django_db_setup, +): + """ + RentCreateUpdateSerializer should allow empty override receivable type + input, when no additional details are known about the service unit or + receivable type. + + This is the case for example when creating new rents with rent types that + don't utilize the rent override receivabletype feature. + """ + serializer = RentCreateUpdateSerializer() + rent_datas_without_override_or_rent_id = [ { "type": rent_type, "override_receivable_type": None, } for rent_type in RentType ] - for data in rent_datas_empty: + for data in rent_datas_without_override_or_rent_id: assert serializer.validate(data) - # Validator should allow AKV and KuVa receivable types, if rent type is valid. - rent_datas_akv_and_kuva = [ + +@pytest.mark.django_db +def test_validate_override_receivable_type_missing_receivabletype( + django_db_setup, + rent_with_override_receivable_type, + rent_types_with_override, +): + """ + RentCreateUpdateSerializer should reject the input when service unit and + rent types require override receivabletype, but it was not supplied, and + the rent ID is known. + + The expected case is during an update of existing rent. + During create, rent ID would not be known in validation. + """ + serializer = RentCreateUpdateSerializer() + rent_datas_valid_types_valid_unit_existing_rent_no_override = [ { - "type": RentType.INDEX2022, - "override_receivable_type": receivable_type_factory(service_unit=unit), + "id": rent_with_override_receivable_type.id, + "type": rent_type, + "override_receivable_type": None, } - for unit in [akv, kuva_lipa, kuva_upa, kuva_nup] + for rent_type in rent_types_with_override ] - for data in rent_datas_akv_and_kuva: - assert serializer.validate(data) + for data in rent_datas_valid_types_valid_unit_existing_rent_no_override: + with pytest.raises(ValidationError): + serializer.validate(data) + - # Validator should allow rent types that can generate automatic invoices. - rent_datas_valid_types = [ +@pytest.mark.django_db +def test_validate_override_receivable_type_invalid_rent_type( + django_db_setup, + receivable_type_factory, + rent_with_override_receivable_type, + rent_without_override_receivable_type, + rent_types_without_override, +): + """ + RentCreateUpdateSerializer should reject the input when rent type doesn't + use override receivabletype in invoicing, but it was supplied. + """ + serializer = RentCreateUpdateSerializer() + rent_datas_invalid_types_invalid_unit = [ { "type": rent_type, - "override_receivable_type": receivable_type_factory(service_unit=akv), + "override_receivable_type": receivable_type_factory( + service_unit=rent_without_override_receivable_type.lease.service_unit + ), } - for rent_type in [ - RentType.FIXED, - RentType.INDEX, - RentType.INDEX2022, - RentType.MANUAL, - ] + for rent_type in rent_types_without_override ] - for data in rent_datas_valid_types: - assert serializer.validate(data) + for data in rent_datas_invalid_types_invalid_unit: + # Flow without rent ID + with pytest.raises(ValidationError): + serializer.validate(data) + + # Flow with rent ID + data["id"] = rent_without_override_receivable_type.id + with pytest.raises(ValidationError): + serializer.validate(data) - # Validator should reject the receivable type input for rent types that - # don't generate automatic invoices. - rent_datas_invalid_types = [ + rent_datas_invalid_types_valid_unit = [ { "type": rent_type, - "override_receivable_type": receivable_type_factory(service_unit=akv), + "override_receivable_type": receivable_type_factory( + service_unit=rent_with_override_receivable_type.lease.service_unit + ), } - for rent_type in [RentType.FREE, RentType.ONE_TIME] + for rent_type in rent_types_without_override ] - for data in rent_datas_invalid_types: + for data in rent_datas_invalid_types_valid_unit: + # Flow without rent ID + with pytest.raises(ValidationError): + serializer.validate(data) + + # Flow with rent ID + data["id"] = rent_with_override_receivable_type.id with pytest.raises(ValidationError): serializer.validate(data)