Try inlining indexes with table args

- Update the Notification and NotificationHistory model to reflect the database.
- Updates to datatypes, removal of indexes and addition of indexes.

Why?
After running the `flask db migrate` command there are many deltas because we did some work to update the notification and notification_history tables, however, the SQLAlchemy models were not updated to reflect those changes. This PR cleans up all those deltas.
However, there are still some differences that can be done but we can look at that in another PR.

app/dao/fact_billing_dao.py

@@ -114,13 +114,12 @@ def fetch_sms_billing_for_all_services(start_date, end_date):
     return query.all()
 
 
-def fetch_letter_costs_and_totals_for_all_services(start_date, end_date):
+def fetch_letter_costs_for_all_services(start_date, end_date):
     query = db.session.query(
         Organisation.name.label("organisation_name"),
         Organisation.id.label("organisation_id"),
         Service.name.label("service_name"),
         Service.id.label("service_id"),
-        func.sum(FactBilling.notifications_sent).label("total_letters"),
         func.sum(FactBilling.notifications_sent * FactBilling.rate).label("letter_cost")
     ).select_from(
         Service

app/models.py

@@ -1412,16 +1412,16 @@ class Notification(db.Model):
     job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False)
     job = db.relationship('Job', backref=db.backref('notifications', lazy='dynamic'))
     job_row_number = db.Column(db.Integer, nullable=True)
-    service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False)
+    service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=False)
     service = db.relationship('Service')
     template_id = db.Column(UUID(as_uuid=True), index=True, unique=False)
     template_version = db.Column(db.Integer, nullable=False)
     template = db.relationship('TemplateHistory')
-    api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), index=True, unique=False)
+    api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), unique=False)
     api_key = db.relationship('ApiKey')
-    key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False)
+    key_type = db.Column(db.String, db.ForeignKey('key_types.name'), unique=False, nullable=False)
     billable_units = db.Column(db.Integer, nullable=False, default=0)
-    notification_type = db.Column(notification_types, index=True, nullable=False)
+    notification_type = db.Column(notification_types, nullable=False)
     created_at = db.Column(
         db.DateTime,
         index=True,
@@ -1441,9 +1441,8 @@ class Notification(db.Model):
         onupdate=datetime.datetime.utcnow)
     status = db.Column(
         'notification_status',
-        db.String,
+        db.Text,
         db.ForeignKey('notification_status_types.name'),
-        index=True,
         nullable=True,
         default='created',
         key='status'  # http://docs.sqlalchemy.org/en/latest/core/metadata.html#sqlalchemy.schema.Column
@@ -1456,7 +1455,7 @@ class Notification(db.Model):
 
     international = db.Column(db.Boolean, nullable=False, default=False)
     phone_prefix = db.Column(db.String, nullable=True)
-    rate_multiplier = db.Column(db.Float(asdecimal=False), nullable=True)
+    rate_multiplier = db.Column(db.Numeric(asdecimal=False), nullable=True)
 
     created_by = db.relationship('User')
     created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), nullable=True)
@@ -1472,7 +1471,8 @@ class Notification(db.Model):
             ['template_id', 'template_version'],
             ['templates_history.id', 'templates_history.version'],
         ),
-        {}
+        UniqueConstraint('job_id', 'job_row_number', name='uq_notifications_job_row_number'),
+        Index('ix_notifications_service_created_at', 'service_id', 'created_at')
     )
 
     @property
@@ -1688,24 +1688,23 @@ class NotificationHistory(db.Model, HistoryModel):
     job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False)
     job = db.relationship('Job')
     job_row_number = db.Column(db.Integer, nullable=True)
-    service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False)
+    service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), unique=False)
     service = db.relationship('Service')
-    template_id = db.Column(UUID(as_uuid=True), index=True, unique=False)
+    template_id = db.Column(UUID(as_uuid=True), unique=False)
     template_version = db.Column(db.Integer, nullable=False)
-    api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), index=True, unique=False)
+    api_key_id = db.Column(UUID(as_uuid=True), db.ForeignKey('api_keys.id'), unique=False)
     api_key = db.relationship('ApiKey')
-    key_type = db.Column(db.String, db.ForeignKey('key_types.name'), index=True, unique=False, nullable=False)
+    key_type = db.Column(db.String, db.ForeignKey('key_types.name'), unique=False, nullable=False)
     billable_units = db.Column(db.Integer, nullable=False, default=0)
