Refactor to split s3_client.py into multiple files

Separated s3_client.py into 3 files - for logos, CSV files and the MOU.
This helps to keep things clearer now that we need to add lots more logo
functions for letters.

tests/app/main/views/test_agreement.py

@@ -78,7 +78,7 @@ def test_downloading_agreement(
     expected_file_served,
 ):
     mock_get_s3_object = mocker.patch(
-        'app.main.s3_client.get_s3_object',
+        'app.s3_client.s3_mou_client.get_s3_object',
         return_value=_MockS3Object(b'foo')
     )
     user = active_user_with_permissions(fake_uuid)
@@ -100,7 +100,7 @@ def test_agreement_cant_be_downloaded_unknown_crown_status(
     fake_uuid,
 ):
     mock_get_s3_object = mocker.patch(
-        'app.main.s3_client.get_s3_object',
+        'app.s3_client.s3_mou_client.get_s3_object',
         return_value=_MockS3Object()
     )
     user = active_user_with_permissions(fake_uuid)
@@ -116,7 +116,7 @@ def test_agreement_requires_login(
     mocker,
 ):
     mock_get_s3_object = mocker.patch(
-        'app.main.s3_client.get_s3_object',
+        'app.s3_client.s3_mou_client.get_s3_object',
         return_value=_MockS3Object()
     )
     response = client.get(url_for('main.download_agreement'))
