diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 25a2e4a42..4e26adb1b 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -66,17 +66,12 @@ def service_name_change_confirm(service_id): form = ConfirmPasswordForm(_check_password) if form.validate_on_submit(): - current_service['name'] = session['service_name_change'] - current_service['email_from'] = email_safe(session['service_name_change']) try: service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], - current_service['message_limit'], - current_service['restricted'], - current_service['users'], - current_service['email_from']) + name=session['service_name_change'], + email_from=email_safe(session['service_name_change']) + ) except HTTPError as e: error_msg = "Duplicate service name '{}'".format(session['service_name_change']) if e.status_code == 400 and error_msg in e.message['name']: @@ -143,14 +138,11 @@ def service_request_to_go_live(service_id): def service_switch_live(service_id): service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], # TODO This limit should be set depending on the agreement signed by # with Notify. - 250000 if current_service['restricted'] else 50, - False if current_service['restricted'] else True, - current_service['users'], - current_service['email_from']) + message_limit=250000 if current_service['restricted'] else 50, + restricted=(not current_service['restricted']) + ) return redirect(url_for('.service_settings', service_id=service_id)) @@ -188,15 +180,10 @@ def service_status_change_confirm(service_id): form = ConfirmPasswordForm(_check_password) if form.validate_on_submit(): - current_service['active'] = True service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], - current_service['message_limit'], - current_service['restricted'], - current_service['users'], - current_service['email_from']) + active=True + ) return redirect(url_for('.service_settings', service_id=service_id)) return render_template( 'views/service-settings/confirm.html', @@ -249,13 +236,8 @@ def service_set_reply_to_email(service_id): message = 'Reply to email set to {}'.format(form.email_address.data) service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], - current_service['message_limit'], - current_service['restricted'], - current_service['users'], - current_service['email_from'], - reply_to_email_address=form.email_address.data) + reply_to_email_address=form.email_address.data + ) flash(message, 'default_with_tick') return redirect(url_for('.service_settings', service_id=service_id)) return render_template( @@ -277,14 +259,8 @@ def service_set_sms_sender(service_id): message = 'Text message sender removed' service_api_client.update_service( current_service['id'], - current_service['name'], - current_service['active'], - current_service['message_limit'], - current_service['restricted'], - current_service['users'], - current_service['email_from'], - current_service['reply_to_email_address'], - sms_sender=form.sms_sender.data if form.sms_sender.data else None) + sms_sender=form.sms_sender.data or None + ) flash(message, 'default_with_tick') return redirect(url_for('.service_settings', service_id=service_id)) return render_template( diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index d1134406f..b6a689659 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -73,38 +73,20 @@ class ServiceAPIClient(NotificationsAPIClient): """ return self.get('/service', *params) - def update_service(self, - service_id, - service_name, - active, - message_limit, - restricted, - users, - email_from, - reply_to_email_address=None, - sms_sender=None): + def update_service( + self, + service_id, + **kwargs + ): """ Update a service. """ - data = { - "id": service_id, - "name": service_name, - "active": active, - "message_limit": message_limit, - "restricted": restricted, - "users": users, - "email_from": email_from, - "reply_to_email_address": reply_to_email_address, - "sms_sender": sms_sender - } - _attach_current_user(data) + _attach_current_user(kwargs) endpoint = "/service/{0}".format(service_id) return self.post(endpoint, data) def update_service_with_properties(self, service_id, properties): - _attach_current_user(properties) - endpoint = "/service/{0}".format(service_id) - return self.post(endpoint, properties) + return self.update_service(service_id, **properties) def remove_user_from_service(self, service_id, user_id): """ diff --git a/tests/__init__.py b/tests/__init__.py index 7841a02e7..855abfe6a 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -40,15 +40,17 @@ def created_by_json(id_, name='', email_address=''): def service_json( - id_, - name, - users, - message_limit=1000, - active=False, - restricted=True, - email_from=None, - reply_to_email_address=None, - research_mode=False): + id_, + name, + users, + message_limit=1000, + active=False, + restricted=True, + email_from=None, + reply_to_email_address=None, + sms_sender=None, + research_mode=False +): return { 'id': id_, 'name': name, @@ -58,6 +60,7 @@ def service_json( 'restricted': restricted, 'email_from': email_from, 'reply_to_email_address': reply_to_email_address, + 'sms_sender': sms_sender, 'research_mode': research_mode } diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 1793211dd..bf769297b 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -80,7 +80,11 @@ def test_switch_service_to_live(app_, assert response.location == url_for( 'main.service_settings', service_id=service_one['id'], _external=True) - mock_update_service.assert_called_with(ANY, ANY, ANY, 250000, False, ANY, ANY) + mock_update_service.assert_called_with( + service_one['id'], + message_limit=250000, + restricted=False + ) def test_switch_service_to_restricted(app_, @@ -100,7 +104,11 @@ def test_switch_service_to_restricted(app_, assert response.location == url_for( 'main.service_settings', service_id=service_one['id'], _external=True) - mock_update_service.assert_called_with(ANY, ANY, ANY, 50, True, ANY, ANY) + mock_update_service.assert_called_with( + service_one['id'], + message_limit=50, + restricted=True + ) def test_should_not_allow_duplicate_names(app_, @@ -159,13 +167,11 @@ def test_should_redirect_after_service_name_confirmation(app_, assert response.status_code == 302 settings_url = url_for('main.service_settings', service_id=service_id, _external=True) assert settings_url == response.location - mock_update_service.assert_called_once_with(service_id, - service_new_name, - service_one['active'], - service_one['message_limit'], - service_one['restricted'], - service_one['users'], - email_safe(service_new_name)) + mock_update_service.assert_called_once_with( + service_id, + name=service_new_name, + email_from=email_safe(service_new_name) + ) assert mock_verify_password.called @@ -564,14 +570,10 @@ def test_set_reply_to_email_address( data=data, follow_redirects=True) assert response.status_code == 200 - mock_update_service.assert_called_with(service_one['id'], - service_one['name'], - service_one['active'], - service_one['message_limit'], - service_one['restricted'], - ANY, - service_one['email_from'], - "test@someservice.gov.uk") + mock_update_service.assert_called_with( + service_one['id'], + reply_to_email_address="test@someservice.gov.uk" + ) def test_if_reply_to_email_address_set_then_form_populated(app_, @@ -710,15 +712,10 @@ def test_set_text_message_sender( follow_redirects=True) assert response.status_code == 200 - mock_update_service.assert_called_with(service_one['id'], - service_one['name'], - service_one['active'], - service_one['message_limit'], - service_one['restricted'], - service_one['users'], - service_one['email_from'], - service_one['reply_to_email_address'], - "elevenchars") + mock_update_service.assert_called_with( + service_one['id'], + sms_sender="elevenchars" + ) def test_if_sms_sender_set_then_form_populated(app_, diff --git a/tests/conftest.py b/tests/conftest.py index 0bcef48af..5aee6f55e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -125,18 +125,20 @@ def mock_create_service(mocker): @pytest.fixture(scope='function') def mock_update_service(mocker): - def _update(service_id, - service_name, - active, - message_limit, - restricted, - users, - email_from, - reply_to_email_address=None, - sms_sender=None): + def _update(service_id, **kwargs): service = service_json( - service_id, service_name, users, message_limit=message_limit, - active=active, restricted=restricted, email_from=email_from, reply_to_email_address=reply_to_email_address) + service_id, + **{key: kwargs.get(key) for key in [ + 'name', + 'users', + 'message_limit', + 'active', + 'restricted', + 'email_from', + 'reply_to_email_address', + 'sms_sender' + ]} + ) return {'data': service} return mocker.patch( @@ -145,14 +147,11 @@ def mock_update_service(mocker): @pytest.fixture(scope='function') def mock_update_service_raise_httperror_duplicate_name(mocker): - def _update(service_id, - service_name, - active, - limit, - restricted, - users, - email_from): - json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(service_name)]}}) + def _update( + service_id, + **kwargs + ): + json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(kwargs.get('name'))]}}) resp_mock = Mock(status_code=400, json=json_mock) http_error = HTTPError(response=resp_mock, message="Default message") raise http_error