From e6e16a81d03f9a9f8213fa4bf3239a19550b8341 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 24 Mar 2022 17:31:53 +0000 Subject: [PATCH] Simplify getting name of email / sms providers Previously we used a combination of "provider.name" and "get_name()" which was confusing. Using a non-property function also gave me the impression that the name was more dynamic than it actually is. --- app/clients/email/__init__.py | 3 ++- app/clients/email/aws_ses.py | 6 +++--- app/clients/email/aws_ses_stub.py | 6 +++--- app/clients/sms/__init__.py | 3 ++- app/clients/sms/firetext.py | 6 +++--- app/clients/sms/mmg.py | 6 +++--- app/delivery/send_to_providers.py | 6 +++--- tests/app/delivery/test_send_to_providers.py | 6 +++--- 8 files changed, 22 insertions(+), 20 deletions(-) diff --git a/app/clients/email/__init__.py b/app/clients/email/__init__.py index e680be23d..004f76b3a 100644 --- a/app/clients/email/__init__.py +++ b/app/clients/email/__init__.py @@ -28,5 +28,6 @@ class EmailClient(Client): def send_email(self, *args, **kwargs): raise NotImplementedError('TODO Need to implement.') - def get_name(self): + @property + def name(self): raise NotImplementedError('TODO Need to implement.') diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 4588a53b7..1f8b2f75c 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -59,11 +59,11 @@ class AwsSesClient(EmailClient): def init_app(self, region, statsd_client, *args, **kwargs): self._client = boto3.client('ses', region_name=region) super(AwsSesClient, self).__init__(*args, **kwargs) - self.name = 'ses' self.statsd_client = statsd_client - def get_name(self): - return self.name + @property + def name(self): + return 'ses' def send_email(self, source, diff --git a/app/clients/email/aws_ses_stub.py b/app/clients/email/aws_ses_stub.py index 4d78e22b5..bce9d3e9a 100644 --- a/app/clients/email/aws_ses_stub.py +++ b/app/clients/email/aws_ses_stub.py @@ -13,12 +13,12 @@ class AwsSesStubClientException(EmailClientException): class AwsSesStubClient(EmailClient): def init_app(self, region, statsd_client, stub_url): - self.name = 'ses' self.statsd_client = statsd_client self.url = stub_url - def get_name(self): - return self.name + @property + def name(self): + return 'ses' def send_email(self, source, diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index e9f90f06a..0ca4ff7ae 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -21,5 +21,6 @@ class SmsClient(Client): def send_sms(self, *args, **kwargs): raise NotImplementedError('TODO Need to implement.') - def get_name(self): + @property + def name(self): raise NotImplementedError('TODO Need to implement.') diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 0d9b23b44..3bece886e 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -68,12 +68,12 @@ class FiretextClient(SmsClient): self.api_key = current_app.config.get('FIRETEXT_API_KEY') self.international_api_key = current_app.config.get('FIRETEXT_INTERNATIONAL_API_KEY') self.from_number = current_app.config.get('FROM_NUMBER') - self.name = 'firetext' self.url = current_app.config.get('FIRETEXT_URL') self.statsd_client = statsd_client - def get_name(self): - return self.name + @property + def name(self): + return 'firetext' def record_outcome(self, success, response): status_code = response.status_code if response else 503 diff --git a/app/clients/sms/mmg.py b/app/clients/sms/mmg.py index ec32fa0a9..1013424de 100644 --- a/app/clients/sms/mmg.py +++ b/app/clients/sms/mmg.py @@ -74,7 +74,6 @@ class MMGClient(SmsClient): self.current_app = current_app self.api_key = current_app.config.get('MMG_API_KEY') self.from_number = current_app.config.get('FROM_NUMBER') - self.name = 'mmg' self.statsd_client = statsd_client self.mmg_url = current_app.config.get('MMG_URL') @@ -94,8 +93,9 @@ class MMGClient(SmsClient): self.statsd_client.incr("clients.mmg.error") self.current_app.logger.warning(log_message) - def get_name(self): - return self.name + @property + def name(self): + return 'mmg' def send_sms(self, to, content, reference, international, multi=True, sender=None): data = { diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 37994691d..162bb3f3a 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -60,7 +60,7 @@ def send_sms_to_provider(notification): key_type = notification.key_type if service.research_mode or notification.key_type == KEY_TYPE_TEST: update_notification_to_sending(notification, provider) - send_sms_response(provider.get_name(), str(notification.id), notification.to) + send_sms_response(provider.name, str(notification.id), notification.to) else: try: @@ -82,7 +82,7 @@ def send_sms_to_provider(notification): except Exception as e: notification.billable_units = template.fragment_count dao_update_notification(notification) - dao_reduce_sms_provider_priority(provider.get_name(), time_threshold=timedelta(minutes=1)) + dao_reduce_sms_provider_priority(provider.name, time_threshold=timedelta(minutes=1)) raise e else: notification.billable_units = template.fragment_count @@ -158,7 +158,7 @@ def send_email_to_provider(notification): def update_notification_to_sending(notification, provider): notification.sent_at = datetime.utcnow() - notification.sent_by = provider.get_name() + notification.sent_by = provider.name if notification.status not in NOTIFICATION_STATUS_TYPES_COMPLETED: notification.status = NOTIFICATION_SENT if notification.international else NOTIFICATION_SENDING dao_update_notification(notification) diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 9fde06690..404d9c513 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -54,7 +54,7 @@ def test_provider_to_use_should_return_random_provider(mocker, notify_db_session ret = send_to_providers.provider_to_use('sms', international=False) mock_choices.assert_called_once_with([mmg, firetext], weights=[25, 75]) - assert ret.get_name() == 'mmg' + assert ret.name == 'mmg' def test_provider_to_use_should_cache_repeated_calls(mocker, notify_db_session): @@ -89,7 +89,7 @@ def test_provider_to_use_should_only_return_mmg_for_international( ret = send_to_providers.provider_to_use('sms', international=True) mock_choices.assert_called_once_with([mmg], weights=[100]) - assert ret.get_name() == 'mmg' + assert ret.name == 'mmg' def test_provider_to_use_should_only_return_active_providers(mocker, restore_provider_details): @@ -101,7 +101,7 @@ def test_provider_to_use_should_only_return_active_providers(mocker, restore_pro ret = send_to_providers.provider_to_use('sms') mock_choices.assert_called_once_with([firetext], weights=[100]) - assert ret.get_name() == 'firetext' + assert ret.name == 'firetext' def test_provider_to_use_raises_if_no_active_providers(mocker, restore_provider_details):