From b776df0dcf8edee9c56e6cf7fe03bc1577ed105b Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Mon, 5 Feb 2024 12:57:48 +0000 Subject: [PATCH 1/8] Add capability to connect to both staged and processed S3 buckets for file uploads --- api/conf/settings.py | 43 +++++++++++---- api/documents/libraries/s3_operations.py | 54 +++++++++++++------ .../upload_document_for_tests/views.py | 5 +- ci.env | 5 +- local.env | 5 +- 5 files changed, 75 insertions(+), 37 deletions(-) diff --git a/api/conf/settings.py b/api/conf/settings.py index 71812f98fa..6d47a43841 100644 --- a/api/conf/settings.py +++ b/api/conf/settings.py @@ -33,6 +33,7 @@ GOV_NOTIFY_ENABLED=(bool, False), DOCUMENT_SIGNING_ENABLED=(bool, False), GIT_COMMIT=(str, ""), + AWS_S3_BUCKETS=(dict, {}), ) # Quick-start development settings - unsuitable for production @@ -233,22 +234,44 @@ # AWS VCAP_SERVICES = env.json("VCAP_SERVICES", {}) +AWS_S3_BUCKETS = {} +FILE_UPLOAD_STAGED_NAME = "file-upload-staged" +FILE_UPLOAD_PROCESSED_NAME = "file-upload-processed" + if VCAP_SERVICES: if "aws-s3-bucket" not in VCAP_SERVICES: raise Exception("S3 Bucket not bound to environment") - - aws_credentials = VCAP_SERVICES["aws-s3-bucket"][0]["credentials"] AWS_ENDPOINT_URL = None - AWS_ACCESS_KEY_ID = aws_credentials["aws_access_key_id"] - AWS_SECRET_ACCESS_KEY = aws_credentials["aws_secret_access_key"] - AWS_REGION = aws_credentials["aws_region"] - AWS_STORAGE_BUCKET_NAME = aws_credentials["bucket_name"] + + for bucket_details in VCAP_SERVICES["aws-s3-bucket"]: + bucket_name = None + if FILE_UPLOAD_PROCESSED_NAME in bucket_details["tags"]: + bucket_name = FILE_UPLOAD_PROCESSED_NAME + elif FILE_UPLOAD_STAGED_NAME in bucket_details["tags"]: + bucket_name = FILE_UPLOAD_STAGED_NAME + else: + # Skip buckets which are not tagged with the expected names + continue + + AWS_S3_BUCKETS[bucket_name] = { + "AWS_ACCESS_KEY_ID": bucket_details["aws_access_key_id"], + "AWS_SECRET_ACCESS_KEY": bucket_details["aws_secret_access_key"], + "AWS_REGION": aws_credentials["aws_region"], + "AWS_STORAGE_BUCKET_NAME": aws_credentials["bucket_name"], + } else: AWS_ENDPOINT_URL = env("AWS_ENDPOINT_URL", default=None) - AWS_ACCESS_KEY_ID = env("AWS_ACCESS_KEY_ID") - AWS_SECRET_ACCESS_KEY = env("AWS_SECRET_ACCESS_KEY") - AWS_REGION = env("AWS_REGION") - AWS_STORAGE_BUCKET_NAME = env("AWS_STORAGE_BUCKET_NAME") + AWS_S3_BUCKETS = env.json("AWS_S3_BUCKETS", {}) + +if FILE_UPLOAD_PROCESSED_NAME not in AWS_S3_BUCKETS: + raise Exception("S3 file upload processed bucket not found") + +if FILE_UPLOAD_STAGED_NAME not in AWS_S3_BUCKETS: + raise Exception("S3 file upload staged bucket not found") + + +FILE_UPLOAD_PROCESSED_BUCKET = AWS_S3_BUCKETS[FILE_UPLOAD_PROCESSED_NAME] +FILE_UPLOAD_STAGED_BUCKET = AWS_S3_BUCKETS[FILE_UPLOAD_STAGED_NAME] if "redis" in VCAP_SERVICES: REDIS_BASE_URL = VCAP_SERVICES["redis"][0]["credentials"]["uri"] diff --git a/api/documents/libraries/s3_operations.py b/api/documents/libraries/s3_operations.py index 0942d45151..a16b3f3123 100644 --- a/api/documents/libraries/s3_operations.py +++ b/api/documents/libraries/s3_operations.py @@ -10,35 +10,53 @@ from django.http import FileResponse -_client = None +_processed_client = None +_staged_client = None def init_s3_client(): # We want to instantiate this once, ideally, but there may be cases where we # want to explicitly re-instiate the client e.g. in tests. - global _client + global _processed_client + global _staged_client additional_s3_params = {} if settings.AWS_ENDPOINT_URL: additional_s3_params["endpoint_url"] = settings.AWS_ENDPOINT_URL - _client = boto3.client( + + _processed_client = boto3.client( + "s3", + aws_access_key_id=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_ACCESS_KEY_ID"], + aws_secret_access_key=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_SECRET_ACCESS_KEY"], + region_name=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], + config=Config(connect_timeout=settings.S3_CONNECT_TIMEOUT, read_timeout=settings.S3_REQUEST_TIMEOUT), + **additional_s3_params, + ) + _staged_client = boto3.client( "s3", - aws_access_key_id=settings.AWS_ACCESS_KEY_ID, - aws_secret_access_key=settings.AWS_SECRET_ACCESS_KEY, - region_name=settings.AWS_REGION, + aws_access_key_id=settings.FILE_UPLOAD_STAGED_BUCKET["AWS_ACCESS_KEY_ID"], + aws_secret_access_key=settings.FILE_UPLOAD_STAGED_BUCKET["AWS_SECRET_ACCESS_KEY"], + region_name=settings.FILE_UPLOAD_STAGED_BUCKET["AWS_REGION"], config=Config(connect_timeout=settings.S3_CONNECT_TIMEOUT, read_timeout=settings.S3_REQUEST_TIMEOUT), **additional_s3_params, ) - return _client + return {"staged": _staged_client, "processed": _processed_client} -init_s3_client() +def _get_bucket_client(bucket): + if bucket == "processed": + return _processed_client, settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"] + elif bucket == "staged": + return _staged_client, settings.FILE_UPLOAD_STAGED_BUCKET["AWS_STORAGE_BUCKET_NAME"] + else: + raise Exception(f"No S3 bucket exists with label '{bucket}'") -def get_object(document_id, s3_key): - logging.info(f"Retrieving file '{s3_key}' on document '{document_id}'") +def get_object(document_id, s3_key, bucket="processed"): + logging.info(f"Retrieving file '{s3_key}' on document '{document_id}' from bucket '{bucket}'") + aws_client, bucket_name = _get_bucket_client(bucket) try: - return _client.get_object(Bucket=settings.AWS_STORAGE_BUCKET_NAME, Key=s3_key) + return aws_client.get_object(Bucket=bucket_name, Key=s3_key) except ReadTimeoutError: logging.warning(f"Timeout exceeded when retrieving file '{s3_key}' on document '{document_id}'") except BotoCoreError as exc: @@ -51,15 +69,17 @@ def generate_s3_key(document_name, file_extension): return f"{document_name}-{uuid.uuid4()}.{file_extension}" -def upload_bytes_file(raw_file, s3_key): - _client.put_object(Bucket=settings.AWS_STORAGE_BUCKET_NAME, Key=s3_key, Body=raw_file) +def upload_bytes_file(raw_file, s3_key, bucket="processed"): + aws_client, bucket_name = _get_bucket_client(bucket) + aws_client.put_object(Bucket=bucket_name, Key=s3_key, Body=raw_file) -def delete_file(document_id, s3_key): - logging.info(f"Deleting file '{s3_key}' on document '{document_id}'") +def delete_file(document_id, s3_key, bucket="processed"): + logging.info(f"Deleting file '{s3_key}' on document '{document_id}' from bucket '{bucket}'") + aws_client, bucket_name = _get_bucket_client(bucket) try: - _client.delete_object(Bucket=settings.AWS_STORAGE_BUCKET_NAME, Key=s3_key) + aws_client.delete_object(Bucket=bucket_name, Key=s3_key) except ReadTimeoutError: logging.warning(f"Timeout exceeded when retrieving file '{s3_key}' on document '{document_id}'") except BotoCoreError as exc: @@ -69,7 +89,7 @@ def delete_file(document_id, s3_key): def document_download_stream(document): - s3_response = get_object(document.id, document.s3_key) + s3_response = get_object(document.id, document.s3_key, "processed") content_type = mimetypes.MimeTypes().guess_type(document.name)[0] response = FileResponse( diff --git a/api/staticdata/upload_document_for_tests/views.py b/api/staticdata/upload_document_for_tests/views.py index bd84fb8749..cd15a9b908 100644 --- a/api/staticdata/upload_document_for_tests/views.py +++ b/api/staticdata/upload_document_for_tests/views.py @@ -6,6 +6,7 @@ from rest_framework.views import APIView from api.core.authentication import SharedAuthentication + from api.conf.settings import env from api.documents.libraries.s3_operations import init_s3_client @@ -22,7 +23,7 @@ def get(self, request): status=status.HTTP_405_METHOD_NOT_ALLOWED, ) - s3 = init_s3_client() + s3 = init_s3_client()["processed"] s3_key = "lite-e2e-test-file.txt" file_to_upload_abs_path = os.path.abspath( @@ -30,7 +31,7 @@ def get(self, request): ) try: - s3.upload_file(file_to_upload_abs_path, settings.AWS_STORAGE_BUCKET_NAME, s3_key) + s3.upload_file(file_to_upload_abs_path, settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], s3_key) except Exception as e: # noqa return JsonResponse( data={"errors": "Error uploading file to S3"}, status=status.HTTP_500_INTERNAL_SERVER_ERROR diff --git a/ci.env b/ci.env index e2e4f9c6e7..e34f5de40a 100644 --- a/ci.env +++ b/ci.env @@ -10,10 +10,7 @@ INTERNAL_USERS='[{"email": "filippo.raimondi@digital.trade.gov.uk", "role": "Sup EXPORTER_USERS='[{"email": "filippo.raimondi@digital.trade.gov.uk", "organisation": "Archway Communications", "role": "Super User"}]' # AWS AWS_ENDPOINT_URL=AWS_ENDPOINT_URL -AWS_ACCESS_KEY_ID=AWS_ACCESS_KEY_ID -AWS_SECRET_ACCESS_KEY=AWS_SECRET_ACCESS_KEY -AWS_STORAGE_BUCKET_NAME=AWS_STORAGE_BUCKET_NAME -AWS_REGION=eu-west-2 +S3_BUCKETS=S3_BUCKETS # AV AV_SERVICE_URL=AV_SERVICE_URL AV_SERVICE_USERNAME=AV_SERVICE_USERNAME diff --git a/local.env b/local.env index 92cdf5f92f..47495b3df2 100644 --- a/local.env +++ b/local.env @@ -33,10 +33,7 @@ LITE_API_ENABLE_ES=True # AWS AWS_ENDPOINT_URL=http://s3:9000 -AWS_ACCESS_KEY_ID=minio_username -AWS_SECRET_ACCESS_KEY=minio_password -AWS_STORAGE_BUCKET_NAME=uploads -AWS_REGION=eu-west-2 +AWS_S3_BUCKETS={"file-upload-processed": {"AWS_ACCESS_KEY_ID": "XXX", "AWS_SECRET_ACCESS_KEY": "XXX", "AWS_STORAGE_BUCKET_NAME": "XXX", "AWS_REGION": "eu-west-2"}, "file-upload-staged": {"AWS_ACCESS_KEY_ID": "XXX", "AWS_SECRET_ACCESS_KEY": "XXX", "AWS_STORAGE_BUCKET_NAME": "XXX", "AWS_REGION": "eu-west-2"}} # AV AV_SERVICE_URL=<> From a8f889618830ec6145fd21980dc7c05fce857bf9 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Tue, 6 Feb 2024 11:39:20 +0000 Subject: [PATCH 2/8] Add celery task for processing files from staged S3 bucket to processed S3 bucket --- api/documents/celery_tasks.py | 17 ++++- api/documents/libraries/process_document.py | 8 +-- api/documents/libraries/s3_operations.py | 45 +++++++++--- .../libraries/tests/test_s3_operations.py | 71 ++++++++++++++----- api/documents/models.py | 7 +- api/documents/tests/test_celery_tasks.py | 23 +++++- api/documents/tests/test_models.py | 12 ++++ docker-compose.yml | 2 +- local.env | 2 +- 9 files changed, 150 insertions(+), 37 deletions(-) create mode 100644 api/documents/tests/test_models.py diff --git a/api/documents/celery_tasks.py b/api/documents/celery_tasks.py index 233b499b26..a3c04498fe 100644 --- a/api/documents/celery_tasks.py +++ b/api/documents/celery_tasks.py @@ -11,6 +11,16 @@ logger = get_task_logger(__name__) +@shared_task( + bind=True, +) +def process_uploaded_document(self, document_id): + """ """ + document = Document.objects.get(id=document_id) + document.move_staged_document() + scan_document_for_viruses.apply_async(args=(document_id,), link_error=delete_document_from_s3.si(document_id)) + + @shared_task( bind=True, autoretry_for=(Exception,), @@ -43,4 +53,9 @@ def delete_document_from_s3(document_id): "Maximum attempts of %s for document %s has been reached calling s3 delete", MAX_ATTEMPTS, document_id ) document = Document.objects.get(id=document_id) - document.delete_s3() + # For now, always attempt to delete from both staged and processed S3 buckets.. + # This is because we cannot be sure right now if we have moved over to using + # two buckets or not. When we are using two S3 buckets, we can be more specific and ensure + # to only target the `delete_document_from_s3()` task at one of the S3 buckets + document.delete_s3(bucket="staged") + document.delete_s3(bucket="processed") diff --git a/api/documents/libraries/process_document.py b/api/documents/libraries/process_document.py index 0b50b86eb6..52539ad718 100644 --- a/api/documents/libraries/process_document.py +++ b/api/documents/libraries/process_document.py @@ -2,7 +2,7 @@ from rest_framework import serializers -from api.documents.celery_tasks import scan_document_for_viruses, delete_document_from_s3 +from api.documents.celery_tasks import process_uploaded_document, delete_document_from_s3 logger = logging.getLogger(__name__) @@ -10,7 +10,7 @@ def process_document(document): try: document_id = str(document.id) - scan_document_for_viruses.apply_async(args=(document_id,), link_error=delete_document_from_s3.si(document_id)) + process_uploaded_document.apply_async(args=(document_id,), link_error=delete_document_from_s3.si(document_id)) except Exception: - logger.exception("Error scanning document with id %s for viruses", document_id) - raise serializers.ValidationError({"document": "Error scanning document for viruses"}) + logger.exception("Error processing document with id %s", document_id) + raise serializers.ValidationError({"document": "Error processing document"}) diff --git a/api/documents/libraries/s3_operations.py b/api/documents/libraries/s3_operations.py index a16b3f3123..51875dc377 100644 --- a/api/documents/libraries/s3_operations.py +++ b/api/documents/libraries/s3_operations.py @@ -4,11 +4,12 @@ import boto3 from botocore.config import Config -from botocore.exceptions import BotoCoreError, ReadTimeoutError +from botocore.exceptions import BotoCoreError, ReadTimeoutError, ClientError from django.conf import settings from django.http import FileResponse +logger = logging.getLogger(__name__) _processed_client = None _staged_client = None @@ -42,6 +43,9 @@ def init_s3_client(): return {"staged": _staged_client, "processed": _processed_client} +init_s3_client() + + def _get_bucket_client(bucket): if bucket == "processed": return _processed_client, settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"] @@ -52,19 +56,44 @@ def _get_bucket_client(bucket): def get_object(document_id, s3_key, bucket="processed"): - logging.info(f"Retrieving file '{s3_key}' on document '{document_id}' from bucket '{bucket}'") + logger.info(f"Retrieving file '{s3_key}' on document '{document_id}' from bucket '{bucket}'") aws_client, bucket_name = _get_bucket_client(bucket) try: return aws_client.get_object(Bucket=bucket_name, Key=s3_key) except ReadTimeoutError: - logging.warning(f"Timeout exceeded when retrieving file '{s3_key}' on document '{document_id}'") + logger.warning(f"Timeout exceeded when retrieving file '{s3_key}' on document '{document_id}'") except BotoCoreError as exc: - logging.warning( + logger.warning( f"An unexpected error occurred when retrieving file '{s3_key}' on document '{document_id}': {exc}" ) +def move_staged_document_to_processed(document_id, s3_key): + logger.info(f"Moving file '{s3_key}' on document '{document_id}' from staged bucket to processed bucket") + # Grab the document from the staged S3 bucket + try: + staged_document = get_object(document_id, s3_key, "staged") + except ClientError as exc: + logger.warning(f"An error occurred when retrieving file '{s3_key}' on document '{document_id}': {exc}") + # TODO: When we move over to using two S3 buckets, we should make this raise an exception. + # For now, this keeps us backward compatible so that we can switch from + # a single S3 bucket to staged/processed buckets more smoothly + return + + # Upload the document to the processed S3 bucket + # NOTE: Ideally we would use AWS' copy operation to copy from bucket to bucket. + # However, the IAM credentials we are using are limited with individual credentials having + # read/write for ONE bucket only - for copying, we would need credentials with read for the + # staged bucket and write for the processed bucket. This might be something to investigate + # with SRE later. + processed_aws_client, processed_bucket_name = _get_bucket_client("processed") + processed_aws_client.put_object(Bucket=processed_bucket_name, Key=s3_key, Body=staged_document["Body"].read()) + + # Delete the document from the staged S3 bucket now we have moved it successfully + delete_file(document_id, s3_key, bucket="staged") + + def generate_s3_key(document_name, file_extension): return f"{document_name}-{uuid.uuid4()}.{file_extension}" @@ -75,17 +104,15 @@ def upload_bytes_file(raw_file, s3_key, bucket="processed"): def delete_file(document_id, s3_key, bucket="processed"): - logging.info(f"Deleting file '{s3_key}' on document '{document_id}' from bucket '{bucket}'") + logger.info(f"Deleting file '{s3_key}' on document '{document_id}' from bucket '{bucket}'") aws_client, bucket_name = _get_bucket_client(bucket) try: aws_client.delete_object(Bucket=bucket_name, Key=s3_key) except ReadTimeoutError: - logging.warning(f"Timeout exceeded when retrieving file '{s3_key}' on document '{document_id}'") + logger.warning(f"Timeout exceeded when retrieving file '{s3_key}' on document '{document_id}'") except BotoCoreError as exc: - logging.warning( - f"An unexpected error occurred when deleting file '{s3_key}' on document '{document_id}': {exc}" - ) + logger.warning(f"An unexpected error occurred when deleting file '{s3_key}' on document '{document_id}': {exc}") def document_download_stream(document): diff --git a/api/documents/libraries/tests/test_s3_operations.py b/api/documents/libraries/tests/test_s3_operations.py index 53d58e6fb3..9e1e0dff0a 100644 --- a/api/documents/libraries/tests/test_s3_operations.py +++ b/api/documents/libraries/tests/test_s3_operations.py @@ -12,17 +12,28 @@ document_download_stream, init_s3_client, get_object, + move_staged_document_to_processed, upload_bytes_file, ) +TEST_AWS_BUCKET_NAME = settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"] + + @patch("api.documents.libraries.s3_operations.boto3") @patch("api.documents.libraries.s3_operations.Config") @override_settings( AWS_ENDPOINT_URL="AWS_ENDPOINT_URL", - AWS_ACCESS_KEY_ID="AWS_ACCESS_KEY_ID", - AWS_SECRET_ACCESS_KEY="AWS_SECRET_ACCESS_KEY", - AWS_REGION="AWS_REGION", + FILE_UPLOAD_PROCESSED_BUCKET={ + "AWS_ACCESS_KEY_ID": "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY": "AWS_SECRET_ACCESS_KEY", + "AWS_REGION": "AWS_REGION", + }, + FILE_UPLOAD_STAGED_BUCKET={ + "AWS_ACCESS_KEY_ID": "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY": "AWS_SECRET_ACCESS_KEY", + "AWS_REGION": "AWS_REGION", + }, S3_CONNECT_TIMEOUT=22, S3_REQUEST_TIMEOUT=44, ) @@ -35,7 +46,8 @@ def test_get_client_without_aws_endpoint_url(self, mock_Config, mock_boto3): mock_boto3.client.return_value = mock_client returned_client = init_s3_client() - self.assertEqual(returned_client, mock_client) + self.assertEqual(returned_client["processed"], mock_client) + self.assertEqual(returned_client["staged"], mock_client) mock_Config.assert_called_with( connect_timeout=22, @@ -58,7 +70,8 @@ def test_get_client_with_aws_endpoint_url(self, mock_Config, mock_boto3): mock_boto3.client.return_value = mock_client returned_client = init_s3_client() - self.assertEqual(returned_client, mock_client) + self.assertEqual(returned_client["processed"], mock_client) + self.assertEqual(returned_client["staged"], mock_client) mock_Config.assert_called_with( connect_timeout=22, @@ -79,10 +92,15 @@ def test_get_client_with_aws_endpoint_url(self, mock_Config, mock_boto3): @override_settings( - AWS_STORAGE_BUCKET_NAME="test-bucket", + FILE_UPLOAD_PROCESSED_BUCKET={ + "AWS_ACCESS_KEY_ID": "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY": "AWS_SECRET_ACCESS_KEY", + "AWS_REGION": "AWS_REGION", + "AWS_STORAGE_BUCKET_NAME": "test-bucket", + }, ) class S3OperationsGetObjectTests(SimpleTestCase): - @patch("api.documents.libraries.s3_operations._client") + @patch("api.documents.libraries.s3_operations._processed_client") def test_get_object(self, mock_client): mock_object = Mock() mock_client.get_object.return_value = mock_object @@ -93,31 +111,52 @@ def test_get_object(self, mock_client): mock_client.get_object.assert_called_with(Bucket="test-bucket", Key="s3-key") +class S3OperationsMoveStagedDocumentToProcessedTests(SimpleTestCase): + @patch("api.documents.libraries.s3_operations._staged_client") + @patch("api.documents.libraries.s3_operations._processed_client") + def test_get_object(self, mock_processed_client, mock_staged_client): + mock_staged_body = Mock() + mock_staged_file = {"Body": mock_staged_body} + mock_staged_client.get_object.return_value = mock_staged_file + + move_staged_document_to_processed("document-id", "s3-key") + + mock_staged_client.get_object.assert_called_with(Bucket="staged", Key="s3-key") + mock_processed_client.put_object.assert_called_with( + Bucket="processed", Key="s3-key", Body=mock_staged_body.read() + ) + mock_staged_client.delete_object.assert_called_with(Bucket="staged", Key="s3-key") + + @contextmanager def _create_bucket(s3): s3.create_bucket( - Bucket=settings.AWS_STORAGE_BUCKET_NAME, + Bucket=TEST_AWS_BUCKET_NAME, CreateBucketConfiguration={ - "LocationConstraint": settings.AWS_REGION, + "LocationConstraint": settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], }, ) yield +def get_s3_client(): + return init_s3_client()["processed"] + + @mock_aws class S3OperationsDeleteFileTests(SimpleTestCase): def test_delete_file(self): - s3 = init_s3_client() + s3 = get_s3_client() with _create_bucket(s3): s3.put_object( - Bucket=settings.AWS_STORAGE_BUCKET_NAME, + Bucket=TEST_AWS_BUCKET_NAME, Key="s3-key", Body=b"test", ) delete_file("document-id", "s3-key") - objs = s3.list_objects(Bucket=settings.AWS_STORAGE_BUCKET_NAME) + objs = s3.list_objects(Bucket=TEST_AWS_BUCKET_NAME) keys = [o["Key"] for o in objs.get("Contents", [])] self.assertNotIn("s3-key", keys) @@ -125,12 +164,12 @@ def test_delete_file(self): @mock_aws class S3OperationsUploadBytesFileTests(SimpleTestCase): def test_upload_bytes_file(self): - s3 = init_s3_client() + s3 = get_s3_client() with _create_bucket(s3): upload_bytes_file(b"test", "s3-key") obj = s3.get_object( - Bucket=settings.AWS_STORAGE_BUCKET_NAME, + Bucket=TEST_AWS_BUCKET_NAME, Key="s3-key", ) self.assertEqual(obj["Body"].read(), b"test") @@ -139,10 +178,10 @@ def test_upload_bytes_file(self): @mock_aws class S3OperationsDocumentDownloadStreamTests(SimpleTestCase): def test_document_download_stream(self): - s3 = init_s3_client() + s3 = get_s3_client() with _create_bucket(s3): s3.put_object( - Bucket=settings.AWS_STORAGE_BUCKET_NAME, + Bucket=TEST_AWS_BUCKET_NAME, Key="s3-key", Body=b"test", ) diff --git a/api/documents/models.py b/api/documents/models.py index 055554db7f..7d2a99a533 100644 --- a/api/documents/models.py +++ b/api/documents/models.py @@ -19,10 +19,13 @@ class Document(TimestampableModel): def __str__(self): return self.name - def delete_s3(self): + def delete_s3(self, bucket="processed"): """Removes the document's file from S3.""" - s3_operations.delete_file(self.id, self.s3_key) + s3_operations.delete_file(self.id, self.s3_key, bucket=bucket) + + def move_staged_document(self): + s3_operations.move_staged_document_to_processed(self.id, self.s3_key) def scan_for_viruses(self): """Retrieves the document's file from S3 and scans it for viruses.""" diff --git a/api/documents/tests/test_celery_tasks.py b/api/documents/tests/test_celery_tasks.py index 43b0ab2813..c55c518b60 100644 --- a/api/documents/tests/test_celery_tasks.py +++ b/api/documents/tests/test_celery_tasks.py @@ -6,10 +6,10 @@ from django.utils.timezone import now from rest_framework.exceptions import ValidationError -from api.documents.celery_tasks import scan_document_for_viruses, delete_document_from_s3 +from api.documents.celery_tasks import scan_document_for_viruses, delete_document_from_s3, process_uploaded_document -class DocumentVirusScan(DataTestClient): +class TestCeleryTasks(DataTestClient): def setUp(self): super().setUp() self.case = self.create_standard_application_case(self.organisation) @@ -41,7 +41,7 @@ def test_document_scan_document_for_viruses_called(self, mock_document_scan_for_ scan_document_for_viruses(str(document.id)) mock_document_scan_for_viruses.called_once() - @mock.patch("api.documents.celery_tasks.scan_document_for_viruses.apply_async") + @mock.patch("api.documents.celery_tasks.process_uploaded_document.apply_async") def test_process_document_raises_validation_exception(self, mock_scan_for_viruses): # given there is a case document @@ -74,3 +74,20 @@ def test_delete_document_calls_s3_delete(self, mock_delete_s3): document = self.create_case_document(case=self.case, user=self.gov_user, name="jimmy") delete_document_from_s3(str(document.id)) mock_delete_s3.called_once() + + @mock.patch("api.documents.models.Document.move_staged_document") + @mock.patch("api.documents.models.Document.scan_for_viruses") + def test_process_uploaded_document_success(self, mock_scan_for_viruses, mock_move_staged_document): + document = self.create_case_document(case=self.case, user=self.gov_user, name="jimmy") + process_uploaded_document(str(document.id)) + mock_move_staged_document.assert_called() + mock_scan_for_viruses.assert_called() + + @mock.patch("api.documents.models.Document.move_staged_document") + @mock.patch("api.documents.models.Document.scan_for_viruses") + def test_process_uploaded_document_failure(self, mock_scan_for_viruses, mock_move_staged_document): + document = self.create_case_document(case=self.case, user=self.gov_user, name="jimmy") + mock_move_staged_document.side_effect = Exception("FAIL") + with pytest.raises(Exception): + process_uploaded_document(str(document.id)) + mock_scan_for_viruses.assert_not_called() diff --git a/api/documents/tests/test_models.py b/api/documents/tests/test_models.py new file mode 100644 index 0000000000..25253bbab6 --- /dev/null +++ b/api/documents/tests/test_models.py @@ -0,0 +1,12 @@ +from unittest.mock import patch + +from api.documents.tests.factories import DocumentFactory +from test_helpers.clients import DataTestClient + + +class DocumentTest(DataTestClient): + @patch("api.documents.libraries.s3_operations.move_staged_document_to_processed") + def test_move_staged_document(self, mocked_move_document): + document = DocumentFactory() + document.move_staged_document() + mocked_move_document.assert_called_with(document.id, document.s3_key) diff --git a/docker-compose.yml b/docker-compose.yml index ab4d4dd12e..029e0724f5 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -104,7 +104,7 @@ services: - 9000:9000 - 9001:9001 entrypoint: sh - command: -c 'mkdir -p /buckets/uploads && minio server /buckets --console-address ":9001"' + command: -c 'mkdir -p /buckets/processed && mkdir -p /buckets/staged && minio server /buckets --console-address ":9001"' environment: - MINIO_ROOT_USER=minio_username - MINIO_ROOT_PASSWORD=minio_password diff --git a/local.env b/local.env index 47495b3df2..91173c6613 100644 --- a/local.env +++ b/local.env @@ -33,7 +33,7 @@ LITE_API_ENABLE_ES=True # AWS AWS_ENDPOINT_URL=http://s3:9000 -AWS_S3_BUCKETS={"file-upload-processed": {"AWS_ACCESS_KEY_ID": "XXX", "AWS_SECRET_ACCESS_KEY": "XXX", "AWS_STORAGE_BUCKET_NAME": "XXX", "AWS_REGION": "eu-west-2"}, "file-upload-staged": {"AWS_ACCESS_KEY_ID": "XXX", "AWS_SECRET_ACCESS_KEY": "XXX", "AWS_STORAGE_BUCKET_NAME": "XXX", "AWS_REGION": "eu-west-2"}} +AWS_S3_BUCKETS={"file-upload-processed": {"AWS_ACCESS_KEY_ID": "minio_username", "AWS_SECRET_ACCESS_KEY": "minio_password", "AWS_STORAGE_BUCKET_NAME": "processed", "AWS_REGION": "eu-west-2"}, "file-upload-staged": {"AWS_ACCESS_KEY_ID": "minio_username", "AWS_SECRET_ACCESS_KEY": "minio_password", "AWS_STORAGE_BUCKET_NAME": "staged", "AWS_REGION": "eu-west-2"}} # AV AV_SERVICE_URL=<> From 0a45e3e8512748a279f5c83fb28d264d55e039c0 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Tue, 6 Feb 2024 15:40:47 +0000 Subject: [PATCH 3/8] Fix test_document_stream.py unit tests --- api/documents/tests/test_document_stream.py | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-) diff --git a/api/documents/tests/test_document_stream.py b/api/documents/tests/test_document_stream.py index e613770c85..90131dcdb4 100644 --- a/api/documents/tests/test_document_stream.py +++ b/api/documents/tests/test_document_stream.py @@ -7,12 +7,7 @@ from test_helpers.clients import DataTestClient -from api.conf.settings import ( - AWS_ACCESS_KEY_ID, - AWS_SECRET_ACCESS_KEY, - AWS_REGION, - AWS_STORAGE_BUCKET_NAME, -) +from api.conf import settings from api.documents.libraries.s3_operations import init_s3_client @@ -23,18 +18,18 @@ def setUp(self): init_s3_client() s3 = boto3.client( "s3", - aws_access_key_id=AWS_ACCESS_KEY_ID, - aws_secret_access_key=AWS_SECRET_ACCESS_KEY, - region_name=AWS_REGION, + aws_access_key_id=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_ACCESS_KEY_ID"], + aws_secret_access_key=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_SECRET_ACCESS_KEY"], + region_name=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], ) s3.create_bucket( - Bucket=AWS_STORAGE_BUCKET_NAME, + Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], CreateBucketConfiguration={ - "LocationConstraint": AWS_REGION, + "LocationConstraint": settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], }, ) s3.put_object( - Bucket=AWS_STORAGE_BUCKET_NAME, + Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], Key="thisisakey", Body=b"test", ) From 1cb0383a4c346b69f0e69858e30650cea66efea2 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Tue, 6 Feb 2024 15:54:01 +0000 Subject: [PATCH 4/8] Fix more unit tests --- api/cases/tests/test_case_documents.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api/cases/tests/test_case_documents.py b/api/cases/tests/test_case_documents.py index af56b478cd..5ac075c4c2 100644 --- a/api/cases/tests/test_case_documents.py +++ b/api/cases/tests/test_case_documents.py @@ -40,15 +40,15 @@ def setUp(self): self.file = self.create_case_document(self.case, self.gov_user, "Test") self.path = "cases:document_download" - s3 = init_s3_client() + s3 = init_s3_client()["processed"] s3.create_bucket( - Bucket=settings.AWS_STORAGE_BUCKET_NAME, + Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], CreateBucketConfiguration={ - "LocationConstraint": settings.AWS_REGION, + "LocationConstraint": settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], }, ) s3.put_object( - Bucket=settings.AWS_STORAGE_BUCKET_NAME, + Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], Key=self.file.s3_key, Body=b"test", ) From aed91dd61357c9f16669274af2f1acaf14c914ec Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Wed, 7 Feb 2024 10:04:31 +0000 Subject: [PATCH 5/8] Mock AWS in tests at session level --- api/cases/tests/test_case_documents.py | 7 ------ api/conf/settings_test.py | 2 ++ api/conftest.py | 27 +++++++++++++++------ api/documents/libraries/s3_operations.py | 1 + api/documents/tests/test_document_stream.py | 15 +----------- api/organisations/tests/test_documents.py | 12 ++++++--- api/test_more_documents.py | 5 ++-- 7 files changed, 34 insertions(+), 35 deletions(-) diff --git a/api/cases/tests/test_case_documents.py b/api/cases/tests/test_case_documents.py index 5ac075c4c2..0ebea43bf5 100644 --- a/api/cases/tests/test_case_documents.py +++ b/api/cases/tests/test_case_documents.py @@ -31,7 +31,6 @@ def test_can_view_all_documents_on_a_case(self): self.assertEqual(len(response_data["documents"]), 2) -@mock_aws class CaseDocumentDownloadTests(DataTestClient): def setUp(self): super().setUp() @@ -41,12 +40,6 @@ def setUp(self): self.path = "cases:document_download" s3 = init_s3_client()["processed"] - s3.create_bucket( - Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], - CreateBucketConfiguration={ - "LocationConstraint": settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], - }, - ) s3.put_object( Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], Key=self.file.s3_key, diff --git a/api/conf/settings_test.py b/api/conf/settings_test.py index e28fcb845a..ff2dd47053 100644 --- a/api/conf/settings_test.py +++ b/api/conf/settings_test.py @@ -8,3 +8,5 @@ SUPPRESS_TEST_OUTPUT = True AWS_ENDPOINT_URL = None +CELERY_TASK_ALWAYS_EAGER = True +CELERY_TASK_STORE_EAGER_RESULT = True diff --git a/api/conftest.py b/api/conftest.py index 944cc97c27..1d593b51d5 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -5,6 +5,7 @@ from django import db from celery import Celery +from moto import mock_aws import re import glob @@ -13,6 +14,8 @@ import pytest # noqa from django.conf import settings +from api.documents.libraries.s3_operations import init_s3_client + def camelcase_to_underscore(string): """SRC: https://djangosnippets.org/snippets/585/""" @@ -130,11 +133,19 @@ def setup(settings): @pytest.fixture(autouse=True) -def celery_app(): - # Setup the celery worker to run in process for tests - os.environ.setdefault("DJANGO_SETTINGS_MODULE", "api.conf.settings") - celeryapp = Celery("api") - celeryapp.autodiscover_tasks(related_name="celery_tasks") - celeryapp.conf.update(CELERY_ALWAYS_EAGER=True) - celeryapp.conf.update(CELERY_TASK_STORE_EAGER_RESULT=True) - return celeryapp +def mock_aws_calls(): + with mock_aws(): + clients = init_s3_client() + clients["processed"].create_bucket( + Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], + CreateBucketConfiguration={ + "LocationConstraint": settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], + }, + ) + clients["staged"].create_bucket( + Bucket=settings.FILE_UPLOAD_STAGED_BUCKET["AWS_STORAGE_BUCKET_NAME"], + CreateBucketConfiguration={ + "LocationConstraint": settings.FILE_UPLOAD_STAGED_BUCKET["AWS_REGION"], + }, + ) + yield diff --git a/api/documents/libraries/s3_operations.py b/api/documents/libraries/s3_operations.py index 51875dc377..6627256582 100644 --- a/api/documents/libraries/s3_operations.py +++ b/api/documents/libraries/s3_operations.py @@ -74,6 +74,7 @@ def move_staged_document_to_processed(document_id, s3_key): # Grab the document from the staged S3 bucket try: staged_document = get_object(document_id, s3_key, "staged") + except ClientError as exc: logger.warning(f"An error occurred when retrieving file '{s3_key}' on document '{document_id}': {exc}") # TODO: When we move over to using two S3 buckets, we should make this raise an exception. diff --git a/api/documents/tests/test_document_stream.py b/api/documents/tests/test_document_stream.py index 90131dcdb4..a5463a86a9 100644 --- a/api/documents/tests/test_document_stream.py +++ b/api/documents/tests/test_document_stream.py @@ -11,23 +11,10 @@ from api.documents.libraries.s3_operations import init_s3_client -@mock_aws class DocumentStream(DataTestClient): def setUp(self): super().setUp() - init_s3_client() - s3 = boto3.client( - "s3", - aws_access_key_id=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_ACCESS_KEY_ID"], - aws_secret_access_key=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_SECRET_ACCESS_KEY"], - region_name=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], - ) - s3.create_bucket( - Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], - CreateBucketConfiguration={ - "LocationConstraint": settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], - }, - ) + s3 = init_s3_client()["processed"] s3.put_object( Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], Key="thisisakey", diff --git a/api/organisations/tests/test_documents.py b/api/organisations/tests/test_documents.py index 0973986946..3e73a50dc5 100644 --- a/api/organisations/tests/test_documents.py +++ b/api/organisations/tests/test_documents.py @@ -2,10 +2,12 @@ from unittest import mock from django.urls import reverse +from django.conf import settings from api.organisations.enums import OrganisationDocumentType from api.organisations.models import DocumentOnOrganisation from test_helpers.clients import DataTestClient +from api.documents.libraries.s3_operations import init_s3_client class OrganisationDocumentViewTests(DataTestClient): @@ -19,6 +21,12 @@ def setUp(self): def create_document_on_organisation(self, name): url = reverse("organisations:documents", kwargs={"pk": self.organisation.pk}) + s3 = init_s3_client()["staged"] + s3.put_object( + Bucket=settings.FILE_UPLOAD_STAGED_BUCKET["AWS_STORAGE_BUCKET_NAME"], + Key=name, + Body=b"test", + ) data = { "document": {"name": name, "s3_key": name, "size": 476}, "expiry_date": "2026-01-01", @@ -63,10 +71,8 @@ def test_list_organisation_documents(self, mock_virus_scan, mock_s3_operations_g self.assertEqual(response.status_code, 200) self.assertEqual(len(response.json()["documents"]), 3) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_retrieve_organisation_documents(self, mock_virus_scan, mock_s3_operations_get_object): - mock_s3_operations_get_object.return_value = self.document_data + def test_retrieve_organisation_documents(self, mock_virus_scan): mock_virus_scan.return_value = False response = self.create_document_on_organisation("some-document-one") self.assertEqual(response.status_code, 201) diff --git a/api/test_more_documents.py b/api/test_more_documents.py index de111c17d2..1b3dc1e4b9 100644 --- a/api/test_more_documents.py +++ b/api/test_more_documents.py @@ -1,6 +1,7 @@ import uuid from unittest import mock +from django.conf import settings from django.urls import reverse from parameterized import parameterized from rest_framework import status @@ -82,12 +83,10 @@ def test_upload_multiple_documents_on_unsubmitted_application(self, mock_virus_s for document in data: self.assertTrue(document in response_data) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.models.Document.delete_s3") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_delete_individual_draft_document(self, mock_virus_scan, mock_delete_s3, mock_s3_operations_get_object): + def test_delete_individual_draft_document(self, mock_virus_scan, mock_delete_s3): """Test success in deleting a document from an unsubmitted application.""" - mock_s3_operations_get_object.return_value = self.data mock_virus_scan.return_value = False self.client.post(self.url_draft, data=self.data, **self.exporter_headers) response = self.client.get(self.url_draft, **self.exporter_headers) From c5549a94f036dc4129bcc1898b5c1d6640ae6057 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Wed, 7 Feb 2024 11:09:11 +0000 Subject: [PATCH 6/8] Stop mocking s3 get_object in any document related unit tests --- api/applications/tests/test_consignee.py | 23 +++++--------- api/applications/tests/test_documents.py | 9 +++--- api/applications/tests/test_end_user.py | 31 ++++++------------- api/applications/tests/test_goods.py | 9 +++--- .../tests/test_goods_type_on_application.py | 23 +++++--------- api/applications/tests/test_third_parties.py | 23 +++++--------- .../tests/test_ultimate_end_users.py | 20 ++++-------- api/cases/tests/test_case_documents.py | 11 ++----- api/cases/tests/test_case_ecju_queries.py | 9 +++--- api/conftest.py | 2 +- api/documents/tests/test_document_stream.py | 9 ++---- api/organisations/tests/test_documents.py | 25 ++++----------- api/test_more_documents.py | 24 +++++--------- test_helpers/file_uploads.py | 18 +++++++++++ 14 files changed, 91 insertions(+), 145 deletions(-) create mode 100644 test_helpers/file_uploads.py diff --git a/api/applications/tests/test_consignee.py b/api/applications/tests/test_consignee.py index b70bfd4d90..497372135b 100644 --- a/api/applications/tests/test_consignee.py +++ b/api/applications/tests/test_consignee.py @@ -11,6 +11,7 @@ from api.parties.models import PartyDocument from api.staticdata.countries.helpers import get_country from test_helpers.clients import DataTestClient +from test_helpers.file_uploads import upload_file class ConsigneeOnDraftTests(DataTestClient): @@ -18,11 +19,13 @@ def setUp(self): super().setUp() self.draft = self.create_draft_standard_application(self.organisation) self.url = reverse("applications:parties", kwargs={"pk": self.draft.id}) + s3_key = "s3_keykey.pdf" self.new_document_data = { "name": "document_name.pdf", - "s3_key": "s3_keykey.pdf", + "s3_key": s3_key, "size": 123456, } + upload_file(s3_key) @parameterized.expand([SubType.GOVERNMENT, SubType.COMMERCIAL, SubType.OTHER]) def test_set_consignee_on_draft_successful(self, data_type): @@ -174,9 +177,8 @@ def test_delete_consignee_on_standard_application_when_application_has_no_consig self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_post_consignee_document_success(self, mock_virus_scan, mock_s3_operations_get_object): + def test_post_consignee_document_success(self, mock_virus_scan): """ Given a standard draft has been created And the draft contains a consignee @@ -184,7 +186,6 @@ def test_post_consignee_document_success(self, mock_virus_scan, mock_s3_operatio When a document is submitted Then a 201 CREATED is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False party = PartyOnApplication.objects.get( application=self.draft, party__type=PartyType.CONSIGNEE, deleted_at__isnull=True @@ -196,9 +197,8 @@ def test_post_consignee_document_success(self, mock_virus_scan, mock_s3_operatio self.assertEqual(response.status_code, status.HTTP_201_CREATED) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_get_consignee_document_success(self, mock_virus_scan, mock_s3_operations_get_object): + def test_get_consignee_document_success(self, mock_virus_scan): """ Given a standard draft has been created And the draft contains a consignee @@ -206,7 +206,6 @@ def test_get_consignee_document_success(self, mock_virus_scan, mock_s3_operation When the document is retrieved Then the data in the document is the same as the data in the attached consignee document """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False party = PartyOnApplication.objects.get( application=self.draft, party__type=PartyType.CONSIGNEE, deleted_at__isnull=True @@ -221,12 +220,9 @@ def test_get_consignee_document_success(self, mock_virus_scan, mock_s3_operation self.assertEqual(response_data["s3_key"], expected["s3_key"]) self.assertEqual(response_data["size"], expected["size"]) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_consignee_document_success( - self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object - ): + def test_delete_consignee_document_success(self, delete_s3_function, mock_virus_scan): """ Given a standard draft has been created And the draft contains an end user @@ -234,7 +230,6 @@ def test_delete_consignee_document_success( When there is an attempt to delete the document Then 204 NO CONTENT is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False party = PartyOnApplication.objects.get( application=self.draft, party__type=PartyType.CONSIGNEE, deleted_at__isnull=True @@ -246,10 +241,9 @@ def test_delete_consignee_document_success( self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) delete_s3_function.assert_called_once() - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_consignee_success(self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object): + def test_delete_consignee_success(self, delete_s3_function, mock_virus_scan): """ Given a standard draft has been created And the draft contains a consignee user @@ -257,7 +251,6 @@ def test_delete_consignee_success(self, delete_s3_function, mock_virus_scan, moc When there is an attempt to delete the consignee Then 200 OK is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False consignee = PartyOnApplication.objects.get( application=self.draft, party__type=PartyType.CONSIGNEE, deleted_at__isnull=True diff --git a/api/applications/tests/test_documents.py b/api/applications/tests/test_documents.py index d604fb729e..909a041e00 100644 --- a/api/applications/tests/test_documents.py +++ b/api/applications/tests/test_documents.py @@ -5,21 +5,22 @@ from api.audit_trail.enums import AuditType from api.audit_trail.models import Audit from test_helpers.clients import DataTestClient +from test_helpers.file_uploads import upload_file class ApplicationDocumentViewTests(DataTestClient): - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.libraries.s3_operations.upload_bytes_file") - def test_audit_trail_create(self, upload_bytes_func, mock_virus_scan, mock_s3_operations_get_object): + def test_audit_trail_create(self, upload_bytes_func, mock_virus_scan): mock_virus_scan.return_value = False application = self.create_draft_standard_application(organisation=self.organisation, user=self.exporter_user) url = reverse("applications:application_documents", kwargs={"pk": application.pk}) + s3_key = "section5_20210223145814.png" data = { "name": "section5.png", - "s3_key": "section5_20210223145814.png", + "s3_key": s3_key, "size": 1, "document_on_organisation": { "expiry_date": "2222-01-01", @@ -27,7 +28,7 @@ def test_audit_trail_create(self, upload_bytes_func, mock_virus_scan, mock_s3_op "document_type": "section-five-certificate", }, } - mock_s3_operations_get_object.return_value = data + upload_file(s3_key) response = self.client.post(url, data, **self.exporter_headers) diff --git a/api/applications/tests/test_end_user.py b/api/applications/tests/test_end_user.py index 219298ed38..63db0af55b 100644 --- a/api/applications/tests/test_end_user.py +++ b/api/applications/tests/test_end_user.py @@ -12,6 +12,7 @@ from api.parties.models import PartyDocument from api.staticdata.countries.helpers import get_country from test_helpers.clients import DataTestClient +from test_helpers.file_uploads import upload_file class EndUserOnDraftTests(DataTestClient): @@ -33,11 +34,13 @@ def setUp(self): "applications:party_document", kwargs={"pk": self.draft.id, "party_pk": self.draft.end_user.party.id} ) + s3_key = "s3_keykey.pdf" self.new_document_data = { "name": "updated_document_name.pdf", - "s3_key": "s3_keykey.pdf", + "s3_key": s3_key, "size": 123456, } + upload_file(s3_key) @parameterized.expand([SubType.GOVERNMENT, SubType.COMMERCIAL, SubType.OTHER]) def test_set_end_user_on_draft_standard_application_successful(self, data_type): @@ -174,9 +177,8 @@ def test_delete_end_user_on_standard_application_when_application_has_no_end_use self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_get_end_user_document_successful(self, mock_virus_scan, mock_s3_operations_get_object): + def test_get_end_user_document_successful(self, mock_virus_scan): """ Given a standard draft has been created And the draft contains an end user @@ -184,7 +186,6 @@ def test_get_end_user_document_successful(self, mock_virus_scan, mock_s3_operati When the document is retrieved Then the data in the document is the same as the data in the attached end user document """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False response = self.client.get(self.document_url, **self.exporter_headers) response_data = response.json()["document"] @@ -210,16 +211,14 @@ def test_get_document_when_no_end_user_exists_failure(self): self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_post_document_when_no_end_user_exists_failure(self, mock_virus_scan, mock_s3_operations_get_object): + def test_post_document_when_no_end_user_exists_failure(self, mock_virus_scan): """ Given a standard draft has been created And the draft does not contain an end user When there is an attempt to submit a document Then a 400 BAD REQUEST is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False PartyOnApplication.objects.get( application=self.draft, party__type=PartyType.END_USER, deleted_at__isnull=True @@ -263,9 +262,8 @@ def test_get_end_user_document_when_document_does_not_exist_failure(self): self.assertEqual(status.HTTP_404_NOT_FOUND, response.status_code) self.assertEqual(None, response.json()["document"]) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_post_end_user_document_success(self, mock_virus_scan, mock_s3_operations_get_object): + def test_post_end_user_document_success(self, mock_virus_scan): """ Given a standard draft has been created And the draft contains an end user @@ -273,7 +271,6 @@ def test_post_end_user_document_success(self, mock_virus_scan, mock_s3_operation When a document is submitted Then a 201 CREATED is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False party = PartyOnApplication.objects.get( application=self.draft, deleted_at__isnull=True, party__type=PartyType.END_USER @@ -285,11 +282,8 @@ def test_post_end_user_document_success(self, mock_virus_scan, mock_s3_operation self.assertEqual(response.status_code, status.HTTP_201_CREATED) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_post_end_user_document_when_a_document_already_exists_success( - self, mock_virus_scan, mock_s3_operations_get_object - ): + def test_post_end_user_document_when_a_document_already_exists_success(self, mock_virus_scan): """ Given a standard draft has been created And the draft contains an end user @@ -297,7 +291,6 @@ def test_post_end_user_document_when_a_document_already_exists_success( When there is an attempt to post a document Then a 400 BAD REQUEST is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False end_user = PartyOnApplication.objects.get( application=self.draft, party__type=PartyType.END_USER, deleted_at__isnull=True @@ -311,10 +304,9 @@ def test_post_end_user_document_when_a_document_already_exists_success( self.assertEqual(party_documents.count(), 1) self.assertEqual(party_documents.first().name, "updated_document_name.pdf") - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_end_user_document_success(self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object): + def test_delete_end_user_document_success(self, delete_s3_function, mock_virus_scan): """ Given a standard draft has been created And the draft contains an end user @@ -322,17 +314,15 @@ def test_delete_end_user_document_success(self, delete_s3_function, mock_virus_s When there is an attempt to delete the document Then 204 NO CONTENT is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False response = self.client.delete(self.document_url, **self.exporter_headers) self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) delete_s3_function.assert_called_once() - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_end_user_success(self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object): + def test_delete_end_user_success(self, delete_s3_function, mock_virus_scan): """ Given a standard draft has been created And the draft contains an end user @@ -340,7 +330,6 @@ def test_delete_end_user_success(self, delete_s3_function, mock_virus_scan, mock When there is an attempt to delete the end user Then 204 NO CONTENT is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False end_user = PartyOnApplication.objects.get( application=self.draft, party__type=PartyType.END_USER, deleted_at__isnull=True diff --git a/api/applications/tests/test_goods.py b/api/applications/tests/test_goods.py index 4a6daa537e..3130107120 100644 --- a/api/applications/tests/test_goods.py +++ b/api/applications/tests/test_goods.py @@ -5,13 +5,13 @@ from api.audit_trail.models import Audit from api.goods.tests.factories import GoodFactory from test_helpers.clients import DataTestClient +from test_helpers.file_uploads import upload_file class ApplicationGoodOnApplicationDocumentViewTests(DataTestClient): - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.libraries.s3_operations.upload_bytes_file") - def test_audit_trail_create(self, upload_bytes_func, mock_virus_scan, mock_s3_operations_get_object): + def test_audit_trail_create(self, upload_bytes_func, mock_virus_scan): mock_virus_scan.return_value = False application = self.create_draft_standard_application(organisation=self.organisation, user=self.exporter_user) good = GoodFactory(organisation=self.organisation) @@ -24,9 +24,10 @@ def test_audit_trail_create(self, upload_bytes_func, mock_virus_scan, mock_s3_op }, ) + s3_key = "section5_20210223145814.png" data = { "name": "section5.png", - "s3_key": "section5_20210223145814.png", + "s3_key": s3_key, "size": 1, "document_on_organisation": { "expiry_date": "2222-01-01", @@ -34,7 +35,7 @@ def test_audit_trail_create(self, upload_bytes_func, mock_virus_scan, mock_s3_op "document_type": "section-five-certificate", }, } - mock_s3_operations_get_object.return_value = data + upload_file(s3_key) response = self.client.post(url, data, **self.exporter_headers) self.assertEqual(response.status_code, 201, response.json()) diff --git a/api/applications/tests/test_goods_type_on_application.py b/api/applications/tests/test_goods_type_on_application.py index 8c89e7ef30..276b581cd8 100644 --- a/api/applications/tests/test_goods_type_on_application.py +++ b/api/applications/tests/test_goods_type_on_application.py @@ -8,6 +8,7 @@ from api.staticdata.control_list_entries.helpers import get_control_list_entry from api.staticdata.control_list_entries.models import ControlListEntry from test_helpers.clients import DataTestClient +from test_helpers.file_uploads import upload_file class GoodsTypeOnApplicationTests(DataTestClient): @@ -33,11 +34,13 @@ def setUp(self): "goods_type_pk": GoodsType.objects.get(application=self.hmrc_query).id, }, ) + s3_key = "s3_keykey.pdf" self.new_document_data = { "name": "document_name.pdf", - "s3_key": "s3_keykey.pdf", + "s3_key": s3_key, "size": 123456, } + upload_file(s3_key) def test_create_goodstype_on_open_application_as_exporter_user_success(self): response = self.client.post(self.url, self.data, **self.exporter_headers) @@ -109,9 +112,8 @@ def test_remove_goodstype_from_open_application_as_exporter_user_success(self): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(GoodsType.objects.all().count(), initial_goods_types_count - 1) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_post_goods_type_document_success(self, mock_virus_scan, mock_s3_operations_get_object): + def test_post_goods_type_document_success(self, mock_virus_scan): """ Given a draft HMRC query has been created And the draft contains a goods type @@ -119,7 +121,6 @@ def test_post_goods_type_document_success(self, mock_virus_scan, mock_s3_operati When a document is submitted Then a 201 CREATED is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False GoodsTypeDocument.objects.get(goods_type__application=self.hmrc_query).delete() count = GoodsTypeDocument.objects.count() @@ -129,9 +130,8 @@ def test_post_goods_type_document_success(self, mock_virus_scan, mock_s3_operati self.assertEqual(response.status_code, status.HTTP_201_CREATED) self.assertEqual(count + 1, GoodsTypeDocument.objects.count()) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_get_goods_type_document_success(self, mock_virus_scan, mock_s3_operations_get_object): + def test_get_goods_type_document_success(self, mock_virus_scan): """ Given a draft HMRC query has been created And the draft contains a goods type @@ -139,7 +139,6 @@ def test_get_goods_type_document_success(self, mock_virus_scan, mock_s3_operatio When the document is retrieved Then the data in the document is the same as the data in the attached goods party document """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False response = self.client.get(self.document_url, **self.hmrc_exporter_headers) response_data = response.json()["document"] @@ -148,12 +147,9 @@ def test_get_goods_type_document_success(self, mock_virus_scan, mock_s3_operatio self.assertEqual(response_data["s3_key"], self.new_document_data["s3_key"]) self.assertEqual(response_data["size"], self.new_document_data["size"]) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_goods_type_document_success( - self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object - ): + def test_delete_goods_type_document_success(self, delete_s3_function, mock_virus_scan): """ Given a draft HMRC query has been created And the draft contains a goods type @@ -161,17 +157,15 @@ def test_delete_goods_type_document_success( When there is an attempt to delete the document Then 204 NO CONTENT is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False response = self.client.delete(self.document_url, **self.hmrc_exporter_headers) self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) delete_s3_function.assert_called_once() - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_goods_type_success(self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object): + def test_delete_goods_type_success(self, delete_s3_function, mock_virus_scan): """ Given a draft HMRC query has been created And the draft contains a goods type @@ -179,7 +173,6 @@ def test_delete_goods_type_success(self, delete_s3_function, mock_virus_scan, mo When there is an attempt to delete goods type Then 200 OK is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False url = reverse( "applications:application_goodstype", diff --git a/api/applications/tests/test_third_parties.py b/api/applications/tests/test_third_parties.py index 484c609940..af2bed3d15 100644 --- a/api/applications/tests/test_third_parties.py +++ b/api/applications/tests/test_third_parties.py @@ -11,6 +11,7 @@ from api.parties.enums import PartyType, PartyRole, SubType from api.parties.models import Party, PartyDocument from test_helpers.clients import DataTestClient +from test_helpers.file_uploads import upload_file class ThirdPartiesOnDraft(DataTestClient): @@ -24,11 +25,13 @@ def setUp(self): "applications:party_document", kwargs={"pk": self.draft.id, "party_pk": self.draft.third_parties.first().party.id}, ) + s3_key = "s3_keykey.pdf" self.new_document_data = { "name": "document_name.pdf", - "s3_key": "s3_keykey.pdf", + "s3_key": s3_key, "size": 123456, } + upload_file(s3_key) @pytest.mark.only def test_set_multiple_third_parties_on_draft_successful(self): @@ -170,9 +173,8 @@ def test_delete_third_party_on_standard_application_when_application_has_no_thir self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_post_third_party_document_success(self, mock_virus_scan, mock_s3_operations_get_object): + def test_post_third_party_document_success(self, mock_virus_scan): """ Given a standard draft has been created And the draft contains a third party @@ -180,7 +182,6 @@ def test_post_third_party_document_success(self, mock_virus_scan, mock_s3_operat When a document is submitted Then a 201 CREATED is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False PartyDocument.objects.filter(party=self.draft.third_parties.all().first().party).delete() @@ -188,9 +189,8 @@ def test_post_third_party_document_success(self, mock_virus_scan, mock_s3_operat self.assertEqual(response.status_code, status.HTTP_201_CREATED) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_get_third_party_document_success(self, mock_virus_scan, mock_s3_operations_get_object): + def test_get_third_party_document_success(self, mock_virus_scan): """ Given a standard draft has been created And the draft contains a third party @@ -198,7 +198,6 @@ def test_get_third_party_document_success(self, mock_virus_scan, mock_s3_operati When the document is retrieved Then the data in the document is the same as the data in the attached third party document """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False response = self.client.get(self.document_url, **self.exporter_headers) @@ -209,12 +208,9 @@ def test_get_third_party_document_success(self, mock_virus_scan, mock_s3_operati self.assertEqual(response_data["s3_key"], expected["s3_key"]) self.assertEqual(response_data["size"], expected["size"]) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_third_party_document_success( - self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object - ): + def test_delete_third_party_document_success(self, delete_s3_function, mock_virus_scan): """ Given a standard draft has been created And the draft contains a third party @@ -222,7 +218,6 @@ def test_delete_third_party_document_success( When there is an attempt to delete the document Then 200 NO CONTENT is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False response = self.client.delete(self.document_url, **self.exporter_headers) @@ -230,10 +225,9 @@ def test_delete_third_party_document_success( self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) delete_s3_function.assert_called_once() - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_third_party_success(self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object): + def test_delete_third_party_success(self, delete_s3_function, mock_virus_scan): """ Given a standard draft has been created And the draft contains a third party @@ -241,7 +235,6 @@ def test_delete_third_party_success(self, delete_s3_function, mock_virus_scan, m When there is an attempt to delete third party Then 200 OK """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False self.assertEqual(self.draft.third_parties.count(), 1) diff --git a/api/applications/tests/test_ultimate_end_users.py b/api/applications/tests/test_ultimate_end_users.py index e2151321f9..8b6e2116a3 100644 --- a/api/applications/tests/test_ultimate_end_users.py +++ b/api/applications/tests/test_ultimate_end_users.py @@ -11,6 +11,7 @@ from api.parties.models import Party from api.parties.models import PartyDocument from test_helpers.clients import DataTestClient +from test_helpers.file_uploads import upload_file class UltimateEndUsersOnDraft(DataTestClient): @@ -28,6 +29,7 @@ def setUp(self): "s3_key": "s3_keykey.pdf", "size": 123456, } + upload_file("s3_keykey.pdf") def test_set_multiple_ultimate_end_users_on_draft_successful(self): PartyOnApplication.objects.filter(application=self.draft, party__type=PartyType.ULTIMATE_END_USER).delete() @@ -140,9 +142,8 @@ def test_delete_ueu_on_standard_application_when_application_has_no_ueu_failure( self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_post_ultimate_end_user_document_success(self, mock_virus_scan, mock_s3_operations_get_object): + def test_post_ultimate_end_user_document_success(self, mock_virus_scan): """ Given a standard draft has been created And the draft contains an ultimate end user @@ -150,7 +151,6 @@ def test_post_ultimate_end_user_document_success(self, mock_virus_scan, mock_s3_ When a document is submitted Then a 201 CREATED is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False PartyDocument.objects.filter(party=self.draft.ultimate_end_users.first().party).delete() @@ -158,9 +158,8 @@ def test_post_ultimate_end_user_document_success(self, mock_virus_scan, mock_s3_ self.assertEqual(response.status_code, status.HTTP_201_CREATED) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_get_ultimate_end_user_document_success(self, mock_virus_scan, mock_s3_operations_get_object): + def test_get_ultimate_end_user_document_success(self, mock_virus_scan): """ Given a standard draft has been created And the draft contains an ultimate end user @@ -168,7 +167,6 @@ def test_get_ultimate_end_user_document_success(self, mock_virus_scan, mock_s3_o When the document is retrieved Then the data in the document is the same as the data in the attached ultimate end user document """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False response = self.client.get(self.document_url, **self.exporter_headers) response_data = response.json()["document"] @@ -178,12 +176,9 @@ def test_get_ultimate_end_user_document_success(self, mock_virus_scan, mock_s3_o self.assertEqual(response_data["s3_key"], expected["s3_key"]) self.assertEqual(response_data["size"], expected["size"]) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_ultimate_end_user_document_success( - self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object - ): + def test_delete_ultimate_end_user_document_success(self, delete_s3_function, mock_virus_scan): """ Given a standard draft has been created And the draft contains an ultimate end user @@ -191,17 +186,15 @@ def test_delete_ultimate_end_user_document_success( When there is an attempt to delete the document Then 204 NO CONTENT is returned """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False response = self.client.delete(self.document_url, **self.exporter_headers) self.assertEqual(response.status_code, status.HTTP_204_NO_CONTENT) delete_s3_function.assert_called_once() - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") @mock.patch("api.documents.models.Document.delete_s3") - def test_delete_ultimate_end_user_success(self, delete_s3_function, mock_virus_scan, mock_s3_operations_get_object): + def test_delete_ultimate_end_user_success(self, delete_s3_function, mock_virus_scan): """ Given a standard draft has been created And the draft contains an ultimate end user @@ -209,7 +202,6 @@ def test_delete_ultimate_end_user_success(self, delete_s3_function, mock_virus_s When there is an attempt to delete the document Then 200 OK """ - mock_s3_operations_get_object.return_value = self.new_document_data mock_virus_scan.return_value = False self.assertEqual(self.draft.ultimate_end_users.count(), 1) party_on_application = self.draft.ultimate_end_users.first() diff --git a/api/cases/tests/test_case_documents.py b/api/cases/tests/test_case_documents.py index 0ebea43bf5..495392686b 100644 --- a/api/cases/tests/test_case_documents.py +++ b/api/cases/tests/test_case_documents.py @@ -9,8 +9,7 @@ from lite_content.lite_api.strings import Documents from test_helpers.clients import DataTestClient - -from api.documents.libraries.s3_operations import init_s3_client +from test_helpers.file_uploads import upload_file class CaseDocumentsTests(DataTestClient): @@ -38,13 +37,7 @@ def setUp(self): self.case = self.submit_application(self.standard_application) self.file = self.create_case_document(self.case, self.gov_user, "Test") self.path = "cases:document_download" - - s3 = init_s3_client()["processed"] - s3.put_object( - Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], - Key=self.file.s3_key, - Body=b"test", - ) + upload_file(self.file.s3_key) def test_download_case_document_success(self): url = reverse(self.path, kwargs={"case_pk": self.case.id, "document_pk": self.file.id}) diff --git a/api/cases/tests/test_case_ecju_queries.py b/api/cases/tests/test_case_ecju_queries.py index 07d40d6137..d11d66f914 100644 --- a/api/cases/tests/test_case_ecju_queries.py +++ b/api/cases/tests/test_case_ecju_queries.py @@ -23,6 +23,7 @@ from api.staticdata.statuses.libraries.get_case_status import get_case_status_by_status from test_helpers.clients import DataTestClient from api.users.tests.factories import ExporterUserFactory +from test_helpers.file_uploads import upload_file faker = Faker() @@ -424,20 +425,20 @@ def test_query_create(self, mock_notify): class ECJUQueriesResponseTests(DataTestClient): - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def add_query_document(self, case_id, query_id, expected_status, mock_virus_scan, mock_s3_operations_get_object): + def add_query_document(self, case_id, query_id, expected_status, mock_virus_scan): mock_virus_scan.return_value = False url = reverse("cases:case_ecju_query_add_document", kwargs={"pk": case_id, "query_pk": query_id}) file_name = faker.file_name() + s3_key = f"{file_name}_{faker.uuid4()}" data = { "name": file_name, - "s3_key": f"{file_name}_{faker.uuid4()}", + "s3_key": s3_key, "description": faker.text(), "size": 1 << 10, "virus_scanned_at": timezone.now(), } - mock_s3_operations_get_object.return_value = data + upload_file(s3_key) response = self.client.post(url, data, **self.exporter_headers) self.assertEqual(response.status_code, expected_status) return response.json() diff --git a/api/conftest.py b/api/conftest.py index 1d593b51d5..9194eca462 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -132,7 +132,7 @@ def setup(settings): settings.HAWK_AUTHENTICATION_ENABLED = False -@pytest.fixture(autouse=True) +@pytest.fixture(autouse=True, scope="session") def mock_aws_calls(): with mock_aws(): clients = init_s3_client() diff --git a/api/documents/tests/test_document_stream.py b/api/documents/tests/test_document_stream.py index a5463a86a9..9a0ef5a60a 100644 --- a/api/documents/tests/test_document_stream.py +++ b/api/documents/tests/test_document_stream.py @@ -6,20 +6,15 @@ from django.urls import reverse from test_helpers.clients import DataTestClient +from test_helpers.file_uploads import upload_file from api.conf import settings -from api.documents.libraries.s3_operations import init_s3_client class DocumentStream(DataTestClient): def setUp(self): super().setUp() - s3 = init_s3_client()["processed"] - s3.put_object( - Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], - Key="thisisakey", - Body=b"test", - ) + upload_file("thisisakey") def test_document_stream_as_caseworker(self): # given there is a case document diff --git a/api/organisations/tests/test_documents.py b/api/organisations/tests/test_documents.py index 3e73a50dc5..2f54026f77 100644 --- a/api/organisations/tests/test_documents.py +++ b/api/organisations/tests/test_documents.py @@ -7,7 +7,7 @@ from api.organisations.enums import OrganisationDocumentType from api.organisations.models import DocumentOnOrganisation from test_helpers.clients import DataTestClient -from api.documents.libraries.s3_operations import init_s3_client +from test_helpers.file_uploads import upload_file class OrganisationDocumentViewTests(DataTestClient): @@ -21,12 +21,7 @@ def setUp(self): def create_document_on_organisation(self, name): url = reverse("organisations:documents", kwargs={"pk": self.organisation.pk}) - s3 = init_s3_client()["staged"] - s3.put_object( - Bucket=settings.FILE_UPLOAD_STAGED_BUCKET["AWS_STORAGE_BUCKET_NAME"], - Key=name, - Body=b"test", - ) + upload_file(name) data = { "document": {"name": name, "s3_key": name, "size": 476}, "expiry_date": "2026-01-01", @@ -35,10 +30,8 @@ def create_document_on_organisation(self, name): } return self.client.post(url, data, **self.exporter_headers) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_create_organisation_document(self, mock_virus_scan, mock_s3_operations_get_object): - mock_s3_operations_get_object.return_value = self.document_data + def test_create_organisation_document(self, mock_virus_scan): mock_virus_scan.return_value = False response = self.create_document_on_organisation("some-document") @@ -55,10 +48,8 @@ def test_create_organisation_document(self, mock_virus_scan, mock_s3_operations_ self.assertEqual(instance.document_type, OrganisationDocumentType.FIREARM_SECTION_FIVE) self.assertEqual(instance.organisation, self.organisation) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_list_organisation_documents(self, mock_virus_scan, mock_s3_operations_get_object): - mock_s3_operations_get_object.return_value = self.document_data + def test_list_organisation_documents(self, mock_virus_scan): mock_virus_scan.return_value = False self.assertEqual(self.create_document_on_organisation("some-document-one").status_code, 201) self.assertEqual(self.create_document_on_organisation("some-document-two").status_code, 201) @@ -108,10 +99,8 @@ def test_retrieve_organisation_documents(self, mock_virus_scan): }, ) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_delete_organisation_documents(self, mock_virus_scan, mock_s3_operations_get_object): - mock_s3_operations_get_object.return_value = self.document_data + def test_delete_organisation_documents(self, mock_virus_scan): mock_virus_scan.return_value = False response = self.create_document_on_organisation("some-document-one") self.assertEqual(response.status_code, 201) @@ -131,10 +120,8 @@ def test_delete_organisation_documents(self, mock_virus_scan, mock_s3_operations with self.assertRaises(DocumentOnOrganisation.DoesNotExist): DocumentOnOrganisation.objects.get(pk=document_on_application_pk) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_update_organisation_documents(self, mock_virus_scan, mock_s3_operations_get_object): - mock_s3_operations_get_object.return_value = self.document_data + def test_update_organisation_documents(self, mock_virus_scan): mock_virus_scan.return_value = False response = self.create_document_on_organisation("some-document-one") self.assertEqual(response.status_code, 201) diff --git a/api/test_more_documents.py b/api/test_more_documents.py index 1b3dc1e4b9..fc7cc3d0e3 100644 --- a/api/test_more_documents.py +++ b/api/test_more_documents.py @@ -9,6 +9,7 @@ from api.applications.libraries.case_status_helpers import get_case_statuses from api.staticdata.statuses.libraries.get_case_status import get_case_status_by_status from test_helpers.clients import DataTestClient +from test_helpers.file_uploads import upload_file class DraftDocumentTests(DataTestClient): @@ -35,12 +36,11 @@ def setUp(self): "size": 476, "description": "banana cake 2", } + upload_file(self.test_filename) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_upload_document_on_unsubmitted_application(self, mock_virus_scan, mock_s3_operations_get_object): + def test_upload_document_on_unsubmitted_application(self, mock_virus_scan): """Test success in adding a document to an unsubmitted application.""" - mock_s3_operations_get_object.return_value = self.data mock_virus_scan.return_value = False self.client.post(self.url_draft, data=self.data, **self.exporter_headers) @@ -58,11 +58,9 @@ def test_upload_document_on_unsubmitted_application(self, mock_virus_scan, mock_ self.assertEqual(len(response_data), 2) self.assertTrue(self.data in response_data) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_upload_multiple_documents_on_unsubmitted_application(self, mock_virus_scan, mock_s3_operations_get_object): + def test_upload_multiple_documents_on_unsubmitted_application(self, mock_virus_scan): """Test success in adding multiple documents to an unsubmitted application.""" - mock_s3_operations_get_object.return_value = self.data mock_virus_scan.return_value = False data = [self.data, self.data2] self.client.post(self.url_draft, data=self.data, **self.exporter_headers) @@ -119,12 +117,8 @@ def test_get_individual_draft_document(self): self.assertEqual(response.json()["document"]["size"], application_document.size) @parameterized.expand(get_case_statuses(read_only=False)) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") - def test_add_document_when_application_in_editable_state_success( - self, editable_status, mock_virus_scan, mock_s3_operations_get_object - ): - mock_s3_operations_get_object.return_value = self.data + def test_add_document_when_application_in_editable_state_success(self, editable_status, mock_virus_scan): mock_virus_scan.return_value = False application = self.create_draft_standard_application(self.organisation) application.status = get_case_status_by_status(editable_status) @@ -137,13 +131,11 @@ def test_add_document_when_application_in_editable_state_success( self.assertEqual(application.applicationdocument_set.count(), 2) @parameterized.expand(get_case_statuses(read_only=False)) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.models.Document.delete_s3") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") def test_delete_document_when_application_in_editable_state_success( - self, editable_status, mock_virus_scan, mock_delete_s3, mock_s3_operations_get_object + self, editable_status, mock_virus_scan, mock_delete_s3 ): - mock_s3_operations_get_object.return_value = self.data mock_virus_scan.return_value = False application = self.create_draft_standard_application(self.organisation) application.status = get_case_status_by_status(editable_status) @@ -175,13 +167,11 @@ def test_add_document_when_application_in_read_only_state_failure(self, read_onl self.assertEqual(application.applicationdocument_set.count(), 1) @parameterized.expand(get_case_statuses(read_only=True)) - @mock.patch("api.documents.libraries.s3_operations.get_object") @mock.patch("api.documents.models.Document.delete_s3") @mock.patch("api.documents.libraries.av_operations.scan_file_for_viruses") def test_delete_document_when_application_in_read_only_state_failure( - self, read_only_status, mock_virus_scan, mock_delete_s3, mock_s3_operations_get_object + self, read_only_status, mock_virus_scan, mock_delete_s3 ): - mock_s3_operations_get_object.return_value = self.data mock_virus_scan.return_value = False application = self.create_draft_standard_application(self.organisation) application.status = get_case_status_by_status(read_only_status) diff --git a/test_helpers/file_uploads.py b/test_helpers/file_uploads.py new file mode 100644 index 0000000000..5b466a4007 --- /dev/null +++ b/test_helpers/file_uploads.py @@ -0,0 +1,18 @@ +from django.conf import settings + +from api.documents.libraries.s3_operations import init_s3_client + + +def upload_file(s3_key, content=b"test"): + """ + Emulates how the frontend does file uploads by uploading to the processed + S3 bucket. + TODO: When we switch the frontend to uploading to the staged bucket instead, + we should change that here. + """ + s3 = init_s3_client()["processed"] + s3.put_object( + Bucket=settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_STORAGE_BUCKET_NAME"], + Key=s3_key, + Body=content, + ) From 428dcbc964ab3182fb9a48f3e013f255522173e7 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Wed, 7 Feb 2024 11:26:01 +0000 Subject: [PATCH 7/8] Fix s3_operations tests --- .../libraries/tests/test_s3_operations.py | 75 ++++++++----------- 1 file changed, 31 insertions(+), 44 deletions(-) diff --git a/api/documents/libraries/tests/test_s3_operations.py b/api/documents/libraries/tests/test_s3_operations.py index 9e1e0dff0a..341b200890 100644 --- a/api/documents/libraries/tests/test_s3_operations.py +++ b/api/documents/libraries/tests/test_s3_operations.py @@ -1,5 +1,6 @@ from contextlib import contextmanager from unittest.mock import Mock, patch +from uuid import uuid4 from moto import mock_aws @@ -128,70 +129,56 @@ def test_get_object(self, mock_processed_client, mock_staged_client): mock_staged_client.delete_object.assert_called_with(Bucket="staged", Key="s3-key") -@contextmanager -def _create_bucket(s3): - s3.create_bucket( - Bucket=TEST_AWS_BUCKET_NAME, - CreateBucketConfiguration={ - "LocationConstraint": settings.FILE_UPLOAD_PROCESSED_BUCKET["AWS_REGION"], - }, - ) - yield - - def get_s3_client(): return init_s3_client()["processed"] -@mock_aws class S3OperationsDeleteFileTests(SimpleTestCase): def test_delete_file(self): s3 = get_s3_client() - with _create_bucket(s3): - s3.put_object( - Bucket=TEST_AWS_BUCKET_NAME, - Key="s3-key", - Body=b"test", - ) + s3_key = f"s3-key-{{uuid4()}}" + s3.put_object( + Bucket=TEST_AWS_BUCKET_NAME, + Key=s3_key, + Body=b"test", + ) - delete_file("document-id", "s3-key") + delete_file("document-id", s3_key) - objs = s3.list_objects(Bucket=TEST_AWS_BUCKET_NAME) - keys = [o["Key"] for o in objs.get("Contents", [])] - self.assertNotIn("s3-key", keys) + objs = s3.list_objects(Bucket=TEST_AWS_BUCKET_NAME) + keys = [o["Key"] for o in objs.get("Contents", [])] + self.assertNotIn(s3_key, keys) -@mock_aws class S3OperationsUploadBytesFileTests(SimpleTestCase): def test_upload_bytes_file(self): s3 = get_s3_client() - with _create_bucket(s3): - upload_bytes_file(b"test", "s3-key") + s3_key = f"s3-key-{{uuid4()}}" + upload_bytes_file(b"test", s3_key) - obj = s3.get_object( - Bucket=TEST_AWS_BUCKET_NAME, - Key="s3-key", - ) - self.assertEqual(obj["Body"].read(), b"test") + obj = s3.get_object( + Bucket=TEST_AWS_BUCKET_NAME, + Key=s3_key, + ) + self.assertEqual(obj["Body"].read(), b"test") -@mock_aws class S3OperationsDocumentDownloadStreamTests(SimpleTestCase): def test_document_download_stream(self): s3 = get_s3_client() - with _create_bucket(s3): - s3.put_object( - Bucket=TEST_AWS_BUCKET_NAME, - Key="s3-key", - Body=b"test", - ) - - mock_document = Mock() - mock_document.id = "document-id" - mock_document.s3_key = "s3-key" - mock_document.name = "test.doc" - - response = document_download_stream(mock_document) + s3_key = f"s3-key-{{uuid4()}}" + s3.put_object( + Bucket=TEST_AWS_BUCKET_NAME, + Key=s3_key, + Body=b"test", + ) + + mock_document = Mock() + mock_document.id = "document-id" + mock_document.s3_key = s3_key + mock_document.name = "test.doc" + + response = document_download_stream(mock_document) self.assertIsInstance(response, FileResponse) self.assertEqual(response.status_code, 200) From e28005fc1aaf326eba6ac0e86f4e1b058e4266a3 Mon Sep 17 00:00:00 2001 From: Brendan Smith Date: Wed, 7 Feb 2024 11:33:24 +0000 Subject: [PATCH 8/8] Fix lint complaint --- api/conftest.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/api/conftest.py b/api/conftest.py index 9194eca462..a6f08ce347 100644 --- a/api/conftest.py +++ b/api/conftest.py @@ -1,10 +1,7 @@ -import os - from django.core.management import call_command from django.db.migrations.executor import MigrationExecutor from django import db -from celery import Celery from moto import mock_aws import re