From 9db6fc83f948eddc4775fdbf06b4b7737f62872e Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 6 Apr 2020 17:57:16 +0100 Subject: [PATCH 01/15] Split letters into zip files based on postage class We will split them into three categories: - first class - second class - international - this zip file will have letters for both europe and rest-of-world postage classes --- app/celery/letters_pdf_tasks.py | 63 ++++++----- tests/app/celery/test_letters_pdf_tasks.py | 126 ++++++++++++++++++--- 2 files changed, 144 insertions(+), 45 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 8c1ce0e3c..9a34ba729 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -137,39 +137,44 @@ def collate_letter_pdfs_to_be_sent(): letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline) - for i, letters in enumerate(group_letters(letters_to_print)): - filenames = [letter['Key'] for letter in letters] + i = 0 + for zip_folder, letters_list in letters_to_print.items(): + for letters in group_letters(letters_list): + i += 1 + filenames = [letter['Key'] for letter in letters] - hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode() - # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP - dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format( - date=print_run_deadline.strftime("%Y-%m-%d"), - num=i + 1, - hash=hash - ) - - current_app.logger.info( - 'Calling task zip-and-send-letter-pdfs for {} pdfs to upload {} with total size {:,} bytes'.format( - len(filenames), - dvla_filename, - sum(letter['Size'] for letter in letters) + hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode() + # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP + dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format( + date=print_run_deadline.strftime("%Y-%m-%d"), + num=i, + hash=hash + ) + + current_app.logger.info( + 'Calling task zip-and-send-letter-pdfs for {} pdfs to upload {} with total size {:,} bytes'.format( + len(filenames), + dvla_filename, + sum(letter['Size'] for letter in letters) + ) + ) + notify_celery.send_task( + name=TaskNames.ZIP_AND_SEND_LETTER_PDFS, + kwargs={ + 'filenames_to_zip': filenames, + 'upload_filename': dvla_filename + }, + queue=QueueNames.PROCESS_FTP, + compression='zlib' ) - ) - notify_celery.send_task( - name=TaskNames.ZIP_AND_SEND_LETTER_PDFS, - kwargs={ - 'filenames_to_zip': filenames, - 'upload_filename': dvla_filename - }, - queue=QueueNames.PROCESS_FTP, - compression='zlib' - ) def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline): letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline) - - letter_pdfs = [] + zip_folders_by_postage = { + "first": "first", "second": "second", "europe": "international", "rest-of-world": "international" + } + letter_pdfs = {"first": [], "second": [], "international": []} for letter in letters_awaiting_sending: try: letter_file_name = get_letter_pdf_filename( @@ -179,7 +184,9 @@ def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline): postage=letter.postage ) letter_head = s3.head_s3_object(current_app.config['LETTERS_PDF_BUCKET_NAME'], letter_file_name) - letter_pdfs.append({"Key": letter_file_name, "Size": letter_head['ContentLength']}) + letter_pdfs[ + zip_folders_by_postage[letter.postage] + ].append({"Key": letter_file_name, "Size": letter_head['ContentLength']}) except BotoClientError as e: current_app.logger.exception( f"Error getting letter from bucket for notification: {letter.id} with reference: {letter.reference}", e) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index c587b646e..31cde9044 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -155,20 +155,19 @@ def test_update_billable_units_for_letter_doesnt_update_if_sent_with_test_key(mo @freeze_time('2020-02-17 18:00:00') def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sample_letter_template): + # second class create_notification( template=sample_letter_template, status='created', reference='ref0', created_at=(datetime.now() - timedelta(hours=2)) ) - create_notification( template=sample_letter_template, status='created', reference='ref1', created_at=(datetime.now() - timedelta(hours=3)) ) - create_notification( template=sample_letter_template, status='created', @@ -176,6 +175,31 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam created_at=(datetime.now() - timedelta(days=2)) ) + # first class + create_notification( + template=sample_letter_template, + status='created', + reference='first_class', + created_at=(datetime.now() - timedelta(hours=4)), + postage="first" + ) + + # international + create_notification( + template=sample_letter_template, + status='created', + reference='international', + created_at=(datetime.now() - timedelta(days=3)), + postage="europe" + ) + create_notification( + template=sample_letter_template, + status='created', + reference='international', + created_at=(datetime.now() - timedelta(days=4)), + postage="rest-of-world" + ) + # notifications we don't expect to get sent to print as they are in the wrong status for status in ['delivered', 'validation-failed', 'cancelled', 'sending']: create_notification( @@ -203,28 +227,46 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam ) mock_s3 = mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ + {'ContentLength': 1}, + {'ContentLength': 1}, {'ContentLength': 2}, {'ContentLength': 1}, {'ContentLength': 3}, + {'ContentLength': 1}, ]) results = get_key_and_size_of_letters_to_be_sent_to_print(datetime.now() - timedelta(minutes=30)) - assert mock_s3.call_count == 3 + assert mock_s3.call_count == 6 mock_s3.assert_has_calls( [ + call(current_app.config[ + 'LETTERS_PDF_BUCKET_NAME' + ], '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF'), + call(current_app.config[ + 'LETTERS_PDF_BUCKET_NAME' + ], '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.C.20200214180000.PDF'), call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF'), + call(current_app.config[ + 'LETTERS_PDF_BUCKET_NAME' + ], '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.C.20200217140000.PDF'), call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF'), call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF'), ] ) - assert len(results) == 3 - assert results == [ - {'Key': '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', 'Size': 2}, - {'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 1}, - {'Key': '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', 'Size': 3}, - ] + assert results == { + "first": [{'Key': '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.C.20200217140000.PDF', 'Size': 1}], + "second": [ + {'Key': '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', 'Size': 2}, + {'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 3}, + {'Key': '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', 'Size': 1}, + ], + "international": [ + {'Key': '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF', 'Size': 1}, + {'Key': '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.C.20200214180000.PDF', 'Size': 1}, + ] + } @freeze_time('2020-02-17 18:00:00') @@ -266,7 +308,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print_catches_exception( ] ) - assert results == [{'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 2}] + assert results["second"] == [{'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 2}] @pytest.mark.parametrize('time_to_run_task', [ @@ -275,20 +317,19 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print_catches_exception( ]) def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mocker, time_to_run_task): with freeze_time("2020-02-17 18:00:00"): + # second class create_notification( template=sample_letter_template, status='created', reference='ref0', created_at=(datetime.now() - timedelta(hours=2)) ) - create_notification( template=sample_letter_template, status='created', reference='ref1', created_at=(datetime.now() - timedelta(hours=3)) ) - create_notification( template=sample_letter_template, status='created', @@ -296,10 +337,38 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock created_at=(datetime.now() - timedelta(days=2)) ) + # first class + create_notification( + template=sample_letter_template, + status='created', + reference='first_class', + created_at=(datetime.now() - timedelta(hours=4)), + postage="first" + ) + + # international + create_notification( + template=sample_letter_template, + status='created', + reference='international', + created_at=(datetime.now() - timedelta(days=3)), + postage="europe" + ) + create_notification( + template=sample_letter_template, + status='created', + reference='international', + created_at=(datetime.now() - timedelta(days=4)), + postage="rest-of-world" + ) + mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ + {'ContentLength': 1}, + {'ContentLength': 1}, {'ContentLength': 2}, {'ContentLength': 1}, {'ContentLength': 3}, + {'ContentLength': 1}, ]) mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') @@ -308,26 +377,49 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock with freeze_time(time_to_run_task): collate_letter_pdfs_to_be_sent() - assert len(mock_celery.call_args_list) == 2 + assert len(mock_celery.call_args_list) == 4 assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', kwargs={ 'filenames_to_zip': [ - '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', - '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF' + '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.C.20200217140000.PDF' ], - 'upload_filename': 'NOTIFY.2020-02-17.001.k3x_WqC5KhB6e2DWv9Ma.ZIP' + 'upload_filename': 'NOTIFY.2020-02-17.001.kHh01fdUxT9iEIYUt5Wx.ZIP' }, queue='process-ftp-tasks', compression='zlib' ) assert mock_celery.call_args_list[1] == call( + name='zip-and-send-letter-pdfs', + kwargs={ + 'filenames_to_zip': [ + '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', + '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF' + ], + 'upload_filename': 'NOTIFY.2020-02-17.002.k3x_WqC5KhB6e2DWv9Ma.ZIP' + }, + queue='process-ftp-tasks', + compression='zlib' + ) + assert mock_celery.call_args_list[2] == call( name='zip-and-send-letter-pdfs', kwargs={ 'filenames_to_zip': [ '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF' ], - 'upload_filename': 'NOTIFY.2020-02-17.002.J85cUw-FWlKuAIOcwdLS.ZIP' + 'upload_filename': 'NOTIFY.2020-02-17.003.J85cUw-FWlKuAIOcwdLS.ZIP' + }, + queue='process-ftp-tasks', + compression='zlib' + ) + assert mock_celery.call_args_list[3] == call( + name='zip-and-send-letter-pdfs', + kwargs={ + 'filenames_to_zip': [ + '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF', + '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.C.20200214180000.PDF' + ], + 'upload_filename': 'NOTIFY.2020-02-17.004.ArkHQVgyuvwCZA-dVExE.ZIP' }, queue='process-ftp-tasks', compression='zlib' From f3c7c098d65908fd4aa54c7807fc3fac0099325d Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 24 Jun 2020 14:59:10 +0100 Subject: [PATCH 02/15] Include postage in zip folder filenames when sending letters Also split letters into zips in 4 categories based on their postage. Also have a separate count for zipfiles in each postage category. --- app/celery/letters_pdf_tasks.py | 15 +++++------- tests/app/celery/test_letters_pdf_tasks.py | 28 +++++++++++++++------- 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 9a34ba729..bb2f82537 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -42,6 +42,7 @@ from app.models import ( NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_VALIDATION_FAILED, NOTIFICATION_VIRUS_SCAN_FAILED, + RESOLVE_POSTAGE_FOR_FILE_NAME ) from app.cronitor import cronitor @@ -137,16 +138,17 @@ def collate_letter_pdfs_to_be_sent(): letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline) - i = 0 for zip_folder, letters_list in letters_to_print.items(): + i = 0 for letters in group_letters(letters_list): i += 1 filenames = [letter['Key'] for letter in letters] hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode() # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP - dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format( + dvla_filename = 'NOTIFY.{date}.{postage}.{num:03}.{hash}.ZIP'.format( date=print_run_deadline.strftime("%Y-%m-%d"), + postage=RESOLVE_POSTAGE_FOR_FILE_NAME[zip_folder], num=i, hash=hash ) @@ -171,10 +173,7 @@ def collate_letter_pdfs_to_be_sent(): def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline): letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline) - zip_folders_by_postage = { - "first": "first", "second": "second", "europe": "international", "rest-of-world": "international" - } - letter_pdfs = {"first": [], "second": [], "international": []} + letter_pdfs = {"first": [], "second": [], "europe": [], "rest-of-world": []} for letter in letters_awaiting_sending: try: letter_file_name = get_letter_pdf_filename( @@ -184,9 +183,7 @@ def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline): postage=letter.postage ) letter_head = s3.head_s3_object(current_app.config['LETTERS_PDF_BUCKET_NAME'], letter_file_name) - letter_pdfs[ - zip_folders_by_postage[letter.postage] - ].append({"Key": letter_file_name, "Size": letter_head['ContentLength']}) + letter_pdfs[letter.postage].append({"Key": letter_file_name, "Size": letter_head['ContentLength']}) except BotoClientError as e: current_app.logger.exception( f"Error getting letter from bucket for notification: {letter.id} with reference: {letter.reference}", e) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 31cde9044..287203af3 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -262,9 +262,11 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam {'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 3}, {'Key': '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', 'Size': 1}, ], - "international": [ - {'Key': '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF', 'Size': 1}, + "europe": [ {'Key': '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.C.20200214180000.PDF', 'Size': 1}, + ], + "rest-of-world": [ + {'Key': '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF', 'Size': 1}, ] } @@ -377,14 +379,14 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock with freeze_time(time_to_run_task): collate_letter_pdfs_to_be_sent() - assert len(mock_celery.call_args_list) == 4 + assert len(mock_celery.call_args_list) == 5 assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', kwargs={ 'filenames_to_zip': [ '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.C.20200217140000.PDF' ], - 'upload_filename': 'NOTIFY.2020-02-17.001.kHh01fdUxT9iEIYUt5Wx.ZIP' + 'upload_filename': 'NOTIFY.2020-02-17.1.001.kHh01fdUxT9iEIYUt5Wx.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -396,7 +398,7 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF' ], - 'upload_filename': 'NOTIFY.2020-02-17.002.k3x_WqC5KhB6e2DWv9Ma.ZIP' + 'upload_filename': 'NOTIFY.2020-02-17.2.001.k3x_WqC5KhB6e2DWv9Ma.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -407,7 +409,7 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock 'filenames_to_zip': [ '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF' ], - 'upload_filename': 'NOTIFY.2020-02-17.003.J85cUw-FWlKuAIOcwdLS.ZIP' + 'upload_filename': 'NOTIFY.2020-02-17.2.002.J85cUw-FWlKuAIOcwdLS.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -416,10 +418,20 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock name='zip-and-send-letter-pdfs', kwargs={ 'filenames_to_zip': [ - '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF', '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.C.20200214180000.PDF' ], - 'upload_filename': 'NOTIFY.2020-02-17.004.ArkHQVgyuvwCZA-dVExE.ZIP' + 'upload_filename': 'NOTIFY.2020-02-17.E.001.4YajCZzgzIl7zf8bjWK2.ZIP' + }, + queue='process-ftp-tasks', + compression='zlib' + ) + assert mock_celery.call_args_list[4] == call( + name='zip-and-send-letter-pdfs', + kwargs={ + 'filenames_to_zip': [ + '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF', + ], + 'upload_filename': 'NOTIFY.2020-02-17.N.001.eSvP8Ph6EBKhh3k7BSA2.ZIP' }, queue='process-ftp-tasks', compression='zlib' From b01ec05aaf68c4558c594f83a99aa7e5e4c776d5 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 26 Jun 2020 15:15:10 +0100 Subject: [PATCH 03/15] run migrations if app is down normally we check the app's status page to see if migrations need running. However, if the _status endpoint doesn't respond with 200, we don't necessarily want to abort the deploy - we may be trying to deploy a code fix that fixes that status endpoint for example. We don't know whether to run the migrations or not, so err on the side of caution by re-running the migration. The migration itself might be the fix that gets the app working after all. had to do a little song and dance because sometimes the response won't be populated before an exception is thrown --- scripts/check_if_new_migration.py | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/scripts/check_if_new_migration.py b/scripts/check_if_new_migration.py index 2878086f6..b49f790a9 100644 --- a/scripts/check_if_new_migration.py +++ b/scripts/check_if_new_migration.py @@ -15,13 +15,24 @@ def get_latest_db_migration_to_apply(): def get_current_db_version(): api_status_url = '{}/_status'.format(os.getenv('API_HOST_NAME')) - response = requests.get(api_status_url) - if response.status_code != 200: - sys.exit('Could not make a request to the API: {}'.format()) - - current_db_version = response.json()['db_version'] - return current_db_version + try: + response = requests.get(api_status_url) + response.raise_for_status() + current_db_version = response.json()['db_version'] + return current_db_version + except requests.exceptions.ConnectionError: + print(f'Could not make web request to {api_status_url}', file=sys.stderr) + return '' + except Exception: # we expect these to be either either a http status code error, or a json decoding error + print( + f'Could not read status endpoint!\n\ncode {response.status_code}\nresponse "{response.text}"', + file=sys.stderr + ) + # if we can't make a request to the API, the API is probably down. By returning a blank string (which won't + # match the filename of the latest migration), we force the migration to run, as the code change to fix the api + # might involve a migration file. + return '' def run(): From 7fb3b3db1887dbd47aa325c5f514e05bf45661c1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 26 Jun 2020 14:23:25 +0100 Subject: [PATCH 04/15] Small changes to tidy up the code --- app/__init__.py | 1 - app/notifications/process_notifications.py | 10 +++------- app/v2/notifications/post_notifications.py | 19 ++++++++----------- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 4563cd473..62fe06c50 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -78,7 +78,6 @@ def create_app(application): from app.config import configs notify_environment = os.environ['NOTIFY_ENVIRONMENT'] - print(notify_environment) application.config.from_object(configs[notify_environment]) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index e5ca0adcc..f6b14df18 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -150,17 +150,13 @@ def persist_notification( notification.postage = postage or template_postage notification.normalised_to = ''.join(notification.to.split()).lower() - # Get service attributes before the commit - service_in_trial_mode = service.restricted - service_id = service.id - # if simulated create a Notification model to return but do not persist the Notification to the dB if not simulated: dao_create_notification(notification) # Only keep track of the daily limit for trial mode services. - if service_in_trial_mode and key_type != KEY_TYPE_TEST: - if redis_store.get(redis.daily_limit_cache_key(service_id)): - redis_store.incr(redis.daily_limit_cache_key(service_id)) + if service.restricted and key_type != KEY_TYPE_TEST: + if redis_store.get(redis.daily_limit_cache_key(service.id)): + redis_store.incr(redis.daily_limit_cache_key(service.id)) current_app.logger.info( "{} {} created at {}".format(notification_type, notification_id, notification_created_at) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index af0d766ea..aa204721a 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -149,7 +149,6 @@ def post_notification(notification_type): notification = process_sms_or_email_notification( form=form, notification_type=notification_type, - api_key=api_user, template=template, template_with_content=template_with_content, template_process_type=template.process_type, @@ -164,7 +163,6 @@ def process_sms_or_email_notification( *, form, notification_type, - api_key, template, template_with_content, template_process_type, @@ -175,7 +173,7 @@ def process_sms_or_email_notification( form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number'] send_to = validate_and_format_recipient(send_to=form_send_to, - key_type=api_key.key_type, + key_type=api_user.key_type, service=service, notification_type=notification_type) @@ -190,8 +188,7 @@ def process_sms_or_email_notification( if document_download_count: # We changed personalisation which means we need to update the content template_with_content.values = personalisation - key_type = api_key.key_type - service_in_research_mode = service.research_mode + resp = create_response_for_post_notification( notification_id=notification_id, client_reference=form.get('reference', None), @@ -203,7 +200,7 @@ def process_sms_or_email_notification( template_with_content=template_with_content ) - if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and api_key.key_type == KEY_TYPE_NORMAL \ + if service.id in current_app.config.get('HIGH_VOLUME_SERVICE') and api_user.key_type == KEY_TYPE_NORMAL \ and notification_type == EMAIL_TYPE: # Put GOV.UK Email notifications onto a queue # To take the pressure off the db for API requests put the notification for our high volume service onto a queue @@ -214,7 +211,7 @@ def process_sms_or_email_notification( form=form, notification_id=str(notification_id), notification_type=notification_type, - api_key=api_key, + api_key=api_user, template=template, service_id=service.id, personalisation=personalisation, @@ -237,8 +234,8 @@ def process_sms_or_email_notification( service=service, personalisation=personalisation, notification_type=notification_type, - api_key_id=api_key.id, - key_type=key_type, + api_key_id=api_user.id, + key_type=api_user.key_type, client_reference=form.get('reference', None), simulated=simulated, reply_to_text=reply_to_text, @@ -248,10 +245,10 @@ def process_sms_or_email_notification( if not simulated: queue_name = QueueNames.PRIORITY if template_process_type == PRIORITY else None send_notification_to_queue_detached( - key_type=key_type, + key_type=api_user.key_type, notification_type=notification_type, notification_id=notification_id, - research_mode=service_in_research_mode, # research_mode is deprecated + research_mode=service.research_mode, # research_mode is deprecated queue=queue_name ) else: From 61de908c5ddf59827118f86d6c1e1aa5c943e5f2 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 30 Jun 2020 17:54:47 +0100 Subject: [PATCH 05/15] Simplify putting letters in right postage folders --- app/celery/letters_pdf_tasks.py | 22 ++++---- app/dao/notifications_dao.py | 5 +- tests/app/celery/test_letters_pdf_tasks.py | 66 ++++------------------ 3 files changed, 24 insertions(+), 69 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index bb2f82537..75d13e18c 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -42,6 +42,7 @@ from app.models import ( NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_VALIDATION_FAILED, NOTIFICATION_VIRUS_SCAN_FAILED, + POSTAGE_TYPES, RESOLVE_POSTAGE_FOR_FILE_NAME ) from app.cronitor import cronitor @@ -135,21 +136,18 @@ def collate_letter_pdfs_to_be_sent(): print_run_deadline = print_run_date.replace( hour=17, minute=30, second=0, microsecond=0 ) + for postage in POSTAGE_TYPES: + letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage) - letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline) - - for zip_folder, letters_list in letters_to_print.items(): - i = 0 - for letters in group_letters(letters_list): - i += 1 + for i, letters in enumerate(group_letters(letters_to_print)): filenames = [letter['Key'] for letter in letters] hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode() # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP dvla_filename = 'NOTIFY.{date}.{postage}.{num:03}.{hash}.ZIP'.format( date=print_run_deadline.strftime("%Y-%m-%d"), - postage=RESOLVE_POSTAGE_FOR_FILE_NAME[zip_folder], - num=i, + postage=RESOLVE_POSTAGE_FOR_FILE_NAME[postage], + num=i + 1, hash=hash ) @@ -171,9 +169,9 @@ def collate_letter_pdfs_to_be_sent(): ) -def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline): - letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline) - letter_pdfs = {"first": [], "second": [], "europe": [], "rest-of-world": []} +def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage): + letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline, postage) + letter_pdfs = [] for letter in letters_awaiting_sending: try: letter_file_name = get_letter_pdf_filename( @@ -183,7 +181,7 @@ def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline): postage=letter.postage ) letter_head = s3.head_s3_object(current_app.config['LETTERS_PDF_BUCKET_NAME'], letter_file_name) - letter_pdfs[letter.postage].append({"Key": letter_file_name, "Size": letter_head['ContentLength']}) + letter_pdfs.append({"Key": letter_file_name, "Size": letter_head['ContentLength']}) except BotoClientError as e: current_app.logger.exception( f"Error getting letter from bucket for notification: {letter.id} with reference: {letter.reference}", e) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 6629a118c..db52bc5af 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -740,7 +740,7 @@ def notifications_not_yet_sent(should_be_sending_after_seconds, notification_typ return notifications -def dao_get_letters_to_be_printed(print_run_deadline): +def dao_get_letters_to_be_printed(print_run_deadline, postage): """ Return all letters created before the print run deadline that have not yet been sent """ @@ -748,7 +748,8 @@ def dao_get_letters_to_be_printed(print_run_deadline): Notification.created_at < convert_bst_to_utc(print_run_deadline), Notification.notification_type == LETTER_TYPE, Notification.status == NOTIFICATION_CREATED, - Notification.key_type == KEY_TYPE_NORMAL + Notification.key_type == KEY_TYPE_NORMAL, + Notification.postage == postage, ).order_by( Notification.created_at ).all() diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 287203af3..4b331f279 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -175,31 +175,6 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam created_at=(datetime.now() - timedelta(days=2)) ) - # first class - create_notification( - template=sample_letter_template, - status='created', - reference='first_class', - created_at=(datetime.now() - timedelta(hours=4)), - postage="first" - ) - - # international - create_notification( - template=sample_letter_template, - status='created', - reference='international', - created_at=(datetime.now() - timedelta(days=3)), - postage="europe" - ) - create_notification( - template=sample_letter_template, - status='created', - reference='international', - created_at=(datetime.now() - timedelta(days=4)), - postage="rest-of-world" - ) - # notifications we don't expect to get sent to print as they are in the wrong status for status in ['delivered', 'validation-failed', 'cancelled', 'sending']: create_notification( @@ -227,48 +202,29 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam ) mock_s3 = mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ - {'ContentLength': 1}, - {'ContentLength': 1}, {'ContentLength': 2}, {'ContentLength': 1}, {'ContentLength': 3}, - {'ContentLength': 1}, ]) - results = get_key_and_size_of_letters_to_be_sent_to_print(datetime.now() - timedelta(minutes=30)) + results = get_key_and_size_of_letters_to_be_sent_to_print(datetime.now() - timedelta(minutes=30), postage='second') - assert mock_s3.call_count == 6 + assert mock_s3.call_count == 3 mock_s3.assert_has_calls( [ - call(current_app.config[ - 'LETTERS_PDF_BUCKET_NAME' - ], '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF'), - call(current_app.config[ - 'LETTERS_PDF_BUCKET_NAME' - ], '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.C.20200214180000.PDF'), call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF'), - call(current_app.config[ - 'LETTERS_PDF_BUCKET_NAME' - ], '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.C.20200217140000.PDF'), call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF'), call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF'), ] ) - assert results == { - "first": [{'Key': '2020-02-17/NOTIFY.FIRST_CLASS.D.1.C.C.20200217140000.PDF', 'Size': 1}], - "second": [ - {'Key': '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', 'Size': 2}, - {'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 3}, - {'Key': '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', 'Size': 1}, - ], - "europe": [ - {'Key': '2020-02-15/NOTIFY.INTERNATIONAL.D.E.C.C.20200214180000.PDF', 'Size': 1}, - ], - "rest-of-world": [ - {'Key': '2020-02-14/NOTIFY.INTERNATIONAL.D.N.C.C.20200213180000.PDF', 'Size': 1}, - ] - } + assert len(results) == 3 + + assert results == [ + {'Key': '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', 'Size': 2}, + {'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 1}, + {'Key': '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', 'Size': 3}, + ] @freeze_time('2020-02-17 18:00:00') @@ -300,7 +256,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print_catches_exception( ClientError(error_response, "File not found") ]) - results = get_key_and_size_of_letters_to_be_sent_to_print(datetime.now() - timedelta(minutes=30)) + results = get_key_and_size_of_letters_to_be_sent_to_print(datetime.now() - timedelta(minutes=30), postage='second') assert mock_head_s3_object.call_count == 2 mock_head_s3_object.assert_has_calls( @@ -310,7 +266,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print_catches_exception( ] ) - assert results["second"] == [{'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 2}] + assert results == [{'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 2}] @pytest.mark.parametrize('time_to_run_task', [ From 9c25b7dfd53e6a423ea2a133937081749122ec26 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Jul 2020 17:00:44 +0100 Subject: [PATCH 06/15] remove broadcast_data from SerialisedTemplate.ALLOWED_PROPERTIES ALLOWED_PROPERTIES is a list containing fields that it will attempt to deserialize into the returned object. If a field isn't present on the underlying data source (whether that's a dump from a marshmallow schema, a blob retrieved from redis, or anything else), then SerialisedModel will raise an exception. However, SerialisedModel isn't involved when setting the cache, so with that said the correct flow for adding a column to a cached database model: PR #1 * add new column to DB * add new column to model, and to template_schema (because that schema is used to create the dict that goes in to redis). New redis keys start getting populated. Deploy that through, and then clear redis PR #2 * add new column to SerialisedTemplate.ALLOWED_PROPERTIES. this means that it'll start reading that value from redis * now instances of the app will always have the new field in their template objects, whether they came from database directly or from the cache This commit removes the field from ALLOWED_PROPERTIES. After it's deployed we'll be able to clear redis, observe redis being populated with the new field, and then we'll be able to re-add it to ALLOWED_PROPERTIES, ready to use. --- app/serialised_models.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/serialised_models.py b/app/serialised_models.py index 50adeceeb..1b17726f0 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -47,7 +47,6 @@ class SerialisedTemplate(SerialisedModel): 'subject', 'template_type', 'version', - 'broadcast_data', } @classmethod From 67f6dcae45c46dc3fe86ba3a980a788bf3842a11 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Jul 2020 11:42:38 +0100 Subject: [PATCH 07/15] add broadcast message crud new blueprint `/service//broadcast-message` with the following endpoints: * GET / - get all broadcast messages for a service * GET / - get a single broadcast message * POST / - create a new broadcast message * POST / - update an existing broadcast message's data * POST //status - move a broadcast message to a new status I've kept the regular data update (eg personalisation, start and end times) separate from the status update, just to keep separation of concerns a bit more rigid, especially around who can update. I've included schemas for the three POSTs, they're pretty straightforward. --- app/__init__.py | 4 + app/broadcast_message/__init__.py | 0 .../broadcast_message_schema.py | 48 +++++++ app/broadcast_message/rest.py | 123 ++++++++++++++++++ app/dao/broadcast_message_dao.py | 26 ++++ app/models.py | 44 ++++++- 6 files changed, 243 insertions(+), 2 deletions(-) create mode 100644 app/broadcast_message/__init__.py create mode 100644 app/broadcast_message/broadcast_message_schema.py create mode 100644 app/broadcast_message/rest.py create mode 100644 app/dao/broadcast_message_dao.py diff --git a/app/__init__.py b/app/__init__.py index 255af272d..d16e0cdbe 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -152,6 +152,7 @@ def register_blueprint(application): from app.template_folder.rest import template_folder_blueprint from app.letter_branding.letter_branding_rest import letter_branding_blueprint from app.upload.rest import upload_blueprint + from app.broadcast_message.rest import broadcast_message_blueprint service_blueprint.before_request(requires_admin_auth) application.register_blueprint(service_blueprint, url_prefix='/service') @@ -237,6 +238,9 @@ def register_blueprint(application): upload_blueprint.before_request(requires_admin_auth) application.register_blueprint(upload_blueprint) + broadcast_message_blueprint.before_request(requires_admin_auth) + application.register_blueprint(broadcast_message_blueprint) + def register_v2_blueprints(application): from app.v2.inbound_sms.get_inbound_sms import v2_inbound_sms_blueprint as get_inbound_sms diff --git a/app/broadcast_message/__init__.py b/app/broadcast_message/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/broadcast_message/broadcast_message_schema.py b/app/broadcast_message/broadcast_message_schema.py new file mode 100644 index 000000000..f8a668127 --- /dev/null +++ b/app/broadcast_message/broadcast_message_schema.py @@ -0,0 +1,48 @@ +from app.schema_validation.definitions import uuid +from app.models import BroadcastStatusType + +create_broadcast_message_schema = { + '$schema': 'http://json-schema.org/draft-04/schema#', + 'description': 'POST create broadcast_message schema', + 'type': 'object', + 'title': 'Create broadcast_message', + 'properties': { + 'template_id': uuid, + 'service_id': uuid, + 'created_by': uuid, + 'personalisation': {'type': 'object'}, + 'starts_at': {'type': 'string', 'format': 'date-time'}, + 'finishes_at': {'type': 'string', 'format': 'date-time'}, + 'areas': {"type": "array", "items": {"type": "string"}}, + }, + 'required': ['template_id', 'service_id', 'created_by'], + 'additionalProperties': False +} + +update_broadcast_message_schema = { + '$schema': 'http://json-schema.org/draft-04/schema#', + 'description': 'POST update broadcast_message schema', + 'type': 'object', + 'title': 'Update broadcast_message', + 'properties': { + 'personalisation': {'type': 'object'}, + 'starts_at': {'type': 'string', 'format': 'date-time'}, + 'finishes_at': {'type': 'string', 'format': 'date-time'}, + 'areas': {"type": "array", "items": {"type": "string"}}, + }, + 'required': [], + 'additionalProperties': False +} + +update_broadcast_message_status_schema = { + '$schema': 'http://json-schema.org/draft-04/schema#', + 'description': 'POST update broadcast_message status schema', + 'type': 'object', + 'title': 'Update broadcast_message', + 'properties': { + 'status': {'type': 'string', 'enum': BroadcastStatusType.STATUSES}, + 'created_by': uuid, + }, + 'required': ['status', 'created_by'], + 'additionalProperties': False +} diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py new file mode 100644 index 000000000..9fa67a44a --- /dev/null +++ b/app/broadcast_message/rest.py @@ -0,0 +1,123 @@ +from datetime import datetime + +import iso8601 +from flask import Blueprint, jsonify, request + +from app.dao.templates_dao import dao_get_template_by_id_and_service_id +from app.dao.users_dao import get_user_by_id +from app.dao.broadcast_message_dao import ( + dao_create_broadcast_message, + dao_get_broadcast_message_by_id_and_service_id, + dao_get_broadcast_messages_for_service, + dao_update_broadcast_message, +) +from app.dao.services_dao import dao_fetch_service_by_id +from app.errors import register_errors +from app.models import BroadcastMessage, BroadcastStatusType +from app.broadcast_message.broadcast_message_schema import ( + create_broadcast_message_schema, + update_broadcast_message_schema, + update_broadcast_message_status_schema, +) +from app.schema_validation import validate + +broadcast_message_blueprint = Blueprint( + 'broadcast_message', + __name__, + url_prefix='/service//broadcast-message' +) +register_errors(broadcast_message_blueprint) + + +def _parse_nullable_datetime(dt): + if dt: + return iso8601.parse_date(dt).replace(tzinfo=None) + return dt + + +@broadcast_message_blueprint.route('', methods=['GET']) +def get_broadcast_messages_for_service(service_id): + # TODO: should this return template content/data in some way? or can we rely on them being cached admin side. + # we might need stuff like template name for showing on the dashboard. + # TODO: should this paginate or filter on dates or anything? + broadcast_messages = [o.serialize() for o in dao_get_broadcast_messages_for_service(service_id)] + return jsonify(broadcast_messages=broadcast_messages) + + +@broadcast_message_blueprint.route('/', methods=['GET']) +def get_broadcast_message(service_id, broadcast_message_id): + return jsonify(dao_get_broadcast_message_by_id_and_service_id(broadcast_message_id, service_id).serialize()) + + +@broadcast_message_blueprint.route('', methods=['POST']) +def create_broadcast_message(service_id): + data = request.get_json() + + validate(data, create_broadcast_message_schema) + service = dao_fetch_service_by_id(data['service_id']) + user = get_user_by_id(data['created_by']) + template = dao_get_template_by_id_and_service_id(data['template_id'], data['service_id']) + + broadcast_message = BroadcastMessage( + service_id=service.id, + template_id=template.id, + template_version=template.version, + personalisation=data.get('personalisation', {}), + areas=data.get('areas', []), + status=BroadcastStatusType.DRAFT, + starts_at=_parse_nullable_datetime(data.get('starts_at')), + finishes_at=_parse_nullable_datetime(data.get('finishes_at')), + created_by_id=user.id, + ) + + dao_create_broadcast_message(broadcast_message) + + return jsonify(broadcast_message.serialize()), 201 + + +@broadcast_message_blueprint.route('/', methods=['POST']) +def update_broadcast_message(service_id, broadcast_message_id): + data = request.get_json() + + validate(data, update_broadcast_message_schema) + + broadcast_message = dao_get_broadcast_message_by_id_and_service_id(broadcast_message_id, service_id) + + if 'personalisation' in data: + broadcast_message.personalisation = data['personalisation'] + if 'starts_at' in data: + broadcast_message.starts_at = _parse_nullable_datetime(data['starts_at']) + if 'finishes_at' in data: + broadcast_message.starts_at = _parse_nullable_datetime(data['finishes_at']) + if 'areas' in data: + broadcast_message.areas = data['areas'] + + dao_update_broadcast_message(broadcast_message) + + return jsonify(broadcast_message.serialize()), 200 + + +@broadcast_message_blueprint.route('//status', methods=['POST']) +def update_broadcast_message_status(service_id, broadcast_message_id): + data = request.get_json() + + validate(data, update_broadcast_message_status_schema) + broadcast_message = dao_get_broadcast_message_by_id_and_service_id(broadcast_message_id, service_id) + + new_status = data['status'] + + # TODO: Restrict status transitions + # TODO: Do we need to validate that the user belongs to the same service, isn't the creator, has permissions, etc? + # or is that admin's job + if new_status == BroadcastStatusType.BROADCASTING: + broadcast_message.approved_at = datetime.utcnow() + broadcast_message.approved_by = get_user_by_id(data['created_by']) + if new_status == BroadcastStatusType.CANCELLED: + broadcast_message.cancelled_at = datetime.utcnow() + broadcast_message.cancelled_by = get_user_by_id(data['created_by']) + + broadcast_message.status = new_status + + dao_update_broadcast_message(broadcast_message) + + return jsonify(broadcast_message.serialize()), 200 diff --git a/app/dao/broadcast_message_dao.py b/app/dao/broadcast_message_dao.py new file mode 100644 index 000000000..068dd3653 --- /dev/null +++ b/app/dao/broadcast_message_dao.py @@ -0,0 +1,26 @@ +from app import db +from app.models import BroadcastMessage +from app.dao.dao_utils import transactional + + +@transactional +def dao_create_broadcast_message(broadcast_message): + db.session.add(broadcast_message) + + +@transactional +def dao_update_broadcast_message(broadcast_message): + db.session.add(broadcast_message) + + +def dao_get_broadcast_message_by_id_and_service_id(broadcast_message_id, service_id): + return BroadcastMessage.query.filter( + BroadcastMessage.id == broadcast_message_id, + BroadcastMessage.service_id == service_id + ).one() + + +def dao_get_broadcast_messages_for_service(service_id): + return BroadcastMessage.query.filter( + BroadcastMessage.service_id == service_id + ).order_by(BroadcastMessage.created_at) diff --git a/app/models.py b/app/models.py index eae9c921a..be00abfd1 100644 --- a/app/models.py +++ b/app/models.py @@ -2174,7 +2174,7 @@ class BroadcastMessage(db.Model): {} ) - id = db.Column(UUID(as_uuid=True), primary_key=True) + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id')) service = db.relationship('Service', backref='broadcast_messages') @@ -2184,6 +2184,8 @@ class BroadcastMessage(db.Model): template = db.relationship('TemplateHistory', backref='broadcast_messages') _personalisation = db.Column(db.String, nullable=True) + # defaults to empty list + areas = db.Column(JSONB(none_as_null=True), nullable=False, default=list) status = db.Column( db.String, @@ -2197,7 +2199,7 @@ class BroadcastMessage(db.Model): finishes_at = db.Column(db.DateTime, nullable=True) # isn't updated if user cancels # these times correspond to when - created_at = db.Column(db.DateTime, nullable=False) + created_at = db.Column(db.DateTime, nullable=False, default=datetime.datetime.utcnow) approved_at = db.Column(db.DateTime, nullable=True) cancelled_at = db.Column(db.DateTime, nullable=True) updated_at = db.Column(db.DateTime, nullable=True, onupdate=datetime.datetime.utcnow) @@ -2209,3 +2211,41 @@ class BroadcastMessage(db.Model): created_by = db.relationship('User', foreign_keys=[created_by_id]) approved_by = db.relationship('User', foreign_keys=[approved_by_id]) cancelled_by = db.relationship('User', foreign_keys=[cancelled_by_id]) + + @property + def personalisation(self): + if self._personalisation: + return encryption.decrypt(self._personalisation) + return {} + + @personalisation.setter + def personalisation(self, personalisation): + self._personalisation = encryption.encrypt(personalisation or {}) + + def serialize(self): + return { + 'id': self.id, + + 'service_id': self.service_id, + + 'template_id': self.template_id, + 'template_version': self.template_version, + 'template_name': self.template.name, + + 'personalisation': self.personalisation, + 'areas': self.areas, + + 'status': self.status, + + 'starts_at': self.starts_at.strftime(DATETIME_FORMAT) if self.starts_at else None, + 'finishes_at': self.finishes_at.strftime(DATETIME_FORMAT) if self.finishes_at else None, + + 'created_at': self.created_at.strftime(DATETIME_FORMAT) if self.created_at else None, + 'approved_at': self.approved_at.strftime(DATETIME_FORMAT) if self.approved_at else None, + 'cancelled_at': self.cancelled_at.strftime(DATETIME_FORMAT) if self.cancelled_at else None, + 'updated_at': self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None, + + 'created_by_id': self.created_by_id, + 'approved_by_id': self.approved_by_id, + 'cancelled_by_id': self.cancelled_by_id, + } From 61a5730596ca2990851bd4e3b69c9e0c578264c9 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Jul 2020 12:59:09 +0100 Subject: [PATCH 08/15] add more friendly datetime validator to jsonschema add `datetime` format (note, not the built-in `date-time`) to our json schemas. this uses the iso8601 library to try and parse the string. also, move `strict-rfc3339` and `rfc3987` (used by jsonschema to validate `date-time` and `uri` formats respectively from test requirements to regular requirements. if they're not installed, validation silently succeeds, so validation wouldnt reject anything bad on prod, only in unit tests. --- app/broadcast_message/broadcast_message_schema.py | 8 ++++---- app/schema_validation/__init__.py | 11 +++++++++++ requirements-app.txt | 2 ++ requirements.txt | 6 ++++-- requirements_for_test.txt | 3 --- 5 files changed, 21 insertions(+), 9 deletions(-) diff --git a/app/broadcast_message/broadcast_message_schema.py b/app/broadcast_message/broadcast_message_schema.py index f8a668127..743ca54d7 100644 --- a/app/broadcast_message/broadcast_message_schema.py +++ b/app/broadcast_message/broadcast_message_schema.py @@ -11,8 +11,8 @@ create_broadcast_message_schema = { 'service_id': uuid, 'created_by': uuid, 'personalisation': {'type': 'object'}, - 'starts_at': {'type': 'string', 'format': 'date-time'}, - 'finishes_at': {'type': 'string', 'format': 'date-time'}, + 'starts_at': {'type': 'string', 'format': 'datetime'}, + 'finishes_at': {'type': 'string', 'format': 'datetime'}, 'areas': {"type": "array", "items": {"type": "string"}}, }, 'required': ['template_id', 'service_id', 'created_by'], @@ -26,8 +26,8 @@ update_broadcast_message_schema = { 'title': 'Update broadcast_message', 'properties': { 'personalisation': {'type': 'object'}, - 'starts_at': {'type': 'string', 'format': 'date-time'}, - 'finishes_at': {'type': 'string', 'format': 'date-time'}, + 'starts_at': {'type': 'string', 'format': 'datetime'}, + 'finishes_at': {'type': 'string', 'format': 'datetime'}, 'areas': {"type": "array", "items": {"type": "string"}}, }, 'required': [], diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 0ddd51f16..f5f2dc9ef 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -54,6 +54,17 @@ def validate_schema_date_with_hour(instance): return True +@format_checker.checks('datetime', raises=ValidationError) +def validate_schema_datetime(instance): + if isinstance(instance, str): + try: + iso8601.parse_date(instance) + except ParseError: + raise ValidationError("datetime format is invalid. It must be a valid ISO8601 date time format, " + "https://en.wikipedia.org/wiki/ISO_8601") + return True + + def validate(json_to_validate, schema): validator = Draft7Validator(schema, format_checker=format_checker) errors = list(validator.iter_errors(json_to_validate)) diff --git a/requirements-app.txt b/requirements-app.txt index 50f3b6f28..ed4803a3a 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -20,6 +20,8 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.5 PyJWT==1.7.1 SQLAlchemy==1.3.17 +strict-rfc3339==0.7 +rfc3987==1.3.8 cachetools==4.1.0 notifications-python-client==5.5.1 diff --git a/requirements.txt b/requirements.txt index e42c6931c..c96dd1dfe 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,6 +22,8 @@ marshmallow==2.21.0 # pyup: <3 # v3 throws errors psycopg2-binary==2.8.5 PyJWT==1.7.1 SQLAlchemy==1.3.17 +strict-rfc3339==0.7 +rfc3987==1.3.8 cachetools==4.1.0 notifications-python-client==5.5.1 @@ -40,14 +42,14 @@ alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.93 +awscli==1.18.96 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.16 +botocore==1.17.19 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 diff --git a/requirements_for_test.txt b/requirements_for_test.txt index f8edc4dc1..9db32b421 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -8,8 +8,5 @@ pytest-cov==2.8.1 pytest-xdist==1.31.0 freezegun==0.3.12 requests-mock==1.7.0 -# optional requirements for jsonschema -strict-rfc3339==0.7 -rfc3987==1.3.8 # used for creating manifest file locally jinja2-cli[yaml]==0.7.0 From efa2e75e569a8b26e8d32efb46d9124d4d7154ff Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Jul 2020 14:19:09 +0100 Subject: [PATCH 09/15] add broadcast message tests sorry for big unhelpful test commit --- tests/app/broadcast_message/__init__.py | 0 tests/app/broadcast_message/test_rest.py | 259 +++++++++++++++++++++++ tests/app/db.py | 29 ++- 3 files changed, 287 insertions(+), 1 deletion(-) create mode 100644 tests/app/broadcast_message/__init__.py create mode 100644 tests/app/broadcast_message/test_rest.py diff --git a/tests/app/broadcast_message/__init__.py b/tests/app/broadcast_message/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py new file mode 100644 index 000000000..14657f5b3 --- /dev/null +++ b/tests/app/broadcast_message/test_rest.py @@ -0,0 +1,259 @@ +import uuid + +from freezegun import freeze_time +import pytest + +from app.models import BROADCAST_TYPE, BroadcastStatusType + +from tests.app.db import create_broadcast_message, create_template, create_service, create_user + + +def test_get_broadcast_message(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t, areas=['place A', 'region B']) + + response = admin_request.get( + 'broadcast_message.get_broadcast_message', + service_id=t.service_id, + broadcast_message_id=bm.id, + _expected_status=200 + ) + + assert response['id'] == str(bm.id) + assert response['template_name'] == t.name + assert response['status'] == BroadcastStatusType.DRAFT + assert response['created_at'] is not None + assert response['starts_at'] is None + assert response['areas'] == ['place A', 'region B'] + assert response['personalisation'] == {} + + +def test_get_broadcast_message_404s_if_message_doesnt_exist(admin_request, sample_service): + err = admin_request.get( + 'broadcast_message.get_broadcast_message', + service_id=sample_service.id, + broadcast_message_id=uuid.uuid4(), + _expected_status=404 + ) + assert err == {'message': 'No result found', 'result': 'error'} + + +def test_get_broadcast_message_404s_if_message_is_for_different_service(admin_request, sample_service): + other_service = create_service(service_name='other') + other_template = create_template(other_service, BROADCAST_TYPE) + bm = create_broadcast_message(other_template) + + err = admin_request.get( + 'broadcast_message.get_broadcast_message', + service_id=sample_service.id, + broadcast_message_id=bm.id, + _expected_status=404 + ) + assert err == {'message': 'No result found', 'result': 'error'} + + +@freeze_time('2020-01-01') +def test_get_broadcast_messages_for_service(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + + with freeze_time('2020-01-01 12:00'): + bm1 = create_broadcast_message(t, personalisation={'foo': 'bar'}) + with freeze_time('2020-01-01 13:00'): + bm2 = create_broadcast_message(t, personalisation={'foo': 'baz'}) + + response = admin_request.get( + 'broadcast_message.get_broadcast_messages_for_service', + service_id=t.service_id, + _expected_status=200 + ) + + assert response['broadcast_messages'][0]['id'] == str(bm1.id) + assert response['broadcast_messages'][1]['id'] == str(bm2.id) + + +@freeze_time('2020-01-01') +def test_create_broadcast_message(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + + response = admin_request.post( + 'broadcast_message.create_broadcast_message', + _data={ + 'template_id': str(t.id), + 'service_id': str(t.service_id), + 'created_by': str(t.created_by_id), + }, + service_id=t.service_id, + _expected_status=201 + ) + + assert response['template_name'] == t.name + assert response['status'] == BroadcastStatusType.DRAFT + assert response['created_at'] is not None + assert response['created_by_id'] == str(t.created_by_id) + assert response['personalisation'] == {} + assert response['areas'] == [] + + +@pytest.mark.parametrize('data, expected_errors', [ + ( + {}, + [ + {'error': 'ValidationError', 'message': 'template_id is a required property'}, + {'error': 'ValidationError', 'message': 'service_id is a required property'}, + {'error': 'ValidationError', 'message': 'created_by is a required property'} + ] + ), + ( + { + 'template_id': str(uuid.uuid4()), + 'service_id': str(uuid.uuid4()), + 'created_by': str(uuid.uuid4()), + 'foo': 'something else' + }, + [ + {'error': 'ValidationError', 'message': 'Additional properties are not allowed (foo was unexpected)'} + ] + ) +]) +def test_create_broadcast_message_400s_if_json_schema_fails_validation( + admin_request, + sample_service, + data, + expected_errors +): + t = create_template(sample_service, BROADCAST_TYPE) + + response = admin_request.post( + 'broadcast_message.create_broadcast_message', + _data=data, + service_id=t.service_id, + _expected_status=400 + ) + assert response['errors'] == expected_errors + + +def test_update_broadcast_message(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t, areas=['manchester']) + + response = admin_request.post( + 'broadcast_message.update_broadcast_message', + _data={'starts_at': '2020-06-01 20:00:01', 'areas': ['london', 'glasgow']}, + service_id=t.service_id, + broadcast_message_id=bm.id, + _expected_status=200 + ) + + assert response['starts_at'] == '2020-06-01T20:00:01.000000Z' + assert response['areas'] == ['london', 'glasgow'] + assert response['updated_at'] is not None + + +@pytest.mark.parametrize('input_dt', [ + '2020-06-01 20:00:01', + '2020-06-01T20:00:01', + '2020-06-01 20:00:01Z', + '2020-06-01T20:00:01+00:00', +]) +def test_update_broadcast_message_allows_sensible_datetime_formats(admin_request, sample_service, input_dt): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t) + + response = admin_request.post( + 'broadcast_message.update_broadcast_message', + _data={'starts_at': input_dt}, + service_id=t.service_id, + broadcast_message_id=bm.id, + _expected_status=200 + ) + + assert response['starts_at'] == '2020-06-01T20:00:01.000000Z' + assert response['updated_at'] is not None + + +def test_update_broadcast_message_doesnt_let_you_update_status(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t) + + response = admin_request.post( + 'broadcast_message.update_broadcast_message', + _data={'areas': ['glasgow'], 'status': BroadcastStatusType.BROADCASTING}, + service_id=t.service_id, + broadcast_message_id=bm.id, + _expected_status=400 + ) + + assert response['errors'] == [{ + 'error': 'ValidationError', + 'message': 'Additional properties are not allowed (status was unexpected)' + }] + + +def test_update_broadcast_message_status(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t, status=BroadcastStatusType.DRAFT) + + response = admin_request.post( + 'broadcast_message.update_broadcast_message_status', + _data={'status': BroadcastStatusType.PENDING_APPROVAL, 'created_by': str(t.created_by_id)}, + service_id=t.service_id, + broadcast_message_id=bm.id, + _expected_status=200 + ) + + assert response['status'] == BroadcastStatusType.PENDING_APPROVAL + assert response['updated_at'] is not None + + +def test_update_broadcast_message_status_doesnt_let_you_update_other_things(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t) + + response = admin_request.post( + 'broadcast_message.update_broadcast_message_status', + _data={'areas': ['glasgow'], 'status': BroadcastStatusType.BROADCASTING, 'created_by': str(t.created_by_id)}, + service_id=t.service_id, + broadcast_message_id=bm.id, + _expected_status=400 + ) + + assert response['errors'] == [{ + 'error': 'ValidationError', + 'message': 'Additional properties are not allowed (areas was unexpected)' + }] + + +def test_update_broadcast_message_status_stores_cancelled_by_and_cancelled_at(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t, status=BroadcastStatusType.BROADCASTING) + canceller = create_user('canceller@gov.uk') + + response = admin_request.post( + 'broadcast_message.update_broadcast_message_status', + _data={'status': BroadcastStatusType.CANCELLED, 'created_by': str(canceller.id)}, + service_id=t.service_id, + broadcast_message_id=bm.id, + _expected_status=200 + ) + + assert response['status'] == BroadcastStatusType.CANCELLED + assert response['cancelled_at'] is not None + assert response['cancelled_by_id'] == str(canceller.id) + + +def test_update_broadcast_message_status_stores_approved_by_and_approved_at(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t, status=BroadcastStatusType.PENDING_APPROVAL) + approver = create_user('approver@gov.uk') + + response = admin_request.post( + 'broadcast_message.update_broadcast_message_status', + _data={'status': BroadcastStatusType.BROADCASTING, 'created_by': str(approver.id)}, + service_id=t.service_id, + broadcast_message_id=bm.id, + _expected_status=200 + ) + + assert response['status'] == BroadcastStatusType.BROADCASTING + assert response['approved_at'] is not None + assert response['approved_by_id'] == str(approver.id) diff --git a/tests/app/db.py b/tests/app/db.py index 08a91e453..e45bff2af 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -59,7 +59,9 @@ from app.models import ( Domain, NotificationHistory, ReturnedLetter, - ServiceContactList + ServiceContactList, + BroadcastMessage, + BroadcastStatusType, ) @@ -984,3 +986,28 @@ def create_service_contact_list( db.session.add(contact_list) db.session.commit() return contact_list + + +def create_broadcast_message( + template, + created_by=None, + personalisation={}, + status=BroadcastStatusType.DRAFT, + starts_at=None, + finishes_at=None, + areas=[], +): + broadcast_message = BroadcastMessage( + service_id=template.service_id, + template_id=template.id, + template_version=template.version, + personalisation=personalisation, + status=BroadcastStatusType.DRAFT, + starts_at=starts_at, + finishes_at=finishes_at, + created_by_id=created_by.id if created_by else template.created_by_id, + areas=areas, + ) + db.session.add(broadcast_message) + db.session.commit() + return broadcast_message From 6b5e2af4970fd7a0bb2e3cbb7d9339fb8cfe6df0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Jul 2020 15:36:08 +0100 Subject: [PATCH 10/15] set finishes_at correctly whoops --- app/broadcast_message/rest.py | 2 +- tests/app/broadcast_message/test_rest.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 9fa67a44a..559574b4d 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -88,7 +88,7 @@ def update_broadcast_message(service_id, broadcast_message_id): if 'starts_at' in data: broadcast_message.starts_at = _parse_nullable_datetime(data['starts_at']) if 'finishes_at' in data: - broadcast_message.starts_at = _parse_nullable_datetime(data['finishes_at']) + broadcast_message.finishes_at = _parse_nullable_datetime(data['finishes_at']) if 'areas' in data: broadcast_message.areas = data['areas'] diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index 14657f5b3..a1aa0f9fe 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -149,6 +149,23 @@ def test_update_broadcast_message(admin_request, sample_service): assert response['updated_at'] is not None +def test_update_broadcast_message_sets_finishes_at_separately(admin_request, sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t, areas=['manchester']) + + response = admin_request.post( + 'broadcast_message.update_broadcast_message', + _data={'starts_at': '2020-06-01 20:00:01', 'finishes_at': '2020-06-02 20:00:01'}, + service_id=t.service_id, + broadcast_message_id=bm.id, + _expected_status=200 + ) + + assert response['starts_at'] == '2020-06-01T20:00:01.000000Z' + assert response['finishes_at'] == '2020-06-02T20:00:01.000000Z' + assert response['updated_at'] is not None + + @pytest.mark.parametrize('input_dt', [ '2020-06-01 20:00:01', '2020-06-01T20:00:01', From eca37d0853d210dffa423e5e9ffb350033140a3a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Jul 2020 18:22:29 +0100 Subject: [PATCH 11/15] add send_broadcast_message task task takes a brodcast_message_id, and makes a post to the cbc-proxy for now, hardcode the url to the notify stub. the stub requires template as the admin/api get it, so use the marshmallow schema to json dump it. Note - this also required us to tweak the BroadcastMessage.serialize function so that it converts uuids in to ids - flask's jsonify function does that for free but requests.post doesn't sadly. if the request fails (either 4xx or 5xx) just raise an exception and let it bubble up for now - in the future we'll add retry logic --- app/celery/broadcast_message_tasks.py | 35 +++++++++++ app/config.py | 10 +++ app/dao/broadcast_message_dao.py | 4 ++ app/models.py | 12 ++-- .../celery/test_broadcast_message_tasks.py | 61 +++++++++++++++++++ 5 files changed, 116 insertions(+), 6 deletions(-) create mode 100644 app/celery/broadcast_message_tasks.py create mode 100644 tests/app/celery/test_broadcast_message_tasks.py diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py new file mode 100644 index 000000000..4451aee7e --- /dev/null +++ b/app/celery/broadcast_message_tasks.py @@ -0,0 +1,35 @@ +import requests +from flask import current_app +from notifications_utils.statsd_decorators import statsd + +from app import notify_celery +from app.schemas import template_schema + + +from app.dao.broadcast_message_dao import dao_get_broadcast_message_by_id + + +@notify_celery.task(name="send-broadcast-message") +@statsd(namespace="tasks") +def send_broadcast_message(broadcast_message_id, provider='stub-1'): + broadcast_message = dao_get_broadcast_message_by_id(broadcast_message_id) + + current_app.logger.info( + f'sending broadcast_message {broadcast_message_id} ' + f'status {broadcast_message.status} to {provider}' + ) + + payload = { + "template": template_schema.dump(broadcast_message.template).data, + "broadcast_message": broadcast_message.serialize(), + } + resp = requests.post( + f'{current_app.config["CBC_PROXY_URL"]}/broadcasts/{provider}', + json=payload + ) + resp.raise_for_status() + + current_app.logger.info( + f'broadcast_message {broadcast_message.id} ' + f'status {broadcast_message.status} sent to {provider}' + ) diff --git a/app/config.py b/app/config.py index 0ada716b0..b0e3b555c 100644 --- a/app/config.py +++ b/app/config.py @@ -112,6 +112,9 @@ class Config(object): # Antivirus ANTIVIRUS_ENABLED = True + # Broadcast Messaging + CBC_PROXY_URL = None + ########################### # Default config values ### ########################### @@ -393,6 +396,8 @@ class Development(Config): API_HOST_NAME = "http://localhost:6011" API_RATE_LIMIT_ENABLED = True + CBC_PROXY_URL = 'http://localhost:8080' + class Test(Development): NOTIFY_EMAIL_DOMAIN = 'test.notify.com' @@ -436,6 +441,8 @@ class Test(Development): FIRETEXT_INBOUND_SMS_AUTH = ['testkey'] TEMPLATE_PREVIEW_API_HOST = 'http://localhost:9999' + CBC_PROXY_URL = 'http://test-cbc-proxy' + MMG_URL = 'https://example.com/mmg' FIRETEXT_URL = 'https://example.com/firetext' @@ -452,6 +459,7 @@ class Preview(Config): INVALID_PDF_BUCKET_NAME = 'preview-letters-invalid-pdf' TRANSIENT_UPLOADED_LETTERS = 'preview-transient-uploaded-letters' LETTER_SANITISE_BUCKET_NAME = 'preview-letters-sanitise' + CBC_PROXY_URL = 'https://notify-stub-cbc-sandbox.cloudapps.digital' FROM_NUMBER = 'preview' API_RATE_LIMIT_ENABLED = True CHECK_PROXY_HEADER = False @@ -469,6 +477,7 @@ class Staging(Config): INVALID_PDF_BUCKET_NAME = 'staging-letters-invalid-pdf' TRANSIENT_UPLOADED_LETTERS = 'staging-transient-uploaded-letters' LETTER_SANITISE_BUCKET_NAME = 'staging-letters-sanitise' + CBC_PROXY_URL = 'https://notify-stub-cbc-sandbox.cloudapps.digital' FROM_NUMBER = 'stage' API_RATE_LIMIT_ENABLED = True CHECK_PROXY_HEADER = True @@ -487,6 +496,7 @@ class Live(Config): INVALID_PDF_BUCKET_NAME = 'production-letters-invalid-pdf' TRANSIENT_UPLOADED_LETTERS = 'production-transient-uploaded-letters' LETTER_SANITISE_BUCKET_NAME = 'production-letters-sanitise' + CBC_PROXY_URL = 'https://notify-stub-cbc-sandbox.cloudapps.digital' FROM_NUMBER = 'GOVUK' PERFORMANCE_PLATFORM_ENABLED = True API_RATE_LIMIT_ENABLED = True diff --git a/app/dao/broadcast_message_dao.py b/app/dao/broadcast_message_dao.py index 068dd3653..8e5849394 100644 --- a/app/dao/broadcast_message_dao.py +++ b/app/dao/broadcast_message_dao.py @@ -20,6 +20,10 @@ def dao_get_broadcast_message_by_id_and_service_id(broadcast_message_id, service ).one() +def dao_get_broadcast_message_by_id(broadcast_message_id): + return BroadcastMessage.query.get(broadcast_message_id) + + def dao_get_broadcast_messages_for_service(service_id): return BroadcastMessage.query.filter( BroadcastMessage.service_id == service_id diff --git a/app/models.py b/app/models.py index be00abfd1..81670144c 100644 --- a/app/models.py +++ b/app/models.py @@ -2224,11 +2224,11 @@ class BroadcastMessage(db.Model): def serialize(self): return { - 'id': self.id, + 'id': str(self.id), - 'service_id': self.service_id, + 'service_id': str(self.service_id), - 'template_id': self.template_id, + 'template_id': str(self.template_id), 'template_version': self.template_version, 'template_name': self.template.name, @@ -2245,7 +2245,7 @@ class BroadcastMessage(db.Model): 'cancelled_at': self.cancelled_at.strftime(DATETIME_FORMAT) if self.cancelled_at else None, 'updated_at': self.updated_at.strftime(DATETIME_FORMAT) if self.updated_at else None, - 'created_by_id': self.created_by_id, - 'approved_by_id': self.approved_by_id, - 'cancelled_by_id': self.cancelled_by_id, + 'created_by_id': str(self.created_by_id), + 'approved_by_id': str(self.approved_by_id), + 'cancelled_by_id': str(self.cancelled_by_id), } diff --git a/tests/app/celery/test_broadcast_message_tasks.py b/tests/app/celery/test_broadcast_message_tasks.py new file mode 100644 index 000000000..1622c2a7e --- /dev/null +++ b/tests/app/celery/test_broadcast_message_tasks.py @@ -0,0 +1,61 @@ +import pytest +import requests_mock +from requests import RequestException + +from app.dao.templates_dao import dao_update_template +from app.models import BROADCAST_TYPE, BroadcastStatusType +from app.celery.broadcast_message_tasks import send_broadcast_message +from tests.app.db import create_template, create_broadcast_message + + +def test_send_broadcast_message_sends_data_correctly(sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t, areas=['london'], status=BroadcastStatusType.BROADCASTING) + + with requests_mock.Mocker() as request_mock: + request_mock.post("http://test-cbc-proxy/broadcasts/stub-1", json={'valid': 'true'}, status_code=200) + send_broadcast_message(broadcast_message_id=str(bm.id)) + + assert request_mock.call_count == 1 + assert request_mock.request_history[0].method == 'POST' + assert request_mock.request_history[0].headers["Content-type"] == "application/json" + + cbc_json = request_mock.request_history[0].json() + assert cbc_json['template']['id'] == str(t.id) + assert cbc_json['template']['template_type'] == BROADCAST_TYPE + assert cbc_json['broadcast_message']['areas'] == ['london'] + + +def test_send_broadcast_message_sends_old_version_of_template(sample_service): + t = create_template(sample_service, BROADCAST_TYPE, content='first content') + bm = create_broadcast_message(t, areas=['london'], status=BroadcastStatusType.BROADCASTING) + + t.content = 'second content' + dao_update_template(t) + assert t.version == 2 + + with requests_mock.Mocker() as request_mock: + request_mock.post("http://test-cbc-proxy/broadcasts/stub-1", json={'valid': 'true'}, status_code=200) + send_broadcast_message(broadcast_message_id=str(bm.id)) + + assert request_mock.call_count == 1 + assert request_mock.request_history[0].method == 'POST' + assert request_mock.request_history[0].headers["Content-type"] == "application/json" + + cbc_json = request_mock.request_history[0].json() + assert cbc_json['template']['id'] == str(t.id) + assert cbc_json['template']['version'] == 1 + assert cbc_json['template']['content'] == 'first content' + + +def test_send_broadcast_message_errors(sample_service): + t = create_template(sample_service, BROADCAST_TYPE) + bm = create_broadcast_message(t, status=BroadcastStatusType.BROADCASTING) + + with requests_mock.Mocker() as request_mock: + request_mock.post("http://test-cbc-proxy/broadcasts/stub-1", text='503 bad gateway', status_code=503) + # we're not retrying or anything for the moment - but this'll ensure any exception gets logged + with pytest.raises(RequestException) as ex: + send_broadcast_message(broadcast_message_id=str(bm.id)) + + assert ex.value.response.status_code == 503 From 403885722e15887b809e56516e7b7638febd94ec Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Jul 2020 18:22:53 +0100 Subject: [PATCH 12/15] trigger send_broadcast_message task when user approves lets leave the cancellation can of worms alone for now --- app/broadcast_message/rest.py | 16 +++++++++++++--- tests/app/broadcast_message/test_rest.py | 8 +++++++- 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/app/broadcast_message/rest.py b/app/broadcast_message/rest.py index 559574b4d..e75a19079 100644 --- a/app/broadcast_message/rest.py +++ b/app/broadcast_message/rest.py @@ -1,8 +1,9 @@ from datetime import datetime import iso8601 -from flask import Blueprint, jsonify, request +from flask import Blueprint, jsonify, request, current_app +from app.config import QueueNames from app.dao.templates_dao import dao_get_template_by_id_and_service_id from app.dao.users_dao import get_user_by_id from app.dao.broadcast_message_dao import ( @@ -14,6 +15,7 @@ from app.dao.broadcast_message_dao import ( from app.dao.services_dao import dao_fetch_service_by_id from app.errors import register_errors from app.models import BroadcastMessage, BroadcastStatusType +from app.celery.broadcast_message_tasks import send_broadcast_message from app.broadcast_message.broadcast_message_schema import ( create_broadcast_message_schema, update_broadcast_message_schema, @@ -107,8 +109,7 @@ def update_broadcast_message_status(service_id, broadcast_message_id): new_status = data['status'] # TODO: Restrict status transitions - # TODO: Do we need to validate that the user belongs to the same service, isn't the creator, has permissions, etc? - # or is that admin's job + # TODO: validate that the user belongs to the same service, isn't the creator, has permissions, etc if new_status == BroadcastStatusType.BROADCASTING: broadcast_message.approved_at = datetime.utcnow() broadcast_message.approved_by = get_user_by_id(data['created_by']) @@ -118,6 +119,15 @@ def update_broadcast_message_status(service_id, broadcast_message_id): broadcast_message.status = new_status + current_app.logger.info( + f'broadcast_message {broadcast_message_id} moving from {broadcast_message.status} to {new_status}' + ) dao_update_broadcast_message(broadcast_message) + if new_status == BroadcastStatusType.BROADCASTING: + send_broadcast_message.apply_async( + kwargs={'broadcast_message_id': str(broadcast_message.id)}, + queue=QueueNames.NOTIFY + ) + return jsonify(broadcast_message.serialize()), 200 diff --git a/tests/app/broadcast_message/test_rest.py b/tests/app/broadcast_message/test_rest.py index a1aa0f9fe..39627e9b1 100644 --- a/tests/app/broadcast_message/test_rest.py +++ b/tests/app/broadcast_message/test_rest.py @@ -258,10 +258,15 @@ def test_update_broadcast_message_status_stores_cancelled_by_and_cancelled_at(ad assert response['cancelled_by_id'] == str(canceller.id) -def test_update_broadcast_message_status_stores_approved_by_and_approved_at(admin_request, sample_service): +def test_update_broadcast_message_status_stores_approved_by_and_approved_at_and_queues_task( + admin_request, + sample_service, + mocker +): t = create_template(sample_service, BROADCAST_TYPE) bm = create_broadcast_message(t, status=BroadcastStatusType.PENDING_APPROVAL) approver = create_user('approver@gov.uk') + mock_task = mocker.patch('app.celery.broadcast_message_tasks.send_broadcast_message.apply_async') response = admin_request.post( 'broadcast_message.update_broadcast_message_status', @@ -274,3 +279,4 @@ def test_update_broadcast_message_status_stores_approved_by_and_approved_at(admi assert response['status'] == BroadcastStatusType.BROADCASTING assert response['approved_at'] is not None assert response['approved_by_id'] == str(approver.id) + mock_task.assert_called_once_with(kwargs={'broadcast_message_id': str(bm.id)}, queue='notify-internal-tasks') From 8b4a954f2b44b7c742fa9b5c7b97b0579f551a92 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 9 Jul 2020 20:10:34 +0100 Subject: [PATCH 13/15] move schema import in to function level solves `AttributeError: 'DummySession' object has no attribute 'query'` if you don't do this you get really hard to diagnose errors in unrelated tests, due to strange import order problems or something --- app/celery/broadcast_message_tasks.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/celery/broadcast_message_tasks.py b/app/celery/broadcast_message_tasks.py index 4451aee7e..837bf352b 100644 --- a/app/celery/broadcast_message_tasks.py +++ b/app/celery/broadcast_message_tasks.py @@ -3,8 +3,6 @@ from flask import current_app from notifications_utils.statsd_decorators import statsd from app import notify_celery -from app.schemas import template_schema - from app.dao.broadcast_message_dao import dao_get_broadcast_message_by_id @@ -12,6 +10,10 @@ from app.dao.broadcast_message_dao import dao_get_broadcast_message_by_id @notify_celery.task(name="send-broadcast-message") @statsd(namespace="tasks") def send_broadcast_message(broadcast_message_id, provider='stub-1'): + # imports of schemas from tasks have to happen within functions to prevent + # `AttributeError: 'DummySession' object has no attribute 'query'` errors in unrelated tests + from app.schemas import template_schema + broadcast_message = dao_get_broadcast_message_by_id(broadcast_message_id) current_app.logger.info( From 34538bcad8ca66f9b5419d43fb9737c0f199a528 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 8 Jul 2020 16:51:41 +0100 Subject: [PATCH 14/15] Fix risk of uncaught exceptions due to gds metrics Similar to https://github.com/alphagov/notifications-admin/pull/3510 Because of https://github.com/alphagov/gds_metrics_python/pull/8 --- app/__init__.py | 4 +++- requirements-app.txt | 2 +- requirements.txt | 8 ++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index d16e0cdbe..79c57e9ef 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -83,6 +83,9 @@ def create_app(application): application.config['NOTIFY_APP_NAME'] = application.name init_app(application) + + # Metrics intentionally high up to give the most accurate timing and reliability that the metric is recorded + metrics.init_app(application) request_helper.init_app(application) db.init_app(application) migrate.init_app(application, db=db) @@ -108,7 +111,6 @@ def create_app(application): redis_store.init_app(application) performance_platform_client.init_app(application) document_download_client.init_app(application) - metrics.init_app(application) register_blueprint(application) register_v2_blueprints(application) diff --git a/requirements-app.txt b/requirements-app.txt index ed4803a3a..92703f439 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -33,4 +33,4 @@ git+https://github.com/alphagov/notifications-utils.git@40.2.1#egg=notifications # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.7.1 -gds-metrics==0.2.0 +gds-metrics==0.2.2 diff --git a/requirements.txt b/requirements.txt index c96dd1dfe..2421420ac 100644 --- a/requirements.txt +++ b/requirements.txt @@ -35,21 +35,21 @@ git+https://github.com/alphagov/notifications-utils.git@40.2.1#egg=notifications # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.7.1 -gds-metrics==0.2.0 +gds-metrics==0.2.2 ## The following requirements were added by pip freeze: alembic==1.4.2 amqp==1.4.9 anyjson==0.3.3 attrs==19.3.0 -awscli==1.18.96 +awscli==1.18.97 bcrypt==3.1.7 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.17.19 +botocore==1.17.20 certifi==2020.6.20 chardet==3.0.4 click==7.1.2 @@ -82,7 +82,7 @@ pytz==2020.1 PyYAML==5.3.1 redis==3.5.3 requests==2.24.0 -rsa==3.4.2 +rsa==4.5 s3transfer==0.3.3 six==1.15.0 smartypants==2.0.1 From ef551247b59c169d80c752f6f4c262b85bff654e Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 10 Jul 2020 11:27:42 +0100 Subject: [PATCH 15/15] Add better logging to starting collate-letter-pdfs-to-be-sent This will help us better understand how far through the task has got if it gets interrupted halfway (as was the case this morning and we struggled to understand). --- app/celery/letters_pdf_tasks.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 75d13e18c..61982e0b4 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -129,6 +129,7 @@ def collate_letter_pdfs_to_be_sent(): that have not yet been sent. If run after midnight, it will collect up letters created before 5:30pm the day before. """ + current_app.logger.info("starting collate-letter-pdfs-to-be-sent") print_run_date = convert_utc_to_bst(datetime.utcnow()) if print_run_date.time() < LETTER_PROCESSING_DEADLINE: print_run_date = print_run_date - timedelta(days=1) @@ -137,6 +138,7 @@ def collate_letter_pdfs_to_be_sent(): hour=17, minute=30, second=0, microsecond=0 ) for postage in POSTAGE_TYPES: + current_app.logger.info(f"starting collate-letter-pdfs-to-be-sent processing for postage class {postage}") letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage) for i, letters in enumerate(group_letters(letters_to_print)): @@ -167,6 +169,9 @@ def collate_letter_pdfs_to_be_sent(): queue=QueueNames.PROCESS_FTP, compression='zlib' ) + current_app.logger.info(f"finished collate-letter-pdfs-to-be-sent processing for postage class {postage}") + + current_app.logger.info("finished collate-letter-pdfs-to-be-sent") def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline, postage):