From 88379c9e465d30b3d2ea03c209bb77d5a900a049 Mon Sep 17 00:00:00 2001
From: Kenneth Kehl <@kkehl@flexion.us>
Date: Fri, 5 Jan 2024 10:35:14 -0800
Subject: [PATCH 1/3] final

---
 app/aws/s3.py                                | 45 ++++++++++++++++++++
 app/celery/provider_tasks.py                 |  4 +-
 app/delivery/send_to_providers.py            | 25 +++++++++--
 app/user/rest.py                             |  4 ++
 migrations/alembic.ini                       |  6 +--
 poetry.lock                                  | 26 ++++++-----
 pyproject.toml                               |  1 +
 tests/app/aws/test_s3.py                     | 30 ++++++++++++-
 tests/app/delivery/test_send_to_providers.py | 27 ++++++++++++
 9 files changed, 148 insertions(+), 20 deletions(-)

diff --git a/app/aws/s3.py b/app/aws/s3.py
index 5fa468a4a..c56c4e3a0 100644
--- a/app/aws/s3.py
+++ b/app/aws/s3.py
@@ -1,5 +1,6 @@
 import botocore
 from boto3 import Session
+from expiringdict import ExpiringDict
 from flask import current_app
 
 from app.clients import AWS_CLIENT_CONFIG
@@ -7,6 +8,9 @@ from app.clients import AWS_CLIENT_CONFIG
 FILE_LOCATION_STRUCTURE = "service-{}-notify/{}.csv"
 
 
+JOBS = ExpiringDict(max_len=100, max_age_seconds=3600)
+
+
 def get_s3_file(bucket_name, file_location, access_key, secret_key, region):
     s3_file = get_s3_object(bucket_name, file_location, access_key, secret_key, region)
     return s3_file.get()["Body"].read().decode("utf-8")
@@ -62,10 +66,51 @@ def get_job_and_metadata_from_s3(service_id, job_id):
 
 
 def get_job_from_s3(service_id, job_id):
+    print(f"ENTERE get_job_from_s3 with {service_id} {job_id}")
     obj = get_s3_object(*get_job_location(service_id, job_id))
+    print(f"obj is {obj}")
     return obj.get()["Body"].read().decode("utf-8")
 
 
+def get_phone_number_from_s3(service_id, job_id, job_row_number):
+    # We don't want to constantly pull down a job from s3 every time we need a phone number.
+    # At the same time we don't want to store it in redis or the db
+    # So this is a little recycling mechanism to reduce the number of downloads.
+    print(
+        f"ENTER GET_PHONE_NUMBER_FROM_S3 WITH JOB_ID {job_id} SERVICE_ID {service_id} JOB_ROW_NUMBER {job_row_number}"
+    )
+    job = JOBS.get(job_id)
+    print(f"JOB FROM EXPIRINGDICT {job} JOB ID {job_id}")
+    if job is None:
+        print("JOB IS NONE")
+        job = get_job_from_s3(service_id, job_id)
+        print(f"FRESH JOB {job}")
+        JOBS[job_id] = job
+        print("ADDED {job} to EXPRING DICT")
+    else:
+        print("JOB IS NOT NONE")
+    job = job.split("\r\n")
+    print(f"JOB AFTER SPLIT {job}")
+    first_row = job[0]
+    job.pop(0)
+    first_row = first_row.split(",")
+    phone_index = 0
+    for item in first_row:
+        if item == "phone number":
+            break
+        phone_index = phone_index + 1
+    correct_row = job[job_row_number]
+    correct_row = correct_row.split(",")
+
+    my_phone = correct_row[phone_index]
+    my_phone = my_phone.replace("(", "")
+    my_phone = my_phone.replace(")", "")
+    my_phone = my_phone.replace("-", "")
+    my_phone = my_phone.replace(" ", "")
+    my_phone = my_phone.replace("+", "")
+    return my_phone
+
+
 def get_job_metadata_from_s3(service_id, job_id):
     obj = get_s3_object(*get_job_location(service_id, job_id))
     return obj.get()["Metadata"]
diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py
index 822b1f119..af310d3d5 100644
--- a/app/celery/provider_tasks.py
+++ b/app/celery/provider_tasks.py
@@ -31,7 +31,7 @@ DELIVERY_RECEIPT_DELAY_IN_SECONDS = 120
 @notify_celery.task(
     bind=True,
     name="check_sms_delivery_receipt",
-    max_retries=48,
+    max_retries=12,
     default_retry_delay=300,
 )
 def check_sms_delivery_receipt(self, message_id, notification_id, sent_at):
@@ -92,7 +92,7 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at):
 
 
 @notify_celery.task(
-    bind=True, name="deliver_sms", max_retries=48, default_retry_delay=300
+    bind=True, name="deliver_sms", max_retries=12, default_retry_delay=300
 )
 def deliver_sms(self, notification_id):
     try:
diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py
index d946ecfae..1a13608e8 100644
--- a/app/delivery/send_to_providers.py
+++ b/app/delivery/send_to_providers.py
@@ -9,7 +9,8 @@ from notifications_utils.template import (
     SMSMessageTemplate,
 )
 
-from app import create_uuid, db, notification_provider_clients
+from app import create_uuid, db, notification_provider_clients, redis_store
+from app.aws.s3 import get_phone_number_from_s3
 from app.celery.test_key_tasks import send_email_response, send_sms_response
 from app.dao.email_branding_dao import dao_get_email_branding_by_id
 from app.dao.notifications_dao import dao_update_notification
@@ -65,8 +66,26 @@ def send_sms_to_provider(notification):
                 # providers as a slow down of our providers can cause us to run out of DB connections
                 # Therefore we pull all the data from our DB models into `send_sms_kwargs`now before
                 # closing the session (as otherwise it would be reopened immediately)
