From 82690d6e3d3ccb70f5176271685bd4ad237dbd2c Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 6 Jun 2024 09:34:49 -0700 Subject: [PATCH 1/4] add debug for user issue --- app/main/views/send.py | 35 ++++++++++++++++++++++++++++++++-- app/utils/csv.py | 18 +++++++++++++++++ notifications_utils/logging.py | 13 +++++++++++++ poetry.lock | 1 + 4 files changed, 65 insertions(+), 2 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index aab2db104..8bb6ce24c 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -3,7 +3,16 @@ import uuid from string import ascii_uppercase from zipfile import BadZipFile -from flask import abort, flash, redirect, render_template, request, session, url_for +from flask import ( + abort, + current_app, + flash, + redirect, + render_template, + request, + session, + url_for, +) from flask_login import current_user from markupsafe import Markup from notifications_python_client.errors import HTTPError @@ -31,12 +40,18 @@ from app.s3_client.s3_csv_client import ( s3upload, set_metadata_on_csv_upload, ) -from app.utils import PermanentRedirect, should_skip_template_page, unicode_truncate +from app.utils import ( + PermanentRedirect, + hilite, + should_skip_template_page, + unicode_truncate, +) from app.utils.csv import Spreadsheet, get_errors_for_csv from app.utils.templates import get_template from app.utils.user import user_has_permissions from notifications_utils import SMS_CHAR_COUNT_LIMIT from notifications_utils.insensitive_dict import InsensitiveDict +from notifications_utils.logging import scrub from notifications_utils.recipients import RecipientCSV, first_column_headings from notifications_utils.sanitise_text import SanitiseASCII @@ -938,6 +953,9 @@ def send_notification(service_id, template_id): ) ) + current_app.logger.info( + hilite(scrub(f"Recipient for the one-off will be {recipient}")) + ) keys = [] values = [] for k, v in session["placeholders"].items(): @@ -971,6 +989,19 @@ def send_notification(service_id, template_id): valid="True", ) + # Here we are attempting to cleverly link the job id to the one-off recipient + # If we know the partial phone number of the recipient, we can search + # on that initially and find this, which will give us the job_id + # And once we know the job_id, we can search on that and it might tell us something + # about report generation. + current_app.logger.info( + hilite( + scrub( + f"Created job to send one-off, recipient is {recipient}, job_id is {upload_id}" + ) + ) + ) + session.pop("recipient") session.pop("placeholders") diff --git a/app/utils/csv.py b/app/utils/csv.py index c3c27ec18..a8d743adc 100644 --- a/app/utils/csv.py +++ b/app/utils/csv.py @@ -1,10 +1,13 @@ import datetime import pytz +from flask import current_app, json from flask_login import current_user from app.models.spreadsheet import Spreadsheet +from app.utils import hilite from app.utils.templates import get_sample_template +from notifications_utils.logging import scrub from notifications_utils.recipients import RecipientCSV @@ -71,7 +74,22 @@ def generate_notifications_csv(**kwargs): # This generates the "batch" csv report if kwargs.get("job_id"): + # The kwargs contain the job id, which is linked to the recipient's partial phone number in other debug + try: + current_app.logger.info( + hilite(f"Setting up report with kwargs {scrub(json.dumps(kwargs))}") + ) + except TypeError: + pass + original_file_contents = s3download(kwargs["service_id"], kwargs["job_id"]) + # This will verify that the user actually did successfully upload a csv for a one-off. Limit the size + # we display to 999 characters, because we don't want to show the contents for reports with thousands of rows. + current_app.logger.info( + hilite( + f"Original csv for job_id {kwargs['job_id']}: {scrub(original_file_contents[0:999])}" + ) + ) original_upload = RecipientCSV( original_file_contents, template=get_sample_template(kwargs["template_type"]), diff --git a/notifications_utils/logging.py b/notifications_utils/logging.py index 6a209cdd3..7c56a00ad 100644 --- a/notifications_utils/logging.py +++ b/notifications_utils/logging.py @@ -1,5 +1,6 @@ import logging import logging.handlers +import re import sys from itertools import product @@ -131,3 +132,15 @@ class JSONFormatter(BaseJSONFormatter): except (KeyError, IndexError) as e: logger.exception("failed to format log message: {} not found".format(e)) return log_record + + +def scrub(msg): + # Eventually we want to scrub all messages in all logs for phone numbers + # and email addresses, masking them. Ultimately this will probably get + # refactored into a 'SafeLogger' subclass or something, but let's start here + # with phones. + phones = re.findall("(?:\\+ *)?\\d[\\d\\- ]{7,}\\d", msg) + phones = [phone.replace("-", "").replace(" ", "") for phone in phones] + for phone in phones: + msg = msg.replace(phone, f"1XXXXX{phone[-5:]}") + return msg diff --git a/poetry.lock b/poetry.lock index 8cb15a59d..331962043 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1641,6 +1641,7 @@ files = [ {file = "msgpack-1.0.8-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:5fbb160554e319f7b22ecf530a80a3ff496d38e8e07ae763b9e82fadfe96f273"}, {file = "msgpack-1.0.8-cp39-cp39-win32.whl", hash = "sha256:f9af38a89b6a5c04b7d18c492c8ccf2aee7048aff1ce8437c4683bb5a1df893d"}, {file = "msgpack-1.0.8-cp39-cp39-win_amd64.whl", hash = "sha256:ed59dd52075f8fc91da6053b12e8c89e37aa043f8986efd89e61fae69dc1b011"}, + {file = "msgpack-1.0.8-py3-none-any.whl", hash = "sha256:24f727df1e20b9876fa6e95f840a2a2651e34c0ad147676356f4bf5fbb0206ca"}, {file = "msgpack-1.0.8.tar.gz", hash = "sha256:95c02b0e27e706e48d0e5426d1710ca78e0f0628d6e89d5b5a5b91a5f12274f3"}, ] From 83f5b6f0ab4ac6dda7532a5d1a7bbb84497f593f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 6 Jun 2024 10:43:41 -0700 Subject: [PATCH 2/4] add comment --- app/utils/csv.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/utils/csv.py b/app/utils/csv.py index a8d743adc..5c5b794de 100644 --- a/app/utils/csv.py +++ b/app/utils/csv.py @@ -75,6 +75,8 @@ def generate_notifications_csv(**kwargs): # This generates the "batch" csv report if kwargs.get("job_id"): # The kwargs contain the job id, which is linked to the recipient's partial phone number in other debug + # Some unit tests are mocking the kwargs and turning them into a function instead of dict, + # hence the try/except. try: current_app.logger.info( hilite(f"Setting up report with kwargs {scrub(json.dumps(kwargs))}") From 934f8927c65e927d69419a97d473219c57801253 Mon Sep 17 00:00:00 2001 From: Jonathan Bobel Date: Thu, 6 Jun 2024 13:50:08 -0400 Subject: [PATCH 3/4] Updated content, removed test no longer needed --- app/templates/views/message-status.html | 40 +++++++++++++++++++------ app/templates/views/trial-mode.html | 2 +- tests/app/main/views/test_index.py | 20 ------------- 3 files changed, 32 insertions(+), 30 deletions(-) diff --git a/app/templates/views/message-status.html b/app/templates/views/message-status.html index 089a29f49..d9dae439f 100644 --- a/app/templates/views/message-status.html +++ b/app/templates/views/message-status.html @@ -9,9 +9,13 @@

