From 1d3a4e5043f0bf185dbcc1e7d338f2f41a2c269e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 29 Jan 2019 11:12:33 +0000 Subject: [PATCH 1/3] =?UTF-8?q?Inherit=20don=E2=80=99t=20duplicate=20API?= =?UTF-8?q?=20client=20constructor?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This removes some code which is duplicative and obscure (ie it’s not very clear why we do `"a" * 73` even though there is a Very Good Reason for doing so). --- app/notify_client/__init__.py | 3 +++ app/notify_client/api_key_api_client.py | 2 -- app/notify_client/billing_api_client.py | 4 ---- app/notify_client/complaint_api_client.py | 4 ---- app/notify_client/email_branding_client.py | 3 --- app/notify_client/events_api_client.py | 2 -- app/notify_client/inbound_number_client.py | 3 --- app/notify_client/invite_api_client.py | 2 -- app/notify_client/job_api_client.py | 3 --- app/notify_client/letter_branding_client.py | 3 --- app/notify_client/letter_jobs_client.py | 3 --- app/notify_client/notification_api_client.py | 2 -- app/notify_client/org_invite_api_client.py | 2 -- app/notify_client/organisations_api_client.py | 3 --- app/notify_client/platform_stats_api_client.py | 4 ---- app/notify_client/provider_client.py | 2 -- app/notify_client/service_api_client.py | 4 ---- app/notify_client/status_api_client.py | 2 -- app/notify_client/template_folder_api_client.py | 4 ---- app/notify_client/template_statistics_api_client.py | 2 -- app/notify_client/user_api_client.py | 2 -- .../notify_client/test_notify_admin_api_client.py | 12 +++++------- 22 files changed, 8 insertions(+), 63 deletions(-) diff --git a/app/notify_client/__init__.py b/app/notify_client/__init__.py index e47930a09..bcd9f8fb9 100644 --- a/app/notify_client/__init__.py +++ b/app/notify_client/__init__.py @@ -16,6 +16,9 @@ class NotifyAdminAPIClient(BaseAPIClient): redis_client = RedisClient() + def __init__(self): + super().__init__("a" * 73, "b") + def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] diff --git a/app/notify_client/api_key_api_client.py b/app/notify_client/api_key_api_client.py index 55bbbe500..1e0bfeec3 100644 --- a/app/notify_client/api_key_api_client.py +++ b/app/notify_client/api_key_api_client.py @@ -7,8 +7,6 @@ KEY_TYPE_TEST = 'test' class ApiKeyApiClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") def get_api_keys(self, service_id): return self.get(url='/service/{}/api-keys'.format(service_id)) diff --git a/app/notify_client/billing_api_client.py b/app/notify_client/billing_api_client.py index ec6c69551..c3a748f2f 100644 --- a/app/notify_client/billing_api_client.py +++ b/app/notify_client/billing_api_client.py @@ -2,10 +2,6 @@ from app.notify_client import NotifyAdminAPIClient class BillingAPIClient(NotifyAdminAPIClient): - # Fudge assert in the super __init__ so - # we can set those variables later. - def __init__(self): - super().__init__("a" * 73, "b") def get_billable_units_ft(self, service_id, year): return self.get( diff --git a/app/notify_client/complaint_api_client.py b/app/notify_client/complaint_api_client.py index 556620444..d9eaf7bd3 100644 --- a/app/notify_client/complaint_api_client.py +++ b/app/notify_client/complaint_api_client.py @@ -2,10 +2,6 @@ from app.notify_client import NotifyAdminAPIClient class ComplaintApiClient(NotifyAdminAPIClient): - # Fudge assert in the super __init__ so - # we can set those variables later. - def __init__(self): - super().__init__("a" * 73, "b") def get_all_complaints(self, page=1): params = {'page': page} diff --git a/app/notify_client/email_branding_client.py b/app/notify_client/email_branding_client.py index 12b40bfd1..776c0d1e6 100644 --- a/app/notify_client/email_branding_client.py +++ b/app/notify_client/email_branding_client.py @@ -3,9 +3,6 @@ from app.notify_client import NotifyAdminAPIClient, cache class EmailBrandingClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") - @cache.set('email_branding-{branding_id}') def get_email_branding(self, branding_id): return self.get(url='/email-branding/{}'.format(branding_id)) diff --git a/app/notify_client/events_api_client.py b/app/notify_client/events_api_client.py index 36cd9729d..b6400c0c9 100644 --- a/app/notify_client/events_api_client.py +++ b/app/notify_client/events_api_client.py @@ -2,8 +2,6 @@ from app.notify_client import NotifyAdminAPIClient class EventsApiClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") def create_event(self, event_type, event_data): data = { diff --git a/app/notify_client/inbound_number_client.py b/app/notify_client/inbound_number_client.py index 5a912f1a6..6b3607f97 100644 --- a/app/notify_client/inbound_number_client.py +++ b/app/notify_client/inbound_number_client.py @@ -3,9 +3,6 @@ from app.notify_client import NotifyAdminAPIClient class InboundNumberClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") - def get_available_inbound_sms_numbers(self): return self.get(url='/inbound-number/available') diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index b16f8841c..ee371003b 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -6,8 +6,6 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache class InviteApiClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") def init_app(self, app): super().init_app(app) diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index c40feebce..c2a882e85 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -18,9 +18,6 @@ class JobApiClient(NotifyAdminAPIClient): NON_SCHEDULED_JOB_STATUSES = JOB_STATUSES - {'scheduled', 'cancelled'} - def __init__(self): - super().__init__("a" * 73, "b") - @staticmethod def __convert_statistics(job): results = defaultdict(int) diff --git a/app/notify_client/letter_branding_client.py b/app/notify_client/letter_branding_client.py index b41bb5dd4..96a0889dc 100644 --- a/app/notify_client/letter_branding_client.py +++ b/app/notify_client/letter_branding_client.py @@ -3,9 +3,6 @@ from app.notify_client import NotifyAdminAPIClient class LetterBrandingClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") - def get_letter_branding(self): return self.get(url='/dvla_organisations') diff --git a/app/notify_client/letter_jobs_client.py b/app/notify_client/letter_jobs_client.py index da6a070e1..6314642ee 100644 --- a/app/notify_client/letter_jobs_client.py +++ b/app/notify_client/letter_jobs_client.py @@ -3,9 +3,6 @@ from app.notify_client import NotifyAdminAPIClient class LetterJobsClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") - def submit_returned_letters(self, references): return self.post( url='/letters/returned', diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index ecd5effcd..a31753542 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -2,8 +2,6 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user class NotificationApiClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") def get_notifications_for_service( self, diff --git a/app/notify_client/org_invite_api_client.py b/app/notify_client/org_invite_api_client.py index f80b20ca3..f65497521 100644 --- a/app/notify_client/org_invite_api_client.py +++ b/app/notify_client/org_invite_api_client.py @@ -3,8 +3,6 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user class OrgInviteApiClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") def init_app(self, app): super().init_app(app) diff --git a/app/notify_client/organisations_api_client.py b/app/notify_client/organisations_api_client.py index f5b31a835..60025856d 100644 --- a/app/notify_client/organisations_api_client.py +++ b/app/notify_client/organisations_api_client.py @@ -3,9 +3,6 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache class OrganisationsClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") - def get_organisations(self): return self.get(url='/organisations') diff --git a/app/notify_client/platform_stats_api_client.py b/app/notify_client/platform_stats_api_client.py index d2cc15f30..a91beba14 100644 --- a/app/notify_client/platform_stats_api_client.py +++ b/app/notify_client/platform_stats_api_client.py @@ -2,10 +2,6 @@ from app.notify_client import NotifyAdminAPIClient class PlatformStatsAPIClient(NotifyAdminAPIClient): - # Fudge assert in the super __init__ so - # we can set those variables later. - def __init__(self): - super().__init__("a" * 73, "b") def get_aggregate_platform_stats(self, params_dict=None): return self.get("/platform-stats", params=params_dict) diff --git a/app/notify_client/provider_client.py b/app/notify_client/provider_client.py index 8289eb5f0..e17ea76be 100644 --- a/app/notify_client/provider_client.py +++ b/app/notify_client/provider_client.py @@ -3,8 +3,6 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user class ProviderClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") def get_all_providers(self): return self.get( diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 78f19896c..9f54b0189 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -3,10 +3,6 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache class ServiceAPIClient(NotifyAdminAPIClient): - # Fudge assert in the super __init__ so - # we can set those variables later. - def __init__(self): - super().__init__("a" * 73, "b") @cache.delete('user-{user_id}') def create_service( diff --git a/app/notify_client/status_api_client.py b/app/notify_client/status_api_client.py index 650741c67..3925a77bd 100644 --- a/app/notify_client/status_api_client.py +++ b/app/notify_client/status_api_client.py @@ -3,8 +3,6 @@ from app.notify_client import NotifyAdminAPIClient class StatusApiClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") def get_status(self, *params): return self.get(url='/_status', *params) diff --git a/app/notify_client/template_folder_api_client.py b/app/notify_client/template_folder_api_client.py index 93677dfc8..23044259a 100644 --- a/app/notify_client/template_folder_api_client.py +++ b/app/notify_client/template_folder_api_client.py @@ -2,10 +2,6 @@ from app.notify_client import NotifyAdminAPIClient, cache class TemplateFolderAPIClient(NotifyAdminAPIClient): - # Fudge assert in the super __init__ so - # we can set those variables later. - def __init__(self): - super().__init__('a' * 73, 'b') @cache.delete('service-{service_id}-template-folders') def create_template_folder( diff --git a/app/notify_client/template_statistics_api_client.py b/app/notify_client/template_statistics_api_client.py index f9f3f3183..8ab5e09a6 100644 --- a/app/notify_client/template_statistics_api_client.py +++ b/app/notify_client/template_statistics_api_client.py @@ -2,8 +2,6 @@ from app.notify_client import NotifyAdminAPIClient class TemplateStatisticsApiClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") def get_template_statistics_for_service(self, service_id, limit_days=None): params = {} diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index fad3368dd..eb3d7afe2 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -18,8 +18,6 @@ ALLOWED_ATTRIBUTES = { class UserApiClient(NotifyAdminAPIClient): - def __init__(self): - super().__init__("a" * 73, "b") def init_app(self, app): super().init_app(app) diff --git a/tests/app/notify_client/test_notify_admin_api_client.py b/tests/app/notify_client/test_notify_admin_api_client.py index 2e542f87e..bcb292e90 100644 --- a/tests/app/notify_client/test_notify_admin_api_client.py +++ b/tests/app/notify_client/test_notify_admin_api_client.py @@ -9,8 +9,6 @@ from app.notify_client import NotifyAdminAPIClient from tests import service_json from tests.conftest import api_user_active, platform_admin_user, set_config -SAMPLE_API_KEY = '{}-{}'.format('a' * 36, 's' * 36) - @pytest.mark.parametrize('method', [ 'put', @@ -26,7 +24,7 @@ SAMPLE_API_KEY = '{}-{}'.format('a' * 36, 's' * 36) None ], ids=['active_service', 'no_service']) def test_active_service_can_be_modified(app_, method, user, service): - api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') + api_client = NotifyAdminAPIClient() with app_.test_request_context() as request_context, app_.test_client() as client: client.login(user) @@ -45,7 +43,7 @@ def test_active_service_can_be_modified(app_, method, user, service): 'delete' ]) def test_inactive_service_cannot_be_modified_by_normal_user(app_, api_user_active, method): - api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') + api_client = NotifyAdminAPIClient() with app_.test_request_context() as request_context, app_.test_client() as client: client.login(api_user_active) @@ -64,7 +62,7 @@ def test_inactive_service_cannot_be_modified_by_normal_user(app_, api_user_activ 'delete' ]) def test_inactive_service_can_be_modified_by_platform_admin(app_, platform_admin_user, method): - api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') + api_client = NotifyAdminAPIClient() with app_.test_request_context() as request_context, app_.test_client() as client: client.login(platform_admin_user) @@ -78,7 +76,7 @@ def test_inactive_service_can_be_modified_by_platform_admin(app_, platform_admin def test_generate_headers_sets_standard_headers(app_): - api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') + api_client = NotifyAdminAPIClient() with set_config(app_, 'ROUTE_SECRET_KEY_1', 'proxy-secret'): api_client.init_app(app_) @@ -93,7 +91,7 @@ def test_generate_headers_sets_standard_headers(app_): def test_generate_headers_sets_request_id_if_in_request_context(app_): - api_client = NotifyAdminAPIClient(SAMPLE_API_KEY, 'base_url') + api_client = NotifyAdminAPIClient() api_client.init_app(app_) with app_.test_request_context() as request_context: From e211fb7f609ba72df7be674ae4b4c9e59ffb2f53 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 29 Jan 2019 11:18:08 +0000 Subject: [PATCH 2/3] Remove duplicative calls to init_app Easier to read without the repetitive boilerplate. --- app/__init__.py | 61 ++++++++++++++++++++++++++----------------------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 5b8cae5bc..0ace264af 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -107,35 +107,40 @@ def create_app(application): asset_fingerprinter._asset_root = application.config['ASSET_PATH'] init_app(application) - antivirus_client.init_app(application) - statsd_client.init_app(application) - zendesk_client.init_app(application) + + for client in ( + antivirus_client, + statsd_client, + zendesk_client, + csrf, + request_helper, + service_api_client, + user_api_client, + api_key_api_client, + job_api_client, + notification_api_client, + status_api_client, + invite_api_client, + org_invite_api_client, + template_statistics_client, + events_api_client, + provider_client, + email_branding_client, + letter_branding_client, + organisations_client, + letter_jobs_client, + inbound_number_client, + billing_api_client, + complaint_api_client, + platform_stats_api_client, + template_folder_api_client, + login_manager, + proxy_fix, + ): + client.init_app(application) + logging.init_app(application, statsd_client) - csrf.init_app(application) - request_helper.init_app(application) - service_api_client.init_app(application) - user_api_client.init_app(application) - api_key_api_client.init_app(application) - job_api_client.init_app(application) - notification_api_client.init_app(application) - status_api_client.init_app(application) - invite_api_client.init_app(application) - org_invite_api_client.init_app(application) - template_statistics_client.init_app(application) - events_api_client.init_app(application) - provider_client.init_app(application) - email_branding_client.init_app(application) - letter_branding_client.init_app(application) - organisations_client.init_app(application) - letter_jobs_client.init_app(application) - inbound_number_client.init_app(application) - billing_api_client.init_app(application) - complaint_api_client.init_app(application) - platform_stats_api_client.init_app(application) - template_folder_api_client.init_app(application) - - login_manager.init_app(application) login_manager.login_view = 'main.sign_in' login_manager.login_message_category = 'default' login_manager.session_protection = None @@ -147,8 +152,6 @@ def create_app(application): from .status import status as status_blueprint application.register_blueprint(status_blueprint) - proxy_fix.init_app(application) - add_template_filters(application) register_errorhandlers(application) From 17c6446b855b230115678385faaa2592898fe694 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 30 Jan 2019 13:45:05 +0000 Subject: [PATCH 3/3] Organise client initialisation - groups them into sensible chunks - alphabetises them --- app/__init__.py | 55 ++++++++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 0ace264af..8f45d5b1d 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -109,33 +109,40 @@ def create_app(application): init_app(application) for client in ( - antivirus_client, - statsd_client, - zendesk_client, + + # Gubbins csrf, - request_helper, - service_api_client, - user_api_client, - api_key_api_client, - job_api_client, - notification_api_client, - status_api_client, - invite_api_client, - org_invite_api_client, - template_statistics_client, - events_api_client, - provider_client, - email_branding_client, - letter_branding_client, - organisations_client, - letter_jobs_client, - inbound_number_client, - billing_api_client, - complaint_api_client, - platform_stats_api_client, - template_folder_api_client, login_manager, proxy_fix, + request_helper, + + # Internal API clients + antivirus_client, + api_key_api_client, + billing_api_client, + complaint_api_client, + email_branding_client, + events_api_client, + inbound_number_client, + invite_api_client, + job_api_client, + letter_branding_client, + letter_jobs_client, + notification_api_client, + org_invite_api_client, + organisations_client, + platform_stats_api_client, + provider_client, + service_api_client, + status_api_client, + template_folder_api_client, + template_statistics_client, + user_api_client, + + # External API clients + statsd_client, + zendesk_client, + ): client.init_app(application)