Make _attach_current_user a pure function

Mutating dictionaries is gross and doesn’t work as you’d expect. Better
to have the function return a new dictionary instead.

Means we can be explicit that `created_by` is one of the allowed params
when updating a service.

app/notify_client/__init__.py

@@ -2,5 +2,7 @@ from flask.ext.login import current_user
 
 
 def _attach_current_user(data):
-    data['created_by'] = current_user.id
+    return dict(
-    return data
+        created_by=current_user.id,
+        **data
+    )

app/notify_client/api_key_api_client.py

@@ -30,7 +30,7 @@ class ApiKeyApiClient(BaseAPIClient):
             'name': key_name,
             'key_type': key_type
         }
-        _attach_current_user(data)
+        data = _attach_current_user(data)
         key = self.post(url='/service/{}/api-key'.format(service_id), data=data)
         return key['data']
 

app/notify_client/invite_api_client.py

@@ -23,7 +23,7 @@ class InviteApiClient(BaseAPIClient):
             'from_user': invite_from_id,
             'permissions': permissions
         }
-        _attach_current_user(data)
+        data = _attach_current_user(data)
         resp = self.post(url='/service/{}/invite'.format(service_id), data=data)
         return InvitedUser(**resp['data'])
 
@@ -40,7 +40,7 @@ class InviteApiClient(BaseAPIClient):
 
     def cancel_invited_user(self, service_id, invited_user_id):
         data = {'status': 'cancelled'}
-        _attach_current_user(data)
+        data = _attach_current_user(data)
         self.post(url='/service/{0}/invite/{1}'.format(service_id, invited_user_id),
                   data=data)
 

app/notify_client/job_api_client.py

@@ -32,6 +32,6 @@ class JobApiClient(BaseAPIClient):
             "original_file_name": original_file_name,
             "notification_count": notification_count
         }
-        _attach_current_user(data)
+        data = _attach_current_user(data)
         resp = self.post(url='/service/{}/job'.format(service_id), data=data)
         return resp['data']

app/notify_client/provider_client.py

@@ -29,5 +29,5 @@ class ProviderClient(BaseAPIClient):
         data = {
             "priority": priority
         }
-        _attach_current_user(data)
+        data = _attach_current_user(data)
         return self.post(url='/provider-details/{}'.format(provider_id), data=data)

app/notify_client/service_api_client.py

@@ -30,7 +30,7 @@ class ServiceAPIClient(NotificationsAPIClient):
             "restricted": restricted,
             "email_from": email_from
         }
-        _attach_current_user(data)
+        data = _attach_current_user(data)
         return self.post("/service", data)['data']['id']
 
     def delete_service(self, service_id):
@@ -81,7 +81,8 @@ class ServiceAPIClient(NotificationsAPIClient):
         """
         Update a service.
         """
-        disallowed_attributes = set(kwargs.keys()) - {
+        data = _attach_current_user(kwargs)
+        disallowed_attributes = set(data.keys()) - {
             'name',
             'users',
             'message_limit',
@@ -89,14 +90,14 @@ class ServiceAPIClient(NotificationsAPIClient):
             'restricted',
             'email_from',
             'reply_to_email_address',
-            'sms_sender'
+            'sms_sender',
+            'created_by'
         }
         if disallowed_attributes:
             raise TypeError('Not allowed to update service attributes: {}'.format(
                 ", ".join(disallowed_attributes)
             ))
 
-        _attach_current_user(kwargs)
         endpoint = "/service/{0}".format(service_id)
         return self.post(endpoint, data)
 
@@ -127,7 +128,7 @@ class ServiceAPIClient(NotificationsAPIClient):
             data.update({
                 'subject': subject
             })
-        _attach_current_user(data)
+        data = _attach_current_user(data)
         endpoint = "/service/{0}/template".format(service_id)
         return self.post(endpoint, data)
 
@@ -146,7 +147,7 @@ class ServiceAPIClient(NotificationsAPIClient):
             data.update({
                 'subject': subject
             })
-        _attach_current_user(data)
+        data = _attach_current_user(data)
         endpoint = "/service/{0}/template/{1}".format(service_id, id_)
         return self.post(endpoint, data)
 
@@ -187,7 +188,7 @@ class ServiceAPIClient(NotificationsAPIClient):
         data = {
             'archived': True
         }
-        _attach_current_user(data)
+        data = _attach_current_user(data)
         return self.post(endpoint, data=data)
 
     def find_all_service_email_from(self, user_id=None):

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)
@@ -40,6 +39,7 @@ def test_client_gets_service(mocker, function, 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'