Delivery status

-

Notify’s real-time dashboard lets you check the status of any message.

-

For security, this information is only available for seven days after a message has been sent. You can download a report, including a list of sent messages, for your own records.

-

This page describes the statuses you'll see when you're signed in to Notify.

+

Notify starts sending your text messages immediately. Each message is sent to its cell phone carrier, which attempts + delivery to the recipient. This process is often almost instantaneous, but can sometimes take a while if a phone is + unavailable. The carrier will continue to try to deliver the message for up to 72 hours.

+

The Notify dashboard provides a high-level view of the number of messages Sent, Pending, Delivered, and Failed. The + dashboard data starts to update about five minutes after a message is sent and will continue to update for four hours.

+

Delivery statuses reflect delivery to a specific cell phone number, not to a person. No status can confirm that a + specific person actually received or read the message.

Text messages

-
+
{% call mapping_table( caption='Message statuses – text messages', field_headings=['Status', 'Description'], @@ -53,12 +57,14 @@ caption_visible=False ) %} {% for message_status, description in [ - ('Total', 'The total number of messages that have been sent during the last seven days.'), - ('Pending', 'Notify has sent the message to the provider. The provider will try to deliver the message to the recipient for up to 72 hours. Notify is waiting for delivery information.'), + ('Scheduled', 'The total number of messages that have been scheduled to be sent at some future time.'), + ('Canceled', 'Messages that were created and scheduled, but canceled prior to Sending.'), + ('Total', 'The total number of messages that have been sent during the specified time.'), + ('Pending', 'Notify has sent the message to the provider. The provider will try to deliver the message to the recipient for up to 72 + hours. “Pending” indicates that Notify is waiting for delivery information.'), ('Delivered', 'The message was successfully delivered. Notify cannot tell you if a user has opened or read a message.'), - ('Failed', 'The provider could not deliver the message. This can happen if the phone number was wrong or if the network operator - rejects the message. If you’re sure that these phone numbers are correct, you should contact us. If not, you should remove them from your database. You’ll still be charged for text messages that - cannot be delivered.' | safe), + ('Failed', 'The message could not be delivered.'), + ('Process error / Delivery not attempted', 'If you receive a large number of process errors, please contact the Notify team.'), ] %} {% call row() %} {{ text_field(message_status) }} @@ -67,5 +73,21 @@ {% endfor %} {% endcall %}
+

