Validate variables in CSV do not exceed daily limit #278
Conversation
- Added has_message_too_long property to the Row class - Added message_to_long variable to the Cell class with accompanying property content_length_error to identify a cell with the aformentioned error - Added a check to the RecipientCSV class that checks if a variable in a CSV exceeds the message limit of 612 when added to the template content
- Raise an execption when length is exceeded - Bubble exception message up to the UI for rendering
notifications_utils/recipients.py
Outdated
| if variable: | ||
| variable_length += sum(1 if char in SanitiseSMS.ALLOWED_CHARACTERS else 2 for char in str(variable)) | ||
| total_length = variable_length + (len(self.template.content) if self.template else 0) | ||
| if total_length > 612: |
There was a problem hiding this comment.
The content size might change again in the future if we restrict to even less than 4 fragments. Would it be easy to make this a variable to be configured?
There was a problem hiding this comment.
I see we have the SMS_CHAR_COUNT_LIMIT constant, is that something we could leverage here?
There was a problem hiding this comment.
Nice catch, we should use that here for sure.
Co-authored-by: Jimmy Royer <jimleroyer@gmail.com>
notifications_utils/recipients.py
Outdated
| rows_too_long = [] | ||
| if self.rows_as_list: | ||
| for row in self.rows_as_list: | ||
| if row.personalisation and self.template and self.template.placeholders: |
There was a problem hiding this comment.
why do we need to check for all 3 vs row.personalisation?
There was a problem hiding this comment.
row.personalisation includes the variable for the phone number from the CSV, causing those characters to be counted toward the limit. Calling as_dict_with_keys(template.placeholders) ensures we're only ever counting the variables input by the user.
We could probably get away without dropping the placeholder check. In some areas of the code, template is not always passed in to new instances of RecipientCSV.
notifications_utils/recipients.py
Outdated
| total_length = variable_length + (len(self.template.content) if self.template else 0) | ||
| if total_length > SMS_CHAR_COUNT_LIMIT: | ||
| rows_too_long.append(row) | ||
| return (row for row in rows_too_long) |
There was a problem hiding this comment.
The intention is to return a generator rather than a list to remain consistent with the other properties that return rows meeting certain criteria. E.g. rows_with_missing_data.
That said this should be refactored to yield the rows instead of storing them, as that's the whole point of using generators.🤦🏻♂️
There was a problem hiding this comment.
Updated, let me know if this makes more sense!
| @@ -276,6 +276,25 @@ def rows_with_missing_data(self): | |||
| def rows_with_message_too_long(self): | |||
| return self._filter_rows("message_too_long") | |||
|
|
|||
| @property | |||
| def rows_with_combined_variable_content_too_long(self): | |||
There was a problem hiding this comment.
where is this function being called?
There was a problem hiding this comment.
- Updated rows_with_combined_variable_content_too_long make use of generators properly - Removed None check
Summary | Résumé
This PR adds validation to the
RecipientCSVclass to ensure that variables inserted do not exceed the 612 character limit, when sending SMS. A new property,rows_with_combined_variable_content_too_long, was added to check that the total message length does not exceed 612 when both the variable content and template content is combined.