Merge pull request #707 from GSA/notify-api-685f

get phone numbers from the jobs in s3

app/aws/s3.py

@@ -1,5 +1,8 @@
+import re
+
 import botocore
 from boto3 import Session
+from expiringdict import ExpiringDict
 from flask import current_app
 
 from app.clients import AWS_CLIENT_CONFIG
@@ -7,6 +10,9 @@ from app.clients import AWS_CLIENT_CONFIG
 FILE_LOCATION_STRUCTURE = "service-{}-notify/{}.csv"
 
 
+JOBS = ExpiringDict(max_len=100, max_age_seconds=3600 * 4)
+
+
 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")
@@ -66,6 +72,31 @@ def get_job_from_s3(service_id, job_id):
     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.
+    job = JOBS.get(job_id)
+    if job is None:
+        job = get_job_from_s3(service_id, job_id)
+        JOBS[job_id] = job
+    job = job.split("\r\n")
+    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 = re.sub(r"[\+\s\(\)\-\.]*", "", my_phone)
+    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"]

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,29 @@ 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,
+                    )
+                except BaseException:
+                    my_phone = redis_store.get(f"2facode_{notification.id}")
+                    if my_phone:
+                        my_phone = my_phone.decode("utf-8")
+                if my_phone is None:
+                    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": notification.normalised_to,
+                    "to": my_phone,
                     "content": str(template),
                     "reference": str(notification.id),
                     "sender": notification.reply_to_text,
@@ -101,7 +123,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(

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:

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
 

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"

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"

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,33 @@ 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,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(
+    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
+    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")

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