Make service API client do partial updates

The service API client was updating every attribute of a service. Which,
while kinda clunky, is fine…

…until something calling it doesn’t pass in every attribute of the
current service. It was then defaulting optional parameters to `None`.
Which resulted in a bug whereby every time a service was set to live,
its `reply_to_address` and `sms_sender_name` got overwritten to be
empty.

This commit changes the `update` method to only require the service ID,
and pass whatever other named arguments it received straight through to
the API. The API handles partial updates just fine (I think).

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'],
+                name=session['service_name_change'],
-                current_service['active'],
+                email_from=email_safe(session['service_name_change'])
-                current_service['message_limit'],
+            )
-                current_service['restricted'],
-                current_service['users'],
-                current_service['email_from'])
         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,
+        message_limit=250000 if current_service['restricted'] else 50,
-        False if current_service['restricted'] else True,
+        restricted=(not current_service['restricted'])
-        current_service['users'],
+    )
-        current_service['email_from'])
     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'],
+            active=True
-            current_service['active'],
+        )
-            current_service['message_limit'],
-            current_service['restricted'],
-            current_service['users'],
-            current_service['email_from'])
         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'],
+            reply_to_email_address=form.email_address.data
-            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)
         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'],
+            sms_sender=form.sms_sender.data or None
-            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)
         flash(message, 'default_with_tick')
         return redirect(url_for('.service_settings', service_id=service_id))
     return render_template(

app/notify_client/service_api_client.py

@@ -73,38 +73,20 @@ class ServiceAPIClient(NotificationsAPIClient):
         """
         return self.get('/service', *params)
 
-    def update_service(self,
+    def update_service(
-                       service_id,
+        self,
-                       service_name,
+        service_id,
-                       active,
+        **kwargs
-                       message_limit,
+    ):
-                       restricted,
-                       users,
-                       email_from,
-                       reply_to_email_address=None,
-                       sms_sender=None):
         """
         Update a service.
         """
-        data = {
+        _attach_current_user(kwargs)
-            "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)
         endpoint = "/service/{0}".format(service_id)
         return self.post(endpoint, data)
 
     def update_service_with_properties(self, service_id, properties):
-        _attach_current_user(properties)
+        return self.update_service(service_id, **properties)
-        endpoint = "/service/{0}".format(service_id)
-        return self.post(endpoint, properties)
 
     def remove_user_from_service(self, service_id, user_id):
         """

tests/__init__.py

@@ -40,15 +40,17 @@ def created_by_json(id_, name='', email_address=''):
 
 
 def service_json(
-        id_,
+    id_,
-        name,
+    name,
-        users,
+    users,
-        message_limit=1000,
+    message_limit=1000,
-        active=False,
+    active=False,
-        restricted=True,
+    restricted=True,
-        email_from=None,
+    email_from=None,
-        reply_to_email_address=None,
+    reply_to_email_address=None,
-        research_mode=False):
+    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
     }
 

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,
+        mock_update_service.assert_called_once_with(
-                                                    service_new_name,
+            service_id,
-                                                    service_one['active'],
+            name=service_new_name,
-                                                    service_one['message_limit'],
+            email_from=email_safe(service_new_name)
-                                                    service_one['restricted'],
+        )
-                                                    service_one['users'],
-                                                    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'],
+        mock_update_service.assert_called_with(
-                                               service_one['name'],
+            service_one['id'],
-                                               service_one['active'],
+            reply_to_email_address="test@someservice.gov.uk"
-                                               service_one['message_limit'],
+        )
-                                               service_one['restricted'],
-                                               ANY,
-                                               service_one['email_from'],
-                                               "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'],
+        mock_update_service.assert_called_with(
-                                               service_one['name'],
+            service_one['id'],
-                                               service_one['active'],
+            sms_sender="elevenchars"
-                                               service_one['message_limit'],
+        )
-                                               service_one['restricted'],
-                                               service_one['users'],
-                                               service_one['email_from'],
-                                               service_one['reply_to_email_address'],
-                                               "elevenchars")
 
 
 def test_if_sms_sender_set_then_form_populated(app_,

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,
+    def _update(service_id, **kwargs):
-                service_name,
-                active,
-                message_limit,
-                restricted,
-                users,
-                email_from,
-                reply_to_email_address=None,
-                sms_sender=None):
         service = service_json(
-            service_id, service_name, users, message_limit=message_limit,
+            service_id,
-            active=active, restricted=restricted, email_from=email_from, reply_to_email_address=reply_to_email_address)
+            **{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,
+    def _update(
-                service_name,
+        service_id,
-                active,
+        **kwargs
-                limit,
+    ):
-                restricted,
+        json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(kwargs.get('name'))]}})
-                users,
-                email_from):
-        json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(service_name)]}})
         resp_mock = Mock(status_code=400, json=json_mock)
         http_error = HTTPError(response=resp_mock, message="Default message")
         raise http_error