Remove FF_EMAIL_DAILY_LIMIT feature flag (#2096)

* Remove FF_EMAIL_DAILY_LIMIT feature flag

* Various test fixes

* Fix tests, squashed a bug

---------

Co-authored-by: Jumana B <[email protected]>

app/celery/tasks.py

@@ -78,7 +78,6 @@
     persist_notifications,
     send_notification_to_queue,
 )
-from app.notifications.validators import check_service_over_daily_message_limit
 from app.types import VerifiedNotification
 from app.utils import get_csv_max_rows, get_delivery_queue_for_template
 from app.v2.errors import (
@@ -300,8 +299,6 @@ def save_smss(self, service_id: Optional[str], signed_notifications: List[Signed
     current_app.logger.debug(f"Sending following sms notifications to AWS: {notification_id_queue.keys()}")
     for notification_obj in saved_notifications:
         try:
-            if not current_app.config["FF_EMAIL_DAILY_LIMIT"]:
-                check_service_over_daily_message_limit(notification_obj.key_type, service)
             queue = notification_id_queue.get(notification_obj.id) or get_delivery_queue_for_template(template)
             send_notification_to_queue(
                 notification_obj,
@@ -423,8 +420,6 @@ def try_to_send_notifications_to_queue(notification_id_queue, service, saved_not
     research_mode = service.research_mode  # type: ignore
     for notification_obj in saved_notifications:
         try:
-            if not current_app.config["FF_EMAIL_DAILY_LIMIT"]:
-                check_service_over_daily_message_limit(notification_obj.key_type, service)
             queue = notification_id_queue.get(notification_obj.id) or get_delivery_queue_for_template(template)
             send_notification_to_queue(
                 notification_obj,

app/config.py

@@ -562,8 +562,6 @@ class Config(object):
     # Feature flag to enable custom retry policies such as lowering retry period for certain priority lanes.
     FF_CELERY_CUSTOM_TASK_PARAMS = env.bool("FF_CELERY_CUSTOM_TASK_PARAMS", True)
     FF_CLOUDWATCH_METRICS_ENABLED = env.bool("FF_CLOUDWATCH_METRICS_ENABLED", False)
-    # Feature flags for email_daily_limit
-    FF_EMAIL_DAILY_LIMIT = env.bool("FF_EMAIL_DAILY_LIMIT", False)
     FF_SALESFORCE_CONTACT = env.bool("FF_SALESFORCE_CONTACT", False)
 
     # SRE Tools auth keys
@@ -671,8 +669,6 @@ class Test(Development):
     CRM_ORG_LIST_URL = "https://test-url.com"
     FAILED_LOGIN_LIMIT = 0
 
-    FF_EMAIL_DAILY_LIMIT = False
-
 
 class Production(Config):
     NOTIFY_EMAIL_DOMAIN = os.getenv("NOTIFY_EMAIL_DOMAIN", "notification.canada.ca")

app/notifications/rest.py

@@ -10,6 +10,7 @@
     EMAIL_TYPE,
     INTERNATIONAL_SMS_TYPE,
     KEY_TYPE_TEAM,
+    KEY_TYPE_TEST,
     LETTER_TYPE,
     SMS_TYPE,
     NotificationType,
@@ -21,6 +22,7 @@
     simulated_recipient,
 )
 from app.notifications.validators import (
+    check_email_daily_limit,
     check_rate_limiting,
     check_template_is_active,
     check_template_is_for_notification_type,
@@ -102,12 +104,17 @@ def send_notification(notification_type: NotificationType):
         raise InvalidRequest(errors, status_code=400)
 
     current_app.logger.info(f"POST to V1 API: send_notification, service_id: {authenticated_service.id}")
+
     check_rate_limiting(authenticated_service, api_user)
 
     template = templates_dao.dao_get_template_by_id_and_service_id(
         template_id=notification_form["template"], service_id=authenticated_service.id
     )
 
+    simulated = simulated_recipient(notification_form["to"], notification_type)
+    if not simulated != api_user.key_type == KEY_TYPE_TEST:
+        check_email_daily_limit(authenticated_service, 1)
+
     check_template_is_for_notification_type(notification_type, template.template_type)
     check_template_is_active(template)
 
@@ -124,7 +131,6 @@ def send_notification(notification_type: NotificationType):
         _service_can_send_internationally(authenticated_service, notification_form["to"])
     # Do not persist or send notification to the queue if it is a simulated recipient
 
-    simulated = simulated_recipient(notification_form["to"], notification_type)
     notification_model = persist_notification(
         template_id=template.id,
         template_version=template.version,

app/notifications/validators.py

@@ -227,17 +227,12 @@ def increment_sms_daily_count_send_warnings_if_needed(service: Service, requeste
 
 
 def increment_email_daily_count_send_warnings_if_needed(service: Service, requested_email=0) -> None:
-    if not current_app.config["FF_EMAIL_DAILY_LIMIT"]:
-        return
-
     increment_todays_email_count(service.id, requested_email)
     send_warning_email_limit_emails_if_needed(service)
 
 
 def check_rate_limiting(service: Service, api_key: ApiKey):
     check_service_over_api_rate_limit_and_update_rate(service, api_key)
-    if not current_app.config["FF_EMAIL_DAILY_LIMIT"]:
-        check_service_over_daily_message_limit(api_key.key_type, service)
 
 
 def warn_about_daily_message_limit(service: Service, messages_sent):

app/service/rest.py

@@ -342,9 +342,7 @@ def update_service(service_id):
 def _warn_service_users_about_message_limit_changed(service_id, data):
     send_notification_to_service_users(
         service_id=service_id,
-        template_id=current_app.config["DAILY_EMAIL_LIMIT_UPDATED_TEMPLATE_ID"]
-        if current_app.config["FF_EMAIL_DAILY_LIMIT"]
-        else current_app.config["DAILY_LIMIT_UPDATED_TEMPLATE_ID"],
+        template_id=current_app.config["DAILY_EMAIL_LIMIT_UPDATED_TEMPLATE_ID"],
         personalisation={
             "service_name": data["name"],
             "message_limit_en": "{:,}".format(data["message_limit"]),

app/service/send_notification.py

@@ -67,13 +67,11 @@ def send_one_off_notification(service_id, post_data):
 
     _, template_with_content = validate_template(template.id, personalisation, service, template.template_type)
 
-    if not current_app.config["FF_EMAIL_DAILY_LIMIT"]:
-        check_service_over_daily_message_limit(KEY_TYPE_NORMAL, service)
     if template.template_type == SMS_TYPE:
         is_test_notification = simulated_recipient(post_data["to"], template.template_type)
         if not is_test_notification:
             check_sms_daily_limit(service, 1)
-    elif template.template_type == EMAIL_TYPE and current_app.config["FF_EMAIL_DAILY_LIMIT"]:
+    elif template.template_type == EMAIL_TYPE:
         check_email_daily_limit(service, 1)  # 1 email
 
     validate_and_format_recipient(
@@ -86,13 +84,11 @@ def send_one_off_notification(service_id, post_data):
 
     validate_created_by(service, post_data["created_by"])
 
-    if not current_app.config["FF_EMAIL_DAILY_LIMIT"]:
-        pass  # will remove this soon, don't bother refactoring
     if template.template_type == SMS_TYPE:
         is_test_notification = simulated_recipient(post_data["to"], template.template_type)
         if not is_test_notification:
             increment_sms_daily_count_send_warnings_if_needed(service, 1)
-    elif template.template_type == EMAIL_TYPE and current_app.config["FF_EMAIL_DAILY_LIMIT"]:
+    elif template.template_type == EMAIL_TYPE:
         increment_email_daily_count_send_warnings_if_needed(service, 1)  # 1 email
 
     sender_id = post_data.get("sender_id", None)

tests/app/celery/test_tasks.py

@@ -1328,12 +1328,11 @@ def test_save_sms_should_save_default_smm_sender_notification_reply_to_text_on(s
     def test_should_save_sms_template_to_and_persist_with_job_id(self, notify_api, sample_job, mocker):
         notification = _notification_json(sample_job.template, to="+1 650 253 2222", job_id=sample_job.id, row_number=2)
         mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async")
-        mock_over_daily_limit = mocker.patch("app.celery.tasks.check_service_over_daily_message_limit")
 
         notification_id = uuid.uuid4()
         now = datetime.utcnow()
-        with set_config_values(notify_api, {"FF_EMAIL_DAILY_LIMIT": False}):
-            save_smss(sample_job.template.service_id, [signer_notification.sign(notification)], notification_id)
+
+        save_smss(sample_job.template.service_id, [signer_notification.sign(notification)], notification_id)
         persisted_notification = Notification.query.one()
         assert persisted_notification.to == "+1 650 253 2222"
         assert persisted_notification.job_id == sample_job.id
@@ -1350,7 +1349,6 @@ def test_should_save_sms_template_to_and_persist_with_job_id(self, notify_api, s
         provider_tasks.deliver_sms.apply_async.assert_called_once_with(
             [str(persisted_notification.id)], queue=QueueNames.SEND_SMS_MEDIUM
         )
-        mock_over_daily_limit.assert_called_once_with("normal", sample_job.service)
 
     def test_save_sms_should_go_to_retry_queue_if_database_errors(self, sample_template, mocker):
         notification = _notification_json(sample_template, "+1 650 253 2222")
@@ -1672,7 +1670,6 @@ def test_should_use_email_template_and_persist(
         self, notify_api, sample_email_template_with_placeholders, sample_api_key, mocker
     ):
         mocker.patch("app.celery.provider_tasks.deliver_email.apply_async")
-        mock_over_daily_limit = mocker.patch("app.celery.tasks.check_service_over_daily_message_limit")
 
         now = datetime(2016, 1, 1, 11, 9, 0)
         notification_id = uuid.uuid4()
@@ -1686,10 +1683,9 @@ def test_should_use_email_template_and_persist(
             )
 
         with freeze_time("2016-01-01 11:10:00.00000"):
-            with set_config_values(notify_api, {"FF_EMAIL_DAILY_LIMIT": False}):
-                save_emails(
-                    sample_email_template_with_placeholders.service_id, [signer_notification.sign(notification)], notification_id
-                )
+            save_emails(
+                sample_email_template_with_placeholders.service_id, [signer_notification.sign(notification)], notification_id
+            )
 
         persisted_notification = Notification.query.one()
         assert persisted_notification.to == "my_email@my_email.com"
@@ -1709,7 +1705,6 @@ def test_should_use_email_template_and_persist(
         provider_tasks.deliver_email.apply_async.assert_called_once_with(
             [str(persisted_notification.id)], queue=QueueNames.SEND_EMAIL_MEDIUM
         )
-        mock_over_daily_limit.assert_called_once_with("normal", sample_email_template_with_placeholders.service)
 
     def test_save_email_should_use_template_version_from_job_not_latest(self, sample_email_template, mocker):
         notification = _notification_json(sample_email_template, "my_email@my_email.com")

tests/app/notifications/rest/test_send_notification.py

@@ -28,7 +28,11 @@
     Template,
 )
 from app.utils import get_document_url
-from app.v2.errors import RateLimitError, TooManyRequestsError
+from app.v2.errors import (
+    RateLimitError,
+    TooManyRequestsError,
+    TrialServiceTooManyEmailRequestsError,
+)
 from tests import create_authorization_header
 from tests.app.conftest import (
     create_sample_api_key,
@@ -385,7 +389,7 @@ def test_should_block_api_call_if_over_day_limit_for_live_service(notify_db, not
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
             mocker.patch(
-                "app.notifications.validators.check_service_over_daily_message_limit",
+                "app.notifications.validators.check_service_over_api_rate_limit_and_update_rate",
                 side_effect=TooManyRequestsError(1),
             )
             mocker.patch("app.celery.provider_tasks.deliver_email.apply_async")
@@ -417,13 +421,14 @@ def test_should_block_api_call_if_over_day_limit_for_live_service(notify_db, not
 def test_should_block_api_call_if_over_day_limit_for_restricted_service(notify_db, notify_db_session, notify_api, mocker):
     with notify_api.test_request_context():
         with notify_api.test_client() as client:
-            mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async")
+            mocker.patch("app.celery.provider_tasks.deliver_email.apply_async")
             mocker.patch(
-                "app.notifications.validators.check_service_over_daily_message_limit",
-                side_effect=TooManyRequestsError(1),
+                "app.notifications.validators.check_email_daily_limit",
+                side_effect=TrialServiceTooManyEmailRequestsError(1),
             )
 
             service = create_sample_service(notify_db, notify_db_session, limit=1, restricted=True)
+            create_sample_service_safelist(notify_db, notify_db_session, service=service, email_address="[email protected]")
             email_template = create_sample_email_template(notify_db, notify_db_session, service=service)
             create_sample_notification(
                 notify_db,

tests/app/notifications/test_validators.py

@@ -217,8 +217,7 @@ def test_check_service_message_limit_records_nearing_daily_limit(
             if limit_type == "sms":
                 increment_sms_daily_count_send_warnings_if_needed(service)
             else:
-                with set_config(notify_api, "FF_EMAIL_DAILY_LIMIT", True):
-                    increment_email_daily_count_send_warnings_if_needed(service)
+                increment_email_daily_count_send_warnings_if_needed(service)
 
             assert redis_get.call_args_list == [
                 call(count_key(limit_type, service.id)),
@@ -290,8 +289,7 @@ def test_check_service_message_limit_in_cache_over_message_limit_fails(
             assert e.value.fields == []
 
             with pytest.raises(TooManyEmailRequestsError) as e:
-                with set_config(notify_api, "FF_EMAIL_DAILY_LIMIT", True):
-                    check_email_daily_limit(service)
+                check_email_daily_limit(service)
             assert e.value.status_code == 429
             assert e.value.message == "Exceeded email daily sending limit of 4 messages"
             assert e.value.fields == []
@@ -350,8 +348,8 @@ def test_check_service_message_limit_skip_statsd_over_message_no_limit_fails_ema
 
         # When
         service = create_sample_service(notify_db, notify_db_session, restricted=True, limit=4, sms_limit=4)
-        with set_config(notify_api, "FF_EMAIL_DAILY_LIMIT", True):
-            check_email_daily_limit(service)
+        check_email_daily_limit(service)
+
         # Then
         app_statsd.statsd_client.incr.assert_not_called()
 

tests/app/service/test_rest.py

@@ -2225,9 +2225,7 @@ def test_update_service_updating_daily_limit_sends_notification_to_users(
     if expected_call:
         send_notification_mock.assert_called_once_with(
             service_id=service.id,
-            template_id=current_app.config["DAILY_EMAIL_LIMIT_UPDATED_TEMPLATE_ID"]
-            if current_app.config["FF_EMAIL_DAILY_LIMIT"]
-            else current_app.config["DAILY_LIMIT_UPDATED_TEMPLATE_ID"],
+            template_id=current_app.config["DAILY_EMAIL_LIMIT_UPDATED_TEMPLATE_ID"],
             personalisation={
                 "service_name": service.name,
                 "message_limit_en": "{:,}".format(new_limit),

tests/app/service/test_send_one_off_notification.py

@@ -21,7 +21,6 @@
     BadRequestError,
     LiveServiceTooManyEmailRequestsError,
     LiveServiceTooManySMSRequestsError,
-    TooManyRequestsError,
 )
 from tests.app.db import (
     create_letter_contact,
@@ -31,7 +30,6 @@
     create_template,
     create_user,
 )
-from tests.conftest import set_config_values
 
 
 @pytest.fixture
@@ -282,12 +280,12 @@ def test_send_one_off_notification_raises_if_cant_send_to_recipient(
     assert "service is in trial mode" in e.value.message
 
 
-def test_send_one_off_notification_raises_if_over_combined_limit(notify_db_session, mocker):
+def test_send_one_off_notification_raises_if_over_combined_limit(notify_db_session, notify_api, mocker):
     service = create_service(message_limit=0)
     template = create_template(service=service)
     mocker.patch(
-        "app.service.send_notification.check_service_over_daily_message_limit",
-        side_effect=TooManyRequestsError(1),
+        "app.service.send_notification.check_sms_daily_limit",
+        side_effect=LiveServiceTooManySMSRequestsError(1),
     )
 
     post_data = {
@@ -296,7 +294,7 @@ def test_send_one_off_notification_raises_if_over_combined_limit(notify_db_sessi
         "created_by": str(service.created_by_id),
     }
 
-    with pytest.raises(TooManyRequestsError):
+    with pytest.raises(LiveServiceTooManySMSRequestsError):
         send_one_off_notification(service.id, post_data)
 
 
@@ -314,9 +312,8 @@ def test_send_one_off_notification_raises_if_over_email_limit(notify_db_session,
         "created_by": str(service.created_by_id),
     }
 
-    with set_config_values(notify_api, {"FF_EMAIL_DAILY_LIMIT": True}):
-        with pytest.raises(LiveServiceTooManyEmailRequestsError):
-            send_one_off_notification(service.id, post_data)
+    with pytest.raises(LiveServiceTooManyEmailRequestsError):
+        send_one_off_notification(service.id, post_data)
 
 
 def test_send_one_off_notification_raises_if_over_sms_daily_limit(notify_db_session, mocker):