From 78e57dbff9d0cda410c43863c0e8994a2e6425ef Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 3 Oct 2019 11:57:24 +0100 Subject: [PATCH] Clear service cache for when updating org branding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Updating an organisation’s branding might now also update the branding of services associated to that organisation. This is similar to how updating an organisation’s type can update the organisation type for its services. In the latter case we already make sure to clear the cached version of these services which is held in Redis. This commit does the same clearing of the caches when updating an organisation’s branding (and does a bit of refactoring to do so without duplication of code.) --- app/main/views/organisations.py | 19 ++++++++----------- app/models/organisation.py | 12 ++++++++++-- app/notify_client/organisations_api_client.py | 2 +- tests/app/main/views/test_agreement.py | 4 ++++ tests/app/main/views/test_service_settings.py | 14 +++++++++++++- 5 files changed, 36 insertions(+), 15 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 3a7b66f10..587dbccf8 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -258,12 +258,9 @@ def edit_organisation_type(org_id): ) if form.validate_on_submit(): - org_service_ids = [service['id'] for service in current_organisation.services] - - organisations_client.update_organisation( - current_organisation.id, - cached_service_ids=org_service_ids, + current_organisation.update( organisation_type=form.organisation_type.data, + delete_services_cache=True, ) return redirect(url_for('.organisation_settings', org_id=org_id)) @@ -365,9 +362,9 @@ def organisation_preview_email_branding(org_id): form = PreviewBranding(branding_style=branding_style) if form.validate_on_submit(): - organisations_client.update_organisation( - org_id, - email_branding_id=form.branding_style.data + current_organisation.update( + email_branding_id=form.branding_style.data, + delete_services_cache=True, ) return redirect(url_for('.organisation_settings', org_id=org_id)) @@ -410,9 +407,9 @@ def organisation_preview_letter_branding(org_id): form = PreviewBranding(branding_style=branding_style) if form.validate_on_submit(): - organisations_client.update_organisation( - org_id, - letter_branding_id=form.branding_style.data + current_organisation.update( + letter_branding_id=form.branding_style.data, + delete_services_cache=True, ) return redirect(url_for('.organisation_settings', org_id=org_id)) diff --git a/app/models/organisation.py b/app/models/organisation.py index 50ccfe059..27a0fe77b 100644 --- a/app/models/organisation.py +++ b/app/models/organisation.py @@ -135,6 +135,10 @@ class Organisation(JSONModel): def services(self): return organisations_client.get_organisation_services(self.id) + @cached_property + def service_ids(self): + return [s['id'] for s in self.services] + @property def live_services(self): return [s for s in self.services if s['active'] and not s['restricted']] @@ -180,8 +184,12 @@ class Organisation(JSONModel): self.letter_branding_id ) - def update(self, **kwargs): - response = organisations_client.update_organisation(self.id, **kwargs) + def update(self, delete_services_cache=False, **kwargs): + response = organisations_client.update_organisation( + self.id, + cached_service_ids=self.service_ids if delete_services_cache else None, + **kwargs + ) self.__init__(response) def associate_service(self, service_id): diff --git a/app/notify_client/organisations_api_client.py b/app/notify_client/organisations_api_client.py index b984c21fb..a62949350 100644 --- a/app/notify_client/organisations_api_client.py +++ b/app/notify_client/organisations_api_client.py @@ -49,7 +49,7 @@ class OrganisationsClient(NotifyAdminAPIClient): def update_organisation(self, org_id, cached_service_ids=None, **kwargs): api_response = self.post(url="/organisations/{}".format(org_id), data=kwargs) - if kwargs.get('organisation_type') and cached_service_ids: + if cached_service_ids: redis_client.delete(*map('service-{}'.format, cached_service_ids)) return api_response diff --git a/tests/app/main/views/test_agreement.py b/tests/app/main/views/test_agreement.py index 42fe31862..43fd713ba 100644 --- a/tests/app/main/views/test_agreement.py +++ b/tests/app/main/views/test_agreement.py @@ -361,6 +361,7 @@ def test_accept_agreement_page_validates( agreement_signed_version=1.2, agreement_signed_on_behalf_of_name='Firstname Lastname', agreement_signed_on_behalf_of_email_address='test@example.com', + cached_service_ids=None, ) ), ( @@ -375,6 +376,7 @@ def test_accept_agreement_page_validates( agreement_signed_version=1.2, agreement_signed_on_behalf_of_name='', agreement_signed_on_behalf_of_email_address='', + cached_service_ids=None, ) ), ( @@ -389,6 +391,7 @@ def test_accept_agreement_page_validates( agreement_signed_version=1.2, agreement_signed_on_behalf_of_name='', agreement_signed_on_behalf_of_email_address='', + cached_service_ids=None, ) ), )) @@ -483,6 +486,7 @@ def test_confirm_agreement_page_persists( agreement_signed=True, agreement_signed_at='2012-01-01 01:01:00', agreement_signed_by_id=fake_uuid, + cached_service_ids=None, ) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 755b81d9b..07fa3def5 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -3196,6 +3196,7 @@ def test_service_preview_letter_branding_saves( client_request, platform_admin_user, mock_get_organisation, + mock_get_organisation_services, mock_update_service, mock_update_organisation, mock_get_all_letter_branding, @@ -3229,6 +3230,11 @@ def test_service_preview_letter_branding_saves( mock_update_organisation.assert_called_once_with( ORGANISATION_ID, letter_branding_id=expected_post_data, + cached_service_ids=[ + '12345', + '67890', + '596364a0-858e-42c8-9062-a8fe822260eb', + ], ) assert mock_update_service.called is False @@ -3407,6 +3413,7 @@ def test_should_set_branding_and_organisations( platform_admin_user, service_one, mock_get_organisation, + mock_get_organisation_services, mock_update_service, mock_update_organisation, posted_value, @@ -3439,7 +3446,12 @@ def test_should_set_branding_and_organisations( elif endpoint == 'main.organisation_preview_email_branding': mock_update_organisation.assert_called_once_with( ORGANISATION_ID, - email_branding_id=submitted_value + email_branding_id=submitted_value, + cached_service_ids=[ + '12345', + '67890', + '596364a0-858e-42c8-9062-a8fe822260eb', + ], ) assert mock_update_service.called is False else: