From a028be238b37cbe2f6f7d323ab8bc6e4e4898177 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 1 Feb 2024 10:48:59 -0800 Subject: [PATCH 1/6] notify-api-784 remove simulated numbers --- app/aws/s3.py | 4 +++- app/config.py | 3 ++- tests/app/notifications/test_process_notification.py | 5 ++--- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 13879bf6f..7ddf9828e 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -122,7 +122,9 @@ def extract_phones(job): current_app.logger.info(f"LENGTH OF ROW IS {len(row)}") if phone_index >= len(row): phones[job_row] = "Error: can't retrieve phone number" - current_app.logger.error("Corrupt csv file, missing columns job_id {job_id} service_id {service_id}") + current_app.logger.error( + "Corrupt csv file, missing columns job_id {job_id} service_id {service_id}" + ) else: my_phone = row[phone_index] my_phone = re.sub(r"[\+\s\(\)\-\.]*", "", my_phone) diff --git a/app/config.py b/app/config.py index 49cbd89c0..b4a5b8aed 100644 --- a/app/config.py +++ b/app/config.py @@ -283,7 +283,8 @@ class Config(object): "simulate-delivered-2@notifications.service.gov.uk", "simulate-delivered-3@notifications.service.gov.uk", ) - SIMULATED_SMS_NUMBERS = ("+12028675000", "+12028675111", "+12028675222") + # 7755 is success, 7167 is failure + SIMULATED_SMS_NUMBERS = ("+14254147755", "+14254147167") FREE_SMS_TIER_FRAGMENT_COUNT = 250000 diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 87c8da988..bbde98da2 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -254,9 +254,8 @@ def test_send_notification_to_queue_throws_exception_deletes_notification( @pytest.mark.parametrize( "to_address, notification_type, expected", [ - ("+12028675000", "sms", True), - ("+12028675111", "sms", True), - ("+12028675222", "sms", True), + ("+14254147755", "sms", True), + ("+14254147167", "sms", True), ("2028675000", "sms", True), ("2028675111", "sms", True), ("simulate-delivered@notifications.service.gov.uk", "email", True), From 82dd29d457dda520c8efd0d3fe17d7e1f3c924c0 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 1 Feb 2024 12:01:29 -0800 Subject: [PATCH 2/6] handle bom in phone number field --- app/aws/s3.py | 12 ++++-------- tests/app/aws/test_s3.py | 7 +++++++ 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 13879bf6f..ca283d743 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -105,24 +105,20 @@ def extract_phones(job): current_app.logger.info(f"HEADERS {first_row}") phone_index = 0 for item in first_row: - if item.lower() == "phone number": + # Note: may contain a BOM and look like \ufeffphone number + if "phone number" in item.lower(): break phone_index = phone_index + 1 + phones = {} job_row = 0 for row in job: row = row.split(",") - # TODO WHY ARE WE CALCULATING PHONE INDEX IN THE LOOP? - phone_index = 0 - for item in first_row: - if item.lower() == "phone number": - break - phone_index = phone_index + 1 current_app.logger.info(f"PHONE INDEX IS NOW {phone_index}") current_app.logger.info(f"LENGTH OF ROW IS {len(row)}") if phone_index >= len(row): phones[job_row] = "Error: can't retrieve phone number" - current_app.logger.error("Corrupt csv file, missing columns job_id {job_id} service_id {service_id}") + current_app.logger.error("Corrupt csv file") else: my_phone = row[phone_index] my_phone = re.sub(r"[\+\s\(\)\-\.]*", "", my_phone) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index ad01a00c5..a5134501c 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -67,6 +67,13 @@ def test_get_s3_file_makes_correct_call(notify_api, mocker): 0, "15553333333", ), + ( + # simulate file saved with utf8withbom + "\\ufeffPHONE NUMBER,Name\r\n5555555550,T 1\r\n5555555551,T 5,3/31/2024\r\n5555555552,T 2", + "eee", + 2, + "5555555552", + ), ], ) def test_get_phone_number_from_s3( From c45e74a74bb593386e21aa602f748c45a03031b7 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 2 Feb 2024 09:03:54 -0800 Subject: [PATCH 3/6] update sent_at --- app/dao/notifications_dao.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 23905944c..ededaec04 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -100,6 +100,7 @@ def _update_notification_status( current_status=notification.status, status=status ) notification.status = status + notification.sent_at = datetime.utcnow() if provider_response: notification.provider_response = provider_response if carrier: @@ -325,6 +326,7 @@ def sanitize_successful_notification_by_id(notification_id, carrier, provider_re "notification_id": notification_id, "carrier": carrier, "response": provider_response, + "sent_at": datetime.utcnow(), } db.session.execute(update_query, input_params) From fad3d4f473ea09a26ee510ea6841f3c6b3cb858b Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 2 Feb 2024 09:32:24 -0800 Subject: [PATCH 4/6] fix tests --- tests/app/notifications/test_process_notification.py | 4 +--- .../app/service/send_notification/test_send_notification.py | 2 +- tests/app/v2/notifications/test_post_notifications.py | 5 ++--- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index bbde98da2..2e302476a 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -256,8 +256,6 @@ def test_send_notification_to_queue_throws_exception_deletes_notification( [ ("+14254147755", "sms", True), ("+14254147167", "sms", True), - ("2028675000", "sms", True), - ("2028675111", "sms", True), ("simulate-delivered@notifications.service.gov.uk", "email", True), ("simulate-delivered-2@notifications.service.gov.uk", "email", True), ("simulate-delivered-3@notifications.service.gov.uk", "email", True), @@ -274,7 +272,7 @@ def test_simulated_recipient(notify_api, to_address, notification_type, expected 'simulate-delivered-2@notifications.service.gov.uk', 'simulate-delivered-2@notifications.service.gov.uk' ) - SIMULATED_SMS_NUMBERS = ('+12028675000', '+12028675111', '+12028675222') + SIMULATED_SMS_NUMBERS = ("+14254147755", "+14254147167") """ formatted_address = None diff --git a/tests/app/service/send_notification/test_send_notification.py b/tests/app/service/send_notification/test_send_notification.py index c65614e22..3ffbb8e2e 100644 --- a/tests/app/service/send_notification/test_send_notification.py +++ b/tests/app/service/send_notification/test_send_notification.py @@ -881,7 +881,7 @@ def test_should_not_persist_notification_or_send_email_if_simulated_email( assert Notification.query.count() == 0 -@pytest.mark.parametrize("to_sms", ["2028675000", "2028675111", "+12028675222"]) +@pytest.mark.parametrize("to_sms", ["+14254147755", "+14254147167"]) def test_should_not_persist_notification_or_send_sms_if_simulated_number( client, to_sms, sample_template, mocker ): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index f461531bc..bb9a58453 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -532,9 +532,8 @@ def test_post_email_notification_returns_201( ("simulate-delivered@notifications.service.gov.uk", EMAIL_TYPE), ("simulate-delivered-2@notifications.service.gov.uk", EMAIL_TYPE), ("simulate-delivered-3@notifications.service.gov.uk", EMAIL_TYPE), - ("2028675000", "sms"), - ("2028675111", "sms"), - ("2028675222", "sms"), + ("+14254147167", "sms"), + ("+14254147755", "sms"), ], ) def test_should_not_persist_or_send_notification_if_simulated_recipient( From 65bc37d20322ffb9113286772e2805de76c0be4f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 2 Feb 2024 10:29:01 -0800 Subject: [PATCH 5/6] fix fstring --- app/aws/s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 7ddf9828e..5921141ca 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -123,7 +123,7 @@ def extract_phones(job): if phone_index >= len(row): phones[job_row] = "Error: can't retrieve phone number" current_app.logger.error( - "Corrupt csv file, missing columns job_id {job_id} service_id {service_id}" + "Corrupt csv file, missing columns or possibly a byte order mark in the file" ) else: my_phone = row[phone_index] From ddad4e409eba37d464951c3edf2fcd39e80e8d1c Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 2 Feb 2024 12:51:11 -0800 Subject: [PATCH 6/6] update send_at when status resolves --- app/dao/notifications_dao.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index ededaec04..ee08f9af0 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -319,9 +319,10 @@ def _filter_query(query, filter_dict=None): def sanitize_successful_notification_by_id(notification_id, carrier, provider_response): update_query = """ update notifications set provider_response=:response, carrier=:carrier, - notification_status='delivered', "to"='1', normalised_to='1' + notification_status='delivered', sent_at=:sent_at, "to"='1', normalised_to='1' where id=:notification_id """ + input_params = { "notification_id": notification_id, "carrier": carrier,