From 2e7e6e81fcbae85f58b6c98ff1a100a0bf60fc47 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Fri, 9 Aug 2024 09:11:28 -0700 Subject: [PATCH] Need to remove priority logic --- app/config.py | 2 - app/dao/provider_details_dao.py | 22 +----- app/notifications/rest.py | 6 +- app/provider_details/rest.py | 1 - app/service/send_notification.py | 6 +- app/v2/notifications/post_notifications.py | 6 +- tests/app/dao/test_provider_details_dao.py | 71 ------------------- tests/app/provider_details/test_rest.py | 19 ----- .../test_send_notification.py | 43 ----------- .../test_send_one_off_notification.py | 18 ----- tests/app/test_config.py | 3 +- 11 files changed, 5 insertions(+), 192 deletions(-) diff --git a/app/config.py b/app/config.py index 65ef6b2d3..75cf0e783 100644 --- a/app/config.py +++ b/app/config.py @@ -11,7 +11,6 @@ from app.cloudfoundry_config import cloud_config class QueueNames(object): PERIODIC = "periodic-tasks" - PRIORITY = "priority-tasks" DATABASE = "database-tasks" SEND_SMS = "send-sms-tasks" CHECK_SMS = "check-sms_tasks" @@ -30,7 +29,6 @@ class QueueNames(object): @staticmethod def all_queues(): return [ - QueueNames.PRIORITY, QueueNames.PERIODIC, QueueNames.DATABASE, QueueNames.SEND_SMS, diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index 73132a44e..9b6b3b726 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -33,20 +33,6 @@ def dao_get_provider_versions(provider_id): ) -def _adjust_provider_priority(provider, new_priority): - current_app.logger.info( - f"Adjusting provider priority - {provider.identifier} going from {provider.priority} to {new_priority}" - ) - provider.priority = new_priority - - # Automatic update so set as notify user - provider.created_by_id = current_app.config["NOTIFY_USER_ID"] - - # update without commit so that both rows can be changed without ending the transaction - # and releasing the for_update lock - _update_provider_details_without_commit(provider) - - def _get_sms_providers_for_update(time_threshold): """ Returns a list of providers, while holding a for_update lock on the provider details table, guaranteeing that those @@ -86,11 +72,7 @@ def get_provider_details_by_notification_type( if supports_international: filters.append(ProviderDetails.supports_international == supports_international) - return ( - ProviderDetails.query.filter(*filters) - .order_by(asc(ProviderDetails.priority)) - .all() - ) + return ProviderDetails.query.filter(*filters).all() @autocommit @@ -135,7 +117,6 @@ def dao_get_provider_stats(): ProviderDetails.id, ProviderDetails.display_name, ProviderDetails.identifier, - ProviderDetails.priority, ProviderDetails.notification_type, ProviderDetails.active, ProviderDetails.updated_at, @@ -149,7 +130,6 @@ def dao_get_provider_stats(): .outerjoin(User, ProviderDetails.created_by_id == User.id) .order_by( ProviderDetails.notification_type, - ProviderDetails.priority, ) .all() ) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index f52bd1933..af1cd3ca4 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -168,11 +168,7 @@ def send_notification(notification_type): reply_to_text=template.reply_to_text, ) if not simulated: - queue_name = ( - QueueNames.PRIORITY - if template.process_type == TemplateProcessType.PRIORITY - else None - ) + queue_name = None send_notification_to_queue(notification=notification_model, queue=queue_name) else: diff --git a/app/provider_details/rest.py b/app/provider_details/rest.py index 9cc9f714a..3a7e62332 100644 --- a/app/provider_details/rest.py +++ b/app/provider_details/rest.py @@ -23,7 +23,6 @@ def get_providers(): "id": row.id, "display_name": row.display_name, "identifier": row.identifier, - "priority": row.priority, "notification_type": row.notification_type, "active": row.active, "updated_at": row.updated_at, diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 4459ded3c..62628b1c4 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -80,11 +80,7 @@ def send_one_off_notification(service_id, post_data): client_reference=client_reference, ) - queue_name = ( - QueueNames.PRIORITY - if template.process_type == TemplateProcessType.PRIORITY - else None - ) + queue_name = None send_notification_to_queue( notification=notification, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 856179f85..eb1457d0a 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -176,11 +176,7 @@ def process_sms_or_email_notification( ) if not simulated: - queue_name = ( - QueueNames.PRIORITY - if template_process_type == TemplateProcessType.PRIORITY - else None - ) + queue_name = None send_notification_to_queue_detached( key_type=api_user.key_type, notification_type=notification_type, diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index b03d965d0..5a6f5c218 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -6,7 +6,6 @@ from sqlalchemy.sql import desc from app import notification_provider_clients from app.dao.provider_details_dao import ( - _adjust_provider_priority, _get_sms_providers_for_update, dao_get_provider_stats, dao_update_provider_details, @@ -33,9 +32,6 @@ def set_primary_sms_provider(identifier): get_alternative_sms_provider(identifier) ) - primary_provider.priority = 10 - secondary_provider.priority = 20 - dao_update_provider_details(primary_provider) dao_update_provider_details(secondary_provider) @@ -55,18 +51,6 @@ def test_can_get_sms_international_providers(notify_db_session): assert all(prov.supports_international for prov in sms_providers) -def test_can_get_sms_providers_in_order_of_priority(notify_db_session): - providers = get_provider_details_by_notification_type(NotificationType.SMS, False) - priorities = [provider.priority for provider in providers] - assert priorities == sorted(priorities) - - -def test_can_get_email_providers_in_order_of_priority(notify_db_session): - providers = get_provider_details_by_notification_type(NotificationType.EMAIL) - - assert providers[0].identifier == "ses" - - def test_can_get_email_providers(notify_db_session): assert len(get_provider_details_by_notification_type(NotificationType.EMAIL)) == 1 types = [ @@ -146,61 +130,6 @@ def test_get_alternative_sms_provider_fails_if_unrecognised(): get_alternative_sms_provider("ses") -@freeze_time("2016-01-01 00:30") -def test_adjust_provider_priority_sets_priority( - restore_provider_details, - notify_user, - sns_provider, -): - # need to update these manually to avoid triggering the `onupdate` clause of the updated_at column - ProviderDetails.query.filter(ProviderDetails.identifier == "sns").update( - {"updated_at": datetime.min} - ) - - _adjust_provider_priority(sns_provider, 50) - - assert sns_provider.updated_at == utc_now() - assert sns_provider.created_by.id == notify_user.id - assert sns_provider.priority == 50 - - -@freeze_time("2016-01-01 00:30") -def test_adjust_provider_priority_adds_history( - restore_provider_details, - notify_user, - sns_provider, -): - # need to update these manually to avoid triggering the `onupdate` clause of the updated_at column - ProviderDetails.query.filter(ProviderDetails.identifier == "sns").update( - {"updated_at": datetime.min} - ) - - old_provider_history_rows = ( - ProviderDetailsHistory.query.filter( - ProviderDetailsHistory.id == sns_provider.id - ) - .order_by(desc(ProviderDetailsHistory.version)) - .all() - ) - - _adjust_provider_priority(sns_provider, 50) - - updated_provider_history_rows = ( - ProviderDetailsHistory.query.filter( - ProviderDetailsHistory.id == sns_provider.id - ) - .order_by(desc(ProviderDetailsHistory.version)) - .all() - ) - - assert len(updated_provider_history_rows) - len(old_provider_history_rows) == 1 - assert ( - updated_provider_history_rows[0].version - old_provider_history_rows[0].version - == 1 - ) - assert updated_provider_history_rows[0].priority == 50 - - @freeze_time("2016-01-01 01:00") def test_get_sms_providers_for_update_returns_providers(restore_provider_details): ProviderDetails.query.filter(ProviderDetails.identifier == "sns").update( diff --git a/tests/app/provider_details/test_rest.py b/tests/app/provider_details/test_rest.py index b0f67a5b6..5deb88bd8 100644 --- a/tests/app/provider_details/test_rest.py +++ b/tests/app/provider_details/test_rest.py @@ -42,7 +42,6 @@ def test_get_provider_contains_correct_fields(client, sample_template): "created_by_name", "display_name", "identifier", - "priority", "notification_type", "active", "updated_at", @@ -53,24 +52,6 @@ def test_get_provider_contains_correct_fields(client, sample_template): assert allowed_keys == set(json_resp[0].keys()) -def test_should_be_able_to_update_priority(client, restore_provider_details): - provider = ProviderDetails.query.first() - - update_resp = client.post( - "/provider-details/{}".format(provider.id), - headers=[ - ("Content-Type", "application/json"), - create_admin_authorization_header(), - ], - data=json.dumps({"priority": 5}), - ) - assert update_resp.status_code == 200 - update_json = json.loads(update_resp.get_data(as_text=True))["provider_details"] - assert update_json["identifier"] == provider.identifier - assert update_json["priority"] == 5 - assert provider.priority == 5 - - def test_should_be_able_to_update_status(client, restore_provider_details): provider = ProviderDetails.query.first() diff --git a/tests/app/service/send_notification/test_send_notification.py b/tests/app/service/send_notification/test_send_notification.py index 036c5bac8..464077bf0 100644 --- a/tests/app/service/send_notification/test_send_notification.py +++ b/tests/app/service/send_notification/test_send_notification.py @@ -1113,49 +1113,6 @@ def test_create_template_raises_invalid_request_when_content_too_large( } -@pytest.mark.parametrize( - "notification_type,send_to", - [ - (NotificationType.SMS, "2028675309"), - ( - NotificationType.EMAIL, - "sample@email.com", - ), - ], -) -def test_send_notification_uses_priority_queue_when_template_is_marked_as_priority( - client, - sample_service, - mocker, - notification_type, - send_to, -): - sample = create_template( - sample_service, - template_type=notification_type, - process_type=TemplateProcessType.PRIORITY, - ) - mocked = mocker.patch( - f"app.celery.provider_tasks.deliver_{notification_type}.apply_async" - ) - - data = {"to": send_to, "template": str(sample.id)} - - auth_header = create_service_authorization_header(service_id=sample.service_id) - - response = client.post( - path=f"/notifications/{notification_type}", - data=json.dumps(data), - headers=[("Content-Type", "application/json"), auth_header], - ) - - response_data = json.loads(response.data)["data"] - notification_id = response_data["notification"]["id"] - - assert response.status_code == 201 - mocked.assert_called_once_with([notification_id], queue="priority-tasks") - - @pytest.mark.parametrize( "notification_type, send_to", [ diff --git a/tests/app/service/send_notification/test_send_one_off_notification.py b/tests/app/service/send_notification/test_send_one_off_notification.py index 9983515c7..889a3e0c1 100644 --- a/tests/app/service/send_notification/test_send_one_off_notification.py +++ b/tests/app/service/send_notification/test_send_one_off_notification.py @@ -161,24 +161,6 @@ def test_send_one_off_notification_calls_persist_correctly_for_email( ) -def test_send_one_off_notification_honors_priority( - notify_db_session, persist_mock, celery_mock -): - service = create_service() - template = create_template(service=service) - template.process_type = TemplateProcessType.PRIORITY - - post_data = { - "template_id": str(template.id), - "to": "202-867-5309", - "created_by": str(service.created_by_id), - } - - send_one_off_notification(service.id, post_data) - - assert celery_mock.call_args[1]["queue"] == QueueNames.PRIORITY - - def test_send_one_off_notification_raises_if_invalid_recipient(notify_db_session): service = create_service() template = create_template(service=service) diff --git a/tests/app/test_config.py b/tests/app/test_config.py index 2d9591be8..46a061ddd 100644 --- a/tests/app/test_config.py +++ b/tests/app/test_config.py @@ -4,10 +4,9 @@ from app.config import QueueNames def test_queue_names_all_queues_correct(): # Need to ensure that all_queues() only returns queue names used in API queues = QueueNames.all_queues() - assert len(queues) == 15 + assert len(queues) == 14 assert set( [ - QueueNames.PRIORITY, QueueNames.PERIODIC, QueueNames.DATABASE, QueueNames.SEND_SMS,