Make some models require all keys in their underlying dictionaries

app/models/__init__.py

@@ -13,7 +13,7 @@ class JSONModelMeta(SerialisedModelMeta, ABCMeta):
 
 
 @total_ordering
-class JSONModel(SerialisedModel, ABC, metaclass=JSONModelMeta):
+class StrictJSONModel(SerialisedModel, ABC, metaclass=JSONModelMeta):
     @property
     @abstractmethod
     def __sort_attribute__(self):
@@ -42,6 +42,8 @@ def __eq__(self, other):
     def __hash__(self):
         return hash(self.id)
 
+
+class JSONModel(StrictJSONModel):
     def __init__(self, _dict):
         # in the case of a bad request _dict may be `None`
         self._dict = _dict or {}

app/models/api_key.py

@@ -3,11 +3,11 @@
 
 from flask import abort
 
-from app.models import JSONModel, ModelList
+from app.models import ModelList, StrictJSONModel
 from app.notify_client.api_key_api_client import api_key_api_client
 
 
-class APIKey(JSONModel):
+class APIKey(StrictJSONModel):
     created_at: datetime
     created_by: Any
     expiry_date: datetime

app/models/contact_list.py

@@ -8,7 +8,7 @@
 from notifications_utils.recipients import RecipientCSV
 from werkzeug.utils import cached_property
 
-from app.models import JSONModel, ModelList
+from app.models import ModelList, StrictJSONModel
 from app.models.job import PaginatedJobsAndScheduledJobs
 from app.notify_client.contact_list_api_client import contact_list_api_client
 from app.s3_client.s3_csv_client import (
@@ -20,7 +20,7 @@
 from app.utils.templates import get_sample_template
 
 
-class ContactList(JSONModel):
+class ContactList(StrictJSONModel):
     id: Any
     created_at: datetime
     created_by: Any

app/models/letter_rates.py

@@ -1,11 +1,11 @@
 from datetime import datetime
 from typing import Any
 
-from app.models import JSONModel, ModelList
+from app.models import ModelList, StrictJSONModel
 from app.notify_client.letter_rate_api_client import letter_rate_api_client
 
 
-class LetterRate(JSONModel):
+class LetterRate(StrictJSONModel):
     sheet_count: int
     rate: float
     post_class: Any

app/models/report_request.py

@@ -5,11 +5,11 @@
 from notifications_utils.s3 import s3download
 
 from app import report_request_api_client
-from app.models import JSONModel
+from app.models import StrictJSONModel
 from app.s3_client import check_s3_object_exists
 
 
-class ReportRequest(JSONModel):
+class ReportRequest(StrictJSONModel):
     id: Any
     user_id: Any
     service_id: Any

app/models/service.py

@@ -13,7 +13,7 @@
     SIGN_IN_METHOD_TEXT,
     SIGN_IN_METHOD_TEXT_OR_EMAIL,
 )
-from app.models import JSONModel
+from app.models import JSONModel, StrictJSONModel
 from app.models.api_key import APIKeys
 from app.models.branding import EmailBranding, LetterBranding
 from app.models.contact_list import ContactLists
@@ -707,7 +707,7 @@ class Services(SerialisedModelCollection):
     model = Service
 
 
-class ServiceJoinRequest(JSONModel):
+class ServiceJoinRequest(StrictJSONModel):
     id: Any
     requester: Any
     service_id: Any
@@ -717,8 +717,6 @@ class ServiceJoinRequest(JSONModel):
     reason: str
     status: str
     contacted_service_users: list[str]
-    requested_service: Any
-    permissions: list[str]
 
     __sort_attribute__ = "id"
 

app/models/sms_rate.py

@@ -1,11 +1,11 @@
 from datetime import datetime
 
 from app.formatters import format_pennies_as_currency
-from app.models import JSONModel
+from app.models import StrictJSONModel
 from app.notify_client.sms_rate_client import sms_rate_api_client
 
 
-class SMSRate(JSONModel):
+class SMSRate(StrictJSONModel):
     rate: float
     valid_from: datetime
 

app/models/template_email_file.py

@@ -5,15 +5,15 @@
 from notifications_utils.base64_uuid import uuid_to_base64
 from notifications_utils.serialised_model import SerialisedModelCollection
 
-from app.models import JSONModel
+from app.models import StrictJSONModel
 from app.s3_client.s3_template_email_file_upload_client import upload_template_email_file_to_s3
 
 
 def _get_file_location(file_id: uuid, service_id: uuid) -> str:
     return f"{service_id}/{file_id}"
 
 