About carrier statuses

+

Sometimes Notify receives more detailed information from the carriers on the status of messages, and these can be found + in the downloadable reports. Not all carriers provide the same level of detail regarding delivery and some delivery + statutes have a slight variation in word choice. Notify includes this information in the reports to provide you as much + detail as possible. Remember, for security purposes, detailed information is only available for seven days after a + message has been sent.

+ +

Opting out

+

A text recipient can opt out of receiving text messages from your phone number at any time by responding “STOP” or + “QUIT” or one of several other keywords. If they opt out,

+ +

Notify.gov does not yet have a way to pull opt-out status and make it available in the UI for agencies to download.

{% endblock %} diff --git a/app/templates/views/trial-mode.html b/app/templates/views/trial-mode.html index 7ca6d2b97..cb0dca0ec 100644 --- a/app/templates/views/trial-mode.html +++ b/app/templates/views/trial-mode.html @@ -21,7 +21,7 @@
  • A text message of 160-306 characters is two parts.

    • For more information on how message parts are calculated, see - Pricing.

    + Tracking usage.

    Before going Live

    Before you request to make your service live so you can send messages to clients:

    diff --git a/tests/app/main/views/test_index.py b/tests/app/main/views/test_index.py index 4ee5a817f..57bdfaa70 100644 --- a/tests/app/main/views/test_index.py +++ b/tests/app/main/views/test_index.py @@ -179,26 +179,6 @@ def test_old_static_pages_redirect(client_request, view, expected_view): ) -def test_message_status_page_contains_message_status_ids(client_request): - # The 'email-statuses' and 'sms-statuses' id are linked to when we display a message status, - # so this test ensures we don't accidentally remove them - page = client_request.get("main.message_status") - - # email-statuses is commented out in view - # assert page.find(id='email-statuses') - assert page.find(id="text-message-statuses") - - -def test_message_status_page_contains_link_to_support(client_request): - page = client_request.get("main.message_status") - sms_status_table = page.find(id="text-message-statuses").findNext("tbody") - - temp_fail_details_cell = sms_status_table.select_one( - "tr:nth-child(4) > td:nth-child(2)" - ) - assert temp_fail_details_cell.find("a").attrs["href"] == url_for("main.support") - - def test_old_using_notify_page(client_request): client_request.get("main.using_notify", _expected_status=410) From 63bd660403f60e81dad05fed8d41a68e17440fda Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 7 Jun 2024 08:03:43 -0700 Subject: [PATCH 4/4] code review feedback --- tests/notifications_utils/test_logging.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/notifications_utils/test_logging.py b/tests/notifications_utils/test_logging.py index cc7f5ee34..858b9352b 100644 --- a/tests/notifications_utils/test_logging.py +++ b/tests/notifications_utils/test_logging.py @@ -49,3 +49,13 @@ def test_base_json_formatter_contains_service_id(): == "message to log" ) assert service_id_filter.filter(record).service_id == "no-service-id" + + +def test_scrub(): + result = logging.scrub( + "This is a message with 17775554324, and also 18884449323 and also 17775554324" + ) + assert ( + result + == "This is a message with 1XXXXX54324, and also 1XXXXX49323 and also 1XXXXX54324" + )