diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index e452befd1..56593ab1d 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -16,14 +16,14 @@ from app.celery.service_callback_tasks import ( create_delivery_status_callback_data, ) from app.config import QueueNames -from app.dao.inbound_sms_dao import delete_inbound_sms_created_more_than_a_week_ago +from app.dao.inbound_sms_dao import delete_inbound_sms_older_than_retention from app.dao.jobs_dao import ( dao_get_jobs_older_than_data_retention, dao_archive_job ) from app.dao.notifications_dao import ( dao_timeout_notifications, - delete_notifications_created_more_than_a_week_ago_by_type, + delete_notifications_older_than_retention_by_type, ) from app.dao.service_callback_api_dao import get_service_delivery_status_callback_api_for_service from app.exceptions import NotificationTechnicalFailureException @@ -64,10 +64,10 @@ def _remove_csv_files(job_types): @notify_celery.task(name="delete-sms-notifications") @cronitor("delete-sms-notifications") @statsd(namespace="tasks") -def delete_sms_notifications_older_than_seven_days(): +def delete_sms_notifications_older_than_retention(): try: start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('sms') + deleted = delete_notifications_older_than_retention_by_type('sms') current_app.logger.info( "Delete {} job started {} finished {} deleted {} sms notifications".format( 'sms', @@ -84,10 +84,10 @@ def delete_sms_notifications_older_than_seven_days(): @notify_celery.task(name="delete-email-notifications") @cronitor("delete-email-notifications") @statsd(namespace="tasks") -def delete_email_notifications_older_than_seven_days(): +def delete_email_notifications_older_than_retention(): try: start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('email') + deleted = delete_notifications_older_than_retention_by_type('email') current_app.logger.info( "Delete {} job started {} finished {} deleted {} email notifications".format( 'email', @@ -104,10 +104,10 @@ def delete_email_notifications_older_than_seven_days(): @notify_celery.task(name="delete-letter-notifications") @cronitor("delete-letter-notifications") @statsd(namespace="tasks") -def delete_letter_notifications_older_than_seven_days(): +def delete_letter_notifications_older_than_retention(): try: start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('letter') + deleted = delete_notifications_older_than_retention_by_type('letter') current_app.logger.info( "Delete {} job started {} finished {} deleted {} letter notifications".format( 'letter', @@ -190,10 +190,10 @@ def send_total_sent_notifications_to_performance_platform(day): @notify_celery.task(name="delete-inbound-sms") @cronitor("delete-inbound-sms") @statsd(namespace="tasks") -def delete_inbound_sms_older_than_seven_days(): +def delete_inbound_sms(): try: start = datetime.utcnow() - deleted = delete_inbound_sms_created_more_than_a_week_ago() + deleted = delete_inbound_sms_older_than_retention() current_app.logger.info( "Delete inbound sms job started {} finished {} deleted {} inbound sms notifications".format( start, diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py index ebe0cfaf9..3380047f3 100644 --- a/app/celery/process_ses_receipts_tasks.py +++ b/app/celery/process_ses_receipts_tasks.py @@ -42,10 +42,10 @@ def process_ses_results(self, response): notification = notifications_dao.dao_get_notification_by_reference(reference) except NoResultFound: message_time = iso8601.parse_date(ses_message['mail']['timestamp']).replace(tzinfo=None) - if datetime.utcnow() - message_time < timedelta(minutes=10): + if datetime.utcnow() - message_time < timedelta(minutes=5): self.retry(queue=QueueNames.RETRY) - elif datetime.utcnow() - message_time < timedelta(days=3): - current_app.logger.error( + else: + current_app.logger.warning( "notification not found for reference: {} (update to {})".format(reference, notification_status) ) return diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 22b0d468b..0b6334479 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -1,8 +1,3 @@ -from datetime import ( - timedelta, - datetime, - date -) from flask import current_app from notifications_utils.statsd_decorators import statsd from sqlalchemy import desc, and_ @@ -10,8 +5,8 @@ from sqlalchemy.orm import aliased from app import db from app.dao.dao_utils import transactional -from app.models import InboundSms -from app.utils import get_london_midnight_in_utc +from app.models import InboundSms, Service, ServiceDataRetention, SMS_TYPE +from app.utils import midnight_n_days_ago @transactional @@ -19,14 +14,15 @@ def dao_create_inbound_sms(inbound_sms): db.session.add(inbound_sms) -def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None): - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) +def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, limit_days=7): q = InboundSms.query.filter( - InboundSms.service_id == service_id, - InboundSms.created_at >= start_date + InboundSms.service_id == service_id ).order_by( InboundSms.created_at.desc() ) + if limit_days is not None: + start_date = midnight_n_days_ago(limit_days) + q = q.filter(InboundSms.created_at >= start_date) if user_number: q = q.filter(InboundSms.user_number == user_number) @@ -60,21 +56,63 @@ def dao_get_paginated_inbound_sms_for_service_for_public_api( def dao_count_inbound_sms_for_service(service_id): - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) + start_date = midnight_n_days_ago(6) return InboundSms.query.filter( InboundSms.service_id == service_id, InboundSms.created_at >= start_date ).count() +def _delete_inbound_sms(datetime_to_delete_from, query_filter): + query_limit = 10000 + + subquery = db.session.query( + InboundSms.id + ).filter( + InboundSms.created_at < datetime_to_delete_from, + *query_filter + ).limit( + query_limit + ).subquery() + + deleted = 0 + # set to nonzero just to enter the loop + number_deleted = 1 + while number_deleted > 0: + number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch') + deleted += number_deleted + + return deleted + + @statsd(namespace="dao") @transactional -def delete_inbound_sms_created_more_than_a_week_ago(): - seven_days_ago = datetime.utcnow() - timedelta(days=7) +def delete_inbound_sms_older_than_retention(): + current_app.logger.info('Deleting inbound sms for services with flexible data retention') - deleted = db.session.query(InboundSms).filter( - InboundSms.created_at < seven_days_ago - ).delete(synchronize_session='fetch') + flexible_data_retention = ServiceDataRetention.query.join( + ServiceDataRetention.service, + Service.inbound_number + ).filter( + ServiceDataRetention.notification_type == SMS_TYPE + ).all() + + deleted = 0 + for f in flexible_data_retention: + n_days_ago = midnight_n_days_ago(f.days_of_retention) + + current_app.logger.info("Deleting inbound sms for service id: {}".format(f.service_id)) + deleted += _delete_inbound_sms(n_days_ago, query_filter=[InboundSms.service_id == f.service_id]) + + current_app.logger.info('Deleting inbound sms for services without flexible data retention') + + seven_days_ago = midnight_n_days_ago(7) + + deleted += _delete_inbound_sms(seven_days_ago, query_filter=[ + InboundSms.service_id.notin_(x.service_id for x in flexible_data_retention), + ]) + + current_app.logger.info('Deleted {} inbound sms'.format(deleted)) return deleted @@ -108,7 +146,7 @@ def dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service( ORDER BY t1.created_at DESC; LIMIT 50 OFFSET :page """ - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) + start_date = midnight_n_days_ago(6) t2 = aliased(InboundSms) q = db.session.query( InboundSms diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 722f03165..4b9b6c7e9 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -293,7 +293,7 @@ def _filter_query(query, filter_dict=None): @statsd(namespace="dao") -def delete_notifications_created_more_than_a_week_ago_by_type(notification_type, qry_limit=10000): +def delete_notifications_older_than_retention_by_type(notification_type, qry_limit=10000): current_app.logger.info( 'Deleting {} notifications for services with flexible data retention'.format(notification_type)) diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index 94cdeb237..26b6dca1f 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -1,7 +1,10 @@ +from sqlalchemy.sql.expression import func + from app import db from app.dao.dao_utils import transactional from app.models import ( Organisation, + Domain, InvitedOrganisationUser, User ) @@ -23,6 +26,21 @@ def dao_get_organisation_by_id(organisation_id): return Organisation.query.filter_by(id=organisation_id).one() +def dao_get_organisation_by_email_address(email_address): + + email_address = email_address.lower() + + for domain in Domain.query.order_by(func.char_length(Domain.domain).desc()).all(): + + if ( + email_address.endswith("@{}".format(domain.domain)) or + email_address.endswith(".{}".format(domain.domain)) + ): + return Organisation.query.filter_by(id=domain.organisation_id).one() + + return None + + def dao_get_organisation_by_service_id(service_id): return Organisation.query.join(Organisation.services).filter_by(id=service_id).first() @@ -34,10 +52,26 @@ def dao_create_organisation(organisation): @transactional def dao_update_organisation(organisation_id, **kwargs): - return Organisation.query.filter_by(id=organisation_id).update( + + domains = kwargs.pop('domains', []) + + organisation = Organisation.query.filter_by(id=organisation_id).update( kwargs ) + if isinstance(domains, list): + + Domain.query.filter_by(organisation_id=organisation_id).delete() + + db.session.bulk_save_objects([ + Domain(domain=domain.lower(), organisation_id=organisation_id) + for domain in domains + ]) + + db.session.commit() + + return organisation + @transactional def dao_add_service_to_organisation(service, organisation_id): diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index eb766c06b..70d7a8a83 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -11,6 +11,7 @@ from app.dao.dao_utils import ( transactional, version_class ) +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.models import ( @@ -151,14 +152,25 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user, service_id=None, service_permissions=None, letter_branding=None): +def dao_create_service( + service, + user, + service_id=None, + service_permissions=None, + letter_branding=None, +): # the default property does not appear to work when there is a difference between the sqlalchemy schema and the # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender # migration is completed, this code should be able to be removed. + if not user: + raise ValueError("Can't create a service without a user") + if service_permissions is None: service_permissions = DEFAULT_SERVICE_PERMISSIONS + organisation = dao_get_organisation_by_email_address(user.email_address) + from app.dao.permissions_dao import permission_dao service.users.append(user) permission_dao.add_default_service_permissions_for_user(user, service) @@ -173,8 +185,20 @@ def dao_create_service(service, user, service_id=None, service_permissions=None, # do we just add the default - or will we get a value from FE? insert_service_sms_sender(service, current_app.config['FROM_NUMBER']) + if letter_branding: service.letter_branding = letter_branding + + if organisation: + + service.organisation = organisation + + if organisation.email_branding_id: + service.email_branding = organisation.email_branding_id + + if organisation.letter_branding_id and not service.letter_branding: + service.letter_branding = organisation.letter_branding_id + db.session.add(service) diff --git a/app/models.py b/app/models.py index c2c92bb4f..57de4a046 100644 --- a/app/models.py +++ b/app/models.py @@ -316,6 +316,12 @@ organisation_to_service = db.Table( ) +class Domain(db.Model): + __tablename__ = "domain" + domain = db.Column(db.String(255), primary_key=True) + organisation_id = db.Column('organisation_id', UUID(as_uuid=True), db.ForeignKey('organisation.id'), nullable=False) + + class Organisation(db.Model): __tablename__ = "organisation" id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4, unique=False) @@ -329,15 +335,50 @@ class Organisation(db.Model): secondary='organisation_to_service', uselist=True) + agreement_signed = db.Column(db.Boolean, nullable=True) + agreement_signed_at = db.Column(db.DateTime, nullable=True) + agreement_signed_by_id = db.Column( + UUID(as_uuid=True), + db.ForeignKey('users.id'), + nullable=True, + ) + agreement_signed_version = db.Column(db.Float, nullable=True) + crown = db.Column(db.Boolean, nullable=True) + organisation_type = db.Column(db.String(255), nullable=True) + + domains = db.relationship( + 'Domain', + ) + + email_branding = db.relationship('EmailBranding') + email_branding_id = db.Column( + UUID(as_uuid=True), + db.ForeignKey('email_branding.id'), + nullable=True, + ) + + letter_branding = db.relationship('LetterBranding') + letter_branding_id = db.Column( + UUID(as_uuid=True), + db.ForeignKey('letter_branding.id'), + nullable=True, + ) + def serialize(self): - serialized = { + return { "id": str(self.id), "name": self.name, "active": self.active, + "crown": self.crown, + "organisation_type": self.organisation_type, + "letter_branding_id": self.letter_branding_id, + "email_branding_id": self.email_branding_id, + "agreement_signed": self.agreement_signed, + "agreement_signed_at": self.agreement_signed_at, + "agreement_signed_by_id": self.agreement_signed_by_id, + "agreement_signed_version": self.agreement_signed_version, } - return serialized - class Service(db.Model, Versioned): __tablename__ = 'services' diff --git a/migrations/versions/0278_add_more_stuff_to_orgs.py b/migrations/versions/0278_add_more_stuff_to_orgs.py new file mode 100644 index 000000000..745c43fed --- /dev/null +++ b/migrations/versions/0278_add_more_stuff_to_orgs.py @@ -0,0 +1,57 @@ +""" + +Revision ID: 0278_add_more_stuff_to_orgs +Revises: 0277_consent_to_research_null +Create Date: 2019-02-26 10:15:22.430340 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0278_add_more_stuff_to_orgs' +down_revision = '0277_consent_to_research_null' + + +def upgrade(): + op.create_table( + 'domain', + sa.Column('domain', sa.String(length=255), nullable=False), + sa.Column('organisation_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.ForeignKeyConstraint(['organisation_id'], ['organisation.id'], ), + sa.PrimaryKeyConstraint('domain') + ) + op.create_index(op.f('ix_domain_domain'), 'domain', ['domain'], unique=True) + + op.add_column('organisation', sa.Column('email_branding_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.create_foreign_key('fk_organisation_email_branding_id', 'organisation', 'email_branding', ['email_branding_id'], ['id']) + + op.add_column('organisation', sa.Column('letter_branding_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.create_foreign_key('fk_organisation_letter_branding_id', 'organisation', 'letter_branding', ['letter_branding_id'], ['id']) + + op.add_column('organisation', sa.Column('agreement_signed', sa.Boolean(), nullable=True)) + op.add_column('organisation', sa.Column('agreement_signed_at', sa.DateTime(), nullable=True)) + op.add_column('organisation', sa.Column('agreement_signed_by_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.add_column('organisation', sa.Column('agreement_signed_version', sa.Float(), nullable=True)) + op.add_column('organisation', sa.Column('crown', sa.Boolean(), nullable=True)) + op.add_column('organisation', sa.Column('organisation_type', sa.String(length=255), nullable=True)) + op.create_foreign_key('fk_organisation_agreement_user_id', 'organisation', 'users', ['agreement_signed_by_id'], ['id']) + + +def downgrade(): + op.drop_constraint('fk_organisation_agreement_user_id', 'organisation', type_='foreignkey') + op.drop_column('organisation', 'organisation_type') + op.drop_column('organisation', 'crown') + op.drop_column('organisation', 'agreement_signed_version') + op.drop_column('organisation', 'agreement_signed_by_id') + op.drop_column('organisation', 'agreement_signed_at') + op.drop_column('organisation', 'agreement_signed') + + op.drop_constraint('fk_organisation_email_branding_id', 'organisation', type_='foreignkey') + op.drop_column('organisation', 'email_branding_id') + + op.drop_constraint('fk_organisation_letter_branding_id', 'organisation', type_='foreignkey') + op.drop_column('organisation', 'letter_branding_id') + + op.drop_index(op.f('ix_domain_domain'), table_name='domain') + op.drop_table('domain') diff --git a/requirements_for_test.txt b/requirements_for_test.txt index fa6a0089d..2f64621bc 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,5 +1,5 @@ -r requirements.txt -flake8==3.7.6 +flake8==3.7.7 moto==1.3.7 pytest==3.10.1 # pyup: <4 pytest-env==0.6.2 diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index 93e047448..179aa95ff 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -11,10 +11,10 @@ from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient from app.celery import nightly_tasks from app.celery.nightly_tasks import ( delete_dvla_response_files_older_than_seven_days, - delete_email_notifications_older_than_seven_days, - delete_inbound_sms_older_than_seven_days, - delete_letter_notifications_older_than_seven_days, - delete_sms_notifications_older_than_seven_days, + delete_email_notifications_older_than_retention, + delete_inbound_sms, + delete_letter_notifications_older_than_retention, + delete_sms_notifications_older_than_retention, raise_alert_if_letter_notifications_still_sending, remove_letter_csv_files, remove_sms_email_csv_files, @@ -157,21 +157,21 @@ def test_remove_csv_files_filters_by_type(mocker, sample_service): def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker): - mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_sms_notifications_older_than_seven_days() + mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + delete_sms_notifications_older_than_retention() mocked.assert_called_once_with('sms') def test_should_call_delete_email_notifications_more_than_week_in_task(notify_api, mocker): mocked_notifications = mocker.patch( - 'app.celery.nightly_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_email_notifications_older_than_seven_days() + 'app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + delete_email_notifications_older_than_retention() mocked_notifications.assert_called_once_with('email') def test_should_call_delete_letter_notifications_more_than_week_in_task(notify_api, mocker): - mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_letter_notifications_older_than_seven_days() + mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + delete_letter_notifications_older_than_retention() mocked.assert_called_once_with('letter') @@ -291,10 +291,10 @@ def test_send_total_sent_notifications_to_performance_platform_calls_with_correc ]) -def test_should_call_delete_inbound_sms_older_than_seven_days(notify_api, mocker): - mocker.patch('app.celery.nightly_tasks.delete_inbound_sms_created_more_than_a_week_ago') - delete_inbound_sms_older_than_seven_days() - assert nightly_tasks.delete_inbound_sms_created_more_than_a_week_ago.call_count == 1 +def test_should_call_delete_inbound_sms(notify_api, mocker): + mocker.patch('app.celery.nightly_tasks.delete_inbound_sms_older_than_retention') + delete_inbound_sms() + assert nightly_tasks.delete_inbound_sms_older_than_retention.call_count == 1 @freeze_time('2017-01-01 10:00:00') diff --git a/tests/app/celery/test_process_ses_receipts_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py index 3cd27cfd1..0905f1069 100644 --- a/tests/app/celery/test_process_ses_receipts_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -117,7 +117,7 @@ def test_ses_callback_should_retry_if_notification_is_new(client, notify_db, moc def test_ses_callback_should_log_if_notification_is_missing(client, notify_db, mocker): mock_retry = mocker.patch('app.celery.process_ses_receipts_tasks.process_ses_results.retry') - mock_logger = mocker.patch('app.celery.process_ses_receipts_tasks.current_app.logger.error') + mock_logger = mocker.patch('app.celery.process_ses_receipts_tasks.current_app.logger.warning') with freeze_time('2017-11-17T12:34:03.646Z'): assert process_ses_results(ses_notification_callback(reference='ref')) is None diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 0776a45fe..91df8ec72 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -19,7 +19,7 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, dao_update_notification, dao_update_notifications_by_reference, - delete_notifications_created_more_than_a_week_ago_by_type, + delete_notifications_older_than_retention_by_type, get_notification_by_id, get_notification_for_job, get_notification_with_personalisation, @@ -79,7 +79,7 @@ def test_should_have_decorated_notifications_dao_functions(): assert get_notification_with_personalisation.__wrapped__.__name__ == 'get_notification_with_personalisation' # noqa assert get_notifications_for_service.__wrapped__.__name__ == 'get_notifications_for_service' # noqa assert get_notification_by_id.__wrapped__.__name__ == 'get_notification_by_id' # noqa - assert delete_notifications_created_more_than_a_week_ago_by_type.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago_by_type' # noqa + assert delete_notifications_older_than_retention_by_type.__wrapped__.__name__ == 'delete_notifications_older_than_retention_by_type' # noqa assert dao_delete_notifications_and_history_by_id.__wrapped__.__name__ == 'dao_delete_notifications_and_history_by_id' # noqa diff --git a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py index 4b3d8c9e8..3e913a673 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py +++ b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py @@ -6,7 +6,7 @@ from datetime import ( import pytest from flask import current_app from freezegun import freeze_time -from app.dao.notifications_dao import delete_notifications_created_more_than_a_week_ago_by_type +from app.dao.notifications_dao import delete_notifications_older_than_retention_by_type from app.models import Notification, NotificationHistory from tests.app.db import ( create_template, @@ -48,7 +48,7 @@ def test_should_delete_notifications_by_type_after_seven_days( assert len(all_notifications) == 30 # Records from before 3rd should be deleted with freeze_time(delete_run_time): - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) remaining_sms_notifications = Notification.query.filter_by(notification_type='sms').all() remaining_letter_notifications = Notification.query.filter_by(notification_type='letter').all() remaining_email_notifications = Notification.query.filter_by(notification_type='email').all() @@ -77,7 +77,7 @@ def test_should_not_delete_notification_history(sample_service, notification_typ create_notification(template=letter_template, status='permanent-failure') assert Notification.query.count() == 3 assert NotificationHistory.query.count() == 3 - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) assert Notification.query.count() == 2 assert NotificationHistory.query.count() == 3 @@ -109,7 +109,7 @@ def test_delete_notifications_for_days_of_retention(sample_service, notification created_at=datetime.utcnow() - timedelta(days=8)) create_service_data_retention(service_id=sample_service.id, notification_type=notification_type) assert len(Notification.query.all()) == 9 - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) assert len(Notification.query.all()) == 7 assert len(Notification.query.filter_by(notification_type=notification_type).all()) == 1 if notification_type == 'letter': @@ -146,7 +146,7 @@ def test_delete_notifications_keep_data_for_days_of_retention_is_longer(sample_s create_notification(template=default_letter_template, status='temporary-failure', created_at=datetime.utcnow() - timedelta(days=8)) assert len(Notification.query.all()) == 9 - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) assert len(Notification.query.filter_by().all()) == 8 assert len(Notification.query.filter_by(notification_type=notification_type).all()) == 2 if notification_type == 'letter': @@ -171,7 +171,7 @@ def test_delete_notifications_delete_notification_type_for_default_time_if_no_da create_notification(template=letter_template, status='temporary-failure', created_at=datetime.utcnow() - timedelta(days=14)) assert len(Notification.query.all()) == 6 - delete_notifications_created_more_than_a_week_ago_by_type('email') + delete_notifications_older_than_retention_by_type('email') assert len(Notification.query.filter_by().all()) == 5 assert len(Notification.query.filter_by(notification_type='email').all()) == 1 @@ -182,7 +182,7 @@ def test_delete_notifications_does_try_to_delete_from_s3_when_letter_has_not_bee create_notification(template=letter_template, status='sending', reference='LETTER_REF') - delete_notifications_created_more_than_a_week_ago_by_type('email', qry_limit=1) + delete_notifications_older_than_retention_by_type('email', qry_limit=1) mock_get_s3.assert_not_called() @@ -196,7 +196,7 @@ def test_delete_notifications_calls_subquery( create_notification(template=sms_template, created_at=datetime.now() - timedelta(days=8)) assert Notification.query.count() == 3 - delete_notifications_created_more_than_a_week_ago_by_type('sms', qry_limit=1) + delete_notifications_older_than_retention_by_type('sms', qry_limit=1) assert Notification.query.count() == 0 diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 6fd43a47b..24eb1761c 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -1,19 +1,18 @@ -from datetime import datetime, timedelta +from datetime import datetime +from itertools import product from freezegun import freeze_time from app.dao.inbound_sms_dao import ( dao_get_inbound_sms_for_service, dao_count_inbound_sms_for_service, - delete_inbound_sms_created_more_than_a_week_ago, + delete_inbound_sms_older_than_retention, dao_get_inbound_sms_by_id, dao_get_paginated_inbound_sms_for_service_for_public_api, dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service ) from tests.conftest import set_config -from tests.app.db import create_inbound_sms, create_service - -from app.models import InboundSms +from tests.app.db import create_inbound_sms, create_service, create_service_data_retention def test_get_all_inbound_sms(sample_service): @@ -59,15 +58,10 @@ def test_get_all_inbound_sms_filters_on_service(notify_db_session): def test_get_all_inbound_sms_filters_on_time(sample_service, notify_db_session): - create_inbound_sms(sample_service, user_number='447700900111', content='111 1', created_at=datetime(2017, 1, 2)) - sms_two = create_inbound_sms( - sample_service, - user_number='447700900111', - content='111 2', - created_at=datetime(2017, 1, 3) - ) + create_inbound_sms(sample_service, created_at=datetime(2017, 8, 6, 22, 59)) # sunday evening + sms_two = create_inbound_sms(sample_service, created_at=datetime(2017, 8, 6, 23, 0)) # monday (7th) morning - with freeze_time('2017-01-09'): + with freeze_time('2017-08-14 12:00'): res = dao_get_inbound_sms_for_service(sample_service.id) assert len(res) == 1 @@ -93,28 +87,43 @@ def test_count_inbound_sms_for_service_filters_messages_older_than_seven_days(sa assert dao_count_inbound_sms_for_service(sample_service.id) == 1 -@freeze_time("2017-01-01 12:00:00") -def test_should_delete_inbound_sms_older_than_seven_days(sample_service): - older_than_seven_days = datetime.utcnow() - timedelta(days=7, seconds=1) - create_inbound_sms(sample_service, created_at=older_than_seven_days) - delete_inbound_sms_created_more_than_a_week_ago() +@freeze_time("2017-06-08 12:00:00") +def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session): + no_retention_service = create_service(service_name='no retention') + short_retention_service = create_service(service_name='three days') + long_retention_service = create_service(service_name='thirty days') - assert len(InboundSms.query.all()) == 0 + services = [short_retention_service, no_retention_service, long_retention_service] + create_service_data_retention(long_retention_service.id, notification_type='sms', days_of_retention=30) + create_service_data_retention(short_retention_service.id, notification_type='sms', days_of_retention=3) + # email retention doesn't affect anything + create_service_data_retention(short_retention_service.id, notification_type='email', days_of_retention=4) -@freeze_time("2017-01-01 12:00:00") -def test_should_not_delete_inbound_sms_before_seven_days(sample_service): - yesterday = datetime.utcnow() - timedelta(days=1) - just_before_seven_days = datetime.utcnow() - timedelta(days=6, hours=23, minutes=59, seconds=59) - older_than_seven_days = datetime.utcnow() - timedelta(days=7, seconds=1) + dates = [ + datetime(2017, 6, 4, 23, 00), # just before three days + datetime(2017, 6, 4, 22, 59), # older than three days + datetime(2017, 5, 31, 23, 00), # just before seven days + datetime(2017, 5, 31, 22, 59), # older than seven days + datetime(2017, 5, 1, 0, 0), # older than thirty days + ] - create_inbound_sms(sample_service, created_at=yesterday) - create_inbound_sms(sample_service, created_at=just_before_seven_days) - create_inbound_sms(sample_service, created_at=older_than_seven_days) + for date, service in product(dates, services): + create_inbound_sms(service, created_at=date) - delete_inbound_sms_created_more_than_a_week_ago() + deleted_count = delete_inbound_sms_older_than_retention() - assert len(InboundSms.query.all()) == 2 + # four deleted for the 3-day service, two for the default seven days one, one for the 30 day + assert deleted_count == 7 + assert { + x.created_at for x in dao_get_inbound_sms_for_service(short_retention_service.id, limit_days=None) + } == set(dates[:1]) + assert { + x.created_at for x in dao_get_inbound_sms_for_service(no_retention_service.id, limit_days=None) + } == set(dates[:3]) + assert { + x.created_at for x in dao_get_inbound_sms_for_service(long_retention_service.id, limit_days=None) + } == set(dates[:4]) def test_get_inbound_sms_by_id_returns(sample_service): diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py index 62c81041a..667fc1438 100644 --- a/tests/app/dao/test_organisation_dao.py +++ b/tests/app/dao/test_organisation_dao.py @@ -1,3 +1,4 @@ +import datetime import uuid import pytest @@ -5,6 +6,7 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from app.dao.organisation_dao import ( dao_get_organisations, + dao_get_organisation_by_email_address, dao_get_organisation_by_id, dao_get_organisation_by_service_id, dao_get_organisation_services, @@ -16,7 +18,14 @@ from app.dao.organisation_dao import ( ) from app.models import 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_organisations_gets_all_organisations_alphabetically_with_active_organisations_first( @@ -47,19 +56,67 @@ def test_get_organisation_by_id_gets_correct_organisation(notify_db, notify_db_s assert organisation_from_db == organisation -def test_update_organisation(notify_db, notify_db_session): - updated_name = 'new name' +def test_update_organisation( + notify_db, + notify_db_session, +): + create_organisation() + + organisation = Organisation.query.one() + user = create_user() + email_branding = create_email_branding() + letter_branding = create_letter_branding() + + data = { + 'name': 'new name', + "crown": True, + "organisation_type": 'local', + "agreement_signed": True, + "agreement_signed_at": datetime.datetime.utcnow(), + "agreement_signed_by_id": user.id, + "agreement_signed_version": 999.99, + "letter_branding_id": letter_branding.id, + "email_branding_id": email_branding.id, + } + + for attribute, value in data.items(): + assert getattr(organisation, attribute) != value + + dao_update_organisation(organisation.id, **data) + + organisation = Organisation.query.one() + + for attribute, value in data.items(): + assert getattr(organisation, attribute) == value + + +@pytest.mark.parametrize('domain_list, expected_domains', ( + (['abc', 'def'], {'abc', 'def'}), + (['ABC', 'DEF'], {'abc', 'def'}), + ([], set()), + (None, {'123', '456'}), + pytest.param( + ['abc', 'ABC'], {'abc'}, + marks=pytest.mark.xfail(raises=IntegrityError) + ), +)) +def test_update_organisation_domains_lowercases( + notify_db, + notify_db_session, + domain_list, + expected_domains, +): create_organisation() organisation = Organisation.query.one() - assert organisation.name != updated_name + # Seed some domains + dao_update_organisation(organisation.id, domains=['123', '456']) - dao_update_organisation(organisation.id, **{'name': updated_name}) + # This should overwrite the seeded domains + dao_update_organisation(organisation.id, domains=domain_list) - organisation = Organisation.query.one() - - assert organisation.name == updated_name + assert {domain.domain for domain in organisation.domains} == expected_domains def test_add_service_to_organisation(notify_db, notify_db_session, sample_service, sample_organisation): @@ -171,3 +228,30 @@ def test_add_user_to_organisation_when_user_does_not_exist(sample_organisation): def test_add_user_to_organisation_when_organisation_does_not_exist(sample_user): with pytest.raises(expected_exception=SQLAlchemyError): dao_add_user_to_organisation(organisation_id=uuid.uuid4(), user_id=sample_user.id) + + +@pytest.mark.parametrize('domain, expected_org', ( + ('unknown.gov.uk', False), + ('example.gov.uk', True), +)) +def test_get_organisation_by_email_address( + admin_request, + sample_user, + domain, + expected_org, +): + + org = create_organisation() + create_domain('example.gov.uk', org.id) + create_domain('test.gov.uk', org.id) + + another_org = create_organisation(name='Another') + create_domain('cabinet-office.gov.uk', another_org.id) + create_domain('cabinetoffice.gov.uk', another_org.id) + + found_org = dao_get_organisation_by_email_address('test@{}'.format(domain)) + + if expected_org: + assert found_org is org + else: + assert found_org is None diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 6461fd93c..cbc6cb24c 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -4,7 +4,7 @@ from datetime import datetime import pytest from freezegun import freeze_time from sqlalchemy.exc import IntegrityError -from sqlalchemy.orm.exc import FlushError, NoResultFound +from sqlalchemy.orm.exc import NoResultFound from app import db from app.dao.inbound_numbers_dao import ( @@ -167,9 +167,9 @@ def test_cannot_create_service_with_no_user(notify_db_session): message_limit=1000, restricted=False, created_by=user) - with pytest.raises(FlushError) as excinfo: + with pytest.raises(ValueError) as excinfo: dao_create_service(service, None) - assert "Can't flush None value found in collection Service.users" in str(excinfo.value) + assert "Can't create a service without a user" in str(excinfo.value) def test_should_add_user_to_service(notify_db_session): diff --git a/tests/app/db.py b/tests/app/db.py index 1b301a2f2..8c082350a 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,3 +1,4 @@ +import random import uuid from datetime import datetime, date @@ -51,7 +52,8 @@ from app.models import ( Complaint, InvitedUser, TemplateFolder, - LetterBranding + LetterBranding, + Domain, ) @@ -324,10 +326,18 @@ def create_inbound_sms( provider="mmg", created_at=None ): + if not service.inbound_number: + create_inbound_number( + # create random inbound number + notify_number or '07{:09}'.format(random.randint(0, 1e9 - 1)), + provider=provider, + service_id=service.id + ) + inbound = InboundSms( service=service, created_at=created_at or datetime.utcnow(), - notify_number=notify_number or service.get_default_sms_sender(), + notify_number=service.get_inbound_number(), user_number=user_number, provider_date=provider_date or datetime.utcnow(), provider_reference=provider_reference or 'foo', @@ -497,6 +507,16 @@ def create_annual_billing( return annual_billing +def create_domain(domain, organisation_id): + + domain = Domain(domain=domain, organisation_id=organisation_id) + + db.session.add(domain) + db.session.commit() + + return domain + + def create_organisation(name='test_org_1', active=True): data = { 'name': name, diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index dee403245..1bd0fecf7 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -32,10 +32,29 @@ def test_get_organisation_by_id(admin_request, notify_db_session): organisation_id=org.id ) - assert set(response.keys()) == {'id', 'name', 'active'} + assert set(response.keys()) == { + 'id', + 'name', + 'active', + 'crown', + 'organisation_type', + 'agreement_signed', + 'agreement_signed_at', + 'agreement_signed_by_id', + 'agreement_signed_version', + 'letter_branding_id', + 'email_branding_id', + } assert response['id'] == str(org.id) assert response['name'] == 'test_org_1' assert response['active'] is True + assert response['crown'] is None + assert response['organisation_type'] is None + assert response['agreement_signed'] is None + assert response['agreement_signed_by_id'] is None + assert response['agreement_signed_version'] is None + assert response['letter_branding_id'] is None + assert response['email_branding_id'] is None def test_post_create_organisation(admin_request, notify_db_session): @@ -91,12 +110,27 @@ def test_post_create_organisation_with_missing_name_gives_validation_error(admin assert response['errors'][0]['message'] == 'name is a required property' -def test_post_update_organisation_updates_fields(admin_request, notify_db_session): +@pytest.mark.parametrize('agreement_signed', ( + None, True, False +)) +@pytest.mark.parametrize('crown', ( + None, True, False +)) +def test_post_update_organisation_updates_fields( + admin_request, + notify_db_session, + agreement_signed, + crown, +): org = create_organisation() data = { 'name': 'new organisation name', - 'active': False + 'active': False, + 'agreement_signed': agreement_signed, + 'crown': crown, } + assert org.agreement_signed is None + assert org.crown is None admin_request.post( 'organisation.update_organisation', @@ -111,6 +145,39 @@ def test_post_update_organisation_updates_fields(admin_request, notify_db_sessio assert organisation[0].id == org.id assert organisation[0].name == data['name'] assert organisation[0].active == data['active'] + assert organisation[0].agreement_signed == agreement_signed + assert organisation[0].crown == crown + assert organisation[0].domains == [] + + +@pytest.mark.parametrize('domain_list', ( + ['example.com'], + ['example.com', 'example.org', 'example.net'], + [], +)) +def test_post_update_organisation_updates_domains( + admin_request, + notify_db_session, + domain_list, +): + org = create_organisation(name='test_org_2') + data = { + 'domains': domain_list, + } + + admin_request.post( + 'organisation.update_organisation', + _data=data, + organisation_id=org.id, + _expected_status=204 + ) + + organisation = Organisation.query.all() + + assert len(organisation) == 1 + assert [ + domain.domain for domain in organisation[0].domains + ] == domain_list def test_post_update_organisation_raises_400_on_existing_org_name( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 22208dd52..744f509e9 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -44,7 +44,9 @@ from tests.app.db import ( create_inbound_number, create_service_sms_sender, create_service_with_defined_sms_sender, - create_letter_branding + create_letter_branding, + create_organisation, + create_domain, ) from tests.app.db import create_user @@ -253,6 +255,55 @@ def test_create_service(admin_request, sample_user): assert service_sms_senders[0].sms_sender == current_app.config['FROM_NUMBER'] +@pytest.mark.parametrize('domain, expected_org', ( + (None, False), + ('', False), + ('unknown.gov.uk', False), + ('unknown-example.gov.uk', False), + ('example.gov.uk', True), + ('test.gov.uk', True), + ('test.example.gov.uk', True), +)) +def test_create_service_with_domain_sets_organisation( + admin_request, + sample_user, + domain, + expected_org, +): + + red_herring_org = create_organisation(name='Sub example') + create_domain('specific.example.gov.uk', red_herring_org.id) + create_domain('aaaaaaaa.example.gov.uk', red_herring_org.id) + + org = create_organisation() + create_domain('example.gov.uk', org.id) + create_domain('test.gov.uk', org.id) + + another_org = create_organisation(name='Another') + create_domain('cabinet-office.gov.uk', another_org.id) + create_domain('cabinetoffice.gov.uk', another_org.id) + + sample_user.email_address = 'test@{}'.format(domain) + + 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), + 'service_domain': domain, + } + + json_resp = admin_request.post('service.create_service', _data=data, _expected_status=201) + + if expected_org: + assert json_resp['data']['organisation'] == str(org.id) + else: + assert json_resp['data']['organisation'] is None + + 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'