-class TemplateEmailFile(JSONModel):
+class TemplateEmailFile(StrictJSONModel):
     id: Any
     service_id: Any
     template_id: Any

app/models/unsubscribe_requests_report.py

@@ -4,12 +4,12 @@
 from flask import abort
 
 from app.formatters import format_date_human, format_time, get_human_day
-from app.models import JSONModel, ModelList
+from app.models import ModelList, StrictJSONModel
 from app.notify_client.service_api_client import service_api_client
 from app.utils.time import to_utc_string
 
 
-class UnsubscribeRequestsReport(JSONModel):
+class UnsubscribeRequestsReport(StrictJSONModel):
     service_id: Any
     count: int
     batch_id: Any

app/utils/templates.py

@@ -17,7 +17,7 @@
 from ordered_set import OrderedSet
 
 from app.extensions import redis_client
-from app.models import JSONModel
+from app.models import StrictJSONModel
 from app.models.template_email_file import TemplateEmailFiles
 from app.notify_client import cache
 
@@ -317,7 +317,7 @@ def placeholders(self):
         return OrderedSet([placeholder for placeholder in self.all_placeholders if placeholder not in self.filenames])
 
 
-class LetterAttachment(JSONModel):
+class LetterAttachment(StrictJSONModel):
     id: Any
     original_filename: Any
     page_count: Any

tests/__init__.py

@@ -382,8 +382,15 @@ def template_version_json(service_id, id_, created_by, version=1, created_at=Non
     return template
 
 
-def api_key_json(id_, name, expiry_date=None, key_type="normal"):
-    return {"id": id_, "name": name, "expiry_date": expiry_date, "key_type": key_type}
+def api_key_json(id_, name, expiry_date=None, created_at=None, created_by=None, key_type="normal"):
+    return {
+        "id": id_,
+        "name": name,
+        "expiry_date": expiry_date,
+        "key_type": key_type,
+        "created_at": created_at,
+        "created_by": created_by,
+    }
 
 
 def invite_json(

tests/app/main/forms/test_create_key_form.py

@@ -1,8 +1,11 @@
+from uuid import uuid4
+
 import pytest
 from werkzeug.datastructures import MultiDict
 
 from app.main.forms import CreateKeyForm
 from app.models.api_key import APIKeys
+from tests import api_key_json
 
 
 @pytest.mark.parametrize(
@@ -22,14 +25,16 @@ def test_return_validation_error_when_key_name_exists(
         "app.models.api_key.api_key_api_client.get_api_keys",
         return_value={
             "apiKeys": [
-                {
-                    "name": "some key",
-                    "expiry_date": expiry_date,
-                },
-                {
-                    "name": "another key",
-                    "expiry_date": None,
-                },
+                api_key_json(
+                    id_=str(uuid4()),
+                    name="some key",
+                    expiry_date=expiry_date,
+                ),
+                api_key_json(
+                    id_=str(uuid4()),
+                    name="another key",
+                    expiry_date=None,
+                ),
             ]
         },
     )

tests/app/main/views/test_send.py

@@ -1570,12 +1570,14 @@ def test_send_one_off_has_correct_page_title(
                     "retention_period": 26,
                     "id": str(uuid.UUID(int=1, version=4)),
                     "link_text": None,
+                    "validate_users_email": True,
                 },
                 {
                     "filename": "picture.png",
                     "retention_period": 90,
                     "id": str(uuid.UUID(int=2, version=4)),
                     "link_text": None,
+                    "validate_users_email": True,
                 },
             ],
             "((one)) ((example.pdf)) ((two)) ((picture.png))",

tests/app/main/views/test_templates.py

@@ -4737,6 +4737,7 @@ def test_letter_attachment_preview_image_shows_overlay_when_content_outside_prin
                     "retention_period": 26,
                     "id": str(uuid.UUID(int=1, version=4)),
                     "link_text": None,
+                    "validate_users_email": True,
                 }
             ],
             "Manage files",
@@ -4752,12 +4753,14 @@ def test_letter_attachment_preview_image_shows_overlay_when_content_outside_prin
                     "retention_period": 26,
                     "id": str(uuid.UUID(int=1, version=4)),
                     "link_text": None,
+                    "validate_users_email": True,
                 },
                 {
                     "filename": "picture.png",
                     "retention_period": 90,
                     "id": str(uuid.UUID(int=2, version=4)),
                     "link_text": None,
+                    "validate_users_email": True,
                 },
             ],
             "Manage files",

tests/app/models/test_base_model.py

@@ -1,6 +1,6 @@
 import pytest
 
-from app.models import JSONModel
+from app.models import JSONModel, StrictJSONModel
 
 
 def test_looks_up_from_dict():
