Skip to content

added validation for retention period for send files by email #4723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rparke wants to merge 1 commit into
base: main
Choose a base branch
from
Open

added validation for retention period for send files by email #4723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,3 +413,7 @@ class CacheKeys:

# Date and time
SECONDS_IN_1_MINUTE = 60

# Document Download
DOCUMENT_DOWNLOAD_RETENTION_UPPER_LIMIT_IN_WEEKS = 78
DOCUMENT_DOWNLOAD_RETENTION_LOWER_LIMIT_IN_WEEKS = 1
23 changes: 22 additions & 1 deletion app/schema_validation/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@
from notifications_utils.recipient_validation.errors import InvalidEmailError, InvalidPhoneError
from notifications_utils.recipient_validation.phone_number import PhoneNumber

from app.constants import (
DOCUMENT_DOWNLOAD_RETENTION_LOWER_LIMIT_IN_WEEKS,
DOCUMENT_DOWNLOAD_RETENTION_UPPER_LIMIT_IN_WEEKS,
)

format_checker = FormatChecker()


Expand Down Expand Up @@ -88,7 +93,12 @@ def validate_schema_retention_period(instance):
if isinstance(instance, str):
period = instance.strip().lower()
match = re.match(r"^(\d+) weeks?$", period)
if match and 1 <= int(match.group(1)) <= 78:
if (
match
and DOCUMENT_DOWNLOAD_RETENTION_LOWER_LIMIT_IN_WEEKS
<= int(match.group(1))
<= DOCUMENT_DOWNLOAD_RETENTION_UPPER_LIMIT_IN_WEEKS
):
return True

raise ValidationError(
Expand Down Expand Up @@ -126,6 +136,17 @@ def send_a_file_confirm_email_before_download(instance):
)


@format_checker.checks("send_file_via_ui_retention_period", raises=ValidationError)
def validate_send_file_via_ui_retention_period(instance):
if isinstance(instance, int):
return (
DOCUMENT_DOWNLOAD_RETENTION_LOWER_LIMIT_IN_WEEKS
<= instance
<= DOCUMENT_DOWNLOAD_RETENTION_UPPER_LIMIT_IN_WEEKS
)
raise ValidationError(f"Unsuported value for for retention period: {instance}. Supported values are 1 to 78")
Copy link
Contributor

@CrystalPea CrystalPea Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep it consistent with the message in validate_schema_retention_period():

Suggested change
raise ValidationError(f"Unsuported value for for retention period: {instance}. Supported values are 1 to 78")
raise ValidationError(f"Unsupported value for for retention period: {instance}. Supported periods are from 1 to 78 weeks.")

Also looks like that and this function have a lot in common - maybe we could refactor them to be one function?

Copy link
Contributor Author

@rparke rparke Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that these are validating in subtly different ways. Document download API expects a string of the form "<number of weeks> weeks", notifications-api just expects an int that represents a number of weeks. So we want the error message to clearly tell us what it expects (an int, not a string).

We could probably refactor those into the same method, but I wanted it to be explicitly exactly what we're validating in the schema "are we validating requests to v2 or are we validating requests from admin to be handled by the send files by email feature?". I'm happy to be persuaded that code maintainability trump that distinction though.

Copy link
Contributor

@CrystalPea CrystalPea Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense! Let's keep separate validation messages then. Maybe this one could be amended to:

Suggested change
raise ValidationError(f"Unsuported value for for retention period: {instance}. Supported values are 1 to 78")
raise ValidationError(f"Unsupported value for for retention period: {instance}. Supported values are integers from 1 to 78")

I would still refactor this:

(
    DOCUMENT_DOWNLOAD_RETENTION_LOWER_LIMIT_IN_WEEKS
    <= instance
    <= DOCUMENT_DOWNLOAD_RETENTION_UPPER_LIMIT_IN_WEEKS
)

into a helper method, like _is_valid_retention_period(number) - what do you think?



