Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pinpoint fields to notifications #2326

Merged
merged 10 commits into from
Oct 25, 2024
Merged

Add pinpoint fields to notifications #2326

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
84bb526
add new fields to database
sastels Oct 15, 2024
3d6846e
tweak new col names and change receipt processing task
sastels Oct 15, 2024
fdbeee7
new test
sastels Oct 15, 2024
46ec67f
format
sastels Oct 15, 2024
5eb0117
add test for missing sms data
sastels Oct 15, 2024
96893e0
Merge branch 'main' into add-pinpoint-fields-to-notifications
sastels Oct 24, 2024
03feb77
ai [review] plz
sastels Oct 24, 2024
e5cd121
move new migrations to 0462
sastels Oct 24, 2024
6d6459a
fixup tests
sastels Oct 24, 2024
8fdf60c
few other ai suggestions I missed
sastels Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions app/aws/mocks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ def pinpoint_delivered_callback(reference=None, timestamp=1467074434, destinatio
"isoCountryCode": "CA",
"mcc": "302",
"mnc": "610",
"carrierName": "Bell Cellular Inc. / Aliant Telecom",
"carrierName": "Bell",
"messageId": reference,
"messageRequestTimestamp": timestamp,
"messageEncoding": "GSM",
Expand All @@ -254,15 +254,38 @@ def pinpoint_shortcode_delivered_callback(reference=None, timestamp=1467074434,
"originationPhoneNumber": "555555",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pinpoint_shortcode_delivered_callback function signature should be updated to include the new carrierName parameter to ensure it is passed correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, we don't want to ever pass it.

"destinationPhoneNumber": destination,
"isoCountryCode": "CA",
"carrierName": "Bell",
"messageId": reference,
"messageRequestTimestamp": timestamp,
"messageEncoding": "GSM",
"messageType": "TRANSACTIONAL",
"messageStatus": "SUCCESSFUL",
"messageStatusDescription": "Message has been accepted by phone carrier",
"totalMessageParts": 1,
"totalMessagePrice": 0.02183,
"totalCarrierFee": 0.005,
"totalMessagePrice": 0.00581,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in totalMessagePrice and totalCarrierFee values should be verified to ensure they are correct and consistent with the new Pinpoint delivery receipts.

"totalCarrierFee": 0.006,
}

return _pinpoint_callback(body)


def pinpoint_delivered_callback_missing_sms_data(reference=None, timestamp=1467074434, destination="+1XXX5550100"):
body = {
"eventType": "TEXT_DELIVERED",
"eventVersion": "1.0",
"eventTimestamp": timestamp,
"isFinal": True,
"originationPhoneNumber": "+13655550100",
"destinationPhoneNumber": destination,
"messageId": reference,
"messageRequestTimestamp": timestamp,
"messageEncoding": "GSM",
"messageType": "TRANSACTIONAL",
"messageStatus": "DELIVERED",
"messageStatusDescription": "Message has been accepted by phone",
"totalMessageParts": 1,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The messageStatusDescription value 'Message has been accepted by phone' seems slightly ambiguous. Ensure this is the correct and intended description for the 'DELIVERED' status.

"totalMessagePrice": 0.00581,
"totalCarrierFee": 0.006,
}

return _pinpoint_callback(body)
Expand Down
14 changes: 14 additions & 0 deletions app/celery/process_pinpoint_receipts_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ def process_pinpoint_results(self, response):
provider_response = receipt["messageStatusDescription"]
isFinal = receipt["isFinal"]

# some of these fields might be missing in the receipt
total_message_price = receipt.get("totalMessagePrice")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a default value for total_message_price in case the key is missing in the receipt. This will ensure that the variable is always initialized.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, want it to be None if it's missing.

total_carrier_fee = receipt.get("totalCarrierFee")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a default value for total_carrier_fee in case the key is missing in the receipt. This will ensure that the variable is always initialized.

iso_country_code = receipt.get("isoCountryCode")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a default value for iso_country_code in case the key is missing in the receipt. This will ensure that the variable is always initialized.

carrier_name = receipt.get("carrierName")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a default value for carrier_name in case the key is missing in the receipt. This will ensure that the variable is always initialized.

message_encoding = receipt.get("messageEncoding")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a default value for message_encoding in case the key is missing in the receipt. This will ensure that the variable is always initialized.

origination_phone_number = receipt.get("originationPhoneNumber")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a default value for origination_phone_number in case the key is missing in the receipt. This will ensure that the variable is always initialized.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So many informative fields that we'll be able to get new insights from. 🥳


notification_status = determine_pinpoint_status(status, provider_response, isFinal)

if notification_status == NOTIFICATION_SENT:
Expand Down Expand Up @@ -88,6 +96,12 @@ def process_pinpoint_results(self, response):
notification=notification,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable total_message_price is used here but is not defined within the provided code snippet. Ensure that total_message_price is defined and assigned a value before this line.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it's define above this

status=notification_status,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable total_carrier_fee is used here but is not defined within the provided code snippet. Ensure that total_carrier_fee is defined and assigned a value before this line.

provider_response=provider_response,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable iso_country_code is used here but is not defined within the provided code snippet. Ensure that iso_country_code is defined and assigned a value before this line.

sms_total_message_price=total_message_price,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable carrier_name is used here but is not defined within the provided code snippet. Ensure that carrier_name is defined and assigned a value before this line.

sms_total_carrier_fee=total_carrier_fee,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable message_encoding is used here but is not defined within the provided code snippet. Ensure that message_encoding is defined and assigned a value before this line.

sms_iso_country_code=iso_country_code,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable origination_phone_number is used here but is not defined within the provided code snippet. Ensure that origination_phone_number is defined and assigned a value before this line.

sms_carrier_name=carrier_name,
sms_message_encoding=message_encoding,
sms_origination_phone_number=origination_phone_number,
)

