Merge pull request #1248 from GSA/notify-apy-283

Need to remove priority logic

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,

app/dao/provider_details_dao.py

@@ -1,7 +1,7 @@
 from datetime import datetime
 
 from flask import current_app
-from sqlalchemy import asc, desc, func
+from sqlalchemy import desc, func
 
 from app import db
 from app.dao.dao_utils import autocommit
@@ -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()
     )

app/models.py

@@ -1297,7 +1297,6 @@ class ProviderDetails(db.Model):
     id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4)
     display_name = db.Column(db.String, nullable=False)
     identifier = db.Column(db.String, nullable=False)
-    priority = db.Column(db.Integer, nullable=False)
     notification_type = enum_column(NotificationType, nullable=False)
     active = db.Column(db.Boolean, default=False, nullable=False)
     version = db.Column(db.Integer, default=1, nullable=False)
@@ -1322,7 +1321,6 @@ class ProviderDetailsHistory(db.Model, HistoryModel):
     id = db.Column(UUID(as_uuid=True), primary_key=True, nullable=False)
     display_name = db.Column(db.String, nullable=False)
     identifier = db.Column(db.String, nullable=False)
-    priority = db.Column(db.Integer, nullable=False)
     notification_type = enum_column(NotificationType, nullable=False)
     active = db.Column(db.Boolean, nullable=False)
     version = db.Column(db.Integer, primary_key=True, nullable=False)

app/notifications/rest.py

@@ -2,9 +2,8 @@ from flask import Blueprint, current_app, jsonify, request
 
 from app import api_user, authenticated_service
 from app.aws.s3 import get_personalisation_from_s3, get_phone_number_from_s3
-from app.config import QueueNames
 from app.dao import notifications_dao
-from app.enums import KeyType, NotificationType, TemplateProcessType
+from app.enums import KeyType, NotificationType
 from app.errors import InvalidRequest, register_errors
 from app.notifications.process_notifications import (
     persist_notification,
@@ -168,11 +167,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:

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,

app/service/send_notification.py

@@ -1,12 +1,11 @@
 from sqlalchemy.orm.exc import NoResultFound
 
-from app.config import QueueNames
 from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
 from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id
 from app.dao.services_dao import dao_fetch_service_by_id
 from app.dao.templates_dao import dao_get_template_by_id_and_service_id
 from app.dao.users_dao import get_user_by_id
-from app.enums import KeyType, NotificationType, TemplateProcessType
+from app.enums import KeyType, NotificationType
 from app.errors import BadRequestError
 from app.notifications.process_notifications import (
     persist_notification,
@@ -80,11 +79,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,

app/v2/notifications/post_notifications.py

@@ -8,7 +8,7 @@ from app import api_user, authenticated_service, document_download_client, encry
 from app.celery.tasks import save_api_email, save_api_sms
 from app.clients.document_download import DocumentDownloadError
 from app.config import QueueNames
-from app.enums import KeyType, NotificationStatus, NotificationType, TemplateProcessType
+from app.enums import KeyType, NotificationStatus, NotificationType
 from app.models import Notification
 from app.notifications.process_notifications import (
     persist_notification,
@@ -85,7 +85,6 @@ def process_sms_or_email_notification(
     notification_type,
     template,
     template_with_content,
-    template_process_type,
     service,
     reply_to_text=None,
 ):
@@ -176,11 +175,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,

migrations/versions/0412_remove_priority.py

@@ -0,0 +1,24 @@
+"""
+
+Revision ID: 0412_remove_priority
+Revises: 411_add_login_uuid
+
+"""
+
+import sqlalchemy as sa
+from alembic import op
+
+revision = "0412_remove_priority"
+down_revision = "0411_add_login_uuid"
+
+
+def upgrade():
+    print("DELETING COLUMNS")
+    op.drop_column("provider_details", "priority")
+    op.drop_column("provider_details_history", "priority")
+
+
+def downgrade():
+    print("ADDING COLUMNS")
+    op.add_column("provider_details", sa.Column("priority", sa.Integer))
+    op.add_column("provider_details_history", sa.Column("priority", sa.Integer))

tests/app/dao/test_provider_details_dao.py

@@ -2,11 +2,9 @@ from datetime import datetime, timedelta
 
 import pytest
 from freezegun import freeze_time
-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,
@@ -16,7 +14,6 @@ from app.dao.provider_details_dao import (
 )
 from app.enums import NotificationType, TemplateType
 from app.models import ProviderDetails, ProviderDetailsHistory
-from app.utils import utc_now
 from tests.app.db import create_ft_billing, create_service, create_template
 from tests.conftest import set_config
 
@@ -33,9 +30,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 +49,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 +128,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(

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()
 
@@ -124,7 +105,6 @@ def test_get_provider_versions_contains_correct_fields(client, notify_db_session
         "created_by",
         "display_name",
         "identifier",
-        "priority",
         "notification_type",
         "active",
         "version",

tests/app/service/send_notification/test_send_notification.py

@@ -11,7 +11,7 @@ from app.dao import notifications_dao
 from app.dao.api_key_dao import save_model_api_key
 from app.dao.services_dao import dao_update_service
 from app.dao.templates_dao import dao_get_all_templates_for_service, dao_update_template
-from app.enums import KeyType, NotificationType, TemplateProcessType, TemplateType
+from app.enums import KeyType, NotificationType, TemplateType
 from app.errors import InvalidRequest, RateLimitError
 from app.models import ApiKey, Notification, NotificationHistory, Template
 from app.service.send_notification import send_one_off_notification
@@ -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",
     [

tests/app/service/send_notification/test_send_one_off_notification.py

@@ -3,14 +3,12 @@ from unittest.mock import Mock
 
 import pytest
 
-from app.config import QueueNames
 from app.dao.service_guest_list_dao import dao_add_and_commit_guest_list_contacts
 from app.enums import (
     KeyType,
     NotificationType,
     RecipientType,
     ServicePermissionType,
-    TemplateProcessType,
     TemplateType,
 )
 from app.errors import BadRequestError
@@ -161,24 +159,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)

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,

tests/conftest.py

@@ -8,7 +8,6 @@ from flask import Flask
 from sqlalchemy_utils import create_database, database_exists, drop_database
 
 from app import create_app
-from app.dao.provider_details_dao import get_provider_details_by_identifier
 
 
 @pytest.fixture(scope="session")
@@ -80,12 +79,8 @@ def _notify_db(notify_api):
 
 @pytest.fixture(scope="function")
 def sms_providers(_notify_db):
-    """
-    In production we randomly choose which provider to use based on their priority. To guarantee tests run the same each
-    time, make sure we always choose sns. You'll need to override them in your tests if you wish to do something
-    different.
-    """
-    get_provider_details_by_identifier("sns").priority = 100
+    pass
+    # get_provider_details_by_identifier("sns").priority = 100
 
 
 @pytest.fixture(scope="function")