-    notification_type = db.Column(notification_types, index=True, nullable=False)
-    created_at = db.Column(db.DateTime, index=True, unique=False, nullable=False)
+    notification_type = db.Column(notification_types, nullable=False)
+    created_at = db.Column(db.DateTime, unique=False, nullable=False)
     sent_at = db.Column(db.DateTime, index=False, unique=False, nullable=True)
     sent_by = db.Column(db.String, nullable=True)
     updated_at = db.Column(db.DateTime, index=False, unique=False, nullable=True, onupdate=datetime.datetime.utcnow)
     status = db.Column(
         'notification_status',
-        db.String,
+        db.Text,
         db.ForeignKey('notification_status_types.name'),
-        index=True,
         nullable=True,
         default='created',
         key='status'  # http://docs.sqlalchemy.org/en/latest/core/metadata.html#sqlalchemy.schema.Column
@@ -1713,9 +1712,9 @@ class NotificationHistory(db.Model, HistoryModel):
     reference = db.Column(db.String, nullable=True, index=True)
     client_reference = db.Column(db.String, nullable=True)
 
-    international = db.Column(db.Boolean, nullable=False, default=False)
+    international = db.Column(db.Boolean, nullable=True, default=False)
     phone_prefix = db.Column(db.String, nullable=True)
-    rate_multiplier = db.Column(db.Float(asdecimal=False), nullable=True)
+    rate_multiplier = db.Column(db.Numeric(asdecimal=False), nullable=True)
 
     created_by_id = db.Column(UUID(as_uuid=True), nullable=True)
 
@@ -1728,7 +1727,10 @@ class NotificationHistory(db.Model, HistoryModel):
             ['template_id', 'template_version'],
             ['templates_history.id', 'templates_history.version'],
         ),
-        {}
+        Index(
+            'ix_notification_history_service_id_composite',
+            'service_id', 'key_type', 'notification_type', 'created_at',
+        )
     )
 
     @classmethod
@@ -1742,6 +1744,22 @@ class NotificationHistory(db.Model, HistoryModel):
         self.status = original.status
 
 
+# Indexes for notification_history and notifications to improve performance of fetch queries.
+Index(
+    'ix_notifications_notification_type_composite',
+    Notification.notification_type,
+    Notification.status,
+    Notification.created_at
+)
+Index(
+    "ix_notifications_service_id_composite",
+    Notification.service_id,
+    Notification.notification_type,
+    Notification.status,
+    Notification.created_at
+)
+
+
 class ScheduledNotification(db.Model):
     __tablename__ = 'scheduled_notifications'
 

app/platform_stats/rest.py

@@ -5,7 +5,7 @@ from flask import Blueprint, jsonify, request
 from app.dao.date_util import get_financial_year_for_datetime
 from app.dao.fact_billing_dao import (
     fetch_billing_details_for_all_services,
-    fetch_letter_costs_and_totals_for_all_services,
+    fetch_letter_costs_for_all_services,
     fetch_letter_line_items_for_all_services,
     fetch_sms_billing_for_all_services,
 )
@@ -67,7 +67,7 @@ def get_data_for_billing_report():
     start_date, end_date = validate_date_range_is_within_a_financial_year(start_date, end_date)
 
     sms_costs = fetch_sms_billing_for_all_services(start_date, end_date)
-    letter_overview = fetch_letter_costs_and_totals_for_all_services(start_date, end_date)
+    letter_costs = fetch_letter_costs_for_all_services(start_date, end_date)
     letter_breakdown = fetch_letter_line_items_for_all_services(start_date, end_date)
 
     lb_by_service = [
@@ -85,31 +85,26 @@ def get_data_for_billing_report():
                 "service_name": s.service_name,
                 "sms_cost": float(s.sms_cost),
                 "sms_fragments": s.chargeable_billable_sms,
-                "total_letters": 0,
                 "letter_cost": 0,
                 "letter_breakdown": ""
             }
             combined[s.service_id] = entry
 
