Refactored to use kwarg

app/main/s3_client.py

@@ -5,8 +5,8 @@ from flask import current_app
 from notifications_utils.s3 import s3upload as utils_s3upload
 
 FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv'
-TEMP_TAG = 'temp-{}_'
+TEMP_TAG = 'temp-{user_id}_'
-LOGO_LOCATION_STRUCTURE = '{}{}-{}'
+LOGO_LOCATION_STRUCTURE = '{temp}{unique_id}-{filename}'
 
 
 def s3upload(service_id, filedata, region):
@@ -35,8 +35,11 @@ def s3download(service_id, upload_id):
 
 
 def upload_logo(filename, filedata, region, user_id):
-    upload_id = str(uuid.uuid4())
+    upload_file_name = LOGO_LOCATION_STRUCTURE.format(
-    upload_file_name = LOGO_LOCATION_STRUCTURE.format(TEMP_TAG.format(user_id), upload_id, filename)
+        temp=TEMP_TAG.format(user_id=user_id),
+        unique_id=str(uuid.uuid4()),
+        filename=filename
+    )
     utils_s3upload(filedata=filedata,
                    region=region,
                    bucket_name=current_app.config['LOGO_UPLOAD_BUCKET_NAME'],
@@ -47,7 +50,7 @@ def upload_logo(filename, filedata, region, user_id):
 
 def persist_logo(filename, user_id):
     try:
-        if filename.startswith(TEMP_TAG.format(user_id)):
+        if filename.startswith(TEMP_TAG.format(user_id=user_id)):
             persisted_filename = filename[len(TEMP_TAG.format(user_id)):]
         else:
             return filename

app/main/views/organisations.py

@@ -65,7 +65,7 @@ def manage_org(logo=None):
                 user_id=session["user_id"]
             )
 
-            if logo and logo.startswith(TEMP_TAG.format(session['user_id'])):
+            if logo and logo.startswith(TEMP_TAG.format(user_id=session['user_id'])):
                 delete_temp_file(logo)
 
             return redirect(
@@ -84,7 +84,6 @@ def manage_org(logo=None):
             org_id = resp['data']['id']
 
         return redirect(url_for('.organisations', organisation_id=org_id))
-
     if org:
         form.name.data = org['name']
         form.colour.data = org['colour']

app/notify_client/organisations_client.py

@@ -26,7 +26,7 @@ class OrganisationsClient(NotifyAdminAPIClient):
             "name": name,
             "colour": colour
         }
-        return self.post("/organisation", data)
+        return self.post(url="/organisation", data=data)
 
     def update_organisation(self, org_id, logo, name, colour):
         data = {
@@ -34,4 +34,4 @@ class OrganisationsClient(NotifyAdminAPIClient):
             "name": name,
             "colour": colour
         }
-        return self.post("/organisation/{}".format(org_id), data)
+        return self.post(url="/organisation/{}".format(org_id), data=data)

tests/app/main/views/test_organisations.py

