Merge pull request #863 from alphagov/allow-partial-service-updates

Allow partial service updates from service API client

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
     }
 

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_,

tests/app/notify_client/test_job_client.py

@@ -7,20 +7,19 @@ def test_client_creates_job_data_correctly(mocker, fake_uuid):
     template_id = fake_uuid
     original_file_name = 'test.csv'
     notification_count = 1
+    mocker.patch('app.notify_client.current_user', id='1')
 
     expected_data = {
         "id": job_id,
         "template": template_id,
         "original_file_name": original_file_name,
-        "notification_count": 1
+        "notification_count": 1,
+        "created_by": '1'
     }
 
     expected_url = '/service/{}/job'.format(service_id)
 
     client = JobApiClient()
-    mock_attach_user = mocker.patch(
-        'app.notify_client.job_api_client._attach_current_user',
-        return_value={'created_by': fake_uuid})
     mock_post = mocker.patch('app.notify_client.job_api_client.JobApiClient.post')
 
     client.create_job(job_id, service_id, template_id, original_file_name, notification_count)

tests/app/notify_client/test_service_api_client.py

@@ -7,17 +7,16 @@ from tests.conftest import fake_uuid
 def test_client_posts_archived_true_when_deleting_template(mocker):
     service_id = fake_uuid
     template_id = fake_uuid
+    mocker.patch('app.notify_client.current_user', id='1')
 
     expected_data = {
         'archived': True,
-        'created_by': fake_uuid
+        'created_by': '1'
     }
     expected_url = '/service/{}/template/{}'.format(service_id, template_id)
 
     client = ServiceAPIClient()
     mock_post = mocker.patch('app.notify_client.service_api_client.ServiceAPIClient.post')
-    mock_attach_user = mocker.patch('app.notify_client.service_api_client._attach_current_user',
-                                    side_effect=lambda x: x.update({'created_by': fake_uuid}))
 
     client.delete_service_template(service_id, template_id)
     mock_post.assert_called_once_with(expected_url, data=expected_data)
@@ -37,3 +36,10 @@ def test_client_gets_service(mocker, function, params):
 
     function(client, 'foo')
     mock_get.assert_called_once_with('/service/foo', params=params)
+
+
+def test_client_only_updates_allowed_attributes(mocker):
+    mocker.patch('app.notify_client.current_user', id='1')
+    with pytest.raises(TypeError) as error:
+        ServiceAPIClient().update_service('service_id', foo='bar')
+    assert str(error.value) == 'Not allowed to update service attributes: foo'

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