diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 9149b72cc..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,21 +117,37 @@ 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 letters in group_letters(letter_pdfs): + for i, letters in enumerate(group_letters(letter_pdfs)): 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 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/app/celery/tasks.py b/app/celery/tasks.py index 1948c0964..2928a29f3 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) @@ -485,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): 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/app/dao/services_dao.py b/app/dao/services_dao.py index e881bf020..dae5f4f82 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, @@ -193,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) @@ -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/models.py b/app/models.py index 57de4a046..b97c0dc76 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 @@ -377,6 +378,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 + ], } @@ -1692,6 +1696,7 @@ class InvitedUser(db.Model): nullable=False, default=SMS_AUTH_TYPE ) + 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/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..ecaa3a58b 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,13 +286,15 @@ 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_schema.load(user_permissions, many=True).data - dao_add_user_to_service(service, user, permissions) + permissions = [ + 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, folder_permissions) data = service_schema.dump(service).data return jsonify(data=data), 201 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') 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) diff --git a/requirements-app.txt b/requirements-app.txt index 5f9b3cff6..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 @@ -13,9 +13,9 @@ click-datetime==0.2 eventlet==0.23.0 gunicorn==19.7.1 iso8601==0.1.12 -jsonschema==3.0.0b3 -marshmallow-sqlalchemy==0.16.0 -marshmallow==2.18.1 +jsonschema==3.0.1 +marshmallow-sqlalchemy==0.16.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 4bbd6cde4..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 @@ -15,9 +15,9 @@ click-datetime==0.2 eventlet==0.23.0 gunicorn==19.7.1 iso8601==0.1.12 -jsonschema==3.0.0b3 -marshmallow-sqlalchemy==0.16.0 -marshmallow==2.18.1 +jsonschema==3.0.1 +marshmallow-sqlalchemy==0.16.1 +marshmallow==2.19.1 psycopg2-binary==2.7.7 PyJWT==1.7.1 SQLAlchemy==1.2.18 @@ -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/requirements_for_test.txt b/requirements_for_test.txt index bf09cd6d8..032bae1a7 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -5,8 +5,8 @@ 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 coveralls==1.7.0 +pytest-xdist==1.27.0 freezegun==0.3.11 requests-mock==1.5.2 # optional requirements for jsonschema 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/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()) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 4a849a005..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']}, + 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']}, + kwargs={ + 'filenames_to_zip': ['C.pdf'], + 'upload_filename': 'NOTIFY.2017-01-02.002.tdr7hcdPieiqjkVoS4kU.ZIP' + }, queue='process-ftp-tasks', compression='zlib' ) 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/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/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/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' 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) diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index 1bd0fecf7..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_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): @@ -44,6 +51,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 +63,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): @@ -180,6 +208,56 @@ 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_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() diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 83757fb94..d8032cfbb 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, @@ -47,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 @@ -304,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' @@ -1080,81 +1114,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, @@ -1197,7 +1157,8 @@ def test_add_existing_user_to_another_service_with_all_permissions_with_new_data {"permission": "manage_api_keys"}, {"permission": "manage_templates"}, {"permission": "view_activity"}, - ] + ], + "folder_permissions": [] } auth_header = create_authorization_header() @@ -1250,9 +1211,14 @@ 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"}, + ], + "folder_permissions": [] + } auth_header = create_authorization_header() @@ -1293,9 +1259,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() @@ -1320,6 +1290,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, @@ -1336,7 +1347,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()