+
+                # We start by trying to get the phone number from a job in s3.  If we fail, we assume
+                # the phone number is for the verification code on login, which is not a job.
+                my_phone = None
+                try:
+                    my_phone = get_phone_number_from_s3(
+                        notification.service_id,
+                        notification.job_id,
+                        notification.job_row_number,
+                    )
+                    print(f"MY PHONE FROM JOB {my_phone}")
+                except BaseException:
+                    my_phone = redis_store.get(f"2facode_{notification.id}")
+                    if my_phone:
+                        my_phone = my_phone.decode("utf-8")
+                        print(f"MY PHONE FROM VERIFY CODE {my_phone}")
+                if my_phone is None:
+                    raise Exception("what happened to the phone number")
                 send_sms_kwargs = {
-                    "to": notification.normalised_to,
+                    "to": my_phone,
                     "content": str(template),
                     "reference": str(notification.id),
                     "sender": notification.reply_to_text,
@@ -101,7 +120,7 @@ def send_email_to_provider(notification):
         html_email = HTMLEmailTemplate(
             template_dict,
             values=notification.personalisation,
-            **get_html_email_options(service)
+            **get_html_email_options(service),
         )
 
         plain_text_email = PlainTextEmailTemplate(
diff --git a/app/user/rest.py b/app/user/rest.py
index 25714d94a..7941504ee 100644
--- a/app/user/rest.py
+++ b/app/user/rest.py
@@ -7,6 +7,7 @@ from flask import Blueprint, abort, current_app, jsonify, request
 from notifications_utils.recipients import is_us_phone_number, use_numeric_sender
 from sqlalchemy.exc import IntegrityError
 
+from app import redis_store
 from app.config import QueueNames
 from app.dao.permissions_dao import permission_dao
 from app.dao.service_user_dao import dao_get_service_user, dao_update_service_user
@@ -337,6 +338,7 @@ def create_2fa_code(
         reply_to = get_sms_reply_to_for_notify_service(recipient, template)
     elif template.template_type == EMAIL_TYPE:
         reply_to = template.service.get_default_reply_to_email_address()
+
     saved_notification = persist_notification(
         template_id=template.id,
         template_version=template.version,
@@ -348,6 +350,8 @@ def create_2fa_code(
         key_type=KEY_TYPE_NORMAL,
         reply_to_text=reply_to,
     )
+
+    redis_store.set(f"2facode_{saved_notification.id}", recipient, ex=1800)
     # Assume that we never want to observe the Notify service's research mode
     # setting for this notification - we still need to be able to log into the
     # admin even if we're doing user research using this service:
diff --git a/migrations/alembic.ini b/migrations/alembic.ini
index c29545c7d..1f996dd97 100644
--- a/migrations/alembic.ini
+++ b/migrations/alembic.ini
@@ -20,17 +20,17 @@ keys = console
 keys = generic
 
 [logger_root]
-level = INFO
+level = WARNING
 handlers =
 qualname =
 
 [logger_sqlalchemy]
-level = INFO
+level = WARNING
 handlers =
 qualname = sqlalchemy.engine
 
 [logger_alembic]
-level = INFO
+level = WARNING
 handlers =
 qualname = alembic
 
diff --git a/poetry.lock b/poetry.lock
index 1cceabfa1..b61d186e7 100644
--- a/poetry.lock
+++ b/poetry.lock
@@ -1197,6 +1197,20 @@ files = [
 [package.extras]
 testing = ["hatch", "pre-commit", "pytest", "tox"]
 
+[[package]]
+name = "expiringdict"
+version = "1.2.2"
+description = "Dictionary with auto-expiring values for caching purposes"
+optional = false
+python-versions = "*"
+files = [
+    {file = "expiringdict-1.2.2-py3-none-any.whl", hash = "sha256:09a5d20bc361163e6432a874edd3179676e935eb81b925eccef48d409a8a45e8"},
+    {file = "expiringdict-1.2.2.tar.gz", hash = "sha256:300fb92a7e98f15b05cf9a856c1415b3bc4f2e132be07daa326da6414c23ee09"},
+]
+
+[package.extras]
+tests = ["coverage", "coveralls", "dill", "mock", "nose"]
+
 [[package]]
 name = "fastjsonschema"
 version = "2.19.0"
@@ -2168,16 +2182,6 @@ files = [
     {file = "MarkupSafe-2.1.3-cp311-cp311-musllinux_1_1_x86_64.whl", hash = "sha256:5bbe06f8eeafd38e5d0a4894ffec89378b6c6a625ff57e3028921f8ff59318ac"},
     {file = "MarkupSafe-2.1.3-cp311-cp311-win32.whl", hash = "sha256:dd15ff04ffd7e05ffcb7fe79f1b98041b8ea30ae9234aed2a9168b5797c3effb"},
     {file = "MarkupSafe-2.1.3-cp311-cp311-win_amd64.whl", hash = "sha256:134da1eca9ec0ae528110ccc9e48041e0828d79f24121a1a146161103c76e686"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-macosx_10_9_universal2.whl", hash = "sha256:f698de3fd0c4e6972b92290a45bd9b1536bffe8c6759c62471efaa8acb4c37bc"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:aa57bd9cf8ae831a362185ee444e15a93ecb2e344c8e52e4d721ea3ab6ef1823"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:ffcc3f7c66b5f5b7931a5aa68fc9cecc51e685ef90282f4a82f0f5e9b704ad11"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:47d4f1c5f80fc62fdd7777d0d40a2e9dda0a05883ab11374334f6c4de38adffd"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-manylinux_2_5_i686.manylinux1_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:1f67c7038d560d92149c060157d623c542173016c4babc0c1913cca0564b9939"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_aarch64.whl", hash = "sha256:9aad3c1755095ce347e26488214ef77e0485a3c34a50c5a5e2471dff60b9dd9c"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_i686.whl", hash = "sha256:14ff806850827afd6b07a5f32bd917fb7f45b046ba40c57abdb636674a8b559c"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8f9293864fe09b8149f0cc42ce56e3f0e54de883a9de90cd427f191c346eb2e1"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-win32.whl", hash = "sha256:715d3562f79d540f251b99ebd6d8baa547118974341db04f5ad06d5ea3eb8007"},
-    {file = "MarkupSafe-2.1.3-cp312-cp312-win_amd64.whl", hash = "sha256:1b8dd8c3fd14349433c79fa8abeb573a55fc0fdd769133baac1f5e07abf54aeb"},
     {file = "MarkupSafe-2.1.3-cp37-cp37m-macosx_10_9_x86_64.whl", hash = "sha256:8e254ae696c88d98da6555f5ace2279cf7cd5b3f52be2b5cf97feafe883b58d2"},
     {file = "MarkupSafe-2.1.3-cp37-cp37m-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:cb0932dc158471523c9637e807d9bfb93e06a95cbf010f1a38b98623b929ef2b"},
     {file = "MarkupSafe-2.1.3-cp37-cp37m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:9402b03f1a1b4dc4c19845e5c749e3ab82d5078d16a2a4c2cd2df62d57bb0707"},
@@ -4697,4 +4701,4 @@ testing = ["big-O", "jaraco.functools", "jaraco.itertools", "more-itertools", "p
 [metadata]
 lock-version = "2.0"
 python-versions = ">=3.9,<3.12"
-content-hash = "a5f9bd15a2dc8c11fd394e13e6ed3e50f7b7c7b5830c619e0a7124dee498c8e9"
+content-hash = "61f89a02495f30c6ba0deae8c4914dc7e52781dd95d897a4c480b120aa25c1af"
diff --git a/pyproject.toml b/pyproject.toml
index 6d3807f7b..2126eadb8 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -25,6 +25,7 @@ click-plugins = "==1.1.1"
 click-repl = "==0.3.0"
 deprecated = "==1.2.14"
 eventlet = "==0.34.2"
+expiringdict = "==1.2.2"
 flask = "~=2.3"
 flask-bcrypt = "==1.0.1"
 flask-marshmallow = "==0.14.0"
diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py
index 2989d86a9..f6f03f90a 100644
--- a/tests/app/aws/test_s3.py
+++ b/tests/app/aws/test_s3.py
@@ -5,7 +5,13 @@ from os import getenv
 import pytest
 from botocore.exceptions import ClientError
 
-from app.aws.s3 import file_exists, get_s3_file, remove_csv_object, remove_s3_object
+from app.aws.s3 import (
+    file_exists,
+    get_phone_number_from_s3,
+    get_s3_file,
+    remove_csv_object,
+    remove_s3_object,
+)
 
 default_access_key = getenv("CSV_AWS_ACCESS_KEY_ID")
 default_secret_key = getenv("CSV_AWS_SECRET_ACCESS_KEY")
@@ -39,6 +45,28 @@ def test_get_s3_file_makes_correct_call(notify_api, mocker):
     )
 
 
+@pytest.mark.parametrize(
+    "job, job_id, job_row_number, expected_phone_number",
+    [
+        ("phone number\r\n+15555555555", "aaa", 0, "15555555555"),
+        (
+            "day of week,favorite color,phone number\r\nmonday,green,15551111111\r\ntuesday,red,15552222222",
+            "bbb",
+            1,
+            "15552222222",
+        ),
+    ],
+)
+def test_get_phone_number_from_s3(
+    mocker, job, job_id, job_row_number, expected_phone_number
+):
+    get_job_mock = mocker.patch("app.aws.s3.get_job_from_s3")
+    get_job_mock.return_value = job
+    print(f"ABOUT TO CALL GET_PHONE_NUMBER_FROM_S3 WITH JOB_ID {job_id}")
+    phone_number = get_phone_number_from_s3("service_id", job_id, job_row_number)
+    assert phone_number == expected_phone_number
+
+
 def test_remove_csv_object(notify_api, mocker):
     get_s3_mock = mocker.patch("app.aws.s3.get_s3_object")
     remove_csv_object("mykey")
diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py
index e98b8a319..4cbf22b9a 100644
--- a/tests/app/delivery/test_send_to_providers.py
+++ b/tests/app/delivery/test_send_to_providers.py
@@ -94,6 +94,9 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
 
     mocker.patch("app.aws_sns_client.send_sms")
 
+    mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+    mock_s3.return_value = "2028675309"
+
     send_to_providers.send_sms_to_provider(db_notification)
 
     aws_sns_client.send_sms.assert_called_once_with(
@@ -186,6 +189,9 @@ def test_send_sms_should_use_template_version_from_notification_not_latest(
         normalised_to="2028675309",
     )
 
+    mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+    mock_s3.return_value = "2028675309"
+
     mocker.patch("app.aws_sns_client.send_sms")
 
     version_on_notification = sample_template.version
@@ -266,6 +272,9 @@ def test_should_send_sms_with_downgraded_content(notify_db_session, mocker):
 
     mocker.patch("app.aws_sns_client.send_sms")
 
+    mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+    mock_phone.return_value = "15555555555"
+
     send_to_providers.send_sms_to_provider(db_notification)
 
     aws_sns_client.send_sms.assert_called_once_with(
@@ -285,6 +294,8 @@ def test_send_sms_should_use_service_sms_sender(
         template=sample_template, reply_to_text=sms_sender.sms_sender
     )
     expected_sender_name = sms_sender.sms_sender
+    mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+    mock_phone.return_value = "15555555555"
 
     send_to_providers.send_sms_to_provider(
         db_notification,
@@ -513,6 +524,9 @@ def test_should_update_billable_units_and_status_according_to_research_mode_and_
     if research_mode:
         sample_template.service.research_mode = True
 
+    mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+    mock_phone.return_value = "15555555555"
+
     send_to_providers.send_sms_to_provider(notification)
     assert notification.billable_units == billable_units
     assert notification.status == expected_status
@@ -552,6 +566,9 @@ def test_should_send_sms_to_international_providers(
         normalised_to="601117224412",
     )
 
+    mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+    mock_s3.return_value = "601117224412"
+
     send_to_providers.send_sms_to_provider(notification_international)
 
     aws_sns_client.send_sms.assert_called_once_with(
@@ -588,6 +605,9 @@ def test_should_handle_sms_sender_and_prefix_message(
     template = create_template(service, content="bar")
     notification = create_notification(template, reply_to_text=sms_sender)
 
+    mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+    mock_phone.return_value = "15555555555"
+
     send_to_providers.send_sms_to_provider(notification)
 
     aws_sns_client.send_sms.assert_called_once_with(
@@ -622,6 +642,9 @@ def test_send_sms_to_provider_should_use_normalised_to(mocker, client, sample_te
     notification = create_notification(
         template=sample_template, to_field="+12028675309", normalised_to="2028675309"
     )
+
+    mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+    mock_s3.return_value = "2028675309"
     send_to_providers.send_sms_to_provider(notification)
     send_mock.assert_called_once_with(
         to=notification.normalised_to,
@@ -677,6 +700,10 @@ def test_send_sms_to_provider_should_return_template_if_found_in_redis(
     notification = create_notification(
         template=sample_template, to_field="+447700900855", normalised_to="447700900855"
     )
+
+    mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3")
+    mock_s3.return_value = "447700900855"
+
     send_to_providers.send_sms_to_provider(notification)
     assert mock_get_template.called is False
     assert mock_get_service.called is False

From a5f78224b21e729e208b33ddd99457bcf770cb46 Mon Sep 17 00:00:00 2001
From: Kenneth Kehl <@kkehl@flexion.us>
Date: Fri, 5 Jan 2024 10:39:07 -0800
Subject: [PATCH 2/3] remove print statements

---
 app/aws/s3.py                     | 12 ------------
 app/delivery/send_to_providers.py |  2 --
 tests/app/aws/test_s3.py          |  1 -
 3 files changed, 15 deletions(-)

diff --git a/app/aws/s3.py b/app/aws/s3.py
index c56c4e3a0..47c307d44 100644
--- a/app/aws/s3.py
+++ b/app/aws/s3.py
@@ -66,9 +66,7 @@ def get_job_and_metadata_from_s3(service_id, job_id):
 
 
 def get_job_from_s3(service_id, job_id):
-    print(f"ENTERE get_job_from_s3 with {service_id} {job_id}")
     obj = get_s3_object(*get_job_location(service_id, job_id))
-    print(f"obj is {obj}")
     return obj.get()["Body"].read().decode("utf-8")
 
 
@@ -76,21 +74,11 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number):
     # We don't want to constantly pull down a job from s3 every time we need a phone number.
     # At the same time we don't want to store it in redis or the db
     # So this is a little recycling mechanism to reduce the number of downloads.
-    print(
-        f"ENTER GET_PHONE_NUMBER_FROM_S3 WITH JOB_ID {job_id} SERVICE_ID {service_id} JOB_ROW_NUMBER {job_row_number}"
-    )
     job = JOBS.get(job_id)
-    print(f"JOB FROM EXPIRINGDICT {job} JOB ID {job_id}")
     if job is None:
-        print("JOB IS NONE")
         job = get_job_from_s3(service_id, job_id)
-        print(f"FRESH JOB {job}")
         JOBS[job_id] = job
-        print("ADDED {job} to EXPRING DICT")
-    else:
-        print("JOB IS NOT NONE")
     job = job.split("\r\n")
-    print(f"JOB AFTER SPLIT {job}")
     first_row = job[0]
     job.pop(0)
     first_row = first_row.split(",")
diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py
index 1a13608e8..68f151863 100644
--- a/app/delivery/send_to_providers.py
+++ b/app/delivery/send_to_providers.py
@@ -76,12 +76,10 @@ def send_sms_to_provider(notification):
                         notification.job_id,
                         notification.job_row_number,
                     )
-                    print(f"MY PHONE FROM JOB {my_phone}")
                 except BaseException:
                     my_phone = redis_store.get(f"2facode_{notification.id}")
                     if my_phone:
                         my_phone = my_phone.decode("utf-8")
-                        print(f"MY PHONE FROM VERIFY CODE {my_phone}")
                 if my_phone is None:
                     raise Exception("what happened to the phone number")
                 send_sms_kwargs = {
diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py
index f6f03f90a..6af212d87 100644
--- a/tests/app/aws/test_s3.py
+++ b/tests/app/aws/test_s3.py
@@ -62,7 +62,6 @@ def test_get_phone_number_from_s3(
 ):
     get_job_mock = mocker.patch("app.aws.s3.get_job_from_s3")
     get_job_mock.return_value = job
-    print(f"ABOUT TO CALL GET_PHONE_NUMBER_FROM_S3 WITH JOB_ID {job_id}")
     phone_number = get_phone_number_from_s3("service_id", job_id, job_row_number)
     assert phone_number == expected_phone_number
 

From 726e0b15fac7a0c4d39c874492e0a635b107780f Mon Sep 17 00:00:00 2001
From: Kenneth Kehl <@kkehl@flexion.us>
Date: Mon, 8 Jan 2024 14:31:28 -0800
Subject: [PATCH 3/3] code review feedback

---
 app/aws/s3.py                     | 10 ++++------
 app/celery/provider_tasks.py      |  4 ++--
 app/delivery/send_to_providers.py |  7 ++++++-
 tests/app/aws/test_s3.py          |  8 +++++++-
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/app/aws/s3.py b/app/aws/s3.py
index 47c307d44..2a631c326 100644
--- a/app/aws/s3.py
+++ b/app/aws/s3.py
@@ -1,3 +1,5 @@
+import re
+
 import botocore
 from boto3 import Session
 from expiringdict import ExpiringDict
@@ -8,7 +10,7 @@ from app.clients import AWS_CLIENT_CONFIG
 FILE_LOCATION_STRUCTURE = "service-{}-notify/{}.csv"
 
 
-JOBS = ExpiringDict(max_len=100, max_age_seconds=3600)
+JOBS = ExpiringDict(max_len=100, max_age_seconds=3600 * 4)
 
 
 def get_s3_file(bucket_name, file_location, access_key, secret_key, region):
@@ -91,11 +93,7 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number):
     correct_row = correct_row.split(",")
 
     my_phone = correct_row[phone_index]
-    my_phone = my_phone.replace("(", "")
-    my_phone = my_phone.replace(")", "")
-    my_phone = my_phone.replace("-", "")
-    my_phone = my_phone.replace(" ", "")
-    my_phone = my_phone.replace("+", "")
+    my_phone = re.sub(r"[\+\s\(\)\-\.]*", "", my_phone)
     return my_phone
 
 
diff --git a/app/celery/provider_tasks.py b/app/celery/provider_tasks.py
index af310d3d5..822b1f119 100644
--- a/app/celery/provider_tasks.py
+++ b/app/celery/provider_tasks.py
@@ -31,7 +31,7 @@ DELIVERY_RECEIPT_DELAY_IN_SECONDS = 120
 @notify_celery.task(
     bind=True,
     name="check_sms_delivery_receipt",
-    max_retries=12,
+    max_retries=48,
     default_retry_delay=300,
 )
 def check_sms_delivery_receipt(self, message_id, notification_id, sent_at):
@@ -92,7 +92,7 @@ def check_sms_delivery_receipt(self, message_id, notification_id, sent_at):
 
 
 @notify_celery.task(
-    bind=True, name="deliver_sms", max_retries=12, default_retry_delay=300
+    bind=True, name="deliver_sms", max_retries=48, default_retry_delay=300
 )
 def deliver_sms(self, notification_id):
     try:
diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py
index 68f151863..79bf77fd2 100644
--- a/app/delivery/send_to_providers.py
+++ b/app/delivery/send_to_providers.py
@@ -81,7 +81,12 @@ def send_sms_to_provider(notification):
                     if my_phone:
                         my_phone = my_phone.decode("utf-8")
                 if my_phone is None:
-                    raise Exception("what happened to the phone number")
+                    si = notification.service_id
+                    ji = notification.job_id
+                    jrn = notification.job_row_number
+                    raise Exception(
+                        f"The phone number for (Service ID: {si}; Job ID: {ji}; Job Row Number {jrn} was not found."
+                    )
                 send_sms_kwargs = {
                     "to": my_phone,
                     "content": str(template),
diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py
index 6af212d87..27df6ac6c 100644
--- a/tests/app/aws/test_s3.py
+++ b/tests/app/aws/test_s3.py
@@ -50,11 +50,17 @@ def test_get_s3_file_makes_correct_call(notify_api, mocker):
     [
         ("phone number\r\n+15555555555", "aaa", 0, "15555555555"),
         (
-            "day of week,favorite color,phone number\r\nmonday,green,15551111111\r\ntuesday,red,15552222222",
+            "day of week,favorite color,phone number\r\nmonday,green,1.555.111.1111\r\ntuesday,red,+1 (555) 222-2222",
             "bbb",
             1,
             "15552222222",
         ),
+        (
+            "day of week,favorite color,phone number\r\nmonday,green,1.555.111.1111\r\ntuesday,red,+1 (555) 222-2222",
+            "ccc",
+            0,
+            "15551111111",
+        ),
     ],
 )
 def test_get_phone_number_from_s3(