From 1a4baf4283ea38ae0859fbe9c4ad5158c3be89a8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 19 Mar 2019 13:48:17 +0000 Subject: [PATCH 01/19] pass upload filename to notify-ftp previously ftp would name the files itself by giving them a timestamp when uploading. we ran into issues with tasks being picked up multiple times and as such, uploading duplicate files. By naming the file before creating the task, we can avoid this issue. Files are now named `NOTIFY.YYYYMMDD######.ZIP` where the number is a counter that increments with each task we've issued in that run of collate-letter-pdfs-for-day --- app/celery/letters_pdf_tasks.py | 11 ++++++++--- tests/app/celery/test_letters_pdf_tasks.py | 4 ++-- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 9149b72cc..409ed7808 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -119,17 +119,22 @@ def collate_letter_pdfs_for_day(date=None): current_app.config['LETTERS_PDF_BUCKET_NAME'], subfolder=date ) - for letters in group_letters(letter_pdfs): + for i, letters in enumerate(group_letters(letter_pdfs)): + dvla_filename = 'NOTIFY.{date}{num:06}.ZIP'.format(date=date.replace('-', ''), num=i + 1) filenames = [letter['Key'] for letter in letters] current_app.logger.info( - 'Calling task zip-and-send-letter-pdfs for {} pdfs of total size {:,} bytes'.format( + 'Calling task zip-and-send-letter-pdfs for {} pdfs to upload {} with total size {:,} bytes'.format( len(filenames), + dvla_filename, sum(letter['Size'] for letter in letters) ) ) notify_celery.send_task( name=TaskNames.ZIP_AND_SEND_LETTER_PDFS, - kwargs={'filenames_to_zip': filenames}, + kwargs={ + 'filenames_to_zip': filenames, + 'upload_filename': dvla_filename + }, queue=QueueNames.PROCESS_FTP, compression='zlib' ) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 4a849a005..d35301ae5 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -231,13 +231,13 @@ def test_collate_letter_pdfs_for_day(notify_api, mocker): mock_group_letters.assert_called_once_with(mock_s3.return_value) assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', - kwargs={'filenames_to_zip': ['A.PDF', 'B.pDf']}, + kwargs={'filenames_to_zip': ['A.PDF', 'B.pDf'], 'upload_filename': 'NOTIFY.20170102000001.ZIP'}, queue='process-ftp-tasks', compression='zlib' ) assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', - kwargs={'filenames_to_zip': ['C.pdf']}, + kwargs={'filenames_to_zip': ['C.pdf'], 'upload_filename': 'NOTIFY.20170102000002.ZIP'}, queue='process-ftp-tasks', compression='zlib' ) From 28ea75728cbcbb347a7e26bdf858050cf06f2c07 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Mar 2019 11:56:17 +0000 Subject: [PATCH 02/19] Return domains in get organisation response We need this so we can disply them in the admin app. --- app/models.py | 3 +++ tests/app/organisation/test_rest.py | 23 ++++++++++++++++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/models.py b/app/models.py index 57de4a046..afd817619 100644 --- a/app/models.py +++ b/app/models.py @@ -377,6 +377,9 @@ class Organisation(db.Model): "agreement_signed_at": self.agreement_signed_at, "agreement_signed_by_id": self.agreement_signed_by_id, "agreement_signed_version": self.agreement_signed_version, + "domains": [ + domain.domain for domain in self.domains + ], } diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 1bd0fecf7..d8cf2b704 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -4,7 +4,7 @@ import pytest from app.models import Organisation from app.dao.organisation_dao import dao_add_service_to_organisation, dao_add_user_to_organisation -from tests.app.db import create_organisation, create_service, create_user +from tests.app.db import create_domain, create_organisation, create_service, create_user def test_get_all_organisations(admin_request, notify_db_session): @@ -44,6 +44,7 @@ def test_get_organisation_by_id(admin_request, notify_db_session): 'agreement_signed_version', 'letter_branding_id', 'email_branding_id', + 'domains', } assert response['id'] == str(org.id) assert response['name'] == 'test_org_1' @@ -55,6 +56,26 @@ def test_get_organisation_by_id(admin_request, notify_db_session): assert response['agreement_signed_version'] is None assert response['letter_branding_id'] is None assert response['email_branding_id'] is None + assert response['domains'] == [] + + +def test_get_organisation_by_id_returns_domains(admin_request, notify_db_session): + + org = create_organisation() + + create_domain('foo.gov.uk', org.id) + create_domain('bar.gov.uk', org.id) + + response = admin_request.get( + 'organisation.get_organisation_by_id', + _expected_status=200, + organisation_id=org.id + ) + + assert set(response['domains']) == { + 'foo.gov.uk', + 'bar.gov.uk', + } def test_post_create_organisation(admin_request, notify_db_session): From 334eb473edad294bc1635b9af93334152e9ec441 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Mar 2019 12:15:25 +0000 Subject: [PATCH 03/19] separate batch num from date DVLA don't care about the naming conventions of zip files, other than it must start with `NOTIFY.` and end with `.ZIP`. So lets format the date in a more readable way, and separate it from the batch number --- app/celery/letters_pdf_tasks.py | 3 ++- tests/app/celery/test_letters_pdf_tasks.py | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 409ed7808..d50c5b42c 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -120,7 +120,8 @@ def collate_letter_pdfs_for_day(date=None): subfolder=date ) for i, letters in enumerate(group_letters(letter_pdfs)): - dvla_filename = 'NOTIFY.{date}{num:06}.ZIP'.format(date=date.replace('-', ''), num=i + 1) + # eg NOTIFY.2018-12-31.001.ZIP + dvla_filename = 'NOTIFY.{date}.{num:03}.ZIP'.format(date=date, num=i + 1) filenames = [letter['Key'] for letter in letters] current_app.logger.info( 'Calling task zip-and-send-letter-pdfs for {} pdfs to upload {} with total size {:,} bytes'.format( diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index d35301ae5..4cc8d0dbd 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -231,13 +231,13 @@ def test_collate_letter_pdfs_for_day(notify_api, mocker): mock_group_letters.assert_called_once_with(mock_s3.return_value) assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', - kwargs={'filenames_to_zip': ['A.PDF', 'B.pDf'], 'upload_filename': 'NOTIFY.20170102000001.ZIP'}, + kwargs={'filenames_to_zip': ['A.PDF', 'B.pDf'], 'upload_filename': 'NOTIFY.2017-01-02.001.ZIP'}, queue='process-ftp-tasks', compression='zlib' ) assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', - kwargs={'filenames_to_zip': ['C.pdf'], 'upload_filename': 'NOTIFY.20170102000002.ZIP'}, + kwargs={'filenames_to_zip': ['C.pdf'], 'upload_filename': 'NOTIFY.2017-01-02.002.ZIP'}, queue='process-ftp-tasks', compression='zlib' ) From 9783ab56b79dd3471fdfddf783bac2f9a7e94033 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Mar 2019 12:21:25 +0000 Subject: [PATCH 04/19] =?UTF-8?q?Don=E2=80=99t=20wipe=20domains=20when=20u?= =?UTF-8?q?pdating=20other=20attributes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The domains for an organisation should only be updated (or wiped) if a new list is explicitly passed in by the admin app. --- app/dao/organisation_dao.py | 2 +- tests/app/organisation/test_rest.py | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index 26b6dca1f..c4c0b03e5 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -53,7 +53,7 @@ def dao_create_organisation(organisation): @transactional def dao_update_organisation(organisation_id, **kwargs): - domains = kwargs.pop('domains', []) + domains = kwargs.pop('domains', None) organisation = Organisation.query.filter_by(id=organisation_id).update( kwargs diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index d8cf2b704..e8569a48b 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -201,6 +201,29 @@ def test_post_update_organisation_updates_domains( ] == domain_list +def test_update_other_organisation_attributes_doesnt_clear_domains( + admin_request, + notify_db_session, +): + org = create_organisation(name='test_org_2') + create_domain('example.gov.uk', org.id) + + admin_request.post( + 'organisation.update_organisation', + _data={ + 'agreement_signed': True, + }, + organisation_id=org.id, + _expected_status=204 + ) + + assert [ + domain.domain for domain in org.domains + ] == [ + 'example.gov.uk' + ] + + def test_post_update_organisation_raises_400_on_existing_org_name( admin_request, sample_organisation): org = create_organisation() From b3976c360a70435d49009b927eb109fc851f4bed Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Mar 2019 14:14:19 +0000 Subject: [PATCH 05/19] Add test for updating default branding --- tests/app/organisation/test_rest.py | 36 ++++++++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index e8569a48b..9fd312d71 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -4,7 +4,14 @@ import pytest from app.models import Organisation from app.dao.organisation_dao import dao_add_service_to_organisation, dao_add_user_to_organisation -from tests.app.db import create_domain, create_organisation, create_service, create_user +from tests.app.db import ( + create_domain, + create_email_branding, + create_letter_branding, + create_organisation, + create_service, + create_user, +) def test_get_all_organisations(admin_request, notify_db_session): @@ -224,6 +231,33 @@ def test_update_other_organisation_attributes_doesnt_clear_domains( ] +def test_update_organisation_default_branding( + admin_request, + notify_db_session, +): + + org = create_organisation(name='Test Organisation') + + email_branding = create_email_branding() + letter_branding = create_letter_branding() + + assert org.email_branding is None + assert org.letter_branding is None + + admin_request.post( + 'organisation.update_organisation', + _data={ + 'email_branding_id': str(email_branding.id), + 'letter_branding_id': str(letter_branding.id), + }, + organisation_id=org.id, + _expected_status=204 + ) + + assert org.email_branding == email_branding + assert org.letter_branding == letter_branding + + def test_post_update_organisation_raises_400_on_existing_org_name( admin_request, sample_organisation): org = create_organisation() From 365abd9a98174c25252a30372ccbf131dfd6c0bf Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 21 Mar 2019 13:26:54 +0000 Subject: [PATCH 06/19] Update pytest-xdist from 1.26.1 to 1.27.0 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 2f64621bc..50829e977 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -5,7 +5,7 @@ pytest==3.10.1 # pyup: <4 pytest-env==0.6.2 pytest-mock==1.10.1 pytest-cov==2.6.1 -pytest-xdist==1.26.1 +pytest-xdist==1.27.0 coveralls==1.6.0 freezegun==0.3.11 requests-mock==1.5.2 From b288031adb6e0207d0802c2c01a68c380eb26252 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 21 Mar 2019 15:40:24 +0000 Subject: [PATCH 07/19] add a hash of letter filenames to the dvla zip file name if we partially retry a day, we would create new zip files, containing different letters (if some were processed succesfully). We need these files to have different filenames to earlier zip files so that we can avoid overwriting log data in zips_sent. Hashing the filename means that we'll only overwrite if it was the same file containing the same content. --- app/celery/letters_pdf_tasks.py | 22 +++++++++++++++++----- tests/app/celery/test_letters_pdf_tasks.py | 18 ++++++++++++++---- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index d50c5b42c..e43af4607 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -2,6 +2,8 @@ import io import math from datetime import datetime from uuid import UUID +from hashlib import sha512 +from base64 import urlsafe_b64encode from PyPDF2.utils import PdfReadError from botocore.exceptions import ClientError as BotoClientError @@ -115,14 +117,24 @@ def collate_letter_pdfs_for_day(date=None): # since it is triggered mid afternoon. date = datetime.utcnow().strftime("%Y-%m-%d") - letter_pdfs = s3.get_s3_bucket_objects( - current_app.config['LETTERS_PDF_BUCKET_NAME'], - subfolder=date + letter_pdfs = sorted( + s3.get_s3_bucket_objects( + current_app.config['LETTERS_PDF_BUCKET_NAME'], + subfolder=date + ), + key=lambda letter: letter['Key'] ) for i, letters in enumerate(group_letters(letter_pdfs)): - # eg NOTIFY.2018-12-31.001.ZIP - dvla_filename = 'NOTIFY.{date}.{num:03}.ZIP'.format(date=date, num=i + 1) filenames = [letter['Key'] for letter in letters] + + hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode() + # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP + dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format( + date=date, + num=i + 1, + hash=hash + ) + current_app.logger.info( 'Calling task zip-and-send-letter-pdfs for {} pdfs to upload {} with total size {:,} bytes'.format( len(filenames), diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 4cc8d0dbd..8396347e6 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -218,7 +218,11 @@ def test_create_letters_gets_the_right_logo_when_service_has_letter_branding_log def test_collate_letter_pdfs_for_day(notify_api, mocker): - mock_s3 = mocker.patch('app.celery.tasks.s3.get_s3_bucket_objects') + mock_s3 = mocker.patch('app.celery.tasks.s3.get_s3_bucket_objects', return_value=[ + {'Key': 'B.pDf', 'Size': 2}, + {'Key': 'A.PDF', 'Size': 1}, + {'Key': 'C.pdf', 'Size': 3} + ]) mock_group_letters = mocker.patch('app.celery.letters_pdf_tasks.group_letters', return_value=[ [{'Key': 'A.PDF', 'Size': 1}, {'Key': 'B.pDf', 'Size': 2}], [{'Key': 'C.pdf', 'Size': 3}] @@ -228,16 +232,22 @@ def test_collate_letter_pdfs_for_day(notify_api, mocker): collate_letter_pdfs_for_day('2017-01-02') mock_s3.assert_called_once_with('test-letters-pdf', subfolder='2017-01-02') - mock_group_letters.assert_called_once_with(mock_s3.return_value) + mock_group_letters.assert_called_once_with(sorted(mock_s3.return_value, key=lambda x: x['Key'])) assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', - kwargs={'filenames_to_zip': ['A.PDF', 'B.pDf'], 'upload_filename': 'NOTIFY.2017-01-02.001.ZIP'}, + kwargs={ + 'filenames_to_zip': ['A.PDF', 'B.pDf'], + 'upload_filename': 'NOTIFY.2017-01-02.001.oqdjIM2-NAUU9Sm5Slmi.ZIP' + }, queue='process-ftp-tasks', compression='zlib' ) assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', - kwargs={'filenames_to_zip': ['C.pdf'], 'upload_filename': 'NOTIFY.2017-01-02.002.ZIP'}, + kwargs={ + 'filenames_to_zip': ['C.pdf'], + 'upload_filename': 'NOTIFY.2017-01-02.002.tdr7hcdPieiqjkVoS4kU.ZIP' + }, queue='process-ftp-tasks', compression='zlib' ) From 3e704079810576eb42b10925c84fb486412c8de5 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 11 Mar 2019 14:30:47 +0000 Subject: [PATCH 08/19] Migration to add folder_permissions to invited_users table Added a new JSONB column, folder_permissions, to the invited_users table to store a list of folders that an invited user can see. This is nullable for now, but will be changed to be non-nullable and back-populated later. --- app/models.py | 4 +++- .../0280_invited_user_folder_perms.py | 21 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) create mode 100644 migrations/versions/0280_invited_user_folder_perms.py diff --git a/app/models.py b/app/models.py index 57de4a046..c90b75fed 100644 --- a/app/models.py +++ b/app/models.py @@ -8,7 +8,8 @@ from sqlalchemy.ext.associationproxy import association_proxy from sqlalchemy.ext.hybrid import hybrid_property from sqlalchemy.dialects.postgresql import ( UUID, - JSON + JSON, + JSONB, ) from sqlalchemy import UniqueConstraint, CheckConstraint, Index from notifications_utils.columns import Columns @@ -1692,6 +1693,7 @@ class InvitedUser(db.Model): nullable=False, default=SMS_AUTH_TYPE ) + folder_permissions = db.Column(JSONB(none_as_null=True), nullable=True, default=[]) # would like to have used properties for this but haven't found a way to make them # play nice with marshmallow yet diff --git a/migrations/versions/0280_invited_user_folder_perms.py b/migrations/versions/0280_invited_user_folder_perms.py new file mode 100644 index 000000000..bee657d04 --- /dev/null +++ b/migrations/versions/0280_invited_user_folder_perms.py @@ -0,0 +1,21 @@ +""" + +Revision ID: 0280_invited_user_folder_perms +Revises: 0279_remove_fk_to_users +Create Date: 2019-03-11 14:38:28.010082 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0280_invited_user_folder_perms' +down_revision = '0279_remove_fk_to_users' + + +def upgrade(): + op.add_column('invited_users', sa.Column('folder_permissions', postgresql.JSONB(none_as_null=True, astext_type=sa.Text()), nullable=True)) + + +def downgrade(): + op.drop_column('invited_users', 'folder_permissions') From 31ddd36e2c38a417f48adbedd0615fa08d67cd35 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 11 Mar 2019 14:33:51 +0000 Subject: [PATCH 09/19] Update invited user tests to take folder_permissions into account --- .../accept_invite/test_accept_invite_rest.py | 1 + tests/app/conftest.py | 3 +- tests/app/dao/test_invited_user_dao.py | 29 +++++++++++++++++-- tests/app/db.py | 3 +- tests/app/invite/test_invite_rest.py | 8 +++-- 5 files changed, 38 insertions(+), 6 deletions(-) diff --git a/tests/app/accept_invite/test_accept_invite_rest.py b/tests/app/accept_invite/test_accept_invite_rest.py index 72e72cea8..f0c806473 100644 --- a/tests/app/accept_invite/test_accept_invite_rest.py +++ b/tests/app/accept_invite/test_accept_invite_rest.py @@ -45,6 +45,7 @@ def test_validate_invitation_token_returns_200_when_token_valid( assert json_resp['data']['service'] == str(sample_invited_user.service_id) assert json_resp['data']['status'] == sample_invited_user.status assert json_resp['data']['permissions'] == sample_invited_user.permissions + assert json_resp['data']['folder_permissions'] == sample_invited_user.folder_permissions if invitation_type == 'organisation': assert json_resp['data'] == sample_invited_org_user.serialize() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 50b86e4d4..986ef0ead 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -752,7 +752,8 @@ def sample_invited_user(notify_db, 'service': service, 'email_address': to_email_address, 'from_user': from_user, - 'permissions': 'send_messages,manage_service,manage_api_keys' + 'permissions': 'send_messages,manage_service,manage_api_keys', + 'folder_permissions': ['folder_1_id', 'folder_2_id'], } invited_user = InvitedUser(**data) save_invited_user(invited_user) diff --git a/tests/app/dao/test_invited_user_dao.py b/tests/app/dao/test_invited_user_dao.py index d70632c6b..a0083b253 100644 --- a/tests/app/dao/test_invited_user_dao.py +++ b/tests/app/dao/test_invited_user_dao.py @@ -26,7 +26,8 @@ def test_create_invited_user(notify_db, notify_db_session, sample_service): 'service': sample_service, 'email_address': email_address, 'from_user': invite_from, - 'permissions': 'send_messages,manage_service' + 'permissions': 'send_messages,manage_service', + 'folder_permissions': [] } invited_user = InvitedUser(**data) @@ -39,6 +40,29 @@ def test_create_invited_user(notify_db, notify_db_session, sample_service): assert len(permissions) == 2 assert 'send_messages' in permissions assert 'manage_service' in permissions + assert invited_user.folder_permissions == [] + + +def test_create_invited_user_sets_default_folder_permissions_of_empty_list( + notify_db, + notify_db_session, + sample_service, +): + assert InvitedUser.query.count() == 0 + invite_from = sample_service.users[0] + + data = { + 'service': sample_service, + 'email_address': 'invited_user@service.gov.uk', + 'from_user': invite_from, + 'permissions': 'send_messages,manage_service', + } + + invited_user = InvitedUser(**data) + save_invited_user(invited_user) + + assert InvitedUser.query.count() == 1 + assert invited_user.folder_permissions == [] def test_get_invited_user_by_service_and_id(notify_db, notify_db_session, sample_invited_user): @@ -124,7 +148,8 @@ def make_invitation(user, service, age=timedelta(hours=0), email_address="test@t service=service, status='pending', created_at=datetime.utcnow() - age, - permissions='manage_settings' + permissions='manage_settings', + folder_permissions=[str(uuid.uuid4())] ) db.session.add(verify_code) db.session.commit() diff --git a/tests/app/db.py b/tests/app/db.py index 8c082350a..8d34e8b80 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -739,7 +739,8 @@ def create_invited_user(service=None, 'service': service, 'email_address': to_email_address, 'from_user': from_user, - 'permissions': 'send_messages,manage_service,manage_api_keys' + 'permissions': 'send_messages,manage_service,manage_api_keys', + 'folder_permissions': [str(uuid.uuid4()), str(uuid.uuid4())] } invited_user = InvitedUser(**data) save_invited_user(invited_user) diff --git a/tests/app/invite/test_invite_rest.py b/tests/app/invite/test_invite_rest.py index d7b89bc48..61ad97934 100644 --- a/tests/app/invite/test_invite_rest.py +++ b/tests/app/invite/test_invite_rest.py @@ -33,6 +33,7 @@ def test_create_invited_user( from_user=str(invite_from.id), permissions='send_messages,manage_service,manage_api_keys', auth_type=EMAIL_AUTH_TYPE, + folder_permissions=['folder_1', 'folder_2', 'folder_3'], **extra_args ) @@ -49,6 +50,7 @@ def test_create_invited_user( assert json_resp['data']['permissions'] == 'send_messages,manage_service,manage_api_keys' assert json_resp['data']['auth_type'] == EMAIL_AUTH_TYPE assert json_resp['data']['id'] + assert json_resp['data']['folder_permissions'] == ['folder_1', 'folder_2', 'folder_3'] notification = Notification.query.first() @@ -73,6 +75,7 @@ def test_create_invited_user_without_auth_type(admin_request, sample_service, mo 'email_address': email_address, 'from_user': str(invite_from.id), 'permissions': 'send_messages,manage_service,manage_api_keys', + 'folder_permissions': [] } json_resp = admin_request.post( @@ -85,7 +88,7 @@ def test_create_invited_user_without_auth_type(admin_request, sample_service, mo assert json_resp['data']['auth_type'] == SMS_AUTH_TYPE -def test_create_invited_user_invalid_email(client, sample_service, mocker): +def test_create_invited_user_invalid_email(client, sample_service, mocker, fake_uuid): mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') email_address = 'notanemail' invite_from = sample_service.users[0] @@ -94,7 +97,8 @@ def test_create_invited_user_invalid_email(client, sample_service, mocker): 'service': str(sample_service.id), 'email_address': email_address, 'from_user': str(invite_from.id), - 'permissions': 'send_messages,manage_service,manage_api_keys' + 'permissions': 'send_messages,manage_service,manage_api_keys', + 'folder_permissions': [fake_uuid, fake_uuid] } data = json.dumps(data) From b0d3bd9046ff5e07d0e19671394aef6c4a0f12e5 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 12 Mar 2019 19:07:12 +0000 Subject: [PATCH 10/19] Update add_user_to_service endoint to only handle new data format Updated the add_user_to_service endpoint to only handle data in the 'new' format (`{"permissions": [...]}` instead of `[permission_1, permission_2]`) since Admin has been updated to send data the new way. This change means that we no longer need the Marshmallow Permission schema, so it can be deleted. --- app/schemas.py | 19 ------- app/service/rest.py | 15 +++--- tests/app/service/test_rest.py | 94 +++++----------------------------- 3 files changed, 22 insertions(+), 106 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index 8a4b1df20..b65c2f5b4 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -539,24 +539,6 @@ class InvitedUserSchema(BaseSchema): raise ValidationError(str(e)) -class PermissionSchema(BaseSchema): - - # Override generated fields - user = field_for(models.Permission, 'user', dump_only=True) - service = field_for(models.Permission, 'service', dump_only=True) - permission = field_for(models.Permission, 'permission') - - __envelope__ = { - 'single': 'permission', - 'many': 'permissions', - } - - class Meta: - model = models.Permission - exclude = ("created_at",) - strict = True - - class EmailDataSchema(ma.Schema): class Meta: @@ -692,7 +674,6 @@ notification_schema = NotificationModelSchema() notification_with_template_schema = NotificationWithTemplateSchema() notification_with_personalisation_schema = NotificationWithPersonalisationSchema() invited_user_schema = InvitedUserSchema() -permission_schema = PermissionSchema() email_data_request_schema = EmailDataSchema() partial_email_data_request_schema = EmailDataSchema(partial_email=True) notifications_filter_schema = NotificationsFilterSchema() diff --git a/app/service/rest.py b/app/service/rest.py index a98c0bc4a..52651e2f8 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -83,7 +83,7 @@ from app.errors import ( register_errors ) from app.letters.utils import letter_print_day -from app.models import LETTER_TYPE, NOTIFICATION_CANCELLED, Service, EmailBranding, LetterBranding +from app.models import LETTER_TYPE, NOTIFICATION_CANCELLED, Permission, Service, EmailBranding, LetterBranding from app.schema_validation import validate from app.service import statistics from app.service.service_data_retention_schema import ( @@ -101,11 +101,11 @@ from app.service.send_notification import send_one_off_notification from app.schemas import ( service_schema, api_key_schema, - permission_schema, notification_with_template_schema, notifications_filter_schema, detailed_service_schema ) +from app.user.users_schema import post_set_permissions_schema from app.utils import pagination_links service_blueprint = Blueprint('service', __name__) @@ -286,12 +286,13 @@ def add_user_to_service(service_id, user_id): raise InvalidRequest(error, status_code=400) data = request.get_json() - if 'permissions' in data: - user_permissions = data['permissions'] - else: - user_permissions = data + validate(data, post_set_permissions_schema) + + permissions = [ + Permission(service_id=service_id, user_id=user_id, permission=p['permission']) + for p in data['permissions'] + ] - permissions = permission_schema.load(user_permissions, many=True).data dao_add_user_to_service(service, user, permissions) data = service_schema.dump(service).data return jsonify(data=data), 201 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 83757fb94..89b4d40d4 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1080,81 +1080,7 @@ def test_default_permissions_are_added_for_user_service(notify_api, assert sorted(default_service_permissions) == sorted(service_permissions) -def test_add_existing_user_to_another_service_with_all_permissions(notify_api, - notify_db, - notify_db_session, - sample_service, - sample_user): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - # check which users part of service - user_already_in_service = sample_service.users[0] - auth_header = create_authorization_header() - - resp = client.get( - '/service/{}/users'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header] - ) - - assert resp.status_code == 200 - result = resp.json - assert len(result['data']) == 1 - assert result['data'][0]['email_address'] == user_already_in_service.email_address - - # add new user to service - user_to_add = User( - name='Invited User', - email_address='invited@digital.cabinet-office.gov.uk', - password='password', - mobile_number='+4477123456' - ) - # they must exist in db first - save_model_user(user_to_add) - - data = [{"permission": "send_emails"}, - {"permission": "send_letters"}, - {"permission": "send_texts"}, - {"permission": "manage_users"}, - {"permission": "manage_settings"}, - {"permission": "manage_api_keys"}, - {"permission": "manage_templates"}, - {"permission": "view_activity"}] - - auth_header = create_authorization_header() - - resp = client.post( - '/service/{}/users/{}'.format(sample_service.id, user_to_add.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=json.dumps(data) - ) - - assert resp.status_code == 201 - - # check new user added to service - auth_header = create_authorization_header() - - resp = client.get( - '/service/{}'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - ) - assert resp.status_code == 200 - json_resp = resp.json - assert str(user_to_add.id) in json_resp['data']['users'] - - # check user has all permissions - auth_header = create_authorization_header() - resp = client.get(url_for('user.get_user', user_id=user_to_add.id), - headers=[('Content-Type', 'application/json'), auth_header]) - - assert resp.status_code == 200 - json_resp = resp.json - permissions = json_resp['data']['permissions'][str(sample_service.id)] - expected_permissions = ['send_texts', 'send_emails', 'send_letters', 'manage_users', - 'manage_settings', 'manage_templates', 'manage_api_keys', 'view_activity'] - assert sorted(expected_permissions) == sorted(permissions) - - -def test_add_existing_user_to_another_service_with_all_permissions_with_new_data_format( +def test_add_existing_user_to_another_service_with_all_permissions( notify_api, notify_db, notify_db_session, @@ -1250,9 +1176,13 @@ def test_add_existing_user_to_another_service_with_send_permissions(notify_api, ) save_model_user(user_to_add) - data = [{"permission": "send_emails"}, + data = { + "permissions": [ + {"permission": "send_emails"}, {"permission": "send_letters"}, - {"permission": "send_texts"}] + {"permission": "send_texts"}, + ] + } auth_header = create_authorization_header() @@ -1293,9 +1223,13 @@ def test_add_existing_user_to_another_service_with_manage_permissions(notify_api ) save_model_user(user_to_add) - data = [{"permission": "manage_users"}, + data = { + "permissions": [ + {"permission": "manage_users"}, {"permission": "manage_settings"}, - {"permission": "manage_templates"}] + {"permission": "manage_templates"}, + ] + } auth_header = create_authorization_header() @@ -1336,7 +1270,7 @@ def test_add_existing_user_to_another_service_with_manage_api_keys(notify_api, ) save_model_user(user_to_add) - data = [{"permission": "manage_api_keys"}] + data = {"permissions": [{"permission": "manage_api_keys"}]} auth_header = create_authorization_header() From 2aa14bc41ca5897dc3d647622322f40387c05b11 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 14 Mar 2019 16:55:48 +0000 Subject: [PATCH 11/19] Set folder permissions when adding a user to a service This sets the folder permissions for a user when adding them to a service. If a user is being added to a service after accepting an invite, we need to account for the possibility that the folders we are trying to add them to have been deleted before they accepted the invite. --- app/dao/services_dao.py | 11 ++++++- app/dao/template_folder_dao.py | 4 +++ app/service/rest.py | 3 +- tests/app/dao/test_services_dao.py | 47 ++++++++++++++++++++++++++++ tests/app/service/test_rest.py | 49 ++++++++++++++++++++++++++++-- 5 files changed, 110 insertions(+), 4 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index e881bf020..4bef1e99c 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -14,6 +14,7 @@ from app.dao.dao_utils import ( from app.dao.organisation_dao import dao_get_organisation_by_email_address from app.dao.service_sms_sender_dao import insert_service_sms_sender from app.dao.service_user_dao import dao_get_service_user +from app.dao.template_folder_dao import dao_get_valid_template_folders_by_id from app.models import ( AnnualBilling, ApiKey, @@ -208,13 +209,21 @@ def dao_update_service(service): db.session.add(service) -def dao_add_user_to_service(service, user, permissions=None): +def dao_add_user_to_service(service, user, permissions=None, folder_permissions=None): permissions = permissions or [] + folder_permissions = folder_permissions or [] + try: from app.dao.permissions_dao import permission_dao service.users.append(user) permission_dao.set_user_service_permission(user, service, permissions, _commit=False) db.session.add(service) + + service_user = dao_get_service_user(user.id, service.id) + valid_template_folders = dao_get_valid_template_folders_by_id(folder_permissions) + service_user.folders = valid_template_folders + db.session.add(service_user) + except Exception as e: db.session.rollback() raise e diff --git a/app/dao/template_folder_dao.py b/app/dao/template_folder_dao.py index a162f79c0..6264f9bcf 100644 --- a/app/dao/template_folder_dao.py +++ b/app/dao/template_folder_dao.py @@ -10,6 +10,10 @@ def dao_get_template_folder_by_id_and_service_id(template_folder_id, service_id) ).one() +def dao_get_valid_template_folders_by_id(folder_ids): + return TemplateFolder.query.filter(TemplateFolder.id.in_(folder_ids)).all() + + @transactional def dao_create_template_folder(template_folder): db.session.add(template_folder) diff --git a/app/service/rest.py b/app/service/rest.py index 52651e2f8..ecaa3a58b 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -292,8 +292,9 @@ def add_user_to_service(service_id, user_id): Permission(service_id=service_id, user_id=user_id, permission=p['permission']) for p in data['permissions'] ] + folder_permissions = data.get('folder_permissions', []) - dao_add_user_to_service(service, user, permissions) + dao_add_user_to_service(service, user, permissions, folder_permissions) data = service_schema.dump(service).data return jsonify(data=data), 201 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index cbc6cb24c..a8665554f 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -46,6 +46,7 @@ from app.models import ( InvitedUser, Service, ServicePermission, + ServiceUser, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, @@ -192,6 +193,52 @@ def test_should_add_user_to_service(notify_db_session): assert new_user in Service.query.first().users +def test_dao_add_user_to_service_sets_folder_permissions(sample_user, sample_service): + folder_1 = create_template_folder(sample_service) + folder_2 = create_template_folder(sample_service) + + assert not folder_1.users + assert not folder_2.users + + folder_permissions = [str(folder_1.id), str(folder_2.id)] + + dao_add_user_to_service(sample_service, sample_user, folder_permissions=folder_permissions) + + service_user = dao_get_service_user(user_id=sample_user.id, service_id=sample_service.id) + assert len(service_user.folders) == 2 + assert folder_1 in service_user.folders + assert folder_2 in service_user.folders + + +def test_dao_add_user_to_service_ignores_folders_which_do_not_exist_when_setting_permissions( + sample_user, + sample_service, + fake_uuid +): + valid_folder = create_template_folder(sample_service) + folder_permissions = [fake_uuid, str(valid_folder.id)] + + dao_add_user_to_service(sample_service, sample_user, folder_permissions=folder_permissions) + + service_user = dao_get_service_user(sample_user.id, sample_service.id) + + assert service_user.folders == [valid_folder] + + +def test_dao_add_user_to_service_raises_error_if_adding_folder_permissions_for_a_different_service( + sample_user, + sample_service, +): + other_service = create_service(service_name='other service') + other_service_folder = create_template_folder(other_service) + folder_permissions = [str(other_service_folder.id)] + + with pytest.raises(IntegrityError) as e: + dao_add_user_to_service(sample_service, sample_user, folder_permissions=folder_permissions) + assert 'insert or update on table "user_folder_permissions" violates foreign key constraint' in str(e.value) + assert ServiceUser.query.count() == 0 + + def test_should_remove_user_from_service(notify_db_session): user = create_user() service = Service(name="service_name", diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 89b4d40d4..80d4eff42 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -11,6 +11,7 @@ from freezegun import freeze_time from app.dao.organisation_dao import dao_add_service_to_organisation from app.dao.service_sms_sender_dao import dao_get_sms_senders_by_service_id from app.dao.services_dao import dao_remove_user_from_service +from app.dao.service_user_dao import dao_get_service_user from app.dao.templates_dao import dao_redact_template from app.dao.users_dao import save_model_user from app.models import ( @@ -38,6 +39,7 @@ from tests.app.db import ( create_service, create_service_with_inbound_number, create_template, + create_template_folder, create_notification, create_reply_to_email, create_letter_contact, @@ -1123,7 +1125,8 @@ def test_add_existing_user_to_another_service_with_all_permissions( {"permission": "manage_api_keys"}, {"permission": "manage_templates"}, {"permission": "view_activity"}, - ] + ], + "folder_permissions": [] } auth_header = create_authorization_header() @@ -1181,7 +1184,8 @@ def test_add_existing_user_to_another_service_with_send_permissions(notify_api, {"permission": "send_emails"}, {"permission": "send_letters"}, {"permission": "send_texts"}, - ] + ], + "folder_permissions": [] } auth_header = create_authorization_header() @@ -1254,6 +1258,47 @@ def test_add_existing_user_to_another_service_with_manage_permissions(notify_api assert sorted(expected_permissions) == sorted(permissions) +def test_add_existing_user_to_another_service_with_folder_permissions(notify_api, + notify_db, + notify_db_session, + sample_service, + sample_user): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + # they must exist in db first + user_to_add = User( + name='Invited User', + email_address='invited@digital.cabinet-office.gov.uk', + password='password', + mobile_number='+4477123456' + ) + save_model_user(user_to_add) + + folder_1 = create_template_folder(sample_service) + folder_2 = create_template_folder(sample_service) + + data = { + "permissions": [{"permission": "manage_api_keys"}], + "folder_permissions": [str(folder_1.id), str(folder_2.id)] + } + + auth_header = create_authorization_header() + + resp = client.post( + '/service/{}/users/{}'.format(sample_service.id, user_to_add.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=json.dumps(data) + ) + + assert resp.status_code == 201 + + new_user = dao_get_service_user(user_id=user_to_add.id, service_id=sample_service.id) + + assert len(new_user.folders) == 2 + assert folder_1 in new_user.folders + assert folder_2 in new_user.folders + + def test_add_existing_user_to_another_service_with_manage_api_keys(notify_api, notify_db, notify_db_session, From 8f5b5d636e407c1933f357e2348c814ca368a78b Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 20 Mar 2019 10:29:42 +0000 Subject: [PATCH 12/19] Make invited_users folder_permissions column non-nullable Now that notifications-admin is always sending through folder_permissions, the folder_permissions column of the invited_user table can be made non-nullable. The migration also backfills the column (to []) to account for existing null values. --- app/models.py | 2 +- .../0281_non_null_folder_permissions.py | 26 +++++++++++++++++++ 2 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 migrations/versions/0281_non_null_folder_permissions.py diff --git a/app/models.py b/app/models.py index c90b75fed..30289da1d 100644 --- a/app/models.py +++ b/app/models.py @@ -1693,7 +1693,7 @@ class InvitedUser(db.Model): nullable=False, default=SMS_AUTH_TYPE ) - folder_permissions = db.Column(JSONB(none_as_null=True), nullable=True, default=[]) + folder_permissions = db.Column(JSONB(none_as_null=True), nullable=False, default=[]) # would like to have used properties for this but haven't found a way to make them # play nice with marshmallow yet diff --git a/migrations/versions/0281_non_null_folder_permissions.py b/migrations/versions/0281_non_null_folder_permissions.py new file mode 100644 index 000000000..9d27e3b02 --- /dev/null +++ b/migrations/versions/0281_non_null_folder_permissions.py @@ -0,0 +1,26 @@ +""" + +Revision ID: 0281_non_null_folder_permissions +Revises: 0280_invited_user_folder_perms +Create Date: 2019-03-20 10:12:24.927129 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0281_non_null_folder_permissions' +down_revision = '0280_invited_user_folder_perms' + + +def upgrade(): + op.execute("UPDATE invited_users SET folder_permissions = '[]' WHERE folder_permissions IS null") + op.alter_column('invited_users', 'folder_permissions', + existing_type=postgresql.JSONB(astext_type=sa.Text()), + nullable=False) + + +def downgrade(): + op.alter_column('invited_users', 'folder_permissions', + existing_type=postgresql.JSONB(astext_type=sa.Text()), + nullable=True) From 6fa7f0290d4e74879113ed8688be6d609fb3a9db Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 22 Mar 2019 12:07:08 +0000 Subject: [PATCH 13/19] ignore case in the cost_threshold in dvla response files we failed when we received UNSORTED instead of Unsorted --- app/celery/tasks.py | 11 +++++------ tests/app/celery/test_ftp_update_tasks.py | 8 ++++---- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 1948c0964..490aa3d40 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -421,12 +421,11 @@ def update_letter_notifications_statuses(self, filename): for update in notification_updates: check_billable_units(update) update_letter_notification(filename, temporary_failures, update) - sorted_letter_counts[update.cost_threshold] += 1 + sorted_letter_counts[update.cost_threshold.lower()] += 1 try: - if sorted_letter_counts.keys() - {'Unsorted', 'Sorted'}: - unknown_status = sorted_letter_counts.keys() - {'Unsorted', 'Sorted'} - + unknown_status = sorted_letter_counts.keys() - {'unsorted', 'sorted'} + if unknown_status: message = 'DVLA response file: {} contains unknown Sorted status {}'.format( filename, unknown_status ) @@ -455,8 +454,8 @@ def persist_daily_sorted_letter_counts(day, file_name, sorted_letter_counts): daily_letter_count = DailySortedLetter( billing_day=day, file_name=file_name, - unsorted_count=sorted_letter_counts['Unsorted'], - sorted_count=sorted_letter_counts['Sorted'] + unsorted_count=sorted_letter_counts['unsorted'], + sorted_count=sorted_letter_counts['sorted'] ) dao_create_or_update_daily_sorted_letter(daily_letter_count) diff --git a/tests/app/celery/test_ftp_update_tasks.py b/tests/app/celery/test_ftp_update_tasks.py index 436846827..70e1479b7 100644 --- a/tests/app/celery/test_ftp_update_tasks.py +++ b/tests/app/celery/test_ftp_update_tasks.py @@ -94,7 +94,7 @@ def test_update_letter_notifications_statuses_raises_error_for_unknown_sorted_st update_letter_notifications_statuses(filename='NOTIFY-20170823160812-RSP.TXT') assert "DVLA response file: {filename} contains unknown Sorted status {unknown_status}".format( - filename="NOTIFY-20170823160812-RSP.TXT", unknown_status="{'Error'}" + filename="NOTIFY-20170823160812-RSP.TXT", unknown_status="{'error'}" ) in str(e) @@ -185,7 +185,7 @@ def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count ): sent_letter_1 = create_notification(sample_letter_template, reference='ref-foo', status=NOTIFICATION_SENDING) sent_letter_2 = create_notification(sample_letter_template, reference='ref-bar', status=NOTIFICATION_SENDING) - valid_file = '{}|Sent|1|Unsorted\n{}|Sent|2|Sorted'.format( + valid_file = '{}|Sent|1|uNsOrTeD\n{}|Sent|2|SORTED'.format( sent_letter_1.reference, sent_letter_2.reference) mocker.patch('app.celery.tasks.s3.get_s3_file', return_value=valid_file) @@ -195,7 +195,7 @@ def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count persist_letter_count_mock.assert_called_once_with(day=date(2017, 8, 23), file_name='NOTIFY-20170823160812-RSP.TXT', - sorted_letter_counts={'Unsorted': 1, 'Sorted': 1}) + sorted_letter_counts={'unsorted': 1, 'sorted': 1}) def test_update_letter_notifications_statuses_persists_daily_sorted_letter_count_with_no_sorted_values( @@ -317,7 +317,7 @@ def test_get_billing_date_in_bst_from_filename(filename_date, billing_date): @freeze_time("2018-01-11 09:00:00") def test_persist_daily_sorted_letter_counts_saves_sorted_and_unsorted_values(client, notify_db_session): - letter_counts = defaultdict(int, **{'Unsorted': 5, 'Sorted': 1}) + letter_counts = defaultdict(int, **{'unsorted': 5, 'sorted': 1}) persist_daily_sorted_letter_counts(date.today(), "test.txt", letter_counts) day = dao_get_daily_sorted_letter_by_billing_day(date.today()) From 05ab6ceeefca415207c5c7d48566b1c400057608 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 21 Mar 2019 11:49:44 +0000 Subject: [PATCH 14/19] Upgrade jsonschema from 3.0.0b3 to 3.0.1 Running `make freeze-requirements` causes Werkzeug to be updated from 0.14.1 to 0.15.1, which means that we need to change the message of 1 test. --- requirements-app.txt | 2 +- requirements.txt | 10 +++++----- tests/app/delivery/test_rest.py | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index 5f9b3cff6..047b0b29d 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -13,7 +13,7 @@ click-datetime==0.2 eventlet==0.23.0 gunicorn==19.7.1 iso8601==0.1.12 -jsonschema==3.0.0b3 +jsonschema==3.0.1 marshmallow-sqlalchemy==0.16.0 marshmallow==2.18.1 psycopg2-binary==2.7.7 diff --git a/requirements.txt b/requirements.txt index 4bbd6cde4..143485b1a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -15,7 +15,7 @@ click-datetime==0.2 eventlet==0.23.0 gunicorn==19.7.1 iso8601==0.1.12 -jsonschema==3.0.0b3 +jsonschema==3.0.1 marshmallow-sqlalchemy==0.16.0 marshmallow==2.18.1 psycopg2-binary==2.7.7 @@ -40,12 +40,12 @@ alembic==1.0.8 amqp==1.4.9 anyjson==0.3.3 attrs==19.1.0 -awscli==1.16.125 +awscli==1.16.129 bcrypt==3.1.6 billiard==3.3.0.23 bleach==3.1.0 boto3==1.6.16 -botocore==1.12.115 +botocore==1.12.119 certifi==2019.3.9 chardet==3.0.4 Click==7.0 @@ -58,7 +58,7 @@ idna==2.8 Jinja2==2.10 jmespath==0.9.4 kombu==3.0.37 -Mako==1.0.7 +Mako==1.0.8 MarkupSafe==1.1.1 mistune==0.8.4 monotonic==1.5 @@ -82,4 +82,4 @@ smartypants==2.0.1 statsd==3.3.0 urllib3==1.24.1 webencodings==0.5.1 -Werkzeug==0.14.1 +Werkzeug==0.15.1 diff --git a/tests/app/delivery/test_rest.py b/tests/app/delivery/test_rest.py index 08a5d0a05..fbf980139 100644 --- a/tests/app/delivery/test_rest.py +++ b/tests/app/delivery/test_rest.py @@ -21,7 +21,7 @@ def test_should_reject_if_invalid_uuid(notify_api): ) body = json.loads(response.get_data(as_text=True)) assert response.status_code == 404 - assert body['message'] == 'The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again.' # noqa + assert body['message'] == 'The requested URL was not found on the server. If you entered the URL manually please check your spelling and try again.' # noqa assert body['result'] == 'error' From 5d03b929930ca354bc048bba226ff84273fdb6f8 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 21 Mar 2019 11:56:32 +0000 Subject: [PATCH 15/19] Upgrade marshmallow-sqlalchemy from 0.16.0 to 0.16.1 --- requirements-app.txt | 2 +- requirements.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index 047b0b29d..b47c65080 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -14,7 +14,7 @@ eventlet==0.23.0 gunicorn==19.7.1 iso8601==0.1.12 jsonschema==3.0.1 -marshmallow-sqlalchemy==0.16.0 +marshmallow-sqlalchemy==0.16.1 marshmallow==2.18.1 psycopg2-binary==2.7.7 PyJWT==1.7.1 diff --git a/requirements.txt b/requirements.txt index 143485b1a..3fb26aa1c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -16,7 +16,7 @@ eventlet==0.23.0 gunicorn==19.7.1 iso8601==0.1.12 jsonschema==3.0.1 -marshmallow-sqlalchemy==0.16.0 +marshmallow-sqlalchemy==0.16.1 marshmallow==2.18.1 psycopg2-binary==2.7.7 PyJWT==1.7.1 From 60be63133c4057bf50e12899a8fb0349a4c9acf9 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 21 Mar 2019 12:03:26 +0000 Subject: [PATCH 16/19] Upgrade marshmallow from 2.18.1 to 2.19.1 --- requirements-app.txt | 2 +- requirements.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index b47c65080..bb9c6f810 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -15,7 +15,7 @@ gunicorn==19.7.1 iso8601==0.1.12 jsonschema==3.0.1 marshmallow-sqlalchemy==0.16.1 -marshmallow==2.18.1 +marshmallow==2.19.1 psycopg2-binary==2.7.7 PyJWT==1.7.1 SQLAlchemy==1.2.18 diff --git a/requirements.txt b/requirements.txt index 3fb26aa1c..1cadf7c00 100644 --- a/requirements.txt +++ b/requirements.txt @@ -17,7 +17,7 @@ gunicorn==19.7.1 iso8601==0.1.12 jsonschema==3.0.1 marshmallow-sqlalchemy==0.16.1 -marshmallow==2.18.1 +marshmallow==2.19.1 psycopg2-binary==2.7.7 PyJWT==1.7.1 SQLAlchemy==1.2.18 From 9b739aaa1ca0b5d9114856944fc8a7afb72ab378 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 21 Mar 2019 13:13:53 +0000 Subject: [PATCH 17/19] Upgrade flask-marshmallow from 0.9.0 to 0.10.0 --- requirements-app.txt | 2 +- requirements.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index bb9c6f810..6853b9177 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -5,7 +5,7 @@ cffi==1.12.1 celery==3.1.26.post2 # pyup: <4 docopt==0.6.2 Flask-Bcrypt==0.7.1 -flask-marshmallow==0.9.0 +flask-marshmallow==0.10.0 Flask-Migrate==2.4.0 git+https://github.com/mitsuhiko/flask-sqlalchemy.git@500e732dd1b975a56ab06a46bd1a20a21e682262#egg=Flask-SQLAlchemy==2.3.2.dev20190108 Flask==1.0.2 diff --git a/requirements.txt b/requirements.txt index 1cadf7c00..0d1dd3003 100644 --- a/requirements.txt +++ b/requirements.txt @@ -7,7 +7,7 @@ cffi==1.12.1 celery==3.1.26.post2 # pyup: <4 docopt==0.6.2 Flask-Bcrypt==0.7.1 -flask-marshmallow==0.9.0 +flask-marshmallow==0.10.0 Flask-Migrate==2.4.0 git+https://github.com/mitsuhiko/flask-sqlalchemy.git@500e732dd1b975a56ab06a46bd1a20a21e682262#egg=Flask-SQLAlchemy==2.3.2.dev20190108 Flask==1.0.2 From 9da9968028fe9ac53d1827c930d410f6486093d7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 22 Mar 2019 14:06:45 +0000 Subject: [PATCH 18/19] downgrade error to info --- app/celery/tasks.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 490aa3d40..2928a29f3 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -484,7 +484,7 @@ def update_letter_notification(filename, temporary_failures, update): msg = "Update letter notification file {filename} failed: notification either not found " \ "or already updated from delivered. Status {status} for notification reference {reference}".format( filename=filename, status=status, reference=update.reference) - current_app.logger.error(msg) + current_app.logger.info(msg) def check_billable_units(notification_update): From ef515400f3870a3a428121611dda253c966ae414 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 22 Mar 2019 15:38:28 +0000 Subject: [PATCH 19/19] =?UTF-8?q?Fix=20automatic=20inheritance=20of=20org?= =?UTF-8?q?=E2=80=99s=20branding?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When creating a service it should inherit it’s organisation’s branding, if that organisation has branding. This wasn’t working because we were referring to the ID of the branding when making the association, not the branding itself. --- app/dao/services_dao.py | 8 ++++---- tests/app/service/test_rest.py | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 4bef1e99c..dae5f4f82 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -194,11 +194,11 @@ def dao_create_service( service.organisation = organisation - if organisation.email_branding_id: - service.email_branding = organisation.email_branding_id + if organisation.email_branding: + service.email_branding = organisation.email_branding - if organisation.letter_branding_id and not service.letter_branding: - service.letter_branding = organisation.letter_branding_id + if organisation.letter_branding and not service.letter_branding: + service.letter_branding = organisation.letter_branding db.session.add(service) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 80d4eff42..d8032cfbb 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -49,6 +49,7 @@ from tests.app.db import ( create_letter_branding, create_organisation, create_domain, + create_email_branding, ) from tests.app.db import create_user @@ -306,6 +307,37 @@ def test_create_service_with_domain_sets_organisation( assert json_resp['data']['organisation'] is None +def test_create_service_inherits_branding_from_organisation( + admin_request, + sample_user, +): + + org = create_organisation() + email_branding = create_email_branding() + org.email_branding = email_branding + letter_branding = create_letter_branding() + org.letter_branding = letter_branding + create_domain('example.gov.uk', org.id) + sample_user.email_address = 'test@example.gov.uk' + + json_resp = admin_request.post( + 'service.create_service', + _data={ + 'name': 'created service', + 'user_id': str(sample_user.id), + 'message_limit': 1000, + 'restricted': False, + 'active': False, + 'email_from': 'created.service', + 'created_by': str(sample_user.id), + }, + _expected_status=201 + ) + + assert json_resp['data']['email_branding'] == str(email_branding.id) + assert json_resp['data']['letter_branding'] == str(letter_branding.id) + + def test_create_service_with_domain_sets_letter_branding(admin_request, sample_user): letter_branding = create_letter_branding( name='test domain', filename='test-domain', domain='test.domain'