added validation for retention period for send files by email

[rparke]

document-download-api already validates at the point of sending, but we should try and catch any errors at the point of file creation too.

[CrystalPea]

Nice one, I left a comment 🙌🏼

[CrystalPea]

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?

[rparke]

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.

[CrystalPea]

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?