From 17fec1c99e0a8e441d1887f2d4c53fa2cb0d7b37 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 28 May 2024 11:27:57 -0700 Subject: [PATCH 1/7] use moto to mock s3 --- notifications_utils/s3.py | 11 ++++ poetry.lock | 78 +++++++++++++++++++++++++++- pyproject.toml | 1 + tests/app/main/views/test_send.py | 14 +++++ tests/notifications_utils/test_s3.py | 2 + 5 files changed, 105 insertions(+), 1 deletion(-) diff --git a/notifications_utils/s3.py b/notifications_utils/s3.py index cdcc70a5c..a2886b381 100644 --- a/notifications_utils/s3.py +++ b/notifications_utils/s3.py @@ -38,6 +38,17 @@ def s3upload( region_name=region, ) _s3 = session.resource("s3", config=AWS_CLIENT_CONFIG) + # This 'proves' that use of moto in the relevant tests in test_send.py + # mocks everything related to S3. What you will see in the logs is: + # Exception: CREATED AT + # + # raise Exception(f"CREATED AT {_s3.Bucket(bucket_name).creation_date}") + if os.getenv("NOTIFY_ENVIRONMENT") == "test": + teststr = str(_s3.Bucket(bucket_name).creation_date).lower() + if "magicmock" not in teststr: + raise Exception( + f"xxxxxtest not mocked, use @mock_aws creation date is {teststr}" + ) key = _s3.Object(bucket_name, file_location) diff --git a/poetry.lock b/poetry.lock index 80a341491..5a32fc4d4 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1297,6 +1297,7 @@ files = [ {file = "lxml-5.2.1-cp36-cp36m-manylinux_2_12_i686.manylinux2010_i686.manylinux_2_17_i686.manylinux2014_i686.whl", hash = "sha256:c38d7b9a690b090de999835f0443d8aa93ce5f2064035dfc48f27f02b4afc3d0"}, {file = "lxml-5.2.1-cp36-cp36m-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:5670fb70a828663cc37552a2a85bf2ac38475572b0e9b91283dc09efb52c41d1"}, {file = "lxml-5.2.1-cp36-cp36m-manylinux_2_28_x86_64.whl", hash = "sha256:958244ad566c3ffc385f47dddde4145088a0ab893504b54b52c041987a8c1863"}, + {file = "lxml-5.2.1-cp36-cp36m-manylinux_2_5_x86_64.manylinux1_x86_64.whl", hash = "sha256:b6241d4eee5f89453307c2f2bfa03b50362052ca0af1efecf9fef9a41a22bb4f"}, {file = "lxml-5.2.1-cp36-cp36m-musllinux_1_1_aarch64.whl", hash = "sha256:2a66bf12fbd4666dd023b6f51223aed3d9f3b40fef06ce404cb75bafd3d89536"}, {file = "lxml-5.2.1-cp36-cp36m-musllinux_1_1_ppc64le.whl", hash = "sha256:9123716666e25b7b71c4e1789ec829ed18663152008b58544d95b008ed9e21e9"}, {file = "lxml-5.2.1-cp36-cp36m-musllinux_1_1_s390x.whl", hash = "sha256:0c3f67e2aeda739d1cc0b1102c9a9129f7dc83901226cc24dd72ba275ced4218"}, @@ -1551,6 +1552,50 @@ files = [ {file = "mistune-0.8.4.tar.gz", hash = "sha256:59a3429db53c50b5c6bcc8a07f8848cb00d7dc8bdb431a4ab41920d201d4756e"}, ] +[[package]] +name = "moto" +version = "5.0.8" +description = "" +optional = false +python-versions = ">=3.8" +files = [ + {file = "moto-5.0.8-py2.py3-none-any.whl", hash = "sha256:7d1035e366434bfa9fcc0621f07d5aa724b6846408071d540137a0554c46f214"}, + {file = "moto-5.0.8.tar.gz", hash = "sha256:517fb808dc718bcbdda54c6ffeaca0adc34cf6e10821bfb01216ce420a31765c"}, +] + +[package.dependencies] +boto3 = ">=1.9.201" +botocore = ">=1.14.0" +cryptography = ">=3.3.1" +Jinja2 = ">=2.10.1" +python-dateutil = ">=2.1,<3.0.0" +requests = ">=2.5" +responses = ">=0.15.0" +werkzeug = ">=0.5,<2.2.0 || >2.2.0,<2.2.1 || >2.2.1" +xmltodict = "*" + +[package.extras] +all = ["PyYAML (>=5.1)", "antlr4-python3-runtime", "aws-xray-sdk (>=0.93,!=0.96)", "cfn-lint (>=0.40.0)", "docker (>=3.0.0)", "graphql-core", "joserfc (>=0.9.0)", "jsondiff (>=1.1.2)", "jsonpath-ng", "multipart", "openapi-spec-validator (>=0.5.0)", "py-partiql-parser (==0.5.5)", "pyparsing (>=3.0.7)", "setuptools"] +apigateway = ["PyYAML (>=5.1)", "joserfc (>=0.9.0)", "openapi-spec-validator (>=0.5.0)"] +apigatewayv2 = ["PyYAML (>=5.1)", "openapi-spec-validator (>=0.5.0)"] +appsync = ["graphql-core"] +awslambda = ["docker (>=3.0.0)"] +batch = ["docker (>=3.0.0)"] +cloudformation = ["PyYAML (>=5.1)", "aws-xray-sdk (>=0.93,!=0.96)", "cfn-lint (>=0.40.0)", "docker (>=3.0.0)", "graphql-core", "joserfc (>=0.9.0)", "jsondiff (>=1.1.2)", "openapi-spec-validator (>=0.5.0)", "py-partiql-parser (==0.5.5)", "pyparsing (>=3.0.7)", "setuptools"] +cognitoidp = ["joserfc (>=0.9.0)"] +dynamodb = ["docker (>=3.0.0)", "py-partiql-parser (==0.5.5)"] +dynamodbstreams = ["docker (>=3.0.0)", "py-partiql-parser (==0.5.5)"] +glue = ["pyparsing (>=3.0.7)"] +iotdata = ["jsondiff (>=1.1.2)"] +proxy = ["PyYAML (>=5.1)", "antlr4-python3-runtime", "aws-xray-sdk (>=0.93,!=0.96)", "cfn-lint (>=0.40.0)", "docker (>=2.5.1)", "graphql-core", "joserfc (>=0.9.0)", "jsondiff (>=1.1.2)", "jsonpath-ng", "multipart", "openapi-spec-validator (>=0.5.0)", "py-partiql-parser (==0.5.5)", "pyparsing (>=3.0.7)", "setuptools"] +resourcegroupstaggingapi = ["PyYAML (>=5.1)", "cfn-lint (>=0.40.0)", "docker (>=3.0.0)", "graphql-core", "joserfc (>=0.9.0)", "jsondiff (>=1.1.2)", "openapi-spec-validator (>=0.5.0)", "py-partiql-parser (==0.5.5)", "pyparsing (>=3.0.7)"] +s3 = ["PyYAML (>=5.1)", "py-partiql-parser (==0.5.5)"] +s3crc32c = ["PyYAML (>=5.1)", "crc32c", "py-partiql-parser (==0.5.5)"] +server = ["PyYAML (>=5.1)", "antlr4-python3-runtime", "aws-xray-sdk (>=0.93,!=0.96)", "cfn-lint (>=0.40.0)", "docker (>=3.0.0)", "flask (!=2.2.0,!=2.2.1)", "flask-cors", "graphql-core", "joserfc (>=0.9.0)", "jsondiff (>=1.1.2)", "jsonpath-ng", "openapi-spec-validator (>=0.5.0)", "py-partiql-parser (==0.5.5)", "pyparsing (>=3.0.7)", "setuptools"] +ssm = ["PyYAML (>=5.1)"] +stepfunctions = ["antlr4-python3-runtime", "jsonpath-ng"] +xray = ["aws-xray-sdk (>=0.93,!=0.96)", "setuptools"] + [[package]] name = "msgpack" version = "1.0.8" @@ -1613,6 +1658,7 @@ files = [ {file = "msgpack-1.0.8-cp39-cp39-musllinux_1_1_x86_64.whl", hash = "sha256:5fbb160554e319f7b22ecf530a80a3ff496d38e8e07ae763b9e82fadfe96f273"}, {file = "msgpack-1.0.8-cp39-cp39-win32.whl", hash = "sha256:f9af38a89b6a5c04b7d18c492c8ccf2aee7048aff1ce8437c4683bb5a1df893d"}, {file = "msgpack-1.0.8-cp39-cp39-win_amd64.whl", hash = "sha256:ed59dd52075f8fc91da6053b12e8c89e37aa043f8986efd89e61fae69dc1b011"}, + {file = "msgpack-1.0.8-py3-none-any.whl", hash = "sha256:24f727df1e20b9876fa6e95f840a2a2651e34c0ad147676356f4bf5fbb0206ca"}, {file = "msgpack-1.0.8.tar.gz", hash = "sha256:95c02b0e27e706e48d0e5426d1710ca78e0f0628d6e89d5b5a5b91a5f12274f3"}, ] @@ -2617,6 +2663,25 @@ requests = ">=2.22,<3" [package.extras] fixture = ["fixtures"] +[[package]] +name = "responses" +version = "0.25.0" +description = "A utility library for mocking out the `requests` Python library." +optional = false +python-versions = ">=3.8" +files = [ + {file = "responses-0.25.0-py3-none-any.whl", hash = "sha256:2f0b9c2b6437db4b528619a77e5d565e4ec2a9532162ac1a131a83529db7be1a"}, + {file = "responses-0.25.0.tar.gz", hash = "sha256:01ae6a02b4f34e39bffceb0fc6786b67a25eae919c6368d05eabc8d9576c2a66"}, +] + +[package.dependencies] +pyyaml = "*" +requests = ">=2.30.0,<3.0" +urllib3 = ">=1.25.10,<3.0" + +[package.extras] +tests = ["coverage (>=6.0.0)", "flake8", "mypy", "pytest (>=7.0.0)", "pytest-asyncio", "pytest-cov", "pytest-httpserver", "tomli", "tomli-w", "types-PyYAML", "types-requests"] + [[package]] name = "rich" version = "13.7.1" @@ -2960,7 +3025,18 @@ files = [ {file = "xlwt-1.3.0.tar.gz", hash = "sha256:c59912717a9b28f1a3c2a98fd60741014b06b043936dcecbc113eaaada156c88"}, ] +[[package]] +name = "xmltodict" +version = "0.13.0" +description = "Makes working with XML feel like you are working with JSON" +optional = false +python-versions = ">=3.4" +files = [ + {file = "xmltodict-0.13.0-py2.py3-none-any.whl", hash = "sha256:aa89e8fd76320154a40d19a0df04a4695fb9dc5ba977cbb68ab3e4eb225e7852"}, + {file = "xmltodict-0.13.0.tar.gz", hash = "sha256:341595a488e3e01a85a9d8911d8912fd922ede5fecc4dce437eb4b6c8d037e56"}, +] + [metadata] lock-version = "2.0" python-versions = "^3.12.2" -content-hash = "6c271d919c3736a844fa3674c1db0891e4c09378e6656b396ff60c594e34a862" +content-hash = "8f58d29f819ca160e10740e9774b5e528675208f0e8f2a51fa88ea0a62ea1dc8" diff --git a/pyproject.toml b/pyproject.toml index 2fc0b3c14..c0da02466 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -82,6 +82,7 @@ flake8-print = "^5.0.0" flake8-pytest-style = "^1.7.2" isort = "^5.13.2" jinja2-cli = {version = "==0.8.2", extras = ["yaml"]} +moto="*" pip-audit = "*" pre-commit = "^3.7.1" pytest = "^8.2.1" diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 449259d8d..efaee4043 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -11,6 +11,7 @@ from zipfile import BadZipFile import pytest from flask import url_for +from moto import mock_aws from notifications_python_client.errors import HTTPError from xlrd.biffh import XLRDError from xlrd.xldate import XLDateAmbiguous, XLDateError, XLDateNegative, XLDateTooLarge @@ -426,6 +427,7 @@ def test_example_spreadsheet( list(zip(test_spreadsheet_files, repeat(True), repeat(302))) + list(zip(test_non_spreadsheet_files, repeat(False), repeat(200))), ) +@mock_aws def test_upload_files_in_different_formats( filename, acceptable_file, @@ -465,6 +467,7 @@ def test_upload_files_in_different_formats( ) +@mock_aws def test_send_messages_sanitises_and_truncates_file_name_for_metadata( client_request, service_one, @@ -572,6 +575,7 @@ def test_shows_error_if_parsing_exception( ) +@mock_aws def test_upload_csv_file_with_errors_shows_check_page_with_errors( client_request, service_one, @@ -619,6 +623,7 @@ def test_upload_csv_file_with_errors_shows_check_page_with_errors( assert "Upload your file again" in page.text +@mock_aws def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( client_request, service_one, @@ -671,6 +676,7 @@ def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( assert page.select("tbody tr td")[1]["colspan"] == "2" +@mock_aws def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors( client_request, service_one, @@ -807,6 +813,7 @@ def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors ), ], ) +@mock_aws def test_upload_csv_file_with_missing_columns_shows_error( client_request, mocker, @@ -882,6 +889,7 @@ def test_upload_csv_size_too_big( assert "File must be smaller than 10Mb" in page.text +@mock_aws def test_upload_valid_csv_redirects_to_check_page( client_request, mock_get_service_template_with_placeholders, @@ -928,6 +936,7 @@ def test_upload_valid_csv_redirects_to_check_page( ), ], ) +@mock_aws def test_upload_valid_csv_shows_preview_and_table( client_request, mocker, @@ -1021,6 +1030,7 @@ def test_upload_valid_csv_shows_preview_and_table( assert normalize_spaces(str(row.select("td")[index])) == cell +@mock_aws def test_show_all_columns_if_there_are_duplicate_recipient_columns( client_request, mocker, @@ -1071,6 +1081,7 @@ def test_show_all_columns_if_there_are_duplicate_recipient_columns( (5, 404), ], ) +@mock_aws def test_404_for_previewing_a_row_out_of_range( client_request, mocker, @@ -1519,6 +1530,7 @@ def test_send_one_off_redirects_to_end_if_step_out_of_bounds( create_active_caseworking_user(), ], ) +@mock_aws def test_send_one_off_redirects_to_start_if_you_skip_steps( client_request, service_one, @@ -1623,6 +1635,7 @@ def test_send_one_off_sms_message_redirects( create_active_caseworking_user(), ], ) +@mock_aws def test_send_one_off_email_to_self_without_placeholders_redirects_to_check_page( client_request, mocker, @@ -1828,6 +1841,7 @@ def test_download_example_csv( assert "text/csv" in response.headers["Content-Type"] +@mock_aws def test_upload_csvfile_with_valid_phone_shows_all_numbers( client_request, mock_get_service_template, diff --git a/tests/notifications_utils/test_s3.py b/tests/notifications_utils/test_s3.py index 46b863c4f..d05fa8fdc 100644 --- a/tests/notifications_utils/test_s3.py +++ b/tests/notifications_utils/test_s3.py @@ -2,6 +2,7 @@ from urllib.parse import parse_qs import botocore import pytest +from moto import mock_aws from notifications_utils.s3 import S3ObjectNotFound, s3download, s3upload @@ -12,6 +13,7 @@ location = "some_file_location" content_type = "binary/octet-stream" +@mock_aws def test_s3upload_save_file_to_bucket(mocker): mocked = mocker.patch("notifications_utils.s3.Session.resource") s3upload( From 9368f6a49622c4780d0ed1ddc8c0c9739128d9ad Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 29 May 2024 14:18:08 -0700 Subject: [PATCH 2/7] remove some fixtures --- .ds.baseline | 4 +- app/s3_client/s3_csv_client.py | 1 + notifications_utils/s3.py | 11 ++ tests/app/main/views/test_send.py | 217 ++++++++++++++++++++++-------- tests/conftest.py | 23 ---- 5 files changed, 174 insertions(+), 82 deletions(-) diff --git a/.ds.baseline b/.ds.baseline index cec28396c..859f30b4d 100644 --- a/.ds.baseline +++ b/.ds.baseline @@ -675,7 +675,7 @@ "filename": "tests/conftest.py", "hashed_secret": "f8377c90fcfd699f0ddbdcb30c2c9183d2d933ea", "is_verified": false, - "line_number": 3289, + "line_number": 3266, "is_secret": false } ], @@ -710,5 +710,5 @@ } ] }, - "generated_at": "2024-05-20T16:03:05Z" + "generated_at": "2024-05-29T21:18:03Z" } diff --git a/app/s3_client/s3_csv_client.py b/app/s3_client/s3_csv_client.py index 21c329887..752f054a4 100644 --- a/app/s3_client/s3_csv_client.py +++ b/app/s3_client/s3_csv_client.py @@ -28,6 +28,7 @@ def get_csv_upload(service_id, upload_id): def s3upload(service_id, filedata): + upload_id = str(uuid.uuid4()) bucket_name, file_location, access_key, secret_key, region = get_csv_location( service_id, upload_id diff --git a/notifications_utils/s3.py b/notifications_utils/s3.py index a2886b381..b0a86844a 100644 --- a/notifications_utils/s3.py +++ b/notifications_utils/s3.py @@ -93,6 +93,17 @@ def s3download( ) s3 = session.resource("s3", config=AWS_CLIENT_CONFIG) key = s3.Object(bucket_name, filename) + # This 'proves' that use of moto in the relevant tests in test_send.py + # mocks everything related to S3. What you will see in the logs is: + # Exception: CREATED AT + # + # raise Exception(f"CREATED AT {_s3.Bucket(bucket_name).creation_date}") + if os.getenv("NOTIFY_ENVIRONMENT") == "test": + teststr = str(s3.Bucket(bucket_name).creation_date).lower() + if "magicmock" not in teststr: + raise Exception( + f"xxxxxtest not mocked, use @mock_aws creation date is {teststr}" + ) return key.get()["Body"] except botocore.exceptions.ClientError as error: raise S3ObjectNotFound(error.response, error.operation_name) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index efaee4043..46240de5a 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -6,6 +6,7 @@ from io import BytesIO from itertools import repeat from os import path from random import randbytes +from unittest.mock import ANY from uuid import uuid4 from zipfile import BadZipFile @@ -18,7 +19,11 @@ from xlrd.xldate import XLDateAmbiguous, XLDateError, XLDateNegative, XLDateTooL from notifications_utils.recipients import RecipientCSV from notifications_utils.template import SMSPreviewTemplate -from tests import validate_route_permission, validate_route_permission_with_client +from tests import ( + sample_uuid, + validate_route_permission, + validate_route_permission_with_client, +) from tests.conftest import ( SERVICE_ONE_ID, create_active_caseworking_user, @@ -436,10 +441,15 @@ def test_upload_files_in_different_formats( service_one, mocker, mock_get_service_template, - mock_s3_set_metadata, - mock_s3_upload, fake_uuid, ): + + mock_s3_set_metadata = mocker.patch( + "app.main.views.send.set_metadata_on_csv_upload" + ) + + mock_s3_upload = mocker.patch("app.main.views.send.s3upload") + with open(filename, "rb") as uploaded: page = client_request.post( "main.send_messages", @@ -458,7 +468,7 @@ def test_upload_files_in_different_formats( "202 205 8823,Still Not Pete,Crimson,Pear" ) mock_s3_set_metadata.assert_called_once_with( - SERVICE_ONE_ID, fake_uuid, original_file_name=filename + SERVICE_ONE_ID, ANY, original_file_name=filename ) else: assert not mock_s3_upload.called @@ -473,13 +483,17 @@ def test_send_messages_sanitises_and_truncates_file_name_for_metadata( service_one, mocker, mock_get_service_template_with_placeholders, - mock_s3_set_metadata, - mock_s3_get_metadata, - mock_s3_upload, mock_s3_download, mock_get_job_doesnt_exist, fake_uuid, ): + + mock_s3_set_metadata = mocker.patch( + "app.main.views.send.set_metadata_on_csv_upload" + ) + + mocker.patch("app.main.views.send.s3upload") + filename = f"😁{'a' * 2000}.csv" client_request.post( @@ -581,15 +595,20 @@ def test_upload_csv_file_with_errors_shows_check_page_with_errors( service_one, mocker, mock_get_service_template_with_placeholders, - mock_s3_set_metadata, - mock_s3_get_metadata, - mock_s3_upload, mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, fake_uuid, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) mocker.patch( "app.main.views.send.s3download", return_value=""" @@ -629,15 +648,21 @@ def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( service_one, mocker, mock_get_empty_service_template_with_optional_placeholder, - mock_s3_set_metadata, - mock_s3_get_metadata, - mock_s3_upload, mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, fake_uuid, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) + mocker.patch( "app.main.views.send.s3download", return_value=""" @@ -682,15 +707,20 @@ def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors service_one, mocker, mock_get_service_template_with_placeholders, - mock_s3_set_metadata, - mock_s3_get_metadata, - mock_s3_upload, mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, fake_uuid, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) big_placeholder = " ".join(["not ok"] * 402) mocker.patch( "app.main.views.send.s3download", @@ -818,9 +848,6 @@ def test_upload_csv_file_with_missing_columns_shows_error( client_request, mocker, mock_get_service_template_with_placeholders, - mock_s3_set_metadata, - mock_s3_get_metadata, - mock_s3_upload, mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, @@ -830,6 +857,15 @@ def test_upload_csv_file_with_missing_columns_shows_error( file_contents, expected_error, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) + mocker.patch("app.main.views.send.s3download", return_value=file_contents) page = client_request.post( @@ -893,10 +929,13 @@ def test_upload_csv_size_too_big( def test_upload_valid_csv_redirects_to_check_page( client_request, mock_get_service_template_with_placeholders, - mock_s3_upload, - mock_s3_set_metadata, fake_uuid, + mocker, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) client_request.post( "main.send_messages", service_id=SERVICE_ONE_ID, @@ -936,7 +975,6 @@ def test_upload_valid_csv_redirects_to_check_page( ), ], ) -@mock_aws def test_upload_valid_csv_shows_preview_and_table( client_request, mocker, @@ -946,13 +984,16 @@ def test_upload_valid_csv_shows_preview_and_table( mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, - mock_s3_get_metadata, - mock_s3_set_metadata, fake_uuid, extra_args, expected_link_in_first_row, expected_message, ): + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) with client_request.session_transaction() as session: session["file_uploads"] = {fake_uuid: {"template_id": fake_uuid}} @@ -1041,8 +1082,12 @@ def test_show_all_columns_if_there_are_duplicate_recipient_columns( mock_get_job_doesnt_exist, mock_get_jobs, fake_uuid, - mock_s3_get_metadata, ): + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) with client_request.session_transaction() as session: session["file_uploads"] = {fake_uuid: {"template_id": fake_uuid}} @@ -1091,12 +1136,17 @@ def test_404_for_previewing_a_row_out_of_range( mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, - mock_s3_get_metadata, - mock_s3_set_metadata, fake_uuid, row_index, expected_status, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) with client_request.session_transaction() as session: session["file_uploads"] = {fake_uuid: {"template_id": fake_uuid}} @@ -1535,7 +1585,6 @@ def test_send_one_off_redirects_to_start_if_you_skip_steps( client_request, service_one, fake_uuid, - mock_s3_upload, mock_get_users_by_service, mock_get_service_statistics, mock_has_no_jobs, @@ -1641,7 +1690,6 @@ def test_send_one_off_email_to_self_without_placeholders_redirects_to_check_page mocker, service_one, mock_get_service_email_template_without_placeholders, - mock_s3_upload, mock_get_users_by_service, mock_get_service_statistics, mock_has_no_jobs, @@ -1849,13 +1897,20 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_get_live_service, mock_get_job_doesnt_exist, mock_get_jobs, - mock_s3_get_metadata, - mock_s3_set_metadata, service_one, fake_uuid, - mock_s3_upload, mocker, ): + + mock_s3_set_metadata = mocker.patch( + "app.main.views.send.set_metadata_on_csv_upload" + ) + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) mocker.patch( "app.main.views.send.s3download", return_value="\n".join( @@ -1909,9 +1964,6 @@ def test_upload_csvfile_with_international_validates( api_user_active, client_request, mock_get_service_template, - mock_s3_set_metadata, - mock_s3_get_metadata, - mock_s3_upload, mock_has_permissions, mock_get_users_by_service, mock_get_service_statistics, @@ -1922,6 +1974,14 @@ def test_upload_csvfile_with_international_validates( should_allow_international, service_one, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) if international_sms_permission: service_one["permissions"] += ("sms", "international_sms") mocker.patch( @@ -1961,10 +2021,15 @@ def test_test_message_can_only_be_sent_now( mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, - mock_s3_get_metadata, - mock_s3_set_metadata, fake_uuid, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) content = client_request.get( "main.check_messages", service_id=service_one["id"], @@ -1986,8 +2051,12 @@ def test_preview_button_is_correctly_labelled( mock_get_job_doesnt_exist, mock_get_jobs, fake_uuid, - mock_s3_get_metadata, ): + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) mocker.patch( "app.main.views.send.s3download", return_value="\n".join(["phone_number"] + (["2028670123"] * 1000)), @@ -2072,7 +2141,6 @@ def test_route_permissions( mock_get_jobs, mock_get_notifications, mock_create_job, - mock_s3_upload, fake_uuid, route, response_code, @@ -2106,8 +2174,8 @@ def test_route_permissions_send_check_notifications( response_code, method, mock_create_job, - mock_s3_upload, ): + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) with client_request.session_transaction() as session: session["recipient"] = "2028675301" session["placeholders"] = {"name": "a"} @@ -2187,8 +2255,6 @@ def test_check_messages_back_link( mock_get_job_doesnt_exist, mock_get_jobs, mock_s3_download, - mock_s3_get_metadata, - mock_s3_set_metadata, fake_uuid, mocker, template_type, @@ -2196,6 +2262,13 @@ def test_check_messages_back_link( extra_args, expected_url, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) content = "Hi there ((name))" if has_placeholders else "Hi there" template_data = create_template( template_id=fake_uuid, template_type=template_type, content=content @@ -2250,8 +2323,12 @@ def test_check_messages_shows_too_many_messages_errors( fake_uuid, num_requested, expected_msg, - mock_s3_get_metadata, ): + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) # csv with 100 phone numbers mocker.patch( "app.main.views.send.s3download", @@ -2295,7 +2372,6 @@ def test_check_messages_shows_too_many_messages_errors( def test_check_messages_shows_trial_mode_error( client_request, - mock_s3_get_metadata, mock_get_users_by_service, mock_get_service_template, mock_has_permissions, @@ -2305,6 +2381,11 @@ def test_check_messages_shows_trial_mode_error( fake_uuid, mocker, ): + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) mocker.patch( "app.main.views.send.s3download", return_value=("phone number,\n2028675209"), # Not in team @@ -2440,8 +2521,12 @@ def test_check_messages_column_error_doesnt_show_optional_columns( mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, - mock_s3_get_metadata, ): + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) mocker.patch( "app.main.views.send.s3download", return_value="\n".join( @@ -2482,10 +2567,17 @@ def test_check_messages_adds_sender_id_in_session_to_metadata( mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, - mock_s3_get_metadata, - mock_s3_set_metadata, fake_uuid, ): + + mock_s3_set_metadata = mocker.patch( + "app.main.views.send.set_metadata_on_csv_upload" + ) + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) mocker.patch( "app.main.views.send.s3download", return_value=("phone number,\n2028675209") ) @@ -2522,11 +2614,15 @@ def test_check_messages_shows_over_max_row_error( mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, - mock_s3_get_metadata, mock_s3_download, fake_uuid, mocker, ): + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) mock_recipients = mocker.patch("app.main.views.send.RecipientCSV").return_value mock_recipients.max_rows = 11111 mock_recipients.__len__.return_value = 99999 @@ -2676,8 +2772,8 @@ def test_send_notification_submits_data( expected_personalisation, mocker, mock_create_job, - mock_s3_upload, ): + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) with client_request.session_transaction() as session: session["recipient"] = recipient session["placeholders"] = placeholders @@ -2704,8 +2800,8 @@ def test_send_notification_clears_session( mock_get_service_template, mocker, mock_create_job, - mock_s3_upload, ): + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) with client_request.session_transaction() as session: session["recipient"] = "2028675301" session["placeholders"] = {"a": "b"} @@ -2766,8 +2862,8 @@ def test_send_notification_redirects_to_view_page( extra_redirect_args, mocker, mock_create_job, - mock_s3_upload, ): + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) with client_request.session_transaction() as session: session["recipient"] = "2028675301" session["placeholders"] = {"a": "b"} @@ -2826,8 +2922,9 @@ def test_send_notification_shows_error_if_400( exception_msg, expected_h1, expected_err_details, - mock_s3_upload, ): + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) + class MockHTTPError(HTTPError): message = exception_msg @@ -2865,8 +2962,9 @@ def test_send_notification_shows_email_error_in_trial_mode( mocker, mock_get_service_email_template, mock_create_job, - mock_s3_upload, ): + mocker.patch("app.main.views.send.s3upload", return_value=sample_uuid()) + class MockHTTPError(HTTPError): message = TRIAL_MODE_MSG status_code = 400 @@ -2912,8 +3010,6 @@ def test_reply_to_is_previewed_if_chosen( mocker, mock_get_service_email_template, mock_s3_download, - mock_s3_get_metadata, - mock_s3_set_metadata, mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, @@ -2924,6 +3020,8 @@ def test_reply_to_is_previewed_if_chosen( extra_args, reply_to_address, ): + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + mocker.patch( "app.main.views.send.s3download", return_value=""" @@ -2967,8 +3065,6 @@ def test_sms_sender_is_previewed( mocker, mock_get_service_template, mock_s3_download, - mock_s3_get_metadata, - mock_s3_set_metadata, mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, @@ -2979,6 +3075,13 @@ def test_sms_sender_is_previewed( extra_args, sms_sender, ): + + mocker.patch("app.main.views.send.set_metadata_on_csv_upload") + + mocker.patch( + "app.main.views.send.get_csv_metadata", + return_value={"original_file_name": "example.csv"}, + ) mocker.patch( "app.main.views.send.s3download", return_value=""" diff --git a/tests/conftest.py b/tests/conftest.py index 00451bdf7..592ec9403 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1843,14 +1843,6 @@ def mock_get_users_by_service(mocker): ) -@pytest.fixture() -def mock_s3_upload(mocker): - def _upload(service_id, filedata): - return sample_uuid() - - return mocker.patch("app.main.views.send.s3upload", side_effect=_upload) - - @pytest.fixture() def mock_s3_download(mocker): def _download(service_id, upload_id): @@ -1863,21 +1855,6 @@ def mock_s3_download(mocker): return mocker.patch("app.main.views.send.s3download", side_effect=_download) -@pytest.fixture() -def mock_s3_get_metadata(mocker): - def _get_metadata(service_id, upload_id): - return {"original_file_name": "example.csv"} - - return mocker.patch( - "app.main.views.send.get_csv_metadata", side_effect=_get_metadata - ) - - -@pytest.fixture() -def mock_s3_set_metadata(mocker): - return mocker.patch("app.main.views.send.set_metadata_on_csv_upload") - - @pytest.fixture() def sample_invite(mocker, service_one): id_ = USER_ONE_ID From e694b9a5f60f6bb67199902df7cecc75f5ffa548 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Wed, 29 May 2024 14:49:06 -0700 Subject: [PATCH 3/7] clean up mock_s3_download --- tests/app/main/views/test_send.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 46240de5a..fd55ae20b 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -483,7 +483,6 @@ def test_send_messages_sanitises_and_truncates_file_name_for_metadata( service_one, mocker, mock_get_service_template_with_placeholders, - mock_s3_download, mock_get_job_doesnt_exist, fake_uuid, ): @@ -1620,7 +1619,6 @@ def test_send_one_off_redirects_to_start_if_index_out_of_bounds_and_some_placeho service_one, fake_uuid, mock_get_service_email_template, - mock_s3_download, mock_get_users_by_service, mock_get_service_statistics, mock_has_no_jobs, @@ -2016,11 +2014,11 @@ def test_test_message_can_only_be_sent_now( mocker, service_one, mock_get_service_template, - mock_s3_download, mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, mock_get_jobs, + mock_s3_download, fake_uuid, ): @@ -3009,7 +3007,6 @@ def test_reply_to_is_previewed_if_chosen( client_request, mocker, mock_get_service_email_template, - mock_s3_download, mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, @@ -3064,7 +3061,6 @@ def test_sms_sender_is_previewed( client_request, mocker, mock_get_service_template, - mock_s3_download, mock_get_users_by_service, mock_get_service_statistics, mock_get_job_doesnt_exist, From df032ba76df80a23941b19a2f978ff1acbeb87f3 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 30 May 2024 07:58:30 -0700 Subject: [PATCH 4/7] remove mock_aws where its not useful --- tests/app/main/views/test_send.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index fd55ae20b..24403e895 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -12,7 +12,6 @@ from zipfile import BadZipFile import pytest from flask import url_for -from moto import mock_aws from notifications_python_client.errors import HTTPError from xlrd.biffh import XLRDError from xlrd.xldate import XLDateAmbiguous, XLDateError, XLDateNegative, XLDateTooLarge @@ -432,7 +431,6 @@ def test_example_spreadsheet( list(zip(test_spreadsheet_files, repeat(True), repeat(302))) + list(zip(test_non_spreadsheet_files, repeat(False), repeat(200))), ) -@mock_aws def test_upload_files_in_different_formats( filename, acceptable_file, @@ -477,7 +475,6 @@ def test_upload_files_in_different_formats( ) -@mock_aws def test_send_messages_sanitises_and_truncates_file_name_for_metadata( client_request, service_one, @@ -588,7 +585,6 @@ def test_shows_error_if_parsing_exception( ) -@mock_aws def test_upload_csv_file_with_errors_shows_check_page_with_errors( client_request, service_one, @@ -641,7 +637,6 @@ def test_upload_csv_file_with_errors_shows_check_page_with_errors( assert "Upload your file again" in page.text -@mock_aws def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( client_request, service_one, @@ -700,7 +695,6 @@ def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( assert page.select("tbody tr td")[1]["colspan"] == "2" -@mock_aws def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors( client_request, service_one, @@ -842,7 +836,6 @@ def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors ), ], ) -@mock_aws def test_upload_csv_file_with_missing_columns_shows_error( client_request, mocker, @@ -924,7 +917,6 @@ def test_upload_csv_size_too_big( assert "File must be smaller than 10Mb" in page.text -@mock_aws def test_upload_valid_csv_redirects_to_check_page( client_request, mock_get_service_template_with_placeholders, @@ -1070,7 +1062,6 @@ def test_upload_valid_csv_shows_preview_and_table( assert normalize_spaces(str(row.select("td")[index])) == cell -@mock_aws def test_show_all_columns_if_there_are_duplicate_recipient_columns( client_request, mocker, @@ -1125,7 +1116,6 @@ def test_show_all_columns_if_there_are_duplicate_recipient_columns( (5, 404), ], ) -@mock_aws def test_404_for_previewing_a_row_out_of_range( client_request, mocker, @@ -1579,7 +1569,6 @@ def test_send_one_off_redirects_to_end_if_step_out_of_bounds( create_active_caseworking_user(), ], ) -@mock_aws def test_send_one_off_redirects_to_start_if_you_skip_steps( client_request, service_one, @@ -1682,7 +1671,6 @@ def test_send_one_off_sms_message_redirects( create_active_caseworking_user(), ], ) -@mock_aws def test_send_one_off_email_to_self_without_placeholders_redirects_to_check_page( client_request, mocker, @@ -1887,7 +1875,6 @@ def test_download_example_csv( assert "text/csv" in response.headers["Content-Type"] -@mock_aws def test_upload_csvfile_with_valid_phone_shows_all_numbers( client_request, mock_get_service_template, From a4fc04a65927f3841477cd87dda74725e26ee701 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 30 May 2024 08:25:00 -0700 Subject: [PATCH 5/7] test that S3 not getting hit in tests --- app/s3_client/__init__.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/s3_client/__init__.py b/app/s3_client/__init__.py index e0933b464..047ae0bb8 100644 --- a/app/s3_client/__init__.py +++ b/app/s3_client/__init__.py @@ -1,3 +1,5 @@ +import os + import botocore from boto3 import Session from botocore.config import Config @@ -29,6 +31,17 @@ def get_s3_object( ) s3 = session.resource("s3", config=AWS_CLIENT_CONFIG) obj = s3.Object(bucket_name, filename) + # This 'proves' that use of moto in the relevant tests in test_send.py + # mocks everything related to S3. What you will see in the logs is: + # Exception: CREATED AT + # + # raise Exception(f"CREATED AT {_s3.Bucket(bucket_name).creation_date}") + if os.getenv("NOTIFY_ENVIRONMENT") == "test": + teststr = str(s3.Bucket(bucket_name).creation_date).lower() + if "magicmock" not in teststr: + raise Exception( + f"xxxxxtest not mocked, use @mock_aws creation date is {teststr}" + ) return obj From 2414b09c0ffd8059834b49ec1c75ba78cd234570 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 31 May 2024 09:47:19 -0700 Subject: [PATCH 6/7] fix flake8 --- app/s3_client/__init__.py | 2 +- notifications_utils/s3.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/s3_client/__init__.py b/app/s3_client/__init__.py index d5fa38189..7de3509d2 100644 --- a/app/s3_client/__init__.py +++ b/app/s3_client/__init__.py @@ -40,7 +40,7 @@ def get_s3_object( teststr = str(s3.Bucket(bucket_name).creation_date).lower() if "magicmock" not in teststr: raise Exception( - f"Test is not mocked, use @mock_aws or the relevant mocker.patch to avoid accessing S3" + "Test is not mocked, use @mock_aws or the relevant mocker.patch to avoid accessing S3" ) return obj diff --git a/notifications_utils/s3.py b/notifications_utils/s3.py index 6f7f2ce1d..d33cbe25a 100644 --- a/notifications_utils/s3.py +++ b/notifications_utils/s3.py @@ -47,7 +47,7 @@ def s3upload( teststr = str(_s3.Bucket(bucket_name).creation_date).lower() if "magicmock" not in teststr: raise Exception( - f"Test is not mocked, use @mock_aws or the relevant mocker.patch to avoid accessing S3" + "Test is not mocked, use @mock_aws or the relevant mocker.patch to avoid accessing S3" ) key = _s3.Object(bucket_name, file_location) @@ -102,7 +102,7 @@ def s3download( teststr = str(s3.Bucket(bucket_name).creation_date).lower() if "magicmock" not in teststr: raise Exception( - f"Test is not mocked, use @mock_aws or the relevant mocker.patch to avoid accessing S3" + "Test is not mocked, use @mock_aws or the relevant mocker.patch to avoid accessing S3" ) return key.get()["Body"] except botocore.exceptions.ClientError as error: From 29dbe45cbe32625cd7280318c9f361293be9fbab Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 31 May 2024 11:27:39 -0700 Subject: [PATCH 7/7] code review feedback --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 7ca7c4c49..2fd078feb 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -82,7 +82,7 @@ flake8-print = "^5.0.0" flake8-pytest-style = "^1.7.2" isort = "^5.13.2" jinja2-cli = {version = "==0.8.2", extras = ["yaml"]} -moto="*" +moto = "*" pip-audit = "*" pre-commit = "^3.7.1" pytest = "^8.2.1"