-    for data in letter_overview:
-        if data.service_id in combined:
-            combined[data.service_id].update(
-                {'total_letters': data.total_letters, 'letter_cost': float(data.letter_cost)}
-            )
-
+    for letter_cost in letter_costs:
+        if letter_cost.service_id in combined:
+            combined[letter_cost.service_id].update({'letter_cost': float(letter_cost.letter_cost)})
         else:
             letter_entry = {
-                "organisation_id": str(data.organisation_id) if data.organisation_id else "",
-                "organisation_name": data.organisation_name or "",
-                "service_id": str(data.service_id),
-                "service_name": data.service_name,
+                "organisation_id": str(letter_cost.organisation_id) if letter_cost.organisation_id else "",
+                "organisation_name": letter_cost.organisation_name or "",
+                "service_id": str(letter_cost.service_id),
+                "service_name": letter_cost.service_name,
                 "sms_cost": 0,
                 "sms_fragments": 0,
-                "total_letters": data.total_letters,
-                "letter_cost": float(data.letter_cost),
+                "letter_cost": float(letter_cost.letter_cost),
                 "letter_breakdown": ""
             }
-            combined[data.service_id] = letter_entry
+            combined[letter_cost.service_id] = letter_entry
     for service_id, breakdown in lb_by_service:
         combined[service_id]['letter_breakdown'] += (breakdown + '\n')
 

app/service/service_broadcast_settings_schema.py

@@ -4,7 +4,7 @@ service_broadcast_settings_schema = {
     "type": "object",
     "title": "Set a services broadcast settings",
     "properties": {
-        "broadcast_channel": {"enum": ["operator", "test", "severe", "government"]},
+        "broadcast_channel": {"enum": ["test", "severe", "government"]},
         "service_mode": {"enum": ["training", "live"]},
         "provider_restriction": {"enum": ["three", "o2", "vodafone", "ee", "all"]}
     },

manifest.yml.j2

@@ -103,8 +103,6 @@ applications:
       - logit-ssl-syslog-drain
       {% if CF_APP == 'notify-api' %}
       - notify-prometheus
-      {% endif %}
-      {% if CF_APP == 'notify-api' or CF_APP == 'notify-delivery-worker-broadcasts' %}
       - notify-splunk
       {% endif %}
 

migrations/versions/0358_operator_channel.py

@@ -1,22 +0,0 @@
-"""
-
-Revision ID: 0358_operator_channel
-Revises: 0357_validate_constraint
-Create Date: 2021-06-09 13:44:12.479191
-
-"""
-from alembic import op
-
-revision = '0358_operator_channel'
-down_revision = '0357_validate_constraint'
-
-
-def upgrade():
-    op.execute("INSERT INTO broadcast_channel_types VALUES ('operator')")
-
-
-def downgrade():
-    # This can't be downgraded if there are rows in service_broadcast_settings which
-    # have the channel set to operator or if broadcasts have already been sent on the
-    # operator channel - it would break foreign key constraints.
-    op.execute("DELETE FROM broadcast_channel_types WHERE name = 'operator'")

tests/app/celery/test_broadcast_message_tasks.py

@@ -221,7 +221,7 @@ def test_send_broadcast_provider_message_sends_data_correctly(
     ['o2', 'O2'],
     ['vodafone', 'Vodafone'],
 ])
-@pytest.mark.parametrize('channel', ['operator', 'test', 'severe', 'government'])
+@pytest.mark.parametrize('channel', ['test', 'severe', 'government'])
 def test_send_broadcast_provider_message_uses_channel_set_on_broadcast_service(
     notify_db, mocker, sample_broadcast_service, provider, provider_capitalised, channel
 ):

tests/app/dao/test_ft_billing_dao.py