def validate(json_to_validate, schema):
validator = Draft7Validator(schema, format_checker=format_checker)
errors = list(validator.iter_errors(json_to_validate))
Expand Down
4 changes: 2 additions & 2 deletions app/template_email_files/template_email_files_schemas.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"filename": {"type": "string"},
"link_text": {"type": "string"},
"service": uuid,
"retention_period": {"type": "integer"},
"retention_period": {"type": "integer", "format": "send_file_via_ui_retention_period"},
"validate_users_email": {"type": "boolean"},
"created_by_id": uuid,
},
Expand All @@ -26,7 +26,7 @@
"filename": {"type": "string"},
"link_text": {"type": "string"},
"service": uuid,
"retention_period": {"type": "integer"},
"retention_period": {"type": "integer", "format": "send_file_via_ui_retention_period"},
"validate_users_email": {"type": "boolean"},
"template_version": {"type": "integer"},
"archived_by_id": uuid,
Expand Down
6 changes: 3 additions & 3 deletions tests/app/dao/test_template_email_files_dao.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def test_create_template_email_files_dao(sample_email_template, sample_service):
"id": "d963f496-b075-4e13-90ae-1f009feddbc6",
"filename": "example.pdf",
"link_text": "click this link!",
"retention_period": 90,
"retention_period": 45,
"validate_users_email": True,
"template_id": str(sample_email_template.id),
"template_version": int(sample_email_template.version),
Expand All @@ -36,7 +36,7 @@ def test_create_template_email_files_dao(sample_email_template, sample_service):
assert str(template_email_file.id) == "d963f496-b075-4e13-90ae-1f009feddbc6"
assert template_email_file.filename == "example.pdf"
assert template_email_file.link_text == "click this link!"
assert template_email_file.retention_period == 90
assert template_email_file.retention_period == 45
assert template_email_file.validate_users_email
assert template_email_file.version == 1
assert template_email_file.created_by_id == sample_service.users[0].id
Expand Down Expand Up @@ -138,7 +138,7 @@ def test_dao_get_template_email_files_by_template_id_historical(sample_email_tem
assert template_email_file_fetched.id == sample_template_email_file.id
assert template_email_file_fetched.filename == sample_template_email_file.filename
assert template_email_file_fetched.link_text == sample_template_email_file.link_text
assert template_email_file_fetched.retention_period == 90 # we want the initial retention period for historical
assert template_email_file_fetched.retention_period == 45 # we want the initial retention period for historical
assert template_email_file_fetched.validate_users_email == sample_template_email_file.validate_users_email
assert template_email_file_fetched.template_version == template_version_to_get
assert template_email_file_fetched.created_by_id == sample_template_email_file.created_by_id
Expand Down
2 changes: 1 addition & 1 deletion tests/app/db.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ def create_template_email_file(
created_by_id,
filename="example.pdf",
link_text="follow this link",
retention_period=90,
retention_period=45,
validate_users_email=True,
):
data = {
Expand Down
30 changes: 18 additions & 12 deletions tests/app/template_email_files/test_rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def test_create_template_email_file_happy_path(sample_service, sample_email_temp
data = {
"filename": "example.pdf",
"link_text": "click this link!",
"retention_period": 90,
"retention_period": 45,
"validate_users_email": True,
"created_by_id": str(sample_service.users[0].id),
}
Expand All @@ -27,7 +27,7 @@ def test_create_template_email_file_happy_path(sample_service, sample_email_temp

# test response contains created email file data
assert response["data"]["filename"] == "example.pdf"
assert response["data"]["retention_period"] == 90
assert response["data"]["retention_period"] == 45
assert response["data"]["link_text"] == "click this link!"
assert response["data"]["validate_users_email"]
assert response["data"]["template_id"] == str(sample_email_template.id)
Expand All @@ -37,7 +37,7 @@ def test_create_template_email_file_happy_path(sample_service, sample_email_temp
# test that email file gets persisted into the database
template_email_file = TemplateEmailFile.query.get(str(response["data"]["id"]))
assert template_email_file.filename == "example.pdf"
assert template_email_file.retention_period == 90
assert template_email_file.retention_period == 45
assert template_email_file.link_text == "click this link!"
assert template_email_file.validate_users_email
assert template_email_file.template_id == sample_email_template.id
Expand All @@ -53,7 +53,7 @@ def test_create_template_email_file_fails_if_template_not_email_type(
data = {
"filename": "example.pdf",
"link_text": "click this link!",
"retention_period": 90,
"retention_period": 45,
"validate_users_email": True,
"created_by_id": str(sample_service.users[0].id),
}
Expand All @@ -73,7 +73,7 @@ def test_create_template_email_file_fails_if_template_does_not_exist(sample_serv
data = {
"filename": "example.pdf",
"link_text": "click this link!",
"retention_period": 90,
"retention_period": 45,
"validate_users_email": True,
"created_by_id": str(sample_service.users[0].id),
}
Expand All @@ -95,7 +95,7 @@ def test_create_template_email_file_fails_if_template_already_has_file_with_same
data = {
"filename": filename,
"link_text": "click this link!",
"retention_period": 90,
"retention_period": 45,
"validate_users_email": True,
"created_by_id": str(sample_service.users[0].id),
}
Expand Down Expand Up @@ -154,14 +154,20 @@ def test_create_template_email_file_creates_file_with_latest_template_version(
"retention_period": "not an integer",
"validate_users_email": True,
},
[{"error": "ValidationError", "message": "retention_period not an integer is not of type integer"}],
[
{"error": "ValidationError", "message": "retention_period not an integer is not of type integer"},
{
"error": "ValidationError",
"message": "retention_period Unsuported value for for retention period: not an integer. Supported values are 1 to 78", # noqa: E501
},
],
),
(
{
"id": "d963f496-b075-4e13-90ae-1f009feddbc6",
"filename": "example.pdf",
"link_text": "click this link!",
"retention_period": 90,
"retention_period": 45,
"validate_users_email": "this is a string",
},
[{"error": "ValidationError", "message": "validate_users_email this is a string is not of type boolean"}],
Expand Down Expand Up @@ -195,15 +201,15 @@ def test_create_template_email_file_raises_exception_for_invalid_data(
{
"filename": "example.pdf",
"link_text": "example.pdf",
"retention_period": 90,
"retention_period": 45,
"validate_users_email": True,
},
),
(
{
"filename": "example.pdf",
"link_text": "example.pdf",
"retention_period": 90,
"retention_period": 45,
"validate_users_email": True,
},
{
Expand Down Expand Up @@ -309,7 +315,7 @@ def test_update_template_email_file(
{"id": str(sample_template_email_file.id), "version": 1}
)
assert template_email_file_history_version_one.link_text == "follow this link"
assert template_email_file_history_version_one.retention_period == 90
assert template_email_file_history_version_one.retention_period == 45
assert template_email_file_history_version_one.validate_users_email is True
assert template_email_file_history_version_one.template_version == 2
assert template_email_file_history_version_one.version == 1
Expand Down Expand Up @@ -358,7 +364,7 @@ def test_archive_template_email_file(client, sample_service, sample_email_templa
data = {
"filename": "example.pdf",
"link_text": "click this link!",
"retention_period": 90,
"retention_period": 45,
"validate_users_email": True,
"template_id": str(sample_email_template.id),
"created_by_id": str(sample_service.users[0].id),
Expand Down