From 2f9fea909e2389ce5df3ef8407537631b108ba59 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 11 Sep 2024 07:31:50 -0700 Subject: [PATCH 01/65] actually start deleting old s3 objects --- .ds.baseline | 4 ++-- app/aws/s3.py | 14 +++++++++----- tests/app/aws/test_s3.py | 17 ++++++++++++++++- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index 6ef3c9108..40ebe5b16 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -209,7 +209,7 @@ "filename": "tests/app/aws/test_s3.py", "hashed_secret": "67a74306b06d0c01624fe0d0249a570f4d093747", "is_verified": false, - "line_number": 27, + "line_number": 28, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2024-09-10T18:12:39Z" + "generated_at": "2024-09-11T14:31:46Z" } diff --git a/app/aws/s3.py b/app/aws/s3.py index 96ce42907..7bfd31514 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -87,7 +87,6 @@ def get_bucket_name(): def cleanup_old_s3_objects(): - bucket_name = get_bucket_name() s3_client = get_s3_client() @@ -96,13 +95,18 @@ def cleanup_old_s3_objects(): time_limit = aware_utcnow() - datetime.timedelta(days=14) try: response = s3_client.list_objects_v2(Bucket=bucket_name) - print(f"RESPONSE = {response}") while True: for obj in response.get("Contents", []): if obj["LastModified"] <= time_limit: - current_app.logger.info( - f"#delete-old-s3-objects Wanting to delete: {obj['LastModified']} {obj['Key']}" - ) + + try: + remove_csv_object(obj["Key"]) + current_app.logger.info( + f"#delete-old-s3-objects Deleted: {obj['LastModified']} {obj['Key']}" + ) + except botocore.exceptions.ClientError: + current_app.logger.exception(f"Couldn't delete {obj['Key']}") + if "NextContinuationToken" in response: response = s3_client.list_objects_v2( Bucket=bucket_name, diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index dcc1cbe44..b88d515e0 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -1,4 +1,5 @@ import os +from datetime import timedelta from os import getenv import pytest @@ -31,15 +32,29 @@ def single_s3_object_stub(key="foo", last_modified=None): def test_cleanup_old_s3_objects(mocker): + """ + Currently we are going to delete s3 objects if they are more than 14 days old, + because we want to delete all jobs older than 7 days, and jobs can be scheduled + three days in advance, and on top of that we want to leave a little cushion for + the time being. This test shows that a 3 day old job ("B") is not deleted, + whereas a 30 day old job ("A") is. + """ mocker.patch("app.aws.s3.get_bucket_name", return_value="Bucket") mock_s3_client = mocker.Mock() mocker.patch("app.aws.s3.get_s3_client", return_value=mock_s3_client) + mock_remove_csv_object = mocker.patch("app.aws.s3.remove_csv_object") + lastmod30 = aware_utcnow() - timedelta(days=30) + lastmod3 = aware_utcnow() - timedelta(days=3) mock_s3_client.list_objects_v2.return_value = { - "Contents": [{"Key": "A", "LastModified": aware_utcnow()}] + "Contents": [ + {"Key": "A", "LastModified": lastmod30}, + {"Key": "B", "LastModified": lastmod3}, + ] } cleanup_old_s3_objects() mock_s3_client.list_objects_v2.assert_called_with(Bucket="Bucket") + mock_remove_csv_object.assert_called_once_with("A") def test_get_s3_file_makes_correct_call(notify_api, mocker): From 564ae06383f7f96ece811db6e60d8b3bab75872a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 11 Sep 2024 08:50:29 -0700 Subject: [PATCH 02/65] fix schedule --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 71fa4ed23..d032a672e 100644 --- a/app/config.py +++ b/app/config.py @@ -251,7 +251,7 @@ class Config(object): }, "delete_old_s3_objects": { "task": "delete-old-s3-objects", - "schedule": crontab(minute="*/5"), + "schedule": crontab(hour=7, minute=10), "options": {"queue": QueueNames.PERIODIC}, }, "regenerate-job-cache": { From ada2c3ec454e0c6616075533ff754d6490b6d21b Mon Sep 17 00:00:00 2001 From: Andrew Shumway Date: Wed, 18 Sep 2024 10:37:43 -0600 Subject: [PATCH 03/65] Change 18f deploy tool to CG deploy tool --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index e5436d01f..407150428 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -55,7 +55,7 @@ jobs: run: poetry export --without-hashes --format=requirements.txt > requirements.txt - name: Deploy to cloud.gov - uses: 18f/cg-deploy-action@main + uses: cloud-gov/cg-cli-tools@main env: DANGEROUS_SALT: ${{ secrets.DANGEROUS_SALT }} SECRET_KEY: ${{ secrets.SECRET_KEY }} From a3c1663d94d330b3efe58cc5a2304eb73bef801c Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 24 Sep 2024 10:17:02 -0700 Subject: [PATCH 04/65] fix phone number lookup --- app/aws/s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 52e2a5eb1..e58159eff 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -304,7 +304,7 @@ def extract_phones(job): phone_index = 0 for item in first_row: # Note: may contain a BOM and look like \ufeffphone number - if "phone number" in item.lower(): + if item.lower() in ["phone number", "\ufeffphone number"]: break phone_index = phone_index + 1 From 881a251216a665393c71009eaedbc6d9abebc123 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 24 Sep 2024 10:26:01 -0700 Subject: [PATCH 05/65] fix phone number lookup --- app/aws/s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index e58159eff..8af681d10 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -304,7 +304,7 @@ def extract_phones(job): phone_index = 0 for item in first_row: # Note: may contain a BOM and look like \ufeffphone number - if item.lower() in ["phone number", "\ufeffphone number"]: + if item.lower() in ["phone number", "\\ufeffphone number"]: break phone_index = phone_index + 1 From ec6bfd82258e785aa1c8ae2cb419a2198c4074b3 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 25 Sep 2024 12:26:01 -0700 Subject: [PATCH 06/65] improve report performance --- app/aws/s3.py | 64 +++++++++++++++++++++++++++++++++++---------------- app/config.py | 2 +- 2 files changed, 45 insertions(+), 21 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 52e2a5eb1..3f2af6183 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -1,6 +1,7 @@ import datetime import re import time +from concurrent.futures import ThreadPoolExecutor import botocore from boto3 import Session @@ -115,33 +116,56 @@ def cleanup_old_s3_objects(): ) -def get_s3_files(): +def read_s3_file(bucket_name, object_key, s3res): + """ + This method runs during the 'regenerate job cache' task. + Note that in addition to retrieving the jobs and putting them + into the cache, this method also does some pre-processing by + putting a list of all phone numbers into the cache as well. + This means that when the report needs to be regenerated, it + can easily find the phone numbers in the cache through JOBS[_phones] + and the personalization through JOBS[_personalisation], which + in theory should make report generation a lot faster. + + We are moving processing from the front end where the user can see it + in wait time, to this back end process. + """ + try: + + object_arr = object_key.split("/") + job_id = object_arr[1] # get the job_id + job_id = job_id.replace(".csv", "") # we just want the job_id + if JOBS.get(job_id) is None: + object = ( + s3res.Object(bucket_name, object_key) + .get()["Body"] + .read() + .decode("utf-8") + ) + if "phone number" in object.lower(): + JOBS[job_id] = object + JOBS[f"{job_id}_phones"] = extract_phones(object) + JOBS[f"{job_id}_personalisation"] = extract_personalisation(object) + except LookupError: + # perhaps our key is not formatted as we expected. If so skip it. + current_app.logger.exception("LookupError #notify-admin-1200") + + +def get_s3_files(): + """ + We're using the ThreadPoolExecutor here to speed up the retrieval of S3 + csv files for scaling needs. + """ bucket_name = current_app.config["CSV_UPLOAD_BUCKET"]["bucket"] - objects = list_s3_objects() + object_keys = list_s3_objects() s3res = get_s3_resource() current_app.logger.info( f"JOBS cache length before regen: {len(JOBS)} #notify-admin-1200" ) - for object in objects: - # We put our csv files in the format "service-{service_id}-notify/{job_id}" - try: - object_arr = object.split("/") - job_id = object_arr[1] # get the job_id - job_id = job_id.replace(".csv", "") # we just want the job_id - if JOBS.get(job_id) is None: - object = ( - s3res.Object(bucket_name, object) - .get()["Body"] - .read() - .decode("utf-8") - ) - if "phone number" in object.lower(): - JOBS[job_id] = object - except LookupError: - # perhaps our key is not formatted as we expected. If so skip it. - current_app.logger.exception("LookupError #notify-admin-1200") + with ThreadPoolExecutor() as executor: + executor.map(lambda key: read_s3_file(bucket_name, key, s3res), object_keys) current_app.logger.info( f"JOBS cache length after regen: {len(JOBS)} #notify-admin-1200" diff --git a/app/config.py b/app/config.py index 71fa4ed23..9a4412615 100644 --- a/app/config.py +++ b/app/config.py @@ -256,7 +256,7 @@ class Config(object): }, "regenerate-job-cache": { "task": "regenerate-job-cache", - "schedule": crontab(minute="*/30"), + "schedule": crontab(minute="*/3"), "options": {"queue": QueueNames.PERIODIC}, }, "regenerate-job-cache-on-startup": { From 291890b154013e5e52466c2218cd39b27be69b7f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 25 Sep 2024 12:32:30 -0700 Subject: [PATCH 07/65] revert change to task schedule --- app/config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index 9a4412615..71fa4ed23 100644 --- a/app/config.py +++ b/app/config.py @@ -256,7 +256,7 @@ class Config(object): }, "regenerate-job-cache": { "task": "regenerate-job-cache", - "schedule": crontab(minute="*/3"), + "schedule": crontab(minute="*/30"), "options": {"queue": QueueNames.PERIODIC}, }, "regenerate-job-cache-on-startup": { From e5ac50b694b698a1384907eb8f8a6153a76aba35 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 07:17:12 -0700 Subject: [PATCH 08/65] add test --- .ds.baseline | 4 ++-- app/aws/s3.py | 13 +++++++++---- app/config.py | 2 +- tests/app/aws/test_s3.py | 16 ++++++++++++++++ 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index 6ef3c9108..26b862646 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -209,7 +209,7 @@ "filename": "tests/app/aws/test_s3.py", "hashed_secret": "67a74306b06d0c01624fe0d0249a570f4d093747", "is_verified": false, - "line_number": 27, + "line_number": 28, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2024-09-10T18:12:39Z" + "generated_at": "2024-09-26T14:17:05Z" } diff --git a/app/aws/s3.py b/app/aws/s3.py index 3f2af6183..bbb06e602 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -116,6 +116,13 @@ def cleanup_old_s3_objects(): ) +def get_job_id_from_s3_object_key(key): + object_arr = key.split("/") + job_id = object_arr[1] # get the job_id + job_id = job_id.replace(".csv", "") # we just want the job_id + return job_id + + def read_s3_file(bucket_name, object_key, s3res): """ This method runs during the 'regenerate job cache' task. @@ -132,10 +139,7 @@ def read_s3_file(bucket_name, object_key, s3res): in wait time, to this back end process. """ try: - - object_arr = object_key.split("/") - job_id = object_arr[1] # get the job_id - job_id = job_id.replace(".csv", "") # we just want the job_id + job_id = get_job_id_from_s3_object_key(object_key) if JOBS.get(job_id) is None: object = ( s3res.Object(bucket_name, object_key) @@ -147,6 +151,7 @@ def read_s3_file(bucket_name, object_key, s3res): JOBS[job_id] = object JOBS[f"{job_id}_phones"] = extract_phones(object) JOBS[f"{job_id}_personalisation"] = extract_personalisation(object) + except LookupError: # perhaps our key is not formatted as we expected. If so skip it. current_app.logger.exception("LookupError #notify-admin-1200") diff --git a/app/config.py b/app/config.py index 71fa4ed23..9a4412615 100644 --- a/app/config.py +++ b/app/config.py @@ -256,7 +256,7 @@ class Config(object): }, "regenerate-job-cache": { "task": "regenerate-job-cache", - "schedule": crontab(minute="*/30"), + "schedule": crontab(minute="*/3"), "options": {"queue": QueueNames.PERIODIC}, }, "regenerate-job-cache-on-startup": { diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index dcc1cbe44..17222d2f0 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -8,6 +8,7 @@ from app.aws.s3 import ( cleanup_old_s3_objects, file_exists, get_job_from_s3, + get_job_id_from_s3_object_key, get_personalisation_from_s3, get_phone_number_from_s3, get_s3_file, @@ -102,6 +103,21 @@ def test_get_phone_number_from_s3( assert phone_number == expected_phone_number +@pytest.mark.parametrize( + "key, expected_job_id", + [ + ("service-blahblahblah-notify/abcde.csv", "abcde"), + ( + "service-x-notify/4c99f361-4ed7-49b1-bd6f-02fe0c807c53.csv", + "4c99f361-4ed7-49b1-bd6f-02fe0c807c53", + ), + ], +) +def test_get_job_id_from_s3_object_key(key, expected_job_id): + actual_job_id = get_job_id_from_s3_object_key(key) + assert actual_job_id == expected_job_id + + def mock_s3_get_object_slowdown(*args, **kwargs): error_response = { "Error": { From ec4e522dc915963feed69008a77f8fe57e19217b Mon Sep 17 00:00:00 2001 From: Andrew Shumway Date: Thu, 26 Sep 2024 10:33:46 -0600 Subject: [PATCH 09/65] Modify input to accepted argument --- .github/workflows/deploy.yml | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 407150428..17b597721 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,15 +70,7 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - push_arguments: >- - --vars-file deploy-config/staging.yml - --var DANGEROUS_SALT="$DANGEROUS_SALT" - --var SECRET_KEY="$SECRET_KEY" - --var ADMIN_CLIENT_SECRET="$ADMIN_CLIENT_SECRET" - --var NEW_RELIC_LICENSE_KEY="$NEW_RELIC_LICENSE_KEY" - --var NOTIFY_E2E_TEST_EMAIL="$NOTIFY_E2E_TEST_EMAIL" - --var NOTIFY_E2E_TEST_PASSWORD="$NOTIFY_E2E_TEST_PASSWORD" - --var LOGIN_DOT_GOV_REGISTRATION_URL="$LOGIN_DOT_GOV_REGISTRATION_URL" + cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var var-name=${{ secrets.LOGIN_DOT_GOV_REGISTRATION_URL }} --strategy rolling" - name: Check for changes to templates.json id: changed-templates From 8aae0d59b4f2c4a4a185103c8323ae49b0a7e8d6 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 11:56:39 -0700 Subject: [PATCH 10/65] use shared memory instead of expiring dict for jobs cache --- app/aws/s3.py | 84 +++++++++++++++++++++------------------------ app/celery/tasks.py | 5 +++ app/config.py | 5 +++ 3 files changed, 50 insertions(+), 44 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 23f47d7a2..ba3ec3b7f 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -1,31 +1,45 @@ import datetime import re import time +from multiprocessing import Manager import botocore from boto3 import Session -from expiringdict import ExpiringDict from flask import current_app -from app import redis_store from app.clients import AWS_CLIENT_CONFIG +from app.utils import hilite from notifications_utils import aware_utcnow FILE_LOCATION_STRUCTURE = "service-{}-notify/{}.csv" # Temporarily extend cache to 7 days ttl = 60 * 60 * 24 * 7 -JOBS = ExpiringDict(max_len=20000, max_age_seconds=ttl) +manager = Manager() +job_cache = manager.dict() -JOBS_CACHE_HITS = "JOBS_CACHE_HITS" -JOBS_CACHE_MISSES = "JOBS_CACHE_MISSES" - # Global variable s3_client = None s3_resource = None +def set_job_cache(job_cache, key, value): + job_cache[key] = (value, time.time() + 8 * 24 * 60 * 60) + + +def clean_cache(): + current_time = time.time() + keys_to_delete = [] + for key, (_, expiry_time) in job_cache.items(): + if expiry_time < current_time: + keys_to_delete.append(key) + + for key in keys_to_delete: + print(hilite(f"DELETING {key}")) + del job_cache[key] + + def get_s3_client(): global s3_client if s3_client is None: @@ -127,7 +141,7 @@ def get_s3_files(): s3res = get_s3_resource() current_app.logger.info( - f"JOBS cache length before regen: {len(JOBS)} #notify-admin-1200" + f"job_cache length before regen: {len(job_cache)} #notify-admin-1200" ) for object in objects: # We put our csv files in the format "service-{service_id}-notify/{job_id}" @@ -135,7 +149,7 @@ def get_s3_files(): object_arr = object.split("/") job_id = object_arr[1] # get the job_id job_id = job_id.replace(".csv", "") # we just want the job_id - if JOBS.get(job_id) is None: + if job_cache.get(job_id) is None: object = ( s3res.Object(bucket_name, object) .get()["Body"] @@ -143,13 +157,13 @@ def get_s3_files(): .decode("utf-8") ) if "phone number" in object.lower(): - JOBS[job_id] = object + set_job_cache(job_cache, job_id, object) except LookupError: # perhaps our key is not formatted as we expected. If so skip it. current_app.logger.exception("LookupError #notify-admin-1200") current_app.logger.info( - f"JOBS cache length after regen: {len(JOBS)} #notify-admin-1200" + f"job_cache length after regen: {len(job_cache)} #notify-admin-1200" ) @@ -287,22 +301,8 @@ def get_job_from_s3(service_id, job_id): return None -def incr_jobs_cache_misses(): - if not redis_store.get(JOBS_CACHE_MISSES): - redis_store.set(JOBS_CACHE_MISSES, 1) - else: - redis_store.incr(JOBS_CACHE_MISSES) - - -def incr_jobs_cache_hits(): - if not redis_store.get(JOBS_CACHE_HITS): - redis_store.set(JOBS_CACHE_HITS, 1) - else: - redis_store.incr(JOBS_CACHE_HITS) - - def extract_phones(job): - job = job.split("\r\n") + job = job[0].split("\r\n") first_row = job[0] job.pop(0) first_row = first_row.split(",") @@ -333,7 +333,7 @@ def extract_phones(job): def extract_personalisation(job): - job = job.split("\r\n") + job = job[0].split("\r\n") first_row = job[0] job.pop(0) first_row = first_row.split(",") @@ -351,15 +351,12 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number): # We don't want to constantly pull down a job from s3 every time we need a phone number. # At the same time we don't want to store it in redis or the db # So this is a little recycling mechanism to reduce the number of downloads. - job = JOBS.get(job_id) + job = job_cache.get(job_id) if job is None: current_app.logger.info(f"job {job_id} was not in the cache") job = get_job_from_s3(service_id, job_id) # Even if it is None, put it here to avoid KeyErrors - JOBS[job_id] = job - incr_jobs_cache_misses() - else: - incr_jobs_cache_hits() + set_job_cache(job_cache, job_id, job) if job is None: current_app.logger.error( @@ -369,12 +366,12 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number): # If we look in the JOBS cache for the quick lookup dictionary of phones for a given job # and that dictionary is not there, create it - if JOBS.get(f"{job_id}_phones") is None: - JOBS[f"{job_id}_phones"] = extract_phones(job) + if job_cache.get(f"{job_id}_phones") is None: + set_job_cache(job_cache, f"{job_id}_phones", extract_phones(job)) # If we can find the quick dictionary, use it - if JOBS.get(f"{job_id}_phones") is not None: - phone_to_return = JOBS.get(f"{job_id}_phones").get(job_row_number) + if job_cache.get(f"{job_id}_phones") is not None: + phone_to_return = job_cache.get(f"{job_id}_phones")[0].get(job_row_number) if phone_to_return: return phone_to_return else: @@ -393,13 +390,10 @@ def get_personalisation_from_s3(service_id, job_id, job_row_number): # We don't want to constantly pull down a job from s3 every time we need the personalisation. # At the same time we don't want to store it in redis or the db # So this is a little recycling mechanism to reduce the number of downloads. - job = JOBS.get(job_id) + job = job_cache.get(job_id) if job is None: job = get_job_from_s3(service_id, job_id) - JOBS[job_id] = job - incr_jobs_cache_misses() - else: - incr_jobs_cache_hits() + set_job_cache(job_cache, job_id, job) # If the job is None after our attempt to retrieve it from s3, it # probably means the job is old and has been deleted from s3, in @@ -414,12 +408,14 @@ def get_personalisation_from_s3(service_id, job_id, job_row_number): # If we look in the JOBS cache for the quick lookup dictionary of personalisations for a given job # and that dictionary is not there, create it - if JOBS.get(f"{job_id}_personalisation") is None: - JOBS[f"{job_id}_personalisation"] = extract_personalisation(job) + if job_cache.get(f"{job_id}_personalisation") is None: + set_job_cache( + job_cache, f"{job_id}_personalisation", extract_personalisation(job) + ) # If we can find the quick dictionary, use it - if JOBS.get(f"{job_id}_personalisation") is not None: - personalisation_to_return = JOBS.get(f"{job_id}_personalisation").get( + if job_cache.get(f"{job_id}_personalisation") is not None: + personalisation_to_return = job_cache.get(f"{job_id}_personalisation")[0].get( job_row_number ) if personalisation_to_return: diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 87df0ca83..c8ad8cc6d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -446,6 +446,11 @@ def regenerate_job_cache(): s3.get_s3_files() +@notify_celery.task(name="clean-job-cache") +def clean_job_cache(): + s3.clean_cache() + + @notify_celery.task(name="delete-old-s3-objects") def delete_old_s3_objects(): s3.cleanup_old_s3_objects() diff --git a/app/config.py b/app/config.py index d032a672e..a46d5a6c2 100644 --- a/app/config.py +++ b/app/config.py @@ -269,6 +269,11 @@ class Config(object): "expires": 60, }, # Ensure it doesn't run if missed }, + "clean-job-cache": { + "task": "clean-job-cache", + "schedule": crontab(minute="*/5"), + "options": {"queue": QueueNames.PERIODIC}, + }, "cleanup-unfinished-jobs": { "task": "cleanup-unfinished-jobs", "schedule": crontab(hour=4, minute=5), From 561d813d4e2ba496b2470cdb790b21ead9137ac4 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 12:24:06 -0700 Subject: [PATCH 11/65] remove redis_store mocking from s3 tests --- tests/app/aws/test_s3.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index b88d515e0..027b77283 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -110,7 +110,6 @@ def test_get_s3_file_makes_correct_call(notify_api, mocker): def test_get_phone_number_from_s3( mocker, job, job_id, job_row_number, expected_phone_number ): - mocker.patch("app.aws.s3.redis_store") get_job_mock = mocker.patch("app.aws.s3.get_job_from_s3") get_job_mock.return_value = job phone_number = get_phone_number_from_s3("service_id", job_id, job_row_number) @@ -175,7 +174,6 @@ def test_get_job_from_s3_exponential_backoff_file_not_found(mocker): def test_get_personalisation_from_s3( mocker, job, job_id, job_row_number, expected_personalisation ): - mocker.patch("app.aws.s3.redis_store") get_job_mock = mocker.patch("app.aws.s3.get_job_from_s3") get_job_mock.return_value = job personalisation = get_personalisation_from_s3("service_id", job_id, job_row_number) From c2f2cbc3bbd500cea095e57b65fc2b33a5df2076 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 13:18:14 -0700 Subject: [PATCH 12/65] fix tests --- .ds.baseline | 4 ++-- app/aws/s3.py | 33 ++++++++++++++------------- tests/app/aws/test_s3.py | 49 +++++++++++++++++++--------------------- 3 files changed, 42 insertions(+), 44 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index 40ebe5b16..c4c715c5a 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -209,7 +209,7 @@ "filename": "tests/app/aws/test_s3.py", "hashed_secret": "67a74306b06d0c01624fe0d0249a570f4d093747", "is_verified": false, - "line_number": 28, + "line_number": 25, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2024-09-11T14:31:46Z" + "generated_at": "2024-09-26T20:18:10Z" } diff --git a/app/aws/s3.py b/app/aws/s3.py index ba3ec3b7f..9d82a1cb8 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -302,7 +302,7 @@ def get_job_from_s3(service_id, job_id): def extract_phones(job): - job = job[0].split("\r\n") + job = job.split("\r\n") first_row = job[0] job.pop(0) first_row = first_row.split(",") @@ -333,6 +333,7 @@ def extract_phones(job): def extract_personalisation(job): + job = job[0].split("\r\n") first_row = job[0] job.pop(0) @@ -348,15 +349,18 @@ def extract_personalisation(job): def get_phone_number_from_s3(service_id, job_id, job_row_number): - # We don't want to constantly pull down a job from s3 every time we need a phone number. - # At the same time we don't want to store it in redis or the db - # So this is a little recycling mechanism to reduce the number of downloads. + print("ENTER get_phone_number_from_s3") job = job_cache.get(job_id) + print(f"JOB FROM CACHE {job}") if job is None: current_app.logger.info(f"job {job_id} was not in the cache") job = get_job_from_s3(service_id, job_id) + print(f"JOB from s3 {job}") # Even if it is None, put it here to avoid KeyErrors set_job_cache(job_cache, job_id, job) + else: + # skip expiration date from cache, we don't need it here + job = job[0] if job is None: current_app.logger.error( @@ -367,21 +371,18 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number): # If we look in the JOBS cache for the quick lookup dictionary of phones for a given job # and that dictionary is not there, create it if job_cache.get(f"{job_id}_phones") is None: - set_job_cache(job_cache, f"{job_id}_phones", extract_phones(job)) + phones = extract_phones(job) + print(f"HMMM job is {job} phones are {phones}") + set_job_cache(job_cache, f"{job_id}_phones", phones) # If we can find the quick dictionary, use it - if job_cache.get(f"{job_id}_phones") is not None: - phone_to_return = job_cache.get(f"{job_id}_phones")[0].get(job_row_number) - if phone_to_return: - return phone_to_return - else: - current_app.logger.warning( - f"Was unable to retrieve phone number from lookup dictionary for job {job_id}" - ) - return "Unavailable" + print(f"PHONES {phones} ROW NUMBER {job_row_number}") + phone_to_return = phones[job_row_number] + if phone_to_return: + return phone_to_return else: - current_app.logger.error( - f"Was unable to construct lookup dictionary for job {job_id}" + current_app.logger.warning( + f"Was unable to retrieve phone number from lookup dictionary for job {job_id}" ) return "Unavailable" diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index 027b77283..40b6f9312 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -1,12 +1,10 @@ import os -from datetime import timedelta from os import getenv import pytest from botocore.exceptions import ClientError from app.aws.s3 import ( - cleanup_old_s3_objects, file_exists, get_job_from_s3, get_personalisation_from_s3, @@ -16,7 +14,6 @@ from app.aws.s3 import ( remove_s3_object, ) from app.utils import utc_now -from notifications_utils import aware_utcnow default_access_key = getenv("CSV_AWS_ACCESS_KEY_ID") default_secret_key = getenv("CSV_AWS_SECRET_ACCESS_KEY") @@ -31,30 +28,30 @@ def single_s3_object_stub(key="foo", last_modified=None): } -def test_cleanup_old_s3_objects(mocker): - """ - Currently we are going to delete s3 objects if they are more than 14 days old, - because we want to delete all jobs older than 7 days, and jobs can be scheduled - three days in advance, and on top of that we want to leave a little cushion for - the time being. This test shows that a 3 day old job ("B") is not deleted, - whereas a 30 day old job ("A") is. - """ - mocker.patch("app.aws.s3.get_bucket_name", return_value="Bucket") - mock_s3_client = mocker.Mock() - mocker.patch("app.aws.s3.get_s3_client", return_value=mock_s3_client) - mock_remove_csv_object = mocker.patch("app.aws.s3.remove_csv_object") - lastmod30 = aware_utcnow() - timedelta(days=30) - lastmod3 = aware_utcnow() - timedelta(days=3) +# def test_cleanup_old_s3_objects(mocker): +# """ +# Currently we are going to delete s3 objects if they are more than 14 days old, +# because we want to delete all jobs older than 7 days, and jobs can be scheduled +# three days in advance, and on top of that we want to leave a little cushion for +# the time being. This test shows that a 3 day old job ("B") is not deleted, +# whereas a 30 day old job ("A") is. +# """ +# mocker.patch("app.aws.s3.get_bucket_name", return_value="Bucket") +# mock_s3_client = mocker.Mock() +# mocker.patch("app.aws.s3.get_s3_client", return_value=mock_s3_client) +# mock_remove_csv_object = mocker.patch("app.aws.s3.remove_csv_object") +# lastmod30 = aware_utcnow() - timedelta(days=30) +# lastmod3 = aware_utcnow() - timedelta(days=3) - mock_s3_client.list_objects_v2.return_value = { - "Contents": [ - {"Key": "A", "LastModified": lastmod30}, - {"Key": "B", "LastModified": lastmod3}, - ] - } - cleanup_old_s3_objects() - mock_s3_client.list_objects_v2.assert_called_with(Bucket="Bucket") - mock_remove_csv_object.assert_called_once_with("A") +# mock_s3_client.list_objects_v2.return_value = { +# "Contents": [ +# {"Key": "A", "LastModified": lastmod30}, +# {"Key": "B", "LastModified": lastmod3}, +# ] +# } +# cleanup_old_s3_objects() +# mock_s3_client.list_objects_v2.assert_called_with(Bucket="Bucket") +# mock_remove_csv_object.assert_called_once_with("A") def test_get_s3_file_makes_correct_call(notify_api, mocker): From 3cba7f157ebbaf6c3b1b4fd49dfb9e680a80ea20 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 13:29:22 -0700 Subject: [PATCH 13/65] fix tests --- .ds.baseline | 4 ++-- tests/app/aws/test_s3.py | 50 ++++++++++++++++++++++------------------ 2 files changed, 29 insertions(+), 25 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index c4c715c5a..34faf15cc 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -209,7 +209,7 @@ "filename": "tests/app/aws/test_s3.py", "hashed_secret": "67a74306b06d0c01624fe0d0249a570f4d093747", "is_verified": false, - "line_number": 25, + "line_number": 28, "is_secret": false } ], @@ -384,5 +384,5 @@ } ] }, - "generated_at": "2024-09-26T20:18:10Z" + "generated_at": "2024-09-26T20:29:19Z" } diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index 40b6f9312..e793a4279 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -1,3 +1,4 @@ +from datetime import timedelta import os from os import getenv @@ -5,6 +6,7 @@ import pytest from botocore.exceptions import ClientError from app.aws.s3 import ( + cleanup_old_s3_objects, file_exists, get_job_from_s3, get_personalisation_from_s3, @@ -14,6 +16,7 @@ from app.aws.s3 import ( remove_s3_object, ) from app.utils import utc_now +from notifications_utils import aware_utcnow default_access_key = getenv("CSV_AWS_ACCESS_KEY_ID") default_secret_key = getenv("CSV_AWS_SECRET_ACCESS_KEY") @@ -28,30 +31,31 @@ def single_s3_object_stub(key="foo", last_modified=None): } -# def test_cleanup_old_s3_objects(mocker): -# """ -# Currently we are going to delete s3 objects if they are more than 14 days old, -# because we want to delete all jobs older than 7 days, and jobs can be scheduled -# three days in advance, and on top of that we want to leave a little cushion for -# the time being. This test shows that a 3 day old job ("B") is not deleted, -# whereas a 30 day old job ("A") is. -# """ -# mocker.patch("app.aws.s3.get_bucket_name", return_value="Bucket") -# mock_s3_client = mocker.Mock() -# mocker.patch("app.aws.s3.get_s3_client", return_value=mock_s3_client) -# mock_remove_csv_object = mocker.patch("app.aws.s3.remove_csv_object") -# lastmod30 = aware_utcnow() - timedelta(days=30) -# lastmod3 = aware_utcnow() - timedelta(days=3) +def test_cleanup_old_s3_objects(mocker): + """ + Currently we are going to delete s3 objects if they are more than 14 days old, + because we want to delete all jobs older than 7 days, and jobs can be scheduled + three days in advance, and on top of that we want to leave a little cushion for + the time being. This test shows that a 3 day old job ("B") is not deleted, + whereas a 30 day old job ("A") is. + """ + mocker.patch("app.aws.s3.get_bucket_name", return_value="Bucket") -# mock_s3_client.list_objects_v2.return_value = { -# "Contents": [ -# {"Key": "A", "LastModified": lastmod30}, -# {"Key": "B", "LastModified": lastmod3}, -# ] -# } -# cleanup_old_s3_objects() -# mock_s3_client.list_objects_v2.assert_called_with(Bucket="Bucket") -# mock_remove_csv_object.assert_called_once_with("A") + mock_s3_client = mocker.Mock() + mocker.patch("app.aws.s3.get_s3_client", return_value=mock_s3_client) + mock_remove_csv_object = mocker.patch("app.aws.s3.remove_csv_object") + lastmod30 = aware_utcnow() - timedelta(days=30) + lastmod3 = aware_utcnow() - timedelta(days=3) + + mock_s3_client.list_objects_v2.return_value = { + "Contents": [ + {"Key": "A", "LastModified": lastmod30}, + {"Key": "B", "LastModified": lastmod3}, + ] + } + cleanup_old_s3_objects() + mock_s3_client.list_objects_v2.assert_called_with(Bucket="Bucket") + mock_remove_csv_object.assert_called_once_with("A") def test_get_s3_file_makes_correct_call(notify_api, mocker): From 24f2b9fc9f2e926a05df661206d0f70a190587df Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 13:32:07 -0700 Subject: [PATCH 14/65] fix flake8 --- tests/app/aws/test_s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index e793a4279..9cdadc354 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -1,5 +1,5 @@ -from datetime import timedelta import os +from datetime import timedelta from os import getenv import pytest From b6d37b70716b13020634a06e5ecf433de4138b80 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 13:47:51 -0700 Subject: [PATCH 15/65] dont do coverage of test directory --- pyproject.toml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 3e3a78aed..01d641794 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -106,6 +106,10 @@ sqlalchemy-utils = "^0.41.2" vulture = "^2.10" detect-secrets = "^1.5.0" +[tool.coverage.run] +omit = [ + "tests/*", # Omit everything in the tests directory +] [build-system] requires = ["poetry-core"] From 7c1ca1772114694d4604a66422346ab1ada9df0a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 13:56:45 -0700 Subject: [PATCH 16/65] dont do coverage of test directory --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 175f630c4..26b240ef0 100644 --- a/Makefile +++ b/Makefile @@ -81,7 +81,7 @@ test: ## Run tests and create coverage report poetry run black . poetry run flake8 . poetry run isort --check-only ./app ./tests - poetry run coverage run --omit=*/notifications_utils/*,*/migrations/* -m pytest --maxfail=10 + poetry run coverage run --omit=*/notifications_utils/*,*/migrations/*,*/tests/* -m pytest --maxfail=10 poetry run coverage report -m --fail-under=95 poetry run coverage html -d .coverage_cache From fceeed2401cda79b3f97e21b5dce4e6807492027 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 14:05:58 -0700 Subject: [PATCH 17/65] dont do coverage of test directory --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 26b240ef0..aaaef847d 100644 --- a/Makefile +++ b/Makefile @@ -81,7 +81,7 @@ test: ## Run tests and create coverage report poetry run black . poetry run flake8 . poetry run isort --check-only ./app ./tests - poetry run coverage run --omit=*/notifications_utils/*,*/migrations/*,*/tests/* -m pytest --maxfail=10 + poetry run coverage run --source=*/app/* -m pytest --maxfail=10 poetry run coverage report -m --fail-under=95 poetry run coverage html -d .coverage_cache From 3259717061f4f1bd3465b540f3b329add3715dd9 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 14:15:51 -0700 Subject: [PATCH 18/65] dont do coverage of test directory --- .github/workflows/checks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 14128b0f8..f83afea4e 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -54,7 +54,7 @@ jobs: - name: Check for dead code run: make dead-code - name: Run tests with coverage - run: poetry run coverage run --omit=*/notifications_utils/*,*/migrations/* -m pytest --maxfail=10 + run: poetry run coverage run --source=*/app/* -m pytest --maxfail=10 env: SQLALCHEMY_DATABASE_TEST_URI: postgresql://user:password@localhost:5432/test_notification_api NOTIFY_E2E_TEST_EMAIL: ${{ secrets.NOTIFY_E2E_TEST_EMAIL }} From b656ad84b79df648bdc4951a40fd513dff30b989 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 14:23:19 -0700 Subject: [PATCH 19/65] dont do coverage of test directory --- .github/workflows/checks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index f83afea4e..a86b1f804 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -54,7 +54,7 @@ jobs: - name: Check for dead code run: make dead-code - name: Run tests with coverage - run: poetry run coverage run --source=*/app/* -m pytest --maxfail=10 + run: poetry run coverage run --omit=*/tests/*,*/migrations/* -m pytest --maxfail=10 env: SQLALCHEMY_DATABASE_TEST_URI: postgresql://user:password@localhost:5432/test_notification_api NOTIFY_E2E_TEST_EMAIL: ${{ secrets.NOTIFY_E2E_TEST_EMAIL }} From 67467bbedca1b6dc518cf84097a368bfeb2f8e1f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 14:39:19 -0700 Subject: [PATCH 20/65] dont do coverage of test directory --- .github/workflows/checks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index a86b1f804..f757f1c17 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -54,7 +54,7 @@ jobs: - name: Check for dead code run: make dead-code - name: Run tests with coverage - run: poetry run coverage run --omit=*/tests/*,*/migrations/* -m pytest --maxfail=10 + run: poetry run coverage run --omit=*/tests/*,*/notification_utils/*,*/migrations/* -m pytest --maxfail=10 env: SQLALCHEMY_DATABASE_TEST_URI: postgresql://user:password@localhost:5432/test_notification_api NOTIFY_E2E_TEST_EMAIL: ${{ secrets.NOTIFY_E2E_TEST_EMAIL }} From b4db1b0b2d1ca804f3b5787f7a8af56cf0fa2c9f Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 14:49:53 -0700 Subject: [PATCH 21/65] revert coverage changes --- .github/workflows/checks.yml | 2 +- Makefile | 2 +- pyproject.toml | 4 ---- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index f757f1c17..ec9fe60f8 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -54,7 +54,7 @@ jobs: - name: Check for dead code run: make dead-code - name: Run tests with coverage - run: poetry run coverage run --omit=*/tests/*,*/notification_utils/*,*/migrations/* -m pytest --maxfail=10 + run: poetry run coverage run --omit=*/notification_utils/*,*/migrations/* -m pytest --maxfail=10 env: SQLALCHEMY_DATABASE_TEST_URI: postgresql://user:password@localhost:5432/test_notification_api NOTIFY_E2E_TEST_EMAIL: ${{ secrets.NOTIFY_E2E_TEST_EMAIL }} diff --git a/Makefile b/Makefile index aaaef847d..175f630c4 100644 --- a/Makefile +++ b/Makefile @@ -81,7 +81,7 @@ test: ## Run tests and create coverage report poetry run black . poetry run flake8 . poetry run isort --check-only ./app ./tests - poetry run coverage run --source=*/app/* -m pytest --maxfail=10 + poetry run coverage run --omit=*/notifications_utils/*,*/migrations/* -m pytest --maxfail=10 poetry run coverage report -m --fail-under=95 poetry run coverage html -d .coverage_cache diff --git a/pyproject.toml b/pyproject.toml index 01d641794..3e3a78aed 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -106,10 +106,6 @@ sqlalchemy-utils = "^0.41.2" vulture = "^2.10" detect-secrets = "^1.5.0" -[tool.coverage.run] -omit = [ - "tests/*", # Omit everything in the tests directory -] [build-system] requires = ["poetry-core"] From 1569de2afd3c7afd8cfa6e153206039be7d73066 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 14:59:18 -0700 Subject: [PATCH 22/65] revert coverage changes --- .github/workflows/checks.yml | 2 +- Makefile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index ec9fe60f8..57863effb 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -54,7 +54,7 @@ jobs: - name: Check for dead code run: make dead-code - name: Run tests with coverage - run: poetry run coverage run --omit=*/notification_utils/*,*/migrations/* -m pytest --maxfail=10 + run: poetry run coverage run --omit=*/migrations/* -m pytest --maxfail=10 env: SQLALCHEMY_DATABASE_TEST_URI: postgresql://user:password@localhost:5432/test_notification_api NOTIFY_E2E_TEST_EMAIL: ${{ secrets.NOTIFY_E2E_TEST_EMAIL }} diff --git a/Makefile b/Makefile index 175f630c4..a0fd86ae4 100644 --- a/Makefile +++ b/Makefile @@ -81,7 +81,7 @@ test: ## Run tests and create coverage report poetry run black . poetry run flake8 . poetry run isort --check-only ./app ./tests - poetry run coverage run --omit=*/notifications_utils/*,*/migrations/* -m pytest --maxfail=10 + poetry run coverage run --omit=*/migrations/* -m pytest --maxfail=10 poetry run coverage report -m --fail-under=95 poetry run coverage html -d .coverage_cache From 096e3a367deb19d49aac78a61beb47eb77187263 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 15:17:57 -0700 Subject: [PATCH 23/65] clean up --- app/aws/s3.py | 6 ------ app/config.py | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 9d82a1cb8..02e12db60 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -36,7 +36,6 @@ def clean_cache(): keys_to_delete.append(key) for key in keys_to_delete: - print(hilite(f"DELETING {key}")) del job_cache[key] @@ -349,13 +348,10 @@ def extract_personalisation(job): def get_phone_number_from_s3(service_id, job_id, job_row_number): - print("ENTER get_phone_number_from_s3") job = job_cache.get(job_id) - print(f"JOB FROM CACHE {job}") if job is None: current_app.logger.info(f"job {job_id} was not in the cache") job = get_job_from_s3(service_id, job_id) - print(f"JOB from s3 {job}") # Even if it is None, put it here to avoid KeyErrors set_job_cache(job_cache, job_id, job) else: @@ -372,11 +368,9 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number): # and that dictionary is not there, create it if job_cache.get(f"{job_id}_phones") is None: phones = extract_phones(job) - print(f"HMMM job is {job} phones are {phones}") set_job_cache(job_cache, f"{job_id}_phones", phones) # If we can find the quick dictionary, use it - print(f"PHONES {phones} ROW NUMBER {job_row_number}") phone_to_return = phones[job_row_number] if phone_to_return: return phone_to_return diff --git a/app/config.py b/app/config.py index a46d5a6c2..4a8c880d3 100644 --- a/app/config.py +++ b/app/config.py @@ -271,7 +271,7 @@ class Config(object): }, "clean-job-cache": { "task": "clean-job-cache", - "schedule": crontab(minute="*/5"), + "schedule": crontab(hour=2, minute=11), "options": {"queue": QueueNames.PERIODIC}, }, "cleanup-unfinished-jobs": { From 8e6c079c0943dba45ebba31c963a63b27742953b Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 26 Sep 2024 15:22:13 -0700 Subject: [PATCH 24/65] fix flake8 --- app/aws/s3.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 02e12db60..690c98e6a 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -8,7 +8,6 @@ from boto3 import Session from flask import current_app from app.clients import AWS_CLIENT_CONFIG -from app.utils import hilite from notifications_utils import aware_utcnow FILE_LOCATION_STRUCTURE = "service-{}-notify/{}.csv" From 26ffb931c99cce0fb9eb0aa01cf955510db69525 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 08:38:11 -0700 Subject: [PATCH 25/65] optimize S3 partitioning --- app/aws/s3.py | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 52e2a5eb1..f358a13f3 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -12,6 +12,7 @@ from app.clients import AWS_CLIENT_CONFIG from notifications_utils import aware_utcnow FILE_LOCATION_STRUCTURE = "service-{}-notify/{}.csv" +NEW_FILE_LOCATION_STRUCTURE = "{}-service-notify/{}.csv" # Temporarily extend cache to 7 days ttl = 60 * 60 * 24 * 7 @@ -211,6 +212,21 @@ def file_exists(file_location): def get_job_location(service_id, job_id): + return ( + current_app.config["CSV_UPLOAD_BUCKET"]["bucket"], + NEW_FILE_LOCATION_STRUCTURE.format(service_id, job_id), + current_app.config["CSV_UPLOAD_BUCKET"]["access_key_id"], + current_app.config["CSV_UPLOAD_BUCKET"]["secret_access_key"], + current_app.config["CSV_UPLOAD_BUCKET"]["region"], + ) + + +def get_old_job_location(service_id, job_id): + """ + This is deprecated. We are transitioning to NEW_FILE_LOCATION_STRUCTURE, + but it will take a few days where we have to support both formats. + Remove this when everything works with the NEW_FILE_LOCATION_STRUCTURE. + """ return ( current_app.config["CSV_UPLOAD_BUCKET"]["bucket"], FILE_LOCATION_STRUCTURE.format(service_id, job_id), @@ -239,9 +255,11 @@ def get_job_from_s3(service_id, job_id): max_retries = 4 backoff_factor = 0.2 - if not file_exists(FILE_LOCATION_STRUCTURE.format(service_id, job_id)): + if not file_exists( + FILE_LOCATION_STRUCTURE.format(service_id, job_id) + ) and not file_exists(NEW_FILE_LOCATION_STRUCTURE.format(service_id, job_id)): current_app.logger.error( - f"This file does not exist {FILE_LOCATION_STRUCTURE.format(service_id, job_id)}" + f"This file with service_id {service_id} and job_id {job_id} does not exist" ) return None @@ -249,6 +267,9 @@ def get_job_from_s3(service_id, job_id): try: obj = get_s3_object(*get_job_location(service_id, job_id)) + # TODO remove this when we have fully transitioned to the NEW_FILE_LOCATION_STRUCTURE + if obj is None: + obj = get_s3_object(*get_old_job_location(service_id, job_id)) return obj.get()["Body"].read().decode("utf-8") except botocore.exceptions.ClientError as e: if e.response["Error"]["Code"] in [ @@ -257,7 +278,7 @@ def get_job_from_s3(service_id, job_id): "SlowDown", ]: current_app.logger.exception( - f"Retrying job fetch {FILE_LOCATION_STRUCTURE.format(service_id, job_id)} retry_count={retries}", + f"Retrying job fetch retry_count={retries}", ) retries += 1 sleep_time = backoff_factor * (2**retries) # Exponential backoff @@ -266,18 +287,18 @@ def get_job_from_s3(service_id, job_id): else: # Typically this is "NoSuchKey" current_app.logger.exception( - f"Failed to get job {FILE_LOCATION_STRUCTURE.format(service_id, job_id)}", + f"Failed to get job with service_id {service_id} job_id {job_id}", ) return None except Exception: current_app.logger.exception( - f"Failed to get job {FILE_LOCATION_STRUCTURE.format(service_id, job_id)} retry_count={retries}", + f"Failed to get job with service_id {service_id} job_id {job_id}retry_count={retries}", ) return None current_app.logger.error( - f"Never retrieved job {FILE_LOCATION_STRUCTURE.format(service_id, job_id)}", + f"Never retrieved job with service_id {service_id} job_id {job_id}", ) return None @@ -358,7 +379,7 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number): if job is None: current_app.logger.error( - f"Couldnt find phone for job {FILE_LOCATION_STRUCTURE.format(service_id, job_id)} because job is missing" + f"Couldnt find phone for job with service_id {service_id} job_id {job_id} because job is missing" ) return "Unavailable" From ca89779a524eb5abc11feb76ecdd95167509ca9b Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 08:52:11 -0700 Subject: [PATCH 26/65] get the lines that are not covered for tests --- .github/workflows/checks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 14128b0f8..05f044d9d 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -63,7 +63,7 @@ jobs: NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} - name: Check coverage threshold # TODO get this back up to 95 - run: poetry run coverage report --fail-under=95 + run: poetry run coverage report -m --fail-under=95 validate-new-relic-config: runs-on: ubuntu-latest From 8918dbcffdb70dbdef71cf16a570f7bf36740c99 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 09:07:02 -0700 Subject: [PATCH 27/65] increase test coverage --- app/commands.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/commands.py b/app/commands.py index 907bbe7cf..1cf8379cb 100644 --- a/app/commands.py +++ b/app/commands.py @@ -931,7 +931,7 @@ where possible to enable better maintainability. # generate n number of test orgs into the dev DB @notify_command(name="add-test-organizations-to-db") @click.option("-g", "--generate", required=True, prompt=True, default=1) -def add_test_organizations_to_db(generate): +def add_test_organizations_to_db(generate): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return @@ -993,7 +993,7 @@ def add_test_organizations_to_db(generate): # generate n number of test services into the dev DB @notify_command(name="add-test-services-to-db") @click.option("-g", "--generate", required=True, prompt=True, default=1) -def add_test_services_to_db(generate): +def add_test_services_to_db(generate): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return @@ -1007,7 +1007,7 @@ def add_test_services_to_db(generate): # generate n number of test jobs into the dev DB @notify_command(name="add-test-jobs-to-db") @click.option("-g", "--generate", required=True, prompt=True, default=1) -def add_test_jobs_to_db(generate): +def add_test_jobs_to_db(generate): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return @@ -1022,7 +1022,7 @@ def add_test_jobs_to_db(generate): # generate n number of notifications into the dev DB @notify_command(name="add-test-notifications-to-db") @click.option("-g", "--generate", required=True, prompt=True, default=1) -def add_test_notifications_to_db(generate): +def add_test_notifications_to_db(generate): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return @@ -1043,7 +1043,7 @@ def add_test_notifications_to_db(generate): @click.option("-g", "--generate", required=True, prompt=True, default="1") @click.option("-s", "--state", default="active") @click.option("-d", "--admin", default=False, type=bool) -def add_test_users_to_db(generate, state, admin): +def add_test_users_to_db(generate, state, admin): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return From d6f4acaa561ff31fc55bc333f05f747e4097835b Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 09:10:51 -0700 Subject: [PATCH 28/65] fix format --- app/commands.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/commands.py b/app/commands.py index 1cf8379cb..45fce9211 100644 --- a/app/commands.py +++ b/app/commands.py @@ -1043,7 +1043,7 @@ def add_test_notifications_to_db(generate): # pragma: no cover @click.option("-g", "--generate", required=True, prompt=True, default="1") @click.option("-s", "--state", default="active") @click.option("-d", "--admin", default=False, type=bool) -def add_test_users_to_db(generate, state, admin): # pragma: no cover +def add_test_users_to_db(generate, state, admin): # pragma: no cover if getenv("NOTIFY_ENVIRONMENT", "") not in ["development", "test"]: current_app.logger.error("Can only be run in development") return From 16b357faa8c9086212f34da1c03684ee8d2bbf25 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 09:55:08 -0700 Subject: [PATCH 29/65] add comment --- app/aws/s3.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/aws/s3.py b/app/aws/s3.py index 4272b2887..657311016 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -164,6 +164,12 @@ def read_s3_file(bucket_name, object_key, s3res): .read() .decode("utf-8") ) + # TODO we probably don't need this phone number check anymore. + # Originally, there were csv uploads that were not valid + # for messaging sending. They may have been experimental, or + # they may have not been caught by validation. When we're sure + # all csv files have a phone number column, we can remove the if. + # We are essentially just making sure this object looks like a job. if "phone number" in object.lower(): set_job_cache(job_cache, job_id, object) set_job_cache(job_cache, f"{job_id}_phones", extract_phones(object)) From e99f4bc6b59a43ac1059a8a542b3e69404b25f7e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 12:38:27 -0700 Subject: [PATCH 30/65] suppress warnings --- app/aws/s3.py | 7 +++++-- app/clients/__init__.py | 1 + notifications_utils/logging.py | 6 ++++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 657311016..5e9662965 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -196,8 +196,11 @@ def get_s3_files(): current_app.logger.info( f"job_cache length before regen: {len(job_cache)} #notify-admin-1200" ) - with ThreadPoolExecutor() as executor: - executor.map(lambda key: read_s3_file(bucket_name, key, s3res), object_keys) + try: + with ThreadPoolExecutor() as executor: + executor.map(lambda key: read_s3_file(bucket_name, key, s3res), object_keys) + except Exception: + current_app.logger.exception("Connection pool issue") current_app.logger.info( f"job_cache length after regen: {len(job_cache)} #notify-admin-1200" diff --git a/app/clients/__init__.py b/app/clients/__init__.py index 9c1f4af68..cfd50d88c 100644 --- a/app/clients/__init__.py +++ b/app/clients/__init__.py @@ -13,6 +13,7 @@ AWS_CLIENT_CONFIG = Config( "addressing_style": "virtual", }, use_fips_endpoint=True, + max_pool_connections=50, ) diff --git a/notifications_utils/logging.py b/notifications_utils/logging.py index 3ec092b8c..dc55ae653 100644 --- a/notifications_utils/logging.py +++ b/notifications_utils/logging.py @@ -39,6 +39,12 @@ def init_app(app): for logger_instance, handler in product(warning_loggers, handlers): logger_instance.addHandler(handler) logger_instance.setLevel(logging.WARNING) + + # Suppress specific loggers to prevent leaking sensitive info + logging.getLogger("boto3").setLevel(logging.ERROR) + logging.getLogger("botocore").setLevel(logging.ERROR) + logging.getLogger("urllib3").setLevel(logging.ERROR) + app.logger.info("Logging configured") From 0d82f89bb5c7b413c3b3d0dbd30b2ad0e95f87c7 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 27 Sep 2024 14:18:42 -0700 Subject: [PATCH 31/65] reduce max connections to 10 --- app/clients/__init__.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/clients/__init__.py b/app/clients/__init__.py index cfd50d88c..993054279 100644 --- a/app/clients/__init__.py +++ b/app/clients/__init__.py @@ -13,7 +13,10 @@ AWS_CLIENT_CONFIG = Config( "addressing_style": "virtual", }, use_fips_endpoint=True, - max_pool_connections=50, + # This is the default but just for doc sake + # there may come a time when increasing this helps + # with job cache management + max_pool_connections=10, ) From e0dab07300f8d13484f0b26d7518c977bb890146 Mon Sep 17 00:00:00 2001 From: Andrew Shumway Date: Mon, 30 Sep 2024 09:24:50 -0600 Subject: [PATCH 32/65] Fix env var syntax --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 17b597721..24ad41f99 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,7 +70,7 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var var-name=${{ secrets.LOGIN_DOT_GOV_REGISTRATION_URL }} --strategy rolling" + cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ DANGEROUS_SALT }} --var var-name=${{ SECRET_KEY }} --var var-name=${{ ADMIN_CLIENT_SECRET }} --var var-name=${{ NEW_RELIC_LICENSE_KEY }} --var var-name=${{ NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ NOTIFY_E2E_TEST_PASSWORD }} --var var-name=${{ LOGIN_DOT_GOV_REGISTRATION_URL }} --strategy rolling" - name: Check for changes to templates.json id: changed-templates From f844da8c68c98f5dbf271c2e35fce885d45aafd2 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Mon, 30 Sep 2024 08:38:49 -0700 Subject: [PATCH 33/65] update comment --- app/clients/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/clients/__init__.py b/app/clients/__init__.py index 993054279..19b719c1c 100644 --- a/app/clients/__init__.py +++ b/app/clients/__init__.py @@ -15,7 +15,7 @@ AWS_CLIENT_CONFIG = Config( use_fips_endpoint=True, # This is the default but just for doc sake # there may come a time when increasing this helps - # with job cache management + # with job cache management. max_pool_connections=10, ) From 6b861f31d70e1a5f658d1ced7281be343083c090 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Mon, 30 Sep 2024 08:45:15 -0700 Subject: [PATCH 34/65] fix debug --- app/aws/s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index f358a13f3..cc25f168a 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -278,7 +278,7 @@ def get_job_from_s3(service_id, job_id): "SlowDown", ]: current_app.logger.exception( - f"Retrying job fetch retry_count={retries}", + f"Retrying job fetch service_id {service_id} job_id {job_id} retry_count={retries}", ) retries += 1 sleep_time = backoff_factor * (2**retries) # Exponential backoff From 544e7e61e49be84724869c7011a5cea3ad7824c9 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Mon, 30 Sep 2024 09:08:18 -0700 Subject: [PATCH 35/65] code review feedback --- app/aws/s3.py | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 5e9662965..256800bf9 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -164,20 +164,13 @@ def read_s3_file(bucket_name, object_key, s3res): .read() .decode("utf-8") ) - # TODO we probably don't need this phone number check anymore. - # Originally, there were csv uploads that were not valid - # for messaging sending. They may have been experimental, or - # they may have not been caught by validation. When we're sure - # all csv files have a phone number column, we can remove the if. - # We are essentially just making sure this object looks like a job. - if "phone number" in object.lower(): - set_job_cache(job_cache, job_id, object) - set_job_cache(job_cache, f"{job_id}_phones", extract_phones(object)) - set_job_cache( - job_cache, - f"{job_id}_personalisation", - extract_personalisation(object), - ) + set_job_cache(job_cache, job_id, object) + set_job_cache(job_cache, f"{job_id}_phones", extract_phones(object)) + set_job_cache( + job_cache, + f"{job_id}_personalisation", + extract_personalisation(object), + ) except LookupError: # perhaps our key is not formatted as we expected. If so skip it. From f4a07621ef115de081a1a92dcc07573fdee42f0f Mon Sep 17 00:00:00 2001 From: Andrew Shumway Date: Mon, 30 Sep 2024 13:36:05 -0600 Subject: [PATCH 36/65] Fix syntax on cf command --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 24ad41f99..801886ad9 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,7 +70,7 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ DANGEROUS_SALT }} --var var-name=${{ SECRET_KEY }} --var var-name=${{ ADMIN_CLIENT_SECRET }} --var var-name=${{ NEW_RELIC_LICENSE_KEY }} --var var-name=${{ NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ NOTIFY_E2E_TEST_PASSWORD }} --var var-name=${{ LOGIN_DOT_GOV_REGISTRATION_URL }} --strategy rolling" + cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=$DANGEROUS_SALT --var var-name=$SECRET_KEY --var var-name=$ADMIN_CLIENT_SECRET --var var-name=$NEW_RELIC_LICENSE_KEY --var var-name=$NOTIFY_E2E_TEST_EMAIL --var var-name=$NOTIFY_E2E_TEST_PASSWORD --var var-name=$LOGIN_DOT_GOV_REGISTRATION_URL --strategy rolling" - name: Check for changes to templates.json id: changed-templates From 70dc52be81d3a51335315d3d9aa189d8a7f309bf Mon Sep 17 00:00:00 2001 From: Andrew Shumway Date: Mon, 30 Sep 2024 14:38:01 -0600 Subject: [PATCH 37/65] Go back to brackets with secrets. in command --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 801886ad9..72496fc7a 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,7 +70,7 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=$DANGEROUS_SALT --var var-name=$SECRET_KEY --var var-name=$ADMIN_CLIENT_SECRET --var var-name=$NEW_RELIC_LICENSE_KEY --var var-name=$NOTIFY_E2E_TEST_EMAIL --var var-name=$NOTIFY_E2E_TEST_PASSWORD --var var-name=$LOGIN_DOT_GOV_REGISTRATION_URL --strategy rolling" + cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var var-name=${{ LOGIN_DOT_GOV_REGISTRATION_URL }} --strategy rolling" - name: Check for changes to templates.json id: changed-templates From 1c231b1bbebe88c8ec9464f3feb3955ce824aeaa Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Mon, 30 Sep 2024 17:27:05 -0400 Subject: [PATCH 38/65] Adjust LOGIN_DOT_GOV_REGISTRATION_URL env var in staging deploy This changeset will hopefully fix the reference to the LOGIN_DOT_GOV_REGISTRATION_URL env var in the new cf_command. Signed-off-by: Carlo Costino --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 72496fc7a..631ec8d52 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,7 +70,7 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var var-name=${{ LOGIN_DOT_GOV_REGISTRATION_URL }} --strategy rolling" + cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var LOGIN_DOT_GOV_REGISTRATION_URL=https://secure.login.gov/openid_connect/authorize?acr_values=http%3A%2F%2Fidmanagement.gov%2Fns%2Fassurance%2Fial%2F1&client_id=urn:gov:gsa:openidconnect.profiles:sp:sso:gsa:notify-gov&nonce=NONCE&prompt=select_account&redirect_uri=https://notify-staging.app.cloud.gov/set-up-your-profile&response_type=code&scope=openid+email&state=STATE --strategy rolling" - name: Check for changes to templates.json id: changed-templates From 1f4905efa05a58ec7c2359f36c825687f377678e Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Tue, 1 Oct 2024 09:50:14 -0400 Subject: [PATCH 39/65] Refer to GitHub Action container environment variable This changeset attempts to fix another issue with the deploy command by setting and referring to an environment variable directly. Signed-off-by: Carlo Costino --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 631ec8d52..0dfbbc603 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,7 +70,7 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var LOGIN_DOT_GOV_REGISTRATION_URL=https://secure.login.gov/openid_connect/authorize?acr_values=http%3A%2F%2Fidmanagement.gov%2Fns%2Fassurance%2Fial%2F1&client_id=urn:gov:gsa:openidconnect.profiles:sp:sso:gsa:notify-gov&nonce=NONCE&prompt=select_account&redirect_uri=https://notify-staging.app.cloud.gov/set-up-your-profile&response_type=code&scope=openid+email&state=STATE --strategy rolling" + cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var LOGIN_DOT_GOV_REGISTRATION_URL=$LOGIN_DOT_GOV_REGISTRATION_URL --strategy rolling" - name: Check for changes to templates.json id: changed-templates From 76eb5281cf7d2e6a7b06391373a04abcdd35564c Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 1 Oct 2024 07:31:51 -0700 Subject: [PATCH 40/65] add documentation for how to rotate DANGEROUS_SALT --- docs/all.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/all.md b/docs/all.md index 3e576b0f2..530c7ca43 100644 --- a/docs/all.md +++ b/docs/all.md @@ -1242,6 +1242,17 @@ Notify.gov DNS records are maintained within [the 18f/dns repository](https://gi - Rename to `api_static_scan_DATE.zip` and add it to 🔒 https://drive.google.com/drive/folders/1dSe9H7Ag_hLfi5hmQDB2ktWaDwWSf4_R - Repeat for https://github.com/GSA/notifications-admin/actions/workflows/daily_checks.yml +## Rotating the DANGEROUS_SALT + + + 1. Start API locally `make run-procfile` + 2. In a separate terminal tab, navigate to the API project and run `poetry run flask command generate-salt` + 3. A random secret will appear in the tab + 4. Go to github->settings->secrets and variables->actions in the admin project and find the DANGEROUS_SALT secret for the admin project for staging. Open it and paste the result of #3 into the secret and save. Repeat for the API project, for staging. + 5. Repeat #3 and #4 but do it for demo + 6. Repeat #3 and #4 but do it for production + +The important thing is to use the same secret for Admin and API on each tier--i.e. you only generate three secrets. ## Known Gotchas From c2ed8f1686d788a3b094c4f42127f69a86ac381e Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 1 Oct 2024 10:47:47 -0700 Subject: [PATCH 41/65] fix personalization_bug for one-offs --- app/aws/s3.py | 18 +++---------- app/delivery/send_to_providers.py | 43 ++++++++++++++++++++++--------- 2 files changed, 34 insertions(+), 27 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 256800bf9..3f2115e5b 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -366,7 +366,6 @@ def extract_phones(job): def extract_personalisation(job): - job = job[0].split("\r\n") first_row = job[0] job.pop(0) @@ -398,11 +397,8 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number): ) return "Unavailable" - # If we look in the job_cache for the quick lookup dictionary of phones for a given job - # and that dictionary is not there, create it - if job_cache.get(f"{job_id}_phones") is None: - phones = extract_phones(job) - set_job_cache(job_cache, f"{job_id}_phones", phones) + phones = extract_phones(job) + set_job_cache(job_cache, f"{job_id}_phones", phones) # If we can find the quick dictionary, use it phone_to_return = phones[job_row_number] @@ -420,9 +416,6 @@ def get_personalisation_from_s3(service_id, job_id, job_row_number): # At the same time we don't want to store it in redis or the db # So this is a little recycling mechanism to reduce the number of downloads. job = job_cache.get(job_id) - if job is None: - job = get_job_from_s3(service_id, job_id) - set_job_cache(job_cache, job_id, job) # If the job is None after our attempt to retrieve it from s3, it # probably means the job is old and has been deleted from s3, in @@ -435,12 +428,7 @@ def get_personalisation_from_s3(service_id, job_id, job_row_number): ) return {} - # If we look in the job_cache for the quick lookup dictionary of personalisations for a given job - # and that dictionary is not there, create it - if job_cache.get(f"{job_id}_personalisation") is None: - set_job_cache( - job_cache, f"{job_id}_personalisation", extract_personalisation(job) - ) + set_job_cache(job_cache, f"{job_id}_personalisation", extract_personalisation(job)) # If we can find the quick dictionary, use it if job_cache.get(f"{job_id}_personalisation") is not None: diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index fbea9a2f7..f0c9391ed 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -13,7 +13,11 @@ from app import ( notification_provider_clients, redis_store, ) -from app.aws.s3 import get_personalisation_from_s3, get_phone_number_from_s3 +from app.aws.s3 import ( + get_job_from_s3, + get_personalisation_from_s3, + get_phone_number_from_s3, +) from app.celery.test_key_tasks import send_email_response, send_sms_response from app.dao.email_branding_dao import dao_get_email_branding_by_id from app.dao.notifications_dao import dao_update_notification @@ -36,17 +40,32 @@ def send_sms_to_provider(notification): Get data for recipient, template, notification and send it to sns. """ - # we no longer store the personalisation in the db, - # need to retrieve from s3 before generating content - # However, we are still sending the initial verify code through personalisation - # so if there is some value there, don't overwrite it - if not notification.personalisation: - personalisation = get_personalisation_from_s3( - notification.service_id, - notification.job_id, - notification.job_row_number, - ) - notification.personalisation = personalisation + # Take this path for report generation, where we know + # everything is in the cache. + personalisation = get_personalisation_from_s3( + notification.service_id, + notification.job_id, + notification.job_row_number, + ) + + # For one-off sends, they may not get into the cache + # by the time we get here, so do this slow direct-from-s3 + # approach. It is okay to be slow, since one-offs have + # to be typed in by hand. + if personalisation == {}: + job = get_job_from_s3(notification.service_id, notification.job_id) + job = job.split("\r\n") + first_row = job[0] + job.pop(0) + first_row = first_row.split(",") + personalisation = {} + job_row = 0 + for row in job: + row = row.split(",") + temp = dict(zip(first_row, row)) + personalisation[job_row] = temp + job_row = job_row + 1 + notification.personalisation = personalisation[notification.job_row_number] service = SerialisedService.from_id(notification.service_id) message_id = None From f55c437c7dc79f291628e9b68d74f702ff03e9ed Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 1 Oct 2024 11:36:09 -0700 Subject: [PATCH 42/65] try fixing tests --- app/delivery/send_to_providers.py | 4 +++- tests/app/delivery/test_send_to_providers.py | 3 ++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index f0c9391ed..c0670cee9 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -65,7 +65,9 @@ def send_sms_to_provider(notification): temp = dict(zip(first_row, row)) personalisation[job_row] = temp job_row = job_row + 1 - notification.personalisation = personalisation[notification.job_row_number] + notification.personalisation = personalisation[notification.job_row_number] + else: + notification.personalisation = personalisation service = SerialisedService.from_id(notification.service_id) message_id = None diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 1c8b9a111..c9660c95d 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -649,7 +649,8 @@ def test_should_update_billable_units_and_status_according_to_research_mode_and_ mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + # So we don't treat it as a one off and have to mock other things + mock_personalisation.return_value = {"ignore": "ignore"} send_to_providers.send_sms_to_provider(notification) assert notification.billable_units == billable_units From b7a6f4a3ba8b531758c19bced797be19ed1e80db Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 1 Oct 2024 11:45:52 -0700 Subject: [PATCH 43/65] try fixing tests --- tests/app/delivery/test_send_to_providers.py | 22 ++++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index c9660c95d..1bfdaab0a 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -201,7 +201,7 @@ def test_should_not_send_sms_message_when_service_is_inactive_notification_is_in mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore": "ignore"} with pytest.raises(NotificationTechnicalFailureException) as e: send_to_providers.send_sms_to_provider(sample_notification) @@ -232,7 +232,7 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( mock_s3_p = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_s3_p.return_value = {} + mock_s3_p.return_value = {"ignore": "ignore"} mocker.patch("app.aws_sns_client.send_sms") @@ -286,7 +286,7 @@ def test_should_have_sending_status_if_fake_callback_function_fails( mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore": "ignore"} sample_notification.key_type = KeyType.TEST with pytest.raises(HTTPError): @@ -311,7 +311,7 @@ def test_should_not_send_to_provider_when_status_is_not_created( mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore": "ignore"} send_to_providers.send_sms_to_provider(notification) @@ -376,7 +376,7 @@ def test_send_sms_should_use_service_sms_sender( mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore": "ignore"} send_to_providers.send_sms_to_provider( db_notification, @@ -408,7 +408,7 @@ def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_c mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore": "ignore"} send_to_providers.send_sms_to_provider(notification) app.aws_ses_client.send_email.assert_not_called() app.delivery.send_to_providers.send_email_response.assert_not_called() @@ -672,7 +672,7 @@ def test_should_set_notification_billable_units_and_reduces_provider_priority_if mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore": "ignore"} # flake8 no longer likes raises with a generic exception try: @@ -707,7 +707,7 @@ def test_should_send_sms_to_international_providers( mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore": "ignore"} send_to_providers.send_sms_to_provider(notification_international) @@ -753,7 +753,7 @@ def test_should_handle_sms_sender_and_prefix_message( mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore": "ignore"} send_to_providers.send_sms_to_provider(notification) @@ -814,7 +814,7 @@ def test_send_sms_to_provider_should_use_normalised_to(mocker, client, sample_te mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore": "ignore"} send_to_providers.send_sms_to_provider(notification) send_mock.assert_called_once_with( to="12028675309", @@ -894,7 +894,7 @@ def test_send_sms_to_provider_should_return_template_if_found_in_redis( mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {} + mock_personalisation.return_value = {"ignore: ignore"} send_to_providers.send_sms_to_provider(notification) assert mock_get_template.called is False From 9c43329d4dab60f12f05480feb769a8af52e9ced Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 1 Oct 2024 12:08:57 -0700 Subject: [PATCH 44/65] fix test --- tests/app/delivery/test_send_to_providers.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 1bfdaab0a..4434dbd2d 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -269,7 +269,6 @@ def test_send_sms_should_use_template_version_from_notification_not_latest( assert persisted_notification.template_version == version_on_notification assert persisted_notification.template_version != t.version assert persisted_notification.status == NotificationStatus.SENDING - assert not persisted_notification.personalisation def test_should_have_sending_status_if_fake_callback_function_fails( @@ -894,7 +893,7 @@ def test_send_sms_to_provider_should_return_template_if_found_in_redis( mock_personalisation = mocker.patch( "app.delivery.send_to_providers.get_personalisation_from_s3" ) - mock_personalisation.return_value = {"ignore: ignore"} + mock_personalisation.return_value = {"ignore": "ignore"} send_to_providers.send_sms_to_provider(notification) assert mock_get_template.called is False From 641d168370d1a3ee23998c16cef2bd5af66c4204 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 1 Oct 2024 12:58:31 -0700 Subject: [PATCH 45/65] fix properly --- app/aws/s3.py | 13 +++++++++++-- app/delivery/send_to_providers.py | 28 ++-------------------------- 2 files changed, 13 insertions(+), 28 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 3f2115e5b..a6b9ba01f 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -366,7 +366,9 @@ def extract_phones(job): def extract_personalisation(job): - job = job[0].split("\r\n") + if isinstance(job, dict): + job = job[0] + job = job.split("\r\n") first_row = job[0] job.pop(0) first_row = first_row.split(",") @@ -416,7 +418,14 @@ def get_personalisation_from_s3(service_id, job_id, job_row_number): # At the same time we don't want to store it in redis or the db # So this is a little recycling mechanism to reduce the number of downloads. job = job_cache.get(job_id) - + if job is None: + current_app.logger.info(f"job {job_id} was not in the cache") + job = get_job_from_s3(service_id, job_id) + # Even if it is None, put it here to avoid KeyErrors + set_job_cache(job_cache, job_id, job) + else: + # skip expiration date from cache, we don't need it here + job = job[0] # If the job is None after our attempt to retrieve it from s3, it # probably means the job is old and has been deleted from s3, in # which case there is nothing we can do. It's unlikely to run into diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index c0670cee9..745b46cab 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -13,11 +13,7 @@ from app import ( notification_provider_clients, redis_store, ) -from app.aws.s3 import ( - get_job_from_s3, - get_personalisation_from_s3, - get_phone_number_from_s3, -) +from app.aws.s3 import get_personalisation_from_s3, get_phone_number_from_s3 from app.celery.test_key_tasks import send_email_response, send_sms_response from app.dao.email_branding_dao import dao_get_email_branding_by_id from app.dao.notifications_dao import dao_update_notification @@ -47,27 +43,7 @@ def send_sms_to_provider(notification): notification.job_id, notification.job_row_number, ) - - # For one-off sends, they may not get into the cache - # by the time we get here, so do this slow direct-from-s3 - # approach. It is okay to be slow, since one-offs have - # to be typed in by hand. - if personalisation == {}: - job = get_job_from_s3(notification.service_id, notification.job_id) - job = job.split("\r\n") - first_row = job[0] - job.pop(0) - first_row = first_row.split(",") - personalisation = {} - job_row = 0 - for row in job: - row = row.split(",") - temp = dict(zip(first_row, row)) - personalisation[job_row] = temp - job_row = job_row + 1 - notification.personalisation = personalisation[notification.job_row_number] - else: - notification.personalisation = personalisation + notification.personalisation = personalisation service = SerialisedService.from_id(notification.service_id) message_id = None From a70b4506bb04c6225accc5b44548aa5bc1e8fe94 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 1 Oct 2024 13:08:33 -0700 Subject: [PATCH 46/65] revert tests --- tests/app/delivery/test_send_to_providers.py | 1242 ++++-------------- 1 file changed, 280 insertions(+), 962 deletions(-) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 4434dbd2d..fbea9a2f7 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -1,990 +1,308 @@ import json -from collections import namedtuple -from unittest.mock import ANY +import os +from contextlib import suppress +from urllib import parse -import pytest +from cachetools import TTLCache, cached from flask import current_app -from requests import HTTPError -import app -from app import aws_sns_client, notification_provider_clients -from app.cloudfoundry_config import cloud_config -from app.dao import notifications_dao -from app.dao.provider_details_dao import get_provider_details_by_identifier -from app.delivery import send_to_providers -from app.delivery.send_to_providers import get_html_email_options, get_logo_url +from app import ( + aws_pinpoint_client, + create_uuid, + db, + notification_provider_clients, + redis_store, +) +from app.aws.s3 import get_personalisation_from_s3, get_phone_number_from_s3 +from app.celery.test_key_tasks import send_email_response, send_sms_response +from app.dao.email_branding_dao import dao_get_email_branding_by_id +from app.dao.notifications_dao import dao_update_notification +from app.dao.provider_details_dao import get_provider_details_by_notification_type +from app.dao.service_sms_sender_dao import dao_get_sms_senders_by_service_id from app.enums import BrandType, KeyType, NotificationStatus, NotificationType from app.exceptions import NotificationTechnicalFailureException -from app.models import EmailBranding, Notification -from app.serialised_models import SerialisedService -from app.utils import utc_now -from tests.app.db import ( - create_email_branding, - create_notification, - create_reply_to_email, - create_service, - create_service_sms_sender, - create_service_with_defined_sms_sender, - create_template, +from app.serialised_models import SerialisedService, SerialisedTemplate +from app.utils import hilite, utc_now +from notifications_utils.template import ( + HTMLEmailTemplate, + PlainTextEmailTemplate, + SMSMessageTemplate, ) -def setup_function(_function): - # pytest will run this function before each test. It makes sure the - # state of the cache is not shared between tests. - send_to_providers.provider_cache.clear() - - -@pytest.mark.parametrize( - "international_provider_priority", - ( - # Since there’s only one international provider it should always - # be used, no matter what its priority is set to - 0, - 50, - 100, - ), -) -def test_provider_to_use_should_only_return_sns_for_international( - mocker, - notify_db_session, - international_provider_priority, -): - sns = get_provider_details_by_identifier("sns") - sns.priority = international_provider_priority - - ret = send_to_providers.provider_to_use(NotificationType.SMS, international=True) - - assert ret.name == "sns" - - -def test_provider_to_use_raises_if_no_active_providers( - mocker, restore_provider_details -): - sns = get_provider_details_by_identifier("sns") - sns.active = False - - # flake8 doesn't like raises with a generic exception - try: - send_to_providers.provider_to_use(NotificationType.SMS) - assert 1 == 0 - except Exception: - assert 1 == 1 - - -def test_should_send_personalised_template_to_correct_sms_provider_and_persist( - sample_sms_template_with_html, mocker -): - - mocker.patch("app.delivery.send_to_providers._get_verify_code", return_value=None) - db_notification = create_notification( - template=sample_sms_template_with_html, - personalisation={}, - status=NotificationStatus.CREATED, - reply_to_text=sample_sms_template_with_html.service.get_default_sms_sender(), - ) - - mocker.patch("app.aws_sns_client.send_sms") - - mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_s3.return_value = "2028675309" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"name": "Jo"} - - send_to_providers.send_sms_to_provider(db_notification) - - aws_sns_client.send_sms.assert_called_once_with( - to="2028675309", - content="Sample service: Hello Jo\nHere is some HTML & entities", - reference=str(db_notification.id), - sender=current_app.config["FROM_NUMBER"], - international=False, - ) - - notification = Notification.query.filter_by(id=db_notification.id).one() - - assert notification.status == NotificationStatus.SENDING - assert notification.sent_at <= utc_now() - assert notification.sent_by == "sns" - assert notification.billable_units == 1 - assert notification.personalisation == {"name": "Jo"} - - -def test_should_send_personalised_template_to_correct_email_provider_and_persist( - sample_email_template_with_html, mocker -): - - mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") - utf8_encoded_email = "jo.smith@example.com".encode("utf-8") - mock_redis.get.return_value = utf8_encoded_email - email = utf8_encoded_email - personalisation = { - "name": "Jo", - } - personalisation = json.dumps(personalisation) - personalisation = personalisation.encode("utf-8") - mock_redis.get.side_effect = [email, personalisation] - db_notification = create_notification( - template=sample_email_template_with_html, - ) - db_notification.personalisation = {"name": "Jo"} - mocker.patch("app.aws_ses_client.send_email", return_value="reference") - send_to_providers.send_email_to_provider(db_notification) - app.aws_ses_client.send_email.assert_called_once_with( - f'"Sample service" ', - "jo.smith@example.com", - "Jo some HTML", - body="Hello Jo\nThis is an email from GOV.\u200bUK with some HTML\n", - html_body=ANY, - reply_to_address=None, - ) - - assert " version_on_notification - - send_to_providers.send_sms_to_provider(db_notification) - - aws_sns_client.send_sms.assert_called_once_with( - to="2028675309", - content="Sample service: This is a template:\nwith a newline", - reference=str(db_notification.id), - sender=current_app.config["FROM_NUMBER"], - international=False, - ) - - t = dao_get_template_by_id(expected_template_id) - - persisted_notification = notifications_dao.get_notification_by_id( - db_notification.id - ) - assert persisted_notification.to == db_notification.to - assert persisted_notification.template_id == expected_template_id - assert persisted_notification.template_version == version_on_notification - assert persisted_notification.template_version != t.version - assert persisted_notification.status == NotificationStatus.SENDING - - -def test_should_have_sending_status_if_fake_callback_function_fails( - sample_notification, mocker -): - mocker.patch( - "app.delivery.send_to_providers.send_sms_response", - side_effect=HTTPError, - ) - - mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_s3.return_value = "2028675309" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"ignore": "ignore"} - - sample_notification.key_type = KeyType.TEST - with pytest.raises(HTTPError): - send_to_providers.send_sms_to_provider(sample_notification) - assert sample_notification.status == NotificationStatus.SENDING - assert sample_notification.sent_by == "sns" - - -def test_should_not_send_to_provider_when_status_is_not_created( - sample_template, mocker -): - notification = create_notification( - template=sample_template, - status=NotificationStatus.SENDING, - ) - mocker.patch("app.aws_sns_client.send_sms") - response_mock = mocker.patch("app.delivery.send_to_providers.send_sms_response") - - mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_s3.return_value = "2028675309" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"ignore": "ignore"} - - send_to_providers.send_sms_to_provider(notification) - - app.aws_sns_client.send_sms.assert_not_called() - response_mock.assert_not_called() - - -def test_should_send_sms_with_downgraded_content(notify_db_session, mocker): - # é, o, and u are in GSM. - # ī, grapes, tabs, zero width space and ellipsis are not - # ó isn't in GSM, but it is in the welsh alphabet so will still be sent - - mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) - mocker.patch( - "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] - ) - msg = "a é ī o u 🍇 foo\tbar\u200bbaz((misc))…" - placeholder = "∆∆∆abc" - gsm_message = "?ódz Housing Service: a é i o u ? foo barbaz???abc..." - service = create_service(service_name="Łódź Housing Service") - template = create_template(service, content=msg) - db_notification = create_notification( - template=template, - ) - db_notification.personalisation = {"misc": placeholder} - db_notification.reply_to_text = "testing" - - mocker.patch("app.aws_sns_client.send_sms") - - mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_phone.return_value = "15555555555" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"misc": placeholder} - - send_to_providers.send_sms_to_provider(db_notification) - - aws_sns_client.send_sms.assert_called_once_with( - to=ANY, content=gsm_message, reference=ANY, sender=ANY, international=False - ) - - -def test_send_sms_should_use_service_sms_sender( - sample_service, sample_template, mocker -): - - mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) - mocker.patch("app.aws_sns_client.send_sms") - - sms_sender = create_service_sms_sender( - service=sample_service, sms_sender="123456", is_default=False - ) - db_notification = create_notification( - template=sample_template, reply_to_text=sms_sender.sms_sender - ) - expected_sender_name = sms_sender.sms_sender - mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_phone.return_value = "15555555555" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"ignore": "ignore"} - - send_to_providers.send_sms_to_provider( - db_notification, - ) - - app.aws_sns_client.send_sms.assert_called_once_with( - to=ANY, - content=ANY, - reference=ANY, - sender=expected_sender_name, - international=False, - ) - - -def test_send_email_to_provider_should_not_send_to_provider_when_status_is_not_created( - sample_email_template, mocker -): - mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") - mock_redis.get.return_value = "test@example.com".encode("utf-8") - - notification = create_notification( - template=sample_email_template, status=NotificationStatus.SENDING - ) - mocker.patch("app.aws_ses_client.send_email") - mocker.patch("app.delivery.send_to_providers.send_email_response") - mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_phone.return_value = "15555555555" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"ignore": "ignore"} - send_to_providers.send_sms_to_provider(notification) - app.aws_ses_client.send_email.assert_not_called() - app.delivery.send_to_providers.send_email_response.assert_not_called() - - -def test_send_email_should_use_service_reply_to_email( - sample_service, sample_email_template, mocker -): - mocker.patch("app.aws_ses_client.send_email", return_value="reference") - - mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") - mock_redis.get.return_value = "test@example.com".encode("utf-8") - - mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") - email = "foo@bar.com".encode("utf-8") - personalisation = {} - - personalisation = json.dumps(personalisation) - personalisation = personalisation.encode("utf-8") - mock_redis.get.side_effect = [email, personalisation] - - db_notification = create_notification( - template=sample_email_template, reply_to_text="foo@bar.com" - ) - create_reply_to_email(service=sample_service, email_address="foo@bar.com") - - send_to_providers.send_email_to_provider(db_notification) - - app.aws_ses_client.send_email.assert_called_once_with( - ANY, - ANY, - ANY, - body=ANY, - html_body=ANY, - reply_to_address="foo@bar.com", - ) - - -def test_get_html_email_renderer_should_return_for_normal_service(sample_service): - options = send_to_providers.get_html_email_options(sample_service) - assert options["govuk_banner"] is True - assert "brand_colour" not in options.keys() - assert "brand_logo" not in options.keys() - assert "brand_text" not in options.keys() - assert "brand_name" not in options.keys() - - -@pytest.mark.parametrize( - "branding_type, govuk_banner", - [(BrandType.ORG, False), (BrandType.BOTH, True), (BrandType.ORG_BANNER, False)], -) -def test_get_html_email_renderer_with_branding_details( - branding_type, govuk_banner, notify_db_session, sample_service -): - email_branding = EmailBranding( - brand_type=branding_type, - colour="#000000", - logo="justice-league.png", - name="Justice League", - text="League of Justice", - ) - sample_service.email_branding = email_branding - notify_db_session.add_all([sample_service, email_branding]) - notify_db_session.commit() - - options = send_to_providers.get_html_email_options(sample_service) - - assert options["govuk_banner"] == govuk_banner - assert options["brand_colour"] == "#000000" - assert options["brand_text"] == "League of Justice" - assert options["brand_name"] == "Justice League" - - if branding_type == BrandType.ORG_BANNER: - assert options["brand_banner"] is True - else: - assert options["brand_banner"] is False - - -def test_get_html_email_renderer_with_branding_details_and_render_govuk_banner_only( - notify_db_session, sample_service -): - sample_service.email_branding = None - notify_db_session.add_all([sample_service]) - notify_db_session.commit() - - options = send_to_providers.get_html_email_options(sample_service) - - assert options == {"govuk_banner": True, "brand_banner": False} - - -def test_get_html_email_renderer_prepends_logo_path(notify_api): - Service = namedtuple("Service", ["email_branding"]) - EmailBranding = namedtuple( - "EmailBranding", - ["brand_type", "colour", "name", "logo", "text"], - ) - - email_branding = EmailBranding( - brand_type=BrandType.ORG, - colour="#000000", - logo="justice-league.png", - name="Justice League", - text="League of Justice", - ) - service = Service( - email_branding=email_branding, - ) - - renderer = send_to_providers.get_html_email_options(service) - - assert ( - renderer["brand_logo"] == "http://static-logos.notify.tools/justice-league.png" - ) - - -def test_get_html_email_renderer_handles_email_branding_without_logo(notify_api): - Service = namedtuple("Service", ["email_branding"]) - EmailBranding = namedtuple( - "EmailBranding", - ["brand_type", "colour", "name", "logo", "text"], - ) - - email_branding = EmailBranding( - brand_type=BrandType.ORG_BANNER, - colour="#000000", - logo=None, - name="Justice League", - text="League of Justice", - ) - service = Service( - email_branding=email_branding, - ) - - renderer = send_to_providers.get_html_email_options(service) - - assert renderer["govuk_banner"] is False - assert renderer["brand_banner"] is True - assert renderer["brand_logo"] is None - assert renderer["brand_text"] == "League of Justice" - assert renderer["brand_colour"] == "#000000" - assert renderer["brand_name"] == "Justice League" - - -@pytest.mark.parametrize( - "base_url, expected_url", - [ - # don't change localhost to prevent errors when testing locally - ("http://localhost:6012", "http://static-logos.notify.tools/filename.png"), - ( - "https://www.notifications.service.gov.uk", - "https://static-logos.notifications.service.gov.uk/filename.png", - ), - ("https://notify.works", "https://static-logos.notify.works/filename.png"), - ( - "https://staging-notify.works", - "https://static-logos.staging-notify.works/filename.png", - ), - ("https://www.notify.works", "https://static-logos.notify.works/filename.png"), - ( - "https://www.staging-notify.works", - "https://static-logos.staging-notify.works/filename.png", - ), - ], -) -def test_get_logo_url_works_for_different_environments(base_url, expected_url): - logo_file = "filename.png" - - logo_url = send_to_providers.get_logo_url(base_url, logo_file) - - assert logo_url == expected_url - - -@pytest.mark.parametrize( - "starting_status, expected_status", - [ - (NotificationStatus.DELIVERED, NotificationStatus.DELIVERED), - (NotificationStatus.CREATED, NotificationStatus.SENDING), - (NotificationStatus.TECHNICAL_FAILURE, NotificationStatus.TECHNICAL_FAILURE), - ], -) -def test_update_notification_to_sending_does_not_update_status_from_a_final_status( - sample_service, notify_db_session, starting_status, expected_status -): - template = create_template(sample_service) - notification = create_notification(template=template, status=starting_status) - send_to_providers.update_notification_to_sending( - notification, - notification_provider_clients.get_client_by_name_and_type( - "sns", NotificationType.SMS - ), - ) - assert notification.status == expected_status - - -def __update_notification(notification_to_update, research_mode, expected_status): - if research_mode or notification_to_update.key_type == KeyType.TEST: - notification_to_update.status = expected_status - - -@pytest.mark.parametrize( - "research_mode,key_type, billable_units, expected_status", - [ - (True, KeyType.NORMAL, 0, NotificationStatus.DELIVERED), - (False, KeyType.NORMAL, 1, NotificationStatus.SENDING), - (False, KeyType.TEST, 0, NotificationStatus.SENDING), - (True, KeyType.TEST, 0, NotificationStatus.SENDING), - (True, KeyType.TEAM, 0, NotificationStatus.DELIVERED), - (False, KeyType.TEAM, 1, NotificationStatus.SENDING), - ], -) -def test_should_update_billable_units_and_status_according_to_research_mode_and_key_type( - sample_template, mocker, research_mode, key_type, billable_units, expected_status -): - - mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) - mocker.patch( - "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] - ) - notification = create_notification( - template=sample_template, - billable_units=0, - status=NotificationStatus.CREATED, - key_type=key_type, - reply_to_text="testing", - ) - mocker.patch("app.aws_sns_client.send_sms") - mocker.patch( - "app.delivery.send_to_providers.send_sms_response", - side_effect=__update_notification(notification, research_mode, expected_status), - ) - - if research_mode: - sample_template.service.research_mode = True - - mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_phone.return_value = "15555555555" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - # So we don't treat it as a one off and have to mock other things - mock_personalisation.return_value = {"ignore": "ignore"} - - send_to_providers.send_sms_to_provider(notification) - assert notification.billable_units == billable_units - assert notification.status == expected_status - - -def test_should_set_notification_billable_units_and_reduces_provider_priority_if_sending_to_provider_fails( - sample_notification, - mocker, -): - mocker.patch("app.aws_sns_client.send_sms", side_effect=Exception()) - - sample_notification.billable_units = 0 - assert sample_notification.sent_by is None - - mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_phone.return_value = "15555555555" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"ignore": "ignore"} - - # flake8 no longer likes raises with a generic exception - try: - send_to_providers.send_sms_to_provider(sample_notification) - assert 1 == 0 - except Exception: - assert 1 == 1 - - assert sample_notification.billable_units == 1 - - -def test_should_send_sms_to_international_providers( - sample_template, sample_user, mocker -): - - mocker.patch("app.delivery.send_to_providers._get_verify_code", return_value=None) - mocker.patch("app.aws_sns_client.send_sms") - - notification_international = create_notification( - template=sample_template, - to_field="+6011-17224412", - personalisation={"name": "Jo"}, - status=NotificationStatus.CREATED, - international=True, - reply_to_text=sample_template.service.get_default_sms_sender(), - normalised_to="601117224412", - ) - - mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_s3.return_value = "601117224412" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"ignore": "ignore"} - - send_to_providers.send_sms_to_provider(notification_international) - - aws_sns_client.send_sms.assert_called_once_with( - to="601117224412", - content=ANY, - reference=str(notification_international.id), - sender=current_app.config["FROM_NUMBER"], - international=True, - ) - - assert notification_international.status == NotificationStatus.SENDING - assert notification_international.sent_by == "sns" - - -@pytest.mark.parametrize( - "sms_sender, expected_sender, prefix_sms, expected_content", - [ - ("foo", "foo", False, "bar"), - ("foo", "foo", True, "Sample service: bar"), - # if 40604 is actually in DB then treat that as if entered manually - ("40604", "40604", False, "bar"), - # 'testing' is the FROM_NUMBER during unit tests - ("testing", "testing", True, "Sample service: bar"), - ("testing", "testing", False, "bar"), - ], -) -def test_should_handle_sms_sender_and_prefix_message( - mocker, sms_sender, prefix_sms, expected_sender, expected_content, notify_db_session -): - - mocker.patch("app.delivery.send_to_providers.redis_store", return_value=None) - mocker.patch("app.aws_sns_client.send_sms") - service = create_service_with_defined_sms_sender( - sms_sender_value=sms_sender, prefix_sms=prefix_sms - ) - template = create_template(service, content="bar") - notification = create_notification(template, reply_to_text=sms_sender) - - mock_phone = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_phone.return_value = "15555555555" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"ignore": "ignore"} - - send_to_providers.send_sms_to_provider(notification) - - aws_sns_client.send_sms.assert_called_once_with( - content=expected_content, - sender=expected_sender, - to=ANY, - reference=ANY, - international=False, - ) - - -def test_send_email_to_provider_uses_reply_to_from_notification( - sample_email_template, mocker -): - mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") - mock_redis.get.side_effect = [ - "test@example.com".encode("utf-8"), - json.dumps({}).encode("utf-8"), +def send_sms_to_provider(notification): + """Final step in the message send flow. + + Get data for recipient, template, + notification and send it to sns. + """ + # we no longer store the personalisation in the db, + # need to retrieve from s3 before generating content + # However, we are still sending the initial verify code through personalisation + # so if there is some value there, don't overwrite it + if not notification.personalisation: + personalisation = get_personalisation_from_s3( + notification.service_id, + notification.job_id, + notification.job_row_number, + ) + notification.personalisation = personalisation + + service = SerialisedService.from_id(notification.service_id) + message_id = None + if not service.active: + technical_failure(notification=notification) + return + + if notification.status == NotificationStatus.CREATED: + # We get the provider here (which is only aws sns) + provider = provider_to_use(NotificationType.SMS, notification.international) + if not provider: + technical_failure(notification=notification) + return + + template_model = SerialisedTemplate.from_id_and_service_id( + template_id=notification.template_id, + service_id=service.id, + version=notification.template_version, + ) + + template = SMSMessageTemplate( + template_model.__dict__, + values=notification.personalisation, + prefix=service.name, + show_prefix=service.prefix_sms, + ) + if notification.key_type == KeyType.TEST: + update_notification_to_sending(notification, provider) + send_sms_response(provider.name, str(notification.id)) + + else: + try: + # End DB session here so that we don't have a connection stuck open waiting on the call + # to one of the SMS providers + # We don't want to tie our DB connections being open to the performance of our SMS + # providers as a slow down of our providers can cause us to run out of DB connections + # Therefore we pull all the data from our DB models into `send_sms_kwargs`now before + # closing the session (as otherwise it would be reopened immediately) + + # We start by trying to get the phone number from a job in s3. If we fail, we assume + # the phone number is for the verification code on login, which is not a job. + recipient = None + # It is our 2facode, maybe + recipient = _get_verify_code(notification) + + if recipient is None: + recipient = get_phone_number_from_s3( + notification.service_id, + notification.job_id, + notification.job_row_number, + ) + + # TODO This is temporary to test the capability of validating phone numbers + # The future home of the validation is TBD + if "+" not in recipient: + recipient_lookup = f"+{recipient}" + else: + recipient_lookup = recipient + if recipient_lookup in current_app.config[ + "SIMULATED_SMS_NUMBERS" + ] and os.getenv("NOTIFY_ENVIRONMENT") in ["development", "test"]: + current_app.logger.info(hilite("#validate-phone-number fired")) + aws_pinpoint_client.validate_phone_number("01", recipient) + else: + current_app.logger.info(hilite("#validate-phone-number not fired")) + + sender_numbers = get_sender_numbers(notification) + if notification.reply_to_text not in sender_numbers: + raise ValueError( + f"{notification.reply_to_text} not in {sender_numbers} #notify-admin-1701" + ) + + send_sms_kwargs = { + "to": recipient, + "content": str(template), + "reference": str(notification.id), + "sender": notification.reply_to_text, + "international": notification.international, + } + db.session.close() # no commit needed as no changes to objects have been made above + + message_id = provider.send_sms(**send_sms_kwargs) + current_app.logger.info(f"got message_id {message_id}") + except Exception as e: + n = notification + msg = f"FAILED send to sms, job_id: {n.job_id} row_number {n.job_row_number} message_id {message_id}" + current_app.logger.exception(hilite(msg)) + + notification.billable_units = template.fragment_count + dao_update_notification(notification) + raise e + else: + # Here we map the job_id and row number to the aws message_id + n = notification + msg = f"Send to aws for job_id {n.job_id} row_number {n.job_row_number} message_id {message_id}" + current_app.logger.info(hilite(msg)) + notification.billable_units = template.fragment_count + update_notification_to_sending(notification, provider) + return message_id + + +def _get_verify_code(notification): + key = f"2facode-{notification.id}".replace(" ", "") + recipient = redis_store.get(key) + with suppress(AttributeError): + recipient = recipient.decode("utf-8") + return recipient + + +def get_sender_numbers(notification): + possible_senders = dao_get_sms_senders_by_service_id(notification.service_id) + sender_numbers = [] + for possible_sender in possible_senders: + sender_numbers.append(possible_sender.sms_sender) + return sender_numbers + + +def send_email_to_provider(notification): + # Someone needs an email, possibly new registration + recipient = redis_store.get(f"email-address-{notification.id}") + recipient = recipient.decode("utf-8") + personalisation = redis_store.get(f"email-personalisation-{notification.id}") + if personalisation: + personalisation = personalisation.decode("utf-8") + notification.personalisation = json.loads(personalisation) + + service = SerialisedService.from_id(notification.service_id) + if not service.active: + technical_failure(notification=notification) + return + if notification.status == NotificationStatus.CREATED: + provider = provider_to_use(NotificationType.EMAIL, False) + template_dict = SerialisedTemplate.from_id_and_service_id( + template_id=notification.template_id, + service_id=service.id, + version=notification.template_version, + ).__dict__ + + html_email = HTMLEmailTemplate( + template_dict, + values=notification.personalisation, + **get_html_email_options(service), + ) + + plain_text_email = PlainTextEmailTemplate( + template_dict, values=notification.personalisation + ) + + if notification.key_type == KeyType.TEST: + notification.reference = str(create_uuid()) + update_notification_to_sending(notification, provider) + send_email_response(notification.reference, recipient) + else: + from_address = ( + f'"{service.name}" <{service.email_from}@' + f'{current_app.config["NOTIFY_EMAIL_DOMAIN"]}>' + ) + + reference = provider.send_email( + from_address, + recipient, + plain_text_email.subject, + body=str(plain_text_email), + html_body=str(html_email), + reply_to_address=notification.reply_to_text, + ) + notification.reference = reference + update_notification_to_sending(notification, provider) + + +def update_notification_to_sending(notification, provider): + notification.sent_at = utc_now() + notification.sent_by = provider.name + if notification.status not in NotificationStatus.completed_types(): + notification.status = NotificationStatus.SENDING + + dao_update_notification(notification) + + +provider_cache = TTLCache(maxsize=8, ttl=10) + + +@cached(cache=provider_cache) +def provider_to_use(notification_type, international=True): + active_providers = [ + p + for p in get_provider_details_by_notification_type( + notification_type, international + ) + if p.active ] - mocker.patch("app.aws_ses_client.send_email", return_value="reference") + if not active_providers: + current_app.logger.error(f"{notification_type} failed as no active providers") + raise Exception(f"No active {notification_type} providers") - db_notification = create_notification( - template=sample_email_template, - reply_to_text="test@test.com", - ) + # we only have sns + chosen_provider = active_providers[0] - send_to_providers.send_email_to_provider(db_notification) - - app.aws_ses_client.send_email.assert_called_once_with( - ANY, - ANY, - ANY, - body=ANY, - html_body=ANY, - reply_to_address="test@test.com", + return notification_provider_clients.get_client_by_name_and_type( + chosen_provider.identifier, notification_type ) -def test_send_sms_to_provider_should_use_normalised_to(mocker, client, sample_template): +def get_logo_url(base_url, logo_file): + base_url = parse.urlparse(base_url) + netloc = base_url.netloc - mocker.patch("app.delivery.send_to_providers._get_verify_code", return_value=None) - mocker.patch( - "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] + if base_url.netloc.startswith("localhost"): + netloc = "notify.tools" + elif base_url.netloc.startswith("www"): + # strip "www." + netloc = base_url.netloc[4:] + + logo_url = parse.ParseResult( + scheme=base_url.scheme, + netloc="static-logos." + netloc, + path=logo_file, + params=base_url.params, + query=base_url.query, + fragment=base_url.fragment, ) - send_mock = mocker.patch("app.aws_sns_client.send_sms") - notification = create_notification( - template=sample_template, - to_field="+12028675309", - normalised_to="2028675309", - reply_to_text="testing", + return parse.urlunparse(logo_url) + + +def get_html_email_options(service): + if service.email_branding is None: + return { + "govuk_banner": True, + "brand_banner": False, + } + if isinstance(service, SerialisedService): + branding = dao_get_email_branding_by_id(service.email_branding) + else: + branding = service.email_branding + + logo_url = ( + get_logo_url(current_app.config["ADMIN_BASE_URL"], branding.logo) + if branding.logo + else None ) - mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_s3.return_value = "12028675309" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"ignore": "ignore"} - send_to_providers.send_sms_to_provider(notification) - send_mock.assert_called_once_with( - to="12028675309", - content=ANY, - reference=str(notification.id), - sender=notification.reply_to_text, - international=False, - ) - - -def test_send_email_to_provider_should_user_normalised_to( - mocker, client, sample_email_template -): - send_mock = mocker.patch("app.aws_ses_client.send_email", return_value="reference") - notification = create_notification( - template=sample_email_template, - ) - mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") - mock_redis.get.return_value = "test@example.com".encode("utf-8") - - mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") - mock_redis.get.return_value = "jo.smith@example.com".encode("utf-8") - email = "test@example.com".encode("utf-8") - personalisation = {} - - personalisation = json.dumps(personalisation) - personalisation = personalisation.encode("utf-8") - mock_redis.get.side_effect = [email, personalisation] - - send_to_providers.send_email_to_provider(notification) - send_mock.assert_called_once_with( - ANY, - "test@example.com", - ANY, - body=ANY, - html_body=ANY, - reply_to_address=notification.reply_to_text, - ) - - -def test_send_sms_to_provider_should_return_template_if_found_in_redis( - mocker, client, sample_template -): - - mocker.patch("app.delivery.send_to_providers._get_verify_code", return_value=None) - mocker.patch( - "app.delivery.send_to_providers.get_sender_numbers", return_value=["testing"] - ) - from app.schemas import service_schema, template_schema - - service_dict = service_schema.dump(sample_template.service) - template_dict = template_schema.dump(sample_template) - - mocker.patch( - "app.redis_store.get", - side_effect=[ - json.dumps({"data": service_dict}).encode("utf-8"), - json.dumps({"data": template_dict}).encode("utf-8"), - ], - ) - mock_get_template = mocker.patch( - "app.dao.templates_dao.dao_get_template_by_id_and_service_id" - ) - mock_get_service = mocker.patch("app.dao.services_dao.dao_fetch_service_by_id") - - send_mock = mocker.patch("app.aws_sns_client.send_sms") - notification = create_notification( - template=sample_template, - to_field="+447700900855", - normalised_to="447700900855", - reply_to_text="testing", - ) - - mock_s3 = mocker.patch("app.delivery.send_to_providers.get_phone_number_from_s3") - mock_s3.return_value = "447700900855" - - mock_personalisation = mocker.patch( - "app.delivery.send_to_providers.get_personalisation_from_s3" - ) - mock_personalisation.return_value = {"ignore": "ignore"} - - send_to_providers.send_sms_to_provider(notification) - assert mock_get_template.called is False - assert mock_get_service.called is False - send_mock.assert_called_once_with( - to="447700900855", - content=ANY, - reference=str(notification.id), - sender=notification.reply_to_text, - international=False, - ) - - -def test_send_email_to_provider_should_return_template_if_found_in_redis( - mocker, client, sample_email_template -): - from app.schemas import service_schema, template_schema - - # mock_redis = mocker.patch("app.delivery.send_to_providers.redis_store") - # mock_redis.get.return_value = "jo.smith@example.com".encode("utf-8") - email = "test@example.com".encode("utf-8") - personalisation = { - "name": "Jo", - } - - personalisation = json.dumps(personalisation) - personalisation = personalisation.encode("utf-8") - # mock_redis.get.side_effect = [email, personalisation] - - service_dict = service_schema.dump(sample_email_template.service) - template_dict = template_schema.dump(sample_email_template) - - mocker.patch( - "app.redis_store.get", - side_effect=[ - email, - personalisation, - json.dumps({"data": service_dict}).encode("utf-8"), - json.dumps({"data": template_dict}).encode("utf-8"), - ], - ) - mock_get_template = mocker.patch( - "app.dao.templates_dao.dao_get_template_by_id_and_service_id" - ) - mock_get_service = mocker.patch("app.dao.services_dao.dao_fetch_service_by_id") - send_mock = mocker.patch("app.aws_ses_client.send_email", return_value="reference") - notification = create_notification( - template=sample_email_template, - ) - - send_to_providers.send_email_to_provider(notification) - assert mock_get_template.called is False - assert mock_get_service.called is False - send_mock.assert_called_once_with( - ANY, - "test@example.com", - ANY, - body=ANY, - html_body=ANY, - reply_to_address=notification.reply_to_text, - ) - - -def test_get_html_email_options_return_email_branding_from_serialised_service( - sample_service, -): - branding = create_email_branding() - sample_service.email_branding = branding - service = SerialisedService.from_id(sample_service.id) - email_options = get_html_email_options(service) - assert email_options is not None - assert email_options == { + return { "govuk_banner": branding.brand_type == BrandType.BOTH, "brand_banner": branding.brand_type == BrandType.ORG_BANNER, "brand_colour": branding.colour, - "brand_logo": get_logo_url(current_app.config["ADMIN_BASE_URL"], branding.logo), + "brand_logo": logo_url, "brand_text": branding.text, "brand_name": branding.name, } -def test_get_html_email_options_add_email_branding_from_service(sample_service): - branding = create_email_branding() - sample_service.email_branding = branding - email_options = get_html_email_options(sample_service) - assert email_options is not None - assert email_options == { - "govuk_banner": branding.brand_type == BrandType.BOTH, - "brand_banner": branding.brand_type == BrandType.ORG_BANNER, - "brand_colour": branding.colour, - "brand_logo": get_logo_url(current_app.config["ADMIN_BASE_URL"], branding.logo), - "brand_text": branding.text, - "brand_name": branding.name, - } +def technical_failure(notification): + notification.status = NotificationStatus.TECHNICAL_FAILURE + dao_update_notification(notification) + raise NotificationTechnicalFailureException( + f"Send {notification.notification_type} for notification id {notification.id} " + f"to provider is not allowed: service {notification.service_id} is inactive" + ) From 05bac16376a34c523b4c627551de705f711ed276 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Tue, 1 Oct 2024 16:08:48 -0400 Subject: [PATCH 47/65] One more attempt at fixing the Login.gov registration URL This changeset attempts another fix by referencing the environment variable in a different fashion, according to the GitHub docs: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/accessing-contextual-information-about-workflow-runs#example-usage-of-the-env-context Signed-off-by: Carlo Costino --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 0dfbbc603..001e1a627 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,7 +70,7 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var LOGIN_DOT_GOV_REGISTRATION_URL=$LOGIN_DOT_GOV_REGISTRATION_URL --strategy rolling" + cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var var-name=${{ env.LOGIN_DOT_GOV_REGISTRATION_URL }} --strategy rolling" - name: Check for changes to templates.json id: changed-templates From ab7e57597a27ac82da8396d0841d930cc8fe6baa Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 1 Oct 2024 13:21:05 -0700 Subject: [PATCH 48/65] don't run coverage on tests --- .github/workflows/checks.yml | 4 ++-- Makefile | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 57863effb..c0374a79f 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -54,7 +54,7 @@ jobs: - name: Check for dead code run: make dead-code - name: Run tests with coverage - run: poetry run coverage run --omit=*/migrations/* -m pytest --maxfail=10 + run: poetry run coverage run --omit=*/migrations/*,*/tests/* -m pytest --maxfail=10 env: SQLALCHEMY_DATABASE_TEST_URI: postgresql://user:password@localhost:5432/test_notification_api NOTIFY_E2E_TEST_EMAIL: ${{ secrets.NOTIFY_E2E_TEST_EMAIL }} @@ -63,7 +63,7 @@ jobs: NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} - name: Check coverage threshold # TODO get this back up to 95 - run: poetry run coverage report --fail-under=95 + run: poetry run coverage report -m --fail-under=95 validate-new-relic-config: runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index a0fd86ae4..7dfa0872d 100644 --- a/Makefile +++ b/Makefile @@ -81,7 +81,7 @@ test: ## Run tests and create coverage report poetry run black . poetry run flake8 . poetry run isort --check-only ./app ./tests - poetry run coverage run --omit=*/migrations/* -m pytest --maxfail=10 + poetry run coverage run --omit=*/migrations/*,*/tests/* -m pytest --maxfail=10 poetry run coverage report -m --fail-under=95 poetry run coverage html -d .coverage_cache From 37e5de331a817f1594e9b2be6245c6f0fdd63314 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 1 Oct 2024 13:31:04 -0700 Subject: [PATCH 49/65] don't run coverage on tests --- .github/workflows/checks.yml | 2 +- Makefile | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index c0374a79f..22c7f9c89 100644 --- a/.github/workflows/checks.yml +++ b/.github/workflows/checks.yml @@ -63,7 +63,7 @@ jobs: NOTIFY_E2E_TEST_PASSWORD: ${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} - name: Check coverage threshold # TODO get this back up to 95 - run: poetry run coverage report -m --fail-under=95 + run: poetry run coverage report -m --fail-under=91 validate-new-relic-config: runs-on: ubuntu-latest diff --git a/Makefile b/Makefile index 7dfa0872d..88cf6f814 100644 --- a/Makefile +++ b/Makefile @@ -83,7 +83,8 @@ test: ## Run tests and create coverage report poetry run isort --check-only ./app ./tests poetry run coverage run --omit=*/migrations/*,*/tests/* -m pytest --maxfail=10 - poetry run coverage report -m --fail-under=95 + ## TODO set this back to 95 asap + poetry run coverage report -m --fail-under=91 poetry run coverage html -d .coverage_cache .PHONY: py-lock From b70d47fa38eb2f33255374eb7baf42924db75bb8 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Tue, 1 Oct 2024 17:09:10 -0400 Subject: [PATCH 50/65] Attempt quoting the Login.gov URL This changeset tries to wrap the Login.gov registration URL with quotes to get it to be properly handled in the shell environment. Signed-off-by: Carlo Costino --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 001e1a627..80b9c5656 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,7 +70,7 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var var-name=${{ env.LOGIN_DOT_GOV_REGISTRATION_URL }} --strategy rolling" + cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var LOGIN_DOT_GOV_REGISTRATION_URL=\"${{ env.LOGIN_DOT_GOV_REGISTRATION_URL }}\" --strategy rolling" - name: Check for changes to templates.json id: changed-templates From f8410a27fdd05072acabb98067b002aeefa82c4d Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Tue, 1 Oct 2024 17:29:05 -0400 Subject: [PATCH 51/65] Attempt quoting the Login.gov URL with var-name This changeset tries to wrap the Login.gov registration URL with quotes to get it to be properly handled in the shell environment using the same pattern as the secret variables. Signed-off-by: Carlo Costino --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 80b9c5656..dcb40de0d 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,7 +70,7 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var LOGIN_DOT_GOV_REGISTRATION_URL=\"${{ env.LOGIN_DOT_GOV_REGISTRATION_URL }}\" --strategy rolling" + cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var var-name=\"${{ env.LOGIN_DOT_GOV_REGISTRATION_URL }}\" --strategy rolling" - name: Check for changes to templates.json id: changed-templates From 05838f964f11b334c57e8760388db173260f6c1f Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Tue, 1 Oct 2024 17:37:03 -0400 Subject: [PATCH 52/65] Revert back to our original formatting, but include the --strategy rolling Signed-off-by: Carlo Costino --- .github/workflows/deploy.yml | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index dcb40de0d..d719d1205 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -70,7 +70,16 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging - cf_command: "push -f manifest.yml --vars-file deploy-config/staging.yml --var var-name=${{ secrets.DANGEROUS_SALT }} --var var-name=${{ secrets.SECRET_KEY }} --var var-name=${{ secrets.ADMIN_CLIENT_SECRET }} --var var-name=${{ secrets.NEW_RELIC_LICENSE_KEY }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_EMAIL }} --var var-name=${{ secrets.NOTIFY_E2E_TEST_PASSWORD }} --var var-name=\"${{ env.LOGIN_DOT_GOV_REGISTRATION_URL }}\" --strategy rolling" + cf_command: >- + --vars-file deploy-config/staging.yml + --var DANGEROUS_SALT="$DANGEROUS_SALT" + --var SECRET_KEY="$SECRET_KEY" + --var ADMIN_CLIENT_SECRET="$ADMIN_CLIENT_SECRET" + --var NEW_RELIC_LICENSE_KEY="$NEW_RELIC_LICENSE_KEY" + --var NOTIFY_E2E_TEST_EMAIL="$NOTIFY_E2E_TEST_EMAIL" + --var NOTIFY_E2E_TEST_PASSWORD="$NOTIFY_E2E_TEST_PASSWORD" + --var LOGIN_DOT_GOV_REGISTRATION_URL="$LOGIN_DOT_GOV_REGISTRATION_URL" + --strategy rolling - name: Check for changes to templates.json id: changed-templates From 182572a6b7f3960496171de4422cbce8141542eb Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Tue, 1 Oct 2024 17:39:20 -0400 Subject: [PATCH 53/65] Forgot to add the push command Signed-off-by: Carlo Costino --- .github/workflows/deploy.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index d719d1205..6145bf296 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -71,6 +71,7 @@ jobs: cf_org: gsa-tts-benefits-studio cf_space: notify-staging cf_command: >- + push -f manifest.yml --vars-file deploy-config/staging.yml --var DANGEROUS_SALT="$DANGEROUS_SALT" --var SECRET_KEY="$SECRET_KEY" From 97ee4fe03200fe07532f08dcb361e16bc28ee331 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 07:31:13 -0700 Subject: [PATCH 54/65] cleanup --- app/aws/s3.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index c652e70de..bd0301d78 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -318,11 +318,17 @@ def get_job_from_s3(service_id, job_id): while retries < max_retries: try: - obj = get_s3_object(*get_job_location(service_id, job_id)) - # TODO remove this when we have fully transitioned to the NEW_FILE_LOCATION_STRUCTURE - if obj is None: + # TODO + # for transition on optimizing the s3 partition, we have + # to check for the file location using the new way and the + # old way. After this has been on production for a few weeks + # we should remove the check for the old way. + try: + obj = get_s3_object(*get_job_location(service_id, job_id)) + return obj.get()["Body"].read().decode("utf-8") + except botocore.exceptions.ClientError: obj = get_s3_object(*get_old_job_location(service_id, job_id)) - return obj.get()["Body"].read().decode("utf-8") + return obj.get()["Body"].read().decode("utf-8") except botocore.exceptions.ClientError as e: if e.response["Error"]["Code"] in [ "Throttling", From c792a2492d41cd543548f997b9540e04ac887d89 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 07:58:24 -0700 Subject: [PATCH 55/65] cleanup --- tests/app/aws/test_s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index 8e3863d5c..e468c4426 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -151,7 +151,7 @@ def test_get_job_from_s3_exponential_backoff_on_throttling(mocker): mocker.patch("app.aws.s3.file_exists", return_value=True) job = get_job_from_s3("service_id", "job_id") assert job is None - assert mock_get_object.call_count == 4 + assert mock_get_object.call_count == 8 def test_get_job_from_s3_exponential_backoff_file_not_found(mocker): From f720b48d0d27b10af299e407b9d8f65d1169d8ec Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 2 Oct 2024 09:54:18 -0700 Subject: [PATCH 56/65] Bug: Invites expiring immediately --- app/service_invite/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/service_invite/rest.py b/app/service_invite/rest.py index dd76ad2bd..f6d9627da 100644 --- a/app/service_invite/rest.py +++ b/app/service_invite/rest.py @@ -86,7 +86,7 @@ def _create_service_invite(invited_user, invite_link_host): redis_store.set( f"email-personalisation-{saved_notification.id}", json.dumps(personalisation), - ex=1800, + ex=2*24*60*60, ) send_notification_to_queue(saved_notification, queue=QueueNames.NOTIFY) From 4abd54094dd7f6d035264cbbb22b80832595fc1b Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Oct 2024 07:00:07 -0700 Subject: [PATCH 57/65] debug s3 partitioning --- app/aws/s3.py | 9 +++++++++ app/job/rest.py | 1 + app/user/rest.py | 1 - 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index bd0301d78..a3cd35811 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -264,6 +264,9 @@ def file_exists(file_location): def get_job_location(service_id, job_id): + current_app.logger.info( + f"#s3-partitioning NEW JOB_LOCATION: {NEW_FILE_LOCATION_STRUCTURE.format(service_id, job_id)}" + ) return ( current_app.config["CSV_UPLOAD_BUCKET"]["bucket"], NEW_FILE_LOCATION_STRUCTURE.format(service_id, job_id), @@ -279,6 +282,9 @@ def get_old_job_location(service_id, job_id): but it will take a few days where we have to support both formats. Remove this when everything works with the NEW_FILE_LOCATION_STRUCTURE. """ + current_app.logger.info( + f"#s3-partitioning OLD JOB LOCATION: {FILE_LOCATION_STRUCTURE.format(service_id, job_id)}" + ) return ( current_app.config["CSV_UPLOAD_BUCKET"]["bucket"], FILE_LOCATION_STRUCTURE.format(service_id, job_id), @@ -486,6 +492,9 @@ def get_personalisation_from_s3(service_id, job_id, job_row_number): def get_job_metadata_from_s3(service_id, job_id): + current_app.logger.info( + f"#s3-partitioning CALLING GET_JOB_METADATA with {service_id}, {job_id}" + ) obj = get_s3_object(*get_job_location(service_id, job_id)) return obj.get()["Metadata"] diff --git a/app/job/rest.py b/app/job/rest.py index 85414a29c..8b3965061 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -175,6 +175,7 @@ def create_job(service_id): original_file_name = data.get("original_file_name") data.update({"service": service_id}) try: + current_app.logger.info(f"#s3-partitioning DATA IN CREATE_JOB: {data}") data.update(**get_job_metadata_from_s3(service_id, data["id"])) except KeyError: raise InvalidRequest( diff --git a/app/user/rest.py b/app/user/rest.py index 847c4ca07..f4f4db947 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -599,7 +599,6 @@ def fetch_user_by_email(): fetched_user = get_user_by_email(email["email"]) debug_not_production(hilite(f"fetched user is {fetched_user}")) result = fetched_user.serialize() - debug_not_production(hilite(f"result is serialized to {result}")) return jsonify(data=result) except Exception as e: debug_not_production(hilite(f"Failed with {e}!!")) From 0c100dd7cc27699c250283cf2c5d9308e36a29df Mon Sep 17 00:00:00 2001 From: Andrew Shumway Date: Fri, 4 Oct 2024 10:26:23 -0600 Subject: [PATCH 58/65] Change references name in deploy tool --- .github/workflows/deploy.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 6145bf296..b2ca9c975 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -55,7 +55,7 @@ jobs: run: poetry export --without-hashes --format=requirements.txt > requirements.txt - name: Deploy to cloud.gov - uses: cloud-gov/cg-cli-tools@main + uses: cloud-gov/cli-tools@main env: DANGEROUS_SALT: ${{ secrets.DANGEROUS_SALT }} SECRET_KEY: ${{ secrets.SECRET_KEY }} From 584f24390ceaafa8cb5714cbc9027dfc96e1832b Mon Sep 17 00:00:00 2001 From: Andrew Shumway Date: Fri, 4 Oct 2024 11:17:34 -0600 Subject: [PATCH 59/65] Switch back repo reference and update deploy tool for demo/prod scripts --- .github/workflows/deploy-demo.yml | 8 +++++--- .github/workflows/deploy-prod.yml | 8 +++++--- .github/workflows/deploy.yml | 2 +- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/workflows/deploy-demo.yml b/.github/workflows/deploy-demo.yml index 9886eff6c..2895a3d6c 100644 --- a/.github/workflows/deploy-demo.yml +++ b/.github/workflows/deploy-demo.yml @@ -49,7 +49,7 @@ jobs: run: poetry export --without-hashes --format=requirements.txt > requirements.txt - name: Deploy to cloud.gov - uses: 18f/cg-deploy-action@main + uses: cloud-gov/cg-cli-tools@main env: DANGEROUS_SALT: ${{ secrets.DANGEROUS_SALT }} SECRET_KEY: ${{ secrets.SECRET_KEY }} @@ -64,8 +64,9 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-demo - push_arguments: >- - --vars-file deploy-config/demo.yml + cf_command: >- + push -f manifest.yml + --vars-file deploy-config/staging.yml --var DANGEROUS_SALT="$DANGEROUS_SALT" --var SECRET_KEY="$SECRET_KEY" --var ADMIN_CLIENT_SECRET="$ADMIN_CLIENT_SECRET" @@ -73,6 +74,7 @@ jobs: --var NOTIFY_E2E_TEST_EMAIL="$NOTIFY_E2E_TEST_EMAIL" --var NOTIFY_E2E_TEST_PASSWORD="$NOTIFY_E2E_TEST_PASSWORD" --var LOGIN_DOT_GOV_REGISTRATION_URL="$LOGIN_DOT_GOV_REGISTRATION_URL" + --strategy rolling - name: Check for changes to templates.json id: changed-templates diff --git a/.github/workflows/deploy-prod.yml b/.github/workflows/deploy-prod.yml index 0778f3c78..ed544ccb0 100644 --- a/.github/workflows/deploy-prod.yml +++ b/.github/workflows/deploy-prod.yml @@ -53,7 +53,7 @@ jobs: run: poetry export --without-hashes --format=requirements.txt > requirements.txt - name: Deploy to cloud.gov - uses: 18f/cg-deploy-action@main + uses: cloud-gov/cg-cli-tools@main env: DANGEROUS_SALT: ${{ secrets.DANGEROUS_SALT }} SECRET_KEY: ${{ secrets.SECRET_KEY }} @@ -68,8 +68,9 @@ jobs: cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-production - push_arguments: >- - --vars-file deploy-config/production.yml + cf_command: >- + push -f manifest.yml + --vars-file deploy-config/staging.yml --var DANGEROUS_SALT="$DANGEROUS_SALT" --var SECRET_KEY="$SECRET_KEY" --var ADMIN_CLIENT_SECRET="$ADMIN_CLIENT_SECRET" @@ -77,6 +78,7 @@ jobs: --var NOTIFY_E2E_TEST_EMAIL="$NOTIFY_E2E_TEST_EMAIL" --var NOTIFY_E2E_TEST_PASSWORD="$NOTIFY_E2E_TEST_PASSWORD" --var LOGIN_DOT_GOV_REGISTRATION_URL="$LOGIN_DOT_GOV_REGISTRATION_URL" + --strategy rolling - name: Check for changes to templates.json id: changed-templates diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index b2ca9c975..6145bf296 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -55,7 +55,7 @@ jobs: run: poetry export --without-hashes --format=requirements.txt > requirements.txt - name: Deploy to cloud.gov - uses: cloud-gov/cli-tools@main + uses: cloud-gov/cg-cli-tools@main env: DANGEROUS_SALT: ${{ secrets.DANGEROUS_SALT }} SECRET_KEY: ${{ secrets.SECRET_KEY }} From 497f91af4dc8baa75847e68ce54edf94cea59da9 Mon Sep 17 00:00:00 2001 From: Andrew Shumway Date: Fri, 4 Oct 2024 11:20:57 -0600 Subject: [PATCH 60/65] Fix deploy config references --- .github/workflows/deploy-demo.yml | 2 +- .github/workflows/deploy-prod.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/deploy-demo.yml b/.github/workflows/deploy-demo.yml index 2895a3d6c..0bb7e1ec8 100644 --- a/.github/workflows/deploy-demo.yml +++ b/.github/workflows/deploy-demo.yml @@ -66,7 +66,7 @@ jobs: cf_space: notify-demo cf_command: >- push -f manifest.yml - --vars-file deploy-config/staging.yml + --vars-file deploy-config/demo.yml --var DANGEROUS_SALT="$DANGEROUS_SALT" --var SECRET_KEY="$SECRET_KEY" --var ADMIN_CLIENT_SECRET="$ADMIN_CLIENT_SECRET" diff --git a/.github/workflows/deploy-prod.yml b/.github/workflows/deploy-prod.yml index ed544ccb0..c84cf7324 100644 --- a/.github/workflows/deploy-prod.yml +++ b/.github/workflows/deploy-prod.yml @@ -70,7 +70,7 @@ jobs: cf_space: notify-production cf_command: >- push -f manifest.yml - --vars-file deploy-config/staging.yml + --vars-file deploy-config/production.yml --var DANGEROUS_SALT="$DANGEROUS_SALT" --var SECRET_KEY="$SECRET_KEY" --var ADMIN_CLIENT_SECRET="$ADMIN_CLIENT_SECRET" From f81785c9b46112788c596ed48d0b9a597b4350da Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Fri, 4 Oct 2024 22:53:23 -0400 Subject: [PATCH 61/65] Update egress proxy deployment steps This changeset updates the egress proxy deployment steps to match the admin repo, based on lessons learned there. Signed-off-by: Carlo Costino --- .github/actions/deploy-proxy/action.yml | 13 +++++++++++++ .github/workflows/deploy.yml | 5 ++++- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/.github/actions/deploy-proxy/action.yml b/.github/actions/deploy-proxy/action.yml index 13bdc494f..339d1fc78 100644 --- a/.github/actions/deploy-proxy/action.yml +++ b/.github/actions/deploy-proxy/action.yml @@ -16,6 +16,19 @@ inputs: runs: using: composite steps: + - name: Install cf-cli + shell: bash + run: | + curl -A "cg-deploy-action" -v -L -o cf-cli_amd64.deb 'https://packages.cloudfoundry.org/stable?release=debian64&version=v8&source=github' + sudo dpkg -i cf-cli_amd64.deb + - name: Login to cf-cli + shell: bash + run: | + cf api api.fr.cloud.gov + cf auth + - name: Target org and space + shell: bash + run: cf target -o ${{ inputs.cf_org }} -s ${{ inputs.cf_space }} - name: Set restricted space egress shell: bash run: ./terraform/set_space_egress.sh -t -s ${{ inputs.cf_space }} diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 6145bf296..f1fdf9df6 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -101,9 +101,12 @@ jobs: .github/actions/deploy-proxy/action.yml .github/workflows/deploy.yml - name: Deploy egress proxy - if: steps.changed-egress-config.outputs.any_changed == 'true' + #if: steps.changed-egress-config.outputs.any_changed == 'true' uses: ./.github/actions/deploy-proxy with: + cf_username: ${{ secrets.CLOUDGOV_USERNAME }} + cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} + cf_org: gsa-tts-benefits-studio cf_space: notify-staging app: notify-api-staging From c2f2e36262810c330e663a0fa9bbd19fd62ca52b Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Wed, 9 Oct 2024 13:52:01 -0600 Subject: [PATCH 62/65] Added missing egress proxy deploy action inputs Signed-off-by: Carlo Costino --- .github/actions/deploy-proxy/action.yml | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.github/actions/deploy-proxy/action.yml b/.github/actions/deploy-proxy/action.yml index 339d1fc78..393dd4b45 100644 --- a/.github/actions/deploy-proxy/action.yml +++ b/.github/actions/deploy-proxy/action.yml @@ -1,6 +1,15 @@ name: Deploy egress proxy description: Set egress space security groups and deploy proxy inputs: + cf_username: + description: The username to authenticate with. + required: true + cf_password: + description: The password to authenticate with. + required: true + cf_org: + description: The org the target app exists in. + required: true cf_space: description: The space the target app exists in. required: true From 22bb1d0b8c4fcc53b4f3ec43c28bbde248d3447e Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Wed, 9 Oct 2024 14:23:25 -0600 Subject: [PATCH 63/65] Swap config vars for env vars Signed-off-by: Carlo Costino --- .github/actions/deploy-proxy/action.yml | 6 ------ .github/workflows/deploy.yml | 5 +++-- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/.github/actions/deploy-proxy/action.yml b/.github/actions/deploy-proxy/action.yml index 393dd4b45..0ffc05066 100644 --- a/.github/actions/deploy-proxy/action.yml +++ b/.github/actions/deploy-proxy/action.yml @@ -1,12 +1,6 @@ name: Deploy egress proxy description: Set egress space security groups and deploy proxy inputs: - cf_username: - description: The username to authenticate with. - required: true - cf_password: - description: The password to authenticate with. - required: true cf_org: description: The org the target app exists in. required: true diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index f1fdf9df6..cdcae16d4 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -103,9 +103,10 @@ jobs: - name: Deploy egress proxy #if: steps.changed-egress-config.outputs.any_changed == 'true' uses: ./.github/actions/deploy-proxy + env: + CF_USERNAME: ${{ secrets.CF_USERNAME }} + CF_PASSWORD: ${{ secrets.CF_PASSWORD }} with: - cf_username: ${{ secrets.CLOUDGOV_USERNAME }} - cf_password: ${{ secrets.CLOUDGOV_PASSWORD }} cf_org: gsa-tts-benefits-studio cf_space: notify-staging app: notify-api-staging From f644f5250ca72832f036d1c640e97687daff5178 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Thu, 10 Oct 2024 13:10:01 -0400 Subject: [PATCH 64/65] Fix environment name references for CF Signed-off-by: Carlo Costino --- .github/workflows/deploy.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index cdcae16d4..43296f9c7 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -104,8 +104,8 @@ jobs: #if: steps.changed-egress-config.outputs.any_changed == 'true' uses: ./.github/actions/deploy-proxy env: - CF_USERNAME: ${{ secrets.CF_USERNAME }} - CF_PASSWORD: ${{ secrets.CF_PASSWORD }} + CF_USERNAME: ${{ secrets.CLOUDGOV_USERNAME }} + CF_PASSWORD: ${{ secrets.CLOUDGOV_PASSWORD }} with: cf_org: gsa-tts-benefits-studio cf_space: notify-staging From 3f6c362f15e2d6276b82eb215ed74a4742e4b9a1 Mon Sep 17 00:00:00 2001 From: Carlo Costino Date: Thu, 10 Oct 2024 18:34:52 -0400 Subject: [PATCH 65/65] Finalize updates for cg-cli-tools This changeset finalizes our updates for the cg-cli-tools across all environments and restores the check for updates to the egress proxy before deploying. Signed-off-by: Carlo Costino --- .github/workflows/deploy-demo.yml | 8 ++++++-- .github/workflows/deploy-prod.yml | 8 ++++++-- .github/workflows/deploy.yml | 2 +- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/.github/workflows/deploy-demo.yml b/.github/workflows/deploy-demo.yml index 0bb7e1ec8..a43b661f3 100644 --- a/.github/workflows/deploy-demo.yml +++ b/.github/workflows/deploy-demo.yml @@ -97,6 +97,10 @@ jobs: - name: Deploy egress proxy if: steps.changed-egress-config.outputs.any_changed == 'true' uses: ./.github/actions/deploy-proxy + env: + CF_USERNAME: ${{ secrets.CLOUDGOV_USERNAME }} + CF_PASSWORD: ${{ secrets.CLOUDGOV_PASSWORD }} with: - cf_space: notify-demo - app: notify-api-demo + cf_org: gsa-tts-benefits-studio + cf_space: notify-staging + app: notify-api-staging diff --git a/.github/workflows/deploy-prod.yml b/.github/workflows/deploy-prod.yml index c84cf7324..23ef3dc6c 100644 --- a/.github/workflows/deploy-prod.yml +++ b/.github/workflows/deploy-prod.yml @@ -101,6 +101,10 @@ jobs: - name: Deploy egress proxy if: steps.changed-egress-config.outputs.any_changed == 'true' uses: ./.github/actions/deploy-proxy + env: + CF_USERNAME: ${{ secrets.CLOUDGOV_USERNAME }} + CF_PASSWORD: ${{ secrets.CLOUDGOV_PASSWORD }} with: - cf_space: notify-production - app: notify-api-production + cf_org: gsa-tts-benefits-studio + cf_space: notify-staging + app: notify-api-staging diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 43296f9c7..d8d0da952 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -101,7 +101,7 @@ jobs: .github/actions/deploy-proxy/action.yml .github/workflows/deploy.yml - name: Deploy egress proxy - #if: steps.changed-egress-config.outputs.any_changed == 'true' + if: steps.changed-egress-config.outputs.any_changed == 'true' uses: ./.github/actions/deploy-proxy env: CF_USERNAME: ${{ secrets.CLOUDGOV_USERNAME }}