diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 6d09fa785..cb9a40c88 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -25,8 +25,8 @@ from app.models import ( EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, - KEY_TYPE_NORMAL -) + KEY_TYPE_NORMAL, + JOB_STATUS_CANCELLED, JOB_STATUS_PENDING, JOB_STATUS_IN_PROGRESS, JOB_STATUS_FINISHED) from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd @@ -38,15 +38,22 @@ def process_job(job_id): start = datetime.utcnow() job = dao_get_job_by_id(job_id) - if job.job_status != 'pending': + if job.job_status != JOB_STATUS_PENDING: return service = job.service + if not service.active: + job.job_status = JOB_STATUS_CANCELLED + dao_update_job(job) + current_app.logger.warn( + "Job {} has been cancelled, service {} is inactive".format(job_id, service.id)) + return + if __sending_limits_for_job_exceeded(service, job, job_id): return - job.job_status = 'in progress' + job.job_status = JOB_STATUS_IN_PROGRESS dao_update_job(job) db_template = dao_get_template_by_id(job.template_id, job.template_version) @@ -62,7 +69,7 @@ def process_job(job_id): process_row(row_number, recipient, personalisation, template, job, service) finished = datetime.utcnow() - job.job_status = 'finished' + job.job_status = JOB_STATUS_FINISHED job.processing_started = start job.processing_finished = finished dao_update_job(job) diff --git a/app/dao/permissions_dao.py b/app/dao/permissions_dao.py index adb484ff9..162d08a46 100644 --- a/app/dao/permissions_dao.py +++ b/app/dao/permissions_dao.py @@ -59,7 +59,8 @@ class PermissionDAO(DAOClass): db.session.commit() def get_permissions_by_user_id(self, user_id): - return self.Meta.model.query.filter_by(user_id=user_id).all() + return self.Meta.model.query.filter_by(user_id=user_id)\ + .join(Permission.service).filter_by(active=True).all() permission_dao = PermissionDAO() diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 887557e53..6daf54b55 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -13,19 +13,26 @@ from app.dao.provider_details_dao import ( get_provider_details_by_notification_type, dao_toggle_sms_provider ) -from app.dao.services_dao import dao_fetch_service_by_id from app.celery.research_mode_tasks import send_sms_response, send_email_response from app.dao.templates_dao import dao_get_template_by_id -from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE +from app.models import SMS_TYPE, KEY_TYPE_TEST, BRANDING_ORG, EMAIL_TYPE, NOTIFICATION_TECHNICAL_FAILURE def send_sms_to_provider(notification): - service = dao_fetch_service_by_id(notification.service_id) - provider = provider_to_use(SMS_TYPE, notification.id) - current_app.logger.info( - "Starting sending SMS {} to provider at {}".format(notification.id, datetime.utcnow()) - ) + service = notification.service + if not service.active: + notification.status = NOTIFICATION_TECHNICAL_FAILURE + dao_update_notification(notification) + current_app.logger.warn( + "Send sms for notification id {} to provider is not allowed: service {} is inactive".format(notification.id, + service.id)) + return + if notification.status == 'created': + provider = provider_to_use(SMS_TYPE, notification.id) + current_app.logger.info( + "Starting sending SMS {} to provider at {}".format(notification.id, datetime.utcnow()) + ) template_model = dao_get_template_by_id(notification.template_id, notification.template_version) template = SMSMessageTemplate( template_model.__dict__, @@ -61,12 +68,20 @@ def send_sms_to_provider(notification): def send_email_to_provider(notification): - service = dao_fetch_service_by_id(notification.service_id) - provider = provider_to_use(EMAIL_TYPE, notification.id) - current_app.logger.info( - "Starting sending EMAIL {} to provider at {}".format(notification.id, datetime.utcnow()) - ) + service = notification.service + if not service.active: + notification.status = NOTIFICATION_TECHNICAL_FAILURE + dao_update_notification(notification) + current_app.logger.warn( + "Send email for notification id {} to provider is not allowed: service {} is inactive".format( + notification.id, + service.id)) + return if notification.status == 'created': + provider = provider_to_use(EMAIL_TYPE, notification.id) + current_app.logger.info( + "Starting sending EMAIL {} to provider at {}".format(notification.id, datetime.utcnow()) + ) template_dict = dao_get_template_by_id(notification.template_id, notification.template_version).__dict__ html_email = HTMLEmailTemplate( diff --git a/app/job/rest.py b/app/job/rest.py index 3a8a60219..082fe6096 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -110,7 +110,9 @@ def get_jobs_by_service(service_id): @job.route('', methods=['POST']) def create_job(service_id): - dao_fetch_service_by_id(service_id) + service = dao_fetch_service_by_id(service_id) + if not service.active: + raise InvalidRequest("Create job is not allowed: service is inactive ", 403) data = request.get_json() diff --git a/app/service/rest.py b/app/service/rest.py index 019b2d0d5..2def9c32c 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -353,20 +353,6 @@ def update_whitelist(service_id): return '', 204 -# Renaming this endpoint to archive -@service_blueprint.route('//deactivate', methods=['POST']) -def deactivate_service(service_id): - service = dao_fetch_service_by_id(service_id) - - if not service.active: - # assume already inactive, don't change service name - return '', 204 - - dao_archive_service(service.id) - - return '', 204 - - @service_blueprint.route('//archive', methods=['POST']) def archive_service(service_id): """ diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 9af7b3ff7..34242d180 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -923,6 +923,22 @@ def test_persist_letter_saves_letter_to_database(sample_letter_job, mocker): assert notification_db.personalisation == personalisation +def test_should_cancel_job_if_service_is_inactive(sample_service, + sample_job, + mocker): + sample_service.active = False + + mocker.patch('app.celery.tasks.s3.get_job_from_s3') + mocker.patch('app.celery.tasks.process_row') + + process_job(sample_job.id) + + job = jobs_dao.dao_get_job_by_id(sample_job.id) + assert job.job_status == 'cancelled' + s3.get_job_from_s3.assert_not_called() + tasks.process_row.assert_not_called() + + @pytest.mark.parametrize('template_type, expected_class', [ (SMS_TYPE, SMSMessageTemplate), (EMAIL_TYPE, WithSubjectTemplate), diff --git a/tests/app/conftest.py b/tests/app/conftest.py index c63085399..d58cfef70 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -121,7 +121,6 @@ def sample_service(notify_db, notify_db_session, service_name="Sample service", user=None, - active=True, restricted=False, limit=1000, email_from=None): @@ -132,7 +131,6 @@ def sample_service(notify_db, data = { 'name': service_name, 'message_limit': limit, - 'active': active, 'restricted': restricted, 'email_from': email_from, 'created_by': user diff --git a/tests/app/dao/test_permissionDAO.py b/tests/app/dao/test_permissionDAO.py new file mode 100644 index 000000000..33dbc2e5a --- /dev/null +++ b/tests/app/dao/test_permissionDAO.py @@ -0,0 +1,26 @@ +from app.dao.permissions_dao import permission_dao +from tests.app.conftest import sample_service as create_service + + +def test_get_permissions_by_user_id_returns_all_permissions(sample_service): + permissions = permission_dao.get_permissions_by_user_id(user_id=sample_service.users[0].id) + assert len(permissions) == 8 + assert sorted(["manage_users", + "manage_templates", + "manage_settings", + "send_texts", + "send_emails", + "send_letters", + "manage_api_keys", + "view_activity"]) == sorted([i.permission for i in permissions]) + + +def test_get_permissions_by_user_id_returns_only_active_service(notify_db, notify_db_session, sample_user): + active_service = create_service(notify_db, notify_db_session, service_name="Active service", user=sample_user) + inactive_service = create_service(notify_db, notify_db_session, service_name="Inactive service", + user=sample_user) + inactive_service.active = False + permissions = permission_dao.get_permissions_by_user_id(user_id=sample_user.id) + assert len(permissions) == 8 + assert active_service in [i.service for i in permissions] + assert inactive_service not in [i.service for i in permissions] diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 22ad99417..90cd4718a 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -117,6 +117,20 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist assert notification.personalisation == {"name": "Jo"} +@pytest.mark.parametrize("client_send", + ["app.aws_ses_client.send_email", + "app.mmg_client.send_sms", + "app.firetext_client.send_sms"]) +def test_should_not_send_message_when_service_is_inactive_notiifcation_is_in_tech_failure( + sample_service, sample_notification, mocker, client_send): + sample_service.active = False + send_mock = mocker.patch(client_send, return_value='reference') + + send_to_providers.send_email_to_provider(sample_notification) + send_mock.assert_not_called() + assert Notification.query.get(sample_notification.id).status == 'technical-failure' + + def test_send_sms_should_use_template_version_from_notification_not_latest( notify_db, notify_db_session, diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index de7405289..30e6262cb 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -170,6 +170,21 @@ def test_create_scheduled_job(notify_api, sample_template, mocker, fake_uuid): assert resp_json['data']['original_file_name'] == 'thisisatest.csv' +def test_create_job_returns_403_if_service_is_not_active(client, fake_uuid, sample_service, mocker): + sample_service.active = False + mock_job_dao = mocker.patch("app.dao.jobs_dao.dao_create_job") + auth_header = create_authorization_header() + response = client.post('/service/{}/job'.format(sample_service.id), + data="", + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 403 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['result'] == 'error' + assert resp_json['message'] == "Create job is not allowed: service is inactive " + mock_job_dao.assert_not_called() + + def test_should_not_create_scheduled_job_more_then_24_hours_hence(notify_api, sample_template, mocker, fake_uuid): with notify_api.test_request_context(): with notify_api.test_client() as client: diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py deleted file mode 100644 index 7ef20d8e8..000000000 --- a/tests/app/service/test_deactivate.py +++ /dev/null @@ -1,123 +0,0 @@ -import uuid -from datetime import datetime - -import pytest -from freezegun import freeze_time - -from app import db -from app.models import Service -from app.dao.services_dao import dao_archive_service -from app.dao.api_key_dao import expire_api_key -from app.dao.templates_dao import dao_update_template - -from tests import create_authorization_header, unwrap_function -from tests.app.conftest import ( - sample_template as create_template, - sample_api_key as create_api_key -) - - -def test_deactivate_only_allows_post(client): - auth_header = create_authorization_header() - response = client.get('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header]) - assert response.status_code == 405 - - -def test_deactivate_service_errors_with_bad_service_id(client): - auth_header = create_authorization_header() - response = client.post('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header]) - assert response.status_code == 404 - - -def test_deactivating_inactive_service_does_nothing(client, sample_service): - auth_header = create_authorization_header() - sample_service.active = False - response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header]) - assert response.status_code == 204 - assert sample_service.name == 'Sample service' - - -@pytest.fixture -def deactivated_service(client, notify_db, notify_db_session, sample_service): - create_template(notify_db, notify_db_session, template_name='a') - create_template(notify_db, notify_db_session, template_name='b') - create_api_key(notify_db, notify_db_session) - create_api_key(notify_db, notify_db_session) - - auth_header = create_authorization_header() - response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header]) - assert response.status_code == 204 - assert response.data == b'' - return sample_service - - -def test_deactivating_service_changes_name_and_email(deactivated_service): - assert deactivated_service.name == '_archived_Sample service' - assert deactivated_service.email_from == '_archived_sample.service' - - -def test_deactivating_service_revokes_api_keys(deactivated_service): - assert len(deactivated_service.api_keys) == 2 - for key in deactivated_service.api_keys: - assert key.expiry_date is not None - assert key.version == 2 - - -def test_deactivating_service_archives_templates(deactivated_service): - assert len(deactivated_service.templates) == 2 - for template in deactivated_service.templates: - assert template.archived is True - assert template.version == 2 - - -def test_deactivating_service_creates_history(deactivated_service): - ServiceHistory = Service.get_history_model() - history = ServiceHistory.query.filter_by( - id=deactivated_service.id - ).order_by( - ServiceHistory.version.desc() - ).first() - - assert history.version == 2 - assert history.active is False - - -@pytest.fixture -def deactivated_service_with_deleted_stuff(client, notify_db, notify_db_session, sample_service): - with freeze_time('2001-01-01'): - template = create_template(notify_db, notify_db_session, template_name='a') - api_key = create_api_key(notify_db, notify_db_session) - - expire_api_key(sample_service.id, api_key.id) - - template.archived = True - dao_update_template(template) - - with freeze_time('2002-02-02'): - auth_header = create_authorization_header() - response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header]) - - assert response.status_code == 204 - assert response.data == b'' - return sample_service - - -def test_deactivating_service_doesnt_affect_existing_archived_templates(deactivated_service_with_deleted_stuff): - assert deactivated_service_with_deleted_stuff.templates[0].archived is True - assert deactivated_service_with_deleted_stuff.templates[0].updated_at == datetime(2001, 1, 1, 0, 0, 0) - assert deactivated_service_with_deleted_stuff.templates[0].version == 2 - - -def test_deactivating_service_doesnt_affect_existing_revoked_api_keys(deactivated_service_with_deleted_stuff): - assert deactivated_service_with_deleted_stuff.api_keys[0].expiry_date == datetime(2001, 1, 1, 0, 0, 0) - assert deactivated_service_with_deleted_stuff.api_keys[0].version == 2 - - -def test_deactivating_service_rolls_back_everything_on_error(sample_service, sample_api_key, sample_template): - unwrapped_deactive_service = unwrap_function(dao_archive_service) - - unwrapped_deactive_service(sample_service.id) - - assert sample_service in db.session.dirty - assert sample_api_key in db.session.dirty - assert sample_template in db.session.dirty