diff --git a/.github/actions/waffles/requirements.txt b/.github/actions/waffles/requirements.txt index 093ca332..bc004edc 100644 --- a/.github/actions/waffles/requirements.txt +++ b/.github/actions/waffles/requirements.txt @@ -1,4 +1,4 @@ docopt==0.6.2 Flask==2.3.3 markupsafe==2.1.5 -git+https://github.com/cds-snc/notifier-utils.git@52.2.3#egg=notifications-utils +git+https://github.com/cds-snc/notifier-utils.git@52.2.4#egg=notifications-utils diff --git a/notifications_utils/columns.py b/notifications_utils/columns.py index 5f42dd35..1820c33f 100644 --- a/notifications_utils/columns.py +++ b/notifications_utils/columns.py @@ -59,6 +59,9 @@ def __init__( else ["phone number", "numéro de téléphone", "to"] ) + # This won't mark a row as too long in all cases. A message can be too long if + # placeholder content is added by a user that exceeds the limit when added to + # the template's content. if template: template.values = row_dict self.message_too_long = template.is_message_too_long() @@ -139,4 +142,9 @@ def __eq__(self, other): @property def recipient_error(self): + # TODO: This is a bandaid solution. We need to establish why we are calling this Cell property on + # Cells that do not represent a recipient value. + if self.error is not None and "Some messages may be too long due to custom content." in self.error: + return False + return self.error not in {None, self.missing_field_error} diff --git a/notifications_utils/recipients.py b/notifications_utils/recipients.py index 958537a1..b59c6a21 100644 --- a/notifications_utils/recipients.py +++ b/notifications_utils/recipients.py @@ -10,7 +10,9 @@ from collections import OrderedDict, namedtuple from ordered_set import OrderedSet from typing import Callable, Dict, List +from notifications_utils import SMS_CHAR_COUNT_LIMIT from flask import current_app +from notifications_utils.sanitise_text import SanitiseSMS from . import EMAIL_REGEX_PATTERN, hostname_part, tld_part from notifications_utils.formatters import strip_and_remove_obscure_whitespace, strip_whitespace from notifications_utils.template import SMSMessageTemplate, Template @@ -274,6 +276,23 @@ 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): + """ + Checks if the length of all variable, plus the length of the template content, + exceeds the SMS limit of 612 characters. Counts non-GSM characters as 2. + """ + if self.rows_as_list: + for row in self.rows_as_list: + if row.personalisation and self.template: + variable_length = 0 + for variable in row.personalisation.as_dict_with_keys(self.template.placeholders).values(): + 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 > SMS_CHAR_COUNT_LIMIT: + yield row + @property def initial_rows_with_errors(self): return islice(self.rows_with_errors, self.max_errors_shown) @@ -359,7 +378,9 @@ def _get_error_for_field(self, key, value): # noqa: C901 if self.is_optional_address_column(key): return - if Columns.make_key(key) in self.recipient_column_headers_as_column_keys: + formatted_key = Columns.make_key(key) + + if formatted_key in self.recipient_column_headers_as_column_keys: if value in [None, ""] or isinstance(value, list): if self.duplicate_recipient_column_headers: return None @@ -370,12 +391,19 @@ def _get_error_for_field(self, key, value): # noqa: C901 except (InvalidEmailError, InvalidPhoneError, InvalidAddressError) as error: return str(error) - if Columns.make_key(key) not in self.placeholders_as_column_keys: + if formatted_key not in self.placeholders_as_column_keys: return if value in [None, ""]: return Cell.missing_field_error + if self.template: + if formatted_key in self.placeholders_as_column_keys and self.template.template_type == "sms": + try: + validate_sms_message_length(value, self.template.content) + except ValueError as error: + return str(error) + class InvalidEmailError(Exception): def __init__(self, message=None): @@ -551,6 +579,14 @@ def validate_recipient(recipient, template_type: str, column=None, international return validators[template_type](recipient, column) +def validate_sms_message_length(variable_content, template_content_length): + variable_length = sum(1 if char in SanitiseSMS.ALLOWED_CHARACTERS else 2 for char in variable_content) + + if variable_length + len(template_content_length) > SMS_CHAR_COUNT_LIMIT: + raise ValueError(f"Maximum {SMS_CHAR_COUNT_LIMIT} characters. Some messages may be too long due to custom content.") + return + + @lru_cache(maxsize=32, typed=False) def format_recipient(recipient): if not isinstance(recipient, str): diff --git a/pyproject.toml b/pyproject.toml index af0d9e33..2231b4a0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,7 @@ include = '(notifications_utils|tests)/.*\.pyi?$' [tool.poetry] name = "notifications-utils" -version = "52.2.3" +version = "52.2.4" description = "Shared python code for Notification - Provides logging utils etc." authors = ["Canadian Digital Service"] license = "MIT license" diff --git a/tests/test_recipient_csv.py b/tests/test_recipient_csv.py index d9ffc306..bdc0a5fb 100644 --- a/tests/test_recipient_csv.py +++ b/tests/test_recipient_csv.py @@ -1,3 +1,4 @@ +from math import floor import pytest import itertools import unicodedata @@ -745,29 +746,57 @@ def test_recipient_safelist(file_contents, template_type, safelist, count_of_row assert recipients.allowed_to_send_to -def test_detects_rows_which_result_in_overly_long_messages(): +@pytest.mark.parametrize( + "template_content, csv, error_rows", + [ + ( + "((placeholder))", + f""" + phone number,placeholder + 6502532222,1 + 6502532222,{"a" * (SMS_CHAR_COUNT_LIMIT - 1)} + 6502532223,{"a" * SMS_CHAR_COUNT_LIMIT} + 6502532224,{"a" * (SMS_CHAR_COUNT_LIMIT + 1)} + """, + {3}, + ), + ( # Placeholder + content length should not exceed SMS_CHAR_COUNT_LIMIT. 72 = content length - placeholder text + "((placeholder)) This is template content. Believe it or not there are character limits.", + f""" + phone number,placeholder + 6502532222,1 + 6502532222,{'a' * (SMS_CHAR_COUNT_LIMIT + 1)} + 6502532222,{'a'* (SMS_CHAR_COUNT_LIMIT - 72)} + 6502532222,{'a' * (SMS_CHAR_COUNT_LIMIT - 73) } + """, + {1}, + ), + ( + "((placeholder1)) This is template content.((placeholder2)) Believe it or not there are character limits.", + f""" + phone number,placeholder1,placeholder2 + 6502532222,1 + 6502532222,{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2))},{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2) + 1)} + 6502532222,{'a'* (floor(SMS_CHAR_COUNT_LIMIT / 2))},{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2) - 72)} + 6502532222,{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2)) },{'a' * (floor(SMS_CHAR_COUNT_LIMIT / 2) - 73)} + """, + {1}, + ), + ], +) +def test_detects_rows_which_result_in_overly_long_messages(template_content, csv, error_rows): template = SMSMessageTemplate( - {"content": "((placeholder))", "template_type": "sms"}, + {"content": template_content, "template_type": "sms"}, sender=None, prefix=None, ) recipients = RecipientCSV( - """ - phone number,placeholder - 6502532222,1 - 6502532222,{one_under} - 6502532223,{exactly} - 6502532224,{one_over} - """.format( - one_under="a" * (SMS_CHAR_COUNT_LIMIT - 1), - exactly="a" * SMS_CHAR_COUNT_LIMIT, - one_over="a" * (SMS_CHAR_COUNT_LIMIT + 1), - ), + csv, template_type=template.template_type, template=template, ) - assert _index_rows(recipients.rows_with_errors) == {3} - assert _index_rows(recipients.rows_with_message_too_long) == {3} + assert _index_rows(recipients.rows_with_errors) == error_rows + assert _index_rows(recipients.rows_with_message_too_long) == error_rows assert recipients.has_errors