#2011 - Eliminate Null Status Reason #2083

k-macmillan · 2024-10-25T20:29:41Z

Description

Refactored process_delivery_status and process_pinpoint_results. The code would update status or status_reason in multiple functions, but we need the branching logic removed from the app and added to the query, so that has been done.

The query now accepts any transition, and the app code determines from a given state, what state we would like to be in. The app code passes it to the query, so if an update came between the two (race condition), the DB won't transition to an invalid state. If a new update comes in there is no way for it to go "backwards" in status accidentally.

issue #2011

How Has This Been Tested?

New unit tests written (over 100, some new, some parameterized). Unit tests cover every possible status transition. Deployed, regression passed.

Checklist

k-macmillan · 2024-10-28T13:45:42Z

app/dao/notifications_dao.py

-def _decide_permanent_temporary_failure(
-    current_status,
-    status,
-):
-    # Firetext will send pending, then send either success or fail.
-    # If we go from pending to delivered we need to set failure type as temporary-failure
-    if current_status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE:
-        status = NOTIFICATION_TEMPORARY_FAILURE
-    return status
-
-


Nothing ever called this code.

cris-oddball · 2024-10-29T16:25:34Z

app/celery/process_delivery_status_result_tasks.py

 def process_delivery_status(
-    self,
+    self: Task,
    event: CeleryEvent,
-) -> bool:
+) -> None:
    """
-    This is a Celery task for updating the delivery status of a notification.
+    This is a Celery task for updating the delivery status of a Twilio notification.


should this be more explicitly named that it processed only Twilio delivery statuses?

I agree. It should be process_twilio_delivery_status.

tests/app/celery/test_process_pinpoint_inbound_sms.py

app/celery/process_delivery_status_result_tasks.py

MackHalliday · 2024-10-29T18:38:07Z

app/celery/process_delivery_status_result_tasks.py


-    body = sqs_message.get('body')
-    current_app.logger.info('retrieved delivery status body: %s', body)
+    notification_platform_status: SmsStatusRecord = get_notification_platform_status(provider, sqs_message.get('body'))


NIT: (and likely not apart of this PR) The code path for get_notification_platform_status is dead end and will result in a NotImplementedError error.

@k-macmillan I'd like to talk about how the notification_platform_status variable is being used further down the code path. Is this being populated as expected?

As I follow the code - this will hit a NotImplementedError.

MackHalliday · 2024-10-29T20:28:36Z

app/dao/notifications_dao.py

-        incoming_status_reason=new_status_reason,
-    )
+    if notification_type == SMS_TYPE:
+        stmt = (


I spent some time looking at this query to see if the efficient could be improved. After some review, I believe the current query is sufficient.

I looked at updating sms_conditions to return a SQL statement that uses CASE instead of a series of OR statements. But could not find anything conclusive that CASE is more performative.

Thank you for checking!

kalbfled · 2024-10-29T17:34:42Z

app/celery/process_delivery_status_result_tasks.py

 def process_delivery_status(
-    self,
+    self: Task,
    event: CeleryEvent,
-) -> bool:
+) -> None:
    """
-    This is a Celery task for updating the delivery status of a notification.
+    This is a Celery task for updating the delivery status of a Twilio notification.


I agree. It should be process_twilio_delivery_status.

kalbfled · 2024-10-29T17:41:09Z

app/celery/process_delivery_status_result_tasks.py

-) -> dict:
+def get_notification_platform_status(
+    provider: SmsClient,
+    body: str | dict[str, str],


You call this upstream as sqs_message.get('body'), which implies it might be None.

Good callout. Wouldn't have caused an issue (caught and raised app/clients/sms/twilio.py:254) but could have been confusing.

Modified the upstream so the function signature doesn't need to change.

kalbfled · 2024-10-29T18:29:48Z

app/celery/process_pinpoint_receipt_tasks.py

-        raise AutoRetryException(f'Found {type(e).__name__}, autoretrying...')
+        current_app.logger.exception('Unable to decode the incoming pinpoint message')
+        statsd_client.incr('clients.sms.pinpoint.status_update.error')
+        raise NonRetryableException(f'Found {type(e).__name__}, {UNABLE_TO_TRANSLATE}')


Suggested change

raise NonRetryableException(f'Found {type(e).__name__}, {UNABLE_TO_TRANSLATE}')

raise NonRetryableException(UNABLE_TO_TRANSLATE) from e

kalbfled · 2024-10-29T18:41:03Z

app/clients/sms/aws_pinpoint.py

 from app.exceptions import InvalidProviderException


 class AwsPinpointException(SmsClientResponseException):
    pass


+# This is not an exhaustive list of all possible record statuses.  See the documentation linked below.
+_sms_record_status_mapping = {


In general, when you declare things like this, please comment. I think this is a mapping to (status, status_reason), but I would have to see how it's used to infer if that's correct. I shouldn't need to do that.

kalbfled · 2024-10-29T18:54:14Z

app/clients/sms/aws_pinpoint.py

+        notification_platform_status = SmsStatusRecord(
+            None,
+            pinpoint_attributes['message_id'],
+            status,
+            status_reason,
+            PINPOINT_PROVIDER,
+            pinpoint_attributes['number_of_message_parts'],
+            delivery_status_message['metrics']['price_in_millicents_usd'],
+            datetime.fromtimestamp(delivery_status_message['event_timestamp'] / 1000),
+        )
+
+        return notification_platform_status


Suggested change

notification_platform_status = SmsStatusRecord(

None,

pinpoint_attributes['message_id'],

status,

status_reason,

PINPOINT_PROVIDER,

pinpoint_attributes['number_of_message_parts'],

delivery_status_message['metrics']['price_in_millicents_usd'],

datetime.fromtimestamp(delivery_status_message['event_timestamp'] / 1000),

)

return notification_platform_status

return SmsStatusRecord(

None,

pinpoint_attributes['message_id'],

status,

status_reason,

PINPOINT_PROVIDER,

pinpoint_attributes['number_of_message_parts'],

delivery_status_message['metrics']['price_in_millicents_usd'],

datetime.fromtimestamp(delivery_status_message['event_timestamp'] / 1000),

)

kalbfled · 2024-10-29T19:51:23Z

app/dao/notifications_dao.py

@@ -341,7 +446,7 @@ def dao_update_notification_by_id(
    **kwargs,
 ) -> Optional[Notification]:
    """
-    Update the notification by ID, ensure kwargs paramaters are named appropriately according to the notification model.
+    Update the SMS notification by ID, ensure kwargs paramaters are named appropriately according to the notification model.


change function name

The docstring was wrong, oops.

kalbfled · 2024-10-29T20:00:56Z

app/models.py

-SERVICE_CALLBACK_TYPES = [DELIVERY_STATUS_CALLBACK_TYPE, COMPLAINT_CALLBACK_TYPE, INBOUND_SMS_CALLBACK_TYPE]
-
+# models.py only constants
+NOTIFICATION_STATUS_TYPES_NON_BILLABLE = list(set(NOTIFICATION_STATUS_TYPES) - set(NOTIFICATION_STATUS_TYPES_BILLABLE))


Slightly more performant:

Suggested change

NOTIFICATION_STATUS_TYPES_NON_BILLABLE = list(set(NOTIFICATION_STATUS_TYPES) - set(NOTIFICATION_STATUS_TYPES_BILLABLE))

NOTIFICATION_STATUS_TYPES_NON_BILLABLE = tuple(frozenset(NOTIFICATION_STATUS_TYPES) - frozenset(NOTIFICATION_STATUS_TYPES_BILLABLE))

>>> tuple(frozenset([1,2,3]) - frozenset([4,5,6])) (1, 2, 3)

Removed. It's not used.

kalbfled · 2024-10-29T20:02:29Z

app/models.py

All of the constants defined as lists should instead be tuples, which are more performant.

kalbfled · 2024-10-29T20:05:51Z

app/notifications/utils.py


+def confirm_subscription(confirmation_request):


Annotation? It quacks like a dictionary.

Letter code, not updating.

kalbfled · 2024-10-29T20:28:17Z

tests/app/conftest.py

@@ -2462,6 +2467,23 @@ def mock_va_profile_response():
        return json.load(f)


+@pytest.fixture
+def x_minutes_ago():


You created a function 5_minutes_ago somewhere above. Should you use this instead?

Got rid of five_minutes_ago. I meant to replace them all with x_minutes_ago because it's more versatile. Cut out the five_minutes_ago method.

cris-oddball

Found where the regression failed.

cris-oddball · 2024-10-30T14:35:45Z

app/constants.py

+DELIVERY_STATUS_CALLBACK_TYPE = 'delivery_status'
+COMPLAINT_CALLBACK_TYPE = 'complaint'
+INBOUND_SMS_CALLBACK_TYPE = 'inbound_sms'
+SERVICE_CALLBACK_TYPES = (DELIVERY_STATUS_CALLBACK_TYPE, COMPLAINT_CALLBACK_TYPE, INBOUND_SMS_CALLBACK_TYPE)


This fails the regression for POST payloads with an invalid value for callback_type. The code now returns a tuple in the error message, not a list. We can change the regression or we can change this back to a list. Please let me know.

These two are only used in validation schema and the validation error raised used string interpolation to add the tuple/list to the validation error, so if we added or removed any it'd also break. Since it's only used in one spot on an error I cast the tuple to a list to maintain backwards compatibility. Also walked ALL validation errors raised in the code, looking for any more and none were found (other than the channels you pointed out).

cris-oddball · 2024-10-30T14:36:11Z

app/constants.py

+SERVICE_CALLBACK_TYPES = (DELIVERY_STATUS_CALLBACK_TYPE, COMPLAINT_CALLBACK_TYPE, INBOUND_SMS_CALLBACK_TYPE)
+WEBHOOK_CHANNEL_TYPE = 'webhook'
+QUEUE_CHANNEL_TYPE = 'queue'
+CALLBACK_CHANNEL_TYPES = (WEBHOOK_CHANNEL_TYPE, QUEUE_CHANNEL_TYPE)


This fails the regression for POST payloads with an invalid value for callback_channel. The code now returns a tuple in the error message, not a list. We can change the regression or we can change this back to a list. Please let me know.

Reverted this change and the service_callback_types change. Validation errors were not backwards compatible.

kalbfled · 2024-10-30T18:37:07Z

app/service/service_data.py

I don't insist on changing anything in this file, but didn't we say we're going to use "|" rather than "Union"?

We lost consistency at some point.

For va-enp-api we are not using Union nor Optional.

k-macmillan added 2 commits October 25, 2024 16:25

Updated constants. Revamped sms delivery status

d556011

merge

7f1fc56

k-macmillan added the minor label Oct 25, 2024

k-macmillan self-assigned this Oct 25, 2024

k-macmillan added 2 commits October 25, 2024 16:49

Fixed import error

52b94c6

Updated tests, 100% covered

2fbbb8f

k-macmillan temporarily deployed to dev October 28, 2024 13:19 — with GitHub Actions Inactive

k-macmillan temporarily deployed to dev October 28, 2024 13:40 — with GitHub Actions Inactive

k-macmillan commented Oct 28, 2024

View reviewed changes

k-macmillan added 7 commits October 28, 2024 10:02

Updated logging

8a64f75

Added provider_updated_at tracking

db6334e

Updated SmsStatusRecord test bodies

9af324f

Updated dict

fa61086

Removed until tests are updated

b894bd8

Added run_tests to gitignore

0323b1d

Resolved timestamp issues

fb9fc99

k-macmillan temporarily deployed to dev October 28, 2024 16:19 — with GitHub Actions Inactive

k-macmillan added 2 commits October 28, 2024 13:13

Updated to allow for updated date info from providers

0f5939f

Removed unnecessary line

041a3f3

k-macmillan temporarily deployed to dev October 28, 2024 17:31 — with GitHub Actions Inactive

Added test case, fixed format string

02ae683

k-macmillan temporarily deployed to dev October 28, 2024 19:36 — with GitHub Actions Inactive

k-macmillan marked this pull request as ready for review October 28, 2024 19:53

k-macmillan requested a review from a team as a code owner October 28, 2024 19:53

cris-oddball reviewed Oct 29, 2024

View reviewed changes

k-macmillan commented Oct 29, 2024

View reviewed changes

tests/app/celery/test_process_pinpoint_inbound_sms.py Show resolved Hide resolved

k-macmillan commented Oct 29, 2024

View reviewed changes

app/celery/process_delivery_status_result_tasks.py Outdated Show resolved Hide resolved

k-macmillan commented Oct 29, 2024

View reviewed changes

app/celery/process_delivery_status_result_tasks.py Show resolved Hide resolved

k-macmillan commented Oct 29, 2024

View reviewed changes

app/celery/process_delivery_status_result_tasks.py Show resolved Hide resolved

MackHalliday reviewed Oct 29, 2024

View reviewed changes

kalbfled requested changes Oct 29, 2024

View reviewed changes

MackHalliday mentioned this pull request Oct 29, 2024

BUG: Missing SID for app: VETEXT in Prod #2019

Closed

6 tasks

Changed lists to tuples and function names updated

54ea917

k-macmillan had a problem deploying to dev October 30, 2024 12:41 — with GitHub Actions Failure

cris-oddball reviewed Oct 30, 2024

View reviewed changes

cast tuple to list for compatibility

4b8f07e

k-macmillan had a problem deploying to dev October 30, 2024 17:23 — with GitHub Actions Failure

Reverted two tuple changes

954b916

k-macmillan temporarily deployed to dev October 30, 2024 18:12 — with GitHub Actions Inactive

k-macmillan requested review from cris-oddball and kalbfled October 30, 2024 18:24

kalbfled approved these changes Oct 30, 2024

View reviewed changes

cris-oddball approved these changes Oct 30, 2024

View reviewed changes

k-macmillan merged commit 0a5118a into main Oct 30, 2024
13 checks passed

k-macmillan deleted the 2011-eliminate-null-status-reason branch October 30, 2024 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2011 - Eliminate Null Status Reason #2083

#2011 - Eliminate Null Status Reason #2083

k-macmillan commented Oct 25, 2024 •

edited

Loading

k-macmillan Oct 28, 2024

cris-oddball Oct 29, 2024

kalbfled Oct 29, 2024

MackHalliday Oct 29, 2024 •

edited

Loading

MackHalliday Oct 29, 2024 •

edited

Loading

MackHalliday Oct 29, 2024 •

edited

Loading

k-macmillan Oct 29, 2024

kalbfled Oct 29, 2024

kalbfled Oct 29, 2024

k-macmillan Oct 29, 2024

kalbfled Oct 29, 2024

kalbfled Oct 29, 2024

kalbfled Oct 29, 2024

kalbfled Oct 29, 2024

k-macmillan Oct 29, 2024

kalbfled Oct 29, 2024

k-macmillan Oct 30, 2024

kalbfled Oct 29, 2024

kalbfled Oct 29, 2024

k-macmillan Oct 29, 2024

kalbfled Oct 29, 2024

k-macmillan Oct 29, 2024

cris-oddball left a comment

cris-oddball Oct 30, 2024

k-macmillan Oct 30, 2024

cris-oddball Oct 30, 2024

k-macmillan Oct 30, 2024

kalbfled Oct 30, 2024

k-macmillan Oct 30, 2024

	raise NonRetryableException(f'Found {type(e).__name__}, {UNABLE_TO_TRANSLATE}')
	raise NonRetryableException(UNABLE_TO_TRANSLATE) from e

	NOTIFICATION_STATUS_TYPES_NON_BILLABLE = list(set(NOTIFICATION_STATUS_TYPES) - set(NOTIFICATION_STATUS_TYPES_BILLABLE))
	NOTIFICATION_STATUS_TYPES_NON_BILLABLE = tuple(frozenset(NOTIFICATION_STATUS_TYPES) - frozenset(NOTIFICATION_STATUS_TYPES_BILLABLE))

#2011 - Eliminate Null Status Reason #2083

#2011 - Eliminate Null Status Reason #2083

Conversation

k-macmillan commented Oct 25, 2024 • edited Loading

Description

How Has This Been Tested?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MackHalliday Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

MackHalliday Oct 29, 2024 • edited Loading

Choose a reason for hiding this comment

MackHalliday Oct 29, 2024 • edited Loading

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

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

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

cris-oddball 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

Choose a reason for hiding this comment

k-macmillan commented Oct 25, 2024 •

edited

Loading

MackHalliday Oct 29, 2024 •

edited

Loading

MackHalliday Oct 29, 2024 •

edited

Loading

MackHalliday Oct 29, 2024 •

edited

Loading