From c13045c314ffa80f3cab5f6d20db9a12c55df43e Mon Sep 17 00:00:00 2001 From: wbanks Date: Fri, 17 Nov 2023 15:53:16 -0500 Subject: [PATCH] Update fetch_todays_total_sms_count no longer count by fragment - Removed a number of tests involving counting of fragments vs limits - Fixed a call to check_sms_daily_limit that was still passing fragments into it's parameters --- app/dao/services_dao.py | 4 +- app/v2/notifications/post_notifications.py | 2 +- tests/app/dao/test_services_dao.py | 7 - .../notifications/test_post_notifications.py | 243 +----------------- 4 files changed, 8 insertions(+), 248 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 3183cbdef9..80d1f25c35 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -443,7 +443,7 @@ def fetch_todays_total_message_count(service_id): def fetch_todays_total_sms_count(service_id): midnight = get_midnight(datetime.now(tz=pytz.utc)) result = ( - db.session.query(func.sum(Notification.billable_units).label("sum_billable_units")) + db.session.query(func.count(Notification.id).label("total_sms_notifications")) .filter( Notification.service_id == service_id, Notification.key_type != KEY_TYPE_TEST, @@ -452,7 +452,7 @@ def fetch_todays_total_sms_count(service_id): ) .first() ) - return 0 if result is None or result.sum_billable_units is None else result.sum_billable_units + return 0 if result is None or result.total_sms_notifications is None else result.total_sms_notifications def fetch_service_email_limit(service_id: uuid.UUID) -> int: diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index cf979b1923..d9d333c0e0 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -297,7 +297,7 @@ def post_notification(notification_type: NotificationType): if template.template_type == SMS_TYPE: is_test_notification = api_user.key_type == KEY_TYPE_TEST or simulated_recipient(form["phone_number"], notification_type) if not is_test_notification: - check_sms_daily_limit(authenticated_service, template_with_content.fragment_count) + check_sms_daily_limit(authenticated_service, 1) current_app.logger.info(f"Trying to send notification for Template ID: {template.id}") diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index ecdb17f634..4a81ea4f86 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -1090,13 +1090,6 @@ def test_only_counts_sms(self): save_notification(create_notification(template=email_template)) assert fetch_todays_total_sms_count(service.id) == 2 - def test_sums_billable_units(self): - service = create_service() - sms_template = create_template(service=service, template_type=SMS_TYPE) - save_notification(create_notification(template=sms_template, billable_units=3)) - save_notification(create_notification(template=sms_template, billable_units=10)) - assert fetch_todays_total_sms_count(service.id) == 13 - def test_returns_0_when_no_messages_for_today(self): assert fetch_todays_total_sms_count(uuid.uuid4()) == 0 diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 5bb2831012..7847c89164 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1450,7 +1450,7 @@ def test_post_notification_without_document_upload_permission(self, client, noti class TestSMSSendFragments: - def test_post_sms_enough_fragments_left(self, notify_api, client, notify_db, notify_db_session, mocker): + def test_post_sms_enough_messages_left(self, notify_api, client, notify_db, notify_db_session, mocker): mocker.patch("app.sms_normal_publish.publish") service = create_service(sms_daily_limit=10, message_limit=100) template = create_sample_template(notify_db, notify_db_session, content=500 * "a", service=service, template_type="sms") @@ -1459,7 +1459,7 @@ def test_post_sms_enough_fragments_left(self, notify_api, client, notify_db, not "template_id": str(template.id), "personalisation": {" Name": "Jo"}, } - for x in range(2): + for x in range(6): create_sample_notification(notify_db, notify_db_session, service=service) auth_header = create_authorization_header(service_id=template.service_id) @@ -1471,7 +1471,7 @@ def test_post_sms_enough_fragments_left(self, notify_api, client, notify_db, not ) assert response.status_code == 201 - def test_post_sms_not_enough_fragments_left(self, notify_api, client, notify_db, notify_db_session, mocker): + def test_post_sms_not_enough_messages_left(self, notify_api, client, notify_db, notify_db_session, mocker): mocker.patch("app.sms_normal_publish.publish") service = create_service(sms_daily_limit=10, message_limit=100) template = create_sample_template(notify_db, notify_db_session, content=500 * "a", service=service, template_type="sms") @@ -1480,7 +1480,7 @@ def test_post_sms_not_enough_fragments_left(self, notify_api, client, notify_db, "template_id": str(template.id), "personalisation": {" Name": "Jo"}, } - for x in range(7): + for x in range(10): create_sample_notification(notify_db, notify_db_session, service=service) auth_header = create_authorization_header(service_id=template.service_id) @@ -1493,7 +1493,7 @@ def test_post_sms_not_enough_fragments_left(self, notify_api, client, notify_db, assert response.status_code == 429 -class TestSMSFragmentCounter: +class TestSMSMessageCounter: # Testing API one-off: # - Sending using TEST, NORMAL, and TEAM API keys with a simulated phone number should not count towards limits # TODO: update these params when we fix https://github.com/cds-snc/notification-planning/issues/855 and remove the xfao; @@ -1772,40 +1772,6 @@ def __send_sms(): response = __send_sms() # send the 11th fragment assert response.status_code == 429 # Ensure send is blocked - def test_API_ONEOFF_cant_hop_over_limit_using_3_fragment_sms(self, notify_api, client, notify_db, notify_db_session, mocker): - # test setup - mocker.patch("app.sms_normal_publish.publish") - send_warning_email = mocker.patch("app.notifications.validators.send_near_sms_limit_email") - - def __send_sms(): - auth_header = create_authorization_header(service_id=template.service_id) - with set_config_values(notify_api, {"REDIS_ENABLED": True}): - response = client.post( - path="/v2/notifications/sms", - data=json.dumps(data), - headers=[("Content-Type", "application/json"), auth_header], - ) - return response - - # Create a service, Set limit to 10 fragments - service = create_service(sms_daily_limit=10, message_limit=100) - - # Create 5 notifications in the db - template = create_sample_template(notify_db, notify_db_session, content="a" * 400, service=service, template_type="sms") - data = { - "phone_number": "+16502532222", - "template_id": str(template.id), - "personalisation": {" Name": "Jo"}, - } - for x in range(7): - create_sample_notification(notify_db, notify_db_session, service=service) - - __send_sms() # send 1 sms (3 fragments) should be at 80% - assert send_warning_email.called - - __send_sms() - response = __send_sms() # send one more, puts us at 11/10 fragments - assert response.status_code == 429 # Ensure send is blocked def test_API_BULK_sends_warning_emails_and_blocks_sends(self, notify_api, client, notify_db, notify_db_session, mocker): # test setup @@ -1850,52 +1816,6 @@ def __send_sms(): response = __send_sms() # send the 11th fragment assert response.status_code == 400 # Ensure send is blocked - not sure why we send a 400 here and a 429 everywhere else - def test_API_BULK_cant_hop_over_limit_1_fragment(self, notify_api, client, notify_db, notify_db_session, mocker): - # test setup - mocker.patch("app.sms_normal_publish.publish") - mocker.patch("app.v2.notifications.post_notifications.create_bulk_job", return_value=str(uuid.uuid4())) - send_warning_email = mocker.patch("app.notifications.validators.send_near_sms_limit_email") - send_limit_reached_email = mocker.patch("app.notifications.validators.send_sms_limit_reached_email") - - def __send_sms(number_to_send=1): - with set_config_values(notify_api, {"REDIS_ENABLED": True}): - numbers = [["9025551234"]] * number_to_send - data = { - "name": "job_name", - "template_id": str(template.id), - "rows": [["phone number"], *numbers], - } - - response = client.post( - "/v2/notifications/bulk", - data=json.dumps(data), - headers=[ - ("Content-Type", "application/json"), - create_authorization_header(service_id=template.service_id), - ], - ) - return response - - # Create a service, Set limit to 10 fragments - service = create_service(sms_daily_limit=10, message_limit=100) - - # Create 7 notifications in the db - template = create_sample_template(notify_db, notify_db_session, content="Hello", service=service, template_type="sms") - for x in range(7): - create_sample_notification(notify_db, notify_db_session, service=service) - - __send_sms(1) # send 1 sms (1 fragment) should be at 80% - assert send_warning_email.called - - response = __send_sms(3) # attempt to send over limit (11 with max 10) - assert response.status_code == 400 - - __send_sms(2) # attempt to send at limit (10 with max 10) - assert send_limit_reached_email.called - - response = __send_sms(1) # attempt to send over limit (11 with max 10)1 - assert response.status_code == 400 # Ensure send is blocked - not sure why we send a 400 here and a 429 everywhere else - # ADMIN def test_ADMIN_ONEOFF_sends_warning_emails_and_blocks_sends(self, notify_api, client, notify_db, notify_db_session, mocker): # test setup @@ -1940,47 +1860,6 @@ def __send_sms(): response = __send_sms() # 11/10 fragments assert response.status_code == 429 # Ensure send is blocked - def test_ADMIN_ONEOFF_cant_hop_over_limit_using_3_fragment_sms( - self, notify_api, client, notify_db, notify_db_session, mocker - ): - # test setup - mocker.patch("app.sms_normal_publish.publish") - - mocker.patch("app.service.send_notification.send_notification_to_queue") - send_warning_email = mocker.patch("app.notifications.validators.send_near_sms_limit_email") - - def __send_sms(): - with set_config_values(notify_api, {"REDIS_ENABLED": True}): - token = create_jwt_token( - current_app.config["ADMIN_CLIENT_SECRET"], client_id=current_app.config["ADMIN_CLIENT_USER_NAME"] - ) - response = client.post( - f"/service/{template.service_id}/send-notification", - json={ - "to": "9025551234", - "template_id": str(template.id), - "created_by": service.users[0].id, - "personalisation": {"var": "var"}, - }, - headers={"Authorization": f"Bearer {token}"}, - ) - return response - - # Create a service, Set limit to 10 fragments - service = create_service(sms_daily_limit=10, message_limit=100) - - # Create 5 notifications in the db - template = create_sample_template(notify_db, notify_db_session, content="a" * 400, service=service, template_type="sms") - for x in range(8): - create_sample_notification(notify_db, notify_db_session, service=service) - - __send_sms() # 9/10 fragments - assert send_warning_email.called - __send_sms() - - response = __send_sms() # 11/10 fragments - assert response.status_code == 429 # Ensure send is blocked - def test_ADMIN_CSV_sends_warning_emails_and_blocks_sends(self, notify_api, client, notify_db, notify_db_session, mocker): # test setup mocker.patch("app.sms_normal_publish.publish") @@ -2036,118 +1915,6 @@ def __send_sms(): response = __send_sms() # 11/10 fragments assert response.status_code == 429 # Ensure send is blocked - def test_ADMIN_CSV_cant_hop_over_limit_using_1_fragment_sms(self, notify_api, client, notify_db, notify_db_session, mocker): - # test setup - mocker.patch("app.sms_normal_publish.publish") - mocker.patch("app.service.send_notification.send_notification_to_queue") - mocker.patch("app.celery.tasks.process_job.apply_async") - - send_warning_email = mocker.patch("app.notifications.validators.send_near_sms_limit_email") - send_limit_reached_email = mocker.patch("app.notifications.validators.send_sms_limit_reached_email") - - def __send_sms(number_to_send=1): - with set_config_values(notify_api, {"REDIS_ENABLED": True}): - phone_numbers = "\r\n6502532222" * number_to_send - mocker.patch("app.job.rest.get_job_from_s3", return_value=f"phone number{phone_numbers}") - mocker.patch( - "app.job.rest.get_job_metadata_from_s3", - return_value={ - "template_id": str(template.id), - "original_file_name": "thisisatest.csv", - "notification_count": f"{number_to_send}", - "valid": "True", - }, - ) - - token = create_jwt_token( - current_app.config["ADMIN_CLIENT_SECRET"], client_id=current_app.config["ADMIN_CLIENT_USER_NAME"] - ) - response = client.post( - f"/service/{template.service_id}/job", - json={ - "id": str(uuid.uuid4()), - "created_by": service.users[0].id, - }, - headers={"Authorization": f"Bearer {token}"}, - ) - return response - - # Create a service, Set limit to 10 fragments - service = create_service(sms_daily_limit=10, message_limit=100) - - # Create 7 notifications in the db - template = create_sample_template(notify_db, notify_db_session, content="Hello", service=service, template_type="sms") - for x in range(7): - create_sample_notification(notify_db, notify_db_session, service=service) - - __send_sms(1) # 8/10 fragments - assert send_warning_email.called - - response = __send_sms(3) # 11/10 fragments - assert response.status_code == 429 - - __send_sms(2) # 10/10 fragments - assert send_limit_reached_email.called - - response = __send_sms(1) # 11/10 fragments - assert response.status_code == 429 # Ensure send is blocked - not sure why we send a 400 here and a 429 everywhere else - - def test_ADMIN_CSV_cant_hop_over_limit_using_2_fragment_sms(self, notify_api, client, notify_db, notify_db_session, mocker): - # test setup - mocker.patch("app.sms_normal_publish.publish") - mocker.patch("app.service.send_notification.send_notification_to_queue") - mocker.patch("app.celery.tasks.process_job.apply_async") - - send_warning_email = mocker.patch("app.notifications.validators.send_near_sms_limit_email") - send_limit_reached_email = mocker.patch("app.notifications.validators.send_sms_limit_reached_email") - - def __send_sms(number_to_send=1): - with set_config_values(notify_api, {"REDIS_ENABLED": True}): - phone_numbers = "\r\n6502532222" * number_to_send - mocker.patch("app.job.rest.get_job_from_s3", return_value=f"phone number{phone_numbers}") - mocker.patch( - "app.job.rest.get_job_metadata_from_s3", - return_value={ - "template_id": str(template.id), - "original_file_name": "thisisatest.csv", - "notification_count": f"{number_to_send}", - "valid": "True", - }, - ) - - token = create_jwt_token( - current_app.config["ADMIN_CLIENT_SECRET"], client_id=current_app.config["ADMIN_CLIENT_USER_NAME"] - ) - response = client.post( - f"/service/{template.service_id}/job", - json={ - "id": str(uuid.uuid4()), - "created_by": service.users[0].id, - }, - headers={"Authorization": f"Bearer {token}"}, - ) - return response - - # Create a service, Set limit to 10 fragments - service = create_service(sms_daily_limit=10, message_limit=100) - - # Create 6 notifications in the db - template = create_sample_template(notify_db, notify_db_session, content="A" * 200, service=service, template_type="sms") - for x in range(6): - create_sample_notification(notify_db, notify_db_session, service=service) - - __send_sms(2) # 8/10 fragments - assert send_warning_email.called - - response = __send_sms(4) # 12/10 fragments - assert response.status_code == 429 - - __send_sms(2) # 10/10 fragments - assert send_limit_reached_email.called - - response = __send_sms(1) # 11/10 fragments - assert response.status_code == 429 # Ensure send is blocked - not sure why we send a 400 here and a 429 everywhere else - class TestBulkSend: @pytest.mark.parametrize("args", [{}, {"rows": [1, 2], "csv": "foo"}], ids=["no args", "both args"])