From 3a66dfc72f2907725d26d5c351fc0ab7f6f4ea10 Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Tue, 25 May 2021 10:06:48 +0100 Subject: [PATCH 1/8] Make factory and serializer names match model name --- api/applications/serializers/good.py | 8 ++++---- .../tests/test_standard_application_submit.py | 4 ++-- api/goods/serializers.py | 18 +++++++++--------- api/goods/tests/factories.py | 2 +- api/letter_templates/context_generator.py | 4 ++-- .../tests/test_context_generation.py | 8 ++++---- 6 files changed, 22 insertions(+), 22 deletions(-) diff --git a/api/applications/serializers/good.py b/api/applications/serializers/good.py index f8c54655d..8c3cae64f 100644 --- a/api/applications/serializers/good.py +++ b/api/applications/serializers/good.py @@ -15,7 +15,7 @@ from api.goods.enums import GoodControlled, ItemType from api.goods.helpers import update_firearms_certificate_data from api.goods.models import Good -from api.goods.serializers import GoodSerializerInternal, FirearmDetailsSerializer +from api.goods.serializers import GoodSerializerInternal, FirearmGoodDetailsSerializer from api.licences.models import GoodOnLicence from api.organisations.models import DocumentOnOrganisation from api.staticdata.control_list_entries.serializers import ControlListEntrySerializer @@ -68,7 +68,7 @@ class GoodOnApplicationViewSerializer(serializers.ModelSerializer): control_list_entries = ControlListEntrySerializer(many=True) audit_trail = serializers.SerializerMethodField() is_good_controlled = KeyValueChoiceField(choices=GoodControlled.choices) - firearm_details = FirearmDetailsSerializer() + firearm_details = FirearmGoodDetailsSerializer() class Meta: model = GoodOnApplication @@ -125,7 +125,7 @@ class GoodOnApplicationCreateSerializer(serializers.ModelSerializer): max_length=100, error_messages={"required": strings.Goods.OTHER_ITEM_TYPE, "blank": strings.Goods.OTHER_ITEM_TYPE}, ) - firearm_details = FirearmDetailsSerializer(required=False) + firearm_details = FirearmGoodDetailsSerializer(required=False) class Meta: model = GoodOnApplication @@ -190,7 +190,7 @@ def create(self, validated_data): if validated_data.get("firearm_details"): firearm_data.update(validated_data["firearm_details"]) firearm_data = update_firearms_certificate_data(validated_data["good"].organisation_id, firearm_data) - serializer = FirearmDetailsSerializer(data=firearm_data) + serializer = FirearmGoodDetailsSerializer(data=firearm_data) serializer.is_valid(raise_exception=True) validated_data["firearm_details"] = serializer.save() return super().create(validated_data) diff --git a/api/applications/tests/test_standard_application_submit.py b/api/applications/tests/test_standard_application_submit.py index a0f49ba00..5d9fb8b48 100644 --- a/api/applications/tests/test_standard_application_submit.py +++ b/api/applications/tests/test_standard_application_submit.py @@ -13,7 +13,7 @@ from api.core.constants import AutoGeneratedDocuments from api.flags.enums import SystemFlags from api.goods.enums import GoodStatus, ItemCategory -from api.goods.tests.factories import FirearmFactory, GoodFactory +from api.goods.tests.factories import FirearmGoodDetailsFactory, GoodFactory from lite_content.lite_api import strings from api.parties.enums import PartyType from api.parties.models import PartyDocument @@ -411,7 +411,7 @@ def test_standard_application_declaration_submit_tcs_false_failure(self): def test_submit_standard_application_adds_system_case_flags_success(self, upload_bytes_file_func, html_to_pdf_func): upload_bytes_file_func.return_value = None html_to_pdf_func.return_value = None - firearm_details = FirearmFactory() + firearm_details = FirearmGoodDetailsFactory() firearm_good = GoodFactory( description="a good", organisation=self.organisation, diff --git a/api/goods/serializers.py b/api/goods/serializers.py index b1011ce01..b453be7e9 100644 --- a/api/goods/serializers.py +++ b/api/goods/serializers.py @@ -90,7 +90,7 @@ def to_internal_value(self, data): raise error -class FirearmDetailsSerializer(serializers.ModelSerializer): +class FirearmGoodDetailsSerializer(serializers.ModelSerializer): type = KeyValueChoiceField( choices=FirearmGoodType.choices, allow_null=False, @@ -166,7 +166,7 @@ class Meta: ) def validate(self, data): - validated_data = super(FirearmDetailsSerializer, self).validate(data) + validated_data = super(FirearmGoodDetailsSerializer, self).validate(data) # Year of manufacture should be in the past and a valid year year_of_manufacture = validated_data.get("year_of_manufacture") @@ -362,7 +362,7 @@ class GoodCreateSerializer(serializers.ModelSerializer): software_or_technology_details = serializers.CharField( allow_null=True, required=False, allow_blank=True, max_length=2000 ) - firearm_details = FirearmDetailsSerializer(allow_null=True, required=False) + firearm_details = FirearmGoodDetailsSerializer(allow_null=True, required=False) class Meta: model = Good @@ -595,12 +595,12 @@ def _delete_pv_grading_details(instance): @staticmethod def _create_firearm_details(firearm_details): - return FirearmDetailsSerializer.create(FirearmDetailsSerializer(), validated_data=firearm_details) + return FirearmGoodDetailsSerializer.create(FirearmGoodDetailsSerializer(), validated_data=firearm_details) @staticmethod def _update_firearm_details(firearm_details, instance): - return FirearmDetailsSerializer.update( - FirearmDetailsSerializer(), validated_data=firearm_details, instance=instance, + return FirearmGoodDetailsSerializer.update( + FirearmGoodDetailsSerializer(), validated_data=firearm_details, instance=instance, ) @@ -717,7 +717,7 @@ class GoodSerializerInternal(serializers.Serializer): is_document_available = serializers.BooleanField() is_document_sensitive = serializers.BooleanField() software_or_technology_details = serializers.CharField() - firearm_details = FirearmDetailsSerializer(allow_null=True, required=False) + firearm_details = FirearmGoodDetailsSerializer(allow_null=True, required=False) is_precedent = serializers.BooleanField(required=False, default=False) def get_documents(self, instance): @@ -726,7 +726,7 @@ def get_documents(self, instance): class TinyGoodDetailsSerializer(serializers.ModelSerializer): - firearm_details = FirearmDetailsSerializer(read_only=True) + firearm_details = FirearmGoodDetailsSerializer(read_only=True) class Meta: model = Good @@ -761,7 +761,7 @@ class GoodSerializerExporter(serializers.Serializer): information_security_details = serializers.CharField() pv_grading_details = PvGradingDetailsSerializer(allow_null=True, required=False) software_or_technology_details = serializers.CharField() - firearm_details = FirearmDetailsSerializer(allow_null=True, required=False) + firearm_details = FirearmGoodDetailsSerializer(allow_null=True, required=False) class GoodSerializerExporterFullDetail(GoodSerializerExporter): diff --git a/api/goods/tests/factories.py b/api/goods/tests/factories.py index f261bc365..23ebd65d6 100644 --- a/api/goods/tests/factories.py +++ b/api/goods/tests/factories.py @@ -42,7 +42,7 @@ def flags(self, create, extracted, **kwargs): self.flags.set(extracted) -class FirearmFactory(factory.django.DjangoModelFactory): +class FirearmGoodDetailsFactory(factory.django.DjangoModelFactory): type = FirearmGoodType.AMMUNITION year_of_manufacture = 2019 calibre = "5.56x45mm" diff --git a/api/letter_templates/context_generator.py b/api/letter_templates/context_generator.py index 4c0835f08..1ec78e279 100644 --- a/api/letter_templates/context_generator.py +++ b/api/letter_templates/context_generator.py @@ -571,7 +571,7 @@ def get_control_list_entries(self, obj): return [clc.rating for clc in obj.control_list_entries.all()] -class FirearmDetailsSerializer(serializers.ModelSerializer): +class FirearmGoodDetailsSerializer(serializers.ModelSerializer): class Meta: model = FirearmGoodDetails fields = [ @@ -659,7 +659,7 @@ class Meta: ] good = GoodSerializer() - firearm_details = FirearmDetailsSerializer() + firearm_details = FirearmGoodDetailsSerializer() is_incorporated = FriendlyBooleanField(source="is_good_incorporated") applied_for_quantity = serializers.SerializerMethodField() applied_for_value = serializers.SerializerMethodField() diff --git a/api/letter_templates/tests/test_context_generation.py b/api/letter_templates/tests/test_context_generation.py index 84df9a12a..da510d5bd 100644 --- a/api/letter_templates/tests/test_context_generation.py +++ b/api/letter_templates/tests/test_context_generation.py @@ -33,7 +33,7 @@ GoodControlled, GoodPvGraded, ) -from api.goods.tests.factories import GoodFactory, FirearmFactory +from api.goods.tests.factories import GoodFactory, FirearmGoodDetailsFactory from api.goodstype.tests.factories import GoodsTypeFactory from api.letter_templates.context_generator import get_document_context from api.licences.enums import LicenceStatus @@ -884,7 +884,7 @@ def test_generate_context_with_category_3_good_details(self): def test_generate_context_with_category_2_good_details_legacy(self): application = self.create_standard_application_case(self.organisation) application.goods.all().delete() - firearm_details = FirearmFactory() + firearm_details = FirearmGoodDetailsFactory() good = GoodFactory( organisation=self.organisation, item_category=ItemCategory.GROUP2_FIREARMS, firearm_details=firearm_details ) @@ -906,8 +906,8 @@ def test_generate_context_with_category_2_good_details_legacy(self): def test_generate_context_with_category_2_good_details(self): application = self.create_standard_application_case(self.organisation) application.goods.all().delete() - firearm_details = FirearmFactory(year_of_manufacture=2020) - firearm_details_on_application = FirearmFactory(year_of_manufacture=1919) + firearm_details = FirearmGoodDetailsFactory(year_of_manufacture=2020) + firearm_details_on_application = FirearmGoodDetailsFactory(year_of_manufacture=1919) good = GoodFactory( organisation=self.organisation, item_category=ItemCategory.GROUP2_FIREARMS, firearm_details=firearm_details ) From 9748acf5eb4e52be259db36b97f4233afbd5480c Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Tue, 25 May 2021 14:50:31 +0100 Subject: [PATCH 2/8] Add additional validation for certificate expiry date --- api/goods/helpers.py | 9 +- api/goods/serializers.py | 2 +- api/goods/tests/test_create.py | 144 ++++++++++---------- api/goods/tests/test_serializers.py | 44 ++++++ api/organisations/serializers.py | 9 ++ api/organisations/tests/test_serializers.py | 52 +++++++ test_helpers/helpers.py | 7 +- 7 files changed, 194 insertions(+), 73 deletions(-) create mode 100644 api/organisations/tests/test_serializers.py diff --git a/api/goods/helpers.py b/api/goods/helpers.py index 1671c2cfe..71ddf219c 100644 --- a/api/goods/helpers.py +++ b/api/goods/helpers.py @@ -1,4 +1,6 @@ from django.utils import timezone +from dateutil.relativedelta import relativedelta + from rest_framework import serializers from rest_framework.exceptions import ValidationError @@ -138,7 +140,7 @@ def validate_firearms_act_certificate_expiry_date(validated_data): if covered_by_firearms_act == "No" or covered_by_firearms_act == "Unsure": return - certificate_missing = validated_data.get("section_certificate_missing", False) == True + certificate_missing = validated_data.get("section_certificate_missing", False) if certificate_missing: if validated_data.get("section_certificate_missing_reason", "") == "": errors["section_certificate_missing_reason"] = "Enter a reason why you do not have a section 1 certificate" @@ -157,6 +159,11 @@ def validate_firearms_act_certificate_expiry_date(validated_data): if date_of_expiry and date_of_expiry < timezone.now().date(): errors["section_certificate_date_of_expiry"] = strings.Goods.FIREARM_GOOD_INVALID_EXPIRY_DATE + difference_in_years = relativedelta(date_of_expiry, timezone.now().date()).years + + if difference_in_years > 5: + errors["section_certificate_date_of_expiry"] = "Expiry date is too far in the future" + if errors: raise serializers.ValidationError(errors) diff --git a/api/goods/serializers.py b/api/goods/serializers.py index b453be7e9..bd6ba89d2 100644 --- a/api/goods/serializers.py +++ b/api/goods/serializers.py @@ -187,7 +187,7 @@ def validate(self, data): raise serializers.ValidationError({"is_replica": "Select yes if the product is a replica firearm"}) if validated_data.get("is_replica") is True: - if "replica_description" not in validated_data or validated_data.get("replica_description") is "": + if "replica_description" not in validated_data or validated_data.get("replica_description") == "": raise serializers.ValidationError({"replica_description": "Enter description"}) if validated_data.get("is_replica") is not None and "firearms" != validated_data.get("type"): raise serializers.ValidationError({"is_replica": "Invalid firearm product type"}) diff --git a/api/goods/tests/test_create.py b/api/goods/tests/test_create.py index 21aa32684..618b5de48 100644 --- a/api/goods/tests/test_create.py +++ b/api/goods/tests/test_create.py @@ -1,3 +1,4 @@ +import unittest import uuid from copy import deepcopy @@ -18,8 +19,9 @@ from api.goods.models import Good from lite_content.lite_api import strings from api.staticdata.control_list_entries.helpers import get_control_list_entry -from api.staticdata.control_list_entries.models import ControlListEntry + from test_helpers.clients import DataTestClient +from test_helpers.helpers import mocked_now URL = reverse("goods:goods") @@ -578,40 +580,41 @@ def test_add_category_two_good_no_year_of_manufacture_in_the_past_success(self, self.assertEqual(good["firearm_details"]["year_of_manufacture"], year) def test_add_firearms_act_section_question_check_errors(self): - data = { - "name": "Rifle", - "description": "Firearm product", - "is_good_controlled": False, - "is_pv_graded": GoodPvGraded.NO, - "item_category": ItemCategory.GROUP2_FIREARMS, - "validate_only": True, - "firearm_details": { - "type": FirearmGoodType.FIREARMS, - "calibre": "9mm", - "year_of_manufacture": "2010", - "is_covered_by_firearm_act_section_one_two_or_five": "", - "firearms_act_section": "firearms_act_section1", - "section_certificate_number": "ABC123", - "section_certificate_date_of_expiry": "2012-12-12", - }, - } - - response = self.client.post(URL, data, **self.exporter_headers) - - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - response = response.json()["errors"] - self.assertEqual( - response["is_covered_by_firearm_act_section_one_two_or_five"][0], - "Select yes if the product is covered by Section 1, Section 2 or Section 5 of the Firearms Act 1968", - ) - - data["firearm_details"]["is_covered_by_firearm_act_section_one_two_or_five"] = "Yes" - data["firearm_details"]["firearms_act_section"] = "" - response = self.client.post(URL, data, **self.exporter_headers) - - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - response = response.json()["errors"] - self.assertEqual(response["firearms_act_section"][0], "Select which section the product is covered by") + with unittest.mock.patch("django.utils.timezone.now", side_effect=mocked_now): + data = { + "name": "Rifle", + "description": "Firearm product", + "is_good_controlled": False, + "is_pv_graded": GoodPvGraded.NO, + "item_category": ItemCategory.GROUP2_FIREARMS, + "validate_only": True, + "firearm_details": { + "type": FirearmGoodType.FIREARMS, + "calibre": "9mm", + "year_of_manufacture": "2010", + "is_covered_by_firearm_act_section_one_two_or_five": "", + "firearms_act_section": "firearms_act_section1", + "section_certificate_number": "ABC123", + "section_certificate_date_of_expiry": "2012-12-12", + }, + } + + response = self.client.post(URL, data, **self.exporter_headers) + + self.assertEquals(response.status_code, status.HTTP_400_BAD_REQUEST) + response = response.json()["errors"] + self.assertEqual( + response["is_covered_by_firearm_act_section_one_two_or_five"][0], + "Select yes if the product is covered by Section 1, Section 2 or Section 5 of the Firearms Act 1968", + ) + + data["firearm_details"]["is_covered_by_firearm_act_section_one_two_or_five"] = "Yes" + data["firearm_details"]["firearms_act_section"] = "" + response = self.client.post(URL, data, **self.exporter_headers) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + response = response.json()["errors"] + self.assertEqual(response["firearms_act_section"][0], "Select which section the product is covered by") def test_add_firearms_certificate_missing_checks(self): data = { @@ -646,41 +649,42 @@ def test_add_firearms_certificate_missing_checks(self): ) def test_add_firearms_certificate_question_check_errors(self): - data = { - "name": "Rifle", - "description": "Firearm product", - "is_good_controlled": False, - "is_pv_graded": GoodPvGraded.NO, - "item_category": ItemCategory.GROUP2_FIREARMS, - "validate_only": True, - "firearm_details": { - "type": FirearmGoodType.AMMUNITION, - "calibre": "0.5", - "year_of_manufacture": "1991", - "is_covered_by_firearm_act_section_one_two_or_five": "Yes", - "firearms_act_section": "firearms_act_section1", - "section_certificate_number": "", - "section_certificate_date_of_expiry": "2012-12-12", - }, - } - - response = self.client.post(URL, data, **self.exporter_headers) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - response = response.json()["errors"] - self.assertEqual(response["section_certificate_number"][0], "Enter the certificate number") - - data["firearm_details"]["section_certificate_number"] = "FR8C1604" - data["firearm_details"]["section_certificate_date_of_expiry"] = None - response = self.client.post(URL, data, **self.exporter_headers) - self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) - response = response.json()["errors"] - - self.assertEqual( - response["section_certificate_date_of_expiry"][0], - "Enter the certificate expiry date and include a day, month and year", - ) - - def test_add_firearms_certificate_missing_checks(self): + with unittest.mock.patch("django.utils.timezone.now", side_effect=mocked_now): + data = { + "name": "Rifle", + "description": "Firearm product", + "is_good_controlled": False, + "is_pv_graded": GoodPvGraded.NO, + "item_category": ItemCategory.GROUP2_FIREARMS, + "validate_only": True, + "firearm_details": { + "type": FirearmGoodType.AMMUNITION, + "calibre": "0.5", + "year_of_manufacture": "1991", + "is_covered_by_firearm_act_section_one_two_or_five": "Yes", + "firearms_act_section": "firearms_act_section1", + "section_certificate_number": "", + "section_certificate_date_of_expiry": "2012-12-12", + }, + } + + response = self.client.post(URL, data, **self.exporter_headers) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + response = response.json()["errors"] + self.assertEqual(response["section_certificate_number"][0], "Enter the certificate number") + + data["firearm_details"]["section_certificate_number"] = "FR8C1604" + data["firearm_details"]["section_certificate_date_of_expiry"] = None + response = self.client.post(URL, data, **self.exporter_headers) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + response = response.json()["errors"] + + self.assertEqual( + response["section_certificate_date_of_expiry"][0], + "Enter the certificate expiry date and include a day, month and year", + ) + + def test_add_firearms_certificate_missing_checks_2(self): data = { "name": "Rifle", "description": "Firearm product", diff --git a/api/goods/tests/test_serializers.py b/api/goods/tests/test_serializers.py index 69ed90404..b2a805c31 100644 --- a/api/goods/tests/test_serializers.py +++ b/api/goods/tests/test_serializers.py @@ -1,6 +1,8 @@ +import pytest import unittest from api.goods import serializers +from test_helpers.helpers import mocked_now class TestPvGradingDetailsSerializer(unittest.TestCase): @@ -14,3 +16,45 @@ def test_pv_grading_details_serializer(self): self.assertEqual(serializer_two.is_valid(), False) self.assertEqual("custom_grading" in serializer_two.errors, False) + + +@pytest.mark.parametrize( + "data,valid,error", + [ + ( + { + "section_certificate_date_of_expiry": "2000-01-01", + "section_certificate_number": "1", + "certificate_missing": False, + }, + False, + "Expiry date must be in the future", + ), + ( + { + "section_certificate_date_of_expiry": "2012-01-01", + "section_certificate_number": "1", + "certificate_missing": False, + }, + True, + None, + ), + ( + { + "section_certificate_date_of_expiry": "2016-01-01", + "section_certificate_number": "1", + "certificate_missing": False, + }, + False, + "Expiry date is too far in the future", + ), + ], +) +@unittest.mock.patch("django.utils.timezone.now", side_effect=mocked_now) +def test_firearms_details_serializer(mock_timezone, data, valid, error): + + serializer = serializers.FirearmGoodDetailsSerializer(data=data) + + assert serializer.is_valid() == valid + if not valid: + assert serializer.errors["section_certificate_date_of_expiry"][0] == error diff --git a/api/organisations/serializers.py b/api/organisations/serializers.py index 6611befb1..461e4fcde 100644 --- a/api/organisations/serializers.py +++ b/api/organisations/serializers.py @@ -2,6 +2,7 @@ from django.db import transaction from django.utils import timezone +from dateutil.relativedelta import relativedelta from phonenumber_field.serializerfields import PhoneNumberField from rest_framework import serializers @@ -470,6 +471,14 @@ class Meta: def get_is_expired(self, instance): return timezone.now().date() > instance.expiry_date + def validate_expiry_date(self, value): + years_from_now = relativedelta(value, timezone.now().date()).years + if years_from_now > 5: + raise serializers.ValidationError("Expiry date is too far in the future") + elif value < timezone.now().date(): + raise serializers.ValidationError("Expiry date must be in the future") + return value + def create(self, validated_data): document_serializer = DocumentSerializer(data=validated_data["document"]) document_serializer.is_valid(raise_exception=True) diff --git a/api/organisations/tests/test_serializers.py b/api/organisations/tests/test_serializers.py new file mode 100644 index 000000000..1317ffbf1 --- /dev/null +++ b/api/organisations/tests/test_serializers.py @@ -0,0 +1,52 @@ +import unittest +import pytest + +from test_helpers.helpers import mocked_now + +from api.organisations import serializers +from api.organisations.enums import OrganisationDocumentType + + +@pytest.mark.parametrize( + "data,valid,error", + [ + ( + { + "expiry_date": "2000-01-01", + "document": {"name": "a-document", "s3_key": "a-document", "size": 476}, + "document_type": OrganisationDocumentType.REGISTERED_FIREARM_DEALER_CERTIFICATE, + "reference_code": "1234", + }, + False, + "Expiry date must be in the future", + ), + ( + { + "expiry_date": "2012-01-01", + "document": {"name": "a-document", "s3_key": "a-document", "size": 476}, + "document_type": OrganisationDocumentType.REGISTERED_FIREARM_DEALER_CERTIFICATE, + "reference_code": "1234", + }, + True, + None, + ), + ( + { + "expiry_date": "2016-01-01", + "document": {"name": "a-document", "s3_key": "a-document", "size": 476}, + "document_type": OrganisationDocumentType.REGISTERED_FIREARM_DEALER_CERTIFICATE, + "reference_code": "1234", + }, + False, + "Expiry date is too far in the future", + ), + ], +) +@unittest.mock.patch("django.utils.timezone.now", side_effect=mocked_now) +def test_document_on_organisation_serializer(mock_timezone, data, valid, error): + + serializer = serializers.DocumentOnOrganisationSerializer(data=data) + + assert serializer.is_valid() == valid + if not valid: + assert serializer.errors["expiry_date"][0] == error diff --git a/test_helpers/helpers.py b/test_helpers/helpers.py index 2b2986521..fad6735db 100644 --- a/test_helpers/helpers.py +++ b/test_helpers/helpers.py @@ -1,4 +1,5 @@ -from django.db import IntegrityError +from datetime import datetime +from django.utils import timezone from faker import Faker from api.core.constants import Roles @@ -9,6 +10,10 @@ faker = Faker() +def mocked_now(): + return datetime(2010, 1, 1, tzinfo=timezone.utc) + + def generate_key_value_pair(key, choices): """ Given a key from a list of choices, generate a key value pair From 3a4dfb91a9618b36a75453e169f93d9aafb61ab3 Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Fri, 11 Jun 2021 16:11:36 +0100 Subject: [PATCH 3/8] Flatten document errors --- api/goods/libraries/save_good.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/api/goods/libraries/save_good.py b/api/goods/libraries/save_good.py index 5d9611c53..e68257a82 100644 --- a/api/goods/libraries/save_good.py +++ b/api/goods/libraries/save_good.py @@ -9,11 +9,18 @@ def create_or_update_good(serializer, data, is_created): errors = serializer.errors pv_grading_errors = errors.pop("pv_grading_details", None) firearm_errors = errors.pop("firearm_details", None) - # The errors need to be flattened otherwise they will be contained within a 'pv_grading_details' dict + # The errors need to be flattened otherwise they will be contained within + # nested 'pv_grading_details', 'firearm_details' and 'document_on_organisation' dictionaries if firearm_errors: - flattened_errors = ( - {**errors, **firearm_errors, **pv_grading_errors} if pv_grading_errors else {**errors, **firearm_errors} - ) + document_on_organisation_errors = firearm_errors.pop("document_on_organisation", None) + flattened_errors = { + **errors, + **firearm_errors, + } + if pv_grading_errors: + flattened_errors = {**flattened_errors, **pv_grading_errors} + if document_on_organisation_errors: + flattened_errors = {**flattened_errors, **document_on_organisation_errors} if ( data["firearm_details"].get("rfd_status") is True and "is_covered_by_firearm_act_section_one_two_or_five" in flattened_errors @@ -26,11 +33,11 @@ def create_or_update_good(serializer, data, is_created): flattened_errors = {**errors, **pv_grading_errors} if pv_grading_errors else errors return JsonResponse(data={"errors": flattened_errors}, status=status.HTTP_400_BAD_REQUEST) + serializer.save() + if str_to_bool(data.get("validate_only")): return JsonResponse(data={"good": serializer.data}, status=status.HTTP_200_OK) - serializer.save() - return JsonResponse( data={"good": serializer.data}, status=status.HTTP_201_CREATED if is_created else status.HTTP_200_OK ) From 39ec409826dc7c1f4c17655842d12dc06b134b5d Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Fri, 11 Jun 2021 16:13:52 +0100 Subject: [PATCH 4/8] Validate is_registered_firearm_dealer and document_on_organisation --- api/goods/serializers.py | 47 +++++++++++++++++++++++++++----- api/organisations/serializers.py | 8 +++++- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/api/goods/serializers.py b/api/goods/serializers.py index bd6ba89d2..ea22d072a 100644 --- a/api/goods/serializers.py +++ b/api/goods/serializers.py @@ -31,6 +31,7 @@ from api.gov_users.serializers import GovUserSimpleSerializer from lite_content.lite_api import strings from api.organisations.models import Organisation +from api.organisations.serializers import DocumentOnOrganisationSerializer from api.queries.goods_query.models import GoodsQuery from api.staticdata.control_list_entries.serializers import ControlListEntrySerializer from api.staticdata.missing_document_reasons.enums import GoodMissingDocumentReasons @@ -137,6 +138,10 @@ class FirearmGoodDetailsSerializer(serializers.ModelSerializer): ) serial_numbers = serializers.ListField(child=serializers.CharField(allow_blank=True), required=False) + is_registered_firearm_dealer = serializers.BooleanField(allow_null=True, required=False) + + document_on_organisation = DocumentOnOrganisationSerializer(allow_null=True, required=False) + class Meta: model = FirearmGoodDetails fields = ( @@ -163,10 +168,27 @@ class Meta: "deactivation_standard_other", "number_of_items", "serial_numbers", + # non model fields below included for validation only + "is_registered_firearm_dealer", + "document_on_organisation", ) + def create(self, validated_data): + # These fields not in the model so remove before create + if "is_registered_firearm_dealer" in validated_data: + validated_data.pop("is_registered_firearm_dealer") + + if validated_data.get("document_on_organisation"): + validated_data["document_on_organisation"] = DocumentOnOrganisationSerializer.create( + DocumentOnOrganisationSerializer(context=self.context), + validated_data=validated_data["document_on_organisation"], + ) + validated_data.pop("document_on_organisation") + + return super().create(validated_data) + def validate(self, data): - validated_data = super(FirearmGoodDetailsSerializer, self).validate(data) + validated_data = super().validate(data) # Year of manufacture should be in the past and a valid year year_of_manufacture = validated_data.get("year_of_manufacture") @@ -207,6 +229,14 @@ def validate(self, data): {"is_sporting_shotgun": [get_sporting_shortgun_errormsg(validated_data.get("type"))]} ) + if ( + "is_registered_firearm_dealer" in validated_data + and validated_data.get("is_registered_firearm_dealer") is None + ): + raise serializers.ValidationError( + {"is_registered_firearm_dealer": ["Select yes if you are a registered firearms dealer"]} + ) + if validated_data.get("has_proof_mark") is False and validated_data.get("no_proof_mark_details") == "": raise serializers.ValidationError({"no_proof_mark_details": ["This field is required"]}) if validated_data.get("is_deactivated"): @@ -422,6 +452,13 @@ def __init__(self, *args, **kwargs): if "section_certificate_date_of_expiry" in firearm_details: firearm_details.pop("section_certificate_date_of_expiry") + # If user has answered No to registered firearms dealer question, don't validate RFD document upload + if ( + not str_to_bool(firearm_details.get("is_registered_firearm_dealer")) + and "document_on_organisation" in firearm_details + ): + firearm_details.pop("document_on_organisation") + if "has_identification_markings" in firearm_details: # Keep only the details relevant for the yes/no answer if str_to_bool(firearm_details.get("has_identification_markings")): @@ -480,8 +517,8 @@ def create(self, validated_data): ) if validated_data.get("firearm_details"): - validated_data["firearm_details"] = GoodCreateSerializer._create_firearm_details( - validated_data["firearm_details"] + validated_data["firearm_details"] = FirearmGoodDetailsSerializer.create( + FirearmGoodDetailsSerializer(context=self.context), validated_data=validated_data["firearm_details"] ) return super(GoodCreateSerializer, self).create(validated_data) @@ -593,10 +630,6 @@ def _delete_pv_grading_details(instance): instance.delete() return None - @staticmethod - def _create_firearm_details(firearm_details): - return FirearmGoodDetailsSerializer.create(FirearmGoodDetailsSerializer(), validated_data=firearm_details) - @staticmethod def _update_firearm_details(firearm_details, instance): return FirearmGoodDetailsSerializer.update( diff --git a/api/organisations/serializers.py b/api/organisations/serializers.py index 461e4fcde..f32be0e65 100644 --- a/api/organisations/serializers.py +++ b/api/organisations/serializers.py @@ -452,8 +452,12 @@ class Meta: class DocumentOnOrganisationSerializer(serializers.ModelSerializer): - document = DocumentSerializer() + document = DocumentSerializer(error_messages={"null": "Select certificate file to upload",},) is_expired = serializers.SerializerMethodField() + reference_code = serializers.CharField(error_messages={"blank": "Enter the certificate number",},) + expiry_date = serializers.DateField( + error_messages={"null": "Enter the certificate expiry date and include a day, month and year",}, + ) class Meta: model = DocumentOnOrganisation @@ -482,6 +486,8 @@ def validate_expiry_date(self, value): def create(self, validated_data): document_serializer = DocumentSerializer(data=validated_data["document"]) document_serializer.is_valid(raise_exception=True) + if self.context["validate_only"]: + return document = document_serializer.save() process_document(document) return super().create({**validated_data, "document": document, "organisation": self.context["organisation"]}) From 37a7d1619b2513d958575d4b633e80f9ce6291ac Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Fri, 11 Jun 2021 16:14:38 +0100 Subject: [PATCH 5/8] Pass validate_only to serializer --- api/goods/views.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/api/goods/views.py b/api/goods/views.py index 48459251a..f6f4cb309 100644 --- a/api/goods/views.py +++ b/api/goods/views.py @@ -47,7 +47,7 @@ from api.goodstype.serializers import ClcControlGoodTypeSerializer from lite_content.lite_api import strings from api.organisations.models import OrganisationDocumentType -from api.organisations.libraries.get_organisation import get_request_user_organisation_id +from api.organisations.libraries.get_organisation import get_request_user_organisation_id, get_request_user_organisation from api.queries.goods_query.models import GoodsQuery from api.staticdata.statuses.enums import CaseStatusEnum from api.users.models import ExporterNotification @@ -197,6 +197,7 @@ def get_paginated_response(self, data): def post(self, request, *args, **kwargs): """ Add a good to to an organisation. """ data = request.data + validate_only = request.data.get("validate_only", False) data["organisation"] = get_request_user_organisation_id(request) data["status"] = GoodStatus.DRAFT @@ -232,7 +233,9 @@ def post(self, request, *args, **kwargs): ): data["firearm_details"]["firearms_act_section"] = "firearms_act_section5" - serializer = GoodCreateSerializer(data=data) + serializer = GoodCreateSerializer( + data=data, context={"validate_only": validate_only, "organisation": get_request_user_organisation(request)} + ) return create_or_update_good(serializer, data, is_created=True) From fc45d56af9305bfe81667731b22835e70f9bd907 Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Tue, 15 Jun 2021 11:03:48 +0100 Subject: [PATCH 6/8] Move error flattening into separate function --- api/goods/libraries/save_good.py | 55 ++++++------ api/goods/tests/test_save_good.py | 134 ++++++++++++++++++++++++++++++ api/organisations/serializers.py | 4 +- 3 files changed, 164 insertions(+), 29 deletions(-) create mode 100644 api/goods/tests/test_save_good.py diff --git a/api/goods/libraries/save_good.py b/api/goods/libraries/save_good.py index e68257a82..0fce44675 100644 --- a/api/goods/libraries/save_good.py +++ b/api/goods/libraries/save_good.py @@ -4,40 +4,41 @@ from api.core.helpers import str_to_bool +def flatten_errors(errors, data): + pv_grading_errors = errors.pop("pv_grading_details", {}) + firearm_errors = errors.pop("firearm_details", {}) + # The errors need to be flattened otherwise they will be contained within + # nested 'pv_grading_details', 'firearm_details' and 'document_on_organisation' dictionaries + document_on_organisation_errors = firearm_errors.pop("document_on_organisation", {}) + + flattened_errors = { + **errors, + **firearm_errors, + **pv_grading_errors, + **document_on_organisation_errors, + } + if data.get("firearm_details"): + if ( + data["firearm_details"].get("rfd_status") is True + and "is_covered_by_firearm_act_section_one_two_or_five" in flattened_errors + ): + flattened_errors["is_covered_by_firearm_act_section_one_two_or_five"] = [ + "Select yes if the product is covered by section 5 of the Firearms Act 1968" + ] + + return flattened_errors + + def create_or_update_good(serializer, data, is_created): if not serializer.is_valid(): - errors = serializer.errors - pv_grading_errors = errors.pop("pv_grading_details", None) - firearm_errors = errors.pop("firearm_details", None) - # The errors need to be flattened otherwise they will be contained within - # nested 'pv_grading_details', 'firearm_details' and 'document_on_organisation' dictionaries - if firearm_errors: - document_on_organisation_errors = firearm_errors.pop("document_on_organisation", None) - flattened_errors = { - **errors, - **firearm_errors, - } - if pv_grading_errors: - flattened_errors = {**flattened_errors, **pv_grading_errors} - if document_on_organisation_errors: - flattened_errors = {**flattened_errors, **document_on_organisation_errors} - if ( - data["firearm_details"].get("rfd_status") is True - and "is_covered_by_firearm_act_section_one_two_or_five" in flattened_errors - ): - flattened_errors["is_covered_by_firearm_act_section_one_two_or_five"] = [ - "Select yes if the product is covered by section 5 of the Firearms Act 1968" - ] - - else: - flattened_errors = {**errors, **pv_grading_errors} if pv_grading_errors else errors + flattened_errors = flatten_errors(serializer.errors, data) return JsonResponse(data={"errors": flattened_errors}, status=status.HTTP_400_BAD_REQUEST) - serializer.save() - if str_to_bool(data.get("validate_only")): return JsonResponse(data={"good": serializer.data}, status=status.HTTP_200_OK) + serializer.save() + return JsonResponse( data={"good": serializer.data}, status=status.HTTP_201_CREATED if is_created else status.HTTP_200_OK ) diff --git a/api/goods/tests/test_save_good.py b/api/goods/tests/test_save_good.py new file mode 100644 index 000000000..5cf9a17e1 --- /dev/null +++ b/api/goods/tests/test_save_good.py @@ -0,0 +1,134 @@ +import json + +from rest_framework import serializers +from rest_framework import status + +from api.goods.libraries.save_good import flatten_errors, create_or_update_good + + +def test_flatten_errors(): + data = { + "pv_grading_details": {"example_field1": "Invalid value",}, + "firearm_details": { + "example_field2": "Invalid value", + "document_on_organisation": {"example_field3": "Invalid value",}, + }, + } + json = { + "pv_grading_details": {"example_field1": ["Example error 1"]}, + "firearm_details": { + "example_field2": ["Example error 2"], + "document_on_organisation": {"example_field3": ["Example error 3"],}, + }, + } + expected = { + "example_field1": ["Example error 1"], + "example_field2": ["Example error 2"], + "example_field3": ["Example error 3"], + } + assert flatten_errors(json, data) == expected + + +def test_flatten_errors_certificate_error(): + data = { + "pv_grading_details": {"example_field1": "Invalid value",}, + "firearm_details": { + "example_field2": "Invalid value", + "rfd_status": True, + "document_on_organisation": {"example_field3": "Invalid value",}, + }, + } + errors = { + "pv_grading_details": {"example_field1": ["Example error 1"]}, + "firearm_details": { + "example_field2": ["Example error 2"], + "is_covered_by_firearm_act_section_one_two_or_five": ["Error message"], + "document_on_organisation": {"example_field3": ["Example error 3"],}, + }, + } + expected = { + "example_field1": ["Example error 1"], + "example_field2": ["Example error 2"], + "example_field3": ["Example error 3"], + "is_covered_by_firearm_act_section_one_two_or_five": [ + "Select yes if the product is covered by section 5 of the Firearms Act 1968" + ], + } + assert flatten_errors(errors, data) == expected + + +class DummyDocumentOnOrganisationSerializer(serializers.Serializer): + document_field = serializers.CharField() + + +class DummyPVGradingDetailsSerializer(serializers.Serializer): + pv_grading_field = serializers.CharField() + + +class DummyFirearmDetailsSerializer(serializers.Serializer): + firearms_field = serializers.CharField() + document_on_organisation = DummyDocumentOnOrganisationSerializer() + + +class DummyGoodSerializer(serializers.Serializer): + some_field = serializers.CharField() + firearm_details = DummyFirearmDetailsSerializer() + pv_grading_details = DummyPVGradingDetailsSerializer() + + def save(self): + pass + + +def test_create_or_update_good_errors(): + data = {"some_field": None, "firearm_details": {"document_on_organisation": {},}, "pv_grading_details": {}} + serializer = DummyGoodSerializer(data=data) + expected = { + "errors": { + "some_field": ["This field may not be null."], + "firearms_field": ["This field is required."], + "pv_grading_field": ["This field is required."], + "document_field": ["This field is required."], + } + } + response = create_or_update_good(serializer, data, False) + assert json.loads(response.content.decode("utf-8")) == expected + assert response.status_code == 400 + + +def test_create_or_update_good_validate_only(): + data = { + "validate_only": True, + "some_field": "foo", + "firearm_details": {"firearms_field": "bar", "document_on_organisation": {"document_field": "dlfkj"},}, + "pv_grading_details": {"pv_grading_field": "baz"}, + } + serializer = DummyGoodSerializer(data=data) + expected = { + "good": { + "some_field": "foo", + "firearm_details": {"firearms_field": "bar", "document_on_organisation": {"document_field": "dlfkj"},}, + "pv_grading_details": {"pv_grading_field": "baz"}, + } + } + response = create_or_update_good(serializer, data, False) + assert json.loads(response.content.decode("utf-8")) == expected + assert response.status_code == 200 + + +def test_create_or_update_good_success(): + data = { + "some_field": "foo", + "firearm_details": {"firearms_field": "bar", "document_on_organisation": {"document_field": "dlfkj"},}, + "pv_grading_details": {"pv_grading_field": "baz"}, + } + serializer = DummyGoodSerializer(data=data) + expected = { + "good": { + "some_field": "foo", + "firearm_details": {"firearms_field": "bar", "document_on_organisation": {"document_field": "dlfkj"},}, + "pv_grading_details": {"pv_grading_field": "baz"}, + } + } + response = create_or_update_good(serializer, data, True) + assert json.loads(response.content.decode("utf-8")) == expected + assert response.status_code == 201 diff --git a/api/organisations/serializers.py b/api/organisations/serializers.py index f32be0e65..f4663d123 100644 --- a/api/organisations/serializers.py +++ b/api/organisations/serializers.py @@ -274,7 +274,7 @@ def validate_phone_number(self, value): @transaction.atomic def create(self, validated_data): - if self.context["validate_only"]: + if self.context.get("validate_only", False): return user_data = validated_data.pop("user") @@ -486,7 +486,7 @@ def validate_expiry_date(self, value): def create(self, validated_data): document_serializer = DocumentSerializer(data=validated_data["document"]) document_serializer.is_valid(raise_exception=True) - if self.context["validate_only"]: + if self.context.get("validate_only", False): return document = document_serializer.save() process_document(document) From a218b2e4c08951aade6ce5e7604368ec6c995939 Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Thu, 17 Jun 2021 09:28:45 +0100 Subject: [PATCH 7/8] Fix expiry date validation logic --- api/goods/tests/test_serializers.py | 9 +++++++++ api/organisations/serializers.py | 2 +- api/organisations/tests/test_serializers.py | 10 ++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/api/goods/tests/test_serializers.py b/api/goods/tests/test_serializers.py index b2a805c31..683f1c843 100644 --- a/api/goods/tests/test_serializers.py +++ b/api/goods/tests/test_serializers.py @@ -48,6 +48,15 @@ def test_pv_grading_details_serializer(self): False, "Expiry date is too far in the future", ), + ( + { + "section_certificate_date_of_expiry": "2015-01-02", + "section_certificate_number": "1", + "certificate_missing": False, + }, + False, + "Expiry date is too far in the future", + ), ], ) @unittest.mock.patch("django.utils.timezone.now", side_effect=mocked_now) diff --git a/api/organisations/serializers.py b/api/organisations/serializers.py index f4663d123..b1c1b3b00 100644 --- a/api/organisations/serializers.py +++ b/api/organisations/serializers.py @@ -477,7 +477,7 @@ def get_is_expired(self, instance): def validate_expiry_date(self, value): years_from_now = relativedelta(value, timezone.now().date()).years - if years_from_now > 5: + if years_from_now >= 5: raise serializers.ValidationError("Expiry date is too far in the future") elif value < timezone.now().date(): raise serializers.ValidationError("Expiry date must be in the future") diff --git a/api/organisations/tests/test_serializers.py b/api/organisations/tests/test_serializers.py index 1317ffbf1..72bce35bc 100644 --- a/api/organisations/tests/test_serializers.py +++ b/api/organisations/tests/test_serializers.py @@ -40,6 +40,16 @@ False, "Expiry date is too far in the future", ), + ( + { + "expiry_date": "2015-01-02", + "document": {"name": "a-document", "s3_key": "a-document", "size": 476}, + "document_type": OrganisationDocumentType.REGISTERED_FIREARM_DEALER_CERTIFICATE, + "reference_code": "1234", + }, + False, + "Expiry date is too far in the future", + ), ], ) @unittest.mock.patch("django.utils.timezone.now", side_effect=mocked_now) From 91859dd89a3908d85c1e5c9f2084e1f331da2a32 Mon Sep 17 00:00:00 2001 From: Edie Pearce Date: Thu, 17 Jun 2021 09:50:58 +0100 Subject: [PATCH 8/8] Fix expiry date logic in firearms serializer --- api/goods/helpers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/goods/helpers.py b/api/goods/helpers.py index 71ddf219c..cd6e27432 100644 --- a/api/goods/helpers.py +++ b/api/goods/helpers.py @@ -161,7 +161,7 @@ def validate_firearms_act_certificate_expiry_date(validated_data): difference_in_years = relativedelta(date_of_expiry, timezone.now().date()).years - if difference_in_years > 5: + if difference_in_years >= 5: errors["section_certificate_date_of_expiry"] = "Expiry date is too far in the future" if errors: