Skip to content

Commit

Permalink
Revert PR #278
Browse files Browse the repository at this point in the history
  • Loading branch information
whabanks committed Apr 26, 2024
1 parent cd287cf commit 3d1ee7b
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 89 deletions.
9 changes: 2 additions & 7 deletions notifications_utils/columns.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,7 @@ def __init__(
template.values = row_dict
self.message_too_long = template.is_message_too_long()

super().__init__(
OrderedDict(
(key, Cell(key, value, error_fn, self.placeholders, len(template.content) if template else None))
for key, value in row_dict.items()
)
)
super().__init__(OrderedDict((key, Cell(key, value, error_fn, self.placeholders)) for key, value in row_dict.items()))

def __getitem__(self, key):
return super().__getitem__(key) or Cell()
Expand Down Expand Up @@ -126,7 +121,7 @@ def recipient_and_personalisation(self):
class Cell:
missing_field_error = "Missing"

def __init__(self, key=None, value=None, error_fn=None, placeholders=None, template_content_length=None):
def __init__(self, key=None, value=None, error_fn=None, placeholders=None):
self.data = value
self.error = error_fn(key, value) if error_fn else None
self.ignore = Columns.make_key(key) not in (placeholders or [])
Expand Down
40 changes: 2 additions & 38 deletions notifications_utils/recipients.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,7 @@
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
Expand Down Expand Up @@ -276,23 +274,6 @@ 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)
Expand Down Expand Up @@ -378,9 +359,7 @@ def _get_error_for_field(self, key, value): # noqa: C901
if self.is_optional_address_column(key):
return

formatted_key = Columns.make_key(key)

if formatted_key in self.recipient_column_headers_as_column_keys:
if Columns.make_key(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
Expand All @@ -391,19 +370,12 @@ def _get_error_for_field(self, key, value): # noqa: C901
except (InvalidEmailError, InvalidPhoneError, InvalidAddressError) as error:
return str(error)

if formatted_key not in self.placeholders_as_column_keys:
if Columns.make_key(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):
Expand Down Expand Up @@ -579,14 +551,6 @@ 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):
Expand Down
59 changes: 15 additions & 44 deletions tests/test_recipient_csv.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
from math import floor
import pytest
import itertools
import unicodedata
Expand Down Expand Up @@ -746,57 +745,29 @@ def test_recipient_safelist(file_contents, template_type, safelist, count_of_row
assert recipients.allowed_to_send_to


@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):
def test_detects_rows_which_result_in_overly_long_messages():
template = SMSMessageTemplate(
{"content": template_content, "template_type": "sms"},
{"content": "((placeholder))", "template_type": "sms"},
sender=None,
prefix=None,
)
recipients = RecipientCSV(
csv,
"""
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),
),
template_type=template.template_type,
template=template,
)
assert _index_rows(recipients.rows_with_errors) == error_rows
assert _index_rows(recipients.rows_with_message_too_long) == error_rows
assert _index_rows(recipients.rows_with_errors) == {3}
assert _index_rows(recipients.rows_with_message_too_long) == {3}
assert recipients.has_errors


Expand Down

0 comments on commit 3d1ee7b

Please sign in to comment.