Add pinpoint fields to notifications #2326

sastels · 2024-10-15T20:06:18Z

Summary | Résumé

Add a few useful fields from the Pinpoint delivery receipts to the notification and notification_history tables.

add new fields to the database and python models
extract the fields out of the Pinpoint delivery receipts and store in the database

Related Issues | Cartes liées

https://app.zenhub.com/workspaces/notify-planning-core-6411dfb7c95fb80014e0cab0/issues/gh/cds-snc/notification-planning-core/438

Test instructions | Instructions pour tester la modification

Kinda hard to test locally :/ Figure the real test is to test in staging after we merge.

Release Instructions | Instructions pour le déploiement

None.

Reviewer checklist | Liste de vérification du réviseur

This PR does not break existing functionality.
This PR does not violate GCNotify's privacy policies.
This PR does not raise new security concerns. Refer to our GC Notify Risk Register document on our Google drive.
This PR does not significantly alter performance.
Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).

⚠ If boxes cannot be checked off before merging the PR, they should be moved to the "Release Instructions" section with appropriate steps required to verify before release. For example, changes to celery code may require tests on staging to verify that performance has not been affected.

migrations/versions/0461_add_pinpoint_fields.py

github-actions · 2024-10-24T13:36:28Z

app/aws/mocks.py

@@ -254,15 +254,38 @@ def pinpoint_shortcode_delivered_callback(reference=None, timestamp=1467074434,
        "originationPhoneNumber": "555555",


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

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

github-actions · 2024-10-24T13:36:28Z

app/aws/mocks.py

        "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,


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

github-actions · 2024-10-24T13:36:28Z

app/aws/mocks.py

+        "messageType": "TRANSACTIONAL",
+        "messageStatus": "DELIVERED",
+        "messageStatusDescription": "Message has been accepted by phone",
+        "totalMessageParts": 1,


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

github-actions · 2024-10-24T13:36:28Z

app/celery/process_pinpoint_receipts_tasks.py

@@ -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")


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.

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

github-actions · 2024-10-24T13:36:28Z

app/celery/process_pinpoint_receipts_tasks.py

@@ -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")
+        total_carrier_fee = receipt.get("totalCarrierFee")


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.

github-actions · 2024-10-24T13:36:30Z

app/models.py

+    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(


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.

github-actions · 2024-10-24T13:36:30Z

tests/app/celery/test_process_pinpoint_receipts_tasks.py

-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")


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

It is being used later in the method...

github-actions · 2024-10-24T13:36:30Z

tests/app/celery/test_process_pinpoint_receipts_tasks.py

@@ -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))


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.

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

github-actions · 2024-10-24T13:36:30Z

tests/app/celery/test_process_pinpoint_receipts_tasks.py

@@ -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))
    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


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

github-actions · 2024-10-24T13:36:30Z

tests/app/celery/test_process_pinpoint_receipts_tasks.py

+
+    process_pinpoint_results(pinpoint_delivered_callback_missing_sms_data(reference="ref"))
+
+    assert mock_callback_task.called_once_with(get_notification_by_id(notification.id))


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.

whabanks

LGTM!
✅ Migration works
✅ New values are passed to the front end from API
✅ Sending and whatnot still works fine locally

jimleroyer

Looks good! I had a few questions.

jimleroyer · 2024-10-24T18:48:40Z

app/celery/process_pinpoint_receipts_tasks.py

+        iso_country_code = receipt.get("isoCountryCode")
+        carrier_name = receipt.get("carrierName")
+        message_encoding = receipt.get("messageEncoding")
+        origination_phone_number = receipt.get("originationPhoneNumber")


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

jimleroyer · 2024-10-24T18:55:03Z

app/dao/notifications_dao.py

@@ -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:


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? 👀

jimleroyer · 2024-10-24T18:58:57Z

migrations/versions/0462_add_pinpoint_fields.py

+    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))


We should update the batch process too. I wonder if we can do it in a different PR or if that will have unintended side-effects when the archival jobs will run. Definitely something to test.

I looked at that but as far as I can tell it should pull all the fields that we've added to the history table automatically

We should definitely test it though. I'll see if I can test it locally at least. On staging I see we have a minimum 3 days for a retention period.

tested locally

sent an SMS,

edited the database to give those fields values since we don't process the AWS receipts locally

also set the dates for the sms in the database to be a month ago