@@ -58,6 +58,17 @@ class Custom(JSONModel):
     assert str(e.value) == "'Custom' object has no attribute 'foo'"
 
 
+def test_strict_model_raises_keyerror_if_item_missing_from_dict_on_instantiation():
+    class Custom(StrictJSONModel):
+        foo: str
+        __sort_attribute__ = "foo"
+
+    with pytest.raises(KeyError) as e:
+        Custom({})
+
+    assert str(e.value) == "'foo'"
+
+
 @pytest.mark.parametrize(
     "json_response",
     (

tests/app/models/test_contact_list.py

@@ -1,9 +1,10 @@
 from app.models.contact_list import ContactList
 from app.models.job import PaginatedJobs
+from tests import contact_list_json
 
 
 def test_get_jobs(mock_get_jobs):
-    contact_list = ContactList({"id": "a", "service_id": "b"})
+    contact_list = ContactList(contact_list_json(id_="a", service_id="b"))
     assert isinstance(contact_list.get_jobs(page=123), PaginatedJobs)
     # mock_get_jobs mocks the underlying API client method, not
     # contact_list.get_jobs

tests/app/utils/test_templates.py

@@ -651,7 +651,7 @@ def test_letter_image_template_marks_first_page_of_attachment(mocker, fake_uuid)
                     "content": "Content",
                     "subject": "Subject",
                     "template_type": "letter",
-                    "letter_attachment": {"id": fake_uuid, "page_count": 3},
+                    "letter_attachment": {"id": fake_uuid, "page_count": 3, "original_filename": "example.pdf"},
                 },
                 image_url="http://example.com/endpoint.png",
             )
@@ -1219,6 +1219,7 @@ def test_TemplateChange_placeholders_removed(old_template, new_template, placeho
                             "retention_period": 26,
                             "id": str(UUID(int=1, version=4)),
                             "link_text": None,
+                            "validate_users_email": True,
                         }
                     ],
                 }
@@ -1235,6 +1236,7 @@ def test_TemplateChange_placeholders_removed(old_template, new_template, placeho
                             "retention_period": 26,
                             "id": str(UUID(int=1, version=4)),
                             "link_text": None,
+                            "validate_users_email": True,
                         }
                     ],
                 }
@@ -1256,6 +1258,7 @@ def test_TemplateChange_placeholders_removed(old_template, new_template, placeho
                             "retention_period": 26,
                             "id": str(UUID(int=1, version=4)),
                             "link_text": None,
+                            "validate_users_email": True,
                         }
                     ],
                 }
@@ -1272,6 +1275,7 @@ def test_TemplateChange_placeholders_removed(old_template, new_template, placeho
                             "retention_period": 26,
                             "id": str(UUID(int=1, version=4)),
                             "link_text": None,
+                            "validate_users_email": True,
                         }
                     ],
                 }
@@ -1293,12 +1297,14 @@ def test_TemplateChange_placeholders_removed(old_template, new_template, placeho
                             "retention_period": 26,
                             "id": str(UUID(int=1, version=4)),
                             "link_text": None,
+                            "validate_users_email": True,
                         },
                         {
                             "filename": "3.pdf",
                             "retention_period": 26,
                             "id": str(UUID(int=2, version=4)),
                             "link_text": None,
+                            "validate_users_email": True,
                         },
                     ],
                 }
@@ -1315,12 +1321,14 @@ def test_TemplateChange_placeholders_removed(old_template, new_template, placeho
                             "retention_period": 26,
                             "id": str(UUID(int=1, version=4)),
                             "link_text": None,
+                            "validate_users_email": True,
                         },
                         {
                             "filename": "3.pdf",
                             "retention_period": 26,
                             "id": str(UUID(int=2, version=4)),
                             "link_text": None,
+                            "validate_users_email": True,
                         },
                     ],
                 }
@@ -1341,7 +1349,15 @@ def test_TemplateChange_email_files_removed(
 
 @pytest.mark.parametrize("service_has_api_keys", (True, False))
 def test_TemplateChange_email_files_and_placeholders_removed(service_has_api_keys, fake_uuid):
-    email_file_data = {"filename": "2.pdf", "retention_period": 26, "id": fake_uuid, "link_text": None}
+    email_file_data = {
+        "filename": "2.pdf",
+        "retention_period": 26,
+        "id": fake_uuid,
+        "link_text": None,
+        "validate_users_email": True,
+        "service_id": fake_uuid,
+        "template_id": fake_uuid,
+    }
     old_template = EmailPreviewTemplate(
         {
             "id": fake_uuid,