From 360d4f2a9a2160df1971e1406e5f338d10778fe2 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 20 Jun 2024 11:55:01 -0700 Subject: [PATCH 1/2] fix raw_set and raw_get --- .../test_service_invite_rest.py | 8 ++++---- tests/app/user/test_rest_verify.py | 20 +++++++++---------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/tests/app/service_invite/test_service_invite_rest.py b/tests/app/service_invite/test_service_invite_rest.py index 0f60dc3e3..327729776 100644 --- a/tests/app/service_invite/test_service_invite_rest.py +++ b/tests/app/service_invite/test_service_invite_rest.py @@ -96,8 +96,8 @@ def test_create_invited_user_without_auth_type( admin_request, sample_service, mocker, invitation_email_template ): - mocker.patch("app.service_invite.rest.redis_store.raw_set") - mocker.patch("app.service_invite.rest.redis_store.raw_get") + mocker.patch("app.service_invite.rest.redis_store.set") + mocker.patch("app.service_invite.rest.redis_store.get") mocker.patch("app.celery.provider_tasks.deliver_email.apply_async") email_address = "invited_user@service.gov.uk" invite_from = sample_service.users[0] @@ -220,8 +220,8 @@ def test_resend_expired_invite( mocker, ): - mocker.patch("app.service_invite.rest.redis_store.raw_set") - mocker.patch("app.service_invite.rest.redis_store.raw_get") + mocker.patch("app.service_invite.rest.redis_store.set") + mocker.patch("app.service_invite.rest.redis_store.get") url = f"/service/{sample_expired_user.service_id}/invite/{sample_expired_user.id}/resend" mock_send = mocker.patch("app.service_invite.rest.send_notification_to_queue") mock_persist = mocker.patch("app.service_invite.rest.persist_notification") diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 81d813039..ff74f6b57 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -201,10 +201,10 @@ def test_send_user_sms_code(client, sample_user, sms_code_template, mocker): """ notify_service = dao_fetch_service_by_id(current_app.config["NOTIFY_SERVICE_ID"]) - mock_redis_get = mocker.patch("app.user.rest.redis_store.raw_get") + mock_redis_get = mocker.patch("app.user.rest.redis_store.get") mock_redis_get.return_value = "foo" - mocker.patch("app.user.rest.redis_store.raw_set") + mocker.patch("app.user.rest.redis_store.set") auth_header = create_admin_authorization_header() mocked = mocker.patch("app.user.rest.create_secret_code", return_value="11111") mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async") @@ -242,10 +242,10 @@ def test_send_user_code_for_sms_with_optional_to_field( Tests POST endpoint /user//sms-code with optional to field """ - mock_redis_get = mocker.patch("app.user.rest.redis_store.raw_get") + mock_redis_get = mocker.patch("app.user.rest.redis_store.get") mock_redis_get.return_value = "foo" - mocker.patch("app.user.rest.redis_store.raw_set") + mocker.patch("app.user.rest.redis_store.set") to_number = "+14254147755" mocked = mocker.patch("app.user.rest.create_secret_code", return_value="11111") mocker.patch("app.celery.provider_tasks.deliver_sms.apply_async") @@ -469,10 +469,10 @@ def test_send_user_email_code( deliver_email = mocker.patch("app.celery.provider_tasks.deliver_email.apply_async") sample_user.auth_type = auth_type - mock_redis_get = mocker.patch("app.user.rest.redis_store.raw_get") + mock_redis_get = mocker.patch("app.user.rest.redis_store.get") mock_redis_get.return_value = "foo" - mocker.patch("app.user.rest.redis_store.raw_set") + mocker.patch("app.user.rest.redis_store.set") admin_request.post( "user.send_user_2fa_code", @@ -497,10 +497,10 @@ def test_send_user_email_code_with_urlencoded_next_param( ): mocker.patch("app.celery.provider_tasks.deliver_email.apply_async") - mock_redis_get = mocker.patch("app.celery.scheduled_tasks.redis_store.raw_get") + mock_redis_get = mocker.patch("app.celery.scheduled_tasks.redis_store.get") mock_redis_get.return_value = "foo" - mocker.patch("app.celery.scheduled_tasks.redis_store.raw_set") + mocker.patch("app.celery.scheduled_tasks.redis_store.set") data = {"to": None, "next": "/services"} admin_request.post( @@ -581,10 +581,10 @@ def test_user_verify_email_code_fails_if_code_already_used( def test_send_user_2fa_code_sends_from_number_for_international_numbers( client, sample_user, mocker, sms_code_template ): - mock_redis_get = mocker.patch("app.user.rest.redis_store.raw_get") + mock_redis_get = mocker.patch("app.user.rest.redis_store.get") mock_redis_get.return_value = "foo" - mocker.patch("app.user.rest.redis_store.raw_set") + mocker.patch("app.user.rest.redis_store.set") sample_user.mobile_number = "+601117224412" auth_header = create_admin_authorization_header() From 2e4fd3b3ac6f844563b48ce229e0dbdcdc4e265a Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 20 Jun 2024 13:30:43 -0700 Subject: [PATCH 2/2] fix tests --- app/delivery/send_to_providers.py | 2 +- app/user/rest.py | 2 +- .../clients/redis/redis_client.py | 16 ++---------- .../test_service_invite_rest.py | 4 +-- .../clients/redis/test_redis_client.py | 26 +------------------ 5 files changed, 7 insertions(+), 43 deletions(-) diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index a347b89ad..b6872620b 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -89,7 +89,7 @@ def send_sms_to_provider(notification): except Exception: # It is our 2facode, maybe key = f"2facode-{notification.id}".replace(" ", "") - recipient = redis_store.raw_get(key) + recipient = redis_store.get(key) if recipient: recipient = recipient.decode("utf-8") diff --git a/app/user/rest.py b/app/user/rest.py index 760334841..049549f2c 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -361,7 +361,7 @@ def create_2fa_code( saved_notification.personalisation = personalisation key = f"2facode-{saved_notification.id}".replace(" ", "") recipient = str(recipient) - redis_store.raw_set(key, recipient, ex=60 * 60) + redis_store.set(key, recipient, ex=60 * 60) # Assume that we never want to observe the Notify service's research mode # setting for this notification - we still need to be able to log into the diff --git a/notifications_utils/clients/redis/redis_client.py b/notifications_utils/clients/redis/redis_client.py index 6e81d85bf..1723dd2c1 100644 --- a/notifications_utils/clients/redis/redis_client.py +++ b/notifications_utils/clients/redis/redis_client.py @@ -133,19 +133,13 @@ class RedisClient: else: return False - def raw_set(self, key, value, ex=None, px=None, nx=False, xx=False): - self.redis_store.set(key, value, ex, px, nx, xx) - def set( self, key, value, ex=None, px=None, nx=False, xx=False, raise_exception=False ): key = prepare_value(key) value = prepare_value(value) if self.active: - try: - self.redis_store.set(key, value, ex, px, nx, xx) - except Exception as e: - self.__handle_exception(e, raise_exception, "set", key) + self.redis_store.set(key, value, ex, px, nx, xx) def incr(self, key, raise_exception=False): key = prepare_value(key) @@ -155,16 +149,10 @@ class RedisClient: except Exception as e: self.__handle_exception(e, raise_exception, "incr", key) - def raw_get(self, key): - return self.redis_store.get(key) - def get(self, key, raise_exception=False): key = prepare_value(key) if self.active: - try: - return self.redis_store.get(key) - except Exception as e: - self.__handle_exception(e, raise_exception, "get", key) + return self.redis_store.get(key) return None diff --git a/tests/app/service_invite/test_service_invite_rest.py b/tests/app/service_invite/test_service_invite_rest.py index 327729776..07d0b4c23 100644 --- a/tests/app/service_invite/test_service_invite_rest.py +++ b/tests/app/service_invite/test_service_invite_rest.py @@ -31,8 +31,8 @@ def test_create_invited_user( extra_args, expected_start_of_invite_url, ): - mocker.patch("app.service_invite.rest.redis_store.raw_set") - mocker.patch("app.service_invite.rest.redis_store.raw_get") + mocker.patch("app.service_invite.rest.redis_store.set") + mocker.patch("app.service_invite.rest.redis_store.get") mocked = mocker.patch("app.celery.provider_tasks.deliver_email.apply_async") email_address = "invited_user@service.gov.uk" diff --git a/tests/notifications_utils/clients/redis/test_redis_client.py b/tests/notifications_utils/clients/redis/test_redis_client.py index c9eb63240..52b227128 100644 --- a/tests/notifications_utils/clients/redis/test_redis_client.py +++ b/tests/notifications_utils/clients/redis/test_redis_client.py @@ -1,5 +1,5 @@ import uuid -from unittest.mock import Mock, call +from unittest.mock import Mock import pytest from freezegun import freeze_time @@ -59,30 +59,6 @@ def failing_redis_client(mocked_redis_client, delete_mock): return mocked_redis_client -def test_should_not_raise_exception_if_raise_set_to_false( - app, caplog, failing_redis_client, mocker -): - mock_logger = mocker.patch("flask.Flask.logger") - - assert failing_redis_client.get("get_key") is None - assert failing_redis_client.set("set_key", "set_value") is None - assert failing_redis_client.incr("incr_key") is None - assert failing_redis_client.exceeded_rate_limit("rate_limit_key", 100, 100) is False - assert failing_redis_client.delete("delete_key") is None - assert failing_redis_client.delete("a", "b", "c") is None - assert failing_redis_client.delete_by_pattern("pattern") == 0 - - assert mock_logger.mock_calls == [ - call.exception("Redis error performing get on get_key"), - call.exception("Redis error performing set on set_key"), - call.exception("Redis error performing incr on incr_key"), - call.exception("Redis error performing rate-limit-pipeline on rate_limit_key"), - call.exception("Redis error performing delete on delete_key"), - call.exception("Redis error performing delete on a, b, c"), - call.exception("Redis error performing delete-by-pattern on pattern"), - ] - - def test_should_raise_exception_if_raise_set_to_true( app, failing_redis_client,