if notification_status != NOTIFICATION_DELIVERED:
Expand Down
26 changes: 25 additions & 1 deletion app/dao/notifications_dao.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,18 @@ def country_records_delivery(phone_prefix):
return dlr and dlr.lower() == "yes"


def _update_notification_status(notification, status, provider_response=None, bounce_response=None):
def _update_notification_status(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function signature for _update_notification_status is getting quite long. Consider using a dictionary or a data class to pass the additional parameters to improve readability and maintainability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine the way it is but not an unreasonable comment.

notification,
status,
provider_response=None,
bounce_response=None,
sms_total_message_price=None,
sms_total_carrier_fee=None,
sms_iso_country_code=None,
sms_carrier_name=None,
sms_message_encoding=None,
sms_origination_phone_number=None,
):
status = _decide_permanent_temporary_failure(current_status=notification.status, status=status)
notification.status = status
if provider_response:
Expand All @@ -208,6 +219,19 @@ def _update_notification_status(notification, status, provider_response=None, bo
notification.feedback_subtype = bounce_response.get("feedback_subtype")
notification.ses_feedback_id = bounce_response.get("ses_feedback_id")
notification.ses_feedback_date = bounce_response.get("ses_feedback_date")
if sms_total_message_price:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a single update statement for the new fields to improve readability and maintainability. For example:

notification.sms_total_message_price = sms_total_message_price
notification.sms_total_carrier_fee = sms_total_carrier_fee
notification.sms_iso_country_code = sms_iso_country_code
notification.sms_carrier_name = sms_carrier_name
notification.sms_message_encoding = sms_message_encoding
notification.sms_origination_phone_number = sms_origination_phone_number

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I was wondering if we need these conditionals too, if a notification field is already None and we assign None again, that should be ok. But there might be a reason of doing it that way? 👀

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a pattern we use already but yeah, I can't really see a reason why it'd make a difference. I'll clean it up as the ai suggests and verify that it's still all good.

notification.sms_total_message_price = sms_total_message_price
if sms_total_carrier_fee:
notification.sms_total_carrier_fee = sms_total_carrier_fee
if sms_iso_country_code:
notification.sms_iso_country_code = sms_iso_country_code
if sms_carrier_name:
notification.sms_carrier_name = sms_carrier_name
if sms_message_encoding:
notification.sms_message_encoding = sms_message_encoding
if sms_origination_phone_number:
notification.sms_origination_phone_number = sms_origination_phone_number

dao_update_notification(notification)
return notification

Expand Down
16 changes: 16 additions & 0 deletions app/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1734,6 +1734,14 @@ class Notification(BaseModel):
ses_feedback_id = db.Column(db.String, nullable=True)
ses_feedback_date = db.Column(db.DateTime, nullable=True)

# SMS columns
sms_total_message_price = db.Column(db.Numeric(), nullable=True)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider specifying a precision and scale for the db.Numeric() type to ensure consistency in how numeric values are stored and retrieved.

sms_total_carrier_fee = db.Column(db.Numeric(), nullable=True)
sms_iso_country_code = db.Column(db.String(), nullable=True)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a length constraint to the db.String() type to ensure that the values stored in sms_iso_country_code do not exceed the expected length (e.g., 2 or 3 characters for country codes).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hrm, a good idea.

sms_carrier_name = db.Column(db.String(), nullable=True)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a length constraint to the db.String() type to ensure that the values stored in sms_carrier_name do not exceed a reasonable length.

sms_message_encoding = db.Column(db.String(), nullable=True)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a length constraint to the db.String() type to ensure that the values stored in sms_message_encoding do not exceed a reasonable length.

sms_origination_phone_number = db.Column(db.String(), nullable=True)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a length constraint to the db.String() type to ensure that the values stored in sms_origination_phone_number do not exceed a reasonable length.


CheckConstraint(
"""
CASE WHEN notification_type = 'letter' THEN
Expand Down Expand Up @@ -2045,6 +2053,14 @@ class NotificationHistory(BaseModel, HistoryModel):
ses_feedback_id = db.Column(db.String, nullable=True)
ses_feedback_date = db.Column(db.DateTime, nullable=True)

# SMS columns
sms_total_message_price = db.Column(db.Numeric(), nullable=True)
sms_total_carrier_fee = db.Column(db.Numeric(), nullable=True)
sms_iso_country_code = db.Column(db.String(), nullable=True)
sms_carrier_name = db.Column(db.String(), nullable=True)
sms_message_encoding = db.Column(db.String(), nullable=True)
sms_origination_phone_number = db.Column(db.String(), nullable=True)

CheckConstraint(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CheckConstraint should be updated to include the new SMS columns if there are any constraints that should apply to them. Currently, it only appears to handle 'letter' type notifications.

"""
CASE WHEN notification_type = 'letter' THEN
Expand Down
42 changes: 42 additions & 0 deletions migrations/versions/0461_add_pinpoint_fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
"""

Revision ID: 0461_add_pinpoint_fields
Revises: 0460_new_service_columns
Create Date: 2024-10-15 18:24:22.926597

"""
import sqlalchemy as sa
from alembic import op

revision = "0461_add_pinpoint_fields"
down_revision = "0460_new_service_columns"


def upgrade():
op.add_column("notifications", sa.Column("sms_total_message_price", sa.Float(), nullable=True))
op.add_column("notifications", sa.Column("sms_total_carrier_fee", sa.Float(), nullable=True))
op.add_column("notifications", sa.Column("sms_iso_country_code", sa.VARCHAR(), nullable=True))
op.add_column("notifications", sa.Column("sms_carrier_name", sa.VARCHAR(), nullable=True))
op.add_column("notifications", sa.Column("sms_message_encoding", sa.VARCHAR(), nullable=True))
op.add_column("notifications", sa.Column("sms_origination_phone_number", sa.VARCHAR(), nullable=True))
op.add_column("notification_history", sa.Column("sms_total_message_price", sa.Float(), nullable=True))
op.add_column("notification_history", sa.Column("sms_total_carrier_fee", sa.Float(), nullable=True))
op.add_column("notification_history", sa.Column("sms_iso_country_code", sa.VARCHAR(), nullable=True))
op.add_column("notification_history", sa.Column("sms_carrier_name", sa.VARCHAR(), nullable=True))
op.add_column("notification_history", sa.Column("sms_message_encoding", sa.VARCHAR(), nullable=True))
op.add_column("notification_history", sa.Column("sms_origination_phone_number", sa.VARCHAR(), nullable=True))


def downgrade():
op.drop_column("notifications", "sms_total_message_price")
op.drop_column("notifications", "sms_total_carrier_fee")
op.drop_column("notifications", "sms_iso_country_code")
op.drop_column("notifications", "sms_carrier_name")
op.drop_column("notifications", "sms_message_encoding")
op.drop_column("notifications", "sms_origination_phone_number")
op.drop_column("notification_history", "sms_total_message_price")
op.drop_column("notification_history", "sms_total_carrier_fee")
op.drop_column("notification_history", "sms_iso_country_code")
op.drop_column("notification_history", "sms_carrier_name")
op.drop_column("notification_history", "sms_message_encoding")
op.drop_column("notification_history", "sms_origination_phone_number")
41 changes: 37 additions & 4 deletions tests/app/celery/test_process_pinpoint_receipts_tasks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from app import statsd_client
from app.aws.mocks import (
pinpoint_delivered_callback,
pinpoint_delivered_callback_missing_sms_data,
pinpoint_failed_callback,
pinpoint_shortcode_delivered_callback,
pinpoint_successful_callback,
Expand All @@ -30,13 +31,15 @@


@pytest.mark.parametrize(
"callback, expected_response",
"callback, expected_response, origination_phone_number",
[
(pinpoint_delivered_callback, "Message has been accepted by phone"),
(pinpoint_shortcode_delivered_callback, "Message has been accepted by phone carrier"),
(pinpoint_delivered_callback, "Message has been accepted by phone", "+13655550100"),
(pinpoint_shortcode_delivered_callback, "Message has been accepted by phone carrier", "555555"),
],
)
def test_process_pinpoint_results_delivered(sample_template, notify_db, notify_db_session, callback, expected_response, mocker):
def test_process_pinpoint_results_delivered(
sample_template, notify_db, notify_db_session, callback, expected_response, origination_phone_number, mocker
):
mock_logger = mocker.patch("app.celery.process_pinpoint_receipts_tasks.current_app.logger.info")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mock_logger variable is defined but not used in the test function. Consider removing it if it's not necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is being used later in the method...

mock_callback_task = mocker.patch("app.notifications.callbacks._check_and_queue_callback_task")

Expand All @@ -56,6 +59,12 @@ def test_process_pinpoint_results_delivered(sample_template, notify_db, notify_d
assert mock_callback_task.called_once_with(get_notification_by_id(notification.id))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mock_callback_task.called_once_with assertion should be mock_callback_task.assert_called_once_with to correctly use the unittest.mock library's assertion method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh.... yeah fixing this led me to realize that the mock wasn't actually mocking the right thing... so good on ya, ai reviewer.

assert get_notification_by_id(notification.id).status == NOTIFICATION_DELIVERED
assert get_notification_by_id(notification.id).provider_response == expected_response
assert float(get_notification_by_id(notification.id).sms_total_message_price) == 0.00581
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider storing the result of get_notification_by_id(notification.id) in a variable to avoid multiple database calls and improve readability.

assert float(get_notification_by_id(notification.id).sms_total_carrier_fee) == 0.006
assert get_notification_by_id(notification.id).sms_iso_country_code == "CA"
assert get_notification_by_id(notification.id).sms_carrier_name == "Bell"
assert get_notification_by_id(notification.id).sms_message_encoding == "GSM"
assert get_notification_by_id(notification.id).sms_origination_phone_number == origination_phone_number

mock_logger.assert_called_once_with(f"Pinpoint callback return status of delivered for notification: {notification.id}")

Expand All @@ -81,6 +90,30 @@ def test_process_pinpoint_results_succeeded(sample_template, notify_db, notify_d
assert get_notification_by_id(notification.id).provider_response is None


def test_process_pinpoint_results_missing_sms_data(sample_template, notify_db, notify_db_session, mocker):
mock_callback_task = mocker.patch("app.notifications.callbacks._check_and_queue_callback_task")

notification = create_sample_notification(
notify_db,
notify_db_session,
template=sample_template,
reference="ref",
status=NOTIFICATION_SENT,
sent_by="pinpoint",
sent_at=datetime.utcnow(),
)
assert get_notification_by_id(notification.id).status == NOTIFICATION_SENT

process_pinpoint_results(pinpoint_delivered_callback_missing_sms_data(reference="ref"))

assert mock_callback_task.called_once_with(get_notification_by_id(notification.id))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method called_once_with should be assert_called_once_with to properly assert that the mock was called exactly once with the specified arguments.

assert get_notification_by_id(notification.id).status == NOTIFICATION_DELIVERED
assert float(get_notification_by_id(notification.id).sms_total_message_price) == 0.00581
assert float(get_notification_by_id(notification.id).sms_total_carrier_fee) == 0.006
assert get_notification_by_id(notification.id).sms_iso_country_code is None
assert get_notification_by_id(notification.id).sms_carrier_name is None


@pytest.mark.parametrize(
"provider_response, expected_status, should_log_warning, should_save_provider_response",
[
Expand Down
Loading