edited the schedule for the archiving task delete-sms-notifications so it would be scheduled in a couple minutes

reran celery locally and waited for it to run

checked the database - sms with correct fields now in history table, and removed from notifications table 🎉

jimleroyer · 2024-10-25T14:46:32Z

Approved. Related to these items:

This PR does not significantly alter performance.

Steve will run performance tests in staging to test the changes and report back on it.

Additional required documentation resulting of these changes is covered (such as the README, setup instructions, a related ADR or the technical documentation).

We should regenerate the db doc hosted at
https://cds-snc.github.io/notification-documentation-database/

Hence these two items are not checks for this PR but we will take actions to address these.

sastels added 4 commits October 15, 2024 14:58

add new fields to database

84bb526

tweak new col names and change receipt processing task

3d6846e

new test

fdbeee7

format

46ec67f

sastels mentioned this pull request Oct 15, 2024

Add Pinpoint costing info to notifications table cds-snc/notification-planning-core#438

Closed

5 tasks

github-advanced-security bot found potential problems Oct 15, 2024

View reviewed changes

migrations/versions/0461_add_pinpoint_fields.py Fixed Show resolved Hide resolved

add test for missing sms data

5eb0117

sastels force-pushed the add-pinpoint-fields-to-notifications branch from 3b248e6 to 5eb0117 Compare October 15, 2024 20:24

sastels marked this pull request as ready for review October 15, 2024 20:33

sastels requested review from jimleroyer and andrewleith October 15, 2024 20:33

sastels and others added 2 commits October 24, 2024 09:34

Merge branch 'main' into add-pinpoint-fields-to-notifications

96893e0

ai [review] plz

03feb77

github-actions bot reviewed Oct 24, 2024

View reviewed changes

sastels added 2 commits October 24, 2024 10:59

move new migrations to 0462

e5cd121

fixup tests

6d6459a

whabanks approved these changes Oct 24, 2024

View reviewed changes

jimleroyer reviewed Oct 24, 2024

View reviewed changes

few other ai suggestions I missed

8fdf60c

sastels requested a review from jimleroyer October 25, 2024 14:20

jimleroyer approved these changes Oct 25, 2024

View reviewed changes

sastels merged commit dfcc4fd into main Oct 25, 2024
5 checks passed

sastels deleted the add-pinpoint-fields-to-notifications branch October 25, 2024 14:55

sastels mentioned this pull request Oct 28, 2024

add new sms fields to QuickSight dataset cds-snc/notification-terraform#1629

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pinpoint fields to notifications #2326

Add pinpoint fields to notifications #2326

sastels commented Oct 15, 2024 •

edited by jimleroyer

Loading

github-actions bot Oct 24, 2024

sastels Oct 24, 2024

github-actions bot Oct 24, 2024

github-actions bot Oct 24, 2024

github-actions bot Oct 24, 2024

sastels Oct 24, 2024

github-actions bot Oct 24, 2024

github-actions bot Oct 24, 2024

github-actions bot Oct 24, 2024

sastels Oct 24, 2024

github-actions bot Oct 24, 2024

sastels Oct 24, 2024

github-actions bot Oct 24, 2024

github-actions bot Oct 24, 2024

whabanks left a comment

jimleroyer left a comment

jimleroyer Oct 24, 2024

jimleroyer Oct 24, 2024

jimleroyer Oct 24, 2024

sastels Oct 24, 2024

sastels Oct 24, 2024

jimleroyer commented Oct 25, 2024

		@@ -254,15 +254,38 @@ def pinpoint_shortcode_delivered_callback(reference=None, timestamp=1467074434,
		"originationPhoneNumber": "555555",

		@@ -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))


		process_pinpoint_results(pinpoint_delivered_callback_missing_sms_data(reference="ref"))

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

Add pinpoint fields to notifications #2326

Add pinpoint fields to notifications #2326

Conversation

sastels commented Oct 15, 2024 • edited by jimleroyer Loading

Summary | Résumé

Related Issues | Cartes liées

Test instructions | Instructions pour tester la modification

Release Instructions | Instructions pour le déploiement

Reviewer checklist | Liste de vérification du réviseur

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

github-actions bot Oct 24, 2024

Choose a reason for hiding this comment

whabanks left a comment

Choose a reason for hiding this comment

jimleroyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimleroyer commented Oct 25, 2024

sastels commented Oct 15, 2024 •

edited by jimleroyer

Loading