From a69551b2cbb903ba78fdd88933d461b4c38c2853 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 5 Nov 2018 16:03:11 +0000 Subject: [PATCH 1/5] Use model to update service properties MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have a lot of places in settings where we update something by passing in the `service_id`. `current_service` already knows about `service_id`, so it’s cleaner to encapsulate these updates inside the model. --- app/main/views/service_settings.py | 36 +++++++++++------------------- app/models/service.py | 6 +++++ 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 25e2f29c3..88cca3cd7 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -108,8 +108,7 @@ def service_name_change_confirm(service_id): if form.validate_on_submit(): try: - service_api_client.update_service( - current_service.id, + current_service.update( name=session['service_name_change'], email_from=email_safe(session['service_name_change']) ) @@ -209,8 +208,7 @@ def submit_request_to_go_live(service_id): @login_required @user_is_platform_admin def service_switch_live(service_id): - service_api_client.update_service( - current_service.id, + current_service.update( # TODO This limit should be set depending on the agreement signed by # with Notify. message_limit=250000 if current_service.trial_mode else 50, @@ -223,9 +221,8 @@ def service_switch_live(service_id): @login_required @user_is_platform_admin def service_switch_research_mode(service_id): - service_api_client.update_service_with_properties( - service_id, - {"research_mode": not current_service.research_mode} + current_service.update_with_properties( + {'research_mode': not current_service.research_mode} ) return redirect(url_for('.service_settings', service_id=service_id)) @@ -258,7 +255,7 @@ def update_service_permissions(service_id, permissions, sms_sender=None): if sms_sender: data['sms_sender'] = sms_sender - service_api_client.update_service_with_properties(service_id, data) + current_service.update_with_properties(data) @main.route("/services//service-settings/can-send-email") @@ -308,8 +305,7 @@ def service_switch_can_upload_document(service_id): if form.validate_on_submit(): contact_type = form.contact_details_type.data - service_api_client.update_service( - current_service.id, + current_service.update( contact_link=form.data[contact_type] ) switch_service_permissions(service_id, 'upload_document') @@ -380,8 +376,7 @@ def service_set_contact_link(service_id): if form.validate_on_submit(): contact_type = form.contact_details_type.data - service_api_client.update_service( - current_service.id, + current_service.update( contact_link=form.data[contact_type] ) return redirect(url_for('.service_settings', service_id=current_service.id)) @@ -525,8 +520,7 @@ def service_set_sms_prefix(service_id): form.enabled.label.text = 'Start all text messages with ‘{}:’'.format(current_service.name) if form.validate_on_submit(): - service_api_client.update_service( - current_service.id, + current_service.update( prefix_sms=(form.enabled.data == 'on') ) return redirect(url_for('.service_settings', service_id=service_id)) @@ -597,7 +591,7 @@ def service_set_postage(service_id): form = ServicePostageForm(postage=current_service.postage) if form.validate_on_submit(): - service_api_client.update_service(service_id, postage=form.postage.data) + current_service.update(postage=form.postage.data) return redirect(url_for(".service_settings", service_id=service_id)) return render_template('views/service-settings/set-postage.html', form=form) @@ -762,8 +756,7 @@ def service_set_letter_contact_block(service_id): form = ServiceLetterContactBlockForm(letter_contact_block=current_service.letter_contact_block) if form.validate_on_submit(): - service_api_client.update_service( - current_service.id, + current_service.update( letter_contact_block=form.letter_contact_block.data.replace('\r', '') or None ) if request.args.get('from_template'): @@ -788,8 +781,7 @@ def set_organisation_type(service_id): free_sms_fragment_limit = current_app.config['DEFAULT_FREE_SMS_FRAGMENT_LIMITS'].get( form.organisation_type.data) - service_api_client.update_service( - service_id, + current_service.update( organisation_type=form.organisation_type.data, ) billing_api_client.create_or_update_free_sms_fragment_limit(service_id, free_sms_fragment_limit) @@ -857,8 +849,7 @@ def service_preview_email_branding(service_id): if form.validate_on_submit(): branding_style = None if form.branding_style.data == 'None' else form.branding_style.data - service_api_client.update_service( - service_id, + current_service.update( email_branding=branding_style ) return redirect(url_for('.service_settings', service_id=service_id)) @@ -879,8 +870,7 @@ def set_letter_branding(service_id): form = LetterBranding(choices=email_branding_client.get_letter_email_branding().items()) if form.validate_on_submit(): - service_api_client.update_service( - service_id, + current_service.update( dvla_organisation=form.dvla_org_id.data ) return redirect(url_for('.service_settings', service_id=service_id)) diff --git a/app/models/service.py b/app/models/service.py index c663962d3..25ca62778 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -47,6 +47,12 @@ class Service(): return self._dict[attr] raise AttributeError('`{}` is not a service attribute'.format(attr)) + def update(self, **kwargs): + return service_api_client.update_service(self.id, **kwargs) + + def update_with_properties(self, properties): + return service_api_client.update_service_with_properties(self.id, properties) + @property def trial_mode(self): return self._dict['restricted'] From 318f846630b93dce8aaed01961b0c88fe7113f97 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 5 Nov 2018 16:53:03 +0000 Subject: [PATCH 2/5] Move service permission handle inside the model These helper functions for modifying a service permission were just floating around loose in the view code. A much better home for them is on the model. This will also make it easy to reuse them in other views if we ever need to. --- app/main/views/service_settings.py | 53 ++++--------------- app/models/service.py | 25 +++++++++ tests/app/main/views/test_service_settings.py | 7 ++- 3 files changed, 40 insertions(+), 45 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 88cca3cd7..c65df9920 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -227,42 +227,11 @@ def service_switch_research_mode(service_id): return redirect(url_for('.service_settings', service_id=service_id)) -def switch_service_permissions(service_id, permission, sms_sender=None): - - force_service_permission( - service_id, - permission, - on=permission not in current_service.permissions, - sms_sender=sms_sender - ) - - -def force_service_permission(service_id, permission, on=False, sms_sender=None): - - permissions, permission = set(current_service.permissions), {permission} - - update_service_permissions( - service_id, - permissions | permission if on else permissions - permission, - sms_sender=sms_sender - ) - - -def update_service_permissions(service_id, permissions, sms_sender=None): - - data = {'permissions': list(permissions)} - - if sms_sender: - data['sms_sender'] = sms_sender - - current_service.update_with_properties(data) - - @main.route("/services//service-settings/can-send-email") @login_required @user_is_platform_admin def service_switch_can_send_email(service_id): - switch_service_permissions(service_id, 'email') + current_service.switch_permission('email') return redirect(url_for('.service_settings', service_id=service_id)) @@ -270,7 +239,7 @@ def service_switch_can_send_email(service_id): @login_required @user_is_platform_admin def service_switch_can_send_sms(service_id): - switch_service_permissions(service_id, 'sms') + current_service.switch_permission('sms') return redirect(url_for('.service_settings', service_id=service_id)) @@ -278,7 +247,7 @@ def service_switch_can_send_sms(service_id): @login_required @user_is_platform_admin def service_switch_email_auth(service_id): - switch_service_permissions(service_id, 'email_auth') + current_service.switch_permission('email_auth') return redirect(url_for('.service_settings', service_id=service_id)) @@ -286,7 +255,7 @@ def service_switch_email_auth(service_id): @login_required @user_is_platform_admin def service_switch_can_send_precompiled_letter(service_id): - switch_service_permissions(service_id, 'precompiled_letter') + current_service.switch_permission('precompiled_letter') return redirect(url_for('.service_settings', service_id=service_id)) @@ -299,7 +268,7 @@ def service_switch_can_upload_document(service_id): # If turning the permission off, or turning it on and the service already has a contact_link, # don't show the form to add the link if current_service.has_permission('upload_document') or current_service.contact_link: - switch_service_permissions(service_id, 'upload_document') + current_service.switch_permission('upload_document') return redirect(url_for('.service_settings', service_id=service_id)) if form.validate_on_submit(): @@ -308,7 +277,7 @@ def service_switch_can_upload_document(service_id): current_service.update( contact_link=form.data[contact_type] ) - switch_service_permissions(service_id, 'upload_document') + current_service.switch_permission('upload_document') return redirect(url_for('.service_settings', service_id=service_id)) return render_template('views/service-settings/contact_link.html', form=form) @@ -318,7 +287,7 @@ def service_switch_can_upload_document(service_id): @login_required @user_is_platform_admin def service_switch_can_edit_folders(service_id): - switch_service_permissions(service_id, 'edit_folders') + current_service.switch_permission('edit_folders') return redirect(url_for('.service_settings', service_id=service_id)) @@ -490,7 +459,7 @@ def service_set_inbound_number(service_id): is_default=True, inbound_number_id=form.inbound_number.data ) - switch_service_permissions(current_service.id, 'inbound_sms') + current_service.force_permission('inbound_sms', on=True) return redirect(url_for('.service_settings', service_id=service_id)) return render_template( 'views/service-settings/set-inbound-number.html', @@ -539,8 +508,7 @@ def service_set_international_sms(service_id): enabled='on' if current_service.has_permission('international_sms') else 'off' ) if form.validate_on_submit(): - force_service_permission( - service_id, + current_service.force_permission( 'international_sms', on=(form.enabled.data == 'on'), ) @@ -570,8 +538,7 @@ def service_set_letters(service_id): enabled='on' if current_service.has_permission('letter') else 'off' ) if form.validate_on_submit(): - force_service_permission( - service_id, + current_service.force_permission( 'letter', on=(form.enabled.data == 'on'), ) diff --git a/app/models/service.py b/app/models/service.py index 25ca62778..4893d9275 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -53,6 +53,31 @@ class Service(): def update_with_properties(self, properties): return service_api_client.update_service_with_properties(self.id, properties) + def switch_permission(self, permission, sms_sender=None): + return self.force_permission( + permission, + on=not self.has_permission(permission), + sms_sender=sms_sender + ) + + def force_permission(self, permission, on=False, sms_sender=None): + + permissions, permission = set(self.permissions), {permission} + + return self.update_permissions( + permissions | permission if on else permissions - permission, + sms_sender=sms_sender + ) + + def update_permissions(self, permissions, sms_sender=None): + + data = {'permissions': list(permissions)} + + if sms_sender: + data['sms_sender'] = sms_sender + + self.update_with_properties(data) + @property def trial_mode(self): return self._dict['restricted'] diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 27c038082..7a33f4d6d 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1170,7 +1170,10 @@ def test_enabling_and_disabling_email_and_sms( mock_get_inbound_number_for_service ): service_one['permissions'] = permissions_before_switch - mocked_fn = mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one) + mocked_fn = mocker.patch( + 'app.notify_client.service_api_client.service_api_client.update_service', + return_value=service_one, + ) response = logged_in_platform_admin_client.get( url_for('main.service_switch_can_send_{}'.format(notification_type), service_id=service_one['id']) @@ -1178,7 +1181,7 @@ def test_enabling_and_disabling_email_and_sms( assert response.status_code == 302 assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) - assert mocked_fn.call_args == call(service_one['id'], {'permissions': permissions_after_switch}) + assert mocked_fn.call_args == call(service_one['id'], permissions=permissions_after_switch) def test_and_more_hint_appears_on_settings_with_more_than_just_a_single_sender( From 48b0d4194e58965164e4bd8e95f6a0928442dfdd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 5 Nov 2018 16:17:44 +0000 Subject: [PATCH 3/5] Use model to toggle research mode Just a nice bit of encapsulation, rather than passing `current_service` through to a method on `current_service`. --- app/main/views/service_settings.py | 4 +--- app/models/service.py | 5 +++++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index c65df9920..09909769b 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -221,9 +221,7 @@ def service_switch_live(service_id): @login_required @user_is_platform_admin def service_switch_research_mode(service_id): - current_service.update_with_properties( - {'research_mode': not current_service.research_mode} - ) + current_service.toggle_research_mode() return redirect(url_for('.service_settings', service_id=service_id)) diff --git a/app/models/service.py b/app/models/service.py index 4893d9275..b34dc95b0 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -78,6 +78,11 @@ class Service(): self.update_with_properties(data) + def toggle_research_mode(self): + self.update_with_properties({ + 'research_mode': not self.research_mode, + }) + @property def trial_mode(self): return self._dict['restricted'] From b3a7210bafec949b23047659d0aab150555781f4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 5 Nov 2018 17:03:37 +0000 Subject: [PATCH 4/5] Stop passing SMS sender when updating permissions This is a redundant argument which no longer seems to be used by anything. --- app/models/service.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index b34dc95b0..2931b0de8 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -53,30 +53,22 @@ class Service(): def update_with_properties(self, properties): return service_api_client.update_service_with_properties(self.id, properties) - def switch_permission(self, permission, sms_sender=None): + def switch_permission(self, permission): return self.force_permission( permission, on=not self.has_permission(permission), - sms_sender=sms_sender ) - def force_permission(self, permission, on=False, sms_sender=None): + def force_permission(self, permission, on=False): permissions, permission = set(self.permissions), {permission} return self.update_permissions( permissions | permission if on else permissions - permission, - sms_sender=sms_sender ) - def update_permissions(self, permissions, sms_sender=None): - - data = {'permissions': list(permissions)} - - if sms_sender: - data['sms_sender'] = sms_sender - - self.update_with_properties(data) + def update_permissions(self, permissions): + return self.update_with_properties({'permissions': list(permissions)}) def toggle_research_mode(self): self.update_with_properties({ From 1c9e42f85d133496d7943387a6daaf17e6ea967e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 5 Nov 2018 17:37:50 +0000 Subject: [PATCH 5/5] Remove update_with_properties method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s more Pythonic, and better abstracted from the underlying API to use keyword arguments instead. --- app/models/service.py | 9 ++------- .../test_inbound_sms_setting.py | 2 +- tests/app/main/views/test_service_settings.py | 17 ++++++++--------- 3 files changed, 11 insertions(+), 17 deletions(-) diff --git a/app/models/service.py b/app/models/service.py index 2931b0de8..6e3bb9794 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -50,9 +50,6 @@ class Service(): def update(self, **kwargs): return service_api_client.update_service(self.id, **kwargs) - def update_with_properties(self, properties): - return service_api_client.update_service_with_properties(self.id, properties) - def switch_permission(self, permission): return self.force_permission( permission, @@ -68,12 +65,10 @@ class Service(): ) def update_permissions(self, permissions): - return self.update_with_properties({'permissions': list(permissions)}) + return self.update(permissions=list(permissions)) def toggle_research_mode(self): - self.update_with_properties({ - 'research_mode': not self.research_mode, - }) + self.update(research_mode=not self.research_mode) @property def trial_mode(self): diff --git a/tests/app/main/views/service_settings/test_inbound_sms_setting.py b/tests/app/main/views/service_settings/test_inbound_sms_setting.py index dfbb7abfd..8114c4faf 100644 --- a/tests/app/main/views/service_settings/test_inbound_sms_setting.py +++ b/tests/app/main/views/service_settings/test_inbound_sms_setting.py @@ -12,7 +12,7 @@ def test_set_inbound_sms_sets_a_number_for_service( mock_no_inbound_number_for_service, mocker ): - mocker.patch('app.service_api_client.update_service_with_properties') + mocker.patch('app.service_api_client.update_service') data = { "inbound_number": "781d9c60-7a7e-46b7-9896-7b045b992fa5", } diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 7a33f4d6d..cb45e6ef0 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1933,7 +1933,7 @@ def test_switch_service_from_research_mode_to_normal( research_mode=True ) mocker.patch('app.service_api_client.get_service', return_value={"data": service}) - update_service_mock = mocker.patch('app.service_api_client.update_service_with_properties', return_value=service) + update_service_mock = mocker.patch('app.service_api_client.update_service', return_value=service) response = logged_in_platform_admin_client.get( url_for('main.service_switch_research_mode', service_id=service['id']) @@ -1941,7 +1941,7 @@ def test_switch_service_from_research_mode_to_normal( assert response.status_code == 302 assert response.location == url_for('main.service_settings', service_id=service['id'], _external=True) update_service_mock.assert_called_with( - service['id'], {"research_mode": False} + service['id'], research_mode=False ) @@ -1956,7 +1956,7 @@ def test_shows_research_mode_indicator( mock_get_service_settings_page_common, ): service_one['research_mode'] = True - mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one) + mocker.patch('app.service_api_client.update_service', return_value=service_one) response = logged_in_client.get(url_for('main.service_settings', service_id=service_one['id'])) assert response.status_code == 200 @@ -2444,7 +2444,7 @@ def test_switch_service_enable_letters( initial_permissions, expected_updated_permissions, ): - mocked_fn = mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one) + mocked_fn = mocker.patch('app.service_api_client.update_service', return_value=service_one) service_one['permissions'] = initial_permissions page = client_request.get( @@ -2465,8 +2465,7 @@ def test_switch_service_enable_letters( _external=True ) ) - - assert set(mocked_fn.call_args[0][1]['permissions']) == set(expected_updated_permissions) + assert set(mocked_fn.call_args[1]['permissions']) == set(expected_updated_permissions) assert mocked_fn.call_args[0][0] == service_one['id'] @@ -2505,7 +2504,7 @@ def test_switch_service_enable_international_sms( post_value, international_sms_permission_expected_in_api_call, ): - mocked_fn = mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one) + mocked_fn = mocker.patch('app.service_api_client.update_service', return_value=service_one) client_request.post( 'main.service_set_international_sms', service_id=service_one['id'], @@ -2516,9 +2515,9 @@ def test_switch_service_enable_international_sms( ) if international_sms_permission_expected_in_api_call: - assert 'international_sms' in mocked_fn.call_args[0][1]['permissions'] + assert 'international_sms' in mocked_fn.call_args[1]['permissions'] else: - assert 'international_sms' not in mocked_fn.call_args[0][1]['permissions'] + assert 'international_sms' not in mocked_fn.call_args[1]['permissions'] assert mocked_fn.call_args[0][0] == service_one['id']