Part 2 of 2: Test updates for single database setup. #1436

Co-authored-by: Kyle MacMillan <[email protected]> Co-authored-by: Michael Wellman <[email protected]>
department-of-veterans-affairs · Feb 5, 2024 · 97ef4da · 97ef4da
1 parent 2c7622f
commit 97ef4da
Show file tree

Hide file tree

Showing 144 changed files with 9,718 additions and 9,504 deletions.
diff --git a/.gitignore b/.gitignore
@@ -91,3 +91,6 @@ user_flows_exit_code.txt
 # These files can get written in the notification_api directory if you run bash commands on a container with read-write access.
 .bash_history
 .python_history
+
+# pdb
+.local/
diff --git a/.talismanrc b/.talismanrc
@@ -47,4 +47,27 @@ fileignoreconfig:
   checksum: a6b9dbff04ca357fed37bb800544195355555c5b5bc5f14e30965121403e5907
 - filename: documents/postman/staging-simplified.postman_environment.json
   checksum: d69f7b3dfd46f9d8fe17c1ec8429fa1a90cd8acea452e2de2f0936f1c0013474
-version: "1.0"
+- filename: app/celery/letters_pdf_tasks.py
+  checksum: 3430294d1d4e37f8886c3103557355d4c89c13bf1186a97709b1cfb86b78b5a3
+- filename: tests/app/template_statistics/test_rest.py
+  checksum: 2cc46d9785849898d614a2d5502cd4934ce54ba85264d0f74f73b7d2952ab78d
+- filename: tests/__init__.py
+  checksum: c95d2ef8fe7b19d074f64070a078b3e44e5fe9500b5601eab93a44e09c7fceac
+- filename: tests/app/conftest.py
+  checksum: 171620a2805d6220305e421cc157be21e8bce7de848f4dc4c71b1136f52927e8
+- filename: tests/app/test_model.py
+  checksum: 4bb0dcb3d87c8a0d2f86cc8b0c110f3d9755a5e589e91d164d79a198607ebeec
+- filename: tests/app/v2/notifications/test_post_notifications.py
+  checksum: e3ec08bd7034c77354f90c7a04b45d43a38e78fff187836ec403d88c1c71c3dc
+- filename: tests/app/notifications/rest/test_send_notification.py
+  checksum: 7ce2246012c5bb46b104c13fa81eb08b4fdec41843fd18e57c34290f6160176f
+- filename: tests/app/notifications/test_process_notifications.py
+  checksum: 98a2894e5d1ed716089cddfed3cecd90d6c968b679be23c3c66d4353d51b9117
+- filename: tests/app/dao/test_ft_billing_dao.py
+  checksum: a6b03b34c13501b03e4f091cf092c4561bf7bf038eca6944a4615a87cde67068
+- filename: app/celery/v3/notification_tasks.py
+  checksum: 968d614d0e6732db265dd2eb448cc566386a7edd55c96575b12c395c6f41a3a4
+- filename: tests/app/dao/test_login_event_dao.py
+  checksum: 6ff592b8e416eb2481fe0733c01d9c91f4c409a878414f8581aefea7f675bc2d
+- filename: tests/app/dao/test_api_key_dao.py
+  checksum: 9a97adf107dbbf0d821a5d210f19b55a27c8b22d0593be3224a10770cd78d4e1
diff --git a/app/__init__.py b/app/__init__.py
@@ -99,13 +99,15 @@ def create_app(application):
     notify_environment = os.getenv('NOTIFY_ENVIRONMENT', 'development')
 
     application.config.from_object(configs[notify_environment])
-    if notify_environment == "test":
+    if notify_environment == 'test':
         # Set the read-db to be the same as the write/default instance.
-        application.config["SQLALCHEMY_BINDS"] = {"read-db": application.config["SQLALCHEMY_DATABASE_URI"]}
-        assert application.config["SQLALCHEMY_DATABASE_URI"].endswith("test_notification_api"), \
-            "Don't run tests against the main writer database."
-        assert application.config["SQLALCHEMY_BINDS"]["read-db"].endswith("test_notification_api"), \
-            "Don't run tests against the main reader database."
+        application.config['SQLALCHEMY_BINDS'] = {'read-db': application.config['SQLALCHEMY_DATABASE_URI']}
+        assert application.config['SQLALCHEMY_DATABASE_URI'].endswith(
+            'test_notification_api'
+        ), "Don't run tests against the main writer database."
+        assert application.config['SQLALCHEMY_BINDS']['read-db'].endswith(
+            'test_notification_api'
+        ), "Don't run tests against the main reader database."
 
     application.config['NOTIFY_APP_NAME'] = application.name
     init_app(application)

diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py
@@ -107,7 +107,6 @@ def timeout_notifications():
     technical_failure_notifications, temporary_failure_notifications = dao_timeout_notifications(
         current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')
     )
-
     notifications = technical_failure_notifications + temporary_failure_notifications
     for notification in notifications:
         # queue callback task only if the service_callback_api exists

diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py
@@ -271,10 +271,7 @@ def process_ses_smtp_results(
                     template_version=template.version,
                     recipient=recipient,
                     service_id=service.id,
-                    personalisation={
-                        'subject': headers["subject"],
-                        'message': message
-                    },
+                    personalisation={'subject': headers['subject'], 'message': message},
                     notification_type=EMAIL_TYPE,
                     api_key_id=None,
                     key_type=KEY_TYPE_NORMAL,

diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py
@@ -368,17 +368,15 @@ def _get_dynamodb_comp_pen_messages(table, message_limit: int) -> list:
     :return: a list of entries from the table that have not been processed yet
     """
 
-    results = table.scan(
-        FilterExpression=boto3.dynamodb.conditions.Attr('is_processed').eq(False),
-        Limit=message_limit
-    )
+    results = table.scan(FilterExpression=boto3.dynamodb.conditions.Attr('is_processed').eq(False), Limit=message_limit)
 
     items: list = results.get('Items')
 
     if items is None:
         current_app.logger.critical(
             'Error in _get_dynamodb_comp_pen_messages trying to read "Items" from dynamodb table scan result. '
-            'Returned results does not include "Items" - results: %s', results
+            'Returned results does not include "Items" - results: %s',
+            results,
         )
         return []
 
@@ -387,117 +385,9 @@ def _get_dynamodb_comp_pen_messages(table, message_limit: int) -> list:
         results = table.scan(
             FilterExpression=boto3.dynamodb.conditions.Attr('is_processed').eq(False),
             Limit=message_limit,
-            ExclusiveStartKey=results['LastEvaluatedKey']
+            ExclusiveStartKey=results['LastEvaluatedKey'],
         )
 
         items.extend(results['Items'])
 
     return items[:message_limit]
-
-
-@notify_celery.task(name='send-scheduled-comp-and-pen-sms')
-@statsd(namespace='tasks')
-def send_scheduled_comp_and_pen_sms():
-    if not is_feature_enabled(FeatureFlag.COMP_AND_PEN_MESSAGES_ENABLED):
-        current_app.logger.warning('Attempted to run send_scheduled_comp_and_pen_sms task, but feature flag disabled.')
-        return
-
-    # this is the agreed upon message per minute limit
-    messages_per_min = 3000
-    dynamodb_table_name = current_app.config['COMP_AND_PEN_DYNAMODB_TABLE_NAME']
-    service_id = current_app.config['COMP_AND_PEN_SERVICE_ID']
-    template_id = current_app.config['COMP_AND_PEN_TEMPLATE_ID']
-
-    # TODO: utils #146 - Debug messages currently don't show up in cloudwatch, requires a configuration change
-    current_app.logger.debug('send_scheduled_comp_and_pen_sms connecting to dynamodb')
-
-    # connect to dynamodb table
-    dynamodb = boto3.resource('dynamodb')
-    table = dynamodb.Table(dynamodb_table_name)
-
-    # get messages to send
-    try:
-        comp_and_pen_messages: list = _get_dynamodb_comp_pen_messages(table, messages_per_min)
-    except Exception as e:
-        current_app.logger.critical(
-            'Exception trying to scan dynamodb table for send_scheduled_comp_and_pen_sms exception_type: %s - '
-            'exception_message: %s', type(e), e
-        )
-        return
-
-    current_app.logger.debug('send_scheduled_comp_and_pen_sms list of items from dynamodb: %s', comp_and_pen_messages)
-
-    # stop if there are no messages
-    if not comp_and_pen_messages:
-        current_app.logger.info(
-            'No Comp and Pen messages to send via send_scheduled_comp_and_pen_sms task. Exiting task.')
-        return
-
-    try:
-        service: Service = dao_fetch_service_by_id(service_id)
-        template: Template = dao_get_template_by_id(template_id)
-    except NoResultFound as e:
-        current_app.logger.error(
-            'No results found in task send_scheduled_comp_and_pen_sms attempting to lookup service or template. Exiting'
-            ' - exception: %s', e)
-        return
-    except Exception as e:
-        current_app.logger.critical(
-            'Error in task send_scheduled_comp_and_pen_sms attempting to lookup service or template Exiting - '
-            'exception: %s', e)
-        return
-
-    # send messages and update entries in dynamodb table
-    for item in comp_and_pen_messages:
-        current_app.logger.info(
-            'sending - item from dynamodb - vaprofile_id: %s | participant_id: %s | payment_id: %s',
-            item.get('vaprofile_id'), item.get('participant_id'), item.get('payment_id')
-        )
-
-        try:
-            # call generic method to send messages
-            send_notification_bypass_route(
-                service=service,
-                template=template,
-                notification_type=SMS_TYPE,
-                personalisation={'paymentAmount': int(item.get('paymentAmount'))},
-                sms_sender_id=service.get_default_sms_sender_id(),
-                recipient_item={
-                    'id_type': IdentifierType.VA_PROFILE_ID.value,
-                    'id_value': str(item.get('vaprofile_id'))
-                },
-            )
-        except Exception as e:
-            current_app.logger.critical(
-                'Error attempting to send Comp and Pen notification with send_scheduled_comp_and_pen_sms | item from '
-                'dynamodb - vaprofile_id: %s | participant_id: %s | payment_id: %s | exception_type: %s - '
-                'exception: %s',
-                item.get('vaprofile_id'), item.get('participant_id'), item.get('payment_id'), type(e), e
-            )
-        else:
-            current_app.logger.info(
-                'sent to queue, updating - item from dynamodb - vaprofile_id: %s | participant_id: %s | payment_id: %s',
-                item.get('vaprofile_id'), item.get('participant_id'), item.get('payment_id')
-            )
-
-        # update dynamodb entries
-        try:
-            updated_item = table.update_item(
-                Key={
-                    'participant_id': item.get('participant_id'),
-                    'payment_id': item.get('payment_id')
-                },
-                UpdateExpression='SET is_processed = :val',
-                ExpressionAttributeValues={
-                    ':val': True
-                },
-                ReturnValues='ALL_NEW'
-            )
-
-            current_app.logger.info('updated_item from dynamodb ("is_processed" shouldb be "True"): %s', updated_item)
-        except Exception as e:
-            current_app.logger.critical(
-                'Exception attempting to update item in dynamodb with participant_id: %s and payment_id: %s - '
-                'exception_type: %s exception_message: %s',
-                item.get('participant_id'), item.get('payment_id'), type(e), e
-            )
diff --git a/app/celery/tasks.py b/app/celery/tasks.py
@@ -81,7 +81,7 @@ def process_job(
     if not service.active:
         job.job_status = JOB_STATUS_CANCELLED
         dao_update_job(job)
-        current_app.logger.warning(f"Job {job_id} has been cancelled, service {service.id} is inactive")
+        current_app.logger.warning(f'Job {job_id} has been cancelled, service {service.id} is inactive')
         return
 
     if __sending_limits_for_job_exceeded(service, job, job_id):

diff --git a/app/celery/v3/notification_tasks.py b/app/celery/v3/notification_tasks.py
@@ -78,26 +78,7 @@ def v3_process_notification(  # noqa: C901
     3. The given service owns the specified template.
     """
 
-    right_now = datetime.utcnow()
-    notification = Notification(
-        id=request_data['id'],
-        to=request_data.get('email_address' if request_data['notification_type'] == EMAIL_TYPE else 'phone_number'),
-        service_id=service_id,
-        template_id=request_data['template_id'],
-        template_version=0,
-        api_key_id=api_key_id,
-        key_type=api_key_type,
-        notification_type=request_data['notification_type'],
-        created_at=right_now,
-        updated_at=right_now,
-        status=NOTIFICATION_PERMANENT_FAILURE,
-        status_reason=None,
-        client_reference=request_data.get('client_reference'),
-        reference=request_data.get('reference'),
-        personalisation=request_data.get('personalisation'),
-        sms_sender_id=request_data.get('sms_sender_id'),
-        billing_code=request_data.get('billing_code'),
-    )
+    notification = v3_create_notification_instance(request_data, service_id, api_key_id, api_key_type)
 
     # TODO - Catch db connection errors and retry?
     with get_reader_session() as reader_session:
@@ -163,15 +144,23 @@ def v3_process_notification(  # noqa: C901
             with get_reader_session() as reader_session:
                 sms_sender = reader_session.execute(query).one().ServiceSmsSender
                 v3_send_sms_notification.delay(notification, sms_sender.sms_sender)
-        except (MultipleResultsFound, NoResultFound):
-            status_reason = f"SMS sender {notification.sms_sender_id} does not exist."
+        except NoResultFound:
+            err = f"SMS sender with id '{notification.sms_sender_id}' does not exist."
 
             # Set sms_sender_id to None so persisting it doesn't raise sqlalchemy.exc.IntegrityError
             # This happens in case user provides invalid sms_sender_id in the request data
             notification.sms_sender_id = None
             notification.status = NOTIFICATION_PERMANENT_FAILURE
             notification.status_reason = 'SMS sender does not exist.'
-            err = f"SMS sender with id '{notification.sms_sender_id}' does not exist."
+            v3_persist_failed_notification(notification, err)
+        except MultipleResultsFound:
+            err = f'Multiple SMS sender ids matched with: {notification.sms_sender_id}'
+
+            # Set sms_sender_id to None so persisting it doesn't raise sqlalchemy.exc.IntegrityError
+            # This happens in case user provides invalid sms_sender_id in the request data
+            notification.sms_sender_id = None
+            notification.status = NOTIFICATION_PERMANENT_FAILURE
+            notification.status_reason = 'SMS sender is invalid'
             v3_persist_failed_notification(notification, err)
 
     return
@@ -199,7 +188,8 @@ def v3_send_email_notification(
     # Persist the notification so related model instances are available to downstream code.
     notification.status = NOTIFICATION_CREATED
     db.session.add(notification)
-    db.session.add(template)
+    # TODO - Is this necessary?  Maybe refresh instead.  The template isn't being modified.
+    # db.session.add(template)
     db.session.commit()
 
     # query = select(Template).where(
@@ -316,6 +306,7 @@ def v3_persist_failed_notification(
     """
     This is a helper to log and persist failed notifications that are not retriable.
     """
+
     assert notification.status is not None
     assert notification.status_reason is not None
 
@@ -330,3 +321,38 @@ def v3_persist_failed_notification(
         except Exception as err:
             db.session.rollback()
             current_app.logger.critical("Unable to save Notification '%s'. Error: '%s'", notification.id, err)
+
+
+def v3_create_notification_instance(
+    request_data: dict,
+    service_id: str,
+    api_key_id: str,
+    api_key_type: str,
+    template_version: int = None,
+) -> Notification:
+    """
+    Create and return a Notification instance, but do not persist it in the database.  The "template_version"
+    parameter is not None when used from unit tests that don't call v3_process_notification, which might change
+    the value of Notification.template_version to something other than 0.
+    """
+
+    right_now = datetime.utcnow()
+    return Notification(
+        id=request_data['id'],
+        to=request_data.get('email_address' if request_data['notification_type'] == EMAIL_TYPE else 'phone_number'),
+        service_id=service_id,
+        template_id=request_data['template_id'],
+        template_version=template_version if (template_version is not None) else 0,
+        api_key_id=api_key_id,
+        key_type=api_key_type,
+        notification_type=request_data['notification_type'],
+        created_at=right_now,
+        updated_at=right_now,
+        status=NOTIFICATION_PERMANENT_FAILURE,
+        status_reason=None,
+        client_reference=request_data.get('client_reference'),
+        reference=request_data.get('reference'),
+        personalisation=request_data.get('personalisation'),
+        sms_sender_id=request_data.get('sms_sender_id'),
+        billing_code=request_data.get('billing_code'),
+    )
diff --git a/app/cronitor.py b/app/cronitor.py
@@ -12,15 +12,15 @@ def ping_cronitor(command):
 
             task_slug = current_app.config['CRONITOR_KEYS'].get(task_name)
             if not task_slug:
-                current_app.logger.error('Cronitor enabled but task_name %s not found in environment', task_name)
+                current_app.logger.error('Cronitor enabled, but task_name %s not found in environment.', task_name)
                 return
 
             if command not in {'run', 'complete', 'fail'}:
                 raise ValueError('command {} not a valid cronitor command'.format(command))
 
             try:
                 resp = requests.get(
-                    'https://cronitor.link/{}/{}'.format(task_slug, command),
+                    f'https://cronitor.link/{task_slug}/{command}',
                     # cronitor limits msg to 1000 characters
                     params={
                         'host': current_app.config['API_HOST_NAME'],