code review feedback

app/__init__.py

@@ -98,6 +98,7 @@ def create_app(application):
     aws_ses_stub_client.init_app(
         stub_url=application.config['SES_STUB_URL']
     )
+    aws_cloudwatch_client.init_app(application)
     # If a stub url is provided for SES, then use the stub client rather than the real SES boto client
     email_clients = [aws_ses_stub_client] if application.config['SES_STUB_URL'] else [aws_ses_client]
     notification_provider_clients.init_app(

app/celery/provider_tasks.py

@@ -4,8 +4,7 @@ from zoneinfo import ZoneInfo
 from flask import current_app
 from sqlalchemy.orm.exc import NoResultFound
 
-from app import notify_celery
+from app import aws_cloudwatch_client, notify_celery
-from app.clients.cloudwatch.aws_cloudwatch import AwsCloudwatchClient
 from app.clients.email import EmailClientNonRetryableException
 from app.clients.email.aws_ses import AwsSesClientThrottlingSendRateException
 from app.clients.sms import SmsClientResponseException
@@ -21,12 +20,18 @@ from app.models import (
 )
 
 
-@notify_celery.task(bind=True, name="check_sms_delivery_receipt", max_retries=3, default_retry_delay=300)
+@notify_celery.task(bind=True, name="check_sms_delivery_receipt", max_retries=48, default_retry_delay=300)
 def check_sms_delivery_receipt(self, message_id, notification_id):
-    current_app.logger.warning(f"CHECKING DELIVERY RECEIPT for {message_id} {notification_id}")
+    """
-    cloudwatch_client = AwsCloudwatchClient()
+    This is called after deliver_sms to check the status of the message. This uses the same number of
-    cloudwatch_client.init_app(current_app)
+    retries and the same delay period as deliver_sms.  In addition, this fires five minutes after
-    status, provider_response = cloudwatch_client.check_sms(message_id, notification_id)
+    deliver_sms initially. So the idea is that most messages will succeed and show up in the logs quickly.
+    Other message will resolve successfully after a retry or to.  A few will fail but it will take up to
+    4 hours to know for sure.  The call to check_sms will raise an exception if neither a success nor a
+    failure appears in the cloudwatch logs, so this should keep retrying until the log appears, or until
+    we run out of retries.
+    """
+    status, provider_response = aws_cloudwatch_client.check_sms(message_id, notification_id)
     if status == 'success':
         status = NOTIFICATION_SENT
     else:

app/clients/cloudwatch/__init__.py

@@ -1,25 +0,0 @@
-from app.clients import Client, ClientException
-
-
-class CloudwatchClientResponseException(ClientException):
-    """
-    Base Exception for SmsClientsResponses
-    """
-
-    def __init__(self, message):
-        self.message = message
-
-    def __str__(self):
-        return "Message {}".format(self.message)
-
-
-class CloudwatchClient(Client):
-    """
-    Base Cloudwatch client for checking sms.
-    """
-
-    def init_app(self, *args, **kwargs):
-        raise NotImplementedError("TODO Need to implement.")
-
-    def check_sms(self, *args, **kwargs):
-        raise NotImplementedError("TODO Need to implement.")

app/clients/cloudwatch/aws_cloudwatch.py

@@ -5,11 +5,11 @@ import time
 
 from boto3 import client
 
-from app.clients.cloudwatch import CloudwatchClient
+from app.clients import Client
 from app.cloudfoundry_config import cloud_config
 
 
-class AwsCloudwatchClient(CloudwatchClient):
+class AwsCloudwatchClient(Client):
     """
     This client is responsible for retrieving sms delivery receipts from cloudwatch.
     """
@@ -21,7 +21,7 @@ class AwsCloudwatchClient(CloudwatchClient):
             aws_access_key_id=cloud_config.sns_access_key,
             aws_secret_access_key=cloud_config.sns_secret_key
         )
-        super(CloudwatchClient, self).__init__(*args, **kwargs)
+        super(Client, self).__init__(*args, **kwargs)
         self.current_app = current_app
         self._valid_sender_regex = re.compile(r"^\+?\d{5,14}$")
 
@@ -61,14 +61,8 @@ class AwsCloudwatchClient(CloudwatchClient):
         return all_log_events
 
     def check_sms(self, message_id, notification_id):
-        """
+        # TODO this clumsy approach to getting the account number will be fixed as part of notify-api #258
-        Go through the cloudwatch logs, filtering by message id.  Check the success logs first.  If we find
+        account_number = cloud_config.ses_domain_arn
-        the message id there, we are done.  Otherwise check the failure logs.  If we don't find the message
-        in the success or failure logs, raise an exception.  This method is called on a five minute delay,
-        which is presumably enough time for the cloudwatch log to be populated.
-        """
-        # TODO presumably there is a better way to get the aws account number
-        account_number = os.getenv("SES_DOMAIN_ARN")
         account_number = account_number.replace('arn:aws:ses:us-west-2:', '')
         account_number = account_number.split(":")
         account_number = account_number[0]

app/cloudfoundry_config.py

@@ -39,6 +39,15 @@ class CloudfoundryConfig:
             domain_arn = getenv('SES_DOMAIN_ARN', 'dev.notify.gov')
         return domain_arn.split('/')[-1]
 
+    # TODO remove this after notifications-api #258
+    @property
+    def ses_domain_arn(self):
+        try:
+            domain_arn = self._ses_credentials('domain_arn')
+        except KeyError:
+            domain_arn = getenv('SES_DOMAIN_ARN', 'dev.notify.gov')
+        return domain_arn
+
     @property
     def ses_region(self):
         try: