Remove FF_SPIKE_SMS_DAILY_LIMIT and FF_SMS_PARTS_UI (#1950)

* Remove FF_SPIKE_SMS_DAILY_LIMIT

* fix

* fix

* changes based on PR

app/config.py

@@ -517,10 +517,6 @@ class Config(object):
     BR_WARNING_PERCENTAGE = 0.05
     BR_CRITICAL_PERCENTAGE = 0.1
 
-    # add and use sms_daily_limit
-    FF_SPIKE_SMS_DAILY_LIMIT = env.bool("FF_SPIKE_SMS_DAILY_LIMIT", False)
-    FF_SMS_PARTS_UI = env.bool("FF_SMS_PARTS_UI", False)
-
     FF_SALESFORCE_CONTACT = env.bool("FF_SALESFORCE_CONTACT", False)
 
     # Feature flags for bounce rate

app/dao/fact_notification_status_dao.py

@@ -244,18 +244,7 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_
         FactNotificationStatus.notification_type.label("notification_type"),
         FactNotificationStatus.notification_status.label("status"),
         *([FactNotificationStatus.template_id.label("template_id")] if by_template else []),
-        *(
-            [
-                case(
-                    [
-                        (FactNotificationStatus.notification_type == "email", FactNotificationStatus.notification_count),
-                    ],
-                    else_=FactNotificationStatus.billable_units,
-                ).label("count")
-            ]
-            if current_app.config["FF_SMS_PARTS_UI"]
-            else [FactNotificationStatus.notification_count.label("count")]
-        ),
+        *([FactNotificationStatus.notification_count.label("count")]),
     ).filter(
         FactNotificationStatus.service_id == service_id,
         FactNotificationStatus.bst_date >= start_date,
@@ -267,18 +256,7 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_
             Notification.notification_type.cast(db.Text),
             Notification.status,
             *([Notification.template_id] if by_template else []),
-            *(
-                [
-                    case(
-                        [
-                            (Notification.notification_type == "email", func.count()),
-                        ],
-                        else_=func.sum(Notification.billable_units),
-                    ).label("count")
-                ]
-                if current_app.config["FF_SMS_PARTS_UI"]
-                else [func.count().label("count")]
-            ),
+            *([func.count().label("count")]),
         )
         .filter(
             Notification.created_at >= midnight_n_days_ago(limit_days),

app/dao/services_dao.py

@@ -471,18 +471,7 @@ def _stats_for_service_query(service_id):
         db.session.query(
             Notification.notification_type,
             Notification.status,
-            *(
-                [
-                    case(
-                        [
-                            (Notification.notification_type == "email", func.count(Notification.id)),
-                        ],
-                        else_=func.sum(Notification.billable_units),
-                    ).label("count")
-                ]
-                if current_app.config["FF_SMS_PARTS_UI"]
-                else [func.count(Notification.id).label("count")]
-            ),
+            *([func.count(Notification.id).label("count")]),
         )
         .filter(
             Notification.service_id == service_id,

app/notifications/validators.py

@@ -214,8 +214,6 @@ def time_until_end_of_day() -> timedelta:
 
 
 def check_sms_limit_increment_redis_send_warnings_if_needed(service: Service, requested_sms=0) -> None:
-    if not current_app.config["FF_SPIKE_SMS_DAILY_LIMIT"]:
-        return
     if not current_app.config["REDIS_ENABLED"]:
         return
 
@@ -295,9 +293,7 @@ def warn_about_daily_message_limit(service: Service, messages_sent):
 def send_near_sms_limit_email(service: Service):
     send_notification_to_service_users(
         service_id=service.id,
-        template_id=current_app.config["NEAR_DAILY_SMS_LIMIT_TEMPLATE_ID"]
-        if current_app.config["FF_SPIKE_SMS_DAILY_LIMIT"]
-        else current_app.config["NEAR_DAILY_LIMIT_TEMPLATE_ID"],
+        template_id=current_app.config["NEAR_DAILY_SMS_LIMIT_TEMPLATE_ID"],
         personalisation={
             "service_name": service.name,
             "contact_url": f"{current_app.config['ADMIN_BASE_URL']}/contact",
@@ -331,9 +327,7 @@ def send_near_email_limit_email(service: Service) -> None:
 def send_sms_limit_reached_email(service: Service):
     send_notification_to_service_users(
         service_id=service.id,
-        template_id=current_app.config["REACHED_DAILY_SMS_LIMIT_TEMPLATE_ID"]
-        if current_app.config["FF_SPIKE_SMS_DAILY_LIMIT"]
-        else current_app.config["REACHED_DAILY_LIMIT_TEMPLATE_ID"],
+        template_id=current_app.config["REACHED_DAILY_SMS_LIMIT_TEMPLATE_ID"],
         personalisation={
             "service_name": service.name,
             "contact_url": f"{current_app.config['ADMIN_BASE_URL']}/contact",

app/service/rest.py

@@ -299,7 +299,7 @@ def update_service(service_id):
     if sms_limit_changed:
         redis_store.delete(near_sms_daily_limit_cache_key(service_id))
         redis_store.delete(over_sms_daily_limit_cache_key(service_id))
-        if not fetched_service.restricted and current_app.config["FF_SPIKE_SMS_DAILY_LIMIT"]:
+        if not fetched_service.restricted:
             _warn_service_users_about_sms_limit_changed(service_id, current_data)
 
     if service_going_live:

app/v2/notifications/post_notifications.py

@@ -160,7 +160,6 @@ def post_bulk():
         raise BadRequestError(message=f"Error decoding arguments: {e.description}", status_code=400)
 
     max_rows = current_app.config["CSV_MAX_ROWS"]
-    check_sms_limit = current_app.config["FF_SPIKE_SMS_DAILY_LIMIT"]
     epoch_seeding_bounce = current_app.config["FF_BOUNCE_RATE_SEED_EPOCH_MS"]
     bounce_rate_v1 = current_app.config["FF_BOUNCE_RATE_BACKEND"]
     if bounce_rate_v1 and epoch_seeding_bounce:
@@ -173,7 +172,7 @@ def post_bulk():
     template = validate_template_exists(form["template_id"], authenticated_service)
     check_service_has_permission(template.template_type, authenticated_service.permissions)
 
-    if template.template_type == SMS_TYPE and check_sms_limit:
+    if template.template_type == SMS_TYPE:
         fragments_sent = fetch_todays_requested_sms_count(authenticated_service.id)
         remaining_messages = authenticated_service.sms_daily_limit - fragments_sent
     else:
@@ -690,13 +689,8 @@ def row_error(row):
                 message=f"Some rows have errors. {errors}.",
                 status_code=400,
             )
-        # TODO:
-        # - right now there are no other errors in RecipientCSV so this else is not needed
-        # - if FF_SPIKE_SMS_DAILY_LIMIT is false we do not want to throw this error if only more_sms_rows_than_can_send is set
-        # - after the FF is turned on / removed, we will restore this else
-        #
-        # else:
-        #     raise NotImplementedError("Got errors but code did not handle")
+        else:
+            raise NotImplementedError("Got errors but code did not handle")
 
 
 def create_bulk_job(service, api_key, template, form, recipient_csv):

tests/app/dao/test_fact_notification_status_dao.py

@@ -51,7 +51,6 @@
     create_template,
     save_notification,
 )
-from tests.conftest import set_config
 
 
 def test_update_fact_notification_status(notify_db_session):
@@ -268,7 +267,6 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days(
             status="delivered",
         )
     )
-
     results = sorted(
         fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id),
         key=lambda x: (x.notification_type, x.status),
@@ -324,34 +322,18 @@ def test_fetch_notification_status_by_template_for_service_for_today_and_7_previ
             status="delivered",
         )
     )
-
-    with set_config(notify_api, "FF_SMS_PARTS_UI", False):
-        results = fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id, by_template=True)
-        assert [
-            ("email Template Name", False, mock.ANY, "email", "delivered", 1),
-            ("email Template Name", False, mock.ANY, "email", "delivered", 3),
-            ("letter Template Name", False, mock.ANY, "letter", "delivered", 5),
-            ("sms Template 1", False, mock.ANY, "sms", "created", 1),
-            ("sms Template Name", False, mock.ANY, "sms", "created", 1),
-            ("sms Template 1", False, mock.ANY, "sms", "delivered", 1),
-            ("sms Template 2", False, mock.ANY, "sms", "delivered", 1),
-            ("sms Template Name", False, mock.ANY, "sms", "delivered", 10),
-            ("sms Template Name", False, mock.ANY, "sms", "delivered", 11),
-        ] == sorted(results, key=lambda x: (x.notification_type, x.status, x.template_name, x.count))
-
-    with set_config(notify_api, "FF_SMS_PARTS_UI", True):
-        results = fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id, by_template=True)
-        assert [
-            ("email Template Name", False, mock.ANY, "email", "delivered", 1),
-            ("email Template Name", False, mock.ANY, "email", "delivered", 3),
-            ("letter Template Name", False, mock.ANY, "letter", "delivered", 5),
-            ("sms Template 1", False, mock.ANY, "sms", "created", 1),
-            ("sms Template Name", False, mock.ANY, "sms", "created", 1),
-            ("sms Template 1", False, mock.ANY, "sms", "delivered", 1),
-            ("sms Template 2", False, mock.ANY, "sms", "delivered", 1),
-            ("sms Template Name", False, mock.ANY, "sms", "delivered", 11),
-            ("sms Template Name", False, mock.ANY, "sms", "delivered", 20),
-        ] == sorted(results, key=lambda x: (x.notification_type, x.status, x.template_name, x.count))
+    results = fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id, by_template=True)
+    assert [
+        ("email Template Name", False, mock.ANY, "email", "delivered", 1),
+        ("email Template Name", False, mock.ANY, "email", "delivered", 3),
+        ("letter Template Name", False, mock.ANY, "letter", "delivered", 5),
+        ("sms Template 1", False, mock.ANY, "sms", "created", 1),
+        ("sms Template Name", False, mock.ANY, "sms", "created", 1),
+        ("sms Template 1", False, mock.ANY, "sms", "delivered", 1),
+        ("sms Template 2", False, mock.ANY, "sms", "delivered", 1),
+        ("sms Template Name", False, mock.ANY, "sms", "delivered", 10),
+        ("sms Template Name", False, mock.ANY, "sms", "delivered", 11),
+    ] == sorted(results, key=lambda x: (x.notification_type, x.status, x.template_name, x.count))
 
 
 def test_get_total_notifications_sent_for_api_key(notify_db_session):

tests/app/dao/test_services_dao.py

@@ -87,7 +87,6 @@
     create_user,
     save_notification,
 )
-from tests.conftest import set_config
 
 # from unittest import mock
 
@@ -966,39 +965,21 @@ def test_fetch_stats_counts_correctly(notify_db_session, notify_api):
     save_notification(create_notification(template=email_template, status="technical-failure"))
     save_notification(create_notification(template=sms_template, status="created", billable_units=10))
 
-    with set_config(notify_api, "FF_SMS_PARTS_UI", False):
-        stats = dao_fetch_stats_for_service(sms_template.service_id, 7)
-        stats = sorted(stats, key=lambda x: (x.notification_type, x.status))
-        assert len(stats) == 3
+    stats = dao_fetch_stats_for_service(sms_template.service_id, 7)
+    stats = sorted(stats, key=lambda x: (x.notification_type, x.status))
+    assert len(stats) == 3
 
-        assert stats[0].notification_type == "email"
-        assert stats[0].status == "created"
-        assert stats[0].count == 2
-
-        assert stats[1].notification_type == "email"
-        assert stats[1].status == "technical-failure"
-        assert stats[1].count == 1
-
-        assert stats[2].notification_type == "sms"
-        assert stats[2].status == "created"
-        assert stats[2].count == 1
-
-    with set_config(notify_api, "FF_SMS_PARTS_UI", True):
-        stats = dao_fetch_stats_for_service(sms_template.service_id, 7)
-        stats = sorted(stats, key=lambda x: (x.notification_type, x.status))
-        assert len(stats) == 3
-
-        assert stats[0].notification_type == "email"
-        assert stats[0].status == "created"
-        assert stats[0].count == 2
+    assert stats[0].notification_type == "email"
+    assert stats[0].status == "created"
+    assert stats[0].count == 2
 
-        assert stats[1].notification_type == "email"
-        assert stats[1].status == "technical-failure"
-        assert stats[1].count == 1
+    assert stats[1].notification_type == "email"
+    assert stats[1].status == "technical-failure"
+    assert stats[1].count == 1
 
-        assert stats[2].notification_type == "sms"
-        assert stats[2].status == "created"
-        assert stats[2].count == 10
+    assert stats[2].notification_type == "sms"
+    assert stats[2].status == "created"
+    assert stats[2].count == 1
 
 
 def test_fetch_stats_counts_should_ignore_team_key(notify_db_session):

tests/app/notifications/test_validators.py

@@ -101,8 +101,7 @@ def test_check_service_message_limit_in_cache_with_unrestricted_service_is_allow
         mocker.patch("app.notifications.validators.redis_store.set")
         mocker.patch("app.notifications.validators.services_dao")
         if limit_type == "sms":
-            with set_config(notify_api, "FF_SPIKE_SMS_DAILY_LIMIT", True):
-                check_sms_daily_limit(sample_service)
+            check_sms_daily_limit(sample_service)
         else:
             check_email_daily_limit(sample_service)
         app.notifications.validators.redis_store.set.assert_not_called()
@@ -119,8 +118,7 @@ def test_check_service_message_limit_in_cache_under_message_limit_passes(
         mocker.patch("app.notifications.validators.redis_store.set")
         mocker.patch("app.notifications.validators.services_dao")
         if limit_type == "sms":
-            with set_config(notify_api, "FF_SPIKE_SMS_DAILY_LIMIT", True):
-                check_sms_daily_limit(sample_service)
+            check_sms_daily_limit(sample_service)
         else:
             check_email_daily_limit(sample_service)
             app.notifications.validators.redis_store.set.assert_not_called()
@@ -152,8 +150,7 @@ def test_should_not_access_database_if_redis_disabled(self, notify_api, sample_s
         with set_config(notify_api, "REDIS_ENABLED", False):
             db_mock = mocker.patch("app.notifications.validators.services_dao")
             check_service_over_daily_message_limit("normal", sample_service)
-            with set_config(notify_api, "FF_SPIKE_SMS_DAILY_LIMIT", True):
-                check_sms_daily_limit(sample_service)
+            check_sms_daily_limit(sample_service)
 
             assert db_mock.method_calls == []
 
@@ -218,8 +215,7 @@ def test_check_service_message_limit_records_nearing_daily_limit(
                 create_sample_notification(notify_db, notify_db_session, service=service, template=template)
 
             if limit_type == "sms":
-                with set_config(notify_api, "FF_SPIKE_SMS_DAILY_LIMIT", True):
-                    check_sms_limit_increment_redis_send_warnings_if_needed(service)
+                check_sms_limit_increment_redis_send_warnings_if_needed(service)
             else:
                 with set_config(notify_api, "FF_EMAIL_DAILY_LIMIT", True):
                     check_email_limit_increment_redis_send_warnings_if_needed(service)
@@ -283,8 +279,7 @@ def test_check_service_message_limit_in_cache_over_message_limit_fails(
             assert e.value.fields == []
 
             with pytest.raises(TooManySMSRequestsError) as e:
-                with set_config(notify_api, "FF_SPIKE_SMS_DAILY_LIMIT", True):
-                    check_sms_daily_limit(service)
+                check_sms_daily_limit(service)
             assert e.value.status_code == 429
             assert e.value.message == "Exceeded SMS daily sending limit of 4 fragments"
             assert e.value.fields == []
@@ -337,8 +332,7 @@ def test_check_service_message_limit_skip_statsd_over_message_no_limit_fails_sms
         # When
         service = create_sample_service(notify_db, notify_db_session, restricted=True, limit=4, sms_limit=4)
         check_service_over_daily_message_limit("normal", service)
-        with set_config(notify_api, "FF_SPIKE_SMS_DAILY_LIMIT", True):
-            check_sms_daily_limit(service)
+        check_sms_daily_limit(service)
         # Then
         app_statsd.statsd_client.incr.assert_not_called()