From 93eab4ca5fdf6b4d0d8664d9df4e4239d10b95a3 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 1 Aug 2017 11:40:26 +0100 Subject: [PATCH] Refactored to use kwarg --- app/main/s3_client.py | 13 +++-- app/main/views/organisations.py | 3 +- app/notify_client/organisations_client.py | 4 +- tests/app/main/views/test_organisations.py | 49 ++++++++++++++---- .../test_organisations_client.py | 50 +++++++++++++++++++ 5 files changed, 100 insertions(+), 19 deletions(-) create mode 100644 tests/app/notify_client/test_organisations_client.py diff --git a/app/main/s3_client.py b/app/main/s3_client.py index 93764348d..64efa11c8 100644 --- a/app/main/s3_client.py +++ b/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-{}_' -LOGO_LOCATION_STRUCTURE = '{}{}-{}' +TEMP_TAG = 'temp-{user_id}_' +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(TEMP_TAG.format(user_id), upload_id, filename) + upload_file_name = LOGO_LOCATION_STRUCTURE.format( + 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 diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index b6a2c7127..0fe976752 100644 --- a/app/main/views/organisations.py +++ b/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'] diff --git a/app/notify_client/organisations_client.py b/app/notify_client/organisations_client.py index f75719a42..aae8d4c07 100644 --- a/app/notify_client/organisations_client.py +++ b/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) diff --git a/tests/app/main/views/test_organisations.py b/tests/app/main/views/test_organisations.py index 9812e2891..ad083f077 100644 --- a/tests/app/main/views/test_organisations.py +++ b/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_filename = LOGO_LOCATION_STRUCTURE.format(TEMP_TAG.format(user_id), fake_uuid, 'test.png') + temp_old_filename = LOGO_LOCATION_STRUCTURE.format( + 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']) - mocked_delete_temp_files_by = mocker.patch('app.main.views.organisations.delete_temp_files_created_by') + mocker.patch('app.main.views.organisations.persist_logo', return_value=update_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), @@ -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'] + ) diff --git a/tests/app/notify_client/test_organisations_client.py b/tests/app/notify_client/test_organisations_client.py new file mode 100644 index 000000000..a0eaa3d1c --- /dev/null +++ b/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 + )