@@ -132,7 +132,8 @@ def request_post_manage_org_redirect(logged_in_platform_admin_client, mocker, fa
     with logged_in_platform_admin_client.session_transaction() as session:
         user_id = session["user_id"]
 
-    temp_filename = LOGO_LOCATION_STRUCTURE.format(TEMP_TAG.format(user_id), fake_uuid, 'test.png')
+    temp_filename = LOGO_LOCATION_STRUCTURE.format(
+        temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename='test.png')
 
     mocker.patch('app.main.views.organisations.upload_logo', return_value=temp_filename)
     mocker.patch('app.main.views.organisations.delete_temp_file')
@@ -163,8 +164,10 @@ def test_deletes_previous_temp_logo_after_uploading_logo(logged_in_platform_admi
     with logged_in_platform_admin_client.session_transaction() as session:
         user_id = session["user_id"]
 
-    temp_old_filename = LOGO_LOCATION_STRUCTURE.format(TEMP_TAG.format(user_id), fake_uuid, 'old_test.png')
+    temp_old_filename = LOGO_LOCATION_STRUCTURE.format(
-    temp_filename = LOGO_LOCATION_STRUCTURE.format(TEMP_TAG.format(user_id), fake_uuid, 'test.png')
+        temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename='old_test.png')
+    temp_filename = LOGO_LOCATION_STRUCTURE.format(
+        temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename='test.png')
 
     mocked_upload_logo = mocker.patch(
         'app.main.views.organisations.upload_logo',
@@ -187,7 +190,8 @@ def test_logo_persisted_when_organisation_saved(logged_in_platform_admin_client,
     with logged_in_platform_admin_client.session_transaction() as session:
         user_id = session["user_id"]
 
-    temp_filename = LOGO_LOCATION_STRUCTURE.format(TEMP_TAG.format(user_id), fake_uuid, 'test.png')
+    temp_filename = LOGO_LOCATION_STRUCTURE.format(
+        temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename='test.png')
 
     mocked_upload_logo = mocker.patch('app.main.views.organisations.upload_logo')
     mocked_persist_logo = mocker.patch('app.main.views.organisations.persist_logo', return_value='test.png')
@@ -211,11 +215,12 @@ def test_existing_organisation_updated_when_organisation_saved(logged_in_platfor
 
     update_org = {'logo': 'test.png', 'colour': 'blue', 'name': 'new name'}
 
-    temp_filename = LOGO_LOCATION_STRUCTURE.format(TEMP_TAG.format(user_id), fake_uuid, update_org['logo'])
+    temp_filename = LOGO_LOCATION_STRUCTURE.format(
+        temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename=update_org['logo'])
 
     mocked_update_org = mocker.patch('app.organisations_client.update_organisation')
-    mocked_persist_logo = mocker.patch('app.main.views.organisations.persist_logo', return_value=update_org['logo'])
+    mocker.patch('app.main.views.organisations.persist_logo', return_value=update_org['logo'])
-    mocked_delete_temp_files_by = mocker.patch('app.main.views.organisations.delete_temp_files_created_by')
+    mocker.patch('app.main.views.organisations.delete_temp_files_created_by')
 
     logged_in_platform_admin_client.post(
         url_for('.manage_org', logo=temp_filename),
@@ -230,6 +235,30 @@ def test_existing_organisation_updated_when_organisation_saved(logged_in_platfor
         name=update_org['name'],
         colour=update_org['colour']
     )
-    assert mocked_persist_logo.called
+
-    assert mocked_delete_temp_files_by.called
+
-    assert mocked_delete_temp_files_by.call_args == call(user_id)
+def test_create_new_organisation_when_organisation_saved(logged_in_platform_admin_client, mocker, fake_uuid):
+    with logged_in_platform_admin_client.session_transaction() as session:
+        user_id = session["user_id"]
+
+    new_org = {'logo': 'test.png', 'colour': 'red', 'name': 'new name'}
+
+    temp_filename = LOGO_LOCATION_STRUCTURE.format(
+        temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename=new_org['logo'])
+
+    mocked_new_org = mocker.patch('app.organisations_client.create_organisation')
+    mocker.patch('app.main.views.organisations.persist_logo', return_value=new_org['logo'])
+    mocker.patch('app.main.views.organisations.delete_temp_files_created_by')
+
+    logged_in_platform_admin_client.post(
+        url_for('.manage_org', logo=temp_filename),
+        content_type='multipart/form-data',
+        data={'colour': new_org['colour'], 'name': new_org['name'], 'cdn_url': 'https://static-logos.cdn.com'}
+    )
+
+    assert mocked_new_org.called
+    assert mocked_new_org.call_args == call(
+        logo=new_org['logo'],
+        name=new_org['name'],
+        colour=new_org['colour']
+    )

tests/app/notify_client/test_organisations_client.py

@@ -0,0 +1,50 @@
+from app.notify_client.organisations_client import OrganisationsClient
+
+
+def test_get_organisation(mocker, fake_uuid):
+    mock_get = mocker.patch('app.notify_client.organisations_client.OrganisationsClient.get')
+    OrganisationsClient().get_organisation(fake_uuid)
+    mock_get.assert_called_once_with(
+        url='/organisation/{}'.format(fake_uuid)
+    )
+
+
+def test_get_organisations(mocker):
+    mock_get = mocker.patch('app.notify_client.organisations_client.OrganisationsClient.get')
+    OrganisationsClient().get_organisations()
+    mock_get.assert_called_once_with(
+        url='/organisation'
+    )
+
+
+def test_get_letter_organisations(mocker):
+    mock_get = mocker.patch('app.notify_client.organisations_client.OrganisationsClient.get')
+    OrganisationsClient().get_letter_organisations()
+    mock_get.assert_called_once_with(
+        url='/dvla_organisations'
+    )
+
+
+def test_create_organisations(mocker):
+    org_data = {'logo': 'test.png', 'name': 'test name', 'colour': 'red'}
+
+    mock_post = mocker.patch('app.notify_client.organisations_client.OrganisationsClient.post')
+    OrganisationsClient().create_organisation(logo=org_data['logo'], name=org_data['name'], colour=org_data['colour'])
+
+    mock_post.assert_called_once_with(
+        url='/organisation',
+        data=org_data
+    )
+
+
+def test_update_organisations(mocker, fake_uuid):
+    org_data = {'logo': 'test.png', 'name': 'test name', 'colour': 'red'}
+
+    mock_post = mocker.patch('app.notify_client.organisations_client.OrganisationsClient.post')
+    OrganisationsClient().update_organisation(
+        org_id=fake_uuid, logo=org_data['logo'], name=org_data['name'], colour=org_data['colour'])
+
+    mock_post.assert_called_once_with(
+        url='/organisation/{}'.format(fake_uuid),
+        data=org_data
+    )