@@ -10,7 +10,7 @@ from app.dao.fact_billing_dao import (
     delete_billing_data_for_service_for_day,
     fetch_billing_data_for_day,
     fetch_billing_totals_for_year,
-    fetch_letter_costs_and_totals_for_all_services,
+    fetch_letter_costs_for_all_services,
     fetch_letter_line_items_for_all_services,
     fetch_monthly_billing_for_year,
     fetch_sms_billing_for_all_services,
@@ -628,26 +628,26 @@ def test_fetch_sms_billing_for_all_services_without_an_organisation_appears(noti
     )
 
 
-def test_fetch_letter_costs_and_totals_for_all_services(notify_db_session):
+def test_fetch_letter_costs_for_all_services(notify_db_session):
     fixtures = set_up_usage_data(datetime(2019, 6, 1))
 
-    results = fetch_letter_costs_and_totals_for_all_services(datetime(2019, 6, 1), datetime(2019, 9, 30))
+    results = fetch_letter_costs_for_all_services(datetime(2019, 6, 1), datetime(2019, 9, 30))
 
     assert len(results) == 3
     assert results[0] == (
         fixtures["org_1"].name, fixtures["org_1"].id,
         fixtures["service_1_sms_and_letter"].name, fixtures["service_1_sms_and_letter"].id,
-        8, Decimal('3.40')
+        Decimal('3.40')
     )
     assert results[1] == (
         fixtures["org_for_service_with_letters"].name, fixtures["org_for_service_with_letters"].id,
         fixtures["service_with_letters"].name, fixtures["service_with_letters"].id,
-        22, Decimal('14.00')
+        Decimal('14.00')
     )
     assert results[2] == (
         None, None,
         fixtures["service_with_letters_without_org"].name, fixtures["service_with_letters_without_org"].id,
-        18, Decimal('24.45')
+        Decimal('24.45')
     )
 
 

tests/app/platform_stats/test_rest.py

@@ -141,7 +141,6 @@ def test_get_data_for_billing_report(notify_db_session, admin_request):
     assert response[0]["service_id"] == str(fixtures["service_1_sms_and_letter"].id)
     assert response[0]["sms_cost"] == 0
     assert response[0]["sms_fragments"] == 0
-    assert response[0]["total_letters"] == 8
     assert response[0]["letter_cost"] == 3.40
     assert response[0]["letter_breakdown"] == "6 second class letters at 45p\n2 first class letters at 35p\n"
     assert response[0]["purchase_order_number"] == "service purchase order number"
@@ -153,7 +152,6 @@ def test_get_data_for_billing_report(notify_db_session, admin_request):
     assert response[1]["service_id"] == str(fixtures["service_with_letters"].id)
     assert response[1]["sms_cost"] == 0
     assert response[1]["sms_fragments"] == 0
-    assert response[1]["total_letters"] == 22
     assert response[1]["letter_cost"] == 14
     assert response[1]["letter_breakdown"] == "20 second class letters at 65p\n2 first class letters at 50p\n"
     assert response[1]["purchase_order_number"] == "org3 purchase order number"
@@ -165,7 +163,6 @@ def test_get_data_for_billing_report(notify_db_session, admin_request):
     assert response[2]["service_id"] == str(fixtures["service_with_sms_without_org"].id)
     assert response[2]["sms_cost"] == 0.33
     assert response[2]["sms_fragments"] == 3
-    assert response[2]["total_letters"] == 0
     assert response[2]["letter_cost"] == 0
     assert response[2]["letter_breakdown"] == ""
     assert response[2]["purchase_order_number"] == "sms purchase order number"
@@ -177,7 +174,6 @@ def test_get_data_for_billing_report(notify_db_session, admin_request):
     assert response[3]["service_id"] == str(fixtures["service_with_letters_without_org"].id)
     assert response[3]["sms_cost"] == 0
     assert response[3]["sms_fragments"] == 0
-    assert response[3]["total_letters"] == 18
     assert response[3]["letter_cost"] == 24.45
     assert response[3]["letter_breakdown"] == (
         "2 second class letters at 35p\n1 first class letters at 50p\n15 international letters at £1.55\n"

tests/app/service/test_rest.py

@@ -290,11 +290,9 @@ def test_get_service_by_id(admin_request, sample_service):
 
 
 @pytest.mark.parametrize('broadcast_channel,allowed_broadcast_provider', (
-    ('operator', 'all'),
     ('test', 'all'),
     ('severe', 'all'),
     ('government', 'all'),
-    ('operator', 'o2'),
     ('test', 'ee'),
     ('severe', 'three'),
     ('government', 'vodafone'),
@@ -3713,7 +3711,7 @@ def test_get_returned_letter(admin_request, sample_letter_template):
     assert response[4]['uploaded_letter_file_name'] == 'filename.pdf'
 
 
-@pytest.mark.parametrize('channel', ["operator", "test", "severe", "government"])
+@pytest.mark.parametrize('channel', ["test", "severe", "government"])
 def test_set_as_broadcast_service_sets_broadcast_channel(
     admin_request, sample_service, broadcast_organisation, channel
 ):