@@ -142,7 +142,7 @@ def test_show_public_agreement_page(
     expected_status,
 ):
     mocker.patch(
-        'app.main.s3_client.get_s3_object',
+        'app.s3_client.s3_mou_client.get_s3_object',
         return_value=_MockS3Object()
     )
     response = client.get(url_for(

tests/app/main/views/test_email_branding.py

@@ -6,7 +6,7 @@ from bs4 import BeautifulSoup
 from flask import url_for
 from notifications_python_client.errors import HTTPError
 
-from app.main.s3_client import LOGO_LOCATION_STRUCTURE, TEMP_TAG
+from app.s3_client.s3_logo_client import LOGO_LOCATION_STRUCTURE, TEMP_TAG
 from tests.conftest import (
     mock_get_email_branding,
     normalize_spaces,

tests/app/s3_client/test_s3_csv_client.py

@@ -0,0 +1,21 @@
+from unittest.mock import Mock
+
+from app.s3_client.s3_csv_client import set_metadata_on_csv_upload
+
+
+def test_sets_metadata(client, mocker):
+    mocked_s3_object = Mock()
+    mocked_get_s3_object = mocker.patch(
+        'app.s3_client.s3_csv_client.get_csv_upload',
+        return_value=mocked_s3_object,
+    )
+
+    set_metadata_on_csv_upload('1234', '5678', foo='bar', baz=True)
+
+    mocked_get_s3_object.assert_called_once_with('1234', '5678')
+    mocked_s3_object.copy_from.assert_called_once_with(
+        CopySource='test-notifications-csv-upload/service-1234-notify/5678.csv',
+        Metadata={'baz': 'True', 'foo': 'bar'},
+        MetadataDirective='REPLACE',
+        ServerSideEncryption='AES256',
+    )

tests/app/s3_client/test_s3_logo_client.py

@@ -1,16 +1,15 @@
 from collections import namedtuple
-from unittest.mock import Mock, call
+from unittest.mock import call
 
 import pytest
 
-from app.main.s3_client import (
+from app.s3_client.s3_logo_client import (
     LOGO_LOCATION_STRUCTURE,
     TEMP_TAG,
     delete_temp_file,
     delete_temp_files_created_by,
     permanent_logo_name,
     persist_logo,
-    set_metadata_on_csv_upload,
     upload_logo,
 )
 
@@ -30,7 +29,7 @@ def upload_filename(fake_uuid):
 def test_upload_logo_calls_correct_args(client, mocker, fake_uuid, upload_filename):
     mocker.patch('uuid.uuid4', return_value=upload_id)
     mocker.patch.dict('flask.current_app.config', {'LOGO_UPLOAD_BUCKET_NAME': bucket})
-    mocked_s3_upload = mocker.patch('app.main.s3_client.utils_s3upload')
+    mocked_s3_upload = mocker.patch('app.s3_client.s3_logo_client.utils_s3upload')
 
     upload_logo(filename=filename, user_id=fake_uuid, filedata=data, region=region)
 
@@ -44,8 +43,8 @@ def test_upload_logo_calls_correct_args(client, mocker, fake_uuid, upload_filena
 
 def test_persist_logo(client, mocker, fake_uuid, upload_filename):
     mocker.patch.dict('flask.current_app.config', {'LOGO_UPLOAD_BUCKET_NAME': bucket})
-    mocked_get_s3_object = mocker.patch('app.main.s3_client.get_s3_object')
-    mocked_delete_s3_object = mocker.patch('app.main.s3_client.delete_s3_object')
+    mocked_get_s3_object = mocker.patch('app.s3_client.s3_logo_client.get_s3_object')
+    mocked_delete_s3_object = mocker.patch('app.s3_client.s3_logo_client.delete_s3_object')
 
     new_filename = permanent_logo_name(upload_filename, fake_uuid)
 
@@ -59,8 +58,8 @@ def test_persist_logo_returns_if_not_temp(client, mocker, fake_uuid):
     filename = 'logo.png'
     persist_logo(filename, filename)
 
-    mocked_get_s3_object = mocker.patch('app.main.s3_client.get_s3_object')
-    mocked_delete_s3_object = mocker.patch('app.main.s3_client.delete_s3_object')
+    mocked_get_s3_object = mocker.patch('app.s3_client.s3_logo_client.get_s3_object')
+    mocked_delete_s3_object = mocker.patch('app.s3_client.s3_logo_client.delete_s3_object')
 
     mocked_get_s3_object.assert_not_called()
     mocked_delete_s3_object.assert_not_called()
@@ -83,8 +82,8 @@ def test_delete_temp_files_created_by_user(client, mocker, fake_uuid):
     obj = namedtuple("obj", ["key"])
     objs = [obj(key='test1'), obj(key='test2')]
 
-    mocker.patch('app.main.s3_client.get_s3_objects_filter_by_prefix', return_value=objs)
-    mocked_delete_s3_object = mocker.patch('app.main.s3_client.delete_s3_object')
+    mocker.patch('app.s3_client.s3_logo_client.get_s3_objects_filter_by_prefix', return_value=objs)
+    mocked_delete_s3_object = mocker.patch('app.s3_client.s3_logo_client.delete_s3_object')
 
     delete_temp_files_created_by(fake_uuid)
 
@@ -94,7 +93,7 @@ def test_delete_temp_files_created_by_user(client, mocker, fake_uuid):
 
 
 def test_delete_single_temp_file(client, mocker, fake_uuid, upload_filename):
-    mocked_delete_s3_object = mocker.patch('app.main.s3_client.delete_s3_object')
+    mocked_delete_s3_object = mocker.patch('app.s3_client.s3_logo_client.delete_s3_object')
 
     delete_temp_file(upload_filename)
 
@@ -103,28 +102,10 @@ def test_delete_single_temp_file(client, mocker, fake_uuid, upload_filename):
 
 def test_does_not_delete_non_temp_file(client, mocker, fake_uuid):
     filename = 'logo.png'
-    mocked_delete_s3_object = mocker.patch('app.main.s3_client.delete_s3_object')
+    mocked_delete_s3_object = mocker.patch('app.s3_client.s3_logo_client.delete_s3_object')
 
     with pytest.raises(ValueError) as error:
         delete_temp_file(filename)
 
     assert mocked_delete_s3_object.called_with_args(filename)
     assert str(error.value) == 'Not a temp file: {}'.format(filename)
-
-
-def test_sets_metadata(client, mocker):
-    mocked_s3_object = Mock()
-    mocked_get_s3_object = mocker.patch(
-        'app.main.s3_client.get_csv_upload',
-        return_value=mocked_s3_object,
-    )
-
-    set_metadata_on_csv_upload('1234', '5678', foo='bar', baz=True)
-
-    mocked_get_s3_object.assert_called_once_with('1234', '5678')
-    mocked_s3_object.copy_from.assert_called_once_with(
-        CopySource='test-notifications-csv-upload/service-1234-notify/5678.csv',
-        Metadata={'baz': 'True', 'foo': 'bar'},
-        MetadataDirective='REPLACE',
-        ServerSideEncryption='AES256',
-    )

tests/app/test_utils.py

@@ -210,7 +210,7 @@ def test_generate_notifications_csv_returns_correct_csv_file(
     expected_1st_row,
 ):
     mocker.patch(
-        'app.main.s3_client.s3download',
+        'app.s3_client.s3_csv_client.s3download',
         return_value=original_file_contents,
     )
     csv_content = generate_notifications_csv(service_id='1234', job_id=fake_uuid, template_type='sms')
@@ -236,7 +236,7 @@ def test_generate_notifications_csv_calls_twice_if_next_link(
 ):
 
     mocker.patch(
-        'app.main.s3_client.s3download',
+        'app.s3_client.s3_csv_client.s3download',
         return_value="""
             phone_number
             07700900000